diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index f231f612302..c63a39ca67d 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 00000000000..11099191d05 --- /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 12f788af136..78b73f7b7ac 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 2a10dd4e234..cf9a1881bb5 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 891665fa874..df6c4b30103 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 51cdde03394..5b078027073 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 3151eebc1d3..a0eed2f65f1 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 f13f19616d6..454845b5fc5 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 0dddff8d39b..07821378183 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 2c045829d16..15b25b93b8b 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) + + user.deactivate(admin) + expect(reviewable.reload.rejected?).to eq(true) + end end describe 'bookmark' do diff --git a/spec/models/web_hook_spec.rb b/spec/models/web_hook_spec.rb index 1c9389a4f36..915374f2f59 100644 --- a/spec/models/web_hook_spec.rb +++ b/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") diff --git a/spec/requests/admin/users_controller_spec.rb b/spec/requests/admin/users_controller_spec.rb index f4a8fca488c..d376bf789e8 100644 --- a/spec/requests/admin/users_controller_spec.rb +++ b/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 diff --git a/spec/requests/reviewables_controller_spec.rb b/spec/requests/reviewables_controller_spec.rb index ebec09fdc10..9362b37549c 100644 --- a/spec/requests/reviewables_controller_spec.rb +++ b/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"