From 7942a2de79c3098468a6351b52a3fd2d0a63a7fb Mon Sep 17 00:00:00 2001 From: Roan Kattouw Date: Mon, 2 May 2016 18:56:13 -0700 Subject: [PATCH] Replace mark-as-read-on-click with ?markasread= URL parameter Notifications were being marked as read in response to a click event in JavaScript, but that causes a jarring effect in the UI, and it's not reliable (the browser could abort the AJAX request). Instead, add ?markasread=XYZ to the end of every primary link URL, where XYZ is the event ID. Bug: T133975 Depends-On: Icb99d5479836fea25a47451b5a758dd71f642f71 Change-Id: I8047d121584b43e6172463a50ad0e0de5f7fa73c --- Hooks.php | 34 ++++++++++++++----- Resources.php | 5 ++- includes/formatters/EchoFlyoutFormatter.php | 2 +- .../formatters/EventPresentationModel.php | 15 +++++++- .../SpecialNotificationsFormatter.php | 2 +- includes/mapper/NotificationMapper.php | 28 +++++++++++++++ modules/ext.echo.init.js | 7 ++++ .../ooui/mw.echo.ui.NotificationItemWidget.js | 3 -- 8 files changed, 80 insertions(+), 16 deletions(-) diff --git a/Hooks.php b/Hooks.php index e687fc661..7c8c2ebfb 100644 --- a/Hooks.php +++ b/Hooks.php @@ -762,11 +762,11 @@ class EchoHooks { // @todo should this really be here? $subtractAlerts = 0; $subtractMessages = 0; + $eventIds = array(); if ( $title->getArticleID() ) { - $mapper = new EchoTargetPageMapper(); - $fetchedTargetPages = $mapper->fetchByUserPageId( $user, $title->getArticleID() ); + $targetPageMapper = new EchoTargetPageMapper(); + $fetchedTargetPages = $targetPageMapper->fetchByUserPageId( $user, $title->getArticleID() ); if ( $fetchedTargetPages ) { - $eventIds = array(); $attribManager = EchoAttributeManager::newFromGlobalVars(); /* @var EchoTargetPage[] $targetPages */ foreach ( $fetchedTargetPages as $id => $targetPages ) { @@ -776,19 +776,35 @@ class EchoHooks { $targetPages[0]->getEventType() ); if ( $section === EchoAttributeManager::MESSAGE ) { - $subtractMessages += 1; + $subtractMessages++; } else { // ALERT - $subtractAlerts += 1; + $subtractAlerts++; } $eventIds[] = $id; } - DeferredUpdates::addCallableUpdate( function () use ( $user, $eventIds ) { - $notifUser = MWEchoNotifUser::newFromUser( $user ); - $notifUser->markRead( $eventIds ); - } ); } } + // Attempt to mark as read the event ID in the ?markasread= parameter, if present + $markAsReadId = $sk->getOutput()->getRequest()->getInt( 'markasread' ); + if ( $markAsReadId !== 0 && !in_array( $markAsReadId, $eventIds ) ) { + $notifMapper = new EchoNotificationMapper(); + $notif = $notifMapper->fetchByUserEvent( $user, $markAsReadId ); + if ( $notif && !$notif->getReadTimestamp() ) { + if ( $notif->getEvent()->getSection() === EchoAttributeManager::MESSAGE ) { + $subtractMessages++; + } else { + $subtractAlerts++; + } + $eventIds[] = $markAsReadId; + } + } + if ( $eventIds ) { + DeferredUpdates::addCallableUpdate( function () use ( $user, $eventIds ) { + $notifUser = MWEchoNotifUser::newFromUser( $user ); + $notifUser->markRead( $eventIds ); + } ); + } // Add a "My notifications" item to personal URLs $notifUser = MWEchoNotifUser::newFromUser( $user ); diff --git a/Resources.php b/Resources.php index d0c522937..09dd24116 100644 --- a/Resources.php +++ b/Resources.php @@ -205,7 +205,10 @@ $wgResourceModules += array( 'scripts' => array( 'ext.echo.init.js', ), - 'dependencies' => array( 'ext.echo.api' ), + 'dependencies' => array( + 'ext.echo.api', + 'mediawiki.Uri', + ), 'targets' => array( 'desktop' ), ), // Base no-js styles diff --git a/includes/formatters/EchoFlyoutFormatter.php b/includes/formatters/EchoFlyoutFormatter.php index d94fc273c..63fe16b4c 100644 --- a/includes/formatters/EchoFlyoutFormatter.php +++ b/includes/formatters/EchoFlyoutFormatter.php @@ -50,7 +50,7 @@ class EchoFlyoutFormatter extends EchoEventFormatter { ) . "\n"; // Add the primary link afterwards, if it has one - $primaryLink = $model->getPrimaryLink(); + $primaryLink = $model->getPrimaryLinkWithMarkAsRead(); if ( $primaryLink !== false ) { $html .= Html::element( 'a', diff --git a/includes/formatters/EventPresentationModel.php b/includes/formatters/EventPresentationModel.php index bf18369d9..9e5eb98ff 100644 --- a/includes/formatters/EventPresentationModel.php +++ b/includes/formatters/EventPresentationModel.php @@ -326,6 +326,19 @@ abstract class EchoEventPresentationModel implements JsonSerializable { */ abstract public function getPrimaryLink(); + /** + * Like getPrimaryLink(), but with the URL altered to add ?markasread=XYZ. When this link is followed, + * the notification is marked as read. + * @return array|bool + */ + public function getPrimaryLinkWithMarkAsRead() { + $primaryLink = $this->getPrimaryLink(); + if ( $primaryLink ) { + $primaryLink['url'] = wfAppendQuery( $primaryLink['url'], array( 'markasread' => $this->event->getId() ) ); + } + return $primaryLink; + } + /** * Array of secondary link details, including possibly-relative URLs, label, * description & icon name. @@ -358,7 +371,7 @@ abstract class EchoEventPresentationModel implements JsonSerializable { 'body' => $body ? $body->escaped() : '', 'icon' => $this->getIconType(), 'links' => array( - 'primary' => $this->getPrimaryLink() ?: array(), + 'primary' => $this->getPrimaryLinkWithMarkAsRead() ?: array(), 'secondary' => array_values( array_filter( $this->getSecondaryLinks() ) ), ), ); diff --git a/includes/formatters/SpecialNotificationsFormatter.php b/includes/formatters/SpecialNotificationsFormatter.php index 4265cc176..e9846b4e5 100644 --- a/includes/formatters/SpecialNotificationsFormatter.php +++ b/includes/formatters/SpecialNotificationsFormatter.php @@ -38,7 +38,7 @@ class SpecialNotificationsFormatter extends EchoEventFormatter { // Add links to the footer, primary goes first, then secondary ones $links = array(); - $primaryLink = $model->getPrimaryLink(); + $primaryLink = $model->getPrimaryLinkWithMarkAsRead(); if ( $primaryLink !== false ) { $links[] = $primaryLink; } diff --git a/includes/mapper/NotificationMapper.php b/includes/mapper/NotificationMapper.php index bdcdc8b0f..2f0c33100 100644 --- a/includes/mapper/NotificationMapper.php +++ b/includes/mapper/NotificationMapper.php @@ -260,6 +260,34 @@ class EchoNotificationMapper extends EchoAbstractMapper { } } + /** + * Create an EchoNotification by user and event ID. + * + * @param User $user + * @param int $eventID + * @return EchoNotification|bool + */ + public function fetchByUserEvent( User $user, $eventId ) { + $dbr = $this->dbFactory->getEchoDb( DB_SLAVE ); + + $row = $dbr->selectRow( + array( 'echo_notification', 'echo_event' ), + '*', + array( + 'notification_user' => $user->getId(), + 'notification_event' => $eventId + ), + __METHOD__ + ); + + if ( $row ) { + return EchoNotification::newFromRow( $row ); + } else { + return false; + } + } + + /** * Fetch a notification by user in the specified offset. The caller should * know that passing a big number for offset is NOT going to work diff --git a/modules/ext.echo.init.js b/modules/ext.echo.init.js index f684d0d30..d87be5518 100644 --- a/modules/ext.echo.init.js +++ b/modules/ext.echo.init.js @@ -1,6 +1,13 @@ ( function ( mw, $ ) { 'use strict'; + // Remove ?markasread=XYZ from the URL + var uri = new mw.Uri(); + if ( uri.query.markasread !== undefined ) { + delete uri.query.markasread; + window.history.replaceState( null, document.title, uri ); + } + mw.echo = mw.echo || {}; // Activate ooui diff --git a/modules/ooui/mw.echo.ui.NotificationItemWidget.js b/modules/ooui/mw.echo.ui.NotificationItemWidget.js index 26621cf0b..58ecdaaa0 100644 --- a/modules/ooui/mw.echo.ui.NotificationItemWidget.js +++ b/modules/ooui/mw.echo.ui.NotificationItemWidget.js @@ -198,9 +198,6 @@ // Source of this notification if it is cross-wiki widget.bundle ? widget.getModel().getSource() : '' ); - - // Toggle the notification as read - widget.model.toggleRead( true ); } ) ); }