Fix CTI ledger booking timestamps#4397
Merged
Merged
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:
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 (1)
📝 WalkthroughWalkthroughHandler DTOs now carry validated timestamps: payments use EventAt, allocations/accruals use BookedAt. Services compute or derive a single event timestamp per operation (or derive BookedAt from service period and payment term), pass validated inputs to handlers, and ledger adapters use those timestamps for transaction booking. Tests and mocks updated to the new contracts. ChangesEvent Timestamp Contracts and Service Propagation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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/billing/charges/usagebased/service/lineengine.go (1)
281-285: ⚡ Quick winUse the run’s service-period end as the canonical correction timestamp.
Small tweak: prefer
run.ServicePeriodTooverstdLine.GetServicePeriod().Tohere, so booking stays anchored to the persisted realization run even if line period shape drifts.Suggested change
if _, err := e.service.runs.CorrectAllCredits(ctx, usagebasedrun.CorrectAllCreditRealizationsInput{ Charge: charge, Run: run, - AllocateAt: stdLine.GetServicePeriod().To, + AllocateAt: run.ServicePeriodTo, CurrencyCalculator: currencyCalculator, }); err != nil {🤖 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/billing/charges/usagebased/service/lineengine.go` around lines 281 - 285, The AllocateAt timestamp passed into e.service.runs.CorrectAllCredits should use the persisted run end instead of the line's period; replace stdLine.GetServicePeriod().To with run.ServicePeriodTo when constructing usagebasedrun.CorrectAllCreditRealizationsInput so the correction is anchored to the run (update the AllocateAt field in the call to e.service.runs.CorrectAllCredits accordingly).openmeter/ledger/chargeadapter/creditpurchase_test.go (1)
240-254: ⚡ Quick winAdd a booked-at assertion in this back-advance settled path.
Nice that
EventAtis passed here too. I’d also assert the group transactions’BookedAtequalseventTime(same as the other settled test) so this path is protected against timestamp regressions as well.🤖 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 240 - 254, Add an assertion that the group transactions created by the settled path have BookedAt equal to eventTime: after calling env.handler.OnCreditPurchasePaymentSettled (where ref.TransactionGroupID is returned and eventTime is defined), fetch the transactions for ref.TransactionGroupID and assert each transaction.BookedAt (or the group's BookedAt) equals eventTime to mirror the other settled test and prevent timestamp regressions.openmeter/ledger/chargeadapter/usagebased_test.go (1)
466-524: ⚡ Quick winPlease add a
EventAt-required test forOnPaymentSettledtoo.You already cover zero
EventAtfor authorization; adding the same negative case for settled would complete the contract checks end-to-end.🤖 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/usagebased_test.go` around lines 466 - 524, Add a negative test in TestOnUsageBasedPaymentSettled that verifies OnPaymentSettled validates the required EventAt field: call env.handler.OnPaymentSettled with an OnPaymentSettledInput whose EventAt is the zero time (or omitted), assert it returns an error and that the returned reference has an empty TransactionGroupID; place this alongside the existing "zero invoice usage is a no-op" case to mirror the authorization test coverage and reference the OnPaymentSettled and OnPaymentSettledInput symbols so the test targets the same handler contract.
🤖 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/billing/worker/subscriptionsync/service/base_test.go`:
- Around line 610-613: The test currently iterates over
actualRealization.LedgerTransactionGroups and asserts BookedAt but silently
succeeds if the slice is empty; update the test to first assert that
actualRealization.LedgerTransactionGroups is non-empty (e.g.,
require/require.NotEmpty or assert.Len>0) before entering the for-loop so the
contract is enforced, then proceed to call
s.assertLedgerTransactionGroupBookedAt(ctx, namespace, transactionGroup.ID,
expectedRealization.BookedAt, fmt.Sprintf("%s: %s", childID,
transactionGroup.Label)).
In
`@openmeter/billing/worker/subscriptionsync/service/sync_credittheninvoice_test.go`:
- Around line 893-894: The new call to clock.FreezeTime(start.Add(time.Minute))
must be paired immediately with defer clock.UnFreeze() in the same scope (or
wrapped in its own block) to avoid leaking frozen time to other tests; update
the occurrence of clock.FreezeTime(start.Add(time.Minute)) and every other
similar clock.FreezeTime(...) use noted in the review by adding a matching defer
clock.UnFreeze() (or enclosing the freeze+assertions in { ... } and deferring
UnFreeze()) so each freeze is restored before leaving that local scope.
---
Nitpick comments:
In `@openmeter/billing/charges/usagebased/service/lineengine.go`:
- Around line 281-285: The AllocateAt timestamp passed into
e.service.runs.CorrectAllCredits should use the persisted run end instead of the
line's period; replace stdLine.GetServicePeriod().To with run.ServicePeriodTo
when constructing usagebasedrun.CorrectAllCreditRealizationsInput so the
correction is anchored to the run (update the AllocateAt field in the call to
e.service.runs.CorrectAllCredits accordingly).
In `@openmeter/ledger/chargeadapter/creditpurchase_test.go`:
- Around line 240-254: Add an assertion that the group transactions created by
the settled path have BookedAt equal to eventTime: after calling
env.handler.OnCreditPurchasePaymentSettled (where ref.TransactionGroupID is
returned and eventTime is defined), fetch the transactions for
ref.TransactionGroupID and assert each transaction.BookedAt (or the group's
BookedAt) equals eventTime to mirror the other settled test and prevent
timestamp regressions.
In `@openmeter/ledger/chargeadapter/usagebased_test.go`:
- Around line 466-524: Add a negative test in TestOnUsageBasedPaymentSettled
that verifies OnPaymentSettled validates the required EventAt field: call
env.handler.OnPaymentSettled with an OnPaymentSettledInput whose EventAt is the
zero time (or omitted), assert it returns an error and that the returned
reference has an empty TransactionGroupID; place this alongside the existing
"zero invoice usage is a no-op" case to mirror the authorization test coverage
and reference the OnPaymentSettled and OnPaymentSettledInput symbols so the test
targets the same handler contract.
🪄 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: 83e9f057-3633-430e-bdf8-e73222a2d9da
📒 Files selected for processing (35)
openmeter/billing/charges/creditpurchase/handler.goopenmeter/billing/charges/creditpurchase/service/external.goopenmeter/billing/charges/creditpurchase/service/invoice.goopenmeter/billing/charges/flatfee/bookedat.goopenmeter/billing/charges/flatfee/handler.goopenmeter/billing/charges/flatfee/service/creditheninvoice.goopenmeter/billing/charges/flatfee/service/creditsonly.goopenmeter/billing/charges/flatfee/service/lineengine.goopenmeter/billing/charges/flatfee/service/payment.goopenmeter/billing/charges/flatfee/service/realizations/correct.goopenmeter/billing/charges/flatfee/service/realizations/creditsonly.goopenmeter/billing/charges/flatfee/service/realizations/credittheninvoice.goopenmeter/billing/charges/flatfee/service/realizations/invoiceaccrued.goopenmeter/billing/charges/service/creditpurchase_test.goopenmeter/billing/charges/service/handlers_test.goopenmeter/billing/charges/service/invoicable_test.goopenmeter/billing/charges/service/taxcode_test.goopenmeter/billing/charges/testutils/handlers.goopenmeter/billing/charges/usagebased/handler.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/credits.goopenmeter/billing/charges/usagebased/service/run/invoice.goopenmeter/billing/charges/usagebased/service/run/payment.goopenmeter/billing/worker/subscriptionsync/service/base_test.goopenmeter/billing/worker/subscriptionsync/service/sync_credittheninvoice_test.goopenmeter/ledger/chargeadapter/creditpurchase.goopenmeter/ledger/chargeadapter/creditpurchase_test.goopenmeter/ledger/chargeadapter/flatfee.goopenmeter/ledger/chargeadapter/flatfee_test.goopenmeter/ledger/chargeadapter/usagebased.goopenmeter/ledger/chargeadapter/usagebased_test.go
turip
force-pushed
the
fix/cti-booked-at-missmatches
branch
from
c557e9e to
c5827bf
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
openmeter/billing/worker/subscriptionsync/service/base_test.go (1)
605-613:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert realization ledger groups are non-empty before checking
BookedAt.Right now the loop can silently pass when there are no realization ledger groups, so this assertion doesn’t fully enforce the timestamp contract.
Suggested patch
func (s *SuiteBase) assertChargeRealizationLedgerTransactions(ctx context.Context, namespace string, childID string, expectedRealization expectedChargeRealization, actualRealization actualChargeRealization) { s.T().Helper() s.Require().False(expectedRealization.BookedAt.IsZero(), "%s: realization booked_at", childID) + s.Require().NotEmpty(actualRealization.LedgerTransactionGroups, "%s: realization ledger transaction groups", childID) for _, transactionGroup := range actualRealization.LedgerTransactionGroups { s.assertLedgerTransactionGroupBookedAt(ctx, namespace, transactionGroup.ID, expectedRealization.BookedAt, fmt.Sprintf("%s: %s", childID, transactionGroup.Label)) } }🤖 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/billing/worker/subscriptionsync/service/base_test.go` around lines 605 - 613, The test helper assertChargeRealizationLedgerTransactions currently iterates actualRealization.LedgerTransactionGroups without asserting it contains entries, so add a precondition check that actualRealization.LedgerTransactionGroups is non-empty (e.g. s.Require().NotEmpty or s.Require().Greater(len(...), 0)) before the for loop to ensure the BookedAt timestamp contract is enforced; keep the existing check of expectedRealization.BookedAt.IsZero and call assertLedgerTransactionGroupBookedAt for each group as before, using the same childID/context in the error message.
🧹 Nitpick comments (1)
openmeter/ledger/chargeadapter/usagebased_test.go (1)
466-524: ⚡ Quick winMissing "event at is required" validation test for
OnPaymentSettled.
TestOnUsageBasedPaymentAuthorizedhas an "event at is required" test, butTestOnUsageBasedPaymentSettleddoesn't have a matching validation test. Worth adding for consistency and to ensure the input validation is covered.🧪 Suggested test case to add
t.Run("zero invoice usage is a no-op", func(t *testing.T) { env := newUsageBasedHandlerTestEnv(t) ref, err := env.handler.OnPaymentSettled(t.Context(), chargeusagebased.OnPaymentSettledInput{ Charge: env.newCharge(productcatalog.CreditThenInvoiceSettlementMode), Run: env.newRunWithAuthorizedPaymentAndInvoiceUsage("line-1", alpacadecimal.NewFromInt(1), alpacadecimal.Zero), EventAt: env.Now(), }) require.NoError(t, err) require.Empty(t, ref.TransactionGroupID) }) + + t.Run("event at is required", func(t *testing.T) { + env := newUsageBasedHandlerTestEnv(t) + + ref, err := env.handler.OnPaymentSettled(t.Context(), chargeusagebased.OnPaymentSettledInput{ + Charge: env.newCharge(productcatalog.CreditThenInvoiceSettlementMode), + Run: env.newRunWithAuthorizedPayment("line-1", alpacadecimal.NewFromInt(10)), + EventAt: time.Time{}, + }) + require.ErrorContains(t, err, "event at is required") + require.Empty(t, ref.TransactionGroupID) + }) }🤖 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/usagebased_test.go` around lines 466 - 524, Add a subtest to TestOnUsageBasedPaymentSettled that verifies input validation rejects a missing EventAt: create env := newUsageBasedHandlerTestEnv(t), call env.handler.OnPaymentSettled with a valid Charge and Run but EventAt set to time.Time{} (zero value), then assert the call returns an error and that the error message contains/indicates "event at is required"; reference the TestOnUsageBasedPaymentSettled test and the handler.OnPaymentSettled method for where to add this check.
🤖 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.
Duplicate comments:
In `@openmeter/billing/worker/subscriptionsync/service/base_test.go`:
- Around line 605-613: The test helper assertChargeRealizationLedgerTransactions
currently iterates actualRealization.LedgerTransactionGroups without asserting
it contains entries, so add a precondition check that
actualRealization.LedgerTransactionGroups is non-empty (e.g.
s.Require().NotEmpty or s.Require().Greater(len(...), 0)) before the for loop to
ensure the BookedAt timestamp contract is enforced; keep the existing check of
expectedRealization.BookedAt.IsZero and call
assertLedgerTransactionGroupBookedAt for each group as before, using the same
childID/context in the error message.
---
Nitpick comments:
In `@openmeter/ledger/chargeadapter/usagebased_test.go`:
- Around line 466-524: Add a subtest to TestOnUsageBasedPaymentSettled that
verifies input validation rejects a missing EventAt: create env :=
newUsageBasedHandlerTestEnv(t), call env.handler.OnPaymentSettled with a valid
Charge and Run but EventAt set to time.Time{} (zero value), then assert the call
returns an error and that the error message contains/indicates "event at is
required"; reference the TestOnUsageBasedPaymentSettled test and the
handler.OnPaymentSettled method for where to add this check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dce89e23-3536-4f3d-b30e-b768e2fe7cef
📒 Files selected for processing (35)
openmeter/billing/charges/creditpurchase/handler.goopenmeter/billing/charges/creditpurchase/service/external.goopenmeter/billing/charges/creditpurchase/service/invoice.goopenmeter/billing/charges/flatfee/bookedat.goopenmeter/billing/charges/flatfee/handler.goopenmeter/billing/charges/flatfee/service/creditheninvoice.goopenmeter/billing/charges/flatfee/service/creditsonly.goopenmeter/billing/charges/flatfee/service/lineengine.goopenmeter/billing/charges/flatfee/service/payment.goopenmeter/billing/charges/flatfee/service/realizations/correct.goopenmeter/billing/charges/flatfee/service/realizations/creditsonly.goopenmeter/billing/charges/flatfee/service/realizations/credittheninvoice.goopenmeter/billing/charges/flatfee/service/realizations/invoiceaccrued.goopenmeter/billing/charges/service/creditpurchase_test.goopenmeter/billing/charges/service/handlers_test.goopenmeter/billing/charges/service/invoicable_test.goopenmeter/billing/charges/service/taxcode_test.goopenmeter/billing/charges/testutils/handlers.goopenmeter/billing/charges/usagebased/handler.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/credits.goopenmeter/billing/charges/usagebased/service/run/invoice.goopenmeter/billing/charges/usagebased/service/run/payment.goopenmeter/billing/worker/subscriptionsync/service/base_test.goopenmeter/billing/worker/subscriptionsync/service/sync_credittheninvoice_test.goopenmeter/ledger/chargeadapter/creditpurchase.goopenmeter/ledger/chargeadapter/creditpurchase_test.goopenmeter/ledger/chargeadapter/flatfee.goopenmeter/ledger/chargeadapter/flatfee_test.goopenmeter/ledger/chargeadapter/usagebased.goopenmeter/ledger/chargeadapter/usagebased_test.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/bookedat_test.go`:
- Around line 13-19: Both helpers (requireLedgerBookedAtEqual and
requireLedgerBookedAtNotEqual) only truncate expected to microseconds, so
normalize both timestamps before comparing: convert actual and expected to UTC
and call Truncate(time.Microsecond) on each, then use Equal in
require.True/require.False; update the comparison expressions in both helper
functions (referencing requireLedgerBookedAtEqual and
requireLedgerBookedAtNotEqual) accordingly.
🪄 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: c156ebc2-7ed5-4017-af03-a3769d6eda44
📒 Files selected for processing (5)
openmeter/ledger/chargeadapter/bookedat_test.goopenmeter/ledger/chargeadapter/creditpurchase_test.goopenmeter/ledger/chargeadapter/flatfee_test.goopenmeter/ledger/chargeadapter/usagebased_test.goopenmeter/ledger/customerbalance/expired_loader_test.go
tothandras
approved these changes
May 21, 2026
GAlexIHU
approved these changes
May 21, 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
3 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.
Business impact
Credit-then-invoice and credit-only charge flows now book ledger transactions at the domain event time they represent instead of deriving timestamps inside the ledger adapter. This makes customer balances and revenue staging auditable by service period:
This prevents balance history from drifting to invoice dates, stored-at cutoffs, or wall-clock correction times, which is especially important for credit allocation, rollback, and subscription sync validation.
Technical notes
Validation
Summary by CodeRabbit