Migrate audit comments to transactions

Summary:
Ref T4896. Depends on D10052. This is the major/scary migration, but not really so bad. It is substantially similar to D8210, but less complex because there are fewer actions here.

This moves `PhabricatorAuditComment` storage to `PhabricatorAuditTransaction`, then reads `PhabricatorAuditComment`s as a proxy around the new objects.

Test Plan:
  - Before migrating, browsed around. Nothing appeared broken.
  - Migrated cleanly.
  - Viewed old transactions (inlines, comments, accept/reject/etc, add auditors, add ccs, implicit CCs).
  - Added all of those comment types.
  - Edited a draft.
  - Deleted a draft.
  - Spot checked the database for sanity.

Reviewers: btrahan, joshuaspence

Reviewed By: joshuaspence

Subscribers: epriestley

Maniphest Tasks: T4896

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

resources/sql/autopatches/20140725.audit.1.migxactions.php

@@ -0,0 +1,150 @@
+<?php
+
+$conn_w = id(new PhabricatorAuditTransaction())->establishConnection('w');
+$rows = new LiskRawMigrationIterator($conn_w, 'audit_comment');
+
+$content_source = PhabricatorContentSource::newForSource(
+  PhabricatorContentSource::SOURCE_LEGACY,
+  array())->serialize();
+
+echo "Migrating Audit comments to modern storage...\n";
+foreach ($rows as $row) {
+  $id = $row['id'];
+  echo "Migrating comment {$id}...\n";
+
+  $comments = queryfx_all(
+    $conn_w,
+    'SELECT * FROM %T WHERE legacyCommentID = %d',
+    'audit_transaction_comment',
+    $id);
+
+  $main_comments = array();
+  $inline_comments = array();
+
+  foreach ($comments as $comment) {
+    if ($comment['pathID']) {
+      $inline_comments[] = $comment;
+    } else {
+      $main_comments[] = $comment;
+    }
+  }
+
+  $metadata = json_decode($row['metadata'], true);
+  if (!is_array($metadata)) {
+    $metadata = array();
+  }
+
+  $xactions = array();
+
+  // Build the main action transaction.
+  switch ($row['action']) {
+    case PhabricatorAuditActionConstants::ADD_AUDITORS:
+      $phids = idx($metadata, 'added-auditors', array());
+      $xactions[] = array(
+        'type' => $row['action'],
+        'old' => null,
+        'new' => array_fuse($phids),
+      );
+      break;
+    case PhabricatorAuditActionConstants::ADD_CCS:
+      $phids = idx($metadata, 'added-ccs', array());
+      $xactions[] = array(
+        'type' => $row['action'],
+        'old' => null,
+        'new' => array_fuse($phids),
+      );
+      break;
+    case PhabricatorAuditActionConstants::COMMENT:
+    case PhabricatorAuditActionConstants::INLINE:
+      // These actions will have their transactions created by other rules.
+      break;
+    default:
+      // Otherwise, this is an accept/concern/etc action.
+      $xactions[] = array(
+        'type' => PhabricatorAuditActionConstants::ACTION,
+        'old' => null,
+        'new' => $row['action'],
+      );
+      break;
+  }
+
+
+  // Build the main comment transaction.
+  foreach ($main_comments as $main) {
+    $xactions[] = array(
+      'type' => PhabricatorTransactions::TYPE_COMMENT,
+      'old' => null,
+      'new' => null,
+      'phid' => $main['transactionPHID'],
+      'comment' => $main,
+    );
+  }
+
+  // Build inline comment transactions.
+  foreach ($inline_comments as $inline) {
+    $xactions[] = array(
+      'type' => PhabricatorAuditActionConstants::INLINE,
+      'old' => null,
+      'new' => null,
+      'phid' => $inline['transactionPHID'],
+      'comment' => $inline,
+    );
+  }
+
+  foreach ($xactions as $xaction) {
+    // Generate a new PHID, if we don't already have one from the comment
+    // table. We pregenerated into the comment table to make this a little
+    // easier, so we only need to write to one table.
+    $xaction_phid = idx($xaction, 'phid');
+    if (!$xaction_phid) {
+      $xaction_phid = PhabricatorPHID::generateNewPHID(
+        PhabricatorApplicationTransactionTransactionPHIDType::TYPECONST,
+        PhabricatorRepositoryCommitPHIDType::TYPECONST);
+    }
+    unset($xaction['phid']);
+
+    $comment_phid = null;
+    $comment_version = 0;
+    if (idx($xaction, 'comment')) {
+      $comment_phid = $xaction['comment']['phid'];
+      $comment_version = 1;
+    }
+
+    $old = idx($xaction, 'old');
+    $new = idx($xaction, 'new');
+    $meta = idx($xaction, 'meta', array());
+
+    queryfx(
+      $conn_w,
+      'INSERT INTO %T (phid, authorPHID, objectPHID, viewPolicy, editPolicy,
+          commentPHID, commentVersion, transactionType, oldValue, newValue,
+          contentSource, metadata, dateCreated, dateModified)
+        VALUES (%s, %s, %s, %s, %s, %ns, %d, %s, %ns, %ns, %s, %s, %d, %d)',
+      'audit_transaction',
+
+      // PHID, authorPHID, objectPHID
+      $xaction_phid,
+      $row['actorPHID'],
+      $row['targetPHID'],
+
+      // viewPolicy, editPolicy, commentPHID, commentVersion
+      'public',
+      $row['actorPHID'],
+      $comment_phid,
+      $comment_version,
+
+      // transactionType, oldValue, newValue, contentSource, metadata
+      $xaction['type'],
+      json_encode($old),
+      json_encode($new),
+      $content_source,
+      json_encode($meta),
+
+      // dates
+      $row['dateCreated'],
+      $row['dateModified']);
+  }
+
+}
+
+echo "Done.\n";

src/__phutil_library_map__.php

@@ -3929,10 +3929,7 @@ phutil_register_library_map(array(
     'PhabricatorAsanaConfigOptions' => 'PhabricatorApplicationConfigOptions',
     'PhabricatorAuditAddCommentController' => 'PhabricatorAuditController',
     'PhabricatorAuditApplication' => 'PhabricatorApplication',
-    'PhabricatorAuditComment' => array(
+    'PhabricatorAuditComment' => 'PhabricatorMarkupInterface',
-      'PhabricatorAuditDAO',
-      'PhabricatorMarkupInterface',
-    ),
     'PhabricatorAuditCommentEditor' => 'PhabricatorEditor',
     'PhabricatorAuditController' => 'PhabricatorController',
     'PhabricatorAuditDAO' => 'PhabricatorLiskDAO',

src/applications/audit/constants/PhabricatorAuditActionConstants.php

@@ -10,6 +10,7 @@ final class PhabricatorAuditActionConstants {
   const ADD_CCS = 'add_ccs';
   const ADD_AUDITORS = 'add_auditors';
   const INLINE = 'audit:inline';
+  const ACTION = 'audit:action';
 
   public static function getActionNameMap() {
     $map = array(

src/applications/audit/editor/PhabricatorAuditCommentEditor.php

@@ -262,23 +262,27 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor {
     $commit->updateAuditStatus($requests);
     $commit->save();
 
+    $content_source = PhabricatorContentSource::newForSource(
+      PhabricatorContentSource::SOURCE_LEGACY,
+      array());
+
     foreach ($comments as $comment) {
       $comment
         ->setActorPHID($actor->getPHID())
         ->setTargetPHID($commit->getPHID())
+        ->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();
 
-      $inline->setAuditCommentID($xaction->getID());
-      $inline->save();
-
       $comments[] = $xaction;
     }
 

src/applications/audit/storage/PhabricatorAuditComment.php

@@ -1,6 +1,6 @@
 <?php
 
-final class PhabricatorAuditComment extends PhabricatorAuditDAO
+final class PhabricatorAuditComment
   implements PhabricatorMarkupInterface {
 
   const METADATA_ADDED_AUDITORS  = 'added-auditors';
@@ -8,73 +8,93 @@ final class PhabricatorAuditComment extends PhabricatorAuditDAO
 
   const MARKUP_FIELD_BODY        = 'markup:body';
 
-  protected $phid;
-  protected $actorPHID;
-  protected $targetPHID;
-  protected $action;
-  protected $content = '';
-  protected $metadata = array();
-
   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) {
 
-    $comments = id(new PhabricatorAuditComment())->loadAllWhere(
+    $xactions = id(new PhabricatorAuditTransactionQuery())
-      'targetPHID = %s',
+      ->setViewer($viewer)
-      $commit_phid);
+      ->withObjectPHIDs(array($commit_phid))
+      ->needComments(true)
+      ->execute();
 
-    if ($comments) {
+    $comments = array();
-      $table = new PhabricatorAuditTransactionComment();
+    foreach ($xactions as $xaction) {
-      $conn_r = $table->establishConnection('r');
+      $comments[] = self::newFromModernTransaction($xaction);
-
-      $data = queryfx_all(
-        $conn_r,
-        'SELECT * FROM %T WHERE legacyCommentID IN (%Ld) AND pathID IS NULL',
-        $table->getTableName(),
-        mpull($comments, 'getID'));
-      $texts = $table->loadAllFromArray($data);
-      $texts = mpull($texts, null, 'getLegacyCommentID');
-
-      foreach ($comments as $comment) {
-        $text = idx($texts, $comment->getID());
-        if ($text) {
-          $comment->setProxyComment($text);
-        }
-      }
     }
 
     return $comments;
   }
 
-  public function getConfiguration() {
+  public function getPHID() {
-    return array(
+    return $this->proxy->getPHID();
-      self::CONFIG_SERIALIZATION => array(
-        'metadata' => self::SERIALIZATION_JSON,
-      ),
-      self::CONFIG_AUX_PHID => true,
-    ) + parent::getConfiguration();
   }
 
-  public function generatePHID() {
+  public function getActorPHID() {
-    return PhabricatorPHID::generateNewPHID('ACMT');
+    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) {
-    // NOTE: We no longer read this field, but there's no cost to continuing
-    // to write it in case something goes horribly wrong, since it makes it
-    // far easier to back out of this.
-    $this->content = $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();
@@ -90,39 +110,116 @@ final class PhabricatorAuditComment extends PhabricatorAuditDAO
     return $this;
   }
 
-  public function setTargetPHID($target_phid) {
+  public function setAction($action) {
-    $this->getProxyComment()->setCommitPHID($target_phid);
+    switch ($action) {
-    return parent::setTargetPHID($target_phid);
+      case PhabricatorAuditActionConstants::INLINE:
+      case PhabricatorAuditActionConstants::ADD_CCS:
+      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 PhabricatorAuditActionConstants::ADD_AUDITORS:
+        return $type;
+      default:
+        return $this->proxy->getNewValue();
+    }
+  }
+
+  public function setMetadata(array $metadata) {
+    if (!$this->proxy->getTransactionType()) {
+      throw new Exception(pht('Call setAction() before getMetadata()!'));
+    }
+
+    $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;
+      default:
+        throw new Exception(pht('No metadata expected!'));
+    }
+
+    $this->proxy->setOldValue(array());
+    $this->proxy->setNewValue(array_fuse($raw_phids));
+
+    return $this;
+  }
+
+  public function getMetadata() {
+    if (!$this->proxy->getTransactionType()) {
+      throw new Exception(pht('Call setAction() before getMetadata()!'));
+    }
+
+    $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),
+        );
+    }
+
+    return array();
   }
 
   public function save() {
-    $this->openTransaction();
+    $this->proxy->openTransaction();
-      $result = parent::save();
+      $this->proxy
+        ->setViewPolicy('public')
+        ->setEditPolicy($this->getActorPHID())
+        ->save();
 
       if (strlen($this->getContent())) {
-        $content_source = PhabricatorContentSource::newForSource(
+        $this->getProxyComment()
-          PhabricatorContentSource::SOURCE_LEGACY,
-          array());
-
-        $xaction_phid = PhabricatorPHID::generateNewPHID(
-          PhabricatorApplicationTransactionTransactionPHIDType::TYPECONST,
-          PhabricatorRepositoryCommitPHIDType::TYPECONST);
-
-        $proxy = $this->getProxyComment();
-        $proxy
           ->setAuthorPHID($this->getActorPHID())
           ->setViewPolicy('public')
           ->setEditPolicy($this->getActorPHID())
-          ->setContentSource($content_source)
           ->setCommentVersion(1)
-          ->setLegacyCommentID($this->getID())
+          ->setTransactionPHID($this->proxy->getPHID())
-          ->setTransactionPHID($xaction_phid)
+          ->save();
+
+        $this->proxy
+          ->setCommentVersion(1)
+          ->setCommentPHID($this->getProxyComment()->getPHID())
           ->save();
       }
+    $this->proxy->saveTransaction();
 
-    $this->saveTransaction();
+    return $this;
+  }
 
-    return $result;
+  public function getDateCreated() {
+    return $this->proxy->getDateCreated();
+  }
+
+  public function getDateModified() {
+    return $this->proxy->getDateModified();
   }
 
 
@@ -130,7 +227,7 @@ final class PhabricatorAuditComment extends PhabricatorAuditDAO
 
 
   public function getMarkupFieldKey($field) {
-    return 'AC:'.$this->getID();
+    return 'AC:'.$this->getPHID();
   }
 
   public function newMarkupEngine($field) {
@@ -146,7 +243,7 @@ final class PhabricatorAuditComment extends PhabricatorAuditDAO
   }
 
   public function shouldUseMarkupCache($field) {
-    return (bool)$this->getID();
+    return (bool)$this->getPHID();
   }
 
 }

src/applications/audit/storage/PhabricatorAuditInlineComment.php

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

src/applications/diffusion/view/DiffusionCommentListView.php

@@ -65,14 +65,14 @@ final class DiffusionCommentListView extends AphrontView {
 
   public function render() {
 
-    $inline_comments = mgroup($this->inlineComments, 'getAuditCommentID');
+    $inline_comments = mgroup($this->inlineComments, 'getTransactionPHID');
 
     $num = 1;
 
     $comments = array();
     foreach ($this->comments as $comment) {
 
-      $inlines = idx($inline_comments, $comment->getID(), array());
+      $inlines = idx($inline_comments, $comment->getPHID(), array());
 
       $view = id(new DiffusionCommentView())
         ->setMarkupEngine($this->getMarkupEngine())