FIX: display warning only if all users already added to the group (#10500)

FIX: display warning only if all users already added to the group (#10500)

If at least one user can be added to the group, proceed with that action

diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index dea5cbb..9e8320c 100644
--- a/app/controllers/groups_controller.rb
+++ b/app/controllers/groups_controller.rb
@@ -329,12 +329,12 @@ class GroupsController < ApplicationController
         'usernames or emails must be present'
       )
     end
-
-    if (usernames = group.users.where(id: users.map(&:id)).pluck(:username)).present?
+    usernames_already_in_group = group.users.where(id: users.map(&:id)).pluck(:username)
+    if usernames_already_in_group.present? && usernames_already_in_group.length == users.length
       render_json_error(I18n.t(
         "groups.errors.member_already_exist",
-        username: usernames.sort.join(", "),
-        count: usernames.size
+        username: usernames_already_in_group.sort.join(", "),
+        count: usernames_already_in_group.size
       ))
     else
       uniq_users = users.uniq
diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb
index d49387d..f7e3697 100644
--- a/spec/requests/groups_controller_spec.rb
+++ b/spec/requests/groups_controller_spec.rb
@@ -1213,7 +1213,7 @@ describe GroupsController do
           expect(response.status).to eq(200)
         end
 
-        it 'fails when multiple member already exists' do
+        it 'adds missing users even if some exists' do
           user2.update!(username: 'alice')
           user3 = Fabricate(:user, username: 'bob')
           [user2, user3].each { |user| group.add(user) }
@@ -1221,14 +1221,28 @@ describe GroupsController do
           expect do
             put "/groups/#{group.id}/members.json",
               params: { user_emails: [user1.email, user2.email, user3.email].join(",") }
+          end.to change { group.users.count }.by(1)
+
+          expect(response.status).to eq(200)
+        end
+
+        it 'displays warning when all members already exists' do
+          user1.update!(username: 'john')
+          user2.update!(username: 'alice')
+          user3 = Fabricate(:user, username: 'bob')
+          [user1, user2, user3].each { |user| group.add(user) }
+
+          expect do
+            put "/groups/#{group.id}/members.json",
+              params: { user_emails: [user1.email, user2.email, user3.email].join(",") }
           end.to change { group.users.count }.by(0)
 
           expect(response.status).to eq(422)
 
           expect(response.parsed_body["errors"]).to include(I18n.t(
             "groups.errors.member_already_exist",
-            username: "alice, bob",
-            count: 2
+            username: "alice, bob, john",
+            count: 3
           ))
         end
       end

GitHub sha: 7833853c

1 Like

This commit appears in #10500 which was approved by martin. It was merged by lis2.