List consecutive translate books rather than ranges by RaymondLuong3 · Pull Request #3700 · sillsdev/web-xforge

RaymondLuong3 · 2026-02-19T18:43:15Z

Testers found that on the summary page the books were listed as ranges in the training step but the translate step books were listed individually. This may be on purpose to make it explicit which books were being drafted. However, the draft history view shows the books as ranges also. This PR makes it more consistent to show books are ranges when they are consecutive.

I also refactored the code to use the i18n.service to localize and format books as ranges. I labeled this as testing not required since it only affect displaying the elements on the page which can be confirmed easily.

Before

After

This change is

codecov · 2026-02-19T18:55:01Z

Codecov Report

❌ Patch coverage is 75.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.73%. Comparing base (595481b) to head (bfd0af3).
⚠️ Report is 2 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...ate/draft-generation/draft-generation.component.ts	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3700      +/-   ##
==========================================
- Coverage   81.74%   81.73%   -0.01%     
==========================================
  Files         619      619              
  Lines       38663    38651      -12     
  Branches     6297     6312      +15     
==========================================
- Hits        31606    31593      -13     
+ Misses       6096     6084      -12     
- Partials      961      974      +13

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

Nateowami · 2026-02-19T19:18:07Z

This may be on purpose to make it explicit which books were being drafted.

Yes, this was intentional, as it should be a most a handful of books, not a large number like with training data.

the draft history view shows the books as ranges also

I think that should be changed.

RaymondLuong3

Thanks for the feedback. I agree, that seems to be the right approach

I think that should be changed.
I see why it was implemented that way. It gets the data from the ScriptureRange. There is a ScriptureRangeParser from SIL.Scripture namespace but that has not been ported to typescript. I am going to leave this as-is since it has such a small impact overall and not worth the work.

@RaymondLuong3 made 1 comment.
Reviewable status: 0 of 5 files reviewed, all discussions resolved.

RaymondLuong3

Whoops, formatting got messed up

@RaymondLuong3 made 1 comment.
Reviewable status: 0 of 5 files reviewed, all discussions resolved.

RaymondLuong3

Ok, it looks like in other parts of SF we assume that the scripture range will just be the book IDs separated by semi-colons. So it is possible to show the translation books individually based on a scripture range. It would be better to port the ScriptureRangeParser into typescript but not sure if we will prioritize that.

@RaymondLuong3 made 1 comment.
Reviewable status: 0 of 9 files reviewed, all discussions resolved.

devin-ai-integration

Devin Review found 2 potential issues.

View 4 additional findings in Devin Review.

devin-ai-integration · 2026-02-20T17:45:59Z

+  getScriptureRangeAsLocalizedBooks(scriptureRange: string): string {
+    return this.i18n.enumerateList(booksFromScriptureRange(scriptureRange).map(b => this.i18n.localizeBook(b)));
+  }


🟡 getScriptureRangeAsLocalizedBooks does not sort books, causing non-canonical display order

The new getScriptureRangeAsLocalizedBooks method in draft-history-entry.component.ts:355-357 does not sort book numbers before localizing and enumerating them. The old code used i18n.formatAndLocalizeScriptureRange, which explicitly sorted books by canonical order (i18n.service.ts:244: .sort((a, b) => a - b)). The new code calls booksFromScriptureRange which returns books in whatever order they appear in the input string — no sorting is performed.

Root Cause

The _scriptureRange is built at draft-history-entry.component.ts:158 by joining multiple translationScriptureRanges items' scriptureRange values with ;. If the API returns ranges in non-canonical order (e.g., "EXO;GEN"), booksFromScriptureRange at utils.ts:172-178 simply splits on ; and maps to book numbers without sorting.

For example, with a scripture range of "EXO;GEN", the old code would display "Genesis - Exodus" (sorted and grouped), while the new code displays "Exodus and Genesis" (unsorted).

Impact: Draft history entries may display translation books in an arbitrary, non-canonical order, which is a regression from previous behavior and confusing for users.

Suggested change

getScriptureRangeAsLocalizedBooks(scriptureRange: string): string {

return this.i18n.enumerateList(booksFromScriptureRange(scriptureRange).map(b => this.i18n.localizeBook(b)));

}

getScriptureRangeAsLocalizedBooks(scriptureRange: string): string {

return this.i18n.enumerateList(

booksFromScriptureRange(scriptureRange)

.sort((a, b) => a - b)

.map(b => this.i18n.localizeBook(b))

);

}

Was this helpful? React with 👍 or 👎 to provide feedback.

Hmm. I'm not sure we need to be concerned about this.

📌 Just saying.

(It looks like my comment was not shown in context, which is a concern Devin raised about sorting. I was expressing that I'm not sure we need to be concerned about what Devin had raised there.)

devin-ai-integration · 2026-02-20T17:46:00Z

+    return this.i18n.enumerateList(
+      booksFromScriptureRange(
+        job.additionalInfo.translationScriptureRanges.map(item => item.scriptureRange).join(';')
+      ).map(b => this.i18n.localizeBook(b))
    );


🟡 getTranslationScriptureRange does not sort books, causing non-canonical display order

The new getTranslationScriptureRange method in draft-generation.component.ts:408-415 does not sort book numbers before localizing and enumerating them. The old code called i18n.formatAndLocalizeScriptureRange, which sorted books canonically before display. The new code passes booksFromScriptureRange(...) output directly to localizeBook and enumerateList without sorting.

Root Cause

The booksFromScriptureRange utility at utils.ts:172-178 returns book numbers in the order they appear in the semicolon-delimited input string. The old formatAndLocalizeScriptureRange at i18n.service.ts:242-244 applied .sort((a, b) => a - b) before processing, but the replacement code omits this step.

For a translationScriptureRanges value like [{scriptureRange: 'LEV'}, {scriptureRange: 'GEN'}], the joined string "LEV;GEN" would display as "Leviticus and Genesis" instead of "Genesis and Leviticus".

Impact: The in-progress draft card title may display translation books in an arbitrary, non-canonical order, which is a regression from previous behavior.

Suggested change

return this.i18n.enumerateList(

booksFromScriptureRange(

job.additionalInfo.translationScriptureRanges.map(item => item.scriptureRange).join(';')

).map(b => this.i18n.localizeBook(b))

);

return this.i18n.enumerateList(

booksFromScriptureRange(

job.additionalInfo.translationScriptureRanges.map(item => item.scriptureRange).join(';')

)

.sort((a, b) => a - b)

.map(b => this.i18n.localizeBook(b))

Was this helpful? React with 👍 or 👎 to provide feedback.

marksvc · 2026-02-20T19:14:51Z

I was confused from the discussion. But I think it makes sense if

This PR makes it more consistent to show books are ranges when they are consecutive.

was actually a mistake and what was meant was

This PR makes it more consistent to list books individually, even when they are consecutive.

marksvc · 2026-02-20T18:05:50Z

  }
+
+  getScriptureRangeAsLocalizedBooks(scriptureRange: string): string {
+    return this.i18n.enumerateList(booksFromScriptureRange(scriptureRange).map(b => this.i18n.localizeBook(b)));


Okay. Though I notice that this converts from book ID strings, to book numbers, and back to book ID strings (before getting a localized string) in its process.

📌 Just saying.

marksvc · 2026-02-20T18:35:07Z

+  getScriptureRangeAsLocalizedBooks(scriptureRange: string): string {
+    return this.i18n.enumerateList(booksFromScriptureRange(scriptureRange).map(b => this.i18n.localizeBook(b)));
+  }


Hmm. I'm not sure we need to be concerned about this.

📌 Just saying.

marksvc · 2026-02-20T18:55:13Z

+  getScriptureRangeAsLocalizedBooks(scriptureRange: string): string {
+    return this.i18n.enumerateList(booksFromScriptureRange(scriptureRange).map(b => this.i18n.localizeBook(b)));
+  }


(It looks like my comment was not shown in context, which is a concern Devin raised about sorting. I was expressing that I'm not sure we need to be concerned about what Devin had raised there.)

marksvc

I did not merge so you can consider the various non-blocking comments first.

@marksvc made 1 comment.
Reviewable status: 0 of 9 files reviewed, 3 unresolved discussions (waiting on RaymondLuong3).

RaymondLuong3

Thanks for the comments. I am satisfied the discussions.

@RaymondLuong3 made 1 comment and resolved 3 discussions.
Reviewable status: 0 of 9 files reviewed, all discussions resolved (waiting on marksvc).

Nateowami · 2026-02-20T20:36:45Z

@RaymondLuong3 Is the PR title not describing the original behavior now?

RaymondLuong3 · 2026-02-20T20:38:12Z

@Nateowami This is the original behaviour for the drafting summary step, but it was not the original for the draft history which is the change made in this PR.

RaymondLuong3 added the testing not required label Feb 19, 2026

RaymondLuong3 force-pushed the fix/summarize-books branch from 46e88fb to de6eec7 Compare February 19, 2026 20:59

RaymondLuong3 commented Feb 19, 2026

View reviewed changes

marksvc assigned marksvc and unassigned marksvc Feb 19, 2026

devin-ai-integration bot reviewed Feb 20, 2026

View reviewed changes

marksvc approved these changes Feb 20, 2026

View reviewed changes

marksvc reviewed Feb 20, 2026

View reviewed changes

RaymondLuong3 commented Feb 20, 2026

View reviewed changes

RaymondLuong3 changed the title ~~Collapse consecutive translate books to ranges~~ List consecutive translate books rather than ranges Feb 20, 2026

RaymondLuong3 added 3 commits February 20, 2026 13:22

Collapse consecutive translate books to ranges

2b79d3e

List individual books for translation

e220161

Show translation books individually

bfd0af3

RaymondLuong3 force-pushed the fix/summarize-books branch from f0088d5 to bfd0af3 Compare February 20, 2026 20:22

RaymondLuong3 merged commit df721c0 into master Feb 20, 2026
20 of 22 checks passed

RaymondLuong3 deleted the fix/summarize-books branch February 20, 2026 20:41

RaymondLuong3 mentioned this pull request Feb 25, 2026

SF-3731 Show message on summary step when no training books selected #3711

Merged

Uh oh!

Conversation

RaymondLuong3 commented Feb 19, 2026 • edited by devin-ai-integration bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Feb 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Nateowami commented Feb 19, 2026

Uh oh!

RaymondLuong3 left a comment

Choose a reason for hiding this comment

Uh oh!

RaymondLuong3 left a comment

Choose a reason for hiding this comment

Uh oh!

RaymondLuong3 left a comment

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration bot left a comment

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration bot Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

marksvc Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

marksvc Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

devin-ai-integration bot Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

marksvc commented Feb 20, 2026

Uh oh!

marksvc Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

marksvc Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

marksvc Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

marksvc left a comment

Choose a reason for hiding this comment

Uh oh!

RaymondLuong3 left a comment

Choose a reason for hiding this comment

Uh oh!

Nateowami commented Feb 20, 2026

Uh oh!

RaymondLuong3 commented Feb 20, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

RaymondLuong3 commented Feb 19, 2026 •

edited by devin-ai-integration bot

Loading

codecov bot commented Feb 19, 2026 •

edited

Loading