diff --git a/src/applications/differential/editor/DifferentialCommentEditor.php b/src/applications/differential/editor/DifferentialCommentEditor.php index e9985d1d90..36d38bc58b 100644 --- a/src/applications/differential/editor/DifferentialCommentEditor.php +++ b/src/applications/differential/editor/DifferentialCommentEditor.php @@ -116,6 +116,7 @@ final class DifferentialCommentEditor extends PhabricatorEditor { $allow_reopen = PhabricatorEnv::getEnvConfig( 'differential.allow-reopen'); $revision_status = $revision->getStatus(); + $update_accepted_status = false; $reviewer_phids = $revision->getReviewers(); if ($reviewer_phids) { @@ -183,6 +184,16 @@ final class DifferentialCommentEditor extends PhabricatorEditor { array(), array($actor_phid)); + // If you are a blocking reviewer, your presence as a reviewer may be + // the only thing keeping a revision from transitioning to "accepted". + // Recalculate state after removing the resigning reviewer. + switch ($revision_status) { + case ArcanistDifferentialRevisionStatus::NEEDS_REVISION: + case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW: + $update_accepted_status = true; + break; + } + break; case DifferentialAction::ACTION_ABANDON: @@ -229,9 +240,6 @@ final class DifferentialCommentEditor extends PhabricatorEditor { "Unexpected revision state '{$revision_status}'!"); } - $revision - ->setStatus(ArcanistDifferentialRevisionStatus::ACCEPTED); - $was_reviewer_already = false; foreach ($revision->getReviewerStatus() as $reviewer) { if ($reviewer->hasAuthority($actor)) { @@ -253,6 +261,8 @@ final class DifferentialCommentEditor extends PhabricatorEditor { $actor_phid, DifferentialReviewerStatus::STATUS_ACCEPTED); } + + $update_accepted_status = true; break; case DifferentialAction::ACTION_REQUEST: @@ -368,6 +378,8 @@ final class DifferentialCommentEditor extends PhabricatorEditor { $revision ->setStatus(ArcanistDifferentialRevisionStatus::NEEDS_REVIEW); + + $update_accepted_status = true; break; case DifferentialAction::ACTION_CLOSE: @@ -527,6 +539,12 @@ final class DifferentialCommentEditor extends PhabricatorEditor { // top of the action list. $revision->save(); + if ($update_accepted_status) { + $revision = DifferentialRevisionEditor::updateAcceptedStatus( + $actor, + $revision); + } + if ($action != DifferentialAction::ACTION_RESIGN) { DifferentialRevisionEditor::addCC( $revision, diff --git a/src/applications/differential/editor/DifferentialRevisionEditor.php b/src/applications/differential/editor/DifferentialRevisionEditor.php index cc1cd6c6a6..bfa438e1b6 100644 --- a/src/applications/differential/editor/DifferentialRevisionEditor.php +++ b/src/applications/differential/editor/DifferentialRevisionEditor.php @@ -381,6 +381,8 @@ final class DifferentialRevisionEditor extends PhabricatorEditor { $changesets = null; $comment = null; + $old_status = $revision->getStatus(); + if ($diff) { $changesets = $diff->loadChangesets(); // TODO: This should probably be in DifferentialFeedbackEditor? @@ -429,6 +431,17 @@ final class DifferentialRevisionEditor extends PhabricatorEditor { $revision->save(); + // If the actor just deleted all the blocking/rejected reviewers, we may + // be able to put the revision into "accepted". + switch ($revision->getStatus()) { + case ArcanistDifferentialRevisionStatus::NEEDS_REVISION: + case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW: + $revision = self::updateAcceptedStatus( + $this->getActor(), + $revision); + break; + } + $this->didWriteRevision(); $event_data = array( @@ -1110,5 +1123,55 @@ final class DifferentialRevisionEditor extends PhabricatorEditor { } } + + /** + * Try to move a revision to "accepted". We look for: + * + * - at least one accepting reviewer who is a user; and + * - no rejects; and + * - no blocking reviewers. + */ + public static function updateAcceptedStatus( + PhabricatorUser $viewer, + DifferentialRevision $revision) { + + $revision = id(new DifferentialRevisionQuery()) + ->setViewer($viewer) + ->withIDs(array($revision->getID())) + ->needRelationships(true) + ->needReviewerStatus(true) + ->needReviewerAuthority(true) + ->executeOne(); + + $has_user_accept = false; + foreach ($revision->getReviewerStatus() as $reviewer) { + $status = $reviewer->getStatus(); + if ($status == DifferentialReviewerStatus::STATUS_BLOCKING) { + // We have a blocking reviewer, so just leave the revision in its + // existing state. + return $revision; + } + + if ($status == DifferentialReviewerStatus::STATUS_REJECTED) { + // We have a rejecting reviewer, so leave the revisoin as is. + return $revision; + } + + if ($reviewer->isUser()) { + if ($status == DifferentialReviewerStatus::STATUS_ACCEPTED) { + $has_user_accept = true; + } + } + } + + if ($has_user_accept) { + $revision + ->setStatus(ArcanistDifferentialRevisionStatus::ACCEPTED) + ->save(); + } + + return $revision; + } + }