diff --git a/app/models/reviewable_user.rb b/app/models/reviewable_user.rb index f1ebf49c520..40185a35dcc 100644 --- a/app/models/reviewable_user.rb +++ b/app/models/reviewable_user.rb @@ -12,7 +12,7 @@ class ReviewableUser < Reviewable return unless pending? actions.add(:approve) if guardian.can_approve?(target) || args[:approved_by_invite] - actions.add(:reject) if guardian.can_delete_user?(target) + actions.add(:reject) end def perform_approve(performed_by, args) @@ -34,13 +34,20 @@ class ReviewableUser < Reviewable end def perform_reject(performed_by, args) - destroyer = UserDestroyer.new(performed_by) unless args[:skip_delete] - # If a user has posts, we won't delete them to preserve their content. - # However the reviable record will be "rejected" and they will remain - # unapproved in the database. A staff member can still approve them - # via the admin. - destroyer.destroy(target) rescue UserDestroyer::PostsExistError + # We'll delete the user if we can + if target.present? + destroyer = UserDestroyer.new(performed_by) + + begin + destroyer.destroy(target) + rescue UserDestroyer::PostsExistError + # If a user has posts, we won't delete them to preserve their content. + # However the reviable record will be "rejected" and they will remain + # unapproved in the database. A staff member can still approve them + # via the admin. + end + end create_result(:success, :rejected) end diff --git a/app/services/user_destroyer.rb b/app/services/user_destroyer.rb index f9a9f594504..f6c40ac47c1 100644 --- a/app/services/user_destroyer.rb +++ b/app/services/user_destroyer.rb @@ -23,15 +23,13 @@ class UserDestroyer prepare_for_destroy(user) if opts[:prepare_for_destroy] == true + result = nil + optional_transaction(open_transaction: opts[:transaction]) do Draft.where(user_id: user.id).delete_all Reviewable.where(created_by_id: user.id).delete_all - if reviewable = Reviewable.find_by(target: user) - reviewable.perform(@actor, :reject, skip_delete: true) rescue Reviewable::InvalidAction - end - if opts[:delete_posts] user.posts.each do |post| @@ -75,47 +73,52 @@ class UserDestroyer # keep track of emails used user_emails = user.user_emails.pluck(:email) - user.destroy.tap do |u| - if u - if opts[:block_email] - user_emails.each do |email| - ScreenedEmail.block(email, ip_address: u.ip_address)&.record_match! - end + if result = user.destroy + if opts[:block_email] + user_emails.each do |email| + ScreenedEmail.block(email, ip_address: result.ip_address)&.record_match! end - - if opts[:block_ip] && u.ip_address - ScreenedIpAddress.watch(u.ip_address)&.record_match! - if u.registration_ip_address && u.ip_address != u.registration_ip_address - ScreenedIpAddress.watch(u.registration_ip_address)&.record_match! - end - end - - Post.unscoped.where(user_id: u.id).update_all(user_id: nil) - - # If this user created categories, fix those up: - Category.where(user_id: u.id).each do |c| - c.user_id = Discourse::SYSTEM_USER_ID - c.save! - if topic = Topic.unscoped.find_by(id: c.topic_id) - topic.recover! - topic.user_id = Discourse::SYSTEM_USER_ID - topic.save! - end - end - - unless opts[:quiet] - if @actor == user - deleted_by = Discourse.system_user - opts[:context] = I18n.t("staff_action_logs.user_delete_self", url: opts[:context]) - else - deleted_by = @actor - end - StaffActionLogger.new(deleted_by).log_user_deletion(user, opts.slice(:context)) - end - MessageBus.publish "/file-change", ["refresh"], user_ids: [u.id] end + + if opts[:block_ip] && result.ip_address + ScreenedIpAddress.watch(result.ip_address)&.record_match! + if result.registration_ip_address && result.ip_address != result.registration_ip_address + ScreenedIpAddress.watch(result.registration_ip_address)&.record_match! + end + end + + Post.unscoped.where(user_id: result.id).update_all(user_id: nil) + + # If this user created categories, fix those up: + Category.where(user_id: result.id).each do |c| + c.user_id = Discourse::SYSTEM_USER_ID + c.save! + if topic = Topic.unscoped.find_by(id: c.topic_id) + topic.recover! + topic.user_id = Discourse::SYSTEM_USER_ID + topic.save! + end + end + + unless opts[:quiet] + if @actor == user + deleted_by = Discourse.system_user + opts[:context] = I18n.t("staff_action_logs.user_delete_self", url: opts[:context]) + else + deleted_by = @actor + end + StaffActionLogger.new(deleted_by).log_user_deletion(user, opts.slice(:context)) + end + MessageBus.publish "/file-change", ["refresh"], user_ids: [result.id] end end + + # After the user is deleted, remove the reviewable + if reviewable = Reviewable.pending.find_by(target: user) + reviewable.perform(@actor, :reject) + end + + result end protected diff --git a/spec/models/reviewable_user_spec.rb b/spec/models/reviewable_user_spec.rb index 95e6c73e995..fed75f86d71 100644 --- a/spec/models/reviewable_user_spec.rb +++ b/spec/models/reviewable_user_spec.rb @@ -87,6 +87,15 @@ RSpec.describe ReviewableUser, type: :model do expect(reviewable.target).to be_present expect(reviewable.target.approved).to eq(false) end + + it "allows us to reject a user who has been deleted" do + reviewable.target.destroy! + reviewable.reload + result = reviewable.perform(moderator, :reject) + expect(result.success?).to eq(true) + expect(reviewable.rejected?).to eq(true) + expect(reviewable.target).to be_blank + end end end