From 537f87562e71acb464e02fc81f2596b01816072c Mon Sep 17 00:00:00 2001
From: Roman Rizzi <rizziromanalejandro@gmail.com>
Date: Mon, 2 Mar 2020 16:33:52 -0300
Subject: [PATCH] FIX: We need to skip users with associated reviewables when
 auto-approving (#9080)

* FIX: We need to skip users with associated reviewables when auto-approving them

* Update spec/initializers/track_setting_changes_spec.rb

* Update spec/initializers/track_setting_changes_spec.rb

Co-authored-by: Robin Ward <robin.ward@gmail.com>
---
 .../initializers/014-track-setting-changes.rb |  5 +++-
 .../track_setting_changes_spec.rb             | 25 +++++++++++++++++++
 2 files changed, 29 insertions(+), 1 deletion(-)
 create mode 100644 spec/initializers/track_setting_changes_spec.rb

diff --git a/config/initializers/014-track-setting-changes.rb b/config/initializers/014-track-setting-changes.rb
index f8276d9dcc6..200f2c2da62 100644
--- a/config/initializers/014-track-setting-changes.rb
+++ b/config/initializers/014-track-setting-changes.rb
@@ -4,7 +4,10 @@ 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)
+
+    User.where(approved: false)
+      .joins("LEFT JOIN reviewables r ON r.target_id = users.id")
+      .where(r: { id: nil }).update_all(approved: true)
   end
 
   if name == :emoji_set
diff --git a/spec/initializers/track_setting_changes_spec.rb b/spec/initializers/track_setting_changes_spec.rb
new file mode 100644
index 00000000000..82b7963ec4d
--- /dev/null
+++ b/spec/initializers/track_setting_changes_spec.rb
@@ -0,0 +1,25 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe 'Setting changes' do
+  describe '#must_approve_users' do
+    before { SiteSetting.must_approve_users = false }
+
+    it 'does not approve a user with associated reviewables' do
+      user_pending_approval = Fabricate(:reviewable_user).target
+
+      SiteSetting.must_approve_users = true
+
+      expect(user_pending_approval.reload.approved?).to eq(false)
+    end
+
+    it 'approves a user with no associated reviewables' do
+      non_approved_user = Fabricate(:user, approved: false)
+
+      SiteSetting.must_approve_users = true
+
+      expect(non_approved_user.reload.approved?).to eq(true)
+    end
+  end
+end