refactoring of site settings

lib/site_setting_extension.rb

@@ -90,8 +90,6 @@ module SiteSettingExtension
     mutex.synchronize do
       ensure_listen_for_changes
       old = current
-      changes = []
-      deletions = []
 
       all_settings = SiteSetting.select([:name,:value,:data_type])
       new_hash =  Hash[*(all_settings.map{|s| [s.name.intern, convert(s.value,s.data_type)]}.to_a.flatten)]
@@ -99,13 +97,7 @@ module SiteSettingExtension
       # add defaults
       new_hash = defaults.merge(new_hash)
 
-      new_hash.each do |name, value|
+      changes,deletions = diff_hash(new_hash, old)
-        changes << [name,value] if !old.has_key?(name) || old[name] != value
-      end
-
-      old.each do |name,value|
-        deletions << [name,value] unless new_hash.has_key?(name)
-      end
 
       if deletions.length > 0 || changes.length > 0
         @current = new_hash
@@ -121,26 +113,29 @@ module SiteSettingExtension
     end
   end
 
+
   def ensure_listen_for_changes
     unless @subscribed
-      pid = process_id
+      MessageBus.subscribe("/site_settings") do |message|
-      MessageBus.subscribe("/site_settings") do |msg|
+        process_message(message)
-        message = msg.data
-        if message["process"] != pid
-          begin
-            @last_message_processed = msg.global_id
-            # picks a db
-            MessageBus.on_connect.call(msg.site_id)
-            SiteSetting.refresh!
-          ensure
-            MessageBus.on_disconnect.call(msg.site_id)
-          end
-        end
       end
       @subscribed = true
     end
   end
 
+  def process_message(message)
+    data = message.data
+    if data["process"] != process_id
+      begin
+        @last_message_processed = message.global_id
+        MessageBus.on_connect.call(message.site_id)
+        SiteSetting.refresh!
+      ensure
+        MessageBus.on_disconnect.call(message.site_id)
+      end
+    end
+  end
+
   def diags
     {
       last_message_processed: @last_message_processed
@@ -192,16 +187,31 @@ module SiteSettingExtension
 
   protected
 
+  def diff_hash(new_hash, old)
+    changes = []
+    deletions = []
+
+    new_hash.each do |name, value|
+      changes << [name,value] if !old.has_key?(name) || old[name] != value
+    end
+
+    old.each do |name,value|
+      deletions << [name,value] unless new_hash.has_key?(name)
+    end
+
+    [changes,deletions]
+  end
+
   def get_data_type(name,val)
     return types[:null] if val.nil?
+    return types[:enum] if enums[name]
 
-    if enums[name]
+    case val
-      types[:enum]
+    when String
-    elsif String === val
       types[:string]
-    elsif Fixnum === val
+    when Fixnum
       types[:fixnum]
-    elsif TrueClass === val || FalseClass === val
+    when TrueClass, FalseClass
       types[:bool]
     else
       raise ArgumentError.new :val

spec/models/site_setting_spec.rb

@@ -1,4 +1,6 @@
 require 'spec_helper'
+require_dependency 'site_setting'
+require_dependency 'site_setting_extension'
 
 describe SiteSetting do
 
@@ -85,7 +87,7 @@ describe SiteSetting do
     end
 
     context "when overridden" do
-      after :each do 
+      after :each do
         SiteSetting.remove_override!(:test_hello?)
       end
 
@@ -175,7 +177,7 @@ describe SiteSetting do
 
   describe 'topic_title_length' do
     it 'returns a range of min/max topic title length' do
-      SiteSetting.topic_title_length.should == 
+      SiteSetting.topic_title_length.should ==
         (SiteSetting.defaults[:min_topic_title_length]..SiteSetting.defaults[:max_topic_title_length])
     end
   end