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

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())

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(
+      $header->setStatus('fa-ban', 'red', pht('Disabled'));
-        'fa-ban',
+    } else if (!$rule->hasValidAuthor()) {
-        'red',
+      $header->setStatus('fa-user', 'red', pht('Author Not Active'));
-        pht('Archived'));
     } else {
-      $header->setStatus(
+      $header->setStatus('fa-check', 'bluegrey', pht('Active'));
-        '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)

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,

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());