Page MenuHomePhabricator

Epic: Rebase and finish Geotargeting (sub national targeting)
Closed, ResolvedPublic4 Estimated 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

Event Timeline

DStrine changed the point value for this task from 0 to 4.
DStrine moved this task from Triage to Sprint +2 on the Fundraising-Backlog board.
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 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.

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

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:

  • 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

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.

image.png (546×809 px, 35 KB)

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 renamed this task from Rebase and finish Geotargeting (sub national targeting) to Epic: Rebase and finish Geotargeting (sub national targeting).Mar 3 2020, 9:44 PM
DStrine added a project: Epic.
DStrine moved this task from Triage to Epics in progress on the Fundraising-Backlog board.

@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?

@spatton @TSkaff Would you guys want to test out the recent work done on Geotargeting? We have a site set up. You'd just need to create an account and give me the info. I'd be happy to set up a call to go over it.

Change 588156 had a related patch set uploaded (by Ejegg; owner: Ejegg):
[mediawiki/extensions/CentralNotice@master] Geotarget list below selector: sort and add separator

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

Change 552121 merged by jenkins-bot:
[mediawiki/extensions/CentralNotice@master] Schema change for region-level geotargeting

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

Change 588156 merged by jenkins-bot:
[mediawiki/extensions/CentralNotice@master] Geotarget list below selector: sort and add separator

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

BM6kim2 raised the priority of this task from Medium to High.
BM6kim2 set Due Date to Aug 13 2020, 12:00 AM.