Move Herald rule querying into HeraldRuleQuery

Summary: Ref T2769. The `HeraldRule` class has some query logic; move it into `HeraldRuleQuery`. Also some minor cleanup.

Test Plan: Ran test console, created a new revision, used `reparse.php --herald`. Verified rules triggered correctly.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2769

Differential Revision: https://secure.phabricator.com/D6689

src/applications/herald/adapter/HeraldCommitAdapter.php

@@ -100,6 +100,7 @@ final class HeraldCommitAdapter extends HeraldAdapter {
           self::ACTION_ADD_CC,
           self::ACTION_REMOVE_CC,
           self::ACTION_EMAIL,
+          self::ACTION_FLAG,
           self::ACTION_NOTHING,
         );
     }

src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php

@@ -258,6 +258,7 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter {
           self::ACTION_ADD_CC,
           self::ACTION_REMOVE_CC,
           self::ACTION_EMAIL,
+          self::ACTION_FLAG,
           self::ACTION_NOTHING,
         );
     }

src/applications/herald/controller/HeraldTestConsoleController.php

@@ -68,9 +68,13 @@ final class HeraldTestConsoleController extends HeraldController {
             throw new Exception("Can not build adapter for object!");
           }
 
-          $rules = HeraldRule::loadAllByContentTypeWithFullData(
-            $adapter->getAdapterContentType(),
-            $object->getPHID());
+          $rules = id(new HeraldRuleQuery())
+            ->setViewer($user)
+            ->withContentTypes(array($adapter->getAdapterContentType()))
+            ->needConditionsAndActions(true)
+            ->needAppliedToPHIDs(array($object->getPHID()))
+            ->needValidateAuthors(true)
+            ->execute();
 
           $engine = new HeraldEngine();
           $effects = $engine->applyRules($rules, $adapter);

src/applications/herald/engine/HeraldEngine.php

@@ -14,15 +14,18 @@ final class HeraldEngine {
     return idx($this->rules, $id);
   }
 
-  public static function loadAndApplyRules(HeraldAdapter $object) {
-    $content_type = $object->getAdapterContentType();
-    $rules = HeraldRule::loadAllByContentTypeWithFullData(
-      $content_type,
-      $object->getPHID());
+  public static function loadAndApplyRules(HeraldAdapter $adapter) {
+    $rules = id(new HeraldRuleQuery())
+      ->setViewer(PhabricatorUser::getOmnipotentUser())
+      ->withContentTypes(array($adapter->getAdapterContentType()))
+      ->needConditionsAndActions(true)
+      ->needAppliedToPHIDs(array($adapter->getPHID()))
+      ->needValidateAuthors(true)
+      ->execute();
 
     $engine = new HeraldEngine();
-    $effects = $engine->applyRules($rules, $object);
-    $engine->applyEffects($effects, $object, $rules);
+    $effects = $engine->applyRules($rules, $adapter);
+    $engine->applyEffects($effects, $adapter, $rules);
 
     return $engine->getTranscript();
   }
@@ -215,7 +218,7 @@ final class HeraldEngine {
     } else if (!$conditions) {
       $reason = "Rule failed automatically because it has no conditions.";
       $result = false;
-    } else if ($rule->hasInvalidOwner()) {
+    } else if (!$rule->hasValidAuthor()) {
       $reason = "Rule failed automatically because its owner is invalid ".
                 "or disabled.";
       $result = false;

src/applications/herald/query/HeraldRuleQuery.php

@@ -9,6 +9,10 @@ final class HeraldRuleQuery
   private $ruleTypes;
   private $contentTypes;
 
+  private $needConditionsAndActions;
+  private $needAppliedToPHIDs;
+  private $needValidateAuthors;
+
   public function withIDs(array $ids) {
     $this->ids = $ids;
     return $this;
@@ -34,6 +38,26 @@ final class HeraldRuleQuery
     return $this;
   }
 
+  public function withExecutableRules($executable) {
+    $this->executable = $executable;
+    return $this;
+  }
+
+  public function needConditionsAndActions($need) {
+    $this->needConditionsAndActions = $need;
+    return $this;
+  }
+
+  public function needAppliedToPHIDs(array $phids) {
+    $this->needAppliedToPHIDs = $phids;
+    return $this;
+  }
+
+  public function needValidateAuthors($need) {
+    $this->needValidateAuthors = $need;
+    return $this;
+  }
+
   public function loadPage() {
     $table = new HeraldRule();
     $conn_r = $table->establishConnection('r');
@@ -49,6 +73,56 @@ final class HeraldRuleQuery
     return $table->loadAllFromArray($data);
   }
 
+  public function willFilterPage(array $rules) {
+    $rule_ids = mpull($rules, 'getID');
+
+    if ($this->needValidateAuthors) {
+      $this->validateRuleAuthors($rules);
+    }
+
+    if ($this->needConditionsAndActions) {
+      $conditions = id(new HeraldCondition())->loadAllWhere(
+        'ruleID IN (%Ld)',
+        $rule_ids);
+      $conditions = mgroup($conditions, 'getRuleID');
+
+      $actions = id(new HeraldAction())->loadAllWhere(
+        'ruleID IN (%Ld)',
+        $rule_ids);
+      $actions = mgroup($actions, 'getRuleID');
+
+      foreach ($rules as $rule) {
+        $rule->attachActions(idx($actions, $rule->getID(), array()));
+        $rule->attachConditions(idx($conditions, $rule->getID(), array()));
+      }
+    }
+
+    if ($this->needAppliedToPHIDs) {
+      $conn_r = id(new HeraldRule())->establishConnection('r');
+      $applied = queryfx_all(
+        $conn_r,
+        'SELECT * FROM %T WHERE ruleID IN (%Ld) AND phid IN (%Ls)',
+        HeraldRule::TABLE_RULE_APPLIED,
+        $rule_ids,
+        $this->needAppliedToPHIDs);
+
+      $map = array();
+      foreach ($applied as $row) {
+        $map[$row['ruleID']][$row['phid']] = true;
+      }
+
+      foreach ($rules as $rule) {
+        foreach ($this->needAppliedToPHIDs as $phid) {
+          $rule->setRuleApplied(
+            $phid,
+            isset($map[$rule->getID()][$phid]));
+        }
+      }
+    }
+
+    return $rules;
+  }
+
   private function buildWhereClause($conn_r) {
     $where = array();
 
@@ -92,4 +166,42 @@ final class HeraldRuleQuery
     return $this->formatWhereClause($where);
   }
 
+  private function validateRuleAuthors(array $rules) {
+
+    // "Global" rules always have valid authors.
+    foreach ($rules as $key => $rule) {
+      if ($rule->isGlobalRule()) {
+        $rule->attachValidAuthor(true);
+        unset($rules[$key]);
+        continue;
+      }
+    }
+
+    if (!$rules) {
+      return;
+    }
+
+    // For personal rules, the author needs to exist and not be disabled.
+    $user_phids = mpull($rules, 'getAuthorPHID');
+    $users = id(new PhabricatorPeopleQuery())
+      ->setViewer($this->getViewer())
+      ->withPHIDs($user_phids)
+      ->execute();
+    $users = mpull($users, null, 'getPHID');
+
+    foreach ($rules as $key => $rule) {
+      $author_phid = $rule->getAuthorPHID();
+      if (empty($users[$author_phid])) {
+        $rule->attachValidAuthor(false);
+        continue;
+      }
+      if ($users[$author_phid]->getIsDisabled()) {
+        $rule->attachValidAuthor(false);
+        continue;
+      }
+
+      $rule->attachValidAuthor(true);
+    }
+  }
+
 }

src/applications/herald/storage/HeraldRule.php

@@ -16,7 +16,7 @@ final class HeraldRule extends HeraldDAO
   protected $configVersion = 9;
 
   private $ruleApplied = array(); // phids for which this rule has been applied
-  private $invalidOwner = false;
+  private $validAuthor = self::ATTACHABLE;
   private $conditions;
   private $actions;
 
@@ -30,77 +30,6 @@ final class HeraldRule extends HeraldDAO
     return PhabricatorPHID::generateNewPHID(HeraldPHIDTypeRule::TYPECONST);
   }
 
-  public static function loadAllByContentTypeWithFullData(
-    $content_type,
-    $object_phid) {
-
-    $rules = id(new HeraldRule())->loadAllWhere(
-      'contentType = %s',
-      $content_type);
-
-    if (!$rules) {
-      return array();
-    }
-
-    self::flagDisabledUserRules($rules);
-
-    $rule_ids = mpull($rules, 'getID');
-
-    $conditions = id(new HeraldCondition())->loadAllWhere(
-      'ruleID in (%Ld)',
-      $rule_ids);
-
-    $actions = id(new HeraldAction())->loadAllWhere(
-      'ruleID in (%Ld)',
-      $rule_ids);
-
-    $applied = queryfx_all(
-      id(new HeraldRule())->establishConnection('r'),
-      'SELECT * FROM %T WHERE phid = %s',
-      self::TABLE_RULE_APPLIED,
-      $object_phid);
-    $applied = ipull($applied, null, 'ruleID');
-
-    $conditions = mgroup($conditions, 'getRuleID');
-    $actions = mgroup($actions, 'getRuleID');
-    $applied = igroup($applied, 'ruleID');
-
-    foreach ($rules as $rule) {
-      $rule->setRuleApplied($object_phid, isset($applied[$rule->getID()]));
-
-      $rule->attachConditions(idx($conditions, $rule->getID(), array()));
-      $rule->attachActions(idx($actions, $rule->getID(), array()));
-    }
-
-    return $rules;
-  }
-
-  private static function flagDisabledUserRules(array $rules) {
-    assert_instances_of($rules, 'HeraldRule');
-
-    $users = array();
-    foreach ($rules as $rule) {
-      if ($rule->getRuleType() != HeraldRuleTypeConfig::RULE_TYPE_PERSONAL) {
-        continue;
-      }
-      $users[$rule->getAuthorPHID()] = true;
-    }
-
-    $handles = id(new PhabricatorObjectHandleData(array_keys($users)))
-      ->setViewer(PhabricatorUser::getOmnipotentUser())
-      ->loadHandles();
-
-    foreach ($rules as $key => $rule) {
-      if ($rule->getRuleType() != HeraldRuleTypeConfig::RULE_TYPE_PERSONAL) {
-        continue;
-      }
-      $handle = $handles[$rule->getAuthorPHID()];
-      if (!$handle->isComplete() || $handle->isDisabled()) {
-        $rule->invalidOwner = true;
-      }
-    }
-  }
-
   public function getRuleApplied($phid) {
     if (idx($this->ruleApplied, $phid) === null) {
       throw new Exception("Call setRuleApplied() before getRuleApplied()!");
@@ -229,8 +158,13 @@ final class HeraldRule extends HeraldDAO
 //    $this->saveTransaction();
   }
 
-  public function hasInvalidOwner() {
-    return $this->invalidOwner;
+  public function hasValidAuthor() {
+    return $this->assertAttached($this->validAuthor);
+  }
+
+  public function attachValidAuthor($valid) {
+    $this->validAuthor = $valid;
+    return $this;
   }
 
   public function isGlobalRule() {

src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php

@@ -21,14 +21,19 @@ final class PhabricatorRepositoryCommitHeraldWorker
       return;
     }
 
-    $rules = HeraldRule::loadAllByContentTypeWithFullData(
-      id(new HeraldCommitAdapter())->getAdapterContentType(),
-      $commit->getPHID());
-
     $adapter = HeraldCommitAdapter::newLegacyAdapter(
       $repository,
       $commit,
       $data);
+
+    $rules = id(new HeraldRuleQuery())
+      ->setViewer(PhabricatorUser::getOmnipotentUser())
+      ->withContentTypes(array($adapter->getAdapterContentType()))
+      ->needConditionsAndActions(true)
+      ->needAppliedToPHIDs(array($adapter->getPHID()))
+      ->needValidateAuthors(true)
+      ->execute();
+
     $engine = new HeraldEngine();
 
     $effects = $engine->applyRules($rules, $adapter);