feat(credit): integration tests#4007
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces transaction authorization status ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ChargeHandler as Charge Handler
participant Ledger
participant RoutingRules as Routing & Rules
participant DB as Database
rect rgba(100, 150, 200, 0.5)
Note over Client,DB: Authorization Flow
Client->>ChargeHandler: OnPaymentAuthorized(charge)
ChargeHandler->>Ledger: CommitGroup([Issue + Fund + Fund(Authorized)])
Ledger->>RoutingRules: ValidateEntries()
RoutingRules->>DB: Check authorization transition
DB-->>RoutingRules: Valid (open→authorized split)
RoutingRules-->>Ledger: OK
Ledger->>DB: Store with split receivables
Ledger-->>ChargeHandler: TransactionGroupID
end
rect rgba(150, 100, 200, 0.5)
Note over Client,DB: Settlement Flow
Client->>ChargeHandler: OnPaymentSettled(charge)
ChargeHandler->>Ledger: CommitGroup([Settle(authorized→open)])
Ledger->>RoutingRules: ValidateEntries()
RoutingRules->>DB: Verify authorization state
DB-->>RoutingRules: Valid (authorized being settled)
RoutingRules-->>Ledger: OK
Ledger->>DB: Move from authorized to open receivable
Ledger-->>ChargeHandler: TransactionGroupID
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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
previously approved these changes
Mar 26, 2026
| return ledgertransaction.GroupReference{}, err | ||
| } | ||
|
|
||
| externalSettlement, err := charge.Intent.Settlement.AsExternalSettlement() |
Member
There was a problem hiding this comment.
Now we also have InvoiceSettlement, we should add support for that too eventually.
| type TransactionAuthorizationStatus string | ||
|
|
||
| const ( | ||
| TransactionAuthorizationStatusOpen TransactionAuthorizationStatus = "open" |
Member
There was a problem hiding this comment.
We can consider naming this pending just so that we have parity with the API
openmeter/api/spec/packages/aip/src/customers/credits/grant.tsp
Lines 56 to 57 in 0fb6b3e
GAlexIHU
force-pushed
the
feat/credit-integration-tests
branch
from
22f7a1d to
5d9da66
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: 2
🧹 Nitpick comments (3)
openmeter/ledger/account/adapter/repo_test.go (1)
230-309: Consider adding a uniqueness constraint test for authorization status.The
TestRepo_SubAccountRouteUniquenessConstraintstests cover priority and cost basis uniqueness, but don't exercise the newtransaction_authorization_statusfield. Since this field is part of the routing key (and thus the unique constraint), it'd be worth adding a test case that verifies two routes with the same currency but different authorization statuses can coexist, and that duplicate authorization statuses on the same account are rejected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/account/adapter/repo_test.go` around lines 230 - 309, Add a subtest to TestRepo_SubAccountRouteUniquenessConstraints exercising the transaction_authorization_status part of the routing key: extend the createRoute helper to accept a transactionAuthorizationStatus parameter and set it on the ent builder (use the SetNillableTransactionAuthorizationStatus / appropriate setter on env.client.LedgerSubAccountRoute.Create()), then add a t.Run that creates two routes on the same account with the same currency and routing key except differing authorization statuses (assert both succeed) and then attempts to create a duplicate with the same authorization status (assert it fails and entdb.IsConstraintError is true); keep references to the existing TestRepo_SubAccountRouteUniquenessConstraints and createRoute helper so the change is localized.openmeter/ledger/routingrules/routingrules.go (1)
218-232: Consider documenting the zero-amount entry behavior.Entries with zero amounts are silently excluded from both negative and positive slices. This is probably intentional (zero doesn't indicate a direction), but a brief comment would clarify for future readers.
📝 Optional: Add a clarifying comment
+// entriesBySign partitions entries into negative and positive amounts. +// Zero-amount entries are excluded as they don't indicate a flow direction. func entriesBySign(entries []EntryView) ([]EntryView, []EntryView) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/routingrules/routingrules.go` around lines 218 - 232, The function entriesBySign currently drops zero-amount entries by only appending entries where entry.Amount().IsNegative() or IsPositive(); add a concise comment inside entriesBySign explaining that zero-amount entries are intentionally omitted (zero has no sign) so readers know this behavior is deliberate and not a bug, referencing the function name entriesBySign and the Amount().IsNegative/IsPositive checks.openmeter/ledger/chargeadapter/flatfee_test.go (1)
202-240: Consider adding an error-path test for double-settlement.The happy paths are well covered, but it might be worth adding a test that calls
OnPaymentSettledtwice on the same charge to verify idempotency or proper error handling. Based on learnings, the customer-scoped lock prevents concurrent execution, but sequential double-calls could still be a concern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/chargeadapter/flatfee_test.go` around lines 202 - 240, Add a test that verifies calling env.handler.OnPaymentSettled twice for the same charge is handled correctly: create an env (newFlatFeeHandlerTestEnv), accrue usage (env.newAccrualInput / env.newChargeWithAccruedUsage), call handler.OnPaymentSettled once and assert success and expected balances/TransactionGroupID, then call handler.OnPaymentSettled a second time with the same charge and assert the system either returns a descriptive "already settled" error from OnPaymentSettled or returns a no-op reference (empty TransactionGroupID) and leaves all account balances unchanged; reference handler.OnPaymentSettled, env.newChargeWithAccruedUsage (or env.newChargeWithCreditRealizationsAndAccruedUsage), and the helper methods sumBalance/receivableSubAccount/authorizedReceivableSubAccount/washSubAccount to locate assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openmeter/ledger/routing.go`:
- Line 248: The new routing key string now includes
"transaction_authorization_status" which breaks the existing V1 format; revert
introducing this field into the V1 key and instead implement explicit
versioning: add a new constant (e.g., RoutingKeyVersionV2) and produce the new
key format only when version == RoutingKeyVersionV2 while preserving the
original V1 serialization in the code that builds keys (the code that currently
concatenates fields like "currency" and "credit_priority"); update
NewSubAccountRouteFromData (or the constructor/parser that reads routing keys)
to detect and parse both V1 and V2 formats (fall back to V1 parsing for existing
keys and parse the extra transaction_authorization_status for V2), so existing
persisted keys remain compatible without migration.
In
`@tools/migrate/migrations/20260326163949_add_ledger_transaction_authorization_status.up.sql`:
- Line 2: The migration adds
ledger_sub_account_routes.transaction_authorization_status as nullable which
will leave existing rows NULL and create divergent routing keys vs. the new
TransactionAuthorizationStatusOpen values passed by the accrual flow; fix by
either (A) modifying the migration to backfill existing rows to 'open' and set a
DEFAULT 'open' (UPDATE ledger_sub_account_routes SET
transaction_authorization_status='open' WHERE transaction_authorization_status
IS NULL; then ALTER ... SET DEFAULT), or (B) keep it nullable but add
application-side normalization in the route lookup used by accrual (treat NULL
as TransactionAuthorizationStatusOpen) so routing keys collapse; choose one
approach and implement the corresponding change referencing
transaction_authorization_status, TransactionAuthorizationStatusOpen, and the
accrual flow that produces the new status.
---
Nitpick comments:
In `@openmeter/ledger/account/adapter/repo_test.go`:
- Around line 230-309: Add a subtest to
TestRepo_SubAccountRouteUniquenessConstraints exercising the
transaction_authorization_status part of the routing key: extend the createRoute
helper to accept a transactionAuthorizationStatus parameter and set it on the
ent builder (use the SetNillableTransactionAuthorizationStatus / appropriate
setter on env.client.LedgerSubAccountRoute.Create()), then add a t.Run that
creates two routes on the same account with the same currency and routing key
except differing authorization statuses (assert both succeed) and then attempts
to create a duplicate with the same authorization status (assert it fails and
entdb.IsConstraintError is true); keep references to the existing
TestRepo_SubAccountRouteUniquenessConstraints and createRoute helper so the
change is localized.
In `@openmeter/ledger/chargeadapter/flatfee_test.go`:
- Around line 202-240: Add a test that verifies calling
env.handler.OnPaymentSettled twice for the same charge is handled correctly:
create an env (newFlatFeeHandlerTestEnv), accrue usage (env.newAccrualInput /
env.newChargeWithAccruedUsage), call handler.OnPaymentSettled once and assert
success and expected balances/TransactionGroupID, then call
handler.OnPaymentSettled a second time with the same charge and assert the
system either returns a descriptive "already settled" error from
OnPaymentSettled or returns a no-op reference (empty TransactionGroupID) and
leaves all account balances unchanged; reference handler.OnPaymentSettled,
env.newChargeWithAccruedUsage (or
env.newChargeWithCreditRealizationsAndAccruedUsage), and the helper methods
sumBalance/receivableSubAccount/authorizedReceivableSubAccount/washSubAccount to
locate assertions.
In `@openmeter/ledger/routingrules/routingrules.go`:
- Around line 218-232: The function entriesBySign currently drops zero-amount
entries by only appending entries where entry.Amount().IsNegative() or
IsPositive(); add a concise comment inside entriesBySign explaining that
zero-amount entries are intentionally omitted (zero has no sign) so readers know
this behavior is deliberate and not a bug, referencing the function name
entriesBySign and the Amount().IsNegative/IsPositive checks.
🪄 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: 2daf512e-46af-4aed-b6cb-d90c7644fb8b
⛔ Files ignored due to path filters (8)
openmeter/ent/db/ledgersubaccountroute.gois excluded by!**/ent/db/**openmeter/ent/db/ledgersubaccountroute/ledgersubaccountroute.gois excluded by!**/ent/db/**openmeter/ent/db/ledgersubaccountroute/where.gois excluded by!**/ent/db/**openmeter/ent/db/ledgersubaccountroute_create.gois excluded by!**/ent/db/**openmeter/ent/db/ledgersubaccountroute_update.gois excluded by!**/ent/db/**openmeter/ent/db/migrate/schema.gois excluded by!**/ent/db/**openmeter/ent/db/mutation.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (32)
.agents/skills/charges/SKILL.md.agents/skills/ledger/SKILL.md.agents/skills/rebase/SKILL.md.agents/skills/test/SKILL.mdAGENTS.mdopenmeter/ent/schema/ledger_account.goopenmeter/ledger/account/adapter/repo_test.goopenmeter/ledger/account/adapter/subaccount.goopenmeter/ledger/accounts.goopenmeter/ledger/chargeadapter/creditpurchase.goopenmeter/ledger/chargeadapter/creditpurchase_test.goopenmeter/ledger/chargeadapter/flatfee.goopenmeter/ledger/chargeadapter/flatfee_test.goopenmeter/ledger/errors.goopenmeter/ledger/historical/adapter/ledger.goopenmeter/ledger/historical/adapter/sumentries_query.goopenmeter/ledger/primitives.goopenmeter/ledger/routing.goopenmeter/ledger/routing_test.goopenmeter/ledger/routingrules/defaults.goopenmeter/ledger/routingrules/routingrules.goopenmeter/ledger/routingrules/routingrules_test.goopenmeter/ledger/routingrules/view.goopenmeter/ledger/testutils/integration.goopenmeter/ledger/transactions/accrual.goopenmeter/ledger/transactions/customer.goopenmeter/ledger/transactions/customer_test.goopenmeter/ledger/transactions/testenv_test.gotest/credits/mockledger.gotest/credits/sanity_test.gotools/migrate/migrations/20260326163949_add_ledger_transaction_authorization_status.down.sqltools/migrate/migrations/20260326163949_add_ledger_transaction_authorization_status.up.sql
💤 Files with no reviewable changes (1)
- test/credits/mockledger.go
turip
approved these changes
Mar 27, 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
Negative balance will come in a separate PR as its way too large. Related extra tests / corrections will also be included in that one.
I we want we can use this PR to improve on test tooling, but that can also be an increment
Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation