From 413fcc12f6e9a1812958b3e0f81db46e3db230eb Mon Sep 17 00:00:00 2001 From: petarpetkovic Date: Wed, 10 Jan 2018 17:26:29 +0100 Subject: [PATCH] Fix seen notifications appearing as unseen on mobile Important note: Make sure to distinguish unseen from unread One way to reproduce minerva and non-minerva notification inconsistencies: - Have all your alerts and notices seen. This is displayed with grayed out number on vector skin or no number at all, if you have (marked as) read. - Generate new alert or notice (one is enough) in your preferred way. - You can check minerva and non-minerva at this step. Both should be in sync. But don't perform any additional action. - Open the notification popup in some non-minerva skin (I have tried with vector and monobook), marking it as seen. - Check the notification icon in minerva. At this point, you should see notification displayed as unseen. The reason bug appeared in the first place is that alert/notice timestamps were mixed up when seen time is obtained. We get seen time from EchoSeenTime class, where we get smaller of the two timestamps, using PHP method `min()`. See I27109ee6a248. Then, we get last unread notification timestamp (which can be either alert or notice), and compare that to seen time. That leads to the situation when you have only one of alerts or notices with unread items, smaller timestamp is used for seen, and most recent for unread, at which point we compare timestamps for two separate things. Previous behavior of getting seen timestamps (using max instead of min) would probably solve the problem, but some other inconsistencies might arrise. This should prevent any weird and unpredictable behavior to happen. Bug: T183076 Change-Id: I20bbd6c590086b1c3eccf82983aad59eb3144a7a --- includes/skins/SkinMinerva.php | 34 +++++++---- tests/phpunit/skins/SkinMinervaTest.php | 80 ++++++++++++++++++------- 2 files changed, 81 insertions(+), 33 deletions(-) diff --git a/includes/skins/SkinMinerva.php b/includes/skins/SkinMinerva.php index b0fc312..cfc9ee5 100644 --- a/includes/skins/SkinMinerva.php +++ b/includes/skins/SkinMinerva.php @@ -346,12 +346,13 @@ class SkinMinerva extends SkinTemplate implements ICustomizableSkin { } /** - * Get the last time user has seen Echo notifications + * Get the last time user has seen particular type of notifications. * @param User $user + * @param string $type Type of seen time to get * @return string|bool Timestamp in TS_ISO_8601 format, or false if no stored time */ - protected function getEchoSeenTime( User $user ) { - return EchoSeenTime::newFromUser( $user )->getTime( 'all', TS_ISO_8601 ); + protected function getEchoSeenTime( User $user, $type = 'all' ) { + return EchoSeenTime::newFromUser( $user )->getTime( $type, TS_ISO_8601 ); } /** @@ -388,20 +389,27 @@ class SkinMinerva extends SkinTemplate implements ICustomizableSkin { // Don't show the secondary button at all $notificationsTitle = null; } else { - $notifUser = $this->getEchoNotifUser( $user ); - $echoSeenTime = $this->getEchoSeenTime( $user ); - $notificationsMsg = $this->msg( 'mobile-frontend-user-button-tooltip' )->text(); - $notifLastUnreadTime = $notifUser->getLastUnreadNotificationTime(); + + $notifUser = $this->getEchoNotifUser( $user ); $count = $notifUser->getNotificationCount(); + $seenAlertTime = $this->getEchoSeenTime( $user, 'alert' ); + $seenMsgTime = $this->getEchoSeenTime( $user, 'message' ); + + $alertNotificationTimestamp = $notifUser->getLastUnreadAlertTime(); + $msgNotificationTimestamp = $notifUser->getLastUnreadMessageTime(); + $isZero = $count === 0; - $hasUnseen = ( - $count > 0 && - $echoSeenTime !== false && - $notifLastUnreadTime !== false && - $echoSeenTime < $notifLastUnreadTime->getTimestamp( TS_ISO_8601 ) - ); + $hasUnseen = $count > 0 && + ( + $seenMsgTime !== false && $msgNotificationTimestamp !== false && + $seenMsgTime < $msgNotificationTimestamp->getTimestamp( TS_ISO_8601 ) + ) || + ( + $seenAlertTime !== false && $alertNotificationTimestamp !== false && + $seenAlertTime < $alertNotificationTimestamp->getTimestamp( TS_ISO_8601 ) + ); $countLabel = $this->getFormattedEchoNotificationCount( $count ); } diff --git a/tests/phpunit/skins/SkinMinervaTest.php b/tests/phpunit/skins/SkinMinervaTest.php index 5a44e38..8de74ec 100644 --- a/tests/phpunit/skins/SkinMinervaTest.php +++ b/tests/phpunit/skins/SkinMinervaTest.php @@ -20,12 +20,18 @@ class Template extends QuickTemplate { } class EchoNotifUser { - public function __construct( $echoLastUnreadNotificationTime, $echoNotificationCount ) { - $this->echoLastUnreadNotificationTime = $echoLastUnreadNotificationTime; + public function __construct( + $lastUnreadAlertTime, $lastUnreadMessageTime, $echoNotificationCount + ) { + $this->lastUnreadAlertTime = $lastUnreadAlertTime; + $this->lastUnreadMessageTime = $lastUnreadMessageTime; $this->echoNotificationCount = $echoNotificationCount; } - public function getLastUnreadNotificationTime() { - return $this->echoLastUnreadNotificationTime; + public function getLastUnreadAlertTime() { + return $this->lastUnreadAlertTime; + } + public function getLastUnreadMessageTime() { + return $this->lastUnreadMessageTime; } public function getNotificationCount() { return $this->echoNotificationCount; @@ -204,15 +210,17 @@ class SkinMinervaTest extends MediaWikiTestCase { * @param bool $useEcho Whether to use Extension:Echo * @param bool $isUserLoggedIn * @param string $newtalks New talk page messages for the current user - * @param MWTimestamp|bool $echoLastUnreadNotificationTime Timestamp or false + * @param MWTimestamp|bool $lastUnreadAlertTime Timestamp or false + * @param MWTimestamp|bool $lastUnreadMessageTime Timestamp or false * @param int|bool $echoNotificationCount - * @param string|bool $echoSeenTime String in format TS_ISO_8601 or false + * @param string|bool $alertSeenTime String in format TS_ISO_8601 or false + * @param string|bool $msgSeenTime String in format TS_ISO_8601 or false * @param string|bool $formattedEchoNotificationCount */ public function testPrepareUserButton( $expectedSecondaryButtonData, $message, $title, $useEcho, $isUserLoggedIn, - $newtalks, $echoLastUnreadNotificationTime = false, - $echoNotificationCount = false, $echoSeenTime = false, + $newtalks, $lastUnreadAlertTime = false, $lastUnreadMessageTime = false, + $echoNotificationCount = false, $alertSeenTime = false, $msgSeenTime = false, $formattedEchoNotificationCount = false ) { $user = $this->getMockBuilder( User::class ) @@ -247,12 +255,15 @@ class SkinMinervaTest extends MediaWikiTestCase { ->method( 'getEchoNotifUser' ) ->will( $this->returnValue( new EchoNotifUser( - $echoLastUnreadNotificationTime, $echoNotificationCount + $lastUnreadAlertTime, $lastUnreadMessageTime, $echoNotificationCount ) ) ); $skin->expects( $this->any() ) ->method( 'getEchoSeenTime' ) - ->will( $this->returnValue( $echoSeenTime ) ); + ->will( $this->returnValueMap( [ + [ $user, 'alert', $alertSeenTime ], + [ $user, 'message', $msgSeenTime ] + ] ) ); $skin->expects( $this->any() ) ->method( 'getFormattedEchoNotificationCount' ) ->will( $this->returnValue( $formattedEchoNotificationCount ) ); @@ -313,6 +324,7 @@ class SkinMinervaTest extends MediaWikiTestCase { [ '', 'Echo, logged in, talk page alert', Title::newFromText( 'Special:Notifications' ), true, true, 'newtalks alert' ], + [ $this->getSecondaryButtonExpectedResult( $title, 'Show my notifications', @@ -322,10 +334,29 @@ class SkinMinervaTest extends MediaWikiTestCase { false, true ), 'Echo, logged in, no talk page alerts, 110 notifications, ' + - 'last un-read nofication time after last echo seen time', + 'last un-read alert time after last alert seen time, ' + + 'last un-read message time after last message seen time', $title, true, true, '', MWTimestamp::getInstance( strtotime( '2017-05-11T21:23:20Z' ) ), - 110, '2017-05-11T20:23:20Z', '99+' ], + MWTimestamp::getInstance( strtotime( '2017-05-12T07:18:42Z' ) ), + 110, '2017-05-11T20:28:11Z', '2017-05-11T20:28:11Z', '99+' ], + + [ $this->getSecondaryButtonExpectedResult( + $title, + 'Show my notifications', + 'Notifications', + 3, + '3', + false, + true + ), 'Echo, logged in, no talk page alerts, 3 notifications, ' + + 'last un-read alert time after last alert seen time, ' + + 'last un-read message time before last message seen time', + $title, true, true, '', + MWTimestamp::getInstance( strtotime( '2017-03-26T14:11:48Z' ) ), + MWTimestamp::getInstance( strtotime( '2017-03-27T17:07:57Z' ) ), + 3, '2017-03-25T00:17:44Z', '2017-03-28T19:00:42Z', '3' ], + [ $this->getSecondaryButtonExpectedResult( $title, 'Show my notifications', @@ -335,10 +366,13 @@ class SkinMinervaTest extends MediaWikiTestCase { false, false ), 'Echo, logged in, no talk page alerts, 3 notifications, ' + - 'last un-read nofication time before last echo seen time', + 'last un-read alert time before last alert seen time, ' + + 'last un-read message time before last message seen time', $title, true, true, '', - MWTimestamp::getInstance( strtotime( '2017-05-11T21:23:20Z' ) ), - 3, '2017-05-11T22:23:20Z', '3' ], + MWTimestamp::getInstance( strtotime( '2017-04-11T13:21:15Z' ) ), + MWTimestamp::getInstance( strtotime( '2017-04-10T15:12:31Z' ) ), + 3, '2017-04-12T10:37:13Z', '2017-04-11T12:55:47Z', '3' ], + [ $this->getSecondaryButtonExpectedResult( $title, 'Show my notifications', @@ -348,8 +382,13 @@ class SkinMinervaTest extends MediaWikiTestCase { false, false ), 'Echo, logged in, no talk page alerts, 5 notifications, ' + - 'no last un-read nofication time', - $title, true, true, '', false, 5, '2017-05-11T22:23:20Z', '5' ], + 'no last un-read alert time, ' + + 'last un-read message time before last message seen time', + $title, true, true, '', + false, + MWTimestamp::getInstance( strtotime( '2017-12-15T08:14:33Z' ) ), + 5, '2017-12-21T18:07:24Z', '2017-12-19T16:55:52Z', '5' ], + [ $this->getSecondaryButtonExpectedResult( $title, 'Show my notifications', @@ -359,10 +398,11 @@ class SkinMinervaTest extends MediaWikiTestCase { true, false ), 'Echo, logged in, no talk page alerts, 0 notifications, ' + - 'no last echo seen time', + 'no last alert and message seen time', $title, true, true, '', - MWTimestamp::getInstance( strtotime( '2017-05-11T21:23:20Z' ) ), - 0, false, '0' ] + MWTimestamp::getInstance( strtotime( '2017-08-09T10:54:07Z' ) ), + MWTimestamp::getInstance( strtotime( '2017-08-11T14:18:36Z' ) ), + 0, false, false, '0' ] ]; }