FIX: Use `saved_change_to_value?` in site_setting_saved event

FIX: Use saved_change_to_value? in site_setting_saved event

Since Rails 5.2, the behavior of attribute_changed? inside after_save callbacks has changed, so we need to use saved_change_to_attribute instead. The site setting local_process_provider in test mode was covering up the issue.

diff --git a/config/initializers/014-track-setting-changes.rb b/config/initializers/014-track-setting-changes.rb
index f4836d9..e55052f 100644
--- a/config/initializers/014-track-setting-changes.rb
+++ b/config/initializers/014-track-setting-changes.rb
@@ -2,7 +2,7 @@
 # existing users are approved.
 DiscourseEvent.on(:site_setting_saved) do |site_setting|
   name = site_setting.name.to_sym
-  next unless site_setting.value_changed?
+  next unless site_setting.saved_change_to_value?
 
   if name == :must_approve_users && site_setting.value == 't'
     User.where(approved: false).update_all(approved: true)
diff --git a/lib/site_settings/local_process_provider.rb b/lib/site_settings/local_process_provider.rb
index dd9a1ce..aaff9f8 100644
--- a/lib/site_settings/local_process_provider.rb
+++ b/lib/site_settings/local_process_provider.rb
@@ -8,7 +8,7 @@ class SiteSettings::LocalProcessProvider
     attr_accessor :name, :data_type, :value
 
     def value_changed?
-      true
+      false
     end
 
     def saved_change_to_value?

GitHub sha: c687e2b9

1 Like

Should we add a spec for this here?

The whole of site settings is stubbed in test mode, and the stub was returning an incorrect value. I have corrected the stub in this commit, so that it matches the actual behaviour.

Ideally we would remove all the stubbing, but I think it would slow down the test suite a lot :thinking:

To be pedantic, it’s not stubbed but there is a totally separate provider used in test mode that doesn’t touch the database. I assume @SamSaffron did this for performance reasons.

3 Likes

I’ve been fighting with this site_setting_saved event during my work on logo resizing, so I’ve made a PR to replace it with something better.

Merged in: DEV: Replace site_setting_saved DiscourseEvent with site_setting_changed (#7401)

1 Like