From b5522018297b030c5a598dc848c49a36532a1cf9 Mon Sep 17 00:00:00 2001
From: Erik Bernhardson <ebernhardson@wikimedia.org>
Date: Mon, 22 Sep 2014 17:24:03 -0700
Subject: [PATCH] I-2. Change the default number of Flow Messages in the flyout

Previously this was loading 25 unread notifications.  If less than 25 were
found it would add in 3 read notifications so it wasn't empty.

This patch changes things around to always return 25 notifications if the
user has at least that many.  Instead of backfilling a static count of 3
we backfill 25 - $count with previously read notifications.

Change-Id: I723316486216d7b9dacfcc9765c1a8212e973518
---
 api/ApiEchoNotifications.php           | 23 +++++++++++------------
 includes/mapper/NotificationMapper.php | 16 +++++++++++-----
 2 files changed, 22 insertions(+), 17 deletions(-)

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 52aca1029..9545c8db9 100644
--- a/includes/mapper/NotificationMapper.php
+++ b/includes/mapper/NotificationMapper.php
@@ -116,13 +116,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 ) {
@@ -143,6 +145,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