Permalink
Browse files

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
  • Loading branch information...
1 parent f3dec13 commit 378feb3ffb3fa4123ea3cd80da0c8c43cfb7606c @epriestley epriestley committed Jul 17, 2012
View
@@ -23,7 +23,8 @@
"search" : "Search",
"daemon" : "Daemons, Tasks and Workers",
"irc" : "IRC",
- "markup" : "Remarkup Extensions"
+ "markup" : "Remarkup Extensions",
+ "metamta" : "MetaMTA (Mail)"
},
"engines" : [
["DivinerArticleEngine", {}],
@@ -751,6 +751,8 @@
'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 @@
'PhabricatorMetaMTAEmailBodyParserTestCase' => 'PhabricatorTestCase',
'PhabricatorMetaMTAListController' => 'PhabricatorMetaMTAController',
'PhabricatorMetaMTAMail' => 'PhabricatorMetaMTADAO',
+ 'PhabricatorMetaMTAMailBodyTestCase' => 'PhabricatorTestCase',
'PhabricatorMetaMTAMailTestCase' => 'PhabricatorTestCase',
'PhabricatorMetaMTAMailingList' => 'PhabricatorMetaMTADAO',
'PhabricatorMetaMTAMailingListEditController' => 'PhabricatorMetaMTAController',
@@ -251,8 +251,7 @@ public function save() {
$adapter->setForbiddenCCs($revision->getUnsubscribedPHIDs());
$xscript = HeraldEngine::loadAndApplyRules($adapter);
- $xscript_uri = PhabricatorEnv::getProductionURI(
- '/herald/transcript/'.$xscript->getID().'/');
+ $xscript_uri = '/herald/transcript/'.$xscript->getID().'/';
$xscript_phid = $xscript->getPHID();
$xscript_header = $xscript->getXHeraldRulesHeader();
@@ -260,34 +260,21 @@ protected function prepareBody() {
}
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();
- if ($reply_instructions) {
- $body .=
- "\nREPLY HANDLER ACTIONS\n".
- " {$reply_instructions}\n";
- }
+ $body->addReplySection($reply_handler->getReplyHandlerInstructions());
if ($this->getHeraldTranscriptURI() && $this->isFirstMailToRecipients()) {
- $manage_uri = PhabricatorEnv::getProductionURI(
- '/herald/view/differential/');
-
+ $manage_uri = '/herald/view/differential/';
$xscript_uri = $this->getHeraldTranscriptURI();
- $body .= <<<EOTEXT
-
-MANAGE HERALD DIFFERENTIAL RULES
- {$manage_uri}
-
-WHY DID I GET THIS EMAIL?
- {$xscript_uri}
-
-EOTEXT;
+ $body->addHeraldSection($manage_uri, $xscript_uri);
}
- return $body;
+ return $body->render();
}
/**
@@ -245,33 +245,21 @@ private function sendEmail($task, $transactions, $email_to, $email_cc) {
$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);
$task_uri = PhabricatorEnv::getURI('/T'.$task->getID());
$reply_handler = $this->buildReplyHandler($task);
+ $body = new PhabricatorMetaMTAMailBody();
+ $body->addRawSection($main_body);
if ($is_create) {
- $body .=
- "\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 DESCRIPTION'), $task->getDescription());
}
+ $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 @@ private function sendEmail($task, $transactions, $email_to, $email_cc) {
->setRelatedPHID($task->getPHID())
->setIsBulk(true)
->setMailTags($mailtags)
- ->setBody($body);
+ ->setBody($body->render());
$mails = $reply_handler->multiplexMail(
$template,
@@ -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));
+ }
+
+}
@@ -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());
+ }
+
+
+}
@@ -111,49 +111,29 @@ public function parseCommit(
$files = $adapter->loadAffectedPaths();
sort($files);
- $files = implode("\n ", $files);
+ $files = implode("\n", $files);
$xscript_id = $xscript->getID();
- $manage_uri = PhabricatorEnv::getProductionURI('/herald/view/commits/');
- $why_uri = PhabricatorEnv::getProductionURI(
- '/herald/transcript/'.$xscript_id.'/');
+ $manage_uri = '/herald/view/commits/';
+ $why_uri = '/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
-DESCRIPTION
-{$description}
-
-DETAILS
- {$commit_uri}
-
-DIFFERENTIAL REVISION
- {$differential}
-
-AFFECTED FILES
- {$files}
-{$reply_instructions}
-MANAGE HERALD COMMIT RULES
- {$manage_uri}
-
-WHY DID I GET THIS EMAIL?
- {$why_uri}
-{$inline_patch_text}
-EOBODY;
+ $body = new PhabricatorMetaMTAMailBody();
+ $body->addRawSection($description);
+ $body->addTextSection(pht('DETAILS'), $commit_uri);
+ $body->addTextSection(pht('DIFFERENTIAL REVISION'), $differential);
+ $body->addTextSection(pht('AFFECTED FILES'), $files);
+ $body->addReplySection($reply_handler->getReplyHandlerInstructions());
+ $body->addHeraldSection($manage_uri, $why_uri);
+ $body->addRawSection($inline_patch_text);
+ $body = $body->render();
$prefix = PhabricatorEnv::getEnvConfig('metamta.diffusion.subject-prefix');
@@ -369,7 +349,7 @@ function_exists('mb_convert_encoding')) {
}
if ($result) {
- $result = "\nPATCH\n\n{$result}\n";
+ $result = "PATCH\n\n{$result}\n";
}
return $result;

0 comments on commit 378feb3

Please sign in to comment.