From 7833853cf1c9550c39edd42887bf237db324b13b Mon Sep 17 00:00:00 2001
From: Krzysztof Kotlarek <kotlarek.krzysztof@gmail.com>
Date: Fri, 21 Aug 2020 13:38:09 +1000
Subject: [PATCH] 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
---
 app/controllers/groups_controller.rb    |  8 ++++----
 spec/requests/groups_controller_spec.rb | 20 +++++++++++++++++---
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb
index dea5cbb542b..9e8320cdbf5 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 d49387de3a6..f7e3697c648 100644
--- a/spec/requests/groups_controller_spec.rb
+++ b/spec/requests/groups_controller_spec.rb
@@ -1213,11 +1213,25 @@ 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) }
 
+          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(",") }
@@ -1227,8 +1241,8 @@ describe GroupsController do
 
           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