Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (9)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds optional EffectiveAt and Priority fields to the credit-purchase intent, persists and maps them in the adapter/ent schema/migrations, wires Priority into ledger routing, and adds tests asserting persistence and FBO routing. No public API signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service as ChargeService
participant Adapter as ChargeAdapter
participant DB as Database
participant Ledger
Client->>Service: Create CreditPurchase Intent (amount, priority?, effectiveAt?)
Service->>Service: Intent.Normalized(), Validate()
Service->>Adapter: CreateCharge(intent)
Adapter->>DB: persist ChargeCreditPurchase (amount, effective_at?, priority?)
DB-->>Adapter: persisted entity
Adapter-->>Service: created Charge
Service->>Ledger: Issue transactions (CreditPriority from intent or default)
Ledger-->>Service: transaction result
Service-->>Client: response (charge created)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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/credit-priority-and-effective-at
branch
from
13fb2ba to
f958a25
Compare
turip
force-pushed
the
feat/credit-priority-and-effective-at
branch
from
f958a25 to
301b456
Compare
turip
added
release-note/feature
turip
force-pushed
the
feat/credit-priority-and-effective-at
branch
from
11a8650 to
c12fc3d
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/credits/sanity_test.go (1)
962-972:⚠️ Potential issue | 🟠 MajorDon’t assume the requested priority bucket already exists.
Line 968 now looks up a concrete priority route with
GetSubAccountForRoute. In the new priority test, only the priority7bucket is created, so asking for the default-priority bucket can error before Line 450 gets to assert zero.💡 Suggested fix
- subAccount, err := customerAccounts.FBOAccount.GetSubAccountForRoute(s.T().Context(), ledger.CustomerFBORouteParams{ + subAccount, err := customerAccounts.FBOAccount.GetOrCreateSubAccountForRoute(s.T().Context(), ledger.CustomerFBORouteParams{ Currency: code, CostBasis: costBasis, CreditPriority: priority, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/credits/sanity_test.go` around lines 962 - 972, The test assumes the requested priority sub-account exists; change mustCustomerFBOBalanceWithPriority to handle a missing priority bucket by calling customerAccounts.FBOAccount.GetSubAccountForRoute (the current call) but if it returns a not-found/missing-subaccount error, return alpacadecimal.Decimal zero instead of failing; otherwise proceed to fetch the balance as before. In short, wrap GetSubAccountForRoute error handling in mustCustomerFBOBalanceWithPriority to detect "sub-account not found" and treat that case as zero balance rather than propagating the error.
🧹 Nitpick comments (1)
test/credits/sanity_test.go (1)
408-451: Please add an external-settlement priority case too.This new test proves priority on the promotional flow, but Line 176 in
openmeter/ledger/chargeadapter/creditpurchase.goalso routesOnCreditPurchaseInitiatedby priority, and that path has the trickier advance/accrued attribution logic.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 `@test/credits/sanity_test.go` around lines 408 - 451, Add a second test case to TestCreditPurchasePersistsEffectiveAtAndPriority that exercises the external-settlement path: create another intent using creditpurchase.NewSettlement(creditpurchase.ExternalSettlement{}) (keep the same effectiveAt and priority), submit it via Charges.Create (using createCreditPurchaseIntent, createLedgerBackedCustomer, and the same servicePeriod), then fetch the charge (mustGetChargeByID -> AsCreditPurchaseCharge) and assert the Intent.EffectiveAt and Intent.Priority match the originals and that balances reflect the external settlement semantics by checking mustCustomerFBOBalanceWithPriority and mustCustomerFBOBalance as done for the promotional case; this covers the OnCreditPurchaseInitiated routing in creditpurchase.go which handles advance/accrued attribution.
🤖 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/billing/charges/creditpurchase/chargecreditpurchase.go`:
- Around line 111-115: The current guard in chargecreditpurchase.go
unconditionally rejects any non-nil EffectiveAt, making the new EffectiveAt
persistence/mapping unreachable and preventing
TestCreditPurchasePersistsEffectiveAtAndPriority from passing; remove or modify
the if i.EffectiveAt != nil check so that non-nil EffectiveAt values are allowed
to proceed to the DB setter/mapping logic (i.e., only return validation errors
from models.NewNillableGenericValidationError(errors.Join(errs...)) when errs is
non-empty), ensuring the create paths that validate intent before DB setters
remain functional and EffectiveAt/priority are persisted.
In `@openmeter/ledger/chargeadapter/creditpurchase.go`:
- Around line 80-85: Replace the use of the full credit field with the
already-reduced value: in the transactions.IssueCustomerReceivableTemplate
construction swap charge.Intent.CreditAmount for the previously-computed
issuableAmount (the value reduced by advanceOutstanding) so customer receivables
are issued only for issuableAmount; ensure this change is made where
IssueCustomerReceivableTemplate is populated and that
h.creditPurchasePriority(charge) and CostBasis usage remain unchanged.
---
Outside diff comments:
In `@test/credits/sanity_test.go`:
- Around line 962-972: The test assumes the requested priority sub-account
exists; change mustCustomerFBOBalanceWithPriority to handle a missing priority
bucket by calling customerAccounts.FBOAccount.GetSubAccountForRoute (the current
call) but if it returns a not-found/missing-subaccount error, return
alpacadecimal.Decimal zero instead of failing; otherwise proceed to fetch the
balance as before. In short, wrap GetSubAccountForRoute error handling in
mustCustomerFBOBalanceWithPriority to detect "sub-account not found" and treat
that case as zero balance rather than propagating the error.
---
Nitpick comments:
In `@test/credits/sanity_test.go`:
- Around line 408-451: Add a second test case to
TestCreditPurchasePersistsEffectiveAtAndPriority that exercises the
external-settlement path: create another intent using
creditpurchase.NewSettlement(creditpurchase.ExternalSettlement{}) (keep the same
effectiveAt and priority), submit it via Charges.Create (using
createCreditPurchaseIntent, createLedgerBackedCustomer, and the same
servicePeriod), then fetch the charge (mustGetChargeByID ->
AsCreditPurchaseCharge) and assert the Intent.EffectiveAt and Intent.Priority
match the originals and that balances reflect the external settlement semantics
by checking mustCustomerFBOBalanceWithPriority and mustCustomerFBOBalance as
done for the promotional case; this covers the OnCreditPurchaseInitiated routing
in creditpurchase.go which handles advance/accrued attribution.
🪄 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: e497e000-5f97-43c5-b11c-a06d636ef4dd
⛔ Files ignored due to path filters (9)
openmeter/ent/db/chargecreditpurchase.gois excluded by!**/ent/db/**openmeter/ent/db/chargecreditpurchase/chargecreditpurchase.gois excluded by!**/ent/db/**openmeter/ent/db/chargecreditpurchase/where.gois excluded by!**/ent/db/**openmeter/ent/db/chargecreditpurchase_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargecreditpurchase_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/**openmeter/ent/db/runtime.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (8)
openmeter/billing/charges/creditpurchase/adapter/charge.goopenmeter/billing/charges/creditpurchase/adapter/mapper.goopenmeter/billing/charges/creditpurchase/chargecreditpurchase.goopenmeter/ent/schema/chargescreditpurchase.goopenmeter/ledger/chargeadapter/creditpurchase.gotest/credits/sanity_test.gotools/migrate/migrations/20260402130955_add_credit_purchase_effective_at_priority.down.sqltools/migrate/migrations/20260402130955_add_credit_purchase_effective_at_priority.up.sql
Comment on lines
80
to
+85
| transactions.IssueCustomerReceivableTemplate{ | ||
| At: charge.CreatedAt, | ||
| Amount: charge.Intent.CreditAmount, | ||
| Currency: charge.Intent.Currency, | ||
| CostBasis: &costBasis, | ||
| At: charge.CreatedAt, | ||
| Amount: charge.Intent.CreditAmount, | ||
| Currency: charge.Intent.Currency, | ||
| CostBasis: &costBasis, | ||
| CreditPriority: lo.ToPtr(h.creditPurchasePriority(charge)), |
Contributor
There was a problem hiding this comment.
Use issuableAmount here instead of the full credit amount.
Line 82 still issues charge.Intent.CreditAmount, even though the block above already reduced it by advanceOutstanding. With a partial backfill, this over-credits the customer and puts the extra balance into the selected priority bucket.
💡 Suggested fix
transactions.IssueCustomerReceivableTemplate{
At: charge.CreatedAt,
- Amount: charge.Intent.CreditAmount,
+ Amount: issuableAmount,
Currency: charge.Intent.Currency,
CostBasis: &costBasis,
CreditPriority: lo.ToPtr(h.creditPurchasePriority(charge)),
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| transactions.IssueCustomerReceivableTemplate{ | |
| At: charge.CreatedAt, | |
| Amount: charge.Intent.CreditAmount, | |
| Currency: charge.Intent.Currency, | |
| CostBasis: &costBasis, | |
| At: charge.CreatedAt, | |
| Amount: charge.Intent.CreditAmount, | |
| Currency: charge.Intent.Currency, | |
| CostBasis: &costBasis, | |
| CreditPriority: lo.ToPtr(h.creditPurchasePriority(charge)), | |
| transactions.IssueCustomerReceivableTemplate{ | |
| At: charge.CreatedAt, | |
| Amount: issuableAmount, | |
| Currency: charge.Intent.Currency, | |
| CostBasis: &costBasis, | |
| CreditPriority: lo.ToPtr(h.creditPurchasePriority(charge)), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/ledger/chargeadapter/creditpurchase.go` around lines 80 - 85,
Replace the use of the full credit field with the already-reduced value: in the
transactions.IssueCustomerReceivableTemplate construction swap
charge.Intent.CreditAmount for the previously-computed issuableAmount (the value
reduced by advanceOutstanding) so customer receivables are issued only for
issuableAmount; ensure this change is made where IssueCustomerReceivableTemplate
is populated and that h.creditPurchasePriority(charge) and CostBasis usage
remain unchanged.
GAlexIHU
reviewed
Apr 2, 2026
| Amount: charge.Intent.CreditAmount, | ||
| Currency: charge.Intent.Currency, | ||
| CostBasis: &costBasis, | ||
| CreditPriority: lo.ToPtr(h.creditPurchasePriority(charge)), |
Contributor
There was a problem hiding this comment.
this is not strictly needed as we default on next layer
GAlexIHU
approved these changes
Apr 2, 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
Add support for priority. EffectiveAt wiring is there, but it's not yet supported by the handler.
Notes for reviewer
Summary by CodeRabbit
New Features
Tests
Chores