Group Maniphest transactions.

Summary:

Test Plan:

Reviewers:

CC:

src/applications/maniphest/controller/createtask/ManiphestTaskCreateController.php

@@ -33,34 +33,61 @@ class ManiphestTaskCreateController extends ManiphestController {
     if ($request->isFormPost()) {
       $task->setTitle($request->getStr('title'));
       $task->setAuthorPHID($user->getPHID());
-      $owner_tokenizer = $request->getArr('assigned_to');
-      $task->setOwnerPHID(reset($owner_tokenizer));
-      $task->setCCPHIDs($request->getArr('cc'));
-      $task->setPriority($request->getInt('priority'));
       $task->setDescription($request->getStr('description'));
 
+      $owner_tokenizer = $request->getArr('assigned_to');
+      $owner_phid = reset($owner_tokenizer);
+
       if (!strlen($task->getTitle())) {
         $e_title = 'Required';
         $errors[] = 'Title is required.';
       }
 
       if (!$errors) {
-        $transaction = new ManiphestTransaction();
+        $changes = array();
-        $transaction->setAuthorPHID($user->getPHID());
+
-        $transaction->setTransactionType(ManiphestTransactionType::TYPE_STATUS);
+        $changes[ManiphestTransactionType::TYPE_STATUS] =
-        $transaction->setNewValue(ManiphestTaskStatus::STATUS_OPEN);
+          ManiphestTaskStatus::STATUS_OPEN;
+
+        if ($request->getInt('priority') != $task->getPriority()) {
+          $changes[ManiphestTransactionType::TYPE_PRIORITY] =
+            $request->getInt('priority');
+        }
+
+        if ($owner_phid) {
+          $changes[ManiphestTransactionType::TYPE_OWNER] = $owner_phid;
+        }
+
+        if ($request->getArr('cc')) {
+          $changes[ManiphestTransactionType::TYPE_CCS] = $request->getArr('cc');
+        }
+
+        $template = new ManiphestTransaction();
+        $template->setAuthorPHID($user->getPHID());
+        $transactions = array();
+
+        foreach ($changes as $type => $value) {
+          $transaction = clone $template;
+          $transaction->setTransactionType($type);
+          $transaction->setNewValue($value);
+          $transactions[] = $transaction;
+        }
 
         $editor = new ManiphestTransactionEditor();
-        $editor->applyTransaction($task, $transaction);
+        $editor->applyTransactions($task, $transactions);
 
         return id(new AphrontRedirectResponse())
           ->setURI('/T'.$task->getID());
       }
+    } else {
+      $task->setCCPHIDs(array(
+        $user->getPHID(),
+      ));
     }
 
     $phids = array_merge(
       array($task->getOwnerPHID()),
-      nonempty($task->getCCPHIDs(), array()));
+      $task->getCCPHIDs());
     $phids = array_filter($phids);
     $phids = array_unique($phids);
 

src/applications/maniphest/controller/transactionsave/ManiphestTransactionSaveController.php

@@ -48,6 +48,8 @@ class ManiphestTransactionSaveController extends ManiphestController {
         break;
       case ManiphestTransactionType::TYPE_CCS:
         $ccs = $request->getArr('ccs');
+        $ccs = array_merge($ccs, $task->getCCPHIDs());
+        $ccs = array_unique($ccs);
         $transaction->setNewValue($ccs);
         break;
       case ManiphestTransactionType::TYPE_PRIORITY:
@@ -58,7 +60,7 @@ class ManiphestTransactionSaveController extends ManiphestController {
     }
 
     $editor = new ManiphestTransactionEditor();
-    $editor->applyTransaction($task, $transaction);
+    $editor->applyTransactions($task, array($transaction));
 
     return id(new AphrontRedirectResponse())
       ->setURI('/T'.$task->getID());

src/applications/maniphest/editor/transaction/ManiphestTransactionEditor.php

@@ -18,83 +18,86 @@
 
 class ManiphestTransactionEditor {
 
-  public function applyTransaction($task, $transaction) {
+  public function applyTransactions($task, array $transactions) {
-
-    $type = $transaction->getTransactionType();
-
-    $new = $transaction->getNewValue();
 
     $email_cc = $task->getCCPHIDs();
 
     $email_to = array();
     $email_to[] = $task->getOwnerPHID();
-    $email_to[] = $transaction->getAuthorPHID();
 
-    switch ($type) {
+    foreach ($transactions as $transaction) {
-      case ManiphestTransactionType::TYPE_NONE:
+      $type = $transaction->getTransactionType();
-        $old = null;
+      $new = $transaction->getNewValue();
-        break;
+      $email_to[] = $transaction->getAuthorPHID();
-      case ManiphestTransactionType::TYPE_STATUS:
-        $old = $task->getStatus();
-        break;
-      case ManiphestTransactionType::TYPE_OWNER:
-        $old = $task->getOwnerPHID();
-        break;
-      case ManiphestTransactionType::TYPE_CCS:
-        $old = $task->getCCPHIDs();
-        $new = array_unique(array_merge($old, $new));
-        break;
-      case ManiphestTransactionType::TYPE_PRIORITY:
-        $old = $task->getPriority();
-        break;
-      default:
-        throw new Exception('Unknown action type.');
-    }
 
-    if (($old !== null) && ($old == $new)) {
-      $transaction->setOldValue(null);
-      $transaction->setNewValue(null);
-      $transaction->setTransactionType(ManiphestTransactionType::TYPE_NONE);
-    } else {
       switch ($type) {
         case ManiphestTransactionType::TYPE_NONE:
+          $old = null;
           break;
         case ManiphestTransactionType::TYPE_STATUS:
-          $task->setStatus($new);
+          $old = $task->getStatus();
           break;
         case ManiphestTransactionType::TYPE_OWNER:
-          $task->setOwnerPHID($new);
+          $old = $task->getOwnerPHID();
           break;
         case ManiphestTransactionType::TYPE_CCS:
-          $task->setCCPHIDs($new);
+          $old = $task->getCCPHIDs();
           break;
         case ManiphestTransactionType::TYPE_PRIORITY:
-          $task->setPriority($new);
+          $old = $task->getPriority();
           break;
         default:
           throw new Exception('Unknown action type.');
       }
 
-      $transaction->setOldValue($old);
+      if (($old !== null) && ($old == $new)) {
-      $transaction->setNewValue($new);
+        $transaction->setOldValue(null);
+        $transaction->setNewValue(null);
+        $transaction->setTransactionType(ManiphestTransactionType::TYPE_NONE);
+      } else {
+        switch ($type) {
+          case ManiphestTransactionType::TYPE_NONE:
+            break;
+          case ManiphestTransactionType::TYPE_STATUS:
+            $task->setStatus($new);
+            break;
+          case ManiphestTransactionType::TYPE_OWNER:
+            $task->setOwnerPHID($new);
+            break;
+          case ManiphestTransactionType::TYPE_CCS:
+            $task->setCCPHIDs($new);
+            break;
+          case ManiphestTransactionType::TYPE_PRIORITY:
+            $task->setPriority($new);
+            break;
+          default:
+            throw new Exception('Unknown action type.');
+        }
+
+        $transaction->setOldValue($old);
+        $transaction->setNewValue($new);
+      }
+
     }
 
     $task->save();
-    $transaction->setTaskID($task->getID());
+    foreach ($transactions as $transaction) {
-    $transaction->save();
+      $transaction->setTaskID($task->getID());
+      $transaction->save();
+    }
 
     $email_to[] = $task->getOwnerPHID();
-    $email_cc = array_merge($email_cc, $task->getCCPHIDs());
+    $email_cc = array_merge(
+      $email_cc,
+      $task->getCCPHIDs());
 
-    $this->sendEmail($task, $transaction, $email_to, $email_cc);
+    $this->sendEmail($task, $transactions, $email_to, $email_cc);
   }
 
-  private function sendEmail($task, $transaction, $email_to, $email_cc) {
+  private function sendEmail($task, $transactions, $email_to, $email_cc) {
     $email_to = array_filter(array_unique($email_to));
     $email_cc = array_filter(array_unique($email_cc));
 
-    $transactions = array($transaction);
-
     $phids = array();
     foreach ($transactions as $transaction) {
       foreach ($transaction->extractPHIDs() as $phid) {
@@ -108,13 +111,29 @@ class ManiphestTransactionEditor {
 
 
     $view = new ManiphestTransactionDetailView();
-    $view->setTransaction($transaction);
+    $view->setTransactionGroup($transactions);
     $view->setHandles($handles);
-
     list($action, $body) = $view->renderForEmail($with_date = false);
 
+    $is_create = false;
+    foreach ($transactions as $transaction) {
+      $type = $transaction->getTransactionType();
+      if (($type == ManiphestTransactionType::TYPE_STATUS) &&
+          ($transaction->getOldValue() === null) &&
+          ($transaction->getNewValue() == ManiphestTaskStatus::STATUS_OPEN)) {
+        $is_create = true;
+      }
+    }
+
     $task_uri = PhabricatorEnv::getURI('/T'.$task->getID());
 
+    if ($is_create) {
+      $body .=
+        "\n\n".
+        "TASK DESCRIPTION\n".
+        "  ".$task->getDescription();
+    }
+
     $body .=
       "\n\n".
       "TASK DETAIL\n".

src/applications/maniphest/editor/transaction/__init__.php

@@ -6,6 +6,7 @@
 
 
 
+phutil_require_module('phabricator', 'applications/maniphest/constants/status');
 phutil_require_module('phabricator', 'applications/maniphest/constants/transactiontype');
 phutil_require_module('phabricator', 'applications/maniphest/view/transactiondetail');
 phutil_require_module('phabricator', 'applications/metamta/storage/mail');

src/applications/maniphest/storage/task/ManiphestTask.php

@@ -45,4 +45,8 @@ class ManiphestTask extends ManiphestDAO {
     return PhabricatorPHID::generateNewPHID('TASK');
   }
 
+  public function getCCPHIDs() {
+    return nonempty($this->ccPHIDs, array());
+  }
+
 }

src/applications/maniphest/storage/task/__init__.php

@@ -9,5 +9,7 @@
 phutil_require_module('phabricator', 'applications/maniphest/storage/base');
 phutil_require_module('phabricator', 'applications/phid/storage/phid');
 
+phutil_require_module('phutil', 'utils');
+
 
 phutil_require_source('ManiphestTask.php');

src/applications/maniphest/storage/transaction/ManiphestTransaction.php

@@ -58,4 +58,29 @@ class ManiphestTransaction extends ManiphestDAO {
     return $phids;
   }
 
+  public function canGroupWith($target) {
+    if ($target->getAuthorPHID() != $this->getAuthorPHID()) {
+      return false;
+    }
+    if ($target->hasComments() && $this->hasComments()) {
+      return false;
+    }
+    $ttime = $target->getDateCreated();
+    $stime = $target->getDateCreated();
+    if (abs($stime - $ttime) > 60) {
+      return false;
+    }
+
+    if ($target->getTransactionType() == $this->getTransactionType()) {
+      return false;
+    }
+
+    return true;
+  }
+
+  public function hasComments() {
+    return (bool)strlen(trim($this->getComments()));
+  }
+
+
 }

src/applications/maniphest/view/transactiondetail/ManiphestTransactionDetailView.php

@@ -18,13 +18,13 @@
 
 class ManiphestTransactionDetailView extends AphrontView {
 
-  private $transaction;
+  private $transactions;
   private $handles;
   private $markupEngine;
   private $forEmail;
 
-  public function setTransaction(ManiphestTransaction $transaction) {
+  public function setTransactionGroup(array $transactions) {
-    $this->transaction = $transaction;
+    $this->transactions = $transactions;
     return $this;
   }
 
@@ -40,34 +40,50 @@ class ManiphestTransactionDetailView extends AphrontView {
 
   public function renderForEmail($with_date) {
     $this->forEmail = true;
-    list ($verb, $desc, $classes) = $this->describeAction();
 
-    $transaction = $this->transaction;
+    $transaction = reset($this->transactions);
     $author = $this->renderHandles(array($transaction->getAuthorPHID()));
 
-    $desc = $author.' '.$desc;
+    $action = null;
-    if ($with_date) {
+    $descs = array();
-      $desc = 'On '.date('M jS \a\t g:i A', $transaction->getDateCreated()).
+    $comments = null;
-              ', '.$desc;
+    foreach ($this->transactions as $transaction) {
+      list($verb, $desc, $classes) = $this->describeAction($transaction);
+      if ($action === null) {
+        $action = $verb;
+      }
+      $desc = $author.' '.$desc.'.';
+      if ($with_date) {
+        $desc = 'On '.date('M jS \a\t g:i A', $transaction->getDateCreated()).
+                ', '.$desc;
+      }
+      $descs[] = $desc;
+      if ($transaction->hasComments()) {
+        $comments = $transaction->getComments();
+      }
     }
 
-    $comments = $transaction->getComments();
+    $descs = implode("\n", $descs);
-    if (strlen(trim($comments))) {
+    if ($comments) {
-      $desc = $desc.":\n".$comments;
+      $descs .= "\n".$comments;
-    } else {
-      $desc = $desc.".";
     }
 
     $this->forEmail = false;
-    return array($verb, $desc);
+    return array($action, $descs);
   }
 
   public function render() {
-    $transaction = $this->transaction;
     $handles = $this->handles;
+    $transactions = $this->transactions;
 
     require_celerity_resource('maniphest-transaction-detail-css');
 
+    foreach ($this->transactions as $transaction) {
+      if ($transaction->hasComments()) {
+        break;
+      }
+    }
+
     $author = $this->handles[$transaction->getAuthorPHID()];
 
     $comments = $transaction->getCache();
@@ -80,18 +96,18 @@ class ManiphestTransactionDetailView extends AphrontView {
       }
     }
 
-    list($verb, $desc, $classes) = $this->describeAction(
+    $more_classes = array();
-      $transaction->getComments());
+    $descs = array();
+    foreach ($transactions as $transaction) {
+      list($verb, $desc, $classes) = $this->describeAction($transaction);
+      $more_classes = array_merge($more_classes, $classes);
+      $descs[] = $author->renderLink().' '.$desc.'.';
+    }
+    $descs = implode('<br />', $descs);
 
     $more_classes = implode(' ', $classes);
 
-    if (strlen(trim($transaction->getComments()))) {
+    if ($transaction->hasComments()) {
-      $punc = ':';
-    } else {
-      $punc = '.';
-    }
-
-    if (strlen(trim($transaction->getComments()))) {
       $comment_block =
         '<div class="maniphest-transaction-comments phabricator-remarkup">'.
           $comments.
@@ -112,24 +128,20 @@ class ManiphestTransactionDetailView extends AphrontView {
             phabricator_format_timestamp($transaction->getDateCreated()).
           '</div>'.
           '<strong>'.
-            $author->renderLink().
+            $descs.
-            ' '.
-            $desc.
-            $punc.
           '</strong>'.
         '</div>'.
         $comment_block.
       '</div>');
   }
 
-  private function describeAction() {
+  private function describeAction($transaction) {
     $verb = null;
     $desc = null;
     $classes = array();
 
     $handles = $this->handles;
 
-    $transaction = $this->transaction;
     $type = $transaction->getTransactionType();
     $author_phid = $transaction->getAuthorPHID();
     $new = $transaction->getNewValue();

src/applications/maniphest/view/transactionlist/ManiphestTransactionListView.php

@@ -46,9 +46,34 @@ class ManiphestTransactionListView extends AphrontView {
   public function render() {
 
     $views = array();
+
+
+    $last = null;
+    $group = array();
+    $groups = array();
     foreach ($this->transactions as $transaction) {
+      if ($last === null) {
+        $last = $transaction;
+        $group[] = $transaction;
+        continue;
+      } else if ($last->canGroupWith($transaction)) {
+        $group[] = $transaction;
+        if ($transaction->hasComments()) {
+          $last = $transaction;
+        }
+      } else {
+        $groups[] = $group;
+        $last = $transaction;
+        $group = array($transaction);
+      }
+    }
+    if ($group) {
+      $groups[] = $group;
+    }
+
+    foreach ($groups as $group) {
       $view = new ManiphestTransactionDetailView($transaction);
-      $view->setTransaction($transaction);
+      $view->setTransactionGroup($group);
       $view->setHandles($this->handles);
       $view->setMarkupEngine($this->markupEngine);
       $views[] = $view->render();