From f7b5955d33866e9679b43d8b8bd592d4a8df3c2a Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 29 Dec 2016 13:36:36 -0800 Subject: [PATCH] Order actions sensibly within Differential revision comment action groups Summary: Ref T11114. See D17114 for some discussion. For review actions: accept, reject, resign. For revision actions, order is basically least-severe to most-severe action pairs: plan changes, request review, close, reopen, abandon, reclaim, commandeer. Test Plan: Viewed revisions as an author and a reviewer, saw sensible action order within action groups. Reviewers: chad Reviewed By: chad Maniphest Tasks: T11114 Differential Revision: https://secure.phabricator.com/D17115 --- .../editor/DifferentialRevisionEditEngine.php | 2 ++ .../differential/storage/DifferentialRevision.php | 5 +++++ .../DifferentialRevisionAbandonTransaction.php | 4 ++++ .../xaction/DifferentialRevisionAcceptTransaction.php | 4 ++++ .../xaction/DifferentialRevisionActionTransaction.php | 9 +++++++++ .../xaction/DifferentialRevisionCloseTransaction.php | 11 +++++++++++ .../DifferentialRevisionCommandeerTransaction.php | 4 ++++ .../DifferentialRevisionPlanChangesTransaction.php | 4 ++++ .../DifferentialRevisionReclaimTransaction.php | 4 ++++ .../xaction/DifferentialRevisionRejectTransaction.php | 4 ++++ .../xaction/DifferentialRevisionReopenTransaction.php | 4 ++++ .../DifferentialRevisionRequestReviewTransaction.php | 4 ++++ .../xaction/DifferentialRevisionResignTransaction.php | 4 ++++ 13 files changed, 63 insertions(+) diff --git a/src/applications/differential/editor/DifferentialRevisionEditEngine.php b/src/applications/differential/editor/DifferentialRevisionEditEngine.php index 95429e051d..dc2ebb2c7a 100644 --- a/src/applications/differential/editor/DifferentialRevisionEditEngine.php +++ b/src/applications/differential/editor/DifferentialRevisionEditEngine.php @@ -229,6 +229,8 @@ final class DifferentialRevisionEditEngine ->setValue(array()); $actions = DifferentialRevisionActionTransaction::loadAllActions(); + $actions = msortv($actions, 'getRevisionActionOrderVector'); + foreach ($actions as $key => $action) { $fields[] = $action->newEditField($object, $viewer); } diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index 9feb1bf228..ab8b8ddc3a 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -447,6 +447,11 @@ final class DifferentialRevision extends DifferentialDAO return ($this->getStatus() == $status_abandoned); } + public function isAccepted() { + $status_accepted = ArcanistDifferentialRevisionStatus::ACCEPTED; + return ($this->getStatus() == $status_accepted); + } + public function getStatusIcon() { $map = array( ArcanistDifferentialRevisionStatus::NEEDS_REVIEW diff --git a/src/applications/differential/xaction/DifferentialRevisionAbandonTransaction.php b/src/applications/differential/xaction/DifferentialRevisionAbandonTransaction.php index 473a47f514..e4fc7f1a5f 100644 --- a/src/applications/differential/xaction/DifferentialRevisionAbandonTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionAbandonTransaction.php @@ -22,6 +22,10 @@ final class DifferentialRevisionAbandonTransaction return 'indigo'; } + protected function getRevisionActionOrder() { + return 500; + } + public function generateOldValue($object) { return $object->isAbandoned(); } diff --git a/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php b/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php index 61178a58cd..c99c663c8d 100644 --- a/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php @@ -22,6 +22,10 @@ final class DifferentialRevisionAcceptTransaction return 'green'; } + protected function getRevisionActionOrder() { + return 500; + } + public function generateOldValue($object) { $actor = $this->getActor(); return $this->isViewerAcceptingReviewer($object, $actor); diff --git a/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php b/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php index f61ed8fffc..e495fff8e7 100644 --- a/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php @@ -19,6 +19,15 @@ abstract class DifferentialRevisionActionTransaction abstract protected function validateAction($object, PhabricatorUser $viewer); abstract protected function getRevisionActionLabel(); + protected function getRevisionActionOrder() { + return 1000; + } + + public function getRevisionActionOrderVector() { + return id(new PhutilSortVector()) + ->addInt($this->getRevisionActionOrder()); + } + protected function getRevisionActionGroupKey() { return DifferentialRevisionEditEngine::ACTIONGROUP_REVISION; } diff --git a/src/applications/differential/xaction/DifferentialRevisionCloseTransaction.php b/src/applications/differential/xaction/DifferentialRevisionCloseTransaction.php index ebbd0069a1..c2ba6c1bb8 100644 --- a/src/applications/differential/xaction/DifferentialRevisionCloseTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionCloseTransaction.php @@ -22,6 +22,10 @@ final class DifferentialRevisionCloseTransaction return 'indigo'; } + protected function getRevisionActionOrder() { + return 300; + } + public function generateOldValue($object) { return $object->isClosed(); } @@ -49,6 +53,13 @@ final class DifferentialRevisionCloseTransaction 'closed. Only open revisions can be closed.')); } + if (!$object->isAccepted()) { + throw new Exception( + pht( + 'You can not close this revision because it has not been accepted. '. + 'Revisions must be accepted before they can be closed.')); + } + $config_key = 'differential.always-allow-close'; if (!PhabricatorEnv::getEnvConfig($config_key)) { if (!$this->isViewerRevisionAuthor($object, $viewer)) { diff --git a/src/applications/differential/xaction/DifferentialRevisionCommandeerTransaction.php b/src/applications/differential/xaction/DifferentialRevisionCommandeerTransaction.php index 7690b79fff..fac7ec8b0b 100644 --- a/src/applications/differential/xaction/DifferentialRevisionCommandeerTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionCommandeerTransaction.php @@ -22,6 +22,10 @@ final class DifferentialRevisionCommandeerTransaction return 'sky'; } + protected function getRevisionActionOrder() { + return 700; + } + public function generateOldValue($object) { return $object->getAuthorPHID(); } diff --git a/src/applications/differential/xaction/DifferentialRevisionPlanChangesTransaction.php b/src/applications/differential/xaction/DifferentialRevisionPlanChangesTransaction.php index e08e3e7277..d33992d36e 100644 --- a/src/applications/differential/xaction/DifferentialRevisionPlanChangesTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionPlanChangesTransaction.php @@ -23,6 +23,10 @@ final class DifferentialRevisionPlanChangesTransaction return 'red'; } + protected function getRevisionActionOrder() { + return 200; + } + public function generateOldValue($object) { $status_planned = ArcanistDifferentialRevisionStatus::CHANGES_PLANNED; return ($object->getStatus() == $status_planned); diff --git a/src/applications/differential/xaction/DifferentialRevisionReclaimTransaction.php b/src/applications/differential/xaction/DifferentialRevisionReclaimTransaction.php index f583f089ef..ce8367f7c3 100644 --- a/src/applications/differential/xaction/DifferentialRevisionReclaimTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionReclaimTransaction.php @@ -22,6 +22,10 @@ final class DifferentialRevisionReclaimTransaction return 'sky'; } + protected function getRevisionActionOrder() { + return 600; + } + public function generateOldValue($object) { return !$object->isAbandoned(); } diff --git a/src/applications/differential/xaction/DifferentialRevisionRejectTransaction.php b/src/applications/differential/xaction/DifferentialRevisionRejectTransaction.php index a47f420ec6..17ecca0e20 100644 --- a/src/applications/differential/xaction/DifferentialRevisionRejectTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionRejectTransaction.php @@ -22,6 +22,10 @@ final class DifferentialRevisionRejectTransaction return 'red'; } + protected function getRevisionActionOrder() { + return 600; + } + public function generateOldValue($object) { $actor = $this->getActor(); return $this->isViewerRejectingReviewer($object, $actor); diff --git a/src/applications/differential/xaction/DifferentialRevisionReopenTransaction.php b/src/applications/differential/xaction/DifferentialRevisionReopenTransaction.php index f1e77bbb48..5024ab99fa 100644 --- a/src/applications/differential/xaction/DifferentialRevisionReopenTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionReopenTransaction.php @@ -22,6 +22,10 @@ final class DifferentialRevisionReopenTransaction return 'sky'; } + protected function getRevisionActionOrder() { + return 400; + } + public function generateOldValue($object) { return !$object->isClosed(); } diff --git a/src/applications/differential/xaction/DifferentialRevisionRequestReviewTransaction.php b/src/applications/differential/xaction/DifferentialRevisionRequestReviewTransaction.php index bd0320624f..cd1c14c66e 100644 --- a/src/applications/differential/xaction/DifferentialRevisionRequestReviewTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionRequestReviewTransaction.php @@ -18,6 +18,10 @@ final class DifferentialRevisionRequestReviewTransaction return 'sky'; } + protected function getRevisionActionOrder() { + return 200; + } + public function generateOldValue($object) { $status_review = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW; return ($object->getStatus() == $status_review); diff --git a/src/applications/differential/xaction/DifferentialRevisionResignTransaction.php b/src/applications/differential/xaction/DifferentialRevisionResignTransaction.php index 05717b8946..f994c2f45e 100644 --- a/src/applications/differential/xaction/DifferentialRevisionResignTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionResignTransaction.php @@ -22,6 +22,10 @@ final class DifferentialRevisionResignTransaction return 'orange'; } + protected function getRevisionActionOrder() { + return 700; + } + public function generateOldValue($object) { $actor = $this->getActor(); return !$this->isViewerAnyReviewer($object, $actor);