From 815f3d99eeeeef9531b6f66c71642eb19f09cb05 Mon Sep 17 00:00:00 2001 From: Jan Drewniak Date: Wed, 29 Aug 2018 14:59:37 +0200 Subject: [PATCH] For page-issues pageLoaded and editClicked events, treat "multiple issues" templates as one issue. When logging the `issuesSeverity` and `sectionNumbers` field, any issues that are part of a "multiple issues" template only send one value. Adds an `isMultiple` property to IssueSummary to determine which issues are part of a multiple-issues template. Bug: T203050 Change-Id: I7d55dfead72439df4accadcdc8623a080e1321c2 --- resources/skins.minerva.scripts/pageIssues.js | 47 +++++++++--- .../skins.minerva.scripts/test_pageIssues.js | 76 ++++++++++++++++++- 2 files changed, 113 insertions(+), 10 deletions(-) diff --git a/resources/skins.minerva.scripts/pageIssues.js b/resources/skins.minerva.scripts/pageIssues.js index 0ea0a18..dabe661 100644 --- a/resources/skins.minerva.scripts/pageIssues.js +++ b/resources/skins.minerva.scripts/pageIssues.js @@ -26,6 +26,7 @@ /** * @typedef {Object} IssueSummary * @prop {string} severity A PageIssue.severity. + * @prop {Boolean} isMultiple Whether or not the issue is part of a "multiple issues" template. * @prop {string} icon HTML string. * @prop {string} text HTML string. */ @@ -36,11 +37,27 @@ } /** - * @param {IssueSummary} summary - * @return {string} + * 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} acc - the return array containing severities + * @param {IssueSummary} summary current IssueSummary object + * @param {number} currentIndex current index of pageIssues + * @param {array} pageIssues array of pageIssues + * + * @return {array} acc */ - function formatPageIssuesSeverity( summary ) { - return summary.severity; + function formatPageIssuesSeverity( acc, summary, currentIndex, pageIssues ) { + var lastItem = pageIssues[ currentIndex - 1 ]; + if ( lastItem && lastItem.isMultiple && summary.isMultiple ) { + acc[ acc.length - 1 ] = pageIssuesParser.maxSeverity( + [ lastItem.severity, summary.severity ] + ); + } else { + acc.push( summary.severity ); + } + return acc; } /** @@ -50,11 +67,12 @@ * @return {IssueSummary} */ function extractMessage( $box ) { - var selector = '.mbox-text, .ambox-text', + var SELECTOR = '.mbox-text, .ambox-text', + MULTIPLE_SELECTOR = '.mw-collapsible-content', $container = $( '
' ), pageIssue; - $box.find( selector ).each( function () { + $box.find( SELECTOR ).each( function () { var contents, $this = $( this ); // Clean up talk page boxes @@ -70,6 +88,7 @@ return { severity: pageIssue.severity, + isMultiple: $box.parent().is( MULTIPLE_SELECTOR ), icon: pageIssue.icon.toHtmlString(), text: $container.html() }; @@ -216,14 +235,23 @@ /** * Returns an array containing the section of each page issue. + * In the case that several page issues are grouped in a 'multiple issues' template, + * returns the section of those issues as one item. * @param {Object} allIssues mapping section {Number} to {IssueSummary} * @return {array} */ function getAllIssuesSections( allIssues ) { return Object.keys( allIssues ).reduce( function ( acc, section ) { if ( allIssues[ section ].length ) { - allIssues[ section ].forEach( function () { - acc.push( section ); + allIssues[ section ].forEach( function ( issue, i ) { + var lastIssue = allIssues[ section ][i - 1]; + // If the last issue belongs to a "Multiple issues" template, + // and so does the current one, don't add the current one. + if ( lastIssue && lastIssue.isMultiple && issue.isMultiple ) { + acc[ acc.length - 1 ] = section; + } else { + acc.push( section ); + } } ); } return acc; @@ -282,7 +310,7 @@ pageIssuesLogger.newPageIssueSchemaData( newTreatmentEnabled, CURRENT_NS, - getIssues( KEYWORD_ALL_SECTIONS ).map( formatPageIssuesSeverity ), + getIssues( KEYWORD_ALL_SECTIONS ).reduce( formatPageIssuesSeverity, [] ), getAllIssuesSections( allIssues ) ) ); @@ -306,6 +334,7 @@ // the subscription call in cleanuptemplates. log: pageIssuesLogger.log, test: { + extractMessage: extractMessage, getAllIssuesSections: getAllIssuesSections, createBanner: createBanner } diff --git a/tests/qunit/skins.minerva.scripts/test_pageIssues.js b/tests/qunit/skins.minerva.scripts/test_pageIssues.js index a7ed350..208eaa7 100644 --- a/tests/qunit/skins.minerva.scripts/test_pageIssues.js +++ b/tests/qunit/skins.minerva.scripts/test_pageIssues.js @@ -1,5 +1,8 @@ ( function ( M ) { var pageIssues = M.require( 'skins.minerva.scripts/pageIssues' ), + util = M.require( 'mobile.startup/util' ), + pageIssuesParser = M.require( 'skins.minerva.scripts/pageIssuesParser' ), + extractMessage = pageIssues.test.extractMessage, createBanner = pageIssues.test.createBanner, MEDIUM_ISSUE = { severity: 'MEDIUM', @@ -61,8 +64,54 @@ } ); } ); + QUnit.test( 'extractMessage', function ( assert ) { + this.sandbox.stub( pageIssuesParser, 'parse' ).returns( + { + severity: 'LOW', + icon: { + toHtmlString: function () { + return ''; + } + } + } + ); + [ + [ + $( '
' ).html( + '
Smelly
' + ).appendTo( '
' ), + { + severity: 'LOW', + isMultiple: true, + icon: '', + text: '

Smelly

' + }, + 'When the box is a child of mw-collapsible-content it isMultiple' + ], + [ + $( '
' ).html( + '
Dirty
' + ), + { + severity: 'LOW', + isMultiple: false, + icon: '', + text: '

Dirty

' + }, + 'When the box is not child of mw-collapsible-content it !isMultiple' + ] + ].forEach( function ( test ) { + assert.deepEqual( + extractMessage( test[ 0 ] ), + test[ 1 ], + test[ 2 ] + ); + } ); + } ); + QUnit.test( 'getAllIssuesSections', function ( assert ) { - var allIssuesOldTreatment, allIssuesNewTreatment; + var multipleIssuesWithDeletion, + multipleIssues, allIssuesOldTreatment, allIssuesNewTreatment; allIssuesOldTreatment = { 0: [ MEDIUM_ISSUE, @@ -70,6 +119,21 @@ MEDIUM_ISSUE ] }; + multipleIssues = { + 0: [ + util.extend( {}, MEDIUM_ISSUE, { isMultiple: true } ), + util.extend( {}, LOW_ISSUE, { isMultiple: true } ), + util.extend( {}, MEDIUM_ISSUE, { isMultiple: true } ) + ] + }; + multipleIssuesWithDeletion = { + 0: [ + HIGH_ISSUE, + util.extend( {}, MEDIUM_ISSUE, { isMultiple: true } ), + util.extend( {}, LOW_ISSUE, { isMultiple: true } ), + util.extend( {}, MEDIUM_ISSUE, { isMultiple: true } ) + ] + }; allIssuesNewTreatment = { 0: [ HIGH_ISSUE, @@ -90,5 +154,15 @@ [ '0', '0', '0', '1' ], 'section numbers correctly extracted from new treatment' ); + assert.deepEqual( + getAllIssuesSections( multipleIssues ), + [ '0' ], + 'multiple issues are packed into one entry since there is one box' + ); + assert.deepEqual( + getAllIssuesSections( multipleIssuesWithDeletion ), + [ '0', '0' ], + 'while multiple issues are grouped, non-multiple issues are still reported' + ); } ); }( mw.mobileFrontend ) );