SF-3610 Check for the latest draft for allowing formatting

[josephmyers]

Before, we only checked if the latest draft contained that book, so older drafts still had the formatting button enabled. Now, we confirm the button is only enabled when the latest revision is selected.

---

This change is 

[codecov]

Codecov Report

❌ Patch coverage is 73.68421% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.46%. Comparing base (e625975) to head (c5977f1).
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...SIL.XForge.Scripture/Services/MachineApiService.cs	70.96%	8 Missing and 1 partial ⚠️
...orge.Scripture/Controllers/MachineApiController.cs	70.83%	7 Missing ⚠️
...slate/draft-generation/draft-generation.service.ts	75.00%	1 Missing and 1 partial ⚠️
...late/editor/editor-draft/editor-draft.component.ts	84.61%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3533      +/-   ##
==========================================
- Coverage   82.47%   82.46%   -0.02%     
==========================================
  Files         605      605              
  Lines       36873    36947      +74     
  Branches     6044     6048       +4     
==========================================
+ Hits        30412    30468      +56     
- Misses       5555     5571      +16     
- Partials      906      908       +2     

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

[Nateowami]

@Nateowami reviewed 1 of 3 files at r1.
Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion

---

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

              [style.cursor]="canConfigureFormatting ? 'pointer' : 'not-allowed'"
            >
              <button mat-button (click)="navigateToFormatting()" [disabled]="!canConfigureFormatting">

If you can't select formatting options, the button shouldn't be available at all. In general we avoid disabling buttons (an exception is when offline, and a message clearly says why functionality is unavailable).

[Nateowami]

Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @josephmyers)

---

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

  get canConfigureFormatting(): boolean {
    return this.doesLatestHaveDraft && this.isSelectedDraftLatest;

The naming here is excellent. Thanks.

[josephmyers]

FYI, I'm also working on the "get latest build" refactor that we talked about last week

[Nateowami]

@josephmyers Can you please update the PR title to reflect the user impact, as requested by the contribution guidelines?

[RaymondLuong3]

Thanks for the work on this. One thing I noticed is that you can probably reuse the 'getLastCompletedPreTranslationBuildAsync()` method in the MachineApiService class and add a new flag to indicate that you want lastest build and not just completed ones.

@RaymondLuong3 reviewed 3 of 10 files at r2, 10 of 10 files at r3, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @josephmyers)

---

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

    /// background job enqueueing – it is a simple convenience accessor.
    /// </remarks>
    public async Task<ServalBuildDto?> GetLastPreTranslationBuildAsync(

It seems like this could be accomplished in GetLastCompletedPreTranslationBuildAsync() by passing in a parameter like 'completedOnly = false. Then you can update the name of the method to just be GetLastPreTranslationBuildAsync`.

We would still need to endpoint you added.

Code quote:

    public async Task<ServalBuildDto?> GetLastPreTranslationBuildAsync(

---

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

    [Test]
    public async Task GetLastPreTranslationBuildAsync_LatestByDateFinished_Success()

You will want to add a test to show that builds that are not completed can also be returned.

Code quote:

    public async Task GetLastPreTranslationBuildAsync_LatestByDateFinished_Success()

---

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

  });

  describe('getLastPreTranslationBuild', () => {

Nit: You could add an additional test for the error state if serval returns an error.

Code quote:

  describe('getLastPreTranslationBuild', () => {

---

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

        })
      )
      .subscribe(() => this.refreshLastPreTranslationBuild());

refreshLastPreTranslationBuild() has a subscription in the function. It is not a good idea to have subscriptions create subscriptions.
It would be better to just pipe getLastPreTranslationBuild into this recipe with a switchMap.

Code quote:

      .subscribe(() => this.refreshLastPreTranslationBuild());

[josephmyers]

Reviewable status: 6 of 13 files reviewed, 3 unresolved discussions (waiting on @RaymondLuong3)

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

refreshLastPreTranslationBuild() has a subscription in the function. It is not a good idea to have subscriptions create subscriptions.
It would be better to just pipe getLastPreTranslationBuild into this recipe with a switchMap.

Done. Let me know if I matched your thoughts

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

It seems like this could be accomplished in GetLastCompletedPreTranslationBuildAsync() by passing in a parameter like 'completedOnly = false. Then you can update the name of the method to just be GetLastPreTranslationBuildAsync`.

We would still need to endpoint you added.

We could. But I'm not seeing a lot of shared code between these two. Even the GetAllBuildsAsync call is different. So I'm not confident there's a ton of benefit to combining these. Wouldn't it just make things longer and more convoluted?

Another slight downside to combining is that it would increase the processing load for this method, since then it would be iterating over all builds, where now it's filtering them out. Maybe this is trivial?

I'm quite open to your thoughts on these items!

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

You will want to add a test to show that builds that are not completed can also be returned.

Done. I changed the later build to canceled, which I believe satisfies this

[RaymondLuong3]

@RaymondLuong3 reviewed 7 of 7 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @josephmyers)

---

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

Previously, josephmyers wrote…

Done. Let me know if I matched your thoughts

Looks better.

---

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

Previously, josephmyers wrote…

We could. But I'm not seeing a lot of shared code between these two. Even the GetAllBuildsAsync call is different. So I'm not confident there's a ton of benefit to combining these. Wouldn't it just make things longer and more convoluted?

Another slight downside to combining is that it would increase the processing load for this method, since then it would be iterating over all builds, where now it's filtering them out. Maybe this is trivial?

I'm quite open to your thoughts on these items!

No a big deal, just thought it might be worth the effort. I'm OK with it as is. Keeping things separate does have its advantages such as keeping the method simpler.

---

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

        })
      )
      .subscribe();

Instead of using the tap above to set this._latestPreTranslationBuild, you can set it in this subscribe. That makes it explicit we want from this recipe.

Code quote:

      .subscribe();