Notifications - fix race condition around "Mark All Read".

Summary:
pre-patch "Mark All Read" marks *all* unread notifications as read. This is a race condition in that the user is looking at some set of notiifcations and that set may update such that the newest notifications aren't shown. An example might be if sitting on the notifications page or having the menu open while a new notification comes in... Note re-opening the menu would show the latest notifications.

This patch makes it so "Mark All Read" links only marks the notifications currently loaded (and older.) Fixes T5764.

Additionally, if there is nothing to "mark read" the button / link "Mark All Read" will have a disabled style and yield a dialog saying "nothing to mark as read".

Test Plan: carefully tracked ?chronoKey populating correctly in various links. Verified query constructed properly too.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T5764

Differential Revision: https://secure.phabricator.com/D10113

src/applications/notification/controller/PhabricatorNotificationClearController.php

@@ -5,6 +5,7 @@ final class PhabricatorNotificationClearController
 
   public function processRequest() {
     $request = $this->getRequest();
+    $chrono_key = $request->getInt('chronoKey');
     $user = $request->getUser();
 
     if ($request->isDialogFormPost()) {
@@ -12,10 +13,11 @@ final class PhabricatorNotificationClearController
 
       queryfx(
         $table->establishConnection('w'),
-        'UPDATE %T SET hasViewed = 1 WHERE
-          userPHID = %s AND hasViewed = 0',
+        'UPDATE %T SET hasViewed = 1 '.
+        'WHERE userPHID = %s AND hasViewed = 0 and chronologicalKey <= %d',
         $table->getTableName(),
-        $user->getPHID());
+        $user->getPHID(),
+        $chrono_key);
 
       return id(new AphrontReloadResponse())
         ->setURI('/notification/');
@@ -23,22 +25,30 @@ final class PhabricatorNotificationClearController
 
     $dialog = new AphrontDialogView();
     $dialog->setUser($user);
-    $dialog->setTitle('Really mark all notifications as read?');
-
-    $is_serious = PhabricatorEnv::getEnvConfig('phabricator.serious-business');
-    if ($is_serious) {
-      $dialog->appendChild(
-        pht(
-          'All unread notifications will be marked as read. You can not '.
-          'undo this action.'));
-    } else {
-      $dialog->appendChild(
-        pht(
-          "You can't ignore your problems forever, you know."));
-    }
-
     $dialog->addCancelButton('/notification/');
-    $dialog->addSubmitButton(pht('Mark All Read'));
+    if ($chrono_key) {
+      $dialog->setTitle('Really mark all notifications as read?');
+      $dialog->addHiddenInput('chronoKey', $chrono_key);
+
+      $is_serious =
+        PhabricatorEnv::getEnvConfig('phabricator.serious-business');
+      if ($is_serious) {
+        $dialog->appendChild(
+          pht(
+            'All unread notifications will be marked as read. You can not '.
+            'undo this action.'));
+      } else {
+        $dialog->appendChild(
+          pht(
+            "You can't ignore your problems forever, you know."));
+      }
+
+      $dialog->addSubmitButton(pht('Mark All Read'));
+    } else {
+      $dialog->setTitle('No notifications to mark as read.');
+      $dialog->appendChild(pht(
+        'You have no unread notifications.'));
+    }
 
     return id(new AphrontDialogResponse())->setDialog($dialog);
   }

src/applications/notification/controller/PhabricatorNotificationListController.php

@@ -39,33 +39,37 @@ final class PhabricatorNotificationListController
         break;
     }
 
-    $notifications = $query->executeWithOffsetPager($pager);
+    $image = id(new PHUIIconView())
+      ->setIconFont('fa-eye-slash');
+    $button = id(new PHUIButtonView())
+      ->setTag('a')
+      ->addSigil('workflow')
+      ->setColor(PHUIButtonView::SIMPLE)
+      ->setIcon($image)
+      ->setText(pht('Mark All Read'));
 
+    $notifications = $query->executeWithOffsetPager($pager);
+    $clear_uri = id(new PhutilURI('/notification/clear/'));
     if ($notifications) {
       $builder = new PhabricatorNotificationBuilder($notifications);
       $builder->setUser($user);
       $view = $builder->buildView()->render();
+      $clear_uri->setQueryParam(
+        'chronoKey',
+        head($notifications)->getChronologicalKey());
     } else {
       $view = phutil_tag_div(
         'phabricator-notification no-notifications',
         $no_data);
+      $button->setDisabled(true);
     }
+    $button->setHref((string) $clear_uri);
 
     $view = id(new PHUIBoxView())
       ->addPadding(PHUI::PADDING_MEDIUM)
       ->addClass('phabricator-notification-list')
       ->appendChild($view);
 
-    $image = id(new PHUIIconView())
-        ->setIconFont('fa-eye-slash');
-    $button = id(new PHUIButtonView())
-        ->setTag('a')
-        ->setColor(PHUIButtonView::SIMPLE)
-        ->setHref('/notification/clear/')
-        ->addSigil('workflow')
-        ->setIcon($image)
-        ->setText(pht('Mark All Read'));
-
     $notif_header = id(new PHUIHeaderView())
       ->setHeader($header)
       ->addActionLink($button);

src/applications/notification/controller/PhabricatorNotificationPanelController.php

@@ -15,15 +15,29 @@ final class PhabricatorNotificationPanelController
 
     $stories = $query->execute();
 
+    $clear_ui_class = 'phabricator-notification-clear-all';
+    $clear_uri = id(new PhutilURI('/notification/clear/'));
     if ($stories) {
       $builder = new PhabricatorNotificationBuilder($stories);
       $notifications_view = $builder->buildView();
       $content = $notifications_view->render();
+      $clear_uri->setQueryParam(
+        'chronoKey',
+        head($stories)->getChronologicalKey());
     } else {
       $content = phutil_tag_div(
         'phabricator-notification no-notifications',
         pht('You have no notifications.'));
+      $clear_ui_class .= ' disabled';
     }
+    $clear_ui = javelin_tag(
+      'a',
+      array(
+        'sigil' => 'workflow',
+        'href' => (string) $clear_uri,
+        'class' => $clear_ui_class,
+      ),
+      pht('Mark All Read'));
 
     $notifications_link = phutil_tag(
       'a',
@@ -50,14 +64,7 @@ final class PhabricatorNotificationPanelController
       '<div class="phabricator-notification-view-all">%s %s %s</div>',
       $header,
       $content,
-      javelin_tag(
-        'a',
-        array(
-          'sigil' => 'workflow',
-          'href' => '/notification/clear/',
-          'class' => 'phabricator-notification-clear-all'
-        ),
-        pht('Mark All Read')),
+      $clear_ui,
       " \xC2\xB7 ",
       phutil_tag(
         'a',