Stop using global vars where not needed

Simplify local process site setting provider
Stop setting up methods over and over
Centralize cache clearing

app/models/site_setting.rb

@@ -23,7 +23,7 @@ class SiteSetting < ActiveRecord::Base
     load_settings(file)
   end
 
-  SiteSettingExtension.class_variable_get(:@@client_settings) << :available_locales
+  client_settings << :available_locales
 
   def self.available_locales
     LocaleSiteSetting.values.map{ |e| e[:value] }.join('|')

lib/site_setting_extension.rb

@@ -22,8 +22,8 @@ module SiteSettingExtension
   end
 
   def current
-    @@containers ||= {}
-    @@containers[provider.current_site] ||= {}
+    @containers ||= {}
+    @containers[provider.current_site] ||= {}
   end
 
   def defaults
@@ -62,6 +62,8 @@ module SiteSettingExtension
       if opts[:refresh] == true
         refresh_settings << name
       end
+
+      current[name] = current_value
       setup_methods(name, current_value)
     end
   end
@@ -69,12 +71,12 @@ module SiteSettingExtension
   # just like a setting, except that it is available in javascript via DiscourseSession
   def client_setting(name, default = nil, opts = {})
     setting(name, default, opts)
-    @@client_settings ||= []
-    @@client_settings << name
+    @client_settings ||= []
+    @client_settings << name
   end
 
   def client_settings
-    @@client_settings
+    @client_settings ||= []
   end
 
   def settings_hash
@@ -92,7 +94,7 @@ module SiteSettingExtension
   end
 
   def client_settings_json_uncached
-    MultiJson.dump(Hash[*@@client_settings.map{|n| [n, self.send(n)]}.flatten])
+    MultiJson.dump(Hash[*@client_settings.map{|n| [n, self.send(n)]}.flatten])
   end
 
   # Retrieve all settings
@@ -125,24 +127,24 @@ module SiteSettingExtension
       ensure_listen_for_changes
       old = current
 
-      all_settings = provider.all
-      new_hash =  Hash[*(all_settings.map{|s| [s.name.intern, convert(s.value,s.data_type)]}.to_a.flatten)]
+      new_hash =  Hash[*(provider.all.map{ |s|
+        [s.name.intern, convert(s.value,s.data_type)]
+      }.to_a.flatten)]
 
-      # add defaults
+      # add defaults, cause they are cached
       new_hash = defaults.merge(new_hash)
+
       changes,deletions = diff_hash(new_hash, old)
 
       if deletions.length > 0 || changes.length > 0
-        @current = new_hash
         changes.each do |name, val|
-          setup_methods name, val
+          current[name] = val
         end
         deletions.each do |name,val|
-          setup_methods name, defaults[name]
+          current[name] = defaults[name]
         end
       end
-
-      Rails.cache.delete(SiteSettingExtension.client_settings_cache_key)
+      clear_cache!
     end
   end
 
@@ -176,16 +178,18 @@ module SiteSettingExtension
   end
 
   def process_id
-    @@process_id ||= SecureRandom.uuid
+    @process_id ||= SecureRandom.uuid
   end
 
   def after_fork
-    @@process_id = nil
+    @process_id = nil
+    ensure_listen_for_changes
   end
 
   def remove_override!(name)
     provider.destroy(name)
     current[name] = defaults[name]
+    clear_cache!
   end
 
   def add_override!(name,val)
@@ -208,6 +212,9 @@ module SiteSettingExtension
     end
 
     provider.save(name, val, type)
+    current[name] = convert(val, type)
+    clear_cache!
+
     @last_message_sent = MessageBus.publish('/site_settings', {process: process_id})
   end
 
@@ -230,6 +237,10 @@ module SiteSettingExtension
 
   protected
 
+  def clear_cache!
+    Rails.cache.delete(SiteSettingExtension.client_settings_cache_key)
+  end
+
   def diff_hash(new_hash, old)
     changes = []
     deletions = []
@@ -277,14 +288,16 @@ module SiteSettingExtension
 
   def setup_methods(name, current_value)
 
-    # trivial multi db support, we can optimize this later
-    current[name] = current_value
     clean_name = name.to_s.sub("?", "")
 
     eval "define_singleton_method :#{clean_name} do
-      c = @@containers[provider.current_site]
-      c = c[name] if c
-      c
+      c = @containers[provider.current_site]
+      if c
+        c[name]
+      else
+        refresh!
+        current[name]
+      end
     end
 
     define_singleton_method :#{clean_name}? do
@@ -293,7 +306,6 @@ module SiteSettingExtension
 
     define_singleton_method :#{clean_name}= do |val|
       add_override!(:#{name}, val)
-      refresh!
     end
     "
   end

lib/site_settings/local_process_provider.rb

@@ -2,34 +2,37 @@ module SiteSettings; end
 
 class SiteSettings::LocalProcessProvider
 
+  attr_accessor :current_site
+
   Setting = Struct.new(:name, :value, :data_type) unless defined? SiteSettings::LocalProcessProvider::Setting
 
-  def initialize(defaults = {})
-    @settings = {}
-    @defaults = {}
-    defaults.each do |name,(value,data_type)|
-      @defaults[name] = Setting.new(name,value,data_type)
+  def settings
+    @settings[current_site] ||= {}
   end
+
+  def initialize()
+    @settings = {}
+    self.current_site = "test"
   end
 
   def all
-    (@defaults.merge @settings).values
+    settings.values
   end
 
   def find(name)
-    @settings[name] || @defaults[name]
+    settings[name]
   end
 
   def save(name, value, data_type)
-    @settings[name] = Setting.new(name,value, data_type)
+    settings[name] = Setting.new(name,value, data_type)
   end
 
   def destroy(name)
-    @settings.delete(name)
+    settings.delete(name)
   end
 
-  def current_site
-    "test"
+  def clear
+    @settings[current_site] = {}
   end
 
 end

spec/components/site_setting_extension_spec.rb

@@ -6,13 +6,73 @@ describe SiteSettingExtension do
 
   class FakeSettings
     extend SiteSettingExtension
-    provider = SiteSettings::LocalProcessProvider
+    self.provider = SiteSettings::LocalProcessProvider.new
+  end
+
+  class FakeSettings2
+    extend SiteSettingExtension
+    self.provider = FakeSettings.provider
   end
 
   let :settings do
     FakeSettings
   end
 
+  let :settings2 do
+    FakeSettings2
+  end
+
+  describe "refresh!" do
+
+    it "will reset to default if provider vanishes" do
+      settings.setting(:hello, 1)
+      settings.hello = 100
+      settings.hello.should == 100
+
+      settings.provider.clear
+      settings.refresh!
+
+      settings.hello.should == 1
+    end
+
+    it "will set to new value if provider changes" do
+
+      settings.setting(:hello, 1)
+      settings.hello = 100
+      settings.hello.should == 100
+
+      settings.provider.save(:hello, 99, SiteSetting.types[:fixnum] )
+      settings.refresh!
+
+      settings.hello.should == 99
+    end
+
+    it "Publishes changes cross sites" do
+      settings.setting(:hello, 1)
+      settings2.setting(:hello, 1)
+
+      settings.hello = 100
+
+      settings2.refresh!
+      settings2.hello.should == 100
+
+      settings.hello = 99
+
+      settings2.refresh!
+      settings2.hello.should == 99
+    end
+
+  end
+
+  describe "multisite" do
+    it "has no db cross talk" do
+      settings.setting(:hello, 1)
+      settings.hello = 100
+      settings.provider.current_site = "boom"
+      settings.hello.should == 1
+    end
+  end
+
   describe "int setting" do
     before do
       settings.setting(:test_setting, 77)