From d6bfdf6ce7d973465710887c42d003373cf56b5d Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 22 Jun 2011 12:41:19 -0700 Subject: [PATCH] Carry "Message-ID" across email replies to prevent Gmail conversation splitting Summary: See T251. In Gmail, conversations split if you reply to them and the next email does not "In-Reply-To" your message ID. When an action is triggered by an email, carry its Message-ID through the stack and use it for "In-Reply-To" and "References" on the subsequent message. Test Plan: Live-patched phabricator.com and replied to a Maniphest thread in Gmail without disrupting the thread. Locally replied to Maniphest and Differential threads and verified Message-ID was carried across the reply boundary. Reviewed By: rm Reviewers: tcook, jungejason, aran, tuomaspelkonen, rm CC: aran, epriestley, rm Differential Revision: 498 --- .../comment/DifferentialCommentEditor.php | 8 +++++ .../mail/base/DifferentialMail.php | 7 +++++ .../replyhandler/DifferentialReplyHandler.php | 9 ++++++ .../ManiphestTransactionEditor.php | 8 +++++ .../replyhandler/ManiphestReplyHandler.php | 2 +- .../storage/mail/PhabricatorMetaMTAMail.php | 30 +++++++++++++++++-- .../PhabricatorMetaMTAReceivedMail.php | 4 +++ 7 files changed, 65 insertions(+), 3 deletions(-) diff --git a/src/applications/differential/editor/comment/DifferentialCommentEditor.php b/src/applications/differential/editor/comment/DifferentialCommentEditor.php index ab84984024..a12adcd878 100644 --- a/src/applications/differential/editor/comment/DifferentialCommentEditor.php +++ b/src/applications/differential/editor/comment/DifferentialCommentEditor.php @@ -28,6 +28,8 @@ class DifferentialCommentEditor { protected $changedByCommit; protected $addedReviewers = array(); + private $parentMessageID; + public function __construct( DifferentialRevision $revision, $actor_phid, @@ -38,6 +40,11 @@ class DifferentialCommentEditor { $this->action = $action; } + public function setParentMessageID($parent_message_id) { + $this->parentMessageID = $parent_message_id; + return $this; + } + public function setMessage($message) { $this->message = $message; return $this; @@ -318,6 +325,7 @@ class DifferentialCommentEditor { ->setCCPHIDs($revision->getCCPHIDs()) ->setChangedByCommit($this->getChangedByCommit()) ->setXHeraldRulesHeader($xherald_header) + ->setParentMessageID($this->parentMessageID) ->send(); $event_data = array( diff --git a/src/applications/differential/mail/base/DifferentialMail.php b/src/applications/differential/mail/base/DifferentialMail.php index b497266f42..8ea77d456e 100644 --- a/src/applications/differential/mail/base/DifferentialMail.php +++ b/src/applications/differential/mail/base/DifferentialMail.php @@ -32,6 +32,7 @@ abstract class DifferentialMail { protected $heraldTranscriptURI; protected $heraldRulesHeader; protected $replyHandler; + protected $parentMessageID; abstract protected function renderSubject(); abstract protected function renderBody(); @@ -53,6 +54,11 @@ abstract class DifferentialMail { return '???'; } + public function setParentMessageID($parent_message_id) { + $this->parentMessageID = $parent_message_id; + return $this; + } + public function setXHeraldRulesHeader($header) { $this->heraldRulesHeader = $header; return $this; @@ -80,6 +86,7 @@ abstract class DifferentialMail { ->setSubject($subject) ->setBody($body) ->setIsHTML($this->shouldMarkMailAsHTML()) + ->setParentMessageID($this->parentMessageID) ->addHeader('Thread-Topic', $this->getRevision()->getTitle()); $template->setThreadID( diff --git a/src/applications/differential/replyhandler/DifferentialReplyHandler.php b/src/applications/differential/replyhandler/DifferentialReplyHandler.php index df0e11077a..ae7be54e1b 100644 --- a/src/applications/differential/replyhandler/DifferentialReplyHandler.php +++ b/src/applications/differential/replyhandler/DifferentialReplyHandler.php @@ -18,6 +18,8 @@ class DifferentialReplyHandler extends PhabricatorMailReplyHandler { + private $receivedMail; + public function validateMailReceiver($mail_receiver) { if (!($mail_receiver instanceof DifferentialRevision)) { throw new Exception("Receiver is not a DifferentialRevision!"); @@ -94,6 +96,7 @@ class DifferentialReplyHandler extends PhabricatorMailReplyHandler { } public function receiveEmail(PhabricatorMetaMTAReceivedMail $mail) { + $this->receivedMail = $mail; $this->handleAction($mail->getCleanTextBody()); } @@ -128,6 +131,12 @@ class DifferentialReplyHandler extends PhabricatorMailReplyHandler { $actor->getPHID(), $command); + // 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) { + $editor->setParentMessageID($this->receivedMail->getMessageID()); + } $editor->setMessage($body); $editor->setAddCC(($command != DifferentialAction::ACTION_RESIGN)); $comment = $editor->save(); diff --git a/src/applications/maniphest/editor/transaction/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/transaction/ManiphestTransactionEditor.php index 75067cd628..a2828a8b98 100644 --- a/src/applications/maniphest/editor/transaction/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/transaction/ManiphestTransactionEditor.php @@ -18,6 +18,13 @@ class ManiphestTransactionEditor { + private $parentMessageID; + + public function setParentMessageID($parent_message_id) { + $this->parentMessageID = $parent_message_id; + return $this; + } + public function applyTransactions($task, array $transactions) { $email_cc = $task->getCCPHIDs(); @@ -202,6 +209,7 @@ class ManiphestTransactionEditor { $template = id(new PhabricatorMetaMTAMail()) ->setSubject($subject) ->setFrom($transaction->getAuthorPHID()) + ->setParentMessageID($this->parentMessageID) ->addHeader('Thread-Topic', 'Maniphest Task '.$task->getID()) ->setThreadID($thread_id, $is_create) ->setRelatedPHID($task->getPHID()) diff --git a/src/applications/maniphest/replyhandler/ManiphestReplyHandler.php b/src/applications/maniphest/replyhandler/ManiphestReplyHandler.php index 004b971c5a..b86f5685f7 100644 --- a/src/applications/maniphest/replyhandler/ManiphestReplyHandler.php +++ b/src/applications/maniphest/replyhandler/ManiphestReplyHandler.php @@ -114,8 +114,8 @@ class ManiphestReplyHandler extends PhabricatorMailReplyHandler { $xactions[] = $xaction; $editor = new ManiphestTransactionEditor(); + $editor->setParentMessageID($mail->getMessageID()); $editor->applyTransactions($task, $xactions); - } } diff --git a/src/applications/metamta/storage/mail/PhabricatorMetaMTAMail.php b/src/applications/metamta/storage/mail/PhabricatorMetaMTAMail.php index eaca3beb30..7ea4ecec31 100644 --- a/src/applications/metamta/storage/mail/PhabricatorMetaMTAMail.php +++ b/src/applications/metamta/storage/mail/PhabricatorMetaMTAMail.php @@ -62,6 +62,27 @@ class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { return idx($this->parameters, $param); } + /** + * In Gmail, conversations will be broken if you reply to a thread and the + * server sends back a response without referencing your Message-ID, even if + * it references a Message-ID earlier in the thread. To avoid this, use the + * parent email's message ID explicitly if it's available. This overwrites the + * "In-Reply-To" and "References" headers we would otherwise generate. This + * needs to be set whenever an action is triggered by an email message. See + * T251 for more details. + * + * @param string The "Message-ID" of the email which precedes this one. + * @return this + */ + public function setParentMessageID($id) { + $this->setParam('parent-message-id', $id); + return $this; + } + + public function getParentMessageID() { + return $this->getParam('parent-message-id'); + } + public function getSubject() { return $this->getParam('subject'); } @@ -276,8 +297,13 @@ class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { if ($is_first && $mailer->supportsMessageIDHeader()) { $mailer->addHeader('Message-ID', $value); } else { - $mailer->addHeader('In-Reply-To', $value); - $mailer->addHeader('References', $value); + $in_reply_to = $value; + $parent_id = $this->getParentMessageID(); + if ($parent_id) { + $in_reply_to = $parent_id; + } + $mailer->addHeader('In-Reply-To', $in_reply_to); + $mailer->addHeader('References', $in_reply_to); } $thread_index = $this->generateThreadIndex($value, $is_first); $mailer->addHeader('Thread-Index', $thread_index); diff --git a/src/applications/metamta/storage/receivedmail/PhabricatorMetaMTAReceivedMail.php b/src/applications/metamta/storage/receivedmail/PhabricatorMetaMTAReceivedMail.php index cafaf174a6..cdd978feef 100644 --- a/src/applications/metamta/storage/receivedmail/PhabricatorMetaMTAReceivedMail.php +++ b/src/applications/metamta/storage/receivedmail/PhabricatorMetaMTAReceivedMail.php @@ -46,6 +46,10 @@ class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO { return $this; } + public function getMessageID() { + return idx($this->headers, 'message-id'); + } + public function processReceivedMail() { $to = idx($this->headers, 'to');