From f331d2603d2f7690c48b974d3aa3df1f398d7571 Mon Sep 17 00:00:00 2001 From: Sam Date: Fri, 1 Jun 2018 12:22:47 +1000 Subject: [PATCH] DEV: improve design of site setting default provider This refactors it so "Defaults provider" is only responsible for "defaults" Locale handling and management of locale settings is moved back into SiteSettingExtension This eliminates complex state management using DistributedCache and makes it way easier to test SiteSettingExtension --- lib/site_setting_extension.rb | 90 ++++++++-- lib/site_settings/defaults_provider.rb | 100 +++-------- .../components/site_setting_extension_spec.rb | 9 + .../site_settings/defaults_provider_spec.rb | 165 +----------------- .../admin/site_settings_controller_spec.rb | 16 +- spec/multisite/jobs_spec.rb | 1 - spec/rails_helper.rb | 1 - 7 files changed, 122 insertions(+), 260 deletions(-) diff --git a/lib/site_setting_extension.rb b/lib/site_setting_extension.rb index 8efee070324..4e525ff72a9 100644 --- a/lib/site_setting_extension.rb +++ b/lib/site_setting_extension.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require_dependency 'site_settings/deprecated_settings' require_dependency 'site_settings/type_supervisor' require_dependency 'site_settings/defaults_provider' @@ -5,12 +7,54 @@ require_dependency 'site_settings/db_provider' module SiteSettingExtension include SiteSettings::DeprecatedSettings - extend Forwardable - def_delegator :defaults, :site_locale, :default_locale - def_delegator :defaults, :site_locale=, :default_locale= - def_delegator :defaults, :has_setting? - def_delegators 'SiteSettings::TypeSupervisor', :types, :supported_types + # support default_locale being set via global settings + # this also adds support for testing the extension and global settings + # for site locale + def self.extended(klass) + if GlobalSetting.respond_to?(:default_locale) && GlobalSetting.default_locale.present? + klass.send :setup_shadowed_methods, :default_locale, GlobalSetting.default_locale + end + end + + # we need a default here to support defaults per locale + def default_locale=(val) + val = val.to_s + raise Discourse::InvalidParameters.new(:value) unless LocaleSiteSetting.valid_value?(val) + if val != self.default_locale + add_override!(:default_locale, val) + refresh! + Discourse.request_refresh! + end + end + + def default_locale? + true + end + + # set up some sort of default so we can look stuff up + def default_locale + # note optimised cause this is called a lot so avoiding .presence which + # adds 2 method calls + locale = current[:default_locale] + if locale && !locale.blank? + locale + else + SiteSettings::DefaultsProvider::DEFAULT_LOCALE + end + end + + def has_setting?(v) + defaults.has_setting?(v) + end + + def supported_types + SiteSettings::TypeSupervisor.supported_types + end + + def types + SiteSettings::TypeSupervisor.types + end def listen_for_changes=(val) @listen_for_changes = val @@ -55,11 +99,11 @@ module SiteSettingExtension end def refresh_settings - @refresh_settings ||= [] + @refresh_settings ||= [:default_locale] end def client_settings - @client_settings ||= [] + @client_settings ||= [:default_locale] end def previews @@ -73,13 +117,17 @@ module SiteSettingExtension def setting(name_arg, default = nil, opts = {}) name = name_arg.to_sym + if name == :default_locale + raise ArgumentError.new("Other settings depend on default locale, you can not configure it like this") + end + shadowed_val = nil mutex.synchronize do defaults.load_setting( name, default, - opts.extract!(*SiteSettings::DefaultsProvider::CONSUMED_OPTS) + opts.delete(:locale_default) ) categories[name] = opts[:category] || :uncategorized @@ -129,7 +177,7 @@ module SiteSettingExtension def settings_hash result = {} - defaults.each_key do |s| + defaults.all.keys.each do |s| result[s] = send(s).to_s end result @@ -147,14 +195,28 @@ module SiteSettingExtension # Retrieve all settings def all_settings(include_hidden = false) - defaults + + locale_setting_hash = + { + setting: 'default_locale', + default: SiteSettings::DefaultsProvider::DEFAULT_LOCALE, + category: 'required', + description: description('default_locale'), + type: SiteSetting.types[SiteSetting.types[:enum]], + preview: nil, + value: self.default_locale, + valid_values: LocaleSiteSetting.values, + translate_names: LocaleSiteSetting.translate_names? + } + + defaults.all(default_locale) .reject { |s, _| !include_hidden && hidden_settings.include?(s) } .map do |s, v| value = send(s) opts = { setting: s, description: description(s), - default: defaults[s].to_s, + default: defaults.get(s, default_locale).to_s, value: value.to_s, category: categories[s], preview: previews[s], @@ -162,7 +224,7 @@ module SiteSettingExtension }.merge(type_supervisor.type_hash(s)) opts - end.unshift(defaults.locale_setting_hash) + end.unshift(locale_setting_hash) end def description(setting) @@ -185,7 +247,7 @@ module SiteSettingExtension [s.name.to_sym, type_supervisor.to_rb_value(s.name, s.value, s.data_type)] }.to_a.flatten)] - defaults_view = defaults.all + defaults_view = defaults.all(new_hash[:default_locale]) # add locale default and defaults based on default_locale, cause they are cached new_hash = defaults_view.merge!(new_hash) @@ -242,7 +304,7 @@ module SiteSettingExtension def remove_override!(name) provider.destroy(name) - current[name] = defaults[name] + current[name] = defaults.get(name, default_locale) clear_cache! end diff --git a/lib/site_settings/defaults_provider.rb b/lib/site_settings/defaults_provider.rb index 7b1467a6535..6340ff91b9f 100644 --- a/lib/site_settings/defaults_provider.rb +++ b/lib/site_settings/defaults_provider.rb @@ -1,31 +1,23 @@ +# frozen_string_literal: true + module SiteSettings; end # A cache for providing default value based on site locale class SiteSettings::DefaultsProvider - include Enumerable - - CONSUMED_OPTS = %i[default locale_default].freeze - DEFAULT_LOCALE_KEY = :default_locale - DEFAULT_LOCALE = 'en'.freeze - DEFAULT_CATEGORY = 'required'.freeze - - @@site_locales ||= DistributedCache.new('site_locales') + DEFAULT_LOCALE = 'en' def initialize(site_setting) @site_setting = site_setting - @site_setting.refresh_settings << DEFAULT_LOCALE_KEY @defaults = {} @defaults[DEFAULT_LOCALE.to_sym] = {} - - refresh_site_locale! end - def load_setting(name_arg, value, opts = {}) + def load_setting(name_arg, value, locale_defaults) name = name_arg.to_sym @defaults[DEFAULT_LOCALE.to_sym][name] = value - if (locale_default = opts[:locale_default]) - locale_default.each do |locale, v| + if (locale_defaults) + locale_defaults.each do |locale, v| locale = locale.to_sym @defaults[locale] ||= {} @defaults[locale][name] = v @@ -34,15 +26,19 @@ class SiteSettings::DefaultsProvider end def db_all - @site_setting.provider.all.delete_if { |s| s.name.to_sym == DEFAULT_LOCALE_KEY } + @site_setting.provider.all end - def all - @defaults[DEFAULT_LOCALE.to_sym].merge(@defaults[self.site_locale.to_sym] || {}) + def all(locale = nil) + if locale + @defaults[DEFAULT_LOCALE.to_sym].merge(@defaults[locale.to_sym] || {}) + else + @defaults[DEFAULT_LOCALE.to_sym].dup + end end - def get(name) - @defaults.dig(self.site_locale.to_sym, name.to_sym) || + def get(name, locale = DEFAULT_LOCALE) + @defaults.dig(locale.to_sym, name.to_sym) || @defaults.dig(DEFAULT_LOCALE.to_sym, name.to_sym) end alias [] get @@ -50,81 +46,25 @@ class SiteSettings::DefaultsProvider # Used to override site settings in dev/test env def set_regardless_of_locale(name, value) name = name.to_sym - if @site_setting.has_setting?(name) + if name == :default_locale || @site_setting.has_setting?(name) @defaults.each { |_, hash| hash.delete(name) } @defaults[DEFAULT_LOCALE.to_sym][name] = value value, type = @site_setting.type_supervisor.to_db_value(name, value) - @defaults[self.site_locale.to_sym] ||= {} - @defaults[self.site_locale.to_sym][name] = @site_setting.type_supervisor.to_rb_value(name, value, type) + @defaults[SiteSetting.default_locale.to_sym] ||= {} + @defaults[SiteSetting.default_locale.to_sym][name] = @site_setting.type_supervisor.to_rb_value(name, value, type) else raise ArgumentError.new("No setting named '#{name}' exists") end end - def site_locale - @@site_locales[current_db] - end - - def site_locale=(val) - val = val.to_s - raise Discourse::InvalidParameters.new(:value) unless LocaleSiteSetting.valid_value?(val) - - if val != @@site_locales[current_db] - @site_setting.provider.save(DEFAULT_LOCALE_KEY, val, SiteSetting.types[:string]) - refresh_site_locale! - @site_setting.refresh! - Discourse.request_refresh! - end - - @@site_locales[current_db] - end - - def each(&block) - self.all.each do |key, value| - block.call(key.to_sym, value) - end - end - - def locale_setting_hash - { - setting: DEFAULT_LOCALE_KEY, - default: DEFAULT_LOCALE, - category: DEFAULT_CATEGORY, - description: @site_setting.description(DEFAULT_LOCALE_KEY), - type: SiteSetting.types[SiteSetting.types[:enum]], - preview: nil, - value: @@site_locales[current_db], - valid_values: LocaleSiteSetting.values, - translate_names: LocaleSiteSetting.translate_names? - } - end - - def refresh_site_locale! - RailsMultisite::ConnectionManagement.each_connection do |db| - @@site_locales[db] = - if GlobalSetting.respond_to?(DEFAULT_LOCALE_KEY) && - (global_val = GlobalSetting.send(DEFAULT_LOCALE_KEY)) && - !global_val.blank? - global_val - elsif (db_val = @site_setting.provider.find(DEFAULT_LOCALE_KEY)) - db_val.value.to_s - else - DEFAULT_LOCALE - end - - @@site_locales[db] - end - end - def has_setting?(name) - has_key?(name.to_sym) || has_key?("#{name.to_s}?".to_sym) + has_key?(name.to_sym) || has_key?("#{name.to_s}?".to_sym) || name.to_sym == :default_locale end private def has_key?(name) - @defaults[self.site_locale.to_sym]&.key?(name) || - @defaults[DEFAULT_LOCALE.to_sym].key?(name) || name == DEFAULT_LOCALE_KEY + @defaults[DEFAULT_LOCALE.to_sym].key?(name) end def current_db diff --git a/spec/components/site_setting_extension_spec.rb b/spec/components/site_setting_extension_spec.rb index 1c8491c0099..f246a5588b9 100644 --- a/spec/components/site_setting_extension_spec.rb +++ b/spec/components/site_setting_extension_spec.rb @@ -505,6 +505,15 @@ describe SiteSettingExtension do end describe "shadowed_by_global" do + + context "default_locale" do + it "supports adding a default locale via a global" do + global_setting :default_locale, 'zh_CN' + settings.default_locale = 'en' + expect(settings.default_locale).to eq('zh_CN') + end + end + context "without global setting" do before do settings.setting(:trout_api_key, 'evil', shadowed_by_global: true) diff --git a/spec/components/site_settings/defaults_provider_spec.rb b/spec/components/site_settings/defaults_provider_spec.rb index e19b369f2bd..e4ca9a91091 100644 --- a/spec/components/site_settings/defaults_provider_spec.rb +++ b/spec/components/site_settings/defaults_provider_spec.rb @@ -26,20 +26,7 @@ describe SiteSettings::DefaultsProvider do new_settings(provider_local) end - describe 'inserts default_locale into refresh' do - it 'when initialize' do - expect(settings.refresh_settings.include?(SiteSettings::DefaultsProvider::DEFAULT_LOCALE_KEY)).to be_truthy - end - end - describe '.db_all' do - it 'collects values from db except default locale' do - settings.provider.save(SiteSettings::DefaultsProvider::DEFAULT_LOCALE_KEY, - 'en', - SiteSetting.types[:string]) - expect(settings.defaults.db_all).to eq([]) - end - it 'can collect values from db' do settings.provider.save('try_a', 1, SiteSetting.types[:integer]) settings.provider.save('try_b', 2, SiteSetting.types[:integer]) @@ -55,11 +42,9 @@ describe SiteSettings::DefaultsProvider do end describe '.all' do - it 'returns all values according to the current locale' do + it 'returns all values according to locale' do expect(settings.defaults.all).to eq(test_override: 'default', test_default: 'test') - settings.defaults.site_locale = 'zh_CN' - settings.defaults.refresh_site_locale! - expect(settings.defaults.all).to eq(test_override: 'cn', test_default: 'test') + expect(settings.defaults.all('zh_CN')).to eq(test_override: 'cn', test_default: 'test') end end @@ -72,11 +57,6 @@ describe SiteSettings::DefaultsProvider do expect(settings.defaults.get('test_override')).to eq 'default' end - it 'returns the default value according to current locale' do - expect(settings.defaults.get(:test_override)).to eq 'default' - settings.defaults.site_locale = 'zh_CN' - expect(settings.defaults.get(:test_override)).to eq 'cn' - end end describe '.set_regardless_of_locale' do @@ -85,8 +65,7 @@ describe SiteSettings::DefaultsProvider do it 'sets the default value to a site setting regardless the locale' do settings.defaults.set_regardless_of_locale(:test_override, val) expect(settings.defaults.get(:test_override)).to eq val - settings.defaults.site_locale = 'zh_CN' - expect(settings.defaults.get(:test_override)).to eq val + expect(settings.defaults.get(:test_override, 'zh_CN')).to eq val end it 'handles the string' do @@ -111,143 +90,13 @@ describe SiteSettings::DefaultsProvider do }.to raise_error(Discourse::InvalidParameters) end end - - describe '.each' do - it 'yields the pair of site settings' do - expect { |b| settings.defaults.each(&b) }.to yield_successive_args([:test_override, 'default'], [:test_default, 'test']) - settings.defaults.site_locale = 'zh_CN' - expect { |b| settings.defaults.each(&b) }.to yield_successive_args([:test_override, 'cn'], [:test_default, 'test']) - end - end - end - - describe '.site_locale' do - it 'returns the current site locale' do - expect(settings.defaults.site_locale).to eq 'en' - end - - context 'when locale is set in the db' do - let(:db_val) { 'zr' } - let(:global_val) { 'gr' } - - before do - settings.provider.save(SiteSettings::DefaultsProvider::DEFAULT_LOCALE_KEY, - db_val, - SiteSetting.types[:string]) - settings.defaults.refresh_site_locale! - end - - it 'should load from database' do - expect(settings.defaults.site_locale).to eq db_val - end - - it 'prioritizes GlobalSetting than value from db' do - GlobalSetting.stubs(:default_locale).returns(global_val) - settings.defaults.refresh_site_locale! - expect(settings.defaults.site_locale).to eq global_val - end - - it 'ignores blank GlobalSetting' do - GlobalSetting.stubs(:default_locale).returns('') - settings.defaults.refresh_site_locale! - expect(settings.defaults.site_locale).to eq db_val - end - end - - end - - describe '.site_locale=' do - it 'should store site locale in a distributed cache' do - expect(settings.defaults.class.class_variable_get(:@@site_locales)) - .to be_a(DistributedCache) - end - - it 'changes and store the current site locale' do - settings.defaults.site_locale = 'zh_CN' - - expect(settings.defaults.site_locale).to eq('zh_CN') - end - - it 'changes and store the current site locale' do - expect { settings.defaults.site_locale = 'random' }.to raise_error(Discourse::InvalidParameters) - expect(settings.defaults.site_locale).to eq 'en' - end - - it "don't change when it's shadowed" do - GlobalSetting.stubs(:default_locale).returns('shadowed') - settings.defaults.site_locale = 'zh_CN' - expect(settings.defaults.site_locale).to eq 'shadowed' - end - - it 'refresh_site_locale! when called' do - settings.defaults.expects(:refresh_site_locale!) - settings.defaults.site_locale = 'zh_CN' - end - - it 'refreshes the client when changed' do - Discourse.expects(:request_refresh!).once - settings.defaults.site_locale = 'zh_CN' - end - - it "doesn't refresh the client when changed" do - Discourse.expects(:request_refresh!).never - settings.defaults.site_locale = 'en' - end - end - - describe '.locale_setting_hash' do - it 'returns the hash for client display' do - result = settings.defaults.locale_setting_hash - - expect(result[:setting]).to eq(SiteSettings::DefaultsProvider::DEFAULT_LOCALE_KEY) - expect(result[:default]).to eq(SiteSettings::DefaultsProvider::DEFAULT_LOCALE) - expect(result[:type]).to eq(SiteSetting.types[SiteSetting.types[:enum]]) - expect(result[:preview]).to be_nil - expect(result[:value]).to eq(SiteSettings::DefaultsProvider::DEFAULT_LOCALE) - expect(result[:category]).to eq(SiteSettings::DefaultsProvider::DEFAULT_CATEGORY) - expect(result[:valid_values]).to eq(LocaleSiteSetting.values) - expect(result[:translate_names]).to eq(LocaleSiteSetting.translate_names?) - expect(result[:description]).not_to be_nil - end end describe '.load_setting' do - it 'adds a setting to the cache' do - settings.defaults.load_setting('new_a', 1) + it 'adds a setting to the cache correctly' do + settings.defaults.load_setting('new_a', 1, zh_CN: 7) expect(settings.defaults[:new_a]).to eq 1 - end - - it 'takes care of locale default' do - settings.defaults.load_setting(:new_b, 1, locale_default: { zh_CN: 2, zh_TW: 2 }) - expect(settings.defaults[:new_b]).to eq 1 - end - end - - describe '.refresh_site_locale!' do - it 'loads the change to locale' do - expect(settings.defaults.site_locale).to eq 'en' - settings.provider.save(SiteSettings::DefaultsProvider::DEFAULT_LOCALE_KEY, - 'zh_CN', - SiteSetting.types[:string]) - settings.defaults.refresh_site_locale! - expect(settings.defaults.site_locale).to eq 'zh_CN' - end - - it 'loads from GlobalSettings' do - expect(settings.defaults.site_locale).to eq 'en' - GlobalSetting.stubs(:default_locale).returns('fr') - settings.defaults.refresh_site_locale! - expect(settings.defaults.site_locale).to eq 'fr' - end - - it 'prioritized GlobalSettings than db' do - expect(settings.defaults.site_locale).to eq 'en' - settings.provider.save(SiteSettings::DefaultsProvider::DEFAULT_LOCALE_KEY, - 'zh_CN', - SiteSetting.types[:string]) - GlobalSetting.stubs(:default_locale).returns('fr') - settings.defaults.refresh_site_locale! - expect(settings.defaults.site_locale).to eq 'fr' + expect(settings.defaults.get(:new_a, 'zh_CN')).to eq 7 end end @@ -266,7 +115,7 @@ describe SiteSettings::DefaultsProvider do end it 'default_locale always exists' do - expect(settings.defaults.has_setting?(SiteSettings::DefaultsProvider::DEFAULT_LOCALE_KEY)).to be_truthy + expect(settings.defaults.has_setting?(:default_locale)).to be_truthy end it 'returns false when the key is not exist' do diff --git a/spec/controllers/admin/site_settings_controller_spec.rb b/spec/controllers/admin/site_settings_controller_spec.rb index b1f6007e932..c65247044c8 100644 --- a/spec/controllers/admin/site_settings_controller_spec.rb +++ b/spec/controllers/admin/site_settings_controller_spec.rb @@ -12,14 +12,18 @@ describe Admin::SiteSettingsController do end context 'index' do - it 'returns success' do + it 'returns valid info' do get :index, format: :json - expect(response).to be_successful - end + json = ::JSON.parse(response.body) + expect(json).to be_present + expect(response.status).to eq(200) + expect(json["site_settings"].length).to be > 100 - it 'returns JSON' do - get :index, format: :json - expect(::JSON.parse(response.body)).to be_present + locale = json["site_settings"].select do |s| + s["setting"] == "default_locale" + end + + expect(locale.length).to eq(1) end end diff --git a/spec/multisite/jobs_spec.rb b/spec/multisite/jobs_spec.rb index 076f6cea54a..2434bda3d49 100644 --- a/spec/multisite/jobs_spec.rb +++ b/spec/multisite/jobs_spec.rb @@ -5,7 +5,6 @@ RSpec.describe "Running Sidekiq Jobs in Multisite" do before do conn.config_filename = "spec/fixtures/multisite/two_dbs.yml" - SiteSetting.defaults.refresh_site_locale! end after do diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index fd977925cd5..e222e65e23e 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -153,7 +153,6 @@ RSpec.configure do |config| SiteSetting.provider.all.each do |setting| SiteSetting.remove_override!(setting.name) end - SiteSetting.defaults.site_locale = SiteSettings::DefaultsProvider::DEFAULT_LOCALE # very expensive IO operations SiteSetting.automatically_download_gravatars = false