Merge branch...

Merge branch '352617-bulk-adding-members-needs-to-consider-user-records-as-well-as-the-ids' into 'master'

Bulk adding members needs to consider user records as well as the ids

See merge request gitlab-org/gitlab!82735

app/services/concerns/members/bulk_create_users.rb

@@ -47,16 +47,15 @@ module Members
             end
           end
 
-          if user_ids.present?
-            # we should handle the idea of existing members where users are passed as users - https://gitlab.com/gitlab-org/gitlab/-/issues/352617
-            # the below will automatically discard invalid user_ids
-            users.concat(User.id_in(user_ids))
+          # the below will automatically discard invalid user_ids
+          users.concat(User.id_in(user_ids)) if user_ids.present?
+          users.uniq! # de-duplicate just in case as there is no controlling if user records and ids are sent multiple times
+
+          if users.present?
             # helps not have to perform another query per user id to see if the member exists later on when fetching
-            existing_members = source.members_and_requesters.where(user_id: user_ids).index_by(&:user_id) # rubocop:disable CodeReuse/ActiveRecord
+            existing_members = source.members_and_requesters.where(user_id: users).index_by(&:user_id) # rubocop:disable CodeReuse/ActiveRecord
           end
 
-          users.uniq! # de-duplicate just in case as there is no controlling if user records and ids are sent multiple times
-
           [emails, users, existing_members]
         end
       end

spec/support/shared_examples/models/member_shared_examples.rb

@@ -371,8 +371,7 @@ RSpec.shared_examples_for "bulk member creation" do
     it 'returns a Member objects' do
       members = described_class.add_users(source, [user1, user2], :maintainer)
 
-      expect(members).to be_a Array
-      expect(members.size).to eq(2)
+      expect(members.map(&:user)).to contain_exactly(user1, user2)
       expect(members).to all(be_a(member_type))
       expect(members).to all(be_persisted)
     end
@@ -394,20 +393,18 @@ RSpec.shared_examples_for "bulk member creation" do
     end
 
     context 'with de-duplication' do
-      it 'with the same user by id and user' do
+      it 'has the same user by id and user' do
         members = described_class.add_users(source, [user1.id, user1, user1.id, user2, user2.id, user2], :maintainer)
 
-        expect(members).to be_a Array
-        expect(members.size).to eq(2)
+        expect(members.map(&:user)).to contain_exactly(user1, user2)
         expect(members).to all(be_a(member_type))
         expect(members).to all(be_persisted)
       end
 
-      it 'with the same user sent more than once' do
+      it 'has the same user sent more than once' do
         members = described_class.add_users(source, [user1, user1], :maintainer)
 
-        expect(members).to be_a Array
-        expect(members.size).to eq(1)
+        expect(members.map(&:user)).to contain_exactly(user1)
         expect(members).to all(be_a(member_type))
         expect(members).to all(be_persisted)
       end
@@ -418,15 +415,35 @@ RSpec.shared_examples_for "bulk member creation" do
         source.add_user(user1, :developer)
       end
 
-      it 'supports existing users as expected' do
+      it 'has the same user sent more than once with the member already existing' do
+        expect do
+          members = described_class.add_users(source, [user1, user1, user2], :maintainer)
+          expect(members.map(&:user)).to contain_exactly(user1, user2)
+          expect(members).to all(be_a(member_type))
+          expect(members).to all(be_persisted)
+        end.to change { Member.count }.by(1)
+      end
+
+      it 'supports existing users as expected with user_ids passed' do
         user3 = create(:user)
 
-        members = described_class.add_users(source, [user1.id, user2, user3.id], :maintainer)
+        expect do
+          members = described_class.add_users(source, [user1.id, user2, user3.id], :maintainer)
+          expect(members.map(&:user)).to contain_exactly(user1, user2, user3)
+          expect(members).to all(be_a(member_type))
+          expect(members).to all(be_persisted)
+        end.to change { Member.count }.by(2)
+      end
+
+      it 'supports existing users as expected without user ids passed' do
+        user3 = create(:user)
 
-        expect(members).to be_a Array
-        expect(members.size).to eq(3)
-        expect(members).to all(be_a(member_type))
-        expect(members).to all(be_persisted)
+        expect do
+          members = described_class.add_users(source, [user1, user2, user3], :maintainer)
+          expect(members.map(&:user)).to contain_exactly(user1, user2, user3)
+          expect(members).to all(be_a(member_type))
+          expect(members).to all(be_persisted)
+        end.to change { Member.count }.by(2)
       end
     end