feat: package and dynamic price to unitconfig translation#4309
Conversation
|
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:
📝 WalkthroughWalkthroughAdds a read-only ChangesDynamic/Package Price Translation to v3 Unit Pricing
Sequence DiagramsequenceDiagram
participant Client
participant v3API as "v3 API Handler"
participant Conv as "Conversion Logic"
participant Resp as "v3 Response"
Client->>v3API: GET /Plan or LIST /Plans
v3API->>Conv: ToAPIBillingPlan(v1Plan)
Conv->>Conv: ToAPIBillingRateCard(v1RateCard)
alt Dynamic price
Conv->>Conv: ToAPIBillingRateCardUnitConfig() -> Multiply unit_config
Conv->>Conv: ToAPIBillingPrice() -> unit_price (amount "1")
else Package price
Conv->>Conv: ToAPIBillingRateCardUnitConfig() -> Divide unit_config + ceiling rounding
Conv->>Conv: ToAPIBillingPrice() -> unit_price (package amount)
end
Conv->>Resp: v3 rate card with unit_price & unit_config
Resp->>Client: v3 API Response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
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 unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
rolosp
force-pushed
the
feat/add-unitconfig-to-ratecards
branch
from
55bbffd to
85beb92
Compare
rolosp
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/plans_v3_test.go (1)
607-638: 💤 Low valueHandy helpers — small naming nit.
findRateCardByKeyandassertUnitPriceAmountare reusable across other v3 plan tests. If you anticipate more callers outside this file, consider moving them next to the other shared assert helpers (assertValidationCode,assertProblemDetail) so they're easy to discover. Totally optional.🤖 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 `@e2e/plans_v3_test.go` around lines 607 - 638, The two helper functions findRateCardByKey and assertUnitPriceAmount are reusable and should be moved next to the other shared test helpers (e.g. where assertValidationCode and assertProblemDetail live) so they’re easier to discover and reuse; relocate these functions from e2e/plans_v3_test.go into the shared helpers test file in the same package, keep their names and signatures (they can remain unexported), update any import or test references if necessary, and run the tests to ensure no package name or test visibility issues.
🤖 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 `@e2e/plans_v3_test.go`:
- Around line 607-638: The two helper functions findRateCardByKey and
assertUnitPriceAmount are reusable and should be moved next to the other shared
test helpers (e.g. where assertValidationCode and assertProblemDetail live) so
they’re easier to discover and reuse; relocate these functions from
e2e/plans_v3_test.go into the shared helpers test file in the same package, keep
their names and signatures (they can remain unexported), update any import or
test references if necessary, and run the tests to ensure no package name or
test visibility issues.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 90c5ceba-81f3-4c90-a2de-a725fcff437e
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (6)
api/spec/packages/aip/src/productcatalog/ratecard.tspapi/v3/api.gen.goapi/v3/handlers/plans/convert.goapi/v3/handlers/plans/convert_test.goapi/v3/handlers/plans/list.goe2e/plans_v3_test.go
💤 Files with no reviewable changes (1)
- api/v3/handlers/plans/list.go
rolosp
force-pushed
the
feat/add-unitconfig-to-ratecards
branch
from
85beb92 to
97a4bf0
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (4)
e2e/plans_v3_test.go (1)
463-605: 💤 Low valueReally nice translation e2e — covers the full read-side contract. 🎉
The test exercises both
GETandLISTpaths, asserts the synthesized unit price and unit config for both dynamic and package, and verifies commitments round-trip. The cross-reference back toconvert.goin the doc comment is super helpful for future readers. The use oft.Context()on line 523 is also a nice match with the test-harness lifecycle guidance.One small nit — when this test fails, the partially-created v1 plan will linger on the shared DB across runs (it uses a unique suffix so it won't collide, but it does accumulate). If you want to be tidy, either an archive+delete in a
t.Cleanupafter the create succeeds, or relying on the suite's broader cleanup, would help. Plenty of other tests here also don't clean up, so feel free to skip if that's the established pattern.🤖 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 `@e2e/plans_v3_test.go` around lines 463 - 605, Test leaves created v1 plan in shared DB on failures; add cleanup to remove it. After the successful create in TestV3PlanReadTranslatesV1DynamicAndPackagePrices (the block that calls v1.CreatePlanWithResponse and sets planID), register a t.Cleanup (or defer) that deletes or archives the plan using planID (call the v1 client delete/archive method or appropriate endpoint) so any partially-created plan is removed when the test finishes; ensure you only register the cleanup after planID is set to a non-empty value and reference planID and v1.* methods to locate where to add the cleanup.api/v3/handlers/plans/convert_test.go (1)
568-620: 💤 Low valueGreat coverage for the new helper — one tiny gap.
You've covered nil/flat/unit/dynamic/package, which hits the main intent. As a small nice-to-have, you could add a tiered/graduated/volume case asserting
unit_configstays nil — that would lock down thedefault:branch behavior and protect against someone accidentally extending the switch later.t.Run("graduated price has no unit config", func(t *testing.T) { p := productcatalog.NewPriceFrom(productcatalog.TieredPrice{ Mode: productcatalog.GraduatedTieredPrice, Tiers: []productcatalog.PriceTier{}, }) result, err := ToAPIBillingRateCardUnitConfig(p) require.NoError(t, err) assert.Nil(t, result) })🤖 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 `@api/v3/handlers/plans/convert_test.go` around lines 568 - 620, Add a test case inside TestToAPIBillingRateCardUnitConfig that constructs a productcatalog.TieredPrice with Mode set to productcatalog.GraduatedTieredPrice (and empty Tiers), calls ToAPIBillingRateCardUnitConfig with that price, asserts no error, and asserts the returned unit config is nil to lock down the default: branch behavior for tiered/graded prices.api/v3/handlers/plans/convert.go (2)
154-190: 💤 Low valueSolid translation helper — one small thought on duplicated
AsPackage/AsDynamiccalls.The synthesis logic reads cleanly and the contract is nicely scoped (only fires for dynamic/package, otherwise nil). One minor observation: this helper plus
ToAPIBillingPriceeach independently callp.AsPackage()/p.AsDynamic()for the same price during a singleToAPIBillingRateCardinvocation. Not a hot path concern, but if you ever want to tighten things, you could fold the unit-config synthesis into a single switch inToAPIBillingRateCardthat destructures the price once and emits both the API price and the unit config together. Totally optional — the current split reads very clearly as-is.🤖 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 `@api/v3/handlers/plans/convert.go` around lines 154 - 190, Duplicate AsPackage/AsDynamic calls occur when ToAPIBillingRateCard invokes both ToAPIBillingPrice and ToAPIBillingRateCardUnitConfig; refactor by collapsing unit-config synthesis into ToAPIBillingRateCard so the price is destructured once: in ToAPIBillingRateCard call p.AsDynamic()/p.AsPackage() a single time, build the API price (used by ToAPIBillingPrice or inline) and simultaneously create the *api.BillingUnitConfig (replacing ToAPIBillingRateCardUnitConfig), returning/attaching both results; update or remove ToAPIBillingRateCardUnitConfig and adjust callers accordingly so no duplicate As*() calls occur.
175-185: 💤 Low valueUpstream v1 validation already guards against this. The
PackagePrice.Validate()method explicitly checks thatQuantityPerPackageis non-zero (openmeter/productcatalog/price.go:978-980), so bad historical rows with zero values can't actually exist in the first place. That said, if you want to be extra defensive here in the conversion path, adding a check forpkg.QuantityPerPackage.IsZero()before returning the config would be a nice belt-and-suspenders safeguard against any future mutations that might bypass validation.🤖 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 `@api/v3/handlers/plans/convert.go` around lines 175 - 185, The conversion for productcatalog.PackagePriceType should defensively check for a zero package quantity before returning the BillingUnitConfig: after calling p.AsPackage() and obtaining pkg, call pkg.QuantityPerPackage.IsZero() (or equivalent) and return an error if true instead of constructing api.BillingUnitConfig, so functions like productcatalog.PackagePriceType handling and api.BillingUnitConfig creation (Operation: api.BillingUnitConfigOperationDivide, ConversionFactor: pkg.QuantityPerPackage.String()) never proceed with a zero QuantityPerPackage despite upstream validation in PackagePrice.Validate().
🤖 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 `@api/v3/handlers/plans/convert_test.go`:
- Around line 568-620: Add a test case inside TestToAPIBillingRateCardUnitConfig
that constructs a productcatalog.TieredPrice with Mode set to
productcatalog.GraduatedTieredPrice (and empty Tiers), calls
ToAPIBillingRateCardUnitConfig with that price, asserts no error, and asserts
the returned unit config is nil to lock down the default: branch behavior for
tiered/graded prices.
In `@api/v3/handlers/plans/convert.go`:
- Around line 154-190: Duplicate AsPackage/AsDynamic calls occur when
ToAPIBillingRateCard invokes both ToAPIBillingPrice and
ToAPIBillingRateCardUnitConfig; refactor by collapsing unit-config synthesis
into ToAPIBillingRateCard so the price is destructured once: in
ToAPIBillingRateCard call p.AsDynamic()/p.AsPackage() a single time, build the
API price (used by ToAPIBillingPrice or inline) and simultaneously create the
*api.BillingUnitConfig (replacing ToAPIBillingRateCardUnitConfig),
returning/attaching both results; update or remove
ToAPIBillingRateCardUnitConfig and adjust callers accordingly so no duplicate
As*() calls occur.
- Around line 175-185: The conversion for productcatalog.PackagePriceType should
defensively check for a zero package quantity before returning the
BillingUnitConfig: after calling p.AsPackage() and obtaining pkg, call
pkg.QuantityPerPackage.IsZero() (or equivalent) and return an error if true
instead of constructing api.BillingUnitConfig, so functions like
productcatalog.PackagePriceType handling and api.BillingUnitConfig creation
(Operation: api.BillingUnitConfigOperationDivide, ConversionFactor:
pkg.QuantityPerPackage.String()) never proceed with a zero QuantityPerPackage
despite upstream validation in PackagePrice.Validate().
In `@e2e/plans_v3_test.go`:
- Around line 463-605: Test leaves created v1 plan in shared DB on failures; add
cleanup to remove it. After the successful create in
TestV3PlanReadTranslatesV1DynamicAndPackagePrices (the block that calls
v1.CreatePlanWithResponse and sets planID), register a t.Cleanup (or defer) that
deletes or archives the plan using planID (call the v1 client delete/archive
method or appropriate endpoint) so any partially-created plan is removed when
the test finishes; ensure you only register the cleanup after planID is set to a
non-empty value and reference planID and v1.* methods to locate where to add the
cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 07a88067-b1cd-42eb-b002-07d85e8ed7c8
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (6)
api/spec/packages/aip/src/productcatalog/ratecard.tspapi/v3/api.gen.goapi/v3/handlers/plans/convert.goapi/v3/handlers/plans/convert_test.goapi/v3/handlers/plans/list.goe2e/plans_v3_test.go
💤 Files with no reviewable changes (1)
- api/v3/handlers/plans/list.go
rolosp
force-pushed
the
feat/add-unitconfig-to-ratecards
branch
from
97a4bf0 to
298e4c2
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
api/v3/handlers/plans/convert_test.go (1)
568-620: 💤 Low value
TestToAPIBillingRateCardUnitConfig— LGTM on the happy paths.One small optional gap: there are no cases for graduated/volume tiered prices to confirm they produce a
nilunit config. If a future change accidentally wires in tiered price handling, this would catch the regression early. Totally fine to leave for later, but it'd make the suite more bulletproof.✨ Suggested additional cases
+ t.Run("graduated tiered price has no unit config", func(t *testing.T) { + p := productcatalog.NewPriceFrom(productcatalog.TieredPrice{ + Mode: productcatalog.GraduatedTieredPrice, + Tiers: []productcatalog.PriceTier{}, + }) + + result, err := ToAPIBillingRateCardUnitConfig(p) + require.NoError(t, err) + assert.Nil(t, result) + }) + + t.Run("volume tiered price has no unit config", func(t *testing.T) { + p := productcatalog.NewPriceFrom(productcatalog.TieredPrice{ + Mode: productcatalog.VolumeTieredPrice, + Tiers: []productcatalog.PriceTier{}, + }) + + result, err := ToAPIBillingRateCardUnitConfig(p) + require.NoError(t, err) + assert.Nil(t, result) + })🤖 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 `@api/v3/handlers/plans/convert_test.go` around lines 568 - 620, Add tests inside TestToAPIBillingRateCardUnitConfig to assert that tiered price types return a nil unit config: create prices with productcatalog.NewPriceFrom(productcatalog.GraduatedPrice{ /* minimal tiers */ }) and productcatalog.NewPriceFrom(productcatalog.VolumeTieredPrice{ /* minimal tiers */ }), call ToAPIBillingRateCardUnitConfig on each, require.NoError(t, err) and assert.Nil(t, result). This ensures ToAPIBillingRateCardUnitConfig (the function under test) continues to return nil for graduated and volume-tiered price types.e2e/plans_v3_test.go (1)
628-638: ⚡ Quick winConsider using
apiv3.Numeric(want)for consistency with the rest of the file.Since
apiv3.Numericis a type alias tostring, the test will work fine as-is. However, the rest of the file (e.g., lines 544, 565, 595, 601) usesapiv3.Numeric("...")explicitly, so wrappingwantin the same cast would make this helper more consistent:Optional consistency improvement
- assert.Equal(t, want, unit.Amount) + assert.Equal(t, apiv3.Numeric(want), unit.Amount)🤖 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 `@e2e/plans_v3_test.go` around lines 628 - 638, The helper assertUnitPriceAmount should compare the unit.Amount using the same apiv3.Numeric typed value used elsewhere; inside assertUnitPriceAmount (function name) wrap the want string with apiv3.Numeric(want) when asserting against unit.Amount so the assertion uses apiv3.Numeric consistently with other tests that set numeric values (rc.Price, AsBillingPriceUnit, BillingRateCard types remain unchanged).
🤖 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 `@api/v3/handlers/plans/convert_test.go`:
- Around line 568-620: Add tests inside TestToAPIBillingRateCardUnitConfig to
assert that tiered price types return a nil unit config: create prices with
productcatalog.NewPriceFrom(productcatalog.GraduatedPrice{ /* minimal tiers */
}) and productcatalog.NewPriceFrom(productcatalog.VolumeTieredPrice{ /* minimal
tiers */ }), call ToAPIBillingRateCardUnitConfig on each, require.NoError(t,
err) and assert.Nil(t, result). This ensures ToAPIBillingRateCardUnitConfig (the
function under test) continues to return nil for graduated and volume-tiered
price types.
In `@e2e/plans_v3_test.go`:
- Around line 628-638: The helper assertUnitPriceAmount should compare the
unit.Amount using the same apiv3.Numeric typed value used elsewhere; inside
assertUnitPriceAmount (function name) wrap the want string with
apiv3.Numeric(want) when asserting against unit.Amount so the assertion uses
apiv3.Numeric consistently with other tests that set numeric values (rc.Price,
AsBillingPriceUnit, BillingRateCard types remain unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c70692fe-2d69-4750-abc8-6afb1a2de4a9
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (6)
api/spec/packages/aip/src/productcatalog/ratecard.tspapi/v3/api.gen.goapi/v3/handlers/plans/convert.goapi/v3/handlers/plans/convert_test.goapi/v3/handlers/plans/list.goe2e/plans_v3_test.go
💤 Files with no reviewable changes (1)
- api/v3/handlers/plans/list.go
🚧 Files skipped from review as they are similar to previous changes (2)
- api/spec/packages/aip/src/productcatalog/ratecard.tsp
- api/v3/handlers/plans/convert.go
rolosp
force-pushed
the
feat/add-unitconfig-to-ratecards
branch
from
298e4c2 to
604fd3e
Compare
rolosp
force-pushed
the
feat/add-unitconfig-to-ratecards
branch
from
604fd3e to
0c46558
Compare
rolosp
force-pushed
the
feat/add-unitconfig-to-ratecards
branch
from
0c46558 to
7c04d41
Compare
tothandras
force-pushed
the
feat/add-unitconfig-to-ratecards
branch
from
7c04d41 to
7d25b83
Compare
rolosp
force-pushed
the
feat/add-unitconfig-to-ratecards
branch
from
7d25b83 to
99c1e93
Compare
rolosp
force-pushed
the
feat/add-unitconfig-to-ratecards
branch
from
476c4e0 to
b7f7e1a
Compare
2 participants
Overview
v3 plan reads previously errored (GET) or silently dropped (LIST) any v1 plan
whose rate cards used
DynamicPriceorPackagePrice, since v3 does notexpose those types. This PR translates them on the read path:
dynamic(multiplier=m)→unit(amount=1)+unit_config{operation=multiply, conversion_factor=m}package(amount=a, qpp=q)→unit(amount=a)+unit_config{operation=divide, conversion_factor=q, rounding=ceiling}unit_configis added to the v3RateCardTypeSpec with@visibility(Lifecycle.Read)so it appears onresponses but is not accepted on create/update (no domain type, no schema
change, no migration)
Notes for reviewer
v3 → v1 write path is untouched; this is read-only translation. The
asymmetry (arbitrary
UnitConfighas no v1 equivalent) only becomes aconcern once v3 authoring is added — punted via the read-only visibility.
Summary by CodeRabbit
New Features
Bug Fixes
Tests