Code review fixes

Author: pmachapman

src/RealtimeServer/scriptureforge/models/text-info.ts

@@ -4,6 +4,9 @@ export interface Chapter {
   isValid: boolean;
   permissions: { [userRef: string]: string };
   hasAudio?: boolean;
+  /**
+   * @deprecated Use SFProjectService.hasDraft() instead
+   */
   hasDraft?: boolean;
   draftApplied?: boolean;
 }

src/RealtimeServer/scriptureforge/services/sf-project-migrations.ts

@@ -638,6 +638,7 @@ class SFProjectMigration28 extends DocMigration {
     const ops: Op[] = [];
     if (doc.data?.texts != null && doc.data?.translateConfig?.draftConfig?.currentScriptureRange == null) {
       const currentScriptureRange = doc.data.texts
+        // eslint-disable-next-line @typescript-eslint/no-deprecated
         .filter((t: TextInfo) => t.chapters.some(c => c.hasDraft))
         .map((t: TextInfo) => Canon.bookNumberToId(t.bookNum, ''))
         .filter((id: string) => id !== '')

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';
@@ -157,8 +158,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 {
@@ -210,6 +209,13 @@ describe('DraftGenerationComponent', () => {
         projectDoc$: of(projectDoc),
         changes$: of(projectDoc)
       });
+      const matchThisProject = {
+        asymmetricMatch: (proj: SFProjectProfile | undefined) =>
+          proj != null && proj.paratextId === projectDoc.data?.paratextId
+      };
+      mockSFProjectService = jasmine.createSpyObj<SFProjectService>(['hasDraft']);
+      mockSFProjectService.hasDraft.withArgs(matchThisProject).and.returnValue(preTranslate);
+      mockSFProjectService.hasDraft.withArgs(matchThisProject, jasmine.anything()).and.returnValue(preTranslate);
     }
 
     get configureDraftButton(): HTMLElement | null {

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.service.spec.ts

@@ -906,16 +906,18 @@ describe('DraftGenerationService', () => {
       const projectDoc: SFProjectProfileDoc = {
         id: projectId,
         data: createTestProjectProfile({
+          translateConfig: {
+            draftConfig: {
+              currentScriptureRange: '1JN;2JN;3JN'
+            }
+          },
           texts: [
             {
               bookNum: 62,
-              chapters: [
-                { number: 1, hasDraft: true },
-                { number: 2, hasDraft: true }
-              ]
+              chapters: [{ number: 1 }, { number: 2 }]
             },
-            { bookNum: 63, chapters: [{ number: 1, hasDraft: true }] },
-            { bookNum: 64, chapters: [{ number: 1, hasDraft: true }] }
+            { bookNum: 63, chapters: [{ number: 1 }] },
+            { bookNum: 64, chapters: [{ number: 1 }] }
           ]
         })
       } as SFProjectProfileDoc;
@@ -951,7 +953,12 @@ describe('DraftGenerationService', () => {
       const projectDoc: SFProjectProfileDoc = {
         id: projectId,
         data: createTestProjectProfile({
-          texts: [{ bookNum: 62, chapters: [{ number: 1, hasDraft: true }] }]
+          translateConfig: {
+            draftConfig: {
+              currentScriptureRange: '1JN'
+            }
+          },
+          texts: [{ bookNum: 62, chapters: [{ number: 1 }] }]
         })
       } as SFProjectProfileDoc;
       const lastCompletedBuild: BuildDto = {

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-generation.service.ts

@@ -389,8 +389,7 @@ export class DraftGenerationService {
       if (books.size === 0) {
         books = new Set<number>(
           // Legacy calculation for very old drafts
-          // eslint-disable-next-line @typescript-eslint/no-deprecated
-          projectDoc.data.texts.filter(text => text.chapters.some(c => c.hasDraft)).map(text => text.bookNum)
+          booksFromScriptureRange(projectDoc.data.translateConfig.draftConfig.currentScriptureRange)
         );
       }
 

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-preview-books/draft-preview-books.component.spec.ts

@@ -142,21 +142,6 @@ describe('DraftPreviewBooks', () => {
     verify(mockedDraftHandlingService.getAndApplyDraftAsync(anything(), anything(), anything(), anything())).times(3);
   }));
 
-  it('can apply chapters with drafts and skips chapters without drafts', fakeAsync(() => {
-    env = new TestEnvironment();
-    const bookWithDraft: BookWithDraft = env.booksWithDrafts[1];
-    setupDialog('project01');
-    when(mockedDraftHandlingService.getAndApplyDraftAsync(anything(), anything(), anything(), anything())).thenResolve(
-      undefined
-    );
-    expect(env.getBookButtonAtIndex(1).querySelector('.book-more')).toBeTruthy();
-    env.component.chooseProjectToAddDraft(bookWithDraft);
-    tick();
-    env.fixture.detectChanges();
-    verify(mockedDialogService.openMatDialog(DraftApplyDialogComponent, anything())).once();
-    verify(mockedDraftHandlingService.getAndApplyDraftAsync(anything(), anything(), anything(), anything())).times(1);
-  }));
-
   it('can apply a historic draft', fakeAsync(() => {
     env = new TestEnvironment({
       additionalInfo: {
@@ -174,7 +159,7 @@ describe('DraftPreviewBooks', () => {
     tick();
     env.fixture.detectChanges();
     verify(mockedDialogService.openMatDialog(DraftApplyDialogComponent, anything())).once();
-    verify(mockedDraftHandlingService.getAndApplyDraftAsync(anything(), anything(), anything(), anything())).times(1);
+    verify(mockedDraftHandlingService.getAndApplyDraftAsync(anything(), anything(), anything(), anything())).times(2);
   }));
 
   it('can open dialog with the current project', fakeAsync(() => {
@@ -436,7 +421,7 @@ class TestEnvironment {
 
   booksWithDrafts: BookWithDraft[] = [
     { bookNumber: 1, bookId: 'GEN', canEdit: true, chaptersWithDrafts: [1, 2, 3], draftApplied: false },
-    { bookNumber: 2, bookId: 'EXO', canEdit: true, chaptersWithDrafts: [1], draftApplied: false },
+    { bookNumber: 2, bookId: 'EXO', canEdit: true, chaptersWithDrafts: [1, 2], draftApplied: false },
     { bookNumber: 3, bookId: 'LEV', canEdit: false, chaptersWithDrafts: [1, 2], draftApplied: false }
   ];
 

src/SIL.XForge.Scripture/Models/DraftConfig.cs

@@ -23,5 +23,8 @@ public class DraftConfig
     /// <summary>
     /// A scripture range containing the books that have been drafted and are available in Scripture Forge.
     /// </summary>
+    /// <remarks>
+    /// This is a combination of the scripture ranges of previous drafts.
+    /// </remarks>
     public string? DraftedScriptureRange { get; set; }
 }

src/SIL.XForge.Scripture/Services/MachineApiService.cs

@@ -1072,73 +1072,9 @@ await translationEnginesClient.GetAllBuildsAsync(translationEngineId, cancellati
                 .MaxBy(b => b.DateFinished);
             if (translationBuild is not null)
             {
-                // Verify that each book/chapter from the translationBuild is marked HasDraft = true
-                // If the projects texts chapters are not all marked as having a draft, then the webhook likely failed
-                // and we want to retrieve the pre-translation status to update the chapters as having a draft
-                Dictionary<string, List<int>> scriptureRangesWithDrafts = [];
-
-                IList<PretranslateCorpus> pretranslateCorpus = translationBuild.Pretranslate ?? [];
-
-                // Retrieve the user secret
-                Attempt<UserSecret> attempt = await userSecrets.TryGetAsync(curUserId, cancellationToken);
-                if (!attempt.TryResult(out UserSecret userSecret))
-                {
-                    throw new DataNotFoundException("The user does not exist.");
-                }
-
-                ScrVers versification =
-                    paratextService.GetParatextSettings(userSecret, project.ParatextId)?.Versification
-                    ?? VerseRef.defaultVersification;
-
-                ScriptureRangeParser scriptureRangeParser = new ScriptureRangeParser(versification);
-                foreach (PretranslateCorpus ptc in pretranslateCorpus)
-                {
-                    // We are using the TranslationBuild.Pretranslate.SourceFilters.ScriptureRange to find the
-                    // books selected for drafting. Some projects may have used the now obsolete field
-                    // TranslationBuild.Pretranslate.ScriptureRange and will not get checked for webhook failures.
-                    foreach (
-                        ParallelCorpusFilter source in ptc.SourceFilters?.Where(s => s.ScriptureRange is not null) ?? []
-                    )
-                    {
-                        foreach (
-                            (string book, List<int> bookChapters) in scriptureRangeParser.GetChapters(
-                                source.ScriptureRange
-                            )
-                        )
-                        {
-                            int bookNum = Canon.BookIdToNumber(book);
-                            // Ensure that if chapters is blank, it contains every chapter in the book
-                            List<int> chapters = bookChapters;
-                            if (chapters.Count == 0)
-                            {
-                                chapters = [.. Enumerable.Range(1, versification.GetLastChapter(bookNum))];
-                            }
-
-                            // Set or merge the list of chapters
-                            if (!scriptureRangesWithDrafts.TryGetValue(book, out List<int> existingChapters))
-                            {
-                                scriptureRangesWithDrafts[book] = chapters;
-                            }
-                            else
-                            {
-                                // Merge new chapters into existing list, avoiding duplicates
-                                foreach (int chapter in chapters.Where(chapter => !existingChapters.Contains(chapter)))
-                                {
-                                    existingChapters.Add(chapter);
-                                }
-
-                                // Add existing chapters to the books chapter list
-                                scriptureRangesWithDrafts[book].AddRange(existingChapters);
-                            }
-                        }
-                    }
-                }
-
                 // See if the current scripture range has changed
-                if (
-                    project.TranslateConfig.DraftConfig.CurrentScriptureRange
-                    != string.Join(';', scriptureRangesWithDrafts.Keys)
-                )
+                string currentScriptureRange = GetDraftedScriptureRange(translationBuild);
+                if (project.TranslateConfig.DraftConfig.CurrentScriptureRange != currentScriptureRange)
                 {
                     backgroundJobClient.Enqueue<IMachineApiService>(r =>
                         r.RetrievePreTranslationStatusAsync(sfProjectId, CancellationToken.None)
@@ -1890,13 +1826,7 @@ await projectSecrets.UpdateAsync(
                 }
 
                 // Store the CurrentScriptureRange based on the books we asked Serval to draft
-                string currentScriptureRange = string.Join(
-                    ';',
-                    translationBuild
-                        .Pretranslate?.SelectMany(p => p.SourceFilters)
-                        .Where(f => f.ScriptureRange != null)
-                        .Select(f => f.ScriptureRange) ?? []
-                );
+                string currentScriptureRange = GetDraftedScriptureRange(translationBuild);
                 if (string.IsNullOrWhiteSpace(currentScriptureRange))
                 {
                     throw new DataNotFoundException(
@@ -2392,6 +2322,35 @@ private static ServalEngineDto CreateDto(TranslationEngine translationEngine) =>
             TargetLanguageTag = translationEngine.TargetLanguage,
         };
 
+    /// <summary>
+    /// Gets the drafted scripture range for a translation build.
+    /// </summary>
+    /// <param name="translationBuild">The  translation build.</param>
+    /// <returns>The scripture range that was drafted.</returns>
+    private static string GetDraftedScriptureRange(TranslationBuild translationBuild)
+    {
+        List<string> booksWithDrafts = [];
+        ScriptureRangeParser scriptureRangeParser = new ScriptureRangeParser();
+        foreach (PretranslateCorpus ptc in translationBuild.Pretranslate ?? [])
+        {
+            // We are using the TranslationBuild.Pretranslate.SourceFilters.ScriptureRange to find the
+            // books selected for drafting. Some projects may have used the now obsolete field
+            // TranslationBuild.Pretranslate.ScriptureRange and will not get checked for webhook failures.
+            foreach (ParallelCorpusFilter source in ptc.SourceFilters?.Where(s => s.ScriptureRange is not null) ?? [])
+            {
+                foreach ((string book, List<int> _) in scriptureRangeParser.GetChapters(source.ScriptureRange))
+                {
+                    if (!booksWithDrafts.Contains(book))
+                    {
+                        booksWithDrafts.Add(book);
+                    }
+                }
+            }
+        }
+
+        return string.Join(';', booksWithDrafts);
+    }
+
     /// <summary>
     /// Gets the highest ranked user id on a project.
     /// </summary>

test/SIL.XForge.Scripture.Tests/Services/MachineApiServiceTests.cs

@@ -3670,7 +3670,7 @@ public async Task RetrievePreTranslationStatusAsync_ReportsErrorWhenProjectSecre
     }
 
     [Test]
-    public async Task RetrievePreTranslationStatusAsync_UpdatesPreTranslationStatusAndTextDocuments()
+    public async Task RetrievePreTranslationStatusAsync_UpdatesPreTranslationTextDocuments()
     {
         // Set up test environment with a completed build
         var env = new TestEnvironment();

test/SIL.XForge.Scripture.Tests/Services/PreTranslationServiceTests.cs

@@ -740,7 +740,7 @@ public TestEnvironment(TestEnvironmentOptions? options = null)
                 )
                 .Returns(MatthewBookUsfm);
             Service = Substitute.ForPartsOf<PreTranslationService>(ProjectSecrets, TranslationEnginesClient);
-            if (options.MockPreTranslationParameters)
+            if (options.MockLegacyPreTranslationParameters)
             {
                 Service
                     .Configure()