diff --git a/includes/NotifUser.php b/includes/NotifUser.php index 4f5dcacd1..a6927e71c 100644 --- a/includes/NotifUser.php +++ b/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 ) { diff --git a/includes/api/ApiEchoNotifications.php b/includes/api/ApiEchoNotifications.php index 7e41579e2..ba539fe1a 100644 --- a/includes/api/ApiEchoNotifications.php +++ b/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, - null, + $limit - $count + 1, + // 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 ) { - $lastItem = array_pop( $result['list'] ); - $result['continue'] = $lastItem['timestamp']['utcunix'] . '|' . $lastItem['id']; - } + if ( count( $result['list'] ) > $limit ) { + $lastItem = array_pop( $result['list'] ); + $result['continue'] = $lastItem['timestamp']['utcunix'] . '|' . $lastItem['id']; } return $result; diff --git a/includes/formatters/EchoFlyoutFormatter.php b/includes/formatters/EchoFlyoutFormatter.php index 4f7e37831..3fedd643a 100644 --- a/includes/formatters/EchoFlyoutFormatter.php +++ b/includes/formatters/EchoFlyoutFormatter.php @@ -1,5 +1,4 @@ fetchByUserInternal( $user, $limit, $eventTypes, array( 'notification_read_timestamp' => null ), $dbSource ); + public function fetchUnreadByUser( User $user, $limit, $continue, array $eventTypes = array(), $dbSource = DB_SLAVE ) { + 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 ); - - // 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 ); + return $this->fetchByUserInternal( $user, $limit, $continue, $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' ), '*', diff --git a/tests/phpunit/mapper/NotificationMapperTest.php b/tests/phpunit/mapper/NotificationMapperTest.php index 03fc008b2..40277d836 100644 --- a/tests/phpunit/mapper/NotificationMapperTest.php +++ b/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 ) {