FIX: Properly support defaults for upload site settings.

app/assets/javascripts/discourse/widgets/home-logo.js.es6

@@ -17,7 +17,7 @@ export default createWidget("home-logo", {
   },
 
   logoUrl() {
-    return this.siteSettings.site_home_logo_url || "";
+    return this.siteSettings.site_logo_url || "";
   },
 
   mobileLogoUrl() {

app/controllers/admin/site_settings_controller.rb

@@ -15,7 +15,7 @@ class Admin::SiteSettingsController < Admin::AdminController
     raise_access_hidden_setting(id)
 
     if SiteSetting.type_supervisor.get_type(id) == :upload
-      value = Upload.get_from_url(value) || ''
+      value = Upload.find_by(url: value) || ''
     end
 
     SiteSetting.set_and_log(id, value, current_user)

app/helpers/application_helper.rb

@@ -219,7 +219,7 @@ module ApplicationHelper
         opts[:twitter_summary_large_image] = twitter_summary_large_image_url
       end
 
-      opts[:image] = SiteSetting.opengraph_image_url.presence ||
+      opts[:image] = SiteSetting.site_opengraph_image_url.presence ||
         twitter_summary_large_image_url.presence ||
         SiteSetting.site_large_icon_url.presence ||
         SiteSetting.site_apple_touch_icon_url.presence ||
@@ -295,7 +295,7 @@ module ApplicationHelper
       if mobile_view? && SiteSetting.site_mobile_logo_url
         SiteSetting.site_mobile_logo_url
       else
-        SiteSetting.site_home_logo_url
+        SiteSetting.site_logo_url
       end
     end
   end

app/helpers/user_notifications_helper.rb

@@ -22,8 +22,7 @@ module UserNotificationsHelper
     logo_url = SiteSetting.site_digest_logo_url
     logo_url = SiteSetting.site_logo_url if logo_url.blank? || logo_url =~ /\.svg$/i
     return nil if logo_url.blank? || logo_url =~ /\.svg$/i
-
-    full_cdn_url(logo_url)
+    logo_url
   end
 
   def html_site_link(color)

app/jobs/scheduled/clean_up_uploads.rb

@@ -7,6 +7,7 @@ module Jobs
 
       # always remove invalid upload records
       Upload
+        .by_users
         .where("retain_hours IS NULL OR created_at < current_timestamp - interval '1 hour' * retain_hours")
         .where("created_at < ?", grace_period.hour.ago)
         .where(url: "")
@@ -44,7 +45,8 @@ module Jobs
         end
       end.compact.uniq
 
-      result = Upload.where("uploads.retain_hours IS NULL OR uploads.created_at < current_timestamp - interval '1 hour' * uploads.retain_hours")
+      result = Upload.by_users
+        .where("uploads.retain_hours IS NULL OR uploads.created_at < current_timestamp - interval '1 hour' * uploads.retain_hours")
         .where("uploads.created_at < ?", grace_period.hour.ago)
         .joins(<<~SQL)
           LEFT JOIN site_settings ss

app/models/report.rb

@@ -1529,7 +1529,9 @@ class Report
     FROM uploads up
     JOIN users u
     ON u.id = up.user_id
-    WHERE up.created_at >= '#{report.start_date}' AND up.created_at <= '#{report.end_date}'
+    WHERE up.created_at >= '#{report.start_date}'
+    AND up.created_at <= '#{report.end_date}'
+    AND up.id > #{Upload::SEEDED_ID_THRESHOLD}
     ORDER BY up.filesize DESC
     LIMIT #{report.limit || 250}
     SQL

app/models/site_setting.rb

@@ -175,69 +175,26 @@ class SiteSetting < ActiveRecord::Base
     site_logo_small_url
     site_mobile_logo_url
     site_favicon_url
-    site_home_logo_url
   }.each { |client_setting| client_settings << client_setting }
 
-  def self.site_home_logo_url
-    upload = SiteSetting.logo
-
-    if SiteSetting.defaults.get(:title) != SiteSetting.title && !upload
-      ''
-    else
-      full_cdn_url(upload ? upload.url : '/images/d-logo-sketch.png')
+  %i{
+    logo
+    logo_small
+    digest_logo
+    mobile_logo
+    large_icon
+    favicon
+    apple_touch_icon
+    twitter_summary_large_image
+    opengraph_image
+    push_notifications_icon
+  }.each do |setting_name|
+    define_singleton_method("site_#{setting_name}_url") do
+      upload = self.public_send(setting_name)
+      upload ? full_cdn_url(upload.url) : ''
     end
   end
 
-  def self.site_logo_url
-    upload = self.logo
-    upload ? full_cdn_url(upload.url) : self.logo_url(warn: false)
-  end
-
-  def self.site_logo_small_url
-    upload = self.logo_small
-    upload ? full_cdn_url(upload.url) : self.logo_small_url(warn: false)
-  end
-
-  def self.site_digest_logo_url
-    upload = self.digest_logo
-    upload ? full_cdn_url(upload.url) : self.digest_logo_url(warn: false)
-  end
-
-  def self.site_mobile_logo_url
-    upload = self.mobile_logo
-    upload ? full_cdn_url(upload.url) : self.mobile_logo_url(warn: false)
-  end
-
-  def self.site_large_icon_url
-    upload = self.large_icon
-    upload ? full_cdn_url(upload.url) : self.large_icon_url(warn: false)
-  end
-
-  def self.site_favicon_url
-    upload = self.favicon
-    upload ? full_cdn_url(upload.url) : self.favicon_url(warn: false)
-  end
-
-  def self.site_apple_touch_icon_url
-    upload = self.apple_touch_icon
-    upload ? full_cdn_url(upload.url) : self.apple_touch_icon_url(warn: false)
-  end
-
-  def self.opengraph_image_url
-    upload = self.opengraph_image
-    upload ? full_cdn_url(upload.url) : self.default_opengraph_image_url(warn: false)
-  end
-
-  def self.site_twitter_summary_large_image_url
-    self.twitter_summary_large_image&.url ||
-      self.twitter_summary_large_image_url(warn: false)
-  end
-
-  def self.site_push_notifications_icon_url
-    SiteSetting.push_notifications_icon&.url ||
-      SiteSetting.push_notifications_icon_url(warn: false)
-  end
-
   def self.shared_drafts_enabled?
     c = SiteSetting.shared_drafts_category
     c.present? && c.to_i != SiteSetting.uncategorized_category_id.to_i

app/models/upload.rb

@@ -10,6 +10,7 @@ class Upload < ActiveRecord::Base
   include ActionView::Helpers::NumberHelper
 
   SHA1_LENGTH = 40
+  SEEDED_ID_THRESHOLD = 0
 
   belongs_to :user
 
@@ -36,6 +37,8 @@ class Upload < ActiveRecord::Base
     UserAvatar.where(custom_upload_id: self.id).update_all(custom_upload_id: nil)
   end
 
+  scope :by_users, -> { where("uploads.id > ?", SEEDED_ID_THRESHOLD) }
+
   def to_s
     self.url
   end

app/services/push_notification_pusher.rb

@@ -67,8 +67,8 @@ class PushNotificationPusher
   protected
 
   def self.get_badge
-    if SiteSetting.site_push_notifications_icon_url.present?
-      SiteSetting.site_push_notifications_icon_url
+    if (url = SiteSetting.site_push_notifications_icon_url).present?
+      url
     else
       ActionController::Base.helpers.image_url("push-notifications/discourse.png")
     end

config/site_settings.yml

@@ -1,6 +1,6 @@
 # Available options:
 #
-# default            - The default value of the setting.
+# default            - The default value of the setting. For upload site settings, use the id of the upload seeded in db/fixtures/010_uploads.rb.
 # client             - Set to true if the javascript should have access to this setting's value.
 # refresh            - Set to true if clients should refresh when the setting is changed.
 # min                - For a string setting, the minimum length. For an integer setting, the minimum value.
@@ -48,14 +48,14 @@ required:
     default: ""
     type: username
   logo:
-    default: ""
+    default: -1
     client: true
     type: upload
   logo_url:
     hidden: true
     default: "/images/d-logo-sketch.png"
   logo_small:
-    default: ""
+    default: -2
     client: true
     type: upload
   logo_small_url:
@@ -83,14 +83,14 @@ required:
     hidden: true
     default: ""
   favicon:
-    default: ""
+    default: -3
     client: true
     type: upload
   favicon_url:
     hidden: true
     default: "/images/default-favicon.ico"
   apple_touch_icon:
-    default: ""
+    default: -4
     client: true
     type: upload
   apple_touch_icon_url:

db/fixtures/010_uploads.rb

@@ -0,0 +1,17 @@
+{
+  -1 => "d-logo-sketch.png",
+  -2 => "d-logo-sketch-small.png",
+  -3 => "default-favicon.ico",
+  -4 => "default-apple-touch-icon.png"
+}.each do |id, filename|
+  path = Rails.root.join("public/images/#{filename}")
+
+  Upload.seed do |upload|
+    upload.id = id
+    upload.user_id = Discourse.system_user.id
+    upload.original_filename = filename
+    upload.url = "/images/#{filename}"
+    upload.filesize = File.size(path)
+    upload.extension = File.extname(path)[1..10]
+  end
+end

lib/file_store/base_store.rb

@@ -92,10 +92,6 @@ module FileStore
     def purge_tombstone(grace_period)
     end
 
-    def get_depth_for(id)
-      [0, Math.log(id / 1_000.0, 16).ceil].max
-    end
-
     def get_path_for(type, id, sha, extension)
       depth = get_depth_for(id)
       tree = File.join(*sha[0, depth].chars, "")
@@ -148,6 +144,12 @@ module FileStore
       raise "Not implemented."
     end
 
+    def get_depth_for(id)
+      depths = [0]
+      depths << Math.log(id / 1_000.0, 16).ceil if id.positive?
+      depths.max
+    end
+
   end
 
 end

lib/site_setting_extension.rb

@@ -230,17 +230,25 @@ module SiteSettingExtension
       .map do |s, v|
 
       value = send(s)
+      type_hash = type_supervisor.type_hash(s)
+      default = defaults.get(s, default_locale).to_s
+
+      if type_hash[:type].to_s == "upload" &&
+         default.to_i < Upload::SEEDED_ID_THRESHOLD
+
+        default = default_uploads[default.to_i]
+      end
 
       opts = {
         setting: s,
         description: description(s),
-        default: defaults.get(s, default_locale).to_s,
+        default: default,
         value: value.to_s,
         category: categories[s],
         preview: previews[s],
         secret: secret_settings.include?(s),
         placeholder: placeholder(s)
-      }.merge(type_supervisor.type_hash(s))
+      }.merge!(type_hash)
 
       opts
     end.unshift(locale_setting_hash)
@@ -450,7 +458,7 @@ module SiteSettingExtension
 
         value = value.to_i
 
-        if value > 0
+        if value != Upload::SEEDED_ID_THRESHOLD
           upload = Upload.find_by(id: value)
           uploads[name] = upload if upload
         end
@@ -495,6 +503,14 @@ module SiteSettingExtension
 
   private
 
+  def default_uploads
+    @default_uploads ||= {}
+
+    @default_uploads[provider.current_site] ||= begin
+      Upload.where("id < ?", Upload::SEEDED_ID_THRESHOLD).pluck(:id, :url).to_h
+    end
+  end
+
   def uploads
     @uploads ||= {}
     @uploads[provider.current_site] ||= {}

lib/site_settings/type_supervisor.rb

@@ -177,7 +177,7 @@ class SiteSettings::TypeSupervisor
     elsif type == self.class.types[:enum]
       val = @defaults_provider[name].is_a?(Integer) ? val.to_i : val.to_s
     elsif type == self.class.types[:upload] && val.present?
-      val = val.id
+      val = val.is_a?(Integer) ? val : val.id
     end
 
     [val, type]

plugins/discourse-narrative-bot/spec/requests/discobot_certificate_spec.rb

@@ -27,7 +27,7 @@ describe "Discobot Certificate" do
       it 'should return the right text' do
         stub_request(:get, /letter_avatar_proxy/).to_return(status: 200)
 
-        stub_request(:get, "http://test.localhost//images/d-logo-sketch-small.png")
+        stub_request(:get, SiteSetting.site_logo_small_url)
           .to_return(status: 200)
 
         get '/discobot/certificate.svg', params: params

spec/components/file_store/base_store_spec.rb

@@ -18,6 +18,15 @@ RSpec.describe FileStore::BaseStore do
           .to eq('original/2X/4/4170ac2a2782a1516fe9e13d7322ae482c1bd594.png')
       end
     end
+
+    describe 'when id is negative' do
+      it 'should return the right depth' do
+        upload.update!(id: -999)
+
+        expect(FileStore::BaseStore.new.get_path_for_upload(upload))
+          .to eq('original/1X/4170ac2a2782a1516fe9e13d7322ae482c1bd594.png')
+      end
+    end
   end
 
   describe '#get_path_for_optimized_image' do

spec/components/site_setting_extension_spec.rb

@@ -711,6 +711,28 @@ describe SiteSettingExtension do
 
   end
 
+  describe '.all_settings' do
+    describe 'uploads settings' do
+      it 'should return the right values' do
+        system_upload = Fabricate(:upload, id: -999)
+        settings.setting(:logo, system_upload.id, type: :upload)
+        settings.refresh!
+        setting = settings.all_settings.last
+
+        expect(setting[:value]).to eq(system_upload.url)
+        expect(setting[:default]).to eq(system_upload.url)
+
+        upload = Fabricate(:upload)
+        settings.logo = upload
+        settings.refresh!
+        setting = settings.all_settings.last
+
+        expect(setting[:value]).to eq(upload.url)
+        expect(setting[:default]).to eq(system_upload.url)
+      end
+    end
+  end
+
   describe '.client_settings_json_uncached' do
     it 'should return the right json value' do
       upload = Fabricate(:upload)

spec/components/site_settings/type_supervisor_spec.rb

@@ -190,6 +190,9 @@ describe SiteSettings::TypeSupervisor do
 
         expect(settings.type_supervisor.to_db_value(:type_upload, upload))
           .to eq([upload.id, SiteSetting.types[:upload]])
+
+        expect(settings.type_supervisor.to_db_value(:type_upload, 1))
+          .to eq([1, SiteSetting.types[:upload]])
       end
 
       it 'returns enum value with string default' do

spec/jobs/clean_up_uploads_spec.rb

@@ -50,6 +50,7 @@ describe Jobs::CleanUpUploads do
       SiteSetting.provider = SiteSettings::DbProvider.new(SiteSetting)
       SiteSetting.clean_orphan_uploads_grace_period_hours = 1
 
+      system_upload = fabricate_upload(id: -999)
       logo_upload = fabricate_upload
       logo_small_upload = fabricate_upload
       digest_logo_upload = fabricate_upload
@@ -84,7 +85,8 @@ describe Jobs::CleanUpUploads do
         opengraph_image_upload,
         twitter_summary_large_image_upload,
         favicon_upload,
-        apple_touch_icon_upload
+        apple_touch_icon_upload,
+        system_upload
       ].each { |record| expect(Upload.exists?(id: record.id)).to eq(true) }
 
       fabricate_upload

spec/models/emoji_spec.rb

@@ -18,7 +18,7 @@ describe Emoji do
   describe '.load_custom' do
     describe 'when a custom emoji has an invalid upload_id' do
       it 'should return the custom emoji without a URL' do
-        CustomEmoji.create!(name: 'test', upload_id: -1)
+        CustomEmoji.create!(name: 'test', upload_id: 9999)
 
         emoji = Emoji.load_custom.first
 

spec/models/site_setting_spec.rb

@@ -150,40 +150,6 @@ describe SiteSetting do
     end
   end
 
-  describe '.site_home_logo_url' do
-    describe 'when logo site setting is set' do
-      let(:upload) { Fabricate(:upload) }
-
-      before do
-        SiteSetting.logo = upload
-      end
-
-      it 'should return the right URL' do
-        expect(SiteSetting.site_home_logo_url)
-          .to eq("#{Discourse.base_url}#{upload.url}")
-      end
-    end
-
-    describe 'when logo site setting is not set' do
-      describe 'when there is a custom title' do
-        before do
-          SiteSetting.title = "test"
-        end
-
-        it 'should return a blank string' do
-          expect(SiteSetting.site_home_logo_url).to eq('')
-        end
-      end
-
-      describe 'when title has not been set' do
-        it 'should return the default logo url' do
-          expect(SiteSetting.site_home_logo_url)
-            .to eq("#{Discourse.base_url}/images/d-logo-sketch.png")
-        end
-      end
-    end
-  end
-
   context 'deprecated site settings' do
     before do
       SiteSetting.force_https = true

spec/requests/admin/site_settings_controller_spec.rb

@@ -51,31 +51,49 @@ describe Admin::SiteSettingsController do
         expect(SiteSetting.test_setting).to eq('')
       end
 
-      it 'allows upload site settings to be updated' do
-        upload = Fabricate(:upload)
+      describe 'upload site settings' do
+        it 'can remove the site setting' do
+          SiteSetting.test_upload = Fabricate(:upload)
 
-        put "/admin/site_settings/test_upload.json", params: {
-          test_upload: upload.url
-        }
+          put "/admin/site_settings/test_upload.json", params: {
+            test_upload: nil
+          }
 
-        expect(response.status).to eq(200)
-        expect(SiteSetting.test_upload).to eq(upload)
+          expect(response.status).to eq(200)
+          expect(SiteSetting.test_upload).to eq(nil)
+        end
 
-        user_history = UserHistory.last
+        it 'can reset the site setting to the default' do
+          SiteSetting.test_upload = nil
+          default_upload = Upload.find(-1)
 
-        expect(user_history.action).to eq(
-          UserHistory.actions[:change_site_setting]
-        )
+          put "/admin/site_settings/test_upload.json", params: {
+            test_upload: default_upload.url
+          }
 
-        expect(user_history.previous_value).to eq(nil)
-        expect(user_history.new_value).to eq(upload.url)
+          expect(response.status).to eq(200)
+          expect(SiteSetting.test_upload).to eq(default_upload)
+        end
 
-        put "/admin/site_settings/test_upload.json", params: {
-          test_upload: nil
-        }
+        it 'can update the site setting' do
+          upload = Fabricate(:upload)
 
-        expect(response.status).to eq(200)
-        expect(SiteSetting.test_upload).to eq(nil)
+          put "/admin/site_settings/test_upload.json", params: {
+            test_upload: upload.url
+          }
+
+          expect(response.status).to eq(200)
+          expect(SiteSetting.test_upload).to eq(upload)
+
+          user_history = UserHistory.last
+
+          expect(user_history.action).to eq(
+            UserHistory.actions[:change_site_setting]
+          )
+
+          expect(user_history.previous_value).to eq(nil)
+          expect(user_history.new_value).to eq(upload.url)
+        end
       end
 
       it 'logs the change' do

spec/requests/exceptions_controller_spec.rb

@@ -10,16 +10,14 @@ RSpec.describe ExceptionsController do
       expect(response.body).to have_tag(
         "img",
         with: {
-          src: SiteSetting.site_home_logo_url
+          src: SiteSetting.site_logo_url
         }
       )
     end
 
     describe "text site logo" do
-      let(:title) { "some awesome title" }
-
       before do
-        SiteSetting.title = title
+        SiteSetting.logo = nil
       end
 
       it "should return the right response" do
@@ -29,7 +27,7 @@ RSpec.describe ExceptionsController do
 
         expect(response.body).to have_tag(
           "h2",
-          text: title
+          text: SiteSetting.title
         )
       end
     end

spec/services/push_notification_pusher_spec.rb

@@ -7,8 +7,11 @@ RSpec.describe PushNotificationPusher do
   end
 
   it "returns custom badges url" do
-    SiteSetting.push_notifications_icon_url = "/test.png"
-    expect(PushNotificationPusher.get_badge).to eq("/test.png")
+    upload = Fabricate(:upload)
+    SiteSetting.push_notifications_icon = upload
+
+    expect(PushNotificationPusher.get_badge)
+      .to eq(UrlHelper.absolute(upload.url))
   end
 
 end

test/javascripts/helpers/site-settings.js

@@ -2,7 +2,7 @@
 Discourse.SiteSettingsOriginal = {
   title: "Discourse Meta",
   site_logo_url: "/assets/logo.png",
-  site_home_logo_url: "/assets/logo.png",
+  site_logo_url: "/assets/logo.png",
   site_logo_small_url: "/assets/logo-single.png",
   site_mobile_logo_url: "",
   site_favicon_url:

test/javascripts/widgets/home-logo-test.js.es6

@@ -10,7 +10,7 @@ const title = "Cool Forum";
 widgetTest("basics", {
   template: '{{mount-widget widget="home-logo" args=args}}',
   beforeEach() {
-    this.siteSettings.site_home_logo_url = bigLogo;
+    this.siteSettings.site_logo_url = bigLogo;
     this.siteSettings.site_logo_small_url = smallLogo;
     this.siteSettings.title = title;
     this.set("args", { minimized: false });
@@ -28,7 +28,7 @@ widgetTest("basics", {
 widgetTest("basics - minimized", {
   template: '{{mount-widget widget="home-logo" args=args}}',
   beforeEach() {
-    this.siteSettings.site_home_logo_url = bigLogo;
+    this.siteSettings.site_logo_url = bigLogo;
     this.siteSettings.site_logo_small_url = smallLogo;
     this.siteSettings.title = title;
     this.set("args", { minimized: true });
@@ -44,7 +44,7 @@ widgetTest("basics - minimized", {
 widgetTest("no logo", {
   template: '{{mount-widget widget="home-logo" args=args}}',
   beforeEach() {
-    this.siteSettings.site_home_logo_url = "";
+    this.siteSettings.site_logo_url = "";
     this.siteSettings.site_logo_small_url = "";
     this.siteSettings.title = title;
     this.set("args", { minimized: false });
@@ -59,7 +59,7 @@ widgetTest("no logo", {
 widgetTest("no logo - minimized", {
   template: '{{mount-widget widget="home-logo" args=args}}',
   beforeEach() {
-    this.siteSettings.site_home_logo_url = "";
+    this.siteSettings.site_logo_url = "";
     this.siteSettings.site_logo_small_url = "";
     this.siteSettings.title = title;
     this.set("args", { minimized: true });
@@ -87,7 +87,7 @@ widgetTest("mobile logo", {
 widgetTest("mobile without logo", {
   template: '{{mount-widget widget="home-logo" args=args}}',
   beforeEach() {
-    this.siteSettings.site_home_logo_url = bigLogo;
+    this.siteSettings.site_logo_url = bigLogo;
     this.site.mobileView = true;
   },
 
@@ -101,7 +101,7 @@ widgetTest("basics, subfolder", {
   template: '{{mount-widget widget="home-logo" args=args}}',
   beforeEach() {
     Discourse.BaseUri = "/forum";
-    this.siteSettings.site_home_logo_url = bigLogo;
+    this.siteSettings.site_logo_url = bigLogo;
     this.siteSettings.site_logo_small_url = smallLogo;
     this.siteSettings.title = title;
     this.set("args", { minimized: false });
@@ -118,7 +118,7 @@ widgetTest("basics, subfolder - minimized", {
   template: '{{mount-widget widget="home-logo" args=args}}',
   beforeEach() {
     Discourse.BaseUri = "/forum";
-    this.siteSettings.site_home_logo_url = bigLogo;
+    this.siteSettings.site_logo_url = bigLogo;
     this.siteSettings.site_logo_small_url = smallLogo;
     this.siteSettings.title = title;
     this.set("args", { minimized: true });