From 1646bc00315ad307013506187ca664dc399e5f20 Mon Sep 17 00:00:00 2001 From: Erick Guan Date: Thu, 3 Aug 2017 20:19:02 +0200 Subject: [PATCH] FIX: fails loud if default setting is not set Noted: - `push_api_secret_key` is set in initializer. Shimed with '' - `default_theme_key` is set in seeding. Shimed with '' --- config/site_settings.yml | 2 + lib/site_settings/yaml_loader.rb | 6 ++- .../site_settings/yaml_loader_spec.rb | 5 +++ spec/fixtures/site_settings/nil_default.yml | 3 ++ spec/integrity/site_setting_spec.rb | 42 +++++++++++++++++++ 5 files changed, 56 insertions(+), 2 deletions(-) create mode 100644 spec/fixtures/site_settings/nil_default.yml create mode 100644 spec/integrity/site_setting_spec.rb diff --git a/config/site_settings.yml b/config/site_settings.yml index 802f5747a0f..697b667c707 100644 --- a/config/site_settings.yml +++ b/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 diff --git a/lib/site_settings/yaml_loader.rb b/lib/site_settings/yaml_loader.rb index 787be9848e2..3f53a28a3d3 100644 --- a/lib/site_settings/yaml_loader.rb +++ b/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/.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! diff --git a/spec/components/site_settings/yaml_loader_spec.rb b/spec/components/site_settings/yaml_loader_spec.rb index 52e1952999f..2a56f92686a 100644 --- a/spec/components/site_settings/yaml_loader_spec.rb +++ b/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) diff --git a/spec/fixtures/site_settings/nil_default.yml b/spec/fixtures/site_settings/nil_default.yml new file mode 100644 index 00000000000..6700ffaf3ed --- /dev/null +++ b/spec/fixtures/site_settings/nil_default.yml @@ -0,0 +1,3 @@ +category1: + title: + hidden: true diff --git a/spec/integrity/site_setting_spec.rb b/spec/integrity/site_setting_spec.rb new file mode 100644 index 00000000000..9c52e9fab67 --- /dev/null +++ b/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