Page MenuHomePhabricator
Files F15412298

D18930.id.diff
No OneTemporary
Actions

D18930.id.diff
View Options

diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php
--- a/src/applications/herald/adapter/HeraldAdapter.php
+++ b/src/applications/herald/adapter/HeraldAdapter.php
@@ -774,6 +774,7 @@
if (!$this->isSingleEventAdapter()) {
$options[] = HeraldRule::REPEAT_FIRST;
+ $options[] = HeraldRule::REPEAT_CHANGE;
}
return $options;
@@ -897,12 +898,15 @@
));
}
- 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
--- a/src/applications/herald/controller/HeraldRuleController.php
+++ b/src/applications/herald/controller/HeraldRuleController.php
@@ -218,7 +218,7 @@
),
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
--- a/src/applications/herald/engine/HeraldEngine.php
+++ b/src/applications/herald/engine/HeraldEngine.php
@@ -14,6 +14,7 @@
private $forbiddenFields = array();
private $forbiddenActions = array();
+ private $skipEffects = array();
public function setDryRun($dry_run) {
$this->dryRun = $dry_run;
@@ -171,13 +172,31 @@
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 @@
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 @@
}
}
+ // 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 @@
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
--- a/src/applications/herald/storage/HeraldRule.php
+++ b/src/applications/herald/storage/HeraldRule.php
@@ -32,6 +32,7 @@
const REPEAT_EVERY = 'every';
const REPEAT_FIRST = 'first';
+ const REPEAT_CHANGE = 'change';
protected function getConfiguration() {
return array(
@@ -282,6 +283,10 @@
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 @@
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:'),
),
);
}

File Metadata

Mime Type
text/plain
Expires
Thu, Mar 20, 12:18 PM (2 d, 9 h ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7707822
Default Alt Text
D18930.id.diff (7 KB)

Event Timeline

Phabricator · Built by Phacility