SF-3488 Force refresh when draft config changes remotely

[josephmyers]

I also added an optional parameter to the dialog service to disallow closing by clicking outside the dialog, since for this case, we want them to explicitly click the button.

---

This change is 

[codecov]

Codecov Report

❌ Patch coverage is 50.00000% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.20%. Comparing base (0803616) to head (8bdd4e1).
⚠️ Report is 1 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...ture/ClientApp/src/xforge-common/dialog.service.ts	33.33%	7 Missing and 1 partial ⚠️
...neration-steps/draft-generation-steps.component.ts	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3381      +/-   ##
==========================================
- Coverage   82.21%   82.20%   -0.01%     
==========================================
  Files         609      609              
  Lines       35901    35911      +10     
  Branches     5864     5846      -18     
==========================================
+ Hits        29515    29521       +6     
- Misses       5508     5528      +20     
+ Partials      878      862      -16     

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

[RaymondLuong3]

Nice work! I like that this dialog will help to fix any data inconsistency when a user is in the process of generating a draft. A few comments below.

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

---

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

            source: { projectRef: 'sourceProject', shortName: 'sP', writingSystem: { tag: 'xyz' } }
          },
          writingSystem: { tag: 'eng' }

In the draft sources above, the source language is eng and the target project's language is xyz. We should make this consistent.

Code quote:

          translateConfig: {
            source: { projectRef: 'sourceProject', shortName: 'sP', writingSystem: { tag: 'xyz' } }
          },
          writingSystem: { tag: 'eng' }

---

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

        async ([{ trainingTargets, trainingSources, draftingSources }, projectId]) => {
          // Force refresh on remote changes
          if (this.draftingSources.length > 0 || this.trainingSources.length > 0 || this.trainingTargets.length > 0) {

You could probably just check if trainingTargets is non-empty, but the way you have it should be more robust.

Code quote:

          if (this.draftingSources.length > 0 || this.trainingSources.length > 0 || this.trainingTargets.length > 0) {

---

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

              true
            );
            this.locationService.reload();

Is it necessary to reload the page? I think we should be able to just close the stepper without reloading.

Code quote:

            this.locationService.reload();

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/dialog.service.ts line 98 at r1 (raw file):

    message: I18nKey | Observable<string>,
    close?: I18nKey | Observable<string>,
    disableClose: boolean = false

Add the disableClose parameter to the method doc.

Code quote:

    disableClose: boolean = false

---

src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 249 at r1 (raw file):

    "overview": "Overview",
    "reference_books": "Reference books",
    "remote_changes": "The draft generation settings for this project have been modified, either by another user or in a separate window. Please review the latest configuration and try starting the draft again.",

In this case the sources have been modified. We probably want to be explicit that the sources have changed. Maybe "The source configuration for draft generation have been modified. Please start a new draft and review the latest source configuration." The advantage here is that the "start over" button is more in line with the message. It also directs the user to click "new draft" which is where they will be able to see the current configuration.

Code quote:

The draft generation settings for this project have been modified,

[josephmyers]

Reviewable status: 1 of 4 files reviewed, 4 unresolved discussions (waiting on @RaymondLuong3)

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

Is it necessary to reload the page? I think we should be able to just close the stepper without reloading.

Done. That's a good point, we could just emit cancel, which would also force them to start over.

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

In the draft sources above, the source language is eng and the target project's language is xyz. We should make this consistent.

Done.

---

src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 249 at r1 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

In this case the sources have been modified. We probably want to be explicit that the sources have changed. Maybe "The source configuration for draft generation have been modified. Please start a new draft and review the latest source configuration." The advantage here is that the "start over" button is more in line with the message. It also directs the user to click "new draft" which is where they will be able to see the current configuration.

I'm fine with whatever wording. Looks like JoEllen likes your proposed change, as well. In terms of functionality, are you saying we should show this message if either the drafting source or the training sources are now different, and in all other cases show the original message?

---

src/SIL.XForge.Scripture/ClientApp/src/xforge-common/dialog.service.ts line 98 at r1 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

Add the disableClose parameter to the method doc.

Done.

[RaymondLuong3]

@RaymondLuong3 reviewed 2 of 3 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @josephmyers)

---

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

Previously, josephmyers wrote…

Done. That's a good point, we could just emit cancel, which would also force them to start over.

Perfect. That will do the job.

---

src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 249 at r1 (raw file):

Previously, josephmyers wrote…

I'm fine with whatever wording. Looks like JoEllen likes your proposed change, as well. In terms of functionality, are you saying we should show this message if either the drafting source or the training sources are now different, and in all other cases show the original message?

No, what I meant was that the only scenario where we would see getDraftProjectSources() emit is if the sources changed in some way. I just meant it would be best to make that explicit when showing the dialog message here. The dialog will not appear in any other situation.

[josephmyers]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @josephmyers)

---

src/SIL.XForge.Scripture/ClientApp/src/assets/i18n/non_checking_en.json line 249 at r1 (raw file):

Previously, RaymondLuong3 (Raymond Luong) wrote…

No, what I meant was that the only scenario where we would see getDraftProjectSources() emit is if the sources changed in some way. I just meant it would be best to make that explicit when showing the dialog message here. The dialog will not appear in any other situation.

Done. Glad I asked!

[RaymondLuong3]

@RaymondLuong3 reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @josephmyers)