From 0d99c84bd78e3363ab5f6f194b042e91e7bd6e33 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 31 Mar 2015 16:48:27 -0700 Subject: [PATCH] Modernize email command parsing Summary: Ref T7199. This prepares for an exciting new world of more powerful "!action" commands. In particular: - We parse multiple commands per mail. - We parse command arguments (these are currently not used). - We parse commands at the beginning or end of mail. Additionally: - Do a quick modernization pass on all handlers. - Break legacy compatibility with really hacky Facebook stuff (see T1992). They've theoretically been on notice for a year and a half, and their setup relies on calling very old reply handler APIs directly. - Some of these handlers had some copy/paste fluff. - The Releeph handler is unreachable, but fix it //in theory//. Test Plan: - Sent mail to a file; used "!unsubscribe". - Sent mail to a legalpad document; used "!unsubscribe". - Sent mail to a task; used various "!close", "!claim", "!assign", etc. - Sent mail to a paste. - Sent mail to a revision; used various "!reject", "!claim", etc. - Tried to send mail to a pull request but it's not actually reachable. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T7199 Differential Revision: https://secure.phabricator.com/D12230 --- .../mail/DifferentialReplyHandler.php | 127 ++++++++---------- .../files/mail/FileReplyHandler.php | 33 ++--- .../legalpad/mail/LegalpadReplyHandler.php | 37 +++-- .../maniphest/mail/ManiphestReplyHandler.php | 12 +- .../PhabricatorMetaMTAEmailBodyParser.php | 68 +++++++--- ...bricatorMetaMTAEmailBodyParserTestCase.php | 72 +++++++++- .../paste/mail/PasteReplyHandler.php | 28 ++-- .../mail/ReleephRequestMailReceiver.php | 4 +- .../mail/ReleephRequestReplyHandler.php | 17 ++- 9 files changed, 231 insertions(+), 167 deletions(-) diff --git a/src/applications/differential/mail/DifferentialReplyHandler.php b/src/applications/differential/mail/DifferentialReplyHandler.php index 4a773dd2bc..05a92cd4ff 100644 --- a/src/applications/differential/mail/DifferentialReplyHandler.php +++ b/src/applications/differential/mail/DifferentialReplyHandler.php @@ -1,13 +1,6 @@ receivedMail = $mail; - $this->handleAction($mail->getCleanTextBody(), $mail->getAttachments()); - } + $revision = $this->getMailReceiver(); + $viewer = $this->getActor(); - public function handleAction($body, array $attachments) { - // all commands start with a bang and separated from the body by a newline - // to make sure that actual feedback text couldn't trigger an action. - // unrecognized commands will be parsed as part of the comment. - $command = DifferentialAction::ACTION_COMMENT; - $supported_commands = $this->getSupportedCommands(); - $regex = "/\A\n*!(".implode('|', $supported_commands).")\n*/"; - $matches = array(); - if (preg_match($regex, $body, $matches)) { - $command = $matches[1]; - $body = trim(str_replace('!'.$command, '', $body)); - } - - $actor = $this->getActor(); - if (!$actor) { - throw new Exception('No actor is set for the reply action.'); - } - - switch ($command) { - case 'unsubscribe': - id(new PhabricatorSubscriptionsEditor()) - ->setActor($actor) - ->setObject($this->getMailReceiver()) - ->unsubscribe(array($actor->getPHID())) - ->save(); - // TODO: Send the user a confirmation email? - return null; - } - - $body = $this->enhanceBodyWithAttachments($body, $attachments); + $body_data = $mail->parseBody(); + $body = $body_data['body']; + $body = $this->enhanceBodyWithAttachments($body, $mail->getAttachments()); $xactions = array(); + $content_source = PhabricatorContentSource::newForSource( + PhabricatorContentSource::SOURCE_EMAIL, + array( + 'id' => $mail->getID(), + )); - if ($command && ($command != DifferentialAction::ACTION_COMMENT)) { - $xactions[] = id(new DifferentialTransaction()) - ->setTransactionType(DifferentialTransaction::TYPE_ACTION) - ->setNewValue($command); + $template = id(new DifferentialTransaction()); + + $commands = $body_data['commands']; + foreach ($commands as $command_argv) { + $command = head($command_argv); + switch ($command) { + case 'unsubscribe': + $xactions[] = id(clone $template) + ->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS) + ->setNewValue( + array( + '-' => array($viewer->getPHID()), + )); + break; + case DifferentialAction::ACTION_ACCEPT: + $accept_key = 'differential.enable-email-accept'; + $can_accept = PhabricatorEnv::getEnvConfig($accept_key); + if (!$can_accept) { + throw new Exception( + pht( + 'You can not !accept revisions over email because '. + 'Differential is configured to disallow this.')); + } + // Fall through... + case DifferentialAction::ACTION_REJECT: + case DifferentialAction::ACTION_ABANDON: + case DifferentialAction::ACTION_RECLAIM: + case DifferentialAction::ACTION_RESIGN: + case DifferentialAction::ACTION_RETHINK: + case DifferentialAction::ACTION_CLAIM: + case DifferentialAction::ACTION_REOPEN: + $xactions[] = id(clone $template) + ->setTransactionType(DifferentialTransaction::TYPE_ACTION) + ->setNewValue($command); + break; + } } - if (strlen($body)) { - $xactions[] = id(new DifferentialTransaction()) - ->setTransactionType(PhabricatorTransactions::TYPE_COMMENT) - ->attachComment( - id(new DifferentialTransactionComment()) - ->setContent($body)); - } + $xactions[] = id(clone $template) + ->setTransactionType(PhabricatorTransactions::TYPE_COMMENT) + ->attachComment( + id(new DifferentialTransactionComment()) + ->setContent($body)); $editor = id(new DifferentialTransactionEditor()) - ->setActor($actor) + ->setActor($viewer) + ->setContentSource($content_source) ->setExcludeMailRecipientPHIDs($this->getExcludeMailRecipientPHIDs()) ->setContinueOnMissingFields(true) ->setContinueOnNoEffect(true); - // NOTE: We have to be careful about this because Facebook's - // implementation jumps straight into handleAction() and will not have - // a PhabricatorMetaMTAReceivedMail object. - if ($this->receivedMail) { - $content_source = PhabricatorContentSource::newForSource( - PhabricatorContentSource::SOURCE_EMAIL, - array( - 'id' => $this->receivedMail->getID(), - )); - $editor->setContentSource($content_source); - $editor->setParentMessageID($this->receivedMail->getMessageID()); - } else { - $content_source = PhabricatorContentSource::newForSource( - PhabricatorContentSource::SOURCE_LEGACY, - array()); - $editor->setContentSource($content_source); - } - - $editor->applyTransactions($this->getMailReceiver(), $xactions); + $editor->applyTransactions($revision, $xactions); } } diff --git a/src/applications/files/mail/FileReplyHandler.php b/src/applications/files/mail/FileReplyHandler.php index aafd2fd43b..23a4735e69 100644 --- a/src/applications/files/mail/FileReplyHandler.php +++ b/src/applications/files/mail/FileReplyHandler.php @@ -32,22 +32,23 @@ final class FileReplyHandler extends PhabricatorMailReplyHandler { )); $xactions = array(); - $command = $body_data['body']; - - switch ($command) { - case 'unsubscribe': - $xaction = id(new PhabricatorFileTransaction()) - ->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS) - ->setNewValue(array('-' => array($actor->getPHID()))); - $xactions[] = $xaction; - break; + $commands = $body_data['commands']; + foreach ($commands as $command) { + switch (head($command)) { + case 'unsubscribe': + $xaction = id(new PhabricatorFileTransaction()) + ->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS) + ->setNewValue(array('-' => array($actor->getPHID()))); + $xactions[] = $xaction; + break; + } } $xactions[] = id(new PhabricatorFileTransaction()) ->setTransactionType(PhabricatorTransactions::TYPE_COMMENT) ->attachComment( - id(new PhabricatorFileTransactionComment()) - ->setContent($body)); + id(new PhabricatorFileTransactionComment()) + ->setContent($body)); $editor = id(new PhabricatorFileEditor()) ->setActor($actor) @@ -55,15 +56,7 @@ final class FileReplyHandler extends PhabricatorMailReplyHandler { ->setContinueOnNoEffect(true) ->setIsPreview(false); - try { - $xactions = $editor->applyTransactions($file, $xactions); - } catch (PhabricatorApplicationTransactionNoEffectException $ex) { - // just do nothing, though unclear why you're sending a blank email - return true; - } - - $head_xaction = head($xactions); - return $head_xaction->getID(); + $editor->applyTransactions($file, $xactions); } } diff --git a/src/applications/legalpad/mail/LegalpadReplyHandler.php b/src/applications/legalpad/mail/LegalpadReplyHandler.php index 9dd6cc8890..35558c4ade 100644 --- a/src/applications/legalpad/mail/LegalpadReplyHandler.php +++ b/src/applications/legalpad/mail/LegalpadReplyHandler.php @@ -31,27 +31,28 @@ final class LegalpadReplyHandler extends PhabricatorMailReplyHandler { 'id' => $mail->getID(), )); - $xactions = array(); - $command = $body_data['command']; - switch ($command) { - case 'unsubscribe': - $xaction = id(new LegalpadTransaction()) - ->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS) - ->setNewValue(array('-' => array($actor->getPHID()))); - $xactions[] = $xaction; - break; + $commands = $body_data['commands']; + foreach ($commands as $command) { + switch (head($command)) { + case 'unsubscribe': + $xaction = id(new LegalpadTransaction()) + ->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS) + ->setNewValue(array('-' => array($actor->getPHID()))); + $xactions[] = $xaction; + break; + } } $xactions[] = id(new LegalpadTransaction()) ->setTransactionType(PhabricatorTransactions::TYPE_COMMENT) ->attachComment( id(new LegalpadTransactionComment()) - ->setDocumentID($document->getID()) - ->setLineNumber(0) - ->setLineLength(0) - ->setContent($body)); + ->setDocumentID($document->getID()) + ->setLineNumber(0) + ->setLineLength(0) + ->setContent($body)); $editor = id(new LegalpadDocumentEditor()) ->setActor($actor) @@ -59,15 +60,7 @@ final class LegalpadReplyHandler extends PhabricatorMailReplyHandler { ->setContinueOnNoEffect(true) ->setIsPreview(false); - try { - $xactions = $editor->applyTransactions($document, $xactions); - } catch (PhabricatorApplicationTransactionNoEffectException $ex) { - // just do nothing, though unclear why you're sending a blank email - return true; - } - - $head_xaction = head($xactions); - return $head_xaction->getID(); + $editor->applyTransactions($document, $xactions); } } diff --git a/src/applications/maniphest/mail/ManiphestReplyHandler.php b/src/applications/maniphest/mail/ManiphestReplyHandler.php index 7f74c8a569..e4bfc5c319 100644 --- a/src/applications/maniphest/mail/ManiphestReplyHandler.php +++ b/src/applications/maniphest/mail/ManiphestReplyHandler.php @@ -63,8 +63,16 @@ final class ManiphestReplyHandler extends PhabricatorMailReplyHandler { } else { - $command = $body_data['command']; - $command_value = $body_data['command_value']; + $commands = $body_data['commands']; + + // TODO: Support multiple commands. + if ($commands) { + $command_argv = head($commands); + } else { + $command_argv = array(); + } + $command = idx($command_argv, 0); + $command_value = idx($command_argv, 1); $ttype = PhabricatorTransactions::TYPE_COMMENT; $new_value = null; diff --git a/src/applications/metamta/parser/PhabricatorMetaMTAEmailBodyParser.php b/src/applications/metamta/parser/PhabricatorMetaMTAEmailBodyParser.php index f138ba243a..05133777aa 100644 --- a/src/applications/metamta/parser/PhabricatorMetaMTAEmailBodyParser.php +++ b/src/applications/metamta/parser/PhabricatorMetaMTAEmailBodyParser.php @@ -16,36 +16,66 @@ final class PhabricatorMetaMTAEmailBodyParser { * please, take this task I took; its hard * * This function parses such an email body and returns a dictionary - * containing a clean body text (e.g. "taking this task"), a $command - * (e.g. !claim, !assign) and a $command_value (e.g. "epriestley" in the - * !assign example.) + * containing a clean body text (e.g. "taking this task"), and a list of + * commands. For example, this body above might parse as: * - * @return dict + * array( + * 'body' => 'please, take this task I took; its hard', + * 'commands' => array( + * array('assign', 'epriestley'), + * ), + * ) + * + * @param string Raw mail text body. + * @return dict Parsed body. */ public function parseBody($body) { $body = $this->stripTextBody($body); - $lines = explode("\n", trim($body)); - $first_line = head($lines); - $command = null; - $command_value = null; - $matches = null; - if (preg_match('/^!(\w+)\s*(\S+)?/', $first_line, $matches)) { - $lines = array_slice($lines, 1); - $body = implode("\n", $lines); - $body = trim($body); + $commands = array(); - $command = $matches[1]; - $command_value = idx($matches, 2); - } + $lines = phutil_split_lines($body, $retain_endings = true); + + // We'll match commands at the beginning and end of the mail, but not + // in the middle of the mail body. + list($top_commands, $lines) = $this->stripCommands($lines); + list($end_commands, $lines) = $this->stripCommands(array_reverse($lines)); + $lines = array_reverse($lines); + $commands = array_merge($top_commands, array_reverse($end_commands)); + + $lines = rtrim(implode('', $lines)); return array( - 'body' => $body, - 'command' => $command, - 'command_value' => $command_value, + 'body' => $lines, + 'commands' => $commands, ); } + private function stripCommands(array $lines) { + $saw_command = false; + $commands = array(); + foreach ($lines as $key => $line) { + if (!strlen(trim($line)) && $saw_command) { + unset($lines[$key]); + continue; + } + + $matches = null; + if (!preg_match('/^\s*!(\w+.*$)/', $line, $matches)) { + break; + } + + $arg_str = $matches[1]; + $argv = preg_split('/[,\s]+/', trim($arg_str)); + $commands[] = $argv; + unset($lines[$key]); + + $saw_command = true; + } + + return array($commands, $lines); + } + public function stripTextBody($body) { return trim($this->stripSignature($this->stripQuotedText($body))); } diff --git a/src/applications/metamta/parser/__tests__/PhabricatorMetaMTAEmailBodyParserTestCase.php b/src/applications/metamta/parser/__tests__/PhabricatorMetaMTAEmailBodyParserTestCase.php index 94473f498c..435243edd4 100644 --- a/src/applications/metamta/parser/__tests__/PhabricatorMetaMTAEmailBodyParserTestCase.php +++ b/src/applications/metamta/parser/__tests__/PhabricatorMetaMTAEmailBodyParserTestCase.php @@ -18,16 +18,56 @@ final class PhabricatorMetaMTAEmailBodyParserTestCase $parser = new PhabricatorMetaMTAEmailBodyParser(); $body_data = $parser->parseBody($body); $this->assertEqual('OKAY', $body_data['body']); - $this->assertEqual('whatevs', $body_data['command']); - $this->assertEqual('dude', $body_data['command_value']); + $this->assertEqual( + array( + array('whatevs', 'dude'), + ), + $body_data['commands']); } + $bodies = $this->getEmailBodiesWithPartialCommands(); foreach ($bodies as $body) { $parser = new PhabricatorMetaMTAEmailBodyParser(); $body_data = $parser->parseBody($body); $this->assertEqual('OKAY', $body_data['body']); - $this->assertEqual('whatevs', $body_data['command']); - $this->assertEqual(null, $body_data['command_value']); + $this->assertEqual( + array( + array('whatevs'), + ), + $body_data['commands']); + } + + $bodies = $this->getEmailBodiesWithMultipleCommands(); + foreach ($bodies as $body) { + $parser = new PhabricatorMetaMTAEmailBodyParser(); + $body_data = $parser->parseBody($body); + $this->assertEqual("preface\n\nOKAY", $body_data['body']); + $this->assertEqual( + array( + array('top1'), + array('top2'), + ), + $body_data['commands']); + } + + $bodies = $this->getEmailBodiesWithSplitCommands(); + foreach ($bodies as $body) { + $parser = new PhabricatorMetaMTAEmailBodyParser(); + $body_data = $parser->parseBody($body); + $this->assertEqual('OKAY', $body_data['body']); + $this->assertEqual( + array( + array('cmd1'), + array('cmd2'), + ), + $body_data['commands']); + } + + $bodies = $this->getEmailBodiesWithMiddleCommands(); + foreach ($bodies as $body) { + $parser = new PhabricatorMetaMTAEmailBodyParser(); + $body_data = $parser->parseBody($body); + $this->assertEqual("HEAD\n!cmd2\nTAIL", $body_data['body']); } } @@ -63,6 +103,30 @@ EOEMAIL; return $with_commands; } + private function getEmailBodiesWithMultipleCommands() { + $bodies = $this->getEmailBodies(); + $with_commands = array(); + foreach ($bodies as $body) { + $with_commands[] = "!top1\n\n!top2\n\npreface\n\n".$body; + } + return $with_commands; + } + + private function getEmailBodiesWithSplitCommands() { + $with_split = array(); + $with_split[] = "!cmd1\n!cmd2\nOKAY"; + $with_split[] = "!cmd1\nOKAY\n!cmd2"; + $with_split[] = "OKAY\n!cmd1\n!cmd2"; + return $with_split; + } + + private function getEmailBodiesWithMiddleCommands() { + $with_middle = array(); + $with_middle[] = "!cmd1\nHEAD\n!cmd2\nTAIL\n!cmd3"; + $with_middle[] = "!cmd1\nHEAD\n!cmd2\nTAIL"; + $with_middle[] = "HEAD\n!cmd2\nTAIL\n!cmd3"; + return $with_middle; + } private function getEmailBodies() { $trailing_space = ' '; diff --git a/src/applications/paste/mail/PasteReplyHandler.php b/src/applications/paste/mail/PasteReplyHandler.php index 1cc5e91d70..523e2ee68d 100644 --- a/src/applications/paste/mail/PasteReplyHandler.php +++ b/src/applications/paste/mail/PasteReplyHandler.php @@ -35,15 +35,17 @@ final class PasteReplyHandler extends PhabricatorMailReplyHandler { $first_line = head($lines); $xactions = array(); - $command = $body_data['command']; - switch ($command) { - case 'unsubscribe': - $xaction = id(new PhabricatorPasteTransaction()) - ->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS) - ->setNewValue(array('-' => array($actor->getPHID()))); - $xactions[] = $xaction; - break; + $commands = $body_data['commands']; + foreach ($commands as $command) { + switch (head($command)) { + case 'unsubscribe': + $xaction = id(new PhabricatorPasteTransaction()) + ->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS) + ->setNewValue(array('-' => array($actor->getPHID()))); + $xactions[] = $xaction; + break; + } } $xactions[] = id(new PhabricatorPasteTransaction()) @@ -58,15 +60,7 @@ final class PasteReplyHandler extends PhabricatorMailReplyHandler { ->setContinueOnNoEffect(true) ->setIsPreview(false); - try { - $xactions = $editor->applyTransactions($paste, $xactions); - } catch (PhabricatorApplicationTransactionNoEffectException $ex) { - // just do nothing, though unclear why you're sending a blank email - return true; - } - - $head_xaction = head($xactions); - return $head_xaction->getID(); + $editor->applyTransactions($paste, $xactions); } } diff --git a/src/applications/releeph/mail/ReleephRequestMailReceiver.php b/src/applications/releeph/mail/ReleephRequestMailReceiver.php index 2c6646f6e6..6d0e06c5c9 100644 --- a/src/applications/releeph/mail/ReleephRequestMailReceiver.php +++ b/src/applications/releeph/mail/ReleephRequestMailReceiver.php @@ -8,11 +8,11 @@ final class ReleephRequestMailReceiver extends PhabricatorObjectMailReceiver { } protected function getObjectPattern() { - return 'RQ[1-9]\d*'; + return 'Y[1-9]\d*'; } protected function loadObject($pattern, PhabricatorUser $viewer) { - $id = (int)substr($pattern, 2); + $id = (int)substr($pattern, 1); return id(new ReleephRequestQuery()) ->setViewer($viewer) diff --git a/src/applications/releeph/mail/ReleephRequestReplyHandler.php b/src/applications/releeph/mail/ReleephRequestReplyHandler.php index 4c36078ea5..0a3fa401f4 100644 --- a/src/applications/releeph/mail/ReleephRequestReplyHandler.php +++ b/src/applications/releeph/mail/ReleephRequestReplyHandler.php @@ -10,11 +10,11 @@ final class ReleephRequestReplyHandler extends PhabricatorMailReplyHandler { public function getPrivateReplyHandlerEmailAddress( PhabricatorObjectHandle $handle) { - return $this->getDefaultPrivateReplyHandlerEmailAddress($handle, 'RERQ'); + return $this->getDefaultPrivateReplyHandlerEmailAddress($handle, 'Y'); } public function getPublicReplyHandlerEmailAddress() { - return $this->getDefaultPublicReplyHandlerEmailAddress('RERQ'); + return $this->getDefaultPublicReplyHandlerEmailAddress('Y'); } protected function receiveEmail(PhabricatorMetaMTAReceivedMail $mail) { @@ -27,11 +27,6 @@ final class ReleephRequestReplyHandler extends PhabricatorMailReplyHandler { 'id' => $mail->getID(), )); - $editor = id(new ReleephRequestTransactionalEditor()) - ->setActor($user) - ->setContentSource($content_source) - ->setParentMessageID($mail->getMessageID()); - $body = $mail->getCleanTextBody(); $xactions = array(); @@ -39,9 +34,13 @@ final class ReleephRequestReplyHandler extends PhabricatorMailReplyHandler { ->setTransactionType(PhabricatorTransactions::TYPE_COMMENT) ->attachComment($body); - $editor->applyTransactions($rq, $xactions); + $editor = id(new ReleephRequestTransactionalEditor()) + ->setActor($user) + ->setContentSource($content_source) + ->setContinueOnNoEffect(true) + ->setParentMessageID($mail->getMessageID()); - return $rq; + $editor->applyTransactions($rq, $xactions); } }