feat: book invoice accrued for usagebased items#4144
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 (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds an "invoice-issued" lifecycle across billing: new invoice-issued handlers and interface methods, state-machine transitions and hooks to finalize realization runs on invoice issuance, ledger-backed invoice-usage accrual and persistence, adapter/handler/API changes, and corresponding wiring and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Invoice as Standard Invoice
participant Billing as Billing Service
participant LineEng as Line Engine
participant StateMach as Charge StateMachine
participant RunSvc as UsageRun Service
participant Handler as UsageBased Handler
participant Ledger as Ledger
participant Adapter as Usage Adapter
Invoice->>Billing: transition -> IssuingChargeBooking / Issued
Billing->>Billing: onInvoiceIssued() (group lines by engine)
Billing->>LineEng: OnInvoiceIssued(ctx, input)
LineEng->>StateMach: Fire TriggerInvoiceIssued for each line
StateMach->>StateMach: OnEntry -> FinalizeInvoiceRun / SnapshotInvoiceUsage
StateMach->>RunSvc: BookAccruedInvoiceUsage(charge, run, line)
RunSvc->>Handler: OnInvoiceUsageAccrued(input)
Handler->>Ledger: Commit ledger transaction group
Ledger-->>Handler: GroupReference{TransactionGroupID}
Handler->>Adapter: CreateRunInvoicedUsage(runID, accruedUsage + txnRef)
Adapter-->>RunSvc: persisted AccruedUsage
RunSvc-->>StateMach: success -> state advances
StateMach-->>LineEng: success
LineEng-->>Billing: success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
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 |
turip
force-pushed
the
fix/usage-based-line-id-persisting
branch
from
398ce07 to
31ebba1
Compare
turip
force-pushed
the
feat/invoice-accrued-v2
branch
from
e0be5ad to
2cb77ba
Compare
turip
added
release-note/feature
turip
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
openmeter/billing/charges/service/base_test.go (1)
272-298: Nice test helper - follows the established pattern!This implementation mirrors the approach in
TestPromotionalCreditPurchase(context snippet 3), which is great for consistency. A couple of minor observations:
Zero-length period (lines 281-284): Setting
From: now, To: nowis unusual sinceClosedPeriodtypically represents a service duration. However, promotional credits don't represent an actual service period—they're instant grants—so the zero-length period makes sense here. Consider adding a brief comment explaining this if you think it might trip up future readers.Float precision (line 280):
alpacadecimal.NewFromFloat(amount)can lose precision with certain decimal values, but for a test helper this is totally fine and keeps things convenient.Everything else looks solid—good use of
s.T().Helper(), proper context propagation, clear assertions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/service/base_test.go` around lines 272 - 298, Add a brief inline comment inside grantPromotionalCredits explaining why the ClosedPeriod uses From: now, To: now (zero-length) to represent an instant promotional credit grant rather than a service duration; leave alpacadecimal.NewFromFloat(amount) as-is for test convenience but you may note its potential precision caveat in the comment if desired.test/billing/lineengine_test.go (1)
338-338: Prefer the test-scoped context here.Using
context.Background()drops cancellation/lifecycle from the suite test.s.T().Context()keeps this flow tied to the harness like the other newer tests do.Suggested tweak
- ctx = context.Background() + ctx = s.T().Context()As per coding guidelines, "Use
t.Context()when atesting.Tortesting.TBis available instead of introducingcontext.Background()to keep cancellation and test-scoped lifecycle tied to the test harness".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/billing/lineengine_test.go` at line 338, Replace the use of context.Background() for the test-scoped context so the test lifecycle and cancellation are tied to the suite; locate the ctx variable assignment (ctx = context.Background()) in the test and change it to use the suite's test context via s.T().Context() (i.e., assign ctx = s.T().Context()) so the test inherits cancellation and timeouts from the harness.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/common/customerbalance.go`:
- Around line 44-45: The code dereferences billingRegistry.Charges when wiring
ChargesService and UsageBasedService, causing a panic if billingRegistry.Charges
is nil; add a nil-guard for billingRegistry.Charges in the provider (where
ChargesService and UsageBasedService are set) — check if billingRegistry.Charges
== nil and return a clear wiring error (or wrap/propagate the existing error
type) instead of accessing Charges.Service or Charges.UsageBasedService; update
the constructor in customerbalance.go to validate billingRegistry.Charges before
assigning ChargesService and UsageBasedService and include that guard in any
related initialization path.
In `@openmeter/billing/charges/service/invoicable_test.go`:
- Around line 1074-1086: The test
TestUsageBasedCreditThenInvoiceFullyCreditedDoesNotAccrueInvoiceUsage in
InvoicableChargesTestSuite needs to explicitly set POSTGRES_HOST=127.0.0.1
before provisioning DB-backed fixtures; add a call at the start of the test
(e.g., via s.T().Setenv("POSTGRES_HOST", "127.0.0.1")) so the DB-backed flow
used by ProvisionBillingProfile and related helpers will not be skipped in
repo-loaded environments.
In `@openmeter/billing/charges/usagebased/adapter/realizationrun.go`:
- Around line 59-60: The Ent update builder call uses a nonexistent method
SetOrClearLineID on the ChargeUsageBasedRunsUpdateOne builder; change the
conditional update in the realization run update path to call
SetNillableBillingInvoiceLineID(input.LineID.OrEmpty()) instead so it matches
the create path and the generated methods (replace SetOrClearLineID with
SetNillableBillingInvoiceLineID on the update variable).
In `@openmeter/billing/charges/usagebased/handler.go`:
- Around line 78-80: The handler currently only rejects negative amounts (if
i.Amount.IsNegative()) while OnInvoiceUsageAccruedInput.Validate() allows zero;
update the validation in handler.go so it rejects zero amounts as well to match
the handler contract — change the condition in the block that appends the error
to treat zero as invalid (e.g., check i.Amount.IsZero() or require
i.Amount.GreaterThan(decimal.Zero)), and adjust the error message if needed;
reference the same validation in OnInvoiceUsageAccruedInput.Validate and the
handler function that currently uses i.Amount.IsNegative().
In `@openmeter/billing/charges/usagebased/service/lineengine.go`:
- Around line 73-79: This branch is hard-failing when a usage-based standard
line's settlement mode isn't CreditThenInvoice, which blocks valid modes like
invoice_only; locate the check that inspects
stateMachine.GetCharge().Intent.SettlementMode alongside stdLine.ID in
lineengine.go and replace the error return with behavior that skips
standard-invoice creation for non-CreditThenInvoice modes (e.g., return nil
without an error) so those charges can follow their own lifecycle; apply the
same change to the equivalent block that uses the same settlement-mode check
later in the file (the block around the other stdLine handling).
In `@openmeter/billing/service/stdinvoicestate.go`:
- Around line 240-243: The OnActive hook on StandardInvoiceStatusIssued
(out.onInvoiceIssued) can fail but Issued has no TriggerFailed path, so errors
from FireAndActivate will escape after finalizeInvoice() runs; move the
onInvoiceIssued activation work into a retryable pre-issued state (or add a new
intermediary state like StandardInvoiceStatusIssuing with Permit(TriggerFailed,
billing.StandardInvoiceStatusIssuingFailed) and have
OnActive(out.onInvoiceIssued) on that state) so failures can transition to
IssuingSyncFailed/failed path and be retried; update stateMachine.Configure
calls to attach OnActive to the new retryable state and ensure TriggerFailed
transitions and FireAndActivate handle the new state flow.
---
Nitpick comments:
In `@openmeter/billing/charges/service/base_test.go`:
- Around line 272-298: Add a brief inline comment inside grantPromotionalCredits
explaining why the ClosedPeriod uses From: now, To: now (zero-length) to
represent an instant promotional credit grant rather than a service duration;
leave alpacadecimal.NewFromFloat(amount) as-is for test convenience but you may
note its potential precision caveat in the comment if desired.
In `@test/billing/lineengine_test.go`:
- Line 338: Replace the use of context.Background() for the test-scoped context
so the test lifecycle and cancellation are tied to the suite; locate the ctx
variable assignment (ctx = context.Background()) in the test and change it to
use the suite's test context via s.T().Context() (i.e., assign ctx =
s.T().Context()) so the test inherits cancellation and timeouts from the
harness.
🪄 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: 4b5e0bb8-9ba0-4acd-b91a-ac5000dcea27
📒 Files selected for processing (32)
app/common/charges.goapp/common/customerbalance.goopenmeter/billing/charges/creditpurchase/lineengine/engine.goopenmeter/billing/charges/flatfee/lineengine/engine.goopenmeter/billing/charges/flatfee/service.goopenmeter/billing/charges/flatfee/service/invoice.goopenmeter/billing/charges/meta/triggers.goopenmeter/billing/charges/models/invoicedusage/stdinvoice.goopenmeter/billing/charges/service/base_test.goopenmeter/billing/charges/service/handlers_test.goopenmeter/billing/charges/service/invoicable_test.goopenmeter/billing/charges/testutils/handlers.goopenmeter/billing/charges/usagebased/adapter.goopenmeter/billing/charges/usagebased/adapter/invoicedusage.goopenmeter/billing/charges/usagebased/adapter/realizationrun.goopenmeter/billing/charges/usagebased/handler.goopenmeter/billing/charges/usagebased/realizationrun.goopenmeter/billing/charges/usagebased/service/creditheninvoice.goopenmeter/billing/charges/usagebased/service/creditsonly.goopenmeter/billing/charges/usagebased/service/lineengine.goopenmeter/billing/charges/usagebased/service/run/correct.goopenmeter/billing/charges/usagebased/service/run/create.goopenmeter/billing/charges/usagebased/service/run/invoice.goopenmeter/billing/lineengine.goopenmeter/billing/lineengine/engine.goopenmeter/billing/service/gatheringinvoicependinglines.goopenmeter/billing/service/stdinvoicestate.goopenmeter/ledger/chargeadapter/usagebased.goopenmeter/ledger/chargeadapter/usagebased_test.gotest/billing/lineengine_test.gotest/billing/suite.gotest/credits/sanity_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
openmeter/ledger/chargeadapter/usagebased_test.go (1)
179-192: Consider adding test coverage for the non-zero amount (happy path).This test validates the zero-amount short-circuit nicely, but the happy path where
OnInvoiceUsageAccruedactually commits a ledger transaction group isn't covered yet. Given that the caller (BookAccruedInvoiceUsage) returns an error ifTransactionGroupIDis empty for non-zero amounts (seeopenmeter/billing/charges/usagebased/service/run/invoice.go:78), having a test that verifies the full commit flow would add confidence.If this is intentional scaffolding for a future PR, feel free to leave a TODO or ignore this suggestion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/chargeadapter/usagebased_test.go` around lines 179 - 192, Add a happy-path subtest to TestOnUsageBasedInvoiceUsageAccrued that calls env.handler.OnInvoiceUsageAccrued with a non-zero Amount (instead of alpacadecimal.Zero) for the CreditThenInvoice settlement mode, then assert no error and that the returned ref.TransactionGroupID is non-empty; also verify any expected ledger commit behavior (e.g., that the ledger/transaction group was created/committed or mocked commit method called) so this covers the flow that BookAccruedInvoiceUsage relies on (see BookAccruedInvoiceUsage in invoice.go and the TransactionGroupID contract).openmeter/billing/charges/service/invoicable_test.go (1)
279-280: Small consistency note on context usage.This test uses
context.Background()while the newer test at line 1188 usess.T().Context(). Consider updating tos.T().Context()for consistency with the preferred approach that ties the context lifecycle to the test harness.✨ Optional fix
func (s *InvoicableChargesTestSuite) TestFlatFeeCreditThenInvoiceFullyCreditedDoesNotAccrueInvoiceUsage() { - ctx := context.Background() + ctx := s.T().Context() ns := s.GetUniqueNamespace("charges-service-flatfee-credit-then-invoice-fully-credited")As per coding guidelines, "Use
t.Context()when atesting.Tortesting.TBis available instead of introducingcontext.Background()to keep cancellation and test-scoped lifecycle tied to the test harness".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/service/invoicable_test.go` around lines 279 - 280, Replace the use of context.Background() in the test function TestFlatFeeCreditThenInvoiceFullyCreditedDoesNotAccrueInvoiceUsage with the test-scoped context s.T().Context(): locate the test function (TestFlatFeeCreditThenInvoiceFullyCreditedDoesNotAccrueInvoiceUsage) in invoicable_test.go and change ctx := context.Background() to ctx := s.T().Context() so the test uses the testing harness' lifecycle-aware context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@openmeter/billing/charges/service/invoicable_test.go`:
- Around line 279-280: Replace the use of context.Background() in the test
function TestFlatFeeCreditThenInvoiceFullyCreditedDoesNotAccrueInvoiceUsage with
the test-scoped context s.T().Context(): locate the test function
(TestFlatFeeCreditThenInvoiceFullyCreditedDoesNotAccrueInvoiceUsage) in
invoicable_test.go and change ctx := context.Background() to ctx :=
s.T().Context() so the test uses the testing harness' lifecycle-aware context.
In `@openmeter/ledger/chargeadapter/usagebased_test.go`:
- Around line 179-192: Add a happy-path subtest to
TestOnUsageBasedInvoiceUsageAccrued that calls env.handler.OnInvoiceUsageAccrued
with a non-zero Amount (instead of alpacadecimal.Zero) for the CreditThenInvoice
settlement mode, then assert no error and that the returned
ref.TransactionGroupID is non-empty; also verify any expected ledger commit
behavior (e.g., that the ledger/transaction group was created/committed or
mocked commit method called) so this covers the flow that
BookAccruedInvoiceUsage relies on (see BookAccruedInvoiceUsage in invoice.go and
the TransactionGroupID contract).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ac4e404d-84e8-407f-ba9e-0157b39134f4
⛔ Files ignored due to path filters (2)
openmeter/ent/db/billinginvoice/billinginvoice.gois excluded by!**/ent/db/**openmeter/ent/db/migrate/schema.gois excluded by!**/ent/db/**
📒 Files selected for processing (9)
.agents/skills/charges/SKILL.mdopenmeter/billing/adapter/invoice.goopenmeter/billing/charges/flatfee/service/invoice.goopenmeter/billing/charges/service/invoicable_test.goopenmeter/billing/service/stdinvoicestate.goopenmeter/billing/stdinvoice.goopenmeter/ledger/chargeadapter/usagebased.goopenmeter/ledger/chargeadapter/usagebased_test.gotest/billing/lineengine_test.go
✅ Files skipped from review due to trivial changes (2)
- .agents/skills/charges/SKILL.md
- test/billing/lineengine_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- openmeter/billing/charges/flatfee/service/invoice.go
GAlexIHU
approved these changes
Apr 16, 2026
| Namespace: input.Charge.Namespace, | ||
| ID: input.Charge.Intent.CustomerID, | ||
| } | ||
| annotations := ledger.ChargeAnnotations(models.NamespacedID{ |
Contributor
There was a problem hiding this comment.
nit: we have helpers for these in annotations.go lets use those or create one there if not present
This was referenced Apr 16, 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
Fixes #(issue)
Notes for reviewer
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests