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

Add enqueueing of Onboarding Progress to the Invite Service

See merge request gitlab-org/gitlab!57372

app/models/members/group_member.rb

@@ -7,7 +7,7 @@ class GroupMember < Member
   SOURCE_TYPE = 'Namespace'
 
   belongs_to :group, foreign_key: 'source_id'
-
+  alias_attribute :namespace_id, :source_id
   delegate :update_two_factor_requirement, to: :user
 
   # Make sure group member points only to group as it source

app/models/members/project_member.rb

@@ -5,6 +5,8 @@ class ProjectMember < Member
 
   belongs_to :project, foreign_key: 'source_id'
 
+  delegate :namespace_id, to: :project
+
   # Make sure project member points only to project as it source
   default_value_for :source_type, SOURCE_TYPE
   validates :source_type, format: { with: /\AProject\z/ }

app/services/members/invite_service.rb

@@ -10,13 +10,14 @@ module Members
 
       @errors = {}
       @emails = params[:email]&.split(',')&.uniq&.flatten
+      @source = params[:source]
     end
 
-    def execute(source)
-      @source = source
+    def execute
       validate_emails!
 
       emails.each(&method(:process_email))
+      enqueue_onboarding_progress_action
       result
     rescue BlankEmailsError, TooManyEmailsError => e
       error(e.message)
@@ -24,7 +25,7 @@ module Members
 
     private
 
-    attr_reader :source, :errors, :emails
+    attr_reader :source, :errors, :emails, :member_created_namespace_id
 
     def validate_emails!
       raise BlankEmailsError, s_('AddMember|Email cannot be blank') if emails.blank?
@@ -88,6 +89,7 @@ module Members
         errors[email] = new_member.errors.full_messages.to_sentence
       else
         after_execute(member: new_member)
+        @member_created_namespace_id ||= new_member.namespace_id
       end
     end
 
@@ -98,6 +100,12 @@ module Members
         success
       end
     end
+
+    def enqueue_onboarding_progress_action
+      return unless member_created_namespace_id
+
+      Namespaces::OnboardingUserAddedWorker.perform_async(member_created_namespace_id)
+    end
   end
 end
 

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

+---
+title: Add enqueueing of Onboarding Progress to the Invite Service
+merge_request: 57372
+author:
+type: other

ee/spec/services/ee/members/invite_service_spec.rb

@@ -9,9 +9,10 @@ RSpec.describe Members::InviteService, :aggregate_failures do
   let_it_be(:subgroup) { create(:group, parent: root_ancestor) }
   let_it_be(:subgroup_project) { create(:project, group: subgroup) }
 
-  let(:params) { { email: %w[email@example.org email2@example.org], access_level: Gitlab::Access::GUEST } }
+  let(:base_params) { { access_level: Gitlab::Access::GUEST, source: project } }
+  let(:params) { { email: %w[email@example.org email2@example.org] } }
 
-  subject(:result) { described_class.new(user, params).execute(project) }
+  subject(:result) { described_class.new(user, base_params.merge(params)).execute }
 
   before_all do
     project.add_maintainer(user)
@@ -90,7 +91,7 @@ RSpec.describe Members::InviteService, :aggregate_failures do
       end
 
       context 'when there are some invalid members' do
-        let(:params) { { email: %w[_bogus_ email2@example.org], access_level: Gitlab::Access::GUEST } }
+        let(:params) { { email: %w[_bogus_ email2@example.org] } }
 
         it 'only creates Audit Events for valid members' do
           expect { result }.to change { AuditEvent.count }.by(1)

lib/api/invitations.rb

@@ -25,11 +25,11 @@ module API
           optional :expires_at, type: DateTime, desc: 'Date string in the format YEAR-MONTH-DAY'
         end
         post ":id/invitations" do
-          source = find_source(source_type, params[:id])
+          params[:source] = find_source(source_type, params[:id])
 
-          authorize_admin_source!(source_type, source)
+          authorize_admin_source!(source_type, params[:source])
 
-          ::Members::InviteService.new(current_user, params).execute(source)
+          ::Members::InviteService.new(current_user, params).execute
         end
 
         desc 'Get a list of group or project invitations viewable by the authenticated user' do

spec/models/members/group_member_spec.rb

@@ -66,6 +66,12 @@ RSpec.describe GroupMember do
 
   it_behaves_like 'members notifications', :group
 
+  describe '#namespace_id' do
+    subject { build(:group_member, source_id: 1).namespace_id }
+
+    it { is_expected.to eq 1 }
+  end
+
   describe '#real_source_type' do
     subject { create(:group_member).real_source_type }
 

spec/models/members/project_member_spec.rb

@@ -13,6 +13,10 @@ RSpec.describe ProjectMember do
     it { is_expected.to validate_inclusion_of(:access_level).in_array(Gitlab::Access.values) }
   end
 
+  describe 'delegations' do
+    it { is_expected.to delegate_method(:namespace_id).to(:project) }
+  end
+
   describe '.access_level_roles' do
     it 'returns Gitlab::Access.options' do
       expect(described_class.access_level_roles).to eq(Gitlab::Access.options)

spec/services/members/invite_service_spec.rb

@@ -2,29 +2,43 @@
 
 require 'spec_helper'
 
-RSpec.describe Members::InviteService, :aggregate_failures do
+RSpec.describe Members::InviteService, :aggregate_failures, :clean_gitlab_redis_shared_state, :sidekiq_inline do
   let_it_be(:project) { create(:project) }
   let_it_be(:user) { project.owner }
   let_it_be(:project_user) { create(:user) }
+  let_it_be(:namespace) { project.namespace }
   let(:params) { {} }
-  let(:base_params) { { access_level: Gitlab::Access::GUEST } }
+  let(:base_params) { { access_level: Gitlab::Access::GUEST, source: project } }
 
-  subject(:result) { described_class.new(user, base_params.merge(params)).execute(project) }
+  subject(:result) { described_class.new(user, base_params.merge(params) ).execute }
 
-  context 'when email is previously unused by current members' do
+  context 'when there is a valid member invited' do
     let(:params) { { email: 'email@example.org' } }
 
     it 'successfully creates a member' do
-      expect { result }.to change(ProjectMember, :count).by(1)
+      expect_to_create_members(count: 1)
       expect(result[:status]).to eq(:success)
     end
+
+    it_behaves_like 'records an onboarding progress action', :user_added
+  end
+
+  context 'when email is not a valid email' do
+    let(:params) { { email: '_bogus_' } }
+
+    it 'returns an error' do
+      expect_not_to_create_members
+      expect(result[:message]['_bogus_']).to eq("Invite email is invalid")
+    end
+
+    it_behaves_like 'does not record an onboarding progress action'
   end
 
   context 'when emails are passed as an array' do
     let(:params) { { email: %w[email@example.org email2@example.org] } }
 
     it 'successfully creates members' do
-      expect { result }.to change(ProjectMember, :count).by(2)
+      expect_to_create_members(count: 2)
       expect(result[:status]).to eq(:success)
     end
   end
@@ -33,33 +47,23 @@ RSpec.describe Members::InviteService, :aggregate_failures do
     let(:params) { { email: '' } }
 
     it 'returns an error' do
-      expect(result[:status]).to eq(:error)
+      expect_not_to_create_members
       expect(result[:message]).to eq('Email cannot be blank')
     end
   end
 
   context 'when email param is not included' do
     it 'returns an error' do
-      expect(result[:status]).to eq(:error)
+      expect_not_to_create_members
       expect(result[:message]).to eq('Email cannot be blank')
     end
   end
 
-  context 'when email is not a valid email' 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]['_bogus_']).to eq("Invite email is invalid")
-    end
-  end
-
   context 'when duplicate email addresses are passed' do
     let(:params) { { email: 'email@example.org,email@example.org' } }
 
     it 'only creates one member per unique address' do
-      expect { result }.to change(ProjectMember, :count).by(1)
+      expect_to_create_members(count: 1)
       expect(result[:status]).to eq(:success)
     end
   end
@@ -71,8 +75,7 @@ RSpec.describe Members::InviteService, :aggregate_failures do
       let(:params) { { email: emails } }
 
       it 'limits the number of emails to 100' do
-        expect { result }.not_to change(ProjectMember, :count)
-        expect(result[:status]).to eq(:error)
+        expect_not_to_create_members
         expect(result[:message]).to eq('Too many users specified (limit is 100)')
       end
     end
@@ -81,8 +84,7 @@ RSpec.describe Members::InviteService, :aggregate_failures do
       let(:params) { { email: 'email@example.org,email2@example.org', limit: 1 } }
 
       it 'limits the number of emails to the limit supplied' do
-        expect { result }.not_to change(ProjectMember, :count)
-        expect(result[:status]).to eq(:error)
+        expect_not_to_create_members
         expect(result[:message]).to eq('Too many users specified (limit is 1)')
       end
     end
@@ -91,7 +93,7 @@ RSpec.describe Members::InviteService, :aggregate_failures do
       let(:params) { { email: emails, limit: -1 } }
 
       it 'does not limit number of emails' do
-        expect { result }.to change(ProjectMember, :count).by(101)
+        expect_to_create_members(count: 101)
         expect(result[:status]).to eq(:success)
       end
     end
@@ -101,7 +103,7 @@ RSpec.describe Members::InviteService, :aggregate_failures do
     let(:params) { { email: project_user.email } }
 
     it 'adds an existing user to members' do
-      expect { result }.to change(ProjectMember, :count).by(1)
+      expect_to_create_members(count: 1)
       expect(result[:status]).to eq(:success)
       expect(project.users).to include project_user
     end
@@ -111,8 +113,7 @@ RSpec.describe Members::InviteService, :aggregate_failures do
     let(:params) { { email: project_user.email, access_level: -1 } }
 
     it 'returns an error' do
-      expect { result }.not_to change(ProjectMember, :count)
-      expect(result[:status]).to eq(:error)
+      expect_not_to_create_members
       expect(result[:message][project_user.email]).to eq("Access level is not included in the list")
     end
   end
@@ -122,7 +123,7 @@ RSpec.describe Members::InviteService, :aggregate_failures do
     let(:params) { { email: "#{invited_member.invite_email},#{project_user.email}" } }
 
     it 'adds new email and returns an error for the already invited email' do
-      expect { result }.to change(ProjectMember, :count).by(1)
+      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(project.users).to include project_user
@@ -134,7 +135,7 @@ RSpec.describe Members::InviteService, :aggregate_failures do
     let(:params) { { email: "#{requested_member.user.email},#{project_user.email}" } }
 
     it 'adds new email and returns an error for the already invited email' do
-      expect { result }.to change(ProjectMember, :count).by(1)
+      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}")
@@ -147,10 +148,19 @@ RSpec.describe Members::InviteService, :aggregate_failures do
     let(:params) { { email: "#{existing_member.user.email},#{project_user.email}" } }
 
     it 'adds new email and returns an error for the already invited email' do
-      expect { result }.to change(ProjectMember, :count).by(1)
+      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(project.users).to include project_user
     end
   end
+
+  def expect_to_create_members(count:)
+    expect { result }.to change(ProjectMember, :count).by(count)
+  end
+
+  def expect_not_to_create_members
+    expect { result }.not_to change(ProjectMember, :count)
+    expect(result[:status]).to eq(:error)
+  end
 end