From b373dcef7448abbc553fc5c5f56498dddc3ddaff Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 31 Dec 2016 09:17:58 -0800 Subject: [PATCH] Restore some minor state behaviors to Differential on EditEngine Summary: Ref T11114. This restores: - Commandeering should exeucte Herald. - Commandeering should swap reviewers. - "Request Review" on an "Accepted" revision should downgrade reviewers so they have to accept again. Test Plan: - Commandeered, saw Herald run and reviewers swap. - Requested review of an accepted revision, saw it drop down to "Needs Review" with "Accepted Prior" on the reviewer. Reviewers: chad Reviewed By: chad Maniphest Tasks: T11114 Differential Revision: https://secure.phabricator.com/D17118 --- .../editor/DifferentialTransactionEditor.php | 98 +++++++++++-------- .../editengine/PhabricatorEditEngine.php | 4 + 2 files changed, 63 insertions(+), 39 deletions(-) diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index f1bed835d9..87a2b2b9c0 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -288,16 +288,26 @@ final class DifferentialTransactionEditor $downgrade_accepts = true; } break; + case DifferentialRevisionRequestReviewTransaction::TRANSACTIONTYPE: + $downgrade_rejects = true; + if ((!$is_sticky_accept) || + ($object->getStatus() != $status_plan)) { + // If the old state isn't "changes planned", downgrade the accepts. + // This exception allows an accepted revision to go through + // "Plan Changes" -> "Request Review" and return to "accepted" if + // the author didn't update the revision, essentially undoing the + // "Plan Changes". + $downgrade_accepts = true; + } + break; + + // TODO: Remove this, obsoleted by ModularTransactions above. case DifferentialTransaction::TYPE_ACTION: switch ($xaction->getNewValue()) { case DifferentialAction::ACTION_REQUEST: $downgrade_rejects = true; if ((!$is_sticky_accept) || ($object->getStatus() != $status_plan)) { - // If the old state isn't "changes planned", downgrade the - // accepts. This exception allows an accepted revision to - // go through Plan Changes -> Request Review to return to - // "accepted" if the author didn't update the revision. $downgrade_accepts = true; } break; @@ -353,6 +363,7 @@ final class DifferentialTransactionEditor } } + $is_commandeer = false; switch ($xaction->getTransactionType()) { case DifferentialTransaction::TYPE_UPDATE: if ($this->getIsCloseByCommit()) { @@ -424,6 +435,10 @@ final class DifferentialTransactionEditor } break; + case DifferentialRevisionCommandeerTransaction::TRANSACTIONTYPE: + $is_commandeer = true; + break; + case DifferentialTransaction::TYPE_ACTION: $action_type = $xaction->getNewValue(); @@ -463,41 +478,7 @@ final class DifferentialTransactionEditor break; case DifferentialAction::ACTION_CLAIM: - // If the user is commandeering, add the previous owner as a - // reviewer and remove the actor. - - $edits = array( - '-' => array( - $actor_phid => $actor_phid, - ), - ); - - $owner_phid = $object->getAuthorPHID(); - if ($owner_phid) { - $reviewer = new DifferentialReviewerProxy( - $owner_phid, - array( - 'status' => DifferentialReviewerStatus::STATUS_ADDED, - )); - - $edits['+'] = array( - $owner_phid => array( - 'data' => $reviewer->getEdgeData(), - ), - ); - } - - // NOTE: We're setting setIsCommandeerSideEffect() on this because - // normally you can't add a revision's author as a reviewer, but - // this action swaps them after validation executes. - - $results[] = id(new DifferentialTransaction()) - ->setTransactionType($type_edge) - ->setMetadataValue('edge:type', $edge_reviewer) - ->setIgnoreOnNoEffect(true) - ->setIsCommandeerSideEffect(true) - ->setNewValue($edits); - + $is_commandeer = true; break; case DifferentialAction::ACTION_RESIGN: // If the user is resigning, add a separate reviewer edit @@ -519,6 +500,10 @@ final class DifferentialTransactionEditor break; } + if ($is_commandeer) { + $results[] = $this->newCommandeerReviewerTransaction($object); + } + if (!$this->didExpandInlineState) { switch ($xaction->getTransactionType()) { case PhabricatorTransactions::TYPE_COMMENT: @@ -1499,6 +1484,10 @@ final class DifferentialTransactionEditor return true; } break; + case DifferentialRevisionCommandeerTransaction::TRANSACTIONTYPE: + // When users commandeer revisions, we may need to trigger + // signatures or author-based rules. + return true; case DifferentialTransaction::TYPE_ACTION: switch ($xaction->getNewValue()) { case DifferentialAction::ACTION_CLAIM: @@ -1922,4 +1911,35 @@ final class DifferentialTransactionEditor return $this; } + private function newCommandeerReviewerTransaction( + DifferentialRevision $revision) { + + $actor_phid = $this->getActingAsPHID(); + $owner_phid = $revision->getAuthorPHID(); + + // If the user is commandeering, add the previous owner as a + // reviewer and remove the actor. + + $edits = array( + '-' => array( + $actor_phid, + ), + '+' => array( + $owner_phid, + ), + ); + + // NOTE: We're setting setIsCommandeerSideEffect() on this because normally + // you can't add a revision's author as a reviewer, but this action swaps + // them after validation executes. + + $xaction_type = DifferentialRevisionReviewersTransaction::TRANSACTIONTYPE; + + return id(new DifferentialTransaction()) + ->setTransactionType($xaction_type) + ->setIgnoreOnNoEffect(true) + ->setIsCommandeerSideEffect(true) + ->setNewValue($edits); + } + } diff --git a/src/applications/transactions/editengine/PhabricatorEditEngine.php b/src/applications/transactions/editengine/PhabricatorEditEngine.php index ecfafad1cf..7626f67b0a 100644 --- a/src/applications/transactions/editengine/PhabricatorEditEngine.php +++ b/src/applications/transactions/editengine/PhabricatorEditEngine.php @@ -1805,6 +1805,10 @@ abstract class PhabricatorEditEngine try { $xactions = $editor->applyTransactions($object, $xactions); + } catch (PhabricatorApplicationTransactionValidationException $ex) { + return id(new PhabricatorApplicationTransactionValidationResponse()) + ->setCancelURI($view_uri) + ->setException($ex); } catch (PhabricatorApplicationTransactionNoEffectException $ex) { return id(new PhabricatorApplicationTransactionNoEffectResponse()) ->setCancelURI($view_uri)