Herald - make herald condition of herald rule display better

Summary: ...by the surprising step of changing how this data is stored from id to phid. Also a small fix to not allow "disabled" rules to be used as herald rule conditions, i.e. can't make a rule that depends on a disabled rule.

Test Plan: viewed existing herald rule that had a rule condition and noted nice new display using handle. made a new rule that had a rule condition and verified it worked correctly.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

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

resources/sql/autopatches/20140210.herald.rule-condition-mig.php

@@ -0,0 +1,31 @@
+<?php
+
+$table = new HeraldCondition();
+$conn_w = $table->establishConnection('w');
+
+echo "Migrating Herald conditions of type Herald rule from IDs to PHIDs...\n";
+foreach (new LiskMigrationIterator($table) as $condition) {
+  if ($condition->getFieldName() != HeraldAdapter::FIELD_RULE) {
+    continue;
+  }
+
+  $value = $condition->getValue();
+  if (!is_numeric($value)) {
+    continue;
+  }
+  $id = $condition->getID();
+  echo "Updating condition {$id}...\n";
+
+  $rule = id(new HeraldRuleQuery())
+    ->setViewer(PhabricatorUser::getOmnipotentUser())
+    ->withIDs(array($value))
+    ->executeOne();
+
+  queryfx(
+    $conn_w,
+    'UPDATE %T SET value = %s WHERE id = %d',
+    $table->getTableName(),
+    json_encode($rule->getPHID()),
+    $id);
+}
+echo "Done.\n";

src/applications/diffusion/herald/HeraldPreCommitContentAdapter.php

@@ -44,7 +44,6 @@ final class HeraldPreCommitContentAdapter extends HeraldPreCommitAdapter {
         self::FIELD_DIFFERENTIAL_REVIEWERS,
         self::FIELD_DIFFERENTIAL_CCS,
         self::FIELD_IS_MERGE_COMMIT,
-        self::FIELD_RULE,
       ),
       parent::getFields());
   }

src/applications/diffusion/herald/HeraldPreCommitRefAdapter.php

@@ -41,7 +41,6 @@ final class HeraldPreCommitRefAdapter extends HeraldPreCommitAdapter {
         self::FIELD_REPOSITORY_PROJECTS,
         self::FIELD_PUSHER,
         self::FIELD_PUSHER_PROJECTS,
-        self::FIELD_RULE,
       ),
       parent::getFields());
   }

src/applications/herald/controller/HeraldRuleController.php

@@ -414,7 +414,7 @@ final class HeraldRuleController extends HeraldController {
     }
 
     $all_rules = $this->loadRulesThisRuleMayDependUpon($rule);
-    $all_rules = mpull($all_rules, 'getName', 'getID');
+    $all_rules = mpull($all_rules, 'getName', 'getPHID');
     asort($all_rules);
 
     $all_fields = $adapter->getFieldNameMap();
@@ -633,6 +633,15 @@ final class HeraldRuleController extends HeraldController {
         ->execute();
     }
 
+    // mark disabled rules as disabled since they are not useful as such;
+    // don't filter though to keep edit cases sane / expected
+    foreach ($all_rules as $current_rule) {
+      if ($current_rule->getIsDisabled()) {
+        $current_rule->makeEphemeral();
+        $current_rule->setName($rule->getName(). ' '.pht('(Disabled)'));
+      }
+    }
+
     // A rule can not depend upon itself.
     unset($all_rules[$rule->getID()]);
 

src/applications/herald/engine/HeraldEngine.php

@@ -20,8 +20,8 @@ final class HeraldEngine {
     return $this->dryRun;
   }
 
-  public function getRule($id) {
+  public function getRule($phid) {
-    return idx($this->rules, $id);
+    return idx($this->rules, $phid);
   }
 
   public function loadRulesForAdapter(HeraldAdapter $adapter) {
@@ -49,7 +49,7 @@ final class HeraldEngine {
     assert_instances_of($rules, 'HeraldRule');
     $t_start = microtime(true);
 
-    $rules = mpull($rules, null, 'getID');
+    $rules = mpull($rules, null, 'getPHID');
 
     $this->transcript = new HeraldTranscript();
     $this->transcript->setObjectPHID((string)$object->getPHID());
@@ -59,7 +59,7 @@ final class HeraldEngine {
     $this->object  = $object;
 
     $effects = array();
-    foreach ($rules as $id => $rule) {
+    foreach ($rules as $phid => $rule) {
       $this->stack = array();
       try {
         if (!$this->getDryRun() &&
@@ -70,7 +70,7 @@ final class HeraldEngine {
           // applied a single time, and it's already been applied...
           // That means automatic failure.
           $xscript = id(new HeraldRuleTranscript())
-            ->setRuleID($id)
+            ->setRuleID($rule->getID())
             ->setResult(false)
             ->setRuleName($rule->getName())
             ->setRuleOwner($rule->getAuthorPHID())
@@ -102,7 +102,7 @@ final class HeraldEngine {
         }
         $rule_matches = false;
       }
-      $this->results[$id] = $rule_matches;
+      $this->results[$phid] = $rule_matches;
 
       if ($rule_matches) {
         foreach ($this->getRuleEffects($rule, $object) as $effect) {
@@ -210,25 +210,25 @@ final class HeraldEngine {
     HeraldRule $rule,
     HeraldAdapter $object) {
 
-    $id = $rule->getID();
+    $phid = $rule->getPHID();
 
-    if (isset($this->results[$id])) {
+    if (isset($this->results[$phid])) {
       // If we've already evaluated this rule because another rule depends
       // on it, we don't need to reevaluate it.
-      return $this->results[$id];
+      return $this->results[$phid];
     }
 
-    if (isset($this->stack[$id])) {
+    if (isset($this->stack[$phid])) {
       // We've recursed, fail all of the rules on the stack. This happens when
       // there's a dependency cycle with "Rule conditions match for rule ..."
       // conditions.
-      foreach ($this->stack as $rule_id => $ignored) {
+      foreach ($this->stack as $rule_phid => $ignored) {
-        $this->results[$rule_id] = false;
+        $this->results[$rule_phid] = false;
       }
       throw new HeraldRecursiveConditionsException();
     }
 
-    $this->stack[$id] = true;
+    $this->stack[$phid] = true;
 
     $all = $rule->getMustMatchAll();