From a1e77aa2edb8578324606793986d813b48b2e104 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Tue, 27 Nov 2018 16:00:31 +0800 Subject: [PATCH] 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. --- .../components/composer-editor.js.es6 | 100 ++++++++++++------ .../engines/discourse-markdown/onebox.js.es6 | 4 +- .../pretty-text/inline-oneboxer.js.es6.erb | 7 +- .../javascripts/pretty-text/oneboxer.js.es6 | 18 +++- .../pretty-text/white-lister.js.es6 | 6 +- lib/cooked_post_processor.rb | 67 ++++++++---- lib/oneboxer.rb | 11 +- spec/components/cooked_post_processor_spec.rb | 82 +++++++++++++- .../acceptance/composer-onebox-test.js.es6 | 47 ++++++++ .../helpers/create-pretender.js.es6 | 14 +++ test/javascripts/helpers/qunit-helpers.js.es6 | 2 + 11 files changed, 291 insertions(+), 67 deletions(-) create mode 100644 test/javascripts/acceptance/composer-onebox-test.js.es6 diff --git a/app/assets/javascripts/discourse/components/composer-editor.js.es6 b/app/assets/javascripts/discourse/components/composer-editor.js.es6 index 5195bdb0c73..8417e9579cc 100644 --- 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, diff --git a/app/assets/javascripts/pretty-text/engines/discourse-markdown/onebox.js.es6 b/app/assets/javascripts/pretty-text/engines/discourse-markdown/onebox.js.es6 index f43798897a4..c367f87ed3b 100644 --- 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]); } diff --git a/app/assets/javascripts/pretty-text/inline-oneboxer.js.es6.erb b/app/assets/javascripts/pretty-text/inline-oneboxer.js.es6.erb index 983549dc4e0..9f66c03e742 100644 --- 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); }); } }); diff --git a/app/assets/javascripts/pretty-text/oneboxer.js.es6 b/app/assets/javascripts/pretty-text/oneboxer.js.es6 index a7bd2b3ab33..4753ec80158 100644 --- 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 }); diff --git a/app/assets/javascripts/pretty-text/white-lister.js.es6 b/app/assets/javascripts/pretty-text/white-lister.js.es6 index 4e1ed92f01c..c303283e360 100644 --- 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]", diff --git a/lib/cooked_post_processor.rb b/lib/cooked_post_processor.rb index f503149f151..8e8199ca430 100644 --- 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) diff --git a/lib/oneboxer.rb b/lib/oneboxer.rb index db0a4da32ca..f2781dd5a59 100644 --- 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 diff --git a/spec/components/cooked_post_processor_spec.rb b/spec/components/cooked_post_processor_spec.rb index fe0fcb45091..83d6714bfe8 100644 --- 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 + + + #{title} + + + + + 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 diff --git a/test/javascripts/acceptance/composer-onebox-test.js.es6 b/test/javascripts/acceptance/composer-onebox-test.js.es6 new file mode 100644 index 00000000000..70f529eac9f --- /dev/null +++ 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(), + ` +


+This is another test This is a great title

+

http://www.example.com/no-title.html

+

This is another test http://www.example.com/no-title.html
+This is another test This is a great title

+

+ `.trim() + ); + } +); diff --git a/test/javascripts/helpers/create-pretender.js.es6 b/test/javascripts/helpers/create-pretender.js.es6 index 09ecf38ed10..20f8acced50 100644 --- 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" || diff --git a/test/javascripts/helpers/qunit-helpers.js.es6 b/test/javascripts/helpers/qunit-helpers.js.es6 index ca775770a74..7799e087bc4 100644 --- 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(); }