From d48a88d4cd789a9b9ece321056f185d99063f4ad Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 24 Feb 2014 15:57:35 -0800 Subject: [PATCH] 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 --- .../DifferentialCommentSaveControllerPro.php | 53 +++++++++++++++++-- .../editor/DifferentialTransactionEditor.php | 28 +++++++++- ...habricatorApplicationTransactionEditor.php | 7 +-- .../PhabricatorApplicationTransaction.php | 17 ++++-- 4 files changed, 91 insertions(+), 14 deletions(-) diff --git a/src/applications/differential/controller/DifferentialCommentSaveControllerPro.php b/src/applications/differential/controller/DifferentialCommentSaveControllerPro.php index 0e00cde75c..8128372187 100644 --- a/src/applications/differential/controller/DifferentialCommentSaveControllerPro.php +++ b/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'); + } + } diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 214c628571..ca7c55bde6 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/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) { diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index c41fbacf62..633fbccccd 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -1335,12 +1335,7 @@ abstract class PhabricatorApplicationTransactionEditor PhabricatorLiskDAO $object, PhabricatorApplicationTransaction $xaction) { - switch ($xaction->getTransactionType()) { - case PhabricatorTransactions::TYPE_COMMENT: - return true; - default: - return false; - } + return $xaction->isCommentTransaction(); } diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php index 78974e1951..0aa3df75a2 100644 --- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php +++ b/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; }