Add inline comment support to "Pro" comment save controller

Summary: Ref T2222. Makes the "pro" controller work with inlines.

Test Plan: Added a bunch of inlines and saved them with the "pro" controller.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

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

src/applications/differential/controller/DifferentialCommentSaveControllerPro.php

@@ -86,8 +86,30 @@ final class DifferentialCommentSaveControllerPro
         ->setNewValue(array('+' => $reviewer_edges));
     }
 
+    $inline_phids = $this->loadUnsubmittedInlinePHIDs($revision);
+    if ($inline_phids) {
+      $inlines = id(new PhabricatorApplicationTransactionCommentQuery())
+        ->setTemplate(new DifferentialTransactionComment())
+        ->setViewer($viewer)
+        ->withPHIDs($inline_phids)
+        ->execute();
+    } else {
+      $inlines = null;
+    }
+
+    foreach ($inlines as $inline) {
+      $xactions[] = id(new DifferentialTransaction())
+        ->setTransactionType($type_inline)
+        ->attachComment($inline);
+    }
+
+    // NOTE: If there are no other transactions, add an empty comment
+    // transaction so that we'll raise a more user-friendly error message,
+    // to the effect of "you can not post an empty comment".
+    $no_xactions = !$xactions;
+
     $comment = $request->getStr('comment');
-    if (strlen($comment)) {
+    if (strlen($comment) || $no_xactions) {
       $xactions[] = id(new DifferentialTransaction())
         ->setTransactionType($type_comment)
         ->attachComment(
@@ -96,7 +118,6 @@ final class DifferentialCommentSaveControllerPro
             ->setContent($comment));
     }
 
-    // TODO: Inlines!
 
     $editor = id(new DifferentialTransactionEditor())
       ->setActor($viewer)
@@ -114,8 +135,6 @@ final class DifferentialCommentSaveControllerPro
         ->setException($ex);
     }
 
-    // TODO: Diff change detection?
-
     $user = $request->getUser();
     $draft = id(new PhabricatorDraft())->loadOneWhere(
       'authorPHID = %s AND draftKey = %s',
@@ -130,4 +149,30 @@ final class DifferentialCommentSaveControllerPro
       ->setURI('/D'.$revision->getID());
   }
 
+
+  private function loadUnsubmittedInlinePHIDs(DifferentialRevision $revision) {
+    $viewer = $this->getRequest()->getUser();
+
+    // TODO: This probably needs to move somewhere more central as we move
+    // away from DifferentialInlineCommentQuery, but
+    // PhabricatorApplicationTransactionCommentQuery is currently `final` and
+    // I'm not yet decided on how to approach that. For now, just get the PHIDs
+    // and then execute a PHID-based query through the standard stack.
+
+    $table = new DifferentialTransactionComment();
+    $conn_r = $table->establishConnection('r');
+
+    $phids = queryfx_all(
+      $conn_r,
+      'SELECT phid FROM %T
+        WHERE revisionPHID = %s
+          AND authorPHID = %s
+          AND transactionPHID IS NULL',
+      $table->getTableName(),
+      $revision->getPHID(),
+      $viewer->getPHID());
+
+    return ipull($phids, 'phid');
+  }
+
 }

src/applications/differential/editor/DifferentialTransactionEditor.php

@@ -12,10 +12,10 @@ final class DifferentialTransactionEditor
     $types[] = PhabricatorTransactions::TYPE_EDIT_POLICY;
 
     $types[] = DifferentialTransaction::TYPE_ACTION;
+    $types[] = DifferentialTransaction::TYPE_INLINE;
 
 /*
 
-    $types[] = DifferentialTransaction::TYPE_INLINE;
     $types[] = DifferentialTransaction::TYPE_UPDATE;
 */
 
@@ -33,6 +33,8 @@ final class DifferentialTransactionEditor
         return $object->getEditPolicy();
       case DifferentialTransaction::TYPE_ACTION:
         return null;
+      case DifferentialTransaction::TYPE_INLINE:
+        return null;
     }
 
     return parent::getCustomTransactionOldValue($object, $xaction);
@@ -47,6 +49,8 @@ final class DifferentialTransactionEditor
       case PhabricatorTransactions::TYPE_EDIT_POLICY:
       case DifferentialTransaction::TYPE_ACTION:
         return $xaction->getNewValue();
+      case DifferentialTransaction::TYPE_INLINE:
+        return null;
     }
 
     return parent::getCustomTransactionNewValue($object, $xaction);
@@ -57,6 +61,8 @@ final class DifferentialTransactionEditor
     PhabricatorApplicationTransaction $xaction) {
 
     switch ($xaction->getTransactionType()) {
+      case DifferentialTransaction::TYPE_INLINE:
+        return $xaction->hasComment();
     }
 
     return parent::transactionHasEffect($object, $xaction);
@@ -76,6 +82,8 @@ final class DifferentialTransactionEditor
         return;
       case PhabricatorTransactions::TYPE_SUBSCRIBERS:
       case PhabricatorTransactions::TYPE_COMMENT:
+      case DifferentialTransaction::TYPE_INLINE:
+        return;
       case PhabricatorTransactions::TYPE_EDGE:
         // TODO: When removing reviewers, we may be able to move the revision
         // to "Accepted".
@@ -101,6 +109,7 @@ final class DifferentialTransactionEditor
       case PhabricatorTransactions::TYPE_EDGE:
       case PhabricatorTransactions::TYPE_COMMENT:
       case DifferentialTransaction::TYPE_ACTION:
+      case DifferentialTransaction::TYPE_INLINE:
         return;
     }
 
@@ -120,6 +129,23 @@ final class DifferentialTransactionEditor
     return $errors;
   }
 
+  protected function sortTransactions(array $xactions) {
+    $head = array();
+    $tail = array();
+
+    // Move bare comments to the end, so the actions precede them.
+    foreach ($xactions as $xaction) {
+      $type = $xaction->getTransactionType();
+      if ($type == DifferentialTransaction::TYPE_INLINE) {
+        $tail[] = $xaction;
+      } else {
+        $head[] = $xaction;
+      }
+    }
+
+    return array_values(array_merge($head, $tail));
+  }
+
   protected function requireCapabilities(
     PhabricatorLiskDAO $object,
     PhabricatorApplicationTransaction $xaction) {

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

@@ -1335,12 +1335,7 @@ abstract class PhabricatorApplicationTransactionEditor
     PhabricatorLiskDAO $object,
     PhabricatorApplicationTransaction $xaction) {
 
-    switch ($xaction->getTransactionType()) {
+    return $xaction->isCommentTransaction();
-      case PhabricatorTransactions::TYPE_COMMENT:
-        return true;
-      default:
-        return false;
-    }
   }
 
 

src/applications/transactions/storage/PhabricatorApplicationTransaction.php

@@ -528,6 +528,19 @@ abstract class PhabricatorApplicationTransaction
     return 1.0;
   }
 
+  public function isCommentTransaction() {
+    if ($this->hasComment()) {
+      return true;
+    }
+
+    switch ($this->getTransactionType()) {
+      case PhabricatorTransactions::TYPE_COMMENT:
+        return true;
+    }
+
+    return false;
+  }
+
   public function getActionName() {
     switch ($this->getTransactionType()) {
       case PhabricatorTransactions::TYPE_COMMENT:
@@ -605,8 +618,6 @@ abstract class PhabricatorApplicationTransaction
    * @return bool True to display in a group with the other transactions.
    */
   public function shouldDisplayGroupWith(array $group) {
-    $type_comment = PhabricatorTransactions::TYPE_COMMENT;
-
     $this_source = null;
     if ($this->getContentSource()) {
       $this_source = $this->getContentSource()->getSource();
@@ -624,7 +635,7 @@ abstract class PhabricatorApplicationTransaction
       }
 
       // Don't group anything into a group which already has a comment.
-      if ($xaction->getTransactionType() == $type_comment) {
+      if ($xaction->isCommentTransaction()) {
         return false;
       }