feat: snapshot unit config on invoice lines - OM-398#4642
Open
rolosp wants to merge 1 commit into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR adds an ChangesApplied unit config snapshot feature
Estimated code review effort: 3 (Moderate) | ~25 minutes 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 |
Contributor
Greptile SummaryThis PR snapshots the rate card's
Confidence Score: 5/5Safe to merge. The migration is additive (nullable column, no default), both billing paths correctly snapshot the config, the write-once DB contract is sound, and the defensive Validate() guard in the UnitConfig mutator closes the zero-factor panic window. The change is well-scoped: one nullable column, two write paths with consistent nil-guards, a correct openmeter/billing/charges/usagebased/service/linemapper.go — direct pointer assignment is inconsistent with the deep-copy pattern used in gatheringinvoice.go. Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 1
🧹 Nitpick comments (1)
test/billing/adapter_test.go (1)
443-469: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win"Re-upsert keeps the snapshot" test can't actually catch an overwrite regression.
The re-upserted
updatedline still carries the exact sameunitConfigvalue/pointer as the original, so this assertion would pass identically whether the upsert path truly preserves the existing DB snapshot on conflict, or naively overwrites it with whateverAppliedUnitConfighappens to be on the incoming struct. Since the write-once contract is described as behavioral (the caller never sends a different config), a regression where a caller mistakenly sends a different/nil config on a later upsert wouldn't be caught here.Consider adding a case where the second upsert carries a different (or nil)
AppliedUnitConfigvalue and asserting the persisted snapshot doesn't change — that would actually exercise the "write-once" guarantee this test's comment describes.🤖 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 `@test/billing/adapter_test.go` around lines 443 - 469, The “re-upserting the line keeps the snapshot” test is not exercising the overwrite path because the second upsert reuses the same AppliedUnitConfig as the original line. Update the test around UpsertInvoiceLines and ListInvoiceLines so the re-upserted StandardLine carries a different or nil AppliedUnitConfig, then assert the persisted UsageBased.AppliedUnitConfig still matches the original snapshot. Use the existing unitConfig and updated line setup to verify the write-once behavior actually survives a conflicting incoming config.
🤖 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/billing/collection_test.go`:
- Around line 185-186: The round-trip test uses non-fatal s.NoError(err) before
dereferencing pointer results like created and stdLine, which can lead to nil
panics and hide the real failure. Update the checks in the test around the
created/standard line assertions to use s.Require().NoError(err) in the same
style already used later in the test, so execution stops immediately on error
before accessing created.Lines or stdLine.UsageBased...
---
Nitpick comments:
In `@test/billing/adapter_test.go`:
- Around line 443-469: The “re-upserting the line keeps the snapshot” test is
not exercising the overwrite path because the second upsert reuses the same
AppliedUnitConfig as the original line. Update the test around
UpsertInvoiceLines and ListInvoiceLines so the re-upserted StandardLine carries
a different or nil AppliedUnitConfig, then assert the persisted
UsageBased.AppliedUnitConfig still matches the original snapshot. Use the
existing unitConfig and updated line setup to verify the write-once behavior
actually survives a conflicting incoming config.
🪄 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: dac6024e-b47d-474b-b639-a3d2b3e3bdbe
⛔ Files ignored due to path filters (10)
openmeter/ent/db/billinginvoiceusagebasedlineconfig.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoiceusagebasedlineconfig/billinginvoiceusagebasedlineconfig.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoiceusagebasedlineconfig/where.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoiceusagebasedlineconfig_create.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoiceusagebasedlineconfig_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/**openmeter/ent/db/setorclear.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (18)
.agents/skills/ent/SKILL.mdopenmeter/billing/adapter/gatheringlines.goopenmeter/billing/adapter/stdinvoicelinemapper.goopenmeter/billing/adapter/stdinvoicelines.goopenmeter/billing/charges/service/unitconfig_rating_test.goopenmeter/billing/charges/usagebased/service/linemapper.goopenmeter/billing/derived.gen.goopenmeter/billing/gatheringinvoice.goopenmeter/billing/gatheringinvoice_test.goopenmeter/billing/rating/service/mutator/unitconfig.goopenmeter/billing/rating/service/mutator/unitconfig_test.goopenmeter/billing/stdinvoiceline.goopenmeter/billing/worker/subscriptionsync/service/targetstate/targetstateitem.goopenmeter/ent/schema/billing.gotest/billing/adapter_test.gotest/billing/collection_test.gotools/migrate/migrations/20260703084504_applied-unit-config-snapshot.down.sqltools/migrate/migrations/20260703084504_applied-unit-config-snapshot.up.sql
rolosp
force-pushed
the
feat/om-398-unit-config-snapshot-on-invoice-lines
branch
from
db6a1b3 to
e4592de
Compare
rolosp
force-pushed
the
feat/om-398-unit-config-snapshot-on-invoice-lines
branch
from
e4592de to
61749a1
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/billing/unitconfig_legacy_test.go (1)
65-99: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSecond
clock.FreezeTimecall isn't paired with its ownUnFreeze.The coding guidelines call for pairing every
clock.FreezeTime(...)with an immediatedefer clock.UnFreeze(). Line 65 does this correctly, but the re-freeze at Line 99 has no matching unfreeze call nearby — it just relies on the single deferred unfreeze from earlier. SinceUnFreezeis presumably idempotent (just clears the frozen state), this isn't causing a bug here, but it deviates from the stated pattern and could be confusing if someone copies this test as a template.🕒 Suggested fix
// Collect after the line's invoice-at so the completed-period line is billable. clock.FreezeTime(invoiceAt.Add(time.Hour)) + defer clock.UnFreeze()As per coding guidelines: "When using
clock.FreezeTime(...)in tests, immediately pair it 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 `@test/billing/unitconfig_legacy_test.go` around lines 65 - 99, The test has a second clock.FreezeTime call that is not immediately paired with its own defer clock.UnFreeze(), which breaks the test pattern. In this test around the repeated FreezeTime usage in unitconfig_legacy_test.go, add a matching UnFreeze right after the second freeze so each frozen time scope is self-contained and consistent with the existing pattern used earlier in the test.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 `@test/billing/unitconfig_legacy_test.go`:
- Around line 65-99: The test has a second clock.FreezeTime call that is not
immediately paired with its own defer clock.UnFreeze(), which breaks the test
pattern. In this test around the repeated FreezeTime usage in
unitconfig_legacy_test.go, add a matching UnFreeze right after the second freeze
so each frozen time scope is self-contained and consistent with the existing
pattern used earlier in the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b5ebe7c1-1e3f-473a-856b-67aef41d0354
⛔ Files ignored due to path filters (10)
openmeter/ent/db/billinginvoiceusagebasedlineconfig.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoiceusagebasedlineconfig/billinginvoiceusagebasedlineconfig.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoiceusagebasedlineconfig/where.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoiceusagebasedlineconfig_create.gois excluded by!**/ent/db/**openmeter/ent/db/billinginvoiceusagebasedlineconfig_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/**openmeter/ent/db/setorclear.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (19)
.agents/skills/ent/SKILL.mdopenmeter/billing/adapter/gatheringlines.goopenmeter/billing/adapter/stdinvoicelinemapper.goopenmeter/billing/adapter/stdinvoicelines.goopenmeter/billing/charges/service/unitconfig_rating_test.goopenmeter/billing/charges/usagebased/service/linemapper.goopenmeter/billing/derived.gen.goopenmeter/billing/gatheringinvoice.goopenmeter/billing/gatheringinvoice_test.goopenmeter/billing/rating/service/mutator/unitconfig.goopenmeter/billing/rating/service/mutator/unitconfig_test.goopenmeter/billing/stdinvoiceline.goopenmeter/billing/worker/subscriptionsync/service/targetstate/targetstateitem.goopenmeter/ent/schema/billing.gotest/billing/adapter_test.gotest/billing/collection_test.gotest/billing/unitconfig_legacy_test.gotools/migrate/migrations/20260703084504_applied-unit-config-snapshot.down.sqltools/migrate/migrations/20260703084504_applied-unit-config-snapshot.up.sql
✅ Files skipped from review due to trivial changes (1)
- .agents/skills/ent/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (17)
- openmeter/billing/adapter/stdinvoicelinemapper.go
- tools/migrate/migrations/20260703084504_applied-unit-config-snapshot.up.sql
- openmeter/billing/gatheringinvoice_test.go
- openmeter/billing/charges/usagebased/service/linemapper.go
- openmeter/billing/charges/service/unitconfig_rating_test.go
- openmeter/billing/rating/service/mutator/unitconfig.go
- openmeter/billing/derived.gen.go
- openmeter/billing/gatheringinvoice.go
- openmeter/ent/schema/billing.go
- openmeter/billing/rating/service/mutator/unitconfig_test.go
- openmeter/billing/stdinvoiceline.go
- tools/migrate/migrations/20260703084504_applied-unit-config-snapshot.down.sql
- openmeter/billing/adapter/stdinvoicelines.go
- openmeter/billing/adapter/gatheringlines.go
- openmeter/billing/worker/subscriptionsync/service/targetstate/targetstateitem.go
- test/billing/adapter_test.go
- test/billing/collection_test.go
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
1 participant
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
Snapshots
applied_unit_configon usage-based invoice lines so re-rating converts from raw metered quantity using the config applied at billing time, even after the rate card changes. Adds one nullable jsonb column; converted value is recomputed on read.Written on both billing paths (charges line-mapper + legacy line-engine via
AsNewStandardLine), sounitConfigworks even when lines route to the legacy engine.StandardLine.GetUnitConfig()now returns the snapshot.Summary by CodeRabbit
unit_configsnapshot (including legacy gathering lines) and exposes it on usage-based invoice lines for inspection.unit_configinputs are now rejected during rating-time mutation (fails gracefully instead of converting).GetUnitConfig()now returns the stored snapshot when available.