From 22b2f0fd7c9db8fde5024c9f0fc20dd278829613 Mon Sep 17 00:00:00 2001 From: Stephen Niedzielski Date: Mon, 13 Aug 2018 16:54:24 -0500 Subject: [PATCH] Hygiene: move page issues A/B test logging to file Refactor the page issues A/B test logging implementation to a distinct new file that only has the responsibility of tracking. T191528 is referenced in this commit as I was having difficulty answering the feedback and bugs reported in the current implementation without working through and restructuring the flow as I understood it. This refactor is merely a byproduct artifact of that effort to focus on the parsing and presentation responsibilities. Bug: T191528 Change-Id: If547a0a67fbc9a532f834fe374abf668309e73df --- includes/MinervaHooks.php | 1 + resources/skins.minerva.scripts/AB.js | 33 ++++- .../skins.minerva.scripts/cleanuptemplates.js | 114 +++++------------- .../skins.minerva.scripts/pageIssueLogger.js | 97 +++++++++++++++ skin.json | 1 + tests/qunit/skins.minerva.scripts/test_AB.js | 10 +- 6 files changed, 166 insertions(+), 90 deletions(-) create mode 100644 resources/skins.minerva.scripts/pageIssueLogger.js diff --git a/includes/MinervaHooks.php b/includes/MinervaHooks.php index 2fbfad9..561580e 100644 --- a/includes/MinervaHooks.php +++ b/includes/MinervaHooks.php @@ -83,6 +83,7 @@ class MinervaHooks { 'targets' => [ 'mobile', 'desktop' ], 'scripts' => [ // additional scaffolding (minus initialisation scripts) + 'resources/skins.minerva.scripts/pageIssueLogger.js', 'resources/skins.minerva.scripts/pageIssueParser.js', 'resources/skins.minerva.scripts/DownloadIcon.js', 'resources/skins.minerva.scripts/AB.js', diff --git a/resources/skins.minerva.scripts/AB.js b/resources/skins.minerva.scripts/AB.js index fbe03c0..101d1a8 100644 --- a/resources/skins.minerva.scripts/AB.js +++ b/resources/skins.minerva.scripts/AB.js @@ -5,6 +5,12 @@ * predefined bucket ("control", "A", "B") and starts an AB-test. */ ( function ( M, mwExperiments ) { + var bucket = { + CONTROL: 'control', + A: 'A', + B: 'B' + }; + /** * Buckets users based on params and exposes an `isEnabled` and `getBucket` method. * @param {Object} config Configuration object for AB test. @@ -14,7 +20,7 @@ * @constructor */ function AB( config ) { - var CONTROL_BUCKET = 'control', + var testName = config.testName, samplingRate = config.samplingRate, sessionId = config.sessionId, @@ -29,23 +35,40 @@ }; /** - * Gets the users AB-test bucket - * @return {string} AB-test bucket, CONTROL_BUCKET by default, "A" or "B" buckets otherwise. + * Gets the users AB-test bucket. + * + * A boolean instead of an enum is usually a code smell. However, the nature of A/B testing is + * to compare an A group's performance to a B group's so a boolean seems natural, even in the + * long term, and preferable to showing bucketing encoding ("A", "B", "control") to callers + * which is necessary if getBucket(). The downside is that now two functions exist where one + * would suffice. + * + * @return {string} AB-test bucket, bucket.CONTROL_BUCKET by default, bucket.A or bucket.B + * buckets otherwise. */ function getBucket() { return mwExperiments.getBucket( test, sessionId ); } + function isA() { + return getBucket() === bucket.A; + } + + function isB() { + return getBucket() === bucket.B; + } + /** * Checks whether or not a user is in the AB-test, * @return {boolean} */ function isEnabled() { - return getBucket() !== CONTROL_BUCKET; + return getBucket() !== bucket.CONTROL; // I.e., `isA() || isB()`; } return { - getBucket: getBucket, + isA: isA, + isB: isB, isEnabled: isEnabled }; } diff --git a/resources/skins.minerva.scripts/cleanuptemplates.js b/resources/skins.minerva.scripts/cleanuptemplates.js index 389592b..c05bce1 100644 --- a/resources/skins.minerva.scripts/cleanuptemplates.js +++ b/resources/skins.minerva.scripts/cleanuptemplates.js @@ -1,12 +1,9 @@ ( function ( M, $ ) { var AB = M.require( 'skins.minerva.scripts/AB' ), - util = M.require( 'mobile.startup/util' ), allIssues = {}, KEYWORD_ALL_SECTIONS = 'all', config = mw.config, user = mw.user, - track = mw.track, - trackSubscribe = mw.trackSubscribe, ACTION_EDIT = config.get( 'wgAction' ) === 'edit', NS_MAIN = 0, NS_TALK = 1, @@ -14,6 +11,7 @@ CURRENT_NS = config.get( 'wgNamespaceNumber' ), allPageIssuesSeverity, Icon = M.require( 'mobile.startup/Icon' ), + pageIssueLogger = M.require( 'skins.minerva.scripts/pageIssueLogger' ), pageIssueParser = M.require( 'skins.minerva.scripts/pageIssueParser' ), CleanupOverlay = M.require( 'mobile.issues/CleanupOverlay' ), // setup ab test @@ -24,78 +22,11 @@ samplingRate: ( CURRENT_NS === NS_MAIN ) ? config.get( 'wgMinervaABSamplingRate', 0 ) : 0, sessionId: user.sessionId() } ), - // Set bucket - isInGroupB = abTest.getBucket() === 'B', - READING_DEPTH_BUCKET_KEYS = { - A: 'page-issues-a_sample', - B: 'page-issues-b_sample' - }, - READING_DEPTH_BUCKET = READING_DEPTH_BUCKET_KEYS[ abTest.getBucket() ], - PAGE_ISSUES_EVENT_DATA = { - pageTitle: config.get( 'wgTitle' ), - namespaceId: CURRENT_NS, - pageIdSource: config.get( 'wgArticleId' ), - issuesVersion: isInGroupB ? 'new2018' : 'old', - isAnon: user.isAnon(), - editCountBucket: getUserEditBuckets(), - pageToken: user.generateRandomSessionId() + - Math.floor( mw.now() ).toString(), - sessionToken: user.sessionId() - }; + newTreatmentEnabled = abTest.isB(); - // start logging if user is in group A or B. - if ( abTest.isEnabled() ) { - // intermediary event bus that extends the event data before being passed to event-logging. - trackSubscribe( 'minerva.PageIssuesAB', function ( topic, data ) { - - // enable logging for pages that have issues. - if ( !getIssues( KEYWORD_ALL_SECTIONS ).length ) { - return; - } - - // define all issues severity once for all subsequent events - if ( !allPageIssuesSeverity ) { - allPageIssuesSeverity = getIssues( KEYWORD_ALL_SECTIONS ).map( formatPageIssuesSeverity ); - } - // if event data does not set issuesSeverity, send `allPageIssuesSeverity`. - if ( !data.issuesSeverity ) { - data.issuesSeverity = allPageIssuesSeverity; - } - - // Log readingDepth schema.(ReadingDepth is guarded against multiple enables). - // See https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/WikimediaEvents/+/437686/ - track( 'wikimedia.event.ReadingDepthSchema.enable', READING_DEPTH_BUCKET ); - // Log PageIssues schema. - track( 'wikimedia.event.PageIssues', util.extend( {}, PAGE_ISSUES_EVENT_DATA, data ) ); - } ); - } - - /** - * converts user edit count into a predefined string - * @return {string} - */ - function getUserEditBuckets() { - var editCount = config.get( 'wgUserEditCount', 0 ); - - if ( editCount === 0 ) { return '0 edits'; } - if ( editCount < 5 ) { return '1-4 edits'; } - if ( editCount < 100 ) { return '5-99 edits'; } - if ( editCount < 1000 ) { return '100-999 edits'; } - if ( editCount >= 1000 ) { return '1000+ edits'; } - - // This is unlikely to ever happen. If so, we'll want to cast to a string - // that is not accepted and allow EventLogging to complain - // about invalid events so we can investigate. - return 'error (' + editCount + ')'; - } - - /** - * Log data to the PageIssuesAB test schema - * @param {Object} data to log - * @ignore - */ - function log( data ) { - track( 'minerva.PageIssuesAB', data ); + function isLoggingRequired( pageIssues ) { + // No logging necessary when the A/B test is disabled (control group). + return abTest.isEnabled() && pageIssues.length; } /** @@ -221,7 +152,7 @@ } $metadata.click( function () { var pageIssue = pageIssueParser.parse( this ); - log( { + pageIssueLogger.log( { action: 'issueClicked', issuesSeverity: [ pageIssue.severity ] } ); @@ -233,7 +164,7 @@ // In group B, we link to all issues no matter where the banner is. $link.attr( 'href', '#/issues/' + KEYWORD_ALL_SECTIONS ); $link.click( function () { - log( { + pageIssueLogger.log( { action: 'issueClicked', // empty array is passed for 'old' treatment. issuesSeverity: [] @@ -300,11 +231,11 @@ headingText = ACTION_EDIT ? mw.msg( 'edithelp' ) : getNamespaceHeadingText( CURRENT_NS ), $lead = page.getLeadSectionElement(), issueOverlayShowAll = CURRENT_NS === NS_CATEGORY || CURRENT_NS === NS_TALK || ACTION_EDIT || !$lead, - inline = isInGroupB && CURRENT_NS === 0, + inline = newTreatmentEnabled && CURRENT_NS === 0, $container = $( '#bodyContent' ); // set A-B test class. - $( 'html' ).addClass( isInGroupB ? 'issues-group-B' : 'issues-group-A' ); + $( 'html' ).addClass( newTreatmentEnabled ? 'issues-group-B' : 'issues-group-A' ); if ( ACTION_EDIT ) { // Editor uses different parent element @@ -321,7 +252,7 @@ } else { // parse lead createBanner( $lead, label, 0, inline, overlayManager ); - if ( isInGroupB ) { + if ( newTreatmentEnabled ) { // parse other sections but only in group B. In treatment A no issues are shown for sections. $lead.nextAll( 'h1,h2,h3,h4,h5,h6' ).each( function ( i, headingEl ) { var $headingEl = $( headingEl ), @@ -334,6 +265,18 @@ } } + if ( isLoggingRequired( getIssues( KEYWORD_ALL_SECTIONS ) ) ) { + // Enable logging. + pageIssueLogger.subscribe( + newTreatmentEnabled, + pageIssueLogger.newPageIssueSchemaData( + newTreatmentEnabled, + CURRENT_NS, + getIssues( KEYWORD_ALL_SECTIONS ).map( formatPageIssuesSeverity ) + ) + ); + } + // Setup the overlay route. overlayManager.add( new RegExp( '^/issues/(\\d+|' + KEYWORD_ALL_SECTIONS + ')$' ), function ( section ) { var overlay = new CleanupOverlay( { @@ -345,19 +288,19 @@ } ); // Tracking overlay close event. overlay.on( 'Overlay-exit', function () { - log( { + pageIssueLogger.log( { action: 'modalClose', issuesSeverity: getIssues( section ).map( formatPageIssuesSeverity ) } ); } ); overlay.on( 'link-edit-click', function ( severity ) { - log( { + pageIssueLogger.log( { action: 'modalEditClicked', issuesSeverity: [ severity ] } ); } ); overlay.on( 'link-internal-click', function ( severity ) { - log( { + pageIssueLogger.log( { action: 'modalInternalClicked', issuesSeverity: [ severity ] } ); @@ -366,7 +309,7 @@ } ); // Tracking pageLoaded event (technically, "issues" loaded). - log( { + pageIssueLogger.log( { action: 'pageLoaded', issuesSeverity: allPageIssuesSeverity } ); @@ -374,7 +317,10 @@ M.define( 'skins.minerva.scripts/cleanuptemplates', { init: initPageIssues, - log: log, + // The logger requires initialization (subscription). Ideally, the logger would be initialized + // and passed to initPageIssues() by the client. Since it's not, expose a log method and hide + // the subscription call in cleanuptemplates. + log: pageIssueLogger.log, test: { createBanner: createBanner } diff --git a/resources/skins.minerva.scripts/pageIssueLogger.js b/resources/skins.minerva.scripts/pageIssueLogger.js new file mode 100644 index 0000000..5cea5c4 --- /dev/null +++ b/resources/skins.minerva.scripts/pageIssueLogger.js @@ -0,0 +1,97 @@ +( function ( M, mwConfig, mwNow, mwTrack, mwTrackSubscribe, mwUser ) { + var + util = M.require( 'mobile.startup/util' ), + EVENT_PAGE_ISSUE_LOG = 'minerva.PageIssuesAB'; + + /** + * @param {boolean} newTreatmentEnabled + * @param {number} namespaceId The namespace for the page that has issues. + * @param {string[]} pageIssueSeverities An array of PageIssue severities. + * @return {Object} A Partial Object meant to be mixed with track data. + */ + function newPageIssueSchemaData( newTreatmentEnabled, namespaceId, pageIssueSeverities ) { + return { + pageTitle: mwConfig.get( 'wgTitle' ), + namespaceId: namespaceId, + pageIdSource: mwConfig.get( 'wgArticleId' ), + issuesVersion: bucketToVersion( newTreatmentEnabled ), + issuesSeverity: pageIssueSeverities, + isAnon: mwUser.isAnon(), + editCountBucket: getUserEditBuckets(), + pageToken: mwUser.generateRandomSessionId() + Math.floor( mwNow() ).toString(), + sessionToken: mwUser.sessionId() + }; + } + + /** + * Enable tracking. + * @param {boolean} newTreatmentEnabled + * @param {Object} pageIssueSchemaData A Partial Object that will be mixed with + * with track data. + * @return {void} + */ + function subscribe( newTreatmentEnabled, pageIssueSchemaData ) { + // intermediary event bus that extends the event data before being passed to event-logging. + mwTrackSubscribe( EVENT_PAGE_ISSUE_LOG, function ( topic, data ) { + var mixedData = util.extend( {}, pageIssueSchemaData, data ); + + // Log readingDepth schema.(ReadingDepth is guarded against multiple enables). + // See https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/WikimediaEvents/+/437686/ + mwTrack( 'wikimedia.event.ReadingDepthSchema.enable', bucketToGroup( newTreatmentEnabled ) ); + // Log PageIssues schema. + mwTrack( 'wikimedia.event.PageIssues', mixedData ); + } ); + } + + /** + * @param {boolean} newTreatmentEnabled + * @return {string} The page issues group associated with the treatment bucket. + */ + function bucketToGroup( newTreatmentEnabled ) { + return newTreatmentEnabled ? 'page-issues-b_sample' : 'page-issues-a_sample'; + } + + /** + * @param {boolean} newTreatmentEnabled + * @return {string} The page issues version associated with the treatment bucket. + */ + function bucketToVersion( newTreatmentEnabled ) { + return newTreatmentEnabled ? 'new2018' : 'old'; + } + + /** + * Converts user edit count into a predefined string. Note: these buckets have *nothing* to do + * with A/B bucketing. + * @return {string} + */ + function getUserEditBuckets() { + var editCount = mwConfig.get( 'wgUserEditCount', 0 ); + + if ( editCount === 0 ) { return '0 edits'; } + if ( editCount < 5 ) { return '1-4 edits'; } + if ( editCount < 100 ) { return '5-99 edits'; } + if ( editCount < 1000 ) { return '100-999 edits'; } + if ( editCount >= 1000 ) { return '1000+ edits'; } + + // This is unlikely to ever happen. If so, we'll want to cast to a string + // that is not accepted and allow EventLogging to complain + // about invalid events so we can investigate. + return 'error (' + editCount + ')'; + } + + /** + * Log data to the PageIssuesAB test schema. It's safe to call this function prior to + * subscription. + * @param {Object} data to log + * @return {void} + */ + function log( data ) { + mwTrack( EVENT_PAGE_ISSUE_LOG, data ); + } + + M.define( 'skins.minerva.scripts/pageIssueLogger', { + newPageIssueSchemaData: newPageIssueSchemaData, + subscribe: subscribe, + log: log + } ); +}( mw.mobileFrontend, mw.config, mw.now, mw.track, mw.trackSubscribe, mw.user ) ); diff --git a/skin.json b/skin.json index a9ff7e0..773ef90 100644 --- a/skin.json +++ b/skin.json @@ -418,6 +418,7 @@ "scripts": [ "resources/skins.minerva.scripts/preInit.js", "resources/skins.minerva.scripts/DownloadIcon.js", + "resources/skins.minerva.scripts/pageIssueLogger.js", "resources/skins.minerva.scripts/pageIssueParser.js", "resources/skins.minerva.scripts/AB.js", "resources/skins.minerva.scripts/cleanuptemplates.js", diff --git a/tests/qunit/skins.minerva.scripts/test_AB.js b/tests/qunit/skins.minerva.scripts/test_AB.js index ee3c79d..5f15017 100644 --- a/tests/qunit/skins.minerva.scripts/test_AB.js +++ b/tests/qunit/skins.minerva.scripts/test_AB.js @@ -24,7 +24,15 @@ for ( i = 0; i < maxUsers; i++ ) { config = util.extend( {}, defaultConfig, { sessionId: mw.user.generateRandomSessionId() } ); bucketingTest = new AB( config ); - userBuckets[ bucketingTest.getBucket() ] += 1; + if ( bucketingTest.isA() ) { + ++userBuckets.A; + } else if ( bucketingTest.isB() ) { + ++userBuckets.B; + } else if ( !bucketingTest.isEnabled() ) { + ++userBuckets.control; + } else { + throw new Error( 'Unknown bucket!' ); + } } assert.strictEqual(