https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/CentralNotice/+/518449/
In order to merge on top of current code, this patch will need a pretty significant rebase with a full rewrite of the changes to some of the files. Let's one of us do that rebase, then respond to awight's code review. A different member of fr-tech should then take the role of code reviewer.
DoD: handle the multiple rounds of code review and feedback
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | • mepps | T226438 Epic: Rebase and finish Geotargeting (sub national targeting) | |||
| Resolved | • mepps | T243011 Collapse countries and regions into one column in the campaign list | |||
| Resolved | • mepps | T243012 Show selected countries and regions under tree selector | |||
| Resolved | • mepps | T243013 Fix tests and code to make Geotargeting pass CI | |||
| Resolved | Ejegg | T243014 Break down geotargeting work into next steps | |||
| Resolved | Ejegg | T241070 Server for previewing/usability testing new CentralNotice features | |||
| Resolved | AndyRussG | T250751 Address final code review comments on geotargeting |
Change 552121 had a related patch set uploaded (by Ejegg; owner: Ejegg):
[mediawiki/extensions/CentralNotice@master] Schema change for region-level geotargeting
Patch set 32 includes all the improvements I can think to make short of throwing out the tree selector to replace it with something that would be clearer for simple country selection. Ready for more review, or suggestions on that alternate admin UI.
Banner allocation page doesn't take this into account.
We need to show the selected countries below the tree selector.
Also collapse the 2 columns that currently separate country and region to just one column.
We should restrict the list of countries to only a few. I'll update this task again with an accurate list.
UI comments:
Add GB to region lists per @Pcoombe:
If it's possible to split out the countries of the UK (England/Scotland/Wales/Northern Ireland), that would be nice. I'm sure Wikimedia UK could make use of it, as they do a lot of work which is targeted at Scotland and Wales specifically.
So I haven't yet implemented any of the changes mentioned in David's and my comments from Dec 18th: https://phabricator.wikimedia.org/T226438#5751615
These two are somewhat related:
These lists should probably both use the same formatting, although it may need to be implemented both in JS and in PHP.
When only countries are selected, we could use the same functions as we currently do, to get the nice abbreviated format when only a couple countries are left off "All except AU, CA, GB, IE, NZ, US". When regions are selected the list would look something like CO, US (MA) to indicate that all of Colombia and the US state of MA are selected.
With only one working day left in this sprint, I'm going to move this to Sprint B, with the idea that we'll break out the two issues Elliott mentioned into separate tasks. Rebasing the patch should probably be a separate task as well.
The big rebase that needed some code rewritten is already done, and I did another simple rebase on Jan 3rd. I'll try another rebase now just to make sure nothing else has come in that will interfere.
Well, the rebase worked fine, but we're now failing tests because of a couple new Phan checks. Fixing one of them just needs a @suppress comment, but this one should probably be fixed in code: https://integration.wikimedia.org/ci/job/mwext-php72-phan-seccheck-docker/29593/console
The counter is kind of under the tree select but I assume we'll address it in T243012
@Ejegg This is what I see in the main CN page under Add a Banner. It looks correct under edit though.
Yep, something's busted there. Looks like some scripts aren't being loaded. The multiselects for language and project are also unstyled.
So in CentralNoticeHooks.php, I think we need to move the jquery.plugin.jstree dependency out of the list for
ext.centralNotice.adminUi.campaignManager and into the list for ext.centralNotice.adminUi.
Theoretically, that should be all we need to load it on that list, since inside CentralNotice::outputListOfNotices we have the line
$out->addModules( 'ext.centralNotice.adminUi' );
However, that should also be loading the jquery multiselect scripts that seem to be missing...
OK, it's due to a change I made to centralnotice.js back in PS25 - I took a chunk of UI initialization code and moved it from centralnotice.js into campaignManager.js:
https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/CentralNotice/+/518449/24..25
Want me to undo that bit?
I had basically fixed it locally to confirm that it was my PS25 change at fault, so I went ahead and pushed that up. The multiselects and the geotargeting bits should now work in the 'add campaign' section like they do on the 'edit campaign' page.
@DStrine @AndyRussG @Ejegg This is now ready to test at http://cn-stage-2.wmflabs.org/wiki/Special:CentralNotice. Anyone who wants to test should create an account and let me know so I can make them an admin. Who should sign off on this?
Change 588156 had a related patch set uploaded (by Ejegg; owner: Ejegg):
[mediawiki/extensions/CentralNotice@master] Geotarget list below selector: sort and add separator
Change 552121 merged by jenkins-bot:
[mediawiki/extensions/CentralNotice@master] Schema change for region-level geotargeting
Change 588156 merged by jenkins-bot:
[mediawiki/extensions/CentralNotice@master] Geotarget list below selector: sort and add separator