ReverseChronologicalPager pages have list headers grouping by date

These list headers are added to the HTML but hidden by default.
They are needed for the mobile version which displays headers
for better scannability.

However the change will only display on:
* Special:Contributions
* history page
* Special:MergeHistory

Bug: T298638
Change-Id: Iaa55eab57f761c9be780aba79cf2b9b212d91657

includes/actions/pagers/HistoryPager.php

@@ -37,6 +37,10 @@ class HistoryPager extends ReverseChronologicalPager {
 	 * @var bool|stdClass
 	 */
 	public $lastRow = false;
+	/**
+	 * @var bool|stdClass|string May be set to 'unknown' when processing the last row
+	 */
+	public $currRow = false;
 	public $lastResultOffset = false;
 
 	public $counter, $historyPage, $buttons, $conds;
@@ -161,30 +165,39 @@ class HistoryPager extends ReverseChronologicalPager {
 	}
 
 	/**
-	 * @param stdClass $row
+	 * Changes getRow behaviour to only render the previous row.
-	 * @return string
+	 *
+	 * @inheritDoc
 	 */
-	public function formatRow( $row ) {
+	protected function getRow( $row ): string {
+		$this->currRow = $row;
 		if ( $this->lastRow ) {
-			$firstInList = $this->counter == 1;
 			$this->counter++;
-
+			$s = parent::getRow( $this->lastRow );
-			$notifTimestamp = $this->getConfig()->get( 'ShowUpdatedMarker' )
-				? $this->watchlistManager
-					->getTitleNotificationTimestamp( $this->getUser(), $this->getTitle() )
-				: false;
-
-			$s = $this->historyLine( $this->lastRow, $row, $notifTimestamp,
-				$firstInList, $this->lastResultOffset );
 		} else {
 			$s = '';
 		}
 		$this->lastRow = $row;
 		$this->lastResultOffset = $this->getResultOffset();
-
 		return $s;
 	}
 
+	/**
+	 * @param stdClass $row Unused because we're actually processing the previous row
+	 * @return string
+	 */
+	public function formatRow( $row ) {
+		$firstInList = $this->counter == 1;
+
+		$notifTimestamp = $this->getConfig()->get( 'ShowUpdatedMarker' )
+			? $this->watchlistManager
+				->getTitleNotificationTimestamp( $this->getUser(), $this->getTitle() )
+			: false;
+
+		return $this->historyLine( $this->lastRow, $this->currRow, $notifTimestamp,
+			$firstInList, $this->lastResultOffset );
+	}
+
 	protected function doBatchLookups() {
 		if ( !$this->getHookRunner()->onPageHistoryPager__doBatchLookups( $this, $this->mResult ) ) {
 			return;
@@ -241,7 +254,7 @@ class HistoryPager extends ReverseChronologicalPager {
 	 */
 	protected function getStartBody() {
 		$this->lastRow = false;
-		$this->counter = 1;
+		$this->counter = 0;
 		$this->oldIdChecked = 0;
 		$s = '';
 		// Button container stored in $this->buttons for re-use in getEndBody()
@@ -286,9 +299,10 @@ class HistoryPager extends ReverseChronologicalPager {
 			$this->buttons .= '</div>';
 
 			$s .= $this->buttons;
-			$s .= '<ul id="pagehistory">' . "\n";
 		}
 
+		$s .= '<div id="pagehistory">';
+
 		return $s;
 	}
 
@@ -312,36 +326,33 @@ class HistoryPager extends ReverseChronologicalPager {
 			return '';
 		}
 
+		# Special handling for the last row.
 		if ( $this->lastRow ) {
-			$firstInList = $this->counter == 1;
+			$this->counter++;
+
 			if ( $this->mIsBackwards ) {
 				# Next row is unknown, but for UI reasons, probably exists if an offset has been specified
 				if ( $this->mOffset == '' ) {
-					$next = null;
+					$row = null;
 				} else {
-					$next = 'unknown';
+					$row = 'unknown';
 				}
 			} else {
 				# The next row is the past-the-end row
-				$next = $this->mPastTheEndRow;
+				$row = $this->mPastTheEndRow;
 			}
-			$this->counter++;
 
-			$notifTimestamp = $this->getConfig()->get( 'ShowUpdatedMarker' )
+			$s = $this->getStartGroup();
-				? $this->watchlistManager
+			$s .= $this->getRow( $row );
-					->getTitleNotificationTimestamp( $this->getUser(), $this->getTitle() )
+			$s .= $this->getEndGroup();
-				: false;
-
-			$s = $this->historyLine( $this->lastRow, $next, $notifTimestamp,
-				$firstInList, $this->lastResultOffset );
 		} else {
 			$s = '';
 		}
-		$s .= "</ul>\n";
 		# Add second buttons only if there is more than one rev
 		if ( $this->getNumRows() > 2 ) {
 			$s .= $this->buttons;
 		}
+		$s .= '</div>'; // closes div#pagehistory
 		$s .= '</form>';
 		return $s;
 	}

includes/pager/IndexPager.php

@@ -100,6 +100,8 @@ abstract class IndexPager extends ContextSource implements Pager {
 	public $mDb;
 	/** @var stdClass|bool|null Extra row fetched at the end to see if the end was reached */
 	public $mPastTheEndRow;
+	/** @var array|null same format as getdate */
+	private $lastHeaderDate;
 
 	/**
 	 * The index to actually be used for ordering. This can be a single column,
@@ -264,7 +266,6 @@ abstract class IndexPager extends ContextSource implements Pager {
 
 		# Plus an extra row so that we can tell the "next" link should be shown
 		$queryLimit = $this->mLimit + 1;
-
 		if ( $this->mOffset == '' ) {
 			$isFirst = true;
 		} else {
@@ -586,6 +587,26 @@ abstract class IndexPager extends ContextSource implements Pager {
 	protected function preprocessResults( $result ) {
 	}
 
+	/**
+	 * Get the HTML of a pager row.
+	 *
+	 * @stable to override
+	 * @since 1.38
+	 * @param stdClass $row
+	 * @return string
+	 */
+	protected function getRow( $row ): string {
+		$s = '';
+		$timestamp = $row->rev_timestamp ?? null;
+		$date = $timestamp ? $this->getDateFromTimestamp( $timestamp ) : null;
+		if ( $date && $this->isHeaderRowNeeded( $date ) ) {
+			$s .= $this->getHeaderRow( $timestamp );
+			$this->lastHeaderDate = $date;
+		}
+		$s .= $this->formatRow( $row );
+		return $s;
+	}
+
 	/**
 	 * Get the formatted result list. Calls getStartBody(), formatRow() and
 	 * getEndBody(), concatenates the results and returns them.
@@ -614,15 +635,17 @@ abstract class IndexPager extends ContextSource implements Pager {
 				for ( $i = $numRows - 1; $i >= 0; $i-- ) {
 					$this->mResult->seek( $i );
 					$row = $this->mResult->fetchObject();
-					$s .= $this->formatRow( $row );
+					$s .= $this->getRow( $row );
 				}
 			} else {
 				$this->mResult->seek( 0 );
 				for ( $i = 0; $i < $numRows; $i++ ) {
 					$row = $this->mResult->fetchObject();
-					$s .= $this->formatRow( $row );
+					$s .= $this->getRow( $row );
 				}
 			}
+			$s .= $this->getEndGroup();
+			$s .= $this->getFooter();
 		} else {
 			$s .= $this->getEmptyBody();
 		}
@@ -630,6 +653,28 @@ abstract class IndexPager extends ContextSource implements Pager {
 		return $s;
 	}
 
+	/**
+	 * End an existing group of page rows.
+	 *
+	 * @stable to override
+	 * @since 1.38
+	 * @return string
+	 */
+	protected function getEndGroup(): string {
+		return '';
+	}
+
+	/**
+	 * Start a new group of page rows.
+	 *
+	 * @stable to override
+	 * @since 1.38
+	 * @return string
+	 */
+	protected function getStartGroup(): string {
+		return '';
+	}
+
 	/**
 	 * ResourceLoader modules that must be loaded to provide correct styling for this pager
 	 *
@@ -641,6 +686,75 @@ abstract class IndexPager extends ContextSource implements Pager {
 		return [ 'mediawiki.pager.styles' ];
 	}
 
+	/**
+	 * Determines if a header row is needed based on the current state of the IndexPager.
+	 *
+	 * @since 1.38
+	 * @param array $date of current row in format returned by getdate.
+	 * @return bool
+	 */
+	protected function isHeaderRowNeeded( array $date ): bool {
+		$showHeading = false;
+		$lastDay = $this->lastHeaderDate['mday'] ?? null;
+		$lastMonth = $this->lastHeaderDate['month'] ?? null;
+		$lastYear = $this->lastHeaderDate['year'] ?? null;
+
+		$showHeading = $lastDay === null || (
+			$date &&
+			$lastMonth && $lastYear &&
+			(
+				$lastDay !== $date['mday'] ||
+				$lastMonth !== $date['month'] ||
+				$lastYear !== $date['year']
+			)
+		);
+		return $date && $showHeading;
+	}
+
+	/**
+	 * Determines whether the header row is the first that will be outputted to the page.
+	 *
+	 * @since 1.38
+	 * @return bool
+	 */
+	final protected function isFirstHeaderRow(): bool {
+		$lastDay = $this->lastHeaderDate['mday'] ?? null;
+		return $lastDay === null;
+	}
+
+	/**
+	 * Get date from the timestamp
+	 *
+	 * @since 1.38
+	 * @param string $timestamp
+	 * @return array|null associative array that matches the return value of getdate
+	 */
+	final protected function getDateFromTimestamp( string $timestamp ) {
+		$time = $timestamp ? wfTimestamp( TS_UNIX, $timestamp ) : null;
+		return $time ? getdate( $time ) : null;
+	}
+
+	/**
+	 * Classes can override this method to introduce headers.
+	 *
+	 * @since 1.38
+	 * @param string $timestamp
+	 * @return string
+	 */
+	protected function getHeaderRow( string $timestamp ): string {
+		return '';
+	}
+
+	/**
+	 * Classes can extend to output a footer at the bottom of the pager list.
+	 *
+	 * @since 1.38
+	 * @return string
+	 */
+	protected function getFooter(): string {
+		return '';
+	}
+
 	/**
 	 * Make a self-link
 	 *

includes/pager/ReverseChronologicalPager.php

@@ -36,6 +36,46 @@ abstract class ReverseChronologicalPager extends IndexPager {
 	/** @var int */
 	public $mDay;
 
+	/**
+	 * @param string $timestamp
+	 * @return string
+	 */
+	protected function getHeaderRow( string $timestamp ): string {
+		$user = $this->getUser();
+		$headingClass = $this->isFirstHeaderRow() ?
+			// We use mw-index-pager- prefix here on the anticipation that this method will
+			// eventually be upstreamed to apply to other pagers. For now we constrain the
+			// change to ReverseChronologicalPager to reduce the risk of pages this touches
+			// in case there are any bugs.
+			'mw-index-pager-list-header-first mw-index-pager-list-header' :
+			'mw-index-pager-list-header';
+
+		$s = $this->getEndGroup();
+		$s .= Html::element( 'h4', [
+				'class' => $headingClass,
+			],
+			$this->getLanguage()->userDate(
+				$timestamp, $user
+			)
+		);
+		$s .= $this->getStartGroup();
+		return $s;
+	}
+
+	/**
+	 * @inheritDoc
+	 */
+	protected function getStartGroup(): string {
+		return "<ul class=\"mw-contributions-list\">\n";
+	}
+
+	/**
+	 * @inheritDoc
+	 */
+	protected function getEndGroup(): string {
+		return '</ul>';
+	}
+
 	/**
 	 * @stable to override
 	 * @return string

includes/specials/SpecialMergeHistory.php

@@ -287,9 +287,7 @@ class SpecialMergeHistory extends SpecialPage {
 
 		if ( $haveRevisions ) {
 			$out->addHTML( $revisions->getNavigationBar() );
-			$out->addHTML( '<ul>' );
 			$out->addHTML( $revisions->getBody() );
-			$out->addHTML( '</ul>' );
 			$out->addHTML( $revisions->getNavigationBar() );
 		} else {
 			$out->addWikiMsg( 'mergehistory-empty' );

includes/specials/pagers/ContribsPager.php

@@ -619,20 +619,6 @@ class ContribsPager extends RangeChronologicalPager {
 		$this->revisions += $revisions;
 	}
 
-	/**
-	 * @return string
-	 */
-	protected function getStartBody() {
-		return "<ul class=\"mw-contributions-list\">\n";
-	}
-
-	/**
-	 * @return string
-	 */
-	protected function getEndBody() {
-		return "</ul>\n";
-	}
-
 	/**
 	 * If the object looks like a revision row, or corresponds to a previously
 	 * cached revision, return the RevisionRecord. Otherwise, return null.

resources/src/mediawiki.action/mediawiki.action.history.js

@@ -4,7 +4,7 @@
 $( function () {
 	var $historyCompareForm = $( '#mw-history-compare' ),
 		$historySubmitter,
-		$lis = $( '#pagehistory > li' );
+		$lis = $( '#pagehistory .mw-contributions-list > li' );
 
 	/**
 	 * @ignore
@@ -93,7 +93,7 @@ $( function () {
 
 		if ( $historySubmitter ) {
 			$copyForm = $historyCompareForm.clone();
-			$copyRadios = $copyForm.find( '#pagehistory > li' ).find( 'input[name="diff"], input[name="oldid"]' );
+			$copyRadios = $copyForm.find( '#pagehistory .mw-contributions-list > li' ).find( 'input[name="diff"], input[name="oldid"]' );
 			$copyAction = $copyForm.find( '> [name="action"]' );
 
 			// Remove action=historysubmit and ids[..]=..

resources/src/mediawiki.pager.styles/IndexPager.less

@@ -1,3 +1,8 @@
-.mw-changeslist-time {
+.mw-changeslist-time,
+.mw-index-pager-list-header {
 	display: none;
 }
+
+.mw-index-pager-list-header:not( .mw-index-pager-list-header-first ) + ul {
+	margin-top: 0;
+}

tests/phpunit/includes/pager/HistoryPagerTest.php

@@ -0,0 +1,200 @@
+<?php
+
+use MediaWiki\Revision\MutableRevisionRecord;
+use Wikimedia\TestingAccessWrapper;
+
+/**
+ * Test class for HistoryPager methods.
+ *
+ * @group Pager
+ */
+class HistoryPagerTest extends MediaWikiLangTestCase {
+
+	/**
+	 * @param array $results for passing to FakeResultWrapper and deriving
+	 *  RevisionRecords and formatted comments.
+	 * @return HistoryPager
+	 */
+	private function getHistoryPager( array $results ) {
+		$wikiPageMock = $this->getMockBuilder( WikiPage::class )
+			->disableOriginalConstructor()
+			->getMock();
+		$contextMock = $this->getMockBuilder( RequestContext::class )
+			->disableOriginalConstructor()
+			->onlyMethods( [ 'getRequest', 'getWikiPage', 'getTitle' ] )
+			->getMock();
+		$contextMock->method( 'getRequest' )->willReturn(
+			new FauxRequest( [] )
+		);
+		$title = Title::newFromText( 'HistoryPagerTest' );
+		$contextMock->method( 'getTitle' )->willReturn( $title );
+
+		$contextMock->method( 'getWikiPage' )->willReturn( $wikiPageMock );
+		$articleMock = $this->getMockBuilder( Article::class )
+			->disableOriginalConstructor()
+			->onlyMethods( [ 'getContext' ] )
+			->getMock();
+		$articleMock->method( 'getContext' )->willReturn( $contextMock );
+
+		$actionMock = $this->getMockBuilder( HistoryAction::class )
+			->setConstructorArgs( [
+				$articleMock,
+				$contextMock
+			] )
+			->getMock();
+		$actionMock->method( 'getArticle' )->willReturn( $articleMock );
+		$actionMock->message = [
+			'cur' => 'cur',
+			'last' => 'last',
+			'tooltip-cur' => '',
+			'tooltip-last' => '',
+		];
+
+		$outputMock = $this->getMockBuilder( OutputPage::class )
+			->disableOriginalConstructor()
+			->onlyMethods( [ 'wrapWikiMsg' ] )
+			->getMock();
+
+		$pager = $this->getMockBuilder( HistoryPager::class )
+			->onlyMethods( [ 'reallyDoQuery', 'doBatchLookups',
+				'getOutput' ] )
+			->setConstructorArgs( [
+				$actionMock
+			] )
+			->getMock();
+
+		$pager->method( 'getOutput' )->willReturn( $outputMock );
+		$pager->method( 'reallyDoQuery' )->willReturn(
+			new FakeResultWrapper( $results )
+		);
+
+		// make all the methods in our mock public
+		$pager = TestingAccessWrapper::newFromObject( $pager );
+		// and update the private properties...
+		$pager->formattedComments = array_map( static function ( $result ) {
+			return 'dummy comment';
+		}, $results );
+
+		$pager->revisions = array_map( static function ( $result ) {
+			$title = Title::newFromText( 'HistoryPagerTest' );
+			$r = new MutableRevisionRecord( $title );
+			$r->setId( $result[ 'rev_id' ] );
+			return $r;
+		}, $results );
+
+		return $pager;
+	}
+
+	/**
+	 * @covers HistoryPager::getBody
+	 */
+	public function testGetBodyEmpty() {
+		$pager = $this->getHistoryPager( [] );
+		$html = $pager->getBody();
+		$this->assertStringContainsString( 'No matching revisions were found.', $html );
+		$this->assertStringNotContainsString( '<h4', $html );
+	}
+
+	/**
+	 * @covers HistoryPager::getBody
+	 */
+	public function testGetBodyOneHeading() {
+		$pager = $this->getHistoryPager(
+			[
+				[
+					'rev_page' => 'title',
+					'ts_tags' => [],
+					'rev_deleted' => false,
+					'rev_minor_edit' => false,
+					'rev_parent_id' => 1,
+					'user_name' => 'Jdlrobson',
+					'rev_id' => 2,
+					'rev_comment_data' => '{}',
+					'rev_comment_cid' => '1',
+					'rev_comment_text' => 'Created page',
+					'rev_timestamp' => '20220101001122',
+				]
+			]
+		);
+		$html = $pager->getBody();
+		$this->assertStringContainsString( '<h4', $html );
+	}
+
+	/**
+	 * @covers HistoryPager::getBody
+	 */
+	public function testGetBodyTwoHeading() {
+		$pagerData = [
+			'rev_page' => 'title',
+			'rev_deleted' => false,
+			'rev_minor_edit' => false,
+			'ts_tags' => [],
+			'rev_parent_id' => 1,
+			'user_name' => 'Jdlrobson',
+			'rev_comment_data' => '{}',
+			'rev_comment_cid' => '1',
+			'rev_comment_text' => 'Fixed typo',
+		];
+		$pager = $this->getHistoryPager(
+			[
+				$pagerData + [
+					'rev_id' => 3,
+					'rev_timestamp' => '20220301001122',
+				],
+				$pagerData + [
+					'rev_id' => 2,
+					'rev_timestamp' => '20220101001122',
+				],
+			]
+		);
+		$html = preg_replace( "/\n+/", '', $pager->getBody() );
+		$firstHeader = '<h4 class="mw-index-pager-list-header-first mw-index-pager-list-header">1 March 2022</h4>'
+			. '<ul class="mw-contributions-list">'
+			. '<li data-mw-revid="3">';
+		$secondHeader = '<h4 class="mw-index-pager-list-header">1 January 2022</h4>'
+			. '<ul class="mw-contributions-list">'
+			. '<li data-mw-revid="2">';
+
+		// Check that the undo links are correct in the topmost displayed row (for rev_id=3).
+		// This is tricky because the other rev number (in this example, '2') is magically
+		// pulled from the next row, before we've processed that row.
+		$this->assertStringContainsString( '&amp;undoafter=2&amp;undo=3', $html );
+
+		$this->assertStringContainsString( $firstHeader, $html );
+		$this->assertStringContainsString( $secondHeader, $html );
+		$this->assertStringContainsString( '<div id="pagehistory"', $html );
+	}
+
+	/**
+	 * @covers HistoryPager::getBody
+	 */
+	public function testGetBodyLastItem() {
+		$pagerData = [
+			'rev_page' => 'title',
+			'rev_deleted' => false,
+			'rev_minor_edit' => false,
+			'ts_tags' => [],
+			'rev_parent_id' => 1,
+			'user_name' => 'Jdlrobson',
+			'rev_comment_data' => '{}',
+			'rev_comment_cid' => '1',
+			'rev_comment_text' => 'Fixed typo',
+		];
+		$pager = $this->getHistoryPager(
+			[
+				$pagerData + [
+					'rev_id' => 2,
+					'rev_timestamp' => '20220301001111',
+				],
+				$pagerData + [
+					'rev_id' => 3,
+					'rev_timestamp' => '20220301001122',
+				],
+			]
+		);
+		$html = preg_replace( "/\n+/", '', $pager->getBody() );
+		$finalList = '</ul><ul class="mw-contributions-list">';
+		$this->assertStringContainsString( $finalList, $html,
+			'The last item is always in its own list and there is no header if the date is the same.' );
+	}
+}