[MediaWiki-commits] [Gerrit] Improve impression diet state machine - change (mediawiki...CentralNotice)
AndyRussG has uploaded a new change for review.
https://gerrit.wikimedia.org/r/258641
Change subject: Improve impression diet state machine
..
Improve impression diet state machine
This fixes two bugs which were causing extra waitdate hides.
* Even when readers have a waitUntil set, increasing the campaign maximum
impressions seen per cycle should result in the additional banners.
* Got rid of an unnecessary error condition. It's clear that every code
path in the main switch will assign a value to the hide variable.
Also gets rid of the waitnorestart state. This was equivalent to waitdate,
just check the single-shot campaign configuration to tell if waitdate is
going to restart or not.
Bug: T121178
Change-Id: I7d1c2b6672d5ef12405d1f43a36ea3f5e01da21c
---
M resources/subscribing/ext.centralNotice.display.state.js
M resources/subscribing/ext.centralNotice.impressionDiet.js
2 files changed, 47 insertions(+), 48 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/CentralNotice
refs/changes/41/258641/1
diff --git a/resources/subscribing/ext.centralNotice.display.state.js
b/resources/subscribing/ext.centralNotice.display.state.js
index 683dbab..856dd8c 100644
--- a/resources/subscribing/ext.centralNotice.display.state.js
+++ b/resources/subscribing/ext.centralNotice.display.state.js
@@ -42,7 +42,7 @@
'close': 1,
'waitdate': 2,
'waitimps': 3,
- 'waiterr': 4,
+ 'waiterr': 4, // Deprecated
'belowMinEdits': 5,
'viewLimit': 6,
'seen-fullscreen': 7,
@@ -51,7 +51,7 @@
'cookies': 10,
'seen': 11,
'empty': 12,
- 'waitnorestart': 13,
+ 'waitnorestart': 13, // Deprecated
'waitnostorage': 14,
'namespace': 15
};
diff --git a/resources/subscribing/ext.centralNotice.impressionDiet.js
b/resources/subscribing/ext.centralNotice.impressionDiet.js
index 9badccc..06e4c49 100644
--- a/resources/subscribing/ext.centralNotice.impressionDiet.js
+++ b/resources/subscribing/ext.centralNotice.impressionDiet.js
@@ -52,8 +52,7 @@
mixin.setPreBannerHandler( function( mixinParams ) {
var forceFlag = mw.util.getParamValue( 'force' ),
- hide = null,
- pastDate, waitForHideImps, waitForShowImps;
+ hide;
// Only use cookies for certain campaigns on the legacy track
// Do this here since it's needed for storageAvailable() (below)
@@ -64,8 +63,8 @@
return;
}
- // Also just hide a banner if we have no way to store counts
if ( !storageAvailable() ) {
+ // Hide the banner if we have no way to store counts.
hide = 'waitnostorage';
} else {
@@ -75,61 +74,61 @@
identifier = mixinParams.cookieName;
counts = getCounts();
- // Compare counts against campaign settings and decide
whether to
- // show a banner
- pastDate = counts.waitUntil < new Date().getTime();
- waitForHideImps = counts.waitCount <
mixinParams.skipInitial;
- waitForShowImps = counts.waitSeenCount <
mixinParams.maximumSeen;
+ if ( counts.waitUntil < new Date().getTime()
+ && counts.waitSeenCount >=
mixinParams.maximumSeen
+ ) {
+ // We're beyond the wait period, and have
nothing to do except
+ // maybe start a new cycle.
- if ( !pastDate ) {
- // We're still waiting for the next cycle to
begin.
- hide = 'waitdate';
- counts.waitCount += 1;
- } else if ( pastDate && waitForHideImps ) {
- // We're still skipping initial impressions.
- hide = 'waitimps';
- counts.waitCount += 1;
- } else if ( pastDate && !waitForHideImps &&
waitForShowImps ) {
- // Show a banner!
- hide = false;
- counts.waitSeenCount += 1;
- counts.seenCount += 1;
-
- // For restartCycleDelay, 0 is a magic number
that means, never
- // restart. TODO Use a checkbox instead of a
magic
[MediaWiki-commits] [Gerrit] Improve impression diet state machine - change (mediawiki...CentralNotice)
jenkins-bot has submitted this change and it was merged.
Change subject: Improve impression diet state machine
..
Improve impression diet state machine
This fixes two bugs which were causing extra waitdate hides.
* Even when readers have a waitUntil set, increasing the campaign maximum
impressions seen per cycle should result in the additional banners.
* Got rid of an unnecessary error condition. It's clear that every code
path in the main switch will assign a value to the hide variable.
Also gets rid of the waitnorestart state. This was equivalent to waitdate,
just check the single-shot campaign configuration to tell if waitdate is
going to restart or not.
Bug: T121178
Change-Id: I7d1c2b6672d5ef12405d1f43a36ea3f5e01da21c
---
M resources/subscribing/ext.centralNotice.display.state.js
M resources/subscribing/ext.centralNotice.impressionDiet.js
2 files changed, 47 insertions(+), 48 deletions(-)
Approvals:
AndyRussG: Looks good to me, approved
jenkins-bot: Verified
diff --git a/resources/subscribing/ext.centralNotice.display.state.js
b/resources/subscribing/ext.centralNotice.display.state.js
index 683dbab..856dd8c 100644
--- a/resources/subscribing/ext.centralNotice.display.state.js
+++ b/resources/subscribing/ext.centralNotice.display.state.js
@@ -42,7 +42,7 @@
'close': 1,
'waitdate': 2,
'waitimps': 3,
- 'waiterr': 4,
+ 'waiterr': 4, // Deprecated
'belowMinEdits': 5,
'viewLimit': 6,
'seen-fullscreen': 7,
@@ -51,7 +51,7 @@
'cookies': 10,
'seen': 11,
'empty': 12,
- 'waitnorestart': 13,
+ 'waitnorestart': 13, // Deprecated
'waitnostorage': 14,
'namespace': 15
};
diff --git a/resources/subscribing/ext.centralNotice.impressionDiet.js
b/resources/subscribing/ext.centralNotice.impressionDiet.js
index 9badccc..06e4c49 100644
--- a/resources/subscribing/ext.centralNotice.impressionDiet.js
+++ b/resources/subscribing/ext.centralNotice.impressionDiet.js
@@ -52,8 +52,7 @@
mixin.setPreBannerHandler( function( mixinParams ) {
var forceFlag = mw.util.getParamValue( 'force' ),
- hide = null,
- pastDate, waitForHideImps, waitForShowImps;
+ hide;
// Only use cookies for certain campaigns on the legacy track
// Do this here since it's needed for storageAvailable() (below)
@@ -64,8 +63,8 @@
return;
}
- // Also just hide a banner if we have no way to store counts
if ( !storageAvailable() ) {
+ // Hide the banner if we have no way to store counts.
hide = 'waitnostorage';
} else {
@@ -75,61 +74,61 @@
identifier = mixinParams.cookieName;
counts = getCounts();
- // Compare counts against campaign settings and decide
whether to
- // show a banner
- pastDate = counts.waitUntil < new Date().getTime();
- waitForHideImps = counts.waitCount <
mixinParams.skipInitial;
- waitForShowImps = counts.waitSeenCount <
mixinParams.maximumSeen;
+ if ( counts.waitUntil < new Date().getTime()
+ && counts.waitSeenCount >=
mixinParams.maximumSeen
+ ) {
+ // We're beyond the wait period, and have
nothing to do except
+ // maybe start a new cycle.
- if ( !pastDate ) {
- // We're still waiting for the next cycle to
begin.
- hide = 'waitdate';
- counts.waitCount += 1;
- } else if ( pastDate && waitForHideImps ) {
- // We're still skipping initial impressions.
- hide = 'waitimps';
- counts.waitCount += 1;
- } else if ( pastDate && !waitForHideImps &&
waitForShowImps ) {
- // Show a banner!
- hide = false;
- counts.waitSeenCount += 1;
- counts.seenCount += 1;
-
- // For restartCycleDelay, 0 is a magic number
that means, never
- // restart. TODO Use a checkbox instead of a
magic number.
- if ( (
[MediaWiki-commits] [Gerrit] Improve impression diet state machine - change (mediawiki...CentralNotice)
jenkins-bot has submitted this change and it was merged.
Change subject: Improve impression diet state machine
..
Improve impression diet state machine
This fixes two bugs which were causing extra waitdate hides.
* Even when readers have a waitUntil set, increasing the campaign maximum
impressions seen per cycle should result in the additional banners.
* Got rid of an unnecessary error condition. It's clear that every code
path in the main switch will assign a value to the hide variable.
Also gets rid of the waitnorestart state. This was equivalent to waitdate,
just check the single-shot campaign configuration to tell if waitdate is
going to restart or not.
Bug: T121178
Change-Id: I7d1c2b6672d5ef12405d1f43a36ea3f5e01da21c
---
M resources/subscribing/ext.centralNotice.display.state.js
M resources/subscribing/ext.centralNotice.impressionDiet.js
2 files changed, 47 insertions(+), 48 deletions(-)
Approvals:
AndyRussG: Looks good to me, approved
jenkins-bot: Verified
diff --git a/resources/subscribing/ext.centralNotice.display.state.js
b/resources/subscribing/ext.centralNotice.display.state.js
index bee9e41..ced7b90 100644
--- a/resources/subscribing/ext.centralNotice.display.state.js
+++ b/resources/subscribing/ext.centralNotice.display.state.js
@@ -42,7 +42,7 @@
'close': 1,
'waitdate': 2,
'waitimps': 3,
- 'waiterr': 4,
+ 'waiterr': 4, // Deprecated
'belowMinEdits': 5,
'viewLimit': 6,
'seen-fullscreen': 7,
@@ -51,7 +51,7 @@
'cookies': 10,
'seen': 11,
'empty': 12,
- 'waitnorestart': 13,
+ 'waitnorestart': 13, // Deprecated
'waitnostorage': 14,
'namespace': 15
};
diff --git a/resources/subscribing/ext.centralNotice.impressionDiet.js
b/resources/subscribing/ext.centralNotice.impressionDiet.js
index 9badccc..06e4c49 100644
--- a/resources/subscribing/ext.centralNotice.impressionDiet.js
+++ b/resources/subscribing/ext.centralNotice.impressionDiet.js
@@ -52,8 +52,7 @@
mixin.setPreBannerHandler( function( mixinParams ) {
var forceFlag = mw.util.getParamValue( 'force' ),
- hide = null,
- pastDate, waitForHideImps, waitForShowImps;
+ hide;
// Only use cookies for certain campaigns on the legacy track
// Do this here since it's needed for storageAvailable() (below)
@@ -64,8 +63,8 @@
return;
}
- // Also just hide a banner if we have no way to store counts
if ( !storageAvailable() ) {
+ // Hide the banner if we have no way to store counts.
hide = 'waitnostorage';
} else {
@@ -75,61 +74,61 @@
identifier = mixinParams.cookieName;
counts = getCounts();
- // Compare counts against campaign settings and decide
whether to
- // show a banner
- pastDate = counts.waitUntil < new Date().getTime();
- waitForHideImps = counts.waitCount <
mixinParams.skipInitial;
- waitForShowImps = counts.waitSeenCount <
mixinParams.maximumSeen;
+ if ( counts.waitUntil < new Date().getTime()
+ && counts.waitSeenCount >=
mixinParams.maximumSeen
+ ) {
+ // We're beyond the wait period, and have
nothing to do except
+ // maybe start a new cycle.
- if ( !pastDate ) {
- // We're still waiting for the next cycle to
begin.
- hide = 'waitdate';
- counts.waitCount += 1;
- } else if ( pastDate && waitForHideImps ) {
- // We're still skipping initial impressions.
- hide = 'waitimps';
- counts.waitCount += 1;
- } else if ( pastDate && !waitForHideImps &&
waitForShowImps ) {
- // Show a banner!
- hide = false;
- counts.waitSeenCount += 1;
- counts.seenCount += 1;
-
- // For restartCycleDelay, 0 is a magic number
that means, never
- // restart. TODO Use a checkbox instead of a
magic number.
- if ( (
