Merge "Also support continuation requests for $unreadFirst"

includes/NotifUser.php

@@ -309,7 +309,7 @@ class MWEchoNotifUser {
 			$eventTypesToLoad = $attributeManager->getUserEnabledEventsbySections( $this->mUser, 'web', array( $section ) );
 		}
 
-		$notifications = $this->notifMapper->fetchUnreadByUser( $this->mUser, 1, $eventTypesToLoad, $dbSource );
+		$notifications = $this->notifMapper->fetchUnreadByUser( $this->mUser, 1, null, $eventTypesToLoad, $dbSource );
 		if ( $notifications ) {
 			$notification = reset( $notifications );
 			$timestamp = $notification->getTimestamp();
@@ -344,7 +344,7 @@ class MWEchoNotifUser {
 			// After this 'mark read', is there any unread edit-user-talk
 			// remaining?  If not, we should clear the newtalk flag.
 			if ( $this->mUser->getNewtalk() ) {
-				$unreadEditUserTalk = $this->notifMapper->fetchUnreadByUser( $this->mUser, 1, array( 'edit-user-talk' ), DB_MASTER );
+				$unreadEditUserTalk = $this->notifMapper->fetchUnreadByUser( $this->mUser, 1, null, array( 'edit-user-talk' ), DB_MASTER );
 				if ( count( $unreadEditUserTalk ) === 0 ) {
 					$this->mUser->setNewtalk( false );
 				}
@@ -379,7 +379,7 @@ class MWEchoNotifUser {
 		$attributeManager = EchoAttributeManager::newFromGlobalVars();
 		$eventTypes = $attributeManager->getUserEnabledEventsbySections( $this->mUser, 'web', $sections );
 
-		$notifs = $this->notifMapper->fetchUnreadByUser( $this->mUser, $wgEchoMaxUpdateCount, $eventTypes );
+		$notifs = $this->notifMapper->fetchUnreadByUser( $this->mUser, $wgEchoMaxUpdateCount, null, $eventTypes );
 
 		$eventIds = array_filter(
 			array_map( function ( EchoNotification $notif ) {

includes/api/ApiEchoNotifications.php

@@ -111,17 +111,46 @@ class ApiEchoNotifications extends ApiQueryBase {
 
 		// Prefer unread notifications. We don't care about next offset in this case
 		if ( $unreadFirst ) {
-			$notifs = $notifMapper->fetchUnreadByUser( $user, $limit, $eventTypes );
+			// query for unread notifications past 'continue' (offset)
+			$notifs = $notifMapper->fetchUnreadByUser( $user, $limit + 1, $continue, $eventTypes );
+
+			/*
+			 * 'continue' has a timestamp & id (to start with, in case there
+			 * would be multiple events with that same timestamp)
+			 * Unread notifications should always load first, but may be older
+			 * than read ones, but we can work with current 'continue' format:
+			 * * if there is no continue, first load unread notifications
+			 * * if there is a continue, fetch unread notifications first:
+			 * * if there are no unread ones, continue must've been about read:
+			 *   fetch 'em
+			 * * if there are unread ones but first one doesn't match continue
+			 *   id, it must've been about read: discard unread & fetch read
+			 */
+			if ( $notifs && $continue ) {
+				/** @var EchoNotification $first */
+				$first = reset( $notifs );
+				$continueId = intval( trim( strrchr( $continue, '|' ), '|' ) );
+				if ( $first->getEvent()->getID() !== $continueId ) {
+					// notification doesn't match continue id, it must've been
+					// about read notifications: discard all unread ones
+					$notifs = array();
+				}
+			}
+
 			// If there are less unread notifications than we requested,
 			// then fill the result with some read notifications
 			$count = count( $notifs );
-			if ( $count < $limit ) {
+			// we need 1 more than $limit, so we can respond 'continue'
+			if ( $count <= $limit ) {
 				// Query planner should be smart enough that passing a short list of ids to exclude
 				// will only visit at most that number of extra rows.
 				$mixedNotifs = $notifMapper->fetchByUser(
 					$user,
-					$limit - $count,
+					$limit - $count + 1,
-					null,
+					// if there were unread notifications, 'continue' was for
+					// unread notifications and we should start fetching read
+					// notifications from start
+					$count > 0 ? null : $continue,
 					$eventTypes,
 					array_keys( $notifs )
 				);
@@ -141,11 +170,9 @@ class ApiEchoNotifications extends ApiQueryBase {
 		}
 
 		// Generate offset if necessary
-		if ( !$unreadFirst ) {
+		if ( count( $result['list'] ) > $limit ) {
-			if ( count( $result['list'] ) > $limit ) {
+			$lastItem = array_pop( $result['list'] );
-				$lastItem = array_pop( $result['list'] );
+			$result['continue'] = $lastItem['timestamp']['utcunix'] . '|' . $lastItem['id'];
-				$result['continue'] = $lastItem['timestamp']['utcunix'] . '|' . $lastItem['id'];
-			}
 		}
 
 		return $result;

includes/formatters/EchoFlyoutFormatter.php

@@ -1,5 +1,4 @@
 <?php
-use EchoLinkNormalizer;
 
 /**
  * A formatter for the notification flyout popup

includes/mapper/NotificationMapper.php

@@ -95,12 +95,13 @@ class EchoNotificationMapper extends EchoAbstractMapper {
 	 * which is done via a deleteJob
 	 * @param User $user
 	 * @param int $limit
+	 * @param string $continue Used for offset
 	 * @param string[] $eventTypes
 	 * @param int $dbSource Use master or slave database
 	 * @return EchoNotification[]
 	 */
-	public function fetchUnreadByUser( User $user, $limit, array $eventTypes = array(), $dbSource = DB_SLAVE ) {
+	public function fetchUnreadByUser( User $user, $limit, $continue, array $eventTypes = array(), $dbSource = DB_SLAVE ) {
-		return $this->fetchByUserInternal( $user, $limit, $eventTypes, array( 'notification_read_timestamp' => null ), $dbSource );
+		return $this->fetchByUserInternal( $user, $limit, $continue, $eventTypes, array( 'notification_read_timestamp' => null ), $dbSource );
 	}
 
 	/**
@@ -121,27 +122,19 @@ class EchoNotificationMapper extends EchoAbstractMapper {
 			$conds[] = 'event_id NOT IN ( ' . $dbr->makeList( $excludeEventIds ) . ' ) ';
 		}
 
-		$offset = $this->extractQueryOffset( $continue );
+		return $this->fetchByUserInternal( $user, $limit, $continue, $eventTypes, $conds );
-
-		// Start points are specified
-		if ( $offset['timestamp'] && $offset['offset'] ) {
-			$ts = $dbr->addQuotes( $dbr->timestamp( $offset['timestamp'] ) );
-			// The offset and timestamp are those of the first notification we want to return
-			$conds[] = "notification_timestamp < $ts OR ( notification_timestamp = $ts AND notification_event <= " . $offset['offset'] . " )";
-		}
-
-		return $this->fetchByUserInternal( $user, $limit, $eventTypes, $conds );
 	}
 
 	/**
 	 * @param User $user the user to get notifications for
 	 * @param int $limit The maximum number of notifications to return
+	 * @param string $continue Used for offset
 	 * @param array $eventTypes Event types to load
 	 * @param array $conds Additional query conditions.
 	 * @param int $dbSource Use master or slave database
 	 * @return EchoNotification[]
 	 */
-	protected function fetchByUserInternal( User $user, $limit, array $eventTypes = array(), array $conds = array(), $dbSource = DB_SLAVE ) {
+	protected function fetchByUserInternal( User $user, $limit, $continue, array $eventTypes = array(), array $conds = array(), $dbSource = DB_SLAVE ) {
 		$dbr = $this->dbFactory->getEchoDb( $dbSource );
 
 		if ( !$eventTypes ) {
@@ -161,6 +154,15 @@ class EchoNotificationMapper extends EchoAbstractMapper {
 			'notification_bundle_base' => 1
 		) + $conds;
 
+		$offset = $this->extractQueryOffset( $continue );
+
+		// Start points are specified
+		if ( $offset['timestamp'] && $offset['offset'] ) {
+			$ts = $dbr->addQuotes( $dbr->timestamp( $offset['timestamp'] ) );
+			// The offset and timestamp are those of the first notification we want to return
+			$conds[] = "notification_timestamp < $ts OR ( notification_timestamp = $ts AND notification_event <= " . $offset['offset'] . " )";
+		}
+
 		$res = $dbr->select(
 			array( 'echo_notification', 'echo_event', 'echo_target_page' ),
 			'*',

tests/phpunit/mapper/NotificationMapperTest.php

@@ -12,7 +12,7 @@ class EchoNotificationMapperTest extends MediaWikiTestCase {
 	public function fetchUnreadByUser( User $user, $limit, array $eventTypes = array() ) {
 		// Unsuccessful select
 		$notifMapper = new EchoNotificationMapper( $this->mockMWEchoDbFactory( array( 'select' => false ) ) );
-		$res = $notifMapper->fetchUnreadByUser( $this->mockUser(), 10, '' );
+		$res = $notifMapper->fetchUnreadByUser( $this->mockUser(), 10, null, '' );
 		$this->assertEmpty( $res );
 
 		// Successful select
@@ -34,11 +34,11 @@ class EchoNotificationMapperTest extends MediaWikiTestCase {
 			)
 		);
 		$notifMapper = new EchoNotificationMapper( $this->mockMWEchoDbFactory( array( 'select' => $dbResult ) ) );
-		$res = $notifMapper->fetchUnreadByUser( $this->mockUser(), 10, '', array() );
+		$res = $notifMapper->fetchUnreadByUser( $this->mockUser(), 10, null, '', array() );
 		$this->assertEmpty( $res );
 
 		$notifMapper = new EchoNotificationMapper( $this->mockMWEchoDbFactory( array( 'select' => $dbResult ) ) );
-		$res = $notifMapper->fetchUnreadByUser( $this->mockUser(), 10, '', array( 'test_event' ) );
+		$res = $notifMapper->fetchUnreadByUser( $this->mockUser(), 10, null, '', array( 'test_event' ) );
 		$this->assertTrue( is_array( $res ) );
 		$this->assertGreaterThan( 0, count( $res ) );
 		foreach ( $res as $row ) {