Repair loss of style with resulting loss of format cog (BL-13953)

[StephenMcConnel]

The repair takes two forms: fix a broken book as best we can, and prevent breakage when changing the page format.

---

This change is 

[StephenMcConnel]

This could possibly be a candidate for cherry-picking to 6.0.

[andrew-polk]

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

---

src/BloomExe/Book/Book.cs line 1821 at r1 (raw file):

            RepairBrokenSmallCoverCredits(OurHtmlDom);
            RepairCoverImageDescriptions(OurHtmlDom);
            RepairMissingStylesAndLangZInTranslationGroups(OurHtmlDom);

I'm pretty torn over the idea of looking for this situation every time we run EnsureUpToDate.

In theory, this is the kind of thing we should be using maintenance level migrations for so we only do it once. Of course, there could, in theory, be other situations which cause this which we haven't fixed....
But I'm not sure that theoretical case warrants this check for every book.

On the other hand, using a whole maintenance level for this fix seems a bit heavy handed.

Thoughts?

Do you have an opinion, @JohnThomson?

---

src/BloomExe/Book/Book.cs line 2307 at r1 (raw file):

                            langZExists = true;
                        var classList = editableDiv.GetAttribute("class").Trim().Split(' ', '\t');
                        var style = classList.FirstOrDefault(x => x.EndsWith("-style"));

HtmlDom has a GetStyle method which looks appropriate here.

---

src/BloomExe/Book/Book.cs line 2313 at r1 (raw file):

                                "class",
                                $"{string.Join(" ", classList)} {styleForRepair}"
                            );

HtmlDom has an AddClass method

[andrew-polk]

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

[JohnThomson]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @andrew-polk and @StephenMcConnel)

---

src/BloomExe/Book/Book.cs line 1821 at r1 (raw file):

Previously, andrew-polk wrote…

I'm pretty torn over the idea of looking for this situation every time we run EnsureUpToDate.

In theory, this is the kind of thing we should be using maintenance level migrations for so we only do it once. Of course, there could, in theory, be other situations which cause this which we haven't fixed....
But I'm not sure that theoretical case warrants this check for every book.

On the other hand, using a whole maintenance level for this fix seems a bit heavy handed.

Thoughts?

Do you have an opinion, @JohnThomson?

I think I would be more inclined to fix this in TranslationGroupManager.MakeElementWithLanguageForOneGroup. That could pretty easily repair an existing editable. If it has to make a new one and the prototype doesn't have a style class, it can repair the new one.
I think the cost of doing it once per editable on a page would be negligible (especially since we already found where it needs doing, so we don't even add a SelectNode), whereas the cost for doing it for a whole book might be nontrivial. And, like this solution, it will be in place if we ever again make a mistake that leaves some languages without a style class.
Of course that would not be a great solution if we actually need to fix this across the whole book (e.g., to publish a broken one).
If it's not feasible I think this is worth a maintenance level. Numbers don't cost anything, and making one more number comparison is a whole lot cheaper than searching the whole book.

---

src/BloomExe/Book/Book.cs line 2320 at r1 (raw file):

                        else
                        {
                            styleForRepair = style; // we expect the style to stay the same inside a translationGroup.

We should hunt for this possibility in a separate loop, unless there is reason to be quite sure that a good block that already has a style will come before any that does not.
Also, check the classes of the TG. Sometimes that has the style, too.

[StephenMcConnel]

Reviewable status: 0 of 5 files reviewed, 4 unresolved discussions (waiting on @andrew-polk and @JohnThomson)

---

src/BloomExe/Book/Book.cs line 1821 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

I think I would be more inclined to fix this in TranslationGroupManager.MakeElementWithLanguageForOneGroup. That could pretty easily repair an existing editable. If it has to make a new one and the prototype doesn't have a style class, it can repair the new one.
I think the cost of doing it once per editable on a page would be negligible (especially since we already found where it needs doing, so we don't even add a SelectNode), whereas the cost for doing it for a whole book might be nontrivial. And, like this solution, it will be in place if we ever again make a mistake that leaves some languages without a style class.
Of course that would not be a great solution if we actually need to fix this across the whole book (e.g., to publish a broken one).
If it's not feasible I think this is worth a maintenance level. Numbers don't cost anything, and making one more number comparison is a whole lot cheaper than searching the whole book.

I've moved the repair code to TranslationGroupManager, and adjusted the unit tests accordingly.

---

src/BloomExe/Book/Book.cs line 2307 at r1 (raw file):

Previously, andrew-polk wrote…

HtmlDom has a GetStyle method which looks appropriate here.

Done.

---

src/BloomExe/Book/Book.cs line 2313 at r1 (raw file):

Previously, andrew-polk wrote…

HtmlDom has an AddClass method

The method has shifted to SafeXmlElement, but I changed to using it.

---

src/BloomExe/Book/Book.cs line 2320 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

We should hunt for this possibility in a separate loop, unless there is reason to be quite sure that a good block that already has a style will come before any that does not.
Also, check the classes of the TG. Sometimes that has the style, too.

Done.

[JohnThomson]

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @andrew-polk)

[JohnThomson]

LGTM. Waiting for Andrew before merging.

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @andrew-polk)

[andrew-polk]

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @JohnThomson)

[andrew-polk]

Dismissed @JohnThomson from a discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @StephenMcConnel)