Page MenuHomePhabricator

Code review, fix major and minor issues, follow up on new requirements, write tests and release management for CN campaign filtering
Closed, ResolvedPublic4 Estimated Story Points

Description

User preferences UI mockups:
https://docs.google.com/presentation/d/1Ocli1ozAlyHPL8YBd2nb5_5fevLXYaFbTLmiC-YZCio/edit#slide=id.gb188dacd48_0_839

I hear there are some patches backing up behind some of the contractor work. The campaign filter features definitely needs to go out but clone campaign could be delayed if needed.

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CentralNotice/+/604279/
https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CentralNotice/+/607461/

DoD:

  • Review campaign filter feature
  • Get patches and feature ready to release in early January.

Based on my and @AndyRussG's review of Campaign Filtering, here's what need to be done:

  • additional review as it's been cursory so far
  • split out schema into separate patch for deployment
  • relabel fields in CampaignType manager UI "label message" and "force selection"
  • rename "Campaign displaying" section header added in user preferences
  • add link to CampaignType Manager in Campaign Manager--there is a link in the upper right corner of the screen
  • rename db field not_campaign_type to be more clear and consistent ("not_type")
  • fix tests broken by the first patch
  • fix tests broken by the second patch
  • fix styling issues brought up in latest comments from DannyS on campaign filter (would be mostly resolved ✱)
  • add tests -- we might want more in AllocationTests but for now there's more test coverage
  • ask for input from core team about changes to user preferences UI
  • smoke test some on the Beta Cluster
  • fix issue: in logs, changes to campaign type results in a blank detailed description of the change
  • improve functionality around translatable names for campaign types (see these four comments)✱
  • use null instead of magic number for no campaign type assigned to a campaign
  • fix use of magic number in CampaignType object to indicate the type is not yet saved to the DB Code removed ✱
  • use divs instead of table for add/edit campaign type form layout (optional) Code removed ✱
  • use OOUI for add/edit campaign type form (optional) Code removed ✱
  • numerous miscelaneous minor code issues (variable and method names, whitespace issues, type hints, inline doc, default parameter values, etc.) (would be partly resolved ✱)
  • validate and catch duplicate campaign type names (currently you get an unfriendly DB error if you create a new type with the same name) Code removed✱
  • don't add an extra DB query to every single pageview that reaches PHP (which includes every pageview for logged-in users) Code removed ✱
  • allow control over ordering of campaign types in user preferences and CN Admin UI (optional)✱

✱ These issues/tasks would be resolved (or partly resolved, where indicated) by following the simplified config-based approach mentioned here.

  • convert to simplified config-based approach

Original RFP/spec that was sent to the contractor:
https://docs.google.com/document/d/1HDODIse0zhWAnUtu5Hmd7did6zofYLN9dwAew55g4RA/edit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Change 647340 had a related patch set uploaded (by Mepps; owner: Mepps):
[mediawiki/extensions/CentralNotice@master] WIP UI Improvements for campaign filtering

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

Change 647340 abandoned by Mepps:
[mediawiki/extensions/CentralNotice@master] WIP UI Improvements for campaign filtering

Reason:
Squashed into earlier commit

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

Change 647349 had a related patch set uploaded (by Mepps; owner: Mepps):
[mediawiki/extensions/CentralNotice@master] Schema updates for campaign filtering

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

Change 647734 had a related patch set uploaded (by Mepps; owner: Mepps):
[mediawiki/extensions/CentralNotice@wmf_deploy] Schema updates for campaign filtering

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

Change 647734 abandoned by Mepps:
[mediawiki/extensions/CentralNotice@wmf_deploy] Schema updates for campaign filtering

Reason:
Local branch mistake

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

mepps renamed this task from Code review and release management for CN to Code review and release management for CN campaign filtering.Dec 14 2020, 4:20 PM
mepps updated the task description. (Show Details)

Change 649420 had a related patch set uploaded (by Mepps; owner: Mepps):
[mediawiki/extensions/CentralNotice@master] Add unit tests for CampaignType

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

@AndyRussG and @mepps The original customer for this is @Seddon and he can help with naming the controls on the preferences page. Could you provide screenshots of what the controls look like on that page?

Screen Shot 2020-12-14 at 1.01.37 PM.png (718×1 px, 81 KB)

Here's the Campaign type manager interface. I updated some of the labels but on second thought the one for "label message" is now too long. I'd love any thoughts from @Seddon!

And here's the user preferences.

Screen Shot 2020-12-14 at 1.04.54 PM.png (188×688 px, 16 KB)

Ah @mepps I think I understand. could you add the categories outlined here:
https://docs.google.com/document/d/1HDODIse0zhWAnUtu5Hmd7did6zofYLN9dwAew55g4RA/edit

They should be:

  • Maintenance
  • Governance
  • Advocacy
  • Event
  • Article Writing
  • Photography
  • Fundraising
  • Special

All of these should show up in user preferences except: Special and Maintenance

@DStrine the campaign types are configurable so the CN admins will add those. This was just test data on my local machine.

Special and Maintenance will be set to "On for all users" or "Force on".

From @Seddon ForceOn -> Display to all users.
Label message -> Translation id.

  • Does label message validate?
  • Check permissions/role for accessing campaign type editing.

Also, I'm trying to understand the required functionality for "Label message key for wikitext".

Currently, it acts as the key for an i18n message that, when set, provides a translatable text used for the name of the campaign type in the User Preferences UI. I guess something like this is needed so that the campaign types will be translatable and show up in the user's language.

However, this isn't mentioned in the spec. I don't remember it being discussed, but it might have been, and I just don't remember...

If we do want it to be a UI message, we might want to always add a cn-specific prefix and set the extra editing protections currently used with other CN-related messages, I guess?

Also, the explanation for the field in qqq.json is kinda cryptic: "Field label in Campaign type form for label message for the type".

(Note that the last patch set from the contractor was parsing this as wikitext. Not sure why. That was causing Phan errors, which is why it was changed in subsequent versions.)

Continuing on the same topic as above... maybe if that is the desired functionality, some further changes are needed?

For example, in that case, I don't see the use of having an untranslatable backup "name" for the type. What about, instead, just having a single name for the campaign type, but with format restrictions such as no spaces or unusual punctuation, and automatically making that the basis for an automatically-prefixed i18n message? (This is more similar to how CN currently works with i18n messages, such as translatable messages in banners.)

Then, in the User Preferences UI, if a translation doesn't exist for the UI language, we might show the untranslated message name with the funny angle brackets? Or make it fall back to English, or use a different fallback method? This would prevent there from being multiple base names for the type (as we have in the current setup, with the campaign type name and the initial English i18n message content stored in different places and easily having different values).

Maybe the campaign type management UI could automatically create the initial Mediawiki: namespace page, offering an easy way to set the initial content? Or maybe this isn't needed, since the types won't change very often?

Also, what about links to the translate UI on Special:CampaignTypes (the campaign types management page). And how will translators be alerted to the need to translate these messages? Do we need message groups?

Thanks!!!

Just a further note on the campaign name/translatable message name topic: in the current implementation, in user preferences, we show the translated message, while in the Admin UI, in places where you assign a type to a campaign, we see the type name. So, the text for the type shown to users for opting in or out comes from a different source (and could end up being slightly different, or potentially even very different) from the text used by CN admins to assign types to campaigns.

Ah @mepps I think I understand. could you add the categories outlined here:
https://docs.google.com/document/d/1HDODIse0zhWAnUtu5Hmd7did6zofYLN9dwAew55g4RA/edit

They should be:

  • Maintenance
  • Governance
  • Advocacy
  • Event
  • Article Writing
  • Photography
  • Fundraising
  • Special

All of these should show up in user preferences except: Special and Maintenance

@DStrine, @Seddon how likely are these to ever change? Or, how often and how much do you think they might change?

Further note on translatable campaign types: Currently the Translate extension wouldn't be available to manage these, since the messages have to be available on all wikis for user preferences, and that extension isn't installed on Wikipedias. So, I think someone would have to get translators to update the Mediawiki: namespace pages one by one for all the wikis?

Of the issues and tasks just added in the description here, one seems pretty significant: the feature as currently implemented will cause an additional DB query on every pageview that reaches PHP, which includes all pageviews for logged-in users. I'm pretty sure that is not OK.

While I guess that is solvable (by using object caching) at this point what I think I'd recommend is, instead, simplifying and removing stuff: assuming that the campaign types won't change often, we could just store them in a configuration variable instead of the database. The variable could have the initial required types as defaults, and we could include the i81n messages in the code itself.

This would solve a bunch of issues just by getting rid of the offending code and the UI bits. It would also speed the availability of translated campaign type names, since i18n messages included in the code are normally translated pretty quickly. Otherwise (as mentioned above) I think we'd be looking at reaching out directly to translators and setting the localized messages for campaign types one by one on every wiki (since the Translate extension, which we use on Meta for in-banner translatable messages, is not installed on all Wikipedias).

The main drawback to a config-variable approach is that a code deploy would be needed to update the campaign types. But I think it's the quickest route to getting a usable, clean MVP.

In any case, I guess we probably need a meeting to hash all this out? Also, I definitely could be missing stuff!!!!

Thanks!!!

@AndyRussG That simplification for first release sounds totally acceptable!

@mepps and I were just talking a bit in IRC and think that we could leave the new notices.not_type column an integer (so if in the future we do add types to the DB, we don't have to alter the notices table again). Then the config var could be something like
[ 1 => 'maintenance', 2 => 'governance' ] and the i18n keys like centralnotice-category-maintenance, centralnotice-category-governance, etc.

Finished reading through all the code (including the config-based version uploaded earlier today by Maggie and Elliott). Currently working on a few minor improvements. As discussed with Elliott on IRC this evening, instead of commenting in Gerrit, I'll just upload some new patch sets. Hope that works for everyone!! One noteworthy change that I'll make, also discussed on IRC, is to use string keys instead of numerical ones for campaign type. This will ensure the category of historical campaigns doesn't change if the config changes. Also, PHP has a weird thing about changing string keys that can be interpreted as integers to integers, without asking. (See https://stackoverflow.com/questions/4100488/a-numeric-string-as-array-key-in-php.) Even with the string keys, we'll still be able to switch from config-set types to DB-stored ones in the future, if we wish.

Change 604279 had a related patch set uploaded (by AndyRussG; owner: AlexPinchuk):
[mediawiki/extensions/CentralNotice@master] Campaign filtering

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

Change 607461 had a related patch set uploaded (by AndyRussG; owner: ItSpiderman):
[mediawiki/extensions/CentralNotice@master] Control displaying campaigns based on user preferences

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

Here's an overview of some of the changes in these patches since a couple days ago:

  • General: reworked code to use string identifiers for campaign types and to use null in the DB for no campaign assigned.
  • Schema: made campaign type fields varchar instead of int.
  • Schema: made default null for campaign type in campaign (notice) table and campaign (notice) log table.
  • Improved "no campaign" text in en.json.
  • Fixed multiple issues in qqq.json.
  • Rolled unrelated formatting improvements into a different change.
  • Removed newline at end of file added to unrelated patch file.
  • Removed label and name fields from CampaignType, and made it create message key and preferences key on-the-fly.
  • Moved decision to show message text or name out of CampaignType and into preferences hook because it's more front-endy.
  • Used a constant for prefix for i18n key.
  • Used a constant for prefix for preference key.
  • Removed CampaignTypeManager, since some methods were unused, and it was simpler to put remaining functionality in static methods in CampaignType.
  • Made getById robust in case config changes and old type IDs remain in the DB.
  • As per Mediawiki convention, removed use of $GLOBALS in CentralNoticeHooks.php.
  • In Campaign::addCampaign(), fixed new param name, removed default value.
  • Added constant for EMPTY_CAMPAIGN_TYPE_OPTION in UI instead of magic number 0.
  • Used correct escaping for message strings in CentralNotice::campaignTypeSelector().
  • Corrections to PHPDoc and formatting in various places.
  • Fixed issue with empty campaign changes log entries in UI.
  • Added entry in README to document new config variable.

Product came back with mockups for user preferences that they would like this to adhere to. I added them in the main description and here:

https://docs.google.com/presentation/d/1Ocli1ozAlyHPL8YBd2nb5_5fevLXYaFbTLmiC-YZCio/edit#slide=id.gb188dacd48_0_839

There is one point on slide 7 point d: we don't currently have any subtext like they are suggesting so no need to provide space for that.

Product came back with mockups for user preferences that they would like this to adhere to. I added them in the main description and here:

here is one point on slide 7 point d: we don't currently have any subtext like they are suggesting so no need to provide space for that.

The improvements we discussed are now in Gerrit. Also cleaned up a few more things; see the Gerrit changes for details.

AndyRussG renamed this task from Code review and release management for CN campaign filtering to Code review, fix major and minor issues, follow up on new requirements, write tests and release management for CN campaign filtering.Jan 4 2021, 9:58 PM

Change 647349 merged by jenkins-bot:
[mediawiki/extensions/CentralNotice@master] Schema updates for campaign filtering

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

Screen Shot 2021-01-06 at 4.45.45 PM.png (900×1 px, 107 KB)

I think we wanted to hide the categories that can't be turned off, but I don't think that's a blocker for +2ing.

Change 604279 merged by jenkins-bot:
[mediawiki/extensions/CentralNotice@master] Campaign filtering

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

Change 607461 merged by jenkins-bot:
[mediawiki/extensions/CentralNotice@master] Control displaying campaigns based on user preferences

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

Change 654682 had a related patch set uploaded (by Mepps; owner: Mepps):
[mediawiki/extensions/CentralNotice@master] CampaignType: Hide forced campaign type selections in user prefs

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

I've +2ed the major patches so they'll be on beta. I also have UI patch and the test patches that need to be +2ed by @AndyRussG.

On the 12th January, I left the following comment on irc:

02:53 <Seddon>mepps: Can we keep it greyed out for now. Could we also add in the following text to the preferences tab:
02:53 <Seddon> https://www.irccloud.com/pastebin/j4VW4me2/



CentralNotice is the Mediawiki extension that delivers announcements of interest to Wikimedia communities and users in the form of a banner.

Below are a series of options that allow you to manage which types of announcements you see. Certain platform notices, such as those relating to site maintenance and special notices considered necessary to all users, will still be displayed. For further information, please read the documentation provided.

02:54 <Seddon> "documentation provided" should then link to https://www.mediawiki.org/wiki/Extension:CentralNotice

Change 657467 had a related patch set uploaded (by AndyRussG; owner: AndyRussG):
[mediawiki/extensions/CentralNotice@master] Explanatory text in user preferences for camapign types

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

On the 12th January, I left the following comment on irc:

02:53 <Seddon>mepps: Can we keep it greyed out for now. Could we also add in the following text to the preferences tab:
02:53 <Seddon> https://www.irccloud.com/pastebin/j4VW4me2/



CentralNotice is the Mediawiki extension that delivers announcements of interest to Wikimedia communities and users in the form of a banner.

Below are a series of options that allow you to manage which types of announcements you see. Certain platform notices, such as those relating to site maintenance and special notices considered necessary to all users, will still be displayed. For further information, please read the documentation provided.

02:54 <Seddon> "documentation provided" should then link to https://www.mediawiki.org/wiki/Extension:CentralNotice

The patch in review adds the introductory text.

I'd like to make some minor comments about the copy itself, please? Apologies for the bother, I recognize it's not really in my purview. It's just that I don't see anything else in user preferences that mentions Mediawiki extensions or even Mediawiki as such, so this feels a bit more technical than what we generally put there. In the patch uploaded to Gerrit, I put basically the same text, but without the technical references, plus a few other minor tweaks. So, in that patch, it's as follows:

Banners display announcements of interest to Wikimedia communities and users.

Below are options that allow you to manage which types of announcements you see. Certain platform notices, such as those relating to site maintenance and special notices considered necessary to all users, will always be displayed. For further information, please read the documentation on Meta-Wiki.

The link goes to the CN Meta page.

If you'd rather it be changed to the exact copy you suggested, or need any other changes, please let me know! Thanks so much!!!!!

Change 657467 merged by jenkins-bot:
[mediawiki/extensions/CentralNotice@master] Explanatory text in user preferences for camapign types

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

AndyRussG updated the task description. (Show Details)

Change 660955 had a related patch set uploaded (by AndyRussG; owner: AndyRussG):
[mediawiki/extensions/CentralNotice@wmf_deploy] Merge branch 'master' into wmf_deploy

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

Change 660955 merged by jenkins-bot:
[mediawiki/extensions/CentralNotice@wmf_deploy] Merge branch 'master' into wmf_deploy

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

Change 649420 merged by jenkins-bot:
[mediawiki/extensions/CentralNotice@master] Add unit tests for CampaignType

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

Change 654682 abandoned by Mepps:

[mediawiki/extensions/CentralNotice@master] CampaignType: Hide forced campaign type selections in user prefs

Reason:

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