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

@@ -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

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