From 36e9ba56027ca650fdf95b8e66ced60b8897e060 Mon Sep 17 00:00:00 2001 From: Ryan Cobb Date: Mon, 8 Dec 2025 18:03:57 -0700 Subject: [PATCH 1/3] Set plan_name_uids on create/update Add before_validation callbacks to automatically populate plan_name_uid fields in preparation for move from plan_id to plan_name_uid. --- app/models/plan_limits.rb | 9 ++ ee/app/models/ee/ci/pending_build.rb | 8 +- ee/app/models/ee/ci/runner.rb | 9 ++ ee/app/models/gitlab_subscription.rb | 9 ++ ee/spec/models/ee/ci/pending_build_spec.rb | 14 ++++ ee/spec/models/ee/ci/runner_spec.rb | 84 +++++++++++++++++++ ee/spec/models/gitlab_subscription_spec.rb | 64 ++++++++++++++ .../subscription_history_spec.rb | 25 ++++++ spec/models/plan_limits_spec.rb | 55 +++++++++++- 9 files changed, 274 insertions(+), 3 deletions(-) diff --git a/app/models/plan_limits.rb b/app/models/plan_limits.rb index e6019e58579567..68c2cf91029eb7 100644 --- a/app/models/plan_limits.rb +++ b/app/models/plan_limits.rb @@ -17,6 +17,8 @@ class PlanLimits < ApplicationRecord belongs_to :plan + before_validation :set_plan_name_uid + validates :notification_limit, numericality: { only_integer: true, greater_than_or_equal_to: 0 } validates :enforcement_limit, numericality: { only_integer: true, greater_than_or_equal_to: 0 } validates :web_hook_calls, numericality: { only_integer: true, greater_than_or_equal_to: 0 }, @@ -78,6 +80,13 @@ def format_limits_history(user, new_limits) limits_history end + + def set_plan_name_uid + return unless plan_id + return unless plan_id_changed? + + self.plan_name_uid = plan.plan_name_uid_before_type_cast + end end PlanLimits.prepend_mod_with('PlanLimits') diff --git a/ee/app/models/ee/ci/pending_build.rb b/ee/app/models/ee/ci/pending_build.rb index 0056c90a7ad43f..57456b813d1e78 100644 --- a/ee/app/models/ee/ci/pending_build.rb +++ b/ee/app/models/ee/ci/pending_build.rb @@ -17,8 +17,12 @@ module PendingBuild override :args_from_build def args_from_build(build) fields = { minutes_exceeded: minutes_exceeded?(build.project) } - fields[:plan_id] = build.project.actual_plan.id if - ::Gitlab::Saas.feature_available?(:ci_runners_allowed_plans) + + if ::Gitlab::Saas.feature_available?(:ci_runners_allowed_plans) + plan = build.project.actual_plan + fields[:plan_id] = plan.id + fields[:plan_name_uid] = plan.plan_name_uid_before_type_cast + end super.merge(fields) end diff --git a/ee/app/models/ee/ci/runner.rb b/ee/app/models/ee/ci/runner.rb index c1c330c586989e..573b647d69428b 100644 --- a/ee/app/models/ee/ci/runner.rb +++ b/ee/app/models/ee/ci/runner.rb @@ -9,6 +9,8 @@ module Runner MOST_ACTIVE_RUNNERS_BUILDS_LIMIT = 1000 prepended do + before_validation :set_allowed_plan_name_uids + has_one :cost_settings, class_name: 'Ci::Minutes::CostSetting', foreign_key: :runner_id, inverse_of: :runner has_many :hosted_runner_monthly_usages, class_name: 'Ci::Minutes::GitlabHostedRunnerMonthlyUsage', @@ -78,6 +80,13 @@ def cost_factor end end + def set_allowed_plan_name_uids + return if allowed_plan_ids.empty? + return unless allowed_plan_ids_changed? + + self.allowed_plan_name_uids = ::Plan.where(id: allowed_plan_ids).map(&:plan_name_uid_before_type_cast) + end + class_methods do def most_active_runners(inner_query_fn = nil) inner_query = ::Ci::RunningBuild.select( diff --git a/ee/app/models/gitlab_subscription.rb b/ee/app/models/gitlab_subscription.rb index f9a66fd6a6027d..e9717db716c639 100644 --- a/ee/app/models/gitlab_subscription.rb +++ b/ee/app/models/gitlab_subscription.rb @@ -12,6 +12,8 @@ class GitlabSubscription < ApplicationRecord attribute :start_date, default: -> { Date.today } + before_validation :set_hosted_plan_name_uid + before_update :set_max_seats_used_changed_at before_update :log_previous_state_for_update, if: :tracked_attributes_changed? before_update :reset_seat_statistics @@ -258,6 +260,13 @@ def reset_seats_usage_callouts Groups::ResetSeatCalloutsWorker.perform_async(namespace_id) end + + def set_hosted_plan_name_uid + return unless hosted_plan_id + return unless hosted_plan_id_changed? + + self.hosted_plan_name_uid = hosted_plan.plan_name_uid_before_type_cast + end end # Added for JiHu diff --git a/ee/spec/models/ee/ci/pending_build_spec.rb b/ee/spec/models/ee/ci/pending_build_spec.rb index fb810da4577070..8c1e2e7ed5902d 100644 --- a/ee/spec/models/ee/ci/pending_build_spec.rb +++ b/ee/spec/models/ee/ci/pending_build_spec.rb @@ -110,6 +110,14 @@ expect(described_class.last.plan_id).to eq(premium_plan.id) end + + it 'sets plan_name_uid alongside plan_id' do + expect { described_class.upsert_from_build!(build) }.to change(described_class, :count).by(1) + + pending_build = described_class.last + expect(pending_build.plan_id).to eq(premium_plan.id) + expect(pending_build.plan_name_uid).to eq(Plan::PLAN_NAME_UID_LIST[:premium]) + end end context 'when not on SaaS' do @@ -118,6 +126,12 @@ expect(described_class.last.plan_id).to be_nil end + + it 'plan_name_uid is also empty' do + expect { described_class.upsert_from_build!(build) }.to change(described_class, :count).by(1) + + expect(described_class.last.plan_name_uid).to be_nil + end end end end diff --git a/ee/spec/models/ee/ci/runner_spec.rb b/ee/spec/models/ee/ci/runner_spec.rb index a234217c814b18..e83bc401838104 100644 --- a/ee/spec/models/ee/ci/runner_spec.rb +++ b/ee/spec/models/ee/ci/runner_spec.rb @@ -359,5 +359,89 @@ it { is_expected.to include(premium_plan.name) } end end + + describe '#set_allowed_plan_name_uids' do + let_it_be(:free_plan) { create(:free_plan) } + let_it_be(:premium_plan) { create(:premium_plan) } + let_it_be(:ultimate_plan) { create(:ultimate_plan) } + + context 'on create' do + it 'sets allowed_plan_name_uids from allowed_plan_ids' do + runner = build(:ci_runner, :instance, allowed_plan_ids: [premium_plan.id, ultimate_plan.id]) + + expect { runner.save! } + .to change { runner.allowed_plan_name_uids } + .from([]) + + expect(runner.allowed_plan_name_uids).to match_array([ + Plan::PLAN_NAME_UID_LIST[:premium], + Plan::PLAN_NAME_UID_LIST[:ultimate] + ]) + end + + it 'sets UIDs for multiple plans' do + runner = create(:ci_runner, :instance, allowed_plan_ids: [ultimate_plan.id, free_plan.id, premium_plan.id]) + + expect(runner.allowed_plan_name_uids).to match_array([ + Plan::PLAN_NAME_UID_LIST[:ultimate], + Plan::PLAN_NAME_UID_LIST[:free], + Plan::PLAN_NAME_UID_LIST[:premium] + ]) + end + end + + context 'on update' do + it 'updates allowed_plan_name_uids when allowed_plan_ids changes' do + runner = create(:ci_runner, :instance, allowed_plan_ids: [premium_plan.id]) + + expect { runner.update!(allowed_plan_ids: [ultimate_plan.id]) } + .to change { runner.reload.allowed_plan_name_uids } + .from([Plan::PLAN_NAME_UID_LIST[:premium]]) + .to([Plan::PLAN_NAME_UID_LIST[:ultimate]]) + end + + it 'does not change allowed_plan_name_uids when allowed_plan_ids stays the same' do + runner = create(:ci_runner, :instance, allowed_plan_ids: [premium_plan.id]) + original_uids = runner.allowed_plan_name_uids + + runner.update!(description: 'Updated description') + + expect(runner.reload.allowed_plan_name_uids).to eq(original_uids) + expect(original_uids).to match_array([Plan::PLAN_NAME_UID_LIST[:premium]]) + end + end + + context 'when using allowed_plans= setter' do + it 'sets both allowed_plan_ids and allowed_plan_name_uids' do + runner = build(:ci_runner, :instance) + runner.allowed_plans = %w[premium ultimate] + + runner.save! + + expect(runner.allowed_plan_ids).to match_array([premium_plan.id, ultimate_plan.id]) + expect(runner.allowed_plan_name_uids).to match_array([ + Plan::PLAN_NAME_UID_LIST[:premium], + Plan::PLAN_NAME_UID_LIST[:ultimate] + ]) + end + end + + context 'when allowed_plan_ids is empty' do + it 'keeps allowed_plan_name_uids empty' do + runner = create(:ci_runner, :instance, allowed_plan_ids: []) + + expect(runner.allowed_plan_name_uids).to eq([]) + end + end + + context 'when a plan ID does not exist' do + it 'only includes UIDs for existing plans' do + non_existent_id = Plan.maximum(:id).to_i + 1 + runner = create(:ci_runner, :instance, allowed_plan_ids: [premium_plan.id, non_existent_id]) + + expect(runner.allowed_plan_name_uids).to match_array([Plan::PLAN_NAME_UID_LIST[:premium]]) + end + end + end end end diff --git a/ee/spec/models/gitlab_subscription_spec.rb b/ee/spec/models/gitlab_subscription_spec.rb index acc27ef29e0de3..1b9177ef54727f 100644 --- a/ee/spec/models/gitlab_subscription_spec.rb +++ b/ee/spec/models/gitlab_subscription_spec.rb @@ -1126,6 +1126,70 @@ end end + describe '#set_hosted_plan_name_uid' do + let_it_be(:namespace) { create(:namespace) } + let(:premium_plan) { create(:premium_plan) } + let(:ultimate_plan) { create(:ultimate_plan) } + + context 'on create' do + it 'sets hosted_plan_name_uid from the associated hosted_plan' do + subscription = build(:gitlab_subscription, namespace: namespace, hosted_plan: premium_plan) + + expect { subscription.save! } + .to change { subscription.hosted_plan_name_uid } + .from(nil).to(Plan::PLAN_NAME_UID_LIST[:premium]) + end + end + + context 'on update' do + it 'updates hosted_plan_name_uid when hosted_plan changes' do + subscription = create(:gitlab_subscription, namespace: namespace, hosted_plan: premium_plan) + + expect { subscription.update!(hosted_plan: ultimate_plan) } + .to change { subscription.reload.hosted_plan_name_uid } + .from(Plan::PLAN_NAME_UID_LIST[:premium]) + .to(Plan::PLAN_NAME_UID_LIST[:ultimate]) + end + + it 'does not change hosted_plan_name_uid when hosted_plan stays the same' do + subscription = create(:gitlab_subscription, namespace: namespace, hosted_plan: premium_plan) + original_uid = subscription.hosted_plan_name_uid + + subscription.update!(seats: 50) + + expect(subscription.reload.hosted_plan_name_uid).to eq(original_uid) + expect(original_uid).to eq(Plan::PLAN_NAME_UID_LIST[:premium]) + end + end + + context 'when using plan_code= setter' do + it 'sets hosted_plan_name_uid via the setter' do + subscription = build(:gitlab_subscription, namespace: namespace) + subscription.plan_code = 'premium' + + expect { subscription.save! } + .to change { subscription.hosted_plan_name_uid } + .from(nil).to(Plan::PLAN_NAME_UID_LIST[:premium]) + end + end + + context 'when hosted_plan is nil' do + it 'leaves hosted_plan_name_uid as nil' do + subscription = create(:gitlab_subscription, namespace: namespace, hosted_plan: premium_plan) + + subscription.update_column(:hosted_plan_id, nil) + subscription.update_column(:hosted_plan_name_uid, nil) + subscription.reload + + # trigger validation without a hosted plan + subscription.seats = 20 + subscription.save! + + expect(subscription.hosted_plan_name_uid).to be_nil + end + end + end + describe 'constants' do it 'defines SERVICE_ACCOUNT_LIMIT_FOR_TRIAL' do expect(described_class::SERVICE_ACCOUNT_LIMIT_FOR_TRIAL).to eq(100) diff --git a/ee/spec/models/gitlab_subscriptions/subscription_history_spec.rb b/ee/spec/models/gitlab_subscriptions/subscription_history_spec.rb index 68a70387e3b30b..19cff7df6264b1 100644 --- a/ee/spec/models/gitlab_subscriptions/subscription_history_spec.rb +++ b/ee/spec/models/gitlab_subscriptions/subscription_history_spec.rb @@ -203,5 +203,30 @@ ) end end + + context 'when tracking hosted_plan_name_uid' do + let_it_be(:premium_plan) { create(:premium_plan) } + let_it_be(:ultimate_plan) { create(:ultimate_plan) } + + it 'captures hosted_plan_name_uid in the history record' do + record = described_class.create_from_change( + :gitlab_subscription_updated, + { + 'id' => 1, + 'namespace_id' => group1.id, + 'hosted_plan_id' => premium_plan.id, + 'hosted_plan_name_uid' => Plan::PLAN_NAME_UID_LIST[:premium], + 'seats' => 10 + } + ) + + expect(record).to be_persisted + expect(record.hosted_plan_name_uid).to eq(Plan::PLAN_NAME_UID_LIST[:premium]) + end + + it 'includes hosted_plan_name_uid in TRACKED_ATTRIBUTES' do + expect(GitlabSubscriptions::SubscriptionHistory::TRACKED_ATTRIBUTES).to include('hosted_plan_name_uid') + end + end end end diff --git a/spec/models/plan_limits_spec.rb b/spec/models/plan_limits_spec.rb index 0ac6287adbfc43..664b8246ccfcd6 100644 --- a/spec/models/plan_limits_spec.rb +++ b/spec/models/plan_limits_spec.rb @@ -279,7 +279,7 @@ end let(:columns_with_nil) do - %w[repository_size plan_name_uid] + %w[repository_size] end let(:datetime_columns) do @@ -323,6 +323,59 @@ end end + describe '#set_plan_name_uid' do + let(:premium_plan) { create(:plan, name: 'premium') } + let(:ultimate_plan) { create(:plan, name: 'ultimate') } + + context 'on create' do + it 'sets plan_name_uid from the associated plan' do + plan_limits = build(:plan_limits, plan: premium_plan) + + expect { plan_limits.save! } + .to change { plan_limits.plan_name_uid } + .from(nil).to(Plan::PLAN_NAME_UID_LIST[:premium]) + end + end + + context 'on update' do + it 'updates plan_name_uid when plan changes' do + plan_limits = create(:plan_limits, plan: premium_plan) + + expect { plan_limits.update!(plan: ultimate_plan) } + .to change { plan_limits.reload.plan_name_uid } + .from(Plan::PLAN_NAME_UID_LIST[:premium]) + .to(Plan::PLAN_NAME_UID_LIST[:ultimate]) + end + + it 'does not change plan_name_uid when plan stays the same' do + plan_limits = create(:plan_limits, plan: premium_plan) + original_uid = plan_limits.plan_name_uid + + plan_limits.update!(ci_pipeline_size: 100) + + expect(plan_limits.reload.plan_name_uid).to eq(original_uid) + expect(original_uid).to eq(Plan::PLAN_NAME_UID_LIST[:premium]) + end + end + + context 'when plan is nil' do + it 'does not raise an error and leaves plan_name_uid unchanged' do + plan_limits = build(:plan_limits, plan: premium_plan) + plan_limits.save! + + original_uid = plan_limits.plan_name_uid + + plan_limits.plan = nil + + # call the callback directly to verify it handles nil gracefully + expect { plan_limits.send(:set_plan_name_uid) }.not_to raise_error + + # plan_name_uid should remain unchanged when plan is nil + expect(plan_limits.plan_name_uid).to eq(original_uid) + end + end + end + describe '#format_limits_history', :freeze_time do let(:user) { create(:user) } let(:plan_limits) { create(:plan_limits, plan: create(:plan, name: 'premium')) } -- GitLab From d27dd6b425d3fecc4c89afe4d879674c37982221 Mon Sep 17 00:00:00 2001 From: Ryan Cobb Date: Mon, 15 Dec 2025 16:52:53 -0700 Subject: [PATCH 2/3] Set plan_name_uids to nil when plan_id is cleared Update callbacks to handle nil plan associations by removing early returns and using safe navigation. This ensures plan_name_uid columns stay in sync when the corresponding plan_id is set to nil. --- app/models/plan_limits.rb | 3 +-- ee/app/models/ee/ci/runner.rb | 1 - ee/app/models/gitlab_subscription.rb | 3 +-- ee/spec/models/ee/ci/runner_spec.rb | 11 +++++++++++ ee/spec/models/gitlab_subscription_spec.rb | 17 ++++++----------- spec/models/plan_limits_spec.rb | 14 ++++++-------- 6 files changed, 25 insertions(+), 24 deletions(-) diff --git a/app/models/plan_limits.rb b/app/models/plan_limits.rb index 68c2cf91029eb7..fcd2649bafc924 100644 --- a/app/models/plan_limits.rb +++ b/app/models/plan_limits.rb @@ -82,10 +82,9 @@ def format_limits_history(user, new_limits) end def set_plan_name_uid - return unless plan_id return unless plan_id_changed? - self.plan_name_uid = plan.plan_name_uid_before_type_cast + self.plan_name_uid = plan&.plan_name_uid_before_type_cast end end diff --git a/ee/app/models/ee/ci/runner.rb b/ee/app/models/ee/ci/runner.rb index 573b647d69428b..bebf17645cbfd2 100644 --- a/ee/app/models/ee/ci/runner.rb +++ b/ee/app/models/ee/ci/runner.rb @@ -81,7 +81,6 @@ def cost_factor end def set_allowed_plan_name_uids - return if allowed_plan_ids.empty? return unless allowed_plan_ids_changed? self.allowed_plan_name_uids = ::Plan.where(id: allowed_plan_ids).map(&:plan_name_uid_before_type_cast) diff --git a/ee/app/models/gitlab_subscription.rb b/ee/app/models/gitlab_subscription.rb index e9717db716c639..bb4cfef28b296a 100644 --- a/ee/app/models/gitlab_subscription.rb +++ b/ee/app/models/gitlab_subscription.rb @@ -262,10 +262,9 @@ def reset_seats_usage_callouts end def set_hosted_plan_name_uid - return unless hosted_plan_id return unless hosted_plan_id_changed? - self.hosted_plan_name_uid = hosted_plan.plan_name_uid_before_type_cast + self.hosted_plan_name_uid = hosted_plan&.plan_name_uid_before_type_cast end end diff --git a/ee/spec/models/ee/ci/runner_spec.rb b/ee/spec/models/ee/ci/runner_spec.rb index e83bc401838104..3c59bb32465abe 100644 --- a/ee/spec/models/ee/ci/runner_spec.rb +++ b/ee/spec/models/ee/ci/runner_spec.rb @@ -434,6 +434,17 @@ end end + context 'when allowed_plan_ids is cleared' do + it 'clears allowed_plan_name_uids' do + runner = create(:ci_runner, :instance, allowed_plan_ids: [premium_plan.id]) + + expect { runner.update!(allowed_plan_ids: []) } + .to change { runner.reload.allowed_plan_name_uids } + .from([Plan::PLAN_NAME_UID_LIST[:premium]]) + .to([]) + end + end + context 'when a plan ID does not exist' do it 'only includes UIDs for existing plans' do non_existent_id = Plan.maximum(:id).to_i + 1 diff --git a/ee/spec/models/gitlab_subscription_spec.rb b/ee/spec/models/gitlab_subscription_spec.rb index 1b9177ef54727f..93c8a97c986a54 100644 --- a/ee/spec/models/gitlab_subscription_spec.rb +++ b/ee/spec/models/gitlab_subscription_spec.rb @@ -1173,19 +1173,14 @@ end end - context 'when hosted_plan is nil' do - it 'leaves hosted_plan_name_uid as nil' do + context 'when hosted_plan is set to nil' do + it 'clears hosted_plan_name_uid' do subscription = create(:gitlab_subscription, namespace: namespace, hosted_plan: premium_plan) - subscription.update_column(:hosted_plan_id, nil) - subscription.update_column(:hosted_plan_name_uid, nil) - subscription.reload - - # trigger validation without a hosted plan - subscription.seats = 20 - subscription.save! - - expect(subscription.hosted_plan_name_uid).to be_nil + expect { subscription.update!(hosted_plan: nil) } + .to change { subscription.reload.hosted_plan_name_uid } + .from(Plan::PLAN_NAME_UID_LIST[:premium]) + .to(nil) end end end diff --git a/spec/models/plan_limits_spec.rb b/spec/models/plan_limits_spec.rb index 664b8246ccfcd6..0514ca5d647d45 100644 --- a/spec/models/plan_limits_spec.rb +++ b/spec/models/plan_limits_spec.rb @@ -358,20 +358,18 @@ end end - context 'when plan is nil' do - it 'does not raise an error and leaves plan_name_uid unchanged' do + context 'when plan is set to nil' do + it 'sets plan_name_uid to nil' do plan_limits = build(:plan_limits, plan: premium_plan) plan_limits.save! - original_uid = plan_limits.plan_name_uid - plan_limits.plan = nil - # call the callback directly to verify it handles nil gracefully + # The callback should handle nil gracefully and set plan_name_uid to nil + # Note: the database has a NOT NULL constraint on plan_id, so this scenario + # can only occur in-memory before validation expect { plan_limits.send(:set_plan_name_uid) }.not_to raise_error - - # plan_name_uid should remain unchanged when plan is nil - expect(plan_limits.plan_name_uid).to eq(original_uid) + expect(plan_limits.plan_name_uid).to be_nil end end end -- GitLab From e2c1b70982e6db4f2788f673bd0c5d89785faaff Mon Sep 17 00:00:00 2001 From: Ryan Cobb Date: Mon, 15 Dec 2025 17:37:52 -0700 Subject: [PATCH 3/3] Populate missing uid columns on every save Update callbacks to set plan_name_uid whenever it's missing, not just when plan_id changes. Add presence validations that run when the corresponding plan_id is set. --- app/models/plan_limits.rb | 3 ++- ee/app/models/ee/ci/runner.rb | 4 +++- ee/app/models/gitlab_subscription.rb | 3 ++- ee/spec/models/ee/ci/runner_spec.rb | 11 +++++++++++ ee/spec/models/gitlab_subscription_spec.rb | 11 +++++++++++ spec/models/ci/runner_spec.rb | 13 ++++++++++--- spec/models/plan_limits_spec.rb | 11 +++++++++++ 7 files changed, 50 insertions(+), 6 deletions(-) diff --git a/app/models/plan_limits.rb b/app/models/plan_limits.rb index fcd2649bafc924..56298a4f1ca7a3 100644 --- a/app/models/plan_limits.rb +++ b/app/models/plan_limits.rb @@ -19,6 +19,7 @@ class PlanLimits < ApplicationRecord before_validation :set_plan_name_uid + validates :plan_name_uid, presence: true, if: :plan_id? validates :notification_limit, numericality: { only_integer: true, greater_than_or_equal_to: 0 } validates :enforcement_limit, numericality: { only_integer: true, greater_than_or_equal_to: 0 } validates :web_hook_calls, numericality: { only_integer: true, greater_than_or_equal_to: 0 }, @@ -82,7 +83,7 @@ def format_limits_history(user, new_limits) end def set_plan_name_uid - return unless plan_id_changed? + return if plan_name_uid.present? && !plan_id_changed? self.plan_name_uid = plan&.plan_name_uid_before_type_cast end diff --git a/ee/app/models/ee/ci/runner.rb b/ee/app/models/ee/ci/runner.rb index bebf17645cbfd2..84d0fecf776fa5 100644 --- a/ee/app/models/ee/ci/runner.rb +++ b/ee/app/models/ee/ci/runner.rb @@ -11,6 +11,8 @@ module Runner prepended do before_validation :set_allowed_plan_name_uids + validates :allowed_plan_name_uids, presence: true, if: -> { allowed_plan_ids.present? } + has_one :cost_settings, class_name: 'Ci::Minutes::CostSetting', foreign_key: :runner_id, inverse_of: :runner has_many :hosted_runner_monthly_usages, class_name: 'Ci::Minutes::GitlabHostedRunnerMonthlyUsage', @@ -81,7 +83,7 @@ def cost_factor end def set_allowed_plan_name_uids - return unless allowed_plan_ids_changed? + return if allowed_plan_name_uids.present? && !allowed_plan_ids_changed? self.allowed_plan_name_uids = ::Plan.where(id: allowed_plan_ids).map(&:plan_name_uid_before_type_cast) end diff --git a/ee/app/models/gitlab_subscription.rb b/ee/app/models/gitlab_subscription.rb index bb4cfef28b296a..d4a9346ab69ba2 100644 --- a/ee/app/models/gitlab_subscription.rb +++ b/ee/app/models/gitlab_subscription.rb @@ -31,6 +31,7 @@ class GitlabSubscription < ApplicationRecord validates :trial_ends_on, :trial_starts_on, presence: true, if: :trial? validates_comparison_of :trial_ends_on, greater_than: :trial_starts_on, if: :trial? + validates :hosted_plan_name_uid, presence: true, if: :hosted_plan_id? delegate :name, :title, to: :hosted_plan, prefix: :plan, allow_nil: true delegate :exclude_guests?, to: :namespace @@ -262,7 +263,7 @@ def reset_seats_usage_callouts end def set_hosted_plan_name_uid - return unless hosted_plan_id_changed? + return if hosted_plan_name_uid.present? && !hosted_plan_id_changed? self.hosted_plan_name_uid = hosted_plan&.plan_name_uid_before_type_cast end diff --git a/ee/spec/models/ee/ci/runner_spec.rb b/ee/spec/models/ee/ci/runner_spec.rb index 3c59bb32465abe..6ea5483b75e8c5 100644 --- a/ee/spec/models/ee/ci/runner_spec.rb +++ b/ee/spec/models/ee/ci/runner_spec.rb @@ -453,6 +453,17 @@ expect(runner.allowed_plan_name_uids).to match_array([Plan::PLAN_NAME_UID_LIST[:premium]]) end end + + context 'when allowed_plan_name_uids is missing' do + it 'populates allowed_plan_name_uids on save' do + runner = create(:ci_runner, :instance, allowed_plan_ids: [premium_plan.id]) + runner.update_column(:allowed_plan_name_uids, []) + + runner.reload.update!(description: 'Updated') + + expect(runner.reload.allowed_plan_name_uids).to match_array([Plan::PLAN_NAME_UID_LIST[:premium]]) + end + end end end end diff --git a/ee/spec/models/gitlab_subscription_spec.rb b/ee/spec/models/gitlab_subscription_spec.rb index 93c8a97c986a54..da23b59c592b88 100644 --- a/ee/spec/models/gitlab_subscription_spec.rb +++ b/ee/spec/models/gitlab_subscription_spec.rb @@ -1183,6 +1183,17 @@ .to(nil) end end + + context 'when hosted_plan_name_uid is missing' do + it 'populates hosted_plan_name_uid on save' do + subscription = create(:gitlab_subscription, namespace: namespace, hosted_plan: premium_plan) + subscription.update_column(:hosted_plan_name_uid, nil) + + subscription.reload.update!(seats: 100) + + expect(subscription.reload.hosted_plan_name_uid).to eq(Plan::PLAN_NAME_UID_LIST[:premium]) + end + end end describe 'constants' do diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb index 7e724dedc00bf2..e71ba0717403ca 100644 --- a/spec/models/ci/runner_spec.rb +++ b/spec/models/ci/runner_spec.rb @@ -1313,15 +1313,22 @@ def expect_db_update end context 'deduplicates on allowed_plan_ids' do + let_it_be(:premium_plan) { create(:premium_plan) } + let_it_be(:ultimate_plan) { create(:ultimate_plan) } + let_it_be(:free_plan) { create(:free_plan) } + let_it_be(:bronze_plan) { create(:bronze_plan) } + before do - create_list(:ci_runner, 2, allowed_plan_ids: [1, 2]) - create_list(:ci_runner, 2, allowed_plan_ids: [3, 4]) + create_list(:ci_runner, 2, allowed_plan_ids: [premium_plan.id, ultimate_plan.id]) + create_list(:ci_runner, 2, allowed_plan_ids: [free_plan.id, bronze_plan.id]) end it 'creates two matchers' do expect(matchers.size).to eq(2) - expect(matchers.map(&:allowed_plan_ids)).to match_array([[1, 2], [3, 4]]) + expect(matchers.map(&:allowed_plan_ids)).to match_array( + [[premium_plan.id, ultimate_plan.id], [free_plan.id, bronze_plan.id]] + ) end end diff --git a/spec/models/plan_limits_spec.rb b/spec/models/plan_limits_spec.rb index 0514ca5d647d45..022fb468f9fc3b 100644 --- a/spec/models/plan_limits_spec.rb +++ b/spec/models/plan_limits_spec.rb @@ -372,6 +372,17 @@ expect(plan_limits.plan_name_uid).to be_nil end end + + context 'when plan_name_uid is missing' do + it 'populates plan_name_uid on save' do + plan_limits = create(:plan_limits, plan: premium_plan) + plan_limits.update_column(:plan_name_uid, nil) + + plan_limits.reload.update!(ci_pipeline_size: 200) + + expect(plan_limits.reload.plan_name_uid).to eq(Plan::PLAN_NAME_UID_LIST[:premium]) + end + end end describe '#format_limits_history', :freeze_time do -- GitLab