From 99034efa8b8d0d7bc32aa28bd78f0aa7cfad12cc Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 5 Oct 2018 11:40:51 -0700
Subject: [PATCH] Make Pholio mail render without a ton of over-escaped HTML

Summary:
Ref T13202. See PHI900. Fixes T12814. Pholio currently builds HTML comments in an older way that can dump a bunch of over-escaped HTML into mail bodies.

Update the logic to be more similar to the Differential rendering logic and stop over-escaping things.

The result isn't perfect, but is dramatically less broken.

Test Plan: {F5919860}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13202, T12814

Differential Revision: https://secure.phabricator.com/D19733
---
 .../editor/DifferentialTransactionEditor.php  |   4 +-
 .../pholio/editor/PholioMockEditor.php        | 104 +++++++++++-------
 .../pholio/storage/PholioMock.php             |   4 +
 .../pholio/storage/PholioTransaction.php      |   9 ++
 4 files changed, 79 insertions(+), 42 deletions(-)

diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php
index c452cb9346..5bc33dabe1 100644
--- a/src/applications/differential/editor/DifferentialTransactionEditor.php
+++ b/src/applications/differential/editor/DifferentialTransactionEditor.php
@@ -636,8 +636,8 @@ final class DifferentialTransactionEditor
 
     $viewer = $this->requireActor();
 
-    $body = new PhabricatorMetaMTAMailBody();
-    $body->setViewer($this->requireActor());
+    $body = id(new PhabricatorMetaMTAMailBody())
+      ->setViewer($viewer);
 
     $revision_uri = $object->getURI();
     $revision_uri = PhabricatorEnv::getProductionURI($revision_uri);
diff --git a/src/applications/pholio/editor/PholioMockEditor.php b/src/applications/pholio/editor/PholioMockEditor.php
index 6bd49d2e7b..d1f7daf3a5 100644
--- a/src/applications/pholio/editor/PholioMockEditor.php
+++ b/src/applications/pholio/editor/PholioMockEditor.php
@@ -128,51 +128,30 @@ final class PholioMockEditor extends PhabricatorApplicationTransactionEditor {
     PhabricatorLiskDAO $object,
     array $xactions) {
 
-    $body = new PhabricatorMetaMTAMailBody();
-    $headers = array();
-    $comments = array();
-    $inline_comments = array();
+    $viewer = $this->requireActor();
 
+    $body = id(new PhabricatorMetaMTAMailBody())
+      ->setViewer($viewer);
+
+    $mock_uri = $object->getURI();
+    $mock_uri = PhabricatorEnv::getProductionURI($mock_uri);
+
+    $this->addHeadersAndCommentsToMailBody(
+      $body,
+      $xactions,
+      pht('View Mock'),
+      $mock_uri);
+
+    $type_inline = PholioMockInlineTransaction::TRANSACTIONTYPE;
+
+    $inlines = array();
     foreach ($xactions as $xaction) {
-      if ($xaction->shouldHide()) {
-        continue;
-      }
-      $comment = $xaction->getComment();
-      switch ($xaction->getTransactionType()) {
-        case PholioMockInlineTransaction::TRANSACTIONTYPE:
-          if ($comment && strlen($comment->getContent())) {
-            $inline_comments[] = $comment;
-          }
-          break;
-        case PhabricatorTransactions::TYPE_COMMENT:
-          if ($comment && strlen($comment->getContent())) {
-            $comments[] = $comment->getContent();
-          }
-        // fallthrough
-        default:
-          $headers[] = id(clone $xaction)
-            ->setRenderingTarget('text')
-            ->getTitle();
-          break;
+      if ($xaction->getTransactionType() == $type_inline) {
+        $inlines[] = $xaction;
       }
     }
 
-    $body->addRawSection(implode("\n", $headers));
-
-    foreach ($comments as $comment) {
-      $body->addRawSection($comment);
-    }
-
-    if ($inline_comments) {
-      $body->addRawSection(pht('INLINE COMMENTS'));
-      foreach ($inline_comments as $comment) {
-        $text = pht(
-          'Image %d: %s',
-          $comment->getImageID(),
-          $comment->getContent());
-        $body->addRawSection($text);
-      }
-    }
+    $this->appendInlineCommentsForMail($object, $inlines, $body);
 
     $body->addLinkSection(
       pht('MOCK DETAIL'),
@@ -181,6 +160,51 @@ final class PholioMockEditor extends PhabricatorApplicationTransactionEditor {
     return $body;
   }
 
+  private function appendInlineCommentsForMail(
+    $object,
+    array $inlines,
+    PhabricatorMetaMTAMailBody $body) {
+
+    if (!$inlines) {
+      return;
+    }
+
+    $viewer = $this->requireActor();
+
+    $header = pht('INLINE COMMENTS');
+    $body->addRawPlaintextSection($header);
+    $body->addRawHTMLSection(phutil_tag('strong', array(), $header));
+
+    $image_ids = array();
+    foreach ($inlines as $inline) {
+      $comment = $inline->getComment();
+      $image_id = $comment->getImageID();
+      $image_ids[$image_id] = $image_id;
+    }
+
+    $images = id(new PholioImageQuery())
+      ->setViewer($viewer)
+      ->withIDs($image_ids)
+      ->execute();
+    $images = mpull($images, null, 'getID');
+
+    foreach ($inlines as $inline) {
+      $comment = $inline->getComment();
+      $content = $comment->getContent();
+      $image_id = $comment->getImageID();
+      $image = idx($images, $image_id);
+      if ($image) {
+        $image_name = $image->getName();
+      } else {
+        $image_name = pht('Unknown (ID %d)', $image_id);
+      }
+
+      $body->addRemarkupSection(
+        pht('Image "%s":', $image_name),
+        $content);
+    }
+  }
+
   protected function getMailSubjectPrefix() {
     return PhabricatorEnv::getEnvConfig('metamta.pholio.subject-prefix');
   }
diff --git a/src/applications/pholio/storage/PholioMock.php b/src/applications/pholio/storage/PholioMock.php
index 523733b3df..569513cb46 100644
--- a/src/applications/pholio/storage/PholioMock.php
+++ b/src/applications/pholio/storage/PholioMock.php
@@ -58,6 +58,10 @@ final class PholioMock extends PholioDAO
     return 'M'.$this->getID();
   }
 
+  public function getURI() {
+    return '/'.$this->getMonogram();
+  }
+
   protected function getConfiguration() {
     return array(
       self::CONFIG_AUX_PHID => true,
diff --git a/src/applications/pholio/storage/PholioTransaction.php b/src/applications/pholio/storage/PholioTransaction.php
index a932cf2a60..240b9d93e6 100644
--- a/src/applications/pholio/storage/PholioTransaction.php
+++ b/src/applications/pholio/storage/PholioTransaction.php
@@ -53,4 +53,13 @@ final class PholioTransaction extends PhabricatorModularTransaction {
     return $tags;
   }
 
+  public function isInlineCommentTransaction() {
+    switch ($this->getTransactionType()) {
+      case PholioMockInlineTransaction::TRANSACTIONTYPE:
+        return true;
+    }
+
+    return parent::isInlineCommentTransaction();
+  }
+
 }