Provide a transaction editor to perform Audit row writes

Summary:
Ref T4896. Replaces more custom stuff with standard stuff. In particular:

  - No more fake proxy writes;
  - no more fake detection of `@mentions`.

For now, the old code still applies most of the effects and handles feed and email.

Test Plan:
  - Added comments.
  - Added comments with inline comments.
  - Added just inline comments.
  - Added comments with Conduit.
  - Previewed comments.
  - Added CCs explicitly and with `@mentions`.
  - Added auditors.
  - Accepted a commit.

Reviewers: joshuaspence, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4896

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

src/__phutil_library_map__.php

@@ -1141,6 +1141,7 @@ phutil_register_library_map(array(
     'PhabricatorAuditCommitStatusConstants' => 'applications/audit/constants/PhabricatorAuditCommitStatusConstants.php',
     'PhabricatorAuditController' => 'applications/audit/controller/PhabricatorAuditController.php',
     'PhabricatorAuditDAO' => 'applications/audit/storage/PhabricatorAuditDAO.php',
+    'PhabricatorAuditEditor' => 'applications/audit/editor/PhabricatorAuditEditor.php',
     'PhabricatorAuditInlineComment' => 'applications/audit/storage/PhabricatorAuditInlineComment.php',
     'PhabricatorAuditListController' => 'applications/audit/controller/PhabricatorAuditListController.php',
     'PhabricatorAuditListView' => 'applications/audit/view/PhabricatorAuditListView.php',
@@ -3934,6 +3935,7 @@ phutil_register_library_map(array(
     'PhabricatorAuditCommentEditor' => 'PhabricatorEditor',
     'PhabricatorAuditController' => 'PhabricatorController',
     'PhabricatorAuditDAO' => 'PhabricatorLiskDAO',
+    'PhabricatorAuditEditor' => 'PhabricatorApplicationTransactionEditor',
     'PhabricatorAuditInlineComment' => 'PhabricatorInlineCommentInterface',
     'PhabricatorAuditListController' => 'PhabricatorAuditController',
     'PhabricatorAuditListView' => 'AphrontView',

src/applications/audit/controller/PhabricatorAuditAddCommentController.php

@@ -12,9 +12,10 @@ final class PhabricatorAuditAddCommentController
     }
 
     $commit_phid = $request->getStr('commit');
-    $commit = id(new PhabricatorRepositoryCommit())->loadOneWhere(
-      'phid = %s',
-      $commit_phid);
+    $commit = id(new DiffusionCommitQuery())
+      ->setViewer($user)
+      ->withPHIDs(array($commit_phid))
+      ->executeOne();
     if (!$commit) {
       return new Aphront404Response();
     }
@@ -36,12 +37,11 @@ final class PhabricatorAuditAddCommentController
             ));
         break;
       case PhabricatorAuditActionConstants::ADD_CCS:
-        $ccs = $request->getArr('ccs');
         $comments[] = id(new PhabricatorAuditComment())
-          ->setAction(PhabricatorAuditActionConstants::ADD_CCS)
-          ->setMetadata(
+          ->setAction(PhabricatorTransactions::TYPE_SUBSCRIBERS)
+          ->setNewValue(
             array(
-              PhabricatorAuditComment::METADATA_ADDED_CCS => $ccs,
+              '+' => $request->getArr('ccs'),
             ));
         break;
       case PhabricatorAuditActionConstants::COMMENT:

src/applications/audit/controller/PhabricatorAuditPreviewController.php

@@ -37,8 +37,13 @@ final class PhabricatorAuditPreviewController
 
       $ccs = $request->getStrList('ccs');
       if ($action == PhabricatorAuditActionConstants::ADD_CCS && $ccs) {
-        $action_xaction->setTransactionType($action);
-        $action_xaction->setNewValue(array_fuse($ccs));
+        $action_xaction->setTransactionType(
+          PhabricatorTransactions::TYPE_SUBSCRIBERS);
+
+        // NOTE: This doesn't get processed before use, so just provide fake
+        // values.
+        $action_xaction->setOldValue(array());
+        $action_xaction->setNewValue($ccs);
       }
 
       $xactions[] = $action_xaction;

src/applications/audit/editor/PhabricatorAuditCommentEditor.php

@@ -38,28 +38,6 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor {
         $commit->getPHID());
     }
 
-    $content_blocks = array();
-    foreach ($comments as $comment) {
-      $content_blocks[] = $comment->getContent();
-    }
-
-    foreach ($inline_comments as $inline) {
-      $content_blocks[] = $inline->getContent();
-    }
-
-    // Find any "@mentions" in the content blocks.
-    $mention_ccs = PhabricatorMarkupEngine::extractPHIDsFromMentions(
-      $this->getActor(),
-      $content_blocks);
-    if ($mention_ccs) {
-      $comments[] = id(new PhabricatorAuditComment())
-        ->setAction(PhabricatorAuditActionConstants::ADD_CCS)
-        ->setMetadata(
-          array(
-            PhabricatorAuditComment::METADATA_ADDED_CCS => $mention_ccs,
-          ));
-    }
-
     // When an actor submits an audit comment, we update all the audit requests
     // they have authority over to reflect the most recent status. The general
     // idea here is that if audit has triggered for, e.g., several packages, but
@@ -93,8 +71,6 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor {
       }
     }
 
-    $add_self_cc = false;
-
     if ($action == PhabricatorAuditActionConstants::CLOSE) {
       if (!PhabricatorEnv::getEnvConfig('audit.can-author-close-audit')) {
         throw new Exception('Cannot Close Audit without enabling'.
@@ -181,7 +157,6 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor {
           case PhabricatorAuditActionConstants::COMMENT:
           case PhabricatorAuditActionConstants::ADD_AUDITORS:
           case PhabricatorAuditActionConstants::ADD_CCS:
-            $add_self_cc = true;
             break;
           case PhabricatorAuditActionConstants::ACCEPT:
             $new_status = PhabricatorAuditStatusConstants::ACCEPTED;
@@ -208,11 +183,6 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor {
     }
 
     $auditors = array();
-    $ccs = array();
-
-    if ($add_self_cc) {
-      $ccs[] = $actor->getPHID();
-    }
 
     foreach ($comments as $comment) {
       $meta = $comment->getMetadata();
@@ -224,20 +194,11 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor {
       foreach ($auditor_phids as $phid) {
         $auditors[] = $phid;
       }
-
-      $cc_phids = idx(
-        $meta,
-        PhabricatorAuditComment::METADATA_ADDED_CCS,
-        array());
-      foreach ($cc_phids as $phid) {
-        $ccs[] = $phid;
-      }
     }
 
     $requests_by_auditor = mpull($requests, null, 'getAuditorPHID');
     $requests_phids = array_keys($requests_by_auditor);
 
-    $ccs = array_diff($ccs, $requests_phids);
     $auditors = array_diff($auditors, $requests_phids);
 
     if ($auditors) {
@@ -256,37 +217,30 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor {
     $commit->updateAuditStatus($requests);
     $commit->save();
 
-    if ($ccs) {
-      id(new PhabricatorSubscriptionsEditor())
-        ->setActor($actor)
-        ->setObject($commit)
-        ->subscribeExplicit($ccs)
-        ->save();
+    // 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());
 
-    foreach ($comments as $comment) {
-      $comment
-        ->setActorPHID($actor->getPHID())
-        ->setTargetPHID($commit->getPHID())
+    $editor = id(new PhabricatorAuditEditor())
+      ->setActor($actor)
+      ->setContinueOnNoEffect(true)
+      ->setContinueOnMissingFields(true)
       ->setContentSource($content_source)
-        ->save();
-    }
-
-    foreach ($inline_comments as $inline) {
-      $xaction = id(new PhabricatorAuditComment())
-        ->setProxyComment($inline->getTransactionCommentForSave())
-        ->setAction(PhabricatorAuditActionConstants::INLINE)
-        ->setActorPHID($actor->getPHID())
-        ->setTargetPHID($commit->getPHID())
-        ->setContentSource($content_source)
-        ->save();
-
-      $comments[] = $xaction;
-    }
+      ->applyTransactions($commit, $xactions);
 
     $feed_dont_publish_phids = array();
     foreach ($requests as $request) {
@@ -396,8 +350,6 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor {
     assert_instances_of($other_comments, 'PhabricatorAuditComment');
     assert_instances_of($inline_comments, 'PhabricatorInlineCommentInterface');
 
-    $any_comment = head($comments);
-
     $commit = $this->commit;
 
     $data = $commit->loadCommitData();
@@ -421,7 +373,12 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor {
       PhabricatorAuditActionConstants::ADD_CCS => 'Added CCs',
       PhabricatorAuditActionConstants::ADD_AUDITORS => 'Added Auditors',
     );
-    $verb = idx($map, $any_comment->getAction(), 'Commented On');
+    if ($comments) {
+      $any_action = head($comments)->getAction();
+    } else {
+      $any_action = null;
+    }
+    $verb = idx($map, $any_action, 'Commented On');
 
     $reply_handler = self::newReplyHandlerForCommit($commit);
 
@@ -444,7 +401,7 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor {
     $email_to = array();
     $email_cc = array();
 
-    $email_to[$any_comment->getActorPHID()] = true;
+    $email_to[$this->getActor()->getPHID()] = true;
 
     $author_phid = $data->getCommitDetail('authorPHID');
     if ($author_phid) {
@@ -493,7 +450,7 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor {
       ->setSubject("{$name}: {$summary}")
       ->setSubjectPrefix($prefix)
       ->setVarySubjectPrefix("[{$verb}]")
-      ->setFrom($any_comment->getActorPHID())
+      ->setFrom($this->getActor()->getPHID())
       ->setThreadID($thread_id, $is_new)
       ->addHeader('Thread-Topic', $thread_topic)
       ->setRelatedPHID($commit->getPHID())

src/applications/audit/editor/PhabricatorAuditEditor.php

@@ -0,0 +1,114 @@
+<?php
+
+final class PhabricatorAuditEditor
+  extends PhabricatorApplicationTransactionEditor {
+
+  public function getTransactionTypes() {
+    $types = parent::getTransactionTypes();
+
+    $types[] = PhabricatorTransactions::TYPE_COMMENT;
+
+    // TODO: These will get modernized eventually, but that can happen one
+    // at a time later on.
+    $types[] = PhabricatorAuditActionConstants::ACTION;
+    $types[] = PhabricatorAuditActionConstants::INLINE;
+    $types[] = PhabricatorAuditActionConstants::ADD_AUDITORS;
+
+    return $types;
+  }
+
+  protected function transactionHasEffect(
+    PhabricatorLiskDAO $object,
+    PhabricatorApplicationTransaction $xaction) {
+
+    switch ($xaction->getTransactionType()) {
+      case PhabricatorAuditActionConstants::INLINE:
+        return $xaction->hasComment();
+    }
+
+    return parent::transactionHasEffect($object, $xaction);
+  }
+
+  protected function getCustomTransactionOldValue(
+    PhabricatorLiskDAO $object,
+    PhabricatorApplicationTransaction $xaction) {
+    switch ($xaction->getTransactionType()) {
+      case PhabricatorAuditActionConstants::ACTION:
+      case PhabricatorAuditActionConstants::INLINE:
+        return null;
+      case PhabricatorAuditActionConstants::ADD_AUDITORS:
+        // TODO: For now, just record the added PHIDs. Eventually, turn these
+        // into real edge transactions, probably?
+        return array();
+    }
+
+    return parent::getCustomTransactionOldValue($object, $xaction);
+  }
+
+  protected function getCustomTransactionNewValue(
+    PhabricatorLiskDAO $object,
+    PhabricatorApplicationTransaction $xaction) {
+
+    switch ($xaction->getTransactionType()) {
+      case PhabricatorAuditActionConstants::ACTION:
+      case PhabricatorAuditActionConstants::INLINE:
+      case PhabricatorAuditActionConstants::ADD_AUDITORS:
+        return $xaction->getNewValue();
+    }
+
+    return parent::getCustomTransactionNewValue($object, $xaction);
+  }
+
+  protected function applyCustomInternalTransaction(
+    PhabricatorLiskDAO $object,
+    PhabricatorApplicationTransaction $xaction) {
+
+    switch ($xaction->getTransactionType()) {
+      case PhabricatorTransactions::TYPE_COMMENT:
+      case PhabricatorTransactions::TYPE_SUBSCRIBERS:
+      case PhabricatorAuditActionConstants::ACTION:
+      case PhabricatorAuditActionConstants::INLINE:
+      case PhabricatorAuditActionConstants::ADD_AUDITORS:
+        return;
+    }
+
+    return parent::applyCustomInternalTransaction($object, $xaction);
+  }
+
+  protected function applyCustomExternalTransaction(
+    PhabricatorLiskDAO $object,
+    PhabricatorApplicationTransaction $xaction) {
+
+    switch ($xaction->getTransactionType()) {
+      case PhabricatorTransactions::TYPE_COMMENT:
+      case PhabricatorTransactions::TYPE_SUBSCRIBERS:
+      case PhabricatorAuditActionConstants::ACTION:
+      case PhabricatorAuditActionConstants::INLINE:
+        return;
+      case PhabricatorAuditActionConstants::ADD_AUDITORS:
+        // TODO: For now, these are applied externally.
+        return;
+    }
+
+    return parent::applyCustomExternalTransaction($object, $xaction);
+  }
+
+  protected function sortTransactions(array $xactions) {
+    $xactions = parent::sortTransactions($xactions);
+
+    $head = array();
+    $tail = array();
+
+    foreach ($xactions as $xaction) {
+      $type = $xaction->getTransactionType();
+      if ($type == PhabricatorAuditActionConstants::INLINE) {
+        $tail[] = $xaction;
+      } else {
+        $head[] = $xaction;
+      }
+    }
+
+    return array_values(array_merge($head, $tail));
+  }
+
+}

src/applications/audit/storage/PhabricatorAuditComment.php

@@ -4,7 +4,6 @@ final class PhabricatorAuditComment
   implements PhabricatorMarkupInterface {
 
   const METADATA_ADDED_AUDITORS  = 'added-auditors';
-  const METADATA_ADDED_CCS       = 'added-ccs';
 
   const MARKUP_FIELD_BODY        = 'markup:body';
 
@@ -114,6 +113,7 @@ final class PhabricatorAuditComment
     switch ($action) {
       case PhabricatorAuditActionConstants::INLINE:
       case PhabricatorAuditActionConstants::ADD_CCS:
+      case PhabricatorTransactions::TYPE_SUBSCRIBERS:
       case PhabricatorAuditActionConstants::ADD_AUDITORS:
         $this->proxy->setTransactionType($action);
         break;
@@ -137,6 +137,7 @@ final class PhabricatorAuditComment
         return PhabricatorAuditActionConstants::COMMENT;
       case PhabricatorAuditActionConstants::INLINE:
       case PhabricatorAuditActionConstants::ADD_CCS:
+      case PhabricatorTransactions::TYPE_SUBSCRIBERS:
       case PhabricatorAuditActionConstants::ADD_AUDITORS:
         return $type;
       default:
@@ -151,9 +152,6 @@ final class PhabricatorAuditComment
 
     $type = $this->proxy->getTransactionType();
     switch ($type) {
-      case PhabricatorAuditActionConstants::ADD_CCS:
-        $raw_phids = idx($metadata, self::METADATA_ADDED_CCS, array());
-        break;
       case PhabricatorAuditActionConstants::ADD_AUDITORS:
         $raw_phids = idx($metadata, self::METADATA_ADDED_AUDITORS, array());
         break;
@@ -161,7 +159,6 @@ final class PhabricatorAuditComment
         throw new Exception(pht('No metadata expected!'));
     }
 
-    $this->proxy->setOldValue(array());
     $this->proxy->setNewValue(array_fuse($raw_phids));
 
     return $this;
@@ -175,10 +172,6 @@ final class PhabricatorAuditComment
     $type = $this->proxy->getTransactionType();
     $new_value = $this->proxy->getNewValue();
     switch ($type) {
-      case PhabricatorAuditActionConstants::ADD_CCS:
-        return array(
-          self::METADATA_ADDED_CCS => array_keys($new_value),
-        );
       case PhabricatorAuditActionConstants::ADD_AUDITORS:
         return array(
           self::METADATA_ADDED_AUDITORS => array_keys($new_value),
@@ -189,28 +182,21 @@ final class PhabricatorAuditComment
   }
 
   public function save() {
-    $this->proxy->openTransaction();
-      $this->proxy
-        ->setViewPolicy('public')
-        ->setEditPolicy($this->getActorPHID())
-        ->save();
-
-      if (strlen($this->getContent())) {
-        $this->getProxyComment()
-          ->setAuthorPHID($this->getActorPHID())
-          ->setViewPolicy('public')
-          ->setEditPolicy($this->getActorPHID())
-          ->setCommentVersion(1)
-          ->setTransactionPHID($this->proxy->getPHID())
-          ->save();
-
-        $this->proxy
-          ->setCommentVersion(1)
-          ->setCommentPHID($this->getProxyComment()->getPHID())
-          ->save();
+    throw new Exception(
+      pht('This object can no longer be written to directly!'));
   }
-    $this->proxy->saveTransaction();
 
+  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;
   }
 

src/applications/audit/storage/PhabricatorAuditInlineComment.php

@@ -18,6 +18,10 @@ final class PhabricatorAuditInlineComment
     return $this->proxy->getTransactionPHID();
   }
 
+  public function getTransactionComment() {
+    return $this->proxy;
+  }
+
   public function getTransactionCommentForSave() {
     $content_source = PhabricatorContentSource::newForSource(
       PhabricatorContentSource::SOURCE_LEGACY,

src/applications/audit/storage/PhabricatorAuditTransaction.php

@@ -83,7 +83,9 @@ final class PhabricatorAuditTransaction
 
     switch ($type) {
       case PhabricatorAuditActionConstants::INLINE:
-        break;
+        return pht(
+          '%s added inline comments.',
+          $author_handle);
       case PhabricatorAuditActionConstants::ADD_CCS:
         if ($add && $rem) {
           return pht(

src/applications/diffusion/conduit/DiffusionCreateCommentConduitAPIMethod.php

@@ -40,10 +40,10 @@ final class DiffusionCreateCommentConduitAPIMethod
 
   protected function execute(ConduitAPIRequest $request) {
     $commit_phid = $request->getValue('phid');
-    $commit = id(new PhabricatorRepositoryCommit())->loadOneWhere(
-      'phid = %s',
-      $commit_phid);
-
+    $commit = id(new DiffusionCommitQuery())
+      ->setViewer($request->getUser())
+      ->withPHIDs(array($commit_phid))
+      ->executeOne();
     if (!$commit) {
       throw new ConduitException('ERR_BAD_COMMIT');
     }