FIX: Throw error when removing a user from group fails (#9162)

This commit ensures that an error is thrown when a user fails to be
removed from a group instead of silently failing.

This means when using the api you will receive a 400 instead of a 200 if
there is a failure. The remove group endpoint allows the removal of
multiple users, this change means that if you try to delete 10 users,
but 1 of them fails you will receive a 400 instead of 200 even though
the other 9 were removed successfully. Rather than adding a bunch more
complexity I think this is more than adequate for most use cases.
This commit is contained in:
Blake Erickson 2020-03-10 15:25:00 -06:00 committed by GitHub
parent 29b35aa64c
commit 6fb4c333b0
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 19 additions and 11 deletions

View File

@ -398,14 +398,16 @@ class GroupsController < ApplicationController
end
users.each do |user|
group.remove(user)
GroupActionLogger.new(current_user, group).log_remove_user_from_group(user)
if group.remove(user)
GroupActionLogger.new(current_user, group).log_remove_user_from_group(user)
else
raise Discourse::InvalidParameters
end
end
render json: success_json.merge!(
usernames: users.map(&:username)
)
end
def request_membership

View File

@ -644,9 +644,11 @@ class Group < ActiveRecord::Base
end
def remove(user)
self.group_users.where(user: user).each(&:destroy)
result = self.group_users.where(user: user).each(&:destroy)
return false if result.blank?
user.update_columns(primary_group_id: nil) if user.primary_group_id == self.id
DiscourseEvent.trigger(:user_removed_from_group, user, self)
true
end
def add_owner(user)

View File

@ -347,8 +347,9 @@ RSpec.describe Admin::UsersController do
describe '#remove_group' do
it "also clears the user's primary group" do
g = Fabricate(:group)
u = Fabricate(:user, primary_group: g)
u = Fabricate(:user)
g = Fabricate(:group, users: [u])
u.update!(primary_group_id: g.id)
delete "/admin/users/#{u.id}/groups/#{g.id}.json"
expect(response.status).to eq(200)

View File

@ -1192,6 +1192,11 @@ describe GroupsController do
expect(response.status).to eq(400)
end
it "raises an error when removing a valid user but is not a member of that group" do
delete "/groups/#{group.id}/members.json", params: { user_id: -1 }
expect(response.status).to eq(400)
end
context "is able to remove a member" do
it "removes by id" do
expect do
@ -1314,12 +1319,10 @@ describe GroupsController do
end
it "only removes users in that group" do
expect do
delete "/groups/#{group1.id}/members.json",
params: { usernames: [user.username, user2.username].join(",") }
end.to change { group1.users.count }.by(-1)
delete "/groups/#{group1.id}/members.json",
params: { usernames: [user.username, user2.username].join(",") }
expect(response.status).to eq(200)
expect(response.status).to eq(400)
end
end
end