FIX: Better handling for toggling `must_approve_users`

FIX: Better handling for toggling must_approve_users

If you turn it on now, default all users to approved since they were previously. Also support approving a user that doesn’t have a reviewable record (it will be created first.)

This also includes a refactor to move class method calls to DiscourseEvent into an initializer. Otherwise the load order of classes makes a difference in the test environment and some settings might be triggered and others not, randomly.

diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index b526e89..976ea91 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -288,7 +288,12 @@ class Admin::UsersController < Admin::AdminController
   end
 
   def approve
-    Reviewable.bulk_perform_targets(current_user, :approve, 'ReviewableUser', [@user.id])
+    guardian.ensure_can_approve!(@user)
+
+    reviewable = ReviewableUser.find_by(target: @user) ||
+      Jobs::CreateUserReviewable.new.execute(user_id: @user.id).reviewable
+
+    reviewable.perform(current_user, :approve)
     render body: nil
   end
 
diff --git a/app/jobs/regular/create_user_reviewable.rb b/app/jobs/regular/create_user_reviewable.rb
index 8d06f94..288ba7f 100644
--- a/app/jobs/regular/create_user_reviewable.rb
+++ b/app/jobs/regular/create_user_reviewable.rb
@@ -1,4 +1,6 @@
 class Jobs::CreateUserReviewable < Jobs::Base
+  attr_reader :reviewable
+
   def execute(args)
     raise Discourse::InvalidParameters unless args[:user_id].present?
 
@@ -11,7 +13,7 @@ class Jobs::CreateUserReviewable < Jobs::Base
     if user = User.find_by(id: args[:user_id])
       return if user.approved?
 
-      reviewable = ReviewableUser.needs_review!(
+      @reviewable = ReviewableUser.needs_review!(
         target: user,
         created_by: Discourse.system_user,
         reviewable_by_moderator: true,
@@ -21,7 +23,7 @@ class Jobs::CreateUserReviewable < Jobs::Base
           email: user.email
         }
       )
-      reviewable.add_score(
+      @reviewable.add_score(
         Discourse.system_user,
         ReviewableScore.types[:needs_approval],
         reason: reason,
diff --git a/app/models/emoji_set_site_setting.rb b/app/models/emoji_set_site_setting.rb
index 003eb77..fcc3759 100644
--- a/app/models/emoji_set_site_setting.rb
+++ b/app/models/emoji_set_site_setting.rb
@@ -2,25 +2,6 @@ require 'enum_site_setting'
 
 class EmojiSetSiteSetting < EnumSiteSetting
 
-  # fix the URLs when changing the site setting
-  DiscourseEvent.on(:site_setting_saved) do |site_setting|
-    if site_setting.name.to_s == "emoji_set" && site_setting.value_changed?
-      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}/"
-
-      Scheduler::Defer.later("Fix Emoji Links") do
-        DB.exec("UPDATE posts SET cooked = REPLACE(cooked, :before, :after) WHERE cooked LIKE :like",
-          before: before,
-          after: after,
-          like: "%#{before}%"
-        )
-      end
-    end
-  end
-
   def self.valid_value?(val)
     values.any? { |v| v[:value] == val.to_s }
   end
diff --git a/app/models/report.rb b/app/models/report.rb
index e5b5e4e..67bcb26 100644
--- a/app/models/report.rb
+++ b/app/models/report.rb
@@ -305,12 +305,6 @@ class Report
     add_counts report, subject, 'topics.created_at'
   end
 
-  DiscourseEvent.on(:site_setting_saved) do |site_setting|
-    if ["backup_location", "s3_backup_bucket"].include?(site_setting.name.to_s)
-      clear_cache(:storage_stats)
-    end
-  end
-
   def rgba_color(hex, opacity = 1)
     if hex.size == 3
       chars = hex.scan(/\w/)
diff --git a/app/models/reviewable_user.rb b/app/models/reviewable_user.rb
index 8feefed..d19f770 100644
--- a/app/models/reviewable_user.rb
+++ b/app/models/reviewable_user.rb
@@ -1,6 +1,7 @@
 require_dependency 'reviewable'
 
 class ReviewableUser < Reviewable
+
   def self.create_for(user)
     create(
       created_by_id: Discourse.system_user.id,
diff --git a/app/models/topic.rb b/app/models/topic.rb
index f7ee82e..e685aa7 100644
--- a/app/models/topic.rb
+++ b/app/models/topic.rb
@@ -33,14 +33,6 @@ class Topic < ActiveRecord::Base
 
   attr_accessor :allowed_user_ids, :tags_changed, :includes_destination_category
 
-  DiscourseEvent.on(:site_setting_saved) do |site_setting|
-    if site_setting.name.to_s == "slug_generation_method" && site_setting.saved_change_to_value?
-      Scheduler::Defer.later("Null topic slug") do
-        Topic.update_all(slug: nil)
-      end
-    end
-  end
-
   def self.max_fancy_title_length
     400
   end
diff --git a/app/serializers/admin_user_serializer.rb b/app/serializers/admin_user_serializer.rb
index 2ed6825..97d587b 100644
--- a/app/serializers/admin_user_serializer.rb
+++ b/app/serializers/admin_user_serializer.rb
@@ -14,8 +14,7 @@ class AdminUserSerializer < AdminUserListSerializer
   has_one :single_sign_on_record, serializer: SingleSignOnRecordSerializer, embed: :objects
 
   def can_approve
-    reviewable = ReviewableUser.find_by(target: object)
-    reviewable.present? && reviewable.actions_for(scope).has?(:approve)
+    scope.can_approve?(object)
   end
 
   def include_can_approve?
diff --git a/config/initializers/014-track-setting-changes.rb b/config/initializers/014-track-setting-changes.rb
new file mode 100644
index 0000000..f4836d9
--- /dev/null
+++ b/config/initializers/014-track-setting-changes.rb
@@ -0,0 +1,38 @@
+# 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.value_changed?
+
+  if name == :must_approve_users && site_setting.value == 't'
+    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}/"
+
+    Scheduler::Defer.later("Fix Emoji Links") do
+      DB.exec("UPDATE posts SET cooked = REPLACE(cooked, :before, :after) WHERE cooked LIKE :like",
+        before: before,
+        after: after,
+        like: "%#{before}%"
+      )
+    end
+  end
+
+  Report.clear_cache(:storage_stats) if [:backup_location, :s3_backup_bucket].include?(name)
+
+  if name == :slug_generation_method
+    Scheduler::Defer.later("Null topic slug") do
+      Topic.update_all(slug: nil)
+    end
+  end
+
+  Jobs.enqueue(:update_s3_inventory) if [:s3_inventory, :s3_upload_bucket].include?(name)
+
+  SvgSprite.expire_cache if name.to_s.include?("_icon")
+end
diff --git a/lib/discourse.rb b/lib/discourse.rb
index 4ef1ebb..a6476dc 100644
--- a/lib/discourse.rb
+++ b/lib/discourse.rb
@@ -468,11 +468,6 @@ module Discourse
     end
   end
 
-  DiscourseEvent.on(:site_setting_saved) do |site_setting|
-    name = site_setting.name.to_s
-    Jobs.enqueue(:update_s3_inventory) if name.include?("s3_inventory") || name == "s3_upload_bucket"
-  end
-
   def self.current_user_provider
     @current_user_provider || Auth::DefaultCurrentUserProvider
   end
diff --git a/lib/guardian.rb b/lib/guardian.rb
index 558de2c..fa24d84 100644
--- a/lib/guardian.rb
+++ b/lib/guardian.rb
@@ -220,7 +220,7 @@ class Guardian
 
   # Can we approve it?
   def can_approve?(target)
-    is_staff? && target && target.active? && not(target.approved?)
+    is_staff? && target && target.active? && !target.approved?
   end
 
   def can_activate?(target)
diff --git a/lib/site_settings/local_process_provider.rb b/lib/site_settings/local_process_provider.rb
index e3974b0..dd9a1ce 100644
--- a/lib/site_settings/local_process_provider.rb
+++ b/lib/site_settings/local_process_provider.rb
@@ -4,7 +4,22 @@ class SiteSettings::LocalProcessProvider
 
   attr_accessor :current_site
 
-  Setting = Struct.new(:name, :value, :data_type) unless defined? SiteSettings::LocalProcessProvider::Setting
+  class Setting
+    attr_accessor :name, :data_type, :value
+
+    def value_changed?
+      true
+    end
+
+    def saved_change_to_value?
+      true
+    end
+
+    def initialize(name, data_type)
+      self.name = name
+      self.data_type = data_type
+    end
+  end
 
   def settings

[... diff too long, it was truncated ...]

GitHub sha: ba6d4b2a

I worry a tiny bit about this safety wise, especially if admins flick it on and off again quickly without understanding the implications.

Eg: you have 10 unapproved accounts, then you turn off the setting by mistake, save, turn it back on, and now the people you did not approve are suddenly approved.

Ideally we should show a warning when we change state:

I am about to approve pre-existing 102 accounts

[OK]

Though yeah wiring that up is fairly annoying.

1 Like

This event is triggered from an after_save hook, so value_changed? will always return false. I think we need to use saved_change_to_value? instead. Here’s a PR for the change: FIX: Use `saved_change_to_value?` in site_setting_saved event by davidtaylorhq · Pull Request #7390 · discourse/discourse · GitHub

1 Like

Thanks. I was mostly moving that code. I saw that there were two ways of doing the same thing so I picked one (which I guess was the wrong one.)

If Rails 5.2 changed it, it means that we had code that was never executing even before my patch.

2 Likes