fix: adjust flat-fee shape to match usage-based#4128
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughSeparated durable flat-fee base-row fields into Changes
Sequence Diagram(s)sequenceDiagram
participant Service as Service / StateMachine
participant Adapter as flatfee.Adapter
participant DB as Database (ent)
participant Realizers as Realization writers
Service->>Adapter: UpdateCharge(ctx, ChargeBase)
Adapter->>DB: Persist base-row (chargemeta Create/Update)\nSet `status_detailed`
DB-->>Adapter: Saved ChargeFlatFee entity
Adapter-->>Service: return ChargeBase (mapped)
alt Create or update realizations
Service->>Realizers: CreateCreditAllocations / CreatePayment / CreateInvoicedUsage
Realizers->>DB: Persist realization rows (credit, payment, accrued usage)
DB-->>Realizers: persisted realization records
Realizers-->>Service: realizations persisted
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
test/credits/sanity_test.go (1)
948-959:⚠️ Potential issue | 🟡 MinorUse
flatfee.StatusFinalconsistently in this subtest.This block still mixes the old and new status types: Line 951 compares
advancedFlatFee.Statustometa.ChargeStatusFinal, while Lines 958-959 already useflatfee.StatusFinal. Withsuite.Equal, that stale assertion can fail on type mismatch even when the string value matches.Suggested fix
- s.Equal(meta.ChargeStatusFinal, advancedFlatFee.Status) + s.Equal(flatfee.StatusFinal, advancedFlatFee.Status)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/credits/sanity_test.go` around lines 948 - 959, The assertion mixes status enum types: replace the use of meta.ChargeStatusFinal in the comparison with advancedFlatFee.Status by using flatfee.StatusFinal so both checks use the same status type; update the assertion that currently reads s.Equal(meta.ChargeStatusFinal, advancedFlatFee.Status) to compare flatfee.StatusFinal with advancedFlatFee.Status (keeping the later check against updatedFlatFeeCharge unchanged).openmeter/billing/charges/flatfee/charge.go (1)
181-203:⚠️ Potential issue | 🟠 MajorRe-add validation for
AmountAfterProration.
State.Validate()no longer guards this field, soChargeBase.Validate()will now allowUpdateCharge()to persist a negativeAmountAfterProration. That turns an internal bug into durable bad billing state.Suggested fix
func (s State) Validate() error { var errs []error if s.AdvanceAfter != nil { if s.AdvanceAfter.IsZero() { errs = append(errs, fmt.Errorf("advance after is required")) } } + + if s.AmountAfterProration.IsNegative() { + errs = append(errs, fmt.Errorf("amount after proration cannot be negative")) + } return models.NewNillableGenericValidationError(errors.Join(errs...)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/flatfee/charge.go` around lines 181 - 203, State.Validate currently omits checks for AmountAfterProration, allowing negative values to be persisted; update State.Validate to validate the AmountAfterProration field (on the State struct) by ensuring it is present/valid and non-negative (e.g., append an error when AmountAfterProration < 0 or is otherwise invalid) so ChargeBase.Validate/UpdateCharge cannot persist a negative amount; add a clear error message like "amountAfterProration must be non-negative" to the errs slice before returning via models.NewNillableGenericValidationError.openmeter/billing/charges/flatfee/adapter.go (1)
55-71:⚠️ Potential issue | 🟠 Major
InitialStatusis effectively required now.
Validate()still allows the zero value, but the create path now callsInitialStatus.ToMetaChargeStatus()unconditionally inopenmeter/billing/charges/flatfee/adapter/charge.go. So invalid input gets through validation and only fails later while building the DB row.Suggested fix
// Initial status is optional, but if it is set, it must be valid - if i.InitialStatus != "" { - if err := i.InitialStatus.Validate(); err != nil { - errs = append(errs, fmt.Errorf("initial status: %w", err)) - } - } + if i.InitialStatus == "" { + errs = append(errs, fmt.Errorf("initial status is required")) + } else if err := i.InitialStatus.Validate(); err != nil { + errs = append(errs, fmt.Errorf("initial status: %w", err)) + } return models.NewNillableGenericValidationError(errors.Join(errs...)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/flatfee/adapter.go` around lines 55 - 71, IntentWithInitialStatus.Validate currently treats InitialStatus as optional but Create path calls InitialStatus.ToMetaChargeStatus unconditionally (see ToMetaChargeStatus usage in charge building), so invalid/zero InitialStatus slips through; update Validate in IntentWithInitialStatus.Validate to require a non-empty, valid InitialStatus by adding a check that InitialStatus != "" and returning a wrapped error (e.g., "initial status: required") if empty, and keep the existing Validate() call to ensure it's well-formed; adjust error returned via models.NewNillableGenericValidationError as needed so Create-time callers no longer receive invalid InitialStatus.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@openmeter/billing/charges/flatfee/adapter.go`:
- Around line 55-71: IntentWithInitialStatus.Validate currently treats
InitialStatus as optional but Create path calls InitialStatus.ToMetaChargeStatus
unconditionally (see ToMetaChargeStatus usage in charge building), so
invalid/zero InitialStatus slips through; update Validate in
IntentWithInitialStatus.Validate to require a non-empty, valid InitialStatus by
adding a check that InitialStatus != "" and returning a wrapped error (e.g.,
"initial status: required") if empty, and keep the existing Validate() call to
ensure it's well-formed; adjust error returned via
models.NewNillableGenericValidationError as needed so Create-time callers no
longer receive invalid InitialStatus.
In `@openmeter/billing/charges/flatfee/charge.go`:
- Around line 181-203: State.Validate currently omits checks for
AmountAfterProration, allowing negative values to be persisted; update
State.Validate to validate the AmountAfterProration field (on the State struct)
by ensuring it is present/valid and non-negative (e.g., append an error when
AmountAfterProration < 0 or is otherwise invalid) so
ChargeBase.Validate/UpdateCharge cannot persist a negative amount; add a clear
error message like "amountAfterProration must be non-negative" to the errs slice
before returning via models.NewNillableGenericValidationError.
In `@test/credits/sanity_test.go`:
- Around line 948-959: The assertion mixes status enum types: replace the use of
meta.ChargeStatusFinal in the comparison with advancedFlatFee.Status by using
flatfee.StatusFinal so both checks use the same status type; update the
assertion that currently reads s.Equal(meta.ChargeStatusFinal,
advancedFlatFee.Status) to compare flatfee.StatusFinal with
advancedFlatFee.Status (keeping the later check against updatedFlatFeeCharge
unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f85b65ef-c9c5-4f90-a106-995853e6768e
⛔ Files ignored due to path filters (8)
openmeter/ent/db/chargeflatfee.gois excluded by!**/ent/db/**openmeter/ent/db/chargeflatfee/chargeflatfee.gois excluded by!**/ent/db/**openmeter/ent/db/chargeflatfee/where.gois excluded by!**/ent/db/**openmeter/ent/db/chargeflatfee_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargeflatfee_update.gois excluded by!**/ent/db/**openmeter/ent/db/migrate/schema.gois excluded by!**/ent/db/**openmeter/ent/db/mutation.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (24)
.agents/skills/charges/SKILL.mdopenmeter/billing/charges/adapter/search_test.goopenmeter/billing/charges/flatfee/adapter.goopenmeter/billing/charges/flatfee/adapter/charge.goopenmeter/billing/charges/flatfee/adapter/mapper.goopenmeter/billing/charges/flatfee/charge.goopenmeter/billing/charges/flatfee/service/create.goopenmeter/billing/charges/flatfee/service/creditsonly.goopenmeter/billing/charges/flatfee/service/invoice.goopenmeter/billing/charges/flatfee/service/payment.goopenmeter/billing/charges/flatfee/statemachine.goopenmeter/billing/charges/service/invoicable_test.goopenmeter/billing/charges/service/lineage_test.goopenmeter/billing/charges/service/truncation_test.goopenmeter/billing/charges/usagebased/adapter.goopenmeter/billing/charges/usagebased/adapter/charge.goopenmeter/billing/charges/usagebased/statemachine.goopenmeter/ent/schema/chargesflatfee.goopenmeter/ledger/chargeadapter/flatfee.goopenmeter/ledger/chargeadapter/flatfee_test.goopenmeter/ledger/customerbalance/calculation.gotest/credits/sanity_test.gotools/migrate/migrations/20260410132752_flat_fee_status_detailed.down.sqltools/migrate/migrations/20260410132752_flat_fee_status_detailed.up.sql
💤 Files with no reviewable changes (3)
- openmeter/billing/charges/usagebased/adapter.go
- openmeter/billing/charges/usagebased/adapter/charge.go
- openmeter/billing/charges/usagebased/statemachine.go
turip
force-pushed
the
refactor/flat-fee-shape
branch
from
b42eae8 to
c478221
Compare
GAlexIHU
approved these changes
Apr 10, 2026
This was referenced Apr 10, 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
2 participants
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.
Overview
Unify external data structure format between usage-based and flat fee charges.
Notes for reviewer
Summary by CodeRabbit
New Features
Refactor