SF-3892 Add support for new Serval Diagnostic messages

[pmachapman]

This change is 

[codecov]

Codecov Report

❌ Patch coverage is 84.42211% with 31 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.26%. Comparing base (b906996) to head (ac618c2).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...administration/build-confidences-export.service.ts	0.00%	18 Missing ⚠️
...late/editor/editor-draft/editor-draft.component.ts	78.26%	2 Missing and 3 partials ⚠️
...p/serval-administration/serval-builds.component.ts	20.00%	4 Missing ⚠️
...aft-history-entry/draft-history-entry.component.ts	57.14%	1 Missing and 2 partials ⚠️
...c/app/serval-administration/serval-build-report.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #4052   +/-   ##
=======================================
  Coverage   81.25%   81.26%           
=======================================
  Files         670      670           
  Lines       43467    43488   +21     
  Branches     7132     7135    +3     
=======================================
+ Hits        35321    35340   +19     
+ Misses       6976     6963   -13     
- Partials     1170     1185   +15     

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

[github-actions]

📸 Screenshot diff deployed! (7 changes)

View the visual diff at: https://pr-4052--sf-screenshot-diffs.netlify.app

[RaymondLuong3]

Thanks for your work on this. I did not do a thorough testing of the PR. I'll let the testers work on that part. A few comments to work through.

@RaymondLuong3 reviewed 57 files and all commit messages, and made 6 comments.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on pmachapman).

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/build-confidences/display-confidence.component.spec.ts line 49 at r1 (raw file):

    text(): DebugElement {
      return this.fixture.debugElement.query(By.css('span'));
    }

Nit: These could be getters instead of methods.

Code quote:

    icon(): DebugElement {
      return this.fixture.debugElement.query(By.css('mat-icon'));
    }

    text(): DebugElement {
      return this.fixture.debugElement.query(By.css('span'));
    }

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.html line 219 at r1 (raw file):

          <app-notice type="error" data-test-id="low-confidence-warning">
            {{ t("low_confidence_books", { number: selectedBooksWithLowConfidence }) }}
          </app-notice>

Is there a reason we are not showing the list of books with low confidence when there are more than one? It seems to me that the best option is to list all books with low confidence.

Code quote:

        } @else if (selectedBooksWithLowConfidence > 1) {
          <app-notice type="error" data-test-id="low-confidence-warning">
            {{ t("low_confidence_books", { number: selectedBooksWithLowConfidence }) }}
          </app-notice>

---

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

  get isLatestBuildCompleted(): boolean {
    return this.builds.length === 0 ? false : this.builds[0].state === BuildStates.Completed;
  }

From Devin:

Formatting gate keyed to oldest build, not latest

isLatestBuildCompleted reads this.builds[0].state, but getBuildHistory returns builds oldest-first (confirmed by the reverse() in loadHistory). So it reflects the oldest build's state. For any project with more than one draft build, the format-draft option is shown or hidden based on the wrong build.

Code quote:

  get isLatestBuildCompleted(): boolean {
    return this.builds.length === 0 ? false : this.builds[0].state === BuildStates.Completed;
  }

---

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

    fixture.detectChanges();
    tick(EDITOR_READY_TIMEOUT);
    expect(component.hasLowConfidence).toBeFalse();

I am confused about this test. Why is it that both the entries returned from getBuildHistory marked with low confidence, but component.hasLowConfidence is false for the first entry?

Code quote:

    expect(component.hasLowConfidence).toBeFalse();

---

src/SIL.XForge.Scripture/Controllers/SFProjectsRpcController.cs line 918 at r1 (raw file):

    }

    public async Task<IRpcMethodResult> SetQualityEstimationConfig(

Normally we would deprecate an endpoint and not immediately remove it to prevent errors from older clients. What is your reasoning for removing the endpoint outright?

Code quote:

    public async Task<IRpcMethodResult> SetQualityEstimationConfig(

[pmachapman]

@pmachapman made 5 comments and resolved 1 discussion.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on RaymondLuong3).

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/build-confidences/display-confidence.component.spec.ts line 49 at r1 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Nit: These could be getters instead of methods.

Done. Thank you!

---

src/SIL.XForge.Scripture/ClientApp/src/app/translate/draft-generation/draft-import-wizard/draft-import-wizard.component.html line 219 at r1 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Is there a reason we are not showing the list of books with low confidence when there are more than one? It seems to me that the best option is to list all books with low confidence.

This was a request from the EITL team, and is also in the design mock up at https://balsamiq.cloud/sghq53/pejrlz8/r78E8

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

From Devin:

Formatting gate keyed to oldest build, not latest

isLatestBuildCompleted reads this.builds[0].state, but getBuildHistory returns builds oldest-first (confirmed by the reverse() in loadHistory). So it reflects the oldest build's state. For any project with more than one draft build, the format-draft option is shown or hidden based on the wrong build.

Done.

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

I am confused about this test. Why is it that both the entries returned from getBuildHistory marked with low confidence, but component.hasLowConfidence is false for the first entry?

They are for different books. I've added comments to clarify.

---

src/SIL.XForge.Scripture/Controllers/SFProjectsRpcController.cs line 918 at r1 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Normally we would deprecate an endpoint and not immediately remove it to prevent errors from older clients. What is your reasoning for removing the endpoint outright?

This endpoint is only called from the Serval Admin page, so I think it is OK to remove outright?

[RaymondLuong3]

@RaymondLuong3 reviewed 11 files and all commit messages, made 1 comment, and resolved 3 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on pmachapman).

---

src/SIL.XForge.Scripture/Controllers/SFProjectsRpcController.cs line 918 at r1 (raw file):

Previously, pmachapman (Peter Chapman) wrote…

This endpoint is only called from the Serval Admin page, so I think it is OK to remove outright?

Yes, I think that is OK.

[RaymondLuong3]

I am guessing we need to wait for Serval 1.20 to be released to prod before we can merge this PR.

[pmachapman]

I am guessing we need to wait for Serval 1.20 to be released to prod before we can merge this PR.

@RaymondLuong3 Yes, that is correct.

[Nateowami]

@Nateowami made 1 comment.
Reviewable status: 53 of 57 files reviewed, 1 unresolved discussion (waiting on RaymondLuong3).

---

src/RealtimeServer/scriptureforge/services/sf-project-migrations.ts line 671 at r3 (raw file):

  async migrateDoc(doc: Doc): Promise<void> {
    const ops: Op[] = [];
    const qualityEstimationConfig = doc.data.translateConfig?.draftConfig?.qualityEstimationConfig;

Not sure whether it matters, but other migrations do doc.data?., contemplating that it could be nulllish. Maybe they're just wrong.

[pmachapman]

@pmachapman made 1 comment.
Reviewable status: 50 of 57 files reviewed, 1 unresolved discussion (waiting on RaymondLuong3).

---

src/RealtimeServer/scriptureforge/services/sf-project-migrations.ts line 671 at r3 (raw file):

Previously, Nateowami wrote…

Not sure whether it matters, but other migrations do doc.data?., contemplating that it could be nulllish. Maybe they're just wrong.

Done. Thank you - yes this would be an issue if the document was deleted from ShareDB, and for some reason not purged by the backend deletion routines.