Page MenuHomePhabricator

More small fixes needed for Campaign fallback
Closed, ResolvedPublic

Description

Here are some more small fixes:

  • in resources/ext.centralNotice.display/index.js line 345, remove the || 100 at the end.
  • in resources/ext.centralNotice.display/index.js improve loop logic so we don't have to null out the local campaign variable.
  • in resources/ext.centralNotice.display/state.js in setCampaign(), also delete bannerCanceledReason property.

Details

Related Gerrit Patches:
mediawiki/extensions/CentralNotice : masterClear out bannerCancelReason when calling setCampaign()
mediawiki/extensions/CentralNotice : masterClear out bannerCancelReason when calling setCampaign()

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 30 2019, 5:26 PM
jgleeson claimed this task.Oct 7 2019, 8:00 PM

Here are some suggestions on what to smoke test for any changes to the fallback loop. You could check all the following situations, each time looking for errors in the browser console and checking the contents of mw.centralNotice.data.

  • No campaigns available for the user. (Loop is not executed, no campaign chosen.)
  • Just one campaign available for the user, and the campaign does not fall back. (Loop runs once, campaign chosen.)
  • Two campaigns, one higher priority than the other, and the higher priority campaign has the "Impression Diet" feature enabled, so it falls back and cedes some pageviews to the lower priority campaign. (Loop exits on second iteration. lower priority campaign chosen.)
  • Same setup as above, but set wgCentralNoticeMaxIterations in LocalSettings.php to 1. (Loop runs once and exits due to reaching the maximum allowable iterations, no campaign chosen.)
  • Same setup as above, but leaving wgCentralNoticeMaxIterations at the default and with the lower priority campaign also having the "impressionDiet" feature enabled, so it also falls back. (Loop exits on the second iteration, no campaign chosen.)

Hope this is helpful! Thanks much!!!!

Change 543203 had a related patch set uploaded (by Jgleeson; owner: Jgleeson):
[mediawiki/extensions/CentralNotice@master] Reduce usage of 'campaign' local var within campaign fallback and delete bannerCancelReason when calling setCampaign()

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

Change 545023 had a related patch set uploaded (by Jgleeson; owner: Jgleeson):
[mediawiki/extensions/CentralNotice@master] Clear out bannerCancelReason when calling setCampaign()

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

Change 545023 abandoned by Jgleeson:
Clear out bannerCancelReason when calling setCampaign()

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

Change 543203 merged by jenkins-bot:
[mediawiki/extensions/CentralNotice@master] Clear out bannerCancelReason when calling setCampaign()

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

DStrine closed this task as Resolved.Tue, Oct 29, 8:38 PM

Note: the second bullet point from the task description has been spun out as T236845.