From 13b3cead06d5d658de69f26582688f470e0365cc Mon Sep 17 00:00:00 2001 From: Blake Erickson Date: Thu, 11 Oct 2018 15:27:41 -0600 Subject: [PATCH] FEATURE: Allow bulk removing users from a group This change maintains backwards compatibility to allow you to remove a single user from a group but allows you to specify a comma separated list of users for bulk removal from a group. Also it extracts out common functionality for fetching users from params used in bulk adding users so it can also be used for removing users. --- app/controllers/groups_controller.rb | 66 ++++++++++++------------- spec/requests/groups_controller_spec.rb | 45 ++++++++++++++++- 2 files changed, 77 insertions(+), 34 deletions(-) diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index e553cfb0ec5..5dad97d0f4d 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -258,20 +258,7 @@ class GroupsController < ApplicationController group = Group.find(params[:id]) group.public_admission ? ensure_logged_in : guardian.ensure_can_edit!(group) - if params[:usernames].present? - users = User.where(username: params[:usernames].split(",")) - raise Discourse::InvalidParameters.new(:usernames) if users.blank? - elsif params[:user_ids].present? - users = User.where(id: params[:user_ids].split(",")) - raise Discourse::InvalidParameters.new(:user_ids) if users.blank? - elsif params[:user_emails].present? - users = User.with_email(params[:user_emails].split(",")) - raise Discourse::InvalidParameters.new(:user_emails) if users.blank? - else - raise Discourse::InvalidParameters.new( - 'user_ids or usernames or user_emails must be present' - ) - end + users = users_from_params if group.public_admission if !guardian.can_log_group_changes?(group) && current_user != users.first @@ -332,21 +319,15 @@ class GroupsController < ApplicationController raise Discourse::NotFound unless group group.public_exit ? ensure_logged_in : guardian.ensure_can_edit!(group) - user = - if params[:user_id].present? - User.find_by(id: params[:user_id]) - elsif params[:username].present? - User.find_by_username(params[:username]) - elsif params[:user_email].present? - User.find_by_email(params[:user_email]) - else - raise Discourse::InvalidParameters.new('user_id or username must be present') - end + # Maintain backwards compatibility + params[:usernames] = params[:username] if params[:username].present? + params[:user_ids] = params[:user_id] if params[:user_id].present? + params[:user_emails] = params[:user_email] if params[:user_email].present? - raise Discourse::NotFound unless user + users = users_from_params if group.public_exit - if !guardian.can_log_group_changes?(group) && current_user != user + if !guardian.can_log_group_changes?(group) && current_user != users.first raise Discourse::InvalidAccess end @@ -355,14 +336,15 @@ class GroupsController < ApplicationController end end - group.remove(user) - GroupActionLogger.new(current_user, group).log_remove_user_from_group(user) - - if group.save && user.save - render json: success_json - else - render_json_error(group) + users.each do |user| + group.remove(user) + GroupActionLogger.new(current_user, group).log_remove_user_from_group(user) end + + render json: success_json.merge!( + usernames: users.map(&:username) + ) + end def request_membership @@ -499,4 +481,22 @@ class GroupsController < ApplicationController guardian.ensure_can_see!(group) if ensure_can_see group end + + def users_from_params + if params[:usernames].present? + users = User.where(username: params[:usernames].split(",")) + raise Discourse::InvalidParameters.new(:usernames) if users.blank? + elsif params[:user_ids].present? + users = User.where(id: params[:user_ids].split(",")) + raise Discourse::InvalidParameters.new(:user_ids) if users.blank? + elsif params[:user_emails].present? + users = User.with_email(params[:user_emails].split(",")) + raise Discourse::InvalidParameters.new(:user_emails) if users.blank? + else + raise Discourse::InvalidParameters.new( + 'user_ids or usernames or user_emails must be present' + ) + end + users + end end diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb index 33fa3077e01..05e18d426b3 100644 --- a/spec/requests/groups_controller_spec.rb +++ b/spec/requests/groups_controller_spec.rb @@ -992,7 +992,7 @@ describe GroupsController do it "raises an error if user to be removed is not found" do delete "/groups/#{group.id}/members.json", params: { user_id: -10 } - expect(response.status).to eq(404) + expect(response.status).to eq(400) end context "is able to remove a member" do @@ -1065,6 +1065,49 @@ describe GroupsController do end end end + + context '#remove_members' do + context "is able to remove several members from a group" do + let(:user1) { Fabricate(:user) } + let(:user2) { Fabricate(:user) } + let(:group1) { Fabricate(:group, users: [user1, user2]) } + + it "removes by username" do + expect do + delete "/groups/#{group1.id}/members.json", + params: { usernames: [user1.username, user2.username].join(",") } + end.to change { group1.users.count }.by(-2) + expect(response.status).to eq(200) + end + + it "removes by id" do + expect do + delete "/groups/#{group1.id}/members.json", + params: { user_ids: [user1.id, user2.id].join(",") } + end.to change { group1.users.count }.by(-2) + + expect(response.status).to eq(200) + end + + it "removes by email" do + expect do + delete "/groups/#{group1.id}/members.json", + params: { user_emails: [user1.email, user2.email].join(",") } + end.to change { group1.users.count }.by(-2) + + expect(response.status).to eq(200) + 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) + + expect(response.status).to eq(200) + end + end + end end end