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

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.

From a1e77aa2edb8578324606793986d813b48b2e104 Mon Sep 17 00:00:00 2001
From: Guo Xiang Tan <gxtan1990@gmail.com>
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.

diff --git a/app/assets/javascripts/discourse/components/composer-editor.js.es6 b/app/assets/javascripts/discourse/components/composer-editor.js.es6
index 5195bdb..8417e95 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,30 +993,57 @@ 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;
 
-      // Short upload urls need resolution
-      resolveAllShortUrls(ajax);
+          $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--;
+                }
 
-      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);
-        }
+                map[text].push(link);
+              }
+            });
+
+          if (Object.keys(oneboxes).length > 0) {
+            this._loadOneboxes(oneboxes);
+          }
+
+          if (Object.keys(inlineOneboxes).length > 0) {
+            this._loadInlineOneboxes(inlineOneboxes);
+          }
+        },
+        450
       );
-      if (Object.keys(inline).length > 0) {
-        Ember.run.debounce(this, this._loadInlineOneboxes, inline, 450);
-      }
+      // Short upload urls need resolution
+      resolveAllShortUrls(ajax);
 
       if (this._enableAdvancedEditorPreviewSync()) {
         this._syncScroll(
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 f437988..c367f87 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 983549d..9f66c03 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 a7bd2b3..4753ec8 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(() => {
   

GitHub

2 Likes

hmmm so previously it would unconditionally resolve as loading and then we would apply the class in a decorator? now we resolve internal to the md engine?

Object.values(oneboxes).forEach(onebox =>
1 Like

technically once we are skipping shouldn’t we just strip off all 3 onebox related classes?

1 Like

Good catch resolved in

Looks like you fixed this in another commit :slight_smile:

Heh I’m actually not sure what the above means but what I changed here is to apply a css class to inline onebox links so that I can target them on the client side. In the future, we might also want to style inline onebox differently and that css class will be useful. Previously, it was just an <a> tag without any indication that the link has been converted into an inline onebox.

2 Likes