FIX: Quoting posts (#9378)

Fixes to the quote feature. Most important changes listed below:

* FIX: Correctly attribute quotes when using Reply button
* FIX: Correctly attribute quotes when using replyAsNewTopic
* FIX: Allow quoting a quote
* FIX: Correctly mark quotes as "full"
* FIX: Don't try to create a quote if it's empty
* DEV: Remove an obsolete method `loadQuote`
  It isn't used in core anymore, the only use in core has been removed over 4 years ago in 3251bcb. It's not used in any plugins in all-the-plugins and all references to it on GitHub are from outdated forks (https://github.com/search?q=%22Post.loadQuote%22&type=Code)

app/assets/javascripts/discourse/components/quote-button.js

@@ -1,6 +1,7 @@
 import { scheduleOnce } from "@ember/runloop";
 import Component from "@ember/component";
 import discourseDebounce from "discourse/lib/debounce";
+import toMarkdown from "discourse/lib/to-markdown";
 import { selectedText, selectedElement } from "discourse/lib/utilities";
 import { INPUT_DELAY } from "discourse-common/config/environment";
 
@@ -38,12 +39,13 @@ export default Component.extend({
     let firstRange, postId;
     for (let r = 0; r < selection.rangeCount; r++) {
       const range = selection.getRangeAt(r);
-
+      const $selectionStart = $(range.startContainer);
-      if ($(range.startContainer.parentNode).closest(".cooked").length === 0)
-        return;
-
       const $ancestor = $(range.commonAncestorContainer);
 
+      if ($selectionStart.closest(".cooked").length === 0) {
+        return;
+      }
+
       firstRange = firstRange || range;
       postId = postId || $ancestor.closest(".boxed, .reply").data("post-id");
 
@@ -55,9 +57,21 @@ export default Component.extend({
       }
     }
 
-    let opts = { raw: true };
+    const _selectedElement = selectedElement();
+    const _selectedText = selectedText();
+
+    const $selectedElement = $(_selectedElement);
+    const cooked =
+      $selectedElement.find(".cooked")[0] ||
+      $selectedElement.closest(".cooked")[0];
+    const postBody = toMarkdown(cooked.innerHTML);
+
+    let opts = {
+      full: _selectedText === postBody
+    };
+
     for (
-      let element = selectedElement();
+      let element = _selectedElement;
       element && element.tagName !== "ARTICLE";
       element = element.parentElement
     ) {
@@ -69,7 +83,6 @@ export default Component.extend({
       }
     }
 
-    const _selectedText = selectedText();
     quoteState.selected(postId, _selectedText, opts);
     this.set("visible", quoteState.buffer.length > 0);
 
@@ -186,8 +199,7 @@ export default Component.extend({
   },
 
   click() {
-    const { postId, buffer, opts } = this.quoteState;
+    this.attrs.selectText().then(() => this._hideButton());
-    this.attrs.selectText(postId, buffer, opts).then(() => this._hideButton());
     return false;
   }
 });

app/assets/javascripts/discourse/controllers/composer.js

@@ -5,7 +5,7 @@ import { inject as service } from "@ember/service";
 import { inject } from "@ember/controller";
 import Controller from "@ember/controller";
 import DiscourseURL from "discourse/lib/url";
-import Quote from "discourse/lib/quote";
+import { buildQuote } from "discourse/lib/quote";
 import Draft from "discourse/models/draft";
 import Composer from "discourse/models/composer";
 import discourseComputed, {
@@ -501,15 +501,14 @@ export default Controller.extend({
 
       if (postId) {
         this.set("model.loading", true);
-        const composer = this;
 
         return this.store.find("post", postId).then(post => {
-          const quote = Quote.build(post, post.raw, {
+          const quote = buildQuote(post, post.raw, {
-            raw: true,
             full: true
           });
+
           toolbarEvent.addText(quote);
-          composer.set("model.loading", false);
+          this.set("model.loading", false);
         });
       }
     },

app/assets/javascripts/discourse/controllers/topic.js

@@ -8,7 +8,7 @@ import { bufferedProperty } from "discourse/mixins/buffered-content";
 import Composer from "discourse/models/composer";
 import DiscourseURL from "discourse/lib/url";
 import Post from "discourse/models/post";
-import Quote from "discourse/lib/quote";
+import { buildQuote } from "discourse/lib/quote";
 import QuoteState from "discourse/lib/quote-state";
 import Topic from "discourse/models/topic";
 import discourseDebounce from "discourse/lib/debounce";
@@ -265,7 +265,8 @@ export default Controller.extend(bufferedProperty("model"), {
       this.send("showFeatureTopic");
     },
 
-    selectText(postId, buffer, opts) {
+    selectText() {
+      const { postId, buffer, opts } = this.quoteState;
       const loadedPost = this.get("model.postStream").findLoadedPost(postId);
       const promise = loadedPost
         ? Promise.resolve(loadedPost)
@@ -274,11 +275,10 @@ export default Controller.extend(bufferedProperty("model"), {
       return promise.then(post => {
         const composer = this.composer;
         const viewOpen = composer.get("model.viewOpen");
-        const quotedText = Quote.build(post, buffer, opts);
 
         // If we can't create a post, delegate to reply as new topic
         if (!viewOpen && !this.get("model.details.can_create_post")) {
-          this.send("replyAsNewTopic", post, quotedText);
+          this.send("replyAsNewTopic", post);
           return;
         }
 
@@ -300,7 +300,9 @@ export default Controller.extend(bufferedProperty("model"), {
           composerOpts.post = composerPost;
         }
 
+        const quotedText = buildQuote(post, buffer, opts);
         composerOpts.quote = quotedText;
+
         if (composer.get("model.viewOpen")) {
           this.appEvents.trigger("composer:insert-block", quotedText);
         } else if (composer.get("model.viewDraft")) {
@@ -483,7 +485,11 @@ export default Controller.extend(bufferedProperty("model"), {
       }
 
       const quotedPost = postStream.findLoadedPost(quoteState.postId);
-      const quotedText = Quote.build(quotedPost, quoteState.buffer);
+      const quotedText = buildQuote(
+        quotedPost,
+        quoteState.buffer,
+        quoteState.opts
+      );
 
       quoteState.clear();
 
@@ -967,14 +973,14 @@ export default Controller.extend(bufferedProperty("model"), {
       }
     },
 
-    replyAsNewTopic(post, quotedText) {
+    replyAsNewTopic(post) {
       const composerController = this.composer;
-
       const { quoteState } = this;
-      quotedText = quotedText || Quote.build(post, quoteState.buffer);
+      const quotedText = buildQuote(post, quoteState.buffer, quoteState.opts);
+
       quoteState.clear();
 
-      var options;
+      let options;
       if (this.get("model.isPrivateMessage")) {
         let users = this.get("model.details.allowed_users");
         let groups = this.get("model.details.allowed_groups");
@@ -998,24 +1004,15 @@ export default Controller.extend(bufferedProperty("model"), {
         };
       }
 
-      composerController
+      composerController.open(options).then(() => {
-        .open(options)
+        const title = Handlebars.escapeExpression(this.model.title);
-        .then(() => {
+        const postUrl = `${location.protocol}//${location.host}${post.url}`;
-          return isEmpty(quotedText) ? "" : quotedText;
+        const postLink = `[${title}](${postUrl})`;
-        })
+        const text = `${I18n.t("post.continue_discussion", {
-        .then(q => {
+          postLink
-          const postUrl = `${location.protocol}//${location.host}${post.get(
+        })}\n\n${quotedText}`;
-            "url"
+
-          )}`;
+        composerController.model.prependText(text, { new_line: true });
-          const postLink = `[${Handlebars.escapeExpression(
-            this.get("model.title")
-          )}](${postUrl})`;
-          composerController
-            .get("model")
-            .prependText(
-              `${I18n.t("post.continue_discussion", { postLink })}\n\n${q}`,
-              { new_line: true }
-            );
       });
     },
 

app/assets/javascripts/discourse/lib/quote.js

@@ -1,43 +1,18 @@
-export default {
+export const QUOTE_REGEXP = /\[quote=([^\]]*)\]((?:[\s\S](?!\[quote=[^\]]*\]))*?)\[\/quote\]/im;
-  REGEXP: /\[quote=([^\]]*)\]((?:[\s\S](?!\[quote=[^\]]*\]))*?)\[\/quote\]/im,
 
-  // Build the BBCode quote around the selected text
+// Build the BBCode quote around the selected text
-  build(post, contents, opts) {
+export function buildQuote(post, contents, opts = {}) {
-    if (!post) {
+  if (!post || !contents) {
     return "";
   }
 
-    if (!contents) contents = "";
-    if (!opts) opts = {};
-
-    const sansQuotes = contents.replace(this.REGEXP, "").trim();
-    if (sansQuotes.length === 0) {
-      return "";
-    }
-
-    // Strip the HTML from cooked
-    const stripped = $("<div/>")
-      .html(post.get("cooked"))
-      .text();
-
-    // Let's remove any non-word characters as a kind of hash.
-    // Yes it's not accurate but it should work almost every time we need it to.
-    // It would be unlikely that the user would quote another post that matches in exactly this way.
-    const sameContent =
-      stripped.replace(/\W/g, "") === contents.replace(/\W/g, "");
-
   const params = [
     opts.username || post.username,
     `post:${opts.post || post.post_number}`,
     `topic:${opts.topic || post.topic_id}`
   ];
 
-    opts = opts || {};
+  if (opts.full) params.push("full:true");
 
-    if (opts["full"] || sameContent) params.push("full:true");
+  return `[quote="${params.join(", ")}"]\n${contents.trim()}\n[/quote]\n\n`;
-
+}
-    return `[quote="${params.join(", ")}"]\n${
-      opts["raw"] ? contents : sansQuotes
-    }\n[/quote]\n\n`;
-  }
-};

app/assets/javascripts/discourse/lib/utilities.js

@@ -149,7 +149,7 @@ export function selectedText() {
 export function selectedElement() {
   const selection = window.getSelection();
   if (selection.rangeCount > 0) {
-    return selection.getRangeAt(0).commonAncestorContainer.parentElement;
+    return selection.getRangeAt(0).commonAncestorContainer;
   }
 }
 

app/assets/javascripts/discourse/models/composer.js

@@ -5,7 +5,7 @@ import { cancel, later, next, throttle } from "@ember/runloop";
 import RestModel from "discourse/models/rest";
 import Topic from "discourse/models/topic";
 import { throwAjaxError } from "discourse/lib/ajax-error";
-import Quote from "discourse/lib/quote";
+import { QUOTE_REGEXP } from "discourse/lib/quote";
 import Draft from "discourse/models/draft";
 import discourseComputed, {
   observes,
@@ -517,10 +517,10 @@ const Composer = RestModel.extend({
       return reply.length;
     }
 
-    while (Quote.REGEXP.test(reply)) {
+    while (QUOTE_REGEXP.test(reply)) {
       // make it global so we can strip as many quotes at once
       // keep in mind nested quotes mean we still need a loop here
-      const regex = new RegExp(Quote.REGEXP.source, "img");
+      const regex = new RegExp(QUOTE_REGEXP.source, "img");
       reply = reply.replace(regex, "");
     }
 

app/assets/javascripts/discourse/models/post.js

@@ -7,7 +7,6 @@ import RestModel from "discourse/models/rest";
 import { popupAjaxError } from "discourse/lib/ajax-error";
 import ActionSummary from "discourse/models/action-summary";
 import { propertyEqual } from "discourse/lib/computed";
-import Quote from "discourse/lib/quote";
 import { postUrl } from "discourse/lib/utilities";
 import { cookAsync } from "discourse/lib/text";
 import { userPath } from "discourse/lib/url";
@@ -468,13 +467,6 @@ Post.reopenClass({
     });
   },
 
-  loadQuote(postId) {
-    return ajax(`/posts/${postId}.json`).then(result => {
-      const post = Post.create(result);
-      return Quote.build(post, post.raw, { raw: true, full: true });
-    });
-  },
-
   loadRawEmail(postId) {
     return ajax(`/posts/${postId}/raw-email.json`);
   }

app/assets/javascripts/discourse/templates/components/d-editor.hbs

@@ -7,7 +7,7 @@
             {{toolbar-popup-menu-options
               content=popupMenuOptions
               onChange=onPopupMenuAction
-              onExpand=(action b.action b)
+              onOpen=(action b.action b)
               class=b.className
               options=(hash
                 popupTitle=b.title

app/assets/javascripts/discourse/templates/topic.hbs

@@ -205,7 +205,6 @@
                 deletePost=(action "deletePost")
                 recoverPost=(action "recoverPost")
                 expandHidden=(action "expandHidden")
-                newTopicAction=(action "replyAsNewTopic")
                 toggleBookmark=(action "toggleBookmark")
                 toggleBookmarkWithReminder=(action "toggleBookmarkWithReminder")
                 togglePostType=(action "togglePostType")

test/javascripts/acceptance/topic-test.js

@@ -328,15 +328,19 @@ QUnit.test("View Hidden Replies", async assert => {
   assert.equal(find(".gap").length, 0, "it hides gap");
 });
 
-QUnit.test("Quoting a quote keeps the original poster name", async assert => {
+function selectText(selector) {
-  await visit("/t/internationalization-localization/280");
+  const range = document.createRange();
+  const node = document.querySelector(selector);
+  range.selectNodeContents(node);
 
   const selection = window.getSelection();
-  const range = document.createRange();
-  range.selectNodeContents($("#post_5 blockquote")[0]);
   selection.removeAllRanges();
   selection.addRange(range);
+}
 
+QUnit.test("Quoting a quote keeps the original poster name", async assert => {
+  await visit("/t/internationalization-localization/280");
+  selectText("#post_5 blockquote");
   await click(".quote-button");
 
   assert.ok(
@@ -346,6 +350,52 @@ QUnit.test("Quoting a quote keeps the original poster name", async assert => {
   );
 });
 
+QUnit.test(
+  "Quoting a quote with the Reply button keeps the original poster name",
+  async assert => {
+    await visit("/t/internationalization-localization/280");
+    selectText("#post_5 blockquote");
+    await click(".reply");
+
+    assert.ok(
+      find(".d-editor-input")
+        .val()
+        .indexOf('quote="codinghorror said, post:3, topic:280"') !== -1
+    );
+  }
+);
+
+QUnit.test(
+  "Quoting a quote with replyAsNewTopic keeps the original poster name",
+  async assert => {
+    await visit("/t/internationalization-localization/280");
+    selectText("#post_5 blockquote");
+    await keyEvent(document, "keypress", "j".charCodeAt(0));
+    await keyEvent(document, "keypress", "t".charCodeAt(0));
+
+    assert.ok(
+      find(".d-editor-input")
+        .val()
+        .indexOf('quote="codinghorror said, post:3, topic:280"') !== -1
+    );
+  }
+);
+
+QUnit.test(
+  "Quoting by selecting text can mark the quote as full",
+  async assert => {
+    await visit("/t/internationalization-localization/280");
+    selectText("#post_5 .cooked");
+    await click(".quote-button");
+
+    assert.ok(
+      find(".d-editor-input")
+        .val()
+        .indexOf('quote="pekka, post:5, topic:280, full:true"') !== -1
+    );
+  }
+);
+
 acceptance("Topic + Post Bookmarks with Reminders", {
   loggedIn: true,
   settings: {

test/javascripts/lib/pretty-text-test.js

@@ -1,4 +1,4 @@
-import Quote from "discourse/lib/quote";
+import { buildQuote } from "discourse/lib/quote";
 import Post from "discourse/models/post";
 import PrettyText, { buildOptions } from "pretty-text/pretty-text";
 import { IMAGE_VERSION as v } from "pretty-text/emoji/version";
@@ -1289,8 +1289,8 @@ QUnit.test("quotes", assert => {
     topic_id: 2
   });
 
-  function formatQuote(val, expected, text) {
+  function formatQuote(val, expected, text, opts) {
-    assert.equal(Quote.build(post, val), expected, text);
+    assert.equal(buildQuote(post, val, opts), expected, text);
   }
 
   formatQuote(undefined, "", "empty string for undefined content");
@@ -1312,12 +1312,13 @@ QUnit.test("quotes", assert => {
   formatQuote(
     "lorem ipsum",
     '[quote="eviltrout, post:1, topic:2, full:true"]\nlorem ipsum\n[/quote]\n\n',
-    "marks quotes as full when the quote is the full message"
+    "marks quotes as full if the `full` option is passed",
+    { full: true }
   );
 
   formatQuote(
     "**lorem** ipsum",
-    '[quote="eviltrout, post:1, topic:2, full:true"]\n**lorem** ipsum\n[/quote]\n\n',
+    '[quote="eviltrout, post:1, topic:2"]\n**lorem** ipsum\n[/quote]\n\n',
     "keeps BBCode formatting"
   );
 
@@ -1340,6 +1341,28 @@ QUnit.test("quotes", assert => {
   );
 });
 
+QUnit.test("quoting a quote", assert => {
+  const post = Post.create({
+    cooked: new PrettyText(defaultOpts).cook(
+      '[quote="sam, post:1, topic:1, full:true"]\nhello\n[/quote]\n*Test*'
+    ),
+    username: "eviltrout",
+    post_number: 1,
+    topic_id: 2
+  });
+
+  const quote = buildQuote(
+    post,
+    '[quote="sam, post:1, topic:1, full:true"]\nhello\n[/quote]'
+  );
+
+  assert.equal(
+    quote,
+    '[quote="eviltrout, post:1, topic:2"]\n[quote="sam, post:1, topic:1, full:true"]\nhello\n[/quote]\n[/quote]\n\n',
+    "allows quoting a quote"
+  );
+});
+
 QUnit.test("quote formatting", assert => {
   assert.cooked(
     '[quote="EvilTrout, post:123, topic:456, full:true"]\n[sam]\n[/quote]',