Implement an "only if the rule did not match last time" policy for Herald rules

Summary: Depends on D18927. Ref T13048. This implements a new policy which allows Herald rules to fire on some kinds of state changes.

Test Plan:
Wrote and tested rules with the new policy:

{F5394971}

{F5394972}

Also wrote and tested rules with the old policies:

{F5394973}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13048

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

src/applications/herald/adapter/HeraldAdapter.php

@@ -774,6 +774,7 @@ abstract class HeraldAdapter extends Phobject {
 
     if (!$this->isSingleEventAdapter()) {
       $options[] = HeraldRule::REPEAT_FIRST;
+      $options[] = HeraldRule::REPEAT_CHANGE;
     }
 
     return $options;
@@ -897,12 +898,15 @@ abstract class HeraldAdapter extends Phobject {
         ));
     }
 
-    if ($rule->isRepeatEvery()) {
-      $action_text =
-        pht('Take these actions every time this rule matches:');
+    if ($rule->isRepeatFirst()) {
+      $action_text = pht(
+        'Take these actions the first time this rule matches:');
+    } else if ($rule->isRepeatOnChange()) {
+      $action_text = pht(
+        'Take these actions if this rule did not match the last time:');
     } else {
-      $action_text =
-        pht('Take these actions the first time this rule matches:');
+      $action_text = pht(
+        'Take these actions every time this rule matches:');
     }
 
     $action_title = phutil_tag(

src/applications/herald/controller/HeraldRuleController.php

@@ -218,7 +218,7 @@ final class HeraldRuleController extends HeraldController {
             ),
             pht('New Action')))
           ->setDescription(pht(
-            'Take these actions %s this rule matches:',
+            'Take these actions %s',
             $repetition_selector))
           ->setContent(javelin_tag(
               'table',

src/applications/herald/engine/HeraldEngine.php

@@ -14,6 +14,7 @@ final class HeraldEngine extends Phobject {
 
   private $forbiddenFields = array();
   private $forbiddenActions = array();
+  private $skipEffects = array();
 
   public function setDryRun($dry_run) {
     $this->dryRun = $dry_run;
@@ -171,13 +172,31 @@ final class HeraldEngine extends Phobject {
       return;
     }
 
-    $rules = mpull($rules, null, 'getID');
-    $applied_ids = array();
+    // Update the "applied" state table. How this table works depends on the
+    // repetition policy for the rule.
+    //
+    // REPEAT_EVERY: We delete existing rows for the rule, then write nothing.
+    // This policy doesn't use any state.
+    //
+    // REPEAT_FIRST: We keep existing rows, then write additional rows for
+    // rules which fired. This policy accumulates state over the life of the
+    // object.
+    //
+    // REPEAT_CHANGE: We delete existing rows, then write all the rows which
+    // matched. This policy only uses the state from the previous run.
 
-    // Mark all the rules that have had their effects applied as having been
-    // executed for the current object.
+    $rules = mpull($rules, null, 'getID');
     $rule_ids = mpull($xscripts, 'getRuleID');
 
+    $delete_ids = array();
+    foreach ($rules as $rule_id => $rule) {
+      if ($rule->isRepeatFirst()) {
+        continue;
+      }
+      $delete_ids[] = $rule_id;
+    }
+
+    $applied_ids = array();
     foreach ($rule_ids as $rule_id) {
       if (!$rule_id) {
         // Some apply transcripts are purely informational and not associated
@@ -190,13 +209,30 @@ final class HeraldEngine extends Phobject {
         continue;
       }
 
-      if ($rule->isRepeatFirst()) {
+      if ($rule->isRepeatFirst() || $rule->isRepeatOnChange()) {
         $applied_ids[] = $rule_id;
       }
     }
 
-    if ($applied_ids) {
+    // Also include "only if this rule did not match the last time" rules
+    // which matched but were skipped in the "applied" list.
+    foreach ($this->skipEffects as $rule_id => $ignored) {
+      $applied_ids[] = $rule_id;
+    }
+
+    if ($delete_ids || $applied_ids) {
       $conn_w = id(new HeraldRule())->establishConnection('w');
+
+      if ($delete_ids) {
+        queryfx(
+          $conn_w,
+          'DELETE FROM %T WHERE phid = %s AND ruleID IN (%Ld)',
+          HeraldRule::TABLE_RULE_APPLIED,
+          $adapter->getPHID(),
+          $delete_ids);
+      }
+
+      if ($applied_ids) {
         $sql = array();
         foreach ($applied_ids as $id) {
           $sql[] = qsprintf(
@@ -212,6 +248,7 @@ final class HeraldEngine extends Phobject {
           implode(', ', $sql));
       }
     }
+  }
 
   public function getTranscript() {
     $this->transcript->save();
@@ -311,6 +348,30 @@ final class HeraldEngine extends Phobject {
       }
     }
 
+    // If this rule matched, and is set to run "if it did not match the last
+    // time", and we matched the last time, we're going to return a match in
+    // the transcript but set a flag so we don't actually apply any effects.
+
+    // We need the rule to match so that storage gets updated properly. If we
+    // just pretend the rule didn't match it won't cause any effects (which
+    // is correct), but it also won't set the "it matched" flag in storage,
+    // so the next run after this one would incorrectly trigger again.
+
+    $is_dry_run = $this->getDryRun();
+    if ($result && !$is_dry_run) {
+      $is_on_change = $rule->isRepeatOnChange();
+      if ($is_on_change) {
+        $did_apply = $rule->getRuleApplied($object->getPHID());
+        if ($did_apply) {
+          $reason = pht(
+            'This rule matched, but did not take any actions because it '.
+            'is configured to act only if it did not match the last time.');
+
+          $this->skipEffects[$rule->getID()] = true;
+        }
+      }
+    }
+
     $this->newRuleTranscript($rule)
       ->setResult($result)
       ->setReason($reason);
@@ -363,6 +424,11 @@ final class HeraldEngine extends Phobject {
     HeraldRule $rule,
     HeraldAdapter $object) {
 
+    $rule_id = $rule->getID();
+    if (isset($this->skipEffects[$rule_id])) {
+      return array();
+    }
+
     $effects = array();
     foreach ($rule->getActions() as $action) {
       $effect = id(new HeraldEffect())

src/applications/herald/storage/HeraldRule.php

@@ -32,6 +32,7 @@ final class HeraldRule extends HeraldDAO
 
   const REPEAT_EVERY = 'every';
   const REPEAT_FIRST = 'first';
+  const REPEAT_CHANGE = 'change';
 
   protected function getConfiguration() {
     return array(
@@ -282,6 +283,10 @@ final class HeraldRule extends HeraldDAO
     return ($this->getRepetitionPolicyStringConstant() === self::REPEAT_FIRST);
   }
 
+  public function isRepeatOnChange() {
+    return ($this->getRepetitionPolicyStringConstant() === self::REPEAT_CHANGE);
+  }
+
   public static function getRepetitionPolicySelectOptionMap() {
     $map = self::getRepetitionPolicyMap();
     return ipull($map, 'select');
@@ -290,10 +295,13 @@ final class HeraldRule extends HeraldDAO
   private static function getRepetitionPolicyMap() {
     return array(
       self::REPEAT_EVERY => array(
-        'select' => pht('every time'),
+        'select' => pht('every time this rule matches:'),
       ),
       self::REPEAT_FIRST => array(
-        'select' => pht('only the first time'),
+        'select' => pht('only the first time this rule matches:'),
+      ),
+      self::REPEAT_CHANGE => array(
+        'select' => pht('if this rule did not match the last time:'),
       ),
     );
   }