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

src/applications/differential/mail/DifferentialReplyHandler.php

@@ -1,13 +1,6 @@
 <?php
 
-/**
- * NOTE: Do not extend this!
- *
- * @concrete-extensible
- */
-class DifferentialReplyHandler extends PhabricatorMailReplyHandler {
-
-  private $receivedMail;
+final class DifferentialReplyHandler extends PhabricatorMailReplyHandler {
 
   public function validateMailReceiver($mail_receiver) {
     if (!($mail_receiver instanceof DifferentialRevision)) {
@@ -48,82 +41,72 @@ class DifferentialReplyHandler extends PhabricatorMailReplyHandler {
   }
 
   protected function receiveEmail(PhabricatorMetaMTAReceivedMail $mail) {
-    $this->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);
   }
 
 }

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);
   }
 
 }

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);
   }
 
 }

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;

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)));
   }

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 = ' ';

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);
   }
 
 }

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)

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);
   }
 
 }