Prestranslation versification mismatch

[Enkidu93]

Partial fix for #442

This solution has been verified by a manual test. However, I'm not convinced that this is the best course of action: My first go at a fix was to parse the pretranslation refs as source refs (which is what they should be, right, given that the Refs property yields the source refs as long as there are source refs which should be the case for any verse that's being pretranslated?) and then change the versification to the target when using the target as a template, but that did not work (it just flipped the issue - i.e., fail when template is set to target, works when set to source). I believe that this is because ScriptureRef.Parse() is happy to construct a verse that doesn't actually exist within the project's versification. That is to say, it doesn't care that the project versification is incorrect. However, the ChangeVersification method does care. The solution I'm giving here does solve this issue, but I'm concerned that it will ultimately cause issues elsewhere (even though tests are passing) since if you set template to target, you'll be potentially parsing source versifications as target versifications.

It seems to me that the best solution would be to parse the pretranslation refs as source refs and convert to target versification when using target as a template and let this be an outstanding issue since this is really the fault of the project. Let me know what you all think. I can't think of a good way to implement our own versification conversion that would account for this issue since we're in a situation in which the project versification is incorrect.

---

This change is 

[Enkidu93]

As per our chat in the stand up, I've made the changes we discussed to the code locally and am testing it now manually.

[Enkidu93]

@ddaspit, is this what you had in mind? Just to verify (unless I've made a mistake), this would leave that 65th verse blank for both template as source and as target (judging from manual testing). Is that what you expected?

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.59%. Comparing base (b91a235) to head (22c88d4).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #471   +/-   ##
=======================================
  Coverage   56.59%   56.59%           
=======================================
  Files         275      275           
  Lines       14174    14174           
  Branches     1908     1908           
=======================================
  Hits         8022     8022           
  Misses       5563     5563           
  Partials      589      589           

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

[ddaspit]

This looks correct, but it seems to work on my test. Check out my test and see if we are testing the same thing.

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @johnml1135)

[johnml1135]

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

[Enkidu93]

This looks correct, but it seems to work on my test. Check out my test and see if we are testing the same thing.

Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @johnml1135)

One difference seems to be that in Serval, the ProjectTextUpdater is using the target project. If you use the target and the corresponding parameters to UpdateUsfm, it still fails. I still haven't figured out what the difference is when it's set to source.

[ddaspit]

From my testing, it works with PretranslationUsfmTemplate.Source, but not with PretranslationUsfmTemplate.Target, because of the error in the versification. If you are seeing the same thing, then it is working as expected.

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

[Enkidu93]

From my testing, it works with PretranslationUsfmTemplate.Source, but not with PretranslationUsfmTemplate.Target, because of the error in the versification. If you are seeing the same thing, then it is working as expected.

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

OK, that's good to know. I reran an E2E manual test just to confirm, and no, it is not working for PretranslationUsfmTemplate.Source either. Still investigating. I'm sure I must be missing something silly.

[ddaspit]

This might be a silly question, but are you re-running the build for this E2E test or are you just retrieving the pretranslations from an already completed build? The bug fix impacts preprocessing, so you will need to run a full build, so that the pretranslations have the correct verse ref.

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

[Enkidu93]

This might be a silly question, but are you re-running the build for this E2E test or are you just retrieving the pretranslations from an already completed build? The bug fix impacts preprocessing, so you will need to run a full build, so that the pretranslations have the correct verse ref.

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

No, no silly questions at this point haha. But yes, I am rerunning the whole E2E. It's definitely something on the preprocess side. I keep getting side-tracked from debugging though.

[Enkidu93]

OK! I've figured it out - kind of. I've edited your test, Damien, and it now fails. When using text ids, it works, but when using chapters, it does not. That's why I couldn't figure it out for the life of me! What's happening is that refSUS 1:65 in the source is being mapped to ref DAG 13:3 in the target. When it runs IsInPretranslate, the text id is still SUS regardless so if you just provide text ids, it works properly. But if you use chapters, it looks at DAG. This seems to me to be an error in the AlignRows function itself - it really shouldn't be aligning those verses under any circumstances (maybe SUS 1:65 -> DAG 13:65, but not 13:3). Now, it might just be that the versification being incorrect is causing this, but I do think this merits further investigation. What do you think is the best course of action, @ddaspit? We could put this PR through and open an issue to investigate and maybe add some more testing in general that involves cross-versification alignment and the deuterocanon - the disparity in verse references between SUS 1:65 and DAG 13:3 (when, if anything, it should be 65) is concerning to me.

[ddaspit]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Enkidu93)