Gut ManiphestTransactionEditor

Summary:
Ref T2217. Removes most of the code from ManiphestTransactionEditor.

  - Provides mail tag support in ManiphestTransactionEditorPro.
  - There was one more write (subscribe/unsubscribe button) that I'd missed; modernize that.

Test Plan:
  - Clicked subscribe/unsubscribe.
  - Made some edits, verified mail had appropriate mail tags.

Reviewers: btrahan, garoevans

Reviewed By: garoevans

CC: aran

Maniphest Tasks: T2217

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

src/applications/maniphest/controller/ManiphestSubscribeController.php

@@ -20,21 +20,29 @@ final class ManiphestSubscribeController extends ManiphestController {
       return new Aphront404Response();
     }
 
+    $ccs = $task->getCCPHIDs();
     switch ($this->action) {
       case 'add':
-        ManiphestTransactionEditor::addCC(
+        $ccs[] = $user->getPHID();
-          $task,
-          $user);
         break;
       case 'rem':
-        ManiphestTransactionEditor::removeCC(
+        $ccs = array_diff($ccs, array($user->getPHID()));
-          $task,
-          $user);
         break;
       default:
         return new Aphront400Response();
     }
 
+    $xaction = id(new ManiphestTransactionPro())
+      ->setTransactionType(ManiphestTransactionPro::TYPE_CCS)
+      ->setNewValue($ccs);
+
+    $editor = id(new ManiphestTransactionEditorPro())
+      ->setActor($user)
+      ->setContentSourceFromRequest($request)
+      ->setContinueOnNoEffect(true)
+      ->setContinueOnMissingFields(true)
+      ->applyTransactions($task, array($xaction));
+
     return id(new AphrontRedirectResponse())->setURI('/T'.$task->getID());
   }
 }

src/applications/maniphest/editor/ManiphestTransactionEditor.php

@@ -5,319 +5,6 @@
  */
 final class ManiphestTransactionEditor extends PhabricatorEditor {
 
-  private $parentMessageID;
-  private $auxiliaryFields = array();
-
-  public function setAuxiliaryFields(array $fields) {
-    assert_instances_of($fields, 'ManiphestCustomField');
-    $this->auxiliaryFields = $fields;
-    return $this;
-  }
-
-  public function setParentMessageID($parent_message_id) {
-    $this->parentMessageID = $parent_message_id;
-    return $this;
-  }
-
-  public function applyTransactions(ManiphestTask $task, array $transactions) {
-    assert_instances_of($transactions, 'ManiphestTransaction');
-
-    $email_cc = $task->getCCPHIDs();
-
-    $email_to = array();
-    $email_to[] = $task->getOwnerPHID();
-
-    $pri_changed = $this->isCreate($transactions);
-    $aux_writes = array();
-
-    foreach ($transactions as $key => $transaction) {
-      $type = $transaction->getTransactionType();
-      $new = $transaction->getNewValue();
-      $email_to[] = $transaction->getAuthorPHID();
-
-      $value_is_phid_set = false;
-
-      switch ($type) {
-        case PhabricatorTransactions::TYPE_COMMENT:
-          $old = null;
-          break;
-        case ManiphestTransactionType::TYPE_STATUS:
-          $old = $task->getStatus();
-          break;
-        case ManiphestTransactionType::TYPE_OWNER:
-          $old = $task->getOwnerPHID();
-          break;
-        case ManiphestTransactionType::TYPE_CCS:
-          $old = $task->getCCPHIDs();
-          $value_is_phid_set = true;
-          break;
-        case ManiphestTransactionType::TYPE_PRIORITY:
-          $old = $task->getPriority();
-          break;
-        case ManiphestTransactionType::TYPE_EDGE:
-          $old = $transaction->getOldValue();
-          break;
-        case ManiphestTransactionType::TYPE_ATTACH:
-          $old = $task->getAttached();
-          break;
-        case ManiphestTransactionType::TYPE_TITLE:
-          $old = $task->getTitle();
-          break;
-        case ManiphestTransactionType::TYPE_DESCRIPTION:
-          $old = $task->getDescription();
-          break;
-        case ManiphestTransactionType::TYPE_PROJECTS:
-          $old = $task->getProjectPHIDs();
-          $value_is_phid_set = true;
-          break;
-        case PhabricatorTransactions::TYPE_CUSTOMFIELD:
-          $aux_key = $transaction->getMetadataValue('customfield:key');
-          if (!$aux_key) {
-            throw new Exception(
-              "Expected 'customfield:key' metadata on TYPE_CUSTOMFIELD ".
-              "transaction.");
-          }
-          // This has already been populated.
-          $old = $transaction->getOldValue();
-          break;
-        default:
-          throw new Exception('Unknown action type.');
-      }
-
-      $old_cmp = $old;
-      $new_cmp = $new;
-      if ($value_is_phid_set) {
-
-        // Normalize the old and new values if they are PHID sets so we don't
-        // get any no-op transactions where the values differ only by keys,
-        // order, duplicates, etc.
-
-        if (is_array($old)) {
-          $old = array_filter($old);
-          $old = array_unique($old);
-          sort($old);
-          $old = array_values($old);
-          $old_cmp = $old;
-        }
-
-        if (is_array($new)) {
-          $new = array_filter($new);
-          $new = array_unique($new);
-          $transaction->setNewValue($new);
-
-          $new_cmp = $new;
-          sort($new_cmp);
-          $new_cmp = array_values($new_cmp);
-        }
-      }
-
-      if (($old !== null) && ($old_cmp == $new_cmp)) {
-        if (count($transactions) > 1 && !$transaction->hasComments()) {
-          // If we have at least one other transaction and this one isn't
-          // doing anything and doesn't have any comments, just throw it
-          // away.
-          unset($transactions[$key]);
-          continue;
-        } else {
-          $transaction->setOldValue(null);
-          $transaction->setNewValue(null);
-          $transaction->setTransactionType(
-            PhabricatorTransactions::TYPE_COMMENT);
-        }
-      } else {
-        switch ($type) {
-          case PhabricatorTransactions::TYPE_COMMENT:
-            break;
-          case ManiphestTransactionType::TYPE_STATUS:
-            $task->setStatus($new);
-            break;
-          case ManiphestTransactionType::TYPE_OWNER:
-            if ($new) {
-              $handle = id(new PhabricatorHandleQuery())
-                ->setViewer($this->getActor())
-                ->withPHIDs(array($new))
-                ->executeOne();
-              $task->setOwnerOrdering($handle->getName());
-            } else {
-              $task->setOwnerOrdering(null);
-            }
-            $task->setOwnerPHID($new);
-            break;
-          case ManiphestTransactionType::TYPE_CCS:
-            $task->setCCPHIDs($new);
-            break;
-          case ManiphestTransactionType::TYPE_PRIORITY:
-            $task->setPriority($new);
-            $pri_changed = true;
-            break;
-          case ManiphestTransactionType::TYPE_ATTACH:
-            $task->setAttached($new);
-            break;
-          case ManiphestTransactionType::TYPE_TITLE:
-            $task->setTitle($new);
-            break;
-          case ManiphestTransactionType::TYPE_DESCRIPTION:
-            $task->setDescription($new);
-            break;
-          case ManiphestTransactionType::TYPE_PROJECTS:
-            $task->setProjectPHIDs($new);
-            break;
-          case PhabricatorTransactions::TYPE_CUSTOMFIELD:
-            $aux_key = $transaction->getMetadataValue('customfield:key');
-            $aux_writes[$aux_key] = $new;
-            break;
-          case ManiphestTransactionType::TYPE_EDGE:
-            // Edge edits are accomplished through PhabricatorEdgeEditor, which
-            // has authority.
-            break;
-          default:
-            throw new Exception('Unknown action type.');
-        }
-
-        $transaction->setOldValue($old);
-        $transaction->setNewValue($new);
-      }
-
-    }
-
-    if ($pri_changed) {
-      $subpriority = ManiphestTransactionEditor::getNextSubpriority(
-        $task->getPriority(),
-        null);
-      $task->setSubpriority($subpriority);
-    }
-
-    $task->save();
-
-    if ($aux_writes) {
-      ManiphestAuxiliaryFieldSpecification::writeLegacyAuxiliaryUpdates(
-        $task,
-        $aux_writes);
-    }
-
-    foreach ($transactions as $transaction) {
-      $transaction->setTransactionTask($task);
-      $transaction->save();
-    }
-
-    $email_to[] = $task->getOwnerPHID();
-    $email_cc = array_merge(
-      $email_cc,
-      $task->getCCPHIDs());
-
-    $mail = $this->sendEmail($task, $transactions, $email_to, $email_cc);
-
-    $this->publishFeedStory(
-      $task,
-      $transactions,
-      $mail->buildRecipientList());
-
-    id(new PhabricatorSearchIndexer())
-      ->indexDocumentByPHID($task->getPHID());
-
-    $fields = PhabricatorCustomField::getObjectFields(
-      $task,
-      PhabricatorCustomField::ROLE_APPLICATIONSEARCH);
-    $fields->readFieldsFromStorage($task);
-    $fields->rebuildIndexes($task);
-
-  }
-
-  protected function getSubjectPrefix() {
-    return PhabricatorEnv::getEnvConfig('metamta.maniphest.subject-prefix');
-  }
-
-  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));
-
-    $phids = array();
-    foreach ($transactions as $transaction) {
-      foreach ($transaction->extractPHIDs() as $phid) {
-        $phids[$phid] = true;
-      }
-    }
-    foreach ($email_to as $phid) {
-      $phids[$phid] = true;
-    }
-    foreach ($email_cc as $phid) {
-      $phids[$phid] = true;
-    }
-    $phids = array_keys($phids);
-
-    $handles = id(new PhabricatorHandleQuery())
-      ->setViewer($this->getActor())
-      ->withPHIDs($phids)
-      ->execute();
-
-    $main_body = array();
-    foreach ($transactions as $transaction) {
-      $main_body[] = id(clone $transaction->getModernTransaction())
-        ->attachViewer($this->getActor())
-        ->setHandles($handles)
-        ->setRenderingTarget('text')
-        ->getTitle();
-    }
-
-    foreach ($transactions as $transaction) {
-      if ($transaction->getComments()) {
-        $main_body[] = null;
-        $main_body[] = $transaction->getComments();
-      }
-    }
-
-    $main_body = implode("\n", $main_body);
-
-    $action = head($transactions)->getModernTransaction()->getActionName();
-
-    $is_create = $this->isCreate($transactions);
-
-    $task_uri = PhabricatorEnv::getProductionURI('/T'.$task->getID());
-
-    $reply_handler = $this->buildReplyHandler($task);
-
-    $body = new PhabricatorMetaMTAMailBody();
-    $body->addRawSection($main_body);
-    if ($is_create) {
-      $body->addTextSection(pht('TASK DESCRIPTION'), $task->getDescription());
-    }
-    $body->addTextSection(pht('TASK DETAIL'), $task_uri);
-    $body->addReplySection($reply_handler->getReplyHandlerInstructions());
-
-    $thread_id = 'maniphest-task-'.$task->getPHID();
-    $task_id = $task->getID();
-    $title = $task->getTitle();
-
-    $mailtags = $this->getMailTags($transactions);
-
-    $template = id(new PhabricatorMetaMTAMail())
-      ->setSubject("T{$task_id}: {$title}")
-      ->setSubjectPrefix($this->getSubjectPrefix())
-      ->setVarySubjectPrefix("[{$action}]")
-      ->setFrom($transaction->getAuthorPHID())
-      ->setParentMessageID($this->parentMessageID)
-      ->addHeader('Thread-Topic', "T{$task_id}: ".$task->getOriginalTitle())
-      ->setThreadID($thread_id, $is_create)
-      ->setRelatedPHID($task->getPHID())
-      ->setExcludeMailRecipientPHIDs($this->getExcludeMailRecipientPHIDs())
-      ->setIsBulk(true)
-      ->setMailTags($mailtags)
-      ->setBody($body->render());
-
-    $mails = $reply_handler->multiplexMail(
-      $template,
-      array_select_keys($handles, $email_to),
-      array_select_keys($handles, $email_cc));
-
-    foreach ($mails as $mail) {
-      $mail->saveAndSend();
-    }
-
-    $template->addTos($email_to);
-    $template->addCCs($email_cc);
-    return $template;
-  }
-
   public function buildReplyHandler(ManiphestTask $task) {
     $handler_object = PhabricatorEnv::newObjectFromConfig(
       'metamta.maniphest.reply-handler');
@@ -326,130 +13,6 @@ final class ManiphestTransactionEditor extends PhabricatorEditor {
     return $handler_object;
   }
 
-  private function publishFeedStory(
-    ManiphestTask $task,
-    array $transactions,
-    array $mailed_phids) {
-    assert_instances_of($transactions, 'ManiphestTransaction');
-
-    $actions = array(ManiphestAction::ACTION_UPDATE);
-    $comments = null;
-    foreach ($transactions as $transaction) {
-      if ($transaction->hasComments()) {
-        $comments = $transaction->getComments();
-      }
-      $type = $transaction->getTransactionType();
-      switch ($type) {
-        case ManiphestTransactionType::TYPE_OWNER:
-          $actions[] = ManiphestAction::ACTION_ASSIGN;
-          break;
-        case ManiphestTransactionType::TYPE_STATUS:
-          if ($task->getStatus() != ManiphestTaskStatus::STATUS_OPEN) {
-            $actions[] = ManiphestAction::ACTION_CLOSE;
-          } else if ($this->isCreate($transactions)) {
-            $actions[] = ManiphestAction::ACTION_CREATE;
-          } else {
-            $actions[] = ManiphestAction::ACTION_REOPEN;
-          }
-          break;
-        default:
-          $actions[] = $type;
-          break;
-      }
-    }
-
-    $action_type = ManiphestAction::selectStrongestAction($actions);
-    $owner_phid = $task->getOwnerPHID();
-    $actor_phid = head($transactions)->getAuthorPHID();
-    $author_phid = $task->getAuthorPHID();
-
-    id(new PhabricatorFeedStoryPublisher())
-      ->setStoryType('PhabricatorFeedStoryManiphest')
-      ->setStoryData(array(
-        'taskPHID'        => $task->getPHID(),
-        'transactionIDs'  => mpull($transactions, 'getID'),
-        'ownerPHID'       => $owner_phid,
-        'action'          => $action_type,
-        'comments'        => $comments,
-        'description'     => $task->getDescription(),
-      ))
-      ->setStoryTime(time())
-      ->setStoryAuthorPHID($actor_phid)
-      ->setRelatedPHIDs(
-        array_merge(
-          array_filter(
-            array(
-              $task->getPHID(),
-              $author_phid,
-              $actor_phid,
-              $owner_phid,
-            )),
-          $task->getProjectPHIDs()))
-      ->setPrimaryObjectPHID($task->getPHID())
-      ->setSubscribedPHIDs(
-        array_merge(
-          array_filter(
-            array(
-              $author_phid,
-              $owner_phid,
-              $actor_phid)),
-          $task->getCCPHIDs()))
-      ->setMailRecipientPHIDs($mailed_phids)
-      ->publish();
-  }
-
-  private function isCreate(array $transactions) {
-    assert_instances_of($transactions, 'ManiphestTransaction');
-    $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;
-      }
-    }
-    return $is_create;
-  }
-
-  private function getMailTags(array $transactions) {
-    assert_instances_of($transactions, 'ManiphestTransaction');
-
-    $tags = array();
-    foreach ($transactions as $xaction) {
-      switch ($xaction->getTransactionType()) {
-        case ManiphestTransactionType::TYPE_STATUS:
-          $tags[] = MetaMTANotificationType::TYPE_MANIPHEST_STATUS;
-          break;
-        case ManiphestTransactionType::TYPE_OWNER:
-          $tags[] = MetaMTANotificationType::TYPE_MANIPHEST_OWNER;
-          break;
-        case ManiphestTransactionType::TYPE_CCS:
-          $tags[] = MetaMTANotificationType::TYPE_MANIPHEST_CC;
-          break;
-        case ManiphestTransactionType::TYPE_PROJECTS:
-          $tags[] = MetaMTANotificationType::TYPE_MANIPHEST_PROJECTS;
-          break;
-        case ManiphestTransactionType::TYPE_PRIORITY:
-          $tags[] = MetaMTANotificationType::TYPE_MANIPHEST_PRIORITY;
-          break;
-        case PhabricatorTransactions::TYPE_COMMENT:
-          // this is a comment which we will check separately below for
-          // content
-          break;
-        default:
-          $tags[] = MetaMTANotificationType::TYPE_MANIPHEST_OTHER;
-          break;
-      }
-
-      if ($xaction->hasComments()) {
-        $tags[] = MetaMTANotificationType::TYPE_MANIPHEST_COMMENT;
-      }
-    }
-
-    return array_unique($tags);
-  }
-
   public static function getNextSubpriority($pri, $sub) {
 
     if ($sub === null) {
@@ -472,39 +35,4 @@ final class ManiphestTransactionEditor extends PhabricatorEditor {
     return (double)(2 << 32);
   }
 
-  public static function addCC(
-    ManiphestTask $task,
-    PhabricatorUser $user) {
-    $current_ccs = $task->getCCPHIDs();
-    $new_ccs = array_merge($current_ccs, array($user->getPHID()));
-
-    $transaction = new ManiphestTransaction();
-    $transaction->setTransactionTask($task);
-    $transaction->setAuthorPHID($user->getPHID());
-    $transaction->setTransactionType(ManiphestTransactionType::TYPE_CCS);
-    $transaction->setNewValue(array_unique($new_ccs));
-    $transaction->setOldValue($current_ccs);
-
-    id(new ManiphestTransactionEditor())
-      ->setActor($user)
-      ->applyTransactions($task, array($transaction));
-  }
-
-  public static function removeCC(
-    ManiphestTask $task,
-    PhabricatorUser $user) {
-    $current_ccs = $task->getCCPHIDs();
-    $new_ccs = array_diff($current_ccs, array($user->getPHID()));
-
-    $transaction = new ManiphestTransaction();
-    $transaction->setTransactionTask($task);
-    $transaction->setAuthorPHID($user->getPHID());
-    $transaction->setTransactionType(ManiphestTransactionType::TYPE_CCS);
-    $transaction->setNewValue(array_unique($new_ccs));
-    $transaction->setOldValue($current_ccs);
-
-    id(new ManiphestTransactionEditor())
-      ->setActor($user)
-      ->applyTransactions($task, array($transaction));
-  }
 }

src/applications/maniphest/storage/ManiphestTransactionPro.php

@@ -414,6 +414,34 @@ final class ManiphestTransactionPro
     return $view->render();
   }
 
+  public function getMailTags() {
+    $tags = array();
+    switch ($this->getTransactionType()) {
+      case self::TYPE_STATUS:
+        $tags[] = MetaMTANotificationType::TYPE_MANIPHEST_STATUS;
+        break;
+      case self::TYPE_OWNER:
+        $tags[] = MetaMTANotificationType::TYPE_MANIPHEST_OWNER;
+        break;
+      case self::TYPE_CCS:
+        $tags[] = MetaMTANotificationType::TYPE_MANIPHEST_CC;
+        break;
+      case self::TYPE_PROJECTS:
+        $tags[] = MetaMTANotificationType::TYPE_MANIPHEST_PROJECTS;
+        break;
+      case self::TYPE_PRIORITY:
+        $tags[] = MetaMTANotificationType::TYPE_MANIPHEST_PRIORITY;
+        break;
+      case self::TYPE_COMMENT:
+        $tags[] = MetaMTANotificationType::TYPE_MANIPHEST_COMMENT;
+        break;
+      default:
+        $tags[] = MetaMTANotificationType::TYPE_MANIPHEST_OTHER;
+        break;
+    }
+    return $tags;
+  }
+
 
 }