Page MenuHomePhabricator

Separate page issue parsing from pageIssues.js
Closed, ResolvedPublic

Description

pageIssues.js has too many responsibilities, including some parsing. This task will remove one of those concerns by excising the parsing code.

At time of writing, pageIssues.js includes a hodgepodge of parsing logic. For example:

	/**
	 * Render a banner in a containing element.
	 * if in group B, a learn more link will be append to any amboxes inside $container
	 * if in group A or control, any amboxes in container will be removed and a link "page issues"
	 * will be rendered above the heading.
	 * This function comes with side effects. It will populate a global "allIssues" object which
	 * will link section numbers to issues.
	 * @param {Page} page to search for page issues inside
	 * @param {string} labelText what the label of the page issues banner should say
	 * @param {string} section that the banner and its issues belong to.
	 *  If string KEYWORD_ALL_SECTIONS banner should apply to entire page.
	 * @param {boolean} inline - if true the first ambox in the section will become the entry point
	 *                           for the issues overlay
	 *  and if false, a link will be rendered under the heading.
	 * @param {OverlayManager} overlayManager
	 * @ignore
	 *
	 * @return {JQuery.Object}
	 */
	function createBanner( page, labelText, section, inline, overlayManager ) {
		var
			$metadata,
			issueUrl = section === KEYWORD_ALL_SECTIONS ? '#/issues/' + KEYWORD_ALL_SECTIONS : '#/issues/' + section,
			selector = 'table.ambox, table.tmbox, table.cmbox, table.fmbox',
			issueSummaries = [];

		if ( section === KEYWORD_ALL_SECTIONS ) {
			$metadata = page.$( selector );
		} else {
			// find heading associated with the section
			$metadata = page.findChildInSectionLead( parseInt( section, 10 ), selector );
		}
		// clean it up a little
		$metadata.find( '.NavFrame' ).remove();
		$metadata.each( function () {
			var issueSummary,
				$this = $( this );

			if ( $this.find( selector ).length === 0 ) {
				issueSummary = pageIssuesParser.extract( $this );
				// Some issues after "extract" has been run will have no text.
				// For example in Template:Talk header the table will be removed and no issue found.
				// These should not be rendered.
				if ( issueSummary.text ) {
					issueSummaries.push( issueSummary );
				}
			}
		} );
		// store it for later
		allIssues[section] = issueSummaries;

		if ( inline ) {
			issueSummaries.forEach( function ( issueSummary, i ) {
				var isGrouped = issueSummary.issue.grouped,
					lastIssueIsGrouped = issueSummaries[ i - 1] &&
						issueSummaries[ i - 1].issue.grouped;
				// only render the first grouped issue of each group
				if ( isGrouped && !lastIssueIsGrouped ) {
					createPageIssueBannerMultiple( issueSummary, mw.msg( 'skin-minerva-issue-learn-more' ), issueUrl, overlayManager );
				} else {
					createPageIssueBanner( issueSummary, mw.msg( 'skin-minerva-issue-learn-more' ), issueUrl, overlayManager );
				}
			} );
		} else if ( issueSummaries.length ) {
			createPageIssueNotice( labelText, section );
		}

		return $metadata;
	}

	/**
	 * Obtains the list of issues for the current page and provided section
	 * @param {number|string} section either KEYWORD_ALL_SECTIONS or a number relating to the
	 *                                section the issues belong to
	 * @return {jQuery.Object[]} array of all issues.
	 */
	function getIssues( section ) {
		if ( section !== KEYWORD_ALL_SECTIONS ) {
			return allIssues[section] || [];
		}
		// Note section.all may not exist, depending on the structure of the HTML page.
		// It will only exist when Minerva has been run in desktop mode.
		// If it's absent, we'll reduce all the other lists into one.
		return allIssues.all || Object.keys( allIssues ).reduce(
			function ( all, key ) {
				return all.concat( allIssues[key] );
			},
			[]
		);
	}

	/**
	 * 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 ( 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.grouped && issue.grouped ) {
						acc[ acc.length - 1 ] = section;
					} else {
						acc.push( section );
					}
				} );
			}
			return acc;
		}, [] );
	}

All that weird code that's massaging data must go... Somewhere else. Maybe some or all of it can be moved to pageIssuesParser.js and / or a new parser focused on issue groups.

The new implementation should be easy to test too.

Additional discussion in this patch.

Acceptance criteria

  • All parsing code is removed from pageIssues.js
  • Some tests exist for the refactored code

Event Timeline

Restricted Application changed the subtype of this task from "Deadline" to "Task". · View Herald TranscriptDec 19 2018, 11:37 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Could we have some sample code describing how the createBanner would look after these changes?

Niedzielski renamed this task from Separate page issue parsing from pageIssues.js to [Analysis] Separate page issue parsing from pageIssues.js.Jan 14 2019, 6:14 PM

Change 487867 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/MinervaNeue@master] Hygiene: favor page issues all sections symbol

https://gerrit.wikimedia.org/r/487867

Change 487872 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/MinervaNeue@master] Hygiene: move variable from file to local scope

https://gerrit.wikimedia.org/r/487872

Change 487902 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/skins/MinervaNeue@master] Hygiene: remove unused page issues function

https://gerrit.wikimedia.org/r/487902

Niedzielski removed Niedzielski as the assignee of this task.Feb 4 2019, 6:42 PM

I submitted a few hygiene patches while analyzing this. In combination with T212376, I think these changes are adequate. The page issues code could still use some big improvements, but the low hanging fruit has been plucked. If there are no objections, I advise that the outstanding patches be considered to resolve both T212371 and T212376.

Niedzielski renamed this task from [Analysis] Separate page issue parsing from pageIssues.js to Separate page issue parsing from pageIssues.js.Feb 4 2019, 6:43 PM
Jdlrobson moved this task from Backlog to Tech debt on the MinervaNeue board.Feb 5 2019, 6:50 PM

Change 487867 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Hygiene: favor page issues all sections symbol

https://gerrit.wikimedia.org/r/487867

Change 487872 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Hygiene: move variable from file to local scope

https://gerrit.wikimedia.org/r/487872

Change 487902 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Hygiene: remove unused page issues function

https://gerrit.wikimedia.org/r/487902

What's left to do on this task?

Per T212371#4925230, I consider this done.