SF-3638 Store the drafted scripture range and current draft's scripture range in the project

[pmachapman]

This PR changes Scripture Forge to store a list of all the books drafted as a scripture range, and a list of all the books in the current draft as a scripture range, rather than the hasDraft flag, which was reset when a new draft is generated.

---

This change is 

[codecov]

Codecov Report

❌ Patch coverage is 97.02970% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.82%. Comparing base (a61171f) to head (b7e6266).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...SIL.XForge.Scripture/Services/MachineApiService.cs	94.44%	0 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3574      +/-   ##
==========================================
+ Coverage   82.81%   82.82%   +0.01%     
==========================================
  Files         608      608              
  Lines       37243    37186      -57     
  Branches     6100     6074      -26     
==========================================
- Hits        30841    30800      -41     
- Misses       5475     5476       +1     
+ Partials      927      910      -17     

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

[marksvc]

@marksvc reviewed 36 of 36 files at r1, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @pmachapman)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-preview-books/draft-preview-books.component.spec.ts line 439 at r1 (raw file):

  booksWithDrafts: BookWithDraft[] = [
    { bookNumber: 1, bookId: 'GEN', canEdit: true, chaptersWithDrafts: [1, 2, 3], draftApplied: false },
    { bookNumber: 2, bookId: 'EXO', canEdit: true, chaptersWithDrafts: [1], draftApplied: false },

It looks like perhaps this line should be changed from [1] to [1,2] now, since bookNum 2 chapter number 2 will now be considered to have a draft, where as before it was not.

I notice this failed a couple tests when I tried it.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.service.ts line 392 at r1 (raw file):

        books = new Set<number>(
          // Legacy calculation for very old drafts
          // eslint-disable-next-line @typescript-eslint/no-deprecated

I wonder if maybe you meant to mark hasDraft or something as deprecated, but that wasn't part of the PR?

I see that hasDraft got marked as deprecated in C#.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.service.spec.ts line 918 at r1 (raw file):

            },
            { bookNum: 63, chapters: [{ number: 1, hasDraft: true }] },
            { bookNum: 64, chapters: [{ number: 1, hasDraft: true }] }

The change to the above lines is confusing. And not necessary for the tests to pass. I wonder if we should be removing the hasDraft field altogether?

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.spec.ts line 189 at r1 (raw file):

            {
              bookNum: 2,
              chapters: [{ number: 1 }],

This might not matter a ton in the current situation, but the change at hand is making hasDraft return true even if this method was called with preTranslate=false. I experimented with how to make the hasDraft return true for projects that were made with preTranslate=true. What do you think about adjusting it in something like the following way?

diff --git a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.spec.ts b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.spec.ts
index 24dac538b..4009f9a1e 100644
--- a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.spec.ts
+++ b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.spec.ts
@@ -4,6 +4,7 @@ import { MatDialogRef, MatDialogState } from '@angular/material/dialog';
 import { provideRouter } from '@angular/router';
 import { SystemRole } from 'realtime-server/lib/esm/common/models/system-role';
 import { createTestUser } from 'realtime-server/lib/esm/common/models/user-test-data';
+import { SFProjectProfile } from 'realtime-server/lib/esm/scriptureforge/models/sf-project';
 import { SFProjectRole } from 'realtime-server/lib/esm/scriptureforge/models/sf-project-role';
 import { createTestProjectProfile } from 'realtime-server/lib/esm/scriptureforge/models/sf-project-test-data';
 import { TextInfoPermission } from 'realtime-server/lib/esm/scriptureforge/models/text-info-permission';
@@ -109,6 +110,7 @@ describe('DraftGenerationComponent', () => {
 
     // Default setup
     setup(): void {
+      mockSFProjectService = jasmine.createSpyObj<SFProjectService>(['hasDraft']);
       mockDialogService = jasmine.createSpyObj<DialogService>(['openGenericDialog', 'message']);
       mockNoticeService = jasmine.createSpyObj<NoticeService>([
         'loadingStarted',
@@ -157,8 +159,6 @@ describe('DraftGenerationComponent', () => {
         newDraftHistory: createTestFeatureFlag(false),
         usfmFormat: createTestFeatureFlag(false)
       });
-      mockSFProjectService = jasmine.createSpyObj<SFProjectService>(['hasDraft']);
-      mockSFProjectService.hasDraft.and.returnValue(true);
     }
 
     static initProject(currentUserId: string, preTranslate: boolean = true): void {
@@ -199,6 +199,14 @@ describe('DraftGenerationComponent', () => {
           }
         })
       } as SFProjectProfileDoc;
+      const matchThisProject = {
+        asymmetricMatch: (proj: SFProjectProfile | undefined) => {
+          if (proj == null) return false;
+          return proj.paratextId === projectDoc.data?.paratextId;
+        }
+      };
+      mockSFProjectService.hasDraft.withArgs(matchThisProject, jasmine.anything()).and.returnValue(preTranslate);
+      mockSFProjectService.hasDraft.withArgs(matchThisProject).and.returnValue(preTranslate);
       mockAuthService = jasmine.createSpyObj<AuthService>(['requestParatextCredentialUpdate'], {
         currentUserId,
         currentUserRoles: [SystemRole.User]

---

src/SIL.XForge.Scripture/Models/DraftConfig.cs line 24 at r1 (raw file):

    /// <summary>
    /// A scripture range containing the books that have been drafted and are available in Scripture Forge.

During the review, I've come to understand that this includes multiple prior drafts, from perhaps different sets of books.

---

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 1074 at r1 (raw file):

            if (translationBuild is not null)
            {
                // Verify that each book/chapter from the translationBuild is marked HasDraft = true

This comment might need to be updated?

---

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 1130 at r1 (raw file):

                                // Add existing chapters to the books chapter list
                                scriptureRangesWithDrafts[book].AddRange(existingChapters);

Hmm. It looks like this will result in re-adding already-recorded chapters into the scriptureRangesWithDrafts[foo] Lists. A few lines above, we do out list<int> existingChapters, and suppose it could be a list of 1,2,3. Then in the foreach, we might add 4,5,6 into existingChapters. Then we AddRange by appending 1,2,3,4,5,6 to the end of the current 1,2,3 list.

Your PR did not introduce this, but it's something I noticed while examining the code.

---

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 1895 at r1 (raw file):

                    ';',
                    translationBuild
                        .Pretranslate?.SelectMany(p => p.SourceFilters)

Hmm. I'm concerned that we may have a semi-colon-delimited list of books that is not in canonical order. But maybe some aspect of this makes it always work out?

Also, it seems like we could have duplicate book identifiers in the result. Though whether or not that is possible may depend on how plural the input here is. But from how it is written, it looks like there may be multiple sets of data coming together to produce the list. So it may need deduplicated as well.

---

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 1918 at r1 (raw file):

                    projectDoc.Data.TranslateConfig.DraftConfig.DraftedScriptureRange ?? string.Empty;
                ScriptureRangeParser scriptureRangeParser = new ScriptureRangeParser();
                List<string> currentBooks = [.. scriptureRangeParser.GetChapters(currentScriptureRange).Keys];

Why are we using ScriptureRangeParser? Are we expecting that the currentScriptureRange might be in a form like "GEN-NUM" rather than "GEN;EXO;LEV;NUM"? Yet we wrote it in whatever form it is into CurrentScriptureRange in the DB a few lines earlier.

Oh, maybe we are using this to split the list apart rather than manually split on semicolons? (i.e., the ScriptureRangeParser is splitting on the semicolons for us?)

---

test/SIL.XForge.Scripture.Tests/Services/MachineApiServiceTests.cs line 3673 at r1 (raw file):

    [Test]
    public async Task RetrievePreTranslationStatusAsync_UpdatesPreTranslationStatusAndTextDocuments()

Would there be any significance to remove "StatusAnd" from the test/method name?

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor.component.spec.ts line 4138 at r1 (raw file):

        });
        when(mockedPermissionsService.canAccessDrafts(anything(), anything())).thenReturn(true);
        when(mockedSFProjectService.hasDraft(anything(), anything())).thenReturn(true);

Shouldn't this be sprinkled in most of the other tests in this describe group as well?

[pmachapman]

Thanks for your helpful comments, Mark!

Reviewable status: 23 of 36 files reviewed, 8 unresolved discussions (waiting on @marksvc)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.spec.ts line 189 at r1 (raw file):

Previously, marksvc wrote…

This might not matter a ton in the current situation, but the change at hand is making hasDraft return true even if this method was called with preTranslate=false. I experimented with how to make the hasDraft return true for projects that were made with preTranslate=true. What do you think about adjusting it in something like the following way?

diff --git a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.spec.ts b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.spec.ts
index 24dac538b..4009f9a1e 100644
--- a/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.spec.ts
+++ b/src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.component.spec.ts
@@ -4,6 +4,7 @@ import { MatDialogRef, MatDialogState } from '@angular/material/dialog';
 import { provideRouter } from '@angular/router';
 import { SystemRole } from 'realtime-server/lib/esm/common/models/system-role';
 import { createTestUser } from 'realtime-server/lib/esm/common/models/user-test-data';
+import { SFProjectProfile } from 'realtime-server/lib/esm/scriptureforge/models/sf-project';
 import { SFProjectRole } from 'realtime-server/lib/esm/scriptureforge/models/sf-project-role';
 import { createTestProjectProfile } from 'realtime-server/lib/esm/scriptureforge/models/sf-project-test-data';
 import { TextInfoPermission } from 'realtime-server/lib/esm/scriptureforge/models/text-info-permission';
@@ -109,6 +110,7 @@ describe('DraftGenerationComponent', () => {
 
     // Default setup
     setup(): void {
+      mockSFProjectService = jasmine.createSpyObj<SFProjectService>(['hasDraft']);
       mockDialogService = jasmine.createSpyObj<DialogService>(['openGenericDialog', 'message']);
       mockNoticeService = jasmine.createSpyObj<NoticeService>([
         'loadingStarted',
@@ -157,8 +159,6 @@ describe('DraftGenerationComponent', () => {
         newDraftHistory: createTestFeatureFlag(false),
         usfmFormat: createTestFeatureFlag(false)
       });
-      mockSFProjectService = jasmine.createSpyObj<SFProjectService>(['hasDraft']);
-      mockSFProjectService.hasDraft.and.returnValue(true);
     }
 
     static initProject(currentUserId: string, preTranslate: boolean = true): void {
@@ -199,6 +199,14 @@ describe('DraftGenerationComponent', () => {
           }
         })
       } as SFProjectProfileDoc;
+      const matchThisProject = {
+        asymmetricMatch: (proj: SFProjectProfile | undefined) => {
+          if (proj == null) return false;
+          return proj.paratextId === projectDoc.data?.paratextId;
+        }
+      };
+      mockSFProjectService.hasDraft.withArgs(matchThisProject, jasmine.anything()).and.returnValue(preTranslate);
+      mockSFProjectService.hasDraft.withArgs(matchThisProject).and.returnValue(preTranslate);
       mockAuthService = jasmine.createSpyObj<AuthService>(['requestParatextCredentialUpdate'], {
         currentUserId,
         currentUserRoles: [SystemRole.User]

Done. Good idea.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.service.ts line 392 at r1 (raw file):

Previously, marksvc wrote…

I wonder if maybe you meant to mark hasDraft or something as deprecated, but that wasn't part of the PR?

I see that hasDraft got marked as deprecated in C#.

hasDraft is deprecated in TypeScript and C#. I have reworked this to not use hasDraft as I wrote this before I wrote the migrator.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.service.spec.ts line 918 at r1 (raw file):

Previously, marksvc wrote…

The change to the above lines is confusing. And not necessary for the tests to pass. I wonder if we should be removing the hasDraft field altogether?

I have removed hasDraft tests, as the migrator removes the need for them.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-preview-books/draft-preview-books.component.spec.ts line 439 at r1 (raw file):

Previously, marksvc wrote…

It looks like perhaps this line should be changed from [1] to [1,2] now, since bookNum 2 chapter number 2 will now be considered to have a draft, where as before it was not.

I notice this failed a couple tests when I tried it.

Yes, it should. I have removed an unnecessary tests, and fixed the other one which failed.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/editor/editor.component.spec.ts line 4138 at r1 (raw file):

Previously, marksvc wrote…

Shouldn't this be sprinkled in most of the other tests in this describe group as well?

I didn't include them, as the other tests didn't call these methods.

---

src/SIL.XForge.Scripture/Models/DraftConfig.cs line 24 at r1 (raw file):

Previously, marksvc wrote…

During the review, I've come to understand that this includes multiple prior drafts, from perhaps different sets of books.

Good point. I've added a remake to explain this.

---

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 1074 at r1 (raw file):

Previously, marksvc wrote…

This comment might need to be updated?

I have removed the comment as it is no longer really applicable.

---

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 1130 at r1 (raw file):

Previously, marksvc wrote…

Hmm. It looks like this will result in re-adding already-recorded chapters into the scriptureRangesWithDrafts[foo] Lists. A few lines above, we do out list<int> existingChapters, and suppose it could be a list of 1,2,3. Then in the foreach, we might add 4,5,6 into existingChapters. Then we AddRange by appending 1,2,3,4,5,6 to the end of the current 1,2,3 list.

Your PR did not introduce this, but it's something I noticed while examining the code.

As we don't need the chapters, I have removed this code.

---

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 1895 at r1 (raw file):

Previously, marksvc wrote…

Hmm. I'm concerned that we may have a semi-colon-delimited list of books that is not in canonical order. But maybe some aspect of this makes it always work out?

Also, it seems like we could have duplicate book identifiers in the result. Though whether or not that is possible may depend on how plural the input here is. But from how it is written, it looks like there may be multiple sets of data coming together to produce the list. So it may need deduplicated as well.

I have updated this to use the same code as GetLastCompletedPreTranslationBuildAsync(), which will stop duplicate book identifiers.

It is OK if these are not in canonical order.

---

src/SIL.XForge.Scripture/Services/MachineApiService.cs line 1918 at r1 (raw file):

Previously, marksvc wrote…

Why are we using ScriptureRangeParser? Are we expecting that the currentScriptureRange might be in a form like "GEN-NUM" rather than "GEN;EXO;LEV;NUM"? Yet we wrote it in whatever form it is into CurrentScriptureRange in the DB a few lines earlier.

Oh, maybe we are using this to split the list apart rather than manually split on semicolons? (i.e., the ScriptureRangeParser is splitting on the semicolons for us?)

Yes, it is doing the splitting for us.

---

test/SIL.XForge.Scripture.Tests/Services/MachineApiServiceTests.cs line 3673 at r1 (raw file):

Previously, marksvc wrote…

Would there be any significance to remove "StatusAnd" from the test/method name?

Done. It is clearer without the StatusAnd.

[marksvc]

@marksvc reviewed 13 of 13 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @pmachapman)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.service.ts line 392 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

hasDraft is deprecated in TypeScript and C#. I have reworked this to not use hasDraft as I wrote this before I wrote the migrator.

Can you point out where hasDraft is deprecated in TypeScript? I would have thought it would be in text-info.ts, but there I read

export interface Chapter {
  number: number;
  lastVerse: number;
  isValid: boolean;
  permissions: { [userRef: string]: string };
  hasAudio?: boolean;
  hasDraft?: boolean;
  draftApplied?: boolean;
}

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.service.ts line 391 at r2 (raw file):

// Legacy ...

Is this comment still accurate?

---

src/SIL.XForge.Scripture/Models/DraftConfig.cs line 24 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

Good point. I've added a remake to explain this.

👍

[pmachapman]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @marksvc)

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.service.ts line 392 at r1 (raw file):

Previously, marksvc wrote…

Can you point out where hasDraft is deprecated in TypeScript? I would have thought it would be in text-info.ts, but there I read

export interface Chapter {
  number: number;
  lastVerse: number;
  isValid: boolean;
  permissions: { [userRef: string]: string };
  hasAudio?: boolean;
  hasDraft?: boolean;
  draftApplied?: boolean;
}

So I wrote it in the .d.ts file, not the .ts file.... :-/ Fixed.

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.service.ts line 391 at r2 (raw file):

Previously, marksvc wrote…

// Legacy ...

Is this comment still accurate?

Yes, as these old drafts will be missing lastCompletedBuild?.additionalInfo?.translationScriptureRanges (see code above). The hasDraft property will be migrated by the migrator.

[marksvc]

@marksvc reviewed 5 of 5 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @pmachapman)