Merge branch '247208-remove-single-member-exists-error' into 'master'

Allows invite modal to re-invite single users

See merge request gitlab-org/gitlab!80177

app/services/concerns/members/bulk_create_users.rb

@@ -48,6 +48,7 @@ module Members
           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))
             # helps not have to perform another query per user id to see if the member exists later on when fetching

app/services/members/creator_service.rb

@@ -67,6 +67,7 @@ module Members
     def create_member_task
       return unless member.persisted?
       return if member_task_attributes.value?(nil)
+      return if member.member_task.present?
 
       member.create_member_task(member_task_attributes)
     end

lib/api/helpers/members_helpers.rb

@@ -62,13 +62,10 @@ module API
       end
 
       def add_single_member_by_user_id(create_service_params)
-        source = create_service_params[:source]
         user_id = create_service_params[:user_ids]
         user = User.find_by(id: user_id) # rubocop: disable CodeReuse/ActiveRecord
 
         if user
-          conflict!('Member already exists') if member_already_exists?(source, user_id)
-
           instance = ::Members::CreateService.new(current_user, create_service_params)
           instance.execute
 
@@ -90,12 +87,6 @@ module API
       def add_single_member?(user_id)
         user_id.present?
       end
-
-      private
-
-      def member_already_exists?(source, user_id)
-        source.members.exists?(user_id: user_id) # rubocop: disable CodeReuse/ActiveRecord
-      end
     end
   end
 end

spec/requests/api/members_spec.rb

@@ -291,6 +291,25 @@ RSpec.describe API::Members do
               user: maintainer
             )
           end
+
+          context 'with an already existing member' do
+            before do
+              source.add_developer(stranger)
+            end
+
+            it 'tracks the invite source from params' do
+              post api("/#{source_type.pluralize}/#{source.id}/members", maintainer),
+                   params: params.merge(invite_source: '_invite_source_')
+
+              expect_snowplow_event(
+                category: 'Members::CreateService',
+                action: 'create_member',
+                label: '_invite_source_',
+                property: 'existing_user',
+                user: maintainer
+              )
+            end
+          end
         end
 
         context 'when executing the Members::CreateService for multiple user_ids' do
@@ -399,6 +418,49 @@ RSpec.describe API::Members do
             expect(member.tasks_to_be_done).to match_array([:code, :ci])
             expect(member.member_task.project_id).to eq(project_id)
           end
+
+          context 'with already existing member' do
+            before do
+              source.add_developer(stranger)
+            end
+
+            it 'does not update tasks to be done if tasks already exist', :aggregate_failures do
+              member = source.members.find_by(user_id: stranger.id)
+              create(:member_task, member: member, project_id: project_id, tasks_to_be_done: %w(code ci))
+
+              expect do
+                post api("/#{source_type.pluralize}/#{source.id}/members", maintainer),
+                     params: {
+                       user_id: stranger.id,
+                       access_level: Member::DEVELOPER,
+                       tasks_to_be_done: %w(issues),
+                       tasks_project_id: project_id
+                     }
+              end.not_to change(MemberTask, :count)
+
+              member.reset
+              expect(response).to have_gitlab_http_status(:created)
+              expect(member.tasks_to_be_done).to match_array([:code, :ci])
+              expect(member.member_task.project_id).to eq(project_id)
+            end
+
+            it 'adds tasks to be done if they do not exist', :aggregate_failures do
+              expect do
+                post api("/#{source_type.pluralize}/#{source.id}/members", maintainer),
+                     params: {
+                       user_id: stranger.id,
+                       access_level: Member::DEVELOPER,
+                       tasks_to_be_done: %w(issues),
+                       tasks_project_id: project_id
+                     }
+              end.to change(MemberTask, :count).by(1)
+
+              member = source.members.find_by(user_id: stranger.id)
+              expect(response).to have_gitlab_http_status(:created)
+              expect(member.tasks_to_be_done).to match_array([:issues])
+              expect(member.member_task.project_id).to eq(project_id)
+            end
+          end
         end
 
         context 'when there are multiple users to add' do
@@ -412,14 +474,68 @@ RSpec.describe API::Members do
               expect(member.member_task.project_id).to eq(project_id)
             end
           end
+
+          context 'with already existing members' do
+            before do
+              source.add_developer(stranger)
+              source.add_developer(developer)
+            end
+
+            it 'does not update tasks to be done if tasks already exist', :aggregate_failures do
+              members = source.members.where(user_id: [developer.id, stranger.id])
+              members.each do |member|
+                create(:member_task, member: member, project_id: project_id, tasks_to_be_done: %w(code ci))
+              end
+
+              expect do
+                post api("/#{source_type.pluralize}/#{source.id}/members", maintainer),
+                     params: {
+                       user_id: [developer.id, stranger.id].join(','),
+                       access_level: Member::DEVELOPER,
+                       tasks_to_be_done: %w(issues),
+                       tasks_project_id: project_id
+                     }
+              end.not_to change(MemberTask, :count)
+
+              expect(response).to have_gitlab_http_status(:created)
+              members.each do |member|
+                member.reset
+                expect(member.tasks_to_be_done).to match_array([:code, :ci])
+                expect(member.member_task.project_id).to eq(project_id)
+              end
+            end
+
+            it 'adds tasks to be done if they do not exist', :aggregate_failures do
+              expect do
+                post api("/#{source_type.pluralize}/#{source.id}/members", maintainer),
+                     params: {
+                       user_id: [developer.id, stranger.id].join(','),
+                       access_level: Member::DEVELOPER,
+                       tasks_to_be_done: %w(issues),
+                       tasks_project_id: project_id
+                     }
+              end.to change(MemberTask, :count).by(2)
+
+              expect(response).to have_gitlab_http_status(:created)
+              members = source.members.where(user_id: [developer.id, stranger.id])
+              members.each do |member|
+                expect(member.tasks_to_be_done).to match_array([:issues])
+                expect(member.member_task.project_id).to eq(project_id)
+              end
+            end
+          end
         end
       end
 
-      it "returns 409 if member already exists" do
+      it "updates a current member" do
+        source.add_guest(stranger)
+
         post api("/#{source_type.pluralize}/#{source.id}/members", maintainer),
-             params: { user_id: maintainer.id, access_level: Member::MAINTAINER }
+             params: { user_id: stranger.id, access_level: Member::MAINTAINER }
 
-        expect(response).to have_gitlab_http_status(:conflict)
+        expect(response).to have_gitlab_http_status(:created)
+        expect(json_response['id']).to eq(stranger.id)
+        expect(json_response['access_level']).to eq(Member::MAINTAINER)
       end
 
       it 'returns 404 when the user_id is not valid' do

spec/services/members/create_service_spec.rb

@@ -117,6 +117,24 @@ RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_
           user: user
         )
       end
+
+      context 'with an already existing member' do
+        before do
+          source.add_developer(member)
+        end
+
+        it 'tracks the invite source from params' do
+          execute_service
+
+          expect_snowplow_event(
+            category: described_class.name,
+            action: 'create_member',
+            label: '_invite_source_',
+            property: 'existing_user',
+            user: user
+          )
+        end
+      end
     end
 
     context 'when it is a net_new_user' do

spec/support/shared_examples/models/member_shared_examples.rb

@@ -301,8 +301,9 @@ RSpec.shared_examples_for "member creation" do
     end
 
     context 'when `tasks_to_be_done` and `tasks_project_id` are passed' do
+      let(:task_project) { source.is_a?(Group) ? create(:project, group: source) : source }
+
       it 'creates a member_task with the correct attributes', :aggregate_failures do
-        task_project = source.is_a?(Group) ? create(:project, group: source) : source
         described_class.new(source, user, :developer, tasks_to_be_done: %w(ci code), tasks_project_id: task_project.id).execute
 
         member = source.members.last
@@ -310,6 +311,43 @@ RSpec.shared_examples_for "member creation" do
         expect(member.tasks_to_be_done).to match_array([:ci, :code])
         expect(member.member_task.project).to eq(task_project)
       end
+
+      context 'with an already existing member' do
+        before do
+          source.add_user(user, :developer)
+        end
+
+        it 'does not update tasks to be done if tasks already exist', :aggregate_failures do
+          member = source.members.find_by(user_id: user.id)
+          create(:member_task, member: member, project: task_project, tasks_to_be_done: %w(code ci))
+
+          expect do
+            described_class.new(source,
+                                user,
+                                :developer,
+                                tasks_to_be_done: %w(issues),
+                                tasks_project_id: task_project.id).execute
+          end.not_to change(MemberTask, :count)
+
+          member.reset
+          expect(member.tasks_to_be_done).to match_array([:code, :ci])
+          expect(member.member_task.project).to eq(task_project)
+        end
+
+        it 'adds tasks to be done if they do not exist', :aggregate_failures do
+          expect do
+            described_class.new(source,
+                                user,
+                                :developer,
+                                tasks_to_be_done: %w(issues),
+                                tasks_project_id: task_project.id).execute
+          end.to change(MemberTask, :count).by(1)
+
+          member = source.members.find_by(user_id: user.id)
+          expect(member.tasks_to_be_done).to match_array([:issues])
+          expect(member.member_task.project).to eq(task_project)
+        end
+      end
     end
   end
 end
@@ -393,14 +431,52 @@ RSpec.shared_examples_for "bulk member creation" do
     end
 
     context 'when `tasks_to_be_done` and `tasks_project_id` are passed' do
+      let(:task_project) { source.is_a?(Group) ? create(:project, group: source) : source }
+
       it 'creates a member_task with the correct attributes', :aggregate_failures do
-        task_project = source.is_a?(Group) ? create(:project, group: source) : source
         members = described_class.add_users(source, [user1], :developer, tasks_to_be_done: %w(ci code), tasks_project_id: task_project.id)
         member = members.last
 
         expect(member.tasks_to_be_done).to match_array([:ci, :code])
         expect(member.member_task.project).to eq(task_project)
       end
+
+      context 'with an already existing member' do
+        before do
+          source.add_user(user1, :developer)
+        end
+
+        it 'does not update tasks to be done if tasks already exist', :aggregate_failures do
+          member = source.members.find_by(user_id: user1.id)
+          create(:member_task, member: member, project: task_project, tasks_to_be_done: %w(code ci))
+
+          expect do
+            described_class.add_users(source,
+                                      [user1.id],
+                                      :developer,
+                                      tasks_to_be_done: %w(issues),
+                                      tasks_project_id: task_project.id)
+          end.not_to change(MemberTask, :count)
+
+          member.reset
+          expect(member.tasks_to_be_done).to match_array([:code, :ci])
+          expect(member.member_task.project).to eq(task_project)
+        end
+
+        it 'adds tasks to be done if they do not exist', :aggregate_failures do
+          expect do
+            described_class.add_users(source,
+                                      [user1.id],
+                                      :developer,
+                                      tasks_to_be_done: %w(issues),
+                                      tasks_project_id: task_project.id)
+          end.to change(MemberTask, :count).by(1)
+
+          member = source.members.find_by(user_id: user1.id)
+          expect(member.tasks_to_be_done).to match_array([:issues])
+          expect(member.member_task.project).to eq(task_project)
+        end
+      end
     end
   end
 end