Attach HeraldRules to HeraldEffects

Summary:
Ref T7731. For no particular reason, we currently put `ruleID` and `rulePHID` on `HeraldEffect` objects.

Pretty much all callers need the `HeraldRule` objects instead, and some go to great lengths to get them.

Just attach the `Rule` objects.

Test Plan: Will test thoroughly after next-ish changeset.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7731

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

src/applications/differential/editor/DifferentialDiffEditor.php

@@ -145,12 +145,7 @@ final class DifferentialDiffEditor
           }
 
           if ($blocking_effect) {
-            $rule = idx($rules, $effect->getRuleID());
+            $rule = $blocking_effect->getRule();
-            if ($rule && strlen($rule->getName())) {
-              $rule_name = $rule->getName();
-            } else {
-              $rule_name = pht('Unnamed Herald Rule');
-            }
 
             $message = $effect->getTarget();
             if (!strlen($message)) {
@@ -164,8 +159,8 @@ final class DifferentialDiffEditor
                 "Creation of this diff was rejected by Herald rule %s.\n".
                 "  Rule: %s\n".
                 "Reason: %s",
-                'H'.$effect->getRuleID(),
+                $rule->getMonogram(),
-                $rule_name,
+                $rule->getName(),
                 $message));
           }
           break;

src/applications/diffusion/engine/DiffusionCommitHookEngine.php

@@ -337,22 +337,16 @@ final class DiffusionCommitHookEngine extends Phobject {
     }
 
     if ($blocking_effect) {
+      $rule = $blocking_effect->getRule();
+
       $this->rejectCode = PhabricatorRepositoryPushLog::REJECT_HERALD;
-      $this->rejectDetails = $blocking_effect->getRulePHID();
+      $this->rejectDetails = $rule->getPHID();
 
       $message = $blocking_effect->getTarget();
       if (!strlen($message)) {
         $message = pht('(None.)');
       }
 
-      $rules = mpull($rules, null, 'getID');
-      $rule = idx($rules, $effect->getRuleID());
-      if ($rule && strlen($rule->getName())) {
-        $rule_name = $rule->getName();
-      } else {
-        $rule_name = pht('Unnamed Herald Rule');
-      }
-
       $blocked_ref_name = coalesce(
         $blocked_update->getRefName(),
         $blocked_update->getRefNewShort());
@@ -364,9 +358,9 @@ final class DiffusionCommitHookEngine extends Phobject {
           "Change: %s\n".
           "  Rule: %s\n".
           "Reason: %s",
-          'H'.$blocking_effect->getRuleID(),
+          $rule->getMonogram(),
           $blocked_name,
-          $rule_name,
+          $rule->getName(),
           $message));
     }
   }

src/applications/herald/adapter/HeraldAdapter.php

@@ -999,11 +999,8 @@ abstract class HeraldAdapter {
   public static function applyFlagEffect(HeraldEffect $effect, $phid) {
     $color = $effect->getTarget();
 
-    // TODO: Silly that we need to load this again here.
+    $rule = $effect->getRule();
-    $rule = id(new HeraldRule())->load($effect->getRuleID());
+    $user = $rule->getAuthor();
-    $user = id(new PhabricatorUser())->loadOneWhere(
-      'phid = %s',
-      $rule->getAuthorPHID());
 
     $flag = PhabricatorFlagQuery::loadUserFlag($user, $phid);
     if ($flag) {

src/applications/herald/adapter/HeraldCommitAdapter.php

@@ -501,7 +501,7 @@ final class HeraldCommitAdapter extends HeraldAdapter {
             if (empty($this->addCCPHIDs[$phid])) {
               $this->addCCPHIDs[$phid] = array();
             }
-            $this->addCCPHIDs[$phid][] = $effect->getRuleID();
+            $this->addCCPHIDs[$phid][] = $effect->getRule()->getID();
           }
           $result[] = new HeraldApplyTranscript(
             $effect,
@@ -513,7 +513,7 @@ final class HeraldCommitAdapter extends HeraldAdapter {
             if (empty($this->auditMap[$phid])) {
               $this->auditMap[$phid] = array();
             }
-            $this->auditMap[$phid][] = $effect->getRuleID();
+            $this->auditMap[$phid][] = $effect->getRule()->getID();
           }
           $result[] = new HeraldApplyTranscript(
             $effect,

src/applications/herald/engine/HeraldEffect.php

@@ -5,10 +5,7 @@ final class HeraldEffect {
   private $objectPHID;
   private $action;
   private $target;
-
+  private $rule;
-  private $ruleID;
-  private $rulePHID;
-
   private $reason;
 
   public function setObjectPHID($object_phid) {
@@ -38,22 +35,13 @@ final class HeraldEffect {
     return $this->target;
   }
 
-  public function setRuleID($rule_id) {
+  public function setRule(HeraldRule $rule) {
-    $this->ruleID = $rule_id;
+    $this->rule = $rule;
     return $this;
   }
 
-  public function getRuleID() {
+  public function getRule() {
-    return $this->ruleID;
+    return $this->rule;
-  }
-
-  public function setRulePHID($rule_phid) {
-    $this->rulePHID = $rule_phid;
-    return $this;
-  }
-
-  public function getRulePHID() {
-    return $this->rulePHID;
   }
 
   public function setReason($reason) {

src/applications/herald/engine/HeraldEngine.php

@@ -368,16 +368,14 @@ final class HeraldEngine {
 
     $effects = array();
     foreach ($rule->getActions() as $action) {
-      $effect = new HeraldEffect();
+      $effect = id(new HeraldEffect())
-      $effect->setObjectPHID($object->getPHID());
+        ->setObjectPHID($object->getPHID())
-      $effect->setAction($action->getAction());
+        ->setAction($action->getAction())
-      $effect->setTarget($action->getTarget());
+        ->setTarget($action->getTarget())
-
+        ->setRule($rule);
-      $effect->setRuleID($rule->getID());
-      $effect->setRulePHID($rule->getPHID());
 
       $name = $rule->getName();
-      $id   = $rule->getID();
+      $id = $rule->getID();
       $effect->setReason(
         pht(
           'Conditions were met for %s',

src/applications/herald/storage/HeraldRule.php

@@ -265,6 +265,10 @@ final class HeraldRule extends HeraldDAO
     return sprintf('~%d%010d', $type_order, $this->getID());
   }
 
+  public function getMonogram() {
+    return 'H'.$this->getID();
+  }
+
 
 /* -(  PhabricatorApplicationTransactionInterface  )------------------------- */
 

src/applications/herald/storage/transcript/HeraldApplyTranscript.php

@@ -16,7 +16,7 @@ final class HeraldApplyTranscript extends Phobject {
 
     $this->setAction($effect->getAction());
     $this->setTarget($effect->getTarget());
-    $this->setRuleID($effect->getRuleID());
+    $this->setRuleID($effect->getRule()->getID());
     $this->setReason($effect->getReason());
     $this->setApplied($applied);
     $this->setAppliedReason($reason);