diff --git a/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php b/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php --- a/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php @@ -50,7 +50,7 @@ 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 @@ } } - 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 --- a/src/applications/differential/xaction/DifferentialRevisionRejectTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionRejectTransaction.php @@ -46,7 +46,7 @@ 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 @@ '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 --- a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php @@ -13,10 +13,10 @@ 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 @@ )); } - protected function isViewerRejectingReviewer( + protected function isViewerFullyRejected( DifferentialRevision $revision, PhabricatorUser $viewer) { - return $this->isViewerReviewerStatusAmong( + return $this->isViewerReviewerStatusFullyAmong( $revision, $viewer, array( @@ -54,18 +54,34 @@ 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(