Merge pull request #5024 from fantasticfears/site-setting

FIX: fails loud if default setting is not set

config/site_settings.yml

@@ -187,6 +187,7 @@ basic:
     client: true
     default: true
   default_theme_key:
+    default: ''
     hidden: true
   relative_date_duration:
     client: true
@@ -1414,6 +1415,7 @@ user_api:
   max_api_keys_per_user:
     default: 10
   push_api_secret_key:
+    default: ''
     hidden: true
   min_trust_level_for_user_api_key:
     default: 1

lib/site_settings/yaml_loader.rb

@@ -13,11 +13,13 @@ class SiteSettings::YamlLoader
           # Get default value for the site setting:
           value = hash.delete('default')
           if value.is_a?(Hash)
-            raise Discourse::Deprecation, "Site setting per env is no longer supported. Error setting: #{setting_name}"
+            raise Discourse::Deprecation, "The site setting `#{setting_name}` can no longer be set based on Rails environment. See also `config/environments/<env>.rb`."
+          elsif value.nil?
+            raise StandardError, "The site setting `#{setting_name}` is missing default value."
           end
 
           if hash['hidden']&.is_a?(Hash)
-            raise Discourse::Deprecation, "Hidden site setting per env is no longer supported. Error setting: #{setting_name}"
+            raise Discourse::Deprecation, "The site setting `#{setting_name}`'s hidden property can no longer be set based on Rails environment. It can only be either `true` or `false`."
           end
 
           yield category, setting_name, value, hash.deep_symbolize_keys!

spec/components/site_settings/yaml_loader_spec.rb

@@ -31,6 +31,7 @@ describe SiteSettings::YamlLoader do
   let(:deprecated_env) { "#{Rails.root}/spec/fixtures/site_settings/deprecated_env.yml" }
   let(:deprecated_hidden) { "#{Rails.root}/spec/fixtures/site_settings/deprecated_hidden.yml" }
   let(:locale_default) { "#{Rails.root}/spec/fixtures/site_settings/locale_default.yml" }
+  let(:nil_default) { "#{Rails.root}/spec/fixtures/site_settings/nil_default.yml" }
 
   it "loads simple settings" do
     receiver.expects(:setting).with('category1', 'title', 'My Site', {}).once
@@ -77,6 +78,10 @@ describe SiteSettings::YamlLoader do
     expect { receiver.load_yaml(deprecated_hidden) }.to raise_error(Discourse::Deprecation)
   end
 
+  it "raises invalid parameter when default value is not present" do
+    expect { receiver.load_yaml(nil_default) }.to raise_error(StandardError)
+  end
+
   it "can load settings with locale default" do
     receiver.expects(:setting).with('search', 'min_search_term_length', 3, min: 2, client: true, locale_default: { zh_CN: 2, zh_TW: 2 })
     receiver.load_yaml(locale_default)

spec/fixtures/site_settings/nil_default.yml

@@ -0,0 +1,3 @@
+category1:
+  title:
+    hidden: true

spec/integrity/site_setting_spec.rb

@@ -0,0 +1,42 @@
+require "rails_helper"
+require "i18n/duplicate_key_finder"
+
+describe "site setting integrity checks" do
+  let(:site_setting_file) { File.join(Rails.root, 'config', 'site_settings.yml') }
+  let(:yaml) { YAML.load_file(site_setting_file) }
+
+  %w(hidden client).each do |property|
+    it "set #{property} value as true or not set" do
+      yaml.each_value do |category|
+        category.each_value do |setting|
+          next unless setting.is_a?(Hash)
+          expect(setting[property] == nil || setting[property] == true).to be_truthy
+        end
+      end
+    end
+  end
+
+  it "has no duplicate keys" do
+    duplicates = DuplicateKeyFinder.new.find_duplicates(site_setting_file)
+    expect(duplicates).to be_empty
+  end
+
+  it "no locale default has different type than default or invalid key" do
+    yaml.each_value do |category|
+      category.each_value do |setting|
+        next unless setting.is_a?(Hash)
+        if setting['locale_default']
+          setting['locale_default'].each_pair do |k, v|
+            expect(LocaleSiteSetting.valid_value?(k.to_s)).to be_truthy
+            case setting['default']
+            when TrueClass, FalseClass
+              expect(v.class == TrueClass || v.class == FalseClass).to be_truthy
+            else
+              expect(v).to be_a_kind_of(setting['default'].class)
+            end
+          end
+        end
+      end
+    end
+  end
+end