Allow accepting accepted revisions, and rejecting rejected revisions

Summary:
Ref T1279. With the new per-reviewer status, you can always accept or reject a revision.

This is primarily cosmetic/UI changes. In particular, you've always been able to reject a rejected revision, the UI just didn't show you an option.

Test Plan: Accepted accepted revisions; rejected rejected revisions. See screenshots.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1279

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

src/applications/differential/controller/DifferentialRevisionViewController.php

@@ -588,9 +588,24 @@ final class DifferentialRevisionViewController extends DifferentialController {
     $viewer_phid = $viewer->getPHID();
     $viewer_is_owner = ($viewer_phid == $revision->getAuthorPHID());
     $viewer_is_reviewer = in_array($viewer_phid, $revision->getReviewers());
-    $viewer_did_accept = ($viewer_phid === $revision->loadReviewedBy());
     $status = $revision->getStatus();
 
+    $viewer_has_accepted = false;
+    $viewer_has_rejected = false;
+    $status_accepted = DifferentialReviewerStatus::STATUS_ACCEPTED;
+    $status_rejected = DifferentialReviewerStatus::STATUS_REJECTED;
+    foreach ($revision->getReviewerStatus() as $reviewer) {
+      if ($reviewer->getReviewerPHID() == $viewer_phid) {
+        if ($reviewer->getStatus() == $status_accepted) {
+          $viewer_has_accepted = true;
+        }
+        if ($reviewer->getStatus() == $status_rejected) {
+          $viewer_has_rejected = true;
+        }
+        break;
+      }
+    }
+
     $allow_self_accept = PhabricatorEnv::getEnvConfig(
       'differential.allow-self-accept');
     $always_allow_close = PhabricatorEnv::getEnvConfig(
@@ -631,12 +646,13 @@ final class DifferentialRevisionViewController extends DifferentialController {
           break;
         case ArcanistDifferentialRevisionStatus::NEEDS_REVISION:
           $actions[DifferentialAction::ACTION_ACCEPT] = true;
+          $actions[DifferentialAction::ACTION_REJECT] = !$viewer_has_rejected;
           $actions[DifferentialAction::ACTION_RESIGN] = $viewer_is_reviewer;
           break;
         case ArcanistDifferentialRevisionStatus::ACCEPTED:
+          $actions[DifferentialAction::ACTION_ACCEPT] = !$viewer_has_accepted;
           $actions[DifferentialAction::ACTION_REJECT] = true;
-          $actions[DifferentialAction::ACTION_RESIGN] =
+          $actions[DifferentialAction::ACTION_RESIGN] = $viewer_is_reviewer;
-            $viewer_is_reviewer && !$viewer_did_accept;
           break;
         case ArcanistDifferentialRevisionStatus::CLOSED:
         case ArcanistDifferentialRevisionStatus::ABANDONED:

src/applications/differential/editor/DifferentialCommentEditor.php

@@ -210,28 +210,23 @@ final class DifferentialCommentEditor extends PhabricatorEditor {
           throw new Exception('You can not accept your own revision.');
         }
 
-        if (($revision_status !=
+        switch ($revision_status) {
-             ArcanistDifferentialRevisionStatus::NEEDS_REVIEW) &&
+          case ArcanistDifferentialRevisionStatus::ABANDONED:
-            ($revision_status !=
+            throw new DifferentialActionHasNoEffectException(
-             ArcanistDifferentialRevisionStatus::NEEDS_REVISION)) {
+              "You can not accept this revision because it has been ".
-
+              "abandoned.");
-          switch ($revision_status) {
+          case ArcanistDifferentialRevisionStatus::CLOSED:
-            case ArcanistDifferentialRevisionStatus::ACCEPTED:
+            throw new DifferentialActionHasNoEffectException(
-              throw new DifferentialActionHasNoEffectException(
+              "You can not accept this revision because it has already ".
-                "You can not accept this revision because someone else ".
+              "been closed.");
-                "already accepted it.");
+          case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW:
-            case ArcanistDifferentialRevisionStatus::ABANDONED:
+          case ArcanistDifferentialRevisionStatus::NEEDS_REVISION:
-              throw new DifferentialActionHasNoEffectException(
+          case ArcanistDifferentialRevisionStatus::ACCEPTED:
-                "You can not accept this revision because it has been ".
+            // We expect "Accept" from these states.
-                "abandoned.");
+            break;
-            case ArcanistDifferentialRevisionStatus::CLOSED:
+          default:
-              throw new DifferentialActionHasNoEffectException(
+            throw new Exception(
-                "You can not accept this revision because it has already ".
+              "Unexpected revision state '{$revision_status}'!");
-                "been closed.");
-            default:
-              throw new Exception(
-                "Unexpected revision state '{$revision_status}'!");
-          }
         }
 
         $revision
@@ -306,9 +301,7 @@ final class DifferentialCommentEditor extends PhabricatorEditor {
           case ArcanistDifferentialRevisionStatus::ACCEPTED:
           case ArcanistDifferentialRevisionStatus::NEEDS_REVISION:
           case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW:
-            // NOTE: We allow you to reject an already-rejected revision
+            // We expect rejects from these states.
-            // because it doesn't create any ambiguity and avoids a rather
-            // needless dialog.
             break;
           case ArcanistDifferentialRevisionStatus::ABANDONED:
             throw new DifferentialActionHasNoEffectException(
@@ -343,6 +336,7 @@ final class DifferentialCommentEditor extends PhabricatorEditor {
           case ArcanistDifferentialRevisionStatus::ACCEPTED:
           case ArcanistDifferentialRevisionStatus::NEEDS_REVISION:
           case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW:
+            // We expect accepts from these states.
             break;
           case ArcanistDifferentialRevisionStatus::ABANDONED:
             throw new DifferentialActionHasNoEffectException(