When deleting inline comments, offer "undo" instead of prompting

Summary:
Ref T2009. Ref T1460.

Fixes T2618. When users hit "Delete" on inline comments, delete immediately and offer them "Undo". If they delete indirectly (e.g., by clicking "Delete" from the preview at the bottom of the page), we still prompt them, because the "Undo" action either won't be available or may not be easy to find. This is a "refdelete".

Fixes T6464. This was just a mess. Make it not as much of a mess. It should work now. Pretty sure.

Fixes T4999. We did not refresh these links often enough to find targets for them, so they could race with content. Reevaluate them after loading new changes.

Test Plan:
  - Deleted and undid deletion of inlines from main view and preview.
  - Clicked "View" on inlines.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T6464, T4999, T2618, T1460, T2009

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

resources/celerity/map.php

@@ -11,7 +11,7 @@ return array(
     'core.pkg.js' => '5a1c336d',
     'darkconsole.pkg.js' => '8ab24e01',
     'differential.pkg.css' => '1940be3f',
-    'differential.pkg.js' => '2b14c4a1',
+    'differential.pkg.js' => 'e62fe1cf',
     'diffusion.pkg.css' => '591664fa',
     'diffusion.pkg.js' => 'bfc0737b',
     'maniphest.pkg.css' => '68d4dd3d',
@@ -360,14 +360,14 @@ return array(
     'rsrc/js/application/dashboard/behavior-dashboard-move-panels.js' => '82439934',
     'rsrc/js/application/dashboard/behavior-dashboard-query-panel-select.js' => '453c5375',
     'rsrc/js/application/dashboard/behavior-dashboard-tab-panel.js' => 'd4eecc63',
-    'rsrc/js/application/differential/ChangesetViewManager.js' => 'a9af1212',
+    'rsrc/js/application/differential/ChangesetViewManager.js' => '88be0133',
-    'rsrc/js/application/differential/DifferentialInlineCommentEditor.js' => 'd3aa4b40',
+    'rsrc/js/application/differential/DifferentialInlineCommentEditor.js' => '1b772f31',
     'rsrc/js/application/differential/behavior-add-reviewers-and-ccs.js' => 'e10f8e18',
     'rsrc/js/application/differential/behavior-comment-jump.js' => '4fdb476d',
-    'rsrc/js/application/differential/behavior-comment-preview.js' => '6932def3',
+    'rsrc/js/application/differential/behavior-comment-preview.js' => '8e1389b5',
     'rsrc/js/application/differential/behavior-diff-radios.js' => 'e1ff79b1',
     'rsrc/js/application/differential/behavior-dropdown-menus.js' => '2035b9cb',
-    'rsrc/js/application/differential/behavior-edit-inline-comments.js' => '7378d48a',
+    'rsrc/js/application/differential/behavior-edit-inline-comments.js' => 'a48aa699',
     'rsrc/js/application/differential/behavior-keyboard-nav.js' => '2c426492',
     'rsrc/js/application/differential/behavior-populate.js' => '8694b1df',
     'rsrc/js/application/differential/behavior-show-field-details.js' => 'bba9eedf',
@@ -509,7 +509,7 @@ return array(
     'aphront-two-column-view-css' => '16ab3ad2',
     'aphront-typeahead-control-css' => '0e403212',
     'auth-css' => '1e655982',
-    'changeset-view-manager' => 'a9af1212',
+    'changeset-view-manager' => '88be0133',
     'config-options-css' => '7fedf08b',
     'config-welcome-css' => '6abd79be',
     'conpherence-durable-column-view' => '9207426d',
@@ -520,7 +520,7 @@ return array(
     'conpherence-widget-pane-css' => '3d575438',
     'differential-changeset-view-css' => '6a8b172a',
     'differential-core-view-css' => '7ac3cabc',
-    'differential-inline-comment-editor' => 'd3aa4b40',
+    'differential-inline-comment-editor' => '1b772f31',
     'differential-results-table-css' => '181aa9d9',
     'differential-revision-add-comment-css' => 'c478bcaa',
     'differential-revision-comment-css' => '48186045',
@@ -569,8 +569,8 @@ return array(
     'javelin-behavior-differential-comment-jump' => '4fdb476d',
     'javelin-behavior-differential-diff-radios' => 'e1ff79b1',
     'javelin-behavior-differential-dropdown-menus' => '2035b9cb',
-    'javelin-behavior-differential-edit-inline-comments' => '7378d48a',
+    'javelin-behavior-differential-edit-inline-comments' => 'a48aa699',
-    'javelin-behavior-differential-feedback-preview' => '6932def3',
+    'javelin-behavior-differential-feedback-preview' => '8e1389b5',
     'javelin-behavior-differential-keyboard-navigation' => '2c426492',
     'javelin-behavior-differential-populate' => '8694b1df',
     'javelin-behavior-differential-show-field-details' => 'bba9eedf',
@@ -931,6 +931,14 @@ return array(
       'javelin-util',
       'phabricator-keyboard-shortcut-manager',
     ),
+    '1b772f31' => array(
+      'javelin-dom',
+      'javelin-util',
+      'javelin-stratcom',
+      'javelin-install',
+      'javelin-request',
+      'javelin-workflow',
+    ),
     '1d298e3a' => array(
       'javelin-install',
       'javelin-util',
@@ -1227,14 +1235,6 @@ return array(
     '6882e80a' => array(
       'javelin-dom',
     ),
-    '6932def3' => array(
-      'javelin-behavior',
-      'javelin-stratcom',
-      'javelin-dom',
-      'javelin-request',
-      'javelin-util',
-      'phabricator-shaped-request',
-    ),
     '69adf288' => array(
       'javelin-install',
     ),
@@ -1316,14 +1316,6 @@ return array(
       'javelin-behavior',
       'javelin-dom',
     ),
-    '7378d48a' => array(
-      'javelin-behavior',
-      'javelin-stratcom',
-      'javelin-dom',
-      'javelin-util',
-      'javelin-vector',
-      'differential-inline-comment-editor',
-    ),
     '73d09eef' => array(
       'javelin-behavior',
       'javelin-vector',
@@ -1500,6 +1492,16 @@ return array(
       'javelin-stratcom',
       'javelin-dom',
     ),
+    '88be0133' => array(
+      'javelin-dom',
+      'javelin-util',
+      'javelin-stratcom',
+      'javelin-install',
+      'javelin-workflow',
+      'javelin-router',
+      'javelin-behavior-device',
+      'javelin-vector',
+    ),
     '88f0c5b3' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -1528,6 +1530,14 @@ return array(
       'javelin-stratcom',
       'javelin-behavior',
     ),
+    '8e1389b5' => array(
+      'javelin-behavior',
+      'javelin-stratcom',
+      'javelin-dom',
+      'javelin-request',
+      'javelin-util',
+      'phabricator-shaped-request',
+    ),
     '8ef9ab58' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -1609,6 +1619,14 @@ return array(
       'javelin-vector',
       'javelin-magical-init',
     ),
+    'a48aa699' => array(
+      'javelin-behavior',
+      'javelin-stratcom',
+      'javelin-dom',
+      'javelin-util',
+      'javelin-vector',
+      'differential-inline-comment-editor',
+    ),
     'a4ae61bf' => array(
       'javelin-install',
       'javelin-dom',
@@ -1629,16 +1647,6 @@ return array(
       'javelin-uri',
       'phabricator-keyboard-shortcut',
     ),
-    'a9af1212' => array(
-      'javelin-dom',
-      'javelin-util',
-      'javelin-stratcom',
-      'javelin-install',
-      'javelin-workflow',
-      'javelin-router',
-      'javelin-behavior-device',
-      'javelin-vector',
-    ),
     'a9f88de2' => array(
       'javelin-behavior',
       'javelin-dom',
@@ -1756,14 +1764,6 @@ return array(
     'd254d646' => array(
       'javelin-util',
     ),
-    'd3aa4b40' => array(
-      'javelin-dom',
-      'javelin-util',
-      'javelin-stratcom',
-      'javelin-install',
-      'javelin-request',
-      'javelin-workflow',
-    ),
     'd4a14807' => array(
       'javelin-install',
       'javelin-dom',

src/applications/audit/storage/PhabricatorAuditInlineComment.php

@@ -290,6 +290,15 @@ final class PhabricatorAuditInlineComment
     return $this->proxy->getHasReplies();
   }
 
+  public function setIsDeleted($is_deleted) {
+    $this->proxy->setIsDeleted($is_deleted);
+    return $this;
+  }
+
+  public function getIsDeleted() {
+    return $this->proxy->getIsDeleted();
+  }
+
 
 /* -(  PhabricatorMarkupInterface Implementation  )-------------------------- */
 

src/applications/differential/query/DifferentialInlineCommentQuery.php

@@ -129,7 +129,7 @@ final class DifferentialInlineCommentQuery
       $where[] = qsprintf(
         $conn_r,
         'changesetID IN (%Ld) AND
-          (authorPHID = %s OR transactionPHID IS NOT NULL)',
+          ((authorPHID = %s AND isDeleted = 0) OR transactionPHID IS NOT NULL)',
         $ids,
         $phid);
     }
@@ -151,7 +151,8 @@ final class DifferentialInlineCommentQuery
 
       $where[] = qsprintf(
         $conn_r,
-        'authorPHID = %s AND revisionPHID = %s AND transactionPHID IS NULL',
+        'authorPHID = %s AND revisionPHID = %s AND transactionPHID IS NULL
+          AND isDeleted = 0',
         $phid,
         $rev_phid);
     }
@@ -159,7 +160,7 @@ final class DifferentialInlineCommentQuery
     if ($this->draftsByAuthors) {
       $where[] = qsprintf(
         $conn_r,
-        'authorPHID IN (%Ls) AND transactionPHID IS NULL',
+        'authorPHID IN (%Ls) AND isDeleted = 0 AND transactionPHID IS NULL',
         $this->draftsByAuthors);
     }
 

src/applications/differential/storage/DifferentialInlineComment.php

@@ -207,6 +207,15 @@ final class DifferentialInlineComment
     return $this->proxy->getHasReplies();
   }
 
+  public function setIsDeleted($is_deleted) {
+    $this->proxy->setIsDeleted($is_deleted);
+    return $this;
+  }
+
+  public function getIsDeleted() {
+    return $this->proxy->getIsDeleted();
+  }
+
 
 /* -(  PhabricatorMarkupInterface Implementation  )-------------------------- */
 

src/infrastructure/diff/PhabricatorInlineCommentController.php

@@ -79,32 +79,40 @@ abstract class PhabricatorInlineCommentController
 
     $this->readRequestParameters();
 
-    switch ($this->getOperation()) {
+    $op = $this->getOperation();
+    switch ($op) {
       case 'delete':
-        $inline = $this->loadCommentForEdit($this->getCommentID());
+      case 'undelete':
-
+      case 'refdelete':
-        if ($request->isFormPost()) {
+        if (!$request->validateCSRF()) {
-          $this->deleteComment($inline);
+          return new Aphront404Response();
-          return $this->buildEmptyResponse();
         }
 
-        $dialog = new AphrontDialogView();
+        // NOTE: For normal deletes, we just process the delete immediately
-        $dialog->setUser($user);
+        // and show an "Undo" action. For deletes by reference from the
-        $dialog->setSubmitURI($request->getRequestURI());
+        // preview ("refdelete"), we prompt first (because the "Undo" may
+        // not draw, or may not be easy to locate).
 
-        $dialog->setTitle(pht('Really delete this comment?'));
+        if ($op == 'refdelete') {
-        $dialog->addHiddenInput('id', $this->getCommentID());
+          if (!$request->isFormPost()) {
-        $dialog->addHiddenInput('op', 'delete');
+            return $this->newDialog()
-        $dialog->appendChild(
+              ->setTitle(pht('Really delete comment?'))
-          phutil_tag('p', array(), pht('Delete this inline comment?')));
+              ->addHiddenInput('id', $this->getCommentID())
+              ->addHiddenInput('op', $op)
+              ->appendParagraph(pht('Delete this inline comment?'))
+              ->addCancelButton('#')
+              ->addSubmitButton(pht('Delete'));
+          }
+        }
 
-        $dialog->addCancelButton('#');
+        $is_delete = ($op == 'delete' || $op == 'refdelete');
-        $dialog->addSubmitButton(pht('Delete'));
 
-        return id(new AphrontDialogResponse())->setDialog($dialog);
+        $inline = $this->loadCommentForEdit($this->getCommentID());
+        $inline->setIsDeleted((int)$is_delete)->save();
+
+        return $this->buildEmptyResponse();
       case 'edit':
         $inline = $this->loadCommentForEdit($this->getCommentID());
-
         $text = $this->getCommentText();
 
         if ($request->isFormPost()) {

src/infrastructure/diff/interface/PhabricatorInlineCommentInterface.php

@@ -25,6 +25,9 @@ interface PhabricatorInlineCommentInterface extends PhabricatorMarkupInterface {
   public function setHasReplies($has_replies);
   public function getHasReplies();
 
+  public function setIsDeleted($deleted);
+  public function getIsDeleted();
+
   public function setContent($content);
   public function getContent();
 

webroot/rsrc/js/application/differential/ChangesetViewManager.js

@@ -359,6 +359,8 @@ JX.install('ChangesetViewManager', {
       if (response.undoTemplates) {
         this._undoTemplates = response.undoTemplates;
       }
+
+      JX.Stratcom.invoke('differential-inline-comment-refresh');
     },
 
     _getContentFrame: function() {

webroot/rsrc/js/application/differential/DifferentialInlineCommentEditor.js

@@ -19,6 +19,7 @@ JX.install('DifferentialInlineCommentEditor', {
   members : {
     _uri : null,
     _undoText : null,
+    _completed: false,
     _skipOverInlineCommentRows : function(node) {
       // TODO: Move this semantic information out of class names.
       while (node && node.className.indexOf('inline') !== -1) {
@@ -73,11 +74,17 @@ JX.install('DifferentialInlineCommentEditor', {
           JX.DOM.remove(rows[ii]);
         }
       }
+      JX.DifferentialInlineCommentEditor._undoRows = [];
     },
     _undo : function() {
       this._removeUndoLink();
 
-      this.setText(this._undoText);
+      if (this._undoText) {
+        this.setText(this._undoText);
+      } else {
+        this.setOperation('undelete');
+      }
+
       this.start();
     },
     _registerUndoListener : function() {
@@ -146,9 +153,18 @@ JX.install('DifferentialInlineCommentEditor', {
         'inline-edit-form',
         onsubmit);
     },
+
+
     _didCompleteWorkflow : function(response) {
       var op = this.getOperation();
 
+      if (op == 'delete' || op == 'refdelete') {
+        this._undoText = null;
+        this._drawUndo();
+      } else {
+        this._removeUndoLink();
+      }
+
       // We don't get any markup back if the user deletes a comment, or saves
       // an empty comment (which effects a delete).
       if (response.markup) {
@@ -156,31 +172,34 @@ JX.install('DifferentialInlineCommentEditor', {
       }
 
       // These operations remove the old row (edit adds a new row first).
-      var remove_old = (op == 'edit' || op == 'delete');
+      var remove_old = (op == 'edit' || op == 'delete' || op == 'refdelete');
       if (remove_old) {
-        JX.DOM.remove(this.getRow());
+        this._setRowState('hidden');
-        var other_rows = this.getOtherRows();
-        for(var i = 0; i < other_rows.length; ++i) {
-          JX.DOM.remove(other_rows[i]);
-        }
       }
 
-      // Once the user saves something, get rid of the 'undo' option. A
+      if (op == 'undelete') {
-      // particular case where we need this is saving a delete, when we might
+        this._setRowState('visible');
-      // otherwise leave around an 'undo' for an earlier edit to the same
+      }
-      // comment.
+
-      this._removeUndoLink();
+      this._completed = true;
 
       JX.Stratcom.invoke('differential-inline-comment-update');
       this.invoke('done');
     },
+
+
     _didCancelWorkflow : function() {
       this.invoke('done');
 
-      var op = this.getOperation();
+      switch (this.getOperation()) {
-      if (op == 'delete') {
+        case 'delete':
-        // No undo for delete, we prompt the user explicitly.
+        case 'refdelete':
-        return;
+          if (!this._completed) {
+            this._setRowState('visible');
+          }
+          return;
+        case 'undelete':
+          return;
       }
 
       var textarea;
@@ -209,6 +228,10 @@ JX.install('DifferentialInlineCommentEditor', {
       // Save the text so we can 'undo' back to it.
       this._undoText = text;
 
+      this._drawUndo();
+    },
+
+    _drawUndo: function() {
       var templates = this.getTemplates();
       var template = this.getOnRight() ? templates.r : templates.l;
       template = JX.$H(template).getNode();
@@ -231,21 +254,18 @@ JX.install('DifferentialInlineCommentEditor', {
       this._registerUndoListener();
 
       var data = this._buildRequestData();
-
       var op = this.getOperation();
 
 
-      if (op == 'delete') {
+      if (op == 'delete' || op == 'refdelete' || op == 'undelete') {
         this._setRowState('loading');
+
         var oncomplete = JX.bind(this, this._didCompleteWorkflow);
-        var onclose = JX.bind(this, function() {
+        var oncancel = JX.bind(this, this._didCancelWorkflow);
-          this._setRowState('visible');
-          this._didCancelWorkflow();
-        });
 
         new JX.Workflow(this._uri, data)
           .setHandler(oncomplete)
-          .setCloseHandler(onclose)
+          .setCloseHandler(oncancel)
           .start();
       } else {
         var handler = JX.bind(this, this._didContinueWorkflow);
@@ -260,7 +280,21 @@ JX.install('DifferentialInlineCommentEditor', {
       }
 
       return this;
+    },
+
+    deleteByID: function(id) {
+      var data = {
+        op: 'refdelete',
+        changesetID: id
+      };
+
+      new JX.Workflow(this._uri, data)
+        .setHandler(function() {
+          JX.Stratcom.invoke('differential-inline-comment-update');
+        })
+        .start();
     }
+
   },
 
   statics : {
@@ -280,7 +314,6 @@ JX.install('DifferentialInlineCommentEditor', {
   properties : {
     operation : null,
     row : null,
-    otherRows: [],
     table : null,
     onRight : null,
     ID : null,

webroot/rsrc/js/application/differential/behavior-comment-preview.js

@@ -49,42 +49,51 @@ JX.behavior('differential-feedback-preview', function(config) {
 
   request.start();
 
-
   function refreshInlinePreview() {
     new JX.Request(config.inlineuri, function(r) {
-        var inline = JX.$(config.inline);
+      var inline = JX.$(config.inline);
 
-        JX.DOM.setContent(inline, JX.$H(r));
+      JX.DOM.setContent(inline, JX.$H(r));
-        JX.Stratcom.invoke('differential-preview-update', null, {
+      JX.Stratcom.invoke('differential-preview-update', null, {
-          container: inline
+        container: inline
-        });
+      });
 
-        // Go through the previews and activate any "View" links where the
+      updateLinks();
-        // actual comment appears in the document.
+    })
-
+    .setTimeout(5000)
-        var links = JX.DOM.scry(
+    .send();
-          inline,
-          'a',
-          'differential-inline-preview-jump');
-        for (var ii = 0; ii < links.length; ii++) {
-          var data = JX.Stratcom.getData(links[ii]);
-          try {
-            JX.$(data.anchor);
-            links[ii].href = '#' + data.anchor;
-            JX.DOM.setContent(links[ii], 'View');
-          } catch (ignored) {
-            // This inline comment isn't visible, e.g. on some other diff.
-          }
-        }
-      })
-      .setTimeout(5000)
-      .send();
   }
 
+  function updateLinks() {
+    var inline = JX.$(config.inline);
+
+    var links = JX.DOM.scry(
+      inline,
+      'a',
+      'differential-inline-preview-jump');
+
+    for (var ii = 0; ii < links.length; ii++) {
+      var data = JX.Stratcom.getData(links[ii]);
+      try {
+        JX.$(data.anchor);
+        links[ii].href = '#' + data.anchor;
+        JX.DOM.setContent(links[ii], 'View');
+      } catch (ignored) {
+        // This inline comment isn't visible, e.g. on some other diff.
+      }
+    }
+  }
+
+
   JX.Stratcom.listen(
     'differential-inline-comment-update',
     null,
     refreshInlinePreview);
 
+  JX.Stratcom.listen(
+    'differential-inline-comment-refresh',
+    null,
+    updateLinks);
+
   refreshInlinePreview();
 });

webroot/rsrc/js/application/differential/behavior-edit-inline-comments.js

@@ -298,25 +298,40 @@ JX.behavior('differential-edit-inline-comments', function(config) {
 
   var handle_inline_action = function(node, op) {
     var data = JX.Stratcom.getData(node);
-    var row  = node.parentNode.parentNode;
+
-    var other_rows = [];
+    // If you click an action in the preview at the bottom of the page, we
+    // find the corresponding node and simulate clicking that, if it's
+    // present on the page. This gives the editor a more consistent view
+    // of the document.
     if (JX.Stratcom.hasSigil(node, 'differential-inline-comment-preview')) {
-      // The DOM structure around the comment is different if it's part of the
+      var nodes = JX.DOM.scry(
-      // preview, so make sure not to pass the wrong container.
+        JX.DOM.getContentFrame(),
-      row = node;
+        'div',
-      if (op === 'delete') {
+        'differential-inline-comment');
-        // Furthermore, deleting a comment in the preview does not automatically
+
-        // delete other occurrences of the same comment, so do that manually.
+      var found = false;
-        var nodes = JX.DOM.scry(
+      var node_data;
-          JX.DOM.getContentFrame(),
+      for (var ii = 0; ii < nodes.length; ++ii) {
-          'div',
+        if (nodes[ii] == node) {
-          'differential-inline-comment');
+          // Don't match the preview itself.
-        for (var i = 0; i < nodes.length; ++i) {
+          continue;
-          if (JX.Stratcom.getData(nodes[i]).id === data.id) {
+        }
-            other_rows.push(nodes[i]);
+        node_data = JX.Stratcom.getData(nodes[ii]);
-          }
+        if (node_data.id == data.id) {
+          node = nodes[ii];
+          data = node_data;
+          found = true;
+          break;
         }
       }
+
+      if (!found) {
+        new JX.DifferentialInlineCommentEditor(config.uri)
+          .deleteByID(data.id);
+        return;
+      }
+
+      op = 'refdelete';
     }
 
     var original = data.original;
@@ -328,6 +343,7 @@ JX.behavior('differential-edit-inline-comments', function(config) {
       reply_phid = data.phid;
     }
 
+    var row = JX.DOM.findAbove(node, 'tr');
     var changeset_root = JX.DOM.findAbove(
       node,
       'div',
@@ -344,7 +360,6 @@ JX.behavior('differential-edit-inline-comments', function(config) {
       .setOnRight(data.on_right)
       .setOriginalText(original)
       .setRow(row)
-      .setOtherRows(other_rows)
       .setTable(row.parentNode)
       .setReplyToCommentPHID(reply_phid)
       .setRenderer(view.getRenderer())