FIX: Reviewables should not be created for users until they are active

FIX: Reviewables should not be created for users until they are active

Conversely, if a user is deactivated the reviewable should automatically be rejected.

Before this fix, if a user was not active they’d still show in the review queue but without an “Approve” button which was confusing.

diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index f231f61..c63a39c 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -310,7 +310,7 @@ class Admin::UsersController < Admin::AdminController
 
   def deactivate
     guardian.ensure_can_deactivate!(@user)
-    @user.deactivate
+    @user.deactivate(current_user)
     StaffActionLogger.new(current_user).log_user_deactivate(@user, I18n.t('user.deactivated_by_staff'), params.slice(:context))
     refresh_browser @user
     render body: nil
diff --git a/app/jobs/regular/create_user_reviewable.rb b/app/jobs/regular/create_user_reviewable.rb
new file mode 100644
index 0000000..1109919
--- /dev/null
+++ b/app/jobs/regular/create_user_reviewable.rb
@@ -0,0 +1,17 @@
+class Jobs::CreateUserReviewable < Jobs::Base
+  def execute(args)
+    raise Discourse::InvalidParameters unless args[:user_id].present?
+
+    if user = User.find_by(id: args[:user_id])
+      return if user.approved?
+
+      reviewable = ReviewableUser.needs_review!(target: user, created_by: Discourse.system_user, reviewable_by_moderator: true)
+      reviewable.add_score(
+        Discourse.system_user,
+        ReviewableScore.types[:needs_approval],
+        force_review: true
+      )
+    end
+
+  end
+end
diff --git a/app/jobs/scheduled/invalidate_inactive_admins.rb b/app/jobs/scheduled/invalidate_inactive_admins.rb
index 12f788a..78b73f7 100644
--- a/app/jobs/scheduled/invalidate_inactive_admins.rb
+++ b/app/jobs/scheduled/invalidate_inactive_admins.rb
@@ -13,7 +13,7 @@ module Jobs
         .each do |user|
 
         User.transaction do
-          user.deactivate
+          user.deactivate(Discourse.system_user)
           user.email_tokens.update_all(confirmed: false, expired: true)
 
           Discourse.authenticators.each do |authenticator|
diff --git a/app/models/email_token.rb b/app/models/email_token.rb
index 2a10dd4..cf9a188 100644
--- a/app/models/email_token.rb
+++ b/app/models/email_token.rb
@@ -64,9 +64,9 @@ class EmailToken < ActiveRecord::Base
       if result[:success]
         # If we are activating the user, send the welcome message
         user.send_welcome_message = !user.active?
-        user.active = true
         user.email = result[:email_token].email
         user.save!
+        user.activate
         user.set_automatic_groups
       end
 
diff --git a/app/models/user.rb b/app/models/user.rb
index 891665f..df6c4b3 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -116,7 +116,6 @@ class User < ActiveRecord::Base
   after_create :set_random_avatar
   after_create :ensure_in_trust_level_group
   after_create :set_default_categories_preferences
-  after_create :create_reviewable
 
   before_save :update_username_lower
   before_save :ensure_password_is_hashed
@@ -898,10 +897,15 @@ class User < ActiveRecord::Base
     else
       self.update!(active: true)
     end
+    create_reviewable
   end
 
-  def deactivate
+  def deactivate(performed_by)
     self.update!(active: false)
+
+    if reviewable = ReviewableUser.pending.find_by(target: self)
+      reviewable.perform(performed_by, :reject)
+    end
   end
 
   def change_trust_level!(level, opts = nil)
@@ -1242,14 +1246,7 @@ class User < ActiveRecord::Base
     return unless SiteSetting.must_approve_users? || SiteSetting.invite_only?
     return if approved?
 
-    reviewable = ReviewableUser.needs_review!(target: self, created_by: Discourse.system_user, reviewable_by_moderator: true)
-    reviewable.add_score(
-      Discourse.system_user,
-      ReviewableScore.types[:needs_approval],
-      force_review: true
-    )
-
-    reviewable
+    Jobs.enqueue(:create_user_reviewable, user_id: self.id)
   end
 
   def create_user_stat
diff --git a/db/migrate/20190103185626_create_reviewable_users.rb b/db/migrate/20190103185626_create_reviewable_users.rb
index 51cdde0..5b07802 100644
--- a/db/migrate/20190103185626_create_reviewable_users.rb
+++ b/db/migrate/20190103185626_create_reviewable_users.rb
@@ -22,7 +22,7 @@ class CreateReviewableUsers < ActiveRecord::Migration[5.2]
           created_at,
           created_at
         FROM users
-        WHERE approved = false
+        WHERE active AND approved = false
       SQL
     end
   end
diff --git a/spec/integration/invite_only_registration_spec.rb b/spec/integration/invite_only_registration_spec.rb
index 3151eeb..a0eed2f 100644
--- a/spec/integration/invite_only_registration_spec.rb
+++ b/spec/integration/invite_only_registration_spec.rb
@@ -8,6 +8,7 @@ describe 'invite only' do
     it 'can create user via API' do
 
       SiteSetting.invite_only = true
+      Jobs.run_immediately!
 
       admin = Fabricate(:admin)
       api_key = Fabricate(:api_key, user: admin)
diff --git a/spec/models/email_token_spec.rb b/spec/models/email_token_spec.rb
index f13f196..454845b 100644
--- a/spec/models/email_token_spec.rb
+++ b/spec/models/email_token_spec.rb
@@ -117,6 +117,7 @@ describe EmailToken do
     context 'confirms the token and redeems invite' do
       before do
         SiteSetting.must_approve_users = true
+        Jobs.run_immediately!
       end
 
       let(:invite) { Fabricate(:invite, email: 'test@example.com', user_id: nil) }
diff --git a/spec/models/reviewable_user_spec.rb b/spec/models/reviewable_user_spec.rb
index 0dddff8..0782137 100644
--- a/spec/models/reviewable_user_spec.rb
+++ b/spec/models/reviewable_user_spec.rb
@@ -3,7 +3,11 @@ require 'rails_helper'
 RSpec.describe ReviewableUser, type: :model do
 
   let(:moderator) { Fabricate(:moderator) }
-  let(:user) { Fabricate(:user) }
+  let(:user) do
+    user = Fabricate(:user)
+    user.activate
+    user
+  end
   let(:admin) { Fabricate(:admin) }
 
   context "actions_for" do
@@ -78,6 +82,7 @@ RSpec.describe ReviewableUser, type: :model do
   describe 'when must_approve_users is true' do
     before do
       SiteSetting.must_approve_users = true
+      Jobs.run_immediately!
     end
 
     it "creates the ReviewableUser for a user, with moderator access" do
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 2c04582..15b25b9 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -125,23 +125,58 @@ describe User do
   end
 
   describe 'reviewable' do
-    let(:user) { Fabricate(:user) }
+    let(:user) { Fabricate(:user, active: false) }
     let(:admin) { Fabricate(:admin) }
 
-    it "creates a reviewable for the user if must_approve_users is true" do
+    before do
+      Jobs.run_immediately!
+    end
+
+    it "creates a reviewable for the user if must_approve_users is true and activate is called" do
       SiteSetting.must_approve_users = true
       user
 
+      # Inactive users don't have reviewables
+      reviewable = ReviewableUser.find_by(target: user)
+      expect(reviewable).to be_blank
+
+      user.activate
       reviewable = ReviewableUser.find_by(target: user)
       expect(reviewable).to be_present
       expect(reviewable.score > 0).to eq(true)
       expect(reviewable.reviewable_scores).to be_present
     end
 
+    it "creates a reviewable for the user if must_approve_users is true and their token is confirmed" do
+      SiteSetting.must_approve_users = true
+      user
+
+      # Inactive users don't have reviewables
+      reviewable = ReviewableUser.find_by(target: user)
+      expect(reviewable).to be_blank
+
+      EmailToken.confirm(user.email_tokens.first.token)
+      expect(user.reload.active).to eq(true)
+      reviewable = ReviewableUser.find_by(target: user)
+      expect(reviewable).to be_present
+    end
+
     it "doesn't create a reviewable if must_approve_users is false" do
       user
       expect(ReviewableUser.find_by(target: user)).to be_blank
     end
+
+    it "will reject a reviewable if the user is deactivated" do
+      SiteSetting.must_approve_users = true
+      user
+
+      user.activate
+      reviewable = ReviewableUser.find_by(target: user)
+      expect(reviewable.pending?).to eq(true)
+

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

GitHub sha: c1ea63bd

1 Like

This commit has been mentioned on Discourse Meta. There might be relevant details there: