From a40748a46cb5a6ee2ff478873195ecb55d07185b Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 6 Apr 2015 10:00:35 -0700 Subject: [PATCH] Lift handling of Herald "email" effect to Adapter Summary: Ref T7731. Every adapter subclass currently implements this effect in an essentially identical way. Some day far from now the effects will be modular and this mess will vanish completely, but reduce its sprawl for now. Test Plan: I'll test this thoroughly at the end of the change sequence since writing rules is a pain. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T7731 Differential Revision: https://secure.phabricator.com/D12268 --- .../editor/DifferentialTransactionEditor.php | 3 +-- .../herald/HeraldPreCommitAdapter.php | 13 +---------- .../herald/adapter/HeraldAdapter.php | 17 ++++++++++++++ .../herald/adapter/HeraldCommitAdapter.php | 13 +---------- .../HeraldDifferentialRevisionAdapter.php | 22 ++++++------------- .../adapter/HeraldManiphestTaskAdapter.php | 13 +---------- .../herald/PhrictionDocumentHeraldAdapter.php | 15 ++----------- 7 files changed, 30 insertions(+), 66 deletions(-) diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index fbac993566..1bd9d1c6a7 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -1731,8 +1731,7 @@ final class DifferentialTransactionEditor } // Save extra email PHIDs for later. - $email_phids = $adapter->getEmailPHIDsAddedByHerald(); - $this->heraldEmailPHIDs = array_keys($email_phids); + $this->heraldEmailPHIDs = $adapter->getEmailPHIDs(); // Apply build plans. HarbormasterBuildable::applyBuildPlans( diff --git a/src/applications/diffusion/herald/HeraldPreCommitAdapter.php b/src/applications/diffusion/herald/HeraldPreCommitAdapter.php index 61bcef4c33..85adffd498 100644 --- a/src/applications/diffusion/herald/HeraldPreCommitAdapter.php +++ b/src/applications/diffusion/herald/HeraldPreCommitAdapter.php @@ -4,7 +4,6 @@ abstract class HeraldPreCommitAdapter extends HeraldAdapter { private $log; private $hookEngine; - private $emailPHIDs = array(); public function setPushLog(PhabricatorRepositoryPushLog $log) { $this->log = $log; @@ -28,10 +27,6 @@ abstract class HeraldPreCommitAdapter extends HeraldAdapter { return $this->log; } - public function getEmailPHIDs() { - return array_values($this->emailPHIDs); - } - public function supportsRuleType($rule_type) { switch ($rule_type) { case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL: @@ -107,13 +102,7 @@ abstract class HeraldPreCommitAdapter extends HeraldAdapter { pht('Did nothing.')); break; case self::ACTION_EMAIL: - foreach ($effect->getTarget() as $phid) { - $this->emailPHIDs[$phid] = $phid; - } - $result[] = new HeraldApplyTranscript( - $effect, - true, - pht('Added mailable to mail targets.')); + $result[] = $this->applyEmailEffect($effect); break; case self::ACTION_BLOCK: $result[] = new HeraldApplyTranscript( diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php index 309f7dd334..8440eb303c 100644 --- a/src/applications/herald/adapter/HeraldAdapter.php +++ b/src/applications/herald/adapter/HeraldAdapter.php @@ -109,6 +109,11 @@ abstract class HeraldAdapter { private $customFields = false; private $customActions = null; private $queuedTransactions = array(); + private $emailPHIDs = array(); + + public function getEmailPHIDs() { + return array_values($this->emailPHIDs); + } public function getCustomActions() { if ($this->customActions === null) { @@ -1032,6 +1037,18 @@ abstract class HeraldAdapter { pht('Added flag.')); } + protected function applyEmailEffect(HeraldEffect $effect) { + + foreach ($effect->getTarget() as $phid) { + $this->emailPHIDs[$phid] = $phid; + } + + return new HeraldApplyTranscript( + $effect, + true, + pht('Added mailable to mail targets.')); + } + public static function getAllAdapters() { static $adapters; if (!$adapters) { diff --git a/src/applications/herald/adapter/HeraldCommitAdapter.php b/src/applications/herald/adapter/HeraldCommitAdapter.php index 9b9c1f8ddd..9580101640 100644 --- a/src/applications/herald/adapter/HeraldCommitAdapter.php +++ b/src/applications/herald/adapter/HeraldCommitAdapter.php @@ -13,7 +13,6 @@ final class HeraldCommitAdapter extends HeraldAdapter { protected $commitData; private $commitDiff; - protected $emailPHIDs = array(); protected $addCCPHIDs = array(); protected $auditMap = array(); protected $buildPlans = array(); @@ -217,10 +216,6 @@ final class HeraldCommitAdapter extends HeraldAdapter { return $this->commit->getPHID(); } - public function getEmailPHIDs() { - return array_keys($this->emailPHIDs); - } - public function getAddCCMap() { return $this->addCCPHIDs; } @@ -499,13 +494,7 @@ final class HeraldCommitAdapter extends HeraldAdapter { pht('Great success at doing nothing.')); break; case self::ACTION_EMAIL: - foreach ($effect->getTarget() as $phid) { - $this->emailPHIDs[$phid] = true; - } - $result[] = new HeraldApplyTranscript( - $effect, - true, - pht('Added address to email targets.')); + $result[] = $this->applyEmailEffect($effect); break; case self::ACTION_ADD_CC: foreach ($effect->getTarget() as $phid) { diff --git a/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php b/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php index 477d38bad5..fafc970654 100644 --- a/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php +++ b/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php @@ -11,7 +11,6 @@ final class HeraldDifferentialRevisionAdapter protected $newCCs = array(); protected $remCCs = array(); - protected $emailPHIDs = array(); protected $addReviewerPHIDs = array(); protected $blockingReviewerPHIDs = array(); protected $buildPlans = array(); @@ -131,10 +130,6 @@ final class HeraldDifferentialRevisionAdapter return $this->remCCs; } - public function getEmailPHIDsAddedByHerald() { - return $this->emailPHIDs; - } - public function getReviewersAddedByHerald() { return $this->addReviewerPHIDs; } @@ -333,8 +328,9 @@ final class HeraldDifferentialRevisionAdapter $this->revision->getPHID()); break; case self::ACTION_EMAIL: + $result[] = $this->applyEmailEffect($effect); + break; case self::ACTION_ADD_CC: - $op = ($action == self::ACTION_EMAIL) ? 'email' : 'CC'; $base_target = $effect->getTarget(); $forbidden = array(); foreach ($base_target as $key => $fbid) { @@ -342,11 +338,7 @@ final class HeraldDifferentialRevisionAdapter $forbidden[] = $fbid; unset($base_target[$key]); } else { - if ($action == self::ACTION_EMAIL) { - $this->emailPHIDs[$fbid] = true; - } else { - $this->newCCs[$fbid] = true; - } + $this->newCCs[$fbid] = true; } } @@ -358,18 +350,18 @@ final class HeraldDifferentialRevisionAdapter $result[] = new HeraldApplyTranscript( $effect, true, - pht('Added these addresses to %s list. '. - 'Others could not be added.', $op)); + pht('Added these addresses to CC list. '. + 'Others could not be added.')); } $result[] = new HeraldApplyTranscript( $failed, false, - pht('%s forbidden, these addresses have unsubscribed.', $op)); + pht('CC forbidden, these addresses have unsubscribed.')); } else { $result[] = new HeraldApplyTranscript( $effect, true, - pht('Added addresses to %s list.', $op)); + pht('Added addresses to CC list.')); } break; case self::ACTION_REMOVE_CC: diff --git a/src/applications/herald/adapter/HeraldManiphestTaskAdapter.php b/src/applications/herald/adapter/HeraldManiphestTaskAdapter.php index 6f9b1db9b8..9830fff894 100644 --- a/src/applications/herald/adapter/HeraldManiphestTaskAdapter.php +++ b/src/applications/herald/adapter/HeraldManiphestTaskAdapter.php @@ -6,11 +6,6 @@ final class HeraldManiphestTaskAdapter extends HeraldAdapter { private $ccPHIDs = array(); private $assignPHID; private $projectPHIDs = array(); - private $emailPHIDs = array(); - - public function getEmailPHIDs() { - return $this->emailPHIDs; - } public function getAdapterApplicationClass() { return 'PhabricatorManiphestApplication'; @@ -178,13 +173,7 @@ final class HeraldManiphestTaskAdapter extends HeraldAdapter { pht('Added addresses to cc list.')); break; case self::ACTION_EMAIL: - foreach ($effect->getTarget() as $phid) { - $this->emailPHIDs[] = $phid; - } - $result[] = new HeraldApplyTranscript( - $effect, - true, - pht('Added addresses to email list.')); + $result[] = $this->applyEmailEffect($effect); break; case self::ACTION_FLAG: $result[] = parent::applyFlagEffect( diff --git a/src/applications/phriction/herald/PhrictionDocumentHeraldAdapter.php b/src/applications/phriction/herald/PhrictionDocumentHeraldAdapter.php index 5e01d75258..fb47ea5031 100644 --- a/src/applications/phriction/herald/PhrictionDocumentHeraldAdapter.php +++ b/src/applications/phriction/herald/PhrictionDocumentHeraldAdapter.php @@ -4,7 +4,6 @@ final class PhrictionDocumentHeraldAdapter extends HeraldAdapter { private $document; private $ccPHIDs = array(); - private $emailPHIDs = array(); public function getAdapterApplicationClass() { return 'PhabricatorPhrictionApplication'; @@ -30,15 +29,11 @@ final class PhrictionDocumentHeraldAdapter extends HeraldAdapter { $this->ccPHIDs = $cc_phids; return $this; } + public function getCcPHIDs() { return $this->ccPHIDs; } - public function getEmailPHIDs() { - return $this->emailPHIDs; - } - - public function getAdapterContentName() { return pht('Phriction Documents'); } @@ -143,13 +138,7 @@ final class PhrictionDocumentHeraldAdapter extends HeraldAdapter { $this->getDocument()->getPHID()); break; case self::ACTION_EMAIL: - foreach ($effect->getTarget() as $phid) { - $this->emailPHIDs[] = $phid; - } - $result[] = new HeraldApplyTranscript( - $effect, - true, - pht('Added addresses to email list.')); + $result[] = $this->applyEmailEffect($effect); break; default: $custom_result = parent::handleCustomHeraldEffect($effect);