From 3841cd9a7f2b83d268a0190b1e54270198d2113b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Mon, 24 Oct 2016 12:46:22 +0200 Subject: [PATCH] FEATURE: onebox everything by default FEATURE: new 'max_oneboxes_per_post' site setting FEATURE: change onebox whitelist to a blacklist PERF: debounce the loading of oneboxes PERF: improve perf of mention links in preview FIX: sort loading of custom oneboxer --- .../components/composer-editor.js.es6 | 75 ++++++++++--------- .../discourse/lib/link-mentions.js.es6 | 51 ++++++------- .../javascripts/pretty-text/oneboxer.js.es6 | 17 ++--- config/locales/server.en.yml | 5 +- config/site_settings.yml | 5 +- lib/oneboxer.rb | 52 +++++-------- lib/site_setting_extension.rb | 7 +- spec/components/oneboxer_spec.rb | 2 + 8 files changed, 98 insertions(+), 116 deletions(-) diff --git a/app/assets/javascripts/discourse/components/composer-editor.js.es6 b/app/assets/javascripts/discourse/components/composer-editor.js.es6 index f16acd43b9a..79c7eb7d893 100644 --- a/app/assets/javascripts/discourse/components/composer-editor.js.es6 +++ b/app/assets/javascripts/discourse/components/composer-editor.js.es6 @@ -2,7 +2,7 @@ import userSearch from 'discourse/lib/user-search'; import { default as computed, on } from 'ember-addons/ember-computed-decorators'; import { linkSeenMentions, fetchUnseenMentions } from 'discourse/lib/link-mentions'; import { linkSeenCategoryHashtags, fetchUnseenCategoryHashtags } from 'discourse/lib/link-category-hashtags'; -import { fetchUnseenTagHashtags, linkSeenTagHashtags } from 'discourse/lib/link-tag-hashtag'; +import { linkSeenTagHashtags, fetchUnseenTagHashtags } from 'discourse/lib/link-tag-hashtag'; import { load } from 'pretty-text/oneboxer'; import { ajax } from 'discourse/lib/ajax'; import InputValidation from 'discourse/models/input-validation'; @@ -41,22 +41,6 @@ export default Ember.Component.extend({ return showPreview ? I18n.t('composer.hide_preview') : I18n.t('composer.show_preview'); }, - _renderUnseenTagHashtags($preview, unseen) { - fetchUnseenTagHashtags(unseen).then(() => { - linkSeenTagHashtags($preview); - }); - }, - - @on('previewRefreshed') - paintTagHashtags($preview) { - if (!this.siteSettings.tagging_enabled) { return; } - - const unseenTagHashtags = linkSeenTagHashtags($preview); - if (unseenTagHashtags.length) { - Ember.run.debounce(this, this._renderUnseenTagHashtags, $preview, unseenTagHashtags, 500); - } - }, - @computed markdownOptions() { return { @@ -152,19 +136,38 @@ export default Ember.Component.extend({ $preview.scrollTop(desired + 50); }, - _renderUnseenMentions: function($preview, unseen) { - fetchUnseenMentions($preview, unseen).then(() => { + _renderUnseenMentions($preview, unseen) { + fetchUnseenMentions(unseen).then(() => { linkSeenMentions($preview, this.siteSettings); this._warnMentionedGroups($preview); }); }, - _renderUnseenCategoryHashtags: function($preview, unseen) { + _renderUnseenCategoryHashtags($preview, unseen) { fetchUnseenCategoryHashtags(unseen).then(() => { linkSeenCategoryHashtags($preview); }); }, + _renderUnseenTagHashtags($preview, unseen) { + fetchUnseenTagHashtags(unseen).then(() => { + linkSeenTagHashtags($preview); + }); + }, + + _loadOneboxes($oneboxes) { + const post = this.get('composer.post'); + let refresh = false; + + // If we are editing a post, we'll refresh its contents once. + if (post && !post.get('refreshedPost')) { + refresh = true; + post.set('refreshedPost', true); + } + + $oneboxes.each((_, o) => load(o, refresh, ajax)); + }, + _warnMentionedGroups($preview) { Ember.run.scheduleOnce('afterRender', () => { this._warnedMentions = this._warnedMentions || []; @@ -481,31 +484,33 @@ export default Ember.Component.extend({ previewUpdated($preview) { // Paint mentions - const unseen = linkSeenMentions($preview, this.siteSettings); - if (unseen.length) { - Ember.run.debounce(this, this._renderUnseenMentions, $preview, unseen, 500); + const unseenMentions = linkSeenMentions($preview, this.siteSettings); + if (unseenMentions.length) { + Ember.run.debounce(this, this._renderUnseenMentions, $preview, unseenMentions, 450); } this._warnMentionedGroups($preview); // Paint category hashtags - const unseenHashtags = linkSeenCategoryHashtags($preview); - if (unseenHashtags.length) { - Ember.run.debounce(this, this._renderUnseenCategoryHashtags, $preview, unseenHashtags, 500); + const unseenCategoryHashtags = linkSeenCategoryHashtags($preview); + if (unseenCategoryHashtags.length) { + Ember.run.debounce(this, this._renderUnseenCategoryHashtags, $preview, unseenCategoryHashtags, 450); } - const post = this.get('composer.post'); - let refresh = false; - - // If we are editing a post, we'll refresh its contents once. This is a feature that - // allows a user to refresh its contents once. - if (post && !post.get('refreshedPost')) { - refresh = true; - post.set('refreshedPost', true); + // Paint tag hashtags + if (this.siteSettings.tagging_enabled) { + const unseenTagHashtags = linkSeenTagHashtags($preview); + if (unseenTagHashtags.length) { + Ember.run.debounce(this, this._renderUnseenTagHashtags, $preview, unseenTagHashtags, 450); + } } // Paint oneboxes - $('a.onebox', $preview).each((i, e) => load(e, refresh, ajax)); + 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); + } + this.trigger('previewRefreshed', $preview); this.sendAction('afterRefresh', $preview); }, diff --git a/app/assets/javascripts/discourse/lib/link-mentions.js.es6 b/app/assets/javascripts/discourse/lib/link-mentions.js.es6 index 149b151617b..d41bd7e5b9a 100644 --- a/app/assets/javascripts/discourse/lib/link-mentions.js.es6 +++ b/app/assets/javascripts/discourse/lib/link-mentions.js.es6 @@ -1,37 +1,34 @@ import { ajax } from 'discourse/lib/ajax'; + function replaceSpan($e, username, opts) { if (opts && opts.group) { - var extra = "", extraClass = ""; + let extra = ""; + let extraClass = ""; if (opts.mentionable) { - extra = " data-name='" + username + "' data-mentionable-user-count='" + opts.mentionable.user_count + "' "; - extraClass = " notify"; + extra = `data-name='${username}' data-mentionable-user-count='${opts.mentionable.user_count}'`; + extraClass = "notify"; } - $e.replaceWith("@" + username + ""); + $e.replaceWith(`@${username}`); } else { - $e.replaceWith("@" + username + ""); + $e.replaceWith(`@${username}`); } } -const found = []; -const foundGroups = []; -const mentionableGroups = []; -const checked = []; +const found = {}; +const foundGroups = {}; +const mentionableGroups = {}; +const checked = {}; function updateFound($mentions, usernames) { Ember.run.scheduleOnce('afterRender', function() { $mentions.each((i, e) => { const $e = $(e); const username = usernames[i]; - if (found.indexOf(username.toLowerCase()) !== -1) { + if (found[username.toLowerCase()]) { replaceSpan($e, username); - } else if (foundGroups.indexOf(username) !== -1) { - const mentionable = _(mentionableGroups).where({name: username}).first(); - replaceSpan($e, username, {group: true, mentionable: mentionable}); - } else if (checked.indexOf(username) !== -1) { + } else if (foundGroups[username]) { + replaceSpan($e, username, { group: true, mentionable: mentionableGroups[username] }); + } else if (checked[username]) { $e.addClass('mention-tested'); } }); @@ -42,22 +39,18 @@ export function linkSeenMentions($elem, siteSettings) { const $mentions = $('span.mention:not(.mention-tested)', $elem); if ($mentions.length) { const usernames = $mentions.map((_, e) => $(e).text().substr(1)); - const unseen = _.uniq(usernames).filter((u) => { - return u.length >= siteSettings.min_username_length && checked.indexOf(u) === -1; - }); updateFound($mentions, usernames); - return unseen; + return _.uniq(usernames).filter(u => !checked[u] && u.length >= siteSettings.min_username_length); } - return []; } -export function fetchUnseenMentions($elem, usernames) { - return ajax("/users/is_local_username", { data: { usernames } }).then(function(r) { - found.push.apply(found, r.valid); - foundGroups.push.apply(foundGroups, r.valid_groups); - mentionableGroups.push.apply(mentionableGroups, r.mentionable_groups); - checked.push.apply(checked, usernames); +export function fetchUnseenMentions(usernames) { + return ajax("/users/is_local_username", { data: { usernames } }).then(r => { + r.valid.forEach(v => found[v] = true); + r.valid_groups.forEach(vg => foundGroups[vg] = true); + r.mentionable_groups.forEach(mg => mentionableGroups[mg] = true); + usernames.forEach(u => checked[u] = true); return r; }); } diff --git a/app/assets/javascripts/pretty-text/oneboxer.js.es6 b/app/assets/javascripts/pretty-text/oneboxer.js.es6 index 31733ab0d54..90be7612c0e 100644 --- a/app/assets/javascripts/pretty-text/oneboxer.js.es6 +++ b/app/assets/javascripts/pretty-text/oneboxer.js.es6 @@ -1,19 +1,12 @@ -/** - A helper for looking up oneboxes and displaying them - - For now it only stores in a local Javascript Object, in future we can change it so it uses localStorage - or some other mechanism. -**/ - const localCache = {}; const failedCache = {}; -// Perform a lookup of a onebox based an anchor element. It will insert a loading -// indicator and remove it when the loading is complete or fails. +// Perform a lookup of a onebox based an anchor element. +// It will insert a loading indicator and remove it when the loading is complete or fails. export function load(e, refresh, ajax) { - var $elem = $(e); + const $elem = $(e); - // If the onebox has loaded, return + // If the onebox has loaded or is loading, return if ($elem.data('onebox-loaded')) return; if ($elem.hasClass('loading-onebox')) return; @@ -41,7 +34,7 @@ export function load(e, refresh, ajax) { }).then(html => { localCache[url] = html; $elem.replaceWith(html); - }, function() { + }, () => { failedCache[url] = true; }).finally(() => { $elem.removeClass('loading-onebox'); diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index ed6a271b533..d40510aea47 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -816,7 +816,7 @@ en: s3_config_warning: 'The server is configured to upload files to s3, but at least one the following setting is not set: s3_access_key_id, s3_secret_access_key or s3_upload_bucket. Go to the Site Settings and update the settings. See "How to set up image uploads to S3?" to learn more.' s3_backup_config_warning: 'The server is configured to upload backups to s3, but at least one the following setting is not set: s3_access_key_id, s3_secret_access_key or s3_backup_bucket. Go to the Site Settings and update the settings. See "How to set up image uploads to S3?" to learn more.' image_magick_warning: 'The server is configured to create thumbnails of large images, but ImageMagick is not installed. Install ImageMagick using your favorite package manager or download the latest release.' - failing_emails_warning: 'There are %{num_failed_jobs} email jobs that failed. Check your app.yml and ensure that the mail server settings are correct. See the failed jobs in Sidekiq.' + failing_emails_warning: 'There are %{num_failed_jobs} email jobs that failed. Check your app.yml and ensure that the mail server settings are correct. See the failed jobs in Sidekiq.' subfolder_ends_in_slash: "Your subfolder setup is incorrect; the DISCOURSE_RELATIVE_URL_ROOT ends in a slash." email_polling_errored_recently: one: "Email polling has generated an error in the past 24 hours. Look at the logs for more details." @@ -874,7 +874,8 @@ en: show_pinned_excerpt_mobile: "Show excerpt on pinned topics in mobile view." show_pinned_excerpt_desktop: "Show excerpt on pinned topics in desktop view." post_onebox_maxlength: "Maximum length of a oneboxed Discourse post in characters." - onebox_domains_whitelist: "A list of domains to allow oneboxing for; these domains should support OpenGraph or oEmbed. Test them at http://iframely.com/debug" + onebox_domains_blacklist: "A list of domains that will never be oneboxed." + max_oneboxes_per_post: "Maximum number of oneboxes in a post." logo_url: "The logo image at the top left of your site, should be a wide rectangle shape. If left blank site title text will be shown." digest_logo_url: "The alternate logo image used at the top of your site's email summary. Should be a wide rectangle shape. Should not be an SVG image. If left blank `logo_url` will be used." diff --git a/config/site_settings.yml b/config/site_settings.yml index 74f5116906d..f93bc567096 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -855,9 +855,12 @@ security: onebox: enable_flash_video_onebox: false post_onebox_maxlength: 500 - onebox_domains_whitelist: + onebox_domains_blacklist: default: '' type: list + max_oneboxes_per_post: + default: 50 + client: true spam: add_rel_nofollow_to_user_content: true diff --git a/lib/oneboxer.rb b/lib/oneboxer.rb index 8430d9ff98f..7e39244fdf0 100644 --- a/lib/oneboxer.rb +++ b/lib/oneboxer.rb @@ -1,11 +1,8 @@ - -Dir["#{Rails.root}/lib/onebox/engine/*_onebox.rb"].each {|f| +Dir["#{Rails.root}/lib/onebox/engine/*_onebox.rb"].sort.each {|f| require_dependency(f.split('/')[-3..-1].join('/')) } module Oneboxer - - # keep reloaders happy unless defined? Oneboxer::Result Result = Struct.new(:doc, :changed) do @@ -120,38 +117,29 @@ module Oneboxer end private - def self.onebox_cache_key(url) - "onebox__#{url}" - end - def self.add_discourse_whitelists - # Add custom domain whitelists - if SiteSetting.onebox_domains_whitelist.present? - domains = SiteSetting.onebox_domains_whitelist.split('|') - whitelist = Onebox::Engine::WhitelistedGenericOnebox.whitelist - whitelist.concat(domains) - whitelist.uniq! + def self.blank_onebox + { preview: "", onebox: "" } end - end - def self.onebox_raw(url) - Rails.cache.fetch(onebox_cache_key(url), expires_in: 1.day){ - # This might be able to move to whenever the SiteSetting changes? - Oneboxer.add_discourse_whitelists + def self.onebox_cache_key(url) + "onebox__#{url}" + end - r = Onebox.preview(url, cache: {}, max_width: 695) - { - onebox: r.to_s, - preview: r.try(:placeholder_html).to_s - } - } - rescue => e - # no point warning here, just cause we have an issue oneboxing a url - # we can later hunt for failed oneboxes by searching logs if needed - Rails.logger.info("Failed to onebox #{url} #{e} #{e.backtrace}") + def self.onebox_raw(url) + Rails.cache.fetch(onebox_cache_key(url), expires_in: 1.day) do + uri = URI(url) rescue nil + return blank_onebox if uri.blank? || SiteSetting.onebox_domains_blacklist.include?(uri.hostname) - # return a blank hash, so rest of the code works - {preview: "", onebox: ""} - end + r = Onebox.preview(url, cache: {}, max_width: 695) + { onebox: r.to_s, preview: r.try(:placeholder_html).to_s } + end + rescue => e + # no point warning here, just cause we have an issue oneboxing a url + # we can later hunt for failed oneboxes by searching logs if needed + Rails.logger.info("Failed to onebox #{url} #{e} #{e.backtrace}") + # return a blank hash, so rest of the code works + blank_onebox + end end diff --git a/lib/site_setting_extension.rb b/lib/site_setting_extension.rb index 65ee570e8d1..3fb3b01550a 100644 --- a/lib/site_setting_extension.rb +++ b/lib/site_setting_extension.rb @@ -348,12 +348,9 @@ module SiteSettingExtension end def filter_value(name, value) - # filter domain name - if %w[disabled_image_download_domains onebox_domains_whitelist exclude_rel_nofollow_domains email_domains_blacklist email_domains_whitelist white_listed_spam_host_domains].include? name + if %w[disabled_image_download_domains onebox_domains_blacklist exclude_rel_nofollow_domains email_domains_blacklist email_domains_whitelist white_listed_spam_host_domains].include? name domain_array = [] - value.split('|').each { |url| - domain_array.push(get_hostname(url)) - } + value.split('|').each { |url| domain_array << get_hostname(url) } value = domain_array.join("|") end value diff --git a/spec/components/oneboxer_spec.rb b/spec/components/oneboxer_spec.rb index 7714d3f2c25..1df8b30027f 100644 --- a/spec/components/oneboxer_spec.rb +++ b/spec/components/oneboxer_spec.rb @@ -2,9 +2,11 @@ require 'rails_helper' require_dependency 'oneboxer' describe Oneboxer do + it "returns blank string for an invalid onebox" do expect(Oneboxer.preview("http://boom.com")).to eq("") expect(Oneboxer.onebox("http://boom.com")).to eq("") end + end