diff --git a/includes/MinervaHooks.php b/includes/MinervaHooks.php index 2ede615..b0ad181 100644 --- a/includes/MinervaHooks.php +++ b/includes/MinervaHooks.php @@ -91,7 +91,6 @@ class MinervaHooks { // additional scaffolding (minus initialisation scripts) 'tests/qunit/skins.minerva.scripts/stubs.js', - 'resources/skins.minerva.scripts/pageIssuesLogger.js', 'resources/skins.minerva.scripts/pageIssuesParser.js', 'resources/skins.minerva.scripts/DownloadIcon.js', 'resources/skins.minerva.scripts/AB.js', @@ -101,7 +100,6 @@ class MinervaHooks { 'tests/qunit/skins.minerva.scripts/DownloadIcon.test.js', 'tests/qunit/skins.minerva.scripts/pageIssuesParser.test.js', 'tests/qunit/skins.minerva.scripts/AB.test.js', - 'tests/qunit/skins.minerva.scripts/PageIssuesOverlay.test.js', 'tests/qunit/skins.minerva.scripts/pageIssues.test.js', 'tests/qunit/skins.minerva.notifications.badge/NotificationBadge.test.js' ], diff --git a/resources/skins.minerva.editor/init.js b/resources/skins.minerva.editor/init.js index 6c63fba..6a83e5a 100644 --- a/resources/skins.minerva.editor/init.js +++ b/resources/skins.minerva.editor/init.js @@ -2,7 +2,6 @@ var router = require( 'mediawiki.router' ), - issues = M.require( 'skins.minerva.scripts/pageIssues' ), overlayManager = M.require( 'skins.minerva.scripts/overlayManager' ), loader = M.require( 'mobile.startup/rlModuleLoader' ), skin = M.require( 'skins.minerva.scripts/skin' ), @@ -32,7 +31,6 @@ */ function onEditLinkClick() { var section = ( new mw.Uri( this.href ) ).query.section || 'all'; - issues.log( { action: 'editClicked' } ); router.navigate( '#/editor/' + section ); // prevent folding section when clicking Edit by stopping propagation return false; diff --git a/resources/skins.minerva.scripts/PageIssuesOverlay.js b/resources/skins.minerva.scripts/PageIssuesOverlay.js index f3418d8..9d47f49 100644 --- a/resources/skins.minerva.scripts/PageIssuesOverlay.js +++ b/resources/skins.minerva.scripts/PageIssuesOverlay.js @@ -13,13 +13,11 @@ * @extends Overlay * * @param {IssueSummary[]} issues list of page issue summaries for display. - * @param {PageIssuesLogger} logger E.g., { log: console.log }. * @param {string} section * @param {number} namespaceID */ - function PageIssuesOverlay( issues, logger, section, namespaceID ) { + function PageIssuesOverlay( issues, section, namespaceID ) { var - options, // Note only the main namespace is expected to make use of section issues, so the // heading will always be minerva-meta-data-issues-section-header regardless of // namespace. @@ -27,24 +25,10 @@ getNamespaceHeadingText( namespaceID ) : mwMsg( 'minerva-meta-data-issues-section-header' ); - this.issues = issues; - this.logger = logger; - this.section = section; - - options = {}; - options.issues = issues; - - // Set default logging data - this.defaultLoggerData = {}; - // In the case of KEYWORD_ALL_SECTIONS all issues are in the overlay and the sectionNumbers - // field should be no different from the default behaviour. - if ( this.section !== KEYWORD_ALL_SECTIONS ) { - this.defaultLoggerData.sectionNumbers = [ this.section ]; - } - options.heading = '' + headingText + ''; - Overlay.call( this, options ); - - this.on( Overlay.EVENT_EXIT, this.onExit.bind( this ) ); + Overlay.call( this, { + issues: issues, + heading: '' + headingText + '' + } ); } OO.mfExtend( PageIssuesOverlay, Overlay, { @@ -54,128 +38,15 @@ */ className: 'overlay overlay-issues', - /** - * @memberof PageIssuesOverlay - * @instance - */ - events: util.extend( {}, Overlay.prototype.events, { - 'click a[href*=redlink]': 'onRedLinkClick', - 'click a:not(.external):not([href*=edit])': 'onInternalClick', - // Only register attempts to edit an existing page (should be the one we are on), - // not internal clicks on redlinks to nonexistent pages: - 'click a[href*="edit"]:not([href*=redlink])': 'onEditClick' - } ), - /** * @memberof PageIssuesOverlay * @instance */ templatePartials: util.extend( {}, Overlay.prototype.templatePartials, { content: mw.template.get( 'skins.minerva.scripts', 'PageIssuesOverlayContent.hogan' ) - } ), - - /** - * Log data via the associated logger, adding sectionNumbers to override the event default - * if applicable. - * @param {Object} data - * @instance - */ - log: function ( data ) { - this.logger.log( util.extend( {}, this.defaultLoggerData, data ) ); - }, - - /** - * Note: an "on enter" state is tracked by the issueClicked log event. - * @return {void} - */ - onExit: function () { - var logData = { - action: 'modalClose', - issuesSeverity: this.issues.map( issueSummaryToSeverity ) - }, - currentSection = this.section; - // When users close the modal, `sectionNumbers` should correlate to each visible issue - // in the modal, provided that this.section is a valid number and not - // `KEYWORD_ALL_SECTIONS`. - if ( this.section !== KEYWORD_ALL_SECTIONS ) { - logData.sectionNumbers = this.issues.map( function () { - return currentSection; - } ); - } - this.log( logData ); - }, - - /** - * Event that is triggered when an internal link inside the overlay is clicked. - * This event will not be triggered if the link contains the edit keyword, - * in which case onEditClick will be - * fired. This is primarily used for instrumenting page issues (see - * https://meta.wikimedia.org/wiki/Schema:PageIssues). - * @param {JQuery.Event} ev - * @memberof PageIssuesOverlay - * @instance - */ - onInternalClick: function ( ev ) { - var severity = parseSeverity( this.$( ev.target ) ); - this.log( { - action: 'modalInternalClicked', - issuesSeverity: [ severity ] - } ); - }, - - /** - * Event that is triggered when a red link (e.g. a link to a page which doesn't exist) - * inside the overlay is clicked. - * @param {JQuery.Event} ev - * @memberof PageIssuesOverlay - * @instance - */ - onRedLinkClick: function ( ev ) { - var severity = parseSeverity( this.$( ev.target ) ); - this.log( { - action: 'modalRedLinkClicked', - issuesSeverity: [ severity ] - } ); - }, - - /** - * Event that is triggered when an edit link inside the overlay is clicked. - * This is primarily - * used for instrumenting page issues (see https://meta.wikimedia.org/wiki/Schema:PageIssues). - * The event will not be triggered in the case of red links. - * See onRedLinkClick for red links. - * @param {JQuery.Event} ev - * @memberof PageIssuesOverlay - * @instance - */ - onEditClick: function ( ev ) { - var severity = parseSeverity( this.$( ev.target ) ); - this.log( { - action: 'modalEditClicked', - issuesSeverity: [ severity ] - } ); - } + } ) } ); - /** - * Obtain severity associated with a given $target node by looking at associated parent node - * (defined by templatePartials, PageIssuesOverlayContent.hogan). - * - * @param {JQuery.Object} $target - * @return {string[]} severity as defined in associated PageIssue - */ - function parseSeverity( $target ) { - return $target.parents( '.issue-notice' ).data( 'severity' ); - } - - /** - * @param {IssueSummary} issue - * @return {string} A PageIssue.severity. - */ - function issueSummaryToSeverity( issue ) { - return issue.severity; - } - /** * Obtain a suitable heading for the issues overlay based on the namespace * @param {number} namespaceID is the namespace to generate heading for diff --git a/resources/skins.minerva.scripts/pageIssues.js b/resources/skins.minerva.scripts/pageIssues.js index 23f7749..63128b7 100644 --- a/resources/skins.minerva.scripts/pageIssues.js +++ b/resources/skins.minerva.scripts/pageIssues.js @@ -1,63 +1,17 @@ ( function ( M, $ ) { - var AB = M.require( 'skins.minerva.scripts/AB' ), - Page = M.require( 'mobile.startup/Page' ), + var Page = M.require( 'mobile.startup/Page' ), allIssues = {}, KEYWORD_ALL_SECTIONS = 'all', config = mw.config, - user = mw.user, NS_MAIN = 0, NS_TALK = 1, NS_CATEGORY = 14, CURRENT_NS = config.get( 'wgNamespaceNumber' ), - pageIssuesLogger = M.require( 'skins.minerva.scripts/pageIssuesLogger' ), pageIssuesParser = M.require( 'skins.minerva.scripts/pageIssuesParser' ), PageIssuesOverlay = M.require( 'skins.minerva.scripts/PageIssuesOverlay' ), - // setup ab test - abTest = new AB( { - testName: 'WME.PageIssuesAB', - // Run AB only on article namespace, otherwise set samplingRate to 0, - // forcing user into control (i.e. ignored/not logged) group. - samplingRate: ( CURRENT_NS === NS_MAIN ) ? config.get( 'wgMinervaABSamplingRate', 0 ) : 0, - sessionId: user.sessionId() - } ), QUERY_STRING_FLAG = mw.util.getParamValue( 'minerva-issues' ), - // Per T204746 a user can request the new treatment regardless of test group - isUserRequestingNewTreatment = QUERY_STRING_FLAG === 'b', - newTreatmentEnabled = abTest.isB() || isUserRequestingNewTreatment; - - function isLoggingRequired( pageIssues ) { - // No logging necessary when the A/B test is disabled (control group). - return !isUserRequestingNewTreatment && abTest.isEnabled() && pageIssues.length; - } - - /** - * Array.reduce callback that returns the severity of page issues. - * In the case that a page-issue is part of a "multiple issues" template, - * returns the maximum severity for that group of issues. - * - * @param {array} formattedArr - the return array containing severities - * @param {IssueSummary} currentItem current IssueSummary object - * @param {number} currentIndex current index of pageIssues - * @param {array} pageIssues array of pageIssues - * - * @return {array} acc - */ - function formatPageIssuesSeverity( formattedArr, currentItem, currentIndex, pageIssues ) { - var lastItem = pageIssues[ currentIndex - 1 ], - issue = currentItem.issue, - lastFormattedIndex = formattedArr.length - 1, - lastFormattedValue = formattedArr[ lastFormattedIndex ]; - // If the last and current item `grouped`, fold the maxSeverity - // of the two items into a single value. - if ( lastItem && lastItem.issue && lastItem.issue.grouped && issue.grouped ) { - formattedArr[ lastFormattedIndex ] = pageIssuesParser.maxSeverity( - [ lastFormattedValue, issue.severity ] - ); - } else { - formattedArr.push( issue.severity ); - } - return formattedArr; - } + // T206179 should update this value to enable it + newTreatmentEnabled = QUERY_STRING_FLAG === 'b'; /** * Create a link element that opens the issues overlay. @@ -138,34 +92,12 @@ $learnMore.appendTo( $metadata.find( '.mbox-text' ) ); } $metadata.click( function () { - var pageIssue = pageIssuesParser.parse( this ); - pageIssuesLogger.log( { - action: 'issueClicked', - issuesSeverity: [ pageIssue.severity ], - sectionNumbers: [ section ] - } ); overlayManager.router.navigate( issueUrl ); return false; } ); } else { $link = createLinkElement( labelText ); $link.attr( 'href', '#/issues/' + section ); - $link.click( function () { - pageIssuesLogger.log( { - action: 'issueClicked', - issuesSeverity: [ - pageIssuesParser.maxSeverity( - getIssues( '0' ) - .map( function ( issue ) { return issue.severity; } ) - ) - ], - // In the old treatment, an issuesClicked event will always be '0' - // as the old treatment is always associated with the lead section and we - // are only sending one maximum severity for all of them. - // An issuesClicked event should only ever be associated with one issue box. - sectionNumbers: [ '0' ] - } ); - } ); if ( $metadata.length ) { $link.insertAfter( $( 'h1#section_0' ) ); $metadata.remove(); @@ -269,39 +201,16 @@ } } - if ( isLoggingRequired( getIssues( KEYWORD_ALL_SECTIONS ) ) ) { - // Enable logging of the PageIssues schema, setting up defaults. - pageIssuesLogger.subscribe( - newTreatmentEnabled, - pageIssuesLogger.newPageIssueSchemaData( - newTreatmentEnabled, - CURRENT_NS, - getIssues( KEYWORD_ALL_SECTIONS ).reduce( formatPageIssuesSeverity, [] ), - getAllIssuesSections( allIssues ) - ) - ); - - // Report that the page has been loaded. - pageIssuesLogger.log( { - action: 'pageLoaded' - } ); - } - // Setup the overlay route. overlayManager.add( new RegExp( '^/issues/(\\d+|' + KEYWORD_ALL_SECTIONS + ')$' ), function ( section ) { return new PageIssuesOverlay( - getIssues( section ), pageIssuesLogger, section, CURRENT_NS ); + getIssues( section ), section, CURRENT_NS ); } ); } M.define( 'skins.minerva.scripts/pageIssues', { init: initPageIssues, - // 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: pageIssuesLogger.log, test: { - formatPageIssuesSeverity: formatPageIssuesSeverity, getAllIssuesSections: getAllIssuesSections, createBanner: createBanner } diff --git a/resources/skins.minerva.scripts/pageIssuesLogger.js b/resources/skins.minerva.scripts/pageIssuesLogger.js deleted file mode 100644 index 9756963..0000000 --- a/resources/skins.minerva.scripts/pageIssuesLogger.js +++ /dev/null @@ -1,114 +0,0 @@ -( function ( M, mwConfig, mwTrack, mwTrackSubscribe, mwUser ) { - var - util = M.require( 'mobile.startup/util' ), - EVENT_PAGE_ISSUE_LOG = 'minerva.PageIssuesAB'; - - /** - * Defines default data for Schema:PageIssues that will be recorded with every event. - * @param {boolean} newTreatmentEnabled - * @param {number} namespaceId The namespace for the page that has issues. - * @param {string[]} pageIssueSeverities An array of PageIssue severities. - * @param {array} pageIssuesSections - * - * @return {Object} A Partial Object meant to be mixed with track data. - */ - function newPageIssueSchemaData( - newTreatmentEnabled, namespaceId, pageIssueSeverities, pageIssuesSections - ) { - return { - pageTitle: mwConfig.get( 'wgTitle' ), - namespaceId: namespaceId, - pageIdSource: mwConfig.get( 'wgArticleId' ), - issuesVersion: bucketToVersion( newTreatmentEnabled ), - issuesSeverity: pageIssueSeverities, - sectionNumbers: pageIssuesSections, - isAnon: mwUser.isAnon(), - editCountBucket: getUserEditBuckets(), - sessionToken: mwUser.sessionId() - }; - } - - /** - * Enable tracking and add page token to every logged event. - * @param {boolean} newTreatmentEnabled - * @param {Object} pageIssueSchemaData A Partial Object that will be mixed - * with track data. - * @return {void} - */ - function subscribe( newTreatmentEnabled, pageIssueSchemaData ) { - // set the page token on the request. - pageIssueSchemaData.pageToken = mw.user.getPageviewToken(); - - // 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 ); - - // Although we use strings inside the issues code (due to the usage of the key word - // `all`) - these need to be numbers to be validated by the schema - mixedData.sectionNumbers = ( mixedData.sectionNumbers || [] ).map( - function ( sectionStr ) { return parseInt( sectionStr, 10 ); } - ); - - // For backwards compatibility with cached WikimediaEvents code - // (remove me in a weeks time!! T207423) - mwTrack( 'wikimedia.event.ReadingDepthSchema.enable', bucketToGroup( newTreatmentEnabled ) ); - // Log readingDepth schema.(ReadingDepth is guarded against multiple enables). - // See https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/WikimediaEvents/+/437686/ - mwTrack( 'wikimedia.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/pageIssuesLogger', { - newPageIssueSchemaData: newPageIssueSchemaData, - subscribe: subscribe, - log: log - } ); -}( mw.mobileFrontend, mw.config, mw.track, mw.trackSubscribe, mw.user ) ); diff --git a/skin.json b/skin.json index 7f0f442..8d134be 100644 --- a/skin.json +++ b/skin.json @@ -158,7 +158,6 @@ } }, "EventLoggingSchemas": { - "PageIssues": 18392542, "WebClientError": 18340282 }, "ResourceModules": { @@ -436,7 +435,6 @@ "resources/skins.minerva.scripts/errorLogging.js", "resources/skins.minerva.scripts/preInit.js", "resources/skins.minerva.scripts/DownloadIcon.js", - "resources/skins.minerva.scripts/pageIssuesLogger.js", "resources/skins.minerva.scripts/pageIssuesParser.js", "resources/skins.minerva.scripts/AB.js", "resources/skins.minerva.scripts/PageIssuesOverlay.js", diff --git a/tests/qunit/skins.minerva.scripts/PageIssuesOverlay.test.js b/tests/qunit/skins.minerva.scripts/PageIssuesOverlay.test.js deleted file mode 100644 index 11c7bac..0000000 --- a/tests/qunit/skins.minerva.scripts/PageIssuesOverlay.test.js +++ /dev/null @@ -1,57 +0,0 @@ -( function ( M ) { - var PageIssuesOverlay = M.require( 'skins.minerva.scripts/PageIssuesOverlay' ); - - QUnit.module( 'Minerva PageIssuesOverlay', { - beforeEach: function () { - this.logger = { - log: this.sandbox.spy() - }; - } - } ); - - QUnit.test( '#log (section=all)', function ( assert ) { - var overlay = new PageIssuesOverlay( [], this.logger, 'all', 0 ); - overlay.onExit(); - assert.strictEqual( this.logger.log.calledOnce, true, 'Logger called once' ); - assert.strictEqual( - this.logger.log.calledWith( { - action: 'modalClose', - issuesSeverity: [] - } ), true, 'sectionNumbers is not set (T202940)' - ); - } ); - - QUnit.test( '#log (section=1)', function ( assert ) { - var overlay = new PageIssuesOverlay( [ - { - severity: 'MEDIUM' - } - ], this.logger, '1', 0 ); - overlay.onExit(); - assert.strictEqual( - this.logger.log.calledWith( { - action: 'modalClose', - issuesSeverity: [ 'MEDIUM' ], - sectionNumbers: [ '1' ] - } ), true, 'sectionNumbers is set' - ); - } ); - - QUnit.test( '#log (section=2) multiple issues', function ( assert ) { - var overlay = new PageIssuesOverlay( - [ { - severity: 'MEDIUM' - }, - { - severity: 'LOW' - } ], this.logger, '2', 0 ); - overlay.onExit(); - assert.strictEqual( - this.logger.log.calledWith( { - action: 'modalClose', - issuesSeverity: [ 'MEDIUM', 'LOW' ], - sectionNumbers: [ '2', '2' ] - } ), true, 'sectionNumbers is set for each issue' - ); - } ); -}( mw.mobileFrontend ) ); diff --git a/tests/qunit/skins.minerva.scripts/pageIssues.test.js b/tests/qunit/skins.minerva.scripts/pageIssues.test.js index 52aaf53..63a1dc8 100644 --- a/tests/qunit/skins.minerva.scripts/pageIssues.test.js +++ b/tests/qunit/skins.minerva.scripts/pageIssues.test.js @@ -3,7 +3,6 @@ util = M.require( 'mobile.startup/util' ), createBanner = pageIssues.test.createBanner, icon = {}, - formatPageIssuesSeverity = pageIssues.test.formatPageIssuesSeverity, MEDIUM_ISSUE = { issue: { severity: 'MEDIUM', @@ -12,24 +11,6 @@ iconString: 'i', text: 't' }, - MEDIUM_MULTIPLE_ISSUE = { - issue: { - severity: 'MEDIUM', - grouped: true, - icon: icon - }, - iconString: 'i', - text: 't' - }, - LOW_MULTIPLE_ISSUE = { - issue: { - severity: 'LOW', - grouped: true, - icon: icon - }, - iconString: 'i', - text: 't' - }, LOW_ISSUE = { issue: { severity: 'LOW', @@ -81,48 +62,6 @@ assert.strictEqual( window.location.hash, '#/issues/' + SECTION ); } ); - // NOTE: Only for PageIssues AB - QUnit.test( 'clicking on the product of createBanner() should trigger a custom event', function ( assert ) { - var mockAction = { - action: 'issueClicked', - issuesSeverity: [ 'MEDIUM' ], - sectionNumbers: [ SECTION ] - }; - mw.trackSubscribe( 'minerva.PageIssuesAB', function ( topic, data ) { - assert.deepEqual( mockAction, data ); - } ); - processedAmbox.click(); - } ); - - QUnit.test( 'formatPageIssuesSeverity', function ( assert ) { - var multipleIssues = [ - MEDIUM_MULTIPLE_ISSUE, - LOW_MULTIPLE_ISSUE, - LOW_MULTIPLE_ISSUE - ], - multipleSingleIssues = [ - LOW_ISSUE, - HIGH_ISSUE, - MEDIUM_ISSUE - ], - mixedMultipleSingle = [ - HIGH_ISSUE, - LOW_MULTIPLE_ISSUE, - MEDIUM_MULTIPLE_ISSUE, - LOW_ISSUE, - MEDIUM_ISSUE, - HIGH_ISSUE - ], - testMultiple = multipleIssues.reduce( formatPageIssuesSeverity, [] ), - testSingle = multipleSingleIssues.reduce( formatPageIssuesSeverity, [] ), - testMixed = mixedMultipleSingle.reduce( formatPageIssuesSeverity, [] ); - - assert.deepEqual( testMultiple, [ 'MEDIUM' ], 'Multiple issues return one maxSeverity value' ); - assert.deepEqual( testSingle, [ 'LOW', 'HIGH', 'MEDIUM' ], 'Single issues return each corresponding severity' ); - assert.deepEqual( testMixed, [ 'HIGH', 'MEDIUM', 'LOW', 'MEDIUM', 'HIGH' ], 'Mixed single/multiple return one value for multiples' ); - - } ); - QUnit.test( 'getAllIssuesSections', function ( assert ) { var multipleIssuesWithDeletion, multipleIssues, allIssuesOldTreatment, allIssuesNewTreatment;