From 65634781b44fb6a5e182c896705d44e04490ba76 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 9 Jun 2016 05:25:14 -0700 Subject: [PATCH] Don't re-mention users for comment edits Summary: Ref T11035. This only fixes half of the issue: comment editing has been fixed, but normal transactions which edit things like descriptions haven't yet. The normal edits aren't fixed because the "oldValues" are populated too late. The code should start working once they get populated sooner, but I don't want to jump the gun on that since it'll probably have some spooky effects. I have some other transaction changes coming down the pipe which should provide a better context for testing "oldValue" population order. Test Plan: - Mentioned `@dog` in a comment. - Removed `@dog` as a subscriber. - Edited the comment, adding some unrelated text at the end (e.g., fixing a typo). - Before change: `@dog` re-added as subscriber. - After change: no re-add. Reviewers: chad Reviewed By: chad Maniphest Tasks: T11035 Differential Revision: https://secure.phabricator.com/D16108 --- src/__phutil_library_map__.php | 4 + .../audit/editor/PhabricatorAuditEditor.php | 4 +- .../editor/DifferentialTransactionEditor.php | 4 +- .../storage/ManiphestTransaction.php | 10 ++- .../data/PhabricatorTransactionChange.php | 37 +++++++++ .../PhabricatorTransactionRemarkupChange.php | 4 + ...torApplicationTransactionCommentEditor.php | 3 + ...habricatorApplicationTransactionEditor.php | 79 ++++++++++++------- .../PhabricatorApplicationTransaction.php | 48 ++++++++++- ...abricatorApplicationTransactionComment.php | 16 ++++ 10 files changed, 167 insertions(+), 42 deletions(-) create mode 100644 src/applications/transactions/data/PhabricatorTransactionChange.php create mode 100644 src/applications/transactions/data/PhabricatorTransactionRemarkupChange.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index f68e463546..cc59c09d15 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3591,6 +3591,8 @@ phutil_register_library_map(array( 'PhabricatorTokensCurtainExtension' => 'applications/tokens/engineextension/PhabricatorTokensCurtainExtension.php', 'PhabricatorTokensSettingsPanel' => 'applications/settings/panel/PhabricatorTokensSettingsPanel.php', 'PhabricatorTooltipUIExample' => 'applications/uiexample/examples/PhabricatorTooltipUIExample.php', + 'PhabricatorTransactionChange' => 'applications/transactions/data/PhabricatorTransactionChange.php', + 'PhabricatorTransactionRemarkupChange' => 'applications/transactions/data/PhabricatorTransactionRemarkupChange.php', 'PhabricatorTransactions' => 'applications/transactions/constants/PhabricatorTransactions.php', 'PhabricatorTransactionsApplication' => 'applications/transactions/application/PhabricatorTransactionsApplication.php', 'PhabricatorTransactionsDestructionEngineExtension' => 'applications/transactions/engineextension/PhabricatorTransactionsDestructionEngineExtension.php', @@ -8397,6 +8399,8 @@ phutil_register_library_map(array( 'PhabricatorTokensCurtainExtension' => 'PHUICurtainExtension', 'PhabricatorTokensSettingsPanel' => 'PhabricatorSettingsPanel', 'PhabricatorTooltipUIExample' => 'PhabricatorUIExample', + 'PhabricatorTransactionChange' => 'Phobject', + 'PhabricatorTransactionRemarkupChange' => 'PhabricatorTransactionChange', 'PhabricatorTransactions' => 'Phobject', 'PhabricatorTransactionsApplication' => 'PhabricatorApplication', 'PhabricatorTransactionsDestructionEngineExtension' => 'PhabricatorDestructionEngineExtension', diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php index d795d4b97d..f9524084a0 100644 --- a/src/applications/audit/editor/PhabricatorAuditEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditEditor.php @@ -542,7 +542,7 @@ final class PhabricatorAuditEditor protected function expandCustomRemarkupBlockTransactions( PhabricatorLiskDAO $object, array $xactions, - $blocks, + array $changes, PhutilMarkupEngine $engine) { // we are only really trying to find unmentionable phids here... @@ -563,7 +563,7 @@ final class PhabricatorAuditEditor return $result; } - $flat_blocks = array_mergev($blocks); + $flat_blocks = mpull($changes, 'getNewValue'); $huge_block = implode("\n\n", $flat_blocks); $phid_map = array(); $phid_map[] = $this->getUnmentionablePHIDMap(); diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 2d2671227d..a87c12c926 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -1303,10 +1303,10 @@ final class DifferentialTransactionEditor protected function expandCustomRemarkupBlockTransactions( PhabricatorLiskDAO $object, array $xactions, - $blocks, + array $changes, PhutilMarkupEngine $engine) { - $flat_blocks = array_mergev($blocks); + $flat_blocks = mpull($changes, 'getNewValue'); $huge_block = implode("\n\n", $flat_blocks); $task_map = array(); diff --git a/src/applications/maniphest/storage/ManiphestTransaction.php b/src/applications/maniphest/storage/ManiphestTransaction.php index 79901d7ae6..1caae54d47 100644 --- a/src/applications/maniphest/storage/ManiphestTransaction.php +++ b/src/applications/maniphest/storage/ManiphestTransaction.php @@ -54,16 +54,18 @@ final class ManiphestTransaction return parent::shouldGenerateOldValue(); } - public function getRemarkupBlocks() { - $blocks = parent::getRemarkupBlocks(); + protected function newRemarkupChanges() { + $changes = array(); switch ($this->getTransactionType()) { case self::TYPE_DESCRIPTION: - $blocks[] = $this->getNewValue(); + $changes[] = $this->newRemarkupChange() + ->setOldValue($this->getOldValue()) + ->setNewValue($this->getNewValue()); break; } - return $blocks; + return $changes; } public function getRequiredHandlePHIDs() { diff --git a/src/applications/transactions/data/PhabricatorTransactionChange.php b/src/applications/transactions/data/PhabricatorTransactionChange.php new file mode 100644 index 0000000000..2fc59ce5e5 --- /dev/null +++ b/src/applications/transactions/data/PhabricatorTransactionChange.php @@ -0,0 +1,37 @@ +transaction = $xaction; + return $this; + } + + final public function getTransaction() { + return $this->transaction; + } + + final public function setOldValue($old_value) { + $this->oldValue = $old_value; + return $this; + } + + final public function getOldValue() { + return $this->oldValue; + } + + final public function setNewValue($new_value) { + $this->newValue = $new_value; + return $this; + } + + final public function getNewValue() { + return $this->newValue; + } + +} diff --git a/src/applications/transactions/data/PhabricatorTransactionRemarkupChange.php b/src/applications/transactions/data/PhabricatorTransactionRemarkupChange.php new file mode 100644 index 0000000000..bf4a4a11b1 --- /dev/null +++ b/src/applications/transactions/data/PhabricatorTransactionRemarkupChange.php @@ -0,0 +1,4 @@ +setTransactionPHID($xaction->getPHID()); $comment->save(); + $old_comment = $xaction->getComment(); + $comment->attachOldComment($old_comment); + $xaction->setCommentVersion($new_version); $xaction->setCommentPHID($comment->getPHID()); $xaction->setViewPolicy($comment->getViewPolicy()); diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 1a4afa2c9f..098b45b9c4 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -1320,17 +1320,28 @@ abstract class PhabricatorApplicationTransactionEditor private function buildSubscribeTransaction( PhabricatorLiskDAO $object, array $xactions, - array $blocks) { + array $changes) { if (!($object instanceof PhabricatorSubscribableInterface)) { return null; } if ($this->shouldEnableMentions($object, $xactions)) { - $texts = array_mergev($blocks); - $phids = PhabricatorMarkupEngine::extractPHIDsFromMentions( + // Identify newly mentioned users. We ignore users who were previously + // mentioned so that we don't re-subscribe users after an edit of text + // which mentions them. + $old_texts = mpull($changes, 'getOldValue'); + $new_texts = mpull($changes, 'getNewValue'); + + $old_phids = PhabricatorMarkupEngine::extractPHIDsFromMentions( $this->getActor(), - $texts); + $old_texts); + + $new_phids = PhabricatorMarkupEngine::extractPHIDsFromMentions( + $this->getActor(), + $new_texts); + + $phids = array_diff($new_phids, $old_phids); } else { $phids = array(); } @@ -1381,11 +1392,6 @@ abstract class PhabricatorApplicationTransactionEditor return $xaction; } - protected function getRemarkupBlocksFromTransaction( - PhabricatorApplicationTransaction $transaction) { - return $transaction->getRemarkupBlocks(); - } - protected function mergeTransactions( PhabricatorApplicationTransaction $u, PhabricatorApplicationTransaction $v) { @@ -1464,15 +1470,12 @@ abstract class PhabricatorApplicationTransactionEditor $xactions = $this->applyImplicitCC($object, $xactions); - $blocks = array(); - foreach ($xactions as $key => $xaction) { - $blocks[$key] = $this->getRemarkupBlocksFromTransaction($xaction); - } + $changes = $this->getRemarkupChanges($xactions); $subscribe_xaction = $this->buildSubscribeTransaction( $object, $xactions, - $blocks); + $changes); if ($subscribe_xaction) { $xactions[] = $subscribe_xaction; } @@ -1484,7 +1487,7 @@ abstract class PhabricatorApplicationTransactionEditor $block_xactions = $this->expandRemarkupBlockTransactions( $object, $xactions, - $blocks, + $changes, $engine); foreach ($block_xactions as $xaction) { @@ -1494,27 +1497,46 @@ abstract class PhabricatorApplicationTransactionEditor return $xactions; } + private function getRemarkupChanges(array $xactions) { + $changes = array(); + + foreach ($xactions as $key => $xaction) { + foreach ($this->getRemarkupChangesFromTransaction($xaction) as $change) { + $changes[] = $change; + } + } + + return $changes; + } + + private function getRemarkupChangesFromTransaction( + PhabricatorApplicationTransaction $transaction) { + return $transaction->getRemarkupChanges(); + } + private function expandRemarkupBlockTransactions( PhabricatorLiskDAO $object, array $xactions, - $blocks, + array $changes, PhutilMarkupEngine $engine) { $block_xactions = $this->expandCustomRemarkupBlockTransactions( $object, $xactions, - $blocks, + $changes, $engine); $mentioned_phids = array(); if ($this->shouldEnableMentions($object, $xactions)) { - foreach ($blocks as $key => $xaction_blocks) { - foreach ($xaction_blocks as $block) { - $engine->markupText($block); - $mentioned_phids += $engine->getTextMetadata( - PhabricatorObjectRemarkupRule::KEY_MENTIONED_OBJECTS, - array()); - } + foreach ($changes as $change) { + // Here, we don't care about processing only new mentions after an edit + // because there is no way for an object to ever "unmention" itself on + // another object, so we can ignore the old value. + $engine->markupText($change->getNewValue()); + + $mentioned_phids += $engine->getTextMetadata( + PhabricatorObjectRemarkupRule::KEY_MENTIONED_OBJECTS, + array()); } } @@ -1559,7 +1581,7 @@ abstract class PhabricatorApplicationTransactionEditor protected function expandCustomRemarkupBlockTransactions( PhabricatorLiskDAO $object, array $xactions, - $blocks, + array $changes, PhutilMarkupEngine $engine) { return array(); } @@ -3096,11 +3118,8 @@ abstract class PhabricatorApplicationTransactionEditor PhabricatorLiskDAO $object, array $xactions) { - $blocks = array(); - foreach ($xactions as $xaction) { - $blocks[] = $this->getRemarkupBlocksFromTransaction($xaction); - } - $blocks = array_mergev($blocks); + $changes = $this->getRemarkupChanges($xactions); + $blocks = mpull($changes, 'getNewValue'); $phids = array(); if ($blocks) { diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php index 3a7e3841a3..a612be6769 100644 --- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php @@ -179,6 +179,50 @@ abstract class PhabricatorApplicationTransaction return $this->assertAttached($this->object); } + public function getRemarkupChanges() { + $changes = $this->newRemarkupChanges(); + assert_instances_of($changes, 'PhabricatorTransactionRemarkupChange'); + + // Convert older-style remarkup blocks into newer-style remarkup changes. + // This builds changes that do not have the correct "old value", so rules + // that operate differently against edits (like @user mentions) won't work + // properly. + foreach ($this->getRemarkupBlocks() as $block) { + $changes[] = $this->newRemarkupChange() + ->setOldValue(null) + ->setNewValue($block); + } + + $comment = $this->getComment(); + if ($comment) { + if ($comment->hasOldComment()) { + $old_value = $comment->getOldComment()->getContent(); + } else { + $old_value = null; + } + + $new_value = $comment->getContent(); + + $changes[] = $this->newRemarkupChange() + ->setOldValue($old_value) + ->setNewValue($new_value); + } + + return $changes; + } + + protected function newRemarkupChanges() { + return array(); + } + + protected function newRemarkupChange() { + return id(new PhabricatorTransactionRemarkupChange()) + ->setTransaction($this); + } + + /** + * @deprecated + */ public function getRemarkupBlocks() { $blocks = array(); @@ -195,10 +239,6 @@ abstract class PhabricatorApplicationTransaction break; } - if ($this->getComment()) { - $blocks[] = $this->getComment()->getContent(); - } - return $blocks; } diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransactionComment.php b/src/applications/transactions/storage/PhabricatorApplicationTransactionComment.php index 75964b218c..3239125249 100644 --- a/src/applications/transactions/storage/PhabricatorApplicationTransactionComment.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransactionComment.php @@ -18,6 +18,8 @@ abstract class PhabricatorApplicationTransactionComment protected $contentSource; protected $isDeleted = 0; + private $oldComment = self::ATTACHABLE; + abstract public function getApplicationTransactionObject(); public function generatePHID() { @@ -85,6 +87,20 @@ abstract class PhabricatorApplicationTransactionComment return $this; } + public function attachOldComment( + PhabricatorApplicationTransactionComment $old_comment) { + $this->oldComment = $old_comment; + return $this; + } + + public function getOldComment() { + return $this->assertAttached($this->oldComment); + } + + public function hasOldComment() { + return ($this->oldComment !== self::ATTACHABLE); + } + /* -( PhabricatorMarkupInterface )----------------------------------------- */