Page MenuHomePhabricator

Rename WPBSkinBlacklist
Closed, ResolvedPublic2 Estimated Story Points

Description

WikidataPageBanner has a config flag called WPBSkinBlacklist and a method isSkinBlacklisted.

  • Rename WPBSkinBlacklist to WPBSkinDisabled
  • Rename isSkinBlacklisted to isSkinDisabled
  • Update mediawiki-config

QA steps

Check https://en.wikivoyage.beta.wmflabs.org/wiki/Banner in all the different skins and ensure it shows.

QA Results - Beta

QA Results - Prod

Related Objects

StatusSubtypeAssignedTask
OpenNone
Resolvedovasileva

Event Timeline

ovasileva set the point value for this task to 2.Jul 1 2020, 4:41 PM

Change 609141 had a related patch set uploaded (by Peter.ovchyn; owner: Peter.ovchyn):
[mediawiki/extensions/WikidataPageBanner@master] Rename WPBSkinBlacklist to WPBSkinEnabled and invert logic

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/WikidataPageBanner/ /609141

@Jdlrobson I renamed opt name and inverted logic accordingly. Is there any instruction on how to change mediawiki-config ?

@Jdlrobson I renamed opt name and inverted logic accordingly. Is there any instruction on how to change mediawiki-config ?

So with how your patch current is, it's pretty difficult.

In Wikimedia production, we generally support two different versions of MW at once, for most of a week. So when you explicitly rename things, without leaving the old version, it's potentially hard to migrate. Depending on how it's used

For migrationary purposes (of config vars, at least. Though, you could argue there should be the same for function renames as per the deprecation policy), it's easier to add the new one, make it have prescedence in the code, wait for the new code to be deployed everwhere and stable, update medawiki-config, and then remove the old variable at some point later.

However, looking at the usage in mediawiki-config, it's a no-op and the same as default, currently:

'wgWPBSkinBlacklist' => [
	'default' => [],
],

So that should really just be removed from InitialiseSettings.php, as it serves no value.

However, your inverting the logic, which then means we've presumably go to list all the skins. Does that really make sense?

You can contribute patches to the operations/mediawiki-config like any other repo. Deployment just requires being included in Backport windows

I can confirm the config is empty so no need to worry about mediawiki config. Have flagged the same concern about inverting logic on the associated gerrit patch.

@Jdlrobson I think the root of issue is in task description and naming. WPBSkinEnabled and isSkinEnabled is something totally opposite to WPBSkinBlacklist and isSkinBlacklisted. I'd rather offer to rename that to WPBSkinDisabled and isSkinDisabled? What do yo think?

Yes, the issue started here. The task description inverted the logic.

Rename WPBSkinBlacklist to WPBSkinEnabled
Rename isSkinBlacklisted to isSkinEnabled

You should however retain the logic in the patch. That's isSkinDisabled() for the method and WPBSkinDisabled for the config variable.

Change 609490 had a related patch set uploaded (by Peter.ovchyn; owner: Peter.ovchyn):
[operations/mediawiki-config@master] Rename WPBSkinBlacklist to WPBSkinDisabled

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

Change 609141 merged by jenkins-bot:
[mediawiki/extensions/WikidataPageBanner@master] Rename WPBSkinBlacklist and isSkinBlacklisted to WPBSkinDisabled and isSkinDisabled respectively

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

Test Result - Beta

Status: ✅ PASS
Environment: beta
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: NA

Test Artifact(s):

QA steps

Check https://en.wikivoyage.beta.wmflabs.org/wiki/Banner in all the different skins and ensure it shows.

✅ AC1: New Vector

Screen Shot 2020-07-25 at 10.06.09 AM.png (1×1 px, 346 KB)

✅ AC2: Legacy Vector
Screen Shot 2020-07-25 at 10.06.23 AM.png (1×1 px, 506 KB)

✅ AC3: Minerva Neue
Screen Shot 2020-07-25 at 10.06.48 AM.png (1×1 px, 392 KB)

✅ AC4: Modern
Screen Shot 2020-07-25 at 10.07.20 AM.png (1×1 px, 499 KB)

✅ AC5: MonoBook
Screen Shot 2020-07-25 at 10.07.41 AM.png (1×1 px, 560 KB)

✅ AC6: Timeless
Screen Shot 2020-07-25 at 10.08.10 AM.png (1×1 px, 386 KB)

Change 609490 merged by jenkins-bot:
[operations/mediawiki-config@master] Remove WPBSkinBlacklist

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

Mentioned in SAL (#wikimedia-operations) [2020-07-27T18:13:28Z] <catrope@deploy1001> Synchronized wmf-config/InitialiseSettings.php: Remove WPBSkinBlacklist (T254675) (duration: 00m 57s)

Edtadros added a subscriber: Edtadros.

Test Result - Prod

Status: ✅ PASS
Environment: testwiki
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: NA

Test Artifact(s):

QA steps

Check https://test.wikipedia.org/wiki/Banner in all the different skins and ensure it shows.

✅ AC1: New Vector

Screen Shot 2020-08-05 at 8.54.52 AM.png (876×1 px, 340 KB)

✅ AC2: Legacy Vector
Screen Shot 2020-08-05 at 8.55.22 AM.png (876×1 px, 362 KB)

✅ AC3: Minerva Neue
Screen Shot 2020-08-05 at 8.55.53 AM.png (876×1 px, 409 KB)

✅ AC4: Modern
Screen Shot 2020-08-05 at 8.56.07 AM.png (876×1 px, 409 KB)

✅ AC5: MonoBook
Screen Shot 2020-08-05 at 8.58.13 AM.png (876×1 px, 346 KB)

✅ AC6: Timeless
Screen Shot 2020-08-05 at 8.58.34 AM.png (876×1 px, 281 KB)