Feat/add entitlement v3 ratecard#4493
Merged
Merged
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:
📝 WalkthroughWalkthroughThis PR adds rate-card entitlement templates as a new feature across the API spec stack. It defines discriminated union types (metered, static, boolean) in TypeSpec, derives JS SDK types and Zod schemas from the spec, implements v3 conversion handlers in both addons and plans packages with usage-period inference logic, validates conversions with round-trip tests, and scaffolds e2e test infrastructure for customer and subscription operations. ChangesRate-Card Entitlement Templates
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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)
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-entitlement-v3-ratecard
branch
from
4a93f25 to
4ef489d
Compare
![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: 6
🧹 Nitpick comments (2)
api/v3/handlers/addons/convert.go (2)
498-624: ⚡ Quick winConsider extracting entitlement conversion to a shared package.
ToAPIBillingRateCardEntitlementandFromAPIBillingRateCardEntitlementare ~120 lines of identical code duplicated across both packages. If either function needs a bug fix or feature change, you'd have to remember to update both places.A shared conversion package (perhaps
api/v3/handlers/sharedorapi/v3/convert) could house these, similar to howlabels.FromMetadatais already shared. This would also make testing easier since you'd only need one set of unit tests for the conversion logic.Not blocking, but worth considering before this pattern spreads to other handlers.
🤖 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/addons/convert.go` around lines 498 - 624, The two large functions ToAPIBillingRateCardEntitlement and FromAPIBillingRateCardEntitlement are duplicated; extract them into a shared package (e.g., api/v3/convert or api/v3/handlers/shared) and replace the local copies with simple delegating calls; specifically, move the implementations (including JSON handling, entitlement type switch, metered/static/boolean logic, and error wrapping) into new functions with the same names in the shared package, update imports where these functions are used, and ensure any helper types or references (productcatalog.MeteredEntitlementTemplate, productcatalog.StaticEntitlementTemplate, apiv3.BillingRateCardEntitlement and related As/From methods, datetime.ISODuration handling, and models.NewGenericValidationError) are accessible from the shared package or passed in, then delete the duplicated implementations in this file and call the centralized versions instead.
498-557: ⚡ Quick winConversion logic looks solid, but it's duplicated in the plans package.
The switch-case handling for metered/static/boolean types is clean, and the error wrapping is thorough. One thing to note: this function is nearly identical to the one in
plans/convert.go. Consider extracting to a shared conversion package to avoid maintaining two copies.🤖 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/addons/convert.go` around lines 498 - 557, The ToAPIBillingRateCardEntitlement conversion logic is duplicated (see ToAPIBillingRateCardEntitlement in this file and the nearly identical function in plans/convert.go); extract the shared switch/cast/json/unmarshal and out population into a single helper in a new shared package (e.g., pkg/convert or apiv3/convert) and replace both implementations to call that helper; specifically move the handling of entitlement.Type(), the metered/static/boolean branches (including metered.AsMetered, static.AsStatic, json.Unmarshal of static.Config, and the FromBillingRateCard* calls) into a shared function (e.g., ConvertEntitlementTemplateToBillingRateCard) and update ToAPIBillingRateCardEntitlement and the function in plans/convert.go to call it and return its result.
🤖 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 `@api/spec/packages/aip/src/entitlements/entitlements.tsp`:
- Around line 36-43: IssueAfterReset.value is declared as float64 but should use
the repo's arbitrary-precision type; change the field type from float64 to
Shared.Numeric for consistency (i.e., update the IssueAfterReset model's value
declaration to Shared.Numeric), keep the `@minValue`(0) constraint and verify the
validation supports Shared.Numeric semantics, and adjust any model imports or
references to ensure Shared is available where IssueAfterReset is defined.
In `@e2e/plan_entitlement_template_v3_test.go`:
- Around line 47-54: The test assumes a pre-existing "tokens_total" meter and
fails in CI; modify the test around the lo.Find(meters.Data...) block to create
the meter when not found by calling c.CreateMeter with an
apiv3.CreateMeterRequest (Key: "tokens_total" and required fields) and assert
success (status == http.StatusOK), then re-fetch or use the returned meter for
assertions; optionally delete the created meter at test teardown to keep tests
isolated.
- Line 27: Replace the British spelling "behaviour" with American "behavior" in
the end-to-end comment string "end-to-end behaviour: create is accepted (201),
the entitlement round-trips on" (the comment in
e2e/plan_entitlement_template_v3_test.go) so the line reads "end-to-end
behavior: create is accepted (201), the entitlement round-trips on".
- Around line 52-53: The equality comparison uses an unnecessary
conversion—remove the explicit string() cast and compare m.Key directly to the
literal (i.e., change return string(m.Key) == "tokens_total" to return m.Key ==
"tokens_total"); this fix should be applied to the closure where m.Key is
checked in the test (MeterKey is already a string alias so no cast is needed).
- Line 127: The predicate that compares feature keys uses an unnecessary
explicit conversion — remove the superfluous string(...) cast around
r.FeatureKey in the comparison so it directly compares r.FeatureKey to
featureKey; locate the comparison expression (the anonymous function returning
string(r.FeatureKey) == featureKey) and change it to directly compare
r.FeatureKey == featureKey to avoid the needless conversion.
- Around line 105-106: The SubjectKeys slice contains an unnecessary type
conversion apiv3.UsageAttributionSubjectKey(subjectKey); remove the redundant
cast and pass subjectKey directly into SubjectKeys (i.e., SubjectKeys:
[]apiv3.UsageAttributionSubjectKey{subjectKey}) ensuring the element type still
matches apiv3.UsageAttributionSubjectKey and updating any imports or variable
declarations if the underlying type differs.
---
Nitpick comments:
In `@api/v3/handlers/addons/convert.go`:
- Around line 498-624: The two large functions ToAPIBillingRateCardEntitlement
and FromAPIBillingRateCardEntitlement are duplicated; extract them into a shared
package (e.g., api/v3/convert or api/v3/handlers/shared) and replace the local
copies with simple delegating calls; specifically, move the implementations
(including JSON handling, entitlement type switch, metered/static/boolean logic,
and error wrapping) into new functions with the same names in the shared
package, update imports where these functions are used, and ensure any helper
types or references (productcatalog.MeteredEntitlementTemplate,
productcatalog.StaticEntitlementTemplate, apiv3.BillingRateCardEntitlement and
related As/From methods, datetime.ISODuration handling, and
models.NewGenericValidationError) are accessible from the shared package or
passed in, then delete the duplicated implementations in this file and call the
centralized versions instead.
- Around line 498-557: The ToAPIBillingRateCardEntitlement conversion logic is
duplicated (see ToAPIBillingRateCardEntitlement in this file and the nearly
identical function in plans/convert.go); extract the shared
switch/cast/json/unmarshal and out population into a single helper in a new
shared package (e.g., pkg/convert or apiv3/convert) and replace both
implementations to call that helper; specifically move the handling of
entitlement.Type(), the metered/static/boolean branches (including
metered.AsMetered, static.AsStatic, json.Unmarshal of static.Config, and the
FromBillingRateCard* calls) into a shared function (e.g.,
ConvertEntitlementTemplateToBillingRateCard) and update
ToAPIBillingRateCardEntitlement and the function in plans/convert.go to call it
and return its result.
🪄 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: 91228799-47b8-49ad-bfcc-911e8347bee3
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (11)
api/spec/packages/aip-client-javascript/src/index.tsapi/spec/packages/aip-client-javascript/src/models/operations/subscriptions.tsapi/spec/packages/aip-client-javascript/src/models/schemas.tsapi/spec/packages/aip-client-javascript/src/models/types.tsapi/spec/packages/aip/src/entitlements/entitlements.tspapi/spec/packages/aip/src/productcatalog/ratecard.tspapi/v3/api.gen.goapi/v3/handlers/addons/convert.goapi/v3/handlers/plans/convert.goe2e/plan_entitlement_template_v3_test.goe2e/v3helpers_test.go
rolosp
force-pushed
the
feat/add-entitlement-v3-ratecard
branch
from
dde7b47 to
2b2f515
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/v3/handlers/plans/convert_test.go (1)
1362-1378: ⚡ Quick winAdd the missing negative-path subtest for usage-period inference.
Nice coverage for the defaulting path. Please also add a case where
usage_periodis omitted andbillingCadenceisnil, and assertFromAPIBillingRateCardEntitlementreturns the validation error. That closes the remaining branch for this behavior.As per coding guidelines, "
**/*_test.go: 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 `@api/v3/handlers/plans/convert_test.go` around lines 1362 - 1378, Add a negative-path subtest that verifies FromAPIBillingRateCardEntitlement returns a validation error when a metered entitlement omits usage_period and billingCadence is nil: create an api.BillingRateCardEntitlement from api.BillingRateCardMeteredEntitlement with Type "metered" and Issue set but no UsagePeriod, call FromAPIBillingRateCardEntitlement with a nil cadence, assert that the call returns a non-nil error (and/or a validation-specific error) instead of succeeding, and ensure the test references FromAPIBillingRateCardEntitlement and AsMetered (if used) to locate the code paths covered.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 `@api/v3/handlers/plans/convert_test.go`:
- Around line 1362-1378: Add a negative-path subtest that verifies
FromAPIBillingRateCardEntitlement returns a validation error when a metered
entitlement omits usage_period and billingCadence is nil: create an
api.BillingRateCardEntitlement from api.BillingRateCardMeteredEntitlement with
Type "metered" and Issue set but no UsagePeriod, call
FromAPIBillingRateCardEntitlement with a nil cadence, assert that the call
returns a non-nil error (and/or a validation-specific error) instead of
succeeding, and ensure the test references FromAPIBillingRateCardEntitlement and
AsMetered (if used) to locate the code paths covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6a9e371f-277c-4d7d-b9c7-05329c59af53
📒 Files selected for processing (1)
api/v3/handlers/plans/convert_test.go
rolosp
force-pushed
the
feat/add-entitlement-v3-ratecard
branch
from
ec5f2d0 to
8953dda
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
api/spec/packages/aip/src/entitlements/entitlements.tsp (1)
57-61: ⚡ Quick winConsider using
Shared.Numericfor thelimitfield for precision consistency.The
limitfield represents a usage allowance quantity. Based on learnings, fields representing amounts, usage values, or quantities should useShared.Numericinstead offloat64to maintain arbitrary precision and consistent serialization across the API specs. This applies to line 61.✨ Suggested change
/** * The amount of usage granted each usage period, in the feature's unit. Usage is * counted against this allowance and the balance resets every usage period. When * `is_soft_limit` is true the subject keeps access after the limit is reached; * otherwise access is denied once the allowance is exhausted. */ `@visibility`(Lifecycle.Read, Lifecycle.Create, Lifecycle.Update) `@summary`("Usage limit") `@minValue`(0) - limit?: float64; + limit?: Shared.Numeric;🤖 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/spec/packages/aip/src/entitlements/entitlements.tsp` around lines 57 - 61, The limit field is typed as float64 but should use Shared.Numeric for arbitrary-precision consistency; update the limit declaration in entitlements.tsp from "limit?: float64;" to "limit?: Shared.Numeric;", add or ensure the Shared namespace/import is present in the file, and adjust any related validation or serialization handling that expects float64 (e.g., consumers of limit or schema generation) to accept Shared.Numeric.Source: Learnings
🤖 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 `@api/v3/handlers/plans/convert_test.go`:
- Around line 1360-1376: Add a new unit test in
api/v3/handlers/plans/convert_test.go that mirrors the metered happy-path but
verifies the validation error path: construct an api.BillingRateCardEntitlement
from api.BillingRateCardMeteredEntitlement with Type "metered" and no Limit, no
UsagePeriod and do not provide a billing cadence pointer; call
FromAPIBillingRateCardEntitlement and assert it returns a non-nil error
(validation error) and no valid domain entitlement, ensuring the test name makes
clear it checks the missing usage_period and billing_cadence validation for
metered entitlements.
- Around line 1300-1377: Add a new subtest inside
TestFromToAPIBillingRateCardEntitlement that exercises the boolean entitlement
variant: construct an api.BillingRateCardEntitlement via
apiEnt.FromBillingRateCardBooleanEntitlement(...) (or the corresponding
constructor), call FromAPIBillingRateCardEntitlement(apiEnt, nil), then
ToAPIBillingRateCardEntitlement(domain), and finally call
out.AsBillingRateCardBooleanEntitlement() (or AsBoolean()) to assert the
returned Type is "boolean" and the boolean value round-trips (true and/or false
cases and nil where applicable). Ensure you use the same helper patterns as the
metered tests (require.NoError, assert.Equal/require.NotNil) and reference the
functions FromAPIBillingRateCardEntitlement and ToAPIBillingRateCardEntitlement
to locate the relevant conversion logic.
---
Nitpick comments:
In `@api/spec/packages/aip/src/entitlements/entitlements.tsp`:
- Around line 57-61: The limit field is typed as float64 but should use
Shared.Numeric for arbitrary-precision consistency; update the limit declaration
in entitlements.tsp from "limit?: float64;" to "limit?: Shared.Numeric;", add or
ensure the Shared namespace/import is present in the file, and adjust any
related validation or serialization handling that expects float64 (e.g.,
consumers of limit or schema generation) to accept Shared.Numeric.
🪄 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: e5730b18-d4ea-4f46-a223-fb5f10cb3f49
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (11)
api/spec/packages/aip-client-javascript/src/index.tsapi/spec/packages/aip-client-javascript/src/models/schemas.tsapi/spec/packages/aip-client-javascript/src/models/types.tsapi/spec/packages/aip/src/entitlements/entitlements.tspapi/spec/packages/aip/src/productcatalog/ratecard.tspapi/v3/api.gen.goapi/v3/handlers/addons/convert.goapi/v3/handlers/plans/convert.goapi/v3/handlers/plans/convert_test.goe2e/plan_entitlement_template_v3_test.goe2e/v3helpers_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- api/spec/packages/aip/src/productcatalog/ratecard.tsp
- e2e/plan_entitlement_template_v3_test.go
- api/v3/handlers/addons/convert.go
- e2e/v3helpers_test.go
- api/v3/handlers/plans/convert.go
rolosp
force-pushed
the
feat/add-entitlement-v3-ratecard
branch
from
8953dda to
606da2d
Compare
rolosp
force-pushed
the
feat/add-entitlement-v3-ratecard
branch
from
606da2d to
497e833
Compare
rolosp
force-pushed
the
feat/add-entitlement-v3-ratecard
branch
from
497e833 to
aed8c4f
Compare
tothandras
approved these changes
Jun 11, 2026
…me amount to values
rolosp
force-pushed
the
feat/add-entitlement-v3-ratecard
branch
from
aed8c4f to
3f5c99e
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
This is a fix for this issue #4487
Summary by CodeRabbit
Release Notes
New Features
Tests