feat: add flat fee credit-then-invoice lifecycle#4351
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughImplements a state-machine-driven lifecycle for flat-fee "credit_then_invoice" charges: per-line state machines owned by the flat-fee line engine, adapter support to assign invoice lines to realization runs, AdvanceAfter timing for gated advancement, new realization/payment statuses, and expanded tests for creation, deletion, invoicing, and payments. ChangesFlat-fee Credit-Then-Invoice Settlement Mode
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
openmeter/billing/charges/flatfee/service/creditheninvoice.go (1)
162-166: 💤 Low valueHeads up:
StartRealizationmutates the caller'sinput.Linein place.Setting
input.Line.CreditsAppliedhere mutates the standard line that the caller passed in (sinceinput.Lineis a pointer). It happens to be whatLineEngine.OnStandardInvoiceCreatedwants (the returnedchargesByIDand laterpersistDetailedLinesflow depend on it), but this side effect on a method parameter is easy to miss for future callers. A short comment onStartRealizationwould help signal the contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/charges/flatfee/service/creditheninvoice.go` around lines 162 - 166, StartRealization mutates the caller's input.Line (a pointer) by setting input.Line.CreditsApplied; add a clear doc comment to the StartRealization function explaining this side-effect and that callers (e.g., LineEngine.OnStandardInvoiceCreated, chargesByID flow, persistDetailedLines) rely on input.Line being updated, so future callers know the contract; optionally note that callers should pass a copy if they want to avoid mutation.openmeter/billing/charges/flatfee/service/lineengine.go (4)
149-196: 💤 Low value
OnMutableStandardLinesDeleteddeletion guard is one-shot — second offending line is silently skipped.The dedup via
cleanedChargeIDsruns after the accrued-usage / payment guards, which is intentional for the credit-correction work. But notice: the guards at lines 170-176 only check on the firststdLinefor a givencharge.ID. If the same charge has multiple deleted standard lines, the second one short-circuits at thecleanedChargeIDscheck above… which is fine. The subtler issue: ifinput.Linesis ordered such that a "safe" line for charge X appears before another line where charge X would have accrued usage/payment (shouldn't normally happen since the guards are per-charge), the guard still fires correctly because we look upchargefromchargesByID, not from the line. So this is actually OK 👍 — but a short comment explaining "guards/cleanup are per-charge, not per-line" would make the intent obvious.Also, consider extracting the loop body into a helper (
cleanChargeForDeletedLine) — the 8-step procedure inside the loop is getting hefty.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/charges/flatfee/service/lineengine.go` around lines 149 - 196, The loop in OnMutableStandardLinesDeleted currently applies per-charge guards and cleanup but lacks an explanatory comment and is large; add a short comment above the loop clarifying “guards/cleanup are per-charge (deduped via cleanedChargeIDs) not per-line” and refactor the loop body into a helper function (e.g., cleanChargeForDeletedLine(ctx, stdLine, charge, e) or a method on LineEngine) that performs: validation against CurrentRun AccruedUsage/Payment, obtaining currency calculator, calling e.service.realizations.CorrectAllCredits, and e.service.adapter.UpsertDetailedLines, while preserving the cleanedChargeIDs dedup logic and existing error messages.
244-251: 💤 Low valueThe type-assertion safety net is good — small nit on the error message.
The
*CreditThenInvoiceStateMachinecast is properly guarded with!ok, which is great. One small thing:newStateMachineForStandardLineis the only path that builds the machine after assertingcredit_then_invoicesettlement mode (line 227), so the cast should always succeed in practice. If it ever fails, we've got a wiring bug — consider making the message scream a bit louder (e.g. "BUG: ...") so it's easy to spot in logs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/charges/flatfee/service/lineengine.go` around lines 244 - 251, The type-assertion failure in newStateMachineForStandardLine currently returns a normal error when stateMachine cannot be cast to *CreditThenInvoiceStateMachine; change the error text to make this a loud/localized wiring bug by prefixing the message with "BUG:" (e.g. "BUG: flat fee charge[%s]: expected credit_then_invoice state machine, got %T") so that any unexpected cast failure in newStateMachineForStandardLine / CreditThenInvoiceStateMachine is obvious in logs and easier to triage; keep the same check (ok) and return style but update the error string to include the "BUG:" marker and include charge.ID and the actual type as currently done.
70-92: ⚖️ Poor tradeoffN+1 charge lookups per standard-line lifecycle event.
newStateMachineForStandardLineruns aGetByIDperstdLine, and the same pattern is repeated acrossOnCollectionCompleted,OnInvoiceIssued,OnPaymentAuthorized, andOnPaymentSettled. For invoices with a single flat-fee line this is a non-issue, but if subscriptions ever batch many flat-fee charges onto one invoice it'll multiply DB roundtrips on every lifecycle hook. A batch fetch (à lagetChargesForMutableStandardLineDeleteusingGetByIDs) followed by per-charge state machine construction would scale better. Not blocking — just flagging now while the engine wiring is fresh.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/charges/flatfee/service/lineengine.go` around lines 70 - 92, The code performs an N+1 DB lookup by calling newStateMachineForStandardLine (which internally does GetByID) for each stdLine in handlers like OnCollectionCompleted, OnInvoiceIssued, OnPaymentAuthorized and OnPaymentSettled; fix this by batching the charge fetch first (use the existing pattern getChargesForMutableStandardLineDelete/GetByIDs to retrieve all charge records for the set of stdLine IDs) and then construct each state machine from those in-memory charge objects instead of calling newStateMachineForStandardLine per line; adapt the callers to accept a map of pre-fetched charges (or add a helper newStateMachineFromCharge) and then reuse stateMachine (AdvanceUntilStateStable, FireAndActivate, GetCharge) logic unchanged.
191-191: 💤 Low valuePassing
niltoUpsertDetailedLinesto delete all lines could use a comment for clarity.The nil parameter does delete all lines (the implementation loops over the lines to build a keep-list, so empty list = delete everything), but it's not obvious from reading the call site. A one-line comment here explaining the convention would help, or if the adapter grows an explicit
DeleteDetailedLines(ctx, chargeID)method down the road, that'd be even clearer. Low priority, but worth flagging.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/charges/flatfee/service/lineengine.go` at line 191, The call in lineengine.go that does e.service.adapter.UpsertDetailedLines(ctx, charge.GetChargeID(), nil) relies on a convention where passing nil removes all lines; add a one-line inline comment at that call site clarifying that nil means "delete all detailed lines" (or alternatively add/use a dedicated adapter method like DeleteDetailedLines(ctx, chargeID) and call that instead) so readers of UpsertDetailedLines and callers like e.service.adapter can immediately understand the intent.test/credits/credit_then_invoice_test.go (2)
1900-1991: 💤 Low valueTiny gap: this test doesn't run charge advancement before deleting.
TestFlatFeeCreditThenInvoiceDeleteActiveChargeBeforeStandardInvoiceDeletesGatheringLineadvances the charge intoStatusActive(line 1969) and then deletes — perfect. One small thing that would harden the test: it doesn't assert the gathering line'sChargeIDmatchesflatFeeChargeID.IDlike sibling tests do (e.g. line 601). Easy add if you want symmetry across the suite.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/credits/credit_then_invoice_test.go` around lines 1900 - 1991, Add an assertion that the returned gathering line(s) belong to the deleted charge by checking the gathering line's ChargeID equals flatFeeChargeID.ID; specifically, in TestFlatFeeCreditThenInvoiceDeleteActiveChargeBeforeStandardInvoiceDeletesGatheringLine after calling s.mustGatheringLinesForCharge(...) (the activeLines/allLines checks) add an assert like s.Equal(flatFeeChargeID.ID, allLines[0].ChargeID) (or similar for activeLines when non-empty) so the gathering line is verified to reference the expected flatFeeChargeID from the test.
961-1092: 💤 Low valueMixing
t.Runands.Runfor subtests inside the same testify suite.The earlier flat-fee tests (lines 526, 644, 747, 1094, 1264) consistently use
s.Run("…", func() { … }), but these later ones flip tot.Run("…", func(t *testing.T) { … }). Both work, but they aren't equivalent inside a testify suite:
s.Runkeepss.T()pointing at the subtest's*testing.T, so suite helpers usings.T().Helper()ands.T().Context()line up with the rightt.- Plain
t.Runshadows the outertinside the closure, buts.T()still returns the parent'sT, which can mismatch ownership/cancellation ifs.T().Context()is used inside.For consistency (and to keep
t.Context()semantics matching the active subtest), it'd be nice to standardize ons.Runhere. Not a defect — just a friendlier-to-read mid-file inconsistency.Also applies to: 1300-1495, 1497-1610, 1612-1754, 1756-1898, 1900-1991
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/credits/credit_then_invoice_test.go` around lines 961 - 1092, Several subtests in this suite use t.Run with a local *testing.T (e.g. the subtests titled "given a partially credited flat fee charge", "when the pending line is collected into a mutable draft invoice", "when the charge delete patch removes the mutable standard line", and "then deleting the mutable line restores the initial ledger state") which breaks testify suite semantics; replace each t.Run("...", func(t *testing.T) { ... }) with s.Run("...", func() { ... }) so the subtest uses the suite's T, remove the inner t parameter from the closure, and update any inside references to t to use s.T() or suite helpers (or remove if unused) to keep context/cancellation and helper behavior consistent with other tests like the earlier flat-fee cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openmeter/billing/charges/flatfee/service/creditheninvoice.go`:
- Around line 188-202: The AreAllPaymentsSettled method on
CreditThenInvoiceStateMachine currently returns true when
Charge.Realizations.CurrentRun is nil, which can prematurely advance a charge to
final state; update the logic so a nil CurrentRun does not imply all payments
are settled—either return false or surface/log an error instead of true. Locate
the AreAllPaymentsSettled function and change the early-return for
s.Charge.Realizations.CurrentRun == nil to a safer behavior (e.g., return false
or call a logging/guard routine), ensuring the check aligns with state
transitions such as StatusActiveAwaitingPaymentSettlement and still returns
payment.StatusSettled only when a non-nil CurrentRun.Payment is settled.
- Line 183: accrueInvoiceUsage currently sets s.Charge.State.AdvanceAfter = nil
in-memory but does not persist it, creating a race if the charge is refetched
before the state machine later runs OnActive(s.ClearAdvanceAfter) during the
StatusFinal transition; update accrueInvoiceUsage to persist the cleared value
immediately (e.g., call the adapter method that writes charge state) after
setting s.Charge.State.AdvanceAfter = nil, or if you intentionally defer
persistence, add a clear comment in accrueInvoiceUsage referencing the
OnActive(s.ClearAdvanceAfter) hook and StatusFinal to explain the guaranteed
state-machine flow; locate the change around the accrueInvoiceUsage function and
the s.Charge.State.AdvanceAfter assignment and either add the adapter
persistence call or the explanatory comment accordingly.
---
Nitpick comments:
In `@openmeter/billing/charges/flatfee/service/creditheninvoice.go`:
- Around line 162-166: StartRealization mutates the caller's input.Line (a
pointer) by setting input.Line.CreditsApplied; add a clear doc comment to the
StartRealization function explaining this side-effect and that callers (e.g.,
LineEngine.OnStandardInvoiceCreated, chargesByID flow, persistDetailedLines)
rely on input.Line being updated, so future callers know the contract;
optionally note that callers should pass a copy if they want to avoid mutation.
In `@openmeter/billing/charges/flatfee/service/lineengine.go`:
- Around line 149-196: The loop in OnMutableStandardLinesDeleted currently
applies per-charge guards and cleanup but lacks an explanatory comment and is
large; add a short comment above the loop clarifying “guards/cleanup are
per-charge (deduped via cleanedChargeIDs) not per-line” and refactor the loop
body into a helper function (e.g., cleanChargeForDeletedLine(ctx, stdLine,
charge, e) or a method on LineEngine) that performs: validation against
CurrentRun AccruedUsage/Payment, obtaining currency calculator, calling
e.service.realizations.CorrectAllCredits, and
e.service.adapter.UpsertDetailedLines, while preserving the cleanedChargeIDs
dedup logic and existing error messages.
- Around line 244-251: The type-assertion failure in
newStateMachineForStandardLine currently returns a normal error when
stateMachine cannot be cast to *CreditThenInvoiceStateMachine; change the error
text to make this a loud/localized wiring bug by prefixing the message with
"BUG:" (e.g. "BUG: flat fee charge[%s]: expected credit_then_invoice state
machine, got %T") so that any unexpected cast failure in
newStateMachineForStandardLine / CreditThenInvoiceStateMachine is obvious in
logs and easier to triage; keep the same check (ok) and return style but update
the error string to include the "BUG:" marker and include charge.ID and the
actual type as currently done.
- Around line 70-92: The code performs an N+1 DB lookup by calling
newStateMachineForStandardLine (which internally does GetByID) for each stdLine
in handlers like OnCollectionCompleted, OnInvoiceIssued, OnPaymentAuthorized and
OnPaymentSettled; fix this by batching the charge fetch first (use the existing
pattern getChargesForMutableStandardLineDelete/GetByIDs to retrieve all charge
records for the set of stdLine IDs) and then construct each state machine from
those in-memory charge objects instead of calling newStateMachineForStandardLine
per line; adapt the callers to accept a map of pre-fetched charges (or add a
helper newStateMachineFromCharge) and then reuse stateMachine
(AdvanceUntilStateStable, FireAndActivate, GetCharge) logic unchanged.
- Line 191: The call in lineengine.go that does
e.service.adapter.UpsertDetailedLines(ctx, charge.GetChargeID(), nil) relies on
a convention where passing nil removes all lines; add a one-line inline comment
at that call site clarifying that nil means "delete all detailed lines" (or
alternatively add/use a dedicated adapter method like DeleteDetailedLines(ctx,
chargeID) and call that instead) so readers of UpsertDetailedLines and callers
like e.service.adapter can immediately understand the intent.
In `@test/credits/credit_then_invoice_test.go`:
- Around line 1900-1991: Add an assertion that the returned gathering line(s)
belong to the deleted charge by checking the gathering line's ChargeID equals
flatFeeChargeID.ID; specifically, in
TestFlatFeeCreditThenInvoiceDeleteActiveChargeBeforeStandardInvoiceDeletesGatheringLine
after calling s.mustGatheringLinesForCharge(...) (the activeLines/allLines
checks) add an assert like s.Equal(flatFeeChargeID.ID, allLines[0].ChargeID) (or
similar for activeLines when non-empty) so the gathering line is verified to
reference the expected flatFeeChargeID from the test.
- Around line 961-1092: Several subtests in this suite use t.Run with a local
*testing.T (e.g. the subtests titled "given a partially credited flat fee
charge", "when the pending line is collected into a mutable draft invoice",
"when the charge delete patch removes the mutable standard line", and "then
deleting the mutable line restores the initial ledger state") which breaks
testify suite semantics; replace each t.Run("...", func(t *testing.T) { ... })
with s.Run("...", func() { ... }) so the subtest uses the suite's T, remove the
inner t parameter from the closure, and update any inside references to t to use
s.T() or suite helpers (or remove if unused) to keep context/cancellation and
helper behavior consistent with other tests like the earlier flat-fee cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9ee5d94c-7561-4fd8-b89d-202b2d7841cc
⛔ Files ignored due to path filters (2)
openmeter/ent/db/chargeflatfee/chargeflatfee.gois excluded by!**/ent/db/**openmeter/ent/db/migrate/schema.gois excluded by!**/ent/db/**
📒 Files selected for processing (22)
.agents/skills/charges/SKILL.mdopenmeter/billing/charges/flatfee/adapter.goopenmeter/billing/charges/flatfee/adapter/charge.goopenmeter/billing/charges/flatfee/adapter/realizationrun.goopenmeter/billing/charges/flatfee/handler.goopenmeter/billing/charges/flatfee/service.goopenmeter/billing/charges/flatfee/service/create.goopenmeter/billing/charges/flatfee/service/creditheninvoice.goopenmeter/billing/charges/flatfee/service/creditsonly.goopenmeter/billing/charges/flatfee/service/invoice.goopenmeter/billing/charges/flatfee/service/lineengine.goopenmeter/billing/charges/flatfee/service/payment.goopenmeter/billing/charges/flatfee/service/statemachine.goopenmeter/billing/charges/flatfee/service/triggers.goopenmeter/billing/charges/flatfee/statemachine.goopenmeter/billing/charges/service/advance.goopenmeter/billing/charges/service/create.goopenmeter/billing/charges/service/handlers_test.goopenmeter/billing/charges/service/invoicable_test.goopenmeter/billing/charges/service/invoice.gotest/credits/credit_then_invoice_test.gotest/credits/sanity_test.go
💤 Files with no reviewable changes (2)
- openmeter/billing/charges/service/advance.go
- openmeter/billing/charges/flatfee/service.go
turip
added
area/billing
release-note/feature
turip
changed the title
turip
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openmeter/billing/charges/service/advance_test.go`:
- Line 38: Replace usages of context.Background() in the tests in
advance_test.go with the test-bound context from the suite's testing.T (e.g.,
use ctx := s.T().Context() or directly s.T().Context()) so the context lifecycle
is tied to the test harness; update each test that currently calls
context.Background() (the three occurrences in this file) to use s.T().Context()
instead, ensuring any variables named ctx are assigned from s.T().Context() and
passed through the same functions as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0668adda-aa2f-40ab-aea8-a036db841e01
📒 Files selected for processing (3)
openmeter/billing/charges/flatfee/service/triggers.goopenmeter/billing/charges/service/advance_test.goopenmeter/billing/charges/service/invoicable_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- openmeter/billing/charges/flatfee/service/triggers.go
turip
force-pushed
the
feat/move-flat-fee-to-state-machine
branch
from
0abcf43 to
1de7f56
Compare
| if len(split) == 0 { | ||
| return meta.ChargeStatusCreated, fmt.Errorf("invalid status: %s", s) | ||
| } |
There was a problem hiding this comment.
status Validate is already called, so this is a noop
There was a problem hiding this comment.
Yes, but I would keep it if you don't mind. It's just there so that I don't have to think about what Validate does.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
openmeter/billing/charges/flatfee/service/statemachine.go (2)
95-108: ⚡ Quick winRemove unused context parameters.
All three of these helper methods accept
context.Contextbut never use it. According to the coding guidelines: "Propagate caller's context through the full call path or remove unused context parameters."Since these methods just perform simple field mutations (
meta.NormalizeTimestampdoesn't need context), the context parameters should be removed unless they're required by an interface contract.Also worth noting: all three methods return
errorbut always returnnil. If these aren't implementing a specific interface, consider returningvoidto simplify the signatures.♻️ Suggested simplification
-func (s *stateMachine) AdvanceAfterServicePeriodFrom(ctx context.Context) error { +func (s *stateMachine) AdvanceAfterServicePeriodFrom() { s.Charge.State.AdvanceAfter = lo.ToPtr(meta.NormalizeTimestamp(s.Charge.Intent.ServicePeriod.From)) - return nil } -func (s *stateMachine) AdvanceAfterServicePeriodTo(ctx context.Context) error { +func (s *stateMachine) AdvanceAfterServicePeriodTo() { s.Charge.State.AdvanceAfter = lo.ToPtr(meta.NormalizeTimestamp(s.Charge.Intent.ServicePeriod.To)) - return nil } -func (s *stateMachine) ClearAdvanceAfter(ctx context.Context) error { +func (s *stateMachine) ClearAdvanceAfter() { s.Charge.State.AdvanceAfter = nil - return nil }As per coding guidelines: "Propagate caller's context through the full call path or remove unused context parameters."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/charges/flatfee/service/statemachine.go` around lines 95 - 108, The three methods AdvanceAfterServicePeriodFrom, AdvanceAfterServicePeriodTo and ClearAdvanceAfter accept a context.Context and return error but neither use ctx nor ever return non-nil; if they are not required by an interface, remove the context parameter and change the return type to void (no error), update bodies to just set s.Charge.State.AdvanceAfter via meta.NormalizeTimestamp(s.Charge.Intent.ServicePeriod.From/To) or nil for ClearAdvanceAfter; if they are required by an interface, keep the signatures but add a comment explaining why ctx/error are unused or consider returning early with nil as currently implemented.
91-93: 💤 Low valueConsider adding a context parameter for consistency.
This method doesn't accept
context.Context, while the three helper methods below do. Whileclock.Now()and the comparison don't strictly need context, having a consistent signature across all these service-period helpers would make the API cleaner and more predictable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/charges/flatfee/service/statemachine.go` around lines 91 - 93, Update the IsInsideServicePeriod method to accept a context.Context for API consistency with the three helper methods below: change the signature of stateMachine.IsInsideServicePeriod to take (ctx context.Context) and update all call sites and tests to pass the incoming context; keep the implementation using clock.Now() unchanged (you don't need to use ctx inside), but add the context import if missing so the package compiles and the method signature matches the other helpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@openmeter/billing/charges/flatfee/service/statemachine.go`:
- Around line 95-108: The three methods AdvanceAfterServicePeriodFrom,
AdvanceAfterServicePeriodTo and ClearAdvanceAfter accept a context.Context and
return error but neither use ctx nor ever return non-nil; if they are not
required by an interface, remove the context parameter and change the return
type to void (no error), update bodies to just set s.Charge.State.AdvanceAfter
via meta.NormalizeTimestamp(s.Charge.Intent.ServicePeriod.From/To) or nil for
ClearAdvanceAfter; if they are required by an interface, keep the signatures but
add a comment explaining why ctx/error are unused or consider returning early
with nil as currently implemented.
- Around line 91-93: Update the IsInsideServicePeriod method to accept a
context.Context for API consistency with the three helper methods below: change
the signature of stateMachine.IsInsideServicePeriod to take (ctx
context.Context) and update all call sites and tests to pass the incoming
context; keep the implementation using clock.Now() unchanged (you don't need to
use ctx inside), but add the context import if missing so the package compiles
and the method signature matches the other helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b6802250-932c-415e-af12-699972616342
📒 Files selected for processing (2)
openmeter/billing/charges/flatfee/service/creditheninvoice.goopenmeter/billing/charges/flatfee/service/statemachine.go
🚧 Files skipped from review as they are similar to previous changes (1)
- openmeter/billing/charges/flatfee/service/creditheninvoice.go
2 participants
Overview
Adds flat-fee credit-then-invoice behavior so flat-fee charges begin in a created state, become active at the start of their service period, and produce invoiceable flat-fee lines at the configured invoice time.
Available customer credits are applied before any remaining fiat amount is invoiced. Fully credited charges can complete without payment settlement, while partially credited or non-credited charges remain active until invoice payment is settled.
Deleting flat-fee charges preserves billing history according to invoice mutability: pending gathering lines and mutable standard lines are removed with draft credit effects corrected, while immutable invoice lines remain in place and keep their booked ledger effects.
Notes for reviewer
Shrink and extend behavior remains out of scope for this change.
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests