AndyRussG has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/391208 )
Change subject: Add API to delay call to record impression
......................................................................
Add API to delay call to record impression
Change-Id: Ida5e777d435d157cfcf6ead7728b35b6beb330b2
Bug: T176334
---
M resources/subscribing/ext.centralNotice.display.js
M tests/qunit/subscribing/ext.centralNotice.display.tests.js
2 files changed, 264 insertions(+), 6 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/CentralNotice
refs/changes/08/391208/1
diff --git a/resources/subscribing/ext.centralNotice.display.js
b/resources/subscribing/ext.centralNotice.display.js
index 959b50b..9ced44b 100644
--- a/resources/subscribing/ext.centralNotice.display.js
+++ b/resources/subscribing/ext.centralNotice.display.js
@@ -39,8 +39,11 @@
// For providing a jQuery.Promise to signal when a banner has
loaded
bannerLoadedDeferredObj,
- // Name of a requested banner; see cn.requestBanner(), below
- requestedBannerName = null;
+ // Name of a requested banner; see cn.requestBanner(), below.
+ requestedBannerName = null,
+
+ // Maximum time to delay the record impression call, in
milliseconds
+ MAX_RECORD_IMPRESSION_DELAY = 250;
// TODO: make data.result options explicit via constants
@@ -199,7 +202,44 @@
.prepend( bannerHtml );
}
+ /**
+ * Adds reallyRecordImpression() as the last handler for
cn.recordImpressionDeferredObj,
+ * then resolves.
+ */
+ function resolveRecordImpressionDeferred() {
+ cn.recordImpressionDeferredObj.done( reallyRecordImpression );
+ cn.recordImpressionDeferredObj.resolve();
+ }
+
function recordImpression() {
+ var timeout,
+ timeoutHasRun = false;
+
+ if ( cn.recordImpressionDelayPromises.length === 0 ) {
+ reallyRecordImpression();
+ return;
+ }
+
+ // If there are promises in cn.recordImpressionDelayPromises,
then
+ // cn.recordImpressionDeferredObj (used in
resolveRecordImpressionDeferred())
+ // should already have been set.
+
+ timeout = setTimeout( function() {
+ timeoutHasRun = true;
+ resolveRecordImpressionDeferred();
+ }, MAX_RECORD_IMPRESSION_DELAY );
+
+ // This function can only run once, so checking that the
timeout hasn't run yet
+ // should be sufficient to prevent extra record impression
calls.
+ $.when.apply( $, cn.recordImpressionDelayPromises ).always(
function () {
+ if ( !timeoutHasRun ) {
+ clearTimeout( timeout );
+ resolveRecordImpressionDeferred();
+ }
+ } );
+ }
+
+ function reallyRecordImpression() {
var state = cn.internal.state,
url;
@@ -321,7 +361,7 @@
} else {
// Otherwise, use a random number and banner weights to
choose from among
- // banners available to the user in this campiagn, in
this bucket. (Most
+ // banners available to the user in this campaign, in
this bucket. (Most
// of the time, there's only one.)
banner = chooser.chooseBanner(
campaign,
@@ -343,7 +383,7 @@
return;
}
- // Pass more info about following banner selection
+ // Pass more info following banner selection
state.setBanner( banner );
if ( cn.kvStore ) {
@@ -470,6 +510,21 @@
processAfterBannerFetch();
},
+
+ /**
+ * Promises to delay the record impression call, if possible;
see
+ * cn.requestRecordImpressionDelay(), below. Only exposed for
use in tests.
+ *
+ * @private
+ */
+ recordImpressionDelayPromises: [],
+
+ /**
+ * For providing a jQuery.Promise to signal when the record
impression call is
+ * about to be sent. (Value will be set to a new deferred
object only as needed.)
+ * @private
+ */
+ recordImpressionDeferredObj: null,
/**
* Attachment point for other objects in this module that are
not meant
@@ -657,6 +712,24 @@
},
/**
+ * Request that, if possible, the record impression call be
delayed until a
+ * promise is resolved. If the promise does not resolve before
+ * MAX_RECORD_IMPRESSION_DELAY milliseconds after the banner is
injected,
+ * the call will be made in any case.
+ *
+ * Returns another promise that will resolve immediately before
the record
+ * impression call is made.
+ *
+ * @param {jquery.Promise} promise
+ * @returns {jquery.Promise}
+ */
+ requestRecordImpressionDelay: function ( promise ) {
+ cn.recordImpressionDelayPromises.push( promise );
+ cn.recordImpressionDeferredObj =
cn.recordImpressionDeferredObj || $.Deferred();
+ return cn.recordImpressionDeferredObj.promise();
+ },
+
+ /**
* Get the value of a property used in campaign/banner
selection and
* display, and for recording the results of that process.
*/
diff --git a/tests/qunit/subscribing/ext.centralNotice.display.tests.js
b/tests/qunit/subscribing/ext.centralNotice.display.tests.js
index f0a858c..03e288e 100644
--- a/tests/qunit/subscribing/ext.centralNotice.display.tests.js
+++ b/tests/qunit/subscribing/ext.centralNotice.display.tests.js
@@ -6,6 +6,7 @@
realGeoIP = mw.geoIP,
realBucketCookie = $.cookie( 'CN' ),
realHideCookie = $.cookie( 'centralnotice_hide_fundraising' ),
+ realSendBeacon = navigator.sendBeacon,
bannerData = {
bannerName: 'test_banner',
campaign: 'test_campaign',
@@ -154,10 +155,16 @@
return deferred.promise();
}
};
+
+ // Reset record impression deferred object and array of
promises for delaying
+ // record impression call
+ mw.centralNotice.recordImpressionDeferredObj = null;
+ mw.centralNotice.recordImpressionDelayPromises = [];
},
teardown: function () {
$.ajax = realAjax;
mw.geoIP = realGeoIP;
+ navigator.sendBeacon = realSendBeacon;
$.cookie( 'centralnotice_hide_fundraising',
realHideCookie, { path: '/' } );
$.cookie( 'CN', realBucketCookie, { path: '/' } );
mw.centralNotice.internal.state.data = {};
@@ -177,6 +184,184 @@
assert.equal( $( 'div#test_banner' ).length, 1 );
} );
+ /**
+ * Create the required state in CN for the record impression call to
occur. The first
+ * campaign in choiceData2Campaigns will be chosen.
+ */
+ function setupForRecordImpressionCall() {
+
+ // Request 100% sample rate for record impression
+
mw.centralNotice.internal.state.urlParams.recordImpressionSampleRate = 1;
+
+ // Use the mock choice data with two campaigns, and force the
first campaign
+ mw.centralNotice.choiceData = choiceData2Campaigns;
+ mw.centralNotice.internal.state.urlParams.randomcampaign = 0.25;
+ mw.centralNotice.chooseAndMaybeDisplay();
+ }
+
+ QUnit.test( 'call record impression', function ( assert ) {
+ assert.expect( 2 );
+
+ // Mock navigator.sendBeacon to capture calls and check data
points sent
+ navigator.sendBeacon = function ( urlString ) {
+
+ // mediawiki.Uri is already a dependency of
ext.centralNotice.display
+ var url = new mw.Uri( urlString );
+ assert.strictEqual( url.query.campaign, 'campaign1',
'record impression campaign' );
+ assert.strictEqual( url.query.banner, 'banner1',
'record impression banner' );
+ };
+
+ setupForRecordImpressionCall();
+
+ // Call reallyInsertBanner() instead of insertBanner() to avoid
the async
+ // DOM-waiting of the latter. This triggers the call to record
impression.
+ mw.centralNotice.reallyInsertBanner( bannerData );
+ } );
+
+ QUnit.test( 'delay record impression call and register tests', function
( assert ) {
+ var deferred = $.Deferred(),
+ recordImpresionPromise,
+ signalTestDone = assert.async();
+
+ assert.expect( 3 );
+
+ // Mock navigator.sendBeacon to check record impression doesn't
fire early
+ navigator.sendBeacon = function () {
+ // If we get here, it's a failure
+ assert.ok( false, 'record impression waits, as
requested' );
+ };
+
+ setupForRecordImpressionCall();
+
+ // Request a delay and capture the promise that should run
right before the
+ // record impression call
+ recordImpresionPromise =
+ mw.centralNotice.requestRecordImpressionDelay(
deferred.promise() );
+
+ recordImpresionPromise.done( function () {
+ mw.centralNotice.registerTest( 'test_test' );
+ } );
+
+ // Call reallyInsertBanner() instead of insertBanner() to avoid
the async
+ // DOM-waiting of the latter. This doesn't trigger the call to
record impression
+ // yet because we requested a delay.
+ mw.centralNotice.reallyInsertBanner( bannerData );
+
+ // Mock navigator.sendBeacon to capture calls and check data
points sent
+ navigator.sendBeacon = function ( urlString ) {
+ var url = new mw.Uri( urlString );
+ assert.strictEqual( url.query.campaign, 'campaign1',
'record impression campaign' );
+ assert.strictEqual( url.query.banner, 'banner1',
'record impression banner' );
+
+ assert.strictEqual(
+ url.query.testIdentifiers,
+ 'test_test',
+ 'record impression test identifier'
+ );
+
+ signalTestDone();
+ };
+
+ // Resolve the promise to let the record impression call go
ahead
+ deferred.resolve();
+ } );
+
+ QUnit.test( 'record impression timeout and register tests', function (
assert ) {
+ var recordImpresionPromise,
+ start = new Date().getTime(),
+ MAX_RECORD_IMPRESSION_DELAY = 250, // Coordinate with
ext.centralnotice.display.js
+ signalTestDone = assert.async();
+
+ assert.expect( 4 );
+ setupForRecordImpressionCall();
+
+ // Request a delay and capture the promise that should run
right before the
+ // record impression call
+ recordImpresionPromise =
+ mw.centralNotice.requestRecordImpressionDelay(
$.Deferred().promise() );
+
+ recordImpresionPromise.done( function () {
+ mw.centralNotice.registerTest( 'test_timeouted_test' );
+ } );
+
+ // Mock navigator.sendBeacon to capture calls and check time
and data points sent
+ navigator.sendBeacon = function ( urlString ) {
+ var url = new mw.Uri( urlString ),
+ delay = new Date().getTime() - start;
+
+ assert.strictEqual( url.query.campaign, 'campaign1',
'record impression campaign' );
+ assert.strictEqual( url.query.banner, 'banner1',
'record impression banner' );
+
+ // 50 ms leewway is bit arbitrary
+ assert.ok(
+ ( delay > MAX_RECORD_IMPRESSION_DELAY - 50 ) &&
+ ( delay < MAX_RECORD_IMPRESSION_DELAY + 50 ),
+ 'record impression called by timeout'
+ );
+
+ assert.strictEqual(
+ url.query.testIdentifiers,
+ 'test_timeouted_test',
+ 'record impression test identifier'
+ );
+
+ signalTestDone();
+ };
+
+ // Call reallyInsertBanner() instead of insertBanner() to avoid
the async
+ // DOM-waiting of the latter. This starts the record impression
timeout.
+ mw.centralNotice.reallyInsertBanner( bannerData );
+
+ // We don't resolve the promise to delay record impression. The
call should
+ // be made by MAX_RECORD_IMPRESSION_DELAY milliseconds (give or
take a bit).
+ } );
+
+ QUnit.test( 'record impression called only once', function ( assert ) {
+ var deferred = $.Deferred(),
+ MAX_RECORD_IMPRESSION_DELAY = 250, // Coordinate with
ext.centralnotice.display.js
+ recordImpresionPromise,
+ signalTestDone = assert.async();
+
+ assert.expect( 2 );
+ setupForRecordImpressionCall();
+
+ // Request a delay and capture the promise that should run
right before the
+ // record impression call
+ recordImpresionPromise =
+ mw.centralNotice.requestRecordImpressionDelay(
deferred.promise() );
+
+ // Mock navigator.sendBeacon to capture calls
+ navigator.sendBeacon = function () {
+ assert.ok( true, 'record impression called once' );
+
+ // Re-mock to fail test if called again
+ navigator.sendBeacon = function () {
+ assert.ok( false, 'record impression not called
twice' );
+ };
+ };
+
+ // Call reallyInsertBanner() instead of insertBanner() to avoid
the async
+ // DOM-waiting of the latter. This starts the record impression
timeout.
+ mw.centralNotice.reallyInsertBanner( bannerData );
+
+ // Set another timeout to resolve the promise requesting a
delay after the timeout
+ // to call record impression has run. By starting this timeout
after the record
+ // impression timeout (above) and making it a bit longer, we
ensure it runs second.
+ setTimeout( function () {
+ assert.equal(
+ recordImpresionPromise.state(),
+ 'resolved',
+ 'record impression call was from timeout'
+ );
+
+ // Resolve the promise that delays the call only up
until the timeout, to
+ // test that record impression isn't called a second
time.
+ deferred.resolve();
+ }, MAX_RECORD_IMPRESSION_DELAY + 1 );
+
+ setTimeout( signalTestDone, MAX_RECORD_IMPRESSION_DELAY + 2 );
+ } );
+
QUnit.test( 'banner= override param', function ( assert ) {
assert.expect( 2 );
mw.centralNotice.internal.state.urlParams.banner =
'test_banner';
@@ -193,7 +378,7 @@
assert.expect( 2 );
mw.centralNotice.choiceData = choiceData2Campaigns;
- // Get the first banner
+ // Get the first campaign
mw.centralNotice.internal.state.urlParams.randomcampaign = 0.25;
$.ajax = function ( params ) {
@@ -202,7 +387,7 @@
};
mw.centralNotice.chooseAndMaybeDisplay();
- // Get the second banner
+ // Get the second campaign
mw.centralNotice.internal.state.urlParams.randomcampaign = 0.75;
$.ajax = function ( params ) {
--
To view, visit https://gerrit.wikimedia.org/r/391208
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ida5e777d435d157cfcf6ead7728b35b6beb330b2
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/CentralNotice
Gerrit-Branch: wmf_deploy
Gerrit-Owner: AndyRussG <andrew.green...@gmail.com>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
