Comments refer to patch set 19 of https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/CentralNotice/+/497611/ (rECNO619cd38294f0)

• bannereditor.js 86: previewDirty is never read; it's only written to. ✔
• bannereditor.js 223-225 and elsewhere: By convention, variables holding jQuery elements begin with $. ✔
• bannereditor.js 224: This DOM lookup only used for initialization here (see point on initialization below). ✔
• bannereditor.js 225: previewPlaceholder can be a private variable outside function scope, so we don't look it up each time. ✔
• bannereditor.js 226: Fragile way of getting banner title; it could be sent specifically to the client code some other way. ✔
• bannereditor.js 227 (and elsewhere): Odd use of this for singleton API access point object? ✔
• bannereditor.js 232-240: Why not put this initialization code with the rest o the initialization stuff at the bottom of the file? ✔
• bannereditor.js 236: use text() message output mode. See https://www.mediawiki.org/wiki/Manual:Messages_API#Output_modes_and_escaping (Ooops! This comment was mistaken. Original code was fine.)
• bannereditor.js 242: Should have a comment explaining that setting disabled activates the barbershop “waiting” animation. ✔
• bannereditor.js 244: Should use the same method for getting banner loader URL as other code. Also, I don't see a dependency for mediawiki.Title. See https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/CentralNotice /+/1caace3c83c775dc8b9ca877a03485aac4bb51d4/resources/subscribing/ext.centralNotice.display.js#158 ✔
• bannereditor.js 244: A comment explaining that the response will call updateBannerPreview() would be quite nice here. ✔
• bannereditor.js 263: (as above) previewPlaceholder can be a private variable that doesn't change, so we don't look it up each time. Also, needs a $, as it's a jQuery element. ✔
• bannereditor.js 270: doPreviewBannerThrottle() is unused. ✔
• bannereditor.js 285: getUnsavedMessagesValues() could be a private function not connected to the public API object. ✔
• bannereditor.js 287 and 291: variables should start with a $, and could be private variables outside function scope, so we don't have to look it up each time. ✔
• SpecialBannerLoader 68-69: check with security about how we're reflecting POSTed content back. Any need for sanitization? Any dangers given the current setup? Also, if this is indeed safe, add a comment about why, and and warning against future changes that could make it unsafe. (spun out as a separate task, T226963)

SpecialBannerLoader 68-69: check with security about how we're reflecting POSTed content back. Any need for sanitization? Any dangers given the current setup? Also, if this is indeed safe, add a comment about why, and and warning against future changes that could make it unsafe.

Added Security tag because of this point. Maybe it'd be easiest just to do a quick call to explain what's going on with this code, to get some general feedback, if possible? Any other channel would also be fine, of course. Thanks much in advance!!

Further comments on patch set 19 of https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/CentralNotice/+/497611/ (rECNO619cd38294f0):

• SpecialBannerLoader 19 and 36: Method param $bannerContent is used once, but later in the same methd $this->bannerContent is used instead. ✔
• SpecialBannerLoader 36: Add the following TODO comments for this method. (As it stands this change is OK, but it complicates already somewhat tangled code.)
  • Method should either get all its info from parameters, or use only instance variables. ✔ (fixed rather than adding TODO)
  • Refactor method to make the determination of whether it's an actual banner display, testing request (on-wiki for a saved version of a banner) or a preview request (live in-editor unsaved preview or on-wiki unsaved preview) elsewhere. Different ways of obtaining banner content should also be elsewhere. ✔ (fixed rather than adding TODO)
• SpecialBannerLoader 208-233:
  • As noted in phpdoc comment, this method should be in the BannerRenderer. Add the string “TODO” to the comment (convention in this codebase). ✔ (fixed rather than adding TODO)
  • We should also use BannerMessage to perform the same removal of tags other than <span> as occurs for normal banner display. (See BannerMessage::toHtml().) Otherwise, the preview might not be the same as the actual banner when displayed to users. ✔
• BannerRenderer.php 118 and 122: For consistency with other variables that determine output, $bannerContent should be sent as a constructor parameter and stored in an instance property. ✔
• BannerRenderer.php 130-149: It'd be prettier to have a separate method to get the base banner content. Skipped, seems unnecessary following refactoring.
• BannerRenderer.php 132: Are we certain MessageCache::singleton()->transform() is the right way to do this? There's no doc on that core method. Check this is right, and perhaps add some doc to core, if so? (I see just a few similar calls in other extensions.) ✔ (added TODO rather than fixing)
• BannerRenderer.php 137-138:
  • Is loading the display module like this ever necessary? ✔
  • Loading RL modules is not be the responsibility of BannerRenderer. ✔

Suggested UI changes (https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/CentralNotice/+/497611/ PS 19 and https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/CentralNotice/+/508720/ PS 13):

• Move and rename update preview button. ✔
• Rename link at the top of the page to preview saved version on-wiki. ✔

Comments on patch set 13 of https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/CentralNotice/+/508720/ (rECNO5de28b87c8e3):

• bannereditor.js 228, 238-242: It's preferable to create these elements and attach handlers with the rest of the initialization code, at the bottom of the file. ✔
• bannereditor.js 291: Shouldn't the banner content be stored in LocalStorage only when this an in-page preview is requested? ✔
• bannereditor.js 222, 260-262, 272-281: Using a callback to trigger the opening of the external preview after fetching the banner works, but it doesn't seem right. Only one caller will ever use only this one callback function. No further callers using different callbacks are forseeable for this code, I think. Instead of a callback parameter, have a boolean parameter that determines whether or not to open the external preview. I think the function could also be renamed to something clearer... maybe something like fetchAndUpdatePreview( openExternalPreview )? ✔
• bannereditor.js 273: Fragile way of getting banner name, repeats code on line 226. ✔
• bannereditor.js 295 and ext.centralNotice.display.js 160: As per convention in this codebase, use all-caps “constants” at the top of each file, and include comments about what the string value must coordinate with. ✔
• ext.centralNotice.display.js 154: fetchBanner() is no longer quite the right name for this method. Let's make a new method, maybe fetchBannerOrRetrieveFromStorage() or something. There we can have the conditional to either retrieve from storage or call fetchBanner(). ✔
• ext.centralNotice.display.state.js 146: This should be in setTestingBannerData() since it's only ever used for a test banner display. ✔
• qqq.json 124: Should say, “Text for link to an in-page preview...”. ✔

I've edited the previous comments to indicate the importance of each issue, as follows:

Needs to be addressed before feature is deployed
Significant tech debt
Moderate tech debt
Minor tech debt

Change 516459 had a related patch set uploaded (by AndyRussG; owner: AndyRussG):
[mediawiki/extensions/CentralNotice@master] [WIP] Banner preview: JS improvements

https://gerrit.wikimedia.org/r/516459

Change 519950 had a related patch set uploaded (by AndyRussG; owner: AndyRussG):
[mediawiki/extensions/CentralNotice@master] Make BannerRenderer link methods static

https://gerrit.wikimedia.org/r/519950

Change 519959 had a related patch set uploaded (by AndyRussG; owner: AndyRussG):
[mediawiki/extensions/CentralNotice@master] Refactor BannerRenderer and SpecialBannerLoader

https://gerrit.wikimedia.org/r/519959

Change 519973 had a related patch set uploaded (by AndyRussG; owner: AndyRussG):
[mediawiki/extensions/CentralNotice@master] Rename link to preview saved version on-wiki

https://gerrit.wikimedia.org/r/519973

Change 519974 had a related patch set uploaded (by AndyRussG; owner: AndyRussG):
[mediawiki/extensions/CentralNotice@master] Rename and move update preview button

https://gerrit.wikimedia.org/r/519974

Patches submitted to fix almost everything. For details, see checkmarks and other notes on each point in the preceding comments.

Of the improvements mentioned above, only the security review remains:

check with security about how we're reflecting POSTed content back. Any need for sanitization? Any dangers given the current setup? Also, if this is indeed safe, add a comment about why, and and warning against future changes that could make it unsafe.

I spun out a separate task for that: {T226963}

Also found of one more follow-up UI improvement; added a separate task for that: T226961: CentralNotice: Don't show live preview section for users without CN rights

Also: T226967: CentralNotice: Error handling for banner preview background calls

@AndyRussG How you would you like this reviewed? I wasn't sure if this is what you and @jgleeson talked about yesterday, but it's a long train of patches and I'm wondering if we should start by +2ing the original patch, or work from the top down.

The changes for this task have been fully +2'ed, but have not merged, since the changes with the initial versions of this feature (https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CentralNotice/+/508720 and https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CentralNotice/+/497611) have not been approved.

As per discussion, those patches should remain unmerged until other follow-on tasks are completed and reviewed.

Thanks so much!!!!

Change 516459 merged by jenkins-bot:
[mediawiki/extensions/CentralNotice@master] Banner preview: JS improvements

https://gerrit.wikimedia.org/r/516459

Change 519950 merged by jenkins-bot:
[mediawiki/extensions/CentralNotice@master] Make BannerRenderer link methods static

https://gerrit.wikimedia.org/r/519950

Change 519959 merged by jenkins-bot:
[mediawiki/extensions/CentralNotice@master] Refactor BannerRenderer and SpecialBannerLoader

https://gerrit.wikimedia.org/r/519959

Change 519973 merged by jenkins-bot:
[mediawiki/extensions/CentralNotice@master] Rename link to preview saved version on-wiki

https://gerrit.wikimedia.org/r/519973

Change 519974 merged by jenkins-bot:
[mediawiki/extensions/CentralNotice@master] Rename and move update preview button

https://gerrit.wikimedia.org/r/519974