From 9af64da8e5b5a5d256b134cb53b21358db393967 Mon Sep 17 00:00:00 2001 From: Jorge Cook Date: Tue, 10 Jun 2025 13:32:35 -0400 Subject: [PATCH 01/16] Add seat removal worker --- .../member_transfers/remove_seats_worker.rb | 87 +++++++++++++++++++ 1 file changed, 87 insertions(+) create mode 100644 ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_seats_worker.rb diff --git a/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_seats_worker.rb b/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_seats_worker.rb new file mode 100644 index 00000000000000..34542f80afe808 --- /dev/null +++ b/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_seats_worker.rb @@ -0,0 +1,87 @@ +# frozen_string_literal: true + +module GitlabSubscriptions + module SeatAssignments + module MemberTransfers + class RemoveSeatsWorker + include ApplicationWorker + + feature_category :seat_cost_management + data_consistency :delayed + urgency :low + + defer_on_database_health_signal :gitlab_main, + [:subscription_seat_assignments, :members], 10.minutes + + idempotent! + + BATCH_SIZE = 1000 + + def perform(old_root_namespace_id, namespace) + return unless namespace + + remove_outdated_seat_assignments(old_root_namespace_id, namespace) + end + + private + + def remove_outdated_seat_assignments(old_root_namespace_id, namespace) + old_root_namespace = Namespace.find_by_id(old_root_namespace_id) + + user_ids_in_transferred_namespace_tree = collect_user_ids(namespace) + + if top_group_to_subgroup?(old_root_namespace_id, namespace.id, namespace.root_ancestor.id) + delete_seats(old_root_namespace, user_ids_in_transferred_namespace_tree) + return + end + + member_ids_in_old_root_namespace = collect_user_ids( + old_root_namespace, + filter_user_ids: user_ids_in_transferred_namespace_tree + ) + + outdated_seat_user_ids = user_ids_in_transferred_namespace_tree.reject do |id| + member_ids_in_old_root_namespace.include?(id) + end + + delete_seats(old_root_namespace, outdated_seat_user_ids) + end + + def delete_seats(namespace, user_ids) + return if user_ids.empty? + + # rubocop:disable CodeReuse/ActiveRecord -- We need this to delete the outdated seat assignments + user_ids.each_slice(BATCH_SIZE) do |batch| + ::GitlabSubscriptions::SeatAssignment.where( + namespace_id: namespace.id, + user_id: batch + ).delete_all + end + # rubocop:enable CodeReuse/ActiveRecord + end + + def top_group_to_subgroup?(old_root_namespace_id, namespace_id, new_root_namespace_id) + old_root_namespace_id == namespace_id && new_root_namespace_id != old_root_namespace_id + end + + def collect_user_ids(namespace, filter_user_ids: nil) + return [] if namespace.nil? + + scope = + if namespace.is_a?(Group) + namespace.hierarchy_members + elsif namespace.is_a?(Project) + namespace.project_members + else + ::Member.in_hierarchy(namespace) + end + + # rubocop:disable CodeReuse/ActiveRecord -- We need this to obtain the user ids of the members + scope = scope.where(user_id: filter_user_ids) if filter_user_ids + scope.pluck(:user_id) + # rubocop:enable CodeReuse/ActiveRecord + end + end + end + end +end -- GitLab From 614056110fd34c6690b00e54f6390a987eb95014 Mon Sep 17 00:00:00 2001 From: Jorge Cook Date: Tue, 10 Jun 2025 13:35:14 -0400 Subject: [PATCH 02/16] Add seat removal worker tests --- .../remove_seats_worker_spec.rb | 158 ++++++++++++++++++ 1 file changed, 158 insertions(+) create mode 100644 ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_seats_worker_spec.rb diff --git a/ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_seats_worker_spec.rb b/ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_seats_worker_spec.rb new file mode 100644 index 00000000000000..362efd4a109284 --- /dev/null +++ b/ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_seats_worker_spec.rb @@ -0,0 +1,158 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe GitlabSubscriptions::SeatAssignments::MemberTransfers::RemoveSeatsWorker, :saas, feature_category: :seat_cost_management do + let(:worker) { described_class.new } + let_it_be(:user) { create(:user, :with_namespace) } + let_it_be(:user_2) { create(:user) } + + describe '#perform' do + context 'with group namespaces' do + let_it_be_with_refind(:root_group) { create(:group_with_plan, plan: :ultimate_plan) } + let_it_be_with_refind(:transferred_group) { create(:group_with_plan, plan: :ultimate_plan, parent: root_group) } + + before do + create(:gitlab_subscription_seat_assignment, :active, user: user, namespace: root_group) + create(:gitlab_subscription_seat_assignment, :active, user: user_2, namespace: root_group) + create(:group_member, group: transferred_group, user: user) + create(:group_member, group: transferred_group, user: user_2) + end + + context 'when a root group namespace has outdated seat assignments after a subgroup transfer' do + let_it_be(:remaining_member) { create(:group_member, group: root_group, user: user) } + let_it_be(:new_parent_group) { create(:group_with_plan, plan: :ultimate_plan) } + + before do + transferred_group.update!(parent: new_parent_group) + end + + it 'removes outdated seat assignments' do + expect do + worker.perform(root_group.id, transferred_group) + end.to change { GitlabSubscriptions::SeatAssignment.by_namespace(root_group).count }.by(-1) + end + + it 'leaves seat assignments of group members' do + worker.perform(root_group.id, transferred_group) + + expect( + GitlabSubscriptions::SeatAssignment.by_namespace(root_group).pluck(:user_id) + ).to match([remaining_member.user_id]) + end + end + + context 'when a root namespace is transferred and becomes a subgroup of another group' do + let_it_be(:new_parent_group) { create(:group_with_plan, plan: :ultimate_plan) } + + before do + create(:group_member, group: root_group, user: user) + create(:group_member, group: root_group, user: user_2) + root_group.update!(parent: new_parent_group) + end + + it 'removes all seat assignments from old root namespace' do + expect do + worker.perform(root_group.id, root_group) + end.to change { GitlabSubscriptions::SeatAssignment.by_namespace(root_group).count }.by(-2) + end + end + + context 'when a subgroup is transferred within the same hierarchy' do + let_it_be(:parent_subgroup) { create(:group_with_plan, plan: :ultimate_plan, parent: root_group) } + let_it_be(:subgroup) { create(:group_with_plan, plan: :ultimate_plan, parent: root_group) } + + before do + create(:group_member, group: subgroup, user: user) + create(:group_member, group: subgroup, user: user_2) + subgroup.update!(parent: parent_subgroup) + end + + it 'does not remove seats' do + expect do + worker.perform(root_group.id, subgroup) + end.not_to change { GitlabSubscriptions::SeatAssignment.by_namespace(root_group).count } + end + end + + context 'when there are no outdated seat assignments' do + let!(:member) { create(:group_member, group: root_group, user: user) } + let!(:member_2) { create(:group_member, group: root_group, user: user_2) } + + it 'does not remove seat assignments' do + expect do + worker.perform(root_group.id, root_group) + end.not_to change { GitlabSubscriptions::SeatAssignment.by_namespace(root_group).count } + end + + it 'has members corresponding to seat assignments for the group' do + worker.perform(root_group.id, root_group) + + expect( + GitlabSubscriptions::SeatAssignment.by_namespace(root_group).pluck(:user_id) + ).to match(::Member.in_hierarchy(root_group).pluck(:user_id).uniq) + end + end + end + + context 'with projects in user namespaces' do + let_it_be_with_refind(:project) { create(:project, namespace: user.namespace) } + let_it_be_with_refind(:root_group) { create(:group_with_plan, plan: :ultimate_plan) } + + context 'when the user namespace has outdated seats after project has been transferred to a group' do + before do + create(:gitlab_subscription_seat_assignment, :active, user: user, namespace: user.namespace) + create(:gitlab_subscription_seat_assignment, :active, user: user_2, namespace: user.namespace) + create(:project_member, project: project, user: user_2) + project.update!(namespace: root_group) + end + + context 'when the user namespace still has members via another project' do + let_it_be(:project_2) { create(:project, namespace: user.namespace) } + + it 'removes outdated seat assignments and leaves the seat of the remaining member' do + expect do + worker.perform(user.namespace.id, project) + end.to change { GitlabSubscriptions::SeatAssignment.by_namespace(user.namespace).count }.by(-1) + end + + it 'leaves the seat of the remaining member' do + worker.perform(user.namespace.id, project) + + expect( + GitlabSubscriptions::SeatAssignment.by_namespace(user.namespace).pluck(:user_id) + ).to match(project_2.project_members.pluck(:user_id)) + end + + context 'when there are no outdated seat assignments' do + before do + create(:project_member, project: project_2, user: user_2) + end + + it 'does not remove seat assignments' do + expect do + worker.perform(user.namespace.id, project) + end.not_to change { GitlabSubscriptions::SeatAssignment.by_namespace(user.namespace).count } + end + + it 'ensures project members match the seat assignments for the project namespace' do + worker.perform(user.namespace.id, project) + + expect( + GitlabSubscriptions::SeatAssignment.by_namespace(project_2.namespace).pluck(:user_id) + ).to match(::Member.in_hierarchy(user.namespace).pluck(:user_id).uniq) + end + end + end + + context 'when the user namespace has no remaining members after project transfer' do + it 'removes outdated seat assignments and leaves the seat of the remaining member' do + worker.perform(user.namespace.id, project) + + expect(GitlabSubscriptions::SeatAssignment.by_namespace(user.namespace)).to be_empty + end + end + end + end + end +end -- GitLab From def40e73628def237eb68a1b1edcd9ebffa0d1ed Mon Sep 17 00:00:00 2001 From: Jorge Cook Date: Tue, 10 Jun 2025 13:36:18 -0400 Subject: [PATCH 03/16] Add sidekiq and all queues yml files --- config/sidekiq_queues.yml | 2 ++ ee/app/workers/all_queues.yml | 10 ++++++++++ 2 files changed, 12 insertions(+) diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index a9ed99b46c1bbf..908830d4a7208b 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -491,6 +491,8 @@ - 1 - - gitlab_subscriptions_seat_assignments_member_transfers_create_project_seats - 1 +- - gitlab_subscriptions_seat_assignments_member_transfers_remove_seats + - 1 - - gitlab_subscriptions_self_managed_duo_core_todo_notification - 1 - - gitlab_subscriptions_trials_apply_trial diff --git a/ee/app/workers/all_queues.yml b/ee/app/workers/all_queues.yml index 71eee7a2922b2b..f04f53b3f0b020 100644 --- a/ee/app/workers/all_queues.yml +++ b/ee/app/workers/all_queues.yml @@ -2483,6 +2483,16 @@ :idempotent: true :tags: [] :queue_namespace: +- :name: gitlab_subscriptions_seat_assignments_member_transfers_remove_seats + :worker_name: GitlabSubscriptions::SeatAssignments::MemberTransfers::RemoveSeatsWorker + :feature_category: :seat_cost_management + :has_external_dependencies: false + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] + :queue_namespace: - :name: gitlab_subscriptions_self_managed_duo_core_todo_notification :worker_name: GitlabSubscriptions::SelfManaged::DuoCoreTodoNotificationWorker :feature_category: :acquisition -- GitLab From f9cbf0ddf2852333de19e6c554cc43d4bf8f3556 Mon Sep 17 00:00:00 2001 From: Jorge Cook Date: Tue, 10 Jun 2025 17:46:16 -0400 Subject: [PATCH 04/16] Fix failing test --- .../remove_seats_worker_spec.rb | 46 ++++++++++--------- 1 file changed, 25 insertions(+), 21 deletions(-) diff --git a/ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_seats_worker_spec.rb b/ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_seats_worker_spec.rb index 362efd4a109284..8db0d4f756dc60 100644 --- a/ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_seats_worker_spec.rb +++ b/ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_seats_worker_spec.rb @@ -2,9 +2,9 @@ require 'spec_helper' -RSpec.describe GitlabSubscriptions::SeatAssignments::MemberTransfers::RemoveSeatsWorker, :saas, feature_category: :seat_cost_management do +RSpec.describe ::GitlabSubscriptions::SeatAssignments::MemberTransfers::RemoveSeatsWorker, :saas, feature_category: :seat_cost_management do let(:worker) { described_class.new } - let_it_be(:user) { create(:user, :with_namespace) } + let_it_be_with_refind(:user) { create(:user, :with_namespace) } let_it_be(:user_2) { create(:user) } describe '#perform' do @@ -96,60 +96,64 @@ end context 'with projects in user namespaces' do - let_it_be_with_refind(:project) { create(:project, namespace: user.namespace) } + let_it_be_with_refind(:user_namespace) { user.namespace } + let_it_be_with_refind(:project) { create(:project, namespace: user_namespace) } let_it_be_with_refind(:root_group) { create(:group_with_plan, plan: :ultimate_plan) } context 'when the user namespace has outdated seats after project has been transferred to a group' do before do - create(:gitlab_subscription_seat_assignment, :active, user: user, namespace: user.namespace) - create(:gitlab_subscription_seat_assignment, :active, user: user_2, namespace: user.namespace) - create(:project_member, project: project, user: user_2) - project.update!(namespace: root_group) + create(:gitlab_subscription_seat_assignment, :active, user: user, namespace: user_namespace) + create(:gitlab_subscription_seat_assignment, :active, user: user_2, namespace: user_namespace) end context 'when the user namespace still has members via another project' do - let_it_be(:project_2) { create(:project, namespace: user.namespace) } + let_it_be_with_refind(:project_2) { create(:project, namespace: user_namespace) } + + before do + create(:project_member, project: project_2, user: user_2) + project_2.update!(namespace: root_group) + end it 'removes outdated seat assignments and leaves the seat of the remaining member' do expect do - worker.perform(user.namespace.id, project) - end.to change { GitlabSubscriptions::SeatAssignment.by_namespace(user.namespace).count }.by(-1) + worker.perform(project.namespace.id, project_2) + end.to change { GitlabSubscriptions::SeatAssignment.by_namespace(project.namespace).count }.by(-1) end it 'leaves the seat of the remaining member' do - worker.perform(user.namespace.id, project) + worker.perform(user_namespace, project_2) expect( - GitlabSubscriptions::SeatAssignment.by_namespace(user.namespace).pluck(:user_id) - ).to match(project_2.project_members.pluck(:user_id)) + GitlabSubscriptions::SeatAssignment.by_namespace(user_namespace).pluck(:user_id) + ).to match(project.project_members.pluck(:user_id)) end context 'when there are no outdated seat assignments' do before do - create(:project_member, project: project_2, user: user_2) + create(:project_member, project: project, user: user_2) end it 'does not remove seat assignments' do expect do - worker.perform(user.namespace.id, project) - end.not_to change { GitlabSubscriptions::SeatAssignment.by_namespace(user.namespace).count } + worker.perform(user_namespace.id, project_2) + end.not_to change { GitlabSubscriptions::SeatAssignment.by_namespace(user_namespace).count } end it 'ensures project members match the seat assignments for the project namespace' do - worker.perform(user.namespace.id, project) + worker.perform(user_namespace.id, project_2) expect( - GitlabSubscriptions::SeatAssignment.by_namespace(project_2.namespace).pluck(:user_id) - ).to match(::Member.in_hierarchy(user.namespace).pluck(:user_id).uniq) + GitlabSubscriptions::SeatAssignment.by_namespace(project.namespace).pluck(:user_id) + ).to match(::Member.in_hierarchy(user_namespace).pluck(:user_id).uniq) end end end context 'when the user namespace has no remaining members after project transfer' do it 'removes outdated seat assignments and leaves the seat of the remaining member' do - worker.perform(user.namespace.id, project) + worker.perform(user_namespace.id, project_2) - expect(GitlabSubscriptions::SeatAssignment.by_namespace(user.namespace)).to be_empty + expect(GitlabSubscriptions::SeatAssignment.by_namespace(user_namespace)).to be_empty end end end -- GitLab From f7a8e144196e9bebb825b1c6b2c69ffbebaa7358 Mon Sep 17 00:00:00 2001 From: Jorge Cook Date: Wed, 11 Jun 2025 21:33:16 -0400 Subject: [PATCH 05/16] Rearrange seat removal tests --- .../remove_seats_worker_spec.rb | 55 +++++++++++-------- 1 file changed, 33 insertions(+), 22 deletions(-) diff --git a/ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_seats_worker_spec.rb b/ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_seats_worker_spec.rb index 8db0d4f756dc60..d38cc20d8b2e49 100644 --- a/ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_seats_worker_spec.rb +++ b/ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_seats_worker_spec.rb @@ -5,7 +5,7 @@ RSpec.describe ::GitlabSubscriptions::SeatAssignments::MemberTransfers::RemoveSeatsWorker, :saas, feature_category: :seat_cost_management do let(:worker) { described_class.new } let_it_be_with_refind(:user) { create(:user, :with_namespace) } - let_it_be(:user_2) { create(:user) } + let_it_be_with_refind(:user_2) { create(:user) } describe '#perform' do context 'with group namespaces' do @@ -127,34 +127,45 @@ GitlabSubscriptions::SeatAssignment.by_namespace(user_namespace).pluck(:user_id) ).to match(project.project_members.pluck(:user_id)) end + end + end + + context 'when there are no outdated seat assignments' do + let_it_be_with_refind(:project_2) { create(:project, namespace: user_namespace) } - context 'when there are no outdated seat assignments' do - before do - create(:project_member, project: project, user: user_2) - end + before do + create(:gitlab_subscription_seat_assignment, :active, user: user, namespace: user_namespace) + create(:gitlab_subscription_seat_assignment, :active, user: user_2, namespace: user_namespace) + create(:project_member, project: project, user: user_2) + create(:project_member, project: project_2, user: user_2) + project_2.update!(namespace: root_group) + end + + it 'does not remove seat assignments' do + expect do + worker.perform(user_namespace.id, project_2) + end.not_to change { GitlabSubscriptions::SeatAssignment.by_namespace(user_namespace).count } + end - it 'does not remove seat assignments' do - expect do - worker.perform(user_namespace.id, project_2) - end.not_to change { GitlabSubscriptions::SeatAssignment.by_namespace(user_namespace).count } - end + it 'ensures project members match the seat assignments for the project namespace' do + worker.perform(user_namespace.id, project_2) - it 'ensures project members match the seat assignments for the project namespace' do - worker.perform(user_namespace.id, project_2) + expect( + GitlabSubscriptions::SeatAssignment.by_namespace(project.namespace).pluck(:user_id) + ).to match(::Member.in_hierarchy(user_namespace).pluck(:user_id).uniq) + end + end - expect( - GitlabSubscriptions::SeatAssignment.by_namespace(project.namespace).pluck(:user_id) - ).to match(::Member.in_hierarchy(user_namespace).pluck(:user_id).uniq) - end - end + context 'when the user namespace has no remaining members after project transfer' do + before do + create(:gitlab_subscription_seat_assignment, :active, user: user, namespace: user_namespace) + project.update!(namespace: root_group) end - context 'when the user namespace has no remaining members after project transfer' do - it 'removes outdated seat assignments and leaves the seat of the remaining member' do - worker.perform(user_namespace.id, project_2) + it 'removes outdated seat assignments and leaves the seat of the remaining member' do + worker.perform(user_namespace.id, project) - expect(GitlabSubscriptions::SeatAssignment.by_namespace(user_namespace)).to be_empty - end + expect(GitlabSubscriptions::SeatAssignment.by_namespace(user_namespace)).to be_empty end end end -- GitLab From 050772b6bac9ceb94c5703839a3bb0e89552c2eb Mon Sep 17 00:00:00 2001 From: Jorge Cook Date: Tue, 17 Jun 2025 17:33:59 -0400 Subject: [PATCH 06/16] Add base remove seats worker and tests --- .../base_remove_seats_worker.rb | 63 +++++++++++++++++++ .../base_remove_seats_worker_spec.rb | 15 +++++ 2 files changed, 78 insertions(+) create mode 100644 ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/base_remove_seats_worker.rb create mode 100644 ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/base_remove_seats_worker_spec.rb diff --git a/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/base_remove_seats_worker.rb b/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/base_remove_seats_worker.rb new file mode 100644 index 00000000000000..2342711ccca0c5 --- /dev/null +++ b/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/base_remove_seats_worker.rb @@ -0,0 +1,63 @@ +# frozen_string_literal: true + +# rubocop:disable Scalability/IdempotentWorker -- Idempotent worker declaration in children +module GitlabSubscriptions + module SeatAssignments + module MemberTransfers + class BaseRemoveSeatsWorker + private + + # rubocop: disable CodeReuse/ActiveRecord -- Needed to use the where clauses + def delete_seats(source, user_ids) + return if user_ids.blank? + + user_ids.each_slice(self.class::BATCH_SIZE) do |batch| + ::GitlabSubscriptions::SeatAssignment.where( + namespace_id: source.id, + user_id: batch + ).delete_all + end + end + + def top_group_to_subgroup?(old_root_namespace_id, current_namespace_id, new_root_namespace_id) + old_root_namespace_id == current_namespace_id && new_root_namespace_id != old_root_namespace_id + end + + def collect_group_user_ids(group, filter_user_ids: nil) + scope = Member.for_self_and_descendants(group) + scope = scope.where(user_id: filter_user_ids) if filter_user_ids + scope.pluck_user_ids + end + + def collect_project_user_ids(project, filter_user_ids: nil) + scope = project.project_members + scope = scope.where(user_id: filter_user_ids) if filter_user_ids + scope.pluck_user_ids + end + + def collect_user_namespace_user_ids(user_namespace, filter_user_ids: nil) + scope = Member.in_hierarchy(user_namespace) + scope = scope.where(user_id: filter_user_ids) if filter_user_ids + scope.pluck_user_ids + end + # rubocop: enable CodeReuse/ActiveRecord + + def collect_user_ids(source, filter_user_ids: nil) + return [] unless source + + case source + when Group + collect_group_user_ids(source, filter_user_ids: filter_user_ids) + else + collect_user_namespace_user_ids(source, filter_user_ids: filter_user_ids) + end + end + + def remove_outdated_seat_assignments(old_root_namespace_id, source) + raise NotImplementedError, "#{self.class.name} must implement #remove_outdated_seat_assignments" + end + end + end + end +end +# rubocop:enable Scalability/IdempotentWorker diff --git a/ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/base_remove_seats_worker_spec.rb b/ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/base_remove_seats_worker_spec.rb new file mode 100644 index 00000000000000..c8b26cc5390809 --- /dev/null +++ b/ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/base_remove_seats_worker_spec.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::GitlabSubscriptions::SeatAssignments::MemberTransfers::BaseRemoveSeatsWorker, :saas, feature_category: :seat_cost_management do + let(:worker) { described_class.new } + + describe '#remove_outdated_seat_assignments' do + it 'raises NotImplementedError' do + expect do + worker.send(:remove_outdated_seat_assignments, 1, double) + end.to raise_error(NotImplementedError) + end + end +end -- GitLab From 003a914bac19c57f9326871dfab820b535a5888b Mon Sep 17 00:00:00 2001 From: Jorge Cook Date: Tue, 17 Jun 2025 17:37:58 -0400 Subject: [PATCH 07/16] Add remove group seats worker and tests --- .../remove_group_seats_worker.rb | 56 ++++++++ .../remove_group_seats_worker_spec.rb | 128 ++++++++++++++++++ 2 files changed, 184 insertions(+) create mode 100644 ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_group_seats_worker.rb create mode 100644 ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_group_seats_worker_spec.rb diff --git a/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_group_seats_worker.rb b/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_group_seats_worker.rb new file mode 100644 index 00000000000000..6656265ddd5220 --- /dev/null +++ b/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_group_seats_worker.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +module GitlabSubscriptions + module SeatAssignments + module MemberTransfers + class RemoveGroupSeatsWorker < BaseRemoveSeatsWorker + include ApplicationWorker + + feature_category :seat_cost_management + data_consistency :delayed + urgency :low + + defer_on_database_health_signal :gitlab_main, + [:subscription_seat_assignments, :members], 10.minutes + + idempotent! + + BATCH_SIZE = 1000 + + def perform(old_root_namespace_id, group_id) + group = Group.find_by_id(group_id) + return unless group + + remove_outdated_seat_assignments(old_root_namespace_id, group) + end + + private + + def remove_outdated_seat_assignments(old_root_namespace_id, group) + old_root_namespace = Namespace.find_by_id(old_root_namespace_id) + return unless old_root_namespace + + user_ids_in_transferred_group_hierarchy = collect_group_user_ids(group) + + if top_group_to_subgroup?(old_root_namespace.id, group.id, group.root_ancestor.id) + delete_seats(old_root_namespace, user_ids_in_transferred_group_hierarchy) + return + end + + member_ids_in_old_root_namespace = collect_user_ids( + old_root_namespace, + filter_user_ids: user_ids_in_transferred_group_hierarchy + ) + + member_ids_set = member_ids_in_old_root_namespace.to_set + + outdated_user_ids = user_ids_in_transferred_group_hierarchy.reject do |id| + member_ids_set.include?(id) + end + + delete_seats(old_root_namespace, outdated_user_ids) + end + end + end + end +end diff --git a/ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_group_seats_worker_spec.rb b/ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_group_seats_worker_spec.rb new file mode 100644 index 00000000000000..ede10a31c39979 --- /dev/null +++ b/ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_group_seats_worker_spec.rb @@ -0,0 +1,128 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe GitlabSubscriptions::SeatAssignments::MemberTransfers::RemoveGroupSeatsWorker, :saas, feature_category: :seat_cost_management do + let(:worker) { described_class.new } + let_it_be_with_refind(:user) { create(:user) } + let_it_be_with_refind(:user_2) { create(:user) } + let_it_be_with_refind(:root_group) { create(:group_with_plan, plan: :ultimate_plan) } + let_it_be_with_refind(:transferred_group) { create(:group_with_plan, plan: :ultimate_plan, parent: root_group) } + + before do + create(:gitlab_subscription_seat_assignment, :active, user: user, namespace: root_group) + create(:gitlab_subscription_seat_assignment, :active, user: user_2, namespace: root_group) + create(:group_member, group: transferred_group, user: user) + create(:group_member, group: transferred_group, user: user_2) + end + + describe '#perform' do + context 'when a root group is transferred to become a subgroup of another group' do + let_it_be(:new_parent_group) { create(:group_with_plan, plan: :ultimate_plan) } + + before do + create(:group_member, group: root_group, user: user) + create(:group_member, group: root_group, user: user_2) + root_group.update!(parent: new_parent_group) + end + + it 'removes all outdated seat assignments from old root namespace' do + expect(GitlabSubscriptions::SeatAssignment.by_namespace(root_group)).not_to be_empty + + worker.perform(root_group.id, root_group.id) + + expect(GitlabSubscriptions::SeatAssignment.by_namespace(root_group)).to be_empty + end + end + + context 'when a subgroup is moved to a different group hierarchy' do + let_it_be(:new_parent_group) { create(:group_with_plan, plan: :ultimate_plan) } + + before do + transferred_group.update!(parent: new_parent_group) + end + + it 'removes outdated seat assignments' do + expect do + worker.perform(root_group.id, transferred_group.id) + end.to change { GitlabSubscriptions::SeatAssignment.by_namespace(root_group).count }.by(-2) + end + end + + context 'when a subgroup is moved within same hierarchy' do + let_it_be(:parent_subgroup) { create(:group_with_plan, plan: :ultimate_plan, parent: root_group) } + + before do + transferred_group.update!(parent: parent_subgroup) + end + + it 'does not remove seat assignments' do + expect do + worker.perform(root_group.id, transferred_group.id) + end.not_to change { GitlabSubscriptions::SeatAssignment.by_namespace(root_group).count } + end + end + + context 'when there are no outdated seat assignments' do + let!(:member) { create(:group_member, group: root_group, user: user) } + let!(:member_2) { create(:group_member, group: root_group, user: user_2) } + + it 'does not remove seat assignments' do + expect do + worker.perform(root_group.id, root_group.id) + end.not_to change { GitlabSubscriptions::SeatAssignment.by_namespace(root_group).count } + end + + it 'has members corresponding to seat assignments for the group' do + worker.perform(root_group.id, root_group.id) + + expect( + GitlabSubscriptions::SeatAssignment.by_namespace(root_group).pluck(:user_id) + ).to match(::Member.in_hierarchy(root_group).pluck(:user_id).uniq) + end + end + end + + describe '#collect_user_ids' do + context 'when a group hierarchy has a parent group' do + let_it_be(:root_group) { create(:group_with_plan, plan: :ultimate_plan) } + let_it_be(:child_group_a) { create(:group, parent: root_group) } + let_it_be(:child_group_b) { create(:group, parent: root_group) } + let_it_be(:child_group_c) { create(:group, parent: child_group_a) } + let_it_be(:user_3) { create(:user) } + let_it_be(:user_4) { create(:user) } + + before do + create(:group_member, group: root_group, user: user) + create(:group_member, group: child_group_a, user: user_2) + create(:group_member, group: child_group_b, user: user_3) + create(:group_member, group: child_group_c, user: user_4) + end + + it 'takes the ids of the whole hierarchy when a root is given' do + expect(worker.send(:collect_user_ids, root_group)).to contain_exactly(user.id, user_2.id, user_3.id, user_4.id) + end + + it 'only takes the ids of the given subhierarchy' do + expect(worker.send(:collect_user_ids, child_group_a)).to contain_exactly(user_2.id, user_4.id) + end + + it 'only takes the ids of the given leaf group' do + expect(worker.send(:collect_user_ids, child_group_b)).to contain_exactly(user_3.id) + expect(worker.send(:collect_user_ids, child_group_c)).to contain_exactly(user_4.id) + end + + it 'returns filtered user IDs when filter_user_ids is provided' do + user_ids = worker.send(:collect_user_ids, child_group_a, filter_user_ids: [user_2.id]) + + expect(user_ids).to contain_exactly(user_2.id) + end + + it 'returns an empty array if no users match the filter' do + user_ids = worker.send(:collect_user_ids, child_group_b, filter_user_ids: [user_4.id]) + + expect(user_ids).to be_empty + end + end + end +end -- GitLab From bd5006a1ef29ca7b197ce0dfb48190b9c6798f93 Mon Sep 17 00:00:00 2001 From: Jorge Cook Date: Tue, 17 Jun 2025 17:40:49 -0400 Subject: [PATCH 08/16] Add remove project seats worker and tests --- .../remove_project_seats_worker.rb | 56 +++++ .../remove_project_seats_worker_spec.rb | 229 ++++++++++++++++++ 2 files changed, 285 insertions(+) create mode 100644 ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_project_seats_worker.rb create mode 100644 ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_project_seats_worker_spec.rb diff --git a/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_project_seats_worker.rb b/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_project_seats_worker.rb new file mode 100644 index 00000000000000..a39e25a7ea4628 --- /dev/null +++ b/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_project_seats_worker.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +module GitlabSubscriptions + module SeatAssignments + module MemberTransfers + class RemoveProjectSeatsWorker < BaseRemoveSeatsWorker + include ApplicationWorker + + feature_category :seat_cost_management + data_consistency :delayed + urgency :low + + defer_on_database_health_signal :gitlab_main, + [:subscription_seat_assignments, :members], 10.minutes + + idempotent! + + BATCH_SIZE = 1000 + + def perform(old_root_namespace_id, project_id) + project = Project.find_by_id(project_id) + return unless project + + remove_outdated_seat_assignments(old_root_namespace_id, project) + end + + private + + def remove_outdated_seat_assignments(old_root_namespace_id, project) + old_root_namespace = Namespace.find_by_id(old_root_namespace_id) + return unless old_root_namespace + + user_ids_in_transferred_project = collect_project_user_ids(project) + + if top_group_to_subgroup?(old_root_namespace.id, project.namespace.id, project.namespace.root_ancestor.id) + delete_seats(old_root_namespace, user_ids_in_transferred_project) + return + end + + member_ids_in_old_root_namespace = collect_user_ids( + old_root_namespace, + filter_user_ids: user_ids_in_transferred_project + ) + + member_ids_set = member_ids_in_old_root_namespace.to_set + + outdated_user_ids = user_ids_in_transferred_project.reject do |id| + member_ids_set.include?(id) + end + + delete_seats(old_root_namespace, outdated_user_ids) + end + end + end + end +end diff --git a/ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_project_seats_worker_spec.rb b/ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_project_seats_worker_spec.rb new file mode 100644 index 00000000000000..4734343abeda80 --- /dev/null +++ b/ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_project_seats_worker_spec.rb @@ -0,0 +1,229 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::GitlabSubscriptions::SeatAssignments::MemberTransfers::RemoveProjectSeatsWorker, :saas, feature_category: :seat_cost_management do + let(:worker) { described_class.new } + let_it_be_with_refind(:user) { create(:user, :with_namespace) } + let_it_be_with_refind(:user_2) { create(:user) } + + describe '#perform' do + context 'when the project root namespace is a user namespace' do + let_it_be_with_refind(:user_namespace) { user.namespace } + let_it_be_with_refind(:project) { create(:project, namespace: user_namespace) } + let_it_be_with_refind(:root_group) { create(:group_with_plan, plan: :ultimate_plan) } + + context 'when the user namespace has outdated seats after project has been transferred to a group' do + before do + create(:gitlab_subscription_seat_assignment, :active, user: user, namespace: user_namespace) + create(:gitlab_subscription_seat_assignment, :active, user: user_2, namespace: user_namespace) + end + + context 'when the user namespace still has members via another project' do + let_it_be_with_refind(:project_2) { create(:project, namespace: user_namespace) } + + before do + create(:project_member, project: project_2, user: user_2) + project_2.update!(namespace: root_group) + end + + it 'removes outdated seat assignments and leaves the seat of the remaining member' do + expect do + worker.perform(project.namespace.id, project_2.id) + end.to change { GitlabSubscriptions::SeatAssignment.by_namespace(project.namespace).count }.by(-1) + end + + it 'leaves the seat of the remaining member' do + worker.perform(user_namespace.id, project_2.id) + + expect( + GitlabSubscriptions::SeatAssignment.by_namespace(user_namespace).pluck(:user_id) + ).to match(project.project_members.pluck(:user_id)) + end + end + end + + context 'when there are no outdated seat assignments' do + let_it_be_with_refind(:project_2) { create(:project, namespace: user_namespace) } + + before do + create(:gitlab_subscription_seat_assignment, :active, user: user, namespace: user_namespace) + create(:gitlab_subscription_seat_assignment, :active, user: user_2, namespace: user_namespace) + create(:project_member, project: project, user: user_2) + create(:project_member, project: project_2, user: user_2) + project_2.update!(namespace: root_group) + end + + it 'does not remove seat assignments' do + expect do + worker.perform(user_namespace.id, project_2.id) + end.not_to change { GitlabSubscriptions::SeatAssignment.by_namespace(user_namespace).count } + end + + it 'ensures project members match the seat assignments for the project namespace' do + worker.perform(user_namespace.id, project_2.id) + + expect( + GitlabSubscriptions::SeatAssignment.by_namespace(project.namespace).pluck(:user_id) + ).to match(::Member.in_hierarchy(user_namespace).pluck(:user_id).uniq) + end + end + + context 'when the user namespace has no remaining members after project transfer' do + before do + create(:gitlab_subscription_seat_assignment, :active, user: user, namespace: user_namespace) + project.update!(namespace: root_group) + end + + it 'removes all seat assignments' do + worker.perform(user_namespace.id, project.id) + + expect(GitlabSubscriptions::SeatAssignment.by_namespace(user_namespace)).to be_empty + end + end + end + + context 'when the project root namespace is a group' do + let_it_be_with_refind(:root_group) { create(:group_with_plan, plan: :ultimate_plan) } + let_it_be_with_refind(:project) { create(:project, namespace: root_group) } + let_it_be_with_refind(:user_3) { create(:user) } + + before do + create(:gitlab_subscription_seat_assignment, :active, user: user, namespace: root_group) + create(:gitlab_subscription_seat_assignment, :active, user: user_2, namespace: root_group) + create(:gitlab_subscription_seat_assignment, :active, user: user_3, namespace: root_group) + create(:project_member, project: project, user: user) + create(:project_member, project: project, user: user_2) + create(:project_member, project: project, user: user_3) + end + + context 'when the project is transferred from a group to a user namespace' do + let_it_be_with_refind(:user_namespace) { user.namespace } + + before do + create(:group_member, group: root_group, user: user_3) + project.update!(namespace: user_namespace) + end + + it 'removes outdated seat assignments in the group' do + expect do + worker.perform(root_group.id, project.id) + end.to change { GitlabSubscriptions::SeatAssignment.by_namespace(root_group).count }.by(-2) + end + + it 'leaves seat assignments of old root namespace members' do + worker.perform(root_group.id, project.id) + + expect( + GitlabSubscriptions::SeatAssignment.by_namespace(root_group).pluck(:user_id) + ).to contain_exactly(user_3.id) + end + + context 'when there are no outdated seat assignments' do + before do + create(:group_member, group: root_group, user: user) + create(:group_member, group: root_group, user: user_2) + end + + it 'does not remove seat assignments' do + expect do + worker.perform(root_group.id, project.id) + end.not_to change { GitlabSubscriptions::SeatAssignment.by_namespace(root_group).count } + end + end + end + + context 'when a project is transferred from a root group to a group in another namespace' do + let_it_be_with_refind(:new_root_group) { create(:group_with_plan, plan: :ultimate_plan) } + + before do + project.update!(namespace: new_root_group) + end + + context 'with remaining group members' do + before do + create(:group_member, group: root_group, user: user_3) + end + + it 'removes outdated seat assignments in the old root group' do + expect do + worker.perform(root_group.id, project.id) + end.to change { GitlabSubscriptions::SeatAssignment.by_namespace(root_group).count }.by(-2) + end + + it 'leaves seat assignments of old root group' do + worker.perform(root_group.id, project.id) + + expect( + GitlabSubscriptions::SeatAssignment.by_namespace(root_group).pluck(:user_id) + ).to contain_exactly(user_3.id) + end + + context 'when there are no outdated seat assignments' do + before do + create(:group_member, group: root_group, user: user) + create(:group_member, group: root_group, user: user_2) + end + + it 'does not remove seat assignments' do + expect do + worker.perform(root_group.id, project.id) + end.not_to change { GitlabSubscriptions::SeatAssignment.by_namespace(root_group).count } + end + end + end + + context 'when the group has no remaining members after project transfer' do + it 'removes all seat assignments' do + expect(GitlabSubscriptions::SeatAssignment.by_namespace(root_group)).not_to be_empty + + worker.perform(root_group.id, project.id) + + expect(GitlabSubscriptions::SeatAssignment.by_namespace(root_group)).to be_empty + end + end + + context 'when there are remaining members via another project' do + let_it_be_with_refind(:project_2) { create(:project, namespace: root_group) } + let_it_be(:project_member) { create(:project_member, project: project_2, user: user) } + + before do + project.update!(namespace: new_root_group) + end + + it 'removes only the seats of non members' do + expect do + worker.perform(root_group.id, project.id) + end.to change { GitlabSubscriptions::SeatAssignment.by_namespace(root_group).count }.by(-2) + end + + it 'leaves the seat of the members of the other project still in hierarchy' do + expect( + GitlabSubscriptions::SeatAssignment.by_namespace(root_group).pluck(:user_id) + ).to contain_exactly(user.id, user_2.id, user_3.id) + + worker.perform(root_group.id, project.id) + + expect( + GitlabSubscriptions::SeatAssignment.by_namespace(root_group).pluck(:user_id) + ).to contain_exactly(project_member.user_id) + end + end + end + + context 'when the project is transferred from a root group to a subgroup in the same hierarchy' do + let_it_be_with_refind(:sub_group) { create(:group_with_plan, plan: :ultimate_plan, parent: root_group) } + + before do + project.update!(namespace: sub_group) + end + + it 'does not remove seat assignments' do + expect do + worker.perform(root_group.id, project.id) + end.not_to change { GitlabSubscriptions::SeatAssignment.by_namespace(root_group).count } + end + end + end + end +end -- GitLab From 45a61760bdf5d1876918100d1ddd6470b3660318 Mon Sep 17 00:00:00 2001 From: Jorge Cook Date: Tue, 17 Jun 2025 17:41:55 -0400 Subject: [PATCH 09/16] Add queues yml files --- config/sidekiq_queues.yml | 4 ++++ ee/app/workers/all_queues.yml | 20 ++++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index 908830d4a7208b..a4defe4e763406 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -491,6 +491,10 @@ - 1 - - gitlab_subscriptions_seat_assignments_member_transfers_create_project_seats - 1 +- - gitlab_subscriptions_seat_assignments_member_transfers_remove_group_seats + - 1 +- - gitlab_subscriptions_seat_assignments_member_transfers_remove_project_seats + - 1 - - gitlab_subscriptions_seat_assignments_member_transfers_remove_seats - 1 - - gitlab_subscriptions_self_managed_duo_core_todo_notification diff --git a/ee/app/workers/all_queues.yml b/ee/app/workers/all_queues.yml index f04f53b3f0b020..e986ddc7875cb7 100644 --- a/ee/app/workers/all_queues.yml +++ b/ee/app/workers/all_queues.yml @@ -2483,6 +2483,26 @@ :idempotent: true :tags: [] :queue_namespace: +- :name: gitlab_subscriptions_seat_assignments_member_transfers_remove_group_seats + :worker_name: GitlabSubscriptions::SeatAssignments::MemberTransfers::RemoveGroupSeatsWorker + :feature_category: :seat_cost_management + :has_external_dependencies: false + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] + :queue_namespace: +- :name: gitlab_subscriptions_seat_assignments_member_transfers_remove_project_seats + :worker_name: GitlabSubscriptions::SeatAssignments::MemberTransfers::RemoveProjectSeatsWorker + :feature_category: :seat_cost_management + :has_external_dependencies: false + :urgency: :low + :resource_boundary: :unknown + :weight: 1 + :idempotent: true + :tags: [] + :queue_namespace: - :name: gitlab_subscriptions_seat_assignments_member_transfers_remove_seats :worker_name: GitlabSubscriptions::SeatAssignments::MemberTransfers::RemoveSeatsWorker :feature_category: :seat_cost_management -- GitLab From 8a1949963506a149be2b725882469ee25db75e1b Mon Sep 17 00:00:00 2001 From: Jorge Cook Date: Wed, 18 Jun 2025 11:33:46 -0400 Subject: [PATCH 10/16] Cleanup base worker --- .../base_remove_seats_worker.rb | 33 ++----------------- 1 file changed, 2 insertions(+), 31 deletions(-) diff --git a/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/base_remove_seats_worker.rb b/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/base_remove_seats_worker.rb index 2342711ccca0c5..448bb7085762ad 100644 --- a/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/base_remove_seats_worker.rb +++ b/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/base_remove_seats_worker.rb @@ -18,43 +18,14 @@ def delete_seats(source, user_ids) ).delete_all end end - - def top_group_to_subgroup?(old_root_namespace_id, current_namespace_id, new_root_namespace_id) - old_root_namespace_id == current_namespace_id && new_root_namespace_id != old_root_namespace_id - end - - def collect_group_user_ids(group, filter_user_ids: nil) - scope = Member.for_self_and_descendants(group) - scope = scope.where(user_id: filter_user_ids) if filter_user_ids - scope.pluck_user_ids - end - - def collect_project_user_ids(project, filter_user_ids: nil) - scope = project.project_members - scope = scope.where(user_id: filter_user_ids) if filter_user_ids - scope.pluck_user_ids - end - - def collect_user_namespace_user_ids(user_namespace, filter_user_ids: nil) - scope = Member.in_hierarchy(user_namespace) - scope = scope.where(user_id: filter_user_ids) if filter_user_ids - scope.pluck_user_ids - end # rubocop: enable CodeReuse/ActiveRecord def collect_user_ids(source, filter_user_ids: nil) - return [] unless source - - case source - when Group - collect_group_user_ids(source, filter_user_ids: filter_user_ids) - else - collect_user_namespace_user_ids(source, filter_user_ids: filter_user_ids) - end + raise NotImplementedError end def remove_outdated_seat_assignments(old_root_namespace_id, source) - raise NotImplementedError, "#{self.class.name} must implement #remove_outdated_seat_assignments" + raise NotImplementedError end end end -- GitLab From cfc43a8346814bbce09009d5706026b3b99a8ecd Mon Sep 17 00:00:00 2001 From: Jorge Cook Date: Wed, 18 Jun 2025 11:36:16 -0400 Subject: [PATCH 11/16] Refine remove group seats worker and tests --- .../remove_group_seats_worker.rb | 16 ++++++++++++++-- .../remove_group_seats_worker_spec.rb | 14 ++++++++++++-- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_group_seats_worker.rb b/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_group_seats_worker.rb index 6656265ddd5220..62b45e289e9ef5 100644 --- a/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_group_seats_worker.rb +++ b/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_group_seats_worker.rb @@ -27,10 +27,10 @@ def perform(old_root_namespace_id, group_id) private def remove_outdated_seat_assignments(old_root_namespace_id, group) - old_root_namespace = Namespace.find_by_id(old_root_namespace_id) + old_root_namespace = Group.find_by_id(old_root_namespace_id) return unless old_root_namespace - user_ids_in_transferred_group_hierarchy = collect_group_user_ids(group) + user_ids_in_transferred_group_hierarchy = collect_user_ids(group) if top_group_to_subgroup?(old_root_namespace.id, group.id, group.root_ancestor.id) delete_seats(old_root_namespace, user_ids_in_transferred_group_hierarchy) @@ -50,6 +50,18 @@ def remove_outdated_seat_assignments(old_root_namespace_id, group) delete_seats(old_root_namespace, outdated_user_ids) end + + # rubocop: disable CodeReuse/ActiveRecord -- Needed to use the where clauses + def collect_user_ids(group, filter_user_ids: nil) + scope = Member.for_self_and_descendants(group) + scope = scope.where(user_id: filter_user_ids) if filter_user_ids + scope.pluck_user_ids + end + # rubocop: enable CodeReuse/ActiveRecord -- Needed to use the where clauses + + def top_group_to_subgroup?(old_root_group_id, current_group_id, new_root_group_id) + old_root_group_id == current_group_id && new_root_group_id != old_root_group_id + end end end end diff --git a/ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_group_seats_worker_spec.rb b/ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_group_seats_worker_spec.rb index ede10a31c39979..3c1edc2a0999a1 100644 --- a/ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_group_seats_worker_spec.rb +++ b/ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_group_seats_worker_spec.rb @@ -17,7 +17,7 @@ end describe '#perform' do - context 'when a root group is transferred to become a subgroup of another group' do + context 'when a root group is transferred to become a subgroup of another group hierarchy' do let_it_be(:new_parent_group) { create(:group_with_plan, plan: :ultimate_plan) } before do @@ -83,7 +83,7 @@ end end - describe '#collect_user_ids' do + describe '#collect_group_user_ids' do context 'when a group hierarchy has a parent group' do let_it_be(:root_group) { create(:group_with_plan, plan: :ultimate_plan) } let_it_be(:child_group_a) { create(:group, parent: root_group) } @@ -123,6 +123,16 @@ expect(user_ids).to be_empty end + + context 'when a group has a project' do + let_it_be(:project) { create(:project, namespace: child_group_c) } + let_it_be(:project_user) { create(:user) } + let_it_be(:project_member) { create(:project_member, project: project, user: project_user) } + + it 'takes the ids of the members of the group including the project' do + expect(worker.send(:collect_user_ids, child_group_c)).to contain_exactly(user_4.id, project_user.id) + end + end end end end -- GitLab From 869da28428a5f3a17c2a24e7f81c8e8a0502a1cd Mon Sep 17 00:00:00 2001 From: Jorge Cook Date: Wed, 18 Jun 2025 11:39:58 -0400 Subject: [PATCH 12/16] Refine remove project seats worker and tests --- .../remove_project_seats_worker.rb | 26 ++++++++++++++----- 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_project_seats_worker.rb b/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_project_seats_worker.rb index a39e25a7ea4628..60e533b7519adb 100644 --- a/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_project_seats_worker.rb +++ b/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_project_seats_worker.rb @@ -30,12 +30,7 @@ def remove_outdated_seat_assignments(old_root_namespace_id, project) old_root_namespace = Namespace.find_by_id(old_root_namespace_id) return unless old_root_namespace - user_ids_in_transferred_project = collect_project_user_ids(project) - - if top_group_to_subgroup?(old_root_namespace.id, project.namespace.id, project.namespace.root_ancestor.id) - delete_seats(old_root_namespace, user_ids_in_transferred_project) - return - end + user_ids_in_transferred_project = collect_user_ids(project) member_ids_in_old_root_namespace = collect_user_ids( old_root_namespace, @@ -50,6 +45,25 @@ def remove_outdated_seat_assignments(old_root_namespace_id, project) delete_seats(old_root_namespace, outdated_user_ids) end + + # rubocop: disable CodeReuse/ActiveRecord -- Needed to use the where clauses + def collect_user_ids(source, filter_user_ids: nil) + return [] unless source + + scope = + case source + when Group + Member.for_self_and_descendants(source) + when Project + source.project_members + else + Member.in_hierarchy(source) + end + + scope = scope.where(user_id: filter_user_ids) if filter_user_ids.present? + scope.pluck_user_ids + end + # rubocop: enable CodeReuse/ActiveRecord -- Needed to use the where clauses end end end -- GitLab From 8a09cf978510378d46942bbfd91357dbe570432e Mon Sep 17 00:00:00 2001 From: Jorge Cook Date: Wed, 18 Jun 2025 12:16:00 -0400 Subject: [PATCH 13/16] Move logic to base worker --- .../base_remove_seats_worker.rb | 26 ++++++++++++++++++- .../remove_group_seats_worker.rb | 24 ++++------------- .../remove_project_seats_worker.rb | 22 +++------------- .../base_remove_seats_worker_spec.rb | 15 ----------- 4 files changed, 34 insertions(+), 53 deletions(-) delete mode 100644 ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/base_remove_seats_worker_spec.rb diff --git a/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/base_remove_seats_worker.rb b/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/base_remove_seats_worker.rb index 448bb7085762ad..b7ce4ba96c484c 100644 --- a/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/base_remove_seats_worker.rb +++ b/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/base_remove_seats_worker.rb @@ -5,6 +5,15 @@ module GitlabSubscriptions module SeatAssignments module MemberTransfers class BaseRemoveSeatsWorker + BATCH_SIZE = 1000 + + def perform(old_root_source_id, source_id) + source = find_source_by_id(source_id) + return unless source + + remove_outdated_seat_assignments(old_root_source_id, source) + end + private # rubocop: disable CodeReuse/ActiveRecord -- Needed to use the where clauses @@ -20,11 +29,26 @@ def delete_seats(source, user_ids) end # rubocop: enable CodeReuse/ActiveRecord + def collect_outdated_user_ids(old_root_source, user_ids_in_transferred_group_hierarchy) + member_ids_in_old_root_source = collect_user_ids( + old_root_source, + filter_user_ids: user_ids_in_transferred_group_hierarchy + ).to_set + + user_ids_in_transferred_group_hierarchy.reject do |id| + member_ids_in_old_root_source.include?(id) + end + end + def collect_user_ids(source, filter_user_ids: nil) raise NotImplementedError end - def remove_outdated_seat_assignments(old_root_namespace_id, source) + def remove_outdated_seat_assignments(old_root_source_id, source) + raise NotImplementedError + end + + def find_source_by_id(source_id) raise NotImplementedError end end diff --git a/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_group_seats_worker.rb b/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_group_seats_worker.rb index 62b45e289e9ef5..3abf5a21c35b83 100644 --- a/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_group_seats_worker.rb +++ b/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_group_seats_worker.rb @@ -15,19 +15,14 @@ class RemoveGroupSeatsWorker < BaseRemoveSeatsWorker idempotent! - BATCH_SIZE = 1000 - - def perform(old_root_namespace_id, group_id) - group = Group.find_by_id(group_id) - return unless group + private - remove_outdated_seat_assignments(old_root_namespace_id, group) + def find_source_by_id(group_id) + Group.find_by_id(group_id) end - private - def remove_outdated_seat_assignments(old_root_namespace_id, group) - old_root_namespace = Group.find_by_id(old_root_namespace_id) + old_root_namespace = find_source_by_id(old_root_namespace_id) return unless old_root_namespace user_ids_in_transferred_group_hierarchy = collect_user_ids(group) @@ -37,16 +32,7 @@ def remove_outdated_seat_assignments(old_root_namespace_id, group) return end - member_ids_in_old_root_namespace = collect_user_ids( - old_root_namespace, - filter_user_ids: user_ids_in_transferred_group_hierarchy - ) - - member_ids_set = member_ids_in_old_root_namespace.to_set - - outdated_user_ids = user_ids_in_transferred_group_hierarchy.reject do |id| - member_ids_set.include?(id) - end + outdated_user_ids = collect_outdated_user_ids(old_root_namespace, user_ids_in_transferred_group_hierarchy) delete_seats(old_root_namespace, outdated_user_ids) end diff --git a/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_project_seats_worker.rb b/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_project_seats_worker.rb index 60e533b7519adb..94675806257878 100644 --- a/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_project_seats_worker.rb +++ b/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_project_seats_worker.rb @@ -15,33 +15,19 @@ class RemoveProjectSeatsWorker < BaseRemoveSeatsWorker idempotent! - BATCH_SIZE = 1000 - - def perform(old_root_namespace_id, project_id) - project = Project.find_by_id(project_id) - return unless project + private - remove_outdated_seat_assignments(old_root_namespace_id, project) + def find_source_by_id(project_id) + Project.find_by_id(project_id) end - private - def remove_outdated_seat_assignments(old_root_namespace_id, project) old_root_namespace = Namespace.find_by_id(old_root_namespace_id) return unless old_root_namespace user_ids_in_transferred_project = collect_user_ids(project) - member_ids_in_old_root_namespace = collect_user_ids( - old_root_namespace, - filter_user_ids: user_ids_in_transferred_project - ) - - member_ids_set = member_ids_in_old_root_namespace.to_set - - outdated_user_ids = user_ids_in_transferred_project.reject do |id| - member_ids_set.include?(id) - end + outdated_user_ids = collect_outdated_user_ids(old_root_namespace, user_ids_in_transferred_project) delete_seats(old_root_namespace, outdated_user_ids) end diff --git a/ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/base_remove_seats_worker_spec.rb b/ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/base_remove_seats_worker_spec.rb deleted file mode 100644 index c8b26cc5390809..00000000000000 --- a/ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/base_remove_seats_worker_spec.rb +++ /dev/null @@ -1,15 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe ::GitlabSubscriptions::SeatAssignments::MemberTransfers::BaseRemoveSeatsWorker, :saas, feature_category: :seat_cost_management do - let(:worker) { described_class.new } - - describe '#remove_outdated_seat_assignments' do - it 'raises NotImplementedError' do - expect do - worker.send(:remove_outdated_seat_assignments, 1, double) - end.to raise_error(NotImplementedError) - end - end -end -- GitLab From 04df32526ba772b126fd2aa001f872b83ab6405d Mon Sep 17 00:00:00 2001 From: Jorge Cook Date: Wed, 18 Jun 2025 12:22:23 -0400 Subject: [PATCH 14/16] Remove old worker implementation --- config/sidekiq_queues.yml | 2 - ee/app/workers/all_queues.yml | 10 - .../base_remove_seats_worker.rb | 6 +- .../member_transfers/remove_seats_worker.rb | 87 --------- .../remove_seats_worker_spec.rb | 173 ------------------ 5 files changed, 3 insertions(+), 275 deletions(-) delete mode 100644 ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_seats_worker.rb delete mode 100644 ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_seats_worker_spec.rb diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml index a4defe4e763406..193127a21a82f8 100644 --- a/config/sidekiq_queues.yml +++ b/config/sidekiq_queues.yml @@ -495,8 +495,6 @@ - 1 - - gitlab_subscriptions_seat_assignments_member_transfers_remove_project_seats - 1 -- - gitlab_subscriptions_seat_assignments_member_transfers_remove_seats - - 1 - - gitlab_subscriptions_self_managed_duo_core_todo_notification - 1 - - gitlab_subscriptions_trials_apply_trial diff --git a/ee/app/workers/all_queues.yml b/ee/app/workers/all_queues.yml index e986ddc7875cb7..20e28393042259 100644 --- a/ee/app/workers/all_queues.yml +++ b/ee/app/workers/all_queues.yml @@ -2503,16 +2503,6 @@ :idempotent: true :tags: [] :queue_namespace: -- :name: gitlab_subscriptions_seat_assignments_member_transfers_remove_seats - :worker_name: GitlabSubscriptions::SeatAssignments::MemberTransfers::RemoveSeatsWorker - :feature_category: :seat_cost_management - :has_external_dependencies: false - :urgency: :low - :resource_boundary: :unknown - :weight: 1 - :idempotent: true - :tags: [] - :queue_namespace: - :name: gitlab_subscriptions_self_managed_duo_core_todo_notification :worker_name: GitlabSubscriptions::SelfManaged::DuoCoreTodoNotificationWorker :feature_category: :acquisition diff --git a/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/base_remove_seats_worker.rb b/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/base_remove_seats_worker.rb index b7ce4ba96c484c..cc8c54a9f52d76 100644 --- a/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/base_remove_seats_worker.rb +++ b/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/base_remove_seats_worker.rb @@ -29,13 +29,13 @@ def delete_seats(source, user_ids) end # rubocop: enable CodeReuse/ActiveRecord - def collect_outdated_user_ids(old_root_source, user_ids_in_transferred_group_hierarchy) + def collect_outdated_user_ids(old_root_source, user_ids_in_transferred_source_hierarchy) member_ids_in_old_root_source = collect_user_ids( old_root_source, - filter_user_ids: user_ids_in_transferred_group_hierarchy + filter_user_ids: user_ids_in_transferred_source_hierarchy ).to_set - user_ids_in_transferred_group_hierarchy.reject do |id| + user_ids_in_transferred_source_hierarchy.reject do |id| member_ids_in_old_root_source.include?(id) end end diff --git a/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_seats_worker.rb b/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_seats_worker.rb deleted file mode 100644 index 34542f80afe808..00000000000000 --- a/ee/app/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_seats_worker.rb +++ /dev/null @@ -1,87 +0,0 @@ -# frozen_string_literal: true - -module GitlabSubscriptions - module SeatAssignments - module MemberTransfers - class RemoveSeatsWorker - include ApplicationWorker - - feature_category :seat_cost_management - data_consistency :delayed - urgency :low - - defer_on_database_health_signal :gitlab_main, - [:subscription_seat_assignments, :members], 10.minutes - - idempotent! - - BATCH_SIZE = 1000 - - def perform(old_root_namespace_id, namespace) - return unless namespace - - remove_outdated_seat_assignments(old_root_namespace_id, namespace) - end - - private - - def remove_outdated_seat_assignments(old_root_namespace_id, namespace) - old_root_namespace = Namespace.find_by_id(old_root_namespace_id) - - user_ids_in_transferred_namespace_tree = collect_user_ids(namespace) - - if top_group_to_subgroup?(old_root_namespace_id, namespace.id, namespace.root_ancestor.id) - delete_seats(old_root_namespace, user_ids_in_transferred_namespace_tree) - return - end - - member_ids_in_old_root_namespace = collect_user_ids( - old_root_namespace, - filter_user_ids: user_ids_in_transferred_namespace_tree - ) - - outdated_seat_user_ids = user_ids_in_transferred_namespace_tree.reject do |id| - member_ids_in_old_root_namespace.include?(id) - end - - delete_seats(old_root_namespace, outdated_seat_user_ids) - end - - def delete_seats(namespace, user_ids) - return if user_ids.empty? - - # rubocop:disable CodeReuse/ActiveRecord -- We need this to delete the outdated seat assignments - user_ids.each_slice(BATCH_SIZE) do |batch| - ::GitlabSubscriptions::SeatAssignment.where( - namespace_id: namespace.id, - user_id: batch - ).delete_all - end - # rubocop:enable CodeReuse/ActiveRecord - end - - def top_group_to_subgroup?(old_root_namespace_id, namespace_id, new_root_namespace_id) - old_root_namespace_id == namespace_id && new_root_namespace_id != old_root_namespace_id - end - - def collect_user_ids(namespace, filter_user_ids: nil) - return [] if namespace.nil? - - scope = - if namespace.is_a?(Group) - namespace.hierarchy_members - elsif namespace.is_a?(Project) - namespace.project_members - else - ::Member.in_hierarchy(namespace) - end - - # rubocop:disable CodeReuse/ActiveRecord -- We need this to obtain the user ids of the members - scope = scope.where(user_id: filter_user_ids) if filter_user_ids - scope.pluck(:user_id) - # rubocop:enable CodeReuse/ActiveRecord - end - end - end - end -end diff --git a/ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_seats_worker_spec.rb b/ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_seats_worker_spec.rb deleted file mode 100644 index d38cc20d8b2e49..00000000000000 --- a/ee/spec/workers/gitlab_subscriptions/seat_assignments/member_transfers/remove_seats_worker_spec.rb +++ /dev/null @@ -1,173 +0,0 @@ -# frozen_string_literal: true - -require 'spec_helper' - -RSpec.describe ::GitlabSubscriptions::SeatAssignments::MemberTransfers::RemoveSeatsWorker, :saas, feature_category: :seat_cost_management do - let(:worker) { described_class.new } - let_it_be_with_refind(:user) { create(:user, :with_namespace) } - let_it_be_with_refind(:user_2) { create(:user) } - - describe '#perform' do - context 'with group namespaces' do - let_it_be_with_refind(:root_group) { create(:group_with_plan, plan: :ultimate_plan) } - let_it_be_with_refind(:transferred_group) { create(:group_with_plan, plan: :ultimate_plan, parent: root_group) } - - before do - create(:gitlab_subscription_seat_assignment, :active, user: user, namespace: root_group) - create(:gitlab_subscription_seat_assignment, :active, user: user_2, namespace: root_group) - create(:group_member, group: transferred_group, user: user) - create(:group_member, group: transferred_group, user: user_2) - end - - context 'when a root group namespace has outdated seat assignments after a subgroup transfer' do - let_it_be(:remaining_member) { create(:group_member, group: root_group, user: user) } - let_it_be(:new_parent_group) { create(:group_with_plan, plan: :ultimate_plan) } - - before do - transferred_group.update!(parent: new_parent_group) - end - - it 'removes outdated seat assignments' do - expect do - worker.perform(root_group.id, transferred_group) - end.to change { GitlabSubscriptions::SeatAssignment.by_namespace(root_group).count }.by(-1) - end - - it 'leaves seat assignments of group members' do - worker.perform(root_group.id, transferred_group) - - expect( - GitlabSubscriptions::SeatAssignment.by_namespace(root_group).pluck(:user_id) - ).to match([remaining_member.user_id]) - end - end - - context 'when a root namespace is transferred and becomes a subgroup of another group' do - let_it_be(:new_parent_group) { create(:group_with_plan, plan: :ultimate_plan) } - - before do - create(:group_member, group: root_group, user: user) - create(:group_member, group: root_group, user: user_2) - root_group.update!(parent: new_parent_group) - end - - it 'removes all seat assignments from old root namespace' do - expect do - worker.perform(root_group.id, root_group) - end.to change { GitlabSubscriptions::SeatAssignment.by_namespace(root_group).count }.by(-2) - end - end - - context 'when a subgroup is transferred within the same hierarchy' do - let_it_be(:parent_subgroup) { create(:group_with_plan, plan: :ultimate_plan, parent: root_group) } - let_it_be(:subgroup) { create(:group_with_plan, plan: :ultimate_plan, parent: root_group) } - - before do - create(:group_member, group: subgroup, user: user) - create(:group_member, group: subgroup, user: user_2) - subgroup.update!(parent: parent_subgroup) - end - - it 'does not remove seats' do - expect do - worker.perform(root_group.id, subgroup) - end.not_to change { GitlabSubscriptions::SeatAssignment.by_namespace(root_group).count } - end - end - - context 'when there are no outdated seat assignments' do - let!(:member) { create(:group_member, group: root_group, user: user) } - let!(:member_2) { create(:group_member, group: root_group, user: user_2) } - - it 'does not remove seat assignments' do - expect do - worker.perform(root_group.id, root_group) - end.not_to change { GitlabSubscriptions::SeatAssignment.by_namespace(root_group).count } - end - - it 'has members corresponding to seat assignments for the group' do - worker.perform(root_group.id, root_group) - - expect( - GitlabSubscriptions::SeatAssignment.by_namespace(root_group).pluck(:user_id) - ).to match(::Member.in_hierarchy(root_group).pluck(:user_id).uniq) - end - end - end - - context 'with projects in user namespaces' do - let_it_be_with_refind(:user_namespace) { user.namespace } - let_it_be_with_refind(:project) { create(:project, namespace: user_namespace) } - let_it_be_with_refind(:root_group) { create(:group_with_plan, plan: :ultimate_plan) } - - context 'when the user namespace has outdated seats after project has been transferred to a group' do - before do - create(:gitlab_subscription_seat_assignment, :active, user: user, namespace: user_namespace) - create(:gitlab_subscription_seat_assignment, :active, user: user_2, namespace: user_namespace) - end - - context 'when the user namespace still has members via another project' do - let_it_be_with_refind(:project_2) { create(:project, namespace: user_namespace) } - - before do - create(:project_member, project: project_2, user: user_2) - project_2.update!(namespace: root_group) - end - - it 'removes outdated seat assignments and leaves the seat of the remaining member' do - expect do - worker.perform(project.namespace.id, project_2) - end.to change { GitlabSubscriptions::SeatAssignment.by_namespace(project.namespace).count }.by(-1) - end - - it 'leaves the seat of the remaining member' do - worker.perform(user_namespace, project_2) - - expect( - GitlabSubscriptions::SeatAssignment.by_namespace(user_namespace).pluck(:user_id) - ).to match(project.project_members.pluck(:user_id)) - end - end - end - - context 'when there are no outdated seat assignments' do - let_it_be_with_refind(:project_2) { create(:project, namespace: user_namespace) } - - before do - create(:gitlab_subscription_seat_assignment, :active, user: user, namespace: user_namespace) - create(:gitlab_subscription_seat_assignment, :active, user: user_2, namespace: user_namespace) - create(:project_member, project: project, user: user_2) - create(:project_member, project: project_2, user: user_2) - project_2.update!(namespace: root_group) - end - - it 'does not remove seat assignments' do - expect do - worker.perform(user_namespace.id, project_2) - end.not_to change { GitlabSubscriptions::SeatAssignment.by_namespace(user_namespace).count } - end - - it 'ensures project members match the seat assignments for the project namespace' do - worker.perform(user_namespace.id, project_2) - - expect( - GitlabSubscriptions::SeatAssignment.by_namespace(project.namespace).pluck(:user_id) - ).to match(::Member.in_hierarchy(user_namespace).pluck(:user_id).uniq) - end - end - - context 'when the user namespace has no remaining members after project transfer' do - before do - create(:gitlab_subscription_seat_assignment, :active, user: user, namespace: user_namespace) - project.update!(namespace: root_group) - end - - it 'removes outdated seat assignments and leaves the seat of the remaining member' do - worker.perform(user_namespace.id, project) - - expect(GitlabSubscriptions::SeatAssignment.by_namespace(user_namespace)).to be_empty - end - end - end - end -end -- GitLab From 90a6c83da13cad1a3854f7650321de876bff5136 Mon Sep 17 00:00:00 2001 From: Jorge Cook Date: Wed, 18 Jun 2025 16:53:25 -0400 Subject: [PATCH 15/16] Add seat removal worker to group transfer service and tests --- ee/app/services/ee/groups/transfer_service.rb | 10 +++ .../ee/groups/transfer_service_spec.rb | 62 +++++++++++++++++++ 2 files changed, 72 insertions(+) diff --git a/ee/app/services/ee/groups/transfer_service.rb b/ee/app/services/ee/groups/transfer_service.rb index ef2cafd060a1d6..4fec01f4a7c138 100644 --- a/ee/app/services/ee/groups/transfer_service.rb +++ b/ee/app/services/ee/groups/transfer_service.rb @@ -66,6 +66,7 @@ def post_update_hooks(updated_project_ids, old_root_ancestor_id) process_group_associations(old_root_ancestor_id, group) # Epics and WorkItems sync_security_policies(group, current_user) + reconcile_seat_assignments(old_root_ancestor_id, group) end def sync_security_policies(group, current_user) @@ -184,6 +185,15 @@ def remove_project_compliance_frameworks(project) ) end end + + def reconcile_seat_assignments(old_root_ancestor_id, group) + return unless ::Gitlab::Saas.feature_available?(:gitlab_com_subscriptions) + return if old_root_ancestor_id == group.root_ancestor.id + + ::GitlabSubscriptions::SeatAssignments::MemberTransfers::RemoveGroupSeatsWorker.perform_async( + old_root_ancestor_id, group.id + ) + end end end end diff --git a/ee/spec/services/ee/groups/transfer_service_spec.rb b/ee/spec/services/ee/groups/transfer_service_spec.rb index 04088cb991c60f..b6a2bed46aa534 100644 --- a/ee/spec/services/ee/groups/transfer_service_spec.rb +++ b/ee/spec/services/ee/groups/transfer_service_spec.rb @@ -618,4 +618,66 @@ end end end + + describe 'subscription seat assignments reconciliation' do + before do + stub_saas_features(gitlab_com_subscriptions: true) + end + + context 'when removing seat assignments' do + context 'when a top level group becomes subgroup' do + it 'enqueues RemoveSeatAssignmentsWorker worker with the group being transferred' do + expect(group.root_ancestor).to eq(group) + expect(::GitlabSubscriptions::SeatAssignments::MemberTransfers::RemoveGroupSeatsWorker) + .to receive(:perform_async).with(group.root_ancestor.id, group.id) + + transfer_service.execute(new_group) + expect(group.root_ancestor).to eq(new_group) + end + end + + context 'when a subgroup becomes top level group' do + let(:subgroup) { create(:group, :private, parent: group) } + let(:transfer_service) { described_class.new(subgroup, user) } + + it 'enqueues RemoveSeatAssignmentsWorker worker with the old group namespace and the group being transferred' do + expect(::GitlabSubscriptions::SeatAssignments::MemberTransfers::RemoveGroupSeatsWorker) + .to receive(:perform_async).with(group.root_ancestor.id, subgroup.id) + + transfer_service.execute(nil) + + expect(subgroup.root_ancestor).to eq(subgroup) + end + end + + context 'when a sub-group becomes a sub-group in a different hierarchy' do + let(:subgroup) { create(:group, :private, parent: group) } + let(:transfer_service) { described_class.new(subgroup, user) } + + it 'enqueues RemoveSeatAssignmentsWorker worker with the old group namespace and the group being transferred' do + expect(::GitlabSubscriptions::SeatAssignments::MemberTransfers::RemoveGroupSeatsWorker) + .to receive(:perform_async).with(group.root_ancestor.id, subgroup.id) + + transfer_service.execute(new_group) + + expect(subgroup.root_ancestor).to eq(new_group) + end + end + + context 'when a sub-group becomes a sub-group in the same hierarchy' do + let(:subgroup_to_be_transferred) { create(:group, :private, parent: group) } + let(:subgroup_to_be_new_parent) { create(:group, :private, parent: group) } + let(:transfer_service) { described_class.new(subgroup_to_be_transferred, user) } + + it 'does not enqueue RemoveSeatAssignmentsWorker worker' do + expect(::GitlabSubscriptions::SeatAssignments::MemberTransfers::RemoveGroupSeatsWorker) + .not_to receive(:perform_async) + + transfer_service.execute(subgroup_to_be_new_parent) + + expect(subgroup_to_be_transferred.root_ancestor).to eq(group) + end + end + end + end end -- GitLab From 64401ab0686dd6f431a6b7a9964086fb2a23255a Mon Sep 17 00:00:00 2001 From: Jorge Cook Date: Wed, 18 Jun 2025 16:54:20 -0400 Subject: [PATCH 16/16] Add seat removal worker to project transfer service and tests --- .../services/ee/projects/transfer_service.rb | 10 ++ .../projects/transfer_service_spec.rb | 93 +++++++++++++++++++ 2 files changed, 103 insertions(+) diff --git a/ee/app/services/ee/projects/transfer_service.rb b/ee/app/services/ee/projects/transfer_service.rb index ede11fcef30899..44f298fdc9cf71 100644 --- a/ee/app/services/ee/projects/transfer_service.rb +++ b/ee/app/services/ee/projects/transfer_service.rb @@ -34,6 +34,7 @@ def post_update_hooks(project, old_group) update_compliance_standards_adherence delete_compliance_statuses sync_security_policies + reconcile_seat_assignments(old_namespace.root_ancestor.id, project) end override :remove_paid_features @@ -82,6 +83,15 @@ def transfer_status_data project_namespace_ids: [project.project_namespace_id] ).execute end + + def reconcile_seat_assignments(old_root_ancestor_id, project) + return unless ::Gitlab::Saas.feature_available?(:gitlab_com_subscriptions) + return if old_root_ancestor_id == project.root_ancestor.id + + ::GitlabSubscriptions::SeatAssignments::MemberTransfers::RemoveProjectSeatsWorker.perform_async( + old_root_ancestor_id, project.id + ) + end end end end diff --git a/ee/spec/services/projects/transfer_service_spec.rb b/ee/spec/services/projects/transfer_service_spec.rb index 62705f333946f5..1110a146d941f6 100644 --- a/ee/spec/services/projects/transfer_service_spec.rb +++ b/ee/spec/services/projects/transfer_service_spec.rb @@ -275,4 +275,97 @@ def operation end end end + + describe 'subscription seat assignments reconciliation' do + let_it_be_with_refind(:project_2) { create(:project, namespace: group) } + + before do + stub_saas_features(gitlab_com_subscriptions: true) + end + + context 'when removing seat assignments' do + context 'when we transfer from user_namespace to a top group' do + it 'enqueues RemoveProjectSeatsWorker with the old project namespace and the project' do + expect(::GitlabSubscriptions::SeatAssignments::MemberTransfers::RemoveProjectSeatsWorker) + .to receive(:perform_async).with(project.namespace.id, project.id) + + subject.execute(group) + + expect(project.namespace).to eq(group) + end + end + + context 'when we transfer from top group to user_namespace' do + it 'enqueues RemoveProjectSeatsWorker worker with the project namespace id and project itself' do + expect(::GitlabSubscriptions::SeatAssignments::MemberTransfers::RemoveProjectSeatsWorker) + .to receive(:perform_async).with(project_2.namespace.id, project_2.id) + + described_class.new(project_2, user).execute(user.namespace) + expect(project_2.namespace).to eq(user.namespace) + end + end + + context 'when we transfer from a top group to another top group' do + let(:new_group) { create(:group, :public, owners: user) } + + it 'enqueues RemoveProjectSeatsWorker with the old group namespace and the project' do + expect(::GitlabSubscriptions::SeatAssignments::MemberTransfers::RemoveProjectSeatsWorker) + .to receive(:perform_async).with(group.id, project_2.id) + + described_class.new(project_2, user).execute(new_group) + + expect(project_2.namespace).to eq(new_group) + end + end + + context 'when a project is transferred to a sub-group in the same hierarchy' do + let(:subgroup_to_be_new_parent) { create(:group, :private, parent: group) } + + it 'does not enqueue RemoveProjectSeatsWorker' do + expect(::GitlabSubscriptions::SeatAssignments::MemberTransfers::RemoveProjectSeatsWorker) + .not_to receive(:perform_async) + + described_class.new(project_2, user).execute(subgroup_to_be_new_parent) + + expect(project_2.namespace.root_ancestor).to eq(group) + end + end + + context 'when a project is transferred from a sub-group to a top group in a different hierarchy' do + let(:parent_group) { create(:group, :private) } + let(:subgroup) { create(:group, :private, parent: parent_group) } + + before do + project_2.update!(namespace: subgroup) + end + + it 'enqueues RemoveProjectSeatsWorker with the old parent group id and the project id' do + expect(::GitlabSubscriptions::SeatAssignments::MemberTransfers::RemoveProjectSeatsWorker) + .to receive(:perform_async).with(parent_group.id, project_2.id) + + described_class.new(project_2, user).execute(group) + + expect(project_2.namespace).to eq(group) + end + end + + context 'when a project is transferred from a top group to a sub group in a different hierarchy' do + let(:new_parent_group) { create(:group, :private, owners: user) } + let(:subgroup) { create(:group, :private, parent: new_parent_group) } + + before do + project_2.update!(namespace: group) + end + + it 'enqueues RemoveProjectSeatsWorker with the old parent group id and the project id' do + expect(::GitlabSubscriptions::SeatAssignments::MemberTransfers::RemoveProjectSeatsWorker) + .to receive(:perform_async).with(group.id, project_2.id) + + described_class.new(project_2, user).execute(subgroup) + + expect(project_2.namespace).to eq(subgroup) + end + end + end + end end -- GitLab