Merge branch '323078-combine-concepts-in-inviting-member-service-classes-5' into 'master'

Refactor member/invitation services to share common code

See merge request gitlab-org/gitlab!57618

app/controllers/admin/groups_controller.rb

@@ -62,7 +62,7 @@ class Admin::GroupsController < Admin::ApplicationController
 
   def members_update
     member_params = params.permit(:user_ids, :access_level, :expires_at)
-    result = Members::CreateService.new(current_user, member_params.merge(limit: -1)).execute(@group)
+    result = Members::CreateService.new(current_user, member_params.merge(limit: -1, source: @group)).execute
 
     if result[:status] == :success
       redirect_to [:admin, @group], notice: _('Users were successfully added.')

app/controllers/concerns/membership_actions.rb

@@ -6,7 +6,7 @@ module MembershipActions
 
   def create
     create_params = params.permit(:user_ids, :access_level, :expires_at)
-    result = Members::CreateService.new(current_user, create_params).execute(membershipable)
+    result = Members::CreateService.new(current_user, create_params.merge({ source: membershipable })).execute
 
     if result[:status] == :success
       redirect_to members_page_url, notice: _('Users were successfully added.')

app/models/member.rb

@@ -284,6 +284,10 @@ class Member < ApplicationRecord
       Gitlab::Access.sym_options
     end
 
+    def valid_email?(email)
+      Devise.email_regexp.match?(email)
+    end
+
     private
 
     def parse_users_list(source, list)
@@ -305,6 +309,7 @@ class Member < ApplicationRecord
 
       if user_ids.present?
         users.concat(User.where(id: 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)
       end
 

app/services/members/create_service.rb

@@ -2,67 +2,98 @@
 
 module Members
   class CreateService < Members::BaseService
-    include Gitlab::Utils::StrongMemoize
+    BlankInvitesError = Class.new(StandardError)
+    TooManyInvitesError = Class.new(StandardError)
 
-    DEFAULT_LIMIT = 100
+    DEFAULT_INVITE_LIMIT = 100
 
-    def execute(source)
-      return error(s_('AddMember|No users specified.')) if user_ids.blank?
+    def initialize(*args)
+      super
 
-      return error(s_("AddMember|Too many users specified (limit is %{user_limit})") % { user_limit: user_limit }) if
-        user_limit && user_ids.size > user_limit
+      @errors = []
+      @invites = invites_from_params&.split(',')&.uniq&.flatten
+      @source = params[:source]
+    end
+
+    def execute
+      validate_invites!
+
+      add_members
+      enqueue_onboarding_progress_action
+      result
+    rescue BlankInvitesError, TooManyInvitesError => e
+      error(e.message)
+    end
+
+    private
+
+    attr_reader :source, :errors, :invites, :member_created_namespace_id
+
+    def invites_from_params
+      params[:user_ids]
+    end
+
+    def validate_invites!
+      raise BlankInvitesError, blank_invites_message if invites.blank?
+
+      return unless user_limit && invites.size > user_limit
+
+      raise TooManyInvitesError,
+            format(s_("AddMember|Too many users specified (limit is %{user_limit})"), user_limit: user_limit)
+    end
+
+    def blank_invites_message
+      s_('AddMember|No users specified.')
+    end
 
+    def add_members
       members = source.add_users(
-        user_ids,
+        invites,
         params[:access_level],
         expires_at: params[:expires_at],
         current_user: current_user
       )
 
-      errors = []
-
-      members.each do |member|
-        if member.invalid?
-          current_error =
-            # Invited users may not have an associated user
-            if member.user.present?
-              "#{member.user.username}: "
-            else
-              ""
-            end
-
-          current_error += member.errors.full_messages.to_sentence
-          errors << current_error
-        else
-          after_execute(member: member)
-        end
-      end
-
-      enqueue_onboarding_progress_action(source) if members.size > errors.size
-
-      return success unless errors.any?
+      members.each { |member| process_result(member) }
+    end
 
-      error(errors.to_sentence)
+    def process_result(member)
+      if member.invalid?
+        add_error_for_member(member)
+      else
+        after_execute(member: member)
+        @member_created_namespace_id ||= member.namespace_id
+      end
     end
 
-    private
+    def add_error_for_member(member)
+      prefix = "#{member.user.username}: " if member.user.present?
 
-    def user_ids
-      strong_memoize(:user_ids) do
-        ids = params[:user_ids] || ''
-        ids.split(',').uniq.flatten
-      end
+      errors << "#{prefix}#{member.errors.full_messages.to_sentence}"
     end
 
     def user_limit
-      limit = params.fetch(:limit, DEFAULT_LIMIT)
+      limit = params.fetch(:limit, DEFAULT_INVITE_LIMIT)
 
       limit && limit < 0 ? nil : limit
     end
 
-    def enqueue_onboarding_progress_action(source)
-      namespace_id = source.is_a?(Project) ? source.namespace_id : source.id
-      Namespaces::OnboardingUserAddedWorker.perform_async(namespace_id)
+    def enqueue_onboarding_progress_action
+      return unless member_created_namespace_id
+
+      Namespaces::OnboardingUserAddedWorker.perform_async(member_created_namespace_id)
+    end
+
+    def result
+      if errors.any?
+        error(formatted_errors)
+      else
+        success
+      end
+    end
+
+    def formatted_errors
+      errors.to_sentence
     end
   end
 end

app/services/members/invite_service.rb

 # frozen_string_literal: true
 
 module Members
-  class InviteService < Members::BaseService
-    BlankEmailsError = Class.new(StandardError)
-    TooManyEmailsError = Class.new(StandardError)
+  class InviteService < Members::CreateService
+    extend ::Gitlab::Utils::Override
 
     def initialize(*args)
       super
 
       @errors = {}
-      @emails = params[:email]&.split(',')&.uniq&.flatten
-      @source = params[:source]
-    end
-
-    def execute
-      validate_emails!
-
-      emails.each(&method(:process_email))
-      enqueue_onboarding_progress_action
-      result
-    rescue BlankEmailsError, TooManyEmailsError => e
-      error(e.message)
     end
 
     private
 
-    attr_reader :source, :errors, :emails, :member_created_namespace_id
-
-    def validate_emails!
-      raise BlankEmailsError, s_('AddMember|Email cannot be blank') if emails.blank?
-
-      if user_limit && emails.size > user_limit
-        raise TooManyEmailsError, s_("AddMember|Too many users specified (limit is %{user_limit})") % { user_limit: user_limit }
-      end
-    end
-
-    def user_limit
-      limit = params.fetch(:limit, Members::CreateService::DEFAULT_LIMIT)
-
-      limit < 0 ? nil : limit
-    end
-
-    def process_email(email)
-      return if existing_member?(email)
-      return if existing_invite?(email)
-      return if existing_request?(email)
-
-      add_member(email)
-    end
-
-    def existing_member?(email)
-      existing_member = source.members.with_user_by_email(email).exists?
-
-      if existing_member
-        errors[email] = s_("AddMember|Already a member of %{source_name}") % { source_name: source.name }
-        return true
-      end
+    alias_method :formatted_errors, :errors
 
-      false
+    def invites_from_params
+      params[:email]
     end
 
-    def existing_invite?(email)
-      existing_invite = source.members.search_invite_email(email).exists?
-
-      if existing_invite
-        errors[email] = s_("AddMember|Member already invited to %{source_name}") % { source_name: source.name }
-        return true
-      end
-
-      false
-    end
-
-    def existing_request?(email)
-      existing_request = source.requesters.with_user_by_email(email).exists?
+    def validate_invites!
+      super
 
-      if existing_request
-        errors[email] = s_("AddMember|Member cannot be invited because they already requested to join %{source_name}") % { source_name: source.name }
-        return true
-      end
+      # we need the below due to add_users hitting Member#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
 
-      false
+      invalid.each { |email| errors[email] = s_('AddMember|Invite email is invalid') }
     end
 
-    def add_member(email)
-      new_member = source.add_user(email, params[:access_level], current_user: current_user, expires_at: params[:expires_at])
-
-      if new_member.invalid?
-        errors[email] = new_member.errors.full_messages.to_sentence
-      else
-        after_execute(member: new_member)
-        @member_created_namespace_id ||= new_member.namespace_id
-      end
+    override :blank_invites_message
+    def blank_invites_message
+      s_('AddMember|Emails cannot be blank')
     end
 
-    def result
-      if errors.any?
-        error(errors)
-      else
-        success
-      end
+    override :add_error_for_member
+    def add_error_for_member(member)
+      errors[invite_email(member)] = member.errors.full_messages.to_sentence
     end
 
-    def enqueue_onboarding_progress_action
-      return unless member_created_namespace_id
-
-      Namespaces::OnboardingUserAddedWorker.perform_async(member_created_namespace_id)
+    def invite_email(member)
+      member.invite_email || member.user.email
     end
   end
 end
-
-Members::InviteService.prepend_if_ee('EE::Members::InviteService')

changelogs/unreleased/323078-combine-concepts-in-inviting-member-service-classes-5.yml

+---
+title: Refactor member/invitation services to share common code
+merge_request: 57618
+author:
+type: other

doc/api/invitations.md

@@ -61,8 +61,8 @@ When there was any error sending the email:
 {
   "status": "error",
   "message": {
-               "test@example.com": "Already invited",
-               "test2@example.com": "Member already exsists"
+               "test@example.com": "Invite email has already been taken",
+               "test2@example.com": "User already exists in source"
              }
 }
 ```

ee/app/controllers/concerns/group_invite_members.rb

@@ -5,11 +5,12 @@ module GroupInviteMembers
 
   def invite_members(group)
     invite_params = {
+      source: group,
       user_ids: emails_param[:emails]&.reject(&:blank?)&.join(','),
       access_level: Gitlab::Access::DEVELOPER
     }
 
-    result = Members::CreateService.new(current_user, invite_params).execute(group)
+    result = Members::CreateService.new(current_user, invite_params).execute
 
     ::Gitlab::Tracking.event(self.class.name, 'invite_members', label: 'new_group_form') if result[:status] == :success
   end

ee/app/controllers/registrations/group_invites_controller.rb

@@ -12,7 +12,7 @@ module Registrations
     end
 
     def create
-      result = Members::CreateService.new(current_user, invite_params).execute(group)
+      result = Members::CreateService.new(current_user, invite_params).execute
 
       if result[:status] == :success
         experiment(:registrations_group_invite, actor: current_user)
@@ -37,6 +37,7 @@ module Registrations
 
     def invite_params
       {
+        source: group,
         user_ids: emails_param[:emails]&.reject(&:blank?)&.join(','),
         access_level: Gitlab::Access::DEVELOPER
       }

ee/app/services/ee/members/create_service.rb

@@ -3,15 +3,30 @@
 module EE
   module Members
     module CreateService
-      extend ::Gitlab::Utils::Override
+      private
+
+      def validate_invites!
+        super
+
+        check_quota!
+      end
 
-      override :execute
-      def execute(source)
-        if invite_quota_exceeded?(source, user_ids)
-          return error(s_("AddMember|Invite limit of %{daily_invites} per day exceeded") % { daily_invites: source.actual_limits.daily_invites })
-        end
+      def check_quota!
+        return unless invite_quota_exceeded?
 
-        super(source)
+        raise ::Members::CreateService::TooManyInvitesError,
+              format(
+                s_("AddMember|Invite limit of %{daily_invites} per day exceeded"),
+                daily_invites: source.actual_limits.daily_invites
+              )
+      end
+
+      def invite_quota_exceeded?
+        return unless source.actual_limits.daily_invites
+
+        invite_count = ::Member.invite.created_today.in_hierarchy(source).count
+
+        source.actual_limits.exceeded?(:daily_invites, invite_count + invites.count)
       end
 
       def after_execute(member:)
@@ -20,8 +35,6 @@ module EE
         log_audit_event(member: member)
       end
 
-      private
-
       def log_audit_event(member:)
         ::AuditEventService.new(
           current_user,
@@ -29,14 +42,6 @@ module EE
           action: :create
         ).for_member(member).security_event
       end
-
-      def invite_quota_exceeded?(source, user_ids)
-        return unless source.actual_limits.daily_invites
-
-        invite_count = ::Member.invite.created_today.in_hierarchy(source).count
-
-        source.actual_limits.exceeded?(:daily_invites, invite_count + user_ids.count)
-      end
     end
   end
 end

ee/app/services/ee/members/invite_service.rb

-# frozen_string_literal: true
-
-module EE
-  module Members
-    module InviteService
-      private
-
-      def validate_emails!
-        super
-
-        if invite_quota_exceeded?
-          raise ::Members::InviteService::TooManyEmailsError,
-                s_("AddMember|Invite limit of %{daily_invites} per day exceeded") %
-                  { daily_invites: source.actual_limits.daily_invites }
-        end
-      end
-
-      def invite_quota_exceeded?
-        return unless source.actual_limits.daily_invites
-
-        invite_count = ::Member.invite.created_today.in_hierarchy(source).count
-
-        source.actual_limits.exceeded?(:daily_invites, invite_count + emails.count)
-      end
-
-      def after_execute(member:)
-        super
-
-        log_audit_event(member: member)
-      end
-
-      def log_audit_event(member:)
-        ::AuditEventService.new(
-          current_user,
-          member.source,
-          action: :create
-        ).for_member(member).security_event
-      end
-    end
-  end
-end

ee/spec/services/ee/members/create_service_spec.rb

@@ -12,7 +12,7 @@ RSpec.describe Members::CreateService do
 
   let(:params) { { user_ids: project_users.map(&:id).join(','), access_level: Gitlab::Access::GUEST } }
 
-  subject { described_class.new(user, params).execute(project) }
+  subject { described_class.new(user, params.merge({ source: project })).execute }
 
   before_all do
     project.add_maintainer(user)

lib/api/members.rb

@@ -100,9 +100,9 @@ module API
           authorize_admin_source!(source_type, source)
 
           if params[:user_id].to_s.include?(',')
-            create_service_params = params.except(:user_id).merge({ user_ids: params[:user_id] })
+            create_service_params = params.except(:user_id).merge({ user_ids: params[:user_id], source: source })
 
-            ::Members::CreateService.new(current_user, create_service_params).execute(source)
+            ::Members::CreateService.new(current_user, create_service_params).execute
           elsif params[:user_id].present?
             member = source.members.find_by(user_id: params[:user_id])
             conflict!('Member already exists') if member

locale/gitlab.pot

@@ -2022,21 +2022,15 @@ msgstr ""
 msgid "AddContextCommits|Add/remove"
 msgstr ""
 
-msgid "AddMember|Already a member of %{source_name}"
+msgid "AddMember|Emails cannot be blank"
 msgstr ""
 
-msgid "AddMember|Email cannot be blank"
+msgid "AddMember|Invite email is invalid"
 msgstr ""
 
 msgid "AddMember|Invite limit of %{daily_invites} per day exceeded"
 msgstr ""
 
-msgid "AddMember|Member already invited to %{source_name}"
-msgstr ""
-
-msgid "AddMember|Member cannot be invited because they already requested to join %{source_name}"
-msgstr ""
-
 msgid "AddMember|No users specified."
 msgstr ""
 

spec/models/member_spec.rb

@@ -438,6 +438,16 @@ RSpec.describe Member do
     it { is_expected.to respond_to(:user_email) }
   end
 
+  describe '.valid_email?' do
+    it 'is a valid email format' do
+      expect(described_class.valid_email?('foo')).to eq(false)
+    end
+
+    it 'is not a valid email format' do
+      expect(described_class.valid_email?('foo@example.com')).to eq(true)
+    end
+  end
+
   describe '.add_user' do
     %w[project group].each do |source_type|
       context "when source is a #{source_type}" do

spec/requests/api/invitations_spec.rb

@@ -102,7 +102,8 @@ RSpec.describe API::Invitations do
                params: { email: stranger.email, access_level: Member::REPORTER }
 
           expect(response).to have_gitlab_http_status(:created)
-          expect(json_response['message'][stranger.email]).to eq("Access level should be greater than or equal to Developer inherited membership from group #{parent.name}")
+          expect(json_response['message'][stranger.email])
+            .to eq("Access level should be greater than or equal to Developer inherited membership from group #{parent.name}")
         end
 
         it 'creates the member if group level is lower' do
@@ -153,10 +154,10 @@ RSpec.describe API::Invitations do
 
       it "returns a message if member already exists" do
         post api("/#{source_type.pluralize}/#{source.id}/invitations", maintainer),
-             params: { email: maintainer.email, access_level: Member::MAINTAINER }
+             params: { email: developer.email, access_level: Member::MAINTAINER }
 
         expect(response).to have_gitlab_http_status(:created)
-        expect(json_response['message'][maintainer.email]).to eq("Already a member of #{source.name}")
+        expect(json_response['message'][developer.email]).to eq("User already exists in source")
       end
 
       it 'returns 404 when the email is not valid' do
@@ -164,7 +165,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('Email cannot be blank')
+        expect(json_response['message']).to eq('Emails cannot be blank')
       end
 
       it 'returns 404 when the email list is not a valid format' do

spec/requests/api/members_spec.rb

@@ -273,7 +273,7 @@ RSpec.describe API::Members do
             user_ids = [stranger.id, access_requester.id].join(',')
 
             allow_next_instance_of(::Members::CreateService) do |service|
-              expect(service).to receive(:execute).with(source).and_return({ status: :error, message: error_message })
+              expect(service).to receive(:execute).and_return({ status: :error, message: error_message })
             end
 
             expect do

spec/services/members/create_service_spec.rb

@@ -2,7 +2,7 @@
 
 require 'spec_helper'
 
-RSpec.describe Members::CreateService, :clean_gitlab_redis_shared_state, :sidekiq_inline do
+RSpec.describe Members::CreateService, :aggregate_failures, :clean_gitlab_redis_shared_state, :sidekiq_inline do
   let_it_be(:source) { create(:project) }
   let_it_be(:user) { create(:user) }
   let_it_be(:member) { create(:user) }
@@ -10,7 +10,7 @@ RSpec.describe Members::CreateService, :clean_gitlab_redis_shared_state, :sideki
   let_it_be(:access_level) { Gitlab::Access::GUEST }
   let(:params) { { user_ids: user_ids, access_level: access_level } }
 
-  subject(:execute_service) { described_class.new(user, params).execute(source) }
+  subject(:execute_service) { described_class.new(user, params.merge({ source: source })).execute }
 
   before do
     if source.is_a?(Project)

spec/services/members/invite_service_spec.rb

@@ -48,14 +48,24 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_
 
     it 'returns an error' do
       expect_not_to_create_members
-      expect(result[:message]).to eq('Email cannot be blank')
+      expect(result[:message]).to eq('Emails cannot be blank')
     end
   end
 
   context 'when email param is not included' do
     it 'returns an error' do
       expect_not_to_create_members
-      expect(result[:message]).to eq('Email cannot be blank')
+      expect(result[:message]).to eq('Emails cannot be blank')
+    end
+  end
+
+  context 'when email is not a valid email format' do
+    let(:params) { { email: '_bogus_' } }
+
+    it 'returns an error' do
+      expect { result }.not_to change(ProjectMember, :count)
+      expect(result[:status]).to eq(:error)
+      expect(result[:message][params[:email]]).to eq("Invite email is invalid")
     end
   end
 
@@ -114,7 +124,8 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_
 
     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
 
@@ -125,7 +136,8 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_
     it 'adds new email and returns an error for the already invited email' do
       expect_to_create_members(count: 1)
       expect(result[:status]).to eq(:error)
-      expect(result[:message][invited_member.invite_email]).to eq("Member already invited to #{project.name}")
+      expect(result[:message][invited_member.invite_email])
+        .to eq("Invite email has already been taken")
       expect(project.users).to include project_user
     end
   end
@@ -138,7 +150,7 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_
       expect_to_create_members(count: 1)
       expect(result[:status]).to eq(:error)
       expect(result[:message][requested_member.user.email])
-        .to eq("Member cannot be invited because they already requested to join #{project.name}")
+        .to eq("User already exists in source")
       expect(project.users).to include project_user
     end
   end
@@ -150,7 +162,8 @@ RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_
     it 'adds new email and returns an error for the already invited email' do
       expect_to_create_members(count: 1)
       expect(result[:status]).to eq(:error)
-      expect(result[:message][existing_member.user.email]).to eq("Already a member of #{project.name}")
+      expect(result[:message][existing_member.user.email])
+        .to eq("User already exists in source")
       expect(project.users).to include project_user
     end
   end