Refactor out `Plan` details from `Namespace` and `GitlabSubscription`

Summary

Namespace and GitlabSubscription models both handle a lot of details from Plan especially by inspecting Plan's constants.

This is a Feature Envy issue that causes a lot of changes to be made or checked when something changes in Plan. E.g. recently we made some changes to introduce an explicit "free plan" record and changes leaked into related models. We should instead have Plan's collaborators to use class/instance methods rather than constants.

Improvements

Plan model is very small today but some domain logic related to it is scattered around models. We are also seeing some knowledge duplication between Plan::ALL_HOSTED_PLANS and Namespace::PLANS which violates the SSOT principle.

Some refactorings:

• GitlabSubscription#has_a_paid_hosted_plan? could use plan.paid_hosted? instead of comparing strings with plan_name.

• GitlabSubscription#upgradable? could use plan.upgradable? instead of being coupled to plan_name != Plan::PAID_HOSTED_PLANS[-1]

• GitlabSubscription#plan_code= could use Plan.for_name(name) returning a Plan.free if name.nil?

• GiltabSubscription.with_a_paid_hosted_plan should instead merge a Plan.paid_hosted scope instead of dealing with Plan's details

• Namespace#*_plan? should be moved to Plan and define a delegation to Namespace#actual_plan

• Namespace#eligible_for_trial? should use actual_plan.eligible_for_trial? rather than implementing this Plan's policy itself.

• Namespace#trial_expired? should simply delegate to actual_plan.free? instead of actual_plan_name == Plan::FREE

• Namespace#plan= could use Plan.for_name(name). However, we have the same code in GitlabSubscription#plan_code= but this instance does not default to free plan. Is this expected? Is it a bug? Seems like the should work exactly the same now that we have Plan.free as explicit record. This code should then by DRYed.

Risks

Because we are refactoring code around Plan we need to be careful not to introduce bugs as it will have direct impact in customer's experience especially for billed customers.

I think this refactoring will consolidate policies around Plan, hence reduce changes of introducing bugs in the long term.

Optional: Missing test coverage

Need to check how good test coverage is around these 3 involved models.

added backstage [DEPRECATED] maintenancerefactor labels

assigned to @fabiopitino

mentioned in merge request !19033 (merged)

/cc @grzesiek @ayufan

marked this issue as related to #32823 (closed)

@fabiopitino Great summary! Who owns the plans? Can we involve them?

@jameslopez would your team normally be involved with plans and subscriptions?

It could theoretically also affect the Data team as some charts and calculations may depend on how plan table contents are planned cc : @kathleentam

@a_akgun Thank you for letting us know!! cc: @mpeychet and @eli_kastelein

@a_akgun this refactoring does not have any impact on the database schema. It's moving plan domain logic where is should belong.

However, this one does !19438 (merged) (if you are interested) but it's moving CI/CD limits out of plans which I doubt are used by Data team because all limits have been 0 so far.

Another related ~"technical debt" issue related to Plan #35161

marked this issue as related to #35161

added devopsfulfillment devopsgrowth labels

added devopscreate groupgitter labels and removed devopsfulfillment devopsgrowth labels

added groupgitaly label and removed groupgitter label

added [Deprecated] Category:Runner devopsverify grouprunner labels and removed devopscreate groupgitaly labels

added 1 deleted label and removed grouprunner label

added devopssystems groupdistribution labels and removed devopsverify + 1 deleted label

removed [Deprecated] Category:Runner devopssystems groupdistribution labels

mentioned in merge request !20303 (closed)

mentioned in issue #32824 (closed)

added Category:Continuous Integration devopsverify grouppipeline execution labels

added workflowscheduling label

unassigned @fabiopitino

added backend label

changed weight to 2

mentioned in merge request !27616 (closed)

Closing as this is being done in !27616 (closed)

closed