Allow branding to be preserved in download-for-edit (BL-13110)

[JohnThomson]

This change is 

[andrew-polk]

Reviewed 11 of 11 files at r1, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @JohnThomson)

---

src/BloomBrowserUI/publish/LibraryPublish/uploadCollisionDlg.tsx line 146 at r1 (raw file):

"Thi is

typo

And I think this needs a new entry in the .xlf?

---

src/BloomExe/Collection/CollectionSettings.cs line 391 at r1 (raw file):

        /// <summary>
        /// Get the branding that the settings file specifies, without checking the subscription code
        /// as we would do if creating the object from the settings file.

I think it would be helpful to expand this comment.
Why do we need to get around the check?
Under what circumstances is that ok?

---

src/BloomExe/Collection/CollectionSettings.cs line 698 at r1 (raw file):

        private string _overrideBrandingForEditDownload;

        public void SetCurrentBook(Book.Book book)

This feels... wrong.
I'm sure you've thought about alternatives... I don't have any myself...
But is there some way to do this without the CollectionSettings class having to have a specific book sent to it?

---

src/BloomExe/Collection/CollectionSettings.cs line 728 at r1 (raw file):

                // variable because we don't have a code...then we'll do the override.
                if (branding == LoadBranding(SettingsFilePath))
                    _overrideBrandingForEditDownload = branding;

I don't understand what is happening here.
The logic feels backwards.
If the branding in the collection settings file matches the branding in the book file, we override.

I know you attempted to explain it in the comment, but I'm not getting it. For one thing, I don't know what "reset the branding" means.

---

src/BloomExe/Collection/CollectionSettings.cs line 947 at r1 (raw file):

        private string _collectionName;
        private string _settingsFilePath;
        private string _country;

What is the purpose of these unused private members?

---

src/BloomExe/Workspace/WorkspaceView.cs line 349 at r1 (raw file):

            // for the first time. But don't bring up the dialog about the problem in the middle of
            // selecting the book.
            Application.Idle += CheckForInvalidBrandingOnIdle;

Sorry, this comment is not sufficient for my brain to process what is going on.

First question: Do we call HandleBookSelectionChanged when selecting the first book? We do need to call CheckForInvalidBranding on the first book, right?

I get the "don't bring up the dialog... in the middle..." part. That's why we do it on idle after selection changed.
But I don't understand the first sentence about selecting a not "download for editing" book for the first time.
I think part of what is throwing me off is the "possibly". Isn't it likely?
But also I'm not sure why information about download for editing is even pertinent here.

---

src/BloomExe/Workspace/WorkspaceView.cs line 960 at r1 (raw file):

                        _collectionSettings.InvalidBranding,
                        _collectionSettings.SubscriptionCode
                    );

It feels odd to call an API method here.
Should it move to CollectionSettings or CollectionModel?

Not worth any heroics.

---

src/BloomExe/web/controllers/CollectionSettingsApi.cs line 651 at r1 (raw file):

        }

        public static void SetupLegacyBrandingForSettingsDiaog(

typo in "Diaog".

Also, I would make it SetUp, not Setup

[JohnThomson]

Reviewable status: 8 of 11 files reviewed, 7 unresolved discussions (waiting on @andrew-polk and @hatton)

---

src/BloomBrowserUI/publish/LibraryPublish/uploadCollisionDlg.tsx line 146 at r1 (raw file):

Previously, andrew-polk wrote…

"Thi is

typo

And I think this needs a new entry in the .xlf?

The new string is in the XLF. This is just a comment that I shortened from something Copilot made. On further thought, I don't think this string needs a comment.

---

src/BloomExe/Collection/CollectionSettings.cs line 391 at r1 (raw file):

Previously, andrew-polk wrote…

I think it would be helpful to expand this comment.
Why do we need to get around the check?
Under what circumstances is that ok?

Done.

---

src/BloomExe/Collection/CollectionSettings.cs line 698 at r1 (raw file):

Previously, andrew-polk wrote…

This feels... wrong.
I'm sure you've thought about alternatives... I don't have any myself...
But is there some way to do this without the CollectionSettings class having to have a specific book sent to it?

Yes, it does feel a bit like just the sort of "add a back channel hack to make it work" that @hatton especially hates. And maybe it is. There are 44 places in our code that expect to get the current branding from the collection settings, and I may be being lazy about fixing all of them. But it's not obvious that there is a better answer. Is it a book responsibility or a collection responsibility to know that this is the one book in the collection that is allowed to use a particular branding without a code? If we're going to make that exception, there has to be an exception to normal practice somewhere. To this point, we've said that branding and codes are the responsibility of the collection. The file that records the information about one book being special is also at the collection level. I'm not convinced it would be an improvement to make everything that wants to know the branding go to a book, or pass a book as an argument. I can try and see what it looks like, but there will be a whole lot more changed files, and maybe other temptations to awkward back channels.
I think we agreed in standup to see how the PR cleans up without that change, and what JohnH thinks both about this and the other questions in the card, before attempting that.

---

src/BloomExe/Collection/CollectionSettings.cs line 728 at r1 (raw file):

Previously, andrew-polk wrote…

I don't understand what is happening here.
The logic feels backwards.
If the branding in the collection settings file matches the branding in the book file, we override.

I know you attempted to explain it in the comment, but I'm not getting it. For one thing, I don't know what "reset the branding" means.

I've made several changes I think should help.

---

src/BloomExe/Collection/CollectionSettings.cs line 947 at r1 (raw file):

Previously, andrew-polk wrote…

What is the purpose of these unused private members?

None. They snuck in when something converted a bunch of properties to have backing fields, and I missed them when reverting that.

---

src/BloomExe/Workspace/WorkspaceView.cs line 349 at r1 (raw file):

Previously, andrew-polk wrote…

Sorry, this comment is not sufficient for my brain to process what is going on.

First question: Do we call HandleBookSelectionChanged when selecting the first book? We do need to call CheckForInvalidBranding on the first book, right?

I get the "don't bring up the dialog... in the middle..." part. That's why we do it on idle after selection changed.
But I don't understand the first sentence about selecting a not "download for editing" book for the first time.
I think part of what is throwing me off is the "possibly". Isn't it likely?
But also I'm not sure why information about download for editing is even pertinent here.

Done.

---

src/BloomExe/Workspace/WorkspaceView.cs line 960 at r1 (raw file):

Previously, andrew-polk wrote…

It feels odd to call an API method here.
Should it move to CollectionSettings or CollectionModel?

Not worth any heroics.

Not sure I see the problem. We're about to launch the settings dialog, and we need to pass some information that dialog will need to the API that it calls to initialize itself.

---

src/BloomExe/web/controllers/CollectionSettingsApi.cs line 651 at r1 (raw file):

Previously, andrew-polk wrote…

typo in "Diaog".

Also, I would make it SetUp, not Setup

Done

[andrew-polk]

Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @hatton and @JohnThomson)

---

src/BloomExe/Collection/CollectionSettings.cs line 728 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

I've made several changes I think should help.

Thanks.

---

src/BloomExe/Collection/CollectionSettings.cs line 947 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

None. They snuck in when something converted a bunch of properties to have backing fields, and I missed them when reverting that.

In that case, there are quite a few more which got converted but not backed out.
e.g. SubscriptionCode, BooksOnWebGoal

---

src/BloomExe/Workspace/WorkspaceView.cs line 349 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Done.

Thanks.

---

src/BloomExe/Workspace/WorkspaceView.cs line 960 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Not sure I see the problem. We're about to launch the settings dialog, and we need to pass some information that dialog will need to the API that it calls to initialize itself.

Ok, not sure what I was thinking. Maybe I wasn't realizing this was a UI class.

[andrew-polk]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @hatton and @JohnThomson)

---

src/BloomBrowserUI/publish/LibraryPublish/uploadCollisionDlg.tsx line 146 at r1 (raw file):

Previously, JohnThomson (John Thomson) wrote…

The new string is in the XLF. This is just a comment that I shortened from something Copilot made. On further thought, I don't think this string needs a comment.

Sorry, I thought it was in the xlf, then when I went to check again, I wasn't seeing it. I'm seeing it now.

But the comment with typo is still here in the code.

[JohnThomson]

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

---

src/BloomBrowserUI/publish/LibraryPublish/uploadCollisionDlg.tsx line 146 at r1 (raw file):

Previously, andrew-polk wrote…

Sorry, I thought it was in the xlf, then when I went to check again, I wasn't seeing it. I'm seeing it now.

But the comment with typo is still here in the code.

Done.

---

src/BloomExe/Collection/CollectionSettings.cs line 947 at r1 (raw file):

Previously, andrew-polk wrote…

In that case, there are quite a few more which got converted but not backed out.
e.g. SubscriptionCode, BooksOnWebGoal

Done.

[andrew-polk]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @hatton)