From 64782efbf85467e173b08c4ee30986a6389f7408 Mon Sep 17 00:00:00 2001 From: Katherine Richards Date: Mon, 15 Sep 2025 16:54:42 -0600 Subject: [PATCH 1/4] Add seat assignment sync service Create GitlabSubscriptions::Members::SeatAssignments::SyncService to create, update, or remove seat assignment records for users with group or project memberships on GitLab.com --- .../gitlab_subscriptions/seat_assignment.rb | 4 + .../members/seat_assignment_service.rb | 68 ++++++ .../seat_assignment_spec.rb | 20 ++ .../members/seat_assignment_service_spec.rb | 211 ++++++++++++++++++ 4 files changed, 303 insertions(+) create mode 100644 ee/app/services/gitlab_subscriptions/members/seat_assignment_service.rb create mode 100644 ee/spec/services/gitlab_subscriptions/members/seat_assignment_service_spec.rb diff --git a/ee/app/models/gitlab_subscriptions/seat_assignment.rb b/ee/app/models/gitlab_subscriptions/seat_assignment.rb index f2779aa7ff5586..a90c3968e3a5ab 100644 --- a/ee/app/models/gitlab_subscriptions/seat_assignment.rb +++ b/ee/app/models/gitlab_subscriptions/seat_assignment.rb @@ -27,6 +27,10 @@ def self.find_by_namespace_and_user(namespace, user) by_namespace(namespace).by_user(user).first end + def self.find_by_namespace_and_users(namespace, users) + by_namespace(namespace).by_user(users) + end + private def gitlab_com_subscription? diff --git a/ee/app/services/gitlab_subscriptions/members/seat_assignment_service.rb b/ee/app/services/gitlab_subscriptions/members/seat_assignment_service.rb new file mode 100644 index 00000000000000..3cd1e84ce540b3 --- /dev/null +++ b/ee/app/services/gitlab_subscriptions/members/seat_assignment_service.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true + +module GitlabSubscriptions + module Members + class SeatAssignmentService + BATCH_SIZE = 100 + + def initialize(user_ids, namespace) + @user_ids = user_ids.compact.uniq + @root_namespace = namespace&.root_ancestor + end + + def execute + return unless ::Gitlab::Saas.feature_available?(:gitlab_com_subscriptions) + return ServiceResponse.error(message: 'Invalid params') unless root_namespace&.group_namespace? + + success_message = nil + + if user_ids.any? + user_ids.each_slice(BATCH_SIZE) do |batch_user_ids| + seat_assignable_user_ids = Member.seat_assignable(users: batch_user_ids, namespace: root_namespace) + .pluck_user_ids + .uniq + user_ids_for_seat_removal = batch_user_ids - seat_assignable_user_ids + + upsert_seat_assignments(seat_assignable_user_ids) if seat_assignable_user_ids.any? + delete_seat_assignments(user_ids_for_seat_removal) if user_ids_for_seat_removal.any? + end + + success_message = 'Seat assignments updated' + end + + ServiceResponse.success(message: success_message) + end + + private + + attr_reader :user_ids, :root_namespace + + def upsert_seat_assignments(seat_assignable_user_ids) + seat_types = seat_type_mappings(seat_assignable_user_ids) + seat_assignments = seat_assignable_user_ids.map { |user_id| seat_assignment(user_id, seat_types[user_id]) } + + GitlabSubscriptions::SeatAssignment.upsert_all(seat_assignments, unique_by: [:namespace_id, :user_id]) + end + + # This method handles both: + # - users who have an existing seat assignment to be deleted + # - users who do not have a seat assignment, for which no action is taken + def delete_seat_assignments(user_ids) + GitlabSubscriptions::SeatAssignment.find_by_namespace_and_users(root_namespace, user_ids).delete_all + end + + def seat_type_mappings(seat_assignable_user_ids) + GitlabSubscriptions::SeatTypeCalculator.bulk_execute(seat_assignable_user_ids, root_namespace) + end + + def seat_assignment(user_id, seat_type) + { + namespace_id: root_namespace.id, + user_id: user_id, + seat_type: seat_type, + organization_id: root_namespace.organization_id + } + end + end + end +end diff --git a/ee/spec/models/gitlab_subscriptions/seat_assignment_spec.rb b/ee/spec/models/gitlab_subscriptions/seat_assignment_spec.rb index 3eeb2d9d57a730..f4a673635a5910 100644 --- a/ee/spec/models/gitlab_subscriptions/seat_assignment_spec.rb +++ b/ee/spec/models/gitlab_subscriptions/seat_assignment_spec.rb @@ -52,6 +52,26 @@ end end + describe '.find_by_namespace_and_users' do + it 'returns records by namespace and users' do + user_2 = create(:user) + user_3 = create(:user) + users = [user, user_2, user_3] + + assignment_1 = create(:gitlab_subscription_seat_assignment, user: user, namespace: namespace) + assignment_2 = create(:gitlab_subscription_seat_assignment, user: user_2, namespace: namespace) + + expect(described_class.find_by_namespace_and_users(namespace, users)).to contain_exactly( + assignment_1, + assignment_2 + ) + end + + it 'returns empty array when no records exist' do + expect(described_class.find_by_namespace_and_users(namespace, [user])).to be_empty + end + end + describe '.dormant_in_namespace', :freeze_time do let_it_be(:dormant_seat_assignment_1) do create(:gitlab_subscription_seat_assignment, namespace: namespace, last_activity_on: 91.days.ago) diff --git a/ee/spec/services/gitlab_subscriptions/members/seat_assignment_service_spec.rb b/ee/spec/services/gitlab_subscriptions/members/seat_assignment_service_spec.rb new file mode 100644 index 00000000000000..8efbcec9687fc6 --- /dev/null +++ b/ee/spec/services/gitlab_subscriptions/members/seat_assignment_service_spec.rb @@ -0,0 +1,211 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe GitlabSubscriptions::Members::SeatAssignmentService, feature_category: :seat_cost_management do + describe '#execute' do + context 'when on saas', :saas do + let_it_be(:user) { create(:user) } + let_it_be(:root_namespace) { create(:group) } + let_it_be(:group) { create(:group, parent: root_namespace) } + let_it_be(:sub_group) { create(:group, parent: group) } + let_it_be(:project) { create(:project, group: group) } + let_it_be(:sub_project) { create(:project, group: sub_group) } + + context 'with valid params' do + shared_examples 'handles seat assignments' do + it 'returns success response' do + result = described_class.new([user.id], namespace).execute + + expect(result).to be_success + expect(result.message).to eq('Seat assignments updated') + end + + context 'when the user has a membership' do + before do + namespace.add_developer(user) + end + + context 'when the user has an existing seat assignment' do + let!(:existing_seat_assignment) do + create(:gitlab_subscription_seat_assignment, + user: user, + namespace: root_namespace, + organization_id: root_namespace.organization_id + ) + end + + it 'does not create a new seat assignment' do + expect { described_class.new([user.id], namespace).execute } + .not_to change { GitlabSubscriptions::SeatAssignment.count } + end + + it 'updates the existing seat assignment' do + existing_seat_assignment.update!(seat_type: nil) + + described_class.new([user.id], namespace).execute + + expect(existing_seat_assignment.reload).to have_attributes( + user: user, + seat_type: 'base', + namespace: root_namespace, + organization_id: root_namespace.organization_id + ) + end + end + + context 'when the user does not have a seat assignment' do + it 'creates a seat assignment' do + expect { described_class.new([user.id], namespace).execute } + .to change { GitlabSubscriptions::SeatAssignment.count }.by(1) + end + end + end + + context 'when the user has no memberships' do + context 'with an existing seat' do + before do + create(:gitlab_subscription_seat_assignment, + user: user, + namespace: root_namespace, + organization_id: root_namespace.organization_id + ) + end + + it 'destroys the seat' do + expect { described_class.new([user.id], namespace).execute } + .to change { GitlabSubscriptions::SeatAssignment.count }.by(-1) + end + + it 'returns success response' do + expect(described_class.new([user.id], namespace).execute).to be_success + end + end + + it 'does not create a seat' do + expect { described_class.new([user.id], namespace).execute } + .not_to change { GitlabSubscriptions::SeatAssignment.count } + end + end + end + + context 'with a group' do + let(:namespace) { group } + + it_behaves_like 'handles seat assignments' + end + + context 'with a sub group' do + let(:namespace) { sub_group } + + it_behaves_like 'handles seat assignments' + end + + context 'with a project' do + let(:namespace) { project } + + it_behaves_like 'handles seat assignments' + end + + context 'with a sub project' do + let(:namespace) { sub_project } + + it_behaves_like 'handles seat assignments' + end + + context 'with multiple users' do + let_it_be(:user_2) { create(:user) } + + it 'returns success response' do + result = described_class.new([user.id, user_2.id], group).execute + + expect(result).to be_success + expect(result.message).to eq('Seat assignments updated') + end + + it 'handles seat assigments' do + # group member with existing seat assignment + group.add_developer(user) + create(:gitlab_subscription_seat_assignment, user: user, seat_type: nil, namespace: root_namespace) + + # seat assignment pending deletion since user does not have a membership + create(:gitlab_subscription_seat_assignment, user: user_2, namespace: root_namespace) + + # sub project member without a seat assignment + user_3 = create(:user) + sub_project.add_maintainer(user_3) + + # user without memberships in namespace hierachy + user_4 = create(:user) + + described_class.new([user.id, user_2.id, user_3.id, user_4.id], group).execute + + expect(GitlabSubscriptions::SeatAssignment.all.pluck(:user_id)).to contain_exactly( + user.id, + user_3.id + ) + end + end + end + + context 'when the root namespace is not a group namespace' do + let(:namespace) { create(:user_namespace) } + + it 'returns an error' do + result = described_class.new([user.id], namespace).execute + + expect(result).to be_error + expect(result.message).to eq('Invalid params') + end + end + + context 'when user ids are not provided' do + it 'returns success response' do + result = described_class.new([], group).execute + + expect(result).to be_success + expect(result.message).to be_nil + end + end + + context 'when nil user_ids are provided' do + it 'returns success response' do + result = described_class.new([nil], group).execute + + expect(result).to be_success + expect(result.message).to be_nil + end + + context 'with valid user ids' do + it 'handles seat assignment for valid user ids' do + result = described_class.new([user.id, nil], group).execute + + expect(result).to be_success + expect(result.message).to eq('Seat assignments updated') + end + end + end + + context 'when duplicate user ids are provided' do + it 'handles seat assignment for the deduplicated user once' do + group.add_developer(user) + service = described_class.new([user.id, user.id], group) + + expect(service).to receive(:upsert_seat_assignments).with([user.id]).once + expect(service).not_to receive(:delete_seat_assignments) + + service.execute + end + end + end + + context 'when on self-managed' do + let(:user) { create(:user) } + let(:namespace) { create(:group) } + + it 'returns nil' do + expect(described_class.new([user.id], namespace).execute).to be_nil + end + end + end +end -- GitLab From 9743e1e5a4ad80772bae2b02e3fad335b74e9f6d Mon Sep 17 00:00:00 2001 From: Katherine Richards Date: Fri, 19 Sep 2025 18:43:43 -0600 Subject: [PATCH 2/4] Address MR feedback --- .../members/seat_assignment_service.rb | 68 --- .../members/seat_assignments/sync_service.rb | 154 +++++++ .../members/seat_assignment_service_spec.rb | 211 --------- .../seat_assignments/sync_service_spec.rb | 426 ++++++++++++++++++ 4 files changed, 580 insertions(+), 279 deletions(-) delete mode 100644 ee/app/services/gitlab_subscriptions/members/seat_assignment_service.rb create mode 100644 ee/app/services/gitlab_subscriptions/members/seat_assignments/sync_service.rb delete mode 100644 ee/spec/services/gitlab_subscriptions/members/seat_assignment_service_spec.rb create mode 100644 ee/spec/services/gitlab_subscriptions/members/seat_assignments/sync_service_spec.rb diff --git a/ee/app/services/gitlab_subscriptions/members/seat_assignment_service.rb b/ee/app/services/gitlab_subscriptions/members/seat_assignment_service.rb deleted file mode 100644 index 3cd1e84ce540b3..00000000000000 --- a/ee/app/services/gitlab_subscriptions/members/seat_assignment_service.rb +++ /dev/null @@ -1,68 +0,0 @@ -# frozen_string_literal: true - -module GitlabSubscriptions - module Members - class SeatAssignmentService - BATCH_SIZE = 100 - - def initialize(user_ids, namespace) - @user_ids = user_ids.compact.uniq - @root_namespace = namespace&.root_ancestor - end - - def execute - return unless ::Gitlab::Saas.feature_available?(:gitlab_com_subscriptions) - return ServiceResponse.error(message: 'Invalid params') unless root_namespace&.group_namespace? - - success_message = nil - - if user_ids.any? - user_ids.each_slice(BATCH_SIZE) do |batch_user_ids| - seat_assignable_user_ids = Member.seat_assignable(users: batch_user_ids, namespace: root_namespace) - .pluck_user_ids - .uniq - user_ids_for_seat_removal = batch_user_ids - seat_assignable_user_ids - - upsert_seat_assignments(seat_assignable_user_ids) if seat_assignable_user_ids.any? - delete_seat_assignments(user_ids_for_seat_removal) if user_ids_for_seat_removal.any? - end - - success_message = 'Seat assignments updated' - end - - ServiceResponse.success(message: success_message) - end - - private - - attr_reader :user_ids, :root_namespace - - def upsert_seat_assignments(seat_assignable_user_ids) - seat_types = seat_type_mappings(seat_assignable_user_ids) - seat_assignments = seat_assignable_user_ids.map { |user_id| seat_assignment(user_id, seat_types[user_id]) } - - GitlabSubscriptions::SeatAssignment.upsert_all(seat_assignments, unique_by: [:namespace_id, :user_id]) - end - - # This method handles both: - # - users who have an existing seat assignment to be deleted - # - users who do not have a seat assignment, for which no action is taken - def delete_seat_assignments(user_ids) - GitlabSubscriptions::SeatAssignment.find_by_namespace_and_users(root_namespace, user_ids).delete_all - end - - def seat_type_mappings(seat_assignable_user_ids) - GitlabSubscriptions::SeatTypeCalculator.bulk_execute(seat_assignable_user_ids, root_namespace) - end - - def seat_assignment(user_id, seat_type) - { - namespace_id: root_namespace.id, - user_id: user_id, - seat_type: seat_type, - organization_id: root_namespace.organization_id - } - end - end - end -end diff --git a/ee/app/services/gitlab_subscriptions/members/seat_assignments/sync_service.rb b/ee/app/services/gitlab_subscriptions/members/seat_assignments/sync_service.rb new file mode 100644 index 00000000000000..311f81e79f3108 --- /dev/null +++ b/ee/app/services/gitlab_subscriptions/members/seat_assignments/sync_service.rb @@ -0,0 +1,154 @@ +# frozen_string_literal: true + +module GitlabSubscriptions + module Members + module SeatAssignments + class SyncService + def initialize(user_ids, namespace) + @user_ids = user_ids.compact.uniq + @root_namespace = namespace&.root_ancestor + @created_count = 0 + @updated_count = 0 + @destroyed_count = 0 + @errors = [] + end + + def execute + return unless ::Gitlab::Saas.feature_available?(:gitlab_com_subscriptions) + return ServiceResponse.error(message: 'Invalid params') unless root_namespace&.group_namespace? + return unless user_ids.any? + + existing_seat_data = existing_seat_assignment_data + seat_changes = calculate_seat_changes(existing_seat_data) + seat_types = seat_types_to_upsert(seat_changes) + + sync_seat_assignments(seat_changes, seat_types, existing_seat_data[:records]) + + log_seat_assignment_sync + end + + private + + attr_reader :user_ids, :root_namespace + attr_accessor :created_count, :updated_count, :destroyed_count, :errors + + def sync_seat_assignments(seat_changes, seat_types, existing_records) + create_seat_assignments(seat_changes[:seats_to_create], seat_types) + update_seat_assignments(seat_changes[:seats_to_update], seat_types, existing_records) + destroy_seat_assignments(seat_changes[:seats_to_remove], existing_records) + end + + def create_seat_assignments(seat_assignable_user_ids, seat_types) + return unless seat_assignable_user_ids.any? + + seat_assignable_user_ids.each do |user_id| + seat_assignment_attrs = seat_assignment(user_id, seat_types[user_id]) + seat = GitlabSubscriptions::SeatAssignment.new(seat_assignment_attrs) + + if seat.save + @created_count += 1 + else + @errors << { user_id: user_id, error: seat.errors.full_messages } + end + end + end + + def update_seat_assignments(seat_assignable_user_ids, seat_types, existing_seat_assignment_records) + return unless seat_assignable_user_ids.any? + + seat_assignable_user_ids.each do |user_id| + seat_assignment_attrs = seat_assignment(user_id, seat_types[user_id]) + seat = existing_seat_assignment_records[user_id] + + if seat.update(seat_assignment_attrs) + @updated_count += 1 + else + @errors << { user_id: user_id, error: seat.errors.full_messages } + end + end + end + + def destroy_seat_assignments(user_ids, existing_seat_assignment_records) + return unless user_ids.any? + + user_ids.each do |user_id| + seat = existing_seat_assignment_records[user_id] + + if seat.destroy + @destroyed_count += 1 + else + @errors << { user_id: user_id, error: seat.errors.full_messages } + end + end + end + + def existing_seat_assignment_data + records = existing_seat_assignments + { + records: records.index_by(&:user_id), + user_ids: records.index_by(&:user_id).keys + } + end + + def calculate_seat_changes(existing_seat_data) + seat_assignable_user_ids = seat_assignable_members + + determine_seat_assignment_changes( + existing_seat_data[:user_ids], + seat_assignable_user_ids + ) + end + + def seat_types_to_upsert(seat_changes) + users_needing_seat_types = seat_changes[:seats_to_create] + seat_changes[:seats_to_update] + seat_type_mappings(users_needing_seat_types) + end + + def seat_type_mappings(seat_assignable_user_ids) + GitlabSubscriptions::SeatTypeCalculator.bulk_execute(seat_assignable_user_ids, root_namespace) + end + + def seat_assignment(user_id, seat_type) + { + namespace_id: root_namespace.id, + user_id: user_id, + seat_type: seat_type, + organization_id: root_namespace.organization_id + } + end + + def determine_seat_assignment_changes(existing_seat_user_ids, seat_assignable_user_ids) + { + seats_to_create: seat_assignable_user_ids - existing_seat_user_ids, + seats_to_update: existing_seat_user_ids & seat_assignable_user_ids, + seats_to_remove: existing_seat_user_ids - seat_assignable_user_ids + } + end + + def existing_seat_assignments + GitlabSubscriptions::SeatAssignment.find_by_namespace_and_users( + root_namespace, user_ids + ) + end + + def seat_assignable_members + Member.seat_assignable(users: user_ids, namespace: root_namespace) + .pluck_user_ids + .uniq + end + + def log_seat_assignment_sync + Gitlab::AppJsonLogger.info( + class: self.class.name, + namespace_id: root_namespace.id, + message: 'Successfully synced seat assignment records', + seats_created: @created_count, + seats_updated: @updated_count, + seats_destroyed: @destroyed_count, + errors: @errors + ) + end + end + end + end +end diff --git a/ee/spec/services/gitlab_subscriptions/members/seat_assignment_service_spec.rb b/ee/spec/services/gitlab_subscriptions/members/seat_assignment_service_spec.rb deleted file mode 100644 index 8efbcec9687fc6..00000000000000 --- a/ee/spec/services/gitlab_subscriptions/members/seat_assignment_service_spec.rb +++ /dev/null @@ -1,211 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe GitlabSubscriptions::Members::SeatAssignmentService, feature_category: :seat_cost_management do - describe '#execute' do - context 'when on saas', :saas do - let_it_be(:user) { create(:user) } - let_it_be(:root_namespace) { create(:group) } - let_it_be(:group) { create(:group, parent: root_namespace) } - let_it_be(:sub_group) { create(:group, parent: group) } - let_it_be(:project) { create(:project, group: group) } - let_it_be(:sub_project) { create(:project, group: sub_group) } - - context 'with valid params' do - shared_examples 'handles seat assignments' do - it 'returns success response' do - result = described_class.new([user.id], namespace).execute - - expect(result).to be_success - expect(result.message).to eq('Seat assignments updated') - end - - context 'when the user has a membership' do - before do - namespace.add_developer(user) - end - - context 'when the user has an existing seat assignment' do - let!(:existing_seat_assignment) do - create(:gitlab_subscription_seat_assignment, - user: user, - namespace: root_namespace, - organization_id: root_namespace.organization_id - ) - end - - it 'does not create a new seat assignment' do - expect { described_class.new([user.id], namespace).execute } - .not_to change { GitlabSubscriptions::SeatAssignment.count } - end - - it 'updates the existing seat assignment' do - existing_seat_assignment.update!(seat_type: nil) - - described_class.new([user.id], namespace).execute - - expect(existing_seat_assignment.reload).to have_attributes( - user: user, - seat_type: 'base', - namespace: root_namespace, - organization_id: root_namespace.organization_id - ) - end - end - - context 'when the user does not have a seat assignment' do - it 'creates a seat assignment' do - expect { described_class.new([user.id], namespace).execute } - .to change { GitlabSubscriptions::SeatAssignment.count }.by(1) - end - end - end - - context 'when the user has no memberships' do - context 'with an existing seat' do - before do - create(:gitlab_subscription_seat_assignment, - user: user, - namespace: root_namespace, - organization_id: root_namespace.organization_id - ) - end - - it 'destroys the seat' do - expect { described_class.new([user.id], namespace).execute } - .to change { GitlabSubscriptions::SeatAssignment.count }.by(-1) - end - - it 'returns success response' do - expect(described_class.new([user.id], namespace).execute).to be_success - end - end - - it 'does not create a seat' do - expect { described_class.new([user.id], namespace).execute } - .not_to change { GitlabSubscriptions::SeatAssignment.count } - end - end - end - - context 'with a group' do - let(:namespace) { group } - - it_behaves_like 'handles seat assignments' - end - - context 'with a sub group' do - let(:namespace) { sub_group } - - it_behaves_like 'handles seat assignments' - end - - context 'with a project' do - let(:namespace) { project } - - it_behaves_like 'handles seat assignments' - end - - context 'with a sub project' do - let(:namespace) { sub_project } - - it_behaves_like 'handles seat assignments' - end - - context 'with multiple users' do - let_it_be(:user_2) { create(:user) } - - it 'returns success response' do - result = described_class.new([user.id, user_2.id], group).execute - - expect(result).to be_success - expect(result.message).to eq('Seat assignments updated') - end - - it 'handles seat assigments' do - # group member with existing seat assignment - group.add_developer(user) - create(:gitlab_subscription_seat_assignment, user: user, seat_type: nil, namespace: root_namespace) - - # seat assignment pending deletion since user does not have a membership - create(:gitlab_subscription_seat_assignment, user: user_2, namespace: root_namespace) - - # sub project member without a seat assignment - user_3 = create(:user) - sub_project.add_maintainer(user_3) - - # user without memberships in namespace hierachy - user_4 = create(:user) - - described_class.new([user.id, user_2.id, user_3.id, user_4.id], group).execute - - expect(GitlabSubscriptions::SeatAssignment.all.pluck(:user_id)).to contain_exactly( - user.id, - user_3.id - ) - end - end - end - - context 'when the root namespace is not a group namespace' do - let(:namespace) { create(:user_namespace) } - - it 'returns an error' do - result = described_class.new([user.id], namespace).execute - - expect(result).to be_error - expect(result.message).to eq('Invalid params') - end - end - - context 'when user ids are not provided' do - it 'returns success response' do - result = described_class.new([], group).execute - - expect(result).to be_success - expect(result.message).to be_nil - end - end - - context 'when nil user_ids are provided' do - it 'returns success response' do - result = described_class.new([nil], group).execute - - expect(result).to be_success - expect(result.message).to be_nil - end - - context 'with valid user ids' do - it 'handles seat assignment for valid user ids' do - result = described_class.new([user.id, nil], group).execute - - expect(result).to be_success - expect(result.message).to eq('Seat assignments updated') - end - end - end - - context 'when duplicate user ids are provided' do - it 'handles seat assignment for the deduplicated user once' do - group.add_developer(user) - service = described_class.new([user.id, user.id], group) - - expect(service).to receive(:upsert_seat_assignments).with([user.id]).once - expect(service).not_to receive(:delete_seat_assignments) - - service.execute - end - end - end - - context 'when on self-managed' do - let(:user) { create(:user) } - let(:namespace) { create(:group) } - - it 'returns nil' do - expect(described_class.new([user.id], namespace).execute).to be_nil - end - end - end -end diff --git a/ee/spec/services/gitlab_subscriptions/members/seat_assignments/sync_service_spec.rb b/ee/spec/services/gitlab_subscriptions/members/seat_assignments/sync_service_spec.rb new file mode 100644 index 00000000000000..bd197ad92f2070 --- /dev/null +++ b/ee/spec/services/gitlab_subscriptions/members/seat_assignments/sync_service_spec.rb @@ -0,0 +1,426 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe GitlabSubscriptions::Members::SeatAssignments::SyncService, feature_category: :seat_cost_management do + describe '#execute' do + context 'when on saas', :saas do + let_it_be(:user) { create(:user) } + let_it_be(:root_namespace) { create(:group) } + let_it_be(:group) { create(:group, parent: root_namespace) } + let_it_be(:sub_group) { create(:group, parent: group) } + let_it_be(:project) { create(:project, group: group) } + let_it_be(:sub_project) { create(:project, group: sub_group) } + + context 'with valid params' do + shared_examples 'handles seat assignments' do + context 'when the user has a membership' do + before do + namespace.add_developer(user) + end + + context 'when the user has an existing seat assignment' do + let!(:existing_seat_assignment) do + create(:gitlab_subscription_seat_assignment, + user: user, + seat_type: nil, + namespace: root_namespace, + organization_id: root_namespace.organization_id + ) + end + + it 'does not create a new seat assignment' do + expect { described_class.new([user.id], namespace).execute } + .not_to change { GitlabSubscriptions::SeatAssignment.count } + end + + it 'updates the existing seat assignment' do + described_class.new([user.id], namespace).execute + + expect(existing_seat_assignment.reload).to have_attributes( + user: user, + seat_type: 'base', + namespace: root_namespace, + organization_id: root_namespace.organization_id + ) + end + + it 'logs the result' do + expect(Gitlab::AppJsonLogger).to receive(:info).with( + hash_including( + namespace_id: root_namespace.id, + message: 'Successfully synced seat assignment records', + errors: [], + seats_created: 0, + seats_updated: 1, + seats_destroyed: 0 + ) + ) + + described_class.new([user.id], namespace).execute + end + end + + context 'when the user does not have a seat assignment' do + it 'creates a seat assignment' do + expect { described_class.new([user.id], namespace).execute } + .to change { GitlabSubscriptions::SeatAssignment.count }.by(1) + end + + it 'logs the result' do + expect(Gitlab::AppJsonLogger).to receive(:info).with( + hash_including( + namespace_id: root_namespace.id, + message: 'Successfully synced seat assignment records', + errors: [], + seats_created: 1, + seats_updated: 0, + seats_destroyed: 0 + ) + ) + + described_class.new([user.id], namespace).execute + end + end + end + + context 'when the user has no memberships' do + context 'with an existing seat' do + before do + create(:gitlab_subscription_seat_assignment, + user: user, + namespace: root_namespace, + organization_id: root_namespace.organization_id + ) + end + + it 'destroys the seat' do + expect { described_class.new([user.id], namespace).execute } + .to change { GitlabSubscriptions::SeatAssignment.count }.by(-1) + end + + it 'logs the result' do + expect(Gitlab::AppJsonLogger).to receive(:info).with( + hash_including( + namespace_id: root_namespace.id, + message: 'Successfully synced seat assignment records', + errors: [], + seats_created: 0, + seats_updated: 0, + seats_destroyed: 1 + ) + ) + + described_class.new([user.id], namespace).execute + end + end + + it 'does not create a seat' do + expect { described_class.new([user.id], namespace).execute } + .not_to change { GitlabSubscriptions::SeatAssignment.count } + end + + it 'logs the result' do + expect(Gitlab::AppJsonLogger).to receive(:info).with( + hash_including( + namespace_id: root_namespace.id, + message: 'Successfully synced seat assignment records', + errors: [], + seats_created: 0, + seats_updated: 0, + seats_destroyed: 0 + ) + ) + + described_class.new([user.id], namespace).execute + end + end + end + + context 'with a group' do + let(:namespace) { group } + + it_behaves_like 'handles seat assignments' + end + + context 'with a sub group' do + let(:namespace) { sub_group } + + it_behaves_like 'handles seat assignments' + end + + context 'with a project' do + let(:namespace) { project } + + it_behaves_like 'handles seat assignments' + end + + context 'with a sub project' do + let(:namespace) { sub_project } + + it_behaves_like 'handles seat assignments' + end + + context 'with multiple users' do + let_it_be(:user_2) { create(:user) } + + it 'handles seat assigments' do + # group member with existing seat assignment + group.add_developer(user) + create(:gitlab_subscription_seat_assignment, user: user, seat_type: nil, namespace: root_namespace) + + # seat assignment pending deletion since user does not have a membership + create(:gitlab_subscription_seat_assignment, user: user_2, namespace: root_namespace) + + # sub project member without a seat assignment + user_3 = create(:user) + sub_project.add_maintainer(user_3) + + # user without memberships in namespace hierachy + user_4 = create(:user) + + expect(Gitlab::AppJsonLogger).to receive(:info).with( + a_hash_including( + namespace_id: root_namespace.id, + message: 'Successfully synced seat assignment records', + errors: [], + seats_created: 1, + seats_updated: 1, + seats_destroyed: 1 + ) + ) + + described_class.new([user.id, user_2.id, user_3.id, user_4.id], group).execute + + expect(GitlabSubscriptions::SeatAssignment.all.pluck(:user_id)).to contain_exactly( + user.id, + user_3.id + ) + end + + context 'when a seat fails to be synced' do + it 'completes the rest of the actions and logs failures' do + group.add_developer(user) + seat_assignment_1 = create(:gitlab_subscription_seat_assignment, + user: user, # error encountered while processing this user + seat_type: 'base', + namespace: root_namespace, + organization_id: root_namespace.organization_id + ) + + user_2 = create(:user) # seat is destroyed for this user + seat_assignment_2 = create(:gitlab_subscription_seat_assignment, user: user_2, namespace: root_namespace) + + user_3 = create(:user) # seat is created for this user + group.add_developer(user_3) + + allow(GitlabSubscriptions::SeatAssignment).to receive(:find_by_namespace_and_users) + .and_return([seat_assignment_1, seat_assignment_2]) + + allow(seat_assignment_1).to receive(:update).and_return(false) + allow(seat_assignment_1).to receive_message_chain(:errors, :full_messages) + .and_return(['Some error message']) + + expect(Gitlab::AppJsonLogger).to receive(:info).with( + a_hash_including( + namespace_id: root_namespace.id, + message: 'Successfully synced seat assignment records', + errors: [{ + user_id: user.id, + error: ['Some error message'] + }], + seats_created: 1, + seats_updated: 0, + seats_destroyed: 1 + ) + ) + + described_class.new([user.id, user_2.id, user_3.id], group).execute + end + end + end + + context 'when it fails to create a seat assignment' do + it 'logs the error' do + group.add_developer(user) + + allow_next_instance_of(GitlabSubscriptions::SeatAssignment) do |seat| + allow(seat).to receive(:save).and_return(false) + allow(seat).to receive_message_chain(:errors, :full_messages).and_return(['Some error message']) + end + + expect(Gitlab::AppJsonLogger).to receive(:info).with( + a_hash_including( + namespace_id: root_namespace.id, + message: 'Successfully synced seat assignment records', + errors: [{ + user_id: user.id, + error: ['Some error message'] + }], + seats_created: 0, + seats_updated: 0, + seats_destroyed: 0 + ) + ) + + described_class.new([user.id], group).execute + end + end + + context 'when it fails to update a seat assignment' do + let!(:existing_seat_assignment) do + create(:gitlab_subscription_seat_assignment, + user: user, + seat_type: 'base', + namespace: root_namespace, + organization_id: root_namespace.organization_id + ) + end + + it 'logs the error' do + group.add_developer(user) + + allow(GitlabSubscriptions::SeatAssignment).to receive(:find_by_namespace_and_users) + .and_return([existing_seat_assignment]) + + allow(existing_seat_assignment).to receive(:update).and_return(false) + allow(existing_seat_assignment).to receive_message_chain(:errors, :full_messages) + .and_return(['Some error message']) + + expect(Gitlab::AppJsonLogger).to receive(:info).with( + a_hash_including( + namespace_id: root_namespace.id, + message: 'Successfully synced seat assignment records', + errors: [{ + user_id: user.id, + error: ['Some error message'] + }], + seats_created: 0, + seats_updated: 0, + seats_destroyed: 0 + ) + ) + + described_class.new([user.id], group).execute + end + end + + context 'when it fails to destroy a seat assignment' do + let!(:existing_seat_assignment) do + create(:gitlab_subscription_seat_assignment, + user: user, + seat_type: 'base', + namespace: root_namespace, + organization_id: root_namespace.organization_id + ) + end + + it 'logs the error' do + allow(GitlabSubscriptions::SeatAssignment).to receive(:find_by_namespace_and_users) + .and_return([existing_seat_assignment]) + + allow(existing_seat_assignment).to receive(:destroy).and_return(false) + allow(existing_seat_assignment).to receive_message_chain(:errors, :full_messages) + .and_return(['Some error message']) + + expect(Gitlab::AppJsonLogger).to receive(:info).with( + a_hash_including( + namespace_id: root_namespace.id, + message: 'Successfully synced seat assignment records', + errors: [{ + user_id: user.id, + error: ['Some error message'] + }], + seats_created: 0, + seats_updated: 0, + seats_destroyed: 0 + ) + ) + + described_class.new([user.id], group).execute + end + end + end + + context 'when the root namespace is not a group namespace' do + let(:namespace) { create(:user_namespace) } + + it 'returns an error' do + result = described_class.new([user.id], namespace).execute + + expect(result).to be_error + expect(result.message).to eq('Invalid params') + end + end + + context 'with empty user_ids provided' do + it 'returns returns early' do + service = described_class.new([], group) + + expect(service).not_to receive(:sync_seat_assignments) + + service.execute + end + end + + context 'when user_ids contains nil ids' do + it 'returns early' do + service = described_class.new([nil], group) + + expect(service).not_to receive(:sync_seat_assignments) + + service.execute + end + + context 'with valid user ids' do + before_all do + group.add_developer(user) + end + + it 'handles seat assignment for valid user ids' do + expect(Gitlab::AppJsonLogger).to receive(:info).with( + a_hash_including( + namespace_id: root_namespace.id, + message: 'Successfully synced seat assignment records', + errors: [], + seats_created: 1, + seats_updated: 0, + seats_destroyed: 0 + ) + ) + + described_class.new([user.id, nil], group).execute + end + end + end + + context 'when duplicate user ids are provided' do + it 'handles seat assignment for the deduplicated user once' do + group.add_developer(user) + + expect(Gitlab::AppJsonLogger).to receive(:info).with( + a_hash_including( + namespace_id: root_namespace.id, + message: 'Successfully synced seat assignment records', + errors: [], + seats_created: 1, + seats_updated: 0, + seats_destroyed: 0 + ) + ) + + described_class.new([user.id, user.id], group).execute + end + end + end + + context 'when on self-managed' do + let(:user) { create(:user) } + let(:namespace) { create(:group) } + + it 'returns nil' do + expect(described_class.new([user.id], namespace).execute).to be_nil + end + end + end +end -- GitLab From 9cca6b3c8d5602ad22d8e22c6845740ce4999633 Mon Sep 17 00:00:00 2001 From: Katherine Richards Date: Mon, 22 Sep 2025 18:20:34 -0600 Subject: [PATCH 3/4] Address MR feedback --- .../members/seat_assignments/sync_service.rb | 118 ++++++++---------- .../seat_assignments/sync_service_spec.rb | 107 +++++++++------- 2 files changed, 113 insertions(+), 112 deletions(-) diff --git a/ee/app/services/gitlab_subscriptions/members/seat_assignments/sync_service.rb b/ee/app/services/gitlab_subscriptions/members/seat_assignments/sync_service.rb index 311f81e79f3108..fadd7f8d44c504 100644 --- a/ee/app/services/gitlab_subscriptions/members/seat_assignments/sync_service.rb +++ b/ee/app/services/gitlab_subscriptions/members/seat_assignments/sync_service.rb @@ -4,109 +4,85 @@ module GitlabSubscriptions module Members module SeatAssignments class SyncService + include Gitlab::Utils::StrongMemoize + def initialize(user_ids, namespace) @user_ids = user_ids.compact.uniq @root_namespace = namespace&.root_ancestor - @created_count = 0 - @updated_count = 0 - @destroyed_count = 0 + @seats_created = [] + @seats_updated = [] + @seats_destroyed = [] @errors = [] end def execute return unless ::Gitlab::Saas.feature_available?(:gitlab_com_subscriptions) - return ServiceResponse.error(message: 'Invalid params') unless root_namespace&.group_namespace? - return unless user_ids.any? - - existing_seat_data = existing_seat_assignment_data - seat_changes = calculate_seat_changes(existing_seat_data) - seat_types = seat_types_to_upsert(seat_changes) + return user_namespace_error if root_namespace&.user_namespace? + return ServiceResponse.success(message: 'Nothing to process for empty user list') unless user_ids.any? - sync_seat_assignments(seat_changes, seat_types, existing_seat_data[:records]) + sync_seat_assignments log_seat_assignment_sync + ServiceResponse.success end private attr_reader :user_ids, :root_namespace - attr_accessor :created_count, :updated_count, :destroyed_count, :errors + attr_accessor :seats_created, :seats_updated, :seats_destroyed, :errors - def sync_seat_assignments(seat_changes, seat_types, existing_records) - create_seat_assignments(seat_changes[:seats_to_create], seat_types) - update_seat_assignments(seat_changes[:seats_to_update], seat_types, existing_records) - destroy_seat_assignments(seat_changes[:seats_to_remove], existing_records) + def sync_seat_assignments + create_seat_assignments + update_seat_assignments + destroy_seat_assignments end - def create_seat_assignments(seat_assignable_user_ids, seat_types) - return unless seat_assignable_user_ids.any? - - seat_assignable_user_ids.each do |user_id| + def create_seat_assignments + seat_changes[:seats_to_create].each do |user_id| seat_assignment_attrs = seat_assignment(user_id, seat_types[user_id]) seat = GitlabSubscriptions::SeatAssignment.new(seat_assignment_attrs) if seat.save - @created_count += 1 + @seats_created << user_id else @errors << { user_id: user_id, error: seat.errors.full_messages } end end end - def update_seat_assignments(seat_assignable_user_ids, seat_types, existing_seat_assignment_records) - return unless seat_assignable_user_ids.any? - - seat_assignable_user_ids.each do |user_id| + def update_seat_assignments + seat_changes[:seats_to_update].each do |user_id| seat_assignment_attrs = seat_assignment(user_id, seat_types[user_id]) - seat = existing_seat_assignment_records[user_id] + seat = existing_seats_by_user_id[user_id] if seat.update(seat_assignment_attrs) - @updated_count += 1 + @seats_updated << user_id else @errors << { user_id: user_id, error: seat.errors.full_messages } end end end - def destroy_seat_assignments(user_ids, existing_seat_assignment_records) - return unless user_ids.any? - - user_ids.each do |user_id| - seat = existing_seat_assignment_records[user_id] + def destroy_seat_assignments + seat_changes[:seats_to_remove].each do |user_id| + seat = existing_seats_by_user_id[user_id] if seat.destroy - @destroyed_count += 1 + @seats_destroyed << user_id else @errors << { user_id: user_id, error: seat.errors.full_messages } end end end - def existing_seat_assignment_data - records = existing_seat_assignments - { - records: records.index_by(&:user_id), - user_ids: records.index_by(&:user_id).keys - } - end - - def calculate_seat_changes(existing_seat_data) - seat_assignable_user_ids = seat_assignable_members + def seat_types + seat_assignable_user_ids = seat_changes[:seats_to_create] + seat_changes[:seats_to_update] - determine_seat_assignment_changes( - existing_seat_data[:user_ids], - seat_assignable_user_ids + GitlabSubscriptions::SeatTypeCalculator.bulk_execute( + seat_assignable_user_ids, root_namespace ) end - - def seat_types_to_upsert(seat_changes) - users_needing_seat_types = seat_changes[:seats_to_create] + seat_changes[:seats_to_update] - seat_type_mappings(users_needing_seat_types) - end - - def seat_type_mappings(seat_assignable_user_ids) - GitlabSubscriptions::SeatTypeCalculator.bulk_execute(seat_assignable_user_ids, root_namespace) - end + strong_memoize_attr :seat_types def seat_assignment(user_id, seat_type) { @@ -117,34 +93,44 @@ def seat_assignment(user_id, seat_type) } end - def determine_seat_assignment_changes(existing_seat_user_ids, seat_assignable_user_ids) + def seat_changes + seat_assignable_user_ids = seat_assignable_members + user_ids_with_seats = existing_seats_by_user_id.keys + { - seats_to_create: seat_assignable_user_ids - existing_seat_user_ids, - seats_to_update: existing_seat_user_ids & seat_assignable_user_ids, - seats_to_remove: existing_seat_user_ids - seat_assignable_user_ids + seats_to_create: seat_assignable_user_ids - user_ids_with_seats, + seats_to_update: user_ids_with_seats & seat_assignable_user_ids, + seats_to_remove: user_ids_with_seats - seat_assignable_user_ids } end - def existing_seat_assignments - GitlabSubscriptions::SeatAssignment.find_by_namespace_and_users( - root_namespace, user_ids - ) + def existing_seats_by_user_id + GitlabSubscriptions::SeatAssignment.find_by_namespace_and_users(root_namespace, user_ids) + .index_by(&:user_id) end + strong_memoize_attr :existing_seats_by_user_id def seat_assignable_members Member.seat_assignable(users: user_ids, namespace: root_namespace) .pluck_user_ids .uniq end + strong_memoize_attr :seat_assignable_members + + def user_namespace_error + ServiceResponse.error( + message: 'Seat assignments unavailable for user namespaces on GitLab.com' + ) + end def log_seat_assignment_sync Gitlab::AppJsonLogger.info( class: self.class.name, namespace_id: root_namespace.id, - message: 'Successfully synced seat assignment records', - seats_created: @created_count, - seats_updated: @updated_count, - seats_destroyed: @destroyed_count, + message: 'Synced seat assignment records', + seats_created_user_ids: @seats_created, + seats_updated_user_ids: @seats_updated, + seats_destroyed_user_ids: @seats_destroyed, errors: @errors ) end diff --git a/ee/spec/services/gitlab_subscriptions/members/seat_assignments/sync_service_spec.rb b/ee/spec/services/gitlab_subscriptions/members/seat_assignments/sync_service_spec.rb index bd197ad92f2070..1bc076e01d3e79 100644 --- a/ee/spec/services/gitlab_subscriptions/members/seat_assignments/sync_service_spec.rb +++ b/ee/spec/services/gitlab_subscriptions/members/seat_assignments/sync_service_spec.rb @@ -49,11 +49,11 @@ expect(Gitlab::AppJsonLogger).to receive(:info).with( hash_including( namespace_id: root_namespace.id, - message: 'Successfully synced seat assignment records', + message: 'Synced seat assignment records', errors: [], - seats_created: 0, - seats_updated: 1, - seats_destroyed: 0 + seats_created_user_ids: [], + seats_updated_user_ids: [user.id], + seats_destroyed_user_ids: [] ) ) @@ -71,11 +71,11 @@ expect(Gitlab::AppJsonLogger).to receive(:info).with( hash_including( namespace_id: root_namespace.id, - message: 'Successfully synced seat assignment records', + message: 'Synced seat assignment records', errors: [], - seats_created: 1, - seats_updated: 0, - seats_destroyed: 0 + seats_created_user_ids: [user.id], + seats_updated_user_ids: [], + seats_destroyed_user_ids: [] ) ) @@ -103,11 +103,11 @@ expect(Gitlab::AppJsonLogger).to receive(:info).with( hash_including( namespace_id: root_namespace.id, - message: 'Successfully synced seat assignment records', + message: 'Synced seat assignment records', errors: [], - seats_created: 0, - seats_updated: 0, - seats_destroyed: 1 + seats_created_user_ids: [], + seats_updated_user_ids: [], + seats_destroyed_user_ids: [user.id] ) ) @@ -124,17 +124,23 @@ expect(Gitlab::AppJsonLogger).to receive(:info).with( hash_including( namespace_id: root_namespace.id, - message: 'Successfully synced seat assignment records', + message: 'Synced seat assignment records', errors: [], - seats_created: 0, - seats_updated: 0, - seats_destroyed: 0 + seats_created_user_ids: [], + seats_updated_user_ids: [], + seats_destroyed_user_ids: [] ) ) described_class.new([user.id], namespace).execute end end + + it 'returns success response' do + result = described_class.new([user.id], namespace).execute + + expect(result).to be_success + end end context 'with a group' do @@ -182,11 +188,11 @@ expect(Gitlab::AppJsonLogger).to receive(:info).with( a_hash_including( namespace_id: root_namespace.id, - message: 'Successfully synced seat assignment records', + message: 'Synced seat assignment records', errors: [], - seats_created: 1, - seats_updated: 1, - seats_destroyed: 1 + seats_created_user_ids: [user_3.id], + seats_updated_user_ids: [user.id], + seats_destroyed_user_ids: [user_2.id] ) ) @@ -224,14 +230,14 @@ expect(Gitlab::AppJsonLogger).to receive(:info).with( a_hash_including( namespace_id: root_namespace.id, - message: 'Successfully synced seat assignment records', + message: 'Synced seat assignment records', errors: [{ user_id: user.id, error: ['Some error message'] }], - seats_created: 1, - seats_updated: 0, - seats_destroyed: 1 + seats_created_user_ids: [user_3.id], + seats_updated_user_ids: [], + seats_destroyed_user_ids: [user_2.id] ) ) @@ -252,14 +258,14 @@ expect(Gitlab::AppJsonLogger).to receive(:info).with( a_hash_including( namespace_id: root_namespace.id, - message: 'Successfully synced seat assignment records', + message: 'Synced seat assignment records', errors: [{ user_id: user.id, error: ['Some error message'] }], - seats_created: 0, - seats_updated: 0, - seats_destroyed: 0 + seats_created_user_ids: [], + seats_updated_user_ids: [], + seats_destroyed_user_ids: [] ) ) @@ -290,14 +296,14 @@ expect(Gitlab::AppJsonLogger).to receive(:info).with( a_hash_including( namespace_id: root_namespace.id, - message: 'Successfully synced seat assignment records', + message: 'Synced seat assignment records', errors: [{ user_id: user.id, error: ['Some error message'] }], - seats_created: 0, - seats_updated: 0, - seats_destroyed: 0 + seats_created_user_ids: [], + seats_updated_user_ids: [], + seats_destroyed_user_ids: [] ) ) @@ -326,14 +332,14 @@ expect(Gitlab::AppJsonLogger).to receive(:info).with( a_hash_including( namespace_id: root_namespace.id, - message: 'Successfully synced seat assignment records', + message: 'Synced seat assignment records', errors: [{ user_id: user.id, error: ['Some error message'] }], - seats_created: 0, - seats_updated: 0, - seats_destroyed: 0 + seats_created_user_ids: [], + seats_updated_user_ids: [], + seats_destroyed_user_ids: [] ) ) @@ -349,18 +355,27 @@ result = described_class.new([user.id], namespace).execute expect(result).to be_error - expect(result.message).to eq('Invalid params') + expect(result.message).to eq( + 'Seat assignments unavailable for user namespaces on GitLab.com' + ) end end context 'with empty user_ids provided' do - it 'returns returns early' do + it 'returns early' do service = described_class.new([], group) expect(service).not_to receive(:sync_seat_assignments) service.execute end + + it 'returns success response' do + result = described_class.new([], group).execute + + expect(result).to be_success + expect(result.message).to eq('Nothing to process for empty user list') + end end context 'when user_ids contains nil ids' do @@ -381,11 +396,11 @@ expect(Gitlab::AppJsonLogger).to receive(:info).with( a_hash_including( namespace_id: root_namespace.id, - message: 'Successfully synced seat assignment records', + message: 'Synced seat assignment records', errors: [], - seats_created: 1, - seats_updated: 0, - seats_destroyed: 0 + seats_created_user_ids: [user.id], + seats_updated_user_ids: [], + seats_destroyed_user_ids: [] ) ) @@ -401,11 +416,11 @@ expect(Gitlab::AppJsonLogger).to receive(:info).with( a_hash_including( namespace_id: root_namespace.id, - message: 'Successfully synced seat assignment records', + message: 'Synced seat assignment records', errors: [], - seats_created: 1, - seats_updated: 0, - seats_destroyed: 0 + seats_created_user_ids: [user.id], + seats_updated_user_ids: [], + seats_destroyed_user_ids: [] ) ) -- GitLab From dfc8a28c37b554e710d46d15e6b5a6e46eef3925 Mon Sep 17 00:00:00 2001 From: Katherine Richards Date: Wed, 24 Sep 2025 11:36:04 -0600 Subject: [PATCH 4/4] Address MR feedback --- .../gitlab_subscriptions/seat_assignment.rb | 2 +- .../members/seat_assignments/sync_service.rb | 8 +- .../seat_assignment_spec.rb | 6 +- .../seat_assignments/sync_service_spec.rb | 145 +++++++----------- 4 files changed, 66 insertions(+), 95 deletions(-) diff --git a/ee/app/models/gitlab_subscriptions/seat_assignment.rb b/ee/app/models/gitlab_subscriptions/seat_assignment.rb index a90c3968e3a5ab..fc8ca45e4435c0 100644 --- a/ee/app/models/gitlab_subscriptions/seat_assignment.rb +++ b/ee/app/models/gitlab_subscriptions/seat_assignment.rb @@ -27,7 +27,7 @@ def self.find_by_namespace_and_user(namespace, user) by_namespace(namespace).by_user(user).first end - def self.find_by_namespace_and_users(namespace, users) + def self.by_namespace_and_users(namespace, users) by_namespace(namespace).by_user(users) end diff --git a/ee/app/services/gitlab_subscriptions/members/seat_assignments/sync_service.rb b/ee/app/services/gitlab_subscriptions/members/seat_assignments/sync_service.rb index fadd7f8d44c504..c7ba68641a5c3f 100644 --- a/ee/app/services/gitlab_subscriptions/members/seat_assignments/sync_service.rb +++ b/ee/app/services/gitlab_subscriptions/members/seat_assignments/sync_service.rb @@ -105,7 +105,7 @@ def seat_changes end def existing_seats_by_user_id - GitlabSubscriptions::SeatAssignment.find_by_namespace_and_users(root_namespace, user_ids) + GitlabSubscriptions::SeatAssignment.by_namespace_and_users(root_namespace, user_ids) .index_by(&:user_id) end strong_memoize_attr :existing_seats_by_user_id @@ -128,9 +128,9 @@ def log_seat_assignment_sync class: self.class.name, namespace_id: root_namespace.id, message: 'Synced seat assignment records', - seats_created_user_ids: @seats_created, - seats_updated_user_ids: @seats_updated, - seats_destroyed_user_ids: @seats_destroyed, + created_seats_user_ids: @seats_created, + updated_seats_user_ids: @seats_updated, + destroyed_seats_user_ids: @seats_destroyed, errors: @errors ) end diff --git a/ee/spec/models/gitlab_subscriptions/seat_assignment_spec.rb b/ee/spec/models/gitlab_subscriptions/seat_assignment_spec.rb index f4a673635a5910..554535dcb74391 100644 --- a/ee/spec/models/gitlab_subscriptions/seat_assignment_spec.rb +++ b/ee/spec/models/gitlab_subscriptions/seat_assignment_spec.rb @@ -52,7 +52,7 @@ end end - describe '.find_by_namespace_and_users' do + describe '.by_namespace_and_users' do it 'returns records by namespace and users' do user_2 = create(:user) user_3 = create(:user) @@ -61,14 +61,14 @@ assignment_1 = create(:gitlab_subscription_seat_assignment, user: user, namespace: namespace) assignment_2 = create(:gitlab_subscription_seat_assignment, user: user_2, namespace: namespace) - expect(described_class.find_by_namespace_and_users(namespace, users)).to contain_exactly( + expect(described_class.by_namespace_and_users(namespace, users)).to contain_exactly( assignment_1, assignment_2 ) end it 'returns empty array when no records exist' do - expect(described_class.find_by_namespace_and_users(namespace, [user])).to be_empty + expect(described_class.by_namespace_and_users(namespace, [user])).to be_empty end end diff --git a/ee/spec/services/gitlab_subscriptions/members/seat_assignments/sync_service_spec.rb b/ee/spec/services/gitlab_subscriptions/members/seat_assignments/sync_service_spec.rb index 1bc076e01d3e79..e6c5f5e2e2eeca 100644 --- a/ee/spec/services/gitlab_subscriptions/members/seat_assignments/sync_service_spec.rb +++ b/ee/spec/services/gitlab_subscriptions/members/seat_assignments/sync_service_spec.rb @@ -4,9 +4,10 @@ RSpec.describe GitlabSubscriptions::Members::SeatAssignments::SyncService, feature_category: :seat_cost_management do describe '#execute' do + let_it_be(:user) { create(:user) } + let_it_be(:root_namespace) { create(:group) } + context 'when on saas', :saas do - let_it_be(:user) { create(:user) } - let_it_be(:root_namespace) { create(:group) } let_it_be(:group) { create(:group, parent: root_namespace) } let_it_be(:sub_group) { create(:group, parent: group) } let_it_be(:project) { create(:project, group: group) } @@ -51,9 +52,9 @@ namespace_id: root_namespace.id, message: 'Synced seat assignment records', errors: [], - seats_created_user_ids: [], - seats_updated_user_ids: [user.id], - seats_destroyed_user_ids: [] + created_seats_user_ids: [], + updated_seats_user_ids: [user.id], + destroyed_seats_user_ids: [] ) ) @@ -73,9 +74,9 @@ namespace_id: root_namespace.id, message: 'Synced seat assignment records', errors: [], - seats_created_user_ids: [user.id], - seats_updated_user_ids: [], - seats_destroyed_user_ids: [] + created_seats_user_ids: [user.id], + updated_seats_user_ids: [], + destroyed_seats_user_ids: [] ) ) @@ -105,9 +106,9 @@ namespace_id: root_namespace.id, message: 'Synced seat assignment records', errors: [], - seats_created_user_ids: [], - seats_updated_user_ids: [], - seats_destroyed_user_ids: [user.id] + created_seats_user_ids: [], + updated_seats_user_ids: [], + destroyed_seats_user_ids: [user.id] ) ) @@ -126,9 +127,9 @@ namespace_id: root_namespace.id, message: 'Synced seat assignment records', errors: [], - seats_created_user_ids: [], - seats_updated_user_ids: [], - seats_destroyed_user_ids: [] + created_seats_user_ids: [], + updated_seats_user_ids: [], + destroyed_seats_user_ids: [] ) ) @@ -168,21 +169,16 @@ end context 'with multiple users' do - let_it_be(:user_2) { create(:user) } - it 'handles seat assigments' do - # group member with existing seat assignment group.add_developer(user) create(:gitlab_subscription_seat_assignment, user: user, seat_type: nil, namespace: root_namespace) - # seat assignment pending deletion since user does not have a membership + user_2 = create(:user) create(:gitlab_subscription_seat_assignment, user: user_2, namespace: root_namespace) - # sub project member without a seat assignment user_3 = create(:user) sub_project.add_maintainer(user_3) - # user without memberships in namespace hierachy user_4 = create(:user) expect(Gitlab::AppJsonLogger).to receive(:info).with( @@ -190,9 +186,9 @@ namespace_id: root_namespace.id, message: 'Synced seat assignment records', errors: [], - seats_created_user_ids: [user_3.id], - seats_updated_user_ids: [user.id], - seats_destroyed_user_ids: [user_2.id] + created_seats_user_ids: [user_3.id], + updated_seats_user_ids: [user.id], + destroyed_seats_user_ids: [user_2.id] ) ) @@ -207,25 +203,21 @@ context 'when a seat fails to be synced' do it 'completes the rest of the actions and logs failures' do group.add_developer(user) - seat_assignment_1 = create(:gitlab_subscription_seat_assignment, - user: user, # error encountered while processing this user + create(:gitlab_subscription_seat_assignment, + user: user, seat_type: 'base', namespace: root_namespace, organization_id: root_namespace.organization_id ) - user_2 = create(:user) # seat is destroyed for this user + user_2 = create(:user) seat_assignment_2 = create(:gitlab_subscription_seat_assignment, user: user_2, namespace: root_namespace) - user_3 = create(:user) # seat is created for this user + user_3 = create(:user) group.add_developer(user_3) - allow(GitlabSubscriptions::SeatAssignment).to receive(:find_by_namespace_and_users) - .and_return([seat_assignment_1, seat_assignment_2]) - - allow(seat_assignment_1).to receive(:update).and_return(false) - allow(seat_assignment_1).to receive_message_chain(:errors, :full_messages) - .and_return(['Some error message']) + allow(GitlabSubscriptions::SeatAssignment).to receive(:by_namespace_and_users) + .and_return([seat_assignment_2]) expect(Gitlab::AppJsonLogger).to receive(:info).with( a_hash_including( @@ -233,11 +225,11 @@ message: 'Synced seat assignment records', errors: [{ user_id: user.id, - error: ['Some error message'] + error: ['Namespace has already been taken'] }], - seats_created_user_ids: [user_3.id], - seats_updated_user_ids: [], - seats_destroyed_user_ids: [user_2.id] + created_seats_user_ids: [user_3.id], + updated_seats_user_ids: [], + destroyed_seats_user_ids: [user_2.id] ) ) @@ -263,9 +255,9 @@ user_id: user.id, error: ['Some error message'] }], - seats_created_user_ids: [], - seats_updated_user_ids: [], - seats_destroyed_user_ids: [] + created_seats_user_ids: [], + updated_seats_user_ids: [], + destroyed_seats_user_ids: [] ) ) @@ -286,7 +278,7 @@ it 'logs the error' do group.add_developer(user) - allow(GitlabSubscriptions::SeatAssignment).to receive(:find_by_namespace_and_users) + allow(GitlabSubscriptions::SeatAssignment).to receive(:by_namespace_and_users) .and_return([existing_seat_assignment]) allow(existing_seat_assignment).to receive(:update).and_return(false) @@ -301,9 +293,9 @@ user_id: user.id, error: ['Some error message'] }], - seats_created_user_ids: [], - seats_updated_user_ids: [], - seats_destroyed_user_ids: [] + created_seats_user_ids: [], + updated_seats_user_ids: [], + destroyed_seats_user_ids: [] ) ) @@ -322,7 +314,7 @@ end it 'logs the error' do - allow(GitlabSubscriptions::SeatAssignment).to receive(:find_by_namespace_and_users) + allow(GitlabSubscriptions::SeatAssignment).to receive(:by_namespace_and_users) .and_return([existing_seat_assignment]) allow(existing_seat_assignment).to receive(:destroy).and_return(false) @@ -337,9 +329,9 @@ user_id: user.id, error: ['Some error message'] }], - seats_created_user_ids: [], - seats_updated_user_ids: [], - seats_destroyed_user_ids: [] + created_seats_user_ids: [], + updated_seats_user_ids: [], + destroyed_seats_user_ids: [] ) ) @@ -362,14 +354,6 @@ end context 'with empty user_ids provided' do - it 'returns early' do - service = described_class.new([], group) - - expect(service).not_to receive(:sync_seat_assignments) - - service.execute - end - it 'returns success response' do result = described_class.new([], group).execute @@ -378,34 +362,24 @@ end end - context 'when user_ids contains nil ids' do - it 'returns early' do - service = described_class.new([nil], group) - - expect(service).not_to receive(:sync_seat_assignments) - - service.execute + context 'when user_ids contains mixed elements' do + before_all do + group.add_developer(user) end - context 'with valid user ids' do - before_all do - group.add_developer(user) - end - - it 'handles seat assignment for valid user ids' do - expect(Gitlab::AppJsonLogger).to receive(:info).with( - a_hash_including( - namespace_id: root_namespace.id, - message: 'Synced seat assignment records', - errors: [], - seats_created_user_ids: [user.id], - seats_updated_user_ids: [], - seats_destroyed_user_ids: [] - ) + it 'handles seat assignment for valid user ids only' do + expect(Gitlab::AppJsonLogger).to receive(:info).with( + a_hash_including( + namespace_id: root_namespace.id, + message: 'Synced seat assignment records', + errors: [], + created_seats_user_ids: [user.id], + updated_seats_user_ids: [], + destroyed_seats_user_ids: [] ) + ) - described_class.new([user.id, nil], group).execute - end + described_class.new([user.id, nil], group).execute end end @@ -418,9 +392,9 @@ namespace_id: root_namespace.id, message: 'Synced seat assignment records', errors: [], - seats_created_user_ids: [user.id], - seats_updated_user_ids: [], - seats_destroyed_user_ids: [] + created_seats_user_ids: [user.id], + updated_seats_user_ids: [], + destroyed_seats_user_ids: [] ) ) @@ -430,11 +404,8 @@ end context 'when on self-managed' do - let(:user) { create(:user) } - let(:namespace) { create(:group) } - it 'returns nil' do - expect(described_class.new([user.id], namespace).execute).to be_nil + expect(described_class.new([user.id], root_namespace).execute).to be_nil end end end -- GitLab