FEATURE: Reimplement `SiteSetting.max_oneboxes_per_post`. (#6668)

Previously, the site setting was only effective on the client side of things. Once the site setting was been reached, all oneboxes are not rendered. This commit changes it such that the site setting is respected both on the client and server side. The first N oneboxes are rendered and once the limit has been reached, subsequent oneboxes will not be rendered.
2018-11-27 16:00:31 +08:00 · 2018-11-27 16:00:31 +08:00 · a1e77aa2ed
parent 74b300110f
commit a1e77aa2ed
11 changed files with 291 additions and 67 deletions
--- a/app/assets/javascripts/discourse/components/composer-editor.js.es6
+++ b/app/assets/javascripts/discourse/components/composer-editor.js.es6
@ -17,7 +17,7 @@ import {
  fetchUnseenTagHashtags
 } from "discourse/lib/link-tag-hashtag";
 import Composer from "discourse/models/composer";
-import { load } from "pretty-text/oneboxer";
+import { load, LOADING_ONEBOX_CSS_CLASS } from "pretty-text/oneboxer";
 import { applyInlineOneboxes } from "pretty-text/inline-oneboxer";
 import { ajax } from "discourse/lib/ajax";
 import InputValidation from "discourse/models/input-validation";
@ -37,7 +37,10 @@ import {
  resolveAllShortUrls
 } from "pretty-text/image-short-url";

-import { INLINE_ONEBOX_LOADING_CSS_CLASS } from "pretty-text/inline-oneboxer";
+import {
+  INLINE_ONEBOX_LOADING_CSS_CLASS,
+  INLINE_ONEBOX_CSS_CLASS
+} from "pretty-text/inline-oneboxer";

 const REBUILD_SCROLL_MAP_EVENTS = ["composer:resized", "composer:typed-reply"];

@ -513,7 +516,7 @@ export default Ember.Component.extend({
    applyInlineOneboxes(inline, ajax);
  },

-  _loadOneboxes($oneboxes) {
+  _loadOneboxes(oneboxes) {
    const post = this.get("composer.post");
    let refresh = false;

@ -523,15 +526,19 @@ export default Ember.Component.extend({
      post.set("refreshedPost", true);
    }

-    $oneboxes.each((_, o) =>
-      load({
-        elem: o,
-        refresh,
-        ajax,
-        categoryId: this.get("composer.category.id"),
-        topicId: this.get("composer.topic.id")
-      })
-    );
+    Object.keys(oneboxes).forEach(oneboxURL => {
+      const onebox = oneboxes[oneboxURL];
+
+      onebox.forEach($onebox => {
+        load({
+          elem: $onebox,
+          refresh,
+          ajax,
+          categoryId: this.get("composer.category.id"),
+          topicId: this.get("composer.topic.id")
+        });
+      });
+    });
  },

  _warnMentionedGroups($preview) {
@ -986,31 +993,58 @@ export default Ember.Component.extend({
      }

      // Paint oneboxes
-      const $oneboxes = $("a.onebox", $preview);
-      if (
-        $oneboxes.length > 0 &&
-        $oneboxes.length <= this.siteSettings.max_oneboxes_per_post
-      ) {
-        Ember.run.debounce(this, this._loadOneboxes, $oneboxes, 450);
-      }
+      Ember.run.debounce(
+        this,
+        () => {
+          const inlineOneboxes = {};
+          const oneboxes = {};

+          let oneboxLeft =
+            this.siteSettings.max_oneboxes_per_post -
+            $(
+              `aside.onebox, a.${INLINE_ONEBOX_CSS_CLASS}, a.${LOADING_ONEBOX_CSS_CLASS}`
+            ).length;
+
+          $preview
+            .find(`a.${INLINE_ONEBOX_LOADING_CSS_CLASS}, a.onebox`)
+            .each((_index, link) => {
+              const $link = $(link);
+              const text = $link.text();
+
+              const isInline =
+                $link.attr("class") === INLINE_ONEBOX_LOADING_CSS_CLASS;
+
+              const map = isInline ? inlineOneboxes : oneboxes;
+
+              if (oneboxLeft <= 0) {
+                if (map[text] !== undefined) {
+                  map[text].push(link);
+                } else if (isInline) {
+                  $link.removeClass(INLINE_ONEBOX_LOADING_CSS_CLASS);
+                }
+              } else {
+                if (!map[text]) {
+                  map[text] = [];
+                  oneboxLeft--;
+                }
+
+                map[text].push(link);
+              }
+            });
+
+          if (Object.keys(oneboxes).length > 0) {
+            this._loadOneboxes(oneboxes);
+          }
+
+          if (Object.keys(inlineOneboxes).length > 0) {
+            this._loadInlineOneboxes(inlineOneboxes);
+          }
+        },
+        450
+      );
      // Short upload urls need resolution
      resolveAllShortUrls(ajax);

-      let inline = {};
-      $(`a.${INLINE_ONEBOX_LOADING_CSS_CLASS}`, $preview).each(
-        (index, link) => {
-          const $link = $(link);
-          $link.removeClass(INLINE_ONEBOX_LOADING_CSS_CLASS);
-          const text = $link.text();
-          inline[text] = inline[text] || [];
-          inline[text].push($link);
-        }
-      );
-      if (Object.keys(inline).length > 0) {
-        Ember.run.debounce(this, this._loadInlineOneboxes, inline, 450);
-      }
-
      if (this._enableAdvancedEditorPreviewSync()) {
        this._syncScroll(
          this._syncEditorAndPreviewScroll,
--- a/app/assets/javascripts/pretty-text/engines/discourse-markdown/onebox.js.es6
+++ b/app/assets/javascripts/pretty-text/engines/discourse-markdown/onebox.js.es6
@ -2,7 +2,8 @@ import { lookupCache } from "pretty-text/oneboxer";

 import {
  cachedInlineOnebox,
-  INLINE_ONEBOX_LOADING_CSS_CLASS
+  INLINE_ONEBOX_LOADING_CSS_CLASS,
+  INLINE_ONEBOX_CSS_CLASS
 } from "pretty-text/inline-oneboxer";

 const ONEBOX = 1;
@ -103,6 +104,7 @@ function applyOnebox(state, silent) {

            if (onebox && onebox.title) {
              text.content = onebox.title;
+              attrs.push(["class", INLINE_ONEBOX_CSS_CLASS]);
            } else {
              attrs.push(["class", INLINE_ONEBOX_LOADING_CSS_CLASS]);
            }
--- a/app/assets/javascripts/pretty-text/inline-oneboxer.js.es6.erb
+++ b/app/assets/javascripts/pretty-text/inline-oneboxer.js.es6.erb
@ -3,6 +3,9 @@ let _cache = {};
 export const INLINE_ONEBOX_LOADING_CSS_CLASS =
  "<%= CookedPostProcessor::INLINE_ONEBOX_LOADING_CSS_CLASS %>";

+export const INLINE_ONEBOX_CSS_CLASS =
+  "<%= CookedPostProcessor::INLINE_ONEBOX_CSS_CLASS %>";
+
 export function applyInlineOneboxes(inline, ajax) {
  Object.keys(inline).forEach(url => {
    // cache a blank locally, so we never trigger a lookup
@ -17,7 +20,9 @@ export function applyInlineOneboxes(inline, ajax) {
        _cache[onebox.url] = onebox;
        let links = inline[onebox.url] || [];
        links.forEach(link => {
-          link.text(onebox.title);
+          $(link).text(onebox.title)
+            .addClass(INLINE_ONEBOX_CSS_CLASS)
+            .removeClass(INLINE_ONEBOX_LOADING_CSS_CLASS);
        });
      }
    });
--- a/app/assets/javascripts/pretty-text/oneboxer.js.es6
+++ b/app/assets/javascripts/pretty-text/oneboxer.js.es6
@ -1,7 +1,15 @@
 let timeout;
 const loadingQueue = [];
-const localCache = {};
-const failedCache = {};
+let localCache = {};
+let failedCache = {};
+
+export const LOADING_ONEBOX_CSS_CLASS = "loading-onebox";
+
+export function resetCache() {
+  loadingQueue.clear();
+  localCache = {};
+  failedCache = {};
+}

 function resolveSize(img) {
  $(img).addClass("size-resolved");
@ -76,7 +84,7 @@ function loadNext(ajax) {
    .finally(() => {
      timeout = Ember.run.later(() => loadNext(ajax), timeoutMs);
      if (removeLoading) {
-        $elem.removeClass("loading-onebox");
+        $elem.removeClass(LOADING_ONEBOX_CSS_CLASS);
        $elem.data("onebox-loaded");
      }
    });
@ -96,7 +104,7 @@ export function load({

  // If the onebox has loaded or is loading, return
  if ($elem.data("onebox-loaded")) return;
-  if ($elem.hasClass("loading-onebox")) return;
+  if ($elem.hasClass(LOADING_ONEBOX_CSS_CLASS)) return;

  const url = elem.href;

@ -112,7 +120,7 @@ export function load({
  }

  // Add the loading CSS class
-  $elem.addClass("loading-onebox");
+  $elem.addClass(LOADING_ONEBOX_CSS_CLASS);

  // Add to the loading queue
  loadingQueue.push({ url, refresh, $elem, categoryId, topicId });
--- a/app/assets/javascripts/pretty-text/white-lister.js.es6
+++ b/app/assets/javascripts/pretty-text/white-lister.js.es6
@ -1,4 +1,7 @@
-import { INLINE_ONEBOX_LOADING_CSS_CLASS } from "pretty-text/inline-oneboxer";
+import {
+  INLINE_ONEBOX_CSS_CLASS,
+  INLINE_ONEBOX_LOADING_CSS_CLASS
+} from "pretty-text/inline-oneboxer";

 // to match:
 // abcd
@ -118,6 +121,7 @@ const DEFAULT_LIST = [
  "a.mention",
  "a.mention-group",
  "a.onebox",
+  `a.${INLINE_ONEBOX_CSS_CLASS}`,
  `a.${INLINE_ONEBOX_LOADING_CSS_CLASS}`,
  "a[data-bbcode]",
  "a[name]",
--- a/lib/cooked_post_processor.rb
+++ b/lib/cooked_post_processor.rb
@ -8,6 +8,9 @@ require_dependency 'quote_comparer'
 class CookedPostProcessor
  include ActionView::Helpers::NumberHelper

+  INLINE_ONEBOX_LOADING_CSS_CLASS = "inline-onebox-loading"
+  INLINE_ONEBOX_CSS_CLASS = "inline-onebox"
+
  attr_reader :cooking_options, :doc

  def initialize(post, opts = {})
@ -30,7 +33,6 @@ class CookedPostProcessor
    DistributedMutex.synchronize("post_process_#{@post.id}") do
      DiscourseEvent.trigger(:before_post_process_cooked, @doc, @post)
      post_process_oneboxes
-      post_process_inline_oneboxes
      post_process_images
      post_process_quotes
      optimize_urls
@ -435,13 +437,35 @@ class CookedPostProcessor
  end

  def post_process_oneboxes
-    Oneboxer.apply(@doc) do |url|
-      @has_oneboxes = true
-      Oneboxer.onebox(url,
-        invalidate_oneboxes: !!@opts[:invalidate_oneboxes],
-        user_id: @post&.user_id,
-        category_id: @post&.topic&.category_id
-      )
+    limit = SiteSetting.max_oneboxes_per_post
+    oneboxes = {}
+    inlineOneboxes = {}
+
+    Oneboxer.apply(@doc, extra_paths: [".#{INLINE_ONEBOX_LOADING_CSS_CLASS}"]) do |url, element|
+      is_onebox = element["class"] == Oneboxer::ONEBOX_CSS_CLASS
+      map = is_onebox ? oneboxes : inlineOneboxes
+      skip_onebox = limit <= 0 && !map[url]
+
+      if skip_onebox
+        remove_inline_onebox_loading_class(element) unless is_onebox
+        next
+      end
+
+      limit -= 1
+      map[url] = true
+
+      if is_onebox
+        @has_oneboxes = true
+
+        Oneboxer.onebox(url,
+          invalidate_oneboxes: !!@opts[:invalidate_oneboxes],
+          user_id: @post&.user_id,
+          category_id: @post&.topic&.category_id
+        )
+      else
+        process_inline_onebox(element)
+        false
+      end
    end

    oneboxed_images.each do |img|
@ -577,23 +601,24 @@ class CookedPostProcessor
    @doc.try(:to_html)
  end

-  INLINE_ONEBOX_LOADING_CSS_CLASS = "inline-onebox-loading"
-
  private

-  def post_process_inline_oneboxes
-    @doc.css(".#{INLINE_ONEBOX_LOADING_CSS_CLASS}").each do |element|
-      inline_onebox = InlineOneboxer.lookup(
-        element.attributes["href"].value,
-        invalidate: !!@opts[:invalidate_oneboxes]
-      )
+  def process_inline_onebox(element)
+    inline_onebox = InlineOneboxer.lookup(
+      element.attributes["href"].value,
+      invalidate: !!@opts[:invalidate_oneboxes]
+    )

-      if title = inline_onebox&.dig(:title)
-        element.children = title
-      end
-
-      element.remove_class(INLINE_ONEBOX_LOADING_CSS_CLASS)
+    if title = inline_onebox&.dig(:title)
+      element.children = title
+      element.add_class(INLINE_ONEBOX_CSS_CLASS)
    end
+
+    remove_inline_onebox_loading_class(element)
+  end
+
+  def remove_inline_onebox_loading_class(element)
+    element.remove_class(INLINE_ONEBOX_LOADING_CSS_CLASS)
  end

  def is_svg?(img)
--- a/lib/oneboxer.rb
+++ b/lib/oneboxer.rb
@ -5,6 +5,8 @@ require_dependency 'final_destination'
 Dir["#{Rails.root}/lib/onebox/engine/*_onebox.rb"].sort.each { |f| require f }

 module Oneboxer
+  ONEBOX_CSS_CLASS = "onebox"
+
  # keep reloaders happy
  unless defined? Oneboxer::Result
    Result = Struct.new(:doc, :changed) do
@ -67,11 +69,11 @@ module Oneboxer
  end

  # Parse URLs out of HTML, returning the document when finished.
-  def self.each_onebox_link(string_or_doc)
+  def self.each_onebox_link(string_or_doc, extra_paths: [])
    doc = string_or_doc
    doc = Nokogiri::HTML::fragment(doc) if doc.is_a?(String)

-    onebox_links = doc.search("a.onebox")
+    onebox_links = doc.css("a.#{ONEBOX_CSS_CLASS}", *extra_paths)
    if onebox_links.present?
      onebox_links.each do |link|
        yield(link['href'], link) if link['href'].present?
@ -83,13 +85,14 @@ module Oneboxer

  HTML5_BLOCK_ELEMENTS ||= %w{address article aside blockquote canvas center dd div dl dt fieldset figcaption figure footer form h1 h2 h3 h4 h5 h6 header hgroup hr li main nav noscript ol output p pre section table tfoot ul video}

-  def self.apply(string_or_doc, args = nil)
+  def self.apply(string_or_doc, extra_paths: nil)
    doc = string_or_doc
    doc = Nokogiri::HTML::fragment(doc) if doc.is_a?(String)
    changed = false

-    each_onebox_link(doc) do |url, element|
+    each_onebox_link(doc, extra_paths: extra_paths) do |url, element|
      onebox, _ = yield(url, element)
+
      if onebox
        parsed_onebox = Nokogiri::HTML::fragment(onebox)
        next unless parsed_onebox.children.count > 0
--- a/spec/components/cooked_post_processor_spec.rb
+++ b/spec/components/cooked_post_processor_spec.rb
@ -20,7 +20,6 @@ describe CookedPostProcessor do

    it "post process in sequence" do
      cpp.expects(:post_process_oneboxes).in_sequence(post_process)
-      cpp.expects(:post_process_inline_oneboxes).in_sequence(post_process)
      cpp.expects(:post_process_images).in_sequence(post_process)
      cpp.expects(:optimize_urls).in_sequence(post_process)
      cpp.expects(:pull_hotlinked_images).in_sequence(post_process)
@ -29,6 +28,87 @@ describe CookedPostProcessor do
      expect(PostUpload.exists?(post: post, upload: upload)).to eq(true)
    end

+    describe 'when post contains oneboxes and inline oneboxes' do
+      let(:url_hostname) { 'meta.discourse.org' }
+
+      let(:url) do
+        "https://#{url_hostname}/t/mini-inline-onebox-support-rfc/66400"
+      end
+
+      let(:title) { 'some title' }
+
+      let(:post) do
+        Fabricate(:post, raw: <<~RAW)
+        #{url}
+        This is a #{url} with path
+
+        https://#{url_hostname}/t/random-url
+
+        This is a https://#{url_hostname}/t/another-random-url test
+        This is a #{url} with path
+
+        #{url}
+        RAW
+      end
+
+      before do
+        SiteSetting.enable_inline_onebox_on_all_domains = true
+
+        %i{head get}.each do |method|
+          stub_request(method, url).to_return(
+            status: 200,
+            body: <<~RAW
+            <html>
+              <head>
+                <title>#{title}</title>
+                <meta property='og:title' content="#{title}">
+                <meta property='og:description' content="some description">
+              </head>
+            </html>
+            RAW
+          )
+        end
+      end
+
+      after do
+        InlineOneboxer.purge(url)
+        Oneboxer.invalidate(url)
+      end
+
+      it 'should respect SiteSetting.max_oneboxes_per_post' do
+        SiteSetting.max_oneboxes_per_post = 2
+        SiteSetting.add_rel_nofollow_to_user_content = false
+
+        cpp.post_process
+
+        expect(cpp.html).to have_tag('a',
+          with: {
+            href: url,
+            class: described_class::INLINE_ONEBOX_CSS_CLASS
+          },
+          text: title,
+          count: 2
+        )
+
+        expect(cpp.html).to have_tag('aside.onebox a', text: title, count: 2)
+
+        expect(cpp.html).to have_tag('aside.onebox a',
+          text: url_hostname,
+          count: 2
+        )
+
+        expect(cpp.html).to have_tag('a',
+          without: {
+            class: described_class::INLINE_ONEBOX_LOADING_CSS_CLASS
+          },
+          text: "https://#{url_hostname}/t/another-random-url",
+          count: 1
+        )
+
+        expect(cpp.html).to have_tag('a.onebox', count: 1)
+      end
+    end
+
    describe 'when post contains inline oneboxes' do
      let(:loading_css_class) do
        described_class::INLINE_ONEBOX_LOADING_CSS_CLASS
--- a/test/javascripts/acceptance/composer-onebox-test.js.es6
+++ b/test/javascripts/acceptance/composer-onebox-test.js.es6
@ -0,0 +1,47 @@
+import { acceptance } from "helpers/qunit-helpers";
+import { INLINE_ONEBOX_CSS_CLASS } from "pretty-text/inline-oneboxer";
+
+acceptance("Composer - Onebox", {
+  loggedIn: true,
+  settings: {
+    max_oneboxes_per_post: 2,
+    enable_markdown_linkify: true
+  }
+});
+
+QUnit.test(
+  "Preview update should respect max_oneboxes_per_post site setting",
+  async assert => {
+    await visit("/t/internationalization-localization/280");
+    await click("#topic-footer-buttons .btn.create");
+
+    await fillIn(
+      ".d-editor-input",
+      `
+http://www.example.com/has-title.html
+This is another test http://www.example.com/has-title.html
+
+http://www.example.com/no-title.html
+
+This is another test http://www.example.com/no-title.html
+This is another test http://www.example.com/has-title.html
+
+http://www.example.com/has-title.html
+      `
+    );
+
+    assert.equal(
+      find(".d-editor-preview:visible")
+        .html()
+        .trim(),
+      `
+<p><aside class=\"onebox\"><article class=\"onebox-body\"><h3><a href=\"http://www.example.com/article.html\">An interesting article</a></h3></article></aside><br>
+This is another test <a href=\"http://www.example.com/has-title.html\" class=\"${INLINE_ONEBOX_CSS_CLASS}\">This is a great title</a></p>
+<p><a href=\"http://www.example.com/no-title.html\" class=\"onebox\" target=\"_blank\">http://www.example.com/no-title.html</a></p>
+<p>This is another test <a href=\"http://www.example.com/no-title.html\" class=\"\">http://www.example.com/no-title.html</a><br>
+This is another test <a href=\"http://www.example.com/has-title.html\" class=\"${INLINE_ONEBOX_CSS_CLASS}\">This is a great title</a></p>
+<p><aside class=\"onebox\"><article class=\"onebox-body\"><h3><a href=\"http://www.example.com/article.html\">An interesting article</a></h3></article></aside></p>
+      `.trim()
+    );
+  }
+);
--- a/test/javascripts/helpers/create-pretender.js.es6
+++ b/test/javascripts/helpers/create-pretender.js.es6
@ -532,6 +532,20 @@ export default function() {
      });
    });

+    this.get("/inline-onebox", request => {
+      if (
+        request.queryParams.urls.includes(
+          "http://www.example.com/has-title.html"
+        )
+      ) {
+        return [
+          200,
+          { "Content-Type": "application/html" },
+          '{"inline-oneboxes":[{"url":"http://www.example.com/has-title.html","title":"This is a great title"}]}'
+        ];
+      }
+    });
+
    this.get("/onebox", request => {
      if (
        request.queryParams.url === "http://www.example.com/has-title.html" ||
--- a/test/javascripts/helpers/qunit-helpers.js.es6
+++ b/test/javascripts/helpers/qunit-helpers.js.es6
@ -13,6 +13,7 @@ import { flushMap } from "discourse/models/store";
 import { clearRewrites } from "discourse/lib/url";
 import { initSearchData } from "discourse/widgets/search-menu";
 import { resetDecorators } from "discourse/widgets/widget";
+import { resetCache as resetOneboxCache } from "pretty-text/oneboxer";
 import { resetCustomPostMessageCallbacks } from "discourse/controllers/topic";

 export function currentUser() {
@ -122,6 +123,7 @@ export function acceptance(name, options) {
      clearRewrites();
      initSearchData();
      resetDecorators();
+      resetOneboxCache();
      resetCustomPostMessageCallbacks();
      Discourse.reset();
    }