From f14d9514c60f90bd389c2b94dd58ac1922531784 Mon Sep 17 00:00:00 2001
From: jdlrobson <jdlrobson@gmail.com>
Date: Thu, 6 Sep 2018 17:49:43 -0700
Subject: [PATCH] Fix `formatPageIssuesSeverity`

The page-issues reducer function that retrieves the severityLevels
for issues was incorrectly comparing two values in the multiple-issues
scenario.

Instead of comparing the severity of the current issue with the previous
value in the accumulator, it was comparing the current issue with the
previous issue in the original array, thus ignoring the
previous `maxSeverity` value in the accumulator.

Tests added and function refactored with more explicit variable names.

Bug: T203725
Change-Id: Ia4c15cbf0c2457d68a7381e8ed04c1a6b3ae65d1
---
 resources/skins.minerva.scripts/pageIssues.js | 23 ++++++----
 .../skins.minerva.scripts/test_pageIssues.js  | 42 +++++++++++++++++++
 2 files changed, 56 insertions(+), 9 deletions(-)

diff --git a/resources/skins.minerva.scripts/pageIssues.js b/resources/skins.minerva.scripts/pageIssues.js
index 8dbd142..9fb8c3f 100644
--- a/resources/skins.minerva.scripts/pageIssues.js
+++ b/resources/skins.minerva.scripts/pageIssues.js
@@ -41,23 +41,27 @@
 	 * 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 {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( 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 ]
+	function formatPageIssuesSeverity( formattedArr, currentItem, currentIndex, pageIssues ) {
+		var lastItem = pageIssues[ currentIndex - 1 ],
+			lastFormattedIndex = formattedArr.length - 1,
+			lastFormattedValue = formattedArr[ lastFormattedIndex ];
+		// If the last and current item `isMultiple`, fold the maxSeverity
+		// of the two items into a single value.
+		if ( lastItem && lastItem.isMultiple && currentItem.isMultiple ) {
+			formattedArr[ lastFormattedIndex ] = pageIssuesParser.maxSeverity(
+				[ lastFormattedValue, currentItem.severity ]
 			);
 		} else {
-			acc.push( summary.severity );
+			formattedArr.push( currentItem.severity );
 		}
-		return acc;
+		return formattedArr;
 	}
 
 	/**
@@ -336,6 +340,7 @@
 		// the subscription call in cleanuptemplates.
 		log: pageIssuesLogger.log,
 		test: {
+			formatPageIssuesSeverity: formatPageIssuesSeverity,
 			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 208eaa7..2959ad3 100644
--- a/tests/qunit/skins.minerva.scripts/test_pageIssues.js
+++ b/tests/qunit/skins.minerva.scripts/test_pageIssues.js
@@ -4,11 +4,24 @@
 		pageIssuesParser = M.require( 'skins.minerva.scripts/pageIssuesParser' ),
 		extractMessage = pageIssues.test.extractMessage,
 		createBanner = pageIssues.test.createBanner,
+		formatPageIssuesSeverity = pageIssues.test.formatPageIssuesSeverity,
 		MEDIUM_ISSUE = {
 			severity: 'MEDIUM',
 			icon: 'i',
 			text: 't'
 		},
+		MEDIUM_MULTIPLE_ISSUE = {
+			severity: 'MEDIUM',
+			isMultiple: true,
+			icon: 'i',
+			text: 't'
+		},
+		LOW_MULTIPLE_ISSUE = {
+			severity: 'LOW',
+			isMultiple: true,
+			icon: 'i',
+			text: 't'
+		},
 		LOW_ISSUE = {
 			severity: 'LOW',
 			icon: 'i',
@@ -64,6 +77,35 @@
 		} );
 	} );
 
+	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( 'extractMessage', function ( assert ) {
 		this.sandbox.stub( pageIssuesParser, 'parse' ).returns(
 			{