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.

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

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

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|

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
 

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

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

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)

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

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

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)
+
+      user.deactivate(admin)
+      expect(reviewable.reload.rejected?).to eq(true)
+    end
   end
 
   describe 'bookmark' do

spec/models/web_hook_spec.rb

@@ -290,6 +290,9 @@ describe WebHook do
       Fabricate(:user_web_hook, active: true)
 
       user
+      user.activate
+      Jobs::CreateUserReviewable.new.execute(user_id: user.id)
+
       job_args = Jobs::EmitWebHookEvent.jobs.last["args"].first
 
       expect(job_args["event_name"]).to eq("user_created")

spec/requests/admin/users_controller_spec.rb

@@ -71,6 +71,8 @@ RSpec.describe Admin::UsersController do
     end
 
     it 'calls approve' do
+      Jobs.run_immediately!
+      evil_trout.activate
       put "/admin/users/#{evil_trout.id}/approve.json"
       expect(response.status).to eq(200)
       evil_trout.reload
@@ -102,6 +104,8 @@ RSpec.describe Admin::UsersController do
     end
 
     it "approves the user when permitted" do
+      Jobs.run_immediately!
+      evil_trout.activate
       put "/admin/users/approve-bulk.json", params: { users: [evil_trout.id] }
       expect(response.status).to eq(200)
       evil_trout.reload

spec/requests/reviewables_controller_spec.rb

@@ -144,8 +144,10 @@ describe ReviewablesController do
       end
 
       it "will use the ReviewableUser serializer for its fields" do
+        Jobs.run_immediately!
         SiteSetting.must_approve_users = true
         user = Fabricate(:user)
+        user.activate
         reviewable = ReviewableUser.find_by(target: user)
 
         get "/review.json"