diff --git a/api/ApiEchoNotifications.php b/api/ApiEchoNotifications.php index 4cad1699d..8f6e9c855 100644 --- a/api/ApiEchoNotifications.php +++ b/api/ApiEchoNotifications.php @@ -124,8 +124,7 @@ class ApiEchoNotifications extends ApiQueryBase { $notifMapper = new EchoNotificationMapper(); - // Unread notifications + possbile 3 read notification depending on result number - // We don't care about next offset in this case + // Prefer unread notifications. We don't care about next offset in this case if ( $unreadFirst ) { wfProfileIn( __METHOD__ . '-fetch-data-unread-first' ); $notifs = $notifMapper->fetchUnreadByUser( $user, $limit, $eventTypes ); @@ -133,17 +132,17 @@ class ApiEchoNotifications extends ApiQueryBase { // then fill the result with some read notifications $count = count( $notifs ); if ( $count < $limit ) { - // We could add another function for "notification_read_timestamp is not null" - // but it's probably not good to add negation condition to a query - $mixedNotifs = $notifMapper->fetchByUser( $user, $count + 3, null, $eventTypes ); + // 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, + $eventTypes, + array_keys( $notifs ) + ); foreach ( $mixedNotifs as $notif ) { - if ( !isset( $notifs[$notif->getEvent()->getId()] ) ) { - if ( $count >= $limit ) { - break; - } - $count++; - $notifs[$notif->getEvent()->getId()] = $notif; - } + $notifs[$notif->getEvent()->getId()] = $notif; } } wfProfileOut( __METHOD__ . '-fetch-data-unread-first' ); diff --git a/includes/mapper/NotificationMapper.php b/includes/mapper/NotificationMapper.php index ee89a0ea8..8844c60a5 100644 --- a/includes/mapper/NotificationMapper.php +++ b/includes/mapper/NotificationMapper.php @@ -118,13 +118,15 @@ class EchoNotificationMapper extends EchoAbstractMapper { /** * Get Notification by user in batch along with limit, offset etc - * @param User the user to get notifications for - * @param int The maximum number of notifications to return - * @param string Used for offset - * @param array Event types to load + * + * @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 $excludeEventIds Event id's to exclude. * @return EchoNotification[] */ - public function fetchByUser( User $user, $limit, $continue, array $eventTypes = array() ) { + public function fetchByUser( User $user, $limit, $continue, array $eventTypes = array(), array $excludeEventIds = array() ) { $dbr = $this->dbFactory->getEchoDb( DB_SLAVE ); if ( !$eventTypes ) { @@ -145,6 +147,10 @@ class EchoNotificationMapper extends EchoAbstractMapper { 'notification_bundle_base' => 1 ); + if ( $excludeEventIds ) { + $conds[] = 'event_id NOT IN ( ' . $dbr->makeList( $excludeEventIds ) . ' ) '; + } + $offset = $this->extractQueryOffset( $continue ); // Start points are specified