From 6f59b2ab8751c152347b0c28ffaf8e63ea9db5ef Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 1 Apr 2015 08:40:42 -0700 Subject: [PATCH] Move Maniphest to modular mail commands Summary: Ref T7199. This fully modularizes mail command handling in Maniphest. I had to add a couple of minor not-totally-solid-feeling tricks to deal with the "create" case, but they feel not-too-bad, and a million times better than what came before. Test Plan: Used all commands with `receive-test`. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T7199 Differential Revision: https://secure.phabricator.com/D12240 --- src/__phutil_library_map__.php | 10 +- .../command/ManiphestAssignEmailCommand.php | 42 +++++ .../command/ManiphestClaimEmailCommand.php | 25 +++ .../command/ManiphestCloseEmailCommand.php | 25 +++ .../command/ManiphestEmailCommand.php | 11 ++ .../maniphest/mail/ManiphestReplyHandler.php | 144 ++---------------- ...atorApplicationTransactionReplyHandler.php | 40 +++-- 7 files changed, 155 insertions(+), 142 deletions(-) create mode 100644 src/applications/maniphest/command/ManiphestAssignEmailCommand.php create mode 100644 src/applications/maniphest/command/ManiphestClaimEmailCommand.php create mode 100644 src/applications/maniphest/command/ManiphestCloseEmailCommand.php create mode 100644 src/applications/maniphest/command/ManiphestEmailCommand.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 873e2e0414..5ac246867d 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1000,8 +1000,11 @@ phutil_register_library_map(array( 'MacroConduitAPIMethod' => 'applications/macro/conduit/MacroConduitAPIMethod.php', 'MacroCreateMemeConduitAPIMethod' => 'applications/macro/conduit/MacroCreateMemeConduitAPIMethod.php', 'MacroQueryConduitAPIMethod' => 'applications/macro/conduit/MacroQueryConduitAPIMethod.php', + 'ManiphestAssignEmailCommand' => 'applications/maniphest/command/ManiphestAssignEmailCommand.php', 'ManiphestBatchEditController' => 'applications/maniphest/controller/ManiphestBatchEditController.php', 'ManiphestBulkEditCapability' => 'applications/maniphest/capability/ManiphestBulkEditCapability.php', + 'ManiphestClaimEmailCommand' => 'applications/maniphest/command/ManiphestClaimEmailCommand.php', + 'ManiphestCloseEmailCommand' => 'applications/maniphest/command/ManiphestCloseEmailCommand.php', 'ManiphestConduitAPIMethod' => 'applications/maniphest/conduit/ManiphestConduitAPIMethod.php', 'ManiphestConfiguredCustomField' => 'applications/maniphest/field/ManiphestConfiguredCustomField.php', 'ManiphestConstants' => 'applications/maniphest/constants/ManiphestConstants.php', @@ -1022,6 +1025,7 @@ phutil_register_library_map(array( 'ManiphestEditPriorityCapability' => 'applications/maniphest/capability/ManiphestEditPriorityCapability.php', 'ManiphestEditProjectsCapability' => 'applications/maniphest/capability/ManiphestEditProjectsCapability.php', 'ManiphestEditStatusCapability' => 'applications/maniphest/capability/ManiphestEditStatusCapability.php', + 'ManiphestEmailCommand' => 'applications/maniphest/command/ManiphestEmailCommand.php', 'ManiphestExcelDefaultFormat' => 'applications/maniphest/export/ManiphestExcelDefaultFormat.php', 'ManiphestExcelFormat' => 'applications/maniphest/export/ManiphestExcelFormat.php', 'ManiphestExportController' => 'applications/maniphest/controller/ManiphestExportController.php', @@ -4241,8 +4245,11 @@ phutil_register_library_map(array( 'MacroConduitAPIMethod' => 'ConduitAPIMethod', 'MacroCreateMemeConduitAPIMethod' => 'MacroConduitAPIMethod', 'MacroQueryConduitAPIMethod' => 'MacroConduitAPIMethod', + 'ManiphestAssignEmailCommand' => 'ManiphestEmailCommand', 'ManiphestBatchEditController' => 'ManiphestController', 'ManiphestBulkEditCapability' => 'PhabricatorPolicyCapability', + 'ManiphestClaimEmailCommand' => 'ManiphestEmailCommand', + 'ManiphestCloseEmailCommand' => 'ManiphestEmailCommand', 'ManiphestConduitAPIMethod' => 'ConduitAPIMethod', 'ManiphestConfiguredCustomField' => array( 'ManiphestCustomField', @@ -4265,6 +4272,7 @@ phutil_register_library_map(array( 'ManiphestEditPriorityCapability' => 'PhabricatorPolicyCapability', 'ManiphestEditProjectsCapability' => 'PhabricatorPolicyCapability', 'ManiphestEditStatusCapability' => 'PhabricatorPolicyCapability', + 'ManiphestEmailCommand' => 'MetaMTAEmailTransactionCommand', 'ManiphestExcelDefaultFormat' => 'ManiphestExcelFormat', 'ManiphestExportController' => 'ManiphestController', 'ManiphestGetTaskTransactionsConduitAPIMethod' => 'ManiphestConduitAPIMethod', @@ -4275,7 +4283,7 @@ phutil_register_library_map(array( 'ManiphestQueryConduitAPIMethod' => 'ManiphestConduitAPIMethod', 'ManiphestQueryStatusesConduitAPIMethod' => 'ManiphestConduitAPIMethod', 'ManiphestRemarkupRule' => 'PhabricatorObjectRemarkupRule', - 'ManiphestReplyHandler' => 'PhabricatorMailReplyHandler', + 'ManiphestReplyHandler' => 'PhabricatorApplicationTransactionReplyHandler', 'ManiphestReportController' => 'ManiphestController', 'ManiphestSchemaSpec' => 'PhabricatorConfigSchemaSpec', 'ManiphestSearchIndexer' => 'PhabricatorSearchDocumentIndexer', diff --git a/src/applications/maniphest/command/ManiphestAssignEmailCommand.php b/src/applications/maniphest/command/ManiphestAssignEmailCommand.php new file mode 100644 index 0000000000..9721a4ea36 --- /dev/null +++ b/src/applications/maniphest/command/ManiphestAssignEmailCommand.php @@ -0,0 +1,42 @@ +setViewer($viewer) + ->withUsernames(array($assign_to)) + ->executeOne(); + if ($assign_user) { + $assign_phid = $assign_user->getPHID(); + } + } + + // Treat bad "!assign" like "!claim". + if (!$assign_phid) { + $assign_phid = $viewer->getPHID(); + } + + $xactions[] = $object->getApplicationTransactionTemplate() + ->setTransactionType(ManiphestTransaction::TYPE_OWNER) + ->setNewValue($assign_phid); + + return $xactions; + } + +} diff --git a/src/applications/maniphest/command/ManiphestClaimEmailCommand.php b/src/applications/maniphest/command/ManiphestClaimEmailCommand.php new file mode 100644 index 0000000000..759c5835d1 --- /dev/null +++ b/src/applications/maniphest/command/ManiphestClaimEmailCommand.php @@ -0,0 +1,25 @@ +getApplicationTransactionTemplate() + ->setTransactionType(ManiphestTransaction::TYPE_OWNER) + ->setNewValue($viewer->getPHID()); + + return $xactions; + } + +} diff --git a/src/applications/maniphest/command/ManiphestCloseEmailCommand.php b/src/applications/maniphest/command/ManiphestCloseEmailCommand.php new file mode 100644 index 0000000000..2750dcadbb --- /dev/null +++ b/src/applications/maniphest/command/ManiphestCloseEmailCommand.php @@ -0,0 +1,25 @@ +getApplicationTransactionTemplate() + ->setTransactionType(ManiphestTransaction::TYPE_STATUS) + ->setNewValue(ManiphestTaskStatus::getDefaultClosedStatus()); + + return $xactions; + } + +} diff --git a/src/applications/maniphest/command/ManiphestEmailCommand.php b/src/applications/maniphest/command/ManiphestEmailCommand.php new file mode 100644 index 0000000000..5826135f95 --- /dev/null +++ b/src/applications/maniphest/command/ManiphestEmailCommand.php @@ -0,0 +1,11 @@ +getDefaultPrivateReplyHandlerEmailAddress($handle, 'T'); + public function getObjectPrefix() { + return 'T'; } - public function getPublicReplyHandlerEmailAddress() { - return $this->getDefaultPublicReplyHandlerEmailAddress('T'); - } + protected function didReceiveMail( + PhabricatorMetaMTAReceivedMail $mail, + $body) { - protected function receiveEmail(PhabricatorMetaMTAReceivedMail $mail) { - // NOTE: We'll drop in here on both the "reply to a task" and "create a - // new task" workflows! Make sure you test both if you make changes! - $task = $this->getMailReceiver(); - $viewer = $this->getActor(); - - $is_new_task = !$task->getID(); - - $body_data = $mail->parseBody(); - $body = $body_data['body']; - $body = $this->enhanceBodyWithAttachments($body, $mail->getAttachments()); - - $content_source = PhabricatorContentSource::newForSource( - PhabricatorContentSource::SOURCE_EMAIL, - array( - 'id' => $mail->getID(), - )); - - $template = new ManiphestTransaction(); + $object = $this->getMailReceiver(); + $is_new = !$object->getID(); $xactions = array(); - if ($is_new_task) { - $xactions[] = id(clone $template) + + if ($is_new) { + $xactions[] = $object->getApplicationTransactionTemplate() ->setTransactionType(ManiphestTransaction::TYPE_TITLE) ->setNewValue(nonempty($mail->getSubject(), pht('Untitled Task'))); - $xactions[] = id(clone $template) + $xactions[] = $object->getApplicationTransactionTemplate() ->setTransactionType(ManiphestTransaction::TYPE_DESCRIPTION) ->setNewValue($body); - } else { - $xactions[] = id(clone $template) - ->setTransactionType(PhabricatorTransactions::TYPE_COMMENT) - ->attachComment( - id(new ManiphestTransactionComment()) - ->setContent($body)); } - $commands = $body_data['commands']; - foreach ($commands as $command_argv) { - $command = head($command_argv); - $args = array_slice($command_argv, 1); - switch ($command) { - case 'close': - $xactions[] = id(clone $template) - ->setTransactionType(ManiphestTransaction::TYPE_STATUS) - ->setNewValue(ManiphestTaskStatus::getDefaultClosedStatus()); - break; - case 'claim': - $xactions[] = id(clone $template) - ->setTransactionType(ManiphestTransaction::TYPE_OWNER) - ->setNewValue($viewer->getPHID()); - break; - case 'assign': - $assign_to = head($args); - if ($assign_to) { - $assign_user = id(new PhabricatorPeopleQuery()) - ->setViewer($viewer) - ->withUsernames(array($assign_to)) - ->executeOne(); - if ($assign_user) { - $assign_phid = $assign_user->getPHID(); - } - } - - // Treat bad "!assign" like "!claim". - if (!$assign_phid) { - $assign_phid = $viewer->getPHID(); - } - - $xactions[] = id(clone $template) - ->setTransactionType(ManiphestTransaction::TYPE_OWNER) - ->setNewValue($assign_phid); - break; - case 'unsubscribe': - $xactions[] = id(clone $template) - ->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS) - ->setNewValue( - array( - '-' => array($viewer->getPHID()), - )); - break; - } - } - - $ccs = $mail->loadCCPHIDs(); - if ($ccs) { - $xactions[] = id(clone $template) - ->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS) - ->setNewValue( - array( - '+' => array($viewer->getPHID()), - )); - } - - $event = new PhabricatorEvent( - PhabricatorEventType::TYPE_MANIPHEST_WILLEDITTASK, - array( - 'task' => $task, - 'mail' => $mail, - 'new' => $is_new_task, - 'transactions' => $xactions, - )); - $event->setUser($viewer); - PhutilEventEngine::dispatchEvent($event); - - $task = $event->getValue('task'); - $xactions = $event->getValue('transactions'); - - $editor = id(new ManiphestTransactionEditor()) - ->setActor($viewer) - ->setParentMessageID($mail->getMessageID()) - ->setExcludeMailRecipientPHIDs($this->getExcludeMailRecipientPHIDs()) - ->setContinueOnNoEffect(true) - ->setContinueOnMissingFields(true) - ->setContentSource($content_source); - - if ($this->getApplicationEmail()) { - $editor->setApplicationEmail($this->getApplicationEmail()); - } - - $editor->applyTransactions($task, $xactions); - - $event = new PhabricatorEvent( - PhabricatorEventType::TYPE_MANIPHEST_DIDEDITTASK, - array( - 'task' => $task, - 'new' => $is_new_task, - 'transactions' => $xactions, - )); - $event->setUser($viewer); - PhutilEventEngine::dispatchEvent($event); + return $xactions; } + } diff --git a/src/applications/transactions/replyhandler/PhabricatorApplicationTransactionReplyHandler.php b/src/applications/transactions/replyhandler/PhabricatorApplicationTransactionReplyHandler.php index 2eb8955f91..cc4c1606f2 100644 --- a/src/applications/transactions/replyhandler/PhabricatorApplicationTransactionReplyHandler.php +++ b/src/applications/transactions/replyhandler/PhabricatorApplicationTransactionReplyHandler.php @@ -43,15 +43,25 @@ abstract class PhabricatorApplicationTransactionReplyHandler return $this->getMailReceiver()->getApplicationTransactionTemplate(); } + protected function didReceiveMail( + PhabricatorMetaMTAReceivedMail $mail, + $body) { + return array(); + } + + protected function shouldCreateCommentFromMailBody() { + return (bool)$this->getMailReceiver()->getID(); + } + final protected function receiveEmail(PhabricatorMetaMTAReceivedMail $mail) { $viewer = $this->getActor(); $object = $this->getMailReceiver(); $body_data = $mail->parseBody(); + $body = $body_data['body']; + $body = $this->enhanceBodyWithAttachments($body, $mail->getAttachments()); - $xactions = $this->processMailCommands( - $mail, - $body_data['commands']); + $xactions = $this->didReceiveMail($mail, $body); // If this object is subscribable, subscribe all the users who were // CC'd on the message. @@ -67,17 +77,23 @@ abstract class PhabricatorApplicationTransactionReplyHandler } } - $body = $body_data['body']; - $body = $this->enhanceBodyWithAttachments($body, $mail->getAttachments()); + $command_xactions = $this->processMailCommands( + $mail, + $body_data['commands']); + foreach ($command_xactions as $xaction) { + $xactions[] = $xaction; + } - $comment = $this - ->newTransaction() - ->getApplicationTransactionCommentObject() - ->setContent($body); + if ($this->shouldCreateCommentFromMailBody()) { + $comment = $this + ->newTransaction() + ->getApplicationTransactionCommentObject() + ->setContent($body); - $xactions[] = $this->newTransaction() - ->setTransactionType(PhabricatorTransactions::TYPE_COMMENT) - ->attachComment($comment); + $xactions[] = $this->newTransaction() + ->setTransactionType(PhabricatorTransactions::TYPE_COMMENT) + ->attachComment($comment); + } $target = $object->getApplicationTransactionObject();