Modernize DifferentialInlineCommentQuery

Summary:
Ref T7447. This class is currently a big mess with a lot of `withWeirdSpecialThingUsedInOnePlace()` type qualifiers.

Try to generalize/normalize it a bit.

Test Plan:
- Viewed inline comments.
- Created a new inline comment.
- Edited an inline comment.
- Marked an inline comment complete.
- Deleted, then undeleted an inline comment.
- Previewed inline comments.
- Viewed drafts as another user, verified they don't show up.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: yelirekim, epriestley

Maniphest Tasks: T7447

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

src/applications/differential/controller/DifferentialChangesetViewController.php

@@ -287,7 +287,8 @@ final class DifferentialChangesetViewController extends DifferentialController {
     }
 
     return id(new DifferentialInlineCommentQuery())
-      ->withViewerAndChangesetIDs($author_phid, $changeset_ids)
+      ->setViewer($this->getViewer())
+      ->withChangesetIDs($changeset_ids)
       ->execute();
   }
 

src/applications/differential/controller/DifferentialInlineCommentEditController.php

@@ -39,13 +39,17 @@ final class DifferentialInlineCommentEditController
 
   protected function loadComment($id) {
     return id(new DifferentialInlineCommentQuery())
+      ->setViewer($this->getViewer())
       ->withIDs(array($id))
+      ->withDeletedDrafts(true)
       ->executeOne();
   }
 
   protected function loadCommentByPHID($phid) {
     return id(new DifferentialInlineCommentQuery())
+      ->setViewer($this->getViewer())
       ->withPHIDs(array($phid))
+      ->withDeletedDrafts(true)
       ->executeOne();
   }
 

src/applications/differential/controller/DifferentialInlineCommentPreviewController.php

@@ -6,8 +6,19 @@ extends PhabricatorInlineCommentPreviewController {
   protected function loadInlineComments() {
     $viewer = $this->getViewer();
 
+    $revision = id(new DifferentialRevisionQuery())
+      ->setViewer($viewer)
+      ->withIDs(array($this->getRevisionID()))
+      ->executeOne();
+    if (!$revision) {
+      return array();
+    }
+
     return id(new DifferentialInlineCommentQuery())
-      ->withDraftComments($viewer->getPHID(), $this->getRevisionID())
+      ->setViewer($this->getViewer())
+      ->withDrafts(true)
+      ->withAuthorPHIDs(array($viewer->getPHID()))
+      ->withRevisionPHIDs(array($revision->getPHID()))
       ->execute();
   }
 

src/applications/differential/controller/DifferentialRevisionViewController.php

@@ -96,7 +96,7 @@ final class DifferentialRevisionViewController extends DifferentialController {
     $props = mpull($props, 'getData', 'getName');
 
     $all_changesets = $changesets;
-    $inlines = $revision->loadInlineComments($all_changesets);
+    $inlines = $revision->loadInlineComments($all_changesets, $user);
 
     $object_phids = array_merge(
       $revision->getReviewers(),
@@ -158,7 +158,10 @@ final class DifferentialRevisionViewController extends DifferentialController {
       $warning = $warning->render();
 
       $my_inlines = id(new DifferentialInlineCommentQuery())
-        ->withDraftComments($user->getPHID(), $this->revisionID)
+        ->setViewer($user)
+        ->withDrafts(true)
+        ->withAuthorPHIDs(array($user->getPHID()))
+        ->withRevisionPHIDs(array($revision->getPHID()))
         ->execute();
 
       $visible_changesets = array();

src/applications/differential/query/DifferentialInlineCommentQuery.php

@@ -6,24 +6,24 @@
 final class DifferentialInlineCommentQuery
   extends PhabricatorOffsetPagedQuery {
 
-  private $revisionIDs;
+  // TODO: Remove this when this query eventually moves to PolicyAware.
-  private $notDraft;
+  private $viewer;
+
   private $ids;
   private $phids;
-  private $commentIDs;
+  private $drafts;
+  private $authorPHIDs;
+  private $changesetIDs;
+  private $revisionPHIDs;
+  private $deletedDrafts;
 
-  private $viewerAndChangesetIDs;
+  public function setViewer(PhabricatorUser $viewer) {
-  private $draftComments;
+    $this->viewer = $viewer;
-  private $draftsByAuthors;
-
-  public function withRevisionIDs(array $ids) {
-    $this->revisionIDs = $ids;
     return $this;
   }
 
-  public function withNotDraft($not_draft) {
+  public function getViewer() {
-    $this->notDraft = $not_draft;
+    return $this->viewer;
-    return $this;
   }
 
   public function withIDs(array $ids) {
@@ -36,18 +36,28 @@ final class DifferentialInlineCommentQuery
     return $this;
   }
 
-  public function withViewerAndChangesetIDs($author_phid, array $ids) {
+  public function withDrafts($drafts) {
-    $this->viewerAndChangesetIDs = array($author_phid, $ids);
+    $this->drafts = $drafts;
     return $this;
   }
 
-  public function withDraftComments($author_phid, $revision_id) {
+  public function withAuthorPHIDs(array $author_phids) {
-    $this->draftComments = array($author_phid, $revision_id);
+    $this->authorPHIDs = $author_phids;
     return $this;
   }
 
-  public function withDraftsByAuthors(array $author_phids) {
+  public function withChangesetIDs(array $ids) {
-    $this->draftsByAuthors = $author_phids;
+    $this->changesetIDs = $ids;
+    return $this;
+  }
+
+  public function withRevisionPHIDs(array $revision_phids) {
+    $this->revisionPHIDs = $revision_phids;
+    return $this;
+  }
+
+  public function withDeletedDrafts($deleted_drafts) {
+    $this->deletedDrafts = $deleted_drafts;
     return $this;
   }
 
@@ -73,6 +83,7 @@ final class DifferentialInlineCommentQuery
   }
 
   public function executeOne() {
+    // TODO: Remove when this query moves to PolicyAware.
     return head($this->execute());
   }
 
@@ -84,33 +95,7 @@ final class DifferentialInlineCommentQuery
       $conn_r,
       'changesetID IS NOT NULL');
 
-    if ($this->revisionIDs) {
+    if ($this->ids !== null) {
-
-      // Look up revision PHIDs.
-      $revision_phids = queryfx_all(
-        $conn_r,
-        'SELECT phid FROM %T WHERE id IN (%Ld)',
-        id(new DifferentialRevision())->getTableName(),
-        $this->revisionIDs);
-
-      if (!$revision_phids) {
-        throw new PhabricatorEmptyQueryException();
-      }
-      $revision_phids = ipull($revision_phids, 'phid');
-
-      $where[] = qsprintf(
-        $conn_r,
-        'revisionPHID IN (%Ls)',
-        $revision_phids);
-    }
-
-    if ($this->notDraft) {
-      $where[] = qsprintf(
-        $conn_r,
-        'transactionPHID IS NOT NULL');
-    }
-
-    if ($this->ids) {
       $where[] = qsprintf(
         $conn_r,
         'id IN (%Ld)',
@@ -124,44 +109,42 @@ final class DifferentialInlineCommentQuery
         $this->phids);
     }
 
-    if ($this->viewerAndChangesetIDs) {
+    if ($this->revisionPHIDs !== null) {
-      list($phid, $ids) = $this->viewerAndChangesetIDs;
       $where[] = qsprintf(
         $conn_r,
-        'changesetID IN (%Ld) AND
+        'revisionPHID IN (%Ls)',
-          ((authorPHID = %s AND isDeleted = 0) OR transactionPHID IS NOT NULL)',
+        $this->revisionPHIDs);
-        $ids,
-        $phid);
     }
 
-    if ($this->draftComments) {
+    if ($this->changesetIDs !== null) {
-      list($phid, $rev_id) = $this->draftComments;
-
-      $rev_phid = queryfx_one(
-        $conn_r,
-        'SELECT phid FROM %T WHERE id = %d',
-        id(new DifferentialRevision())->getTableName(),
-        $rev_id);
-
-      if (!$rev_phid) {
-        throw new PhabricatorEmptyQueryException();
-      }
-
-      $rev_phid = $rev_phid['phid'];
-
       $where[] = qsprintf(
         $conn_r,
-        'authorPHID = %s AND revisionPHID = %s AND transactionPHID IS NULL
+        'changesetID IN (%Ld)',
-          AND isDeleted = 0',
+        $this->changesetIDs);
-        $phid,
-        $rev_phid);
     }
 
-    if ($this->draftsByAuthors) {
+    if ($this->drafts === null) {
+      if ($this->deletedDrafts) {
         $where[] = qsprintf(
           $conn_r,
-        'authorPHID IN (%Ls) AND isDeleted = 0 AND transactionPHID IS NULL',
+          '(authorPHID = %s) OR (transactionPHID IS NOT NULL)',
-        $this->draftsByAuthors);
+          $this->getViewer()->getPHID());
+      } else {
+        $where[] = qsprintf(
+          $conn_r,
+          '(authorPHID = %s AND isDeleted = 0)
+            OR (transactionPHID IS NOT NULL)',
+          $this->getViewer()->getPHID());
+      }
+    } else if ($this->drafts) {
+      $where[] = qsprintf(
+        $conn_r,
+        '(authorPHID = %s AND isDeleted = 0) AND (transactionPHID IS NULL)',
+        $this->getViewer()->getPHID());
+    } else {
+      $where[] = qsprintf(
+        $conn_r,
+        'transactionPHID IS NOT NULL');
     }
 
     return $this->formatWhereClause($where);

src/applications/differential/storage/DifferentialRevision.php

@@ -281,14 +281,16 @@ final class DifferentialRevision extends DifferentialDAO
   }
 
   public function loadInlineComments(
-    array &$changesets) {
+    array &$changesets,
+    PhabricatorUser $viewer) {
     assert_instances_of($changesets, 'DifferentialChangeset');
 
     $inline_comments = array();
 
     $inline_comments = id(new DifferentialInlineCommentQuery())
-      ->withRevisionIDs(array($this->getID()))
+      ->setViewer($viewer)
-      ->withNotDraft(true)
+      ->withDrafts(false)
+      ->withRevisionPHIDs(array($this->getPHID()))
       ->execute();
 
     $load_changesets = array();
@@ -535,7 +537,7 @@ final class DifferentialRevision extends DifferentialDAO
       ->execute();
     // NOTE: this mutates $changesets to include changesets for all inline
     // comments...!
-    $inlines = $this->loadInlineComments($changesets);
+    $inlines = $this->loadInlineComments($changesets, $request->getViewer());
     $changesets = mpull($changesets, null, 'getID');
 
     return $timeline
@@ -569,18 +571,6 @@ final class DifferentialRevision extends DifferentialDAO
         self::TABLE_COMMIT,
         $this->getID());
 
-      try {
-        $inlines = id(new DifferentialInlineCommentQuery())
-          ->withRevisionIDs(array($this->getID()))
-          ->execute();
-        foreach ($inlines as $inline) {
-          $inline->delete();
-        }
-      } catch (PhabricatorEmptyQueryException $ex) {
-        // TODO: There's still some funky legacy wrapping going on here, and
-        // we might catch a raw query exception.
-      }
-
       // we have to do paths a little differentally as they do not have
       // an id or phid column for delete() to act on
       $dummy_path = new DifferentialAffectedPath();