Skip to content

Conversation

@josephmyers
Copy link
Collaborator

@josephmyers josephmyers commented Nov 6, 2025

Note that the task description says "they should also be able to view answers that community checkers submit." This could indicate that we're wanting to only show answers from Community Checker users (and not all answers), but as we've never done that before, and this is labeled a bug, I'm going to opt for not adding that feature until someone tells me I should.

This change is Reviewable

@josephmyers josephmyers added the will require testing PR should not be merged until testers confirm testing is complete label Nov 6, 2025
@josephmyers josephmyers marked this pull request as ready for review November 6, 2025 22:56
@josephmyers
Copy link
Collaborator Author

I'll update the tests after we get confirmation on the desired behavior

@codecov
Copy link

codecov bot commented Nov 6, 2025

Codecov Report

❌ Patch coverage is 73.68421% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.80%. Comparing base (b9a5295) to head (4546465).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
...ing/checking-answers/checking-answers.component.ts 66.66% 2 Missing and 1 partial ⚠️
...checking-questions/checking-questions.component.ts 80.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3560      +/-   ##
==========================================
- Coverage   82.80%   82.80%   -0.01%     
==========================================
  Files         608      608              
  Lines       37187    37201      +14     
  Branches     6098     6104       +6     
==========================================
+ Hits        30794    30803       +9     
- Misses       5468     5485      +17     
+ Partials      925      913      -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@marksvc marksvc self-assigned this Nov 7, 2025
@marksvc
Copy link
Collaborator

marksvc commented Nov 7, 2025

Note that the task description says "they should also be able to view answers that community checkers submit." This could indicate that we're wanting to only show answers from Community Checker users (and not all answers

I'm confused about what you are meaning by this :)

@marksvc
Copy link
Collaborator

marksvc commented Nov 7, 2025

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-answers.component.ts line 325 at r1 (raw file):

  get shouldShowAnswers(): boolean {
    return (
      !this.answerFormVisible &&

tttt

@marksvc
Copy link
Collaborator

marksvc commented Nov 7, 2025

Copy link
Collaborator

@marksvc marksvc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marksvc reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @josephmyers)

@pmachapman pmachapman added ready to test will require testing PR should not be merged until testers confirm testing is complete and removed will require testing PR should not be merged until testers confirm testing is complete ready to test labels Nov 9, 2025
@josephmyers
Copy link
Collaborator Author

Note that the task description says "they should also be able to view answers that community checkers submit." This could indicate that we're wanting to only show answers from Community Checker users (and not all answers

I'm confused about what you are meaning by this :)

It's some ambiguity in the description. I'm taking it to mean that this flag should allow them to see all answers, just like Admin's can. However, you could interpret this to mean that they should only be able to see answers from the role Community Checker (the SF user role). We've never done this before, so if this is a bug, then I'd expect there to be remnants of it working like this (and just needing some fixing to restore the behavior). Since there's nothing like that, I think it's unlikely that's the intent. That's all!

@josephmyers josephmyers force-pushed the fix/SF-3619 branch 2 times, most recently from 196cc21 to 8004b59 Compare November 13, 2025 20:37
Copy link
Collaborator

@Nateowami Nateowami left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @marksvc)


src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-questions/checking-questions.component.ts line 225 at r2 (raw file):

  get canManageQuestions(): boolean {
    return this.isProjectAdmin || this.canAddAndEditQuestions;

This is also wrong.


src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-answers.component.ts line 295 at r2 (raw file):

  }

  get canAddAndEditQuestions(): boolean {

Given the methods appear to be identical, shouldn't these be service methods, not component getters? I would think the most logical place to put them would be in the SFProjectService.


src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-answers.component.ts line 316 at r2 (raw file):

  get canManageQuestions(): boolean {
    return this.isProjectAdmin || this.canAddAndEditQuestions;

This is wrong. There should not be a check for the user's role, only a permissions check. In general, any time you see isProjectAdmin, it's not using permissions correctly. You can probably delete isProjectAdmin from this component (and from checking questions).

Copy link
Collaborator

@Nateowami Nateowami left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @josephmyers and @marksvc)


src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking-answers/checking-answers.component.ts line 295 at r2 (raw file):

Previously, Nateowami wrote…

Given the methods appear to be identical, shouldn't these be service methods, not component getters? I would think the most logical place to put them would be in the SFProjectService.

permissions.service.ts‎ might be another good option (possibly better)

@RaymondLuong3 RaymondLuong3 added testing complete Testing of PR is complete and should no longer hold up merging of the PR and removed will require testing PR should not be merged until testers confirm testing is complete labels Nov 17, 2025
@RaymondLuong3 RaymondLuong3 merged commit 142f019 into master Nov 19, 2025
22 of 23 checks passed
@RaymondLuong3 RaymondLuong3 deleted the fix/SF-3619 branch November 19, 2025 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testing complete Testing of PR is complete and should no longer hold up merging of the PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants