From 7826acc4a77974c241522c5b5ebbaa0a08812654 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Thu, 18 Apr 2019 16:48:01 +0100 Subject: [PATCH] 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. --- .../initializers/014-track-setting-changes.rb | 16 ++-- lib/discourse_event.rb | 3 + lib/site_setting_extension.rb | 4 + .../components/site_setting_extension_spec.rb | 73 +++++++++++++++++++ 4 files changed, 86 insertions(+), 10 deletions(-) diff --git a/config/initializers/014-track-setting-changes.rb b/config/initializers/014-track-setting-changes.rb index e55052fcacb..41a7374571c 100644 --- a/config/initializers/014-track-setting-changes.rb +++ b/config/initializers/014-track-setting-changes.rb @@ -1,19 +1,15 @@ -# 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' +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. + 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", diff --git a/lib/discourse_event.rb b/lib/discourse_event.rb index 6a45571bf25..c61a70b031e 100644 --- a/lib/discourse_event.rb +++ b/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 diff --git a/lib/site_setting_extension.rb b/lib/site_setting_extension.rb index 9c545245ee0..e62ca8a4419 100644 --- a/lib/site_setting_extension.rb +++ b/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! diff --git a/spec/components/site_setting_extension_spec.rb b/spec/components/site_setting_extension_spec.rb index daf158bbe8b..7593d0f514f 100644 --- a/spec/components/site_setting_extension_spec.rb +++ b/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)