fix: flat fee invoice-at lifecycle#4618
Conversation
Greptile SummaryThis PR moves flat-fee lifecycle timing to
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (4): Last reviewed commit: "fix(billing): schedule recreated flat fe..." | Re-trigger Greptile |
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughFlat-fee charge timing now uses ChangesFlat-fee InvoiceAt timing changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
8078b2b to
32e395c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
openmeter/billing/charges/service/invoicable_test.go (1)
945-973: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a pre-
invoice_atassertion to lock in the new gate.This only checks the happy path once the clock reaches
invoice_at. If pending lines accidentally become invoiceable earlier, this test still passes. A quickInvoicePendingLinesassertion before Line 967 expecting no invoices would pin the new timing contract directly. As per path instructions,**/*_test.go:Suggest missing tests only when the PR introduces behavior without meaningful coverage, or when the current tests would pass despite a concrete regression in the changed code.🤖 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/service/invoicable_test.go` around lines 945 - 973, The timing gate around invoicing pending flat-fee lines is only covered at invoice_at, so the test in invoicable_test.go could still pass even if lines become invoiceable too early. Add an assertion in the same test around BillingService.InvoicePendingLines that runs before invoiceAt and verifies no invoices are created, using the existing test setup around clock.FreezeTime, invoiceAt, and InvoicePendingLines to lock in the pre-invoice_at contract.Source: Path instructions
openmeter/ledger/customerbalance/service_test.go (1)
370-384: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the balance at
invoice_atbefore advancing the charge.Right now the key live-balance check happens only after
AdvanceCharge. IfchargeHasStartedregressed to a status-based check, this test would still pass once the state machine moves the charge toactive. A balance assertion right after Line 370 and before Line 371 would pin theinvoice_atrule directly. As per path instructions,**/*_test.go:Suggest missing tests only when the PR introduces behavior without meaningful coverage, or when the current tests would pass despite a concrete regression in the changed code.🤖 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/customerbalance/service_test.go` around lines 370 - 384, Add a balance assertion in customerbalance/service_test.go using the existing test flow around AdvanceCharge: check the live balance immediately after clock.FreezeTime(invoiceAt) and before calling flatFeeService.AdvanceCharge. This should use the same requireBalance helper and the current charge/servicePeriod setup so the test directly verifies the invoice_at rule even if chargeHasStarted regresses to a status-based check.Source: Path instructions
🤖 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/charges/service/invoicable_test.go`:
- Line 967: The test in invoicable_test.go freezes time with
clock.FreezeTime(invoiceAt) but does not pair it with a same-scope cleanup; add
a defer clock.UnFreeze() immediately after the freeze in the same subtest scope.
Keep the cleanup local to the test block around the FreezeTime call so it does
not rely on outer teardown and remains stable if the test structure changes.
In `@openmeter/ledger/customerbalance/service_test.go`:
- Around line 370-403: The test uses multiple clock.FreezeTime calls in separate
phases of the customer balance scenario, but each freeze is not immediately
paired with a same-scope defer clock.UnFreeze(), which makes the cleanup
fragile. Update the test around the AdvanceCharge and requireBalance steps so
each frozen-clock phase has its own local cleanup using clock.UnFreeze(), and
keep the pairing close to the FreezeTime call to preserve isolation even if the
phases are reordered.
---
Nitpick comments:
In `@openmeter/billing/charges/service/invoicable_test.go`:
- Around line 945-973: The timing gate around invoicing pending flat-fee lines
is only covered at invoice_at, so the test in invoicable_test.go could still
pass even if lines become invoiceable too early. Add an assertion in the same
test around BillingService.InvoicePendingLines that runs before invoiceAt and
verifies no invoices are created, using the existing test setup around
clock.FreezeTime, invoiceAt, and InvoicePendingLines to lock in the
pre-invoice_at contract.
In `@openmeter/ledger/customerbalance/service_test.go`:
- Around line 370-384: Add a balance assertion in
customerbalance/service_test.go using the existing test flow around
AdvanceCharge: check the live balance immediately after
clock.FreezeTime(invoiceAt) and before calling flatFeeService.AdvanceCharge.
This should use the same requireBalance helper and the current
charge/servicePeriod setup so the test directly verifies the invoice_at rule
even if chargeHasStarted regresses to a status-based check.
🪄 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: 738b3129-0702-491b-9597-20dd6479ae66
📒 Files selected for processing (7)
openmeter/billing/charges/flatfee/service/create.goopenmeter/billing/charges/flatfee/service/creditheninvoice.goopenmeter/billing/charges/flatfee/service/creditsonly.goopenmeter/billing/charges/flatfee/service/statemachine.goopenmeter/billing/charges/service/invoicable_test.goopenmeter/ledger/customerbalance/service.goopenmeter/ledger/customerbalance/service_test.go
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 `@test/credits/credit_then_invoice_test.go`:
- Line 2135: The `clock.FreezeTime(...)` usage in `credit_then_invoice_test.go`
is leaking shared frozen state across subtests. Update each affected subtest in
the `Test...` flow to freeze time within that subtest’s own scope and
immediately pair it with `defer clock.UnFreeze()` in the same block. Use the
local subtest bodies around the `clock.FreezeTime(servicePeriod.To)` calls so
each case manages its own clock state independently.
🪄 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: 9961247e-dbbf-4340-9d05-caa6af004258
📒 Files selected for processing (8)
openmeter/billing/charges/flatfee/service/create.goopenmeter/billing/charges/flatfee/service/creditheninvoice.goopenmeter/billing/charges/flatfee/service/creditsonly.goopenmeter/billing/charges/flatfee/service/statemachine.goopenmeter/billing/charges/service/invoicable_test.goopenmeter/ledger/customerbalance/service.goopenmeter/ledger/customerbalance/service_test.gotest/credits/credit_then_invoice_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- openmeter/billing/charges/flatfee/service/create.go
- openmeter/ledger/customerbalance/service.go
- openmeter/billing/charges/flatfee/service/creditheninvoice.go
- openmeter/ledger/customerbalance/service_test.go
- openmeter/billing/charges/flatfee/service/statemachine.go
- openmeter/billing/charges/service/invoicable_test.go
- openmeter/billing/charges/flatfee/service/creditsonly.go
f97ce33 to
3d37f24
Compare
Summary
Fixes flat-fee lifecycle scheduling when
invoice_atand the service period do not start at the same time.invoice_atcreatedtoactiveatinvoice_at, then remain open until the flat-fee ledgerbooked_atinvoice_atRoot Cause
Flat-fee invoice collection was governed by
invoice_at, but the charge lifecycle and customer balance gates still assumed service-period start for flat fees. That let invoice collection try to process charge lines while the charge remainedcreated, producing unsupportedfinal_invoice_createdtransitions.For credits-only flat fees, there is an additional balance window: allocation is booked at flat-fee
booked_at(service_period.fromfor in-advance,service_period.tofor in-arrears), while invoice readiness can be earlier. The charge must remain non-final untilbooked_atso customer balance can continue projecting the live impact before the ledger booking is visible.Validation
env POSTGRES_HOST=127.0.0.1 go test -tags=dynamic ./openmeter/ledger/customerbalanceenv POSTGRES_HOST=127.0.0.1 go test -tags=dynamic ./openmeter/billing/charges/flatfee/service ./openmeter/billing/charges/serviceenv POSTGRES_HOST=127.0.0.1 go vet -tags=dynamic ./openmeter/ledger/customerbalance ./openmeter/billing/charges/flatfee/service ./openmeter/billing/charges/servicemake test-nocachecurrently fails intest/creditsbecause existing flat-fee credit-then-invoice tests still expect service-period-start activation.make lint-go-fastcould not run in the ambient shell becausegolangci-lintis not installed there.Summary by CodeRabbit
Bug Fixes
Tests