Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughAdds intermediary payment booking/authorization/settlement states and TriggerAuthorized, wires a PaymentAuthorized service endpoint, adds OnPaymentAuthorized/OnPaymentSettled line‑engine hooks (no‑ops in implementations), updates app-usage detection, expands tests, and documents lifecycle/retry/advance contracts. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant InvoiceService as InvoiceService
participant StateMachine as StateMachine
participant LineEngine as LineEngine
Client->>InvoiceService: PaymentAuthorized / TriggerAuthorized
activate InvoiceService
InvoiceService->>StateMachine: FireAndActivate(TriggerAuthorized)
activate StateMachine
StateMachine->>StateMachine: Transition → payment_processing.booking_authorized
StateMachine->>LineEngine: OnPaymentAuthorized(input)
activate LineEngine
LineEngine-->>StateMachine: (error / nil)
deactivate LineEngine
alt success
StateMachine->>StateMachine: Transition → payment_processing.authorized
else failure
StateMachine->>StateMachine: Transition → payment_processing.booking_authorized_failed
end
Note over StateMachine,LineEngine: Optional booking_authorized_and_settled / booking_settled flows
StateMachine->>LineEngine: OnPaymentSettled(input)
activate LineEngine
LineEngine-->>StateMachine: (error / nil)
deactivate LineEngine
alt success
StateMachine->>StateMachine: Transition → paid (final)
else failure
StateMachine->>StateMachine: Transition → payment_processing.booking_settled_failed
end
StateMachine-->>InvoiceService: updated invoice
deactivate StateMachine
InvoiceService-->>Client: StandardInvoice
deactivate InvoiceService
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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
feat/payment-settlement
branch
from
60c7866 to
0e65dc7
Compare
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openmeter/billing/charges/service/creditpurchase_test.go (1)
479-594:⚠️ Potential issue | 🟡 MinorPlease keep a dedicated test for the explicit
PaymentAuthorizedpath.This now only covers the pending → paid shortcut, so a regression in the new standalone
PaymentProcessingAuthorizedflow for invoice-settled credit purchases would slip through here unnoticed. As per coding guidelines,**/*_test.go: "Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/service/creditpurchase_test.go` around lines 479 - 594, The test suite removed a focused test for the explicit PaymentAuthorized path; restore a dedicated unit/integration test that exercises the standalone PaymentAuthorized flow (distinct from the pending→paid shortcut) by wiring CreditPurchaseTestHandler.onCreditPurchasePaymentAuthorized and asserting it is invoked (and that no InvoiceSettlement is created) when the charge enters the PaymentProcessingAuthorized state; use the existing helpers and calls such as BillingService.ApproveInvoice, CreditPurchaseTestHandler.onCreditPurchasePaymentAuthorized, PaymentProcessingAuthorized/PaymentAuthorized states, and mustGetChargeByID/AsCreditPurchaseCharge to validate the authorized callback, the Realizations (CreditGrantRealization present, InvoiceSettlement nil), and charge Status, separate from the settled test that uses HandlePaymentTrigger.
🧹 Nitpick comments (3)
openmeter/billing/adapter/invoice.go (1)
845-863: Status list extension looks right. ✅All the new non-final payment-processing substates are correctly added so
IsAppUsedtreats invoices parked in booking/authorization intermediates as still referencing the app. Matches thefailedStatuses/validStatusesadditions instdinvoice.go.Tiny nit (totally optional): this
StatusIn(...)list is getting long and is the kind of thing that's easy to forget to update when a new substate lands. Could be worth extracting a package-levelnonFinalInvoiceStatusesslice (or deriving it asall \ finalStatuses) so there's a single source of truth. Not for this PR though.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/adapter/invoice.go` around lines 845 - 863, The StatusIn(...) call in invoice.go is getting long and error-prone; extract the repeated list of non-final invoice statuses into a single package-level slice (e.g., nonFinalInvoiceStatuses) and replace the inline billinginvoice.StatusIn(...) arguments with billinginvoice.StatusIn(nonFinalInvoiceStatuses...) (or derive it as allStatuses minus finalStatuses if preferable); update any other places that use the same set (e.g., IsAppUsed-related checks) to reference this single slice so future substate additions only need one change..agents/skills/billing/SKILL.md (1)
233-234: Tiny formatting nit: missing blank line before the next##heading.Line 233 ends the "Current test coverage pattern" paragraph and line 234 immediately starts
## Gathering vs Standard Invoices. Most markdown renderers tolerate this, but a blank line keeps things tidy and matches the style elsewhere in the file. Same situation at line 494 → 495 right before## References.✏️ Suggested tweak
Use those tests as the template for new billing line-engine lifecycle behavior. + ## Gathering vs Standard Invoices- **Payment/issuing side-effect hooks are not line-mutating hooks**: `OnInvoiceIssued`, `OnPaymentAuthorized`, and `OnPaymentSettled` return only `error`. If you need to mutate invoice lines, do it earlier in `OnStandardInvoiceCreated` or `OnCollectionCompleted`. + ## References🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/billing/SKILL.md around lines 233 - 234, Add a blank line before the "## Gathering vs Standard Invoices" heading and another blank line before the "## References" heading to separate them from the preceding paragraph ("Current test coverage pattern") and match the file's markdown style; edit .agents/skills/billing/SKILL.md to insert an empty line immediately above the "## Gathering vs Standard Invoices" and above the "## References" headings.openmeter/billing/charges/service/invoicable_test.go (1)
225-282: Tiny nit: assertion styles are split between the two new callbacks.The
onPaymentAuthorizedcallback in the approve-invoice subtest usess.Require()/s.Nil/s.Equal(suite, fail-fast), while theonPaymentAuthorized/onPaymentSettledcallbacks in the trigger-paid subtest useassert.*with the*testing.Tclosure parameter (non-fail-fast). Both work, but using one style would read a bit more uniformly — and since these callbacks fire from inside service calls,assert.*(what you picked in the second subtest) is arguably the safer default so a failing assertion doesn't halt the goroutine unexpectedly. Totally optional though.🤖 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 225 - 282, The test mixes suite-style assertions and testing.T assertions across callbacks; make them consistent by changing the callbacks in the approve-invoice subtest to use the same testing.T-based assert style as the trigger-paid subtest. Specifically, update the handler passed into s.FlatFeeTestHandler.onPaymentAuthorized (and the invoiceUsageAccrued callback if it uses suite assertions) so it uses the newCountedLedgerTransactionCallback.Handler signature with func(t *testing.T, ...) and assert.* calls instead of s.Require()/s.Nil()/s.Equal(), keeping the same expectation checks and referencing authorizedCallback.Handler and invoiceUsageAccruedCallback.Handler to locate the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/billing/lineengine_test.go`:
- Around line 239-273: The two helper methods markInvoicePaid and
markInvoiceAuthorized currently hardcode app.AppTypeSandbox when calling
BillingService.TriggerInvoice; update them to use the configured app type used
by the test setup (e.g., the app value provided by
createMeteredDraftInvoiceWaitingForCollectionForApp or the suite field that
stores the test App) so triggers run against the correct payment app; locate the
TriggerInvoice calls inside markInvoicePaid and markInvoiceAuthorized and
replace app.AppTypeSandbox with the suite/configured app type (the same value
used when provisioning SetupCustomInvoicing(...).App).
---
Outside diff comments:
In `@openmeter/billing/charges/service/creditpurchase_test.go`:
- Around line 479-594: The test suite removed a focused test for the explicit
PaymentAuthorized path; restore a dedicated unit/integration test that exercises
the standalone PaymentAuthorized flow (distinct from the pending→paid shortcut)
by wiring CreditPurchaseTestHandler.onCreditPurchasePaymentAuthorized and
asserting it is invoked (and that no InvoiceSettlement is created) when the
charge enters the PaymentProcessingAuthorized state; use the existing helpers
and calls such as BillingService.ApproveInvoice,
CreditPurchaseTestHandler.onCreditPurchasePaymentAuthorized,
PaymentProcessingAuthorized/PaymentAuthorized states, and
mustGetChargeByID/AsCreditPurchaseCharge to validate the authorized callback,
the Realizations (CreditGrantRealization present, InvoiceSettlement nil), and
charge Status, separate from the settled test that uses HandlePaymentTrigger.
---
Nitpick comments:
In @.agents/skills/billing/SKILL.md:
- Around line 233-234: Add a blank line before the "## Gathering vs Standard
Invoices" heading and another blank line before the "## References" heading to
separate them from the preceding paragraph ("Current test coverage pattern") and
match the file's markdown style; edit .agents/skills/billing/SKILL.md to insert
an empty line immediately above the "## Gathering vs Standard Invoices" and
above the "## References" headings.
In `@openmeter/billing/adapter/invoice.go`:
- Around line 845-863: The StatusIn(...) call in invoice.go is getting long and
error-prone; extract the repeated list of non-final invoice statuses into a
single package-level slice (e.g., nonFinalInvoiceStatuses) and replace the
inline billinginvoice.StatusIn(...) arguments with
billinginvoice.StatusIn(nonFinalInvoiceStatuses...) (or derive it as allStatuses
minus finalStatuses if preferable); update any other places that use the same
set (e.g., IsAppUsed-related checks) to reference this single slice so future
substate additions only need one change.
In `@openmeter/billing/charges/service/invoicable_test.go`:
- Around line 225-282: The test mixes suite-style assertions and testing.T
assertions across callbacks; make them consistent by changing the callbacks in
the approve-invoice subtest to use the same testing.T-based assert style as the
trigger-paid subtest. Specifically, update the handler passed into
s.FlatFeeTestHandler.onPaymentAuthorized (and the invoiceUsageAccrued callback
if it uses suite assertions) so it uses the
newCountedLedgerTransactionCallback.Handler signature with func(t *testing.T,
...) and assert.* calls instead of s.Require()/s.Nil()/s.Equal(), keeping the
same expectation checks and referencing authorizedCallback.Handler and
invoiceUsageAccruedCallback.Handler to locate the code.
🪄 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: 52351c01-b753-4705-85ae-3122fc897273
⛔ 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 (17)
.agents/skills/billing/SKILL.mdopenmeter/billing/adapter/invoice.goopenmeter/billing/charges/creditpurchase/lineengine/engine.goopenmeter/billing/charges/flatfee/lineengine/engine.goopenmeter/billing/charges/service/creditpurchase_test.goopenmeter/billing/charges/service/invoicable_test.goopenmeter/billing/charges/service/invoice.goopenmeter/billing/charges/usagebased/service/lineengine.goopenmeter/billing/lineengine.goopenmeter/billing/lineengine/engine.goopenmeter/billing/service.goopenmeter/billing/service/invoice.goopenmeter/billing/service/stdinvoicestate.goopenmeter/billing/stdinvoice.goopenmeter/billing/stdinvoicestate.gotest/billing/lineengine_test.gotest/credits/sanity_test.go
Member
Author
|
Credit Pruchase will receive full support for lineengine later, then we will add authorization support. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/billing/lineengine_test.go (2)
969-977: Redundant assertion after the hard equality check.Line 969 already asserts the exact status is
PaymentProcessingBookingAuthorizedAndSettledFailed, so thes.Contains([...two statuses...], invoice.Status)right after can never add coverage — it's dead weight (and if the first ever flakes, the second is moot). Either drop theEqualand keepContains(if either terminal-failed status is actually valid here), or drop theContains. My hunch is that since the sandbox path is deterministic here, just keep theEqual.♻️ Suggested cleanup
s.Equal(ombilling.StandardInvoiceStatusPaymentProcessingBookingAuthorizedAndSettledFailed, invoice.Status) - - s.Contains( - []ombilling.StandardInvoiceStatus{ - ombilling.StandardInvoiceStatusPaymentProcessingBookingAuthorizedAndSettledFailed, - ombilling.StandardInvoiceStatusPaymentProcessingBookingSettledFailed, - }, - invoice.Status, - ) s.True(invoice.StatusDetails.Failed)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/billing/lineengine_test.go` around lines 969 - 977, The test currently makes a hard equality assertion s.Equal(ombilling.StandardInvoiceStatusPaymentProcessingBookingAuthorizedAndSettledFailed, invoice.Status) and then immediately calls s.Contains([...], invoice.Status), which is redundant; remove the s.Contains(...) block that references ombilling.StandardInvoiceStatusPaymentProcessingBookingAuthorizedAndSettledFailed and ombilling.StandardInvoiceStatusPaymentProcessingBookingSettledFailed so the test only asserts the deterministic expected status via s.Equal.
495-498: Tiny nit: duplicatePaidassertion.Line 497 re-asserts the same
StandardInvoiceStatusPaidcheck that already happened on line 495 with the sameinvoicevalue in between — safe to delete one for clarity.♻️ Suggested cleanup
s.Equal(ombilling.StandardInvoiceStatusPaid, invoice.Status) s.Equal(1, onInvoiceIssuedCnt) - s.Equal(ombilling.StandardInvoiceStatusPaid, invoice.Status)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/billing/lineengine_test.go` around lines 495 - 498, Remove the duplicate assertion of the invoice status: delete the second s.Equal(ombilling.StandardInvoiceStatusPaid, invoice.Status) so the test only asserts invoice.Status once (keep the check and the onInvoiceIssuedCnt assertion), ensuring only one StandardInvoiceStatusPaid assertion remains for the invoice variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/billing/lineengine_test.go`:
- Around line 239-257: The helper function markInvoicePaid is defined but
unused; either remove it or wire it into the relevant test flow (e.g., call
markInvoicePaid from TestOnPaymentSettledIsCalled or the test that needs the
sandbox-paid transition) so it is exercised when you refactor to the custom
invoicing app; locate the markInvoicePaid function and either delete it if
unused or add a call to markInvoicePaid(ctx, invoiceID) in
TestOnPaymentSettledIsCalled (or the appropriate test) after obtaining the
invoice ID and before asserting settled behavior, leveraging
mustGetInvoiceAppType as already implemented.
---
Nitpick comments:
In `@test/billing/lineengine_test.go`:
- Around line 969-977: The test currently makes a hard equality assertion
s.Equal(ombilling.StandardInvoiceStatusPaymentProcessingBookingAuthorizedAndSettledFailed,
invoice.Status) and then immediately calls s.Contains([...], invoice.Status),
which is redundant; remove the s.Contains(...) block that references
ombilling.StandardInvoiceStatusPaymentProcessingBookingAuthorizedAndSettledFailed
and ombilling.StandardInvoiceStatusPaymentProcessingBookingSettledFailed so the
test only asserts the deterministic expected status via s.Equal.
- Around line 495-498: Remove the duplicate assertion of the invoice status:
delete the second s.Equal(ombilling.StandardInvoiceStatusPaid, invoice.Status)
so the test only asserts invoice.Status once (keep the check and the
onInvoiceIssuedCnt assertion), ensuring only one StandardInvoiceStatusPaid
assertion remains for the invoice variable.
🪄 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: 0a96dc7f-24ac-4a7f-b51e-407e3beca82f
📒 Files selected for processing (1)
test/billing/lineengine_test.go
turip
force-pushed
the
feat/payment-settlement
branch
from
65f548b to
0b63d60
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.agents/skills/billing/SKILL.md (1)
175-175: Tiny wording tweak for readability.At Line 175, “exact same” can be shortened to “same” without losing meaning.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/skills/billing/SKILL.md at line 175, Replace the phrase "exact same" with "same" in the line that currently reads "must return standard lines reusing the exact same line IDs as the input gathering lines" so it becomes "must return standard lines reusing the same line IDs as the input gathering lines"; locate this text in SKILL.md (the line containing "must return standard lines reusing the exact same line IDs") and perform the edit.openmeter/billing/charges/service/invoicable_test.go (1)
264-282: Minor: callback assertions switch from suite-style to package-levelassert— consider picking one.The
approve invoice accrues usage without authorizing paymentsubtest (lines 225-229) uses suite-bound helpers (s.Require().NotNil,s.Nil,s.Equal), while thetrigger paid authorizes then settles paymentcallbacks (lines 268-282) switch toassert.Nil(t, ...)/assert.NotNil(t, ...)/assert.Equal(t, ...). Both work, but mixing them in the same test makes the file a little harder to scan. Sincesis in scope in these closures too, sticking with the suite-bound style would keep things uniform and give you the nicer "failed assertion shows up on the test" behavior without threadingtaround.♻️ Example normalization
- authorizedCallback := newCountedLedgerTransactionCallback[flatfee.Charge]() - s.FlatFeeTestHandler.onPaymentAuthorized = authorizedCallback.Handler(s.T(), func(t *testing.T, charge flatfee.Charge) { - assert.Nil(t, charge.Realizations.Payment) - assert.NotNil(t, charge.Realizations.AccruedUsage) - assert.Equal(t, flatfee.StatusActive, charge.Status) - }) - - settledCallback := newCountedLedgerTransactionCallback[flatfee.Charge]() - s.FlatFeeTestHandler.onPaymentSettled = settledCallback.Handler(s.T(), func(t *testing.T, charge flatfee.Charge) { - assert.NotNil(t, charge.Realizations.Payment) - assert.NotNil(t, charge.Realizations.Payment.Authorized) - assert.Nil(t, charge.Realizations.Payment.Settled) - assert.Equal(t, authorizedCallback.id, charge.Realizations.Payment.Authorized.TransactionGroupID) - assert.Equal(t, payment.StatusAuthorized, charge.Realizations.Payment.Status) - assert.Equal(t, flatfee.StatusActive, charge.Status) - }) + authorizedCallback := newCountedLedgerTransactionCallback[flatfee.Charge]() + s.FlatFeeTestHandler.onPaymentAuthorized = authorizedCallback.Handler(s.T(), func(t *testing.T, charge flatfee.Charge) { + s.Nil(charge.Realizations.Payment) + s.NotNil(charge.Realizations.AccruedUsage) + s.Equal(flatfee.StatusActive, charge.Status) + }) + + settledCallback := newCountedLedgerTransactionCallback[flatfee.Charge]() + s.FlatFeeTestHandler.onPaymentSettled = settledCallback.Handler(s.T(), func(t *testing.T, charge flatfee.Charge) { + s.Require().NotNil(charge.Realizations.Payment) + s.NotNil(charge.Realizations.Payment.Authorized) + s.Nil(charge.Realizations.Payment.Settled) + s.Equal(authorizedCallback.id, charge.Realizations.Payment.Authorized.TransactionGroupID) + s.Equal(payment.StatusAuthorized, charge.Realizations.Payment.Status) + s.Equal(flatfee.StatusActive, charge.Status) + })If you go this route, the
assertimport on line 12 can also be dropped.🤖 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 264 - 282, The callbacks attached to s.FlatFeeTestHandler (authorizedCallback.Handler and settledCallback.Handler) use package-level assert.* (assert.Nil, assert.NotNil, assert.Equal) while the surrounding tests use the suite-bound s.Require()/s.Assertions; update the callback closures to use s.Require().Nil/NotNil/Equal (or s.Assertions.*) and reference the suite via the outer s rather than threading t through, and then remove the now-unused "assert" import from the file; target the closures created by newCountedLedgerTransactionCallback and the onPaymentAuthorized/onPaymentSettled handlers when making this change.openmeter/billing/charges/service/invoice.go (1)
55-67: Merge the identical branches using a shared case label.The
PaymentProcessingAuthorizedandPaymentProcessingBookingAuthorizedAndSettledcases dispatch identical processors and can be combined for clarity:♻️ Suggested merge
- case billing.StandardInvoiceStatusPaymentProcessingAuthorized: - return s.handleChargeEvent(ctx, invoice, processorByType{ - flatFee: s.flatFeeService.PostInvoicePaymentAuthorized, - usageBased: noop[usagebased.Charge], - creditPurchase: s.creditPurchaseService.PostInvoicePaymentAuthorized, - }) - - case billing.StandardInvoiceStatusPaymentProcessingBookingAuthorizedAndSettled: + case billing.StandardInvoiceStatusPaymentProcessingAuthorized, + billing.StandardInvoiceStatusPaymentProcessingBookingAuthorizedAndSettled: return s.handleChargeEvent(ctx, invoice, processorByType{ flatFee: s.flatFeeService.PostInvoicePaymentAuthorized, usageBased: noop[usagebased.Charge], creditPurchase: s.creditPurchaseService.PostInvoicePaymentAuthorized, })The
BookingAuthorizedAndSettledstate's settlement is already handled via the state machine'sOnActivehandler (onPaymentAuthorizedAndSettled), which explicitly calls bothonPaymentAuthorizedandonPaymentSettledbefore the charge event dispatch. This is by design to keep charge-side ledger booking consistent when payment apps jump directly from pending to paid.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/service/invoice.go` around lines 55 - 67, Two switch branches for billing.StandardInvoiceStatusPaymentProcessingAuthorized and billing.StandardInvoiceStatusPaymentProcessingBookingAuthorizedAndSettled dispatch identical processorByType handlers; merge them into a single case label to avoid duplication by combining the two case lines into one (e.g. case billing.StandardInvoiceStatusPaymentProcessingAuthorized, billing.StandardInvoiceStatusPaymentProcessingBookingAuthorizedAndSettled:) and keep the existing call to s.handleChargeEvent(ctx, invoice, processorByType{ flatFee: s.flatFeeService.PostInvoicePaymentAuthorized, usageBased: noop[usagebased.Charge], creditPurchase: s.creditPurchaseService.PostInvoicePaymentAuthorized }) so handleChargeEvent, processorByType, flatFeeService.PostInvoicePaymentAuthorized and creditPurchaseService.PostInvoicePaymentAuthorized remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.agents/skills/billing/SKILL.md:
- Line 175: Replace the phrase "exact same" with "same" in the line that
currently reads "must return standard lines reusing the exact same line IDs as
the input gathering lines" so it becomes "must return standard lines reusing the
same line IDs as the input gathering lines"; locate this text in SKILL.md (the
line containing "must return standard lines reusing the exact same line IDs")
and perform the edit.
In `@openmeter/billing/charges/service/invoicable_test.go`:
- Around line 264-282: The callbacks attached to s.FlatFeeTestHandler
(authorizedCallback.Handler and settledCallback.Handler) use package-level
assert.* (assert.Nil, assert.NotNil, assert.Equal) while the surrounding tests
use the suite-bound s.Require()/s.Assertions; update the callback closures to
use s.Require().Nil/NotNil/Equal (or s.Assertions.*) and reference the suite via
the outer s rather than threading t through, and then remove the now-unused
"assert" import from the file; target the closures created by
newCountedLedgerTransactionCallback and the onPaymentAuthorized/onPaymentSettled
handlers when making this change.
In `@openmeter/billing/charges/service/invoice.go`:
- Around line 55-67: Two switch branches for
billing.StandardInvoiceStatusPaymentProcessingAuthorized and
billing.StandardInvoiceStatusPaymentProcessingBookingAuthorizedAndSettled
dispatch identical processorByType handlers; merge them into a single case label
to avoid duplication by combining the two case lines into one (e.g. case
billing.StandardInvoiceStatusPaymentProcessingAuthorized,
billing.StandardInvoiceStatusPaymentProcessingBookingAuthorizedAndSettled:) and
keep the existing call to s.handleChargeEvent(ctx, invoice, processorByType{
flatFee: s.flatFeeService.PostInvoicePaymentAuthorized, usageBased:
noop[usagebased.Charge], creditPurchase:
s.creditPurchaseService.PostInvoicePaymentAuthorized }) so handleChargeEvent,
processorByType, flatFeeService.PostInvoicePaymentAuthorized and
creditPurchaseService.PostInvoicePaymentAuthorized remain unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2ad9d4e8-63db-44fb-99c1-b4fcdf0df3c4
⛔ 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 (17)
.agents/skills/billing/SKILL.mdopenmeter/billing/adapter/invoice.goopenmeter/billing/charges/creditpurchase/lineengine/engine.goopenmeter/billing/charges/flatfee/lineengine/engine.goopenmeter/billing/charges/service/creditpurchase_test.goopenmeter/billing/charges/service/invoicable_test.goopenmeter/billing/charges/service/invoice.goopenmeter/billing/charges/usagebased/service/lineengine.goopenmeter/billing/lineengine.goopenmeter/billing/lineengine/engine.goopenmeter/billing/service.goopenmeter/billing/service/invoice.goopenmeter/billing/service/stdinvoicestate.goopenmeter/billing/stdinvoice.goopenmeter/billing/stdinvoicestate.gotest/billing/lineengine_test.gotest/credits/sanity_test.go
✅ Files skipped from review due to trivial changes (2)
- openmeter/billing/stdinvoicestate.go
- test/credits/sanity_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- openmeter/billing/service.go
- openmeter/billing/adapter/invoice.go
- openmeter/billing/charges/flatfee/lineengine/engine.go
- openmeter/billing/lineengine/engine.go
- openmeter/billing/lineengine.go
- openmeter/billing/stdinvoice.go
- openmeter/billing/charges/creditpurchase/lineengine/engine.go
- openmeter/billing/charges/usagebased/service/lineengine.go
- openmeter/billing/charges/service/creditpurchase_test.go
- test/billing/lineengine_test.go
- openmeter/billing/service/stdinvoicestate.go
turip
added
release-note/feature
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
openmeter/billing/charges/service/invoicable_test.go (1)
281-288: Tiny nil-safety heads-up on the settled-hook assertions.
assert.NotNilis non-fatal, so ifcharge.Realizations.Paymentever ends up nil here (regression later), the next lines (charge.Realizations.Payment.Authorized,.Status) will nil-panic instead of giving you a clean assertion failure. Usingrequire/s.Require()for theNotNilchecks (or an early-return guard) would make future breakage report much more cleanly. Not a blocker — just a future-you kindness.🤖 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 281 - 288, The settled-hook assertions in s.FlatFeeTestHandler.onPaymentSettled use assert.NotNil which is non-fatal and can lead to nil-panics when accessing charge.Realizations.Payment.Authorized or .Status; change those initial null checks to fatal assertions (use require.NotNil or s.Require().NotNil) for charge.Realizations.Payment and charge.Realizations.Payment.Authorized, or add an immediate guard/early return after checking charge.Realizations.Payment to prevent subsequent dereferences in the settledCallback.Handler(s.T(), ...) closure.test/billing/lineengine_test.go (2)
494-498: Duplicate status assertion.Lines 495 and 497 both assert
invoice.Status == StandardInvoiceStatusPaidwith the sameinvoicevalue in between — only the intervenings.Equal(1, onInvoiceIssuedCnt)runs. You can drop one of them.🧹 Proposed cleanup
invoice, err = s.BillingService.ApproveInvoice(ctx, invoice.GetInvoiceID()) s.Require().NoError(err) s.Equal(ombilling.StandardInvoiceStatusPaid, invoice.Status) s.Equal(1, onInvoiceIssuedCnt) - s.Equal(ombilling.StandardInvoiceStatusPaid, invoice.Status) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/billing/lineengine_test.go` around lines 494 - 498, The test contains a duplicated assertion checking invoice.Status equals ombilling.StandardInvoiceStatusPaid (the two s.Equal calls around the onInvoiceIssuedCnt check); remove one of the redundant s.Equal(ombilling.StandardInvoiceStatusPaid, invoice.Status) assertions so the test still verifies invoice.Status once while keeping the s.Equal(1, onInvoiceIssuedCnt) check intact.
967-982: Overlapping status assertions read a bit muddled.Line 969 asserts the exact status
PaymentProcessingBookingAuthorizedAndSettledFailed, then lines 971–977 immediately assertContains(...)over the two *Failed variants. If the hardEqualpasses, theContainsis redundant; if it fails, both assertions fire and the output is harder to read. Probably cleaner to pick one: either trust that the sandbox/custom-invoicing setup always lands in the combined-settled-failed status and keep theEqual, or drop theEqualand keep theContainsas defensive coverage for both code paths (similar to whatTestOnPaymentSettledIsCalleddoes).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/billing/lineengine_test.go` around lines 967 - 982, The test currently asserts invoice.Status twice (first with s.Equal for ombilling.StandardInvoiceStatusPaymentProcessingBookingAuthorizedAndSettledFailed and then s.Contains over a slice with that value and ombilling.StandardInvoiceStatusPaymentProcessingBookingSettledFailed), which is redundant and makes failures noisy; pick one approach: either remove the s.Contains block and keep the strict s.Equal check on invoice.Status (if the sandbox always yields the combined-settled-failed status), or remove the s.Equal and keep the s.Contains check to defensively accept either failed variant (also update or add a short comment above the chosen assertion to explain the expectation); adjust only the assertions around invoice.Status (in the test where s.BillingService.ApproveInvoice is called) and leave the subsequent checks on StatusDetails, AvailableActions, and ValidationIssues unchanged.openmeter/billing/charges/service/invoice.go (1)
55-70: Tiny DRY nit: both branches dispatch the same processor set.The
PaymentProcessingAuthorizedandPaymentProcessingBookingAuthorizedAndSettledcases wire up identicalprocessorByTypevalues, so they can share a single dispatch. Totally optional given the existing "temporary hack" comment already signals this will be replaced by line-engine dispatch — happy to leave it as-is if you'd rather not touch it.♻️ Optional refactor to share the processor set
- case billing.StandardInvoiceStatusPaymentProcessingAuthorized: - return s.handleChargeEvent(ctx, invoice, processorByType{ - flatFee: s.flatFeeService.PostInvoicePaymentAuthorized, - usageBased: noop[usagebased.Charge], - creditPurchase: s.creditPurchaseService.PostInvoicePaymentAuthorized, - }) - - // Temporary hack to handle the case where the invoice is authorized and settled in one go - // This should be removed once we transition to the line-engine based invocation of hooks in all - // charge types. - case billing.StandardInvoiceStatusPaymentProcessingBookingAuthorizedAndSettled: + // The BookingAuthorizedAndSettled branch is a temporary hack for apps that jump straight + // from pending to paid; it should go away once all charge types flow through line-engine hooks. + case billing.StandardInvoiceStatusPaymentProcessingAuthorized, + billing.StandardInvoiceStatusPaymentProcessingBookingAuthorizedAndSettled: return s.handleChargeEvent(ctx, invoice, processorByType{ flatFee: s.flatFeeService.PostInvoicePaymentAuthorized, usageBased: noop[usagebased.Charge], creditPurchase: s.creditPurchaseService.PostInvoicePaymentAuthorized, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/service/invoice.go` around lines 55 - 70, Both switch/case branches for billing.StandardInvoiceStatusPaymentProcessingAuthorized and billing.StandardInvoiceStatusPaymentProcessingBookingAuthorizedAndSettled create the exact same processorByType and call s.handleChargeEvent; extract the shared processorByType construction into a single variable and reuse it for both cases (or collapse the two cases to fall through to one handler). Update the switch to either declare a variable like processors := processorByType{ flatFee: s.flatFeeService.PostInvoicePaymentAuthorized, usageBased: noop[usagebased.Charge], creditPurchase: s.creditPurchaseService.PostInvoicePaymentAuthorized } and pass processors to s.handleChargeEvent for both cases, or make the second case fall through to the first to avoid duplication while keeping behavior of s.handleChargeEvent unchanged.
🤖 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 281-288: The settled-hook assertions in
s.FlatFeeTestHandler.onPaymentSettled use assert.NotNil which is non-fatal and
can lead to nil-panics when accessing charge.Realizations.Payment.Authorized or
.Status; change those initial null checks to fatal assertions (use
require.NotNil or s.Require().NotNil) for charge.Realizations.Payment and
charge.Realizations.Payment.Authorized, or add an immediate guard/early return
after checking charge.Realizations.Payment to prevent subsequent dereferences in
the settledCallback.Handler(s.T(), ...) closure.
In `@openmeter/billing/charges/service/invoice.go`:
- Around line 55-70: Both switch/case branches for
billing.StandardInvoiceStatusPaymentProcessingAuthorized and
billing.StandardInvoiceStatusPaymentProcessingBookingAuthorizedAndSettled create
the exact same processorByType and call s.handleChargeEvent; extract the shared
processorByType construction into a single variable and reuse it for both cases
(or collapse the two cases to fall through to one handler). Update the switch to
either declare a variable like processors := processorByType{ flatFee:
s.flatFeeService.PostInvoicePaymentAuthorized, usageBased:
noop[usagebased.Charge], creditPurchase:
s.creditPurchaseService.PostInvoicePaymentAuthorized } and pass processors to
s.handleChargeEvent for both cases, or make the second case fall through to the
first to avoid duplication while keeping behavior of s.handleChargeEvent
unchanged.
In `@test/billing/lineengine_test.go`:
- Around line 494-498: The test contains a duplicated assertion checking
invoice.Status equals ombilling.StandardInvoiceStatusPaid (the two s.Equal calls
around the onInvoiceIssuedCnt check); remove one of the redundant
s.Equal(ombilling.StandardInvoiceStatusPaid, invoice.Status) assertions so the
test still verifies invoice.Status once while keeping the s.Equal(1,
onInvoiceIssuedCnt) check intact.
- Around line 967-982: The test currently asserts invoice.Status twice (first
with s.Equal for
ombilling.StandardInvoiceStatusPaymentProcessingBookingAuthorizedAndSettledFailed
and then s.Contains over a slice with that value and
ombilling.StandardInvoiceStatusPaymentProcessingBookingSettledFailed), which is
redundant and makes failures noisy; pick one approach: either remove the
s.Contains block and keep the strict s.Equal check on invoice.Status (if the
sandbox always yields the combined-settled-failed status), or remove the s.Equal
and keep the s.Contains check to defensively accept either failed variant (also
update or add a short comment above the chosen assertion to explain the
expectation); adjust only the assertions around invoice.Status (in the test
where s.BillingService.ApproveInvoice is called) and leave the subsequent checks
on StatusDetails, AvailableActions, and ValidationIssues unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4b21c1c0-bbe8-4470-92b7-6711cf921670
📒 Files selected for processing (5)
.agents/skills/billing/SKILL.mdopenmeter/billing/charges/service/invoicable_test.goopenmeter/billing/charges/service/invoice.goopenmeter/server/server_test.gotest/billing/lineengine_test.go
✅ Files skipped from review due to trivial changes (1)
- .agents/skills/billing/SKILL.md
turip
force-pushed
the
feat/payment-settlement
branch
from
d052d3c to
bfb3159
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openmeter/billing/service/stdinvoicestate.go (1)
978-1030: Tiny DRY opportunity for the two payment hooks.
onPaymentAuthorized(978-999) andonPaymentSettled(1009-1030) are essentially the same walk-group-validate-invoke loop, differing only in input type, error message, and engine method. Totally fine as-is, but since you'll likely grow more of these (andonPaymentAuthorizedAndSettledalready composes them), a small helper could cut the duplication:♻️ Optional sketch
func (m *InvoiceStateMachine) forEachEngineLineGroup( ctx context.Context, phase string, invoke func(ctx context.Context, engine billing.LineEngine, input billing.StandardLineEventInput) error, ) error { groupedLines, err := m.Service.lineEngines.groupStandardLinesByEngine(m.Invoice.Lines.OrEmpty()) if err != nil { return fmt.Errorf("grouping standard lines by engine: %w", err) } for _, grouped := range groupedLines { input := billing.StandardLineEventInput{Invoice: m.Invoice, Lines: grouped.Lines} if err := input.Validate(); err != nil { return fmt.Errorf("validating %s input for engine %s: %w", phase, grouped.Engine.GetLineEngineType(), err) } if err := invoke(ctx, grouped.Engine, input); err != nil { return billing.NewLineEngineValidationError(grouped.Engine, err) } } return nil }Then
onPaymentAuthorized/onPaymentSettled/onInvoiceIssuedbecome three-liners. Defer to your taste. 🙂As per coding guidelines ("make readability and maintainability a priority, even potentially suggest restructuring the code to improve them").
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/service/stdinvoicestate.go` around lines 978 - 1030, The onPaymentAuthorized and onPaymentSettled methods in InvoiceStateMachine duplicate the same "group -> build input -> validate -> call engine" loop; extract that loop into a helper on InvoiceStateMachine (e.g., forEachEngineLineGroup) that calls Service.lineEngines.groupStandardLinesByEngine(m.Invoice.Lines.OrEmpty()), constructs a common input (Invoice + Lines), runs Validate() with a phase string for error messages, and invokes a provided callback which calls the appropriate engine method; then rewrite onPaymentAuthorized to call the helper with a callback that calls grouped.Engine.OnPaymentAuthorized and onPaymentSettled to call grouped.Engine.OnPaymentSettled, preserving billing.NewLineEngineValidationError wrapping and existing error messages.
🤖 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/service/stdinvoicestate.go`:
- Around line 978-1030: The onPaymentAuthorized and onPaymentSettled methods in
InvoiceStateMachine duplicate the same "group -> build input -> validate -> call
engine" loop; extract that loop into a helper on InvoiceStateMachine (e.g.,
forEachEngineLineGroup) that calls
Service.lineEngines.groupStandardLinesByEngine(m.Invoice.Lines.OrEmpty()),
constructs a common input (Invoice + Lines), runs Validate() with a phase string
for error messages, and invokes a provided callback which calls the appropriate
engine method; then rewrite onPaymentAuthorized to call the helper with a
callback that calls grouped.Engine.OnPaymentAuthorized and onPaymentSettled to
call grouped.Engine.OnPaymentSettled, preserving
billing.NewLineEngineValidationError wrapping and existing error messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 603f934e-4ea6-42b1-bc07-bdd20e45aaff
⛔ 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 (18)
.agents/skills/billing/SKILL.mdopenmeter/billing/adapter/invoice.goopenmeter/billing/charges/creditpurchase/lineengine/engine.goopenmeter/billing/charges/flatfee/lineengine/engine.goopenmeter/billing/charges/service/creditpurchase_test.goopenmeter/billing/charges/service/invoicable_test.goopenmeter/billing/charges/service/invoice.goopenmeter/billing/charges/usagebased/service/lineengine.goopenmeter/billing/lineengine.goopenmeter/billing/lineengine/engine.goopenmeter/billing/service.goopenmeter/billing/service/invoice.goopenmeter/billing/service/stdinvoicestate.goopenmeter/billing/stdinvoice.goopenmeter/billing/stdinvoicestate.goopenmeter/server/server_test.gotest/billing/lineengine_test.gotest/credits/sanity_test.go
✅ Files skipped from review due to trivial changes (2)
- openmeter/billing/stdinvoicestate.go
- openmeter/billing/charges/service/invoice.go
🚧 Files skipped from review as they are similar to previous changes (11)
- openmeter/billing/service.go
- openmeter/billing/charges/flatfee/lineengine/engine.go
- openmeter/billing/lineengine/engine.go
- openmeter/billing/charges/usagebased/service/lineengine.go
- openmeter/server/server_test.go
- openmeter/billing/charges/service/creditpurchase_test.go
- openmeter/billing/adapter/invoice.go
- openmeter/billing/stdinvoice.go
- test/credits/sanity_test.go
- test/billing/lineengine_test.go
- .agents/skills/billing/SKILL.md
tothandras
approved these changes
Apr 17, 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.
Summary
This branch extends the billing invoice state machine around payment settlement and aligns the surrounding hooks, tests, and Stripe integration.
Changes
Notes for reviewer
Summary by CodeRabbit
New Features
Tests
Documentation