From d0127f95e541a5c987bc24757d65b831274dd3e1 Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Mon, 14 Oct 2013 12:29:41 -0700 Subject: [PATCH] Maniphest - add support for !assign command Summary: also try to centralize some of the command parsing logic. note that differential is still an exception here. it uses a whitelist-style regex. i think long-term we should have this for every app but changing it seemed too big for this diff. Fixes T3937. Test Plan: echo '!assign btrahan' | ./bin/mail receive-test --as xerxes --to T22 ; echo '!claim' | ./bin/mail receive-test --as xerxes --to T22 unit tests passed, though my new one is silly Reviewers: epriestley Reviewed By: epriestley CC: Korvin, epriestley, aran Maniphest Tasks: T3937 Differential Revision: https://secure.phabricator.com/D7307 --- .../mail/ConpherenceReplyHandler.php | 1 - .../files/mail/FileReplyHandler.php | 17 ++----- .../legalpad/mail/LegalpadReplyHandler.php | 16 ++----- .../maniphest/mail/ManiphestReplyHandler.php | 34 ++++++++------ .../PhabricatorMetaMTAEmailBodyParser.php | 45 ++++++++++++++++++- ...bricatorMetaMTAEmailBodyParserTestCase.php | 38 ++++++++++++++++ .../PhabricatorMailReplyHandler.php | 2 +- .../PhabricatorMetaMTAReceivedMail.php | 9 +++- .../paste/mail/PasteReplyHandler.php | 14 ++---- 9 files changed, 120 insertions(+), 56 deletions(-) diff --git a/src/applications/conpherence/mail/ConpherenceReplyHandler.php b/src/applications/conpherence/mail/ConpherenceReplyHandler.php index d3bf511624..02019d53ec 100644 --- a/src/applications/conpherence/mail/ConpherenceReplyHandler.php +++ b/src/applications/conpherence/mail/ConpherenceReplyHandler.php @@ -69,7 +69,6 @@ final class ConpherenceReplyHandler extends PhabricatorMailReplyHandler { ->setParentMessageID($mail->getMessageID()); $body = $mail->getCleanTextBody(); - $body = trim($body); $file_phids = $mail->getAttachments(); $body = $this->enhanceBodyWithAttachments( $body, diff --git a/src/applications/files/mail/FileReplyHandler.php b/src/applications/files/mail/FileReplyHandler.php index 2f77c815a7..077b9d121e 100644 --- a/src/applications/files/mail/FileReplyHandler.php +++ b/src/applications/files/mail/FileReplyHandler.php @@ -32,8 +32,8 @@ final class FileReplyHandler extends PhabricatorMailReplyHandler { $actor = $this->getActor(); $file = $this->getMailReceiver(); - $body = $mail->getCleanTextBody(); - $body = trim($body); + $body_data = $mail->parseBody(); + $body = $body_data['body']; $body = $this->enhanceBodyWithAttachments($body, $mail->getAttachments()); $content_source = PhabricatorContentSource::newForSource( @@ -42,19 +42,8 @@ final class FileReplyHandler extends PhabricatorMailReplyHandler { 'id' => $mail->getID(), )); - $lines = explode("\n", trim($body)); - $first_line = head($lines); - $xactions = array(); - $command = null; - $matches = null; - if (preg_match('/^!(\w+)/', $first_line, $matches)) { - $lines = array_slice($lines, 1); - $body = implode("\n", $lines); - $body = trim($body); - - $command = $matches[1]; - } + $command = $body_data['body']; switch ($command) { case 'unsubscribe': diff --git a/src/applications/legalpad/mail/LegalpadReplyHandler.php b/src/applications/legalpad/mail/LegalpadReplyHandler.php index 30befc3316..7ae85873af 100644 --- a/src/applications/legalpad/mail/LegalpadReplyHandler.php +++ b/src/applications/legalpad/mail/LegalpadReplyHandler.php @@ -37,8 +37,8 @@ final class LegalpadReplyHandler extends PhabricatorMailReplyHandler { $actor = $this->getActor(); $document = $this->getMailReceiver(); - $body = $mail->getCleanTextBody(); - $body = trim($body); + $body_data = $mail->parseBody(); + $body = $body_data['body']; $body = $this->enhanceBodyWithAttachments($body, $mail->getAttachments()); $content_source = PhabricatorContentSource::newForSource( @@ -47,19 +47,9 @@ final class LegalpadReplyHandler extends PhabricatorMailReplyHandler { 'id' => $mail->getID(), )); - $lines = explode("\n", trim($body)); - $first_line = head($lines); $xactions = array(); - $command = null; - $matches = null; - if (preg_match('/^!(\w+)/', $first_line, $matches)) { - $lines = array_slice($lines, 1); - $body = implode("\n", $lines); - $body = trim($body); - - $command = $matches[1]; - } + $command = $body_data['command']; switch ($command) { case 'unsubscribe': diff --git a/src/applications/maniphest/mail/ManiphestReplyHandler.php b/src/applications/maniphest/mail/ManiphestReplyHandler.php index 5f9b90d360..084d55d38d 100644 --- a/src/applications/maniphest/mail/ManiphestReplyHandler.php +++ b/src/applications/maniphest/mail/ManiphestReplyHandler.php @@ -45,8 +45,8 @@ final class ManiphestReplyHandler extends PhabricatorMailReplyHandler { $user = $this->getActor(); - $body = $mail->getCleanTextBody(); - $body = trim($body); + $body_data = $mail->parseBody(); + $body = $body_data['body']; $body = $this->enhanceBodyWithAttachments($body, $mail->getAttachments()); $xactions = array(); @@ -73,18 +73,9 @@ final class ManiphestReplyHandler extends PhabricatorMailReplyHandler { $task->setPriority(ManiphestTaskPriority::getDefaultPriority()); } else { - $lines = explode("\n", trim($body)); - $first_line = head($lines); - $command = null; - $matches = null; - if (preg_match('/^!(\w+)/', $first_line, $matches)) { - $lines = array_slice($lines, 1); - $body = implode("\n", $lines); - $body = trim($body); - - $command = $matches[1]; - } + $command = $body_data['command']; + $command_value = $body_data['command_value']; $ttype = PhabricatorTransactions::TYPE_COMMENT; $new_value = null; @@ -97,6 +88,23 @@ final class ManiphestReplyHandler extends PhabricatorMailReplyHandler { $ttype = ManiphestTransaction::TYPE_OWNER; $new_value = $user->getPHID(); break; + case 'assign': + $ttype = ManiphestTransaction::TYPE_OWNER; + if ($command_value) { + $assign_users = id(new PhabricatorPeopleQuery()) + ->setViewer($user) + ->withUsernames(array($command_value)) + ->execute(); + if ($assign_users) { + $assign_user = head($assign_users); + $new_value = $assign_user->getPHID(); + } + } + // assign to the user by default + if (!$new_value) { + $new_value = $user->getPHID(); + } + break; case 'unsubscribe': $is_unsub = true; $ttype = ManiphestTransaction::TYPE_CCS; diff --git a/src/applications/metamta/parser/PhabricatorMetaMTAEmailBodyParser.php b/src/applications/metamta/parser/PhabricatorMetaMTAEmailBodyParser.php index 0e35fa9388..1ea3a1bab6 100644 --- a/src/applications/metamta/parser/PhabricatorMetaMTAEmailBodyParser.php +++ b/src/applications/metamta/parser/PhabricatorMetaMTAEmailBodyParser.php @@ -2,8 +2,51 @@ final class PhabricatorMetaMTAEmailBodyParser { + /** + * Mails can have bodies such as + * + * !claim + * + * taking this task + * + * Or + * + * !assign epriestley + * + * 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.) + * + * @return dict + */ + 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); + + $command = $matches[1]; + $command_value = idx($matches, 2); + } + + return array( + 'body' => $body, + 'command' => $command, + 'command_value' => $command_value); + } + public function stripTextBody($body) { - return $this->stripSignature($this->stripQuotedText($body)); + return trim($this->stripSignature($this->stripQuotedText($body))); } private function stripQuotedText($body) { diff --git a/src/applications/metamta/parser/__tests__/PhabricatorMetaMTAEmailBodyParserTestCase.php b/src/applications/metamta/parser/__tests__/PhabricatorMetaMTAEmailBodyParserTestCase.php index ccdebf9d72..1f62d60290 100644 --- a/src/applications/metamta/parser/__tests__/PhabricatorMetaMTAEmailBodyParserTestCase.php +++ b/src/applications/metamta/parser/__tests__/PhabricatorMetaMTAEmailBodyParserTestCase.php @@ -12,6 +12,44 @@ final class PhabricatorMetaMTAEmailBodyParserTestCase } } + public function testEmailBodyCommandParsing() { + $bodies = $this->getEmailBodiesWithFullCommands(); + 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('dude', $body_data['command_value']); + } + $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']); + } + } + + private function getEmailBodiesWithFullCommands() { + $bodies = $this->getEmailBodies(); + $with_commands = array(); + foreach ($bodies as $body) { + $with_commands[] = "!whatevs dude\n" . $body; + } + return $with_commands; + } + + private function getEmailBodiesWithPartialCommands() { + $bodies = $this->getEmailBodies(); + $with_commands = array(); + foreach ($bodies as $body) { + $with_commands[] = "!whatevs\n" . $body; + } + return $with_commands; + } + + private function getEmailBodies() { $trailing_space = ' '; diff --git a/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php b/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php index 07245a50a5..4278ce2bc6 100644 --- a/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php +++ b/src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php @@ -307,7 +307,7 @@ EOBODY; return $this->getSingleReplyHandlerPrefix($address); } - final protected function enhanceBodyWithAttachments( + final protected function enhanceBodyWithAttachments( $body, array $attachments, $format = '- {F%d, layout=link}') { diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php index 0b7b7c9040..1648a5e14f 100644 --- a/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php +++ b/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php @@ -120,12 +120,17 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO { } public function getCleanTextBody() { - $body = idx($this->bodies, 'text'); - + $body = $this->getRawTextBody(); $parser = new PhabricatorMetaMTAEmailBodyParser(); return $parser->stripTextBody($body); } + public function parseBody() { + $body = $this->getRawTextBody(); + $parser = new PhabricatorMetaMTAEmailBodyParser(); + return $parser->parseBody($body); + } + public function getRawTextBody() { return idx($this->bodies, 'text'); } diff --git a/src/applications/paste/mail/PasteReplyHandler.php b/src/applications/paste/mail/PasteReplyHandler.php index 93f3f79520..4c585b1e5b 100644 --- a/src/applications/paste/mail/PasteReplyHandler.php +++ b/src/applications/paste/mail/PasteReplyHandler.php @@ -32,8 +32,8 @@ final class PasteReplyHandler extends PhabricatorMailReplyHandler { $actor = $this->getActor(); $paste = $this->getMailReceiver(); - $body = $mail->getCleanTextBody(); - $body = trim($body); + $body_data = $mail->parseBody(); + $body = $body_data['body']; $body = $this->enhanceBodyWithAttachments($body, $mail->getAttachments()); $content_source = PhabricatorContentSource::newForSource( @@ -46,15 +46,7 @@ final class PasteReplyHandler extends PhabricatorMailReplyHandler { $first_line = head($lines); $xactions = array(); - $command = null; - $matches = null; - if (preg_match('/^!(\w+)/', $first_line, $matches)) { - $lines = array_slice($lines, 1); - $body = implode("\n", $lines); - $body = trim($body); - - $command = $matches[1]; - } + $command = $body_data['command']; switch ($command) { case 'unsubscribe':