From 395d17e2ac715b98ffb47407340dd69bda418ead Mon Sep 17 00:00:00 2001
From: Blake Erickson <o.blakeerickson@gmail.com>
Date: Wed, 22 Jul 2020 14:27:43 -0600
Subject: [PATCH] DEV: Show failed to remove members from bulk groups api

Before this commit if you were bulk removing group members and passed in
a user who wasn't currently a member of that group the whole request
would fail. This change will return a 200 response now listing the users
that were removed and those that were skipped.
---
 app/controllers/groups_controller.rb    | 13 +++++++++++--
 spec/requests/groups_controller_spec.rb | 15 ++++++++++++---
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index 08205d06635..b15dc18a061 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -412,16 +412,25 @@ class GroupsController < ApplicationController
       end
     end
 
+    removed_users = []
+    skipped_users = []
+
     users.each do |user|
       if group.remove(user)
+        removed_users << user.username
         GroupActionLogger.new(current_user, group).log_remove_user_from_group(user)
       else
-        raise Discourse::InvalidParameters
+        if group.users.exclude? user
+          skipped_users << user.username
+        else
+          raise Discourse::InvalidParameters
+        end
       end
     end
 
     render json: success_json.merge!(
-      usernames: users.map(&:username)
+      usernames: removed_users,
+      skipped_usernames: skipped_users
     )
   end
 
diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb
index eb6b4d56d56..fdf77e65266 100644
--- a/spec/requests/groups_controller_spec.rb
+++ b/spec/requests/groups_controller_spec.rb
@@ -1191,12 +1191,18 @@ 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 }
+
+        response_body = response.parsed_body
         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
+      it "returns skipped_usernames response body 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)
+
+        response_body = response.parsed_body
+        expect(response.status).to eq(200)
+        expect(response_body["usernames"]).to eq([])
+        expect(response_body["skipped_usernames"].first).to eq("system")
       end
 
       context "is able to remove a member" do
@@ -1324,7 +1330,10 @@ describe GroupsController do
             delete "/groups/#{group1.id}/members.json",
               params: { usernames: [user.username, user2.username].join(",") }
 
-            expect(response.status).to eq(400)
+            response_body = response.parsed_body
+            expect(response.status).to eq(200)
+            expect(response_body["usernames"].first).to eq(user2.username)
+            expect(response_body["skipped_usernames"].first).to eq(user.username)
           end
         end
       end