Restore some missing features from Maniphest mail

Summary: Ref T2217. Fixes two issues: # The "task created" email didn't include the task description, but should. # We were treaging the "status" event as the "create", but that's kind of a mess. Treat the "title" event as the "create" instead. This makes initial emails say "[Created]". Test Plan: Created some tasks, got better emails. Reviewers: btrahan, chad Reviewed By: chad CC: aran Maniphest Tasks: T2217 Differential Revision: https://secure.phabricator.com/D7115
2013-09-25 11:16:55 -07:00
parent bb4bf01bdc
commit 6d45a2e09b
5 changed files with 102 additions and 97 deletions
--- a/src/applications/maniphest/controller/ManiphestTaskEditController.php
+++ b/src/applications/maniphest/controller/ManiphestTaskEditController.php
@@ -106,22 +106,17 @@ final class ManiphestTaskEditController extends ManiphestController {

      $workflow = '';

-      if ($task->getID()) {
-        if ($new_title != $task->getTitle()) {
-          $changes[ManiphestTransaction::TYPE_TITLE] = $new_title;
-        }
-        if ($new_desc != $task->getDescription()) {
-          $changes[ManiphestTransaction::TYPE_DESCRIPTION] = $new_desc;
-        }
-        if ($new_status != $task->getStatus()) {
-          $changes[ManiphestTransaction::TYPE_STATUS] = $new_status;
-        }
-      } else {
-        $task->setTitle($new_title);
-        $task->setDescription($new_desc);
-        $changes[ManiphestTransaction::TYPE_STATUS] =
-          ManiphestTaskStatus::STATUS_OPEN;
+      if ($new_title != $task->getTitle()) {
+        $changes[ManiphestTransaction::TYPE_TITLE] = $new_title;
+      }
+      if ($new_desc != $task->getDescription()) {
+        $changes[ManiphestTransaction::TYPE_DESCRIPTION] = $new_desc;
+      }
+      if ($new_status != $task->getStatus()) {
+        $changes[ManiphestTransaction::TYPE_STATUS] = $new_status;
+      }

+      if (!$task->getID()) {
        $workflow = 'create';
      }

@@ -166,6 +161,8 @@ final class ManiphestTaskEditController extends ManiphestController {
      }

      if ($errors) {
+        $task->setTitle($new_title);
+        $task->setDescription($new_desc);
        $task->setPriority($request->getInt('priority'));
        $task->setOwnerPHID($owner_phid);
        $task->setCCPHIDs($request->getArr('cc'));
--- a/src/applications/maniphest/editor/ManiphestTransactionEditorPro.php
+++ b/src/applications/maniphest/editor/ManiphestTransactionEditorPro.php
@@ -148,6 +148,12 @@ final class ManiphestTransactionEditorPro

    $body = parent::buildMailBody($object, $xactions);

+    if ($this->getIsNewObject()) {
+      $body->addTextSection(
+        pht('TASK DESCRIPTION'),
+        $object->getDescription());
+    }
+
    $body->addTextSection(
      pht('TASK DETAIL'),
      PhabricatorEnv::getProductionURI('/T'.$object->getID()));
--- a/src/applications/maniphest/storage/ManiphestTask.php
+++ b/src/applications/maniphest/storage/ManiphestTask.php
@@ -22,9 +22,9 @@ final class ManiphestTask extends ManiphestDAO
  protected $priority;
  protected $subpriority = 0;

-  protected $title;
+  protected $title = '';
  protected $originalTitle;
-  protected $description;
+  protected $description = '';
  protected $originalEmailSource;
  protected $mailKey;

--- a/src/applications/maniphest/storage/ManiphestTransaction.php
+++ b/src/applications/maniphest/storage/ManiphestTransaction.php
@@ -80,13 +80,14 @@ final class ManiphestTransaction
    $new = $this->getNewValue();

    switch ($this->getTransactionType()) {
+      case self::TYPE_TITLE:
+        if (!strlen($old)) {
+          return 'green';
+        }
+
      case self::TYPE_STATUS:
        if ($new == ManiphestTaskStatus::STATUS_OPEN) {
-          if ($old) {
-            return 'violet';
-          } else {
-            return 'green';
-          }
+          return 'green';
        } else {
          return 'black';
        }
@@ -111,24 +112,22 @@ final class ManiphestTransaction

    switch ($this->getTransactionType()) {
      case self::TYPE_TITLE:
-        return pht('Retitled');
+        if (!strlen($old)) {
+          return pht('Created');
+        } else {
+          return pht('Retitled');
+        }

      case self::TYPE_STATUS:
-        if ($new == ManiphestTaskStatus::STATUS_OPEN) {
-          if ($old) {
+        switch ($new) {
+          case ManiphestTaskStatus::STATUS_OPEN:
            return pht('Reopened');
-          } else {
-            return pht('Created');
-          }
-        } else {
-          switch ($new) {
-            case ManiphestTaskStatus::STATUS_CLOSED_SPITE:
-              return pht('Spited');
-            case ManiphestTaskStatus::STATUS_CLOSED_DUPLICATE:
-              return pht('Merged');
-            default:
-              return pht('Closed');
-          }
+          case ManiphestTaskStatus::STATUS_CLOSED_SPITE:
+            return pht('Spited');
+          case ManiphestTaskStatus::STATUS_CLOSED_DUPLICATE:
+            return pht('Merged');
+          default:
+            return pht('Closed');
        }

      case self::TYPE_DESCRIPTION:
@@ -174,20 +173,22 @@ final class ManiphestTransaction

    switch ($this->getTransactionType()) {
      case self::TYPE_TITLE:
-        return 'edit';
-
-      case self::TYPE_STATUS:
-        if ($new == ManiphestTaskStatus::STATUS_OPEN) {
+        if (!strlen($old)) {
          return 'create';
        } else {
-          switch ($new) {
-            case ManiphestTaskStatus::STATUS_CLOSED_SPITE:
-              return 'dislike';
-            case ManiphestTaskStatus::STATUS_CLOSED_DUPLICATE:
-              return 'delete';
-            default:
-              return 'check';
-          }
+          return 'edit';
+        }
+
+      case self::TYPE_STATUS:
+        switch ($new) {
+          case ManiphestTaskStatus::STATUS_OPEN:
+            return 'create';
+          case ManiphestTaskStatus::STATUS_CLOSED_SPITE:
+            return 'dislike';
+          case ManiphestTaskStatus::STATUS_CLOSED_DUPLICATE:
+            return 'delete';
+          default:
+            return 'check';
        }

      case self::TYPE_DESCRIPTION:
@@ -225,11 +226,19 @@ final class ManiphestTransaction

    switch ($this->getTransactionType()) {
      case self::TYPE_TITLE:
-        return pht(
-          '%s changed the title from "%s" to "%s".',
-          $this->renderHandleLink($author_phid),
-          $old,
-          $new);
+        if (!strlen($old)) {
+          return pht(
+            '%s created this task.',
+            $this->renderHandleLink($author_phid),
+            $old,
+            $new);
+        } else {
+          return pht(
+            '%s changed the title from "%s" to "%s".',
+            $this->renderHandleLink($author_phid),
+            $old,
+            $new);
+        }

      case self::TYPE_DESCRIPTION:
        return pht(
@@ -237,36 +246,29 @@ final class ManiphestTransaction
          $this->renderHandleLink($author_phid));

      case self::TYPE_STATUS:
-        if ($new == ManiphestTaskStatus::STATUS_OPEN) {
-          if ($old) {
+        switch ($new) {
+          case ManiphestTaskStatus::STATUS_OPEN:
            return pht(
              '%s reopened this task.',
              $this->renderHandleLink($author_phid));
-          } else {
+
+          case ManiphestTaskStatus::STATUS_CLOSED_SPITE:
            return pht(
-              '%s created this task.',
+              '%s closed this task out of spite.',
              $this->renderHandleLink($author_phid));
-          }
-        } else {
-          switch ($new) {
-            case ManiphestTaskStatus::STATUS_CLOSED_SPITE:
-              return pht(
-                '%s closed this task out of spite.',
-                $this->renderHandleLink($author_phid));
-            case ManiphestTaskStatus::STATUS_CLOSED_DUPLICATE:
-              return pht(
-                '%s closed this task as a duplicate.',
-                $this->renderHandleLink($author_phid));
-            default:
-              $status_name = idx(
-                ManiphestTaskStatus::getTaskStatusMap(),
-                $new,
-                '???');
-              return pht(
-                '%s closed this task as "%s".',
-                $this->renderHandleLink($author_phid),
-                $status_name);
-          }
+          case ManiphestTaskStatus::STATUS_CLOSED_DUPLICATE:
+            return pht(
+              '%s closed this task as a duplicate.',
+              $this->renderHandleLink($author_phid));
+          default:
+            $status_name = idx(
+              ManiphestTaskStatus::getTaskStatusMap(),
+              $new,
+              '???');
+            return pht(
+              '%s closed this task as "%s".',
+              $this->renderHandleLink($author_phid),
+              $status_name);
        }

      case self::TYPE_OWNER:
@@ -403,12 +405,19 @@ final class ManiphestTransaction

    switch ($this->getTransactionType()) {
      case self::TYPE_TITLE:
-        return pht(
-          '%s renamed %s from "%s" to "%s".',
-          $this->renderHandleLink($author_phid),
-          $this->renderHandleLink($object_phid),
-          $old,
-          $new);
+        if (!strlen($old)) {
+          return pht(
+            '%s created %s.',
+            $this->renderHandleLink($author_phid),
+            $this->renderHandleLink($object_phid));
+        } else {
+          return pht(
+            '%s renamed %s from "%s" to "%s".',
+            $this->renderHandleLink($author_phid),
+            $this->renderHandleLink($object_phid),
+            $old,
+            $new);
+        }

      case self::TYPE_DESCRIPTION:
        return pht(
@@ -418,17 +427,10 @@ final class ManiphestTransaction

      case self::TYPE_STATUS:
        if ($new == ManiphestTaskStatus::STATUS_OPEN) {
-          if ($old) {
-            return pht(
-              '%s reopened %s.',
-              $this->renderHandleLink($author_phid),
-              $this->renderHandleLink($object_phid));
-          } else {
-            return pht(
-              '%s created %s.',
-              $this->renderHandleLink($author_phid),
-              $this->renderHandleLink($object_phid));
-          }
+          return pht(
+            '%s reopened %s.',
+            $this->renderHandleLink($author_phid),
+            $this->renderHandleLink($object_phid));
        } else {
          switch ($new) {
            case ManiphestTaskStatus::STATUS_CLOSED_SPITE:
--- a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php
+++ b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php
@@ -62,7 +62,7 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
   * @return this
   */
  public function setMailTags(array $tags) {
-    $this->setParam('mailtags', $tags);
+    $this->setParam('mailtags', array_unique($tags));
    return $this;
  }