diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php index bafad0d440..d67c69f584 100644 --- a/src/applications/herald/adapter/HeraldAdapter.php +++ b/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( diff --git a/src/applications/herald/controller/HeraldRuleController.php b/src/applications/herald/controller/HeraldRuleController.php index 906afe835e..c61c29e90e 100644 --- a/src/applications/herald/controller/HeraldRuleController.php +++ b/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', diff --git a/src/applications/herald/engine/HeraldEngine.php b/src/applications/herald/engine/HeraldEngine.php index afeba459ff..739e83e4e8 100644 --- a/src/applications/herald/engine/HeraldEngine.php +++ b/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,26 +209,44 @@ 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'); - $sql = array(); - foreach ($applied_ids as $id) { - $sql[] = qsprintf( + + if ($delete_ids) { + queryfx( $conn_w, - '(%s, %d)', + 'DELETE FROM %T WHERE phid = %s AND ruleID IN (%Ld)', + HeraldRule::TABLE_RULE_APPLIED, $adapter->getPHID(), - $id); + $delete_ids); + } + + if ($applied_ids) { + $sql = array(); + foreach ($applied_ids as $id) { + $sql[] = qsprintf( + $conn_w, + '(%s, %d)', + $adapter->getPHID(), + $id); + } + queryfx( + $conn_w, + 'INSERT IGNORE INTO %T (phid, ruleID) VALUES %Q', + HeraldRule::TABLE_RULE_APPLIED, + implode(', ', $sql)); } - queryfx( - $conn_w, - 'INSERT IGNORE INTO %T (phid, ruleID) VALUES %Q', - HeraldRule::TABLE_RULE_APPLIED, - implode(', ', $sql)); } } @@ -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()) diff --git a/src/applications/herald/storage/HeraldRule.php b/src/applications/herald/storage/HeraldRule.php index 8fd1127eb7..44ef68ac03 100644 --- a/src/applications/herald/storage/HeraldRule.php +++ b/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:'), ), ); }