SF-3322 Allow selecting book and chapter in community checking

[RaymondLuong3]

This PR allows users on the community checking app to select the book and chapter they want to navigate to without requiring the user to be on the "all books" scope. If the user does select a chapter or book with the select, the scope is automatically reset to all books and they will need to set the filter again to their preference.

---

This change is 

[codecov]

Codecov Report

Attention: Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.

Project coverage is 81.99%. Comparing base (811f14d) to head (52d33d8).

Files with missing lines	Patch %	Lines
...pp/src/app/checking/checking/checking.component.ts	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3223      +/-   ##
==========================================
+ Coverage   81.98%   81.99%   +0.01%     
==========================================
  Files         605      605              
  Lines       34694    34694              
  Branches     5622     5601      -21     
==========================================
+ Hits        28444    28448       +4     
- Misses       5411     5419       +8     
+ Partials      839      827      -12     

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

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[josephmyers]

Thanks for your work on this. I have to apologize for not thinking through all the steps when I made this task. I do think it it'd be an improvement to, instead, preserve the active range if possible. For example, if the range is set to "current book" and the user changes their chapter but stays in that book, the range is reset. How difficult would it be to pivot a little and preserve the range if possible? I'll keep thinking of scenarios, but I'm coming up short on times when we actually need to reset their range.

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RaymondLuong3)

---

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

      .subscribe((state: BreakpointState) => {
        // setting isScreenSmall causes `ExpressionChangedAfterItHasBeenCheckedError`, so wrap in setTimeout
        setTimeout(() => (this.isScreenSmall = state.matches));

It's really weird that this just cropped up. Can you detail this any more? Is it due to your template change (prevNextHidden)?

[RaymondLuong3]

That is good thinking. As far as I can tell, the only situation where we want to scope to not be updated to 'all' is when the user selects a chapter when the scope is set to book. In every other scenario, the scope will get reset to 'all'. I've updated the code to reflect his behaviour.

Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @josephmyers)

---

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

Previously, josephmyers wrote…

It's really weird that this just cropped up. Can you detail this any more? Is it due to your template change (prevNextHidden)?

Yes, so in the html the previous state of prevNextHidden depended on the value of isScreenSmall and activeQuestionScope. Now that activeQuestionScope is not relevant for this, then the value assignment is more likely to change after change detection is run. You can see just 10 lines above that this was already causing us issues.

[josephmyers]

I don't follow your line of thinking. In every other case, we should preserve the range if possible, unless you're thinking of something I'm not. Can you explain your reasoning? I'll describe how the app is working as you've changed it, and hopefully it will become apparent why that's not ideal.

Preface: we need to consider the use cases when the range is "current book" and when it's "current chapter." For example, what happens when it's set to X and they change chapters? Change books? Select question, navigate away, and come back?

Scenario 1: Range - current book
If you change the chapter, the range remains. Great. If you change the book, it resets. This is bad, because I as a user want to see the questions for that book. Now I've lost my place in the massive list of questions, and I can't see the questions for the book I just selected (without scrolling a lot). So again, we should preserve the filter/range when possible.

Part of the problem, though not all of it, is that when we change the chapter or book, we don't update their active question. This results in a quasi-viewer mode, where the user can navigate around to a totally different chapter than their question just below. This is cool, in one way, but confusing for the user and problematic for the app. So when they change, we need to update the active question (to the first question in the visible questions, just like normal).

Scenario 2: Range - current chapter
Changing chapters resets the range to all books. This will be really frustrating for navigation. Why would we/they want this? What am I missing? We need to preserve the filter/range when possible (and update the active question).

Scenario 3: Range - current chapter
Changing books resets the range, as well. This would be frustrating, again, because you can't visibly see the questions for that book without scrolling through tons of questions. If they change the book in this case, we want to preserve the range/filter and show the questions for the new book at chapter 1 (and update the active question).

Reviewable status: 3 of 4 files reviewed, 1 unresolved discussion (waiting on @RaymondLuong3)

[RaymondLuong3]

Thanks for the explanation. My understanding was that if the list of questions would not be impacted by updating the book/chapter, then we should not update the scope. But if the user updated the book/chapter and the list of questions would updated, then we would reset the scope. Something like "The book and chapter you selected is outside of the current set of questions, so I'll give you all the questions."

But if I put myself in the role of a community checker, that probably doesn't make a lot of sense as you have described. I've updated the PR to follow the rule that the scope is never reset. If the user navigates to a book/chapter and the current set of questions does not contain the selected questions, then I have updated the logic to select the first question in the list. But if the currently selected question is within the list, then just keep that question selected. Let me know what you think.

Reviewable status: 1 of 4 files reviewed, 1 unresolved discussion (waiting on @josephmyers)

[josephmyers]

Man, this is starting to look really solid. Good job. This change really makes navigation with a scope so useful. For example, with the questions scoped to a book, changing chapters selects a question in that chapter and scrolls to it. Awesome!

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @RaymondLuong3)

---

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

    } else if (changes.routeBookChapter != null) {
      // If the route book/chapter changes, activate the first question on the new chapter
      this.activateStoredQuestion();

Need test coverage

---

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

      this.routeBookChapter == null ||
      (this.activeQuestionDoc?.data != null &&
        bookChapterMatchesVerseRef(this.routeBookChapter, this.activeQuestionDoc.data.verseRef))

This else if condition is really hard to read. Can you clean it up?

---

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

    );
    // If no question was previously active, choose the first question
    questionToActivate ??= this.activeQuestionDoc == null ? this.questionDocs[0] : undefined;

Great job making this easier to read!

---

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

    if (questionToActivate != null) {
      this.activateQuestion(questionToActivate, actionSource);

One small bug: when the scope is "all books" or "current book," navigating to a chapter with no questions keeps the selected question. This bug is not present for "current chapter," likely because for "current chapter" the visible questions is empty. Do we have a way to deselect the active question when the questions list is not empty? It may be enough to set activeQuestionDoc to undefined, as earlier in this file.

---

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

                  qd.updateFileCache();
                  if (isActiveQuestionDoc) {
                    qd.updateAnswerFileCache();

Can you discuss why this isn't needed anymore? Based on the tests you updated, it seems like this was previously being called more than it needed to be. So this is cleanup?

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking.component.ts line 606 at r3 (raw file):

            routeScope !== prevScope ||
            (routeBookNum !== prevBookNum && prevScope !== 'all') ||
            ((routeChapter == null ? undefined : parseInt(routeChapter)) !== prevChapterNum && prevScope === 'chapter')

You've tinkered with the conditions here. Why was this needed? Be sure to update the comment

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking.component.ts line 694 at r3 (raw file):

          } else {
            // Visible questions didn't change, but active question must update on route change
            // this.questionsList?.activateStoredQuestion();

Cool. I agree, it makes more sense to update within the component itself, not here. Just be sure to remove this.

---

src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking.component.ts line 1098 at r3 (raw file):

      this.activeQuestionFilter = QuestionFilter.None;
    }
    this.questionsList?.activateQuestion(questionDoc);

Moving this outside the !navigate block makes sense to me. I'm just wondering how this change is related to the current task.

Tiny rant: This is why I always leave detailed commit messages. If changing this doesn't break any tests, and there's no commit trail, we have essentially no idea why this was done the way it was and if it's okay to change it.