Page MenuHomePhabricator

Collapse countries and regions into one column in the campaign list
Closed, ResolvedPublic

Event Timeline

I see that this is sort of done except it currently lists regions by their number (otherwise might be longer). Do we want to change that?

When I pull down the latest version of the patch I still see separate columns for country and region: F31550196. Do you have local work to combine them?

image.png (575×778 px, 62 KB)

This is what I see in the edit screen @Ejegg. What did you have in mind?

Oops sorry, I misread the task, and also the way I set up the test data confused me.. This is what I see for this and now I get what you mean. It looks like we've previously used the countries column. Which one should we use going forward? Also it's a little weird that if a region of a country is included but not the whole country, it doesn't count as a country. I can see the argument for it but it's confusing as a user.

image.png (86×1 px, 19 KB)

This ticket is about fixing it in the campaign list (the main page that shows up on Special:CentralNotice).

This other ticket: T243012 is about showing the full list under the tree selector (rather than just the count of countries and regions) so that people can see at a glance what's selected, without scrolling through the whole tree selector.

Ah, just saw your update. So we should probably change the title of the column (maybe Geotargeting?). The list would look something like CO, US (MA) to indicate that all of Colombia and the US state of MA are selected.

I'm thinking about the best way to approach this.

The current way it works is that the countries and regions are selected separately in CNCampaignPager. Then for the countries table it uses listContries from CentralNotice.php and for the regions table it uses listRegions. These functions are only used here. I'm trying to decide whether to create a new function that lists them both or combine the lists in the table or try to combine the query output (I'm not sure that's doable). I guess the questions is whether we want to list all the countries then all the countries with the regions or try to combine the lists. As a user, if it's a long list, it'd be easy to look for a country with regions in the first countries list, not see it, then think something is missing.

I would expect to see the countries in alphabetical order whether they had regions or not, and have the regions listed in parenthesis after their relevant country. So something like

BR, CA (AB, BC), CO, US (MA)

To represent all of Brazil and Columbia, the US state of MA, and the Canada provinces of AB and BC.

In working on this I've noticed this "All except" language being used, but it's not being applied correctly. In the example in the screenshot, the regions listed are the ones selected, not the ones excepted. (Also this is my dumb way of combining them but obviously I need to deal with that initial comma.

image.png (177×1 px, 45 KB)

Hmm, and while Portuguese regions DO have numeric identifiers, US regions should be using the 2 letter state abbreviations.

So it's in the array_diff function in CentralNotice.php:1788 that causes this. Otherwise it would be strings. There are a few things to approach on this task.

Documenting my current thinking: need to tackle the array_diff error listed above. It seems to be how the array is passed in to the makeShortList. Also I want to move the list logic to one function in CentralNotice whereas now it's split up into two separate functions of listCountries and listRegions neither of which is used outside of CNCampaignPager.

I fixed the All except/numerical code bug! The all list was returning the array_keys instead of the values. Now working on getting one cohesive list.

Change 571768 had a related patch set uploaded (by Mepps; owner: Mepps):
[mediawiki/extensions/CentralNotice@master] Geotargeting: Collapse countries and regions into one column and fix regions code bug

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

Hmm I also just noticed that the countries and regions still show up even if geotargeting is not checked.

Change 571768 merged by jenkins-bot:
[mediawiki/extensions/CentralNotice@master] Geotargeting: Collapse countries and regions into one column

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