fix(ledger): book advance attribution at purchase time - #4767
Conversation
📝 WalkthroughWalkthroughCredit purchase ledger attribution now separates service-period effective time from purchase-time advance attribution. New tests verify past and future effective periods, capped advance allocation, template booking timestamps, balances, promotional grant timestamps, and lineage identifiers. ChangesCredit purchase timing
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant issueCreditPurchaseGroup
participant clockNow
participant LedgerTemplates
issueCreditPurchaseGroup->>clockNow: read current time for future effective periods
issueCreditPurchaseGroup->>LedgerTemplates: post advance attribution at selected timestamp
issueCreditPurchaseGroup->>LedgerTemplates: post issuance and settlement at effectiveAt
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
0e96582 to
5f4e6a8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/ledger/chargeadapter/creditpurchase_test.go (1)
205-294: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFreeze the clock in these future-effective credit-purchase tests.
env.Now()reads fromclock.Now(), and these tests don’t callclock.FreezeTimebeforeOnCreditPurchaseInitiated; the handler resolves future attribution withclock.Now()for effective-periodIssueCustomerReceivablescenarios. Capture a frozen purchase time and make the handler use that purchase time for future-effective attribution, otherwise exactbookedAtand balance boundary checks can become microsecond-flaky.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/ledger/chargeadapter/creditpurchase_test.go` around lines 205 - 294, Freeze the test clock at the captured purchase time in both future-effective tests, before calling OnCreditPurchaseInitiated. Use the test environment’s clock-freezing helper so handler calls resolve clock.Now() to purchasedAt, preserving deterministic bookedAt and balance boundary assertions.
🧹 Nitpick comments (1)
openmeter/ledger/chargeadapter/creditpurchase_test.go (1)
1011-1035: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew helper duplicates the query in
transactionBookedAtTimes.
transactionBookedAtByTemplateCodere-issues the sameWhere/Order/Allquery as the pre-existingtransactionBookedAtTimes(lines 987-1009), differing only in how the results are grouped. Consider extracting the shared query into a small private helper both can call, so the two output shapes are derived from one query path.As per coding guidelines,
avoid duplicate test helpers, and add helpers only when reused or when they express non-obvious domain semantics.♻️ Proposed refactor
+func (e *creditPurchaseHandlerTestEnv) queryTransactionRows(t *testing.T, groupID string) []*db.LedgerTransaction { + t.Helper() + + transactionRows, err := e.DB.LedgerTransaction.Query(). + Where( + ledgertransactiondb.Namespace(e.Namespace), + ledgertransactiondb.GroupID(groupID), + ). + Order( + ledgertransactiondb.ByCreatedAt(), + ledgertransactiondb.ByID(), + ). + All(t.Context()) + require.NoError(t, err) + require.NotEmpty(t, transactionRows, "expected at least one ledger transaction for group") + + return transactionRows +} + func (e *creditPurchaseHandlerTestEnv) transactionBookedAtByTemplateCode(t *testing.T, groupID string) map[string][]time.Time { t.Helper() - - transactionRows, err := e.DB.LedgerTransaction.Query(). - Where( - ledgertransactiondb.Namespace(e.Namespace), - ledgertransactiondb.GroupID(groupID), - ). - Order( - ledgertransactiondb.ByCreatedAt(), - ledgertransactiondb.ByID(), - ). - All(t.Context()) - require.NoError(t, err) - require.NotEmpty(t, transactionRows, "expected at least one ledger transaction for group") + transactionRows := e.queryTransactionRows(t, groupID) out := make(map[string][]time.Time, len(transactionRows))(Note: adjust the row type name to match the actual generated ent type.)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/ledger/chargeadapter/creditpurchase_test.go` around lines 1011 - 1035, Extract the shared ledger transaction query from transactionBookedAtTimes and transactionBookedAtByTemplateCode into one private helper returning the queried transaction rows. Update both helpers to reuse it while preserving their existing output shapes and assertions, including ordering and error handling.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openmeter/ledger/chargeadapter/creditpurchase.go`:
- Around line 303-311: Update the settlement entry built with
SettleCustomerReceivableFromPaymentTemplate so a past-effective promotional
grant uses the recording/materialization timestamp instead of effectiveAt.
Preserve effectiveAt for purchase-time attribution and only alter settlement
timing when the grant is backdated, keeping ledger ordering consistent.
---
Outside diff comments:
In `@openmeter/ledger/chargeadapter/creditpurchase_test.go`:
- Around line 205-294: Freeze the test clock at the captured purchase time in
both future-effective tests, before calling OnCreditPurchaseInitiated. Use the
test environment’s clock-freezing helper so handler calls resolve clock.Now() to
purchasedAt, preserving deterministic bookedAt and balance boundary assertions.
---
Nitpick comments:
In `@openmeter/ledger/chargeadapter/creditpurchase_test.go`:
- Around line 1011-1035: Extract the shared ledger transaction query from
transactionBookedAtTimes and transactionBookedAtByTemplateCode into one private
helper returning the queried transaction rows. Update both helpers to reuse it
while preserving their existing output shapes and assertions, including ordering
and error handling.
🪄 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 Plus
Run ID: 900bf32c-a5ef-40bd-a9ad-9cb71d085ca4
📒 Files selected for processing (3)
openmeter/billing/charges/creditpurchase/service/promotional_test.goopenmeter/ledger/chargeadapter/creditpurchase.goopenmeter/ledger/chargeadapter/creditpurchase_test.go
2 participants
Summary
effective_atBehavior
effective_atInvoice-settled purchases keep their existing trigger at invoice draft creation. This PR does not add payment control or recurring subscription grants.
Cancellation / void limitation
The existing void path operates on currently issued future-balance-overage value for the source charge. Before a future grant reaches
effective_at, its scheduled remainder has no spendable balance slice to void, and the purchase-time advance attribution is not such a slice. As a result, future-effective grants remain non-voidable beforeeffective_at; this PR does not add cancellation of scheduled ledger entries or claim to solve that lifecycle. Void behavior for remaining issued value aftereffective_atis unchanged.Tests
nix develop --impure .#ci -c env POSTGRES_HOST=127.0.0.1 go vet -tags=dynamic ./openmeter/ledger/chargeadapter ./openmeter/billing/charges/creditpurchase/servicenix develop --impure .#ci -c env POSTGRES_HOST=127.0.0.1 go test -tags=dynamic -v ./openmeter/ledger/chargeadapter ./openmeter/billing/charges/creditpurchase/servicenix develop --impure .#ci -c env POSTGRES_HOST=127.0.0.1 go test -tags=dynamic -v ./test/creditsAll database-backed tests ran against PostgreSQL; none were skipped.
Summary by CodeRabbit
Bug Fixes
Tests
Greptile Summary
This PR changes when credit purchases are recorded in the ledger. The main changes are:
Confidence Score: 5/5
This looks safe to merge.
Important Files Changed
Reviews (2): Last reviewed commit: "fix(ledger): book advance attribution at..." | Re-trigger Greptile
Context used: