From 51fead17cf132864165b912af44f02683d0aa871 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 24 Jul 2015 08:19:47 -0700 Subject: [PATCH] Modularize "Send an Email" Herald actions Summary: Ref T8726. No surprises. Test Plan: Created rules using both action variants, applied upgrade, saw rules still work correctly. {F658842} Reviewers: btrahan Reviewed By: btrahan Subscribers: joshuaspence, epriestley Maniphest Tasks: T8726 Differential Revision: https://secure.phabricator.com/D13701 --- .../sql/autopatches/20150724.herald.2.sql | 13 ++++ src/__phutil_library_map__.php | 6 ++ .../HeraldDifferentialRevisionAdapter.php | 2 - .../diffusion/herald/HeraldCommitAdapter.php | 2 - .../herald/HeraldPreCommitAdapter.php | 2 - .../herald/adapter/HeraldAdapter.php | 41 +++-------- .../herald/HeraldManiphestTaskAdapter.php | 2 - .../PhabricatorMetaMTAEmailHeraldAction.php | 69 +++++++++++++++++++ ...bricatorMetaMTAEmailOthersHeraldAction.php | 32 +++++++++ ...habricatorMetaMTAEmailSelfHeraldAction.php | 34 +++++++++ .../herald/PhrictionDocumentHeraldAdapter.php | 18 ----- ...icatorSubscriptionsAddSelfHeraldAction.php | 8 --- ...ubscriptionsAddSubscribersHeraldAction.php | 8 --- .../PhabricatorSubscriptionsHeraldAction.php | 8 +++ ...torSubscriptionsRemoveSelfHeraldAction.php | 8 --- ...criptionsRemoveSubscribersHeraldAction.php | 8 --- .../PhabricatorUSEnglishTranslation.php | 13 ++++ 17 files changed, 183 insertions(+), 91 deletions(-) create mode 100644 resources/sql/autopatches/20150724.herald.2.sql create mode 100644 src/applications/metamta/herald/PhabricatorMetaMTAEmailHeraldAction.php create mode 100644 src/applications/metamta/herald/PhabricatorMetaMTAEmailOthersHeraldAction.php create mode 100644 src/applications/metamta/herald/PhabricatorMetaMTAEmailSelfHeraldAction.php diff --git a/resources/sql/autopatches/20150724.herald.2.sql b/resources/sql/autopatches/20150724.herald.2.sql new file mode 100644 index 0000000000..eef860a24c --- /dev/null +++ b/resources/sql/autopatches/20150724.herald.2.sql @@ -0,0 +1,13 @@ +UPDATE {$NAMESPACE}_herald.herald_actionrecord a + JOIN {$NAMESPACE}_herald.herald_rule r + ON a.ruleID = r.id + SET a.action = 'email.other' + WHERE r.ruleType != 'personal' + AND a.action = 'email'; + +UPDATE {$NAMESPACE}_herald.herald_actionrecord a + JOIN {$NAMESPACE}_herald.herald_rule r + ON a.ruleID = r.id + SET a.action = 'email.self' + WHERE r.ruleType = 'personal' + AND a.action = 'email'; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 8904abc6de..f46f0538b1 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2274,6 +2274,9 @@ phutil_register_library_map(array( 'PhabricatorMetaMTADAO' => 'applications/metamta/storage/PhabricatorMetaMTADAO.php', 'PhabricatorMetaMTAEmailBodyParser' => 'applications/metamta/parser/PhabricatorMetaMTAEmailBodyParser.php', 'PhabricatorMetaMTAEmailBodyParserTestCase' => 'applications/metamta/parser/__tests__/PhabricatorMetaMTAEmailBodyParserTestCase.php', + 'PhabricatorMetaMTAEmailHeraldAction' => 'applications/metamta/herald/PhabricatorMetaMTAEmailHeraldAction.php', + 'PhabricatorMetaMTAEmailOthersHeraldAction' => 'applications/metamta/herald/PhabricatorMetaMTAEmailOthersHeraldAction.php', + 'PhabricatorMetaMTAEmailSelfHeraldAction' => 'applications/metamta/herald/PhabricatorMetaMTAEmailSelfHeraldAction.php', 'PhabricatorMetaMTAErrorMailAction' => 'applications/metamta/action/PhabricatorMetaMTAErrorMailAction.php', 'PhabricatorMetaMTAMail' => 'applications/metamta/storage/PhabricatorMetaMTAMail.php', 'PhabricatorMetaMTAMailBody' => 'applications/metamta/view/PhabricatorMetaMTAMailBody.php', @@ -6170,6 +6173,9 @@ phutil_register_library_map(array( 'PhabricatorMetaMTADAO' => 'PhabricatorLiskDAO', 'PhabricatorMetaMTAEmailBodyParser' => 'Phobject', 'PhabricatorMetaMTAEmailBodyParserTestCase' => 'PhabricatorTestCase', + 'PhabricatorMetaMTAEmailHeraldAction' => 'HeraldAction', + 'PhabricatorMetaMTAEmailOthersHeraldAction' => 'PhabricatorMetaMTAEmailHeraldAction', + 'PhabricatorMetaMTAEmailSelfHeraldAction' => 'PhabricatorMetaMTAEmailHeraldAction', 'PhabricatorMetaMTAErrorMailAction' => 'PhabricatorSystemAction', 'PhabricatorMetaMTAMail' => array( 'PhabricatorMetaMTADAO', diff --git a/src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php b/src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php index 47a132338a..a2d7f002d3 100644 --- a/src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php +++ b/src/applications/differential/herald/HeraldDifferentialRevisionAdapter.php @@ -162,7 +162,6 @@ final class HeraldDifferentialRevisionAdapter case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL: return array_merge( array( - self::ACTION_EMAIL, self::ACTION_ADD_REVIEWERS, self::ACTION_ADD_BLOCKING_REVIEWERS, self::ACTION_APPLY_BUILD_PLANS, @@ -172,7 +171,6 @@ final class HeraldDifferentialRevisionAdapter case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL: return array_merge( array( - self::ACTION_EMAIL, self::ACTION_ADD_REVIEWERS, self::ACTION_ADD_BLOCKING_REVIEWERS, ), diff --git a/src/applications/diffusion/herald/HeraldCommitAdapter.php b/src/applications/diffusion/herald/HeraldCommitAdapter.php index 302027b539..41971e1c2f 100644 --- a/src/applications/diffusion/herald/HeraldCommitAdapter.php +++ b/src/applications/diffusion/herald/HeraldCommitAdapter.php @@ -89,7 +89,6 @@ final class HeraldCommitAdapter extends HeraldAdapter { case HeraldRuleTypeConfig::RULE_TYPE_OBJECT: return array_merge( array( - self::ACTION_EMAIL, self::ACTION_AUDIT, self::ACTION_APPLY_BUILD_PLANS, ), @@ -97,7 +96,6 @@ final class HeraldCommitAdapter extends HeraldAdapter { case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL: return array_merge( array( - self::ACTION_EMAIL, self::ACTION_AUDIT, ), parent::getActions($rule_type)); diff --git a/src/applications/diffusion/herald/HeraldPreCommitAdapter.php b/src/applications/diffusion/herald/HeraldPreCommitAdapter.php index 7e5e117d85..448f4eaefd 100644 --- a/src/applications/diffusion/herald/HeraldPreCommitAdapter.php +++ b/src/applications/diffusion/herald/HeraldPreCommitAdapter.php @@ -80,13 +80,11 @@ abstract class HeraldPreCommitAdapter extends HeraldAdapter { return array_merge( array( self::ACTION_BLOCK, - self::ACTION_EMAIL, ), parent::getActions($rule_type)); case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL: return array_merge( array( - self::ACTION_EMAIL, ), parent::getActions($rule_type)); } diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php index 4e7e49ea66..3de06e63d6 100644 --- a/src/applications/herald/adapter/HeraldAdapter.php +++ b/src/applications/herald/adapter/HeraldAdapter.php @@ -26,7 +26,6 @@ abstract class HeraldAdapter extends Phobject { const CONDITION_IS_TRUE = 'true'; const CONDITION_IS_FALSE = 'false'; - const ACTION_EMAIL = 'email'; const ACTION_AUDIT = 'audit'; const ACTION_ASSIGN_TASK = 'assigntask'; const ACTION_ADD_PROJECTS = 'addprojects'; @@ -56,6 +55,14 @@ abstract class HeraldAdapter extends Phobject { return array_values($this->forcedEmailPHIDs); } + public function addEmailPHID($phid, $force) { + $this->emailPHIDs[$phid] = $phid; + if ($force) { + $this->forcedEmailPHIDs[$phid] = $phid; + } + return $this; + } + public function getCustomActions() { if ($this->customActions === null) { $custom_actions = id(new PhutilSymbolLoader()) @@ -721,7 +728,6 @@ abstract class HeraldAdapter extends Phobject { case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL: case HeraldRuleTypeConfig::RULE_TYPE_OBJECT: $standard = array( - self::ACTION_EMAIL => pht('Send an email to'), self::ACTION_AUDIT => pht('Trigger an Audit by'), self::ACTION_ASSIGN_TASK => pht('Assign task to'), self::ACTION_ADD_PROJECTS => pht('Add projects'), @@ -735,7 +741,6 @@ abstract class HeraldAdapter extends Phobject { break; case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL: $standard = array( - self::ACTION_EMAIL => pht('Send me an email'), self::ACTION_AUDIT => pht('Trigger an Audit by me'), self::ACTION_ASSIGN_TASK => pht('Assign task to me'), self::ACTION_ADD_REVIEWERS => pht('Add me as a reviewer'), @@ -779,7 +784,6 @@ abstract class HeraldAdapter extends Phobject { $rule_type = $rule->getRuleType(); if ($rule_type == HeraldRuleTypeConfig::RULE_TYPE_PERSONAL) { switch ($action->getAction()) { - case self::ACTION_EMAIL: case self::ACTION_AUDIT: case self::ACTION_ASSIGN_TASK: case self::ACTION_ADD_REVIEWERS: @@ -820,7 +824,6 @@ abstract class HeraldAdapter extends Phobject { if ($is_personal) { switch ($action) { - case self::ACTION_EMAIL: case self::ACTION_AUDIT: case self::ACTION_ASSIGN_TASK: case self::ACTION_ADD_REVIEWERS: @@ -833,9 +836,6 @@ abstract class HeraldAdapter extends Phobject { } } else { switch ($action) { - case self::ACTION_EMAIL: - return $this->buildTokenizerFieldValue( - new PhabricatorMetaMTAMailableDatasource()); case self::ACTION_ADD_PROJECTS: case self::ACTION_REMOVE_PROJECTS: return $this->buildTokenizerFieldValue( @@ -1204,8 +1204,6 @@ abstract class HeraldAdapter extends Phobject { case self::ACTION_ADD_PROJECTS: case self::ACTION_REMOVE_PROJECTS: return $this->applyProjectsEffect($effect); - case self::ACTION_EMAIL: - return $this->applyEmailEffect($effect); default: break; } @@ -1253,29 +1251,6 @@ abstract class HeraldAdapter extends Phobject { pht('Added projects.')); } - - /** - * @task apply - */ - private function applyEmailEffect(HeraldEffect $effect) { - foreach ($effect->getTarget() as $phid) { - $this->emailPHIDs[$phid] = $phid; - - // If this is a personal rule, we'll force delivery of a real email. This - // effect is stronger than notification preferences, so you get an actual - // email even if your preferences are set to "Notify" or "Ignore". - $rule = $effect->getRule(); - if ($rule->isPersonalRule()) { - $this->forcedEmailPHIDs[$phid] = $phid; - } - } - - return new HeraldApplyTranscript( - $effect, - true, - pht('Added mailable to mail targets.')); - } - public function loadEdgePHIDs($type) { if (!isset($this->edgeCache[$type])) { $phids = PhabricatorEdgeQuery::loadDestinationPHIDs( diff --git a/src/applications/maniphest/herald/HeraldManiphestTaskAdapter.php b/src/applications/maniphest/herald/HeraldManiphestTaskAdapter.php index 0ef1399e54..83ca7c5333 100644 --- a/src/applications/maniphest/herald/HeraldManiphestTaskAdapter.php +++ b/src/applications/maniphest/herald/HeraldManiphestTaskAdapter.php @@ -72,14 +72,12 @@ final class HeraldManiphestTaskAdapter extends HeraldAdapter { case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL: return array_merge( array( - self::ACTION_EMAIL, self::ACTION_ASSIGN_TASK, ), parent::getActions($rule_type)); case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL: return array_merge( array( - self::ACTION_EMAIL, self::ACTION_ASSIGN_TASK, ), parent::getActions($rule_type)); diff --git a/src/applications/metamta/herald/PhabricatorMetaMTAEmailHeraldAction.php b/src/applications/metamta/herald/PhabricatorMetaMTAEmailHeraldAction.php new file mode 100644 index 0000000000..07f216447d --- /dev/null +++ b/src/applications/metamta/herald/PhabricatorMetaMTAEmailHeraldAction.php @@ -0,0 +1,69 @@ +getAdapter(); + + foreach ($phids as $phid) { + $adapter->addEmailPHID($phid, $force); + } + + if ($force) { + $this->logEffect(self::DO_FORCE, $phids); + } else { + $this->logEffect(self::DO_SEND, $phids); + } + } + + protected function getActionEffectMap() { + return array( + self::DO_SEND => array( + 'icon' => 'fa-envelope', + 'color' => 'green', + 'name' => pht('Sent Mail'), + ), + self::DO_FORCE => array( + 'icon' => 'fa-envelope', + 'color' => 'blue', + 'name' => pht('Forced Mail'), + ), + ); + } + + public function renderActionEffectDescription($type, $data) { + switch ($type) { + case self::DO_SEND: + return pht( + 'Queued email to be delivered to %s target(s): %s.', + new PhutilNumber(count($data)), + $this->renderHandleList($data)); + case self::DO_FORCE: + return pht( + 'Queued email to be delivered to %s target(s), ignoring their '. + 'notification preferences: %s.', + new PhutilNumber(count($data)), + $this->renderHandleList($data)); + } + } + +} diff --git a/src/applications/metamta/herald/PhabricatorMetaMTAEmailOthersHeraldAction.php b/src/applications/metamta/herald/PhabricatorMetaMTAEmailOthersHeraldAction.php new file mode 100644 index 0000000000..d569848e01 --- /dev/null +++ b/src/applications/metamta/herald/PhabricatorMetaMTAEmailOthersHeraldAction.php @@ -0,0 +1,32 @@ +applyEmail($effect->getTarget(), $force = false); + } + + public function getHeraldActionStandardType() { + return self::STANDARD_PHID_LIST; + } + + protected function getDatasource() { + return new PhabricatorMetaMTAMailableDatasource(); + } + + public function renderActionDescription($value) { + return pht('Send an email to: %s.', $this->renderHandleList($value)); + } + +} diff --git a/src/applications/metamta/herald/PhabricatorMetaMTAEmailSelfHeraldAction.php b/src/applications/metamta/herald/PhabricatorMetaMTAEmailSelfHeraldAction.php new file mode 100644 index 0000000000..67563da521 --- /dev/null +++ b/src/applications/metamta/herald/PhabricatorMetaMTAEmailSelfHeraldAction.php @@ -0,0 +1,34 @@ +getRule()->getAuthorPHID(); + + // For personal rules, we'll force delivery of a real email. This effect + // is stronger than notification preferences, so you get an actual email + // even if your preferences are set to "Notify" or "Ignore". + + return $this->applyEmail(array($phid), $force = true); + } + + public function getHeraldActionStandardType() { + return self::STANDARD_NONE; + } + + public function renderActionDescription($value) { + return pht('Send an email to rule author.'); + } + +} diff --git a/src/applications/phriction/herald/PhrictionDocumentHeraldAdapter.php b/src/applications/phriction/herald/PhrictionDocumentHeraldAdapter.php index 17128a5773..575d4b3dd8 100644 --- a/src/applications/phriction/herald/PhrictionDocumentHeraldAdapter.php +++ b/src/applications/phriction/herald/PhrictionDocumentHeraldAdapter.php @@ -48,24 +48,6 @@ final class PhrictionDocumentHeraldAdapter extends HeraldAdapter { } } - public function getActions($rule_type) { - switch ($rule_type) { - case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL: - return array_merge( - array( - self::ACTION_EMAIL, - ), - parent::getActions($rule_type)); - case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL: - return array_merge( - array( - self::ACTION_EMAIL, - ), - parent::getActions($rule_type)); - } - } - - public function getHeraldName() { return pht('Wiki Document %d', $this->getDocument()->getID()); } diff --git a/src/applications/subscriptions/herald/PhabricatorSubscriptionsAddSelfHeraldAction.php b/src/applications/subscriptions/herald/PhabricatorSubscriptionsAddSelfHeraldAction.php index 44199422b0..20df5e06c4 100644 --- a/src/applications/subscriptions/herald/PhabricatorSubscriptionsAddSelfHeraldAction.php +++ b/src/applications/subscriptions/herald/PhabricatorSubscriptionsAddSelfHeraldAction.php @@ -9,14 +9,6 @@ final class PhabricatorSubscriptionsAddSelfHeraldAction return pht('Add me as a subscriber'); } - public function getActionGroupKey() { - return HeraldSupportActionGroup::ACTIONGROUPKEY; - } - - public function supportsObject($object) { - return ($object instanceof PhabricatorSubscribableInterface); - } - public function supportsRuleType($rule_type) { return ($rule_type == HeraldRuleTypeConfig::RULE_TYPE_PERSONAL); } diff --git a/src/applications/subscriptions/herald/PhabricatorSubscriptionsAddSubscribersHeraldAction.php b/src/applications/subscriptions/herald/PhabricatorSubscriptionsAddSubscribersHeraldAction.php index 7fbec15e9b..14f72d313a 100644 --- a/src/applications/subscriptions/herald/PhabricatorSubscriptionsAddSubscribersHeraldAction.php +++ b/src/applications/subscriptions/herald/PhabricatorSubscriptionsAddSubscribersHeraldAction.php @@ -9,14 +9,6 @@ final class PhabricatorSubscriptionsAddSubscribersHeraldAction return pht('Add subscribers'); } - public function getActionGroupKey() { - return HeraldSupportActionGroup::ACTIONGROUPKEY; - } - - public function supportsObject($object) { - return ($object instanceof PhabricatorSubscribableInterface); - } - public function supportsRuleType($rule_type) { return ($rule_type != HeraldRuleTypeConfig::RULE_TYPE_PERSONAL); } diff --git a/src/applications/subscriptions/herald/PhabricatorSubscriptionsHeraldAction.php b/src/applications/subscriptions/herald/PhabricatorSubscriptionsHeraldAction.php index 66e0d62959..374fcfa1fb 100644 --- a/src/applications/subscriptions/herald/PhabricatorSubscriptionsHeraldAction.php +++ b/src/applications/subscriptions/herald/PhabricatorSubscriptionsHeraldAction.php @@ -12,6 +12,14 @@ abstract class PhabricatorSubscriptionsHeraldAction const DO_SUBSCRIBED = 'do.subscribed'; const DO_UNSUBSCRIBED = 'do.unsubscribed'; + public function getActionGroupKey() { + return HeraldSupportActionGroup::ACTIONGROUPKEY; + } + + public function supportsObject($object) { + return ($object instanceof PhabricatorSubscribableInterface); + } + protected function applySubscribe(array $phids, $is_add) { $adapter = $this->getAdapter(); diff --git a/src/applications/subscriptions/herald/PhabricatorSubscriptionsRemoveSelfHeraldAction.php b/src/applications/subscriptions/herald/PhabricatorSubscriptionsRemoveSelfHeraldAction.php index 324a04c449..5d751cb54a 100644 --- a/src/applications/subscriptions/herald/PhabricatorSubscriptionsRemoveSelfHeraldAction.php +++ b/src/applications/subscriptions/herald/PhabricatorSubscriptionsRemoveSelfHeraldAction.php @@ -9,14 +9,6 @@ final class PhabricatorSubscriptionsRemoveSelfHeraldAction return pht('Remove me as a subscriber'); } - public function getActionGroupKey() { - return HeraldSupportActionGroup::ACTIONGROUPKEY; - } - - public function supportsObject($object) { - return ($object instanceof PhabricatorSubscribableInterface); - } - public function supportsRuleType($rule_type) { return ($rule_type == HeraldRuleTypeConfig::RULE_TYPE_PERSONAL); } diff --git a/src/applications/subscriptions/herald/PhabricatorSubscriptionsRemoveSubscribersHeraldAction.php b/src/applications/subscriptions/herald/PhabricatorSubscriptionsRemoveSubscribersHeraldAction.php index 71f433811e..67ece2cb72 100644 --- a/src/applications/subscriptions/herald/PhabricatorSubscriptionsRemoveSubscribersHeraldAction.php +++ b/src/applications/subscriptions/herald/PhabricatorSubscriptionsRemoveSubscribersHeraldAction.php @@ -9,14 +9,6 @@ final class PhabricatorSubscriptionsRemoveSubscribersHeraldAction return pht('Remove subscribers'); } - public function getActionGroupKey() { - return HeraldSupportActionGroup::ACTIONGROUPKEY; - } - - public function supportsObject($object) { - return ($object instanceof PhabricatorSubscribableInterface); - } - public function supportsRuleType($rule_type) { return ($rule_type != HeraldRuleTypeConfig::RULE_TYPE_PERSONAL); } diff --git a/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php b/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php index c52137bdf2..a23ce1293c 100644 --- a/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php +++ b/src/infrastructure/internationalization/translation/PhabricatorUSEnglishTranslation.php @@ -1290,6 +1290,19 @@ final class PhabricatorUSEnglishTranslation 'Removed subscribers: %2$s.', ), + 'Queued email to be delivered to %s target(s): %s.' => array( + 'Queued email to be delivered to target: %2$s.', + 'Queued email to be delivered to targets: %2$s.', + ), + + 'Queued email to be delivered to %s target(s), ignoring their '. + 'notification preferences: %s.' => array( + 'Queued email to be delivered to target, ignoring notification '. + 'preferences: %2$s.', + 'Queued email to be delivered to targets, ignoring notification '. + 'preferences: %2$s.', + ), + ); } -- 2.51.2