Put all DifferentialComment loading behind DifferentialCommentQuery

Summary:
Ref T2222.

I'm thinking about how I want to approach the Asana sync, and I want to try to do T2222 first so that we can build it cleanly on top of ApplicationTransactions. I think we can at least walk down this road a little bit and if it turns out to be scary we can take another approach.

I was generally very happy with how the auth migration turned out (seemingly, it was almost completely clean), and want to pursue a similar strategy here. Basically:

  - Wrap the new objects in the old objects for reads/writes.
  - Migrate all the existing data to the new table.
  - Everything hard is done; move things over a piece at a time at a leisurely pace in lots of smallish, relatively-easy-to-understand changes.

This deletes or abstracts all reads of the DifferentialComment table. In particular, these things are **deleted**:

  - The script `undo_commits.php`, which I haven't pointed anyone at in a very long time.
  - The `differential.getrevisionfeedback` Conduit method, which has been marked deprecated for a year or more.
  - The `/stats/` interface in Differential, which should be rebuilt on Fact and has never been exposed in the UI. It does a ton of joins and such which are prohibitively difficult to migrate.

This leaves a small number of reading interfaces, which I replaced with a new `DifferentialCommentQuery`. Some future change will make this actually load transactions and wrap them with DifferentialComment interfaces.

Test Plan: Viewed a revision; made revision comments

Reviewers: btrahan

Reviewed By: btrahan

CC: edward, chad, aran

Maniphest Tasks: T2222

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

scripts/repository/undo_commits.php

@@ -1,178 +0,0 @@
-#!/usr/bin/env php
-<?php
-
-$root = dirname(dirname(dirname(__FILE__)));
-require_once $root.'/scripts/__init_script__.php';
-
-$args = new PhutilArgumentParser($argv);
-$args->setTagline('reopen reviews accidentally closed by a bad push');
-$args->setSynopsis(<<<EOSYNOPSIS
-**undo_commits.php** --repository __callsign__ < __commits__
-
-    Reopen code reviews accidentally closed by a bad push. If someone
-    pushes a bunch of commits to a tracked branch that they shouldn't
-    have, you can pipe in all the commit hashes to this script to
-    "undo" the damage in Differential after you revert the commits.
-
-    To use this script:
-
-      1. Identify the commits you want to undo the effects of.
-      2. Put all their identifiers (commit hashes in git/hg, revision
-         numbers in svn) into a file, one per line.
-      3. Pipe that file into this script with relevant arguments.
-      4. Revisions marked "closed" by those commits will be
-         restored to their previous state.
-
-EOSYNOPSIS
-  );
-$args->parseStandardArguments();
-$args->parse(
-  array(
-    array(
-      'name'    => 'repository',
-      'param'   => 'callsign',
-      'help'    => 'Callsign for the repository these commits appear in.',
-    ),
-  ));
-
-$callsign = $args->getArg('repository');
-if (!$callsign) {
-  $args->printHelpAndExit();
-}
-
-$repository = id(new PhabricatorRepository())->loadOneWhere(
-  'callsign = %s',
-  $callsign);
-
-if (!$repository) {
-  throw new Exception("No repository with callsign '{$callsign}'!");
-}
-
-echo "Reading commit identifiers from stdin...\n";
-
-$identifiers = @file_get_contents('php://stdin');
-$identifiers = trim($identifiers);
-$identifiers = explode("\n", $identifiers);
-
-echo "Read ".count($identifiers)." commit identifiers.\n";
-
-if (!$identifiers) {
-  throw new Exception("You must provide commmit identifiers on stdin!");
-}
-
-echo "Looking up commits...\n";
-$commits = id(new PhabricatorRepositoryCommit())->loadAllWhere(
-  'repositoryID = %d AND commitIdentifier IN (%Ls)',
-  $repository->getID(),
-  $identifiers);
-
-echo "Found ".count($commits)." matching commits.\n";
-
-if (!$commits) {
-  throw new Exception("None of the commits could be found!");
-}
-
-$commit_phids = mpull($commits, 'getPHID', 'getPHID');
-
-echo "Looking up revisions marked 'closed' by these commits...\n";
-$revision_ids = queryfx_all(
-  id(new DifferentialRevision())->establishConnection('r'),
-  'SELECT DISTINCT revisionID from %T WHERE commitPHID IN (%Ls)',
-  DifferentialRevision::TABLE_COMMIT,
-  $commit_phids);
-$revision_ids = ipull($revision_ids, 'revisionID');
-
-echo "Found ".count($revision_ids)." associated revisions.\n";
-if (!$revision_ids) {
-  echo "Done -- nothing to do.\n";
-  return;
-}
-
-$status_closed = ArcanistDifferentialRevisionStatus::CLOSED;
-
-$revisions = array();
-$map = array();
-
-if ($revision_ids) {
-  foreach ($revision_ids as $revision_id) {
-    echo "Assessing revision D{$revision_id}...\n";
-    $revision = id(new DifferentialRevision())->load($revision_id);
-
-    if ($revision->getStatus() != $status_closed) {
-      echo "Revision is not 'closed', skipping.\n";
-    }
-
-    $assoc_commits = queryfx_all(
-      $revision->establishConnection('r'),
-      'SELECT commitPHID FROM %T WHERE revisionID = %d',
-      DifferentialRevision::TABLE_COMMIT,
-      $revision_id);
-    $assoc_commits = ipull($assoc_commits, 'commitPHID', 'commitPHID');
-
-    if (array_diff_key($assoc_commits, $commit_phids)) {
-      echo "Revision is associated with other commits, skipping.\n";
-    }
-
-    $comments = id(new DifferentialComment())->loadAllWhere(
-      'revisionID = %d',
-      $revision_id);
-
-    $new_status = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW;
-    foreach ($comments as $comment) {
-      switch ($comment->getAction()) {
-        case DifferentialAction::ACTION_ACCEPT:
-          $new_status = ArcanistDifferentialRevisionStatus::ACCEPTED;
-          break;
-        case DifferentialAction::ACTION_REJECT:
-        case DifferentialAction::ACTION_RETHINK:
-          $new_status = ArcanistDifferentialRevisionStatus::NEEDS_REVISION;
-          break;
-        case DifferentialAction::ACTION_ABANDON:
-          $new_status = ArcanistDifferentialRevisionStatus::ABANDONED;
-          break;
-        case DifferentialAction::ACTION_RECLAIM:
-        case DifferentialAction::ACTION_UPDATE:
-          $new_status = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW;
-          break;
-      }
-    }
-
-    $revisions[$revision_id] = $revision;
-    $map[$revision_id] = $new_status;
-  }
-}
-
-if (!$revisions) {
-  echo "Done -- nothing to do.\n";
-}
-
-echo "Found ".count($revisions)." revisions to update:\n\n";
-foreach ($revisions as $id => $revision) {
-
-  $old_status = ArcanistDifferentialRevisionStatus::getNameForRevisionStatus(
-    $revision->getStatus());
-  $new_status = ArcanistDifferentialRevisionStatus::getNameForRevisionStatus(
-    $map[$id]);
-
-  echo "    - D{$id}: ".$revision->getTitle()."\n";
-  echo "      Will update: {$old_status} -> {$new_status}\n\n";
-}
-
-$ok = phutil_console_confirm('Apply these changes?');
-if (!$ok) {
-  echo "Aborted.\n";
-  exit(1);
-}
-
-echo "Saving changes...\n";
-foreach ($revisions as $id => $revision) {
-  queryfx(
-    $revision->establishConnection('r'),
-    'UPDATE %T SET status = %d WHERE id = %d',
-    $revision->getTableName(),
-    $map[$id],
-    $id);
-}
-echo "Done.\n";
-
-

src/__phutil_library_map__.php

@@ -141,7 +141,6 @@ phutil_register_library_map(array(
     'ConduitAPI_differential_getdiff_Method' => 'applications/differential/conduit/ConduitAPI_differential_getdiff_Method.php',
     'ConduitAPI_differential_getrevision_Method' => 'applications/differential/conduit/ConduitAPI_differential_getrevision_Method.php',
     'ConduitAPI_differential_getrevisioncomments_Method' => 'applications/differential/conduit/ConduitAPI_differential_getrevisioncomments_Method.php',
-    'ConduitAPI_differential_getrevisionfeedback_Method' => 'applications/differential/conduit/ConduitAPI_differential_getrevisionfeedback_Method.php',
     'ConduitAPI_differential_markcommitted_Method' => 'applications/differential/conduit/ConduitAPI_differential_markcommitted_Method.php',
     'ConduitAPI_differential_parsecommitmessage_Method' => 'applications/differential/conduit/ConduitAPI_differential_parsecommitmessage_Method.php',
     'ConduitAPI_differential_query_Method' => 'applications/differential/conduit/ConduitAPI_differential_query_Method.php',
@@ -331,6 +330,7 @@ phutil_register_library_map(array(
     'DifferentialCommentEditor' => 'applications/differential/editor/DifferentialCommentEditor.php',
     'DifferentialCommentMail' => 'applications/differential/mail/DifferentialCommentMail.php',
     'DifferentialCommentPreviewController' => 'applications/differential/controller/DifferentialCommentPreviewController.php',
+    'DifferentialCommentQuery' => 'applications/differential/query/DifferentialCommentQuery.php',
     'DifferentialCommentSaveController' => 'applications/differential/controller/DifferentialCommentSaveController.php',
     'DifferentialCommitsFieldSpecification' => 'applications/differential/field/specification/DifferentialCommitsFieldSpecification.php',
     'DifferentialConflictsFieldSpecification' => 'applications/differential/field/specification/DifferentialConflictsFieldSpecification.php',
@@ -390,8 +390,6 @@ phutil_register_library_map(array(
     'DifferentialRevertPlanFieldSpecification' => 'applications/differential/field/specification/DifferentialRevertPlanFieldSpecification.php',
     'DifferentialReviewRequestMail' => 'applications/differential/mail/DifferentialReviewRequestMail.php',
     'DifferentialReviewedByFieldSpecification' => 'applications/differential/field/specification/DifferentialReviewedByFieldSpecification.php',
-    'DifferentialReviewerStats' => 'applications/differential/stats/DifferentialReviewerStats.php',
-    'DifferentialReviewerStatsTestCase' => 'applications/differential/stats/__tests__/DifferentialReviewerStatsTestCase.php',
     'DifferentialReviewersFieldSpecification' => 'applications/differential/field/specification/DifferentialReviewersFieldSpecification.php',
     'DifferentialRevision' => 'applications/differential/storage/DifferentialRevision.php',
     'DifferentialRevisionCommentListView' => 'applications/differential/view/DifferentialRevisionCommentListView.php',
@@ -408,7 +406,6 @@ phutil_register_library_map(array(
     'DifferentialRevisionListView' => 'applications/differential/view/DifferentialRevisionListView.php',
     'DifferentialRevisionMailReceiver' => 'applications/differential/mail/DifferentialRevisionMailReceiver.php',
     'DifferentialRevisionQuery' => 'applications/differential/query/DifferentialRevisionQuery.php',
-    'DifferentialRevisionStatsController' => 'applications/differential/controller/DifferentialRevisionStatsController.php',
     'DifferentialRevisionStatsView' => 'applications/differential/view/DifferentialRevisionStatsView.php',
     'DifferentialRevisionStatus' => 'applications/differential/constants/DifferentialRevisionStatus.php',
     'DifferentialRevisionStatusFieldSpecification' => 'applications/differential/field/specification/DifferentialRevisionStatusFieldSpecification.php',
@@ -2025,7 +2022,6 @@ phutil_register_library_map(array(
     'ConduitAPI_differential_getdiff_Method' => 'ConduitAPIMethod',
     'ConduitAPI_differential_getrevision_Method' => 'ConduitAPIMethod',
     'ConduitAPI_differential_getrevisioncomments_Method' => 'ConduitAPI_differential_Method',
-    'ConduitAPI_differential_getrevisionfeedback_Method' => 'ConduitAPIMethod',
     'ConduitAPI_differential_markcommitted_Method' => 'ConduitAPIMethod',
     'ConduitAPI_differential_parsecommitmessage_Method' => 'ConduitAPIMethod',
     'ConduitAPI_differential_query_Method' => 'ConduitAPIMethod',
@@ -2210,6 +2206,7 @@ phutil_register_library_map(array(
     'DifferentialCommentEditor' => 'PhabricatorEditor',
     'DifferentialCommentMail' => 'DifferentialMail',
     'DifferentialCommentPreviewController' => 'DifferentialController',
+    'DifferentialCommentQuery' => 'PhabricatorOffsetPagedQuery',
     'DifferentialCommentSaveController' => 'DifferentialController',
     'DifferentialCommitsFieldSpecification' => 'DifferentialFieldSpecification',
     'DifferentialConflictsFieldSpecification' => 'DifferentialFieldSpecification',
@@ -2268,7 +2265,6 @@ phutil_register_library_map(array(
     'DifferentialRevertPlanFieldSpecification' => 'DifferentialFieldSpecification',
     'DifferentialReviewRequestMail' => 'DifferentialMail',
     'DifferentialReviewedByFieldSpecification' => 'DifferentialFieldSpecification',
-    'DifferentialReviewerStatsTestCase' => 'PhabricatorTestCase',
     'DifferentialReviewersFieldSpecification' => 'DifferentialFieldSpecification',
     'DifferentialRevision' =>
     array(
@@ -2287,7 +2283,6 @@ phutil_register_library_map(array(
     'DifferentialRevisionListController' => 'DifferentialController',
     'DifferentialRevisionListView' => 'AphrontView',
     'DifferentialRevisionMailReceiver' => 'PhabricatorObjectMailReceiver',
-    'DifferentialRevisionStatsController' => 'DifferentialController',
     'DifferentialRevisionStatsView' => 'AphrontView',
     'DifferentialRevisionStatusFieldSpecification' => 'DifferentialFieldSpecification',
     'DifferentialRevisionUpdateHistoryView' => 'AphrontView',

src/applications/differential/application/PhabricatorApplicationDifferential.php

@@ -42,7 +42,6 @@ final class PhabricatorApplicationDifferential extends PhabricatorApplication {
         '' => 'DifferentialRevisionListController',
         'filter/(?P<filter>\w+)/(?:(?P<username>[\w._-]+)/)?' =>
           'DifferentialRevisionListController',
-        'stats/(?P<filter>\w+)/' => 'DifferentialRevisionStatsController',
         'diff/' => array(
           '(?P<id>[1-9]\d*)/' => 'DifferentialDiffViewController',
           'create/' => 'DifferentialDiffCreateController',

src/applications/differential/conduit/ConduitAPI_differential_getrevisioncomments_Method.php

@@ -34,9 +34,9 @@ final class ConduitAPI_differential_getrevisioncomments_Method
       return $results;
     }
 
-    $comments = id(new DifferentialComment())->loadAllWhere(
+    $comments = id(new DifferentialCommentQuery())
-      'revisionID IN (%Ld)',
+      ->withRevisionIDs($revision_ids)
-      $revision_ids);
+      ->execute();
 
     $with_inlines = $request->getValue('inlines');
     if ($with_inlines) {

src/applications/differential/conduit/ConduitAPI_differential_getrevisionfeedback_Method.php

@@ -1,67 +0,0 @@
-<?php
-
-/**
- * @group conduit
- */
-final class ConduitAPI_differential_getrevisionfeedback_Method
-  extends ConduitAPIMethod {
-
-  public function getMethodStatus() {
-    return self::METHOD_STATUS_DEPRECATED;
-  }
-
-  public function getMethodStatusDescription() {
-    return "Replaced by 'differential.getrevisioncomments'.";
-  }
-
-  public function getMethodDescription() {
-    return "Retrieve Differential Revision Feedback.";
-  }
-
-  public function defineParamTypes() {
-    return array(
-      'ids' => 'required list<int>',
-    );
-  }
-
-  public function defineReturnType() {
-    return 'nonempty list<dict<string, wild>>';
-  }
-
-  public function defineErrorTypes() {
-    return array(
-    );
-  }
-
-  protected function execute(ConduitAPIRequest $request) {
-    $results = array();
-    $revision_ids = $request->getValue('ids');
-
-    if (!$revision_ids) {
-      return $results;
-    }
-
-    $comments = id(new DifferentialComment())->loadAllWhere(
-      'revisionID IN (%Ld)',
-      $revision_ids);
-
-    // Helper dictionary to keep track of where the id/action pair is
-    // stored in results array.
-    $indexes = array();
-    foreach ($comments as $comment) {
-      $action = $comment->getAction();
-      $revision_id = $comment->getRevisionID();
-
-      if (isset($indexes[$action.$revision_id])) {
-        $results[$indexes[$action.$revision_id]]['count']++;
-      } else {
-        $indexes[$action.$revision_id] = count($results);
-        $results[] = array('id'     => $revision_id,
-                           'action' => $action,
-                           'count'  => 1);
-      }
-    }
-
-    return $results;
-  }
-}

src/applications/differential/controller/DifferentialRevisionStatsController.php

@@ -1,163 +0,0 @@
-<?php
-
-final class DifferentialRevisionStatsController extends DifferentialController {
-  private $filter;
-
-  private function loadRevisions($phid) {
-    $table = new DifferentialRevision();
-    $conn_r = $table->establishConnection('r');
-    $rows = queryfx_all(
-      $conn_r,
-      'SELECT revisions.* FROM %T revisions ' .
-      'JOIN %T comments ON comments.revisionID = revisions.id ' .
-      'JOIN (' .
-      ' SELECT revisionID FROM %T WHERE objectPHID = %s ' .
-      ' UNION ALL ' .
-      ' SELECT id from differential_revision WHERE authorPHID = %s) rel ' .
-      'ON (comments.revisionID = rel.revisionID)' .
-      'WHERE comments.action = %s' .
-      'AND comments.authorPHID = %s',
-      $table->getTableName(),
-      id(new DifferentialComment())->getTableName(),
-      DifferentialRevision::RELATIONSHIP_TABLE,
-      $phid,
-      $phid,
-      $this->filter,
-      $phid);
-    return $table->loadAllFromArray($rows);
-  }
-
-  private function loadComments($phid) {
-    $table = new DifferentialComment();
-    $conn_r = $table->establishConnection('r');
-    $rows = queryfx_all(
-      $conn_r,
-      'SELECT comments.* FROM %T comments ' .
-      'JOIN (' .
-      ' SELECT revisionID FROM %T WHERE objectPHID = %s ' .
-      ' UNION ALL ' .
-      ' SELECT id from differential_revision WHERE authorPHID = %s) rel ' .
-      'ON (comments.revisionID = rel.revisionID)' .
-      'WHERE comments.action = %s' .
-      'AND comments.authorPHID = %s',
-      $table->getTableName(),
-      DifferentialRevision::RELATIONSHIP_TABLE,
-      $phid,
-      $phid,
-      $this->filter,
-      $phid);
-
-    return $table->loadAllFromArray($rows);
-  }
-
-  private function loadDiffs(array $revisions) {
-    if (!$revisions) {
-      return array();
-    }
-
-    $diff_teml = new DifferentialDiff();
-    $diffs = $diff_teml->loadAllWhere(
-      'revisionID in (%Ld)',
-      array_keys($revisions));
-    return $diffs;
-  }
-
-  public function willProcessRequest(array $data) {
-    $this->filter = idx($data, 'filter');
-  }
-
-  public function processRequest() {
-    $request = $this->getRequest();
-    $user = $request->getUser();
-
-    if ($request->isFormPost()) {
-      $phid_arr = $request->getArr('view_user');
-      $view_target = head($phid_arr);
-      return id(new AphrontRedirectResponse())
-        ->setURI($request->getRequestURI()->alter('phid', $view_target));
-    }
-
-    $params = array_filter(
-      array(
-        'phid' => $request->getStr('phid'),
-      ));
-
-    // Fill in the defaults we'll actually use for calculations if any
-    // parameters are missing.
-    $params += array(
-      'phid' => $user->getPHID(),
-    );
-
-    $side_nav = new AphrontSideNavFilterView();
-    $side_nav->setBaseURI(id(new PhutilURI('/differential/stats/'))
-                          ->alter('phid', $params['phid']));
-    foreach (array(
-               DifferentialAction::ACTION_CLOSE,
-               DifferentialAction::ACTION_ACCEPT,
-               DifferentialAction::ACTION_REJECT,
-               DifferentialAction::ACTION_UPDATE,
-               DifferentialAction::ACTION_COMMENT,
-             ) as $action) {
-      $verb = ucfirst(DifferentialAction::getActionPastTenseVerb($action));
-      $side_nav->addFilter($action, $verb);
-    }
-    $this->filter =
-      $side_nav->selectFilter($this->filter,
-                              DifferentialAction::ACTION_CLOSE);
-
-    $panels = array();
-    $handles = $this->loadViewerHandles(array($params['phid']));
-
-    $filter_form = id(new AphrontFormView())
-      ->setAction('/differential/stats/'.$this->filter.'/')
-      ->setUser($user);
-
-    $filter_form->appendChild(
-      $this->renderControl($params['phid'], $handles));
-    $filter_form->appendChild(id(new AphrontFormSubmitControl())
-                              ->setValue(pht('Filter Revisions')));
-
-    $side_nav->appendChild($filter_form);
-
-    $comments = $this->loadComments($params['phid']);
-    $revisions = $this->loadRevisions($params['phid']);
-    $diffs = $this->loadDiffs($revisions);
-
-    $panel = new AphrontPanelView();
-    $panel->setHeader(pht('Differential rate analysis'));
-    $panel->appendChild(
-      id(new DifferentialRevisionStatsView())
-      ->setComments($comments)
-      ->setFilter($this->filter)
-      ->setRevisions($revisions)
-      ->setDiffs($diffs)
-      ->setUser($user));
-    $panels[] = $panel;
-
-    foreach ($panels as $panel) {
-      $side_nav->appendChild($panel);
-    }
-
-    return $this->buildStandardPageResponse(
-      $side_nav,
-      array(
-        'title' => pht('Differential Statistics'),
-      ));
-  }
-
-  private function renderControl($view_phid, $handles) {
-    $value = array();
-    if ($view_phid) {
-      $value = array(
-        $view_phid => $handles[$view_phid]->getFullName(),
-      );
-    }
-    return id(new AphrontFormTokenizerControl())
-      ->setDatasource('/typeahead/common/users/')
-      ->setLabel(pht('View User'))
-      ->setName('view_user')
-      ->setValue($value)
-      ->setLimit(1);
-  }
-
-}

src/applications/differential/query/DifferentialCommentQuery.php

@@ -0,0 +1,43 @@
+<?php
+
+/**
+ * Temporary wrapper for transitioning Differential to ApplicationTransactions.
+ */
+final class DifferentialCommentQuery
+  extends PhabricatorOffsetPagedQuery {
+
+  private $revisionIDs;
+
+  public function withRevisionIDs(array $ids) {
+    $this->revisionIDs = $ids;
+    return $this;
+  }
+
+  public function execute() {
+    $table = new DifferentialComment();
+    $conn_r = $table->establishConnection('r');
+
+    $data = queryfx_all(
+      $conn_r,
+      'SELECT * FROM %T %Q %Q',
+      $table->getTableName(),
+      $this->buildWhereClause($conn_r),
+      $this->buildLimitClause($conn_r));
+
+    return $table->loadAllFromArray($data);
+  }
+
+  private function buildWhereClause(AphrontDatabaseConnection $conn_r) {
+    $where = array();
+
+    if ($this->revisionIDs) {
+      $where[] = qsprintf(
+        $conn_r,
+        'revisionID IN (%Ld)',
+        $this->revisionIDs);
+    }
+
+    return $this->formatWhereClause($where);
+  }
+
+}

src/applications/differential/search/DifferentialSearchIndexer.php

@@ -53,7 +53,9 @@ final class DifferentialSearchIndexer
         time());
     }
 
-    $comments = $rev->loadRelatives(new DifferentialComment(), 'revisionID');
+    $comments = id(new DifferentialCommentQuery())
+      ->withRevisionIDs(array($rev->getID()))
+      ->execute();
 
     $inlines = $rev->loadRelatives(
       new DifferentialInlineComment(),

src/applications/differential/stats/DifferentialReviewerStats.php

@@ -1,226 +0,0 @@
-<?php
-
-final class DifferentialReviewerStats {
-  private $since = 0;
-  private $until;
-
-  public function setSince($value) {
-    $this->since = $value;
-    return $this;
-  }
-
-  public function setUntil($value) {
-    $this->until = $value;
-    return $this;
-  }
-
-  /**
-   * @return array($reviewed, $not_reviewed)
-   */
-  public function computeTimes(
-    DifferentialRevision $revision,
-    array $comments) {
-    assert_instances_of($comments, 'DifferentialComment');
-
-    $add_rev = DifferentialComment::METADATA_ADDED_REVIEWERS;
-    $rem_rev = DifferentialComment::METADATA_REMOVED_REVIEWERS;
-
-    $date = $revision->getDateCreated();
-
-    // Find out original reviewers.
-    $reviewers = array_fill_keys($revision->getReviewers(), $date);
-    foreach (array_reverse($comments) as $comment) {
-      $metadata = $comment->getMetadata();
-      foreach (idx($metadata, $add_rev, array()) as $phid) {
-        unset($reviewers[$phid]);
-      }
-      foreach (idx($metadata, $rem_rev, array()) as $phid) {
-        $reviewers[$phid] = $date;
-      }
-    }
-
-    $reviewed = array();
-    $not_reviewed = array();
-    $status = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW;
-
-    foreach ($comments as $comment) {
-      $date = $comment->getDateCreated();
-      $old_status = $status;
-
-      switch ($comment->getAction()) {
-        case DifferentialAction::ACTION_UPDATE:
-          if ($status != ArcanistDifferentialRevisionStatus::CLOSED &&
-              $status != ArcanistDifferentialRevisionStatus::ACCEPTED) {
-            $status = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW;
-          }
-          break;
-        case DifferentialAction::ACTION_REQUEST:
-        case DifferentialAction::ACTION_RECLAIM:
-          $status = ArcanistDifferentialRevisionStatus::NEEDS_REVIEW;
-          break;
-        case DifferentialAction::ACTION_REJECT:
-        case DifferentialAction::ACTION_RETHINK:
-          $status = ArcanistDifferentialRevisionStatus::NEEDS_REVISION;
-          break;
-        case DifferentialAction::ACTION_ACCEPT:
-          $status = ArcanistDifferentialRevisionStatus::ACCEPTED;
-          break;
-        case DifferentialAction::ACTION_CLOSE:
-          $status = ArcanistDifferentialRevisionStatus::CLOSED;
-          break;
-        case DifferentialAction::ACTION_ABANDON:
-          $status = ArcanistDifferentialRevisionStatus::ABANDONED;
-          break;
-      }
-
-      // Update current reviewers.
-      $metadata = $comment->getMetadata();
-      foreach (idx($metadata, $add_rev, array()) as $phid) {
-        // If someone reviewed a revision without being its reviewer then give
-        // him zero response time.
-        $reviewers[$phid] = $date;
-      }
-      foreach (idx($metadata, $rem_rev, array()) as $phid) {
-        $start = idx($reviewers, $phid);
-        if ($start !== null) {
-          if ($date >= $this->since) {
-            $reviewed[$phid][] = $date - $start;
-          }
-          unset($reviewers[$phid]);
-        }
-      }
-
-      // TODO: Respect workdays and status away.
-
-      if ($old_status != $status) {
-        if ($status == ArcanistDifferentialRevisionStatus::NEEDS_REVIEW) {
-          $reviewers = array_fill_keys(array_keys($reviewers), $date);
-        } else if ($date >= $this->since) {
-          if ($old_status == ArcanistDifferentialRevisionStatus::NEEDS_REVIEW) {
-            foreach ($reviewers as $phid => $start) {
-              if ($phid == $comment->getAuthorPHID()) {
-                $reviewed[$phid][] = $date - $start;
-              } else {
-                $not_reviewed[$phid][] = $date - $start;
-              }
-            }
-          }
-        }
-      }
-    }
-
-    if ($status == ArcanistDifferentialRevisionStatus::NEEDS_REVIEW) {
-      $date = ($this->until !== null ? $this->until : time());
-      if ($date >= $this->since) {
-        foreach ($reviewers as $phid => $start) {
-          $not_reviewed[$phid][] = $date - $start;
-        }
-      }
-    }
-
-    return array($reviewed, $not_reviewed);
-  }
-
-  public function loadAvgs() {
-    $limit = 1000;
-    $conn_r = id(new DifferentialRevision())->establishConnection('r');
-
-    $sums = array();
-    $counts = array();
-    $all_not_reviewed = array();
-
-    $last_id = 0;
-    do {
-      $where = '';
-      if ($this->until !== null) {
-        $where .= qsprintf(
-          $conn_r,
-          ' AND dateCreated < %d',
-          $this->until);
-      }
-      if ($this->since) {
-        $where .= qsprintf(
-          $conn_r,
-          ' AND (dateModified > %d OR status = %s)',
-          $this->since,
-          ArcanistDifferentialRevisionStatus::NEEDS_REVIEW);
-      }
-      $revisions = id(new DifferentialRevision())->loadAllWhere(
-        'id > %d%Q ORDER BY id LIMIT %d',
-        $last_id,
-        $where,
-        $limit);
-
-      if (!$revisions) {
-        break;
-      }
-      $last_id = last_key($revisions);
-
-      $relations = queryfx_all(
-        $conn_r,
-        'SELECT * FROM %T WHERE revisionID IN (%Ld) AND relation = %s',
-        DifferentialRevision::RELATIONSHIP_TABLE,
-        array_keys($revisions),
-        DifferentialRevision::RELATION_REVIEWER);
-      $relations = igroup($relations, 'revisionID');
-
-      $where = '';
-      if ($this->until !== null) {
-        $where = qsprintf(
-          $conn_r,
-          ' AND dateCreated < %d',
-          $this->until);
-      }
-      $all_comments = id(new DifferentialComment())->loadAllWhere(
-        'revisionID IN (%Ld)%Q ORDER BY revisionID, id',
-        array_keys($revisions),
-        $where);
-      $all_comments = mgroup($all_comments, 'getRevisionID');
-
-      foreach ($revisions as $id => $revision) {
-        $revision->attachRelationships(idx($relations, $id, array()));
-        $comments = idx($all_comments, $id, array());
-
-        list($reviewed, $not_reviewed) =
-          $this->computeTimes($revision, $comments);
-
-        foreach ($reviewed as $phid => $times) {
-          $sums[$phid] = idx($sums, $phid, 0) + array_sum($times);
-          $counts[$phid] = idx($counts, $phid, 0) + count($times);
-        }
-
-        foreach ($not_reviewed as $phid => $times) {
-          $all_not_reviewed[$phid][] = $times;
-        }
-      }
-    } while (count($revisions) >= $limit);
-
-    foreach ($all_not_reviewed as $phid => $not_reviewed) {
-      if (!array_key_exists($phid, $counts)) {
-        // If the person didn't make any reviews than take maximum time because
-        // he is at least that slow.
-        $sums[$phid] = max(array_map('max', $not_reviewed));
-        $counts[$phid] = 1;
-        continue;
-      }
-      $avg = $sums[$phid] / $counts[$phid];
-      foreach ($not_reviewed as $times) {
-        foreach ($times as $time) {
-          // Don't shorten the average time just because the reviewer was lucky
-          // to be in a group with someone faster.
-          if ($time > $avg) {
-            $sums[$phid] += $time;
-            $counts[$phid]++;
-          }
-        }
-      }
-    }
-
-    $avgs = array();
-    foreach ($sums as $phid => $sum) {
-      $avgs[$phid] = $sum / $counts[$phid];
-    }
-    return $avgs;
-  }
-
-}

src/applications/differential/stats/__tests__/DifferentialReviewerStatsTestCase.php

@@ -1,100 +0,0 @@
-<?php
-
-final class DifferentialReviewerStatsTestCase extends PhabricatorTestCase {
-
-  public function testReviewerStats() {
-    $revision = new DifferentialRevision();
-    $revision->setDateCreated(1);
-    $revision->attachRelationships(array(
-      $this->newReviewer('R1'),
-      $this->newReviewer('R3'),
-    ));
-
-    $comments = array(
-      $this->newComment(2, 'A', DifferentialAction::ACTION_COMMENT),
-      $this->newComment(4, 'A', DifferentialAction::ACTION_ADDREVIEWERS,
-        array(DifferentialComment::METADATA_ADDED_REVIEWERS => array('R3'))),
-      $this->newComment(8, 'R1', DifferentialAction::ACTION_REJECT),
-      $this->newComment(16, 'A', DifferentialAction::ACTION_COMMENT),
-      $this->newComment(32, 'A', DifferentialAction::ACTION_UPDATE),
-      $this->newComment(64, 'A', DifferentialAction::ACTION_UPDATE),
-      $this->newComment(128, 'A', DifferentialAction::ACTION_COMMENT),
-      $this->newComment(256, 'R2', DifferentialAction::ACTION_RESIGN,
-        array(DifferentialComment::METADATA_REMOVED_REVIEWERS => array('R2'))),
-      $this->newComment(512, 'R3', DifferentialAction::ACTION_ACCEPT),
-      $this->newComment(1024, 'A', DifferentialAction::ACTION_UPDATE),
-      // TODO: claim, abandon, reclaim
-    );
-
-    $stats = new DifferentialReviewerStats();
-    list($reviewed, $not_reviewed) = $stats->computeTimes($revision, $comments);
-
-    ksort($reviewed);
-    $this->assertEqual(
-      array(
-        'R1' => array(8 - 1),
-        'R2' => array(256 - 32),
-        'R3' => array(512 - 32),
-      ),
-      $reviewed);
-
-    ksort($not_reviewed);
-    $this->assertEqual(
-      array(
-        'R1' => array(512 - 32),
-        'R2' => array(8 - 1),
-        'R3' => array(8 - 4),
-      ),
-      $not_reviewed);
-  }
-
-  public function testReviewerStatsSince() {
-    $revision = new DifferentialRevision();
-    $revision->setDateCreated(1);
-    $revision->attachRelationships(array($this->newReviewer('R')));
-
-    $comments = array(
-      $this->newComment(2, 'R', DifferentialAction::ACTION_REJECT),
-      $this->newComment(4, 'A', DifferentialAction::ACTION_REQUEST),
-      $this->newComment(8, 'R', DifferentialAction::ACTION_ACCEPT),
-    );
-
-    $stats = new DifferentialReviewerStats();
-    $stats->setSince(4);
-    list($reviewed, $not_reviewed) = $stats->computeTimes($revision, $comments);
-
-    $this->assertEqual(array('R' => array(8 - 4)), $reviewed);
-    $this->assertEqual(array(), $not_reviewed);
-  }
-
-  public function testReviewerStatsRequiresReview() {
-    $revision = new DifferentialRevision();
-    $revision->setDateCreated(1);
-    $revision->attachRelationships(array($this->newReviewer('R')));
-
-    $comments = array();
-
-    $stats = new DifferentialReviewerStats();
-    $stats->setUntil(2);
-    list($reviewed, $not_reviewed) = $stats->computeTimes($revision, $comments);
-
-    $this->assertEqual(array(), $reviewed);
-    $this->assertEqual(array('R' => array(2 - 1)), $not_reviewed);
-  }
-
-  private function newReviewer($phid) {
-    return array(
-      'relation' => DifferentialRevision::RELATION_REVIEWER,
-      'objectPHID' => $phid,
-    );
-  }
-
-  private function newComment($date, $author, $action, $metadata = array()) {
-    return id(new DifferentialComment())
-      ->setDateCreated($date)
-      ->setAuthorPHID($author)
-      ->setAction($action)
-      ->setMetadata($metadata);
-  }
-
-}

src/applications/differential/storage/DifferentialRevision.php

@@ -153,9 +153,9 @@ final class DifferentialRevision extends DifferentialDAO
     if (!$this->getID()) {
       return array();
     }
-    return id(new DifferentialComment())->loadAllWhere(
+    return id(new DifferentialCommentQuery())
-      'revisionID = %d',
+      ->withRevisionIDs(array($this->getID()))
-      $this->getID());
+      ->execute();
   }
 
   public function loadActiveDiff() {
@@ -192,9 +192,9 @@ final class DifferentialRevision extends DifferentialDAO
         self::TABLE_COMMIT,
         $this->getID());
 
-      $comments = id(new DifferentialComment())->loadAllWhere(
+      $comments = id(new DifferentialCommentQuery())
-        'revisionID = %d',
+        ->withRevisionIDs(array($this->getID()))
-        $this->getID());
+        ->execute();
       foreach ($comments as $comment) {
         $comment->delete();
       }

src/applications/releeph/field/specification/ReleephDiffChurnFieldSpecification.php

@@ -19,9 +19,9 @@ final class ReleephDiffChurnFieldSpecification
     }
 
     $diff_rev = $this->getReleephRequest()->loadDifferentialRevision();
-    $comments = $diff_rev->loadRelatives(
+    $comments = id(new DifferentialRevisionQuery())
-      new DifferentialComment(),
+      ->withRevisionIDs(array($diff_rev->getID()))
-      'revisionID');
+      ->excute();
 
     $counts = array();
     foreach ($comments as $comment) {