Page MenuHomePhabricator

CentralNotice: Campaign Settings - Multiselects - If all items selected and saved, further attempts to make changes to selection are not saved.
Closed, ResolvedPublic2 Estimated Story Points

Event Timeline

The select form element work fine with JS disabled. CentralNotice does little other than activate this widget (which is included in CN code), so the first thing I'm looking at is a possible bug in the widget itself... Thanks!!!

Seems to be a bug in the "Remove all" code...

Some more details:

  • Clicking "Remove all" does correctly de-select all options in the hidden multiple select form element.
  • When this happens, no project_languages parameters are included in the form POST. This is the correct client behaviour for that state. However, it isn't handled correctly on the server--in such cases, no changes to languages are saved. The same thing happens for projects and countries, too.
  • Removing all languages, projects or (for geotargeted campaigns) countries should not be allowed (since it would result in a campaign that is never shown). However, silently discarding attempted changes like this is not OK; rather, the user should see an error message. Here's a new task for this issue: T185873.
  • In addition, clicking "Remove all" breaks the JS widget's link with the options in the hidden select element, for all the items that were removed. So, after removing all selected items, attempts to add back any of the items removed do not re-select those items in the hidden element. The form is then submitted with all items de-selected, and the server discards the changes (see above). I imagine this is the actual use case we're running into with this bug: a user clicks "Remove all", then tries to add back certain elements. However, the JS bug causes them not to be really added, and the server-side bug means we silently ignore the empty submission.

Change 406598 had a related patch set uploaded (by AndyRussG; owner: AndyRussG):
[mediawiki/extensions/CentralNotice@master] Admin UI: Fix multiselects by selecting options using prop()

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

The attached patch fixes the JS issue, which addresses the main use case in the bug's description. With the fix, if the user clicks to remove all items from a multiselect (whether or not they clicked to add all items before), then re-adds any of them, the changes should save correctly.

The issue of silently discarding changes if the user tries to save a campaign with all items removed from a multiselect, instead of showing an error, is not addressed by the patch. (Task for that: T185873.)

The issue was that the old version of the multiselect jquery.ui module included in CN uses attr() instead of prop() to set the selected state of options in the hidden select input element. The patch changes it to use prop() throughout. This is also what the current version of the library does and should be compatible with browsers that support Mediawiki JS (see [[ http://api.jquery.com/prop/ | prop()]]).

These old widgets should be removed. Taks for that: T185913. (See also T100270.)

Thanks much, all!!! :D

Change 406598 merged by jenkins-bot:
[mediawiki/extensions/CentralNotice@master] Admin UI: Fix multiselects by selecting options using prop()

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

Hello. I'm running into the same issue again. I've got a request from @NahidSultan on behalf of WM-BN to amend the languages of this campaing and it's still not possible to do a batch removal. I'm not removing 400+ languages one by one... :-(

Thanks @MarcoAurelio and @Jseddon ! The fix for this is deployed, so I'm closing the ticket... Please re-open if you run into this again (or make new ticket if you think it's a different issue). :)

AndyRussG set the point value for this task to 2.Aug 21 2019, 5:23 AM