AndyRussG has uploaded a new change for review. https://gerrit.wikimedia.org/r/284934
Change subject: [Draft] Use LocalStorage for impression diet ...................................................................... [Draft] Use LocalStorage for impression diet We now only fall back to cookies for campaigns whose category is in $wgCentralNoticeCategoriesUsingLegacy. Bug: T132639 Change-Id: I1f4dc2361765a16856038f864ac318bf9d9a9d12 --- M resources/subscribing/ext.centralNotice.impressionDiet.js 1 file changed, 63 insertions(+), 67 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/CentralNotice refs/changes/34/284934/1 diff --git a/resources/subscribing/ext.centralNotice.impressionDiet.js b/resources/subscribing/ext.centralNotice.impressionDiet.js index 06e4c49..4436eaa 100644 --- a/resources/subscribing/ext.centralNotice.impressionDiet.js +++ b/resources/subscribing/ext.centralNotice.impressionDiet.js @@ -23,16 +23,13 @@ * * Flow chart: https://commons.wikimedia.org/wiki/File:CentralNotice_-_wait_cookie_code_flow.png * (FIXME: update ^^ with new parameter names) - * - * TODO: Deprecate cookies and switch everything to KV storage. */ ( function ( $, mw ) { 'use strict'; var cn = mw.centralNotice, mixin = new cn.Mixin( 'impressionDiet' ), - useCookies, - identifier, + useCookies, identifier, /** * Object with data used to determine whether to hide the banner @@ -51,66 +48,72 @@ COUNTS_STORAGE_TTL = 365; mixin.setPreBannerHandler( function( mixinParams ) { - var forceFlag = mw.util.getParamValue( 'force' ), - hide; - - // Only use cookies for certain campaigns on the legacy track - // Do this here since it's needed for storageAvailable() (below) - useCookies = cn.getDataProperty( 'campaignCategoryUsesLegacy' ); + var hide; // URL forced a banner, or banner was hidden already - if ( forceFlag || cn.isBannerCanceled() ) { + if ( mw.util.getParamValue( 'force' ) || cn.isBannerCanceled() ) { return; } - if ( !storageAvailable() ) { - // Hide the banner if we have no way to store counts. - hide = 'waitnostorage'; + // Check if and how we can store counts + // If we can use LocalStorage, do so + if ( cn.kvStore.isAvailable() ) { + useCookies = false; + + // Otherwise, if allowed for this campaign category (usually FR), try + // falling back to cookies + } else if ( + cn.getDataProperty( 'campaignCategoryUsesLegacy' ) && + cn.cookiesEnabled() ) + + useCookies = true; + + // Oh well... no options for storing stuff, so hide banner and bow out } else { + cn.cancelBanner( 'waitnostorage' ); + return; + } - // Normal code path: a means of storing counts is available + identifier = mixinParams.cookieName; // TODO change param name + counts = getCounts(); - identifier = mixinParams.cookieName; - counts = getCounts(); + 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 ( 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 ( mixinParams.restartCycleDelay !== 0 ) { - // Begin a new cycle by clearing counters. - counts.waitCount = 0; - counts.waitSeenCount = 0; - } + if ( mixinParams.restartCycleDelay !== 0 ) { + // Begin a new cycle by clearing counters. + counts.waitCount = 0; + counts.waitSeenCount = 0; } + } - // Compare counts against campaign settings and decide whether to - // show a banner + // Compare counts against campaign settings and decide whether to + // show a banner - if ( counts.waitSeenCount < mixinParams.maximumSeen ) { - // You haven't seen the maximum count of banners per cycle! + if ( counts.waitSeenCount < mixinParams.maximumSeen ) { + // You haven't seen the maximum count of banners per cycle! - if ( counts.waitCount < mixinParams.skipInitial ) { - // Skip initial impressions. - hide = 'waitimps'; - // TODO: rename skippedThisCycle - counts.waitCount += 1; - } else { - // Show a banner--you win! - hide = false; - } + if ( counts.waitCount < mixinParams.skipInitial ) { + // Skip initial impressions. + hide = 'waitimps'; + // TODO: rename skippedThisCycle + counts.waitCount += 1; } else { - // Wait for the next cycle to begin. - hide = 'waitdate'; + // Show a banner--you win! + hide = false; } + } else { + // Wait for the next cycle to begin. + hide = 'waitdate'; } if ( hide ) { // Hide based on the results. - cn.internal.state.cancelBanner( hide ); + cn.cancelBanner( hide ); } else { // Count shown impression. // TODO: rename seenThisCycle @@ -131,10 +134,6 @@ storeCounts( counts ); } ); - function storageAvailable() { - return useCookies ? cn.cookiesEnabled() : cn.kvStore.isAvailable(); - } - function getZeroedCounts() { return { seenCount: 0, @@ -142,6 +141,11 @@ waitUntil: 0, waitSeenCount: 0 }; + } + + function removeCookies() { + $.removeCookie( identifier, { path: '/' } ); + $.removeCookie( identifier + WAIT_COOKIE_SUFFIX, { path: '/' } ); } /** @@ -152,8 +156,16 @@ var rawCookie, rawWaitCookie, waitData, kvStoreCounts; - // Reset counters on demand + // Reset counters on demand (for testing) if ( mw.util.getParamValue( 'reset' ) === '1' ) { + + // In this case, if we're not using cookies, but there happen to + // be cookies on the client, we want to get rid of them, to prevent + // ending up with counts in both cookies and LocalStorage + if ( !useCookes && identifier ) { + removeCookies(); + } + return getZeroedCounts(); } @@ -171,8 +183,7 @@ // both cookies. In this case, counts will be stored in the KV store // later on. if ( rawCookie && !useCookies ) { - $.removeCookie( identifier, { path: '/' } ); - $.removeCookie( identifier + WAIT_COOKIE_SUFFIX, { path: '/' } ); + removeCookies(); } // If there was a cookie, no matter what, we get counts from there @@ -193,26 +204,13 @@ // Handling cases where there was an identifier but no cookie - // Because of a previous error deployed to WMF production, there - // may be clients that are supposed to use cookies but are using - // the KV store. For only those clients, we'll keep using the - // KV store, rather than attempting a reverse migration. So, no - // matter what, if there's an identifier, we check the KV store. kvStoreCounts = cn.kvStore.getItem( IMPRESSION_DIET_KV_STORE_KEY + '_' + identifier, cn.kvStore.contexts.GLOBAL ); - // If we are supposed to use cookies but have a KV store value, - // keep using that (in storeCounts(), below). - if ( kvStoreCounts && useCookies ) { - useCookies = false; - } - // If we have an identifier, didn't get a cookie value (above), - // then return either what was in the KV store, or zeroed counts, - // regardless of whether we are supposed to use cookies (see - // comment above about error deployed to WMF production). + // then return either what was in the KV store, or zeroed counts return kvStoreCounts || getZeroedCounts(); } @@ -238,8 +236,6 @@ /* * Store updated counts - * - * TODO: Deprecate legacy cookie-based storage */ function storeCounts( counts ) { -- To view, visit https://gerrit.wikimedia.org/r/284934 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I1f4dc2361765a16856038f864ac318bf9d9a9d12 Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/extensions/CentralNotice Gerrit-Branch: master Gerrit-Owner: AndyRussG <andrew.green...@gmail.com> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
