diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index e35257ff9b..97ad60fc12 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -339,7 +339,7 @@ final class DifferentialTransactionEditor } } - if ($downgrade_accepts) { + if ($downgrade_accepts || $downgrade_rejects) { $void_type = DifferentialRevisionVoidTransaction::TRANSACTIONTYPE; $results[] = id(new DifferentialTransaction()) ->setTransactionType($void_type) @@ -659,11 +659,8 @@ final class DifferentialTransactionEditor $reviewer_status = $reviewer->getReviewerStatus(); switch ($reviewer_status) { case DifferentialReviewerStatus::STATUS_REJECTED: - $action_phid = $reviewer->getLastActionDiffPHID(); $active_phid = $active_diff->getPHID(); - $is_current = ($action_phid == $active_phid); - - if ($is_current) { + if ($reviewer->isRejected($active_phid)) { $has_rejecting_reviewer = true; } break; diff --git a/src/applications/differential/storage/DifferentialReviewer.php b/src/applications/differential/storage/DifferentialReviewer.php index 6912e2af1e..b202baddf2 100644 --- a/src/applications/differential/storage/DifferentialReviewer.php +++ b/src/applications/differential/storage/DifferentialReviewer.php @@ -54,6 +54,25 @@ final class DifferentialReviewer return ($this->getReviewerStatus() == $status_resigned); } + public function isRejected($diff_phid) { + $status_rejected = DifferentialReviewerStatus::STATUS_REJECTED; + + if ($this->getReviewerStatus() != $status_rejected) { + return false; + } + + if ($this->getVoidedPHID()) { + return false; + } + + if ($this->isCurrentAction($diff_phid)) { + return true; + } + + return false; + } + + public function isAccepted($diff_phid) { $status_accepted = DifferentialReviewerStatus::STATUS_ACCEPTED; @@ -68,6 +87,21 @@ final class DifferentialReviewer return false; } + if ($this->isCurrentAction($diff_phid)) { + return true; + } + + $sticky_key = 'differential.sticky-accept'; + $is_sticky = PhabricatorEnv::getEnvConfig($sticky_key); + + if ($is_sticky) { + return true; + } + + return false; + } + + private function isCurrentAction($diff_phid) { if (!$diff_phid) { return true; } @@ -82,13 +116,6 @@ final class DifferentialReviewer return true; } - $sticky_key = 'differential.sticky-accept'; - $is_sticky = PhabricatorEnv::getEnvConfig($sticky_key); - - if ($is_sticky) { - return true; - } - return false; } diff --git a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php index c0971a5a2b..4e3e7b29e6 100644 --- a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php @@ -100,7 +100,10 @@ abstract class DifferentialRevisionReviewTransaction $active_phid = $this->getActiveDiffPHID($revision); $status_accepted = DifferentialReviewerStatus::STATUS_ACCEPTED; + $status_rejected = DifferentialReviewerStatus::STATUS_REJECTED; + $is_accepted = ($require_status == $status_accepted); + $is_rejected = ($require_status == $status_rejected); // Otherwise, check that all reviews they have authority over are in // the desired set of states. @@ -121,6 +124,12 @@ abstract class DifferentialRevisionReviewTransaction } } + if ($is_rejected) { + if (!$reviewer->isRejected($active_phid)) { + return false; + } + } + // This is a broader check to see if we can update the diff where the // last action occurred. if ($reviewer->getLastActionDiffPHID() != $active_phid) { diff --git a/src/applications/differential/xaction/DifferentialRevisionVoidTransaction.php b/src/applications/differential/xaction/DifferentialRevisionVoidTransaction.php index e40cb8af62..ae684d94fe 100644 --- a/src/applications/differential/xaction/DifferentialRevisionVoidTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionVoidTransaction.php @@ -25,10 +25,10 @@ final class DifferentialRevisionVoidTransaction 'SELECT reviewerPHID FROM %T WHERE revisionPHID = %s AND voidedPHID IS NULL - AND reviewerStatus = %s', + AND reviewerStatus IN (%Ls)', $table_name, $object->getPHID(), - DifferentialReviewerStatus::STATUS_ACCEPTED); + $this->getVoidableStatuses()); return ipull($rows, 'reviewerPHID'); } @@ -47,11 +47,11 @@ final class DifferentialRevisionVoidTransaction 'UPDATE %T SET voidedPHID = %s WHERE revisionPHID = %s AND voidedPHID IS NULL - AND reviewerStatus = %s', + AND reviewerStatus IN (%Ls)', $table_name, $this->getActingAsPHID(), $object->getPHID(), - DifferentialReviewerStatus::STATUS_ACCEPTED); + $this->getVoidableStatuses()); } public function shouldHide() { @@ -60,4 +60,11 @@ final class DifferentialRevisionVoidTransaction return true; } + private function getVoidableStatuses() { + return array( + DifferentialReviewerStatus::STATUS_ACCEPTED, + DifferentialReviewerStatus::STATUS_REJECTED, + ); + } + }