golem
/
MinervaNeue
9
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

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
This commit is contained in:
Stephen Niedzielski 2018-08-13 16:54:24 -05:00 committed by Jdlrobson
parent 42481de7c7
commit 22b2f0fd7c
6 changed files with 166 additions and 90 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
includes/MinervaHooks.php
View File

@ -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',

33
resources/skins.minerva.scripts/AB.js
View File

@ -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
};
}

114
resources/skins.minerva.scripts/cleanuptemplates.js
View File

@ -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
}

97
resources/skins.minerva.scripts/pageIssueLogger.js Normal file
View File

@ -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<Schema:PageIssues> 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<Schema:PageIssues> 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 ) );

1
skin.json
View File

@ -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",

10
tests/qunit/skins.minerva.scripts/test_AB.js
View File

@ -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(