From e25638dab0d4b98f99c8fe8976ccaae8f4fb9db3 Mon Sep 17 00:00:00 2001
From: Neil Lalonde <neillalonde@gmail.com>
Date: Wed, 24 Jul 2013 13:48:55 -0400
Subject: [PATCH] add a way to delete posts and topics when deleting a user
with UserDestroyer
---
app/controllers/admin/users_controller.rb | 12 ++-
lib/guardian.rb | 4 +-
lib/user_destroyer.rb | 10 +-
spec/components/guardian_spec.rb | 6 +-
spec/components/user_destroyer_spec.rb | 97 +++++++++++--------
.../admin/users_controller_spec.rb | 7 ++
6 files changed, 87 insertions(+), 49 deletions(-)
diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index 1fad23fe2..4c5dd3ea5 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -117,10 +117,14 @@ class Admin::UsersController < Admin::AdminController
def destroy
user = User.where(id: params[:id]).first
guardian.ensure_can_delete_user!(user)
- if UserDestroyer.new(current_user).destroy(user)
- render json: {deleted: true}
- else
- render json: {deleted: false, user: AdminDetailedUserSerializer.new(user, root: false).as_json}
+ begin
+ if UserDestroyer.new(current_user).destroy(user, params.slice(:delete_posts))
+ render json: {deleted: true}
+ else
+ render json: {deleted: false, user: AdminDetailedUserSerializer.new(user, root: false).as_json}
+ end
+ rescue UserDestroyer::PostsExistError
+ raise Discourse::InvalidAccess.new("User #{user.username} has #{user.post_count} posts, so can't be deleted.")
end
end
diff --git a/lib/guardian.rb b/lib/guardian.rb
index 33a239cca..2030a572c 100644
--- a/lib/guardian.rb
+++ b/lib/guardian.rb
@@ -150,8 +150,8 @@ class Guardian
user && is_staff?
end
- def can_delete_user?(user_to_delete)
- can_administer?(user_to_delete) && user_to_delete.post_count <= 0
+ def can_delete_user?(user)
+ can_administer?(user)
end
# Can we see who acted on a post in a particular way?
diff --git a/lib/user_destroyer.rb b/lib/user_destroyer.rb
index e23f60054..be949bea1 100644
--- a/lib/user_destroyer.rb
+++ b/lib/user_destroyer.rb
@@ -11,10 +11,16 @@ class UserDestroyer
# Returns false if the user failed to be deleted.
# Returns a frozen instance of the User if the delete succeeded.
- def destroy(user)
+ def destroy(user, opts={})
raise Discourse::InvalidParameters.new('user is nil') unless user and user.is_a?(User)
- raise PostsExistError if user.post_count != 0
+ raise PostsExistError if !opts[:delete_posts] && user.post_count != 0
User.transaction do
+ if opts[:delete_posts]
+ user.posts.each do |post|
+ PostDestroyer.new(@admin, post).destroy
+ end
+ raise PostsExistError if user.reload.post_count != 0
+ end
user.destroy.tap do |u|
if u
Post.with_deleted.where(user_id: user.id).update_all("nuked_user = true")
diff --git a/spec/components/guardian_spec.rb b/spec/components/guardian_spec.rb
index 1758f0141..f69e9d0ae 100644
--- a/spec/components/guardian_spec.rb
+++ b/spec/components/guardian_spec.rb
@@ -1008,9 +1008,9 @@ describe Guardian do
end
context "for admins" do
- it "is false if user has posts" do
- Fabricate(:post, user: user)
- Guardian.new(admin).can_delete_user?(user).should be_false
+ it "is true if user has posts" do # UserDestroyer is responsible for checking for posts
+ user.stubs(:post_count).returns(1)
+ Guardian.new(admin).can_delete_user?(user).should be_true
end
it "is true if user has no posts" do
diff --git a/spec/components/user_destroyer_spec.rb b/spec/components/user_destroyer_spec.rb
index 8d17fc685..c23cb7c27 100644
--- a/spec/components/user_destroyer_spec.rb
+++ b/spec/components/user_destroyer_spec.rb
@@ -35,8 +35,6 @@ describe UserDestroyer do
@user = Fabricate(:user)
end
- subject(:destroy) { UserDestroyer.new(@admin).destroy(@user) }
-
it 'raises an error when user is nil' do
expect { UserDestroyer.new(@admin).destroy(nil) }.to raise_error(Discourse::InvalidParameters)
end
@@ -45,58 +43,79 @@ describe UserDestroyer do
expect { UserDestroyer.new(@admin).destroy('nothing') }.to raise_error(Discourse::InvalidParameters)
end
- context 'user has posts' do
- before do
- Fabricate(:post, user: @user)
+ shared_examples "successfully destroy a user" do
+ it 'should delete the user' do
+ expect { destroy }.to change { User.count }.by(-1)
end
- it 'should not delete the user' do
- expect { destroy rescue nil }.to_not change { User.count }
+ it 'should return the deleted user record' do
+ return_value = destroy
+ return_value.should == @user
+ return_value.should be_destroyed
end
- it 'should raise an error' do
- expect { destroy }.to raise_error( UserDestroyer::PostsExistError )
+ it 'should log the action' do
+ StaffActionLogger.any_instance.expects(:log_user_deletion).with(@user).once
+ destroy
end
- it 'should not log the action' do
- StaffActionLogger.any_instance.expects(:log_user_deletion).never
- destroy rescue nil
+ it 'should unregister the nickname as the discourse hub if hub integration is enabled' do
+ SiteSetting.stubs(:call_discourse_hub?).returns(true)
+ DiscourseHub.expects(:unregister_nickname).with(@user.username)
+ destroy
end
- it 'should not unregister the user at the discourse hub' do
+ it 'should not try to unregister the nickname as the discourse hub if hub integration is disabled' do
+ SiteSetting.stubs(:call_discourse_hub?).returns(false)
DiscourseHub.expects(:unregister_nickname).never
- destroy rescue nil
+ destroy
+ end
+ end
+
+ context 'user has posts' do
+ let!(:post) { Fabricate(:post, user: @user) }
+
+ context "delete_posts is false" do
+ subject(:destroy) { UserDestroyer.new(@admin).destroy(@user) }
+
+ it 'should not delete the user' do
+ expect { destroy rescue nil }.to_not change { User.count }
+ end
+
+ it 'should raise an error' do
+ expect { destroy }.to raise_error( UserDestroyer::PostsExistError )
+ end
+
+ it 'should not log the action' do
+ StaffActionLogger.any_instance.expects(:log_user_deletion).never
+ destroy rescue nil
+ end
+
+ it 'should not unregister the user at the discourse hub' do
+ DiscourseHub.expects(:unregister_nickname).never
+ destroy rescue nil
+ end
+ end
+
+ context "delete_posts is true" do
+ subject(:destroy) { UserDestroyer.new(@admin).destroy(@user, delete_posts: true) }
+
+ include_examples "successfully destroy a user"
+
+ it "deletes the posts" do
+ destroy
+ post.reload.deleted_at.should_not be_nil
+ post.nuked_user.should be_true
+ end
end
end
context 'user has no posts' do
context 'and destroy succeeds' do
- it 'should delete the user' do
- expect { destroy }.to change { User.count }.by(-1)
- end
- it 'should return the deleted user record' do
- return_value = destroy
- return_value.should == @user
- return_value.should be_destroyed
- end
+ subject(:destroy) { UserDestroyer.new(@admin).destroy(@user) }
- it 'should log the action' do
- StaffActionLogger.any_instance.expects(:log_user_deletion).with(@user).once
- destroy
- end
-
- it 'should unregister the nickname as the discourse hub if hub integration is enabled' do
- SiteSetting.stubs(:call_discourse_hub?).returns(true)
- DiscourseHub.expects(:unregister_nickname).with(@user.username)
- destroy
- end
-
- it 'should not try to unregister the nickname as the discourse hub if hub integration is disabled' do
- SiteSetting.stubs(:call_discourse_hub?).returns(false)
- DiscourseHub.expects(:unregister_nickname).never
- destroy
- end
+ include_examples "successfully destroy a user"
it "should mark the user's deleted posts as belonging to a nuked user" do
post = Fabricate(:post, user: @user, deleted_at: 1.hour.ago)
@@ -106,6 +125,8 @@ describe UserDestroyer do
end
context 'and destroy fails' do
+ subject(:destroy) { UserDestroyer.new(@admin).destroy(@user) }
+
before do
@user.stubs(:destroy).returns(false)
end
diff --git a/spec/controllers/admin/users_controller_spec.rb b/spec/controllers/admin/users_controller_spec.rb
index 454041934..20bf3d592 100644
--- a/spec/controllers/admin/users_controller_spec.rb
+++ b/spec/controllers/admin/users_controller_spec.rb
@@ -218,6 +218,13 @@ describe Admin::UsersController do
response.should be_forbidden
end
+ it "doesn't return an error if the user has posts and delete_posts == true" do
+ Fabricate(:post, user: @delete_me)
+ UserDestroyer.any_instance.expects(:destroy).with(@delete_me, has_entry('delete_posts' => true)).returns(true)
+ xhr :delete, :destroy, id: @delete_me.id, delete_posts: true
+ response.should be_success
+ end
+
it "deletes the user record" do
UserDestroyer.any_instance.expects(:destroy).returns(true)
xhr :delete, :destroy, id: @delete_me.id