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

epriestley 12 years ago c6add6ae 8aa8ef49

+84 -3

2 changed files

expand all

src

applications

differential

editor

DifferentialCommentEditor.php

DifferentialRevisionEditor.php

+21 -3

src/applications/differential/editor/DifferentialCommentEditor.php

··· 116 116 $allow_reopen = PhabricatorEnv::getEnvConfig( 117 117 'differential.allow-reopen'); 118 118 $revision_status = $revision->getStatus(); 119 + $update_accepted_status = false; 119 120 120 121 $reviewer_phids = $revision->getReviewers(); 121 122 if ($reviewer_phids) { ··· 183 184 array(), 184 185 array($actor_phid)); 185 186 187 + // If you are a blocking reviewer, your presence as a reviewer may be 188 + // the only thing keeping a revision from transitioning to "accepted". 189 + // Recalculate state after removing the resigning reviewer. 190 + switch ($revision_status) { 191 + case ArcanistDifferentialRevisionStatus::NEEDS_REVISION: 192 + case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW: 193 + $update_accepted_status = true; 194 + break; 195 + } 196 + 186 197 break; 187 198 188 199 case DifferentialAction::ACTION_ABANDON: ··· 229 240 "Unexpected revision state '{$revision_status}'!"); 230 241 } 231 242 232 - $revision 233 - ->setStatus(ArcanistDifferentialRevisionStatus::ACCEPTED); 234 - 235 243 $was_reviewer_already = false; 236 244 foreach ($revision->getReviewerStatus() as $reviewer) { 237 245 if ($reviewer->hasAuthority($actor)) { ··· 253 261 $actor_phid, 254 262 DifferentialReviewerStatus::STATUS_ACCEPTED); 255 263 } 264 + 265 + $update_accepted_status = true; 256 266 break; 257 267 258 268 case DifferentialAction::ACTION_REQUEST: ··· 368 378 369 379 $revision 370 380 ->setStatus(ArcanistDifferentialRevisionStatus::NEEDS_REVIEW); 381 + 382 + $update_accepted_status = true; 371 383 break; 372 384 373 385 case DifferentialAction::ACTION_CLOSE: ··· 526 538 // by "updated". Notably, this allows "ping" comments to push it to the 527 539 // top of the action list. 528 540 $revision->save(); 541 + 542 + if ($update_accepted_status) { 543 + $revision = DifferentialRevisionEditor::updateAcceptedStatus( 544 + $actor, 545 + $revision); 546 + } 529 547 530 548 if ($action != DifferentialAction::ACTION_RESIGN) { 531 549 DifferentialRevisionEditor::addCC(

+63

src/applications/differential/editor/DifferentialRevisionEditor.php

··· 381 381 382 382 $changesets = null; 383 383 $comment = null; 384 + $old_status = $revision->getStatus(); 385 + 384 386 if ($diff) { 385 387 $changesets = $diff->loadChangesets(); 386 388 // TODO: This should probably be in DifferentialFeedbackEditor? ··· 428 430 } 429 431 430 432 $revision->save(); 433 + 434 + // If the actor just deleted all the blocking/rejected reviewers, we may 435 + // be able to put the revision into "accepted". 436 + switch ($revision->getStatus()) { 437 + case ArcanistDifferentialRevisionStatus::NEEDS_REVISION: 438 + case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW: 439 + $revision = self::updateAcceptedStatus( 440 + $this->getActor(), 441 + $revision); 442 + break; 443 + } 431 444 432 445 $this->didWriteRevision(); 433 446 ··· 1108 1121 if ($revision->getTestPlan() === null) { 1109 1122 $revision->setTestPlan(''); 1110 1123 } 1124 + } 1125 + 1126 + 1127 + /** 1128 + * Try to move a revision to "accepted". We look for: 1129 + * 1130 + * - at least one accepting reviewer who is a user; and 1131 + * - no rejects; and 1132 + * - no blocking reviewers. 1133 + */ 1134 + public static function updateAcceptedStatus( 1135 + PhabricatorUser $viewer, 1136 + DifferentialRevision $revision) { 1137 + 1138 + $revision = id(new DifferentialRevisionQuery()) 1139 + ->setViewer($viewer) 1140 + ->withIDs(array($revision->getID())) 1141 + ->needRelationships(true) 1142 + ->needReviewerStatus(true) 1143 + ->needReviewerAuthority(true) 1144 + ->executeOne(); 1145 + 1146 + $has_user_accept = false; 1147 + foreach ($revision->getReviewerStatus() as $reviewer) { 1148 + $status = $reviewer->getStatus(); 1149 + if ($status == DifferentialReviewerStatus::STATUS_BLOCKING) { 1150 + // We have a blocking reviewer, so just leave the revision in its 1151 + // existing state. 1152 + return $revision; 1153 + } 1154 + 1155 + if ($status == DifferentialReviewerStatus::STATUS_REJECTED) { 1156 + // We have a rejecting reviewer, so leave the revisoin as is. 1157 + return $revision; 1158 + } 1159 + 1160 + if ($reviewer->isUser()) { 1161 + if ($status == DifferentialReviewerStatus::STATUS_ACCEPTED) { 1162 + $has_user_accept = true; 1163 + } 1164 + } 1165 + } 1166 + 1167 + if ($has_user_accept) { 1168 + $revision 1169 + ->setStatus(ArcanistDifferentialRevisionStatus::ACCEPTED) 1170 + ->save(); 1171 + } 1172 + 1173 + return $revision; 1111 1174 } 1112 1175 1113 1176 }

Configure Feed

Configure Feed