feat: snapshot unit config on invoice lines - OM-398#4642
Open
rolosp wants to merge 3 commits into
Open
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 ignored due to path filters (10)
📒 Files selected for processing (19)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (16)
📝 WalkthroughWalkthroughThis PR adds unit-config snapshots to billing lines, persists them through gathering and invoice storage, propagates them into standard lines, and validates unit configs before rating mutation. Tests cover round trips, cloning, and invalid input handling. ChangesApplied unit config snapshot feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant StateItem
participant GatheringLine
participant Adapter
participant Database
participant StandardLine
StateItem->>GatheringLine: GetExpectedLine()
GatheringLine->>GatheringLine: clone UnitConfig if present
GatheringLine->>Adapter: upsert usage-based line config
Adapter->>Database: persist unit_config
Database-->>Adapter: read stored snapshot
Adapter->>StandardLine: map UnitConfig
GatheringLine->>StandardLine: AsNewStandardLine()
StandardLine-->>StateItem: GetUnitConfig()
sequenceDiagram
participant RatingService
participant UnitConfigMutator
participant Line
RatingService->>UnitConfigMutator: Mutate(unitConfig, line)
UnitConfigMutator->>UnitConfigMutator: unitConfig.Validate()
alt invalid config
UnitConfigMutator-->>RatingService: error "invalid unit_config on line"
else valid config
UnitConfigMutator->>Line: ApplyUnitConfig
UnitConfigMutator-->>RatingService: success
end
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 completes the
Confidence Score: 5/5Safe to merge — the change is additive (nullable column, no existing row affected), both write paths are covered by tests, and the upsert conflict-resolution logic is validated by a dedicated mixed-batch adapter test. The implementation follows the established Ent upsert pattern consistently: The generated 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
2 times, most recently
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
turip
reviewed
Jul 6, 2026
| // row is never re-upserted with a different (or removed) unit_config. Left unset | ||
| // when nil (no unit_config rate card) so non-unit_config lines write NULL on | ||
| // create, matching the standard-line convention; because the value is never | ||
| // changed non-nil->nil on an existing row, ResolveWithNewValues preserving an |
Member
There was a problem hiding this comment.
This is fine, but I would prefer that we do:
UpdateUnitConfig() in the upsert items, as that reflects a codebug if we are not updating it
We shuold also do the same for all missing optional fields.
(Sorry, I know that this is housekeeping as we had this bad practice already, but this behavior will make updates special, and as price is mutable so unit config is via manual invoice edits or charges patching)
| UBPConfigID: ubpLine.ID, | ||
| FeatureKey: lo.FromPtr(ubpLine.FeatureKey), | ||
| Price: lo.FromPtr(ubpLine.Price), | ||
| UnitConfig: ubpLine.AppliedUnitConfig, |
Member
There was a problem hiding this comment.
Why not UnitConfig? this is a diverging pattern.
| // following the price precedent: the guarantee is behavioral — finalized lines | ||
| // are never re-upserted through this path, and no caller carries a different | ||
| // config for an existing line — not enforced at the SQL level. Left unset when | ||
| // nil so a conflicting re-upsert cannot clear an already-captured snapshot. |
Member
There was a problem hiding this comment.
charges/lines can be updated via invoice edit apis.
| @@ -0,0 +1,2 @@ | |||
| -- modify "billing_invoice_usage_based_line_configs" table | |||
| ALTER TABLE "billing_invoice_usage_based_line_configs" ADD COLUMN "applied_unit_config" jsonb NULL; | |||
rolosp
force-pushed
the
feat/om-398-unit-config-snapshot-on-invoice-lines
branch
from
61749a1 to
e35777d
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/billing/adapter_test.go (1)
415-441: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd given/when/then intent comments to these subtests.
The
"snapshot round-trips through persistence"and"line without a unit config stays nil"subtests don't open with an intent comment, while the later two subtests in this same test do. As per path instructions,**/*_test.gonon-trivial subtests should start with concise given/when/then comments — worth adding here for consistency with the rest of the test.✏️ Example
s.Run("snapshot round-trips through persistence", func() { + // given: a line created with a unit_config; when: reading it back; + // then: the persisted snapshot and GetUnitConfig() both match the original config. readBack, err := s.BillingAdapter.ListInvoiceLines(ctx, billing.ListInvoiceLinesAdapterInput{🤖 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 415 - 441, Add concise given/when/then intent comments at the start of the two subtests in s.Run so they match the style used by the later subtests in adapter_test.go. In the "snapshot round-trips through persistence" and "line without a unit config stays nil" blocks, insert a short comment before the assertions that states the setup, action, and expected outcome, keeping the wording consistent with the surrounding billing adapter tests.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.
Nitpick comments:
In `@test/billing/adapter_test.go`:
- Around line 415-441: Add concise given/when/then intent comments at the start
of the two subtests in s.Run so they match the style used by the later subtests
in adapter_test.go. In the "snapshot round-trips through persistence" and "line
without a unit config stays nil" blocks, insert a short comment before the
assertions that states the setup, action, and expected outcome, keeping the
wording consistent with the surrounding billing adapter tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 77e95bfd-0f80-4beb-a2f1-5557a97e4997
⛔ 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 (2)
- openmeter/billing/derived.gen.go
- .agents/skills/ent/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (10)
- openmeter/billing/rating/service/mutator/unitconfig.go
- openmeter/billing/worker/subscriptionsync/service/targetstate/targetstateitem.go
- openmeter/billing/charges/service/unitconfig_rating_test.go
- openmeter/billing/charges/usagebased/service/linemapper.go
- openmeter/billing/adapter/gatheringlines.go
- test/billing/collection_test.go
- test/billing/unitconfig_legacy_test.go
- openmeter/billing/gatheringinvoice.go
- openmeter/billing/gatheringinvoice_test.go
- openmeter/billing/rating/service/mutator/unitconfig_test.go
rolosp
force-pushed
the
feat/om-398-unit-config-snapshot-on-invoice-lines
branch
from
e362114 to
8fe9912
Compare
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
2 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.
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 (NULLwhen unset) and exposes it on usage-based invoice lines, including legacy gathering lines. Standard lines can surface the snapshot viaGetUnitConfig().unit_configwhen omitted; conflict re-sync refreshes the snapshot per line.unit_configmutation now validates inputs to fail gracefully on corrupt data.descriptionandratecard_discounts(plus additional related fields).