From f38a565aa5b675e813a9955a7e41474cbf3bbef0 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 27 Dec 2013 13:16:33 -0800 Subject: [PATCH] Use radio buttons with explanatory text to select commit rule types Summary: Ref T4264. Instead of a dropdown, make this step more informative. Test Plan: {F93928} Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T4264 Differential Revision: https://secure.phabricator.com/D7846 --- .../herald/HeraldPreCommitContentAdapter.php | 10 ++++ .../herald/HeraldPreCommitRefAdapter.php | 10 ++++ .../herald/adapter/HeraldAdapter.php | 14 +++++- .../herald/adapter/HeraldCommitAdapter.php | 7 +++ .../HeraldDifferentialRevisionAdapter.php | 7 +++ .../adapter/HeraldManiphestTaskAdapter.php | 5 ++ .../adapter/HeraldPholioMockAdapter.php | 5 ++ .../herald/controller/HeraldNewController.php | 46 ++++++++++++++++--- 8 files changed, 96 insertions(+), 8 deletions(-) diff --git a/src/applications/diffusion/herald/HeraldPreCommitContentAdapter.php b/src/applications/diffusion/herald/HeraldPreCommitContentAdapter.php index ac922eaa5b..53840c8029 100644 --- a/src/applications/diffusion/herald/HeraldPreCommitContentAdapter.php +++ b/src/applications/diffusion/herald/HeraldPreCommitContentAdapter.php @@ -31,6 +31,16 @@ final class HeraldPreCommitContentAdapter extends HeraldAdapter { return pht('Commit Hook: Commit Content'); } + public function getAdapterSortOrder() { + return 2500; + } + + public function getAdapterContentDescription() { + return pht( + "React to commits being pushed to hosted repositories.\n". + "Hook rules can block changes."); + } + public function getFieldNameMap() { return array( ) + parent::getFieldNameMap(); diff --git a/src/applications/diffusion/herald/HeraldPreCommitRefAdapter.php b/src/applications/diffusion/herald/HeraldPreCommitRefAdapter.php index adfe5951fe..1395ab9eae 100644 --- a/src/applications/diffusion/herald/HeraldPreCommitRefAdapter.php +++ b/src/applications/diffusion/herald/HeraldPreCommitRefAdapter.php @@ -34,6 +34,16 @@ final class HeraldPreCommitRefAdapter extends HeraldAdapter { return pht('Commit Hook: Branches/Tags/Bookmarks'); } + public function getAdapterSortOrder() { + return 2000; + } + + public function getAdapterContentDescription() { + return pht( + "React to branches and tags being pushed to hosted repositories.\n". + "Hook rules can block changes."); + } + public function getFieldNameMap() { return array( self::FIELD_REF_TYPE => pht('Ref type'), diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php index a821d173a1..e38235fafd 100644 --- a/src/applications/herald/adapter/HeraldAdapter.php +++ b/src/applications/herald/adapter/HeraldAdapter.php @@ -136,9 +136,21 @@ abstract class HeraldAdapter { } abstract public function getAdapterContentName(); + abstract public function getAdapterContentDescription(); abstract public function getAdapterApplicationClass(); abstract public function getObject(); + public function getAdapterSortKey() { + return sprintf( + '%08d%s', + $this->getAdapterSortOrder(), + $this->getAdapterContentName()); + } + + public function getAdapterSortOrder() { + return 1000; + } + /* -( Fields )------------------------------------------------------------- */ @@ -814,6 +826,7 @@ abstract class HeraldAdapter { $adapters = id(new PhutilSymbolLoader()) ->setAncestorClass(__CLASS__) ->loadObjects(); + $adapters = msort($adapters, 'getAdapterSortKey'); } return $adapters; } @@ -846,7 +859,6 @@ abstract class HeraldAdapter { $map[$type] = $name; } - asort($map); return $map; } diff --git a/src/applications/herald/adapter/HeraldCommitAdapter.php b/src/applications/herald/adapter/HeraldCommitAdapter.php index e81f8b7001..cd6cebd2b0 100644 --- a/src/applications/herald/adapter/HeraldCommitAdapter.php +++ b/src/applications/herald/adapter/HeraldCommitAdapter.php @@ -42,6 +42,13 @@ final class HeraldCommitAdapter extends HeraldAdapter { return pht('Commits'); } + public function getAdapterContentDescription() { + return pht( + "React to new commits appearing in tracked repositories.\n". + "Commit rules can send email, flag commits, trigger audits, ". + "and run build plans."); + } + public function getFieldNameMap() { return array( self::FIELD_NEED_AUDIT_FOR_PACKAGE => diff --git a/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php b/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php index 7a9116d3a4..976154634d 100644 --- a/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php +++ b/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php @@ -36,6 +36,13 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter { return pht('Differential Revisions'); } + public function getAdapterContentDescription() { + return pht( + "React to revisions being created or updated.\n". + "Revision rules can send email, flag revisions, add reviewers, ". + "and run build plans."); + } + public function getFields() { return array_merge( array( diff --git a/src/applications/herald/adapter/HeraldManiphestTaskAdapter.php b/src/applications/herald/adapter/HeraldManiphestTaskAdapter.php index eb996e1004..6fec583819 100644 --- a/src/applications/herald/adapter/HeraldManiphestTaskAdapter.php +++ b/src/applications/herald/adapter/HeraldManiphestTaskAdapter.php @@ -14,6 +14,11 @@ final class HeraldManiphestTaskAdapter extends HeraldAdapter { return 'PhabricatorApplicationManiphest'; } + public function getAdapterContentDescription() { + return pht( + 'React to tasks being created or updated.'); + } + public function setTask(ManiphestTask $task) { $this->task = $task; return $this; diff --git a/src/applications/herald/adapter/HeraldPholioMockAdapter.php b/src/applications/herald/adapter/HeraldPholioMockAdapter.php index d117214de5..a853120306 100644 --- a/src/applications/herald/adapter/HeraldPholioMockAdapter.php +++ b/src/applications/herald/adapter/HeraldPholioMockAdapter.php @@ -12,6 +12,11 @@ final class HeraldPholioMockAdapter extends HeraldAdapter { return 'PhabricatorApplicationPholio'; } + public function getAdapterContentDescription() { + return pht( + 'React to mocks being created or updated.'); + } + public function getObject() { return $this->mock; } diff --git a/src/applications/herald/controller/HeraldNewController.php b/src/applications/herald/controller/HeraldNewController.php index 48595f9247..e65c322f71 100644 --- a/src/applications/herald/controller/HeraldNewController.php +++ b/src/applications/herald/controller/HeraldNewController.php @@ -53,19 +53,21 @@ final class HeraldNewController extends HeraldController { ->setUser($user) ->setAction($this->getApplicationURI('new/')); - $rule_types = $this->renderRuleTypeControl($rule_type_map, $e_rule); + $content_types = $this->renderContentTypeControl( + $content_type_map, + $e_type); + + $rule_types = $this->renderRuleTypeControl( + $rule_type_map, + $e_rule); switch ($step) { case 0: default: $form ->addHiddenInput('step', 1) - ->appendChild( - id(new AphrontFormSelectControl()) - ->setLabel(pht('New Rule for')) - ->setName('content_type') - ->setValue($request->getStr('content_type')) - ->setOptions($content_type_map)); + ->appendChild($content_types); + $cancel_text = null; $cancel_uri = $this->getApplicationURI(); break; @@ -73,7 +75,16 @@ final class HeraldNewController extends HeraldController { $form ->addHiddenInput('content_type', $request->getStr('content_type')) ->addHiddenInput('step', 2) + ->appendChild( + id(new AphrontFormStaticControl()) + ->setLabel(pht('Rule for')) + ->setValue( + phutil_tag( + 'strong', + array(), + idx($content_type_map, $content_type)))) ->appendChild($rule_types); + $cancel_text = pht('Back'); $cancel_uri = id(new PhutilURI('new/')) ->setQueryParams( @@ -112,6 +123,27 @@ final class HeraldNewController extends HeraldController { )); } + private function renderContentTypeControl(array $content_type_map, $e_type) { + $request = $this->getRequest(); + + $radio = id(new AphrontFormRadioButtonControl()) + ->setLabel(pht('New Rule for')) + ->setName('content_type') + ->setValue($request->getStr('content_type')) + ->setError($e_type); + + foreach ($content_type_map as $value => $name) { + $adapter = HeraldAdapter::getAdapterForContentType($value); + $radio->addButton( + $value, + $name, + phutil_escape_html_newlines($adapter->getAdapterContentDescription())); + } + + return $radio; + } + + private function renderRuleTypeControl(array $rule_type_map, $e_rule) { $request = $this->getRequest();