DEV: Replace site_setting_saved DiscourseEvent with site_setting_changed (#7401)

* DEV: Replace site_setting_saved DiscourseEvent with site_setting_changed

site_setting_saved is confusing for a few reasons:
- It is attached to the after_save of the ActiveRecord model. This is confusing because it only works 'properly' with the db_provider
- It passes the activerecord model as a parameter, which is confusing because you get access to the 'database' version of the setting, rather than the ruby setting. For example, booleans appear as 'y' or 'n' strings.
- When the event is called, the local process cache has not yet been updated. So if you call SiteSetting.setting_name inside the event handler, you will receive the old site setting value

I have deprecated that event, and added a new site_setting_changed event. It passes three parameters:
- Setting name (symbol)
- Old value (in ruby format)
- New value (in ruby format)

It is triggered after the setting has been persisted, and the local process cache has been updated.

This commit also includes a test case which describes the confusing behavior. This can be removed once site_setting_saved is removed.

config/initializers/014-track-setting-changes.rb

@@ -1,19 +1,15 @@
+DiscourseEvent.on(:site_setting_changed) do |name, old_value, new_value|
   # Enabling `must_approve_users` on an existing site is odd, so we assume that the
   # existing users are approved.
-DiscourseEvent.on(:site_setting_saved) do |site_setting|
-  name = site_setting.name.to_sym
-  next unless site_setting.saved_change_to_value?
-
-  if name == :must_approve_users && site_setting.value == 't'
+  if name == :must_approve_users && new_value == true
     User.where(approved: false).update_all(approved: true)
   end
 
   if name == :emoji_set
     Emoji.clear_cache
 
-    previous_value = site_setting.attribute_in_database(:value) || SiteSetting.defaults[:emoji_set]
-    before = "/images/emoji/#{previous_value}/"
-    after = "/images/emoji/#{site_setting.value}/"
+    before = "/images/emoji/#{old_value}/"
+    after = "/images/emoji/#{new_value}/"
 
     Scheduler::Defer.later("Fix Emoji Links") do
       DB.exec("UPDATE posts SET cooked = REPLACE(cooked, :before, :after) WHERE cooked LIKE :like",

lib/discourse_event.rb

@@ -14,6 +14,9 @@ class DiscourseEvent
   end
 
   def self.on(event_name, &block)
+    if event_name == :site_setting_saved
+      Discourse.deprecate("The :site_setting_saved event is deprecated. Please use :site_setting_changed instead", since: "2.3.0beta8", drop_from: "2.4")
+    end
     events[event_name] << block
   end
 

lib/site_setting_extension.rb

@@ -344,19 +344,23 @@ module SiteSettingExtension
   end
 
   def remove_override!(name)
+    old_val = current[name]
     provider.destroy(name)
     current[name] = defaults.get(name, default_locale)
     clear_uploads_cache(name)
     clear_cache!
+    DiscourseEvent.trigger(:site_setting_changed, name, old_val, current[name]) if old_val != current[name]
   end
 
   def add_override!(name, val)
+    old_val = current[name]
     val, type = type_supervisor.to_db_value(name, val)
     provider.save(name, val, type)
     current[name] = type_supervisor.to_rb_value(name, val)
     clear_uploads_cache(name)
     notify_clients!(name) if client_settings.include? name
     clear_cache!
+    DiscourseEvent.trigger(:site_setting_changed, name, old_val, current[name]) if old_val != current[name]
   end
 
   def notify_changed!

spec/components/site_setting_extension_spec.rb

@@ -152,6 +152,79 @@ describe SiteSettingExtension do
     end
   end
 
+  describe "DiscourseEvent" do
+    before do
+      settings.setting(:test_setting, 1)
+      settings.refresh!
+    end
+
+    it "triggers events correctly" do
+      settings.setting(:test_setting, 1)
+      settings.refresh!
+
+      override_events = DiscourseEvent.track_events { settings.test_setting = 2 }
+      no_change_events = DiscourseEvent.track_events { settings.test_setting = 2 }
+      default_events = DiscourseEvent.track_events { settings.test_setting = 1 }
+
+      expect(override_events.map { |e| e[:event_name] }).to contain_exactly(:site_setting_changed, :site_setting_saved)
+      expect(no_change_events.map { |e| e[:event_name] }).to contain_exactly(:site_setting_saved)
+      expect(default_events.map { |e| e[:event_name] }).to contain_exactly(:site_setting_changed, :site_setting_saved)
+
+      changed_event_1 = override_events.find { |e| e[:event_name] == :site_setting_changed }
+      changed_event_2 = default_events.find { |e| e[:event_name] == :site_setting_changed }
+
+      expect(changed_event_1[:params]).to eq([:test_setting, 1, 2])
+      expect(changed_event_2[:params]).to eq([:test_setting, 2, 1])
+    end
+
+    it "provides the correct values when using site_setting_changed" do
+      event_new_value = nil
+      event_old_value = nil
+      site_setting_value = nil
+
+      test_lambda = -> (name, old_val, new_val) do
+        event_old_value = old_val
+        event_new_value = new_val
+        site_setting_value = settings.test_setting
+      end
+
+      begin
+        DiscourseEvent.on(:site_setting_changed, &test_lambda)
+        settings.test_setting = 2
+      ensure
+        DiscourseEvent.off(:site_setting_changed, &test_lambda)
+      end
+
+      expect(event_old_value).to eq(1)
+      expect(event_new_value).to eq(2)
+      expect(site_setting_value).to eq(2)
+    end
+
+    it "can produce confusing results when using site_setting_saved" do
+      # site_setting_saved is deprecated. This test case illustrates why it can be confusing
+
+      active_record_value = nil
+      site_setting_value = nil
+
+      test_lambda = -> (setting) do
+        active_record_value = setting.value
+        site_setting_value = settings.test_setting
+      end
+
+      begin
+        DiscourseEvent.on(:site_setting_saved, &test_lambda)
+        settings.test_setting = 2
+      ensure
+        DiscourseEvent.off(:site_setting_saved, &test_lambda)
+      end
+
+      # Problem 1, the site_setting_changed event gives us the database value, not the ruby value
+      expect(active_record_value).to eq("2")
+      # Problem 2, calling SiteSetting.test_setting inside the event will still return the old value
+      expect(site_setting_value).to eq(1)
+    end
+  end
+
   describe "int setting" do
     before do
       settings.setting(:test_setting, 77)