Replace "loadUnsubmittedInlineComments()" with a modern "DiffQuery"

Summary: Ref T13513. All queries now go through a reasonably minimal set of pathways and should have consistent behavior.

Test Plan:
- Loaded a revision with inlines.
- Created a new empty inline, reloaded page, saw it vanish.
- Created a new empty inline, typed draft text, did not save, reloaded page, saw draft present.
- Created a new empty inline, typed draft text. Submitted feedback, got prompt, answered "Y", saw draft text submit.
- Created a new empty inline, typed draft text, scrolled down to bottom of page, typed non-draft text, saw preview include draft text.
- Marked and submitted "Done".
- Used hide/show on inlines, verified state persisted.
- Did much of the same stuff in Diffusion, where it all works the same way (except: there's no prompt when submitting draft is-editing inlines).

Maniphest Tasks: T13513

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

Author: epriestley

src/applications/differential/editor/DifferentialRevisionEditEngine.php

@@ -302,24 +302,14 @@ private function isCustomFieldEnabled(DifferentialRevision $revision, $key) {
 
   protected function newAutomaticCommentTransactions($object) {
     $viewer = $this->getViewer();
-    $xactions = array();
-
-    $inlines = DifferentialTransactionQuery::loadUnsubmittedInlineComments(
-      $viewer,
-      $object);
-    $inlines = msort($inlines, 'getID');
 
     $editor = $object->getApplicationTransactionEditor()
       ->setActor($viewer);
 
-    $query_template = id(new DifferentialDiffInlineCommentQuery())
-      ->withRevisionPHIDs(array($object->getPHID()));
-
     $xactions = $editor->newAutomaticInlineTransactions(
       $object,
-      $inlines,
       DifferentialTransaction::TYPE_INLINE,
-      $query_template);
+      new DifferentialDiffInlineCommentQuery());
 
     return $xactions;
   }

src/applications/differential/engine/DifferentialRevisionDraftEngine.php

@@ -7,9 +7,11 @@ protected function hasCustomDraftContent() {
     $viewer = $this->getViewer();
     $revision = $this->getObject();
 
-    $inlines = DifferentialTransactionQuery::loadUnsubmittedInlineComments(
-      $viewer,
-      $revision);
+    $inlines = id(new DifferentialDiffInlineCommentQuery())
+      ->setViewer($viewer)
+      ->withRevisionPHIDs(array($revision->getPHID()))
+      ->withPublishableComments(true)
+      ->execute();
 
     return (bool)$inlines;
   }

src/applications/differential/query/DifferentialTransactionQuery.php

@@ -7,49 +7,4 @@ public function getTemplateApplicationTransaction() {
     return new DifferentialTransaction();
   }
 
-  public static function loadUnsubmittedInlineComments(
-    PhabricatorUser $viewer,
-    DifferentialRevision $revision) {
-
-    $inlines = id(new DifferentialDiffInlineCommentQuery())
-      ->setViewer($viewer)
-      ->withRevisionPHIDs(array($revision->getPHID()))
-      ->withAuthorPHIDs(array($viewer->getPHID()))
-      ->withHasTransaction(false)
-      ->withIsDeleted(false)
-      ->needReplyToComments(true)
-      ->execute();
-
-    foreach ($inlines as $key => $inline) {
-      $inlines[$key] = DifferentialInlineComment::newFromModernComment(
-        $inline);
-    }
-
-    PhabricatorInlineComment::loadAndAttachVersionedDrafts(
-      $viewer,
-      $inlines);
-
-    // Don't count void inlines when considering draft state.
-    foreach ($inlines as $key => $inline) {
-      if ($inline->isVoidComment($viewer)) {
-        unset($inlines[$key]);
-        continue;
-      }
-
-      // For other inlines: if they have a nonempty draft state, set their
-      // content to the draft state content. We want to submit the comment
-      // as it is currently shown to the user, not as it was stored the last
-      // time they clicked "Save".
-
-      $draft_content = $inline->getContentForEdit($viewer);
-      if (strlen($draft_content)) {
-        $inline->setContent($draft_content);
-      }
-    }
-
-    $inlines = mpull($inlines, 'getStorageObject');
-
-    return $inlines;
-  }
-
 }

src/applications/diffusion/editor/DiffusionCommitEditEngine.php

@@ -126,27 +126,14 @@ protected function buildCustomEditFields($object) {
 
   protected function newAutomaticCommentTransactions($object) {
     $viewer = $this->getViewer();
-    $xactions = array();
-
-    $inlines = id(new DiffusionDiffInlineCommentQuery())
-      ->setViewer($viewer)
-      ->withObjectPHIDs(array($object->getPHID()))
-      ->withPublishableComments(true)
-      ->needReplyToComments(true)
-      ->execute();
-    $inlines = msort($inlines, 'getID');
 
     $editor = $object->getApplicationTransactionEditor()
       ->setActor($viewer);
 
-    $query_template = id(new DiffusionDiffInlineCommentQuery())
-      ->withCommitPHIDs(array($object->getPHID()));
-
     $xactions = $editor->newAutomaticInlineTransactions(
       $object,
-      $inlines,
       PhabricatorAuditActionConstants::INLINE,
-      $query_template);
+      new DiffusionDiffInlineCommentQuery());
 
     return $xactions;
   }

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

@@ -5027,18 +5027,23 @@ private function buildHistoryMail(PhabricatorLiskDAO $object) {
 
   public function newAutomaticInlineTransactions(
     PhabricatorLiskDAO $object,
-    array $inlines,
     $transaction_type,
     PhabricatorCursorPagedPolicyAwareQuery $query_template) {
 
+    $actor = $this->getActor();
+
+    $inlines = id(clone $query_template)
+      ->setViewer($actor)
+      ->withObjectPHIDs(array($object->getPHID()))
+      ->withPublishableComments(true)
+      ->needAppliedDrafts(true)
+      ->needReplyToComments(true)
+      ->execute();
+    $inlines = msort($inlines, 'getID');
+
     $xactions = array();
 
     foreach ($inlines as $key => $inline) {
-      if ($inline->isEmptyInlineComment()) {
-        unset($inlines[$key]);
-        continue;
-      }
-
       $xactions[] = $object->getApplicationTransactionTemplate()
         ->setTransactionType($transaction_type)
         ->attachComment($inline);
@@ -5065,31 +5070,17 @@ protected function newInlineStateTransaction(
 
     $state_map = PhabricatorTransactions::getInlineStateMap();
 
-    $query = id(clone $query_template)
+    $inline_query = id(clone $query_template)
       ->setViewer($this->getActor())
-      ->withFixedStates(array_keys($state_map));
-
-    $inlines = array();
-
-    $inlines[] = id(clone $query)
-      ->withAuthorPHIDs(array($actor_phid))
-      ->withHasTransaction(false)
-      ->execute();
+      ->withObjectPHIDs(array($object->getPHID()))
+      ->withFixedStates(array_keys($state_map))
+      ->withPublishableComments(true);
 
     if ($actor_is_author) {
-      $inlines[] = id(clone $query)
-        ->withHasTransaction(true)
-        ->execute();
+      $inline_query->withPublishedComments(true);
     }
 
-    $inlines = array_mergev($inlines);
-
-    foreach ($inlines as $key => $inline) {
-      if ($inline->isEmptyInlineComment()) {
-        unset($inlines[$key]);
-        continue;
-      }
-    }
+    $inlines = $inline_query->execute();
 
     if (!$inlines) {
       return null;

src/infrastructure/diff/query/PhabricatorDiffInlineCommentQuery.php

@@ -8,6 +8,7 @@ abstract class PhabricatorDiffInlineCommentQuery
   private $publishedComments;
   private $publishableComments;
   private $needHidden;
+  private $needAppliedDrafts;
 
   abstract protected function buildInlineCommentWhereClauseParts(
     AphrontDatabaseConnection $conn);
@@ -41,6 +42,11 @@ final public function needHidden($need_hidden) {
     return $this;
   }
 
+  final public function needAppliedDrafts($need_applied) {
+    $this->needAppliedDrafts = $need_applied;
+    return $this;
+  }
+
   protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
     $where = parent::buildWhereClauseParts($conn);
     $alias = $this->getPrimaryTableAlias();
@@ -124,65 +130,121 @@ protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) {
     return $where;
   }
 
-  protected function willFilterPage(array $comments) {
+  protected function willFilterPage(array $inlines) {
+    $viewer = $this->getViewer();
+
     if ($this->needReplyToComments) {
       $reply_phids = array();
-      foreach ($comments as $comment) {
-        $reply_phid = $comment->getReplyToCommentPHID();
+      foreach ($inlines as $inline) {
+        $reply_phid = $inline->getReplyToCommentPHID();
         if ($reply_phid) {
           $reply_phids[] = $reply_phid;
         }
       }
 
       if ($reply_phids) {
-        $reply_comments = newv(get_class($this), array())
+        $reply_inlines = newv(get_class($this), array())
           ->setViewer($this->getViewer())
           ->setParentQuery($this)
           ->withPHIDs($reply_phids)
           ->execute();
-        $reply_comments = mpull($reply_comments, null, 'getPHID');
+        $reply_inlines = mpull($reply_inlines, null, 'getPHID');
       } else {
-        $reply_comments = array();
+        $reply_inlines = array();
       }
 
-      foreach ($comments as $key => $comment) {
-        $reply_phid = $comment->getReplyToCommentPHID();
+      foreach ($inlines as $key => $inline) {
+        $reply_phid = $inline->getReplyToCommentPHID();
         if (!$reply_phid) {
-          $comment->attachReplyToComment(null);
+          $inline->attachReplyToComment(null);
           continue;
         }
-        $reply = idx($reply_comments, $reply_phid);
+        $reply = idx($reply_inlines, $reply_phid);
         if (!$reply) {
-          $this->didRejectResult($comment);
-          unset($comments[$key]);
+          $this->didRejectResult($inline);
+          unset($inlines[$key]);
           continue;
         }
-        $comment->attachReplyToComment($reply);
+        $inline->attachReplyToComment($reply);
       }
     }
 
-    if (!$comments) {
-      return $comments;
+    if (!$inlines) {
+      return $inlines;
     }
 
     if ($this->needHidden) {
-      $viewer = $this->getViewer();
       $viewer_phid = $viewer->getPHID();
 
       if ($viewer_phid) {
         $hidden = $this->loadHiddenCommentIDs(
           $viewer_phid,
-          $comments);
+          $inlines);
       } else {
         $hidden = array();
       }
 
-      foreach ($comments as $inline) {
+      foreach ($inlines as $inline) {
         $inline->attachIsHidden(isset($hidden[$inline->getID()]));
       }
     }
 
-    return $comments;
+    if (!$inlines) {
+      return $inlines;
+    }
+
+    $need_drafts = $this->needAppliedDrafts;
+    $drop_void = $this->publishableComments;
+    $convert_objects = ($need_drafts || $drop_void);
+
+    if ($convert_objects) {
+      $inlines = mpull($inlines, 'newInlineCommentObject');
+
+      PhabricatorInlineComment::loadAndAttachVersionedDrafts(
+        $viewer,
+        $inlines);
+
+      if ($need_drafts) {
+        // Don't count void inlines when considering draft state.
+        foreach ($inlines as $key => $inline) {
+          if ($inline->isVoidComment($viewer)) {
+            $this->didRejectResult($inline->getStorageObject());
+            unset($inlines[$key]);
+            continue;
+          }
+
+          // For other inlines: if they have a nonempty draft state, set their
+          // content to the draft state content. We want to submit the comment
+          // as it is currently shown to the user, not as it was stored the last
+          // time they clicked "Save".
+
+          $draft_content = $inline->getContentForEdit($viewer);
+          if (strlen($draft_content)) {
+            $inline->setContent($draft_content);
+          }
+        }
+      }
+
+      // If we're loading publishable comments, discard any comments that are
+      // empty.
+      if ($drop_void) {
+        foreach ($inlines as $key => $inline) {
+          if ($inline->getTransactionPHID()) {
+            continue;
+          }
+
+          if ($inline->isVoidComment($viewer)) {
+            $this->didRejectResult($inline->getStorageObject());
+            unset($inlines[$key]);
+            continue;
+          }
+        }
+      }
+
+      $inlines = mpull($inlines, 'getStorageObject');
+    }
+
+    return $inlines;
   }
 
 }