Apply normal Audit actions directly with Transaction editor

Summary: Ref T4896. This converts the last "CommentEditor" to a transaction editor and removes a large part of the old code.

Test Plan:
  - Added comments.
  - Accepted / added auditors.
  - Added inline comments.

Reviewers: joshuaspence, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4896

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

src/__phutil_library_map__.php

@@ -1136,7 +1136,6 @@ phutil_register_library_map(array(
     'PhabricatorAuditActionConstants' => 'applications/audit/constants/PhabricatorAuditActionConstants.php',
     'PhabricatorAuditAddCommentController' => 'applications/audit/controller/PhabricatorAuditAddCommentController.php',
     'PhabricatorAuditApplication' => 'applications/audit/application/PhabricatorAuditApplication.php',
-    'PhabricatorAuditComment' => 'applications/audit/storage/PhabricatorAuditComment.php',
     'PhabricatorAuditCommentEditor' => 'applications/audit/editor/PhabricatorAuditCommentEditor.php',
     'PhabricatorAuditCommitStatusConstants' => 'applications/audit/constants/PhabricatorAuditCommitStatusConstants.php',
     'PhabricatorAuditController' => 'applications/audit/controller/PhabricatorAuditController.php',
@@ -3931,7 +3930,6 @@ phutil_register_library_map(array(
     'PhabricatorAsanaConfigOptions' => 'PhabricatorApplicationConfigOptions',
     'PhabricatorAuditAddCommentController' => 'PhabricatorAuditController',
     'PhabricatorAuditApplication' => 'PhabricatorApplication',
-    'PhabricatorAuditComment' => 'PhabricatorMarkupInterface',
     'PhabricatorAuditCommentEditor' => 'PhabricatorEditor',
     'PhabricatorAuditController' => 'PhabricatorController',
     'PhabricatorAuditDAO' => 'PhabricatorLiskDAO',

src/applications/audit/controller/PhabricatorAuditAddCommentController.php

@@ -21,22 +21,20 @@ final class PhabricatorAuditAddCommentController
       return new Aphront404Response();
     }
 
-    $phids = array($commit_phid);
+    $xactions = array();
-
-    $comments = array();
 
     // make sure we only add auditors or ccs if the action matches
     $action = $request->getStr('action');
     switch ($action) {
       case PhabricatorAuditActionConstants::ADD_AUDITORS:
         $auditors = $request->getArr('auditors');
-        $comments[] = id(new PhabricatorAuditComment())
+        $xactions[] = id(new PhabricatorAuditTransaction())
-          ->setAction(PhabricatorAuditActionConstants::ADD_AUDITORS)
+          ->setTransactionType(PhabricatorAuditActionConstants::ADD_AUDITORS)
           ->setNewValue(array_fuse($auditors));
         break;
       case PhabricatorAuditActionConstants::ADD_CCS:
-        $comments[] = id(new PhabricatorAuditComment())
+        $xactions[] = id(new PhabricatorAuditTransaction())
-          ->setAction(PhabricatorTransactions::TYPE_SUBSCRIBERS)
+          ->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS)
           ->setNewValue(
             array(
               '+' => $request->getArr('ccs'),
@@ -46,25 +44,36 @@ final class PhabricatorAuditAddCommentController
         // We'll deal with this below.
         break;
       default:
-        $comments[] = id(new PhabricatorAuditComment())
+        $xactions[] = id(new PhabricatorAuditTransaction())
-          ->setAction($action);
+          ->setTransactionType(PhabricatorAuditActionConstants::ACTION)
+          ->setNewValue($action);
         break;
     }
 
     $content = $request->getStr('content');
     if (strlen($content)) {
-      $comments[] = id(new PhabricatorAuditComment())
+      $xactions[] = id(new PhabricatorAuditTransaction())
-        ->setAction(PhabricatorAuditActionConstants::COMMENT)
+        ->setTransactionType(PhabricatorTransactions::TYPE_COMMENT)
-        ->setContent($content);
+        ->attachComment(
+          id(new PhabricatorAuditTransactionComment())
+            ->setCommitPHID($commit->getPHID())
+            ->setContent($content));
     }
 
-    id(new PhabricatorAuditCommentEditor($commit))
+    $inlines = PhabricatorAuditInlineComment::loadDraftComments(
-      ->setActor($user)
+      $user,
-      ->setAttachInlineComments(true)
+      $commit->getPHID());
-      ->addComments($comments);
+    foreach ($inlines as $inline) {
+      $xactions[] = id(new PhabricatorAuditTransaction())
+        ->setTransactionType(PhabricatorAuditActionConstants::INLINE)
+        ->attachComment($inline->getTransactionComment());
+    }
 
-    $handles = $this->loadViewerHandles($phids);
+    id(new PhabricatorAuditEditor())
-    $uri = $handles[$commit_phid]->getURI();
+      ->setActor($user)
+      ->setContentSourceFromRequest($request)
+      ->setContinueOnMissingFields(true)
+      ->applyTransactions($commit, $xactions);
 
     $draft = id(new PhabricatorDraft())->loadOneWhere(
       'authorPHID = %s AND draftKey = %s',
@@ -74,6 +83,10 @@ final class PhabricatorAuditAddCommentController
       $draft->delete();
     }
 
+    $monogram = $commit->getRepository()->getMonogram();
+    $identifier = $commit->getCommitIdentifier();
+    $uri = '/'.$monogram.$identifier;
+
     return id(new AphrontRedirectResponse())->setURI($uri);
   }
 

src/applications/audit/editor/PhabricatorAuditCommentEditor.php

@@ -2,64 +2,6 @@
 
 final class PhabricatorAuditCommentEditor extends PhabricatorEditor {
 
-  private $commit;
-  private $attachInlineComments;
-
-  public function __construct(PhabricatorRepositoryCommit $commit) {
-    $this->commit = $commit;
-    return $this;
-  }
-
-  public function setAttachInlineComments($attach_inline_comments) {
-    $this->attachInlineComments = $attach_inline_comments;
-    return $this;
-  }
-
-  public function addComments(array $comments) {
-    assert_instances_of($comments, 'PhabricatorAuditComment');
-
-    $commit = $this->commit;
-    $actor = $this->getActor();
-
-    $other_comments = PhabricatorAuditComment::loadComments(
-      $actor,
-      $commit->getPHID());
-
-    $inline_comments = array();
-    if ($this->attachInlineComments) {
-      $inline_comments = PhabricatorAuditInlineComment::loadDraftComments(
-        $actor,
-        $commit->getPHID());
-    }
-
-    // Convert old comments into real transactions and apply them with a
-    // normal editor.
-
-    $xactions = array();
-    foreach ($comments as $comment) {
-      $xactions[] = $comment->getTransactionForSave();
-    }
-
-    foreach ($inline_comments as $inline) {
-      $xactions[] = id(new PhabricatorAuditTransaction())
-        ->setTransactionType(PhabricatorAuditActionConstants::INLINE)
-        ->attachComment($inline->getTransactionComment());
-    }
-
-    $content_source = PhabricatorContentSource::newForSource(
-      PhabricatorContentSource::SOURCE_LEGACY,
-      array());
-
-    $editor = id(new PhabricatorAuditEditor())
-      ->setActor($actor)
-      ->setContinueOnNoEffect(true)
-      ->setContinueOnMissingFields(true)
-      ->setContentSource($content_source)
-      ->setExcludeMailRecipientPHIDs($this->getExcludeMailRecipientPHIDs())
-      ->applyTransactions($commit, $xactions);
-  }
-
-
   /**
    * Load the PHIDs for all objects the user has the authority to act as an
    * audit for. This includes themselves, and any packages they are an owner

src/applications/audit/mail/PhabricatorAuditReplyHandler.php

@@ -52,7 +52,7 @@ final class PhabricatorAuditReplyHandler extends PhabricatorMailReplyHandler {
           ->setCommitPHID($commit->getPHID())
           ->setContent($message));
 
-    $editor = id(new PhabricatorAuditEditor($commit))
+    $editor = id(new PhabricatorAuditEditor())
       ->setActor($actor)
       ->setContentSource($content_source)
       ->setExcludeMailRecipientPHIDs($this->getExcludeMailRecipientPHIDs())

src/applications/audit/storage/PhabricatorAuditComment.php

@@ -1,197 +0,0 @@
-<?php
-
-final class PhabricatorAuditComment
-  implements PhabricatorMarkupInterface {
-
-  const MARKUP_FIELD_BODY        = 'markup:body';
-
-  private $proxyComment;
-  private $proxy;
-
-  public function __construct() {
-    $this->proxy = new PhabricatorAuditTransaction();
-  }
-
-  public function __clone() {
-    $this->proxy = clone $this->proxy;
-    if ($this->proxyComment) {
-      $this->proxyComment = clone $this->proxyComment;
-    }
-  }
-
-  public static function newFromModernTransaction(
-    PhabricatorAuditTransaction $xaction) {
-
-    $obj = new PhabricatorAuditComment();
-    $obj->proxy = $xaction;
-
-    if ($xaction->hasComment()) {
-      $obj->proxyComment = $xaction->getComment();
-    }
-
-    return $obj;
-  }
-
-  public static function loadComments(
-    PhabricatorUser $viewer,
-    $commit_phid) {
-
-    $xactions = id(new PhabricatorAuditTransactionQuery())
-      ->setViewer($viewer)
-      ->withObjectPHIDs(array($commit_phid))
-      ->needComments(true)
-      ->execute();
-
-    $comments = array();
-    foreach ($xactions as $xaction) {
-      $comments[] = self::newFromModernTransaction($xaction);
-    }
-
-    return $comments;
-  }
-
-  public function getPHID() {
-    return $this->proxy->getPHID();
-  }
-
-  public function getActorPHID() {
-    return $this->proxy->getAuthorPHID();
-  }
-
-  public function setActorPHID($actor_phid) {
-    $this->proxy->setAuthorPHID($actor_phid);
-    return $this;
-  }
-
-  public function setTargetPHID($target_phid) {
-    $this->getProxyComment()->setCommitPHID($target_phid);
-    $this->proxy->setObjectPHID($target_phid);
-    return $this;
-  }
-
-  public function getTargetPHID() {
-    return $this->proxy->getObjectPHID();
-  }
-
-  public function getContent() {
-    return $this->getProxyComment()->getContent();
-  }
-
-  public function setContent($content) {
-    $this->getProxyComment()->setContent($content);
-    return $this;
-  }
-
-  public function setContentSource($content_source) {
-    $this->proxy->setContentSource($content_source);
-    $this->proxyComment->setContentSource($content_source);
-    return $this;
-  }
-
-  public function getContentSource() {
-    return $this->proxy->getContentSource();
-  }
-
-  private function getProxyComment() {
-    if (!$this->proxyComment) {
-      $this->proxyComment = new PhabricatorAuditTransactionComment();
-    }
-    return $this->proxyComment;
-  }
-
-  public function setProxyComment(PhabricatorAuditTransactionComment $proxy) {
-    if ($this->proxyComment) {
-      throw new Exception(pht('You can not overwrite a proxy comment.'));
-    }
-    $this->proxyComment = $proxy;
-    return $this;
-  }
-
-  public function setAction($action) {
-    switch ($action) {
-      case PhabricatorAuditActionConstants::INLINE:
-      case PhabricatorAuditActionConstants::ADD_CCS:
-      case PhabricatorTransactions::TYPE_SUBSCRIBERS:
-      case PhabricatorAuditActionConstants::ADD_AUDITORS:
-        $this->proxy->setTransactionType($action);
-        break;
-      case PhabricatorAuditActionConstants::COMMENT:
-        $this->proxy->setTransactionType(PhabricatorTransactions::TYPE_COMMENT);
-        break;
-      default:
-        $this->proxy
-          ->setTransactionType(PhabricatorAuditActionConstants::ACTION)
-          ->setNewValue($action);
-        break;
-    }
-
-    return $this;
-  }
-
-  public function getAction() {
-    $type = $this->proxy->getTransactionType();
-    switch ($type) {
-      case PhabricatorTransactions::TYPE_COMMENT:
-        return PhabricatorAuditActionConstants::COMMENT;
-      case PhabricatorAuditActionConstants::INLINE:
-      case PhabricatorAuditActionConstants::ADD_CCS:
-      case PhabricatorTransactions::TYPE_SUBSCRIBERS:
-      case PhabricatorAuditActionConstants::ADD_AUDITORS:
-        return $type;
-      default:
-        return $this->proxy->getNewValue();
-    }
-  }
-
-  public function save() {
-    throw new Exception(
-      pht('This object can no longer be written to directly!'));
-  }
-
-  public function getTransactionForSave() {
-    $xaction = $this->proxy;
-    if (strlen($this->getContent())) {
-      $xaction->attachComment($this->getProxyComment());
-    }
-
-    return $xaction;
-  }
-
-  public function setNewValue($value) {
-    $this->proxy->setNewValue($value);
-    return $this;
-  }
-
-  public function getDateCreated() {
-    return $this->proxy->getDateCreated();
-  }
-
-  public function getDateModified() {
-    return $this->proxy->getDateModified();
-  }
-
-
-/* -(  PhabricatorMarkupInterface Implementation  )-------------------------- */
-
-
-  public function getMarkupFieldKey($field) {
-    return 'AC:'.$this->getPHID();
-  }
-
-  public function newMarkupEngine($field) {
-    return PhabricatorMarkupEngine::newDiffusionMarkupEngine();
-  }
-
-  public function getMarkupText($field) {
-    return $this->getContent();
-  }
-
-  public function didMarkupText($field, $output, PhutilMarkupEngine $engine) {
-    return $output;
-  }
-
-  public function shouldUseMarkupCache($field) {
-    return (bool)$this->getPHID();
-  }
-
-}