From c6add6ae7329efc5f16d278f2c9df66662285205 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 6 Oct 2013 17:09:56 -0700 Subject: [PATCH] Make "reject" and "blocking reviewer" block acceptance in Differential Summary: Ref T1279. This is a logical change. - "Reject" (nee "Request Changes") is now sticky. The review won't transition to "Accepted" until the reviewer clears their objection. In practice, I think it always worked like this anyway (without technical enforcement, users just followed this rule naturally, since disobeying this rule is kind of a dick move) so I don't expect this to change much. I think this rule is easier to understand than the old rule now, given the multi-reviewer status and blocking reviewers. - "Blocking Reviewer" and "Reject" now prevent a revision from transitioning to "Accepted". When reviewers accept, resign, or are removed, we do a check to see if the reivsion has: at least one user reviewer who has accepted; zero rejects; and zero blocks. If all conditions are satisfied, we transition it to "accepted". Practically, the primary net effect of this is just to make blocking reviews actually block. This is pretty messy, but there's not much we can do about it until after T2222, since we have two completely separate editor pathways which are both responsible for adjusting status. Eventually, these can merge into a single sane editor which implements reasonable rules in reaonable ways. But that day is not today. Test Plan: With three users and a project, made a bunch of accepts, rejects, resigns and reviewer removals. I think I probably covered most of the pathways? There are a lot of interactions here. Reviewers: btrahan Reviewed By: btrahan CC: aran, wisutsak.jaisue.7 Maniphest Tasks: T1279 Differential Revision: https://secure.phabricator.com/D7245 --- .../editor/DifferentialCommentEditor.php | 24 ++++++- .../editor/DifferentialRevisionEditor.php | 63 +++++++++++++++++++ 2 files changed, 84 insertions(+), 3 deletions(-) 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; + } + }