From a90b16e83a33ab7a37c815e43bcd75d006d0bd49 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 24 Jan 2018 19:35:36 -0800 Subject: [PATCH] Define available Herald rule repetition options in terms of "isSingleEventAdapter()" Summary: Depends on D18924. Ref T13048. Each adapter defines which repetition options ("every time", "only the first time") users may select for rules. Currently, this is all explicit and hard-coded. However, every adapter really just implements this rule (except for some bugs, see below): > You can pick "only the first time" if this adapter fires more than once on the same object. Since we already have a `isSingleEventAdapter()` method which lets us tell if an adapter fires more than once, just write this rule in the base class and delete all the copy/pasting. This also fixes two bugs because of the copy/pasting: Pholio Mocks and Phriction Documents did not allow you to write "only the first time" rules. There's no reason for this, they just didn't copy/paste enough methods when they were implemented. This will make a future diff (which introduces an "if the rule did not match last time" policy) cleaner. Test Plan: - Checked several different types of rules, saw appropriate options in the dropdown (pre-commit: no options; tasks: first or every). - Checked mocks and wiki docs, saw that you can now write "only the first time" rules. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13048 Differential Revision: https://secure.phabricator.com/D18925 --- .../PhabricatorCalendarEventHeraldAdapter.php | 7 ------- .../herald/HeraldDifferentialDiffAdapter.php | 6 ------ .../herald/HeraldDifferentialRevisionAdapter.php | 7 ------- .../herald/adapter/HeraldAdapter.php | 16 +++++++++++++--- .../herald/HeraldManiphestTaskAdapter.php | 7 ------- .../PhabricatorMailOutboundMailHeraldAdapter.php | 6 ------ .../phame/herald/HeraldPhameBlogAdapter.php | 7 ------- .../phame/herald/HeraldPhamePostAdapter.php | 7 ------- .../herald/HeraldPonderQuestionAdapter.php | 7 ------- .../herald/PhabricatorProjectHeraldAdapter.php | 7 ------- 10 files changed, 13 insertions(+), 64 deletions(-) diff --git a/src/applications/calendar/herald/PhabricatorCalendarEventHeraldAdapter.php b/src/applications/calendar/herald/PhabricatorCalendarEventHeraldAdapter.php index 1fc3a56317..49d5f38959 100644 --- a/src/applications/calendar/herald/PhabricatorCalendarEventHeraldAdapter.php +++ b/src/applications/calendar/herald/PhabricatorCalendarEventHeraldAdapter.php @@ -49,13 +49,6 @@ final class PhabricatorCalendarEventHeraldAdapter extends HeraldAdapter { } } - public function getRepetitionOptions() { - return array( - HeraldRepetitionPolicyConfig::EVERY, - HeraldRepetitionPolicyConfig::FIRST, - ); - } - public function getHeraldName() { return $this->getObject()->getMonogram(); } diff --git a/src/applications/differential/herald/HeraldDifferentialDiffAdapter.php b/src/applications/differential/herald/HeraldDifferentialDiffAdapter.php index 9528978050..a6b2e7c36e 100644 --- a/src/applications/differential/herald/HeraldDifferentialDiffAdapter.php +++ b/src/applications/differential/herald/HeraldDifferentialDiffAdapter.php @@ -53,12 +53,6 @@ final class HeraldDifferentialDiffAdapter extends HeraldDifferentialAdapter { } } - public function getRepetitionOptions() { - return array( - HeraldRepetitionPolicyConfig::FIRST, - ); - } - public function getHeraldName() { return pht('New Diff'); } diff --git a/src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php b/src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php index e20a4fd37d..9f7f8fe3f2 100644 --- a/src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php +++ b/src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php @@ -69,13 +69,6 @@ final class HeraldDifferentialRevisionAdapter } } - public function getRepetitionOptions() { - return array( - HeraldRepetitionPolicyConfig::EVERY, - HeraldRepetitionPolicyConfig::FIRST, - ); - } - public static function newLegacyAdapter( DifferentialRevision $revision, DifferentialDiff $diff) { diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php index 1c50c0b5ee..cdf1d3a864 100644 --- a/src/applications/herald/adapter/HeraldAdapter.php +++ b/src/applications/herald/adapter/HeraldAdapter.php @@ -764,9 +764,19 @@ abstract class HeraldAdapter extends Phobject { public function getRepetitionOptions() { - return array( - HeraldRepetitionPolicyConfig::EVERY, - ); + $options = array(); + + $options[] = HeraldRepetitionPolicyConfig::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; + } + + return $options; } protected function initializeNewAdapter() { diff --git a/src/applications/maniphest/herald/HeraldManiphestTaskAdapter.php b/src/applications/maniphest/herald/HeraldManiphestTaskAdapter.php index 8503839e3e..1aa544de57 100644 --- a/src/applications/maniphest/herald/HeraldManiphestTaskAdapter.php +++ b/src/applications/maniphest/herald/HeraldManiphestTaskAdapter.php @@ -33,13 +33,6 @@ final class HeraldManiphestTaskAdapter extends HeraldAdapter { return true; } - public function getRepetitionOptions() { - return array( - HeraldRepetitionPolicyConfig::EVERY, - HeraldRepetitionPolicyConfig::FIRST, - ); - } - public function supportsRuleType($rule_type) { switch ($rule_type) { case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL: diff --git a/src/applications/metamta/herald/PhabricatorMailOutboundMailHeraldAdapter.php b/src/applications/metamta/herald/PhabricatorMailOutboundMailHeraldAdapter.php index 6ca797833e..f96a6f86bd 100644 --- a/src/applications/metamta/herald/PhabricatorMailOutboundMailHeraldAdapter.php +++ b/src/applications/metamta/herald/PhabricatorMailOutboundMailHeraldAdapter.php @@ -49,12 +49,6 @@ final class PhabricatorMailOutboundMailHeraldAdapter return true; } - public function getRepetitionOptions() { - return array( - HeraldRepetitionPolicyConfig::FIRST, - ); - } - public function supportsRuleType($rule_type) { switch ($rule_type) { case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL: diff --git a/src/applications/phame/herald/HeraldPhameBlogAdapter.php b/src/applications/phame/herald/HeraldPhameBlogAdapter.php index d8956ba4a6..d9368a97d2 100644 --- a/src/applications/phame/herald/HeraldPhameBlogAdapter.php +++ b/src/applications/phame/herald/HeraldPhameBlogAdapter.php @@ -24,13 +24,6 @@ final class HeraldPhameBlogAdapter extends HeraldAdapter { return true; } - public function getRepetitionOptions() { - return array( - HeraldRepetitionPolicyConfig::EVERY, - HeraldRepetitionPolicyConfig::FIRST, - ); - } - public function supportsRuleType($rule_type) { switch ($rule_type) { case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL: diff --git a/src/applications/phame/herald/HeraldPhamePostAdapter.php b/src/applications/phame/herald/HeraldPhamePostAdapter.php index 4776bbbdbc..72e7124271 100644 --- a/src/applications/phame/herald/HeraldPhamePostAdapter.php +++ b/src/applications/phame/herald/HeraldPhamePostAdapter.php @@ -24,13 +24,6 @@ final class HeraldPhamePostAdapter extends HeraldAdapter { return true; } - public function getRepetitionOptions() { - return array( - HeraldRepetitionPolicyConfig::EVERY, - HeraldRepetitionPolicyConfig::FIRST, - ); - } - public function supportsRuleType($rule_type) { switch ($rule_type) { case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL: diff --git a/src/applications/ponder/herald/HeraldPonderQuestionAdapter.php b/src/applications/ponder/herald/HeraldPonderQuestionAdapter.php index f3f47681bb..f9434c7f78 100644 --- a/src/applications/ponder/herald/HeraldPonderQuestionAdapter.php +++ b/src/applications/ponder/herald/HeraldPonderQuestionAdapter.php @@ -39,13 +39,6 @@ final class HeraldPonderQuestionAdapter extends HeraldAdapter { return true; } - public function getRepetitionOptions() { - return array( - HeraldRepetitionPolicyConfig::EVERY, - HeraldRepetitionPolicyConfig::FIRST, - ); - } - public function supportsRuleType($rule_type) { switch ($rule_type) { case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL: diff --git a/src/applications/project/herald/PhabricatorProjectHeraldAdapter.php b/src/applications/project/herald/PhabricatorProjectHeraldAdapter.php index f6d0984cb6..72f064a5c6 100644 --- a/src/applications/project/herald/PhabricatorProjectHeraldAdapter.php +++ b/src/applications/project/herald/PhabricatorProjectHeraldAdapter.php @@ -24,13 +24,6 @@ final class PhabricatorProjectHeraldAdapter extends HeraldAdapter { return true; } - public function getRepetitionOptions() { - return array( - HeraldRepetitionPolicyConfig::EVERY, - HeraldRepetitionPolicyConfig::FIRST, - ); - } - public function supportsRuleType($rule_type) { switch ($rule_type) { case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL: