From ba115480ba782587f07cb9d08ac7a77927910a71 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9gis=20Hanol?= <regis@hanol.fr>
Date: Mon, 6 Feb 2017 14:45:04 +0100
Subject: [PATCH] FIX: wasn't extracting links to quoted posts

---
 Gemfile.lock                                  |  4 +-
 .../discourse_onebox_sanitize_config.rb       |  9 ++++
 lib/onebox/engine/discourse_local_onebox.rb   |  2 +-
 .../templates/discourse_topic_onebox.hbs      |  2 +-
 lib/oneboxer.rb                               |  5 +-
 lib/pretty_text.rb                            | 50 ++++++++-----------
 spec/components/pretty_text_spec.rb           | 11 ++--
 spec/jobs/process_post_spec.rb                | 20 ++++++--
 8 files changed, 57 insertions(+), 46 deletions(-)
 create mode 100644 lib/onebox/discourse_onebox_sanitize_config.rb

diff --git a/Gemfile.lock b/Gemfile.lock
index 7ac4aa416fa..7fcbf60bc30 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -206,7 +206,7 @@ GEM
     omniauth-twitter (1.3.0)
       omniauth-oauth (~> 1.1)
       rack
-    onebox (1.7.8)
+    onebox (1.7.9)
       fast_blank (>= 1.0.0)
       htmlentities (~> 4.3.4)
       moneta (~> 0.8)
@@ -476,4 +476,4 @@ DEPENDENCIES
   unicorn
 
 BUNDLED WITH
-   1.14.2
+   1.14.3
diff --git a/lib/onebox/discourse_onebox_sanitize_config.rb b/lib/onebox/discourse_onebox_sanitize_config.rb
new file mode 100644
index 00000000000..2b2d83f9895
--- /dev/null
+++ b/lib/onebox/discourse_onebox_sanitize_config.rb
@@ -0,0 +1,9 @@
+class Sanitize
+  module Config
+
+    DISCOURSE_ONEBOX ||= freeze_config merge(ONEBOX,
+      attributes: merge(ONEBOX[:attributes], 'aside' => [:data])
+    )
+
+  end
+end
diff --git a/lib/onebox/engine/discourse_local_onebox.rb b/lib/onebox/engine/discourse_local_onebox.rb
index 44b60030390..629cfd6d914 100644
--- a/lib/onebox/engine/discourse_local_onebox.rb
+++ b/lib/onebox/engine/discourse_local_onebox.rb
@@ -69,7 +69,7 @@ module Onebox
             first_post = topic.ordered_posts.first
 
             args = {
-              topic: topic.id,
+              topic_id: topic.id,
               avatar: PrettyText.avatar_img(topic.user.avatar_template, "tiny"),
               original_url: @url,
               title: PrettyText.unescape_emoji(CGI::escapeHTML(topic.title)),
diff --git a/lib/onebox/templates/discourse_topic_onebox.hbs b/lib/onebox/templates/discourse_topic_onebox.hbs
index 87e40915714..c6800ad8b45 100644
--- a/lib/onebox/templates/discourse_topic_onebox.hbs
+++ b/lib/onebox/templates/discourse_topic_onebox.hbs
@@ -1,4 +1,4 @@
-<aside class='quote topic-onebox' data-post="1" data-topic="{{topic}}">
+<aside class='quote' data-post="1" data-topic="{{topic_id}}">
   <div class='title'>
     {{{avatar}}}
     <a href="{{original_url}}">{{{title}}}</a> {{{category_html}}}
diff --git a/lib/oneboxer.rb b/lib/oneboxer.rb
index db043464381..cef2fe53df2 100644
--- a/lib/oneboxer.rb
+++ b/lib/oneboxer.rb
@@ -1,3 +1,4 @@
+require_dependency "./lib/onebox/discourse_onebox_sanitize_config"
 Dir["#{Rails.root}/lib/onebox/engine/*_onebox.rb"].sort.each { |f| require f }
 
 module Oneboxer
@@ -142,8 +143,8 @@ module Oneboxer
       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)
-
-        r = Onebox.preview(url, cache: {}, max_width: 695)
+        options = { cache: {}, max_width: 695, sanitize_config: Sanitize::Config::DISCOURSE_ONEBOX }
+        r = Onebox.preview(url, options)
         { onebox: r.to_s, preview: r.try(:placeholder_html).to_s }
       end
     rescue => e
diff --git a/lib/pretty_text.rb b/lib/pretty_text.rb
index 7c4e26225c0..036a50d6bd1 100644
--- a/lib/pretty_text.rb
+++ b/lib/pretty_text.rb
@@ -270,44 +270,36 @@ module PrettyText
     end
   end
 
-  class DetectedLink
-    attr_accessor :is_quote, :url
-
-    def initialize(url, is_quote=false)
-      @url = url
-      @is_quote = is_quote
-    end
-  end
-
+  class DetectedLink < Struct.new(:url, :is_quote); end
 
   def self.extract_links(html)
     links = []
     doc = Nokogiri::HTML.fragment(html)
+
     # remove href inside quotes & elided part
-    doc.css("aside.quote:not(.topic-onebox) a, .elided a").each { |l| l["href"] = "" }
+    doc.css("aside.quote a, .elided a").each { |a| a["href"] = "" }
 
-    # extract all links from the post
-    doc.css("a").each { |l|
-      unless l["href"].blank? || "#".freeze == l["href"][0]
-        links << DetectedLink.new(l["href"])
+    # extract all links
+    doc.css("a").each do |a|
+      if a["href"].present? && a["href"][0] != "#".freeze
+        links << DetectedLink.new(a["href"], false)
       end
-    }
-
-    # extract links to quotes
-    doc.css("aside.quote[data-topic]").each do |a|
-      topic_id = a['data-topic']
-
-      url = "/t/topic/#{topic_id}"
-      if post_number = a['data-post']
-        url << "/#{post_number}"
-      end
-
-      links << DetectedLink.new(url, true)
     end
 
-    # Extract Youtube links
-    doc.css("div[data-youtube-id]").each do |d|
-      links << DetectedLink.new("https://www.youtube.com/watch?v=#{d['data-youtube-id']}", false)
+    # extract quotes
+    doc.css("aside.quote[data-topic]").each do |aside|
+      if aside["data-topic"].present?
+        url = "/t/topic/#{aside["data-topic"]}"
+        url << "/#{aside["data-post"]}" if aside["data-post"].present?
+        links << DetectedLink.new(url, true)
+      end
+    end
+
+    # extract Youtube links
+    doc.css("div[data-youtube-id]").each do |div|
+      if div["data-youtube-id"].present?
+        links << DetectedLink.new("https://www.youtube.com/watch?v=#{div['data-youtube-id']}", false)
+      end
     end
 
     links
diff --git a/spec/components/pretty_text_spec.rb b/spec/components/pretty_text_spec.rb
index b63d77584b4..3a409471f4e 100644
--- a/spec/components/pretty_text_spec.rb
+++ b/spec/components/pretty_text_spec.rb
@@ -225,12 +225,11 @@ HTML
         <a href='http://body_and_quote.com'>http://useless2.com</a>
         ")
 
-      expect(links.map{|l| [l.url, l.is_quote]}.to_a.sort).to eq(
-        [["http://body_only.com",false],
-         ["http://body_and_quote.com", false],
-         ["/t/topic/1234",true]
-        ].sort
-      )
+      expect(links.map { |l| [l.url, l.is_quote] }.sort).to eq([
+        ["http://body_only.com", false],
+        ["http://body_and_quote.com", false],
+        ["/t/topic/1234", true],
+      ].sort)
     end
 
     it "should not preserve tags in code blocks" do
diff --git a/spec/jobs/process_post_spec.rb b/spec/jobs/process_post_spec.rb
index ff085e30aba..e240f6ef9d5 100644
--- a/spec/jobs/process_post_spec.rb
+++ b/spec/jobs/process_post_spec.rb
@@ -9,9 +9,7 @@ describe Jobs::ProcessPost do
 
   context 'with a post' do
 
-    let(:post) do
-      Fabricate(:post)
-    end
+    let(:post) { Fabricate(:post) }
 
     it 'does not erase posts when CookedPostProcessor malfunctions' do
       # Look kids, an actual reason why you want to use mocks
@@ -35,7 +33,6 @@ describe Jobs::ProcessPost do
     end
 
     it 'processes posts' do
-
       post = Fabricate(:post, raw: "<img src='#{Discourse.base_url_no_prefix}/awesome/picture.png'>")
       expect(post.cooked).to match(/http/)
 
@@ -51,7 +48,20 @@ describe Jobs::ProcessPost do
       expect { Jobs::ProcessPost.new.execute(post_id: post.id) }.to change { TopicLink.count }.by(1)
     end
 
+    it "extracts links to quoted posts" do
+      quoted_post = Fabricate(:post, raw: "This is a post with a link to https://www.discourse.org", post_number: 42)
+      post.update_columns(raw: "This quote is the best\n\n[quote=\"#{quoted_post.user.username}, topic:#{quoted_post.topic_id}, post:#{quoted_post.post_number}\"]#{quoted_post.excerpt}[/quote]")
+      # when creating a quote, we also create the reflexion link
+      expect { Jobs::ProcessPost.new.execute(post_id: post.id) }.to change { TopicLink.count }.by(2)
+    end
+
+    it "extracts links to oneboxed topics" do
+      oneboxed_post = Fabricate(:post)
+      post.update_columns(raw: "This post is the best\n\n#{oneboxed_post.full_url}")
+      # when creating a quote, we also create the reflexion link
+      expect { Jobs::ProcessPost.new.execute(post_id: post.id) }.to change { TopicLink.count }.by(2)
+    end
+
   end
 
-
 end