[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Page MenuHomePhabricator

Campaign fallen back to gets incorrect status code and may be incorrectly hidden
Closed, ResolvedPublic2 Estimated Story Points

Description

In many cases, when a campaign cedes a page view to another campaign, the second campaign gets as its status code the code for the first campaign.

Here is data for one day of impressions. status0_code is the status code of the first campaign in campaignStatuses. When a campaign was fallen back from, there is a value for status1_code, which should be the status code of the second campaign chosen, and when that campaign was fallen back from, there is a value for status2_code, etc. final_status_code is the legacy status code field and should should be the status code of the last campaign chosen.

Actual impression numbers have been replaced here by the percentage of total impressions for the day, 2019-12-03.

Here is the query used for this data: P9870.

Note, for example, several cases of the non-fundraising campaign getting a 2.9 status code, which is for when a FR banner is hidden because the user donated.

status0_campaignstatus0_codestatus1_campaignstatus1_codestatus2_campaignstatus2_codestatus3_campaignstatus3_codefinal_campaignfinal_status_codesample_rateprecentage_impressions
C1920_en6C_dsk_FR6NoneNoneNoneNoneNoneNoneC1920_en6C_dsk_FR6157.341179
C1920_en6C_dsk_FR2.2wikiscience20192.2NoneNoneNoneNonewikiscience20192.2121.525727
C1920_en6C_dsk_FR2.1wikiscience20192.1NoneNoneNoneNonewikiscience20192.1114.604889
C1920_en6C_dsk_FR2.2wikiscience20196NoneNoneNoneNonewikiscience2019612.737611
C1920_en6C_dsk_FR2.9wikiscience20192.9NoneNoneNoneNonewikiscience20192.911.870389
C1920_en6C_dsk_low_FR6NoneNoneNoneNoneNoneNoneC1920_en6C_dsk_low_FR610.578348
C1920_en6C_dsk_FR7NoneNoneNoneNoneNoneNoneC1920_en6C_dsk_FR710.238746
C1920_en6C_dsk_FR2.2C1920_en6C_dsk_low_FR2.2wikiscience20192.2NoneNonewikiscience20192.210.219917
C1920_en6C_dsk_low_FR2.2C1920_en6C_dsk_FR2.2wikiscience20192.2NoneNonewikiscience20192.210.219821
C1920_en6C_dsk_FR2.2wikiscience20192.1NoneNoneNoneNonewikiscience20192.110.200840
C1920_en6C_dsk_FR2.1C1920_en6C_dsk_low_FR2.1wikiscience20192.1NoneNonewikiscience20192.110.149235
C1920_en6C_dsk_low_FR2.1C1920_en6C_dsk_FR2.1wikiscience20192.1NoneNonewikiscience20192.110.148557
C1920_en6C_dsk_FR2.2C1920_en6C_dsk_low_FR2.2wikiscience20196NoneNonewikiscience2019610.028206
C1920_en6C_dsk_low_FR2.2C1920_en6C_dsk_FR2.2wikiscience20196NoneNonewikiscience2019610.028099
C1920_en6C_dsk_FR2.2wikiscience20192.14NoneNoneNoneNonewikiscience20192.1410.023700
C1920_en6C_dsk_low_FR2.9C1920_en6C_dsk_FR2.9wikiscience20192.9NoneNonewikiscience20192.910.018608
C1920_en6C_dsk_FR2.9C1920_en6C_dsk_low_FR2.9wikiscience20192.9NoneNonewikiscience20192.910.018605
C1920_en6C_dsk_FR2.14wikiscience20192.14NoneNoneNoneNonewikiscience20192.1410.014741
C1920_en6C_dsk_FR2.9wikiscience20192.1NoneNoneNoneNonewikiscience20192.110.014152
C1920_en6C_dsk_FR2.2wikiscience20197NoneNoneNoneNonewikiscience2019710.009344
C1920_en6C_dsk_low_FR7NoneNoneNoneNoneNoneNoneC1920_en6C_dsk_low_FR710.002413
C1920_en6C_dsk_FR2.2C1920_en6C_dsk_low_FR2.2wikiscience20192.1NoneNonewikiscience20192.110.002043
C1920_en6C_dsk_low_FR2.2C1920_en6C_dsk_FR2.2wikiscience20192.1NoneNonewikiscience20192.110.001992
wikiscience20192.2NoneNoneNoneNoneNoneNonewikiscience20192.20.010.000675
wikiscience20196NoneNoneNoneNoneNoneNonewikiscience201960.010.000639
C1920_en6C_dsk_low_FR2.2C1920_en6C_dsk_FR2.2wikiscience20192.14NoneNonewikiscience20192.1410.000260
C1920_en6C_dsk_FR2.2C1920_en6C_dsk_low_FR2.2wikiscience20192.14NoneNonewikiscience20192.1410.000212
C1920_en6C_dsk_low_FR2.9C1920_en6C_dsk_FR2.9wikiscience20192.1NoneNonewikiscience20192.110.000158
C1920_en6C_dsk_FR2.14C1920_en6C_dsk_low_FR2.14wikiscience20192.14NoneNonewikiscience20192.1410.000146
C1920_en6C_dsk_low_FR2.14C1920_en6C_dsk_FR2.14wikiscience20192.14NoneNonewikiscience20192.1410.000146
C1920_en6C_dsk_FR3NoneNoneNoneNoneNoneNoneC1920_en6C_dsk_FR310.000143
C1920_en6C_dsk_FR2.2C1920_en6C_dsk_low_FR2.2wikiscience20197NoneNonewikiscience2019710.000134
C1920_en6C_dsk_FR2.9C1920_en6C_dsk_low_FR2.9wikiscience20192.1NoneNonewikiscience20192.110.000131
C1920_en6C_dsk_low_FR2.2C1920_en6C_dsk_FR2.2wikiscience20197NoneNonewikiscience2019710.000075
C1920_en6C_dsk_low_FR2.1wikiscience20192.1NoneNoneNoneNonewikiscience20192.110.000030
C1920_en6C_dsk_FR5NoneNoneNoneNoneNoneNoneC1920_en6C_dsk_FR510.000030
wikiscience20192.1NoneNoneNoneNoneNoneNonewikiscience20192.10.010.000024
C1920_en6C_dsk_low_FR2.2wikiscience20192.2NoneNoneNoneNonewikiscience20192.210.000015
wikiscience20197NoneNoneNoneNoneNoneNonewikiscience201970.010.000009
asian_month_20192.2wikiscience20192.2NoneNoneNoneNonewikiscience20192.20.010.000003
NoneNoneNoneNoneNoneNoneNoneNoneC1920_en6C_dsk_FR610.000003
C1920_en6C_dsk_low_FR2.9wikiscience20192.9NoneNoneNoneNonewikiscience20192.910.000003

Event Timeline

AndyRussG renamed this task from Campaign fallen back to gets incorrect status code to Campaign fallen back to gets incorrect status code and may be incorrectly hidden.Jan 7 2020, 9:17 PM

For my own notes, all of this happens in state.js.

@AndyRussG Is there any documentation of what these status codes mean?

Also @AndyRussG what is the correct, expected behavior here?

@AndyRussG I tried copying and pasting that query using impyla but I'm getting:
"HiveServer2Error: Error while compiling statement: FAILED: ParseException line 4:0 missing EOF at 'SELECT' near ')'". -- This is resolved so this comment can be ignored.

@AndyRussG Is there any documentation of what these status codes mean?

Yep! Here is the doc. The code itself also documents this; at the beginning of state.js, see STATUSES for the overall statuses and REASONS for the reasons. The status code is just the status number, a dot, and the reason number. So, for 2.1 the overall status is banner_canceled and the reason is close, which indicates the banner was not shown because the user had closed a banner in the same category recently on a previous pageview.

what is the correct, expected behavior here?

The correct behaviour is that, with campaign fallback, the status code for a campaign fallen back from should have no impact on the status code of any subsequent campaigns that we try. The campaignStatuses property in the data we send back is an array of the campaigns chosen and their respective status codes, in the order in which they were chosen.

From the query in the task description, we can see that a non-FR campaign was correctly selected many times when the FR campaign opted not to show a banner. However, the status code from the FR campaign often carried over to the non-FR campaign. It's clear that something's wrong because the non-FR campaign sometimes got a 2.9 status code, which is banner cancelled because the user has donated. However, non-FR campaigns should never get that status code.

Hope this is useful!! Thanks for working on this!

@AndyRussG Okay, I can see what you mean. When I look at the loop what I see is that state.setCampaign is called early on and that should set the status to CAMPAIGN_CHOSEN for the campaign that gets selected. cancelBanner is called if hide.shouldHide() is true. Just recording my findings as I go.

@AndyRussG Okay, I can see what you mean. When I look at the loop what I see is that state.setCampaign is called early on and that should set the status to CAMPAIGN_CHOSEN for the campaign that gets selected. cancelBanner is called if hide.shouldHide() is true. Just recording my findings as I go.

Yep!! All correct :)

@AndyRussG, @Ejegg and I think we see what happened, and I've made a commit. However, I don't know how to test it. Any thoughts on that?

Change 564062 had a related patch set uploaded (by Mepps; owner: Mepps):
[mediawiki/extensions/CentralNotice@master] Reset hide.shouldHide and hide.reason when setting new category

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

I'm trying to test my patch locally but the banners still aren't displaying with "error: Not Found" in mw.centralNotice.data.errorMsg.

To test the fix locally:

  • Set up two campaigns, one higher priority than the other.
  • Include a close button in the banner for the campaign with higher priority.
  • The higher priority campaign should always show, until the close button is clicked on the banner. On subsequent pageviews after the button has been clicked, the lower priority campaign should show.
  • To test again, clear cookies for the local wiki.

You can also use this procedure to reproduce the bug in the current, broken version of the code: use the same setup, and click the close button in the banner of the higher-priority campaign. In the broken state, no campaigns should show on subsequent pageviews, and in campaignStatuses, both campaigns should get the banner hidden status code.

Thanks for working on this!!

Change 564062 merged by jenkins-bot:
[mediawiki/extensions/CentralNotice@master] Reset hide shouldHide,reason private variables when setting new category

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

Change 566563 had a related patch set uploaded (by Mepps; owner: Mepps):
[mediawiki/extensions/CentralNotice@wmf_deploy] Reset hide shouldHide,reason private variables when setting new category

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

Change 566563 merged by jenkins-bot:
[mediawiki/extensions/CentralNotice@wmf_deploy] Reset hide shouldHide,reason private variables when setting new category

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

Change 566901 had a related patch set uploaded (by Catrope; owner: Mepps):
[mediawiki/extensions/CentralNotice@wmf/1.35.0-wmf.16] Reset hide shouldHide,reason private variables when setting new category

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

Change 566901 merged by Catrope:
[mediawiki/extensions/CentralNotice@wmf/1.35.0-wmf.16] Reset hide shouldHide,reason private variables when setting new category

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

Change 566902 had a related patch set uploaded (by Catrope; owner: Mepps):
[mediawiki/extensions/CentralNotice@wmf/1.35.0-wmf.15] Reset hide shouldHide,reason private variables when setting new category

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

Change 566902 merged by Catrope:
[mediawiki/extensions/CentralNotice@wmf/1.35.0-wmf.15] Reset hide shouldHide,reason private variables when setting new category

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

Mentioned in SAL (#wikimedia-operations) [2020-01-24T00:34:48Z] <catrope@deploy1001> Synchronized php-1.35.0-wmf.15/extensions/CentralNotice/resources/ext.centralNotice.display/hide.js: T240802 (duration: 01m 07s)

Mentioned in SAL (#wikimedia-operations) [2020-01-24T00:36:02Z] <catrope@deploy1001> Synchronized php-1.35.0-wmf.16/extensions/CentralNotice/resources/ext.centralNotice.display/hide.js: T240802 (duration: 01m 05s)