Mark notification as read upon visiting a page

Change-Id: I84493fbf742acd90645d484d42f049796a5e48ee

Hooks.php

@@ -651,6 +651,23 @@ class EchoHooks {
 			return true;
 		}
 
+		// Attempt to mark a notification as read when visiting a page,
+		// ideally this should be deferred to end of request and update
+		// the notification count accordingly
+		// @Fixme - Find a better place to put this code
+		if ( $title->getArticleID() ) {
+			$mapper = new EchoTargetPageMapper();
+			$targetPages = $mapper->fetchByUserPageId( $user, $title->getArticleID() );
+			if ( $targetPages ) {
+				$eventIds = array();
+				foreach ( $targetPages as $targetPage ) {
+					$eventIds[] = $targetPage->getEventId();
+				}
+				$notifUser = MWEchoNotifUser::newFromUser( $user );
+				$notifUser->markRead( $eventIds );
+			}
+		}
+
 		// Add a "My notifications" item to personal URLs
 		if ( $user->getOption( 'echo-notify-show-link' ) ) {
 			$notificationCount = MWEchoNotifUser::newFromUser( $user )->getNotificationCount();

includes/NotifUser.php

@@ -29,6 +29,12 @@ class MWEchoNotifUser {
 	 */
 	private $notifMapper;
 
+	/**
+	 * Target page mapper
+	 * @var EchoTargetPageMapper
+	 */
+	private $targetPageMapper;
+
 	/**
 	 * Whether to check cache for section status
 	 */
@@ -40,21 +46,24 @@ class MWEchoNotifUser {
 	/**
 	 * Usually client code doesn't need to initialize the object directly
 	 * because it could be obtained from factory method newFromUser()
-	 * @param User
-	 * @param BagOStuff
-	 * @param EchoUserNotificationGateway
-	 * @param EchoNotificationMapper
+	 * @param User $user
+	 * @param BagOStuff $cache
+	 * @param EchoUserNotificationGateway $userNotifGateway
+	 * @param EchoNotificationMapper $notifMapper
+	 * @param EchoTargetPageMapper $targetPageMapper
 	 */
 	public function __construct(
 		User $user,
 		BagOStuff $cache,
 		EchoUserNotificationGateway $userNotifGateway,
-		EchoNotificationMapper $notifMapper
+		EchoNotificationMapper $notifMapper,
+		EchoTargetPageMapper $targetPageMapper
 	) {
 		$this->mUser = $user;
 		$this->userNotifGateway = $userNotifGateway;
 		$this->cache = $cache;
 		$this->notifMapper = $notifMapper;
+		$this->targetPageMapper = $targetPageMapper;
 	}
 
 	/**
@@ -71,7 +80,8 @@ class MWEchoNotifUser {
 		return new MWEchoNotifUser(
 			$user, $wgMemc,
 			new EchoUserNotificationGateway( $user, MWEchoDbFactory::newFromDefault() ),
-			new EchoNotificationMapper()
+			new EchoNotificationMapper(),
+			new EchoTargetPageMapper()
 		);
 	}
 
@@ -266,6 +276,9 @@ class MWEchoNotifUser {
 
 		$res = $this->userNotifGateway->markRead( $eventIds );
 		if ( $res ) {
+			// Delete records from echo_target_page
+			$this->targetPageMapper->deleteByUserEvents( $this->mUser, $eventIds );
+			// Update notification count in cache
 			$this->resetNotificationCount( DB_MASTER );
 		}
 		return $res;
@@ -297,23 +310,28 @@ class MWEchoNotifUser {
 		$eventTypes = $attributeManager->getUserEnabledEventsbySections( $this->mUser, 'web', $sections );
 
 		$notifs = $this->notifMapper->fetchUnreadByUser( $this->mUser, $wgEchoMaxUpdateCount, $eventTypes );
-		$res = $this->markRead( array_map(
-			function( EchoNotification $notif ) {
-				// This should not happen at all, but just 0 in
+
+		$eventIds = array_filter(
+			array_map( function( EchoNotification $notif ) {
+				// This should not happen at all, but use 0 in
 				// such case so to keep the code running
 				if ( $notif->getEvent() ) {
 					return $notif->getEvent()->getId();
 				} else {
 					return 0;
 				}
-			},
-			$notifs
-		) );
-		if ( $res && count( $notifs ) < $wgEchoMaxUpdateCount ) {
-			$this->flagCacheWithNoTalkNotification();
+			}, $notifs )
+		);
+
+		$res = $this->markRead( $eventIds );
+		if ( $res ) {
+			// Delete records from echo_target_page
+			$this->targetPageMapper->deleteByUserEvents( $this->mUser, $eventIds );
+			if ( count( $notifs ) < $wgEchoMaxUpdateCount ) {
+				$this->flagCacheWithNoTalkNotification();
+			}
 		}
 		return $res;
-
 	}
 
 	/**

tests/NotifUserTest.php

@@ -94,7 +94,8 @@ class MWEchoNotifUserTest extends MediaWikiTestCase {
 			User::newFromId( 2 ),
 			$wgMemc,
 			$this->mockEchoUserNotificationGateway( array( 'markRead' => true ) ),
-			$this->mockEchoNotificationMapper()
+			$this->mockEchoNotificationMapper(),
+			$this->mockEchoTargetPageMapper()
 		);
 		$this->assertFalse( $notifUser->markRead( array() ) );
 		$this->assertTrue( $notifUser->markRead( array( 1 ) ) );
@@ -103,7 +104,8 @@ class MWEchoNotifUserTest extends MediaWikiTestCase {
 			User::newFromId( 2 ),
 			$wgMemc,
 			$this->mockEchoUserNotificationGateway( array( 'markRead' => false ) ),
-			$this->mockEchoNotificationMapper()
+			$this->mockEchoNotificationMapper(),
+			$this->mockEchoTargetPageMapper()
 		);
 		$this->assertFalse( $notifUser->markRead( array() ) );
 		$this->assertFalse( $notifUser->markRead( array( 1 ) ) );
@@ -117,7 +119,8 @@ class MWEchoNotifUserTest extends MediaWikiTestCase {
 			User::newFromId( 2 ),
 			$wgMemc,
 			$this->mockEchoUserNotificationGateway( array( 'markRead' => true ) ),
-			$this->mockEchoNotificationMapper( array( $this->mockEchoNotification() ) )
+			$this->mockEchoNotificationMapper( array( $this->mockEchoNotification() ) ),
+			$this->mockEchoTargetPageMapper()
 		);
 		$this->assertTrue( $notifUser->markAllRead() );
 
@@ -126,7 +129,8 @@ class MWEchoNotifUserTest extends MediaWikiTestCase {
 			User::newFromId( 2 ),
 			$wgMemc,
 			$this->mockEchoUserNotificationGateway( array( 'markRead' => false ) ),
-			$this->mockEchoNotificationMapper( array( $this->mockEchoNotification() ) )
+			$this->mockEchoNotificationMapper( array( $this->mockEchoNotification() ) ),
+			$this->mockEchoTargetPageMapper()
 		);
 		$this->assertFalse( $notifUser->markAllRead() );
 
@@ -135,7 +139,8 @@ class MWEchoNotifUserTest extends MediaWikiTestCase {
 			User::newFromId( 2 ),
 			$wgMemc,
 			$this->mockEchoUserNotificationGateway( array( 'markRead' => true ) ),
-			$this->mockEchoNotificationMapper()
+			$this->mockEchoNotificationMapper(),
+			$this->mockEchoTargetPageMapper()
 		);
 		$this->assertFalse( $notifUser->markAllRead() );
 
@@ -144,7 +149,8 @@ class MWEchoNotifUserTest extends MediaWikiTestCase {
 			User::newFromId( 2 ),
 			$wgMemc,
 			$this->mockEchoUserNotificationGateway( array( 'markRead' => false ) ),
-			$this->mockEchoNotificationMapper()
+			$this->mockEchoNotificationMapper(),
+			$this->mockEchoTargetPageMapper()
 		);
 		$this->assertFalse( $notifUser->markAllRead() );
 	}
@@ -172,6 +178,16 @@ class MWEchoNotifUserTest extends MediaWikiTestCase {
 		return $mapper;
 	}
 
+	public function mockEchoTargetPageMapper( array $result = array() ) {
+		$mapper = $this->getMockBuilder( 'EchoTargetPageMapper' )
+			->disableOriginalConstructor()
+			->getMock();
+		$mapper->expects( $this->any() )
+			->method( 'deleteByUserEvents' )
+			->will( $this->returnValue( $result ) );
+		return $mapper;
+	}
+
 	protected function mockEchoNotification() {
 		$notification = $this->getMockBuilder( 'EchoNotification' )
 			->disableOriginalConstructor()