SF-3360 Allow Restore for chapters with invalid USFM

[josephmyers]

I removed the requirement that a chapter have valid USFM for the history revert feature to work. This allows users to restore revisions to get themselves out of unsupported USFM situations. I also verified that restoring a revision with unsupported USFM properly removes the ability to edit.

---

This change is 

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.92%. Comparing base (7d300ed) to head (51bc4f5).
Report is 1 commits behind head on master.

✅ All tests successful. No failed tests found.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3200   +/-   ##
=======================================
  Coverage   81.91%   81.92%           
=======================================
  Files         604      604           
  Lines       34644    34656   +12     
  Branches     5612     5614    +2     
=======================================
+ Hits        28380    28392   +12     
  Misses       5426     5426           
  Partials      838      838           

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

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[RaymondLuong3]

Thanks for working on this. I can confirm that the restore button is available when the formatting is invalid. However, when I try to restore the version, the invalid error is still visible. I expect this is likely because we are restoring the text but not updating the textInfo for the particular chapter in the project document.

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @josephmyers)

[josephmyers]

Hm, this sounds weird, as I just tested this yesterday. Tell me more about what you're seeing, as you may have tried a use case I missed. Is the text editable? I'm guessing your "invalid error" is the "unsupported USFM warning"? What USFM did you use to cause this?

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

[josephmyers]

I see it now. I am able to get this by restoring a valid "revert" revision, where older valid revisions work fine. I agree, there's something weird about the restoration itself. I'll look into this

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

[josephmyers]

So something is actually rejecting the restore (even though it says it successfully restored). For these revisions, the warning says to fix the error in Paratext. So you Sync to Paratext. Now back in your editor the previous revision is present, and Paratext S/R finds no changes. A fun problem for Monday!

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

[josephmyers]

Got a chance to spend a little more time on this today. Something is quite wrong with the isValid field on some of our snapshots. Many, but not all, of the snapshots I'm seeing from Paratext with invalid markers are considered valid in SF. I see there's been a good bit of flux to the DeltaUsxMapper in the recent months -- that's where I'll start tomorrow.

[josephmyers]

My changes exposed/introduced a bug which puts the project in a bad state if you restore an invalid revision not from Paratext. Be sure to delete any corrupted projects when testing.

[josephmyers]

Technically, you can still see some weird behavior with SF revisions that are nearby PT revisions. Those SF revisions will inherit the validity of the nearest PT revision. I think. Will investigate more tomorrow.

Edit: It's not the nearest. All SF revisions use the latest PT revision when checking validity. So that's a problem

[josephmyers]

Thanks for finding that Raymond. It uncovered a bug in the ParatextService, where we weren't validating the correct data for SF-generated revisions. I've pushed a fix and feel pretty good about it. I also disabled the ability to restore invalid revisions, but we can revert that commit, as I see it as optional (though I haven't tested reverting it).

Reviewable status: 3 of 10 files reviewed, all discussions resolved (waiting on @RaymondLuong3)

[RaymondLuong3]

Great job looking over this and tracking down the problem with detecting if a version is valid. I tried out this branch and things are working as expected. I honestly thing it is much better to not allow reverting a version where the formatting is invalid. There might be a situation where the version is the desired version, even with the invalid formatting, but until someone complains, limiting reverts to valid versions only makes sense.

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

---

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

    public static bool IsDeltaValid(Delta delta, double version = 1)
    {
        var xNodes = ProcessDelta(delta);

Normally we want to be explicit with the type and not user var unless the type is obvious like calling a constructor.

Code quote:

        var xNodes = ProcessDelta(delta);

---

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

        var xNodes = ProcessDelta(delta);
        var usxRootElement = new XElement("usx", xNodes);
        usxRootElement.SetAttributeValue("version", string.Format("{0:0.0}", version));

This looks like the default version will be 1:1.1, but that is not what I would have expected. Would it not be 1.0.0?

Code quote:

string.Format("{0:0.0}", version));

---

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

        {
            // We have the snapshot, but we need to determine if it's valid
            var isValid = DeltaUsxMapper.IsDeltaValid(new Delta(snapshot.Data.Ops), snapshot.Version);

This seems a little strange to me that the snapshot versions is being used to set the usx version. Should the usx version be 1.0.0, and the snapshot version is unrelated?

Code quote:

      var isValid = DeltaUsxMapper.IsDeltaValid(new Delta(snapshot.Data.Ops), snapshot.Version);

[josephmyers]

Yeah, I can get on board with that.

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

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

Normally we want to be explicit with the type and not user var unless the type is obvious like calling a constructor.

Done.

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

This looks like the default version will be 1:1.1, but that is not what I would have expected. Would it not be 1.0.0?

It would be 1.0, which I believe is what we'd want. You can test it out here: https://www.mobzystems.com/online/format-tester/

---

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

Previously, RaymondLuong3 (Raymond Luong) wrote…

This seems a little strange to me that the snapshot versions is being used to set the usx version. Should the usx version be 1.0.0, and the snapshot version is unrelated?

This is a good point. I had thought they maybe were interrelated, but I see that is wrong. I think for the purposes of validity, we can maybe fudge to 1.0 for now?

[RaymondLuong3]

Let's finally get this to testers!

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

---

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

Previously, josephmyers wrote…

It would be 1.0, which I believe is what we'd want. You can test it out here: https://www.mobzystems.com/online/format-tester/

Ok, I was completely off in my understanding of string.Format. Thanks for the helpful link!

---

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

Previously, josephmyers wrote…

This is a good point. I had thought they maybe were interrelated, but I see that is wrong. I think for the purposes of validity, we can maybe fudge to 1.0 for now?

That works for me.