Skip to content

SF-3715 Send checkers to first book even when books are out of order#3688

Open
Nateowami wants to merge 1 commit intomasterfrom
fix/SF-3715-send-checkers-correct-book
Open

SF-3715 Send checkers to first book even when books are out of order#3688
Nateowami wants to merge 1 commit intomasterfrom
fix/SF-3715-send-checkers-correct-book

Conversation

@Nateowami
Copy link
Collaborator

@Nateowami Nateowami commented Feb 14, 2026

This change is Reviewable

@Nateowami Nateowami added ready to test will require testing PR should not be merged until testers confirm testing is complete and removed ready to test labels Feb 14, 2026
@codecov
Copy link

codecov bot commented Feb 14, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.82%. Comparing base (2640199) to head (ba5bda4).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3688   +/-   ##
=======================================
  Coverage   81.82%   81.82%           
=======================================
  Files         619      619           
  Lines       38619    38621    +2     
  Branches     6317     6293   -24     
=======================================
+ Hits        31601    31603    +2     
- Misses       6044     6057   +13     
+ Partials      974      961   -13     

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

@pmachapman pmachapman self-assigned this Feb 15, 2026
@pmachapman pmachapman self-requested a review February 15, 2026 18:22
Copy link
Collaborator

@pmachapman pmachapman left a comment

Choose a reason for hiding this comment

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

@pmachapman reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Nateowami).


src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/resume-checking.service.ts line 118 at r1 (raw file):

      // No questions, or all questions already answered.  Send user to first book/chapter.
      const firstTextWithChapters = projectDoc.data?.texts
        .sort((a, b) => a.bookNum - b.bookNum)

Can you please add a .slice() before the .sort()?

Applying the sort() directly to the texts array can under certain circumstances cause the project doc in IndexedDB to have the texts array stored in a different order to MongoDB (cf. #2492).

Thanks!

Code quote:

      const firstTextWithChapters = projectDoc.data?.texts
        .sort((a, b) => a.bookNum - b.bookNum)

@Nateowami Nateowami force-pushed the fix/SF-3715-send-checkers-correct-book branch from b52272e to ba5bda4 Compare February 16, 2026 14:20
Copy link
Collaborator Author

@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.

@Nateowami made 1 comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @pmachapman).


src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/resume-checking.service.ts line 118 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Can you please add a .slice() before the .sort()?

Applying the sort() directly to the texts array can under certain circumstances cause the project doc in IndexedDB to have the texts array stored in a different order to MongoDB (cf. #2492).

Thanks!

Thank you for catching this. I don't think I would have caught it, even though I know sort is in place, if I stopped to think about it.

What I'm not sure is how to avoid this mistake in the future, since depending on context, cloning the array isn't always necessary.

Copy link
Collaborator

@pmachapman pmachapman left a comment

Choose a reason for hiding this comment

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

:lgtm:

@pmachapman reviewed 1 file and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Nateowami).

@pmachapman pmachapman added ready to test and removed will require testing PR should not be merged until testers confirm testing is complete labels Feb 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants