Improve behavior when user submits a no-op action in Differential

Summary:
See T730 and the slightly-less-pretty version of this in D1398.

When a user takes an action in Differential that has no effect (for instance,
accepting an already-accepted revision), prompt them:

  Action Has No Effect

  You can not accept this revision because it has already been accepted.

  Do you want to post the feedback anyway, as a normal comment?

                        [Cancel] [Post as Comment]

If they have no comment text, the dialog only says "Cancel".

I think this is probably the best way to balance all the concerns here -- it
might occasionally be a little annoying, but that should be rare, and it should
never be confusing (the current workflow is extremely confusing).

This also fixes the issue where you can add all sorts of CCs who are already
part of the revision, either explicitly or via mentions.

Test Plan:
Posted some has-effect and has-no-effect comments, made different
choices in the dialog, everything seems to work OK?

Reviewers: vrana, btrahan, jungejason

Reviewed By: vrana

CC: aran, vrana

Maniphest Tasks: T730

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

src/__phutil_library_map__.php

@@ -160,6 +160,7 @@ phutil_register_library_map(array(
     'DarkConsoleXHProfPluginAPI' => 'aphront/console/plugin/xhprof/api',
     'DatabaseConfigurationProvider' => 'applications/base/storage/configuration',
     'DifferentialAction' => 'applications/differential/constants/action',
+    'DifferentialActionHasNoEffectException' => 'applications/differential/exception/noeffect',
     'DifferentialAddCommentView' => 'applications/differential/view/addcomment',
     'DifferentialAffectedPath' => 'applications/differential/storage/affectedpath',
     'DifferentialApplyPatchFieldSpecification' => 'applications/differential/field/specification/applypatch',
@@ -191,6 +192,7 @@ phutil_register_library_map(array(
     'DifferentialDiffProperty' => 'applications/differential/storage/diffproperty',
     'DifferentialDiffTableOfContentsView' => 'applications/differential/view/difftableofcontents',
     'DifferentialDiffViewController' => 'applications/differential/controller/diffview',
+    'DifferentialException' => 'applications/differential/exception/base',
     'DifferentialExceptionMail' => 'applications/differential/mail/exception',
     'DifferentialExportPatchFieldSpecification' => 'applications/differential/field/specification/exportpatch',
     'DifferentialFieldDataNotAvailableException' => 'applications/differential/field/exception/notavailable',
@@ -908,6 +910,7 @@ phutil_register_library_map(array(
     'DarkConsoleRequestPlugin' => 'DarkConsolePlugin',
     'DarkConsoleServicesPlugin' => 'DarkConsolePlugin',
     'DarkConsoleXHProfPlugin' => 'DarkConsolePlugin',
+    'DifferentialActionHasNoEffectException' => 'DifferentialException',
     'DifferentialAddCommentView' => 'AphrontView',
     'DifferentialAffectedPath' => 'DifferentialDAO',
     'DifferentialApplyPatchFieldSpecification' => 'DifferentialFieldSpecification',

src/applications/differential/controller/commentsave/DifferentialCommentSaveController.php

@@ -46,13 +46,44 @@ class DifferentialCommentSaveController extends DifferentialController {
         'ip' => $request->getRemoteAddr(),
       ));
 
-    $editor
+    try {
-      ->setMessage($comment)
+      $editor
-      ->setContentSource($content_source)
+        ->setMessage($comment)
-      ->setAttachInlineComments(true)
+        ->setContentSource($content_source)
-      ->setAddedReviewers($reviewers)
+        ->setAttachInlineComments(true)
-      ->setAddedCCs($ccs)
+        ->setAddedReviewers($reviewers)
-      ->save();
+        ->setAddedCCs($ccs)
+        ->save();
+    } catch (DifferentialActionHasNoEffectException $no_effect) {
+      $has_inlines = id(new DifferentialInlineComment())->loadAllWhere(
+        'authorPHID = %s AND revisionID = %d AND commentID IS NULL',
+        $request->getUser()->getPHID(),
+        $revision->getID());
+
+      $dialog = new AphrontDialogView();
+      $dialog->setUser($request->getUser());
+      $dialog->addCancelButton('/D'.$revision_id);
+
+      $dialog->addHiddenInput('revision_id',  $revision_id);
+      $dialog->addHiddenInput('action',       'none');
+      $dialog->addHiddenInput('reviewers',    $reviewers);
+      $dialog->addHiddenInput('ccs',          $ccs);
+      $dialog->addHiddenInput('comment',      $comment);
+
+      $dialog->setTitle('Action Has No Effect');
+      $dialog->appendChild(
+        '<p>'.phutil_escape_html($no_effect->getMessage()).'</p>');
+
+      if (strlen($comment) || $has_inlines) {
+        $dialog->addSubmitButton('Post as Comment');
+        $dialog->appendChild('<br />');
+        $dialog->appendChild(
+          '<p>Do you want to post your feedback anyway, as a normal '.
+          'comment?</p>');
+      }
+
+      return id(new AphrontDialogResponse())->setDialog($dialog);
+    }
 
     // TODO: Diff change detection?
 

src/applications/differential/controller/commentsave/__init__.php

@@ -7,13 +7,17 @@
 
 
 phutil_require_module('phabricator', 'aphront/response/400');
+phutil_require_module('phabricator', 'aphront/response/dialog');
 phutil_require_module('phabricator', 'aphront/response/redirect');
 phutil_require_module('phabricator', 'applications/differential/controller/base');
 phutil_require_module('phabricator', 'applications/differential/editor/comment');
+phutil_require_module('phabricator', 'applications/differential/storage/inlinecomment');
 phutil_require_module('phabricator', 'applications/differential/storage/revision');
 phutil_require_module('phabricator', 'applications/draft/storage/draft');
 phutil_require_module('phabricator', 'applications/metamta/contentsource/source');
+phutil_require_module('phabricator', 'view/dialog');
 
+phutil_require_module('phutil', 'markup');
 phutil_require_module('phutil', 'utils');
 
 

src/applications/differential/editor/comment/DifferentialCommentEditor.php

@@ -105,40 +105,58 @@ class DifferentialCommentEditor {
 
     $metadata = array();
 
+    $inline_comments = array();
+    if ($this->attachInlineComments) {
+      $inline_comments = id(new DifferentialInlineComment())->loadAllWhere(
+        'authorPHID = %s AND revisionID = %d AND commentID IS NULL',
+        $this->actorPHID,
+        $revision->getID());
+    }
+
     switch ($action) {
       case DifferentialAction::ACTION_COMMENT:
+        if (!$this->message && !$inline_comments) {
+          throw new DifferentialActionHasNoEffectException(
+            "You are submitting an empty comment with no action: ".
+            "you must act on the revision or post a comment.");
+        }
         break;
 
       case DifferentialAction::ACTION_RESIGN:
         if ($actor_is_author) {
           throw new Exception('You can not resign from your own revision!');
         }
-        if (isset($reviewer_phids[$actor_phid])) {
+        if (empty($reviewer_phids[$actor_phid])) {
-          DifferentialRevisionEditor::alterReviewers(
+          throw new DifferentialActionHasNoEffectException(
-            $revision,
+            "You can not resign from this revision because you are not ".
-            $reviewer_phids,
+            "a reviewer.");
-            $rem = array($actor_phid),
-            $add = array(),
-            $actor_phid);
         }
+        DifferentialRevisionEditor::alterReviewers(
+          $revision,
+          $reviewer_phids,
+          $rem = array($actor_phid),
+          $add = array(),
+          $actor_phid);
         break;
 
       case DifferentialAction::ACTION_ABANDON:
         if (!($actor_is_author || $actor_is_admin)) {
-          throw new Exception('You can only abandon your revisions.');
+          throw new Exception('You can only abandon your own revisions.');
-        }
-        if ($revision_status ==
-            ArcanistDifferentialRevisionStatus::COMMITTED) {
-          throw new Exception('You can not abandon a committed revision.');
-        }
-        if ($revision_status ==
-            ArcanistDifferentialRevisionStatus::ABANDONED) {
-          $action = DifferentialAction::ACTION_COMMENT;
-          break;
         }
 
-        $revision
+        if ($revision_status == ArcanistDifferentialRevisionStatus::COMMITTED) {
-          ->setStatus(ArcanistDifferentialRevisionStatus::ABANDONED);
+          throw new DifferentialActionHasNoEffectException(
+            "You can not abandon this revision because it has already ".
+            "been committed.");
+        }
+
+        if ($revision_status == ArcanistDifferentialRevisionStatus::ABANDONED) {
+          throw new DifferentialActionHasNoEffectException(
+            "You can not abandon this revision because it has already ".
+            "been abandoned.");
+        }
+
+        $revision->setStatus(ArcanistDifferentialRevisionStatus::ABANDONED);
         break;
 
       case DifferentialAction::ACTION_ACCEPT:
@@ -149,8 +167,24 @@ class DifferentialCommentEditor {
              ArcanistDifferentialRevisionStatus::NEEDS_REVIEW) &&
             ($revision_status !=
              ArcanistDifferentialRevisionStatus::NEEDS_REVISION)) {
-          $action = DifferentialAction::ACTION_COMMENT;
+
-          break;
+          switch ($revision_status) {
+            case ArcanistDifferentialRevisionStatus::ACCEPTED:
+              throw new DifferentialActionHasNoEffectException(
+                "You can not accept this revision because someone else ".
+                "already accepted it.");
+            case ArcanistDifferentialRevisionStatus::ABANDONED:
+              throw new DifferentialActionHasNoEffectException(
+                "You can not accept this revision because it has been ".
+                "abandoned.");
+            case ArcanistDifferentialRevisionStatus::COMMITTED:
+              throw new DifferentialActionHasNoEffectException(
+                "You can not accept this revision because it has already ".
+                "been committed.");
+            default:
+              throw new Exception(
+                "Unexpected revision state '{$revision_status}'!");
+          }
         }
 
         $revision
@@ -170,16 +204,30 @@ class DifferentialCommentEditor {
         if (!$actor_is_author) {
           throw new Exception('You must own a revision to request review.');
         }
-        if (($revision_status !=
+
-             ArcanistDifferentialRevisionStatus::NEEDS_REVISION) &&
+        switch ($revision_status) {
-            ($revision_status !=
+          case ArcanistDifferentialRevisionStatus::ACCEPTED:
-             ArcanistDifferentialRevisionStatus::ACCEPTED)) {
+          case ArcanistDifferentialRevisionStatus::NEEDS_REVISION:
-          $action = DifferentialAction::ACTION_COMMENT;
+            $revision->setStatus(
-          break;
+              ArcanistDifferentialRevisionStatus::NEEDS_REVIEW);
+            break;
+          case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW:
+            throw new DifferentialActionHasNoEffectException(
+              "You can not request review of this revision because it has ".
+              "been abandoned.");
+          case ArcanistDifferentialRevisionStatus::ABANDONED:
+            throw new DifferentialActionHasNoEffectException(
+              "You can not request review of this revision because it has ".
+              "been abandoned.");
+          case ArcanistDifferentialRevisionStatus::COMMITTED:
+            throw new DifferentialActionHasNoEffectException(
+              "You can not request review of this revision because it has ".
+              "already been committed.");
+          default:
+            throw new Exception(
+              "Unexpected revision state '{$revision_status}'!");
         }
 
-        $revision
-          ->setStatus(ArcanistDifferentialRevisionStatus::NEEDS_REVIEW);
         break;
 
       case DifferentialAction::ACTION_REJECT:
@@ -187,12 +235,26 @@ class DifferentialCommentEditor {
           throw new Exception(
             'You can not request changes to your own revision.');
         }
-        if (($revision_status !=
+
-             ArcanistDifferentialRevisionStatus::NEEDS_REVIEW) &&
+        switch ($revision_status) {
-            ($revision_status !=
+          case ArcanistDifferentialRevisionStatus::ACCEPTED:
-             ArcanistDifferentialRevisionStatus::ACCEPTED)) {
+          case ArcanistDifferentialRevisionStatus::NEEDS_REVISION:
-          $action = DifferentialAction::ACTION_COMMENT;
+          case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW:
-          break;
+            // NOTE: We allow you to reject an already-rejected revision
+            // because it doesn't create any ambiguity and avoids a rather
+            // needless dialog.
+            break;
+          case ArcanistDifferentialRevisionStatus::ABANDONED:
+            throw new DifferentialActionHasNoEffectException(
+              "You can not request changes to this revision because it has ".
+              "been abandoned.");
+          case ArcanistDifferentialRevisionStatus::COMMITTED:
+            throw new DifferentialActionHasNoEffectException(
+              "You can not request changes to this revision because it has ".
+              "already been committed.");
+          default:
+            throw new Exception(
+              "Unexpected revision state '{$revision_status}'!");
         }
 
         if (!isset($reviewer_phids[$actor_phid])) {
@@ -213,12 +275,23 @@ class DifferentialCommentEditor {
           throw new Exception(
             "You can not plan changes to somebody else's revision");
         }
-        if (($revision_status !=
+
-             ArcanistDifferentialRevisionStatus::NEEDS_REVIEW) &&
+        switch ($revision_status) {
-            ($revision_status !=
+          case ArcanistDifferentialRevisionStatus::ACCEPTED:
-             ArcanistDifferentialRevisionStatus::ACCEPTED)) {
+          case ArcanistDifferentialRevisionStatus::NEEDS_REVISION:
-          $action = DifferentialAction::ACTION_COMMENT;
+          case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW:
-          break;
+            break;
+          case ArcanistDifferentialRevisionStatus::ABANDONED:
+            throw new DifferentialActionHasNoEffectException(
+              "You can not plan changes to this revision because it has ".
+              "been abandoned.");
+          case ArcanistDifferentialRevisionStatus::COMMITTED:
+            throw new DifferentialActionHasNoEffectException(
+              "You can not plan changes to this revision because it has ".
+              "already been committed.");
+          default:
+            throw new Exception(
+              "Unexpected revision state '{$revision_status}'!");
         }
 
         $revision
@@ -229,11 +302,13 @@ class DifferentialCommentEditor {
         if (!$actor_is_author) {
           throw new Exception('You can not reclaim a revision you do not own.');
         }
-        if ($revision_status !=
+
-            ArcanistDifferentialRevisionStatus::ABANDONED) {
+
-          $action = DifferentialAction::ACTION_COMMENT;
+        if ($revision_status != ArcanistDifferentialRevisionStatus::ABANDONED) {
-          break;
+          throw new DifferentialActionHasNoEffectException(
+            "You can not reclaim this revision because it is not abandoned.");
         }
+
         $revision
           ->setStatus(ArcanistDifferentialRevisionStatus::NEEDS_REVIEW);
         break;
@@ -245,6 +320,8 @@ class DifferentialCommentEditor {
 
       case DifferentialAction::ACTION_ADDREVIEWERS:
         $added_reviewers = $this->getAddedReviewers();
+        $user_tried_to_add = count($added_reviewers);
+
         foreach ($added_reviewers as $k => $user_phid) {
           if ($user_phid == $revision->getAuthorPHID()) {
             unset($added_reviewers[$k]);
@@ -268,21 +345,27 @@ class DifferentialCommentEditor {
           $metadata[$key] = $added_reviewers;
 
         } else {
-          $action = DifferentialAction::ACTION_COMMENT;
+          if ($user_tried_to_add == 0) {
+            throw new DifferentialActionHasNoEffectException(
+              "You can not add reviewers, because you did not specify any ".
+              "reviewers.");
+          } else if ($user_tried_to_add == 1) {
+            throw new DifferentialActionHasNoEffectException(
+              "You can not add that reviewer, because they are already an ".
+              "author or reviewer.");
+          } else {
+            throw new DifferentialActionHasNoEffectException(
+              "You can not add those reviewers, because they are all already ".
+              "authors or reviewers.");
+          }
         }
+
         break;
       case DifferentialAction::ACTION_ADDCCS:
         $added_ccs = $this->getAddedCCs();
+        $user_tried_to_add = count($added_ccs);
 
-        $current_ccs = $revision->getCCPHIDs();
+        $added_ccs = $this->filterAddedCCs($added_ccs);
-        if ($current_ccs) {
-          $current_ccs = array_fill_keys($current_ccs, true);
-          foreach ($added_ccs as $k => $cc) {
-            if (isset($current_ccs[$cc])) {
-              unset($added_ccs[$k]);
-            }
-          }
-        }
 
         if ($added_ccs) {
           foreach ($added_ccs as $cc) {
@@ -296,7 +379,19 @@ class DifferentialCommentEditor {
           $metadata[$key] = $added_ccs;
 
         } else {
-          $action = DifferentialAction::ACTION_COMMENT;
+          if ($user_tried_to_add == 0) {
+            throw new DifferentialActionHasNoEffectException(
+              "You can not add CCs, because you did not specify any ".
+              "CCs.");
+          } else if ($user_tried_to_add == 1) {
+            throw new DifferentialActionHasNoEffectException(
+              "You can not add that CC, because they are already an ".
+              "author, reviewer or CC.");
+          } else {
+            throw new DifferentialActionHasNoEffectException(
+              "You can not add those CCs, because they are all already ".
+              "authors, reviewers or CCs.");
+          }
         }
         break;
       default:
@@ -318,14 +413,6 @@ class DifferentialCommentEditor {
         $this->actorPHID);
     }
 
-    $inline_comments = array();
-    if ($this->attachInlineComments) {
-      $inline_comments = id(new DifferentialInlineComment())->loadAllWhere(
-        'authorPHID = %s AND revisionID = %d AND commentID IS NULL',
-        $this->actorPHID,
-        $revision->getID());
-    }
-
     $comment = id(new DifferentialComment())
       ->setAuthorPHID($this->actorPHID)
       ->setRevisionID($revision->getID())
@@ -362,15 +449,7 @@ class DifferentialCommentEditor {
     $mention_ccs = PhabricatorMarkupEngine::extractPHIDsFromMentions(
       $content_blocks);
     if ($mention_ccs) {
-      $current_ccs = $revision->getCCPHIDs();
+      $mention_ccs = $this->filterAddedCCs($mention_ccs);
-      if ($current_ccs) {
-        $current_ccs = array_fill_keys($current_ccs, true);
-        foreach ($mention_ccs as $key => $mention_cc) {
-          if (isset($current_ccs[$mention_cc])) {
-            unset($mention_ccs[$key]);
-          }
-        }
-      }
       if ($mention_ccs) {
         $metadata = $comment->getMetadata();
         $metacc = idx(
@@ -447,4 +526,28 @@ class DifferentialCommentEditor {
     return $comment;
   }
 
+  private function filterAddedCCs(array $ccs) {
+    $revision = $this->revision;
+
+    $current_ccs = $revision->getCCPHIDs();
+    $current_ccs = array_fill_keys($current_ccs, true);
+
+    $reviewer_phids = $revision->getReviewers();
+    $reviewer_phids = array_fill_keys($reviewer_phids, true);
+
+    foreach ($ccs as $key => $cc) {
+      if (isset($current_ccs[$cc])) {
+        unset($ccs[$key]);
+      }
+      if (isset($reviewer_phids[$cc])) {
+        unset($ccs[$key]);
+      }
+      if ($cc == $revision->getAuthorPHID()) {
+        unset($ccs[$key]);
+      }
+    }
+
+    return $ccs;
+  }
+
 }

src/applications/differential/editor/comment/__init__.php

@@ -10,6 +10,7 @@ phutil_require_module('arcanist', 'differential/constants/revisionstatus');
 
 phutil_require_module('phabricator', 'applications/differential/constants/action');
 phutil_require_module('phabricator', 'applications/differential/editor/revision');
+phutil_require_module('phabricator', 'applications/differential/exception/noeffect');
 phutil_require_module('phabricator', 'applications/differential/mail/comment');
 phutil_require_module('phabricator', 'applications/differential/storage/changeset');
 phutil_require_module('phabricator', 'applications/differential/storage/comment');

src/applications/differential/exception/base/DifferentialException.php

@@ -0,0 +1,21 @@
+<?php
+
+/*
+ * Copyright 2012 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+abstract class DifferentialException extends Exception {
+
+}

src/applications/differential/exception/base/__init__.php

@@ -0,0 +1,10 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+
+phutil_require_source('DifferentialException.php');

src/applications/differential/exception/noeffect/DifferentialActionHasNoEffectException.php

@@ -0,0 +1,24 @@
+<?php
+
+/*
+ * Copyright 2012 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+final class DifferentialActionHasNoEffectException
+  extends DifferentialException {
+
+
+
+}

src/applications/differential/exception/noeffect/__init__.php

@@ -0,0 +1,12 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'applications/differential/exception/base');
+
+
+phutil_require_source('DifferentialActionHasNoEffectException.php');

src/applications/differential/view/addcomment/DifferentialAddCommentView.php

@@ -73,6 +73,7 @@ final class DifferentialAddCommentView extends AphrontView {
 
     $form = new AphrontFormView();
     $form
+      ->setWorkflow(true)
       ->setUser($this->user)
       ->setAction($this->actionURI)
       ->addHiddenInput('revision_id', $revision->getID())

src/view/dialog/AphrontDialogView.php

@@ -1,7 +1,7 @@
 <?php
 
 /*
- * Copyright 2011 Facebook, Inc.
+ * Copyright 2012 Facebook, Inc.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -64,7 +64,13 @@ class AphrontDialogView extends AphrontView {
   }
 
   public function addHiddenInput($key, $value) {
-    $this->hidden[] = array($key, $value);
+    if (is_array($value)) {
+      foreach ($value as $hidden_key => $hidden_value) {
+        $this->hidden[] = array($key.'['.$hidden_key.']', $hidden_value);
+      }
+    } else {
+      $this->hidden[] = array($key, $value);
+    }
     return $this;
   }