Refactor setting e-mail subjects

Summary:
It seems that Outlook and Mail.app mostly ignores the threading headers and thread primarily by subject.
They are also very picky about the Re: part in the header.
I guess that's because users of these clients often hit Reply when they want to create a new message to the sender of an e-mail.

We need both of these applications to work with the same setting because we don't use multiplexing to prevent sending multiple e-mails to people in lists.
I also believe that the default behavior should just work in most setups.

I've tried several different combinations of putting "Re:" and none of them seems to always work in both clients.

This diff at least adds more abstraction to the code which should prevent copy/paste errors (two fixed by this diff!).

Test Plan: Sent several e-mails with varying subject, verified that they look as before in Outlook and Mail.app.

Reviewers: epriestley, nh

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D2709

src/applications/audit/editor/PhabricatorAuditCommentEditor.php

@@ -394,8 +394,6 @@ final class PhabricatorAuditCommentEditor {
     $reply_handler = self::newReplyHandlerForCommit($commit);
 
     $prefix = PhabricatorEnv::getEnvConfig('metamta.diffusion.subject-prefix');
-    $subject = "{$prefix} {$name}: {$summary}";
-    $vary_subject = "{$prefix} [{$verb}] {$name}: {$summary}";
 
     $threading = self::getMailThreading($commit->getPHID());
     list($thread_id, $thread_topic) = $threading;
@@ -434,8 +432,9 @@ final class PhabricatorAuditCommentEditor {
     $is_new = false;
 
     $template = id(new PhabricatorMetaMTAMail())
-      ->setSubject($subject)
+      ->setSubject("{$name}: {$summary}")
-      ->setVarySubject($subject)
+      ->setSubjectPrefix($prefix)
+      ->setVarySubjectPrefix("[{$verb}]")
       ->setFrom($comment->getActorPHID())
       ->setThreadID($thread_id, $is_new)
       ->addHeader('Thread-Topic', $thread_topic)

src/applications/differential/mail/DifferentialCCWelcomeMail.php

@@ -18,8 +18,8 @@
 
 final class DifferentialCCWelcomeMail extends DifferentialReviewRequestMail {
 
-  protected function renderVarySubject() {
+  protected function renderVaryPrefix() {
-    return '[Added to CC] '.$this->renderSubject();
+    return '[Added to CC]';
   }
 
   protected function renderBody() {

src/applications/differential/mail/DifferentialCommentMail.php

@@ -75,9 +75,9 @@ final class DifferentialCommentMail extends DifferentialMail {
     return $tags;
   }
 
-  protected function renderVarySubject() {
+  protected function renderVaryPrefix() {
     $verb = ucwords($this->getVerb());
-    return "[{$verb}] ".$this->renderSubject();
+    return "[{$verb}]";
   }
 
   protected function getVerb() {

src/applications/differential/mail/DifferentialDiffContentMail.php

@@ -25,8 +25,8 @@ final class DifferentialDiffContentMail extends DifferentialMail {
     $this->content = $content;
   }
 
-  protected function renderVarySubject() {
+  protected function renderVaryPrefix() {
-    return '[Content] '.$this->renderSubject();
+    return '[Content]';
   }
 
   protected function renderBody() {

src/applications/differential/mail/DifferentialExceptionMail.php

@@ -33,11 +33,11 @@ final class DifferentialExceptionMail extends DifferentialMail {
   }
 
   protected function renderSubject() {
-    return "Exception: unable to process your mail request.";
+    return "Exception: unable to process your mail request";
   }
 
-  protected function renderVarySubject() {
+  protected function renderVaryPrefix() {
-    return $this->renderSubject();
+    return '';
   }
 
   protected function buildBody() {

src/applications/differential/mail/DifferentialMail.php

@@ -41,7 +41,7 @@ abstract class DifferentialMail {
     return "D{$id}: {$title}";
   }
 
-  abstract protected function renderVarySubject();
+  abstract protected function renderVaryPrefix();
   abstract protected function renderBody();
 
   public function setActorHandle($actor_handle) {
@@ -78,8 +78,6 @@ abstract class DifferentialMail {
     }
 
     $cc_phids     = $this->getCCPHIDs();
-    $subject      = $this->buildSubject();
-    $vary_subject = $this->buildVarySubject();
     $body         = $this->buildBody();
     $attachments  = $this->buildAttachments();
 
@@ -92,8 +90,9 @@ abstract class DifferentialMail {
     }
 
     $template
-      ->setSubject($subject)
+      ->setSubject($this->renderSubject())
-      ->setVarySubject($vary_subject)
+      ->setSubjectPrefix($this->getSubjectPrefix())
+      ->setVarySubjectPrefix($this->renderVaryPrefix())
       ->setBody($body)
       ->setIsHTML($this->shouldMarkMailAsHTML())
       ->setParentMessageID($this->parentMessageID)
@@ -202,14 +201,6 @@ abstract class DifferentialMail {
     return PhabricatorEnv::getEnvConfig('metamta.differential.subject-prefix');
   }
 
-  protected function buildSubject() {
-    return trim($this->getSubjectPrefix().' '.$this->renderSubject());
-  }
-
-  protected function buildVarySubject() {
-    return trim($this->getSubjectPrefix().' '.$this->renderVarySubject());
-  }
-
   protected function shouldMarkMailAsHTML() {
     return false;
   }

src/applications/differential/mail/DifferentialNewDiffMail.php

@@ -18,7 +18,7 @@
 
 final class DifferentialNewDiffMail extends DifferentialReviewRequestMail {
 
-  protected function renderVarySubject() {
+  protected function renderVaryPrefix() {
     $revision = $this->getRevision();
     $line_count = $revision->getLineCount();
     $lines = ($line_count == 1 ? "1 line" : "{$line_count} lines");
@@ -29,7 +29,7 @@ final class DifferentialNewDiffMail extends DifferentialReviewRequestMail {
       $verb = 'Updated';
     }
 
-    return "[{$verb}, {$lines}] ".$this->renderSubject();
+    return "[{$verb}, {$lines}]";
   }
 
   protected function renderBody() {

src/applications/maniphest/editor/ManiphestTransactionEditor.php

@@ -269,15 +269,13 @@ final class ManiphestTransactionEditor {
     $thread_id = 'maniphest-task-'.$task->getPHID();
     $task_id = $task->getID();
     $title = $task->getTitle();
-    $prefix = $this->getSubjectPrefix();
-    $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)
+      ->setSubject("T{$task_id}: {$title}")
-      ->setVarySubject($vary_subject)
+      ->setSubjectPrefix($this->getSubjectPrefix())
+      ->setVarySubjectPrefix("[{$action}]")
       ->setFrom($transaction->getAuthorPHID())
       ->setParentMessageID($this->parentMessageID)
       ->addHeader('Thread-Topic', 'Maniphest Task '.$task->getID())

src/applications/metamta/storage/PhabricatorMetaMTAMail.php

@@ -154,8 +154,13 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
     return $this;
   }
 
-  public function setVarySubject($subject) {
+  public function setSubjectPrefix($prefix) {
-    $this->setParam('vary-subject', $subject);
+    $this->setParam('subject-prefix', $prefix);
+    return $this;
+  }
+
+  public function setVarySubjectPrefix($prefix) {
+    $this->setParam('vary-subject-prefix', $prefix);
     return $this;
   }
 
@@ -416,21 +421,7 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
               }
             }
 
-            $alt_subject = idx($params, 'vary-subject');
+            $subject = array();
-            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');
@@ -442,11 +433,31 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
               }
 
               if ($add_re) {
-                $value = 'Re: '.$value;
+                $subject[] = 'Re:';
               }
             }
 
-            $mailer->setSubject($value);
+            $subject[] = trim(idx($params, 'subject-prefix'));
+
+            $vary_prefix = idx($params, 'vary-subject-prefix');
+            if ($vary_prefix != '') {
+              $use_subject = PhabricatorEnv::getEnvConfig(
+                'metamta.vary-subjects');
+
+              if ($prefs) {
+                $use_subject = $prefs->getPreference(
+                  PhabricatorUserPreferences::PREFERENCE_VARY_SUBJECT,
+                  $use_subject);
+              }
+
+              if ($use_subject) {
+                $subject[] = $vary_prefix;
+              }
+            }
+
+            $subject[] = $value;
+
+            $mailer->setSubject(implode(' ', array_filter($subject)));
             break;
           case 'is-html':
             if ($value) {
@@ -491,7 +502,8 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
           case 'mailtags':
             // Handled below.
             break;
-          case 'vary-subject':
+          case 'subject-prefix':
+          case 'vary-subject-prefix':
             // Handled above.
             break;
           default:

src/applications/owners/mail/PackageMail.php

@@ -181,15 +181,13 @@ abstract class PackageMail {
     $package = $this->getPackage();
     $prefix = PhabricatorEnv::getEnvConfig('metamta.package.subject-prefix');
     $verb = $this->getVerb();
-    $package_title = $this->renderPackageTitle();
-    $subject = trim("{$prefix} {$package_title}");
-    $vary_subject = trim("{$prefix} [{$verb}] {$package_title}");
     $threading = $this->getMailThreading();
     list($thread_id, $thread_topic) = $threading;
 
     $template = id(new PhabricatorMetaMTAMail())
-      ->setSubject($subject)
+      ->setSubject($this->renderPackageTitle())
-      ->setVarySubject($vary_subject)
+      ->setSubjectPrefix($prefix)
+      ->setVarySubjectPrefix("[{$verb}]")
       ->setFrom($package->getActorPHID())
       ->setThreadID($thread_id, $this->isNewThread())
       ->addHeader('Thread-Topic', $thread_topic)

src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php

@@ -152,17 +152,15 @@ EOBODY;
 
     $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());
     list($thread_id, $thread_topic) = $threading;
 
     $template = new PhabricatorMetaMTAMail();
     $template->setRelatedPHID($commit->getPHID());
-    $template->setSubject($subject);
+    $template->setSubject("{$commit_name}: {$name}");
-    $template->setVarySubject($subject);
+    $template->setSubjectPrefix($prefix);
+    $template->setVarySubjectPrefix("[Commit]");
     $template->setBody($body);
     $template->setThreadID($thread_id, $is_new = true);
     $template->addHeader('Thread-Topic', $thread_topic);