Page MenuHomePhabricator

Rebase and finish Geotargeting (sub national targeting)
Open, MediumPublic4 Estimate Story Points

Description

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

Details

Related Gerrit Patches:
mediawiki/extensions/CentralNotice : masterSchema change for region-level geotargeting

Event Timeline

DStrine created this task.Jun 24 2019, 6:26 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 24 2019, 6:26 PM
DStrine updated the task description. (Show Details)Jun 24 2019, 8:07 PM
DStrine changed the point value for this task from 0 to 4.
DStrine renamed this task from Review Geotargeting to Review Geotargeting (sub national targeting).Sep 30 2019, 5:48 PM
Ejegg renamed this task from Review Geotargeting (sub national targeting) to Rebase and finish Geotargeting (sub national targeting).Nov 6 2019, 7:57 PM
Ejegg updated the task description. (Show Details)
Ejegg claimed this task.Nov 8 2019, 10:27 PM
Ejegg triaged this task as Medium priority.
Ejegg moved this task from Backlog to Doing on the Fundraising Sprint Visual Basic Instinct board.

Change 552121 had a related patch set uploaded (by Ejegg; owner: Ejegg):
[mediawiki/extensions/CentralNotice@master] Schema change for region-level geotargeting

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

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.

Ejegg added a comment.Dec 18 2019, 4:29 PM

UI comments:

  • Cut down region lists to just IN, US & CA
  • Allocation page - choosing a country and NO REGION should show all the region-level targeted banners in the country with some indication of which region each is targeted to.
  • Need to show list of selected countries below tree selector.
  • Collapse countries and regions into one column in the campaign list
Ejegg added a subscriber: Pcoombe.Dec 20 2019, 2:48 PM

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.

TSkaff added a subscriber: TSkaff.Dec 20 2019, 4:09 PM
mepps claimed this task.Jan 9 2020, 3:39 PM
mepps added a subscriber: Ejegg.
Ejegg added a comment.Jan 16 2020, 3:07 PM

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:

  • Need to show list of selected countries (and regions) below tree selector.
  • Collapse countries and regions into one column in the campaign list

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

mepps added a comment.Tue, Feb 4, 9:57 PM

The counter is kind of under the tree select but I assume we'll address it in T243012

mepps added a comment.Wed, Feb 5, 8:47 PM

@Ejegg This is what I see in the main CN page under Add a Banner. It looks correct under edit though.

Ejegg added a comment.Wed, Feb 5, 9:26 PM

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...

Ejegg added a comment.Wed, Feb 5, 9:32 PM

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?

Ejegg added a comment.Wed, Feb 5, 10:31 PM

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.