From 46feccdfcf1963d7d8d38b2c282b1e702f059a76 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 10 Dec 2018 08:20:22 -0800 Subject: [PATCH] Share more inline "Done" code between Differential and Diffusion Summary: Ref T13222. See PHI995. Before making a change to inline rendering, consolidate this code for generating the "alice added inlines comments." and "alice marked X inlines as done." transactions. Both Differential and Diffusion have four very similar chunks of code. Merge them into shared methods and reduce code duplication across the methods. (In the next change, I plan to hide the "done" story when the mark affects your own inline, since users marking their own inlines as "done" is generally not very interesting or useful.) Test Plan: As author and reviewer/auditor, added inlines in Differential and Diffusion. As author, marked own and others inlines as done and undone. Got sensible transaction rendering and persistence of "Done". Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13222 Differential Revision: https://secure.phabricator.com/D19858 --- .../audit/editor/PhabricatorAuditEditor.php | 45 ++---------- .../editor/DifferentialRevisionEditEngine.php | 37 +++------- .../editor/DifferentialTransactionEditor.php | 48 ++----------- .../editor/DiffusionCommitEditEngine.php | 37 +++------- ...habricatorApplicationTransactionEditor.php | 71 +++++++++++++++++++ 5 files changed, 103 insertions(+), 135 deletions(-) diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php index 3f65cc80f8..165f8ef84f 100644 --- a/src/applications/audit/editor/PhabricatorAuditEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditEditor.php @@ -251,47 +251,16 @@ final class PhabricatorAuditEditor case PhabricatorTransactions::TYPE_COMMENT: $this->didExpandInlineState = true; - $actor_phid = $this->getActingAsPHID(); - $author_phid = $object->getAuthorPHID(); - $actor_is_author = ($actor_phid == $author_phid); + $query_template = id(new DiffusionDiffInlineCommentQuery()) + ->withCommitPHIDs(array($object->getPHID())); - $state_map = PhabricatorTransactions::getInlineStateMap(); + $state_xaction = $this->newInlineStateTransaction( + $object, + $query_template); - $query = id(new DiffusionDiffInlineCommentQuery()) - ->setViewer($this->getActor()) - ->withCommitPHIDs(array($object->getPHID())) - ->withFixedStates(array_keys($state_map)); - - $inlines = array(); - - $inlines[] = id(clone $query) - ->withAuthorPHIDs(array($actor_phid)) - ->withHasTransaction(false) - ->execute(); - - if ($actor_is_author) { - $inlines[] = id(clone $query) - ->withHasTransaction(true) - ->execute(); + if ($state_xaction) { + $xactions[] = $state_xaction; } - - $inlines = array_mergev($inlines); - - if (!$inlines) { - break; - } - - $old_value = mpull($inlines, 'getFixedState', 'getPHID'); - $new_value = array(); - foreach ($old_value as $key => $state) { - $new_value[$key] = $state_map[$state]; - } - - $xactions[] = id(new PhabricatorAuditTransaction()) - ->setTransactionType(PhabricatorTransactions::TYPE_INLINESTATE) - ->setIgnoreOnNoEffect(true) - ->setOldValue($old_value) - ->setNewValue($new_value); break; } } diff --git a/src/applications/differential/editor/DifferentialRevisionEditEngine.php b/src/applications/differential/editor/DifferentialRevisionEditEngine.php index ffae7fab16..07b693044f 100644 --- a/src/applications/differential/editor/DifferentialRevisionEditEngine.php +++ b/src/applications/differential/editor/DifferentialRevisionEditEngine.php @@ -279,36 +279,17 @@ final class DifferentialRevisionEditEngine $object); $inlines = msort($inlines, 'getID'); - foreach ($inlines as $inline) { - $xactions[] = id(new DifferentialTransaction()) - ->setTransactionType(DifferentialTransaction::TYPE_INLINE) - ->attachComment($inline); - } + $editor = $object->getApplicationTransactionEditor() + ->setActor($viewer); - $viewer_phid = $viewer->getPHID(); - $viewer_is_author = ($object->getAuthorPHID() == $viewer_phid); - if ($viewer_is_author) { - $state_map = PhabricatorTransactions::getInlineStateMap(); + $query_template = id(new DifferentialDiffInlineCommentQuery()) + ->withRevisionPHIDs(array($object->getPHID())); - $inlines = id(new DifferentialDiffInlineCommentQuery()) - ->setViewer($viewer) - ->withRevisionPHIDs(array($object->getPHID())) - ->withFixedStates(array_keys($state_map)) - ->execute(); - if ($inlines) { - $old_value = mpull($inlines, 'getFixedState', 'getPHID'); - $new_value = array(); - foreach ($old_value as $key => $state) { - $new_value[$key] = $state_map[$state]; - } - - $xactions[] = id(new DifferentialTransaction()) - ->setTransactionType(PhabricatorTransactions::TYPE_INLINESTATE) - ->setIgnoreOnNoEffect(true) - ->setOldValue($old_value) - ->setNewValue($new_value); - } - } + $xactions = $editor->newAutomaticInlineTransactions( + $object, + $inlines, + DifferentialTransaction::TYPE_INLINE, + $query_template); return $xactions; } diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 853b024e27..8072c19e9a 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -247,50 +247,16 @@ final class DifferentialTransactionEditor case DifferentialTransaction::TYPE_INLINE: $this->didExpandInlineState = true; - $actor_phid = $this->getActingAsPHID(); - $author_phid = $object->getAuthorPHID(); - $actor_is_author = ($actor_phid == $author_phid); + $query_template = id(new DifferentialDiffInlineCommentQuery()) + ->withRevisionPHIDs(array($object->getPHID())); - $state_map = PhabricatorTransactions::getInlineStateMap(); + $state_xaction = $this->newInlineStateTransaction( + $object, + $query_template); - $query = id(new DifferentialDiffInlineCommentQuery()) - ->setViewer($this->getActor()) - ->withRevisionPHIDs(array($object->getPHID())) - ->withFixedStates(array_keys($state_map)); - - $inlines = array(); - - // We're going to undraft any "done" marks on your own inlines. - $inlines[] = id(clone $query) - ->withAuthorPHIDs(array($actor_phid)) - ->withHasTransaction(false) - ->execute(); - - // If you're the author, we also undraft any "done" marks on other - // inlines. - if ($actor_is_author) { - $inlines[] = id(clone $query) - ->withHasTransaction(true) - ->execute(); + if ($state_xaction) { + $results[] = $state_xaction; } - - $inlines = array_mergev($inlines); - - if (!$inlines) { - break; - } - - $old_value = mpull($inlines, 'getFixedState', 'getPHID'); - $new_value = array(); - foreach ($old_value as $key => $state) { - $new_value[$key] = $state_map[$state]; - } - - $results[] = id(new DifferentialTransaction()) - ->setTransactionType(PhabricatorTransactions::TYPE_INLINESTATE) - ->setIgnoreOnNoEffect(true) - ->setOldValue($old_value) - ->setNewValue($new_value); break; } } diff --git a/src/applications/diffusion/editor/DiffusionCommitEditEngine.php b/src/applications/diffusion/editor/DiffusionCommitEditEngine.php index 8392504def..667af46550 100644 --- a/src/applications/diffusion/editor/DiffusionCommitEditEngine.php +++ b/src/applications/diffusion/editor/DiffusionCommitEditEngine.php @@ -170,36 +170,17 @@ final class DiffusionCommitEditEngine $raw = true); $inlines = msort($inlines, 'getID'); - foreach ($inlines as $inline) { - $xactions[] = $object->getApplicationTransactionTemplate() - ->setTransactionType(PhabricatorAuditActionConstants::INLINE) - ->attachComment($inline); - } + $editor = $object->getApplicationTransactionEditor() + ->setActor($viewer); - $viewer_phid = $viewer->getPHID(); - $viewer_is_author = ($object->getAuthorPHID() == $viewer_phid); - if ($viewer_is_author) { - $state_map = PhabricatorTransactions::getInlineStateMap(); + $query_template = id(new DiffusionDiffInlineCommentQuery()) + ->withCommitPHIDs(array($object->getPHID())); - $inlines = id(new DiffusionDiffInlineCommentQuery()) - ->setViewer($viewer) - ->withCommitPHIDs(array($object->getPHID())) - ->withFixedStates(array_keys($state_map)) - ->execute(); - if ($inlines) { - $old_value = mpull($inlines, 'getFixedState', 'getPHID'); - $new_value = array(); - foreach ($old_value as $key => $state) { - $new_value[$key] = $state_map[$state]; - } - - $xactions[] = $object->getApplicationTransactionTemplate() - ->setTransactionType(PhabricatorTransactions::TYPE_INLINESTATE) - ->setIgnoreOnNoEffect(true) - ->setOldValue($old_value) - ->setNewValue($new_value); - } - } + $xactions = $editor->newAutomaticInlineTransactions( + $object, + $inlines, + PhabricatorAuditActionConstants::INLINE, + $query_template); return $xactions; } diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index f354de6a1a..634b4b92db 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -4594,4 +4594,75 @@ abstract class PhabricatorApplicationTransactionEditor return $mail; } + public function newAutomaticInlineTransactions( + PhabricatorLiskDAO $object, + array $inlines, + $transaction_type, + PhabricatorCursorPagedPolicyAwareQuery $query_template) { + + $xactions = array(); + + foreach ($inlines as $inline) { + $xactions[] = $object->getApplicationTransactionTemplate() + ->setTransactionType($transaction_type) + ->attachComment($inline); + } + + $state_xaction = $this->newInlineStateTransaction( + $object, + $query_template); + + if ($state_xaction) { + $xactions[] = $state_xaction; + } + + return $xactions; + } + + protected function newInlineStateTransaction( + PhabricatorLiskDAO $object, + PhabricatorCursorPagedPolicyAwareQuery $query_template) { + + $actor_phid = $this->getActingAsPHID(); + $author_phid = $object->getAuthorPHID(); + $actor_is_author = ($actor_phid == $author_phid); + + $state_map = PhabricatorTransactions::getInlineStateMap(); + + $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(); + + if ($actor_is_author) { + $inlines[] = id(clone $query) + ->withHasTransaction(true) + ->execute(); + } + + $inlines = array_mergev($inlines); + + if (!$inlines) { + return null; + } + + $old_value = mpull($inlines, 'getFixedState', 'getPHID'); + $new_value = array(); + foreach ($old_value as $key => $state) { + $new_value[$key] = $state_map[$state]; + } + + return $object->getApplicationTransactionTemplate() + ->setTransactionType(PhabricatorTransactions::TYPE_INLINESTATE) + ->setIgnoreOnNoEffect(true) + ->setOldValue($old_value) + ->setNewValue($new_value); + } + }