SF-3141 Fix selected reference books persisting between steps

[kylebuss]

This PR fixes the resource books selected for training not persisting between steps. It adds a variable to store the books that will be used for training.

---

This change is 

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.89%. Comparing base (c2d46c7) to head (99a7fa9).
Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2927   +/-   ##
=======================================
  Coverage   80.89%   80.89%           
=======================================
  Files         533      533           
  Lines       31221    31222    +1     
  Branches     5066     5088   +22     
=======================================
+ Hits        25255    25257    +2     
+ Misses       5215     5203   -12     
- Partials      751      762   +11     

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

[Nateowami]

This is better, but it still only properly handles a single reference project. If I select a second reference project, it has the same problem as before.

[kylebuss]

I've confirmed that without fast translating enabled the correct books for both source and additional source are sent to Serval for training without this fix.

Unless users are enabling fast translating we are safe to not fix this now and address it with Joseph's PR for the additional drafting steps.

[Nateowami]

@kylebuss What if the user goes back to a previous step? Is it really about "enabling fast translating", or is it about going to a different step before generating the draft? Users can go back to previous steps.

[kylebuss]

Good point. I've made the additional training source selected books persistent through changing steps.

[josephmyers]

Thanks for picking this up. Please add a unit test to verify the properties in question after the step changes.

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

---

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

  userSelectedAdditionalSourceTrainingBooks: number[] = [];

  refBooksSelectedTrainingBooks: number[] = [];

What is this variable name supposed to mean? Can we make this any better/clearer?

---

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

      newSelectedTrainingBooks.includes(b)
    );
    this.userSelectedAdditionalSourceTrainingBooks = this.selectableAdditionalSourceTrainingBooks.filter(b =>

It's not really your doing, but this class is turning into a massive state-driven mess. Why are we not wanting to update this field now? This is a red flag, from a maintainability standpoint. If we can't update a "training books" variable in the "update training books" method, we've got a problem.

States are not bad in small number, but this file has no less than 32 mindless state variables to maintain (L 60-102). Yikes. Any way we can reduce this will also reduce future bugs.

The root bug here is that the "update training books" method is overwriting the user-selected training books. (The bug has nothing to do with the additional source books, near as I can tell.) This method is called every time the step changes.

From a design perspective, why is the user selection being overwritten whenever the step changes? This is the real problem. The only time we should blow away a user's choice is if it's no longer valid, e.g. if the user goes back and removes a book selected in a future step. Before, it was sufficient for us to simply reset everything on step change; now, that's obviously not good enough. We could be smarter about when we overwrite the user selection, e.g. only when a selected book is invalidated.

Wouldn't it make more sense to only overwrite the user selection initially (done in setInitialTrainingBooks) and when a preceding selection invalidates a succeeding one?

To get away from all these states would be a little more work, and I don't want to put that on you. But let's try to fix the states we have and avoid adding in more, if possible.

[kylebuss]

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @josephmyers)

---

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

Previously, josephmyers wrote…

What is this variable name supposed to mean? Can we make this any better/clearer?

Done. This was removed after reviewing your other comments.

---

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

The root bug here is that the "update training books" method is overwriting the user-selected training books. (The bug has nothing to do with the additional source books, near as I can tell.) This method is called every time the step changes.

Both the user selected source and user selected alternate source were being overwritten in the method. The state I added was doing the same thing that could be done with userSelectedSourceTrainingBooks so I've removed my initial changes and updated the method to correctly update source and additional source books.

[josephmyers]

Thanks for fixing this up. I do think it'd be useful to have a unit test for this, but in interest of time, I'll approve this. (I'm also reworking some things in this component in my final step branch, so the test would likely change anyway)

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @kylebuss)

---

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

Previously, kylebuss (Kyle Buss) wrote…

The root bug here is that the "update training books" method is overwriting the user-selected training books. (The bug has nothing to do with the additional source books, near as I can tell.) This method is called every time the step changes.

Both the user selected source and user selected alternate source were being overwritten in the method. The state I added was doing the same thing that could be done with userSelectedSourceTrainingBooks so I've removed my initial changes and updated the method to correctly update source and additional source books.

Fantastic. Thanks for the second look.