From 8a8123c9db3bc53bcd93e3e8e79e90607b08f47c Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 14 Nov 2018 14:50:40 -0800 Subject: [PATCH] Replace the primary "Disabled/Enabled" Herald Rule filter with "Active/Inactive", considering author status Summary: Ref T13216. See PHI947. In Herald, Personal rules do not run if their author's account is disabled. This isn't communicated very clearly in the UI, and the way the SearchEngine/Query are set up isn't great. Define "active" as "rule will actually run", which specifically means "rule is enabled, and has a valid (non-disabled) author if it needs one". Change the meaning of the "Active" default filter from "rule is enabled" to "rule is enabled, and has a valid author if it needs one". Refine the status badge on the view controller to show this "invalid author" state. Tweak the language for "Disable/Enable" to be more consistent -- we currently call it "disabled" in some cases and "archived" in others. Test Plan: - Disabled a user account and saw their personal rules behave properly with the new filters/options/view controller. - Disabled/enabled a rule, saw consistent text. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13216 Differential Revision: https://secure.phabricator.com/D19805 --- .../controller/HeraldDisableController.php | 8 ++--- .../controller/HeraldRuleViewController.php | 18 ++++------ .../herald/query/HeraldRuleQuery.php | 36 ++++++++++++++++++- .../herald/query/HeraldRuleSearchEngine.php | 22 ++++++++++-- 4 files changed, 65 insertions(+), 19 deletions(-) diff --git a/src/applications/herald/controller/HeraldDisableController.php b/src/applications/herald/controller/HeraldDisableController.php index bdbefa55ea..def87049f7 100644 --- a/src/applications/herald/controller/HeraldDisableController.php +++ b/src/applications/herald/controller/HeraldDisableController.php @@ -44,13 +44,13 @@ final class HeraldDisableController extends HeraldController { } if ($is_disable) { - $title = pht('Really archive this rule?'); + $title = pht('Really disable this rule?'); $body = pht('This rule will no longer activate.'); - $button = pht('Archive Rule'); + $button = pht('Disable Rule'); } else { - $title = pht('Really activate this rule?'); + $title = pht('Really enable this rule?'); $body = pht('This rule will become active again.'); - $button = pht('Activate Rule'); + $button = pht('Enable Rule'); } $dialog = id(new AphrontDialogView()) diff --git a/src/applications/herald/controller/HeraldRuleViewController.php b/src/applications/herald/controller/HeraldRuleViewController.php index 9e696c23c4..70a2d20224 100644 --- a/src/applications/herald/controller/HeraldRuleViewController.php +++ b/src/applications/herald/controller/HeraldRuleViewController.php @@ -14,6 +14,7 @@ final class HeraldRuleViewController extends HeraldController { ->setViewer($viewer) ->withIDs(array($id)) ->needConditionsAndActions(true) + ->needValidateAuthors(true) ->executeOne(); if (!$rule) { return new Aphront404Response(); @@ -26,15 +27,11 @@ final class HeraldRuleViewController extends HeraldController { ->setHeaderIcon('fa-bullhorn'); if ($rule->getIsDisabled()) { - $header->setStatus( - 'fa-ban', - 'red', - pht('Archived')); + $header->setStatus('fa-ban', 'red', pht('Disabled')); + } else if (!$rule->hasValidAuthor()) { + $header->setStatus('fa-user', 'red', pht('Author Not Active')); } else { - $header->setStatus( - 'fa-check', - 'bluegrey', - pht('Active')); + $header->setStatus('fa-check', 'bluegrey', pht('Active')); } $curtain = $this->buildCurtain($rule); @@ -90,16 +87,15 @@ final class HeraldRuleViewController extends HeraldController { if ($rule->getIsDisabled()) { $disable_uri = "disable/{$id}/enable/"; $disable_icon = 'fa-check'; - $disable_name = pht('Activate Rule'); + $disable_name = pht('Enable Rule'); } else { $disable_uri = "disable/{$id}/disable/"; $disable_icon = 'fa-ban'; - $disable_name = pht('Archive Rule'); + $disable_name = pht('Disable Rule'); } $curtain->addAction( id(new PhabricatorActionView()) - ->setName(pht('Disable Rule')) ->setHref($this->getApplicationURI($disable_uri)) ->setIcon($disable_icon) ->setName($disable_name) diff --git a/src/applications/herald/query/HeraldRuleQuery.php b/src/applications/herald/query/HeraldRuleQuery.php index 4fd48d3109..e6dba43c7a 100644 --- a/src/applications/herald/query/HeraldRuleQuery.php +++ b/src/applications/herald/query/HeraldRuleQuery.php @@ -8,6 +8,7 @@ final class HeraldRuleQuery extends PhabricatorCursorPagedPolicyAwareQuery { private $ruleTypes; private $contentTypes; private $disabled; + private $active; private $datasourceQuery; private $triggerObjectPHIDs; @@ -45,6 +46,11 @@ final class HeraldRuleQuery extends PhabricatorCursorPagedPolicyAwareQuery { return $this; } + public function withActive($active) { + $this->active = $active; + return $this; + } + public function withDatasourceQuery($query) { $this->datasourceQuery = $query; return $this; @@ -92,10 +98,31 @@ final class HeraldRuleQuery extends PhabricatorCursorPagedPolicyAwareQuery { } } - if ($this->needValidateAuthors) { + if ($this->needValidateAuthors || ($this->active !== null)) { $this->validateRuleAuthors($rules); } + if ($this->active !== null) { + $need_active = (bool)$this->active; + foreach ($rules as $key => $rule) { + if ($rule->getIsDisabled()) { + $is_active = false; + } else if (!$rule->hasValidAuthor()) { + $is_active = false; + } else { + $is_active = true; + } + + if ($is_active != $need_active) { + unset($rules[$key]); + } + } + } + + if (!$rules) { + return array(); + } + if ($this->needConditionsAndActions) { $conditions = id(new HeraldCondition())->loadAllWhere( 'ruleID IN (%Ld)', @@ -213,6 +240,13 @@ final class HeraldRuleQuery extends PhabricatorCursorPagedPolicyAwareQuery { (int)$this->disabled); } + if ($this->active !== null) { + $where[] = qsprintf( + $conn, + 'rule.isDisabled = %d', + (int)(!$this->active)); + } + if ($this->datasourceQuery !== null) { $where[] = qsprintf( $conn, diff --git a/src/applications/herald/query/HeraldRuleSearchEngine.php b/src/applications/herald/query/HeraldRuleSearchEngine.php index 3a6da6145a..47a6832731 100644 --- a/src/applications/herald/query/HeraldRuleSearchEngine.php +++ b/src/applications/herald/query/HeraldRuleSearchEngine.php @@ -11,7 +11,8 @@ final class HeraldRuleSearchEngine extends PhabricatorApplicationSearchEngine { } public function newQuery() { - return new HeraldRuleQuery(); + return id(new HeraldRuleQuery()) + ->needValidateAuthors(true); } protected function buildCustomSearchFields() { @@ -41,7 +42,14 @@ final class HeraldRuleSearchEngine extends PhabricatorApplicationSearchEngine { pht('Search for rules affecting given types of content.')) ->setOptions($content_types), id(new PhabricatorSearchThreeStateField()) - ->setLabel(pht('Rule Status')) + ->setLabel(pht('Active Rules')) + ->setKey('active') + ->setOptions( + pht('(Show All)'), + pht('Show Only Active Rules'), + pht('Show Only Inactive Rules')), + id(new PhabricatorSearchThreeStateField()) + ->setLabel(pht('Disabled Rules')) ->setKey('disabled') ->setOptions( pht('(Show All)'), @@ -69,6 +77,10 @@ final class HeraldRuleSearchEngine extends PhabricatorApplicationSearchEngine { $query->withDisabled($map['disabled']); } + if ($map['active'] !== null) { + $query->withActive($map['active']); + } + return $query; } @@ -99,7 +111,8 @@ final class HeraldRuleSearchEngine extends PhabricatorApplicationSearchEngine { case 'all': return $query; case 'active': - return $query->setParameter('disabled', false); + return $query + ->setParameter('active', true); case 'authored': return $query ->setParameter('authorPHIDs', array($viewer_phid)) @@ -145,6 +158,9 @@ final class HeraldRuleSearchEngine extends PhabricatorApplicationSearchEngine { if ($rule->getIsDisabled()) { $item->setDisabled(true); $item->addIcon('fa-lock grey', pht('Disabled')); + } else if (!$rule->hasValidAuthor()) { + $item->setDisabled(true); + $item->addIcon('fa-user grey', pht('Author Not Active')); } $content_type_name = idx($content_type_map, $rule->getContentType());