SF-3193 Fix additional training data option not visible

[RaymondLuong3]

After the draft generation steps component was converted to not subclass the subscription disposable class, the subscribe method needed to be explicitly called in order for the logic to enable additional training data to be run. I have also added a test which will catch future regressions showing the additional training data option.

---

This change is 

[codecov]

Codecov Report

Attention: Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.

Project coverage is 82.38%. Comparing base (0bf447a) to head (c3ac2d4).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
...neration-steps/draft-generation-steps.component.ts	92.30%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2994      +/-   ##
==========================================
+ Coverage   82.33%   82.38%   +0.04%     
==========================================
  Files         543      543              
  Lines       31663    31663              
  Branches     5147     5125      -22     
==========================================
+ Hits        26069    26084      +15     
+ Misses       4840     4823      -17     
- Partials      754      756       +2     

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

[Nateowami]

Reviewable status: 0 of 2 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.ts line 229 at r1 (raw file):

        filterNullish(),
        map(async projectDoc => {
          this.projectChanged = true;

Why can't we use a utility like distinctUntilChanged

[RaymondLuong3]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @Nateowami)

---

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

Previously, Nateowami wrote…

Why can't we use a utility like distinctUntilChanged

I'm not sure how that would help in this situation. The trainingDataQuery will emit based on any updates to the query. We just have to be sure that we don't reset the initial training data files unless the user changes projects. (This might never happen since the user would have to paste a URL into the browser.)

[siltomato]

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @marksvc, @Nateowami, and @RaymondLuong3)

---

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

        takeUntilDestroyed(this.destroyRef),
        filterNullish(),
        map(async projectDoc => {

Would switchMap be more appropriate here?

[siltomato]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @marksvc, @Nateowami, and @RaymondLuong3)

---

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

        if (this.projectChanged) {
          // Set the selection based on previous builds
          this.projectChanged = false;

This took me a minute to understand. Maybe renaming the flag to something like isTrainingDataInitialized or isProjectTrainingDataInitialized (and inverting it) would read better?

if (!this.isTrainingDataInitialized) {
    this.isTrainingDataInitialized = true;

    // Set the selection based on previous builds
    this.setInitialTrainingDataFiles(this.availableTrainingFiles.map(d => d.dataId));
}

Code quote:

this.projectChanged = false;

[marksvc]

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @Nateowami, @RaymondLuong3, and @siltomato)

---

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

Previously, siltomato wrote…

This took me a minute to understand. Maybe renaming the flag to something like isTrainingDataInitialized or isProjectTrainingDataInitialized (and inverting it) would read better?

if (!this.isTrainingDataInitialized) {
    this.isTrainingDataInitialized = true;

    // Set the selection based on previous builds
    this.setInitialTrainingDataFiles(this.availableTrainingFiles.map(d => d.dataId));
}

Yeah, perhaps
isProjectTrainingDataInitialized or
isInitialTrainingDataSetForProject.

---

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

        })
      )
      .subscribe(() => {

❓ I don't understand why it's okay now but wasn't before, since we are still using .subscribe( and takeUndilDestroyed(this.destroyRef). Was the this.trainingDataSub...subscribe( not calling something that was needed/adequate but the this.activatedProjectprojectDoc$...map(...().subscribe( is calling what is needed/adequate??

---

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

      .subscribe(() => {
        this.availableTrainingFiles = [];
        if (this.activatedProject.projectDoc?.data?.translateConfig.draftConfig.additionalTrainingData) {

❓ Why the change to use this.activatedProject? Is there a reason we want to be sure we are operating on the activatedProject rather than the next projectDoc coming down the line from this.activatedProject.projectDoc$? It seems like there could in principle be a mismatch between the local projectDoc and the this.activatedProject.projectDoc.

[RaymondLuong3]

Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @marksvc and @siltomato)

---

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

Previously, siltomato wrote…

Would switchMap be more appropriate here?

Thanks, that should be the right way to go

---

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

Previously, marksvc wrote…

❓ I don't understand why it's okay now but wasn't before, since we are still using .subscribe( and takeUndilDestroyed(this.destroyRef). Was the this.trainingDataSub...subscribe( not calling something that was needed/adequate but the this.activatedProjectprojectDoc$...map(...().subscribe( is calling what is needed/adequate??

The issue was that before there were two different observables, and the subscription to the merge(this.trainingDataQuery.localChanges$, ...) was active, but the observable for the projectDoc$ was not subscribed to. Since it was not subscribed, all the logic is never run. So in this change, I updated it to only be 1 observable to be subscribed to.

---

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

Previously, marksvc wrote…

❓ Why the change to use this.activatedProject? Is there a reason we want to be sure we are operating on the activatedProject rather than the next projectDoc coming down the line from this.activatedProject.projectDoc$? It seems like there could in principle be a mismatch between the local projectDoc and the this.activatedProject.projectDoc.

There will not be a mismatch since we can have confidence that the projectId and the projectDoc will be for the same project. The main reasoning for the change is because we are doing a switchMap transformation of the observable, and preserving the projectDoc just for the id was not necessary

---

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

Previously, marksvc wrote…

Yeah, perhaps
isProjectTrainingDataInitialized or
isInitialTrainingDataSetForProject.

Done.

[siltomato]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @marksvc)

[siltomato]

Reviewed 1 of 2 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @marksvc)

[marksvc]

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

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

The issue was that before there were two different observables, and the subscription to the merge(this.trainingDataQuery.localChanges$, ...) was active, but the observable for the projectDoc$ was not subscribed to. Since it was not subscribed, all the logic is never run. So in this change, I updated it to only be 1 observable to be subscribed to.

Wow. Thank you.

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

There will not be a mismatch since we can have confidence that the projectId and the projectDoc will be for the same project. The main reasoning for the change is because we are doing a switchMap transformation of the observable, and preserving the projectDoc just for the id was not necessary

Right, there is no local projectDoc. Thank you.