From ab8f7907dee8ca585f95ab510ccbe3bc97c8722e Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Thu, 29 Jan 2015 14:15:38 -0800 Subject: [PATCH] Herald - add support for application emails. Summary: Fixes T5039. The trick / possibly lame part here is we only match 1 application email and its undefined which one. e.g. if a user emails us at address x, y, and z only one of those will pick up the mail. Ergo, don't let users define non-sensical herald conditions like "matches all". Also document what I think was non-intuitive about the code with an inline comment; we have to return an array with just a phid from an object and out of context it feels very "what the...???" Note this needs to be deployed to other applications still, but I think its okay to close T5039 aggressively here since its done from a user story perspective. Test Plan: set up a herald rule to flag tasks created as blue via app email x. sent an email to x via `bin/mail receive-test` and verified the task had the blue flag Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin, epriestley Maniphest Tasks: T5039 Differential Revision: https://secure.phabricator.com/D11564 --- resources/celerity/map.php | 22 ++++++------ .../herald/adapter/HeraldAdapter.php | 35 +++++++++++++++++-- .../adapter/HeraldManiphestTaskAdapter.php | 1 + .../controller/HeraldRuleController.php | 1 + .../mail/ManiphestCreateMailReceiver.php | 1 + .../maniphest/mail/ManiphestReplyHandler.php | 1 + .../PhabricatorMailReplyHandler.php | 11 ++++++ ...habricatorApplicationTransactionEditor.php | 12 +++++++ .../js/application/herald/HeraldRuleEditor.js | 1 + 9 files changed, 72 insertions(+), 13 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index b7cf8e9ed3..716ff1e61f 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -378,7 +378,7 @@ return array( 'rsrc/js/application/doorkeeper/behavior-doorkeeper-tag.js' => 'e5822781', 'rsrc/js/application/files/behavior-icon-composer.js' => '8ef9ab58', 'rsrc/js/application/files/behavior-launch-icon-composer.js' => '48086888', - 'rsrc/js/application/herald/HeraldRuleEditor.js' => '335fd41f', + 'rsrc/js/application/herald/HeraldRuleEditor.js' => '6e2de6f2', 'rsrc/js/application/herald/PathTypeahead.js' => 'f7fc67ec', 'rsrc/js/application/herald/herald-rule-editor.js' => '7ebaeed3', 'rsrc/js/application/maniphest/behavior-batch-editor.js' => 'f24f3253', @@ -530,7 +530,7 @@ return array( 'global-drag-and-drop-css' => '697324ad', 'harbormaster-css' => '49d64eb4', 'herald-css' => '826075fa', - 'herald-rule-editor' => '335fd41f', + 'herald-rule-editor' => '6e2de6f2', 'herald-test-css' => '778b008e', 'homepage-panel-css' => 'e34bf140', 'inline-comment-summary-css' => '8cfd34e8', @@ -1028,15 +1028,6 @@ return array( 'javelin-behavior-device', 'phabricator-title', ), - '335fd41f' => array( - 'multirow-row-manager', - 'javelin-install', - 'javelin-util', - 'javelin-dom', - 'javelin-stratcom', - 'javelin-json', - 'phabricator-prefab', - ), '3ab51e2c' => array( 'javelin-behavior', 'javelin-behavior-device', @@ -1292,6 +1283,15 @@ return array( 'javelin-typeahead', 'javelin-uri', ), + '6e2de6f2' => array( + 'multirow-row-manager', + 'javelin-install', + 'javelin-util', + 'javelin-dom', + 'javelin-stratcom', + 'javelin-json', + 'phabricator-prefab', + ), '6e8cefa4' => array( 'javelin-install', 'javelin-dom', diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php index 9c56a6001f..ecacffcf6c 100644 --- a/src/applications/herald/adapter/HeraldAdapter.php +++ b/src/applications/herald/adapter/HeraldAdapter.php @@ -39,6 +39,7 @@ abstract class HeraldAdapter { const FIELD_AUTHOR_RAW = 'author-raw'; const FIELD_COMMITTER_RAW = 'committer-raw'; const FIELD_IS_NEW_OBJECT = 'new-object'; + const FIELD_APPLICATION_EMAIL = 'applicaton-email'; const FIELD_TASK_PRIORITY = 'taskpriority'; const FIELD_TASK_STATUS = 'taskstatus'; const FIELD_ARCANIST_PROJECT = 'arcanist-project'; @@ -98,11 +99,13 @@ abstract class HeraldAdapter { const VALUE_BUILD_PLAN = 'buildplan'; const VALUE_TASK_PRIORITY = 'taskpriority'; const VALUE_TASK_STATUS = 'taskstatus'; - const VALUE_ARCANIST_PROJECT = 'arcanistprojects'; - const VALUE_LEGAL_DOCUMENTS = 'legaldocuments'; + const VALUE_ARCANIST_PROJECT = 'arcanistprojects'; + const VALUE_LEGAL_DOCUMENTS = 'legaldocuments'; + const VALUE_APPLICATION_EMAIL = 'applicationemail'; private $contentSource; private $isNewObject; + private $applicationEmail; private $customFields = false; private $customActions = null; private $queuedTransactions = array(); @@ -156,6 +159,16 @@ abstract class HeraldAdapter { return $this; } + public function setApplicationEmail( + PhabricatorMetaMTAApplicationEmail $email) { + $this->applicationEmail = $email; + return $this; + } + + public function getApplicationEmail() { + return $this->applicationEmail; + } + abstract public function getPHID(); abstract public function getHeraldName(); @@ -169,6 +182,14 @@ abstract class HeraldAdapter { return true; case self::FIELD_IS_NEW_OBJECT: return $this->getIsNewObject(); + case self::FIELD_APPLICATION_EMAIL: + $value = array(); + // while there is only one match by implementation, we do set + // comparisons on phids, so return an array with just the phid + if ($this->getApplicationEmail()) { + $value[] = $this->getApplicationEmail()->getPHID(); + } + return $value; default: if ($this->isHeraldCustomKey($field_name)) { return $this->getCustomFieldValue($field_name); @@ -312,6 +333,7 @@ abstract class HeraldAdapter { self::FIELD_AUTHOR_RAW => pht('Raw author name'), self::FIELD_COMMITTER_RAW => pht('Raw committer name'), self::FIELD_IS_NEW_OBJECT => pht('Is newly created?'), + self::FIELD_APPLICATION_EMAIL => pht('Receiving email address'), self::FIELD_TASK_PRIORITY => pht('Task priority'), self::FIELD_TASK_STATUS => pht('Task status'), self::FIELD_ARCANIST_PROJECT => pht('Arcanist Project'), @@ -401,6 +423,13 @@ abstract class HeraldAdapter { self::CONDITION_EXISTS, self::CONDITION_NOT_EXISTS, ); + case self::FIELD_APPLICATION_EMAIL: + return array( + self::CONDITION_INCLUDE_ANY, + self::CONDITION_INCLUDE_NONE, + self::CONDITION_EXISTS, + self::CONDITION_NOT_EXISTS, + ); case self::FIELD_DIFF_FILE: case self::FIELD_BRANCHES: return array( @@ -874,6 +903,8 @@ abstract class HeraldAdapter { return self::VALUE_PROJECT; case self::FIELD_REVIEWERS: return self::VALUE_USER_OR_PROJECT; + case self::FIELD_APPLICATION_EMAIL: + return self::VALUE_APPLICATION_EMAIL; default: return self::VALUE_USER; } diff --git a/src/applications/herald/adapter/HeraldManiphestTaskAdapter.php b/src/applications/herald/adapter/HeraldManiphestTaskAdapter.php index 385c8fab9c..6f9b1db9b8 100644 --- a/src/applications/herald/adapter/HeraldManiphestTaskAdapter.php +++ b/src/applications/herald/adapter/HeraldManiphestTaskAdapter.php @@ -91,6 +91,7 @@ final class HeraldManiphestTaskAdapter extends HeraldAdapter { self::FIELD_TASK_PRIORITY, self::FIELD_TASK_STATUS, self::FIELD_IS_NEW_OBJECT, + self::FIELD_APPLICATION_EMAIL, ), parent::getFields()); } diff --git a/src/applications/herald/controller/HeraldRuleController.php b/src/applications/herald/controller/HeraldRuleController.php index 11bfd7fc8a..27ace9dd2f 100644 --- a/src/applications/herald/controller/HeraldRuleController.php +++ b/src/applications/herald/controller/HeraldRuleController.php @@ -602,6 +602,7 @@ final class HeraldRuleController extends HeraldController { 'user' => new PhabricatorPeopleDatasource(), 'email' => new PhabricatorMetaMTAMailableDatasource(), 'userorproject' => new PhabricatorProjectOrUserDatasource(), + 'applicationemail' => new PhabricatorMetaMTAApplicationEmailDatasource(), ); foreach ($sources as $key => $source) { diff --git a/src/applications/maniphest/mail/ManiphestCreateMailReceiver.php b/src/applications/maniphest/mail/ManiphestCreateMailReceiver.php index fecfca9410..bea3a4f24e 100644 --- a/src/applications/maniphest/mail/ManiphestCreateMailReceiver.php +++ b/src/applications/maniphest/mail/ManiphestCreateMailReceiver.php @@ -41,6 +41,7 @@ final class ManiphestCreateMailReceiver extends PhabricatorMailReceiver { $handler->setActor($sender); $handler->setExcludeMailRecipientPHIDs( $mail->loadExcludeMailRecipientPHIDs()); + $handler->setApplicationEmail($this->getApplicationEmail()); $handler->processEmail($mail); $mail->setRelatedPHID($task->getPHID()); diff --git a/src/applications/maniphest/mail/ManiphestReplyHandler.php b/src/applications/maniphest/mail/ManiphestReplyHandler.php index 698a02403c..7c06804d9e 100644 --- a/src/applications/maniphest/mail/ManiphestReplyHandler.php +++ b/src/applications/maniphest/mail/ManiphestReplyHandler.php @@ -171,6 +171,7 @@ final class ManiphestReplyHandler extends PhabricatorMailReplyHandler { ->setContinueOnNoEffect(true) ->setContinueOnMissingFields(true) ->setContentSource($content_source) + ->setApplicationEmail($this->getApplicationEmail()) ->applyTransactions($task, $xactions); $event = new PhabricatorEvent( diff --git a/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php b/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php index 582e48d7f6..353875a6cd 100644 --- a/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php +++ b/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php @@ -3,6 +3,7 @@ abstract class PhabricatorMailReplyHandler { private $mailReceiver; + private $applicationEmail; private $actor; private $excludePHIDs = array(); @@ -16,6 +17,16 @@ abstract class PhabricatorMailReplyHandler { return $this->mailReceiver; } + public function setApplicationEmail( + PhabricatorMetaMTAApplicationEmail $email) { + $this->applicationEmail = $email; + return $this; + } + + public function getApplicationEmail() { + return $this->applicationEmail; + } + final public function setActor(PhabricatorUser $actor) { $this->actor = $actor; return $this; diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index ee1b9f4e7c..dcde28f19e 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -22,6 +22,7 @@ abstract class PhabricatorApplicationTransactionEditor private $heraldTranscript; private $subscribers; private $unmentionablePHIDMap = array(); + private $applicationEmail; private $isPreview; private $isHeraldEditor; @@ -185,6 +186,16 @@ abstract class PhabricatorApplicationTransactionEditor return $this->unmentionablePHIDMap; } + public function setApplicationEmail( + PhabricatorMetaMTAApplicationEmail $email) { + $this->applicationEmail = $email; + return $this; + } + + public function getApplicationEmail() { + return $this->applicationEmail; + } + public function getTransactionTypes() { $types = array(); @@ -2427,6 +2438,7 @@ abstract class PhabricatorApplicationTransactionEditor $adapter = $this->buildHeraldAdapter($object, $xactions); $adapter->setContentSource($this->getContentSource()); $adapter->setIsNewObject($this->getIsNewObject()); + $adapter->setApplicationEmail($this->getApplicationEmail()); $xscript = HeraldEngine::loadAndApplyRules($adapter); $this->setHeraldAdapter($adapter); diff --git a/webroot/rsrc/js/application/herald/HeraldRuleEditor.js b/webroot/rsrc/js/application/herald/HeraldRuleEditor.js index 30f34d27b3..36f28bc865 100644 --- a/webroot/rsrc/js/application/herald/HeraldRuleEditor.js +++ b/webroot/rsrc/js/application/herald/HeraldRuleEditor.js @@ -222,6 +222,7 @@ JX.install('HeraldRuleEditor', { case 'taskstatus': case 'arcanistprojects': case 'legaldocuments': + case 'applicationemail': var tokenizer = this._newTokenizer(type); input = tokenizer[0]; get_fn = tokenizer[1];