From 2b9838b482af694f9577be80b7aac80fd8fb0117 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 11 Aug 2017 14:47:39 -0700 Subject: [PATCH] Modularize remaining TYPE_ACTION transactions in Differential, reducing calls to ArcanistDifferentialRevisionStatus Summary: Ref T2543. This cleans up a couple of remaining rough edges: - We could do an older TYPE_ACTION "close" via the daemons. - We could do an older TYPE_ACTION "close" via `arc close-revision`, explicitly or implicitly in `arc land`, via API (`differential.close`). - We could do an older TYPE_ACTION "rethink" ("Plan Changes") via the API, via `arc diff --plan-changes` (`differential.createcomment`). Move these to modern modular transactions, then get rid of all the validation and application logic for them. This nukes a bunch of `ArcanistDifferentialRevision::...` junk. Test Plan: - Used `bin/repository reparse --message rXYZ...` to reparse a commit, closing a corresponding revision. - Used `differential.close` to close a revision. - Used `differential.createcomment` to plan changes to a revision. - Reviewed transaction log for full "closed by commit" message (linking to commit and mentioning author). - Grepped for `::TYPE_ACTION` to look for remaining callsites, didn't find any. - Grepped for `differential.close` and `differential.createcomment` in `arcanist/` to look for anything suspicious, seemed clean. Reviewers: chad Reviewed By: chad Maniphest Tasks: T2543 Differential Revision: https://secure.phabricator.com/D18412 --- .../DifferentialCloseConduitAPIMethod.php | 5 +- ...ferentialCreateCommentConduitAPIMethod.php | 8 +- .../editor/DifferentialTransactionEditor.php | 239 ------------------ .../DifferentialRevisionCloseTransaction.php | 53 +++- ...torRepositoryCommitMessageParserWorker.php | 8 +- 5 files changed, 63 insertions(+), 250 deletions(-) diff --git a/src/applications/differential/conduit/DifferentialCloseConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialCloseConduitAPIMethod.php index 30f8a5e5b7..12ecda1262 100644 --- a/src/applications/differential/conduit/DifferentialCloseConduitAPIMethod.php +++ b/src/applications/differential/conduit/DifferentialCloseConduitAPIMethod.php @@ -52,8 +52,9 @@ final class DifferentialCloseConduitAPIMethod $xactions = array(); $xactions[] = id(new DifferentialTransaction()) - ->setTransactionType(DifferentialTransaction::TYPE_ACTION) - ->setNewValue(DifferentialAction::ACTION_CLOSE); + ->setTransactionType( + DifferentialRevisionCloseTransaction::TRANSACTIONTYPE) + ->setNewValue(true); $content_source = $request->newContentSource(); diff --git a/src/applications/differential/conduit/DifferentialCreateCommentConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialCreateCommentConduitAPIMethod.php index 943c3e7702..21b055a1e4 100644 --- a/src/applications/differential/conduit/DifferentialCreateCommentConduitAPIMethod.php +++ b/src/applications/differential/conduit/DifferentialCreateCommentConduitAPIMethod.php @@ -63,6 +63,7 @@ final class DifferentialCreateCommentConduitAPIMethod 'resign' => DifferentialRevisionResignTransaction::TRANSACTIONTYPE, 'request_review' => DifferentialRevisionRequestReviewTransaction::TRANSACTIONTYPE, + 'rethink' => DifferentialRevisionPlanChangesTransaction::TRANSACTIONTYPE, ); $action = $request->getValue('action'); @@ -76,9 +77,10 @@ final class DifferentialCreateCommentConduitAPIMethod case 'none': break; default: - $xactions[] = id(new DifferentialTransaction()) - ->setTransactionType(DifferentialTransaction::TYPE_ACTION) - ->setNewValue($action); + throw new Exception( + pht( + 'Unsupported action "%s".', + $action)); break; } } diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 9390d1d2af..8089438ab9 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -69,7 +69,6 @@ final class DifferentialTransactionEditor $types[] = PhabricatorTransactions::TYPE_EDIT_POLICY; $types[] = PhabricatorTransactions::TYPE_INLINESTATE; - $types[] = DifferentialTransaction::TYPE_ACTION; $types[] = DifferentialTransaction::TYPE_INLINE; $types[] = DifferentialTransaction::TYPE_UPDATE; @@ -81,8 +80,6 @@ final class DifferentialTransactionEditor PhabricatorApplicationTransaction $xaction) { switch ($xaction->getTransactionType()) { - case DifferentialTransaction::TYPE_ACTION: - return null; case DifferentialTransaction::TYPE_INLINE: return null; case DifferentialTransaction::TYPE_UPDATE: @@ -101,7 +98,6 @@ final class DifferentialTransactionEditor PhabricatorApplicationTransaction $xaction) { switch ($xaction->getTransactionType()) { - case DifferentialTransaction::TYPE_ACTION: case DifferentialTransaction::TYPE_UPDATE: return $xaction->getNewValue(); case DifferentialTransaction::TYPE_INLINE: @@ -120,28 +116,6 @@ final class DifferentialTransactionEditor switch ($xaction->getTransactionType()) { case DifferentialTransaction::TYPE_INLINE: return $xaction->hasComment(); - case DifferentialTransaction::TYPE_ACTION: - $status_closed = ArcanistDifferentialRevisionStatus::CLOSED; - $status_abandoned = ArcanistDifferentialRevisionStatus::ABANDONED; - $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW; - $status_revision = ArcanistDifferentialRevisionStatus::NEEDS_REVISION; - $status_plan = ArcanistDifferentialRevisionStatus::CHANGES_PLANNED; - - $action_type = $xaction->getNewValue(); - switch ($action_type) { - case DifferentialAction::ACTION_CLOSE: - return ($object->getStatus() != $status_closed); - case DifferentialAction::ACTION_ABANDON: - return ($object->getStatus() != $status_abandoned); - case DifferentialAction::ACTION_RECLAIM: - return ($object->getStatus() == $status_abandoned); - case DifferentialAction::ACTION_REOPEN: - return ($object->getStatus() == $status_closed); - case DifferentialAction::ACTION_RETHINK: - return ($object->getStatus() != $status_plan); - case DifferentialAction::ACTION_CLAIM: - return ($actor_phid != $object->getAuthorPHID()); - } } return parent::transactionHasEffect($object, $xaction); @@ -183,38 +157,6 @@ final class DifferentialTransactionEditor // TODO: Update the `diffPHID` once we add that. return; - case DifferentialTransaction::TYPE_ACTION: - switch ($xaction->getNewValue()) { - case DifferentialAction::ACTION_ABANDON: - $object->setStatus(ArcanistDifferentialRevisionStatus::ABANDONED); - return; - case DifferentialAction::ACTION_RETHINK: - $object->setStatus($status_plan); - return; - case DifferentialAction::ACTION_RECLAIM: - $object->setStatus($status_review); - return; - case DifferentialAction::ACTION_REOPEN: - $object->setStatus($status_review); - return; - case DifferentialAction::ACTION_CLOSE: - $old_status = $object->getStatus(); - $object->setStatus(ArcanistDifferentialRevisionStatus::CLOSED); - $was_accepted = ($old_status == $status_accepted); - $object->setProperty( - DifferentialRevision::PROPERTY_CLOSED_FROM_ACCEPTED, - $was_accepted); - return; - case DifferentialAction::ACTION_CLAIM: - $object->setAuthorPHID($this->getActingAsPHID()); - return; - default: - throw new Exception( - pht( - 'Differential action "%s" is not a valid action!', - $xaction->getNewValue())); - } - break; } return parent::applyCustomInternalTransaction($object, $xaction); @@ -356,16 +298,6 @@ final class DifferentialTransactionEditor case DifferentialRevisionCommandeerTransaction::TRANSACTIONTYPE: $is_commandeer = true; break; - - case DifferentialTransaction::TYPE_ACTION: - $action_type = $xaction->getNewValue(); - - switch ($action_type) { - case DifferentialAction::ACTION_CLAIM: - $is_commandeer = true; - break; - } - break; } if ($is_commandeer) { @@ -375,7 +307,6 @@ final class DifferentialTransactionEditor if (!$this->didExpandInlineState) { switch ($xaction->getTransactionType()) { case PhabricatorTransactions::TYPE_COMMENT: - case DifferentialTransaction::TYPE_ACTION: case DifferentialTransaction::TYPE_UPDATE: case DifferentialTransaction::TYPE_INLINE: $this->didExpandInlineState = true; @@ -421,8 +352,6 @@ final class DifferentialTransactionEditor PhabricatorApplicationTransaction $xaction) { switch ($xaction->getTransactionType()) { - case DifferentialTransaction::TYPE_ACTION: - return; case DifferentialTransaction::TYPE_INLINE: $reply = $xaction->getComment()->getReplyToComment(); if ($reply && !$reply->getHasReplies()) { @@ -660,172 +589,12 @@ final class DifferentialTransactionEditor $xaction); } break; - case DifferentialTransaction::TYPE_ACTION: - $error = $this->validateDifferentialAction( - $object, - $type, - $xaction, - $xaction->getNewValue()); - if ($error) { - $errors[] = new PhabricatorApplicationTransactionValidationError( - $type, - pht('Invalid'), - $error, - $xaction); - } - break; } } return $errors; } - private function validateDifferentialAction( - DifferentialRevision $revision, - $type, - DifferentialTransaction $xaction, - $action) { - - $author_phid = $revision->getAuthorPHID(); - $actor_phid = $this->getActingAsPHID(); - $actor_is_author = ($author_phid == $actor_phid); - - $config_abandon_key = 'differential.always-allow-abandon'; - $always_allow_abandon = PhabricatorEnv::getEnvConfig($config_abandon_key); - - $config_close_key = 'differential.always-allow-close'; - $always_allow_close = PhabricatorEnv::getEnvConfig($config_close_key); - - $config_reopen_key = 'differential.allow-reopen'; - $allow_reopen = PhabricatorEnv::getEnvConfig($config_reopen_key); - - $config_self_accept_key = 'differential.allow-self-accept'; - $allow_self_accept = PhabricatorEnv::getEnvConfig($config_self_accept_key); - - $revision_status = $revision->getStatus(); - - $status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED; - $status_abandoned = ArcanistDifferentialRevisionStatus::ABANDONED; - $status_closed = ArcanistDifferentialRevisionStatus::CLOSED; - - switch ($action) { - case DifferentialAction::ACTION_CLAIM: - // You can claim a revision if you're not the owner. If you are, this - // is a no-op rather than invalid. - - if ($revision_status == $status_closed) { - return pht( - 'You can not commandeer this revision because it has already been '. - 'closed.'); - } - break; - - case DifferentialAction::ACTION_ABANDON: - if (!$actor_is_author && !$always_allow_abandon) { - return pht( - 'You can not abandon this revision because you do not own it. '. - 'You can only abandon revisions you own.'); - } - - if ($revision_status == $status_closed) { - return pht( - 'You can not abandon this revision because it has already been '. - 'closed.'); - } - - // NOTE: Abandons of already-abandoned revisions are treated as no-op - // instead of invalid. Other abandons are OK. - - break; - - case DifferentialAction::ACTION_RECLAIM: - if (!$actor_is_author) { - return pht( - 'You can not reclaim this revision because you do not own '. - 'it. You can only reclaim revisions you own.'); - } - - if ($revision_status == $status_closed) { - return pht( - 'You can not reclaim this revision because it has already been '. - 'closed.'); - } - - // NOTE: Reclaims of other non-abandoned revisions are treated as no-op - // instead of invalid. - - break; - - case DifferentialAction::ACTION_REOPEN: - if (!$allow_reopen) { - return pht( - 'The reopen action is not enabled on this Phabricator install. '. - 'Adjust your configuration to enable it.'); - } - - // NOTE: If the revision is not closed, this is caught as a no-op - // instead of an invalid transaction. - - break; - - case DifferentialAction::ACTION_RETHINK: - if (!$actor_is_author) { - return pht( - 'You can not plan changes to this revision because you do not '. - 'own it. To plan changes to a revision, you must be its owner.'); - } - - switch ($revision_status) { - case ArcanistDifferentialRevisionStatus::ACCEPTED: - case ArcanistDifferentialRevisionStatus::NEEDS_REVISION: - case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW: - // These are OK. - break; - case ArcanistDifferentialRevisionStatus::CHANGES_PLANNED: - // Let this through, it's a no-op. - break; - case ArcanistDifferentialRevisionStatus::ABANDONED: - return pht( - 'You can not plan changes to this revision because it has '. - 'been abandoned.'); - case ArcanistDifferentialRevisionStatus::CLOSED: - return pht( - 'You can not plan changes to this revision because it has '. - 'already been closed.'); - default: - throw new Exception( - pht( - 'Encountered unexpected revision status ("%s") when '. - 'validating "%s" action.', - $revision_status, - $action)); - } - break; - - case DifferentialAction::ACTION_CLOSE: - // We force revisions closed when we discover a corresponding commit. - // In this case, revisions are allowed to transition to closed from - // any state. This is an automated action taken by the daemons. - - if (!$this->getIsCloseByCommit()) { - if (!$actor_is_author && !$always_allow_close) { - return pht( - 'You can not close this revision because you do not own it. To '. - 'close a revision, you must be its owner.'); - } - - if ($revision_status != $status_accepted) { - return pht( - 'You can not close this revision because it has not been '. - 'accepted. You can only close accepted revisions.'); - } - } - break; - } - - return null; - } - protected function sortTransactions(array $xactions) { $xactions = parent::sortTransactions($xactions); @@ -1233,14 +1002,6 @@ final class DifferentialTransactionEditor // 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: - // When users commandeer revisions, we may need to trigger - // signatures or author-based rules. - return true; - } - break; } } diff --git a/src/applications/differential/xaction/DifferentialRevisionCloseTransaction.php b/src/applications/differential/xaction/DifferentialRevisionCloseTransaction.php index 30ca39a362..203b84a191 100644 --- a/src/applications/differential/xaction/DifferentialRevisionCloseTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionCloseTransaction.php @@ -46,6 +46,12 @@ final class DifferentialRevisionCloseTransaction } protected function validateAction($object, PhabricatorUser $viewer) { + if ($this->getEditor()->getIsCloseByCommit()) { + // If we're closing a revision because we discovered a commit, we don't + // care what state it was in. + return; + } + if ($object->isClosed()) { throw new Exception( pht( @@ -74,9 +80,50 @@ final class DifferentialRevisionCloseTransaction } public function getTitle() { - return pht( - '%s closed this revision.', - $this->renderAuthor()); + if (!$this->getMetadataValue('isCommitClose')) { + return pht( + '%s closed this revision.', + $this->renderAuthor()); + } + + $commit_phid = $this->getMetadataValue('commitPHID'); + $committer_phid = $this->getMetadataValue('committerPHID'); + $author_phid = $this->getMetadataValue('authorPHID'); + + if ($committer_phid) { + $committer_name = $this->renderHandle($committer_phid); + } else { + $committer_name = $this->getMetadataValue('committerName'); + } + + if ($author_phid) { + $author_name = $this->renderHandle($author_phid); + } else { + $author_name = $this->getMetadatavalue('authorName'); + } + + $same_phid = + strlen($committer_phid) && + strlen($author_phid) && + ($committer_phid == $author_phid); + + $same_name = + !strlen($committer_phid) && + !strlen($author_phid) && + ($committer_name == $author_name); + + if ($same_name || $same_phid) { + return pht( + 'Closed by commit %s (authored by %s).', + $this->renderHandle($commit_phid), + $author_name); + } else { + return pht( + 'Closed by commit %s (authored by %s, committed by %s).', + $this->renderHandle($commit_phid), + $author_name, + $committer_name); + } } public function getTitleForFeed() { diff --git a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php index ab11a83b38..0d0ff28d95 100644 --- a/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php +++ b/src/applications/repository/worker/commitmessageparser/PhabricatorRepositoryCommitMessageParserWorker.php @@ -205,9 +205,11 @@ abstract class PhabricatorRepositoryCommitMessageParserWorker $should_close = !$revision->isPublished() && $should_autoclose; if ($should_close) { - $commit_close_xaction = id(new DifferentialTransaction()) - ->setTransactionType(DifferentialTransaction::TYPE_ACTION) - ->setNewValue(DifferentialAction::ACTION_CLOSE) + $type_close = DifferentialRevisionCloseTransaction::TRANSACTIONTYPE; + + $commit_close_xaction = id(new DifferentialTransaction()) + ->setTransactionType($type_close) + ->setNewValue(true) ->setMetadataValue('isCommitClose', true); $commit_close_xaction->setMetadataValue(