From d60d4e6a05212f5e31c93c36ee39c775eafba56a Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 12 Sep 2019 12:54:08 -0700 Subject: [PATCH] Don't present users with Herald fields/actions for uninstalled applications, unless the rule already uses them Summary: Fixes T7961. Currently, we present Herald users with actions like "Require legalpad signatures" and "Run build plans" even if Legalpad and Harbormaster are not installed. Instead, allow fields and actions to be made "unavailable", which means that we won't present them as options when adding to new or existing rules. If you edit a rule which already uses one of these fields or actions, it isn't affected. Test Plan: - Created a rule with a legalpad action, uninstalled legalpad, edited the rule. Action remained untouched. - Created a new rule, wasn't offered the legalpad action. - Reinstalled the application, saw the action again. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T7961 Differential Revision: https://secure.phabricator.com/D20808 --- resources/celerity/map.php | 6 +-- .../HarbormasterRunBuildPlansHeraldAction.php | 4 ++ .../herald/action/HeraldAction.php | 4 ++ .../herald/adapter/HeraldAdapter.php | 20 ++++++++++ .../controller/HeraldRuleController.php | 10 ++++- src/applications/herald/field/HeraldField.php | 4 ++ .../LegalpadRequireSignatureHeraldAction.php | 5 +++ .../js/application/herald/HeraldRuleEditor.js | 40 +++++++++++++++---- 8 files changed, 80 insertions(+), 13 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 651714b117..f5ff8827ae 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -397,7 +397,7 @@ return array( 'rsrc/js/application/files/behavior-icon-composer.js' => '38a6cedb', 'rsrc/js/application/files/behavior-launch-icon-composer.js' => 'a17b84f1', 'rsrc/js/application/harbormaster/behavior-harbormaster-log.js' => 'b347a301', - 'rsrc/js/application/herald/HeraldRuleEditor.js' => '27daef73', + 'rsrc/js/application/herald/HeraldRuleEditor.js' => '2633bef7', 'rsrc/js/application/herald/PathTypeahead.js' => 'ad486db3', 'rsrc/js/application/herald/herald-rule-editor.js' => '0922e81d', 'rsrc/js/application/maniphest/behavior-batch-selector.js' => '139ef688', @@ -571,7 +571,7 @@ return array( 'global-drag-and-drop-css' => '1d2713a4', 'harbormaster-css' => '8dfe16b2', 'herald-css' => '648d39e2', - 'herald-rule-editor' => '27daef73', + 'herald-rule-editor' => '2633bef7', 'herald-test-css' => 'e004176f', 'inline-comment-summary-css' => '81eb368d', 'javelin-aphlict' => '022516b4', @@ -1117,7 +1117,7 @@ return array( 'javelin-json', 'phabricator-draggable-list', ), - '27daef73' => array( + '2633bef7' => array( 'multirow-row-manager', 'javelin-install', 'javelin-util', diff --git a/src/applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php b/src/applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php index 9fc053e8ae..6934a2e3d9 100644 --- a/src/applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php +++ b/src/applications/harbormaster/herald/HarbormasterRunBuildPlansHeraldAction.php @@ -96,4 +96,8 @@ final class HarbormasterRunBuildPlansHeraldAction return $record->getTarget(); } + public function isActionAvailable() { + return id(new PhabricatorHarbormasterApplication())->isInstalled(); + } + } diff --git a/src/applications/herald/action/HeraldAction.php b/src/applications/herald/action/HeraldAction.php index a9740d1736..d914c97a1c 100644 --- a/src/applications/herald/action/HeraldAction.php +++ b/src/applications/herald/action/HeraldAction.php @@ -405,4 +405,8 @@ abstract class HeraldAction extends Phobject { return array(); } + public function isActionAvailable() { + return true; + } + } diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php index cb8545b71d..2832c6d9f4 100644 --- a/src/applications/herald/adapter/HeraldAdapter.php +++ b/src/applications/herald/adapter/HeraldAdapter.php @@ -373,6 +373,16 @@ abstract class HeraldAdapter extends Phobject { return $field->getFieldGroupKey(); } + public function isFieldAvailable($field_key) { + $field = $this->getFieldImplementation($field_key); + + if (!$field) { + return null; + } + + return $field->isFieldAvailable(); + } + /* -( Conditions )--------------------------------------------------------- */ @@ -765,6 +775,16 @@ abstract class HeraldAdapter extends Phobject { return $action->getActionGroupKey(); } + public function isActionAvailable($action_key) { + $action = $this->getActionImplementation($action_key); + + if (!$action) { + return null; + } + + return $action->isActionAvailable(); + } + public function getActions($rule_type) { $actions = array(); foreach ($this->getActionsForRuleType($rule_type) as $key => $action) { diff --git a/src/applications/herald/controller/HeraldRuleController.php b/src/applications/herald/controller/HeraldRuleController.php index 73c1e8c39c..5e13522b6e 100644 --- a/src/applications/herald/controller/HeraldRuleController.php +++ b/src/applications/herald/controller/HeraldRuleController.php @@ -684,7 +684,10 @@ final class HeraldRuleController extends HeraldController { $group_map = array(); foreach ($field_map as $field_key => $field_name) { $group_key = $adapter->getFieldGroupKey($field_key); - $group_map[$group_key][$field_key] = $field_name; + $group_map[$group_key][$field_key] = array( + 'name' => $field_name, + 'available' => $adapter->isFieldAvailable($field_key), + ); } return $this->getGroups( @@ -696,7 +699,10 @@ final class HeraldRuleController extends HeraldController { $group_map = array(); foreach ($action_map as $action_key => $action_name) { $group_key = $adapter->getActionGroupKey($action_key); - $group_map[$group_key][$action_key] = $action_name; + $group_map[$group_key][$action_key] = array( + 'name' => $action_name, + 'available' => $adapter->isActionAvailable($action_key), + ); } return $this->getGroups( diff --git a/src/applications/herald/field/HeraldField.php b/src/applications/herald/field/HeraldField.php index 6f14091aff..2a1e89d558 100644 --- a/src/applications/herald/field/HeraldField.php +++ b/src/applications/herald/field/HeraldField.php @@ -241,4 +241,8 @@ abstract class HeraldField extends Phobject { return false; } + public function isFieldAvailable() { + return true; + } + } diff --git a/src/applications/legalpad/herald/LegalpadRequireSignatureHeraldAction.php b/src/applications/legalpad/herald/LegalpadRequireSignatureHeraldAction.php index 7ff69d37d5..b477c2ce35 100644 --- a/src/applications/legalpad/herald/LegalpadRequireSignatureHeraldAction.php +++ b/src/applications/legalpad/herald/LegalpadRequireSignatureHeraldAction.php @@ -130,4 +130,9 @@ final class LegalpadRequireSignatureHeraldAction 'Require document signatures: %s.', $this->renderHandleList($value)); } + + public function isActionAvailable() { + return id(new PhabricatorLegalpadApplication())->isInstalled(); + } + } diff --git a/webroot/rsrc/js/application/herald/HeraldRuleEditor.js b/webroot/rsrc/js/application/herald/HeraldRuleEditor.js index 443fe9811e..254061533a 100644 --- a/webroot/rsrc/js/application/herald/HeraldRuleEditor.js +++ b/webroot/rsrc/js/application/herald/HeraldRuleEditor.js @@ -350,8 +350,10 @@ JX.install('HeraldRuleEditor', { sigil: 'field-select' }; - var field_select = this._renderGroupSelect(groups, attrs); - field_select.value = this._config.conditions[row_id][0]; + var field_select = this._renderGroupSelect( + groups, + attrs, + this._config.conditions[row_id][0]); var field_cell = JX.$N('td', {sigil: 'field-cell'}, field_select); @@ -367,18 +369,38 @@ JX.install('HeraldRuleEditor', { } }, - _renderGroupSelect: function(groups, attrs) { + _renderGroupSelect: function(groups, attrs, value) { var optgroups = []; for (var ii = 0; ii < groups.length; ii++) { var group = groups[ii]; var options = []; for (var k in group.options) { - options.push(JX.$N('option', {value: k}, group.options[k])); + var option = group.options[k]; + + var name = option.name; + var available = option.available; + + // See T7961. If the option is not marked as "available", we only + // include it in the dropdown if the dropdown already has it as a + // value. We want to hide options provided by applications which are + // not installed, but do not want to break existing rules. + + if (available || (k === value)) { + options.push(JX.$N('option', {value: k}, name)); + } + } + if (options.length) { + optgroups.push(JX.$N('optgroup', {label: group.label}, options)); } - optgroups.push(JX.$N('optgroup', {label: group.label}, options)); } - return JX.$N('select', attrs, optgroups); + var select = JX.$N('select', attrs, optgroups); + + if (value !== undefined) { + select.value = value; + } + + return select; }, _newAction : function(data) { @@ -402,8 +424,10 @@ JX.install('HeraldRuleEditor', { sigil: 'action-select' }; - var action_select = this._renderGroupSelect(groups, attrs); - action_select.value = action[0]; + var action_select = this._renderGroupSelect( + groups, + attrs, + action[0]); var action_cell = JX.$N('td', {sigil: 'action-cell'}, action_select);