Skip to content

feat(products): add rate overrides on rate phases - #5842

Merged
rsempe merged 13 commits into
mainfrom
products-and-plans-rate-phases
Aug 26, 2026
Merged

feat(products): add rate overrides on rate phases#5842
rsempe merged 13 commits into
mainfrom
products-and-plans-rate-phases

Conversation

@rsempe

@rsempe rsempe commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Context

A phase can replace the card's active rate with its own pricing: the rate override.

Description

  • RateOverrides::CreateService — creates the override with the same model/item/timing compatibility matrix as catalog rates; spend floors stay arrears-only; structural card fields (currency, billing timing, proration, …) are rejected with not_overridable instead of being silently dropped.
  • Overrides attach through phase authoring: nested in whole-sequence replace, and per-phase create/update over REST and GraphQL (rate_override input; explicit null clears).
  • Override lifecycle is transactional: replacing or clearing discards the superseded record, destroying a phase discards its override.
  • REST and GraphQL expose rate_override on phases, preloaded/dataloaded.

@rsempe
rsempe force-pushed the products-and-plans-rate-phases branch from 06f3a43 to c7f4515 Compare July 2, 2026 10:25
@rsempe
rsempe force-pushed the products-and-plans-rate-phases branch from c7f4515 to 26cfb0b Compare July 2, 2026 10:40
@rsempe rsempe changed the title feat(products): rate phase CRUD (list + replace-sequence) feat(products): rate phase CRUD + overrides Jul 2, 2026
@rsempe
rsempe force-pushed the products-and-plans-rate-phases branch from 26cfb0b to a250989 Compare July 3, 2026 08:12
@rsempe
rsempe force-pushed the products-and-plans-rate-phases branch from a250989 to 1af88e9 Compare July 3, 2026 14:02
@rsempe
rsempe force-pushed the products-and-plans-rate-phases branch from 1af88e9 to b3b9122 Compare July 3, 2026 14:23
@rsempe
rsempe force-pushed the products-and-plans-rate-phases branch from b3b9122 to 8c539e6 Compare July 3, 2026 14:46
@rsempe
rsempe force-pushed the products-and-plans-rate-phases branch from 8c539e6 to 05b7f68 Compare July 6, 2026 09:43
@rsempe
rsempe force-pushed the products-and-plans-rate-phases branch from 05b7f68 to 1d84df3 Compare July 6, 2026 13:20
@rsempe
rsempe force-pushed the products-and-plans-rate-phases branch from 1d84df3 to f0caaab Compare July 6, 2026 14:17
@rsempe
rsempe force-pushed the products-and-plans-rate-phases branch from f0caaab to 7fee236 Compare July 6, 2026 14:34
@rsempe
rsempe force-pushed the products-and-plans-rate-phases branch from 7fee236 to d82dc0d Compare July 8, 2026 13:57
@rsempe
rsempe force-pushed the products-and-plans-rate-phases branch from d82dc0d to 74dabb0 Compare July 9, 2026 08:56
@rsempe
rsempe force-pushed the products-and-plans-rate-phases branch from 74dabb0 to a3daa0c Compare July 9, 2026 10:02
@lago-claude-ai-agent

Copy link
Copy Markdown
Contributor

Automated pre-review (advisory, not a required check) — verdict: HOLD · CI green

HOLD — the new phase APIs bypass feature gating and leave rate-override lifecycle gaps.

  • Add RequiresProductCatalog to all three GraphQL rate-phase mutations and cover disabled organizations; every sibling catalog mutation is gated, while these currently mutate catalog data without the flag.
  • Make override replacement/clearing and phase deletion soft-discard the detached RateOverride in the same transaction (historical Fee associations already use with_discarded), and cover replacement, clearing, destroy, and failed-save rollback; UpdateService can also persist a new orphan override if rate_phase.save! fails.
  • Distinguish omitted rate_phases from an explicitly empty array in PlanRateCards::CreateService; the current .present? path silently creates default and bypasses ReplaceService's mandatory-sequence validation. Add REST/GraphQL/service coverage for rate_phases: [].

@lago-claude-ai-agent

Copy link
Copy Markdown
Contributor

Automated pre-review (advisory, not a required check) — verdict: HOLD · CI green

HOLD — the new rate-phase APIs have feature-guard, sequence-integrity, override-lifecycle, and query-loading gaps.

  • Add RequiresProductCatalog to all three GraphQL mutations and cover the disabled-catalog case; the REST endpoint and sibling catalog mutations are guarded, but these mutations remain callable.
  • Normalize/cast billing_interval_cycle_count before validating phase order. REST can send "", which the current nil-only create/update checks accept before Active Record casts it to nil, allowing a non-terminal indefinite phase; add create and update coverage for this input.
  • Make override replacement/removal atomic and discard superseded overrides. Update currently persists a new override before saving the phase, leaks it if the phase save fails, and leaves the previous override kept; destroy also leaves its phase's override kept. Cover replace, clear, delete, and rollback paths.
  • Batch/preload rate_phases and rate_override for both transports. The REST index serializer and GraphQL collection fields currently issue per-record association queries despite the existing dataloader pattern.

@lago-claude-ai-agent

Copy link
Copy Markdown
Contributor

Automated pre-review (advisory, not a required check) — verdict: HOLD · CI green

HOLD — the new REST update path can silently remove pricing or return a 500 for malformed input.

  • RatePhases::UpdateService#build_override treats an empty override hash as nil, so rate_override: {} (including a hash emptied by strong params) clears and discards an existing override. Only explicit null should clear it; distinguish the cases and add a regression spec.
  • RatePhasesController#phase_params calls include? on params[:rate_phase] before requiring it, so a missing root parameter raises NoMethodError instead of producing the normal bad-request response. Require the parameter first and cover this case.

@lago-claude-ai-agent

Copy link
Copy Markdown
Contributor

Automated pre-review (advisory, not a required check) — verdict: HOLD · CI green

HOLD — several explicit REST inputs currently succeed with unintended results.

  • Treat only an explicit null override as a clear: RatePhases::UpdateService#build_override conflates {} with null, so an empty REST object silently discards the existing override instead of validating its required fields. Add service/request coverage.
  • Preserve the empty-sequence validation when creating an applied rate card: rate_phases: [] is treated as omitted by present? and silently creates the default phase, bypassing ReplaceService's mandatory-value failure. Cover both REST and GraphQL entry points.
  • Require the rate_phase wrapper before inspecting it in phase_params; a missing wrapper currently calls include? on nil and returns a 500 instead of the normal parameter-missing response. Add request coverage.

@lago-claude-ai-agent

Copy link
Copy Markdown
Contributor

Automated pre-review (advisory, not a required check) — verdict: PASS · CI green

PASS — The rate-phase REST and GraphQL paths are organization-scoped, preserve sequence invariants transactionally, roll back invalid nested overrides, and have focused service/request/mutation coverage. Generated GraphQL schemas match the new API surface, with no unrelated semantic changes or secrets found.

@lago-claude-ai-agent

Copy link
Copy Markdown
Contributor

Automated pre-review (advisory, not a required check) — verdict: HOLD · CI green

HOLD — malformed override input can raise, and concurrent phase edits can break sequence invariants.

  • RateOverrides::CreateService forwards rate_model without enum validation before RateOverride#validate_properties calls to_sym; REST permits numeric scalar input, so rate_model: 1 raises instead of returning a validation error. Empty properties for graduated_percentage also reach the shared validator's nil.map. Validate the enum/property shape before model validation and add regression coverage.
  • RatePhases::{Create,Update,Destroy}Service reads and validates siblings before entering an unlocked transaction. Concurrent edits can both pass the last/terminal checks, remove every phase or leave an indefinite nonterminal phase; concurrent inserts can instead raise RecordNotUnique. Lock the parent across the complete read/check/write sequence and cover the invariant.

@lago-claude-ai-agent

Copy link
Copy Markdown
Contributor

Automated pre-review (advisory, not a required check) — verdict: HOLD · CI green

Phase mutations are unsafe under concurrent requests.

  • Lock the parent and reload phase/sibling state before validation and mutation in the create, update, and destroy services. As written, concurrent inserts/deletes can raise an unhandled unique-position error or use stale terminal state, while concurrent override replacements can leave an orphaned override. Add focused regression coverage for the serialized behavior.

@lago-claude-ai-agent

Copy link
Copy Markdown
Contributor

Automated pre-review (advisory, not a required check) — verdict: PASS · CI green

PASS — The REST and GraphQL rate-phase operations are organization-scoped, preserve sequence and override lifecycle invariants transactionally, and have focused service, request, and mutation coverage.

@lago-claude-ai-agent

Copy link
Copy Markdown
Contributor

Automated pre-review (advisory, not a required check) — verdict: HOLD · CI green

HOLD — Creating a graduated_percentage override with missing ranges raises instead of returning a validation error.

  • RateOverrides::CreateService accepts this rate model and calls save!, but Charges::Validators::GraduatedPercentageService#ranges calls .map on nil for {rate_model: "graduated_percentage", rate_properties: {}}, an input reachable through both new API surfaces. Make this return a validation failure and add regression coverage.

@lago-claude-ai-agent

Copy link
Copy Markdown
Contributor

Automated pre-review (advisory, not a required check) — verdict: HOLD · CI green

HOLD — RatePhases::UpdateService does not participate in the parent locking used by the other sequence mutations.

  • Wrap the lock check, terminal-phase check, and write in parent.with_lock, with a regression spec. As written, an update that makes the current tail indefinite can race a concurrent insert and persist an indefinite non-terminal phase, violating the sequence invariant.

@lago-claude-ai-agent

Copy link
Copy Markdown
Contributor

Automated pre-review (advisory, not a required check) — verdict: PASS · CI green

PASS — Rate override creation, replacement, clearing, validation, and serialization are consistently wired across REST, GraphQL, and phase lifecycle services. Focused specs cover nested creation, validation failures, rollback, and soft deletion of superseded overrides.

@lago-claude-ai-agent

Copy link
Copy Markdown
Contributor

Automated pre-review (advisory, not a required check) — verdict: HOLD · CI green

HOLD — an invalid graduated-percentage override can raise a 500 instead of returning validation errors.

  • RateOverride#validate_properties invokes Charges::Validators::GraduatedPercentageService, whose range parsing calls .map on a missing graduated_percentage_ranges; both REST and GraphQL accept rate_properties: {}. Make this path return a validation failure and add a regression spec through the new override surface.

@lago-claude-ai-agent

Copy link
Copy Markdown
Contributor

Automated pre-review (advisory, not a required check) — verdict: HOLD · CI green

HOLD — concurrent override replacements can leave a kept, unattached override.

  • RatePhases::UpdateService locks the parent but keeps using the phase loaded before that lock. If two requests loaded the same original override, the second update replaces the first but tries to discard only the already-discarded original. Reload the phase inside the lock before reading rate_override_id, and add a stale-instance regression test covering two serialized replacements.

Comment thread app/services/rate_phases/update_service.rb
rsempe added 13 commits August 26, 2026 15:41
## Context

Creating a rate override happens in the context of a specific rate card, which
determines whether a pricing-unit conversion rate is required. Overrides are
attached to the rate phases of a plan's rate card entry.

## Description

Add RateOverrides::CreateService, which builds a RateOverride from the given
pricing parameters. It requires a pricing_unit_conversion_rate when the rate
card carries a pricing unit, and surfaces rate-property validation errors from
the model. Wiring overrides into the rate phase replace flow comes next.
A rate phase can override the rate its card would otherwise apply. The replace
flow is where the override content is submitted alongside each phase, so it must
create the overrides and attach them to the phases it builds.

Extend RatePhases::ReplaceService to accept an optional rate_override per phase.
Each override is created through RateOverrides::CreateService in the context of
the entry's rate card and linked to its phase; a failed override validation
propagates and rolls back the whole replace. The overrides of the phases being
replaced are discarded alongside them.
The rate phase replace endpoint accepts an optional rate override per phase, and
the list/replace responses need to surface the resulting override.

Permit a nested rate_override in the rate phase replace payload and serialize it
back on each phase (null when the phase inherits the card's active rate) via a
new RateOverrideSerializer.
The GraphQL replace-sequence mutation must accept an optional override per phase
and return the resulting override, mirroring the REST contract.

Add the RateOverride type and RateOverrideInput, reusing the rate card rate
model and billing-interval enums. Accept a rate_override on each PhaseInput and
expose it on the RatePhase type (null when the phase inherits the card's active
rate).
## Context

A rate on an advance card rejects a positive min_amount_cents, because a
spend floor true-ups against a closed period and advance billing has none.
The same value submitted through a phase rate override was stored without
complaint: overrides run the rate-model compatibility matrix, but the
min_amount timing rule is a model validation on RateCardRate with no
equivalent on RateOverride (QA-20). Two paths to the same priced object,
one guard on only one of them.

## Description

Apply the rule in the override creation service, next to the compatibility
check, returning the same field and code as the rate layer:
min_amount_cents not_allowed_for_billing_timing. Phase create, update and
replace all build overrides through this service, so every path is covered.
An override request naming a structural card field, billing_timing, currency
or proration, returned 200 with the field silently dropped by strong params —
while the pricing part of the same request was applied. A plan author could
believe a phase is advance-billed, EUR or prorated when the card still decides
all three (QA-21). These are not unknown keys: they are real card fields, so
dropping them hides a contract misunderstanding.

Let the structural card fields through the controllers so the override
creation service can reject them explicitly: each returns not_overridable on
its own key, before anything is written, making the request atomic. Genuinely
unknown keys keep the API-wide tolerant behaviour, and GraphQL is unaffected
since typed inputs already reject unknown arguments.
Review of the v2 error-code inventory flagged non_terminal_indefinite as the
most cryptic code in the API: decoding it requires knowing both jargon terms.
non_contiguous_position had the same problem to a lesser degree. Both name
the internal invariant instead of telling the caller what to change.

Rename non_terminal_indefinite to indefinite_phase_must_be_last and
non_contiguous_position to positions_must_be_contiguous, joining the
must_be_* family of requirement-statement codes.
Pre-review of the rate phase APIs surfaced four gaps: the GraphQL
mutations skipped the product catalog guard, a blank cycle count
slipped past the sequence checks and persisted as an indefinite phase
mid-sequence, override replacement leaked superseded records, and the
phase and override associations loaded one query per record.

Gate the three rate phase mutations with RequiresProductCatalog like
the sibling catalog mutations. Normalize a blank
billing_interval_cycle_count to nil before the sequence checks on
create and update. Replace or clear a phase override inside a
transaction and discard the superseded record, by id after the save:
discarding the loaded association target writes the old foreign key
back through its has_one inverse and silently undoes the replacement.
Discard a phase's override when the phase is destroyed. Preload the
override on the REST index and dataload the phase and override
GraphQL associations. Carry an explicit null rate_override through
strong parameters so an override can be cleared over REST.
Pre-review flagged three explicit inputs that succeeded with
unintended results: an empty rate_override object cleared the existing
override instead of failing validation, and a request without the
rate_phase wrapper crashed with a 500 instead of the parameter-missing
response.

Treat only an omitted or null rate_override as a skip or clear so an
empty object flows into the create service and fails on its missing
fields. Require the rate_phase wrapper before inspecting it for the
explicit-null carry-through. Cover the empty object at the service and
request layers and the missing wrapper at the request layer.
## Context

Companion to the rate card rate change: rateProperties on the rate
override input and object was a raw JSON scalar.

## Description

Reuse the generic Properties and PropertiesInput types on the rate
override GraphQL surface. No service change: the override create
service already converts its params deeply, and REST is untouched.
## Context

Update was the one sequence mutation outside the parent lock: making
the tail indefinite could race a concurrent insert and persist a
non-terminal indefinite phase.

## Description

Run the guards, terminal check and write of the update service under
the same parent lock as create, destroy and replace, with a spec
pinning the lock.
## Context

The graduated percentage validator crash is fixed at the shared
validator level; this pins the behaviour through the override
surface.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants