fix(api): entitlements static config#4512
Merged
Merged
Conversation
Contributor
|
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 selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughUnwraps legacy JSON-string-wrapped static entitlement configs for v3 reads, re-encodes configs as string tokens for domain storage on writes, centralizes billing-cadence parsing, standardizes metered usage-period validation errors, and adds unit and E2E tests covering cross-version behavior. ChangesEntitlement config and billing cadence handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
api/v3/handlers/plans/convert_test.go (1)
1354-1367: ⚡ Quick winTighten the large-integer assertion to verify JSON shape, not just substring presence.
Current check on Line 1366 proves the digits are present, but it doesn’t fully prove the returned payload shape. Consider asserting exact JSON (or decoding and checking the
idtoken explicitly) so this test fails on subtle representation regressions too.🤖 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 1354 - 1367, The test currently only checks that the digits "9007199254740993" appear in raw JSON; instead decode the resulting JSON (out.MarshalJSON result in raw) into a small struct or map and assert the id field equals the integer 9007199254740993 (or assert exact expected JSON string) to verify shape and type; locate the test block using FromAPIBillingRateCardEntitlement, ToAPIBillingRateCardEntitlement, apiEnt and out and replace the assert.Contains on raw with a strict decode-and-equal assertion against the id field.api/v3/handlers/addons/convert_test.go (2)
150-159: ⚡ Quick winAssert validation error classification, not just presence.
On Line 157 you already assert an error exists; it’s worth also asserting
models.IsGenericValidationError(err)so this test protects the intended API validation contract (not just any error).🤖 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_test.go` around lines 150 - 159, The test currently only asserts an error was returned when converting a metered entitlement; update the case inside the t.Run ("metered — missing usage period with no billing cadence is a validation error") to also assert the error is classified as a generic validation error by calling models.IsGenericValidationError(err) (keep the existing require.Error(t, err) and assert.Nil(t, domain) checks). Locate the conversion call FromAPIBillingRateCardEntitlement and add an assertion that models.IsGenericValidationError(err) is true to ensure the validation contract is enforced.
38-101: ⚡ Quick winAdd the same large-integer static-config round-trip case here for addon parity.
Nice coverage here overall. One gap: this suite doesn’t pin the
9007199254740993-style case, so addon and plan converter tests can drift on numeric preservation behavior. Adding that one case would lock in the no-float64-loss contract on the addon path too (same conversion logic, same regression risk).🤖 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_test.go` around lines 38 - 101, Add a t.Run case in TestFromToAPIBillingRateCardEntitlement that verifies a large integer preserves identity through the static-config round-trip: use roundTripStaticConfig(t, 9007199254740993) (or as a numeric literal convertible to interface{}) and assert equality with the original value, mirroring the existing scalar/string/array tests; place it alongside the other "static — ..." subtests so the addon conversion path (roundTripStaticConfig, FromAPIBillingRateCardEntitlement/ToAPIBillingRateCardEntitlement) is pinned to preserve large-integer semantics.
🤖 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/addons/convert.go`:
- Around line 525-536: The current unwrap logic for static.Config misinterprets
legacy JSON string values like "\"true\"" as wrapped JSON because
json.Unmarshal(..., &text) + json.Valid(text) will match primitives; change the
heuristic so after successfully unmarshalling into text you only unwrap when the
inner text is clearly structured JSON (e.g., its first non-space character is
'{' or '[') — otherwise keep config = json.RawMessage(static.Config); update the
block handling static.Config/config in convert.go accordingly (look for the
variables static.Config, text, config).
---
Nitpick comments:
In `@api/v3/handlers/addons/convert_test.go`:
- Around line 150-159: The test currently only asserts an error was returned
when converting a metered entitlement; update the case inside the t.Run
("metered — missing usage period with no billing cadence is a validation error")
to also assert the error is classified as a generic validation error by calling
models.IsGenericValidationError(err) (keep the existing require.Error(t, err)
and assert.Nil(t, domain) checks). Locate the conversion call
FromAPIBillingRateCardEntitlement and add an assertion that
models.IsGenericValidationError(err) is true to ensure the validation contract
is enforced.
- Around line 38-101: Add a t.Run case in
TestFromToAPIBillingRateCardEntitlement that verifies a large integer preserves
identity through the static-config round-trip: use roundTripStaticConfig(t,
9007199254740993) (or as a numeric literal convertible to interface{}) and
assert equality with the original value, mirroring the existing
scalar/string/array tests; place it alongside the other "static — ..." subtests
so the addon conversion path (roundTripStaticConfig,
FromAPIBillingRateCardEntitlement/ToAPIBillingRateCardEntitlement) is pinned to
preserve large-integer semantics.
In `@api/v3/handlers/plans/convert_test.go`:
- Around line 1354-1367: The test currently only checks that the digits
"9007199254740993" appear in raw JSON; instead decode the resulting JSON
(out.MarshalJSON result in raw) into a small struct or map and assert the id
field equals the integer 9007199254740993 (or assert exact expected JSON string)
to verify shape and type; locate the test block using
FromAPIBillingRateCardEntitlement, ToAPIBillingRateCardEntitlement, apiEnt and
out and replace the assert.Contains on raw with a strict decode-and-equal
assertion against the id field.
🪄 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: dbe4e1e7-2a50-4973-868f-6b56706c39c6
📒 Files selected for processing (5)
api/v3/handlers/addons/convert.goapi/v3/handlers/addons/convert_test.goapi/v3/handlers/plans/convert.goapi/v3/handlers/plans/convert_test.goe2e/plan_entitlement_template_v3_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/plan_entitlement_template_v3_test.go (1)
188-236: ⚡ Quick winUse given/when/then comments in these subtests.
These are non-trivial lifecycle subtests, but the inline commentary is still free-form. Please switch the section comments to concise given/when/then intent markers so the setup/action/assert flow is easier to scan.
As per coding guidelines, "For service and lifecycle subtests in Go, start each subtest body with concise intent comments using given-when-then format when the scenario is non-trivial".
Also applies to: 238-307
🤖 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/plan_entitlement_template_v3_test.go` around lines 188 - 236, The subtest starting with t.Run("write via v3 (JSON object) -> read via v1 (JSON string token)", ...) (and the similar subtest at 238-307) lacks structured intent comments; update the subtest body to use concise given/when/then markers: add a "given" comment before setup steps (uniqueKey, v3.CreateFeature, unmarshalling configObj, ent.FromBillingRateCardStaticEntitlement, and CreatePlan), a "when" comment before the action (the v1.GetPlanWithResponse call), and a "then" comment before assertions (v1StaticConfig usage, json.Unmarshal into inner and assert.JSONEq). Keep comments short and placed immediately above the appropriate code blocks to make the lifecycle flow clear.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 `@e2e/plan_entitlement_template_v3_test.go`:
- Around line 188-236: The subtest starting with t.Run("write via v3 (JSON
object) -> read via v1 (JSON string token)", ...) (and the similar subtest at
238-307) lacks structured intent comments; update the subtest body to use
concise given/when/then markers: add a "given" comment before setup steps
(uniqueKey, v3.CreateFeature, unmarshalling configObj,
ent.FromBillingRateCardStaticEntitlement, and CreatePlan), a "when" comment
before the action (the v1.GetPlanWithResponse call), and a "then" comment before
assertions (v1StaticConfig usage, json.Unmarshal into inner and assert.JSONEq).
Keep comments short and placed immediately above the appropriate code blocks to
make the lifecycle flow clear.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9c11dc11-e438-4b9d-b399-013d599f25ef
📒 Files selected for processing (1)
e2e/plan_entitlement_template_v3_test.go
tothandras
added
release-note/ignore
rolosp
approved these changes
Jun 11, 2026
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.
Summary by CodeRabbit
Bug Fixes
Tests