Skip to content

Commit

Permalink
FIX: Show a json api response when deleting a user with posts
Browse files Browse the repository at this point in the history 
A 500 error was actually caused with no response when using the api, so
it wasn't very clear that you need to delete the posts first when using
the api.
  • Loading branch information
@oblakeerickson
oblakeerickson committed May 10, 2018
1 parent 186623a commit bd352a1
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 3 deletions.
5 changes: 4 additions & 1 deletion app/controllers/admin/users_controller.rb
View file
Expand Up @@ -379,7 +379,10 @@ def destroy
}
end
rescue UserDestroyer::PostsExistError
raise Discourse::InvalidAccess.new("User #{user.username} has #{user.post_count} posts, so can't be deleted.")
render json: {

This comment has been minimized.

Sign in to view

Copy link
@tgxworld

tgxworld May 11, 2018

Contributor

@oblakeerickson The change here doesn't look right to me. If a Discourse::InvalidAccess error is raised, it should be rescued in application controller. We're changing the status code from 403 to 200 which I don't think is correct.

This comment has been minimized.

Sign in to view

Copy link
@oblakeerickson

oblakeerickson May 11, 2018

Author Member

Yes, but a 403 should be raised if I don't have permission to do it, like if I'm just a regular user trying to delete another user, but it also isn't returning a 403. It is actually returning a 500 error:

#<Net::HTTPInternalServerError 500 Internal Server Error readbody=true>

I think it is okay to return 200 because the http request was a success and the server handled it correctly, but you either need to pass the delete_posts flag or delete all the posts first if you want the user to actually be deleted.

I would be okay if we fixed it to actually return a 403 if you think that is best.

This comment has been minimized.

Sign in to view

Copy link
@oblakeerickson

oblakeerickson May 11, 2018

Author Member

maybe its returning a 500 because we are trying to raise inside of the hijack request?

This comment has been minimized.

Sign in to view

Copy link
@tgxworld

tgxworld May 11, 2018 •
edited

Contributor

maybe its returning a 500 because we are trying to raise inside of the hijack request?

Ah ha that is it. It looks like hijack would just re-raise the error https://github.com/discourse/discourse/blob/master/lib/hijack.rb#L58-L59

I personally think it has to be a different status code. When interacting with an API, a 200 response code indicates that the server did what I wanted it to do. It would be strange if we require people to check for the response.body to see if the user was actually deleted even though they received a 200 response.

This comment has been minimized.

Sign in to view

Copy link
@oblakeerickson

oblakeerickson May 11, 2018 •
edited

Author Member

Okay now that we understand where the 500 was really coming from I'll see if I can fix it actually return a 403.
deleted: false,
message: "User #{user.username} has #{user.post_count} posts, so they can't be deleted."
}
end
end
end
Expand Down
6 changes: 4 additions & 2 deletions spec/controllers/admin/users_controller_spec.rb
View file
Expand Up @@ -529,9 +529,11 @@
_post = create_post(topic: topic, user: delete_me)
end

it "returns an error" do
it "returns an api response that the user can't be deleted because it has posts" do
delete :destroy, params: { id: delete_me.id }, format: :json
expect(response).to be_forbidden
expect(response).to be_success
json = ::JSON.parse(response.body)
expect(json['deleted']).to eq(false)
end

it "doesn't return an error if delete_posts == true" do
Expand Down

0 comments on commit bd352a1

Please sign in to comment.