FIX: `UserNotificationsHelper#logo_url' to work with S3 based uploads.

FIX: `UserNotificationsHelper#logo_url' to work with S3 based uploads.
From 27c793a192ea0443373dc65517b50dfa21480441 Mon Sep 17 00:00:00 2001
From: Guo Xiang Tan <tgx_world@hotmail.com>
Date: Thu, 6 Dec 2018 09:37:35 +0800
Subject: [PATCH] FIX: `UserNotificationsHelper#logo_url' to work with S3 based
 uploads.

https://meta.discourse.org/t/digest-logo-not-working/103255

diff --git a/app/helpers/user_notifications_helper.rb b/app/helpers/user_notifications_helper.rb
index 5d9c98a..176dab9 100644
--- a/app/helpers/user_notifications_helper.rb
+++ b/app/helpers/user_notifications_helper.rb
@@ -1,4 +1,5 @@
 module UserNotificationsHelper
+  include GlobalPath
 
   def indent(text, by = 2)
     spacer = " " * by
@@ -22,11 +23,9 @@ module UserNotificationsHelper
     logo_url = SiteSetting.site_logo_url if logo_url.blank? || logo_url =~ /\.svg$/i
     return nil if logo_url.blank? || logo_url =~ /\.svg$/i
 
-    if logo_url !~ /http(s)?\:\/\//
-      logo_url = "#{Discourse.base_url}#{logo_url}"
-    end
-
-    logo_url
+    uri = URI.parse(UrlHelper.absolute(upload_cdn_path(logo_url)))
+    uri.scheme = SiteSetting.scheme if uri.scheme.blank?
+    uri.to_s
   end
 
   def html_site_link(color)
diff --git a/spec/helpers/user_notifications_helper_spec.rb b/spec/helpers/user_notifications_helper_spec.rb
index 2ca6916..735a69e 100644
--- a/spec/helpers/user_notifications_helper_spec.rb
+++ b/spec/helpers/user_notifications_helper_spec.rb
@@ -1,7 +1,7 @@
 require 'rails_helper'
 
 describe UserNotificationsHelper do
-  describe 'email_excerpt' do
+  describe '#email_excerpt' do
     let(:paragraphs) { [
       "<p>This is the first paragraph, but you should read more.</p>",
       "<p>And here is its friend, the second paragraph.</p>"
@@ -53,4 +53,72 @@ describe UserNotificationsHelper do
       expect(helper.email_excerpt(cooked)).to eq "<p>BEFORE</p><blockquote>\n  <p>This is a user quote</p>\n</blockquote><p>AFTER</p>"
     end
   end
+
+  describe '#logo_url' do
+    describe 'local store' do
+      let(:upload) { Fabricate(:upload, sha1: "somesha1") }
+
+      before do
+        SiteSetting.logo = upload
+      end
+
+      it 'should return the right URL' do
+        expect(helper.logo_url).to eq(
+          "http://test.localhost/uploads/default/original/1X/somesha1.png"
+        )
+      end
+
+      describe 'when cdn path is configured' do
+        before do
+          GlobalSetting.expects(:cdn_url)
+            .returns('https://some.localcdn.com')
+            .at_least_once
+        end
+
+        it 'should return the right URL' do
+          expect(helper.logo_url).to eq(
+            "https://some.localcdn.com/uploads/default/original/1X/somesha1.png"
+          )
+        end
+      end
+
+      describe 'when logo is an SVG' do
+        let(:upload) { Fabricate(:upload, extension: "svg") }
+
+        it 'should return nil' do
+          expect(helper.logo_url).to eq(nil)
+        end
+      end
+    end
+
+    describe 's3 store' do
+      let(:upload) { Fabricate(:upload_s3, sha1: "somesha1") }
+
+      before do
+        SiteSetting.enable_s3_uploads = true
+        SiteSetting.s3_upload_bucket = "s3-upload-bucket"
+        SiteSetting.s3_access_key_id = "some key"
+        SiteSetting.s3_secret_access_key = "some secret key"
+        SiteSetting.logo = upload
+      end
+
+      it 'should return the right URL' do
+        expect(helper.logo_url).to eq(
+          "http://s3-upload-bucket.s3.dualstack.us-east-1.amazonaws.com/original/1X/somesha1.png"
+        )
+      end
+
+      describe 'when cdn path is configured' do
+        before do
+          SiteSetting.s3_cdn_url = 'https://some.cdn.com'
+        end
+
+        it 'should return the right url' do
+          expect(helper.logo_url).to eq(
+            "https://some.cdn.com/original/1X/somesha1.png"
+          )
+        end
+      end
+    end
+  end
 end

GitHub

This commit has been mentioned on Discourse Meta. There might be relevant details there:

Note that we don’t recommend using expects but there was no sane way for me to do this without leaking state.

This commit has been mentioned on Discourse Meta. There might be relevant details there:

This commit has been mentioned on Discourse Meta. There might be relevant details there: