fix(billing): repair persisted charge discount IDs - #4892
Conversation
📝 WalkthroughWalkthrough
ChangesCharge discount correlation handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ 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 |
![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/billing/charges/service/pendinglines_test.go (1)
172-183: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd a flat-fee stale-snapshot regression case.
Line 175 collects only the usage-based line because the flat-fee line has a later
InvoiceAt. A regression in the changed flat-fee line builder can pass this test. Persist a stale flat-fee percentage correlation ID, collect that line, and assert the effective flat-fee intent ID.As per path instructions, “Make sure the tests are comprehensive and cover the changes.”
🤖 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/pendinglines_test.go` around lines 172 - 183, Extend the pending-lines regression coverage around InvoicePendingLines and the existing usage assertion to persist a stale flat-fee percentage correlation ID, collect the flat-fee line after its later InvoiceAt, and assert its generated RateCardDiscounts flat-fee correlation ID matches the effective flat-fee intent rather than the stale snapshot. Keep the usage-based scenario and assertions intact.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 `@tools/migrate/charge_discount_correlation_ids_test.go`:
- Around line 176-190: Expand the migration fixtures and assertions in the
relevant test to cover discount objects containing an explicit JSON null
correlationID, distinct from SQL NULL discounts. Add this case for the
applicable charge tables and assert the migration generates a non-empty
correlation ID while preserving existing and empty-string behaviors.
---
Outside diff comments:
In `@openmeter/billing/charges/service/pendinglines_test.go`:
- Around line 172-183: Extend the pending-lines regression coverage around
InvoicePendingLines and the existing usage assertion to persist a stale flat-fee
percentage correlation ID, collect the flat-fee line after its later InvoiceAt,
and assert its generated RateCardDiscounts flat-fee correlation ID matches the
effective flat-fee intent rather than the stale snapshot. Keep the usage-based
scenario and assertions intact.
🪄 Autofix
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: eb6f8f6f-3bd8-4a5c-9740-18c446fcee88
⛔ Files ignored due to path filters (1)
tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (6)
openmeter/billing/charges/flatfee/service/lineengine.goopenmeter/billing/charges/service/pendinglines_test.goopenmeter/billing/charges/usagebased/service/lineengine.gotools/migrate/charge_discount_correlation_ids_test.gotools/migrate/migrations/20260809172658_backfill_persisted_charge_discount_correlation_ids.down.sqltools/migrate/migrations/20260809172658_backfill_persisted_charge_discount_correlation_ids.up.sql
turip
added
kind/bug
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openmeter/billing/charges/service/pendinglines_test.go (1)
210-212: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPair the second clock freeze with its cleanup.
When the test calls
clock.FreezeTime(flatLine.InvoiceAt)at Line 211, adddefer clock.UnFreeze()immediately after it. The existing defer at Line 49 cleans up at function exit, but it does not follow the required local lifecycle pattern.Proposed cleanup
- clock.FreezeTime(flatLine.InvoiceAt) + clock.FreezeTime(flatLine.InvoiceAt) + defer clock.UnFreeze()As per coding guidelines, “Pair
clock.FreezeTime(...)immediately withdefer clock.UnFreeze()in the same scope.”🤖 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/pendinglines_test.go` around lines 210 - 212, In the test flow around the second clock.FreezeTime call for flatLine.InvoiceAt, immediately add a matching defer clock.UnFreeze() in the same scope. Keep the existing function-level cleanup unchanged.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.
Nitpick comments:
In `@openmeter/billing/charges/service/pendinglines_test.go`:
- Around line 210-212: In the test flow around the second clock.FreezeTime call
for flatLine.InvoiceAt, immediately add a matching defer clock.UnFreeze() in the
same scope. Keep the existing function-level cleanup unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c984e31-b8df-495d-a93a-9ca36dc42123
📒 Files selected for processing (2)
openmeter/billing/charges/service/pendinglines_test.gotools/migrate/charge_discount_correlation_ids_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/migrate/charge_discount_correlation_ids_test.go
2 participants
Summary
Root cause
The previous repair only updated gathering invoice-line snapshots. Detailed rating reads discounts from the persisted charge's effective intent, so legacy charges could still reach rating without a correlation ID. Create-time normalization does not repair those existing rows, and the rating failure occurs before the later persistence fallback can run.
Impact
Legacy charge-backed invoice lines can be collected and rated after deployment. Existing correlation IDs are preserved; only missing or empty IDs are generated. Standard-line construction also treats the effective charge intent as the authoritative discount identity instead of carrying a stale gathering snapshot forward.
Validation
POSTGRES_HOST=127.0.0.1 go test -tags=dynamic ./openmeter/billing/charges/... ./tools/migratemake migrate-check-lint migrate-check-validatenix develop --impure .#ci -c golangci-lint run --config .golangci-fast.yaml ./openmeter/billing/charges/... ./tools/migrate/...Summary by CodeRabbit
Bug Fixes
Data Integrity
Greptile Summary
This follow-up repairs legacy persisted charge discount identities and refreshes gathering-line discount snapshots from effective charge intents during standard invoice construction.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Sequence Diagram
Reviews (3): Last reviewed commit: "test(billing): pair frozen clock cleanup" | Re-trigger Greptile
Context used: