SF-3617 Warn user when training source books are blank

[RaymondLuong3]

Source projects can have books that have the template but are essentially empty if there are no translated verses. The result is that users may choose to translate or train on empty source books without knowing it. This PR will notify users if their source book is empty and will not allow these books to be chosen for translation or training sources.

If a book as a single verse translated, it can be used as a source. But we can adjust this if desired.

---

This change is 

[codecov]

Codecov Report

❌ Patch coverage is 88.23529% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.91%. Comparing base (648dfa2) to head (a2c477d).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...rc/app/shared/progress-service/progress.service.ts	86.36%	2 Missing and 1 partial ⚠️
...neration-steps/draft-generation-steps.component.ts	89.65%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3562   +/-   ##
=======================================
  Coverage   82.91%   82.91%           
=======================================
  Files         605      605           
  Lines       36988    37027   +39     
  Branches     6056     6046   -10     
=======================================
+ Hits        30667    30701   +34     
- Misses       5395     5412   +17     
+ Partials      926      914   -12     

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

[marksvc]

@marksvc reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @RaymondLuong3)

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/progress-service/progress.service.ts line 251 at r1 (raw file):

      const chapterTextDocPromises = Promise.all(bookChapters.map(c => this.projectService.getText(c)));
      // set the map of books to text docs
      void chapterTextDocPromises.then(textDocs => {

This section, with its promise handling, seemed a bit awkward, but I assume you are doing it this way so we have a performance gain.

---

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

      { text: { bookNum: 3 }, translated: 100, percentage: 100 } as TextProgress,
      { text: { bookNum: 4 }, translated: 100, percentage: 100 } as TextProgress,
      { text: { bookNum: 5 }, translated: 100, percentage: 100 } as TextProgress,

Hmm, things were making sense until I saw this setting saying that book 5 does have progress. So it wouldn't be empty. How does this relate to other diffs below then?

---

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

              isPresentInASource = true;
              if (await this.bookHasVerseContent(trainingSources[0]!.projectRef, bookNum)) {
                this.availableTrainingBooks[trainingSources[0]!.projectRef].push({

We get so many errors from SF from trying to read fields on undefined values. Avoiding non-null assertions can (1) give more meaningful error reporting, and/or (2) shepherd our code to do something more appropriate when a problem occurs.

For example, instead of

function doSomething(arg) {
  arg!.process();
}

and hearing that a error occurred because process is not a method of undefined, we can instead write

function doSomething(arg) {
  if (arg == null) throw new Error("Can not do activity with null arg.");
  arg.process();
}

to get .. maybe a better error message. Or we could write

function doSomething(arg) {
  if (arg == null) return;
  arg.process();
}

if that would be appropriate, to guide the program when arg is null.

Anyway, I encourage avoiding the non-null assertion operator (!) when possible.

---

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

  private async bookHasVerseContent(projectId: string, bookNum: number): Promise<boolean> {
    if (!this.sourceProgress.has(projectId)) return false;
    return (this.sourceProgress.get(projectId)!.find(p => p.text.bookNum === bookNum)?.translated ?? 0) > 0;

Good to know, this one can be written without the non-null assertion in

if foo.has,
foo.get!

by doing something like

const progress: TextProgress[] | undefined = this.sourceProgress.get(projectId);
if (progress == null) return false;
return (progress.find(p => p.text.bookNum === bookNum)?.translated ?? 0) > 0;

where we just make one call to foo.get, which returns undefined if it is not found.

[RaymondLuong3]

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

---

src/SIL.XForge.Scripture/ClientApp/src/app/shared/progress-service/progress.service.ts line 251 at r1 (raw file):

Previously, marksvc wrote…

This section, with its promise handling, seemed a bit awkward, but I assume you are doing it this way so we have a performance gain.

That is correct. I wouldn't normally write it this way but I think there is a noticeable performance gain (I have not tested that). In other places in this file we do a similar thing to access the text docs.

---

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

Previously, marksvc wrote…

We get so many errors from SF from trying to read fields on undefined values. Avoiding non-null assertions can (1) give more meaningful error reporting, and/or (2) shepherd our code to do something more appropriate when a problem occurs.

For example, instead of

function doSomething(arg) {
  arg!.process();
}

and hearing that a error occurred because process is not a method of undefined, we can instead write

function doSomething(arg) {
  if (arg == null) throw new Error("Can not do activity with null arg.");
  arg.process();
}

to get .. maybe a better error message. Or we could write

function doSomething(arg) {
  if (arg == null) return;
  arg.process();
}

if that would be appropriate, to guide the program when arg is null.

Anyway, I encourage avoiding the non-null assertion operator (!) when possible.

Good thinking. I've updated this to not use not null assertions where I could.

---

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

Previously, marksvc wrote…

Good to know, this one can be written without the non-null assertion in

if foo.has,
foo.get!

by doing something like

const progress: TextProgress[] | undefined = this.sourceProgress.get(projectId);
if (progress == null) return false;
return (progress.find(p => p.text.bookNum === bookNum)?.translated ?? 0) > 0;

where we just make one call to foo.get, which returns undefined if it is not found.

Nice!

---

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

Previously, marksvc wrote…

Hmm, things were making sense until I saw this setting saying that book 5 does have progress. So it wouldn't be empty. How does this relate to other diffs below then?

Each describe has its own mocks. If we need book 5 to be empty, the describes would have to set that up. This one is for the test wide default. I'll add default to the name.

[marksvc]

@marksvc reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @RaymondLuong3)

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

Each describe has its own mocks. If we need book 5 to be empty, the describes would have to set that up. This one is for the test wide default. I'll add default to the name.

Ah! Thank you.

---

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

              }
            } else {
              this.unusableTrainingSourceBooks.push(bookNum);

Hmm. I was looking at this a bit more. I'm surprised at how this is written, since the lines below this line go on to check if the second training source has this particular book. It seems that the first block could end up marking the book as an "unusable" training source book, but the upcoming block could find that it is in fact usable. I haven't studied the component enough to know if that is a mistake in behaviour.

It wasn't introduced by your change.

[RaymondLuong3]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @marksvc)

---

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

Previously, marksvc wrote…

Hmm. I was looking at this a bit more. I'm surprised at how this is written, since the lines below this line go on to check if the second training source has this particular book. It seems that the first block could end up marking the book as an "unusable" training source book, but the upcoming block could find that it is in fact usable. I haven't studied the component enough to know if that is a mistake in behaviour.

It wasn't introduced by your change.

That is a known deficiency that should be addressed, but probably in a separate PR.