Merge branch '334062-further-refactor-member-creation-classes' into 'master'

Further refactor member creation classes

See merge request gitlab-org/gitlab!67541

app/models/group.rb

@@ -297,7 +297,7 @@ class Group < Namespace
   end
 
   def add_users(users, access_level, current_user: nil, expires_at: nil)
-    Members::Groups::CreatorService.add_users( # rubocop:disable CodeReuse/ServiceClass
+    Members::Groups::BulkCreatorService.add_users( # rubocop:disable CodeReuse/ServiceClass
       self,
       users,
       access_level,

app/models/members/project_member.rb

@@ -44,7 +44,7 @@ class ProjectMember < Member
         project_ids.each do |project_id|
           project = Project.find(project_id)
 
-          Members::Projects::CreatorService.add_users( # rubocop:disable CodeReuse/ServiceClass
+          Members::Projects::BulkCreatorService.add_users( # rubocop:disable CodeReuse/ServiceClass
             project,
             users,
             access_level,

app/models/project_team.rb

@@ -42,7 +42,7 @@ class ProjectTeam
   end
 
   def add_users(users, access_level, current_user: nil, expires_at: nil)
-    Members::Projects::CreatorService.add_users( # rubocop:disable CodeReuse/ServiceClass
+    Members::Projects::BulkCreatorService.add_users( # rubocop:disable CodeReuse/ServiceClass
       project,
       users,
       access_level,

app/services/concerns/members/bulk_create_users.rb

+# frozen_string_literal: true
+
+module Members
+  module BulkCreateUsers
+    extend ActiveSupport::Concern
+
+    included do
+      class << self
+        def add_users(source, users, access_level, current_user: nil, expires_at: nil)
+          return [] unless users.present?
+
+          emails, users, existing_members = parse_users_list(source, users)
+
+          Member.transaction do
+            (emails + users).map! do |user|
+              new(source,
+                  user,
+                  access_level,
+                  existing_members: existing_members,
+                  current_user: current_user,
+                  expires_at: expires_at)
+                .execute
+            end
+          end
+        end
+
+        private
+
+        def parse_users_list(source, list)
+          emails = []
+          user_ids = []
+          users = []
+          existing_members = {}
+
+          list.each do |item|
+            case item
+            when User
+              users << item
+            when Integer
+              user_ids << item
+            when /\A\d+\Z/
+              user_ids << item.to_i
+            when Devise.email_regexp
+              emails << item
+            end
+          end
+
+          if user_ids.present?
+            # the below will automatically discard invalid user_ids
+            users.concat(User.id_in(user_ids))
+            # 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
+          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
+    end
+
+    def initialize(source, user, access_level, **args)
+      super
+
+      @existing_members = args[:existing_members] || (raise ArgumentError, "existing_members must be included in the args hash")
+    end
+
+    private
+
+    attr_reader :existing_members
+
+    def find_or_initialize_member_by_user
+      existing_members[user.id] || source.members.build(user_id: user.id)
+    end
+  end
+end

app/services/members/creator_service.rb

@@ -12,54 +12,6 @@ module Members
       def access_levels
         raise NotImplementedError
       end
-
-      def add_users(source, users, access_level, current_user: nil, expires_at: nil)
-        return [] unless users.present?
-
-        emails, users, existing_members = parse_users_list(source, users)
-
-        Member.transaction do
-          (emails + users).map! do |user|
-            new(source,
-                user,
-                access_level,
-                existing_members: existing_members,
-                current_user: current_user,
-                expires_at: expires_at)
-              .execute
-          end
-        end
-      end
-
-      private
-
-      def parse_users_list(source, list)
-        emails = []
-        user_ids = []
-        users = []
-        existing_members = {}
-
-        list.each do |item|
-          case item
-          when User
-            users << item
-          when Integer
-            user_ids << item
-          when /\A\d+\Z/
-            user_ids << item.to_i
-          when Devise.email_regexp
-            emails << item
-          end
-        end
-
-        if user_ids.present?
-          users.concat(User.id_in(user_ids))
-          # the below will automatically discard invalid user_ids
-          existing_members = source.members_and_requesters.where(user_id: user_ids).index_by(&:user_id) # rubocop:todo CodeReuse/ActiveRecord
-        end
-
-        [emails, users, existing_members]
-      end
     end
 
     def initialize(source, user, access_level, **args)
@@ -149,18 +101,10 @@ module Members
     end
 
     def find_or_initialize_member_by_user
-      if existing_members
-        # TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/334062
-        # i'm not so sure this is needed as the parse_users_list looks at members_and_requesters...
-        # so it is like we could just do a find or initialize by here and be fine
-        existing_members[user.id] || source.members.build(user_id: user.id)
-      else
-        source.members_and_requesters.find_or_initialize_by(user_id: user.id) # rubocop:todo CodeReuse/ActiveRecord
-      end
-    end
-
-    def existing_members
-      args[:existing_members]
+      # have to use members and requesters here since project/group limits on requested_at being nil for members and
+      # wouldn't be found in `source.members` if it already existed
+      # this of course will not treat active invites the same since we aren't searching on email
+      source.members_and_requesters.find_or_initialize_by(user_id: user.id) # rubocop:disable CodeReuse/ActiveRecord
     end
 
     def ldap

app/services/members/groups/bulk_creator_service.rb

+# frozen_string_literal: true
+
+module Members
+  module Groups
+    class BulkCreatorService < Members::Groups::CreatorService
+      include Members::BulkCreateUsers
+    end
+  end
+end

app/services/members/projects/bulk_creator_service.rb

+# frozen_string_literal: true
+
+module Members
+  module Projects
+    class BulkCreatorService < Members::Projects::CreatorService
+      include Members::BulkCreateUsers
+    end
+  end
+end

spec/services/members/groups/bulk_creator_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Members::Groups::BulkCreatorService do
+  it_behaves_like 'bulk member creation' do
+    let_it_be(:source, reload: true) { create(:group, :public) }
+    let_it_be(:member_type) { GroupMember }
+  end
+end

spec/services/members/projects/bulk_creator_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Members::Projects::BulkCreatorService do
+  it_behaves_like 'bulk member creation' do
+    let_it_be(:source, reload: true) { create(:project, :public) }
+    let_it_be(:member_type) { ProjectMember }
+  end
+end

spec/support/shared_examples/models/member_shared_examples.rb

@@ -300,8 +300,21 @@ RSpec.shared_examples_for "member creation" do
       end
     end
   end
+end
+
+RSpec.shared_examples_for "bulk member creation" do
+  let_it_be(:user) { create(:user) }
+  let_it_be(:admin) { create(:admin) }
+
+  describe '#execute' do
+    it 'raises an error when exiting_members is not passed in the args hash' do
+      expect do
+        described_class.new(source, user, :maintainer, current_user: user).execute
+      end.to raise_error(ArgumentError, 'existing_members must be included in the args hash')
+    end
+  end
 
-  describe '.add_users' do
+  describe '.add_users', :aggregate_failures do
     let_it_be(:user1) { create(:user) }
     let_it_be(:user2) { create(:user) }
 
@@ -310,8 +323,8 @@ RSpec.shared_examples_for "member creation" do
 
       expect(members).to be_a Array
       expect(members.size).to eq(2)
-      expect(members.first).to be_a member_type
-      expect(members.first).to be_persisted
+      expect(members).to all(be_a(member_type))
+      expect(members).to all(be_persisted)
     end
 
     it 'returns an empty array' do
@@ -329,5 +342,42 @@ RSpec.shared_examples_for "member creation" do
       expect(members.size).to eq(4)
       expect(members.first).to be_invite
     end
+
+    context 'with de-duplication' do
+      it 'with 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).to all(be_a(member_type))
+        expect(members).to all(be_persisted)
+      end
+
+      it 'with 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).to all(be_a(member_type))
+        expect(members).to all(be_persisted)
+      end
+    end
+
+    context 'when a member already exists' do
+      before do
+        source.add_user(user1, :developer)
+      end
+
+      it 'supports existing users as expected' do
+        user3 = create(:user)
+
+        members = described_class.add_users(source, [user1.id, user2, user3.id], :maintainer)
+
+        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)
+      end
+    end
   end
 end