From 269dd81f91784d40abdd9af2d95061cdb3d2c394 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 18 Jan 2017 12:56:49 -0800
Subject: [PATCH] Allow users to re-accept or re-reject a revision if they have
 authority over package/project reviewers not yet in the target state

Summary:
To set this up:

  - alice accepts a revision.
  - Something adds a package or project she has authority over as a reviewer.
  - Because alice has already accepted, she can not re-accept, but she should be able to (in order to accept on behalf of the new project or package).

Test Plan:
  - Created a revision.
  - Accepted as user "dog".
  - Added "dog project".
  - Re-accepted.
  - Could not three-accept.
  - Removed "dog project.
  - Rejected.
  - Added "dog project".
  - Re-rejected.
  - Could not three-reject.

Reviewers: chad, eadler

Reviewed By: chad, eadler

Differential Revision: https://secure.phabricator.com/D17226
---
 .../DifferentialRevisionAcceptTransaction.php |  4 +--
 .../DifferentialRevisionRejectTransaction.php |  4 +--
 .../DifferentialRevisionReviewTransaction.php | 28 +++++++++++++++----
 3 files changed, 26 insertions(+), 10 deletions(-)

diff --git a/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php b/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php
index df4e3b4d33..3a64a44767 100644
--- a/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php
+++ b/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php
@@ -50,7 +50,7 @@ final class DifferentialRevisionAcceptTransaction
 
   public function generateOldValue($object) {
     $actor = $this->getActor();
-    return $this->isViewerAcceptingReviewer($object, $actor);
+    return $this->isViewerFullyAccepted($object, $actor);
   }
 
   public function applyExternalEffects($object, $value) {
@@ -79,7 +79,7 @@ final class DifferentialRevisionAcceptTransaction
       }
     }
 
-    if ($this->isViewerAcceptingReviewer($object, $viewer)) {
+    if ($this->isViewerFullyAccepted($object, $viewer)) {
       throw new Exception(
         pht(
           'You can not accept this revision because you have already '.
diff --git a/src/applications/differential/xaction/DifferentialRevisionRejectTransaction.php b/src/applications/differential/xaction/DifferentialRevisionRejectTransaction.php
index 96812de06b..b4c71209aa 100644
--- a/src/applications/differential/xaction/DifferentialRevisionRejectTransaction.php
+++ b/src/applications/differential/xaction/DifferentialRevisionRejectTransaction.php
@@ -46,7 +46,7 @@ final class DifferentialRevisionRejectTransaction
 
   public function generateOldValue($object) {
     $actor = $this->getActor();
-    return $this->isViewerRejectingReviewer($object, $actor);
+    return $this->isViewerFullyRejected($object, $actor);
   }
 
   public function applyExternalEffects($object, $value) {
@@ -72,7 +72,7 @@ final class DifferentialRevisionRejectTransaction
           'not own.'));
     }
 
-    if ($this->isViewerRejectingReviewer($object, $viewer)) {
+    if ($this->isViewerFullyRejected($object, $viewer)) {
       throw new Exception(
         pht(
           'You can not request changes to this revision because you have '.
diff --git a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php
index de75977220..7e0c60a7b1 100644
--- a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php
+++ b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php
@@ -13,10 +13,10 @@ abstract class DifferentialRevisionReviewTransaction
     return ($this->getViewerReviewerStatus($revision, $viewer) !== null);
   }
 
-  protected function isViewerAcceptingReviewer(
+  protected function isViewerFullyAccepted(
     DifferentialRevision $revision,
     PhabricatorUser $viewer) {
-    return $this->isViewerReviewerStatusAmong(
+    return $this->isViewerReviewerStatusFullyAmong(
       $revision,
       $viewer,
       array(
@@ -24,10 +24,10 @@ abstract class DifferentialRevisionReviewTransaction
       ));
   }
 
-  protected function isViewerRejectingReviewer(
+  protected function isViewerFullyRejected(
     DifferentialRevision $revision,
     PhabricatorUser $viewer) {
-    return $this->isViewerReviewerStatusAmong(
+    return $this->isViewerReviewerStatusFullyAmong(
       $revision,
       $viewer,
       array(
@@ -54,18 +54,34 @@ abstract class DifferentialRevisionReviewTransaction
     return null;
   }
 
-  protected function isViewerReviewerStatusAmong(
+  protected function isViewerReviewerStatusFullyAmong(
     DifferentialRevision $revision,
     PhabricatorUser $viewer,
     array $status_list) {
 
+    // If the user themselves is not a reviewer, the reviews they have
+    // authority over can not all be in any set of states since their own
+    // personal review has no state.
     $status = $this->getViewerReviewerStatus($revision, $viewer);
     if ($status === null) {
       return false;
     }
 
+    // Otherwise, check that all reviews they have authority over are in
+    // the desired set of states.
     $status_map = array_fuse($status_list);
-    return isset($status_map[$status]);
+    foreach ($revision->getReviewerStatus() as $reviewer) {
+      if (!$reviewer->hasAuthority($viewer)) {
+        continue;
+      }
+
+      $status = $reviewer->getStatus();
+      if (!isset($status_map[$status])) {
+        return false;
+      }
+    }
+
+    return true;
   }
 
   protected function applyReviewerEffect(