Conversation
## Context When subscriptions gained the ability to target a specific billing entity, the upgrade path was overlooked. Upgrading a subscription that was explicitly bound to a non-default entity silently dropped the binding, leaving the new subscription's billing_entity_id NULL and falling back to the customer's default at billing time. The same regression affected indirect upgrade callers in Plans::UpdateService and Plans::UpdateAmountService when promoting pending subscriptions. ## Description Extract resolve_billing_entity into a shared concern used by both Subscriptions::CreateService and Subscriptions::PlanUpgradeService. On upgrade the resolution order is: explicit billing_entity_id / billing_entity_code in params (when multi_entity_billing is enabled), else the previous subscription's raw billing_entity_id, else NULL. The carry-over from the previous subscription is independent of the feature flag. Single-entity orgs are unaffected because the FK is NULL for everyone, and an explicit binding made while the flag was on is preserved across upgrades even if the flag is later toggled off. The carry-over uses the raw billing_entity_id column rather than the Subscription#billing_entity reader, which would otherwise materialise the customer's current default and collapse the "inherit at billing time" semantic into a hardcoded snapshot.
## Context Comments added in the previous commit duplicated what tests and the self-evident parameter names already convey. ## Description Remove the two block comments explaining the flag-independent fallback and the raw-FK choice. The "Flag OFF carries over" spec and the fallback_id parameter name carry the same meaning.
## Context The shared concern previously accepted a fallback_id parameter and ran a SELECT to load the BillingEntity record for the upgrade carry-over path. The carry-over is a one-caller concept (only PlanUpgradeService has a previous subscription to carry from), and the loaded record was only used to read back its own primary key. ## Description Remove fallback_id from the concern so it owns just the param-based resolution shared by CreateService and PlanUpgradeService. Move the carry-over to PlanUpgradeService, assigning billing_entity_id directly on the new subscription. This eliminates a database round-trip per upgrade when the previous subscription was bound to an entity and keeps the concern's surface aligned with what is genuinely shared.
lovrocolic
reviewed
May 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Multi-billing-entity support has been rolling out across the API a slice at a time. Subscription create landed in #5467 and wallets in #5471. The upgrade path was the next obvious gap: if you'd explicitly bound a subscription to a non-default entity (say
euinstead of the customer's defaultus), upgrading the plan silently dropped that binding. The new subscription ended up withbilling_entity_idNULL and quietly inherited the customer's default at billing time. Thebilling_entity_idandbilling_entity_codeparams that already work on create were silently ignored on upgrade, so operators couldn't re-bind during an upgrade either.The same gap was hiding inside pending-subscription promotion: when an operator raised a plan's amount above the previous one (via
Plans::UpdateServiceorPlans::UpdateAmountService), the pending subscription got promoted throughPlanUpgradeService, and the binding was lost the same way.Description
There are two moving parts. The resolution rule (read params, gated by
multi_entity_billing, scoped to the customer's organization, not-found error on unknown id/code) is the same oneCreateServicealready had inline. I extracted it into a small shared concern,Subscriptions::Concerns::BillingEntityResolutionConcern, so create and upgrade now share a single source of truth.The new behaviour on upgrade is straightforward: resolve the entity from params if the flag is on, else carry over
current_subscription.billing_entity_id. The carry-over reads the raw FK column rather than theSubscription#billing_entityreader, because the reader would otherwise materialise the customer's current default and freeze it into the new row, collapsing the "inherit at billing time" semantic. The carry-over runs regardless of the feature flag, so an explicit binding made while the flag was on survives even if the flag is later toggled off; on single-entity orgs every FK is NULL, so this branch is a no-op for them.The two pending-promotion paths (
Plans::UpdateServiceandPlans::UpdateAmountService) needed no code change. They already callPlanUpgradeService, and once upgrade does the carry-over, those inherit the behaviour. Two new specs confirm it end-to-end.Behaviour by scenario
eubilling_entity_id= NULL (binding lost)eubilling_entity_code: us, current sub bound toeuusbilling_entity_codenot_found_failure(resource: "billing_entity")eueu(carry-over independent of flag)Plans::UpdateService/UpdateAmountServiceHow to try locally
Spin up a customer on an org with
multi_entity_billingenabled and at least two billing entities. Call themeuandus.eu:billing_entity_*in the body:billing_entity_id. It now matches the previous one. Before this change it would have been NULL, and the next invoice would have been numbered under the customer's default entity.To exercise the explicit-override path, replace the second body with
"billing_entity_code":"us". The new subscription binds tous, and the termination invoice for the previous subscription stays undereu.The full test suite for the change:
226 examples, 0 failures.