FEATURE: Update topic/comment embedding parameters (#20181)

This commit implements many changes to topic and comments embedding. It
deprecates the class_name field from EmbeddableHost and suggests using
the className parameter. discourse_username parameter has been
deprecated and it will fetch it from embedded site from the author or
discourse-username meta.

See the updated code sample from Admin > Customize > Embedding page.

* FEATURE: Add className parameter for Discourse embed

* DEV: Hide class_name from EmbeddableHost

* DEV: Deprecate class_name field of EmbeddableHost

* FEATURE: Use either author or discourse-username meta tag

* DEV: Deprecate discourse_username parameter

* DEV: Improve embed code sample

app/assets/javascripts/admin/addon/components/embeddable-host.hbs

@@ -9,15 +9,6 @@
       autofocus={{true}}
     />
   </td>
-  <td class="editing-input">
-    <div class="label">{{i18n "admin.embedding.class_name"}}</div>
-    <Input
-      @value={{this.buffered.class_name}}
-      placeholder="class"
-      @enter={{action "save"}}
-      class="class-name"
-    />
-  </td>
   <td class="editing-input">
     <div class="label">{{i18n "admin.embedding.allowed_paths"}}</div>
     <Input
@@ -54,12 +45,6 @@
     <div class="label">{{i18n "admin.embedding.host"}}</div>
     {{this.host.host}}
   </td>
-  <td>
-    <div class="label">
-      {{i18n "admin.embedding.class_name"}}
-    </div>
-    {{this.host.class_name}}
-  </td>
   <td>
     <div class="label">
       {{i18n "admin.embedding.allowed_paths"}}

app/assets/javascripts/admin/addon/controllers/admin-embedding.js

@@ -16,10 +16,14 @@ export default Controller.extend({
   @discourseComputed("embedding.base_url")
   embeddingCode(baseUrl) {
     const html = `<div id='discourse-comments'></div>
+<meta name='discourse-username' content='DISCOURSE_USERNAME'>
 
 <script type="text/javascript">
-  DiscourseEmbed = { discourseUrl: '${baseUrl}/',
-                     discourseEmbedUrl: 'REPLACE_ME' };
+  DiscourseEmbed = {
+    discourseUrl: '${baseUrl}/',
+    discourseEmbedUrl: 'EMBED_URL',
+    // className: 'CLASS_NAME',
+  };
 
   (function() {
     var d = document.createElement('script'); d.type = 'text/javascript'; d.async = true;

app/assets/javascripts/admin/addon/templates/embedding.hbs

@@ -2,10 +2,9 @@
   {{#if this.embedding.embeddable_hosts}}
     <table class="embedding grid">
       <thead>
-        <th style="width: 25%">{{i18n "admin.embedding.host"}}</th>
-        <th style="width: 15%">{{i18n "admin.embedding.class_name"}}</th>
-        <th style="width: 25%">{{i18n "admin.embedding.allowed_paths"}}</th>
-        <th style="width: 25%">{{i18n "admin.embedding.category"}}</th>
+        <th style="width: 30%">{{i18n "admin.embedding.host"}}</th>
+        <th style="width: 30%">{{i18n "admin.embedding.allowed_paths"}}</th>
+        <th style="width: 30%">{{i18n "admin.embedding.category"}}</th>
         <th style="width: 10%">&nbsp;</th>
       </thead>
       <tbody>
@@ -28,7 +27,7 @@
 
 {{#if this.showSecondary}}
   <div class="embedding-secondary">
-    <p>{{html-safe (i18n "admin.embedding.sample")}}</p>
+    {{html-safe (i18n "admin.embedding.sample")}}
     <HighlightedCode @code={{this.embeddingCode}} @lang="html" />
   </div>
 

app/controllers/admin/embeddable_hosts_controller.rb

@@ -25,7 +25,6 @@ class Admin::EmbeddableHostsController < Admin::AdminController
   def save_host(host, action)
     host.host = params[:embeddable_host][:host]
     host.allowed_paths = params[:embeddable_host][:allowed_paths]
-    host.class_name = params[:embeddable_host][:class_name]
     host.category_id = params[:embeddable_host][:category_id]
     host.category_id = SiteSetting.uncategorized_category_id if host.category_id.blank?
 

app/controllers/embed_controller.rb

@@ -160,14 +160,30 @@ class EmbedController < ApplicationController
 
   def prepare_embeddable
     response.headers.delete("X-Frame-Options")
-    @embeddable_css_class = ""
-    embeddable_host = EmbeddableHost.record_for_url(request.referer)
-    @embeddable_css_class =
-      " class=\"#{embeddable_host.class_name}\"" if embeddable_host.present? &&
-      embeddable_host.class_name.present?
 
-    @data_referer = request.referer
-    @data_referer = "*" if SiteSetting.embed_any_origin? && @data_referer.blank?
+    embeddable_host = EmbeddableHost.record_for_url(request.referer)
+
+    @embeddable_css_class =
+      if params[:class_name]
+        " class=\"#{CGI.escapeHTML(params[:class_name])}\""
+      elsif embeddable_host.present? && embeddable_host.class_name.present?
+        Discourse.deprecate(
+          "class_name field of EmbeddableHost has been deprecated. Prefer passing class_name as a parameter.",
+          since: "3.1.0.beta1",
+          drop_from: "3.2",
+        )
+
+        " class=\"#{CGI.escapeHTML(embeddable_host.class_name)}\""
+      else
+        ""
+      end
+
+    @data_referer =
+      if SiteSetting.embed_any_origin? && @data_referer.blank?
+        "*"
+      else
+        request.referer
+      end
   end
 
   def ensure_api_request

app/models/topic_embed.rb

@@ -145,7 +145,8 @@ class TopicEmbed < ActiveRecord::Base
     end
 
     raw_doc = Nokogiri.HTML5(html)
-    auth_element = raw_doc.at('meta[@name="author"]')
+    auth_element =
+      raw_doc.at('meta[@name="discourse-username"]') || raw_doc.at('meta[@name="author"]')
     if auth_element.present?
       response.author = User.where(username_lower: auth_element[:content].strip).first
     end
@@ -202,12 +203,13 @@ class TopicEmbed < ActiveRecord::Base
     response
   end
 
-  def self.import_remote(import_user, url, opts = nil)
+  def self.import_remote(url, opts = nil)
     opts = opts || {}
     response = find_remote(url)
     return if response.nil?
 
     response.title = opts[:title] if opts[:title].present?
+    import_user = opts[:user] if opts[:user].present?
     import_user = response.author if response.author.present?
 
     TopicEmbed.import(import_user, url, response.title, response.body)

config/locales/client.en.yml

@@ -6113,10 +6113,14 @@ en:
       embedding:
         get_started: "If you'd like to embed Discourse on another website, begin by adding its host."
         confirm_delete: "Are you sure you want to delete that host?"
-        sample: "Paste the following HTML code into your site to create and embed Discourse topics. Replace <b>REPLACE_ME</b> with the canonical URL of the page you are embedding it on."
+        sample: |
+          <p>Paste the following HTML code into your site to create and embed Discourse topics. Replace <b>EMBED_URL</b> with the canonical URL of the page you are embedding it on.</p>
+
+          <p>If you want to customize the style, uncomment and replace <b>CLASS_NAME</b> with a CSS class defined in the <i>Embedded CSS</i> of your theme.</p>
+
+          <p>Replace <b>DISCOURSE_USERNAME</b> with the Discourse username of the author that should create the topic. Discourse will automatically lookup the user by the <code>content</code> attribute of the <code>&lt;meta&gt;</code> tags with <code>name</code> attribute set to <code>discourse-username</code> or <code>author</code>. The <code>discourseUserName</code> parameter has been deprecated and will be removed in Discourse 3.2.</p>
         title: "Embedding"
         host: "Allowed Hosts"
-        class_name: "Class Name"
         allowed_paths: "Path Allowlist"
         edit: "edit"
         category: "Post to Category"

lib/topic_retriever.rb

@@ -4,7 +4,6 @@ class TopicRetriever
   def initialize(embed_url, opts = nil)
     @embed_url = embed_url
     @opts = opts || {}
-    @author_username = @opts[:author_username]
   end
 
   def retrieve
@@ -35,21 +34,18 @@ class TopicRetriever
     # It's possible another process or job found the embed already. So if that happened bail out.
     return if TopicEmbed.where(embed_url: @embed_url).exists?
 
-    fetch_http
+    if @opts[:author_username].present?
+      Discourse.deprecate(
+        "discourse_username parameter has been deprecated. Prefer passing author name using a <meta> tag.",
+        since: "3.1.0.beta1",
+        drop_from: "3.2",
+      )
     end
 
-  def fetch_http
-    if @author_username.nil?
     username =
-        SiteSetting.embed_by_username.presence || SiteSetting.site_contact_username.presence ||
-          Discourse.system_user.username
-    else
-      username = @author_username
-    end
+      @opts[:author_username].present || SiteSetting.embed_by_username.presence ||
+        SiteSetting.site_contact_username.presence || Discourse.system_user.username
 
-    user = User.where(username_lower: username.downcase).first
-    return if user.blank?
-
-    TopicEmbed.import_remote(user, @embed_url)
+    TopicEmbed.import_remote(@embed_url, user: User.find_by(username_lower: username.downcase))
   end
 end

public/javascripts/embed.js

@@ -34,6 +34,10 @@
     queryParams.topic_id = DE.topicId;
   }
 
+  if (DE.className) {
+    queryParams.class_name = DE.className;
+  }
+
   var src = DE.discourseUrl + "embed/comments";
   var keys = Object.keys(queryParams);
   if (keys.length > 0) {

script/import_scripts/disqus.rb

@@ -64,7 +64,7 @@ class ImportScripts::Disqus < ImportScripts::Base
 
       topic_user = find_existing_user(t[:author_email], t[:author_username])
       begin
-        post = TopicEmbed.import_remote(topic_user, t[:link], title: title)
+        post = TopicEmbed.import_remote(t[:link], title: title, user: topic_user)
         post.topic.update_column(:category_id, @category.id)
       rescue OpenURI::HTTPError
         post = nil

spec/lib/topic_retriever_spec.rb

@@ -2,8 +2,7 @@
 
 RSpec.describe TopicRetriever do
   let(:embed_url) { "http://eviltrout.com/2013/02/10/why-discourse-uses-emberjs.html" }
-  let(:author_username) { "eviltrout" }
-  let(:topic_retriever) { TopicRetriever.new(embed_url, author_username: author_username) }
+  let(:topic_retriever) { TopicRetriever.new(embed_url) }
 
   it "can initialize without optional parameters" do
     t = TopicRetriever.new(embed_url)

spec/requests/admin/embeddable_hosts_controller_spec.rb

@@ -56,7 +56,6 @@ RSpec.describe Admin::EmbeddableHostsController do
             params: {
               embeddable_host: {
                 host: "test.com",
-                class_name: "test-class",
                 category_id: category.id,
               },
             }
@@ -67,7 +66,7 @@ RSpec.describe Admin::EmbeddableHostsController do
           UserHistory.where(
             acting_user_id: admin.id,
             action: UserHistory.actions[:embeddable_host_update],
-            new_value: "category_id: #{category.id}, class_name: test-class, host: test.com",
+            new_value: "category_id: #{category.id}, host: test.com",
           ).exists?
 
         expect(history_exists).to eq(true)
@@ -82,7 +81,6 @@ RSpec.describe Admin::EmbeddableHostsController do
             params: {
               embeddable_host: {
                 host: "test.com",
-                class_name: "test-class",
                 category_id: category.id,
               },
             }

spec/requests/embed_controller_spec.rb

@@ -5,32 +5,13 @@ RSpec.describe EmbedController do
   let(:embed_url_secure) { "https://eviltrout.com/2013/02/10/why-discourse-uses-emberjs.html" }
   let(:discourse_username) { "eviltrout" }
 
-  it "is 404 without an embed_url" do
-    get "/embed/comments"
-    expect(response.body).to match(I18n.t("embed.error"))
-  end
-
-  it "raises an error with a missing host" do
-    get "/embed/comments", params: { embed_url: embed_url }
-    expect(response.body).to match(I18n.t("embed.error"))
-  end
-
-  describe "by topic id" do
-    let(:headers) { { "REFERER" => "http://eviltrout.com/some-page" } }
-
-    before { Fabricate(:embeddable_host) }
-
-    it "allows a topic to be embedded by id" do
-      topic = Fabricate(:topic)
-      get "/embed/comments", params: { topic_id: topic.id }, headers: headers
-      expect(response.status).to eq(200)
-    end
-  end
+  fab!(:topic) { Fabricate(:topic) }
 
   describe "#info" do
     context "without api key" do
       it "fails" do
         get "/embed/info.json"
+
         expect(response.body).to match(I18n.t("embed.error"))
       end
     end
@@ -51,10 +32,9 @@ RSpec.describe EmbedController do
                 HTTP_API_USERNAME: "system",
               }
 
-          json = response.parsed_body
-          expect(json["topic_id"]).to eq(topic_embed.topic.id)
-          expect(json["post_id"]).to eq(topic_embed.post.id)
-          expect(json["topic_slug"]).to eq(topic_embed.topic.slug)
+          expect(response.parsed_body["topic_id"]).to eq(topic_embed.topic.id)
+          expect(response.parsed_body["post_id"]).to eq(topic_embed.post.id)
+          expect(response.parsed_body["topic_slug"]).to eq(topic_embed.topic.slug)
         end
       end
 
@@ -79,6 +59,7 @@ RSpec.describe EmbedController do
   describe "#topics" do
     it "raises an error when not enabled" do
       get "/embed/topics?embed_id=de-1234"
+
       expect(response.status).to eq(400)
     end
 
@@ -87,15 +68,16 @@ RSpec.describe EmbedController do
 
       it "raises an error with a weird id" do
         get "/embed/topics?discourse_embed_id=../asdf/-1234", headers: headers
+
         expect(response.status).to eq(400)
       end
 
       it "returns a list of topics" do
-        topic = Fabricate(:topic)
         get "/embed/topics?discourse_embed_id=de-1234",
             headers: {
               "REFERER" => "https://example.com/evil-trout",
             }
+
         expect(response.status).to eq(200)
         expect(response.headers["X-Frame-Options"]).to be_nil
         expect(response.body).to match("data-embed-id=\"de-1234\"")
@@ -104,7 +86,6 @@ RSpec.describe EmbedController do
       end
 
       it "returns a list of top topics" do
-        bad_topic = Fabricate(:topic)
         good_topic = Fabricate(:topic, like_count: 1000, posts_count: 100)
         TopTopic.refresh!
 
@@ -116,13 +97,11 @@ RSpec.describe EmbedController do
         expect(response.headers["X-Frame-Options"]).to be_nil
         expect(response.body).to match("data-embed-id=\"de-1234\"")
         expect(response.body).to match("data-topic-id=\"#{good_topic.id}\"")
-        expect(response.body).not_to match("data-topic-id=\"#{bad_topic.id}\"")
+        expect(response.body).not_to match("data-topic-id=\"#{topic.id}\"")
         expect(response.body).to match("data-referer=\"https://example.com/evil-trout\"")
       end
 
       it "returns a list of topics if the top_period is not valid" do
-        topic1 = Fabricate(:topic)
-        topic2 = Fabricate(:topic)
         good_topic = Fabricate(:topic, like_count: 1000, posts_count: 100)
         TopTopic.refresh!
         TopicQuery.any_instance.expects(:list_top_for).never
@@ -131,21 +110,21 @@ RSpec.describe EmbedController do
             headers: {
               "REFERER" => "https://example.com/evil-trout",
             }
+
         expect(response.status).to eq(200)
         expect(response.headers["X-Frame-Options"]).to be_nil
         expect(response.body).to match("data-embed-id=\"de-1234\"")
         expect(response.body).to match("data-topic-id=\"#{good_topic.id}\"")
-        expect(response.body).to match("data-topic-id=\"#{topic1.id}\"")
-        expect(response.body).to match("data-topic-id=\"#{topic2.id}\"")
+        expect(response.body).to match("data-topic-id=\"#{topic.id}\"")
         expect(response.body).to match("data-referer=\"https://example.com/evil-trout\"")
       end
 
       it "wraps the list in a custom class" do
-        topic = Fabricate(:topic)
         get "/embed/topics?discourse_embed_id=de-1234&embed_class=my-special-class",
             headers: {
               "REFERER" => "https://example.com/evil-trout",
             }
+
         expect(response.status).to eq(200)
         expect(response.headers["X-Frame-Options"]).to be_nil
         expect(response.body).to match("class='topics-list my-special-class'")
@@ -153,37 +132,69 @@ RSpec.describe EmbedController do
 
       it "returns no referer if not supplied" do
         get "/embed/topics?discourse_embed_id=de-1234"
+
         expect(response.status).to eq(200)
         expect(response.body).to match("data-referer=\"\"")
       end
 
       it "returns * for the referer if `embed_any_origin` is set" do
         SiteSetting.embed_any_origin = true
+
         get "/embed/topics?discourse_embed_id=de-1234"
+
         expect(response.status).to eq(200)
         expect(response.body).to match("data-referer=\"\\*\"")
       end
 
       it "disallows indexing the embed topic list" do
-        topic = Fabricate(:topic)
         get "/embed/topics?discourse_embed_id=de-1234",
             headers: {
               "REFERER" => "https://example.com/evil-trout",
             }
+
         expect(response.status).to eq(200)
         expect(response.headers["X-Robots-Tag"]).to match(/noindex/)
       end
     end
   end
 
+  describe "#comments" do
+    it "is 404 without an embed_url" do
+      get "/embed/comments"
+
+      expect(response.body).to match(I18n.t("embed.error"))
+    end
+
+    it "raises an error with a missing host" do
+      get "/embed/comments", params: { embed_url: embed_url }
+
+      expect(response.body).to match(I18n.t("embed.error"))
+    end
+
+    describe "by topic id" do
+      fab!(:embeddable_host) { Fabricate(:embeddable_host) }
+
+      it "allows a topic to be embedded by id" do
+        get "/embed/comments",
+            params: {
+              topic_id: topic.id,
+            },
+            headers: {
+              "REFERER" => "http://eviltrout.com/some-page",
+            }
+
+        expect(response.status).to eq(200)
+      end
+    end
+
     context "with a host" do
-    let!(:embeddable_host) { Fabricate(:embeddable_host) }
-    let(:headers) { { "REFERER" => embed_url } }
+      fab!(:embeddable_host) { Fabricate(:embeddable_host) }
 
       before { Jobs.run_immediately! }
 
-    it "doesn't raises an error with no referer" do
+      it "doesn't raise an error with no referer" do
         get "/embed/comments", params: { embed_url: embed_url }
+
         expect(response.body).not_to match(I18n.t("embed.error"))
       end
 
@@ -196,18 +207,16 @@ RSpec.describe EmbedController do
           name: "embedded_scss",
           target_id: 0,
           type_id: 1,
-        value: ".test-osama-15 {\n" + "    color: red;\n" + "}\n",
+          value: ".test-osama-15 { color: red }",
         )
 
         topic_embed = Fabricate(:topic_embed, embed_url: embed_url)
         post = Fabricate(:post, topic: topic_embed.topic)
 
-      get "/embed/comments", params: { embed_url: embed_url }, headers: headers
+        get "/embed/comments", params: { embed_url: embed_url }, headers: { "REFERER" => embed_url }
 
         html = Nokogiri::HTML5.fragment(response.body)
-      css_link = html.at("link[data-target=embedded_theme]").attribute("href").value
-
-      get css_link
+        get html.at("link[data-target=embedded_theme]").attribute("href").value
         expect(response.status).to eq(200)
         expect(response.body).to include(".test-osama-15")
       end
@@ -237,67 +246,94 @@ RSpec.describe EmbedController do
       end
 
       context "with success" do
-      after do
+        it "tells the topic retriever to work when no previous embed is found" do
+          TopicRetriever.any_instance.expects(:retrieve)
+
+          get "/embed/comments",
+              params: {
+                embed_url: embed_url,
+              },
+              headers: {
+                "REFERER" => embed_url,
+              }
+
           expect(response.status).to eq(200)
           expect(response.headers["X-Frame-Options"]).to be_nil
         end
 
-      it "tells the topic retriever to work when no previous embed is found" do
-        TopicEmbed.expects(:topic_id_for_embed).returns(nil)
-        retriever = mock
-        TopicRetriever.expects(:new).returns(retriever)
-        retriever.expects(:retrieve)
-        get "/embed/comments", params: { embed_url: embed_url }, headers: headers
-      end
-
         it "displays the right view" do
           topic_embed = Fabricate(:topic_embed, embed_url: embed_url)
 
-        get "/embed/comments", params: { embed_url: embed_url_secure }, headers: headers
+          get "/embed/comments",
+              params: {
+                embed_url: embed_url_secure,
+              },
+              headers: {
+                "REFERER" => embed_url,
+              }
 
+          expect(response.status).to eq(200)
+          expect(response.headers["X-Frame-Options"]).to be_nil
           expect(response.body).to match(I18n.t("embed.start_discussion"))
         end
 
         it "creates a topic view when a topic_id is found" do
           topic_embed = Fabricate(:topic_embed, embed_url: embed_url)
+
           post = Fabricate(:post, topic: topic_embed.topic)
 
-        get "/embed/comments", params: { embed_url: embed_url }, headers: headers
+          get "/embed/comments",
+              params: {
+                embed_url: embed_url,
+              },
+              headers: {
+                "REFERER" => embed_url,
+              }
 
+          expect(response.status).to eq(200)
+          expect(response.headers["X-Frame-Options"]).to be_nil
           expect(response.body).to match(I18n.t("embed.continue"))
           expect(response.body).to match(post.cooked)
           expect(response.body).to match("<span class='replies'>1 reply</span>")
 
           small_action = Fabricate(:small_action, topic: topic_embed.topic)
 
-        get "/embed/comments", params: { embed_url: embed_url }, headers: headers
+          get "/embed/comments",
+              params: {
+                embed_url: embed_url,
+              },
+              headers: {
+                "REFERER" => embed_url,
+              }
 
+          expect(response.status).to eq(200)
+          expect(response.headers["X-Frame-Options"]).to be_nil
           expect(response.body).not_to match("post-#{small_action.id}")
           expect(response.body).to match("<span class='replies'>1 reply</span>")
         end
 
         it "provides the topic retriever with the discourse username when provided" do
-        retriever = mock
-        retriever.expects(:retrieve).returns(nil)
-        TopicRetriever
-          .expects(:new)
-          .with(embed_url, has_entry(author_username: discourse_username))
-          .returns(retriever)
+          TopicRetriever.any_instance.expects(:retrieve).returns(nil)
 
           get "/embed/comments",
               params: {
                 embed_url: embed_url,
                 discourse_username: discourse_username,
               },
-            headers: headers
+              headers: {
+                "REFERER" => embed_url,
+              }
+
+          expect(response.status).to eq(200)
+          expect(response.headers["X-Frame-Options"]).to be_nil
         end
       end
     end
 
     context "with multiple hosts" do
-    before do
-      Fabricate(:embeddable_host)
-      Fabricate(:embeddable_host, host: "http://discourse.org")
+      fab!(:embeddable_host_1) { Fabricate(:embeddable_host) }
+      fab!(:embeddable_host_2) { Fabricate(:embeddable_host, host: "http://discourse.org") }
+      fab!(:embeddable_host_3) do
         Fabricate(:embeddable_host, host: "https://example.com/1234", class_name: "example")
       end
 
@@ -349,6 +385,19 @@ RSpec.describe EmbedController do
 
           expect(response.body).to match('class="example"')
         end
+
+        it "contains custom class name from params" do
+          get "/embed/comments",
+              params: {
+                embed_url: embed_url,
+                class_name: "param-class-name",
+              },
+              headers: {
+                "REFERER" => "https://example.com/some-other-path",
+              }
+
+          expect(response.body).to match('class="param-class-name"')
+        end
       end
 
       context "with CSP frame-ancestors enabled" do
@@ -372,4 +421,5 @@ RSpec.describe EmbedController do
         end
       end
     end
+  end
 end