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.

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
 

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,

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

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/)

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,

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

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?

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

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

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)

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
     @settings[current_site] ||= {}
@@ -26,8 +41,14 @@ class SiteSettings::LocalProcessProvider
   def save(name, value, data_type)
     # NOTE: convert to string to simulate the conversion that is happening
     # when using DbProvider
-    value = value.to_s
-    settings[name] = Setting.new(name, value, data_type)
+    setting = settings[name]
+    if setting.blank?
+      setting = Setting.new(name, data_type)
+      settings[name] = setting
+    end
+    setting.value = value.to_s
+    DiscourseEvent.trigger(:site_setting_saved, setting)
+    setting
   end
 
   def destroy(name)

lib/svg_sprite/svg_sprite.rb

@@ -329,7 +329,6 @@ License - https://fontawesome.com/license/free (Icons: CC BY 4.0, Fonts: SIL OFL
   end
 
   DiscourseEvent.on(:site_setting_saved) do |site_setting|
-    expire_cache if site_setting.name.to_s.include?("_icon")
   end
 
   def self.plugin_icons

spec/components/site_settings/db_provider_spec.rb

@@ -14,7 +14,7 @@ describe SiteSettings::DbProvider do
   end
 
   # integration test, requires db access
-  it "act correctly" do
+  it "acts correctly" do
     setting = Struct.new(:name, :value, :data_type)
 
     SiteSetting.destroy_all

spec/components/site_settings/local_process_provider_spec.rb

@@ -9,16 +9,10 @@ describe SiteSettings::LocalProcessProvider do
     expect(actual.data_type).to eq(expected.data_type)
   end
 
-  let :provider do
-    SiteSettings::LocalProcessProvider.new
-  end
+  let(:provider) { described_class.new }
 
   def setting(name, value, data_type)
-    OpenStruct.new.tap do |setting|
-      setting.name = name
-      setting.value = value
-      setting.data_type = data_type
-    end
+    described_class::Setting.new(name, data_type).tap { |s| s.value = value }
   end
 
   describe "all" do

spec/jobs/enqueue_digest_emails_spec.rb

@@ -15,12 +15,13 @@ describe Jobs::EnqueueDigestEmails do
     end
 
     context 'unapproved users' do
-      let!(:unapproved_user) { Fabricate(:active_user, approved: false, last_emailed_at: 8.days.ago, last_seen_at: 10.days.ago) }
 
       before do
         SiteSetting.must_approve_users = true
       end
 
+      let!(:unapproved_user) { Fabricate(:active_user, approved: false, last_emailed_at: 8.days.ago, last_seen_at: 10.days.ago) }
+
       it 'should enqueue the right digest emails' do
         expect(Jobs::EnqueueDigestEmails.new.target_user_ids.include?(unapproved_user.id)).to eq(false)
 

spec/jobs/notify_mailing_list_subscribers_spec.rb

@@ -42,6 +42,8 @@ describe Jobs::NotifyMailingListSubscribers do
       before do
         SiteSetting.login_required = true
         SiteSetting.must_approve_users = true
+
+        User.update_all(approved: false)
       end
       include_examples "no emails"
     end

spec/models/category_spec.rb

@@ -434,7 +434,7 @@ describe Category do
     end
 
     it 'triggers a extensibility event' do
-      event = DiscourseEvent.track_events { @category.destroy }.first
+      event = DiscourseEvent.track(:category_destroyed) { @category.destroy }
 
       expect(event[:event_name]).to eq(:category_destroyed)
       expect(event[:params].first).to eq(@category)

spec/models/reviewable_user_spec.rb

@@ -100,6 +100,15 @@ RSpec.describe ReviewableUser, type: :model do
     end
   end
 
+  describe "changing must_approve_users" do
+    it "will approve any existing users" do
+      user = Fabricate(:user)
+      expect(user).not_to be_approved
+      SiteSetting.must_approve_users = true
+      expect(user.reload).to be_approved
+    end
+  end
+
   describe 'when must_approve_users is true' do
     before do
       SiteSetting.must_approve_users = true

spec/requests/admin/users_controller_spec.rb

@@ -56,12 +56,11 @@ RSpec.describe Admin::UsersController do
   end
 
   describe '#approve' do
+    let(:evil_trout) { Fabricate(:evil_trout) }
     before do
       SiteSetting.must_approve_users = true
     end
 
-    let(:evil_trout) { Fabricate(:evil_trout) }
-
     it "raises an error when the user doesn't have permission" do
       sign_in(user)
       put "/admin/users/#{evil_trout.id}/approve.json"
@@ -70,6 +69,15 @@ RSpec.describe Admin::UsersController do
       expect(evil_trout.approved).to eq(false)
     end
 
+    it "will create a reviewable if one does not exist" do
+      evil_trout.update!(active: true)
+      expect(ReviewableUser.find_by(target: evil_trout)).to be_blank
+      put "/admin/users/#{evil_trout.id}/approve.json"
+      expect(response.code).to eq("200")
+      expect(ReviewableUser.find_by(target: evil_trout)).to be_present
+      expect(evil_trout.reload).to be_approved
+    end
+
     it 'calls approve' do
       Jobs.run_immediately!
       evil_trout.activate

spec/requests/session_controller_spec.rb

@@ -1111,6 +1111,7 @@ RSpec.describe SessionController do
 
         context 'with an unapproved user' do
           before do
+            user.update_columns(approved: false)
             post "/session.json", params: {
               login: user.email, password: 'myawesomepassword'
             }