From 48fcfeadafe2324258f4b903ce99a8448b6ea057 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 29 Dec 2016 13:14:09 -0800 Subject: [PATCH] Allow comment actions to be grouped; group Differential "Review" and "Revision" actions Summary: Ref T11114. Differential has more actions than it once did, and may have further actions in the future. Make this dropdown a little easier to parse by grouping similar types of actions, like "Accept" and "Reject". (The action order still needs to be tweaked a bit.) Test Plan: {F2274526} Reviewers: chad Reviewed By: chad Subscribers: eadler Maniphest Tasks: T11114 Differential Revision: https://secure.phabricator.com/D17114 --- src/__phutil_library_map__.php | 10 +- .../editor/DifferentialRevisionEditEngine.php | 14 ++ .../DifferentialRevisionAcceptTransaction.php | 2 +- .../DifferentialRevisionActionTransaction.php | 117 +---------------- .../DifferentialRevisionRejectTransaction.php | 2 +- .../DifferentialRevisionResignTransaction.php | 2 +- .../DifferentialRevisionReviewTransaction.php | 120 ++++++++++++++++++ .../PhabricatorEditEngineCommentAction.php | 10 ++ ...habricatorEditEngineCommentActionGroup.php | 27 ++++ .../editengine/PhabricatorEditEngine.php | 7 + .../editfield/PhabricatorEditField.php | 13 +- ...catorApplicationTransactionCommentView.php | 60 ++++++++- 12 files changed, 262 insertions(+), 122 deletions(-) create mode 100644 src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php create mode 100644 src/applications/transactions/commentaction/PhabricatorEditEngineCommentActionGroup.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index d7da81de3f..f84b565b38 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -557,6 +557,7 @@ phutil_register_library_map(array( 'DifferentialRevisionRequiredActionResultBucket' => 'applications/differential/query/DifferentialRevisionRequiredActionResultBucket.php', 'DifferentialRevisionResignTransaction' => 'applications/differential/xaction/DifferentialRevisionResignTransaction.php', 'DifferentialRevisionResultBucket' => 'applications/differential/query/DifferentialRevisionResultBucket.php', + 'DifferentialRevisionReviewTransaction' => 'applications/differential/xaction/DifferentialRevisionReviewTransaction.php', 'DifferentialRevisionReviewersHeraldField' => 'applications/differential/herald/DifferentialRevisionReviewersHeraldField.php', 'DifferentialRevisionReviewersTransaction' => 'applications/differential/xaction/DifferentialRevisionReviewersTransaction.php', 'DifferentialRevisionSearchConduitAPIMethod' => 'applications/differential/conduit/DifferentialRevisionSearchConduitAPIMethod.php', @@ -2554,6 +2555,7 @@ phutil_register_library_map(array( 'PhabricatorEditEngineAPIMethod' => 'applications/transactions/editengine/PhabricatorEditEngineAPIMethod.php', 'PhabricatorEditEngineColumnsCommentAction' => 'applications/transactions/commentaction/PhabricatorEditEngineColumnsCommentAction.php', 'PhabricatorEditEngineCommentAction' => 'applications/transactions/commentaction/PhabricatorEditEngineCommentAction.php', + 'PhabricatorEditEngineCommentActionGroup' => 'applications/transactions/commentaction/PhabricatorEditEngineCommentActionGroup.php', 'PhabricatorEditEngineConfiguration' => 'applications/transactions/storage/PhabricatorEditEngineConfiguration.php', 'PhabricatorEditEngineConfigurationDefaultCreateController' => 'applications/transactions/controller/PhabricatorEditEngineConfigurationDefaultCreateController.php', 'PhabricatorEditEngineConfigurationDefaultsController' => 'applications/transactions/controller/PhabricatorEditEngineConfigurationDefaultsController.php', @@ -5183,7 +5185,7 @@ phutil_register_library_map(array( 'PhabricatorConduitResultInterface', ), 'DifferentialRevisionAbandonTransaction' => 'DifferentialRevisionActionTransaction', - 'DifferentialRevisionAcceptTransaction' => 'DifferentialRevisionActionTransaction', + 'DifferentialRevisionAcceptTransaction' => 'DifferentialRevisionReviewTransaction', 'DifferentialRevisionActionTransaction' => 'DifferentialRevisionTransactionType', 'DifferentialRevisionAffectedFilesHeraldField' => 'DifferentialRevisionHeraldField', 'DifferentialRevisionAuthorHeraldField' => 'DifferentialRevisionHeraldField', @@ -5223,7 +5225,7 @@ phutil_register_library_map(array( 'DifferentialRevisionPlanChangesTransaction' => 'DifferentialRevisionActionTransaction', 'DifferentialRevisionQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'DifferentialRevisionReclaimTransaction' => 'DifferentialRevisionActionTransaction', - 'DifferentialRevisionRejectTransaction' => 'DifferentialRevisionActionTransaction', + 'DifferentialRevisionRejectTransaction' => 'DifferentialRevisionReviewTransaction', 'DifferentialRevisionRelationship' => 'PhabricatorObjectRelationship', 'DifferentialRevisionRelationshipSource' => 'PhabricatorObjectRelationshipSource', 'DifferentialRevisionReopenTransaction' => 'DifferentialRevisionActionTransaction', @@ -5232,8 +5234,9 @@ phutil_register_library_map(array( 'DifferentialRevisionRepositoryTransaction' => 'DifferentialRevisionTransactionType', 'DifferentialRevisionRequestReviewTransaction' => 'DifferentialRevisionActionTransaction', 'DifferentialRevisionRequiredActionResultBucket' => 'DifferentialRevisionResultBucket', - 'DifferentialRevisionResignTransaction' => 'DifferentialRevisionActionTransaction', + 'DifferentialRevisionResignTransaction' => 'DifferentialRevisionReviewTransaction', 'DifferentialRevisionResultBucket' => 'PhabricatorSearchResultBucket', + 'DifferentialRevisionReviewTransaction' => 'DifferentialRevisionActionTransaction', 'DifferentialRevisionReviewersHeraldField' => 'DifferentialRevisionHeraldField', 'DifferentialRevisionReviewersTransaction' => 'DifferentialRevisionTransactionType', 'DifferentialRevisionSearchConduitAPIMethod' => 'PhabricatorSearchEngineAPIMethod', @@ -7536,6 +7539,7 @@ phutil_register_library_map(array( 'PhabricatorEditEngineAPIMethod' => 'ConduitAPIMethod', 'PhabricatorEditEngineColumnsCommentAction' => 'PhabricatorEditEngineCommentAction', 'PhabricatorEditEngineCommentAction' => 'Phobject', + 'PhabricatorEditEngineCommentActionGroup' => 'Phobject', 'PhabricatorEditEngineConfiguration' => array( 'PhabricatorSearchDAO', 'PhabricatorApplicationTransactionInterface', diff --git a/src/applications/differential/editor/DifferentialRevisionEditEngine.php b/src/applications/differential/editor/DifferentialRevisionEditEngine.php index 4be1e0bb4a..95429e051d 100644 --- a/src/applications/differential/editor/DifferentialRevisionEditEngine.php +++ b/src/applications/differential/editor/DifferentialRevisionEditEngine.php @@ -9,6 +9,9 @@ final class DifferentialRevisionEditEngine const KEY_UPDATE = 'update'; + const ACTIONGROUP_REVIEW = 'review'; + const ACTIONGROUP_REVISION = 'revision'; + public function getEngineName() { return pht('Revisions'); } @@ -87,6 +90,17 @@ final class DifferentialRevisionEditEngine return $this->diff; } + protected function newCommentActionGroups() { + return array( + id(new PhabricatorEditEngineCommentActionGroup()) + ->setKey(self::ACTIONGROUP_REVIEW) + ->setLabel(pht('Review Actions')), + id(new PhabricatorEditEngineCommentActionGroup()) + ->setKey(self::ACTIONGROUP_REVISION) + ->setLabel(pht('Revision Actions')), + ); + } + protected function buildCustomEditFields($object) { $viewer = $this->getViewer(); diff --git a/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php b/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php index 65159ab2c0..61178a58cd 100644 --- a/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php @@ -1,7 +1,7 @@ getPHID() === $revision->getAuthorPHID()); } - protected function isViewerAnyReviewer( - DifferentialRevision $revision, - PhabricatorUser $viewer) { - return ($this->getViewerReviewerStatus($revision, $viewer) !== null); - } - - protected function isViewerAcceptingReviewer( - DifferentialRevision $revision, - PhabricatorUser $viewer) { - return $this->isViewerReviewerStatusAmong( - $revision, - $viewer, - array( - DifferentialReviewerStatus::STATUS_ACCEPTED, - )); - } - - protected function isViewerRejectingReviewer( - DifferentialRevision $revision, - PhabricatorUser $viewer) { - return $this->isViewerReviewerStatusAmong( - $revision, - $viewer, - array( - DifferentialReviewerStatus::STATUS_REJECTED, - )); - } - - protected function getViewerReviewerStatus( - DifferentialRevision $revision, - PhabricatorUser $viewer) { - - if (!$viewer->getPHID()) { - return null; - } - - foreach ($revision->getReviewerStatus() as $reviewer) { - if ($reviewer->getReviewerPHID() != $viewer->getPHID()) { - continue; - } - - return $reviewer->getStatus(); - } - - return null; - } - - protected function isViewerReviewerStatusAmong( - DifferentialRevision $revision, - PhabricatorUser $viewer, - array $status_list) { - - $status = $this->getViewerReviewerStatus($revision, $viewer); - if ($status === null) { - return false; - } - - $status_map = array_fuse($status_list); - return isset($status_map[$status]); - } - - protected function applyReviewerEffect( - DifferentialRevision $revision, - PhabricatorUser $viewer, - $value, - $status) { - - $map = array(); - - // When you accept or reject, you may accept or reject on behalf of all - // reviewers you have authority for. When you resign, you only affect - // yourself. - $with_authority = ($status != DifferentialReviewerStatus::STATUS_RESIGNED); - if ($with_authority) { - foreach ($revision->getReviewerStatus() as $reviewer) { - if ($reviewer->hasAuthority($viewer)) { - $map[$reviewer->getReviewerPHID()] = $status; - } - } - } - - // In all cases, you affect yourself. - $map[$viewer->getPHID()] = $status; - - // Convert reviewer statuses into edge data. - foreach ($map as $reviewer_phid => $reviewer_status) { - $map[$reviewer_phid] = array( - 'data' => array( - 'status' => $reviewer_status, - ), - ); - } - - $src_phid = $revision->getPHID(); - $edge_type = DifferentialRevisionHasReviewerEdgeType::EDGECONST; - - $editor = new PhabricatorEdgeEditor(); - foreach ($map as $dst_phid => $edge_data) { - if ($status == DifferentialReviewerStatus::STATUS_RESIGNED) { - // TODO: For now, we just remove these reviewers. In the future, we will - // store resignations explicitly. - $editor->removeEdge($src_phid, $edge_type, $dst_phid); - } else { - $editor->addEdge($src_phid, $edge_type, $dst_phid, $edge_data); - } - } - - $editor->save(); - } - public function newEditField( DifferentialRevision $revision, PhabricatorUser $viewer) { @@ -167,6 +61,9 @@ abstract class DifferentialRevisionActionTransaction $description = $this->getRevisionActionDescription(); $field->setActionDescription($description); + + $group_key = $this->getRevisionActionGroupKey(); + $field->setCommentActionGroupKey($group_key); } } diff --git a/src/applications/differential/xaction/DifferentialRevisionRejectTransaction.php b/src/applications/differential/xaction/DifferentialRevisionRejectTransaction.php index b5ca2971cc..a47f420ec6 100644 --- a/src/applications/differential/xaction/DifferentialRevisionRejectTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionRejectTransaction.php @@ -1,7 +1,7 @@ getViewerReviewerStatus($revision, $viewer) !== null); + } + + protected function isViewerAcceptingReviewer( + DifferentialRevision $revision, + PhabricatorUser $viewer) { + return $this->isViewerReviewerStatusAmong( + $revision, + $viewer, + array( + DifferentialReviewerStatus::STATUS_ACCEPTED, + )); + } + + protected function isViewerRejectingReviewer( + DifferentialRevision $revision, + PhabricatorUser $viewer) { + return $this->isViewerReviewerStatusAmong( + $revision, + $viewer, + array( + DifferentialReviewerStatus::STATUS_REJECTED, + )); + } + + protected function getViewerReviewerStatus( + DifferentialRevision $revision, + PhabricatorUser $viewer) { + + if (!$viewer->getPHID()) { + return null; + } + + foreach ($revision->getReviewerStatus() as $reviewer) { + if ($reviewer->getReviewerPHID() != $viewer->getPHID()) { + continue; + } + + return $reviewer->getStatus(); + } + + return null; + } + + protected function isViewerReviewerStatusAmong( + DifferentialRevision $revision, + PhabricatorUser $viewer, + array $status_list) { + + $status = $this->getViewerReviewerStatus($revision, $viewer); + if ($status === null) { + return false; + } + + $status_map = array_fuse($status_list); + return isset($status_map[$status]); + } + + protected function applyReviewerEffect( + DifferentialRevision $revision, + PhabricatorUser $viewer, + $value, + $status) { + + $map = array(); + + // When you accept or reject, you may accept or reject on behalf of all + // reviewers you have authority for. When you resign, you only affect + // yourself. + $with_authority = ($status != DifferentialReviewerStatus::STATUS_RESIGNED); + if ($with_authority) { + foreach ($revision->getReviewerStatus() as $reviewer) { + if ($reviewer->hasAuthority($viewer)) { + $map[$reviewer->getReviewerPHID()] = $status; + } + } + } + + // In all cases, you affect yourself. + $map[$viewer->getPHID()] = $status; + + // Convert reviewer statuses into edge data. + foreach ($map as $reviewer_phid => $reviewer_status) { + $map[$reviewer_phid] = array( + 'data' => array( + 'status' => $reviewer_status, + ), + ); + } + + $src_phid = $revision->getPHID(); + $edge_type = DifferentialRevisionHasReviewerEdgeType::EDGECONST; + + $editor = new PhabricatorEdgeEditor(); + foreach ($map as $dst_phid => $edge_data) { + if ($status == DifferentialReviewerStatus::STATUS_RESIGNED) { + // TODO: For now, we just remove these reviewers. In the future, we will + // store resignations explicitly. + $editor->removeEdge($src_phid, $edge_type, $dst_phid); + } else { + $editor->addEdge($src_phid, $edge_type, $dst_phid, $edge_data); + } + } + + $editor->save(); + } + +} diff --git a/src/applications/transactions/commentaction/PhabricatorEditEngineCommentAction.php b/src/applications/transactions/commentaction/PhabricatorEditEngineCommentAction.php index dc676630ba..1c1c0c0725 100644 --- a/src/applications/transactions/commentaction/PhabricatorEditEngineCommentAction.php +++ b/src/applications/transactions/commentaction/PhabricatorEditEngineCommentAction.php @@ -7,6 +7,7 @@ abstract class PhabricatorEditEngineCommentAction extends Phobject { private $value; private $initialValue; private $order; + private $groupKey; abstract public function getPHUIXControlType(); abstract public function getPHUIXControlSpecification(); @@ -20,6 +21,15 @@ abstract class PhabricatorEditEngineCommentAction extends Phobject { return $this->key; } + public function setGroupKey($group_key) { + $this->groupKey = $group_key; + return $this; + } + + public function getGroupKey() { + return $this->groupKey; + } + public function setLabel($label) { $this->label = $label; return $this; diff --git a/src/applications/transactions/commentaction/PhabricatorEditEngineCommentActionGroup.php b/src/applications/transactions/commentaction/PhabricatorEditEngineCommentActionGroup.php new file mode 100644 index 0000000000..fb3cbae583 --- /dev/null +++ b/src/applications/transactions/commentaction/PhabricatorEditEngineCommentActionGroup.php @@ -0,0 +1,27 @@ +key = $key; + return $this; + } + + public function getKey() { + return $this->key; + } + + public function setLabel($label) { + $this->label = $label; + return $this; + } + + public function getLabel() { + return $this->label; + } + +} diff --git a/src/applications/transactions/editengine/PhabricatorEditEngine.php b/src/applications/transactions/editengine/PhabricatorEditEngine.php index f45da8d148..b7d8e07a76 100644 --- a/src/applications/transactions/editengine/PhabricatorEditEngine.php +++ b/src/applications/transactions/editengine/PhabricatorEditEngine.php @@ -1549,6 +1549,9 @@ abstract class PhabricatorEditEngine $view->setCommentActions($comment_actions); + $comment_groups = $this->newCommentActionGroups(); + $view->setCommentActionGroups($comment_groups); + return $view; } @@ -2157,6 +2160,10 @@ abstract class PhabricatorEditEngine return $request->getURIData('editAction'); } + protected function newCommentActionGroups() { + return array(); + } + /* -( Form Pages )--------------------------------------------------------- */ diff --git a/src/applications/transactions/editfield/PhabricatorEditField.php b/src/applications/transactions/editfield/PhabricatorEditField.php index a0b63e2da1..2b0c93568b 100644 --- a/src/applications/transactions/editfield/PhabricatorEditField.php +++ b/src/applications/transactions/editfield/PhabricatorEditField.php @@ -25,6 +25,7 @@ abstract class PhabricatorEditField extends Phobject { private $commentActionLabel; private $commentActionValue; + private $commentActionGroupKey; private $commentActionOrder = 1000; private $hasCommentActionValue; @@ -245,6 +246,15 @@ abstract class PhabricatorEditField extends Phobject { return $this->commentActionLabel; } + public function setCommentActionGroupKey($key) { + $this->commentActionGroupKey = $key; + return $this; + } + + public function getCommentActionGroupKey() { + return $this->commentActionGroupKey; + } + public function setCommentActionOrder($order) { $this->commentActionOrder = $order; return $this; @@ -719,7 +729,8 @@ abstract class PhabricatorEditField extends Phobject { ->setKey($this->getKey()) ->setLabel($label) ->setValue($this->getValueForCommentAction($value)) - ->setOrder($this->getCommentActionOrder()); + ->setOrder($this->getCommentActionOrder()) + ->setGroupKey($this->getCommentActionGroupKey()); return $action; } diff --git a/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php b/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php index f752858cc1..13054f63c2 100644 --- a/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php +++ b/src/applications/transactions/view/PhabricatorApplicationTransactionCommentView.php @@ -24,6 +24,7 @@ class PhabricatorApplicationTransactionCommentView extends AphrontView { private $currentVersion; private $versionedDraft; private $commentActions; + private $commentActionGroups = array(); private $transactionTimeline; public function setObjectPHID($object_phid) { @@ -118,6 +119,16 @@ class PhabricatorApplicationTransactionCommentView extends AphrontView { return $this->commentActions; } + public function setCommentActionGroups(array $groups) { + assert_instances_of($groups, 'PhabricatorEditEngineCommentActionGroup'); + $this->commentActionGroups = $groups; + return $this; + } + + public function getCommentActionGroups() { + return $this->commentActionGroups; + } + public function setNoPermission($no_permission) { $this->noPermission = $no_permission; return $this; @@ -279,16 +290,13 @@ class PhabricatorApplicationTransactionCommentView extends AphrontView { 'type' => $comment_action->getPHUIXControlType(), 'spec' => $comment_action->getPHUIXControlSpecification(), 'initialValue' => $comment_action->getInitialValue(), + 'groupKey' => $comment_action->getGroupKey(), ); $type_map[$key] = $comment_action; } - $options = array(); - $options['+'] = pht('Add Action...'); - foreach ($action_map as $key => $item) { - $options[$key] = $item['label']; - } + $options = $this->newCommentActionOptions($action_map); $action_id = celerity_generate_unique_node_id(); $input_id = celerity_generate_unique_node_id(); @@ -424,4 +432,46 @@ class PhabricatorApplicationTransactionCommentView extends AphrontView { return $this->commentID; } + private function newCommentActionOptions(array $action_map) { + $options = array(); + $options['+'] = pht('Add Action...'); + + // Merge options into groups. + $groups = array(); + foreach ($action_map as $key => $item) { + $group_key = $item['groupKey']; + if (!isset($groups[$group_key])) { + $groups[$group_key] = array(); + } + $groups[$group_key][$key] = $item; + } + + $group_specs = $this->getCommentActionGroups(); + $group_labels = mpull($group_specs, 'getLabel', 'getKey'); + + // Reorder groups to put them in the same order as the recognized + // group definitions. + $groups = array_select_keys($groups, array_keys($group_labels)) + $groups; + + // Move options with no group to the end. + $default_group = idx($groups, ''); + if ($default_group) { + unset($groups['']); + $groups[''] = $default_group; + } + + foreach ($groups as $group_key => $group_items) { + if (strlen($group_key)) { + $group_label = idx($group_labels, $group_key, $group_key); + $options[$group_label] = ipull($group_items, 'label'); + } else { + foreach ($group_items as $key => $item) { + $options[$key] = $item['label']; + } + } + } + + return $options; + } + }