Merge branch...

Merge branch '350999-deprecate-use-of-members-api-for-invite-modal-in-favor-of-invitations-api-7' into 'master'

Update Invitations Service to accept users

See merge request gitlab-org/gitlab!83534

app/services/members/create_service.rb

@@ -14,7 +14,7 @@ module Members
       super
 
       @errors = []
-      @invites = invites_from_params&.split(',')&.uniq&.flatten
+      @invites = invites_from_params
       @source = params[:source]
     end
 
@@ -43,7 +43,9 @@ module Members
     attr_reader :source, :errors, :invites, :member_created_namespace_id, :members
 
     def invites_from_params
-      params[:user_ids]
+      return params[:user_ids] if params[:user_ids].is_a?(Array)
+
+      params[:user_ids]&.to_s&.split(',')&.uniq&.flatten || []
     end
 
     def validate_invite_source!
@@ -87,6 +89,7 @@ module Members
       end
     end
 
+    # overridden
     def add_error_for_member(member)
       prefix = "#{member.user.username}: " if member.user.present?
 
@@ -117,7 +120,8 @@ module Members
     def create_tasks_to_be_done
       return if params[:tasks_to_be_done].blank? || params[:tasks_project_id].blank?
 
-      valid_members = members.select { |member| member.valid? && member.member_task.valid? }
+      # Only create task issues for existing users. Tasks for new users are created when they signup.
+      valid_members = members.select { |member| member.valid? && member.member_task.valid? && member.user.present? }
       return unless valid_members.present?
 
       # We can take the first `member_task` here, since all tasks will have the same attributes needed

app/services/members/invite_service.rb

@@ -7,6 +7,8 @@ module Members
     def initialize(*args)
       super
 
+      @invites += parsed_emails
+
       @errors = {}
     end
 
@@ -14,38 +16,63 @@ module Members
 
     alias_method :formatted_errors, :errors
 
-    def invites_from_params
-      params[:email]
+    def parsed_emails
+      # can't put this in the initializer since `invites_from_params` is called in super class
+      # and needs it
+      @parsed_emails ||= (formatted_param(params[:email]) || [])
+    end
+
+    def formatted_param(parameter)
+      parameter&.split(',')&.uniq&.flatten
     end
 
     def validate_invitable!
       super
 
+      return if params[:email].blank?
+
       # we need the below due to add_users hitting Members::CreatorService.parse_users_list and ignoring invalid emails
       # ideally we wouldn't need this, but we can't really change the add_users method
-      valid, invalid = invites.partition { |email| Member.valid_email?(email) }
-      @invites = valid
+      invalid_emails.each { |email| errors[email] = s_('AddMember|Invite email is invalid') }
+    end
 
-      invalid.each { |email| errors[email] = s_('AddMember|Invite email is invalid') }
+    def invalid_emails
+      parsed_emails.each_with_object([]) do |email, invalid|
+        next if Member.valid_email?(email)
+
+        invalid << email
+        @invites.delete(email)
+      end
     end
 
     override :blank_invites_message
     def blank_invites_message
-      s_('AddMember|Emails cannot be blank')
+      s_('AddMember|Invites cannot be blank')
     end
 
     override :add_error_for_member
     def add_error_for_member(member)
-      errors[invite_email(member)] = member.errors.full_messages.to_sentence
+      errors[invited_object(member)] = member.errors.full_messages.to_sentence
     end
 
-    override :create_tasks_to_be_done
-    def create_tasks_to_be_done
-      # Only create task issues for existing users. Tasks for new users are created when they signup.
-    end
+    def invited_object(member)
+      return member.invite_email if member.invite_email
 
-    def invite_email(member)
-      member.invite_email || member.user.email
+      # There is a case where someone was invited by email, but the `user` record exists.
+      # The member record returned will not have an invite_email attribute defined since
+      # the CreatorService finds `user` record sometimes by email.
+      # At that point we lose the info of whether this invite was done by `user` or by email.
+      # Here we will give preference to check invites by user_id first.
+      # There is also a case where a user could be invited by their email and
+      # at the same time via the API in the same request.
+      # This would would mean the same user is invited as user_id and email.
+      # However, that isn't as likely from the UI at least since the token generator checks
+      # for that case and doesn't allow email being used if the user exists as a record already.
+      if member.user_id.to_s.in?(invites)
+        member.user.username
+      else
+        member.user.all_emails.detect { |email| email.in?(invites) }
+      end
     end
   end
 end

locale/gitlab.pot

@@ -2334,15 +2334,15 @@ msgstr ""
 msgid "AddContextCommits|Add/remove"
 msgstr ""
 
-msgid "AddMember|Emails cannot be blank"
-msgstr ""
-
 msgid "AddMember|Invite email is invalid"
 msgstr ""
 
 msgid "AddMember|Invite limit of %{daily_invites} per day exceeded"
 msgstr ""
 
+msgid "AddMember|Invites cannot be blank"
+msgstr ""
+
 msgid "AddMember|No invite source provided."
 msgstr ""
 

spec/requests/api/invitations_spec.rb

@@ -240,7 +240,7 @@ RSpec.describe API::Invitations do
              params: { email: '', access_level: Member::MAINTAINER }
 
         expect(response).to have_gitlab_http_status(:created)
-        expect(json_response['message']).to eq('Emails cannot be blank')
+        expect(json_response['message']).to eq('Invites cannot be blank')
       end
 
       it 'returns 404 when the email list is not a valid format' do

spec/services/members/create_service_spec.rb

@@ -6,6 +6,7 @@ RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_
   let_it_be(:source, reload: true) { create(:project) }
   let_it_be(:user) { create(:user) }
   let_it_be(:member) { create(:user) }
+  let_it_be(:user_invited_by_id) { create(:user) }
   let_it_be(:user_ids) { member.id.to_s }
   let_it_be(:access_level) { Gitlab::Access::GUEST }
 
@@ -49,6 +50,36 @@ RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_
       expect(OnboardingProgress.completed?(source.namespace, :user_added)).to be(true)
     end
 
+    context 'when user_id is passed as an integer' do
+      let(:user_ids) { member.id }
+
+      it 'successfully creates member' do
+        expect(execute_service[:status]).to eq(:success)
+        expect(source.users).to include member
+        expect(OnboardingProgress.completed?(source.namespace, :user_added)).to be(true)
+      end
+    end
+
+    context 'with user_ids as an array of integers' do
+      let(:user_ids) { [member.id, user_invited_by_id.id] }
+
+      it 'successfully creates members' do
+        expect(execute_service[:status]).to eq(:success)
+        expect(source.users).to include(member, user_invited_by_id)
+        expect(OnboardingProgress.completed?(source.namespace, :user_added)).to be(true)
+      end
+    end
+
+    context 'with user_ids as an array of strings' do
+      let(:user_ids) { [member.id.to_s, user_invited_by_id.id.to_s] }
+
+      it 'successfully creates members' do
+        expect(execute_service[:status]).to eq(:success)
+        expect(source.users).to include(member, user_invited_by_id)
+        expect(OnboardingProgress.completed?(source.namespace, :user_added)).to be(true)
+      end
+    end
+
     context 'when executing on a group' do
       let_it_be(:source) { create(:group) }
 
@@ -191,6 +222,15 @@ RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_
       )
     end
 
+    context 'when it is an invite by email passed to user_ids' do
+      let(:user_ids) { 'email@example.org' }
+
+      it 'does not create task issues' do
+        expect(TasksToBeDone::CreateWorker).not_to receive(:perform_async)
+        execute_service
+      end
+    end
+
     context 'when passing many user ids' do
       before do
         stub_licensed_features(multiple_issue_assignees: false)

spec/services/members/invite_service_spec.rb

@@ -6,6 +6,7 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_
   let_it_be(:project, reload: true) { create(:project) }
   let_it_be(:user) { project.first_owner }
   let_it_be(:project_user) { create(:user) }
+  let_it_be(:user_invited_by_id) { create(:user) }
   let_it_be(:namespace) { project.namespace }
 
   let(:params) { {} }
@@ -41,19 +42,27 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_
     end
   end
 
-  context 'when email is not a valid email' do
-    let(:params) { { email: '_bogus_' } }
+  context 'when invites are passed as array' do
+    context 'with emails' do
+      let(:params) { { email: %w[email@example.org email2@example.org] } }
 
-    it 'returns an error' do
-      expect_not_to_create_members
-      expect(result[:message]['_bogus_']).to eq("Invite email is invalid")
+      it 'successfully creates members' do
+        expect_to_create_members(count: 2)
+        expect(result[:status]).to eq(:success)
+      end
     end
 
-    it_behaves_like 'does not record an onboarding progress action'
+    context 'with user_ids as integers' do
+      let(:params) { { user_ids: [project_user.id, user_invited_by_id.id] } }
+
+      it 'successfully creates members' do
+        expect_to_create_members(count: 2)
+        expect(result[:status]).to eq(:success)
+      end
     end
 
-  context 'when emails are passed as an array' do
-    let(:params) { { email: %w[email@example.org email2@example.org] } }
+    context 'with user_ids as strings' do
+      let(:params) { { user_ids: [project_user.id.to_s, user_invited_by_id.id.to_s] } }
 
       it 'successfully creates members' do
         expect_to_create_members(count: 2)
@@ -61,42 +70,175 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_
       end
     end
 
+    context 'with a mixture of emails and user_ids' do
+      let(:params) do
+        { user_ids: [project_user.id, user_invited_by_id.id], email: %w[email@example.org email2@example.org] }
+      end
+
+      it 'successfully creates members' do
+        expect_to_create_members(count: 4)
+        expect(result[:status]).to eq(:success)
+      end
+    end
+  end
+
+  context 'with multiple invites passed as strings' do
+    context 'with emails' do
+      let(:params) { { email: 'email@example.org,email2@example.org' } }
+
+      it 'successfully creates members' do
+        expect_to_create_members(count: 2)
+        expect(result[:status]).to eq(:success)
+      end
+    end
+
+    context 'with user_ids' do
+      let(:params) { { user_ids: "#{project_user.id},#{user_invited_by_id.id}" } }
+
+      it 'successfully creates members' do
+        expect_to_create_members(count: 2)
+        expect(result[:status]).to eq(:success)
+      end
+    end
+
+    context 'with a mixture of emails and user_ids' do
+      let(:params) do
+        { user_ids: "#{project_user.id},#{user_invited_by_id.id}", email: 'email@example.org,email2@example.org' }
+      end
+
+      it 'successfully creates members' do
+        expect_to_create_members(count: 4)
+        expect(result[:status]).to eq(:success)
+      end
+    end
+  end
+
+  context 'when invites formats are mixed' do
+    context 'when user_ids is an array and emails is a string' do
+      let(:params) do
+        { user_ids: [project_user.id, user_invited_by_id.id], email: 'email@example.org,email2@example.org' }
+      end
+
+      it 'successfully creates members' do
+        expect_to_create_members(count: 4)
+        expect(result[:status]).to eq(:success)
+      end
+    end
+
+    context 'when user_ids is a string and emails is an array' do
+      let(:params) do
+        { user_ids: "#{project_user.id},#{user_invited_by_id.id}", email: %w[email@example.org email2@example.org] }
+      end
+
+      it 'successfully creates members' do
+        expect_to_create_members(count: 4)
+        expect(result[:status]).to eq(:success)
+      end
+    end
+  end
+
+  context 'when invites are passed in different formats' do
     context 'when emails are passed as an empty string' do
       let(:params) { { email: '' } }
 
       it 'returns an error' do
         expect_not_to_create_members
-      expect(result[:message]).to eq('Emails cannot be blank')
+        expect(result[:message]).to eq('Invites cannot be blank')
+      end
+    end
+
+    context 'when user_ids are passed as an empty string' do
+      let(:params) { { user_ids: '' } }
+
+      it 'returns an error' do
+        expect_not_to_create_members
+        expect(result[:message]).to eq('Invites cannot be blank')
+      end
+    end
+
+    context 'when user_ids and emails are both passed as empty strings' do
+      let(:params) { { user_ids: '', email: '' } }
+
+      it 'returns an error' do
+        expect_not_to_create_members
+        expect(result[:message]).to eq('Invites cannot be blank')
+      end
+    end
+
+    context 'when user_id is passed as an integer' do
+      let(:params) { { user_ids: project_user.id } }
+
+      it 'successfully creates member' do
+        expect_to_create_members(count: 1)
+        expect(result[:status]).to eq(:success)
       end
     end
 
-  context 'when email param is not included' do
+    context 'when invite params are not included' do
       it 'returns an error' do
         expect_not_to_create_members
-      expect(result[:message]).to eq('Emails cannot be blank')
+        expect(result[:message]).to eq('Invites cannot be blank')
+      end
     end
   end
 
   context 'when email is not a valid email format' do
+    context 'with singular email' do
       let(:params) { { email: '_bogus_' } }
 
       it 'returns an error' do
-      expect { result }.not_to change(ProjectMember, :count)
+        expect_not_to_create_members
         expect(result[:status]).to eq(:error)
         expect(result[:message][params[:email]]).to eq("Invite email is invalid")
       end
+
+      it_behaves_like 'does not record an onboarding progress action'
+    end
+
+    context 'with user_id and singular invalid email' do
+      let(:params) { { user_ids: project_user.id, email: '_bogus_' } }
+
+      it 'has partial success' do
+        expect_to_create_members(count: 1)
+        expect(project.users).to include project_user
+
+        expect(result[:status]).to eq(:error)
+        expect(result[:message][params[:email]]).to eq("Invite email is invalid")
+      end
+    end
   end
 
-  context 'when duplicate email addresses are passed' do
+  context 'with duplicate invites' do
+    context 'with duplicate emails' do
       let(:params) { { email: 'email@example.org,email@example.org' } }
 
-    it 'only creates one member per unique address' do
+      it 'only creates one member per unique invite' do
         expect_to_create_members(count: 1)
         expect(result[:status]).to eq(:success)
       end
     end
 
-  context 'when observing email limits' do
+    context 'with duplicate user ids' do
+      let(:params) { { user_ids: "#{project_user.id},#{project_user.id}" } }
+
+      it 'only creates one member per unique invite' do
+        expect_to_create_members(count: 1)
+        expect(result[:status]).to eq(:success)
+      end
+    end
+
+    context 'with duplicate member by adding as user id and email' do
+      let(:params) { { user_ids: project_user.id, email: project_user.email } }
+
+      it 'only creates one member per unique invite' do
+        expect_to_create_members(count: 1)
+        expect(result[:status]).to eq(:success)
+      end
+    end
+  end
+
+  context 'when observing invite limits' do
+    context 'with emails and in general' do
       let_it_be(:emails) { Array(1..101).map { |n| "email#{n}@example.com" } }
 
       context 'when over the allowed default limit of emails' do
@@ -127,7 +269,19 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_
       end
     end
 
-  context 'when email belongs to an existing user' do
+    context 'with user_ids' do
+      let(:user_ids) { 1.upto(101).to_a.join(',') }
+      let(:params) { { user_ids: user_ids } }
+
+      it 'limits the number of users to 100' do
+        expect_not_to_create_members
+        expect(result[:message]).to eq('Too many users specified (limit is 100)')
+      end
+    end
+  end
+
+  context 'with an existing user' do
+    context 'with email' do
       let(:params) { { email: project_user.email } }
 
       it 'adds an existing user to members' do
@@ -137,17 +291,68 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_
       end
     end
 
+    context 'with user_id' do
+      let(:params) { { user_ids: project_user.id } }
+
+      it_behaves_like 'records an onboarding progress action', :user_added
+
+      it 'adds an existing user to members' do
+        expect_to_create_members(count: 1)
+        expect(result[:status]).to eq(:success)
+        expect(project.users).to include project_user
+      end
+
+      context 'when assigning tasks to be done' do
+        let(:params) do
+          { user_ids: project_user.id, tasks_to_be_done: %w(ci code), tasks_project_id: project.id }
+        end
+
+        it 'creates 2 task issues', :aggregate_failures do
+          expect(TasksToBeDone::CreateWorker)
+            .to receive(:perform_async)
+                  .with(anything, user.id, [project_user.id])
+                  .once
+                  .and_call_original
+          expect { result }.to change { project.issues.count }.by(2)
+
+          expect(project.issues).to all have_attributes(project: project, author: user)
+        end
+      end
+    end
+  end
+
   context 'when access level is not valid' do
+    context 'with email' do
       let(:params) { { email: project_user.email, access_level: -1 } }
 
       it 'returns an error' do
         expect_not_to_create_members
-      expect(result[:message][project_user.email])
-        .to eq("Access level is not included in the list")
+        expect(result[:message][project_user.email]).to eq("Access level is not included in the list")
+      end
+    end
+
+    context 'with user_id' do
+      let(:params) { { user_ids: user_invited_by_id.id, access_level: -1 } }
+
+      it 'returns an error' do
+        expect_not_to_create_members
+        expect(result[:message][user_invited_by_id.username]).to eq("Access level is not included in the list")
+      end
+    end
+
+    context 'with a mix of user_id and email' do
+      let(:params) { { user_ids: user_invited_by_id.id, email: project_user.email, access_level: -1 } }
+
+      it 'returns errors' do
+        expect_not_to_create_members
+        expect(result[:message][project_user.email]).to eq("Access level is not included in the list")
+        expect(result[:message][user_invited_by_id.username]).to eq("Access level is not included in the list")
+      end
     end
   end
 
-  context 'when invite already exists for an included email' do
+  context 'when member already exists' do
+    context 'with email' do
       let!(:invited_member) { create(:project_member, :invited, project: project) }
       let(:params) { { email: "#{invited_member.invite_email},#{project_user.email}" } }
 
@@ -160,29 +365,99 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_
       end
     end
 
-  context 'when access request already exists for an included email' do
-    let!(:requested_member) { create(:project_member, :access_request, project: project) }
-    let(:params) { { email: "#{requested_member.user.email},#{project_user.email}" } }
+    context 'when email is already a member with a user on the project' do
+      let!(:existing_member) { create(:project_member, :guest, project: project) }
+      let(:params) { { email: "#{existing_member.user.email}" } }
 
-    it 'adds new email and returns an error for the already invited email' do
+      it 'returns an error for the already invited email' do
+        expect_not_to_create_members
+        expect(result[:message][existing_member.user.email]).to eq("User already exists in source")
+      end
+
+      context 'when email belongs to an existing user as a secondary email' do
+        let(:secondary_email) { create(:email, email: 'secondary@example.com', user: existing_member.user) }
+        let(:params) { { email: "#{secondary_email.email}" } }
+
+        it 'returns an error for the already invited email' do
+          expect_not_to_create_members
+          expect(result[:message][secondary_email.email]).to eq("User already exists in source")
+        end
+      end
+    end
+
+    context 'with user_id that already exists' do
+      let!(:existing_member) { create(:project_member, project: project, user: project_user) }
+      let(:params) { { user_ids: existing_member.user_id } }
+
+      it 'does not add the member again and is successful' do
+        expect_to_create_members(count: 0)
+        expect(project.users).to include project_user
+      end
+    end
+
+    context 'with user_id that already exists with a lower access_level' do
+      let!(:existing_member) { create(:project_member, :developer, project: project, user: project_user) }
+      let(:params) { { user_ids: existing_member.user_id, access_level: ProjectMember::MAINTAINER } }
+
+      it 'does not add the member again and updates the access_level' do
+        expect_to_create_members(count: 0)
+        expect(project.users).to include project_user
+        expect(existing_member.reset.access_level).to eq ProjectMember::MAINTAINER
+      end
+    end
+
+    context 'with user_id that already exists with a higher access_level' do
+      let!(:existing_member) { create(:project_member, :developer, project: project, user: project_user) }
+      let(:params) { { user_ids: existing_member.user_id, access_level: ProjectMember::GUEST } }
+
+      it 'does not add the member again and updates the access_level' do
+        expect_to_create_members(count: 0)
+        expect(project.users).to include project_user
+        expect(existing_member.reset.access_level).to eq ProjectMember::GUEST
+      end
+    end
+
+    context 'with user_id that already exists in a parent group' do
+      let_it_be(:group) { create(:group) }
+      let_it_be(:group_member) { create(:group_member, :developer, source: group, user: project_user) }
+      let_it_be(:project, reload: true) { create(:project, group: group) }
+      let_it_be(:user) { project.creator }
+
+      before_all do
+        project.add_maintainer(user)
+      end
+
+      context 'when access_level is lower than inheriting member' do
+        let(:params) { { user_ids: group_member.user_id, access_level: ProjectMember::GUEST }}
+
+        it 'does not add the member and returns an error' do
+          msg = "Access level should be greater than or equal " \
+                "to Developer inherited membership from group #{group.name}"
+
+          expect_not_to_create_members
+          expect(result[:message][project_user.username]).to eq msg
+        end
+      end
+
+      context 'when access_level is the same as the inheriting member' do
+        let(:params) { { user_ids: group_member.user_id, access_level: ProjectMember::DEVELOPER }}
+
+        it 'adds the member with correct access_level' do
           expect_to_create_members(count: 1)
-      expect(result[:status]).to eq(:error)
-      expect(result[:message][requested_member.user.email])
-        .to eq("User already exists in source")
           expect(project.users).to include project_user
+          expect(project.project_members.last.access_level).to eq ProjectMember::DEVELOPER
         end
       end
 
-  context 'when email is already a member on the project' do
-    let!(:existing_member) { create(:project_member, :guest, project: project) }
-    let(:params) { { email: "#{existing_member.user.email},#{project_user.email}" } }
+      context 'when access_level is greater than the inheriting member' do
+        let(:params) { { user_ids: group_member.user_id, access_level: ProjectMember::MAINTAINER }}
 
-    it 'adds new email and returns an error for the already invited email' do
+        it 'adds the member with correct access_level' do
           expect_to_create_members(count: 1)
-      expect(result[:status]).to eq(:error)
-      expect(result[:message][existing_member.user.email])
-        .to eq("User already exists in source")
           expect(project.users).to include project_user
+          expect(project.project_members.last.access_level).to eq ProjectMember::MAINTAINER
+        end
+      end
     end
   end