From 6d45a2e09bd6540f6fd5991e09a2445c2ebb0419 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 25 Sep 2013 11:16:55 -0700 Subject: [PATCH] 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 --- .../ManiphestTaskEditController.php | 27 ++- .../editor/ManiphestTransactionEditorPro.php | 6 + .../maniphest/storage/ManiphestTask.php | 4 +- .../storage/ManiphestTransaction.php | 160 +++++++++--------- .../storage/PhabricatorMetaMTAMail.php | 2 +- 5 files changed, 102 insertions(+), 97 deletions(-) diff --git a/src/applications/maniphest/controller/ManiphestTaskEditController.php b/src/applications/maniphest/controller/ManiphestTaskEditController.php index 8d2af82b53..8fc1a313ca 100644 --- 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')); diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditorPro.php b/src/applications/maniphest/editor/ManiphestTransactionEditorPro.php index 8574a45c02..563dd60378 100644 --- 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())); diff --git a/src/applications/maniphest/storage/ManiphestTask.php b/src/applications/maniphest/storage/ManiphestTask.php index 5fa93f7cde..9e5bb93f82 100644 --- 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; diff --git a/src/applications/maniphest/storage/ManiphestTransaction.php b/src/applications/maniphest/storage/ManiphestTransaction.php index e5bec43d24..8514c7cbd3 100644 --- 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: diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php index 1bfcfa4a98..56f751c63d 100644 --- 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; }