From c45876841585a543a61f3fb8ed022cde0d3dc273 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 12 Apr 2012 09:31:03 -0700 Subject: [PATCH] Fix various threading issues, particularly in Gmail Summary: - Add an explicit multiplexing option, and enable it by default. This is necessary for Mail.app to coexist with other clients ("Re:" breaks outlook at the very least, and generally sucks in the common case), and allows users with flexible clients to enable subject variance. - Add an option for subject line variance. Default to not varying the subject, so mail no longer says [Committed], [Closed], etc. This is so the defaults thread correctly in Gmail (not entirely sure this actually works). - Add a preference to enable subject line variance. - Unless all mail is multiplexed, don't enable or respect the "Re" or "vary subject" preferences. These are currently shown and respected in non-multiplex cases, which creates inconsistent results. NOTE: @jungejason @nh @vrana This changes the default behavior (from non-multiplexing to multiplexing), and might break Facebook's integration. You should be able to keep the same behavior by setting the options appropriately, although if you can get the new defaults working they're probably better. Test Plan: Send mail from Maniphest, Differential and Audit. Updated preferences. Enabled/disabled multiplexing. Things seem OK? NOTE: I haven't actually been able to repro the Gmail threading issue so I'm not totally sure what's going on there, maybe it started respecting "Re:" (or always has), but @cpiro and @20after4 both reported it independently. This fixes a bunch of bugs in any case and gives us more conservative set of defaults. I'll see if I can buff out the Gmail story a bit but every client is basically a giant black box of mystery. :/ Reviewers: btrahan, vrana, jungejason, nh Reviewed By: btrahan CC: cpiro, 20after4, aran Maniphest Tasks: T1097, T847 Differential Revision: https://secure.phabricator.com/D2206 --- conf/default.conf.php | 41 ++++++++- .../comment/PhabricatorAuditCommentEditor.php | 11 ++- .../mail/base/DifferentialMail.php | 26 ++++-- .../ccwelcome/DifferentialCCWelcomeMail.php | 5 +- .../mail/comment/DifferentialCommentMail.php | 8 +- .../DifferentialDiffContentMail.php | 4 +- .../exception/DifferentialExceptionMail.php | 4 + .../mail/newdiff/DifferentialNewDiffMail.php | 18 +--- .../ManiphestTransactionEditor.php | 6 +- .../base/PhabricatorMailReplyHandler.php | 32 ++++--- .../metamta/replyhandler/base/__init__.php | 1 + .../storage/mail/PhabricatorMetaMTAMail.php | 69 ++++++++++++--- ...EmailPreferenceSettingsPanelController.php | 87 ++++++++++++++----- .../settings/panels/emailpref/__init__.php | 1 + .../PhabricatorUserPreferences.php | 1 + ...habricatorRepositoryCommitHeraldWorker.php | 6 +- 16 files changed, 228 insertions(+), 92 deletions(-) diff --git a/conf/default.conf.php b/conf/default.conf.php index 6556406956..8d2218557a 100644 --- a/conf/default.conf.php +++ b/conf/default.conf.php @@ -145,6 +145,35 @@ return array( // Domain used to generate Message-IDs. 'metamta.domain' => 'example.com', + // When a message is sent to multiple recipients (for example, several + // reviewers on a code review), Phabricator can either deliver one email to + // everyone (e.g., "To: alincoln, usgrant, htaft") or separate emails to each + // user (e.g., "To: alincoln", "To: usgrant", "To: htaft"). The major + // advantages and disadvantages of each approach are: + // + // - One mail to everyone: + // - Recipients can see To/Cc at a glance. + // - If you use mailing lists, you won't get duplicate mail if you're + // a normal recipient and also Cc'd on a mailing list. + // - Getting threading to work properly is harder, and probably requires + // making mail less useful by turning off options. + // - Sometimes people will "Reply All" and everyone will get two mails, + // one from the user and one from Phabricator turning their mail into + // a comment. + // - Not supported with a private reply-to address. + // - One mail to each user: + // - Recipients need to look in the mail body to see To/Cc. + // - If you use mailing lists, recipients may sometimes get duplicate + // mail. + // - Getting threading to work properly is easier, and threading settings + // can be customzied by each user. + // - "Reply All" no longer spams all other users. + // - Required if private reply-to addresses are configured. + // + // In the code, splitting one outbound email into one-per-recipient is + // sometimes referred to as "multiplexing". + 'metamta.one-mail-per-recipient' => true, + // When a user takes an action which generates an email notification (like // commenting on a Differential revision), Phabricator can either send that // mail "From" the user's email address (like "alincoln@logcabin.com") or @@ -166,6 +195,7 @@ return array( // mostly never arrive. 'metamta.can-send-as-user' => false, + // Adapter class to use to transmit mail to the MTA. The default uses // PHPMailerLite, which will invoke "sendmail". This is appropriate // if sendmail actually works on your host, but if you haven't configured mail @@ -329,9 +359,18 @@ return array( // Mail.app on OS X Lion won't respect threading headers unless the subject // is prefixed with "Re:". If you enable this option, Phabricator will add - // "Re:" to the subject line of all mail which is expected to thread. + // "Re:" to the subject line of all mail which is expected to thread. If + // you've set 'metamta.one-mail-per-recipient', users can override this + // setting in their preferences. 'metamta.re-prefix' => false, + // If true, allow MetaMTA to change mail subjects to put text like + // '[Accepted]' and '[Commented]' in them. This makes subjects more useful, + // but might break threading on some clients. If you've set + // 'metamta.one-mail-per-recipient', users can override this setting in their + // preferences. + 'metamta.vary-subjects' => true, + // -- Auth ------------------------------------------------------------------ // diff --git a/src/applications/audit/editor/comment/PhabricatorAuditCommentEditor.php b/src/applications/audit/editor/comment/PhabricatorAuditCommentEditor.php index 4b27424ff8..4dd7ee3718 100644 --- a/src/applications/audit/editor/comment/PhabricatorAuditCommentEditor.php +++ b/src/applications/audit/editor/comment/PhabricatorAuditCommentEditor.php @@ -283,12 +283,12 @@ final class PhabricatorAuditCommentEditor { $reply_handler = self::newReplyHandlerForCommit($commit); $prefix = PhabricatorEnv::getEnvConfig('metamta.diffusion.subject-prefix'); - $subject = "{$prefix} [{$verb}] {$name}: {$summary}"; + $subject = "{$prefix} {$name}: {$summary}"; + $vary_subject = "{$prefix} [{$verb}] {$name}: {$summary}"; $threading = self::getMailThreading($commit->getPHID()); list($thread_id, $thread_topic) = $threading; - $is_new = !count($other_comments); $body = $this->renderMailBody( $comment, "{$name}: {$summary}", @@ -311,8 +311,13 @@ final class PhabricatorAuditCommentEditor { $phids = array_merge($email_to, $email_cc); $handles = id(new PhabricatorObjectHandleData($phids))->loadHandles(); + // NOTE: Always set $is_new to false, because the "first" mail in the + // thread is the Herald notification of the commit. + $is_new = false; + $template = id(new PhabricatorMetaMTAMail()) ->setSubject($subject) + ->setVarySubject($subject) ->setFrom($comment->getActorPHID()) ->setThreadID($thread_id, $is_new) ->addHeader('Thread-Topic', $thread_topic) @@ -332,7 +337,7 @@ final class PhabricatorAuditCommentEditor { public static function getMailThreading($phid) { return array( - '', + 'diffusion-audit-'.$phid, 'Diffusion Audit '.$phid, ); } diff --git a/src/applications/differential/mail/base/DifferentialMail.php b/src/applications/differential/mail/base/DifferentialMail.php index 686eecbc4c..9694306ef7 100644 --- a/src/applications/differential/mail/base/DifferentialMail.php +++ b/src/applications/differential/mail/base/DifferentialMail.php @@ -34,7 +34,14 @@ abstract class DifferentialMail { protected $replyHandler; protected $parentMessageID; - abstract protected function renderSubject(); + protected function renderSubject() { + $revision = $this->getRevision(); + $title = $revision->getTitle(); + $id = $revision->getID(); + return "D{$id}: {$title}"; + } + + abstract protected function renderVarySubject(); abstract protected function renderBody(); public function setActorHandle($actor_handle) { @@ -70,10 +77,11 @@ abstract class DifferentialMail { throw new Exception('No "To:" users provided!'); } - $cc_phids = $this->getCCPHIDs(); - $subject = $this->buildSubject(); - $body = $this->buildBody(); - $attachments = $this->buildAttachments(); + $cc_phids = $this->getCCPHIDs(); + $subject = $this->buildSubject(); + $vary_subject = $this->buildVarySubject(); + $body = $this->buildBody(); + $attachments = $this->buildAttachments(); $template = new PhabricatorMetaMTAMail(); $actor_handle = $this->getActorHandle(); @@ -85,6 +93,7 @@ abstract class DifferentialMail { $template ->setSubject($subject) + ->setVarySubject($vary_subject) ->setBody($body) ->setIsHTML($this->shouldMarkMailAsHTML()) ->setParentMessageID($this->parentMessageID) @@ -197,6 +206,10 @@ abstract class DifferentialMail { return trim($this->getSubjectPrefix().' '.$this->renderSubject()); } + protected function buildVarySubject() { + return trim($this->getSubjectPrefix().' '.$this->renderVarySubject()); + } + protected function shouldMarkMailAsHTML() { return false; } @@ -320,8 +333,7 @@ EOTEXT; protected function getThreadID() { $phid = $this->getRevision()->getPHID(); - $domain = PhabricatorEnv::getEnvConfig('metamta.domain'); - return ""; + return "differential-rev-{$phid}-req"; } public function setComment($comment) { diff --git a/src/applications/differential/mail/ccwelcome/DifferentialCCWelcomeMail.php b/src/applications/differential/mail/ccwelcome/DifferentialCCWelcomeMail.php index a7db4f6ce9..85776461de 100644 --- a/src/applications/differential/mail/ccwelcome/DifferentialCCWelcomeMail.php +++ b/src/applications/differential/mail/ccwelcome/DifferentialCCWelcomeMail.php @@ -18,9 +18,8 @@ final class DifferentialCCWelcomeMail extends DifferentialReviewRequestMail { - protected function renderSubject() { - $revision = $this->getRevision(); - return 'Added to CC: '.$revision->getTitle(); + protected function renderVarySubject() { + return '[Added to CC] '.$this->renderSubject(); } protected function renderBody() { diff --git a/src/applications/differential/mail/comment/DifferentialCommentMail.php b/src/applications/differential/mail/comment/DifferentialCommentMail.php index 03973d33a5..fb78849b5e 100644 --- a/src/applications/differential/mail/comment/DifferentialCommentMail.php +++ b/src/applications/differential/mail/comment/DifferentialCommentMail.php @@ -75,13 +75,9 @@ final class DifferentialCommentMail extends DifferentialMail { return $tags; } - protected function renderSubject() { + protected function renderVarySubject() { $verb = ucwords($this->getVerb()); - $revision = $this->getRevision(); - $title = $revision->getTitle(); - $id = $revision->getID(); - $subject = "[{$verb}] D{$id}: {$title}"; - return $subject; + return "[{$verb}] ".$this->renderSubject(); } protected function getVerb() { diff --git a/src/applications/differential/mail/diffcontent/DifferentialDiffContentMail.php b/src/applications/differential/mail/diffcontent/DifferentialDiffContentMail.php index f021ae4d2f..110c98117c 100644 --- a/src/applications/differential/mail/diffcontent/DifferentialDiffContentMail.php +++ b/src/applications/differential/mail/diffcontent/DifferentialDiffContentMail.php @@ -25,8 +25,8 @@ final class DifferentialDiffContentMail extends DifferentialMail { $this->content = $content; } - protected function renderSubject() { - return "Content: ".$this->getRevision()->getTitle(); + protected function renderVarySubject() { + return '[Content] '.$this->renderSubject(); } protected function renderBody() { diff --git a/src/applications/differential/mail/exception/DifferentialExceptionMail.php b/src/applications/differential/mail/exception/DifferentialExceptionMail.php index a0484c7f52..ef11cf9e6b 100644 --- a/src/applications/differential/mail/exception/DifferentialExceptionMail.php +++ b/src/applications/differential/mail/exception/DifferentialExceptionMail.php @@ -36,6 +36,10 @@ final class DifferentialExceptionMail extends DifferentialMail { return "Exception: unable to process your mail request."; } + protected function renderVarySubject() { + return $this->renderSubject(); + } + protected function buildBody() { $exception = $this->exception; $original_body = $this->originalBody; diff --git a/src/applications/differential/mail/newdiff/DifferentialNewDiffMail.php b/src/applications/differential/mail/newdiff/DifferentialNewDiffMail.php index b1d1b46182..f3167caa64 100644 --- a/src/applications/differential/mail/newdiff/DifferentialNewDiffMail.php +++ b/src/applications/differential/mail/newdiff/DifferentialNewDiffMail.php @@ -18,7 +18,7 @@ final class DifferentialNewDiffMail extends DifferentialReviewRequestMail { - protected function renderSubject() { + protected function renderVarySubject() { $revision = $this->getRevision(); $line_count = $revision->getLineCount(); $lines = ($line_count == 1 ? "1 line" : "{$line_count} lines"); @@ -29,21 +29,7 @@ final class DifferentialNewDiffMail extends DifferentialReviewRequestMail { $verb = 'Updated'; } - $revision_id = $revision->getID(); - $revision_title = $revision->getTitle(); - - return "[{$verb}, {$lines}] D{$revision_id}: {$revision_title}"; - } - - protected function buildSubject() { - if (!$this->isFirstMailToRecipients()) { - return parent::buildSubject(); - } - - $prefix = $this->getSubjectPrefix(); - $subject = $this->renderSubject(); - - return trim("{$prefix} {$subject}"); + return "[{$verb}, {$lines}] ".$this->renderSubject(); } protected function renderBody() { diff --git a/src/applications/maniphest/editor/transaction/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/transaction/ManiphestTransactionEditor.php index ac22b5ee54..94dfa2eee3 100644 --- a/src/applications/maniphest/editor/transaction/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/transaction/ManiphestTransactionEditor.php @@ -266,16 +266,18 @@ final class ManiphestTransactionEditor { " ".$reply_instructions."\n"; } - $thread_id = 'getPHID().'>'; + $thread_id = 'maniphest-task-'.$task->getPHID(); $task_id = $task->getID(); $title = $task->getTitle(); $prefix = $this->getSubjectPrefix(); - $subject = trim("{$prefix} [{$action}] T{$task_id}: {$title}"); + $subject = trim("{$prefix} T{$task_id}: {$title}"); + $vary_subject = trim("{$prefix} [{$action}] T{$task_id}: {$title}"); $mailtags = $this->getMailTags($transactions); $template = id(new PhabricatorMetaMTAMail()) ->setSubject($subject) + ->setVarySubject($vary_subject) ->setFrom($transaction->getAuthorPHID()) ->setParentMessageID($this->parentMessageID) ->addHeader('Thread-Topic', 'Maniphest Task '.$task->getID()) diff --git a/src/applications/metamta/replyhandler/base/PhabricatorMailReplyHandler.php b/src/applications/metamta/replyhandler/base/PhabricatorMailReplyHandler.php index c4d7afa79b..57ffee38dc 100644 --- a/src/applications/metamta/replyhandler/base/PhabricatorMailReplyHandler.php +++ b/src/applications/metamta/replyhandler/base/PhabricatorMailReplyHandler.php @@ -80,22 +80,26 @@ abstract class PhabricatorMailReplyHandler { $result = array(); - // If private replies are not supported, simply send one email to all - // recipients and CCs. This covers cases where we have no reply handler, - // or we have a public reply handler. - if (!$this->supportsPrivateReplies()) { - $mail = clone $mail_template; - $mail->addTos(mpull($to_handles, 'getPHID')); - $mail->addCCs(mpull($cc_handles, 'getPHID')); + // If MetaMTA is configured to always multiplex, skip the single-email + // case. + if (!PhabricatorMetaMTAMail::shouldMultiplexAllMail()) { + // If private replies are not supported, simply send one email to all + // recipients and CCs. This covers cases where we have no reply handler, + // or we have a public reply handler. + if (!$this->supportsPrivateReplies()) { + $mail = clone $mail_template; + $mail->addTos(mpull($to_handles, 'getPHID')); + $mail->addCCs(mpull($cc_handles, 'getPHID')); - if ($this->supportsPublicReplies()) { - $reply_to = $this->getPublicReplyHandlerEmailAddress(); - $mail->setReplyTo($reply_to); + if ($this->supportsPublicReplies()) { + $reply_to = $this->getPublicReplyHandlerEmailAddress(); + $mail->setReplyTo($reply_to); + } + + $result[] = $mail; + + return $result; } - - $result[] = $mail; - - return $result; } // Merge all the recipients together. TODO: We could keep the CCs as real diff --git a/src/applications/metamta/replyhandler/base/__init__.php b/src/applications/metamta/replyhandler/base/__init__.php index 145c5f6fd3..02f6d6d33d 100644 --- a/src/applications/metamta/replyhandler/base/__init__.php +++ b/src/applications/metamta/replyhandler/base/__init__.php @@ -6,6 +6,7 @@ +phutil_require_module('phabricator', 'applications/metamta/storage/mail'); phutil_require_module('phabricator', 'applications/metamta/storage/receivedmail'); phutil_require_module('phabricator', 'applications/phid/constants'); phutil_require_module('phabricator', 'infrastructure/env'); diff --git a/src/applications/metamta/storage/mail/PhabricatorMetaMTAMail.php b/src/applications/metamta/storage/mail/PhabricatorMetaMTAMail.php index 458c7545c0..3240ad2e77 100644 --- a/src/applications/metamta/storage/mail/PhabricatorMetaMTAMail.php +++ b/src/applications/metamta/storage/mail/PhabricatorMetaMTAMail.php @@ -149,6 +149,11 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { return $this; } + public function setVarySubject($subject) { + $this->setParam('vary-subject', $subject); + return $this; + } + public function setBody($body) { $this->setParam('body', $body); return $this; @@ -395,23 +400,44 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { $mailer->setBody($value); break; case 'subject': + // Only try to use preferences if everything is multiplexed, so we + // get consistent behavior. + $use_prefs = self::shouldMultiplexAllMail(); + + $prefs = null; + if ($use_prefs) { + $to = idx($params, 'to', array()); + $user = id(new PhabricatorUser())->loadOneWhere( + 'phid = %s', + head($to)); + if ($user) { + $prefs = $user->loadPreferences(); + } + } + + $alt_subject = idx($params, 'vary-subject'); + if ($alt_subject) { + $use_subject = PhabricatorEnv::getEnvConfig( + 'metamta.vary-subjects'); + + if ($prefs) { + $use_subject = $prefs->getPreference( + PhabricatorUserPreferences::PREFERENCE_VARY_SUBJECT, + $use_subject); + } + + if ($use_subject) { + $value = $alt_subject; + } + } + if ($is_threaded) { $add_re = PhabricatorEnv::getEnvConfig('metamta.re-prefix'); - // If this message has a single recipient, respect their "Re:" - // preference. Otherwise, use the global setting. - - $to = idx($params, 'to', array()); - $cc = idx($params, 'cc', array()); - if (count($to) == 1 && count($cc) == 0) { - $user = id(new PhabricatorUser())->loadOneWhere( - 'phid = %s', - head($to)); - if ($user) { - $prefs = $user->loadPreferences(); - $pref_key = PhabricatorUserPreferences::PREFERENCE_RE_PREFIX; - $add_re = $prefs->getPreference($pref_key, $add_re); - } + if ($prefs) { + $add_re = $prefs->getPreference( + PhabricatorUserPreferences::PREFERENCE_RE_PREFIX, + $add_re); } if ($add_re) { @@ -434,6 +460,14 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { } break; case 'thread-id': + + // NOTE: Gmail freaks out about In-Reply-To and References which + // aren't in the form ""; this is also required + // by RFC 2822, although some clients are more liberal in what they + // accept. + $domain = PhabricatorEnv::getEnvConfig('metamta.domain'); + $value = '<'.$value.'@'.$domain.'>'; + if ($is_first && $mailer->supportsMessageIDHeader()) { $mailer->addHeader('Message-ID', $value); } else { @@ -456,6 +490,9 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { case 'mailtags': // Handled below. break; + case 'vary-subject': + // Handled above. + break; default: // Just discard. } @@ -634,4 +671,8 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { return $emails; } + public static function shouldMultiplexAllMail() { + return PhabricatorEnv::getEnvConfig('metamta.one-mail-per-recipient'); + } + } diff --git a/src/applications/people/controller/settings/panels/emailpref/PhabricatorUserEmailPreferenceSettingsPanelController.php b/src/applications/people/controller/settings/panels/emailpref/PhabricatorUserEmailPreferenceSettingsPanelController.php index 8b7e5c73d2..d8d3b73dc7 100644 --- a/src/applications/people/controller/settings/panels/emailpref/PhabricatorUserEmailPreferenceSettingsPanelController.php +++ b/src/applications/people/controller/settings/panels/emailpref/PhabricatorUserEmailPreferenceSettingsPanelController.php @@ -26,24 +26,34 @@ final class PhabricatorUserEmailPreferenceSettingsPanelController $preferences = $user->loadPreferences(); $pref_re_prefix = PhabricatorUserPreferences::PREFERENCE_RE_PREFIX; + $pref_vary = PhabricatorUserPreferences::PREFERENCE_VARY_SUBJECT; $pref_no_self_mail = PhabricatorUserPreferences::PREFERENCE_NO_SELF_MAIL; $errors = array(); if ($request->isFormPost()) { - if ($request->getStr($pref_re_prefix) == 'default') { - $preferences->unsetPreference($pref_re_prefix); - } else { - $preferences->setPreference( - $pref_re_prefix, - $request->getBool($pref_re_prefix)); + if (PhabricatorMetaMTAMail::shouldMultiplexAllMail()) { + if ($request->getStr($pref_re_prefix) == 'default') { + $preferences->unsetPreference($pref_re_prefix); + } else { + $preferences->setPreference( + $pref_re_prefix, + $request->getBool($pref_re_prefix)); + } + + if ($request->getStr($pref_vary) == 'default') { + $preferences->unsetPreference($pref_vary); + } else { + $preferences->setPreference( + $pref_vary, + $request->getBool($pref_vary)); + } } $preferences->setPreference( $pref_no_self_mail, $request->getStr($pref_no_self_mail)); - $new_tags = $request->getArr('mailtags'); $mailtags = $preferences->getPreference('mailtags', array()); foreach ($this->getMailTags() as $key => $label) { @@ -75,15 +85,28 @@ final class PhabricatorUserEmailPreferenceSettingsPanelController ? 'Enabled' : 'Disabled'; + $vary_default = PhabricatorEnv::getEnvConfig('metamta.vary-subjects') + ? 'Vary' + : 'Do Not Vary'; + $re_prefix_value = $preferences->getPreference($pref_re_prefix); if ($re_prefix_value === null) { - $re_prefix_value = 'defualt'; + $re_prefix_value = 'default'; } else { $re_prefix_value = $re_prefix_value ? 'true' : 'false'; } + $vary_value = $preferences->getPreference($pref_vary); + if ($vary_value === null) { + $vary_value = 'default'; + } else { + $vary_value = $vary_value + ? 'true' + : 'false'; + } + $form = new AphrontFormView(); $form ->setUser($user) @@ -97,21 +120,39 @@ final class PhabricatorUserEmailPreferenceSettingsPanelController '1' => 'Do not send me an email when I take an action', )) ->setCaption('You can disable email about your own actions.') - ->setValue($preferences->getPreference($pref_no_self_mail, 0))) - ->appendChild( - id(new AphrontFormSelectControl()) - ->setLabel('Add "Re:" Prefix') - ->setName($pref_re_prefix) - ->setCaption( - 'Enable this option to fix threading in Mail.app on OS X Lion, '. - 'or if you like "Re:" in your email subjects.') - ->setOptions( - array( - 'default' => 'Use Server Default ('.$re_prefix_default.')', - 'true' => 'Enable "Re:" prefix', - 'false' => 'Disable "Re:" prefix', - )) - ->setValue($re_prefix_value)); + ->setValue($preferences->getPreference($pref_no_self_mail, 0))); + + if (PhabricatorMetaMTAMail::shouldMultiplexAllMail()) { + $form + ->appendChild( + id(new AphrontFormSelectControl()) + ->setLabel('Add "Re:" Prefix') + ->setName($pref_re_prefix) + ->setCaption( + 'Enable this option to fix threading in Mail.app on OS X Lion, '. + 'or if you like "Re:" in your email subjects.') + ->setOptions( + array( + 'default' => 'Use Server Default ('.$re_prefix_default.')', + 'true' => 'Enable "Re:" prefix', + 'false' => 'Disable "Re:" prefix', + )) + ->setValue($re_prefix_value)) + ->appendChild( + id(new AphrontFormSelectControl()) + ->setLabel('Vary Subjects') + ->setName($pref_vary) + ->setCaption( + 'This option adds more information email subjects, but may '. + 'break threading in some clients.') + ->setOptions( + array( + 'default' => 'Use Server Default ('.$vary_default.')', + 'true' => 'Vary Subjects', + 'false' => 'Do Not Vary Subjects', + )) + ->setValue($vary_value)); + } $form ->appendChild( diff --git a/src/applications/people/controller/settings/panels/emailpref/__init__.php b/src/applications/people/controller/settings/panels/emailpref/__init__.php index c6bf80fd00..21869c2bd2 100644 --- a/src/applications/people/controller/settings/panels/emailpref/__init__.php +++ b/src/applications/people/controller/settings/panels/emailpref/__init__.php @@ -8,6 +8,7 @@ phutil_require_module('phabricator', 'aphront/response/redirect'); phutil_require_module('phabricator', 'applications/metamta/constants/notificationtype'); +phutil_require_module('phabricator', 'applications/metamta/storage/mail'); phutil_require_module('phabricator', 'applications/people/controller/settings/panels/base'); phutil_require_module('phabricator', 'applications/people/storage/preferences'); phutil_require_module('phabricator', 'infrastructure/env'); diff --git a/src/applications/people/storage/preferences/PhabricatorUserPreferences.php b/src/applications/people/storage/preferences/PhabricatorUserPreferences.php index 0e8339b96c..c1512a0a00 100644 --- a/src/applications/people/storage/preferences/PhabricatorUserPreferences.php +++ b/src/applications/people/storage/preferences/PhabricatorUserPreferences.php @@ -25,6 +25,7 @@ final class PhabricatorUserPreferences extends PhabricatorUserDAO { const PREFERENCE_RE_PREFIX = 're-prefix'; const PREFERENCE_NO_SELF_MAIL = 'self-mail'; const PREFERENCE_MAILTAGS = 'mailtags'; + const PREFERENCE_VARY_SUBJECT = 'vary-subject'; const PREFERENCE_SEARCHBAR_JUMP = 'searchbar-jump'; const PREFERENCE_SEARCH_SHORTCUT = 'search-shortcut'; diff --git a/src/applications/repository/worker/herald/PhabricatorRepositoryCommitHeraldWorker.php b/src/applications/repository/worker/herald/PhabricatorRepositoryCommitHeraldWorker.php index ad35e89df2..203dee9e33 100644 --- a/src/applications/repository/worker/herald/PhabricatorRepositoryCommitHeraldWorker.php +++ b/src/applications/repository/worker/herald/PhabricatorRepositoryCommitHeraldWorker.php @@ -150,7 +150,10 @@ WHY DID I GET THIS EMAIL? EOBODY; - $subject = "[Herald/Commit] {$commit_name} ({$who}) {$name}"; + $prefix = PhabricatorEnv::getEnvConfig('metamta.diffusion.subject-prefix'); + + $subject = trim("{$prefix} {$commit_name}: {$name}"); + $vary_subject = trim("{$prefix} [Commit] {$commit_name}: {$name}"); $threading = PhabricatorAuditCommentEditor::getMailThreading( $commit->getPHID()); @@ -159,6 +162,7 @@ EOBODY; $template = new PhabricatorMetaMTAMail(); $template->setRelatedPHID($commit->getPHID()); $template->setSubject($subject); + $template->setVarySubject($subject); $template->setBody($body); $template->setThreadID($thread_id, $is_new = true); $template->addHeader('Thread-Topic', $thread_topic);