diff --git a/resources/sql/patches/041.heraldrepetition.sql b/resources/sql/patches/041.heraldrepetition.sql new file mode 100644 index 0000000000..ee97fd1d3a --- /dev/null +++ b/resources/sql/patches/041.heraldrepetition.sql @@ -0,0 +1,7 @@ +CREATE TABLE phabricator_herald.herald_ruleapplied ( + ruleID int unsigned not null, + phid varchar(64) binary not null, + PRIMARY KEY(ruleID, phid) +) ENGINE=InnoDB; + +ALTER TABLE phabricator_herald.herald_rule add repetitionPolicy int unsigned; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 0b2c60a1e1..cff72691d4 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -237,6 +237,7 @@ phutil_register_library_map(array( 'HeraldObjectAdapter' => 'applications/herald/adapter/base', 'HeraldObjectTranscript' => 'applications/herald/storage/transcript/object', 'HeraldRecursiveConditionsException' => 'applications/herald/engine/engine/exception', + 'HeraldRepetitionPolicyConfig' => 'applications/herald/config/repetitionpolicy', 'HeraldRule' => 'applications/herald/storage/rule', 'HeraldRuleController' => 'applications/herald/controller/rule', 'HeraldRuleTranscript' => 'applications/herald/storage/transcript/rule', diff --git a/src/applications/differential/editor/revision/DifferentialRevisionEditor.php b/src/applications/differential/editor/revision/DifferentialRevisionEditor.php index c97bed4404..d095e4dbcf 100644 --- a/src/applications/differential/editor/revision/DifferentialRevisionEditor.php +++ b/src/applications/differential/editor/revision/DifferentialRevisionEditor.php @@ -289,13 +289,19 @@ class DifferentialRevisionEditor { array_keys($add['ccs']), $this->actorPHID); - // Add the author to the relevant set of users so they get a copy of the - // email. + // Add the author and users included from Herald rules to the relevant set + // of users so they get a copy of the email. if (!$this->silentUpdate) { if ($is_new) { $add['rev'][$this->getActorPHID()] = true; + if ($diff) { + $add['rev'] += $adapter->getEmailPHIDsAddedByHerald(); + } } else { $stable['rev'][$this->getActorPHID()] = true; + if ($diff) { + $stable['rev'] += $adapter->getEmailPHIDsAddedByHerald(); + } } } @@ -356,13 +362,6 @@ class DifferentialRevisionEditor { // TODO // $revision->saveTransaction(); - $event = array( - 'revision_id' => $revision->getID(), - 'PHID' => $revision->getPHID(), - 'action' => $is_new ? 'create' : 'update', - 'actor' => $this->getActorPHID(), - ); - // TODO: Move this into a worker task thing. PhabricatorSearchDifferentialIndexer::indexRevision($revision); diff --git a/src/applications/herald/adapter/differential/HeraldDifferentialRevisionAdapter.php b/src/applications/herald/adapter/differential/HeraldDifferentialRevisionAdapter.php index b4025a656f..11668483f6 100644 --- a/src/applications/herald/adapter/differential/HeraldDifferentialRevisionAdapter.php +++ b/src/applications/herald/adapter/differential/HeraldDifferentialRevisionAdapter.php @@ -27,6 +27,7 @@ class HeraldDifferentialRevisionAdapter extends HeraldObjectAdapter { protected $newCCs = array(); protected $remCCs = array(); + protected $emailPHIDs = array(); protected $repository; protected $affectedPackages; @@ -64,6 +65,10 @@ class HeraldDifferentialRevisionAdapter extends HeraldObjectAdapter { return $this->remCCs; } + public function getEmailPHIDsAddedByHerald() { + return $this->emailPHIDs; + } + public function getPHID() { return $this->revision->getPHID(); } @@ -247,7 +252,9 @@ class HeraldDifferentialRevisionAdapter extends HeraldObjectAdapter { true, 'OK, did nothing.'); break; + case HeraldActionConfig::ACTION_EMAIL: case HeraldActionConfig::ACTION_ADD_CC: + $op = ($action == HeraldActionConfig::ACTION_EMAIL) ? 'email' : 'CC'; $base_target = $effect->getTarget(); $forbidden = array(); foreach ($base_target as $key => $fbid) { @@ -255,7 +262,11 @@ class HeraldDifferentialRevisionAdapter extends HeraldObjectAdapter { $forbidden[] = $fbid; unset($base_target[$key]); } else { - $this->newCCs[$fbid] = true; + if ($action == HeraldActionConfig::ACTION_EMAIL) { + $this->emailPHIDs[$fbid] = true; + } else { + $this->newCCs[$fbid] = true; + } } } @@ -267,17 +278,18 @@ class HeraldDifferentialRevisionAdapter extends HeraldObjectAdapter { $result[] = new HeraldApplyTranscript( $effect, true, - 'Added these addresses to CC list. Others could not be added.'); + 'Added these addresses to '.$op.' list. '. + 'Others could not be added.'); } $result[] = new HeraldApplyTranscript( $failed, false, - 'CC forbidden, these addresses have unsubscribed.'); + $op.' forbidden, these addresses have unsubscribed.'); } else { $result[] = new HeraldApplyTranscript( $effect, true, - 'Added addresses to CC list.'); + 'Added addresses to '.$op.' list.'); } break; case HeraldActionConfig::ACTION_REMOVE_CC: diff --git a/src/applications/herald/config/action/HeraldActionConfig.php b/src/applications/herald/config/action/HeraldActionConfig.php index 6f806822cc..3974c6f59c 100644 --- a/src/applications/herald/config/action/HeraldActionConfig.php +++ b/src/applications/herald/config/action/HeraldActionConfig.php @@ -41,6 +41,7 @@ class HeraldActionConfig { array( self::ACTION_ADD_CC, self::ACTION_REMOVE_CC, + self::ACTION_EMAIL, self::ACTION_NOTHING, )); case HeraldContentTypeConfig::CONTENT_TYPE_COMMIT: diff --git a/src/applications/herald/config/repetitionpolicy/HeraldRepetitionPolicyConfig.php b/src/applications/herald/config/repetitionpolicy/HeraldRepetitionPolicyConfig.php new file mode 100644 index 0000000000..1c37c906a7 --- /dev/null +++ b/src/applications/herald/config/repetitionpolicy/HeraldRepetitionPolicyConfig.php @@ -0,0 +1,64 @@ + 0, + self::EVERY => 1, + ); + + private static $policyMap = array( + self::FIRST => 'only the first time', + self::EVERY => 'every time', + ); + + public static function getMap() { + return self::$policyMap; + } + + public static function getMapForContentType($type) { + switch ($type) { + case HeraldContentTypeConfig::CONTENT_TYPE_DIFFERENTIAL: + return array_select_keys( + self::$policyMap, + array( + self::EVERY, + self::FIRST, + )); + + case HeraldContentTypeConfig::CONTENT_TYPE_COMMIT: + case HeraldContentTypeConfig::CONTENT_TYPE_MERGE: + case HeraldContentTypeConfig::CONTENT_TYPE_OWNERS: + return array(); + + default: + throw new Exception("Unknown content type '{$type}'."); + } + } + + public static function toInt($str) { + return idx(self::$policyIntMap, $str, self::$policyIntMap[self::EVERY]); + } + + public static function toString($int) { + return idx(array_flip(self::$policyIntMap), $int, self::EVERY); + } +} diff --git a/src/applications/herald/config/repetitionpolicy/__init__.php b/src/applications/herald/config/repetitionpolicy/__init__.php new file mode 100644 index 0000000000..9f18e371d3 --- /dev/null +++ b/src/applications/herald/config/repetitionpolicy/__init__.php @@ -0,0 +1,14 @@ +setName($request->getStr('name')); $rule->setMustMatchAll(($request->getStr('must_match') == 'all')); + $repetition_policy_param = $request->getStr('repetition_policy'); + $rule->setRepetitionPolicy( + HeraldRepetitionPolicyConfig::toInt($repetition_policy_param) + ); + if (!strlen($rule->getName())) { $e_name = "Required"; $errors[] = "Rule must have a name."; @@ -250,6 +255,45 @@ class HeraldRuleController extends HeraldController { $action = '/herald/rule/'.$rule->getID().'/'; } + // Make the selector for choosing how often this rule should be repeated + $repetition_selector = ""; + $repetition_policy = HeraldRepetitionPolicyConfig::toString( + $rule->getRepetitionPolicy() + ); + $repetition_options = HeraldRepetitionPolicyConfig::getMapForContentType( + $rule->getContentType() + ); + + if (empty($repetition_options)) { + // default option is 'every time' + $repetition_selector = idx( + HeraldRepetitionPolicyConfig::getMap(), + HeraldRepetitionPolicyConfig::EVERY + ); + } else if (count($repetition_options) == 1) { + // if there's only 1 option, just pick it for the user + $repetition_selector = reset($repetition_options); + } else { + // give the user all the options for this rule type + $tags = array(); + + foreach ($repetition_options as $name => $option) { + $tags[] = phutil_render_tag( + 'option', + array ( + 'selected' => ($repetition_policy == $name) ? 'selected' : null, + 'value' => $name, + ), + phutil_escape_html($option) + ); + } + + $repetition_selector = + ''; + } + require_celerity_resource('herald-css'); $type_name = $content_type_map[$rule->getContentType()]; @@ -320,7 +364,9 @@ class HeraldRuleController extends HeraldController { ), 'Create New Action'). ''. - '

Take these actions:

'. + '

'. + 'Take these actions '.$repetition_selector.' this rule matches:'. + '

'. '
'. javelin_render_tag( 'table', diff --git a/src/applications/herald/controller/rule/__init__.php b/src/applications/herald/controller/rule/__init__.php index 6e4663485b..4ee071a424 100644 --- a/src/applications/herald/controller/rule/__init__.php +++ b/src/applications/herald/controller/rule/__init__.php @@ -12,6 +12,7 @@ phutil_require_module('phabricator', 'applications/herald/config/action'); phutil_require_module('phabricator', 'applications/herald/config/condition'); phutil_require_module('phabricator', 'applications/herald/config/contenttype'); phutil_require_module('phabricator', 'applications/herald/config/field'); +phutil_require_module('phabricator', 'applications/herald/config/repetitionpolicy'); phutil_require_module('phabricator', 'applications/herald/config/valuetype'); phutil_require_module('phabricator', 'applications/herald/controller/base'); phutil_require_module('phabricator', 'applications/herald/storage/action'); diff --git a/src/applications/herald/engine/engine/HeraldEngine.php b/src/applications/herald/engine/engine/HeraldEngine.php index 5cc8f8338c..14339f0e32 100644 --- a/src/applications/herald/engine/engine/HeraldEngine.php +++ b/src/applications/herald/engine/engine/HeraldEngine.php @@ -51,11 +51,27 @@ class HeraldEngine { $effects = array(); foreach ($rules as $id => $rule) { - - $this->stack = array(); try { - $rule_matches = $this->doesRuleMatch($rule, $object); + if (($rule->getRepetitionPolicy() == + HeraldRepetitionPolicyConfig::FIRST) && + $rule->getRuleApplied($object->getPHID())) { + // This rule is only supposed to be applied a single time, and it's + // aleady been applied, so this is an automatic failure. + $xscript = id(new HeraldRuleTranscript()) + ->setRuleID($id) + ->setResult(false) + ->setRuleName($rule->getName()) + ->setRuleOwner($rule->getAuthorPHID()) + ->setReason( + "This rule is only supposed to be repeated a single time, ". + "and it has already been applied." + ); + $this->transcript->addRuleTranscript($xscript); + $rule_matches = false; + } else { + $rule_matches = $this->doesRuleMatch($rule, $object); + } } catch (HeraldRecursiveConditionsException $ex) { $names = array(); foreach ($this->stack as $rule_id => $ignored) { @@ -101,12 +117,10 @@ class HeraldEngine { } public function applyEffects(array $effects, HeraldObjectAdapter $object) { - if ($object instanceof DryRunHeraldable) { - $this->transcript->setDryRun(true); - } else { - $this->transcript->setDryRun(false); - } - foreach ($object->applyHeraldEffects($effects) as $apply_xscript) { + $this->transcript->setDryRun($object instanceof HeraldDryRunAdapter); + + $xscripts = $object->applyHeraldEffects($effects); + foreach ($xscripts as $apply_xscript) { if (!($apply_xscript instanceof HeraldApplyTranscript)) { throw new Exception( "Heraldable must return HeraldApplyTranscripts from ". @@ -114,6 +128,17 @@ class HeraldEngine { } $this->transcript->addApplyTranscript($apply_xscript); } + + if (!$this->transcript->getDryRun()) { + // Mark all the rules that have had their effects applied as having been + // executed for the current object. + $ruleIDs = mpull($xscripts, 'getRuleID'); + foreach ($ruleIDs as $ruleID) { + id(new HeraldRule()) + ->setID($ruleID) + ->saveRuleApplied($object->getPHID()); + } + } } public function getTranscript() { diff --git a/src/applications/herald/engine/engine/__init__.php b/src/applications/herald/engine/engine/__init__.php index 5b7eab228c..3898e3bb64 100644 --- a/src/applications/herald/engine/engine/__init__.php +++ b/src/applications/herald/engine/engine/__init__.php @@ -8,6 +8,7 @@ phutil_require_module('phabricator', 'applications/herald/config/condition'); phutil_require_module('phabricator', 'applications/herald/config/field'); +phutil_require_module('phabricator', 'applications/herald/config/repetitionpolicy'); phutil_require_module('phabricator', 'applications/herald/engine/effect'); phutil_require_module('phabricator', 'applications/herald/engine/engine/exception'); phutil_require_module('phabricator', 'applications/herald/storage/rule'); diff --git a/src/applications/herald/storage/rule/HeraldRule.php b/src/applications/herald/storage/rule/HeraldRule.php index 76b11a8aa3..fd43ec9bbe 100644 --- a/src/applications/herald/storage/rule/HeraldRule.php +++ b/src/applications/herald/storage/rule/HeraldRule.php @@ -18,13 +18,18 @@ class HeraldRule extends HeraldDAO { + const TABLE_RULE_APPLIED = 'herald_ruleapplied'; + protected $name; protected $authorPHID; protected $contentType; protected $mustMatchAll; + protected $repetitionPolicy; - protected $configVersion = 7; + protected $configVersion = 8; + + private $ruleApplied = array(); // phids for which this rule has been applied public static function loadAllByContentTypeWithFullData($content_type) { $rules = id(new HeraldRule())->loadAllWhere( @@ -45,10 +50,19 @@ class HeraldRule extends HeraldDAO { 'ruleID in (%Ld)', $rule_ids); + $applied = queryfx_all( + id(new HeraldRule())->establishConnection('r'), + 'SELECT * FROM %T WHERE ruleID in (%Ld)', + self::TABLE_RULE_APPLIED, $rule_ids + ); + $conditions = mgroup($conditions, 'getRuleID'); $actions = mgroup($actions, 'getRuleID'); + $applied = igroup($applied, 'ruleID'); + foreach ($rules as $rule) { + $rule->attachAllRuleApplied(idx($applied, $rule->getID(), array())); $rule->attachConditions(idx($conditions, $rule->getID(), array())); $rule->attachActions(idx($actions, $rule->getID(), array())); } @@ -56,6 +70,35 @@ class HeraldRule extends HeraldDAO { return $rules; } + public function getRuleApplied($phid) { + // defaults to false because (ruleID, phid) pairs not in the db imply + // a rule that's not been applied before + return idx($this->ruleApplied, $phid, false); + } + + public function setRuleApplied($phid) { + $this->ruleApplied[$phid] = true; + } + + public function attachAllRuleApplied(array $applied) { + // turn array of array(ruleID, phid) into array of ruleID => true + $this->ruleApplied = array_fill_keys(ipull($applied, 'phid'), true); + } + + public function saveRuleApplied($phid) { + if (!$this->getID()) { + throw new Exception("Save rule before saving children."); + } + + queryfx( + $this->establishConnection('w'), + 'INSERT IGNORE INTO %T (phid, ruleID) VALUES (%s, %d)', + self::TABLE_RULE_APPLIED, $phid, $this->getID() + ); + + $this->setRuleApplied($phid); + } + public function loadConditions() { if (!$this->getID()) { return array();