Centralize rendering of application mail bodies

Summary: This is a minor quality-of-life improvement to prevent D2968 from being as nasty as it is.

Test Plan: Ran unit tests; generated Differential, Maniphest and Diffusion emails and verified the bodies looked sensible.

Reviewers: btrahan, vrana

Reviewed By: vrana

CC: aran

Maniphest Tasks: T931

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

.divinerconfig

@@ -23,7 +23,8 @@
     "search" : "Search",
     "daemon" : "Daemons, Tasks and Workers",
     "irc" : "IRC",
-    "markup" : "Remarkup Extensions"
+    "markup" : "Remarkup Extensions",
+    "metamta" : "MetaMTA (Mail)"
   },
   "engines" : [
     ["DivinerArticleEngine", {}],

src/__phutil_library_map__.php

@@ -751,6 +751,8 @@ phutil_register_library_map(array(
     'PhabricatorMetaMTAEmailBodyParserTestCase' => 'applications/metamta/__tests__/PhabricatorMetaMTAEmailBodyParserTestCase.php',
     'PhabricatorMetaMTAListController' => 'applications/metamta/controller/PhabricatorMetaMTAListController.php',
     'PhabricatorMetaMTAMail' => 'applications/metamta/storage/PhabricatorMetaMTAMail.php',
+    'PhabricatorMetaMTAMailBody' => 'applications/metamta/view/PhabricatorMetaMTAMailBody.php',
+    'PhabricatorMetaMTAMailBodyTestCase' => 'applications/metamta/view/__tests__/PhabricatorMetaMTAMailBodyTestCase.php',
     'PhabricatorMetaMTAMailTestCase' => 'applications/metamta/storage/__tests__/PhabricatorMetaMTAMailTestCase.php',
     'PhabricatorMetaMTAMailingList' => 'applications/metamta/storage/PhabricatorMetaMTAMailingList.php',
     'PhabricatorMetaMTAMailingListEditController' => 'applications/metamta/controller/PhabricatorMetaMTAMailingListEditController.php',
@@ -1763,6 +1765,7 @@ phutil_register_library_map(array(
     'PhabricatorMetaMTAEmailBodyParserTestCase' => 'PhabricatorTestCase',
     'PhabricatorMetaMTAListController' => 'PhabricatorMetaMTAController',
     'PhabricatorMetaMTAMail' => 'PhabricatorMetaMTADAO',
+    'PhabricatorMetaMTAMailBodyTestCase' => 'PhabricatorTestCase',
     'PhabricatorMetaMTAMailTestCase' => 'PhabricatorTestCase',
     'PhabricatorMetaMTAMailingList' => 'PhabricatorMetaMTADAO',
     'PhabricatorMetaMTAMailingListEditController' => 'PhabricatorMetaMTAController',

src/applications/differential/editor/DifferentialRevisionEditor.php

@@ -251,8 +251,7 @@ final class DifferentialRevisionEditor {
       $adapter->setForbiddenCCs($revision->getUnsubscribedPHIDs());
 
       $xscript = HeraldEngine::loadAndApplyRules($adapter);
-      $xscript_uri = PhabricatorEnv::getProductionURI(
+      $xscript_uri = '/herald/transcript/'.$xscript->getID().'/';
-        '/herald/transcript/'.$xscript->getID().'/');
       $xscript_phid = $xscript->getPHID();
       $xscript_header = $xscript->getXHeraldRulesHeader();
 

src/applications/differential/mail/DifferentialMail.php

@@ -260,34 +260,21 @@ abstract class DifferentialMail {
   }
 
   protected function buildBody() {
+    $main_body = $this->renderBody();
 
-    $body = $this->renderBody();
+    $body = new PhabricatorMetaMTAMailBody();
+    $body->addRawSection($main_body);
 
     $reply_handler = $this->getReplyHandler();
-    $reply_instructions = $reply_handler->getReplyHandlerInstructions();
+    $body->addReplySection($reply_handler->getReplyHandlerInstructions());
-    if ($reply_instructions) {
-      $body .=
-        "\nREPLY HANDLER ACTIONS\n".
-        "  {$reply_instructions}\n";
-    }
 
     if ($this->getHeraldTranscriptURI() && $this->isFirstMailToRecipients()) {
-      $manage_uri = PhabricatorEnv::getProductionURI(
+      $manage_uri = '/herald/view/differential/';
-        '/herald/view/differential/');
-
       $xscript_uri = $this->getHeraldTranscriptURI();
-      $body .= <<<EOTEXT
+      $body->addHeraldSection($manage_uri, $xscript_uri);
-
-MANAGE HERALD DIFFERENTIAL RULES
-  {$manage_uri}
-
-WHY DID I GET THIS EMAIL?
-  {$xscript_uri}
-
-EOTEXT;
     }
 
-    return $body;
+    return $body->render();
   }
 
   /**

src/applications/maniphest/editor/ManiphestTransactionEditor.php

@@ -245,7 +245,7 @@ final class ManiphestTransactionEditor {
     $view->setTransactionGroup($transactions);
     $view->setHandles($handles);
     $view->setAuxiliaryFields($this->auxiliaryFields);
-    list($action, $body) = $view->renderForEmail($with_date = false);
+    list($action, $main_body) = $view->renderForEmail($with_date = false);
 
     $is_create = $this->isCreate($transactions);
 
@@ -253,25 +253,13 @@ final class ManiphestTransactionEditor {
 
     $reply_handler = $this->buildReplyHandler($task);
 
+    $body = new PhabricatorMetaMTAMailBody();
+    $body->addRawSection($main_body);
     if ($is_create) {
-      $body .=
+      $body->addTextSection(pht('TASK DESCRIPTION'), $task->getDescription());
-        "\n\n".
-        "TASK DESCRIPTION\n".
-        "  ".$task->getDescription();
-    }
-
-    $body .=
-      "\n\n".
-      "TASK DETAIL\n".
-      "  ".$task_uri."\n";
-
-    $reply_instructions = $reply_handler->getReplyHandlerInstructions();
-    if ($reply_instructions) {
-      $body .=
-        "\n".
-        "REPLY HANDLER ACTIONS\n".
-        "  ".$reply_instructions."\n";
     }
+    $body->addTextSection(pht('TASK DETAIL'), $task_uri);
+    $body->addReplySection($reply_handler->getReplyHandlerInstructions());
 
     $thread_id = 'maniphest-task-'.$task->getPHID();
     $task_id = $task->getID();
@@ -290,7 +278,7 @@ final class ManiphestTransactionEditor {
       ->setRelatedPHID($task->getPHID())
       ->setIsBulk(true)
       ->setMailTags($mailtags)
-      ->setBody($body);
+      ->setBody($body->render());
 
     $mails = $reply_handler->multiplexMail(
       $template,

src/applications/metamta/view/PhabricatorMetaMTAMailBody.php

@@ -0,0 +1,125 @@
+<?php
+
+/*
+ * Copyright 2012 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/**
+ * Render the body of an application email by building it up section-by-section.
+ *
+ * @task compose  Composition
+ * @task render   Rendering
+ * @group metamta
+ */
+final class PhabricatorMetaMTAMailBody {
+
+  private $sections = array();
+
+
+/* -(  Composition  )-------------------------------------------------------- */
+
+
+  /**
+   * Add a raw block of text to the email. This will be rendered as-is.
+   *
+   * @param string Block of text.
+   * @return this
+   * @task compose
+   */
+  public function addRawSection($text) {
+    if (strlen($text)) {
+      $this->sections[] = rtrim($text);
+    }
+    return $this;
+  }
+
+
+  /**
+   * Add a block of text with a section header. This is rendered like this:
+   *
+   *    HEADER
+   *      Text is indented.
+   *
+   * @param string Header text.
+   * @param string Section text.
+   * @return this
+   * @task compose
+   */
+  public function addTextSection($header, $text) {
+    $this->sections[] = $header."\n".$this->indent($text);
+    return $this;
+  }
+
+
+  /**
+   * Add a Herald section with a rule management URI and a transcript URI.
+   *
+   * @param string URI to rule management.
+   * @param string URI to rule transcripts.
+   * @return this
+   * @task compose
+   */
+  public function addHeraldSection($rules_uri, $xscript_uri) {
+    $this->addTextSection(
+      pht('MANAGE HERALD RULES'),
+      PhabricatorEnv::getProductionURI($rules_uri));
+    $this->addTextSection(
+      pht('WHY DID I GET THIS EMAIL?'),
+      PhabricatorEnv::getProductionURI($xscript_uri));
+    return $this;
+  }
+
+
+  /**
+   * Add a section with reply handler instructions.
+   *
+   * @param string Reply handler instructions.
+   * @return this
+   * @task compose
+   */
+  public function addReplySection($instructions) {
+    if (strlen($instructions)) {
+      $this->addTextSection(pht('REPLY HANDLER ACTIONS'), $instructions);
+    }
+    return $this;
+  }
+
+
+/* -(  Rendering  )---------------------------------------------------------- */
+
+
+  /**
+   * Render the email body.
+   *
+   * @return string Rendered body.
+   * @task render
+   */
+  public function render() {
+    return implode("\n\n", $this->sections)."\n";
+  }
+
+
+  /**
+   * Indent a block of text for rendering under a section heading.
+   *
+   * @param string Text to indent.
+   * @return string Indented text.
+   * @task render
+   */
+  private function indent($text) {
+    return rtrim("  ".str_replace("\n", "\n  ", $text));
+  }
+
+}

src/applications/metamta/view/__tests__/PhabricatorMetaMTAMailBodyTestCase.php

@@ -0,0 +1,57 @@
+<?php
+
+/*
+ * Copyright 2012 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/**
+ * @group metamta
+ */
+final class PhabricatorMetaMTAMailBodyTestCase extends PhabricatorTestCase {
+
+  public function testBodyRender() {
+
+    $env = PhabricatorEnv::beginScopedEnv();
+    $env->overrideEnvConfig('phabricator.base-uri', 'http://test.com/');
+
+    $body = new PhabricatorMetaMTAMailBody();
+    $body->addRawSection("salmon");
+    $body->addTextSection("HEADER", "bass\ntrout\n");
+    $body->addHeraldSection("/rules/", "/xscript/");
+    $body->addReplySection("pike");
+
+    $expect = <<<EOTEXT
+salmon
+
+HEADER
+  bass
+  trout
+
+MANAGE HERALD RULES
+  http://test.com/rules/
+
+WHY DID I GET THIS EMAIL?
+  http://test.com/xscript/
+
+REPLY HANDLER ACTIONS
+  pike
+
+EOTEXT;
+
+    $this->assertEqual($expect, $body->render());
+  }
+
+
+}

src/applications/repository/worker/PhabricatorRepositoryCommitHeraldWorker.php

@@ -111,49 +111,29 @@ final class PhabricatorRepositoryCommitHeraldWorker
 
     $files = $adapter->loadAffectedPaths();
     sort($files);
-    $files = implode("\n  ", $files);
+    $files = implode("\n", $files);
 
     $xscript_id = $xscript->getID();
 
-    $manage_uri = PhabricatorEnv::getProductionURI('/herald/view/commits/');
+    $manage_uri = '/herald/view/commits/';
-    $why_uri = PhabricatorEnv::getProductionURI(
+    $why_uri = '/herald/transcript/'.$xscript_id.'/';
-      '/herald/transcript/'.$xscript_id.'/');
 
     $reply_handler = PhabricatorAuditCommentEditor::newReplyHandlerForCommit(
       $commit);
 
-    $reply_instructions = $reply_handler->getReplyHandlerInstructions();
-    if ($reply_instructions) {
-      $reply_instructions =
-        "\n".
-        "REPLY HANDLER ACTIONS\n".
-        "  ".$reply_instructions."\n";
-    }
-
     $template = new PhabricatorMetaMTAMail();
 
     $inline_patch_text = $this->buildPatch($template, $repository, $commit);
 
-    $body = <<<EOBODY
+    $body = new PhabricatorMetaMTAMailBody();
-DESCRIPTION
+    $body->addRawSection($description);
-{$description}
+    $body->addTextSection(pht('DETAILS'), $commit_uri);
-
+    $body->addTextSection(pht('DIFFERENTIAL REVISION'), $differential);
-DETAILS
+    $body->addTextSection(pht('AFFECTED FILES'), $files);
-  {$commit_uri}
+    $body->addReplySection($reply_handler->getReplyHandlerInstructions());
-
+    $body->addHeraldSection($manage_uri, $why_uri);
-DIFFERENTIAL REVISION
+    $body->addRawSection($inline_patch_text);
-  {$differential}
+    $body = $body->render();
-
-AFFECTED FILES
-  {$files}
-{$reply_instructions}
-MANAGE HERALD COMMIT RULES
-  {$manage_uri}
-
-WHY DID I GET THIS EMAIL?
-  {$why_uri}
-{$inline_patch_text}
-EOBODY;
 
     $prefix = PhabricatorEnv::getEnvConfig('metamta.diffusion.subject-prefix');
 
@@ -369,7 +349,7 @@ EOBODY;
     }
 
     if ($result) {
-      $result = "\nPATCH\n\n{$result}\n";
+      $result = "PATCH\n\n{$result}\n";
     }
 
     return $result;