diff --git a/lib/site_setting_extension.rb b/lib/site_setting_extension.rb index 22f2f06f75b..05c8234fd42 100644 --- a/lib/site_setting_extension.rb +++ b/lib/site_setting_extension.rb @@ -1,7 +1,18 @@ require_dependency 'enum' +require_dependency 'site_settings/db_provider' module SiteSettingExtension + # part 1 of refactor, centralizing the dependency here + def provider=(val) + @provider = val + refresh! + end + + def provider + @provider ||= SiteSettings::DbProvider.new(SiteSetting) + end + def types @types ||= Enum.new(:string, :time, :fixnum, :float, :bool, :null, :enum) end @@ -12,7 +23,7 @@ module SiteSettingExtension def current @@containers ||= {} - @@containers[RailsMultisite::ConnectionManagement.current_db] ||= {} + @@containers[provider.current_site] ||= {} end def defaults @@ -27,7 +38,10 @@ module SiteSettingExtension mutex.synchronize do self.defaults[name] = default current_value = current.has_key?(name) ? current[name] : default - enums[name] = opts[:enum] if opts[:enum] + if opts[:enum] + enum = opts[:enum] + enums[name] = enum.is_a?(String) ? enum.constantize : enum + end setup_methods(name, current_value) end end @@ -74,29 +88,21 @@ module SiteSettingExtension I18n.t("site_settings.#{setting}") end - # table is not in the db yet, initial migration, etc - def table_exists? - @table_exists = ActiveRecord::Base.connection.table_exists? 'site_settings' if @table_exists == nil - @table_exists - end - def self.client_settings_cache_key "client_settings_json" end # refresh all the site settings def refresh! - return unless table_exists? mutex.synchronize do ensure_listen_for_changes old = current - all_settings = SiteSetting.select([:name,:value,:data_type]) + all_settings = provider.all new_hash = Hash[*(all_settings.map{|s| [s.name.intern, convert(s.value,s.data_type)]}.to_a.flatten)] # add defaults new_hash = defaults.merge(new_hash) - changes,deletions = diff_hash(new_hash, old) if deletions.length > 0 || changes.length > 0 @@ -129,7 +135,7 @@ module SiteSettingExtension begin @last_message_processed = message.global_id MessageBus.on_connect.call(message.site_id) - SiteSetting.refresh! + refresh! ensure MessageBus.on_disconnect.call(message.site_id) end @@ -147,14 +153,11 @@ module SiteSettingExtension end def remove_override!(name) - return unless table_exists? - SiteSetting.where(name: name).destroy_all + provider.destroy(name) + current[name] = defaults[name] end def add_override!(name,val) - return unless table_exists? - - setting = SiteSetting.where(name: name).first type = get_data_type(name, defaults[name]) if type == types[:bool] && val != true && val != false @@ -173,18 +176,10 @@ module SiteSettingExtension raise Discourse::InvalidParameters.new(:value) unless enum_class(name).valid_value?(val) end - if setting - setting.value = val - setting.data_type = type - setting.save - else - SiteSetting.create!(name: name, value: val, data_type: type) - end - + provider.save(name, val, type) @last_message_sent = MessageBus.publish('/site_settings', {process: process_id}) end - protected def diff_hash(new_hash, old) @@ -225,7 +220,7 @@ module SiteSettingExtension when types[:string], types[:enum] value when types[:bool] - value == "t" + value == true || value == "t" || value == "true" when types[:null] nil end @@ -235,16 +230,12 @@ module SiteSettingExtension def setup_methods(name, current_value) # trivial multi db support, we can optimize this later - db = RailsMultisite::ConnectionManagement.current_db - - @@containers ||= {} - @@containers[db] ||= {} - @@containers[db][name] = current_value + current[name] = current_value setter = ("#{name}=").sub("?","") eval "define_singleton_method :#{name} do - c = @@containers[RailsMultisite::ConnectionManagement.current_db] + c = @@containers[provider.current_site] c = c[name] if c c end @@ -265,7 +256,6 @@ module SiteSettingExtension end def enum_class(name) - enums[name] = enums[name].constantize unless enums[name].is_a?(Class) enums[name] end diff --git a/lib/site_settings/db_provider.rb b/lib/site_settings/db_provider.rb new file mode 100644 index 00000000000..bf2a3c3686b --- /dev/null +++ b/lib/site_settings/db_provider.rb @@ -0,0 +1,65 @@ +module SiteSettings; end + +class SiteSettings::DbProvider + + def initialize(model) + @model = model + end + + def all + return [] unless table_exists? + + # note, not leaking out AR records, cause I want all editing to happen + # via this API + SqlBuilder.new("select name, data_type, value from #{@model.table_name}").map_exec(OpenStruct) + end + + def find(name) + return nil unless table_exists? + + # note, not leaking out AR records, cause I want all editing to happen + # via this API + SqlBuilder.new("select name, data_type, value from #{@model.table_name} where name = :name") + .map_exec(OpenStruct, name: name) + .first + end + + def save(name, value, data_type) + + return unless table_exists? + + count = @model.update_all({ + name: name, + value: value, + data_type: data_type, + updated_at: Time.now + }, { + name: name + }) + + if count == 0 + @model.create!(name: name, value: value, data_type: data_type) + end + + true + end + + def destroy(name) + return unless table_exists? + + @model.where(name: name).destroy_all + end + + def current_site + RailsMultisite::ConnectionManagement.current_db + end + + protected + + # table is not in the db yet, initial migration, etc + def table_exists? + @table_exists = ActiveRecord::Base.connection.table_exists? @model.table_name if @table_exists == nil + @table_exists + end + +end diff --git a/lib/site_settings/local_process_provider.rb b/lib/site_settings/local_process_provider.rb new file mode 100644 index 00000000000..3146b96969a --- /dev/null +++ b/lib/site_settings/local_process_provider.rb @@ -0,0 +1,31 @@ +module SiteSettings; end + +class SiteSettings::LocalProcessProvider + + Setting = Struct.new(:name, :value, :data_type) + + def initialize + @settings = {} + end + + def all + @settings.values + end + + def find(name) + @settings[name] + end + + def save(name, value, data_type) + @settings[name] = Setting.new(name,value, data_type) + end + + def destroy(name) + @settings.delete(name) + end + + def current_site + "test" + end + +end diff --git a/spec/components/site_setting_extenstion_spec.rb b/spec/components/site_setting_extenstion_spec.rb new file mode 100644 index 00000000000..37eaf068bf1 --- /dev/null +++ b/spec/components/site_setting_extenstion_spec.rb @@ -0,0 +1,159 @@ +require 'spec_helper' +require_dependency 'site_setting_extension' +require_dependency 'site_settings/local_process_provider' + +describe SiteSettingExtension do + + class FakeSettings + extend SiteSettingExtension + provider = SiteSettings::LocalProcessProvider + end + + let :settings do + FakeSettings + end + + describe "int setting" do + before do + settings.setting(:test_setting, 77) + settings.refresh! + end + + it "should have a key in all_settings" do + settings.all_settings.detect {|s| s[:setting] == :test_setting }.should be_present + end + + it "should have the correct desc" do + I18n.expects(:t).with("site_settings.test_setting").returns("test description") + settings.description(:test_setting).should == "test description" + end + + it "should have the correct default" do + settings.test_setting.should == 77 + end + + context "when overidden" do + after :each do + settings.remove_override!(:test_setting) + end + + it "should have the correct override" do + settings.test_setting = 100 + settings.test_setting.should == 100 + end + + it "should coerce correct string to int" do + settings.test_setting = "101" + settings.test_setting.should.eql? 101 + end + + it "should coerce incorrect string to 0" do + settings.test_setting = "pie" + settings.test_setting.should.eql? 0 + end + + it "should not set default when reset" do + settings.test_setting = 100 + settings.setting(:test_setting, 77) + settings.refresh! + settings.test_setting.should_not == 77 + end + end + end + + describe "remove_override" do + it "correctly nukes overrides" do + settings.setting(:test_override, "test") + settings.test_override = "bla" + settings.remove_override!(:test_override) + expect(settings.test_override).to eq("test") + end + end + + describe "string setting" do + before do + settings.setting(:test_str, "str") + settings.refresh! + end + + it "should have the correct default" do + settings.test_str.should == "str" + end + + context "when overridden" do + after :each do + settings.remove_override!(:test_str) + end + + it "should coerce int to string" do + settings.test_str = 100 + settings.test_str.should.eql? "100" + end + end + end + + describe "bool setting" do + before do + settings.setting(:test_hello?, false) + settings.refresh! + end + + it "should have the correct default" do + settings.test_hello?.should == false + end + + context "when overridden" do + after do + settings.remove_override!(:test_hello?) + end + + it "should have the correct override" do + settings.test_hello = true + settings.test_hello?.should == true + end + + it "should coerce true strings to true" do + settings.test_hello = "true" + settings.test_hello?.should.eql? true + end + + it "should coerce all other strings to false" do + settings.test_hello = "f" + settings.test_hello?.should.eql? false + end + + it "should not set default when reset" do + settings.test_hello = true + settings.setting(:test_hello?, false) + settings.refresh! + settings.test_hello?.should_not == false + end + end + end + + describe 'enum setting' do + before do + @enum_class = Enum.new(:test) + settings.setting(:test_enum, 'en', enum: @enum_class) + settings.refresh! + end + + it 'should have the correct default' do + expect(settings.test_enum).to eq('en') + end + + context 'when overridden' do + + it 'stores valid values' do + @enum_class.expects(:valid_value?).with('fr').returns(true) + settings.test_enum = 'fr' + expect(settings.test_enum).to eq('fr') + end + + it 'rejects invalid values' do + @enum_class.expects(:valid_value?).with('gg').returns(false) + expect {settings.test_enum = 'gg' }.to raise_error(Discourse::InvalidParameters) + end + end + end +end diff --git a/spec/components/site_settings/db_provider_spec.rb b/spec/components/site_settings/db_provider_spec.rb new file mode 100644 index 00000000000..ded17ce96d6 --- /dev/null +++ b/spec/components/site_settings/db_provider_spec.rb @@ -0,0 +1,51 @@ +require 'spec_helper' +require_dependency 'site_settings/db_provider' + +describe SiteSettings::DbProvider do + + def expect_same_setting(actual, expected) + expect(actual.name).to eq(expected.name) + expect(actual.value).to eq(expected.value) + expect(actual.data_type).to eq(expected.data_type) + end + + let :provider do + SiteSettings::DbProvider.new(SiteSetting) + end + + # integration test, requires db access + it "act correctly" do + setting = Struct.new(:name, :value, :data_type) + + SiteSetting.destroy_all + + expect(provider.all.length).to eq(0) + expect(provider.find("test")).to eq(nil) + + + provider.save("test", "one", 1) + found = provider.find("test") + + expect_same_setting(found, setting.new("test", "one", 1)) + + provider.save("test", "two", 2) + found = provider.find("test") + + expect_same_setting(found, setting.new("test", "two", 2)) + + provider.save("test2", "three", 3) + + all = provider.all.sort{|a,b| a.name <=> b.name} + + expect_same_setting(all[0], setting.new("test", "two", 2)) + expect_same_setting(all[1], setting.new("test2", "three", 3)) + expect(all.length).to eq(2) + + provider.destroy("test") + expect(provider.all.length).to eq(1) + end + + it "returns the correct site name" do + expect(provider.current_site).to eq(RailsMultisite::ConnectionManagement.current_db) + end +end diff --git a/spec/components/site_settings/local_process_provider_spec.rb b/spec/components/site_settings/local_process_provider_spec.rb new file mode 100644 index 00000000000..e4db9c90f6f --- /dev/null +++ b/spec/components/site_settings/local_process_provider_spec.rb @@ -0,0 +1,70 @@ +require 'spec_helper' +require_dependency 'site_settings/local_process_provider' + +describe SiteSettings::LocalProcessProvider do + + def expect_same_setting(actual, expected) + expect(actual.name).to eq(expected.name) + expect(actual.value).to eq(expected.value) + expect(actual.data_type).to eq(expected.data_type) + end + + let :provider do + SiteSettings::LocalProcessProvider.new + end + + + def setting(name,value,data_type) + OpenStruct.new.tap do |setting| + setting.name = name + setting.value = value + setting.data_type = data_type + end + end + + describe "all" do + it "starts off empty" do + expect(provider.all).to eq([]) + end + + it "can allows additional settings" do + provider.save("test", "bla", 2) + expect_same_setting(provider.all[0], setting("test","bla",2)) + end + + it "does not leak new stuff into list" do + provider.save("test", "bla", 2) + provider.save("test", "bla1", 2) + expect_same_setting(provider.all[0], setting("test","bla1",2)) + expect(provider.all.length).to eq(1) + end + end + + describe "find" do + it "starts off empty" do + expect(provider.find("bla")).to eq(nil) + end + + it "can find a new setting" do + provider.save("one","two",3) + expect_same_setting(provider.find("one"),setting("one", "two", 3)) + end + + it "can amend a setting" do + provider.save("one","three",4) + expect_same_setting(provider.find("one"),setting("one", "three", 4)) + end + end + + describe "destroy" do + it "can destroy a setting" do + provider.save("one","three",4) + provider.destroy("one") + expect(provider.find("one")).to eq(nil) + end + end + + it "returns the correct site name" do + expect(provider.current_site).to eq("test") + end +end diff --git a/spec/models/site_setting_spec.rb b/spec/models/site_setting_spec.rb index d3aa41f09dd..103eaac56ae 100644 --- a/spec/models/site_setting_spec.rb +++ b/spec/models/site_setting_spec.rb @@ -4,173 +4,29 @@ require_dependency 'site_setting_extension' describe SiteSetting do - describe "int setting" do - before :all do - SiteSetting.setting(:test_setting, 77) - SiteSetting.refresh! - end - - it "should have a key in all_settings" do - SiteSetting.all_settings.detect {|s| s[:setting] == :test_setting }.should be_present - end - - it "should have the correct desc" do - I18n.expects(:t).with("site_settings.test_setting").returns("test description") - SiteSetting.description(:test_setting).should == "test description" - end - - it "should have the correct default" do - SiteSetting.test_setting.should == 77 - end - - context "when overidden" do - after :each do - SiteSetting.remove_override!(:test_setting) - end - - it "should have the correct override" do - SiteSetting.test_setting = 100 - SiteSetting.test_setting.should == 100 - end - - it "should coerce correct string to int" do - SiteSetting.test_setting = "101" - SiteSetting.test_setting.should.eql? 101 - end - - #POSSIBLE BUG - it "should coerce incorrect string to 0" do - SiteSetting.test_setting = "pie" - SiteSetting.test_setting.should.eql? 0 - end - - #POSSIBLE BUG - it "should not set default when reset" do - SiteSetting.test_setting = 100 - SiteSetting.setting(:test_setting, 77) - SiteSetting.refresh! - SiteSetting.test_setting.should_not == 77 - end - end - end - - describe "string setting" do - before :all do - SiteSetting.setting(:test_str, "str") - SiteSetting.refresh! - end - - it "should have the correct default" do - SiteSetting.test_str.should == "str" - end - - context "when overridden" do - after :each do - SiteSetting.remove_override!(:test_str) - end - - it "should coerce int to string" do - SiteSetting.test_str = 100 - SiteSetting.test_str.should.eql? "100" - end - end - end - - describe "bool setting" do - before :all do - SiteSetting.setting(:test_hello?, false) - SiteSetting.refresh! - end - - it "should have the correct default" do - SiteSetting.test_hello?.should == false - end - - context "when overridden" do - after :each do - SiteSetting.remove_override!(:test_hello?) - end - - it "should have the correct override" do - SiteSetting.test_hello = true - SiteSetting.test_hello?.should == true - end - - it "should coerce true strings to true" do - SiteSetting.test_hello = "true" - SiteSetting.test_hello?.should.eql? true - end - - it "should coerce all other strings to false" do - SiteSetting.test_hello = "f" - SiteSetting.test_hello?.should.eql? false - end - - #POSSIBLE BUG - it "should not set default when reset" do - SiteSetting.test_hello = true - SiteSetting.setting(:test_hello?, false) - SiteSetting.refresh! - SiteSetting.test_hello?.should_not == false - end - end - end - - describe 'enum setting' do - before :all do - @enum_class = Class.new - SiteSetting.setting(:test_enum, 'en', enum: @enum_class) - SiteSetting.refresh! - end - - it 'should have the correct default' do - expect(SiteSetting.test_enum).to eq('en') - end - - context 'when overridden' do - after :each do - SiteSetting.remove_override!(:test_enum) - end - - it 'stores valid values' do - @enum_class.expects(:valid_value?).with('fr').returns(true) - SiteSetting.test_enum = 'fr' - expect(SiteSetting.test_enum).to eq('fr') - end - - it 'rejects invalid values' do - @enum_class.expects(:valid_value?).with('gg').returns(false) - expect { SiteSetting.test_enum = 'gg' }.to raise_error(Discourse::InvalidParameters) - end - end - end - describe 'call_discourse_hub?' do it 'should be true when enforce_global_nicknames is true and discourse_org_access_key is set' do SiteSetting.enforce_global_nicknames = true + SiteSetting.enforce_global_nicknames.should == true SiteSetting.discourse_org_access_key = 'asdfasfsafd' - SiteSetting.refresh! SiteSetting.call_discourse_hub?.should == true end it 'should be false when enforce_global_nicknames is false and discourse_org_access_key is set' do SiteSetting.enforce_global_nicknames = false SiteSetting.discourse_org_access_key = 'asdfasfsafd' - SiteSetting.refresh! SiteSetting.call_discourse_hub?.should == false end it 'should be false when enforce_global_nicknames is true and discourse_org_access_key is not set' do SiteSetting.enforce_global_nicknames = true SiteSetting.discourse_org_access_key = '' - SiteSetting.refresh! SiteSetting.call_discourse_hub?.should == false end it 'should be false when enforce_global_nicknames is false and discourse_org_access_key is not set' do SiteSetting.enforce_global_nicknames = false SiteSetting.discourse_org_access_key = '' - SiteSetting.refresh! SiteSetting.call_discourse_hub?.should == false end end @@ -194,4 +50,20 @@ describe SiteSetting do end end + describe 'in test we do some judo to ensure SiteSetting is always reset between tests' do + + it 'is always the correct default' do + expect(SiteSetting.contact_email).to eq('') + end + + it 'sets a setting' do + SiteSetting.contact_email = 'sam@sam.com' + end + + it 'is always the correct default' do + expect(SiteSetting.contact_email).to eq('') + end + + end + end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 0a8fd70abb8..e9f2a0ffc41 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -59,6 +59,7 @@ Spork.prefork do config.fail_fast = ENV['RSPEC_FAIL_FAST'] == "1" config.include Helpers config.mock_framework = :mocha + config.order = 'random' # If you're not using ActiveRecord, or you'd prefer not to run each of your # examples within a transaction, remove the following line or assign false @@ -77,10 +78,15 @@ Spork.prefork do config.before do # disable all observers, enable as needed during specs ActiveRecord::Base.observers.disable :all + SiteSetting.provider.all.each do |setting| + SiteSetting.remove_override!(setting.name) + end end config.before(:all) do DiscoursePluginRegistry.clear + require_dependency 'site_settings/local_process_provider' + SiteSetting.provider = SiteSettings::LocalProcessProvider.new end end @@ -114,6 +120,7 @@ Spork.each_run do $redis.client.reconnect Rails.cache.reconnect MessageBus.after_fork + end def build(*args)