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

@@ -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',

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();

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();

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() {

src/applications/transactions/data/PhabricatorTransactionChange.php

@@ -0,0 +1,37 @@
+<?php
+
+abstract class PhabricatorTransactionChange extends Phobject {
+
+  private $transaction;
+  private $oldValue;
+  private $newValue;
+
+  final public function setTransaction(
+    PhabricatorApplicationTransaction $xaction) {
+    $this->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;
+  }
+
+}

src/applications/transactions/data/PhabricatorTransactionRemarkupChange.php

@@ -0,0 +1,4 @@
+<?php
+
+final class PhabricatorTransactionRemarkupChange
+  extends PhabricatorTransactionChange {}

src/applications/transactions/editor/PhabricatorApplicationTransactionCommentEditor.php

@@ -64,6 +64,9 @@ final class PhabricatorApplicationTransactionCommentEditor
         $comment->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());

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) {

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;
   }
 

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  )----------------------------------------- */