mirror of
https://we.phorge.it/source/phorge.git
synced 2024-12-23 14:00:56 +01:00
Remove "HeraldRepetitionPolicyConfig" and hide storage details inside HeraldRule
Summary: Depends on D18925. Ref T13048. Currently, HeraldRule stores policies as integers (0 or 1) in the database. The application tries to mostly use strings ("first", "every"), but doesn't do a good job of hiding the fact that the values are integers deeper in the stack. So we end up with a lot of code like this: ```lang=php $stored_int_value = $rule->getRepetitionPolicy(); $equivalent_string = HeraldRepetitionPolicyConfig::getString($stored_int_value); $is_first = ($equivalent_string === HeraldRepetitionPolicyConfig::FIRST); ``` This happens in several places and is generally awful. Replace it with: ```lang=php $is_first = $rule->isRepeatFirst(); ``` To do this, merge `HeraldRepetitionPolicyConfig` into `HeraldRule` and hide all the mess inside the methods. (This may let us just get rid of the integers in a later change, although I'm not sure I want to commit to that.) Test Plan: - Grepped for `HeraldRepetitionPolicyConfig`, no more hits. - Grepped for `setRepetitionPolicy(...)` and `getRepetitionPolicy(...)`. There are no remaining callers outside of `HeraldRule`. - Browed and edited several rules. I'll vet this more convincingly after adding the new repetition rule. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13048 Differential Revision: https://secure.phabricator.com/D18926
This commit is contained in:
parent
a90b16e83a
commit
1dd1237c92
8 changed files with 84 additions and 50 deletions
|
@ -1399,7 +1399,6 @@ phutil_register_library_map(array(
|
|||
'HeraldRelatedFieldGroup' => 'applications/herald/field/HeraldRelatedFieldGroup.php',
|
||||
'HeraldRemarkupFieldValue' => 'applications/herald/value/HeraldRemarkupFieldValue.php',
|
||||
'HeraldRemarkupRule' => 'applications/herald/remarkup/HeraldRemarkupRule.php',
|
||||
'HeraldRepetitionPolicyConfig' => 'applications/herald/config/HeraldRepetitionPolicyConfig.php',
|
||||
'HeraldRule' => 'applications/herald/storage/HeraldRule.php',
|
||||
'HeraldRuleController' => 'applications/herald/controller/HeraldRuleController.php',
|
||||
'HeraldRuleDatasource' => 'applications/herald/typeahead/HeraldRuleDatasource.php',
|
||||
|
@ -6602,7 +6601,6 @@ phutil_register_library_map(array(
|
|||
'HeraldRelatedFieldGroup' => 'HeraldFieldGroup',
|
||||
'HeraldRemarkupFieldValue' => 'HeraldFieldValue',
|
||||
'HeraldRemarkupRule' => 'PhabricatorObjectRemarkupRule',
|
||||
'HeraldRepetitionPolicyConfig' => 'Phobject',
|
||||
'HeraldRule' => array(
|
||||
'HeraldDAO',
|
||||
'PhabricatorApplicationTransactionInterface',
|
||||
|
|
|
@ -766,14 +766,14 @@ abstract class HeraldAdapter extends Phobject {
|
|||
public function getRepetitionOptions() {
|
||||
$options = array();
|
||||
|
||||
$options[] = HeraldRepetitionPolicyConfig::EVERY;
|
||||
$options[] = HeraldRule::REPEAT_EVERY;
|
||||
|
||||
// Some rules, like pre-commit rules, only ever fire once. It doesn't
|
||||
// make sense to use state-based repetition policies like "only the first
|
||||
// time" for these rules.
|
||||
|
||||
if (!$this->isSingleEventAdapter()) {
|
||||
$options[] = HeraldRepetitionPolicyConfig::FIRST;
|
||||
$options[] = HeraldRule::REPEAT_FIRST;
|
||||
}
|
||||
|
||||
return $options;
|
||||
|
@ -897,10 +897,7 @@ abstract class HeraldAdapter extends Phobject {
|
|||
));
|
||||
}
|
||||
|
||||
$integer_code_for_every = HeraldRepetitionPolicyConfig::toInt(
|
||||
HeraldRepetitionPolicyConfig::EVERY);
|
||||
|
||||
if ($rule->getRepetitionPolicy() == $integer_code_for_every) {
|
||||
if ($rule->isRepeatEvery()) {
|
||||
$action_text =
|
||||
pht('Take these actions every time this rule matches:');
|
||||
} else {
|
||||
|
|
|
@ -1,28 +0,0 @@
|
|||
<?php
|
||||
|
||||
final class HeraldRepetitionPolicyConfig extends Phobject {
|
||||
|
||||
const FIRST = 'first'; // only execute the first time (no repeating)
|
||||
const EVERY = 'every'; // repeat every time
|
||||
|
||||
private static $policyIntMap = array(
|
||||
self::FIRST => 0,
|
||||
self::EVERY => 1,
|
||||
);
|
||||
|
||||
public static function getMap() {
|
||||
return array(
|
||||
self::EVERY => pht('every time'),
|
||||
self::FIRST => pht('only the first time'),
|
||||
);
|
||||
}
|
||||
|
||||
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);
|
||||
}
|
||||
|
||||
}
|
|
@ -373,8 +373,7 @@ final class HeraldRuleController extends HeraldController {
|
|||
// mutate current rule, so it would be sent to the client in the right state
|
||||
$rule->setMustMatchAll((int)$match_all);
|
||||
$rule->setName($new_name);
|
||||
$rule->setRepetitionPolicy(
|
||||
HeraldRepetitionPolicyConfig::toInt($repetition_policy_param));
|
||||
$rule->setRepetitionPolicyStringConstant($repetition_policy_param);
|
||||
$rule->attachConditions($conditions);
|
||||
$rule->attachActions($actions);
|
||||
|
||||
|
@ -594,11 +593,10 @@ final class HeraldRuleController extends HeraldController {
|
|||
* time) this rule matches..." element.
|
||||
*/
|
||||
private function renderRepetitionSelector($rule, HeraldAdapter $adapter) {
|
||||
$repetition_policy = HeraldRepetitionPolicyConfig::toString(
|
||||
$rule->getRepetitionPolicy());
|
||||
$repetition_policy = $rule->getRepetitionPolicyStringConstant();
|
||||
|
||||
$repetition_options = $adapter->getRepetitionOptions();
|
||||
$repetition_names = HeraldRepetitionPolicyConfig::getMap();
|
||||
$repetition_names = HeraldRule::getRepetitionPolicySelectOptionMap();
|
||||
$repetition_map = array_select_keys($repetition_names, $repetition_options);
|
||||
|
||||
if (count($repetition_map) < 2) {
|
||||
|
|
|
@ -66,8 +66,10 @@ final class HeraldRuleEditor
|
|||
$object->setMustMatchAll((int)$new_state['match_all']);
|
||||
$object->attachConditions($new_state['conditions']);
|
||||
$object->attachActions($new_state['actions']);
|
||||
$object->setRepetitionPolicy(
|
||||
HeraldRepetitionPolicyConfig::toInt($new_state['repetition_policy']));
|
||||
|
||||
$new_repetition = $new_state['repetition_policy'];
|
||||
$object->setRepetitionPolicyStringConstant($new_repetition);
|
||||
|
||||
return $object;
|
||||
}
|
||||
|
||||
|
|
|
@ -9,7 +9,7 @@ final class HeraldRuleSerializer extends Phobject {
|
|||
(bool)$rule->getMustMatchAll(),
|
||||
$rule->getConditions(),
|
||||
$rule->getActions(),
|
||||
HeraldRepetitionPolicyConfig::toString($rule->getRepetitionPolicy()));
|
||||
$rule->getRepetitionPolicyStringConstant());
|
||||
}
|
||||
|
||||
public function serializeRuleComponents(
|
||||
|
|
|
@ -68,9 +68,7 @@ final class HeraldEngine extends Phobject {
|
|||
foreach ($rules as $phid => $rule) {
|
||||
$this->stack = array();
|
||||
|
||||
$policy_first = HeraldRepetitionPolicyConfig::FIRST;
|
||||
$policy_first_int = HeraldRepetitionPolicyConfig::toInt($policy_first);
|
||||
$is_first_only = ($rule->getRepetitionPolicy() == $policy_first_int);
|
||||
$is_first_only = $rule->isRepeatFirst();
|
||||
|
||||
try {
|
||||
if (!$this->getDryRun() &&
|
||||
|
@ -175,8 +173,6 @@ final class HeraldEngine extends Phobject {
|
|||
|
||||
$rules = mpull($rules, null, 'getID');
|
||||
$applied_ids = array();
|
||||
$first_policy = HeraldRepetitionPolicyConfig::toInt(
|
||||
HeraldRepetitionPolicyConfig::FIRST);
|
||||
|
||||
// Mark all the rules that have had their effects applied as having been
|
||||
// executed for the current object.
|
||||
|
@ -194,7 +190,7 @@ final class HeraldEngine extends Phobject {
|
|||
continue;
|
||||
}
|
||||
|
||||
if ($rule->getRepetitionPolicy() == $first_policy) {
|
||||
if ($rule->isRepeatFirst()) {
|
||||
$applied_ids[] = $rule_id;
|
||||
}
|
||||
}
|
||||
|
|
|
@ -30,6 +30,9 @@ final class HeraldRule extends HeraldDAO
|
|||
private $actions;
|
||||
private $triggerObject = self::ATTACHABLE;
|
||||
|
||||
const REPEAT_EVERY = 'every';
|
||||
const REPEAT_FIRST = 'first';
|
||||
|
||||
protected function getConfiguration() {
|
||||
return array(
|
||||
self::CONFIG_AUX_PHID => true,
|
||||
|
@ -254,6 +257,74 @@ final class HeraldRule extends HeraldDAO
|
|||
}
|
||||
|
||||
|
||||
/* -( Repetition Policies )------------------------------------------------ */
|
||||
|
||||
|
||||
public function getRepetitionPolicyStringConstant() {
|
||||
$map = self::getRepetitionPolicyMap();
|
||||
$map = ipull($map, 'key.string', 'key.int');
|
||||
|
||||
return idx($map, $this->getRepetitionPolicyIntegerConstant());
|
||||
}
|
||||
|
||||
public function getRepetitionPolicyIntegerConstant() {
|
||||
$map = self::getRepetitionPolicyMap();
|
||||
$map = ipull($map, 'key.int', 'key.int');
|
||||
$int = $this->getRepetitionPolicy();
|
||||
|
||||
if (!isset($map[$int])) {
|
||||
return head_key($map);
|
||||
}
|
||||
|
||||
return $int;
|
||||
}
|
||||
|
||||
public function setRepetitionPolicyStringConstant($value) {
|
||||
$map = self::getRepetitionPolicyMap();
|
||||
$map = ipull($map, 'key.int', 'key.string');
|
||||
|
||||
if (!isset($map[$value])) {
|
||||
throw new Exception(
|
||||
pht(
|
||||
'Rule repetition string constant "%s" is unknown.',
|
||||
$value));
|
||||
}
|
||||
|
||||
$int = $map[$value];
|
||||
|
||||
return $this->setRepetitionPolicy($int);
|
||||
}
|
||||
|
||||
public function isRepeatEvery() {
|
||||
return ($this->getRepetitionPolicyStringConstant() === self::REPEAT_EVERY);
|
||||
}
|
||||
|
||||
public function isRepeatFirst() {
|
||||
return ($this->getRepetitionPolicyStringConstant() === self::REPEAT_FIRST);
|
||||
}
|
||||
|
||||
public static function getRepetitionPolicySelectOptionMap() {
|
||||
$map = self::getRepetitionPolicyMap();
|
||||
$map = ipull($map, 'select', 'key.string');
|
||||
return $map;
|
||||
}
|
||||
|
||||
private static function getRepetitionPolicyMap() {
|
||||
return array(
|
||||
self::REPEAT_EVERY => array(
|
||||
'key.int' => 1,
|
||||
'key.string' => self::REPEAT_EVERY,
|
||||
'select' => pht('every time'),
|
||||
),
|
||||
self::REPEAT_FIRST => array(
|
||||
'key.int' => 0,
|
||||
'key.string' => self::REPEAT_FIRST,
|
||||
'select' => pht('only the first time'),
|
||||
),
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
/* -( PhabricatorApplicationTransactionInterface )------------------------- */
|
||||
|
||||
|
||||
|
|
Loading…
Reference in a new issue