From ec7f418a22188bcdf7749c8bb75d8d3ddc2927b0 Mon Sep 17 00:00:00 2001
From: Gerhard Schlager <mail@gerhard-schlager.at>
Date: Thu, 31 Jan 2019 20:44:20 +0100
Subject: [PATCH] REFACTOR: Simplify finding the opengraph image

* removes deprecation warnings for "logo url"
* adds the "large icon" as fallback before the "apple touch icon"
---
 app/helpers/application_helper.rb       | 20 +++++++++----------
 spec/helpers/application_helper_spec.rb | 26 ++++++++++++++++++-------
 2 files changed, 29 insertions(+), 17 deletions(-)

diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
index 810cfe146e..4963b95832 100644
--- a/app/helpers/application_helper.rb
+++ b/app/helpers/application_helper.rb
@@ -212,18 +212,18 @@ module ApplicationHelper
     opts ||= {}
     opts[:url] ||= "#{Discourse.base_url_no_prefix}#{request.fullpath}"
 
-    twitter_summary_large_image_url =
-      SiteSetting.site_twitter_summary_large_image_url
+    if opts[:image].blank?
+      twitter_summary_large_image_url = SiteSetting.site_twitter_summary_large_image_url
 
-    opengraph_image_url = SiteSetting.opengraph_image_url
+      if twitter_summary_large_image_url.present?
+        opts[:twitter_summary_large_image] = twitter_summary_large_image_url
+      end
 
-    if opts[:image].blank? && (opengraph_image_url.present? || twitter_summary_large_image_url.present?)
-      opts[:twitter_summary_large_image] = twitter_summary_large_image_url if twitter_summary_large_image_url.present?
-      opts[:image] = opengraph_image_url.present? ? opengraph_image_url : twitter_summary_large_image_url
-    elsif opts[:image].blank? && SiteSetting.site_apple_touch_icon_url.present?
-      opts[:image] = SiteSetting.site_apple_touch_icon_url
-    elsif opts[:image].blank? && SiteSetting.logo_url.present?
-      opts[:image] = SiteSetting.logo_url
+      opts[:image] = SiteSetting.opengraph_image_url.presence ||
+        twitter_summary_large_image_url.presence ||
+        SiteSetting.site_large_icon_url.presence ||
+        SiteSetting.site_apple_touch_icon_url.presence ||
+        SiteSetting.site_logo_url.presence
     end
 
     # Use the correct scheme for opengraph/twitter image
diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb
index 23bb27761c..b769cadb3a 100644
--- a/spec/helpers/application_helper_spec.rb
+++ b/spec/helpers/application_helper_spec.rb
@@ -232,18 +232,30 @@ describe ApplicationHelper do
 
   describe 'crawlable_meta_data' do
     context "opengraph image" do
-      it 'returns default_opengraph_image_url' do
-        SiteSetting.default_opengraph_image_url = "/images/og-image.png"
+      it 'returns the correct image' do
+        SiteSetting.default_opengraph_image_url = '/images/og-image.png'
+        SiteSetting.twitter_summary_large_image_url = '/images/twitter.png'
+        SiteSetting.large_icon_url = '/images/large_icon.png'
+        SiteSetting.apple_touch_icon_url = '/images/default-apple-touch-icon.png'
+        SiteSetting.logo_url = '/images/d-logo-sketch.png'
+
+        expect(helper.crawlable_meta_data(image: "some-image.png")).to include("some-image.png")
         expect(helper.crawlable_meta_data).to include("/images/og-image.png")
-      end
 
-      it 'returns apple_touch_icon_url if default_opengraph_image_url is blank' do
+        SiteSetting.default_opengraph_image_url = ''
+        expect(helper.crawlable_meta_data).to include("/images/twitter.png")
+
+        SiteSetting.twitter_summary_large_image_url = ''
+        expect(helper.crawlable_meta_data).to include("/images/large_icon.png")
+
+        SiteSetting.large_icon_url = ''
         expect(helper.crawlable_meta_data).to include("/images/default-apple-touch-icon.png")
-      end
 
-      it 'returns logo_url if apple_touch_icon_url is blank' do
-        SiteSetting.apple_touch_icon_url = ""
+        SiteSetting.apple_touch_icon_url = ''
         expect(helper.crawlable_meta_data).to include("/images/d-logo-sketch.png")
+
+        SiteSetting.logo_url = ''
+        expect(helper.crawlable_meta_data).to_not include("/images")
       end
     end
   end