Allow Differential Mail to accept multiple comments for one mail

Summary:
Ref T2222. Currently, one `DifferentialComment` can do a lot of things (add ccs, reviewers, comments, inline comments, and perform state changes). In the future, each `ApplicationTransaction` does only one thing. This is the primary piece of complexity which makes the upcoming migration risky, because each comment needs to migrate into multiple transactions.

I want to mitigate this complexity as much as possible before the migration itself happens. One approach I'm going to use to do that is to start writing one comment per effect now, so the mapping is more direct when the migration itself happens and the write code can be straightforward (one row per save()) after the migration.

This tackles a small piece of that, which is the mail Differential sends. Currently, Differential mail acts on a single comment. Instead, allow it to act on a list of comments, but always give it one comment for now. In the future, we can hand it several comments instead and still get the expected behavior.

This change should have no impact on any application behaviors.

Test Plan:
  - Commented;
  - commented with inline;
  - added reviewers;
  - added CCs;
  - added CCs via mentions;
  - updated revision;
  - looked at all the mail, all of which seemed sane.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

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

src/applications/differential/editor/DifferentialCommentEditor.php

@@ -647,7 +647,7 @@ final class DifferentialCommentEditor extends PhabricatorEditor {
       $mail = id(new DifferentialCommentMail(
         $revision,
         $actor_handle,
-        $comment,
+        array($comment),
         $changesets,
         $inline_comments))
         ->setActor($actor)

src/applications/differential/editor/DifferentialRevisionEditor.php

@@ -381,24 +381,21 @@ final class DifferentialRevisionEditor extends PhabricatorEditor {
     $actor_handle = $handles[$this->getActorPHID()];
 
     $changesets = null;
-    $comment = null;
     $old_status = $revision->getStatus();
 
     if ($diff) {
       $changesets = $diff->loadChangesets();
       // TODO: This should probably be in DifferentialFeedbackEditor?
       if (!$is_new) {
-        $comment = $this->createComment();
-      }
-      if ($comment) {
+        $this->createComment();
         $mail[] = id(new DifferentialNewDiffMail(
             $revision,
             $actor_handle,
             $changesets))
           ->setActor($this->getActor())
-          ->setIsFirstMailAboutRevision($is_new)
-          ->setIsFirstMailToRecipients($is_new)
-          ->setComments($this->getComments())
+          ->setIsFirstMailAboutRevision(false)
+          ->setIsFirstMailToRecipients(false)
+          ->setCommentText($this->getComments())
           ->setToPHIDs(array_keys($stable['rev']))
           ->setCCPHIDs(array_keys($stable['ccs']));
       }
@@ -804,8 +801,6 @@ final class DifferentialRevisionEditor extends PhabricatorEditor {
     }
 
     $comment->save();
-
-    return $comment;
   }
 
   private function updateAuxiliaryFields() {

src/applications/differential/mail/DifferentialCommentMail.php

@@ -19,56 +19,60 @@ final class DifferentialCommentMail extends DifferentialMail {
   public function __construct(
     DifferentialRevision $revision,
     PhabricatorObjectHandle $actor,
-    DifferentialComment $comment,
+    array $comments,
     array $changesets,
     array $inline_comments) {
+
+    assert_instances_of($comments, 'DifferentialComment');
     assert_instances_of($changesets, 'DifferentialChangeset');
     assert_instances_of($inline_comments, 'PhabricatorInlineCommentInterface');
 
     $this->setRevision($revision);
     $this->setActorHandle($actor);
-    $this->setComment($comment);
+    $this->setComments($comments);
     $this->setChangesets($changesets);
     $this->setInlineComments($inline_comments);
 
   }
 
   protected function getMailTags() {
-    $tags    = array();
-    $comment = $this->getComment();
-    $action  = $comment->getAction();
+    $tags = array();
 
-    switch ($action) {
-      case DifferentialAction::ACTION_ADDCCS:
-        $tags[] = MetaMTANotificationType::TYPE_DIFFERENTIAL_CC;
-        break;
-      case DifferentialAction::ACTION_CLOSE:
-        $tags[] = MetaMTANotificationType::TYPE_DIFFERENTIAL_CLOSED;
-        break;
-      case DifferentialAction::ACTION_ADDREVIEWERS:
-        $tags[] = MetaMTANotificationType::TYPE_DIFFERENTIAL_REVIEWERS;
-        break;
-      case DifferentialAction::ACTION_COMMENT:
-        // this is a comment which we will check separately below for content
-        break;
-      default:
-        $tags[] = MetaMTANotificationType::TYPE_DIFFERENTIAL_OTHER;
-        break;
-    }
+    foreach ($this->getComments() as $comment) {
+      $action = $comment->getAction();
 
-    $has_comment = strlen(trim($comment->getContent()));
-    $has_inlines = (bool)$this->getInlineComments();
-
-    if ($has_comment || $has_inlines) {
       switch ($action) {
+        case DifferentialAction::ACTION_ADDCCS:
+          $tags[] = MetaMTANotificationType::TYPE_DIFFERENTIAL_CC;
+          break;
         case DifferentialAction::ACTION_CLOSE:
-          // Commit comments are auto-generated and not especially interesting,
-          // so don't tag them as having a comment.
+          $tags[] = MetaMTANotificationType::TYPE_DIFFERENTIAL_CLOSED;
+          break;
+        case DifferentialAction::ACTION_ADDREVIEWERS:
+          $tags[] = MetaMTANotificationType::TYPE_DIFFERENTIAL_REVIEWERS;
+          break;
+        case DifferentialAction::ACTION_COMMENT:
+          // this is a comment which we will check separately below for content
           break;
         default:
-          $tags[] = MetaMTANotificationType::TYPE_DIFFERENTIAL_COMMENT;
+          $tags[] = MetaMTANotificationType::TYPE_DIFFERENTIAL_OTHER;
           break;
       }
+
+      $has_comment = strlen(trim($comment->getContent()));
+      $has_inlines = (bool)$this->getInlineComments();
+
+      if ($has_comment || $has_inlines) {
+        switch ($action) {
+          case DifferentialAction::ACTION_CLOSE:
+            // Commit comments are auto-generated and not especially
+            // interesting, so don't tag them as having a comment.
+            break;
+          default:
+            $tags[] = MetaMTANotificationType::TYPE_DIFFERENTIAL_COMMENT;
+            break;
+        }
+      }
     }
 
     if (!$tags) {
@@ -84,7 +88,9 @@ final class DifferentialCommentMail extends DifferentialMail {
   }
 
   protected function getVerb() {
-    $comment = $this->getComment();
+    // NOTE: Eventually, this will use getStrongestAction() transaction logic.
+    // For now, pick the first comment.
+    $comment = head($this->getComments());
     $action = $comment->getAction();
     $verb = DifferentialAction::getActionPastTenseVerb($action);
     return $verb;
@@ -94,16 +100,22 @@ final class DifferentialCommentMail extends DifferentialMail {
     parent::prepareBody();
 
     // If the commented added reviewers or CCs, list them explicitly.
-    $meta = $this->getComment()->getMetadata();
-    $m_reviewers = idx(
-      $meta,
-      DifferentialComment::METADATA_ADDED_REVIEWERS,
-      array());
-    $m_cc = idx(
-      $meta,
-      DifferentialComment::METADATA_ADDED_CCS,
-      array());
-    $load = array_merge($m_reviewers, $m_cc);
+    $load = array();
+    foreach ($this->comments as $comment) {
+      $meta = $comment->getMetadata();
+      $m_reviewers = idx(
+        $meta,
+        DifferentialComment::METADATA_ADDED_REVIEWERS,
+        array());
+      $m_cc = idx(
+        $meta,
+        DifferentialComment::METADATA_ADDED_CCS,
+        array());
+      $load[] = $m_reviewers;
+      $load[] = $m_cc;
+    }
+
+    $load = array_mergev($load);
     if ($load) {
       $handles = id(new PhabricatorHandleQuery())
         ->setViewer($this->getActor())
@@ -119,8 +131,10 @@ final class DifferentialCommentMail extends DifferentialMail {
   }
 
   protected function renderBody() {
+    // TODO: This will be ApplicationTransactions eventually, but split the
+    // difference for now.
 
-    $comment = $this->getComment();
+    $comment = head($this->getComments());
 
     $actor = $this->getActorName();
     $name  = $this->getRevision()->getTitle();
@@ -139,10 +153,12 @@ final class DifferentialCommentMail extends DifferentialMail {
 
     $body[] = null;
 
-    $content = $comment->getContent();
-    if (strlen($content)) {
-      $body[] = $this->formatText($content);
-      $body[] = null;
+    foreach ($this->getComments() as $comment) {
+      $content = $comment->getContent();
+      if (strlen($content)) {
+        $body[] = $this->formatText($content);
+        $body[] = null;
+      }
     }
 
     if ($this->getChangedByCommit()) {

src/applications/differential/mail/DifferentialMail.php

@@ -9,7 +9,7 @@ abstract class DifferentialMail extends PhabricatorMail {
   protected $actorHandle;
 
   protected $revision;
-  protected $comment;
+  protected $comments;
   protected $changesets;
   protected $inlineComments;
   protected $isFirstMailAboutRevision;
@@ -370,13 +370,13 @@ abstract class DifferentialMail extends PhabricatorMail {
     return "D{$id}: {$title}";
   }
 
-  public function setComment($comment) {
-    $this->comment = $comment;
+  public function setComments(array $comments) {
+    $this->comments = $comments;
     return $this;
   }
 
-  public function getComment() {
-    return $this->comment;
+  public function getComments() {
+    return $this->comments;
   }
 
   public function setChangesets($changesets) {

src/applications/differential/mail/DifferentialReviewRequestMail.php

@@ -4,17 +4,17 @@ abstract class DifferentialReviewRequestMail extends DifferentialMail {
 
   const MAX_AFFECTED_FILES = 1000;
 
-  protected $comments;
+  protected $commentText;
 
   private $patch;
 
-  public function setComments($comments) {
-    $this->comments = $comments;
+  public function setCommentText($comment_text) {
+    $this->commentText = $comment_text;
     return $this;
   }
 
-  public function getComments() {
-    return $this->comments;
+  public function getCommentText() {
+    return $this->commentText;
   }
 
   public function __construct(
@@ -46,8 +46,8 @@ abstract class DifferentialReviewRequestMail extends DifferentialMail {
 
     $body = array();
     if (!$this->isFirstMailToRecipients()) {
-      if (strlen($this->getComments())) {
-        $body[] = $this->formatText($this->getComments());
+      if (strlen($this->getCommentText())) {
+        $body[] = $this->formatText($this->getCommentText());
         $body[] = null;
       }
     }