Drive Differential review request e-mail message by field specification

Summary:
This also changes some stuff:

- Reviewers used to be at top, now they are under comments.
- Primary reviewer is now rendered as first.

Test Plan: Added `renderValueForMail()` to a custom field, created diff, commented on it and verified e-mails.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

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

src/__phutil_library_map__.php

@@ -268,6 +268,7 @@ phutil_register_library_map(array(
     'DifferentialLintStatus' => 'applications/differential/constants/DifferentialLintStatus.php',
     'DifferentialLocalCommitsView' => 'applications/differential/view/DifferentialLocalCommitsView.php',
     'DifferentialMail' => 'applications/differential/mail/DifferentialMail.php',
+    'DifferentialMailPhase' => 'applications/differential/constants/DifferentialMailPhase.php',
     'DifferentialManiphestTasksFieldSpecification' => 'applications/differential/field/specification/DifferentialManiphestTasksFieldSpecification.php',
     'DifferentialNewDiffMail' => 'applications/differential/mail/DifferentialNewDiffMail.php',
     'DifferentialPathFieldSpecification' => 'applications/differential/field/specification/DifferentialPathFieldSpecification.php',

src/applications/differential/constants/DifferentialMailPhase.php

@@ -0,0 +1,25 @@
+<?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.
+ */
+
+final class DifferentialMailPhase {
+
+  const WELCOME = 1;
+  const UPDATE = 2;
+  const COMMENT = 3;
+
+}

src/applications/differential/field/selector/DifferentialDefaultFieldSelector.php

@@ -99,9 +99,11 @@ final class DifferentialDefaultFieldSelector
     $map = array_select_keys(
       $map,
       array(
+        'DifferentialReviewersFieldSpecification',
         'DifferentialSummaryFieldSpecification',
         'DifferentialTestPlanFieldSpecification',
         'DifferentialRevisionIDFieldSpecification',
+        'DifferentialManiphestTasksFieldSpecification',
         'DifferentialBranchFieldSpecification',
         'DifferentialCommitsFieldSpecification',
       )) + $map;

src/applications/differential/field/specification/DifferentialBranchFieldSpecification.php

@@ -38,7 +38,7 @@ final class DifferentialBranchFieldSpecification
     return phutil_escape_html($branch);
   }
 
-  public function renderValueForMail() {
+  public function renderValueForMail($phase) {
     $status = $this->getRevision()->getStatus();
 
     if ($status != ArcanistDifferentialRevisionStatus::NEEDS_REVISION &&

src/applications/differential/field/specification/DifferentialCommitsFieldSpecification.php

@@ -50,7 +50,7 @@ final class DifferentialCommitsFieldSpecification
     return $revision->getCommitPHIDs();
   }
 
-  public function renderValueForMail() {
+  public function renderValueForMail($phase) {
     $revision = $this->getRevision();
 
     if ($revision->getStatus() != ArcanistDifferentialRevisionStatus::CLOSED) {

src/applications/differential/field/specification/DifferentialFieldSpecification.php

@@ -375,11 +375,12 @@ abstract class DifferentialFieldSpecification {
    * Return plain text to render in e-mail messages. The text may span
    * multiple lines.
    *
+   * @return int One of DifferentialMailPhase constants.
    * @return string|null Plain text, or null for no message.
    *
    * @task mail
    */
-  public function renderValueForMail() {
+  public function renderValueForMail($phase) {
     return null;
   }
 

src/applications/differential/field/specification/DifferentialManiphestTasksFieldSpecification.php

@@ -156,4 +156,23 @@ final class DifferentialManiphestTasksFieldSpecification
     return $task_phids;
   }
 
+  public function renderValueForMail($phase) {
+    if ($phase == DifferentialMailPhase::COMMENT) {
+      return null;
+    }
+
+    if (!$this->maniphestTasks) {
+      return null;
+    }
+
+    $handles = id(new PhabricatorObjectHandleData($this->maniphestTasks))
+      ->loadHandles();
+    $body = array();
+    $body[] = 'MANIPHEST TASKS';
+    foreach ($handles as $handle) {
+      $body[] = '  '.PhabricatorEnv::getProductionURI($handle->getURI());
+    }
+    return implode("\n", $body);
+  }
+
 }

src/applications/differential/field/specification/DifferentialReviewersFieldSpecification.php

@@ -164,4 +164,22 @@ final class DifferentialReviewersFieldSpecification
     return array();
   }
 
+  public function renderValueForMail($phase) {
+    if ($phase == DifferentialMailPhase::COMMENT) {
+      return null;
+    }
+
+    if (!$this->reviewers) {
+      return null;
+    }
+
+    $handles = id(new PhabricatorObjectHandleData($this->reviewers))
+      ->loadHandles();
+    $handles = array_select_keys(
+      $handles,
+      array($this->getRevision()->getPrimaryReviewer())) + $handles;
+    $names = mpull($handles, 'getName');
+    return 'Reviewers: '.implode(', ', $names);
+  }
+
 }

src/applications/differential/field/specification/DifferentialRevisionIDFieldSpecification.php

@@ -104,7 +104,7 @@ final class DifferentialRevisionIDFieldSpecification
     return 'D'.$revision->getID();
   }
 
-  public function renderValueForMail() {
+  public function renderValueForMail($phase) {
     $uri = PhabricatorEnv::getProductionURI('/D'.$this->id);
     return "REVISION DETAIL\n  {$uri}";
   }

src/applications/differential/field/specification/DifferentialSummaryFieldSpecification.php

@@ -74,4 +74,16 @@ final class DifferentialSummaryFieldSpecification
     return (string)$value;
   }
 
+  public function renderValueForMail($phase) {
+    if ($phase != DifferentialMailPhase::WELCOME) {
+      return null;
+    }
+
+    if ($this->summary == '') {
+      return null;
+    }
+
+    return $this->summary;
+  }
+
 }

src/applications/differential/field/specification/DifferentialTestPlanFieldSpecification.php

@@ -107,6 +107,18 @@ final class DifferentialTestPlanFieldSpecification
     return $value;
   }
 
+  public function renderValueForMail($phase) {
+    if ($phase != DifferentialMailPhase::WELCOME) {
+      return null;
+    }
+
+    if ($this->plan == '') {
+      return null;
+    }
+
+    return "TEST PLAN\n".preg_replace('/^/m', '  ', $this->plan);
+  }
+
   private function isRequired() {
     return PhabricatorEnv::getEnvConfig('differential.require-test-plan-field');
   }

src/applications/differential/mail/DifferentialCCWelcomeMail.php

@@ -29,8 +29,6 @@ final class DifferentialCCWelcomeMail extends DifferentialReviewRequestMail {
     $body = array();
 
     $body[] = "{$actor} added you to the CC list for the revision \"{$name}\".";
-    $body[] = $this->renderReviewersLine();
-    $body[] = null;
 
     $body[] = $this->renderReviewRequestBody();
 

src/applications/differential/mail/DifferentialCommentMail.php

@@ -161,18 +161,7 @@ final class DifferentialCommentMail extends DifferentialMail {
       $body[] = null;
     }
 
-    $selector = DifferentialFieldSelector::newSelector();
+    $body[] = $this->renderAuxFields(DifferentialMailPhase::COMMENT);
-    $aux_fields = $selector->sortFieldsForMail(
-      $selector->getFieldSpecifications());
-
-    foreach ($aux_fields as $field) {
-      $field->setRevision($this->getRevision());
-      $text = $field->renderValueForMail();
-      if ($text !== null) {
-        $body[] = $text;
-        $body[] = null;
-      }
-    }
 
     return implode("\n", $body);
   }

src/applications/differential/mail/DifferentialMail.php

@@ -354,11 +354,6 @@ EOTEXT;
     return $this->changesets;
   }
 
-  protected function getManiphestTaskPHIDs() {
-    return $this->getRevision()->getAttachedPHIDs(
-      PhabricatorPHIDConstants::PHID_TYPE_TASK);
-  }
-
   public function setInlineComments(array $inline_comments) {
     assert_instances_of($inline_comments, 'PhabricatorInlineCommentInterface');
     $this->inlineComments = $inline_comments;
@@ -369,6 +364,24 @@ EOTEXT;
     return $this->inlineComments;
   }
 
+  protected function renderAuxFields($phase) {
+    $selector = DifferentialFieldSelector::newSelector();
+    $aux_fields = $selector->sortFieldsForMail(
+      $selector->getFieldSpecifications());
+
+    $body = array();
+    foreach ($aux_fields as $field) {
+      $field->setRevision($this->getRevision());
+      $text = $field->renderValueForMail($phase);
+      if ($text !== null) {
+        $body[] = $text;
+        $body[] = null;
+      }
+    }
+
+    return implode("\n", $body);
+  }
+
   public function renderRevisionDetailLink() {
     $uri = $this->getRevisionURI();
     return "REVISION DETAIL\n  {$uri}";

src/applications/differential/mail/DifferentialNewDiffMail.php

@@ -44,8 +44,6 @@ final class DifferentialNewDiffMail extends DifferentialReviewRequestMail {
     } else {
       $body[] = "{$actor} updated the revision \"{$name}\".";
     }
-    $body[] = $this->renderReviewersLine();
-    $body[] = null;
 
     $body[] = $this->renderReviewRequestBody();
 

src/applications/differential/mail/DifferentialReviewRequestMail.php

@@ -40,47 +40,21 @@ abstract class DifferentialReviewRequestMail extends DifferentialMail {
     $this->setChangesets($changesets);
   }
 
-  protected function renderReviewersLine() {
-    $reviewers = $this->getRevision()->getReviewers();
-    $handles = id(new PhabricatorObjectHandleData($reviewers))->loadHandles();
-    return 'Reviewers: '.$this->renderHandleList($handles, $reviewers);
-  }
-
   protected function renderReviewRequestBody() {
     $revision = $this->getRevision();
 
     $body = array();
-    if ($this->isFirstMailToRecipients()) {
+    if (!$this->isFirstMailToRecipients()) {
-      if ($revision->getSummary() != '') {
-        $body[] = $this->formatText($revision->getSummary());
-        $body[] = null;
-      }
-
-      if ($revision->getTestPlan() != '') {
-        $body[] = 'TEST PLAN';
-        $body[] = $this->formatText($revision->getTestPlan());
-        $body[] = null;
-      }
-    } else {
       if (strlen($this->getComments())) {
         $body[] = $this->formatText($this->getComments());
         $body[] = null;
       }
     }
 
-    $body[] = $this->renderRevisionDetailLink();
+    $phase = ($this->isFirstMailToRecipients() ?
-    $body[] = null;
+      DifferentialMailPhase::WELCOME :
-
+      DifferentialMailPhase::UPDATE);
-    $task_phids = $this->getManiphestTaskPHIDs();
+    $body[] = $this->renderAuxFields($phase);
-    if ($task_phids) {
-      $handles = id(new PhabricatorObjectHandleData($task_phids))
-        ->loadHandles();
-      $body[] = 'MANIPHEST TASKS';
-      foreach ($handles as $handle) {
-        $body[] = '  '.PhabricatorEnv::getProductionURI($handle->getURI());
-      }
-      $body[] = null;
-    }
 
     $changesets = $this->getChangesets();
     if ($changesets) {