feat(productcatalog): expose taxCodeId in taxConfig API#4224
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds an optional TaxCodeId/tax_code_id to tax config models and a new productcatalog.ResolveTaxConfig function; updates API generators, mappings, service callsites to delegate tax-code resolution/backfill, and expands tests to cover TaxCodeID-first semantics and backfilling behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller (service)
participant Resolver as productcatalog.ResolveTaxConfig
participant TaxSvc as taxcode.Service
rect rgba(200,230,255,0.5)
Caller->>Resolver: ResolveTaxConfig(ctx, svc, namespace, cfg)
end
alt cfg == nil
Resolver-->>Caller: no-op / nil
else if cfg.TaxCodeID != nil
Resolver->>TaxSvc: GetTaxCode(ctx, TaxCodeID)
alt found
TaxSvc-->>Resolver: TaxCode (with app mappings)
Resolver->>Resolver: backfill cfg.Stripe from mapping (or clear if none)
Resolver-->>Caller: success (cfg mutated)
else not found
TaxSvc-->>Resolver: not found
Resolver-->>Caller: validation error (TaxCode not found)
end
else if cfg.Stripe != nil && cfg.Stripe.Code != ""
Resolver->>TaxSvc: GetOrCreateByAppMapping(ctx, app=Stripe, code)
TaxSvc-->>Resolver: TaxCode (created or existing)
Resolver->>Resolver: set cfg.TaxCodeID from TaxCode.ID
Resolver-->>Caller: success (cfg mutated)
else
Resolver-->>Caller: no-op (nothing to resolve)
end
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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
borbelyr-kong
added
release-note/misc
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
openmeter/subscription/service/service_test.go (1)
1104-1105: Small test harness tweak: prefert.Context()here.These subtests can use the test-scoped context directly instead of
context.WithCancel(context.Background()).✅ Suggested change
- ctx, cancel := context.WithCancel(context.Background()) - defer cancel() + ctx := t.Context()As per coding guidelines,
**/*_test.go: “In tests, prefert.Context()when atesting.Tortesting.TBis available instead ofcontext.Background()to keep cancellation and test-scoped lifecycle tied to the test harness”.Also applies to: 1182-1183
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/subscription/service/service_test.go` around lines 1104 - 1105, Replace the manual test context created with context.WithCancel by using the test-scoped context t.Context(): where you currently have "ctx, cancel := context.WithCancel(context.Background()); defer cancel()", change it to "ctx := t.Context()" in the test function(s) (and similarly update the other occurrence around lines referenced in the review); this ties cancellation/lifecycle to the testing harness and removes the unused cancel variable.openmeter/productcatalog/addon/service/addon.go (1)
206-239: Please add a mismatch guard when bothstripe.codeandtaxCodeIdare provided.Right now, if both are sent and disagree, the Stripe branch wins and the provided
taxCodeIdis silently ignored. That makes ambiguous payloads hard to debug.💡 Suggested guard
case meta.TaxConfig.Stripe != nil && meta.TaxConfig.Stripe.Code != "": // Existing path: resolve/create TaxCode from Stripe code. resolved, err := s.taxCode.GetOrCreateByAppMapping(ctx, taxcode.GetOrCreateByAppMappingInput{ Namespace: namespace, AppType: app.AppTypeStripe, TaxCode: meta.TaxConfig.Stripe.Code, }) if err != nil { return fmt.Errorf("failed to resolve tax code for stripe code %s: %w", meta.TaxConfig.Stripe.Code, err) } + if meta.TaxConfig.TaxCodeID != nil && *meta.TaxConfig.TaxCodeID != resolved.ID { + return models.NewGenericValidationError( + fmt.Errorf("taxCodeId %s does not match stripe code %s", *meta.TaxConfig.TaxCodeID, meta.TaxConfig.Stripe.Code), + ) + } tc = resolved🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/addon/service/addon.go` around lines 206 - 239, Add a guard to detect and reject mismatched inputs when both meta.TaxConfig.Stripe.Code and meta.TaxConfig.TaxCodeID are provided: in the decision logic around the switch (or at the start of the Stripe case in addon.go) compare the Stripe code to the taxCode resolved by s.taxCode.GetTaxCode/GetOrCreateByAppMapping (or fetch the TaxCode by TaxCodeID and compare its Stripe app mapping) and if they disagree return a validation error (e.g., models.NewGenericValidationError) instead of silently preferring Stripe; reference meta.TaxConfig.Stripe, meta.TaxConfig.TaxCodeID, s.taxCode.GetOrCreateByAppMapping and s.taxCode.GetTaxCode to locate where to implement the check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@openmeter/productcatalog/addon/service/addon.go`:
- Around line 206-239: Add a guard to detect and reject mismatched inputs when
both meta.TaxConfig.Stripe.Code and meta.TaxConfig.TaxCodeID are provided: in
the decision logic around the switch (or at the start of the Stripe case in
addon.go) compare the Stripe code to the taxCode resolved by
s.taxCode.GetTaxCode/GetOrCreateByAppMapping (or fetch the TaxCode by TaxCodeID
and compare its Stripe app mapping) and if they disagree return a validation
error (e.g., models.NewGenericValidationError) instead of silently preferring
Stripe; reference meta.TaxConfig.Stripe, meta.TaxConfig.TaxCodeID,
s.taxCode.GetOrCreateByAppMapping and s.taxCode.GetTaxCode to locate where to
implement the check.
In `@openmeter/subscription/service/service_test.go`:
- Around line 1104-1105: Replace the manual test context created with
context.WithCancel by using the test-scoped context t.Context(): where you
currently have "ctx, cancel := context.WithCancel(context.Background()); defer
cancel()", change it to "ctx := t.Context()" in the test function(s) (and
similarly update the other occurrence around lines referenced in the review);
this ties cancellation/lifecycle to the testing harness and removes the unused
cancel variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3e6c43a6-d2a7-4e91-b538-aea0c15b51d8
⛔ Files ignored due to path filters (6)
api/client/go/client.gen.gois excluded by!api/client/**api/client/javascript/src/client/schemas.tsis excluded by!api/client/**api/client/javascript/src/zod/index.tsis excluded by!api/client/**api/client/python/openmeter/_generated/models/_models.pyis excluded by!**/_generated/**,!api/client/**api/openapi.cloud.yamlis excluded by!**/openapi.cloud.yamlapi/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (9)
api/api.gen.goapi/spec/packages/legacy/src/productcatalog/tax.tspopenmeter/productcatalog/addon/service/addon.goopenmeter/productcatalog/addon/service/taxcode_test.goopenmeter/productcatalog/http/mapping.goopenmeter/productcatalog/plan/service/plan.goopenmeter/productcatalog/plan/service/taxcode_test.goopenmeter/subscription/service/service_test.goopenmeter/subscription/service/synchelpers.go
borbelyr-kong
force-pushed
the
feat/expose-taxcode-on-api
branch
from
ebb412b to
ddf19d9
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
openmeter/subscription/service/service_test.go (1)
1104-1105: Uset.Context()in these new subtests.These new DB-backed subtests can take their context straight from the test harness instead of wrapping
context.Background()again.As per coding guidelines, "In tests, prefer
t.Context()when atesting.Tortesting.TBis available instead of usingcontext.Background()to keep cancellation and lifecycle tied to the test harness."Also applies to: 1182-1183
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/subscription/service/service_test.go` around lines 1104 - 1105, Replace local context creation using context.WithCancel(context.Background()) in the DB-backed subtests with the test harness context t.Context(); specifically, remove the ctx, cancel := context.WithCancel(context.Background()) and defer cancel() in the subtests and use ctx := t.Context() (or pass t.Context() directly) so the subtest lifecycle and cancellation are tied to testing.T; update the occurrences around the blocks that use ctx (the variables created by context.WithCancel) at the two locations mentioned to use t.Context() instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openmeter/productcatalog/addon/service/addon.go`:
- Around line 206-239: The switch currently prefers meta.TaxConfig.Stripe.Code
over meta.TaxConfig.TaxCodeID and silently overwrites the supplied TaxCodeID;
update the logic in the addon service (the switch handling meta.TaxConfig in
addon.go, where s.taxCode.GetOrCreateByAppMapping and s.taxCode.GetTaxCode are
used and tc is set) to validate when both meta.TaxConfig.Stripe != nil and
meta.TaxConfig.TaxCodeID != nil: resolve the tax code for the Stripe code via
GetOrCreateByAppMapping (or GetTaxCode after mapping) and resolve the tax code
by TaxCodeID via GetTaxCode, compare their canonical IDs (e.g., resolved.ID or
NamespacedID), and if they differ return a GenericValidationError indicating the
mismatch; only if they match proceed to set tc and backfill the other
mapping/field (do not silently overwrite the provided TaxCodeID).
---
Nitpick comments:
In `@openmeter/subscription/service/service_test.go`:
- Around line 1104-1105: Replace local context creation using
context.WithCancel(context.Background()) in the DB-backed subtests with the test
harness context t.Context(); specifically, remove the ctx, cancel :=
context.WithCancel(context.Background()) and defer cancel() in the subtests and
use ctx := t.Context() (or pass t.Context() directly) so the subtest lifecycle
and cancellation are tied to testing.T; update the occurrences around the blocks
that use ctx (the variables created by context.WithCancel) at the two locations
mentioned to use t.Context() instead.
🪄 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: a81f8f21-ba0a-422d-9805-295628ca1d91
⛔ Files ignored due to path filters (7)
api/client/go/client.gen.gois excluded by!api/client/**api/client/javascript/src/client/schemas.tsis excluded by!api/client/**api/client/javascript/src/zod/index.tsis excluded by!api/client/**api/client/python/openmeter/_generated/models/_models.pyis excluded by!**/_generated/**,!api/client/**api/openapi.cloud.yamlis excluded by!**/openapi.cloud.yamlapi/openapi.yamlis excluded by!**/openapi.yamlapi/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (15)
api/api.gen.goapi/spec/packages/aip/src/billing/tax.tspapi/spec/packages/legacy/src/productcatalog/tax.tspapi/v3/api.gen.goapi/v3/handlers/billingprofiles/convert.gen.goapi/v3/handlers/billingprofiles/convert.goopenmeter/billing/service/profile.goopenmeter/productcatalog/addon/service/addon.goopenmeter/productcatalog/addon/service/taxcode_test.goopenmeter/productcatalog/http/mapping.goopenmeter/productcatalog/plan/service/plan.goopenmeter/productcatalog/plan/service/taxcode_test.goopenmeter/subscription/service/service_test.goopenmeter/subscription/service/synchelpers.goopenmeter/taxcode/taxcode.go
💤 Files with no reviewable changes (1)
- api/v3/handlers/billingprofiles/convert.go
✅ Files skipped from review due to trivial changes (2)
- api/spec/packages/aip/src/billing/tax.tsp
- api/spec/packages/legacy/src/productcatalog/tax.tsp
🚧 Files skipped from review as they are similar to previous changes (2)
- api/api.gen.go
- openmeter/productcatalog/http/mapping.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/billing/taxcode_dual_write_test.go`:
- Around line 473-475: Update the C4 docstring at the top of the test to match
the new semantics: instead of saying "FK must be erased, not left stale" change
it to reflect that a bare TaxCodeID passed by the caller is honored (i.e., the
FK may be preserved/used) and that this test covers the branch where callers
pass a stale TaxCodeID alongside a nil Stripe object; update the header comment
near TestProfileUpdateBareTaxCodeIDIsHonored so it mirrors the explanatory lines
already present in the body (lines describing behavior on nil Stripe and non-nil
DefaultTaxConfig).
- Around line 153-155: Update the test docstring to match the new behavior:
change the comment above the renamed test (A4b) to state that the adapter now
preserves TaxCodeID (does not erase the stale FK) and backfills Stripe.Code from
the app mapping during resolveDefaultTaxCode, so the intent aligns with the
assertions that TaxCodeID is preserved and Stripe.Code is populated; reference
the test name (A4b) and the resolveDefaultTaxCode logic to locate where the
comment should be edited.
🪄 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: e288bcf7-a31d-41ac-a63b-40464a233cff
📒 Files selected for processing (1)
test/billing/taxcode_dual_write_test.go
borbelyr-kong
force-pushed
the
feat/expose-taxcode-on-api
branch
from
915a6f6 to
450939e
Compare
borbelyr-kong
force-pushed
the
feat/expose-taxcode-on-api
branch
from
d4de06e to
6459b31
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
openmeter/subscription/workflow/service/subscription_test.go (1)
837-858: Avoid reusing the samerc1pointer across multiple phases.Nice improvement overall, but this still leaves one mutable alias in the test input (
&rc1used twice). Using distinct clones per phase would fully eliminate cross-phase coupling in this setup.Suggested refactor
rc1 := productcatalog.UsageBasedRateCard{ RateCardMeta: productcatalog.RateCardMeta{ Key: subscriptiontestutils.ExampleFeatureKey, @@ }, BillingCadence: subscriptiontestutils.ISOMonth, } + rc1Phase1 := rc1.Clone() + rc1Phase3 := rc1.Clone() @@ RateCards: productcatalog.RateCards{ - &rc1, + rc1Phase1, }, @@ RateCards: productcatalog.RateCards{ - &rc1, + rc1Phase3, },Also applies to: 900-902, 924-925
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/subscription/workflow/service/subscription_test.go` around lines 837 - 858, The test currently reuses the same rc1 UsageBasedRateCard instance pointer across multiple phases which creates mutable aliasing; update the test to produce distinct instances (clones) for each phase instead of passing &rc1 multiple times — for example create separate variables like rc1PhaseA, rc1PhaseB by deep-copying the RateCardMeta/UsageBasedRateCard fields or by calling a helper that returns a new productcatalog.UsageBasedRateCard populated from rc1 (preserving RateCardMeta, EntitlementTemplate, TaxConfig, Price and BillingCadence) and use those distinct variables wherever &rc1 is currently passed (references to rc1, UsageBasedRateCard, RateCardMeta, NewEntitlementTemplateFrom, NewPriceFrom indicate where to clone).openmeter/productcatalog/addon/service/addon.go (1)
203-205: Wrap resolver errors with rate-card context.This currently returns the raw resolver error from inside a loop, which makes debugging harder when multiple rate cards exist.
💡 Suggested tweak
- if err := productcatalog.ResolveTaxConfig(ctx, s.taxCode, namespace, meta.TaxConfig); err != nil { - return err - } + if err := productcatalog.ResolveTaxConfig(ctx, s.taxCode, namespace, meta.TaxConfig); err != nil { + return fmt.Errorf("resolving tax config for rate card %q: %w", meta.Key, err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/addon/service/addon.go` around lines 203 - 205, The loop calling productcatalog.ResolveTaxConfig currently returns the raw error directly; change it to wrap the error with rate-card context so callers know which rate card failed: when ResolveTaxConfig(ctx, s.taxCode, namespace, meta.TaxConfig) returns err, return a wrapped error that includes identifying info such as the rate card identifier/metadata (e.g. meta.Name or meta.ID), the namespace variable, and s.taxCode (use fmt.Errorf("resolving tax config for rate-card %s namespace %s taxCode %s: %w", meta.Name, namespace, s.taxCode, err) or your project’s errors.Wrapf style) so the error message clearly identifies the failing rate card and inputs.openmeter/subscription/service/service_test.go (1)
1104-1105: Uset.Context()directly in these test subtests.These test blocks should use
t.Context()instead of manually creating a context withcontext.Background()andWithCancel(). This keeps the context lifecycle tied to the test harness, which is cleaner and ensures proper cleanup when the test finishes.- ctx, cancel := context.WithCancel(context.Background()) - defer cancel() + ctx := t.Context()This applies to lines 1104-1105 and 1182-1183.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/subscription/service/service_test.go` around lines 1104 - 1105, Replace the manual context creation (ctx, cancel := context.WithCancel(context.Background()); defer cancel()) used inside the test subtests with the test harness context by using ctx := t.Context(), updating the subtests that currently declare variables ctx and cancel (and remove the defer cancel call); apply this change to both occurrences where ctx/cancel are created so the subtest context lifecycle is tied to t.Context() instead of a manually created background context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@openmeter/productcatalog/addon/service/addon.go`:
- Around line 203-205: The loop calling productcatalog.ResolveTaxConfig
currently returns the raw error directly; change it to wrap the error with
rate-card context so callers know which rate card failed: when
ResolveTaxConfig(ctx, s.taxCode, namespace, meta.TaxConfig) returns err, return
a wrapped error that includes identifying info such as the rate card
identifier/metadata (e.g. meta.Name or meta.ID), the namespace variable, and
s.taxCode (use fmt.Errorf("resolving tax config for rate-card %s namespace %s
taxCode %s: %w", meta.Name, namespace, s.taxCode, err) or your project’s
errors.Wrapf style) so the error message clearly identifies the failing rate
card and inputs.
In `@openmeter/subscription/service/service_test.go`:
- Around line 1104-1105: Replace the manual context creation (ctx, cancel :=
context.WithCancel(context.Background()); defer cancel()) used inside the test
subtests with the test harness context by using ctx := t.Context(), updating the
subtests that currently declare variables ctx and cancel (and remove the defer
cancel call); apply this change to both occurrences where ctx/cancel are created
so the subtest context lifecycle is tied to t.Context() instead of a manually
created background context.
In `@openmeter/subscription/workflow/service/subscription_test.go`:
- Around line 837-858: The test currently reuses the same rc1 UsageBasedRateCard
instance pointer across multiple phases which creates mutable aliasing; update
the test to produce distinct instances (clones) for each phase instead of
passing &rc1 multiple times — for example create separate variables like
rc1PhaseA, rc1PhaseB by deep-copying the RateCardMeta/UsageBasedRateCard fields
or by calling a helper that returns a new productcatalog.UsageBasedRateCard
populated from rc1 (preserving RateCardMeta, EntitlementTemplate, TaxConfig,
Price and BillingCadence) and use those distinct variables wherever &rc1 is
currently passed (references to rc1, UsageBasedRateCard, RateCardMeta,
NewEntitlementTemplateFrom, NewPriceFrom indicate where to clone).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1638b5be-28fc-4bc7-9426-a2ea06030409
⛔ Files ignored due to path filters (7)
api/client/go/client.gen.gois excluded by!api/client/**api/client/javascript/src/client/schemas.tsis excluded by!api/client/**api/client/javascript/src/zod/index.tsis excluded by!api/client/**api/client/python/openmeter/_generated/models/_models.pyis excluded by!**/_generated/**,!api/client/**api/openapi.cloud.yamlis excluded by!**/openapi.cloud.yamlapi/openapi.yamlis excluded by!**/openapi.yamlapi/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (23)
api/api.gen.goapi/spec/packages/aip/src/billing/tax.tspapi/spec/packages/legacy/src/productcatalog/tax.tspapi/v3/api.gen.goapi/v3/handlers/billingprofiles/convert.gen.goapi/v3/handlers/billingprofiles/convert.goopenmeter/billing/service/profile.goopenmeter/productcatalog/addon/service/addon.goopenmeter/productcatalog/addon/service/taxcode_test.goopenmeter/productcatalog/http/mapping.goopenmeter/productcatalog/plan/service/plan.goopenmeter/productcatalog/plan/service/taxcode_test.goopenmeter/productcatalog/subscription/service/change_test.goopenmeter/productcatalog/subscription/service/migrate_test.goopenmeter/productcatalog/subscription/service/plan_test.goopenmeter/productcatalog/tax.goopenmeter/subscription/addon/diff/restore_test.goopenmeter/subscription/service/service_test.goopenmeter/subscription/service/synchelpers.goopenmeter/subscription/workflow/service/addon_test.goopenmeter/subscription/workflow/service/subscription_test.goopenmeter/taxcode/taxcode.gotest/billing/taxcode_dual_write_test.go
💤 Files with no reviewable changes (1)
- api/v3/handlers/billingprofiles/convert.go
✅ Files skipped from review due to trivial changes (5)
- api/spec/packages/legacy/src/productcatalog/tax.tsp
- api/spec/packages/aip/src/billing/tax.tsp
- openmeter/productcatalog/subscription/service/plan_test.go
- openmeter/taxcode/taxcode.go
- openmeter/productcatalog/subscription/service/migrate_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- openmeter/productcatalog/http/mapping.go
- api/v3/handlers/billingprofiles/convert.gen.go
- openmeter/subscription/service/synchelpers.go
- test/billing/taxcode_dual_write_test.go
- openmeter/productcatalog/plan/service/taxcode_test.go
- openmeter/productcatalog/addon/service/taxcode_test.go
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/billing/taxcode_dual_write_test.go (1)
153-155:⚠️ Potential issue | 🟡 MinorThese two test banners still describe the old FK-clearing behavior.
Quick cleanup needed: the headers at Line 153 and Line 519 still say stale FK should be erased, but both tests now assert the opposite (
TaxCodeIDis honored/preserved with Stripe backfill).Suggested docstring fix
-// A4b: Round-trip clear — fetch the profile (TaxCodeID populated), clear only Stripe in-place, -// then update; the stale FK must be erased, not persisted. Behavior is kept non-nil so that -// DefaultTaxConfig is not normalized to nil by the adapter. +// A4b: Round-trip read-modify-write — fetch profile (TaxCodeID populated), clear only Stripe +// in-place, then update. Bare TaxCodeID is honored, so FK is preserved and Stripe.Code is +// backfilled from stored app mapping. Behavior stays non-nil to avoid nil-normalization.-// C4: Round-trip Stripe clear with pre-populated TaxCodeID — FK must be erased, not left stale. -// Covers the branch where the caller explicitly passes a stale TaxCodeID alongside nil Stripe. -// Behavior is kept non-nil so DefaultTaxConfig is not normalized to nil by the adapter. +// C4: Bare TaxCodeID input is honored — caller supplies TaxCodeID without Stripe.Code. +// Service validates the entity, preserves FK, and backfills Stripe.Code from app mapping. +// Behavior stays non-nil to avoid nil-normalization by the adapter.Also applies to: 519-521
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/billing/taxcode_dual_write_test.go` around lines 153 - 155, Update the two test banners in test/billing/taxcode_dual_write_test.go that still claim the "stale FK must be erased" behavior to reflect the current behavior where TaxCodeID is honored/preserved (Stripe backfill) and DefaultTaxConfig remains non-nil; specifically edit the header/comments that reference the FK-clearing behavior near the blocks testing round-trip clear and Stripe backfill so they describe that TaxCodeID is preserved and not erased, matching the test assertions that expect TaxCodeID to be retained.
🧹 Nitpick comments (1)
test/billing/taxcode_dual_write_test.go (1)
157-157: Use test-scoped context instead ofcontext.Background().Replace
ctx := context.Background()withctx := s.T().Context()on lines 157, 434, 479, and 523 to tie cancellation and lifecycle to the test harness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/billing/taxcode_dual_write_test.go` at line 157, The tests currently create a root context with ctx := context.Background(); replace those with a test-scoped context by assigning ctx := s.T().Context() wherever ctx is initialized (search for ctx := context.Background() in this test file) so the context lifecycle and cancellation are tied to the test harness rather than the global background context; update all occurrences to use s.T().Context() and ensure subsequent code uses that ctx variable unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@test/billing/taxcode_dual_write_test.go`:
- Around line 153-155: Update the two test banners in
test/billing/taxcode_dual_write_test.go that still claim the "stale FK must be
erased" behavior to reflect the current behavior where TaxCodeID is
honored/preserved (Stripe backfill) and DefaultTaxConfig remains non-nil;
specifically edit the header/comments that reference the FK-clearing behavior
near the blocks testing round-trip clear and Stripe backfill so they describe
that TaxCodeID is preserved and not erased, matching the test assertions that
expect TaxCodeID to be retained.
---
Nitpick comments:
In `@test/billing/taxcode_dual_write_test.go`:
- Line 157: The tests currently create a root context with ctx :=
context.Background(); replace those with a test-scoped context by assigning ctx
:= s.T().Context() wherever ctx is initialized (search for ctx :=
context.Background() in this test file) so the context lifecycle and
cancellation are tied to the test harness rather than the global background
context; update all occurrences to use s.T().Context() and ensure subsequent
code uses that ctx variable unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2faf9518-cdf3-45b8-8c80-4eec66e98e74
📒 Files selected for processing (1)
test/billing/taxcode_dual_write_test.go
mark-vass-konghq
previously approved these changes
Apr 29, 2026
tothandras
requested changes
Apr 29, 2026
tothandras
previously approved these changes
Apr 29, 2026
borbelyr-kong
force-pushed
the
feat/expose-taxcode-on-api
branch
from
ef89d3c to
74bfcfd
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openmeter/productcatalog/subscription/service/change_test.go (1)
50-57:⚠️ Potential issue | 🟠 MajorPrefer
t.Context()overcontext.Background()in these test subtests.All three instances (lines 56, 162, 290) have
t *testing.Tavailable in scope. Usingt.Context()keeps cancellation and lifecycle properly tied to the test harness. Since thecontextimport is only used for theseBackground()calls, you can also remove it entirely.Suggested refactor
import ( - "context" "fmt" "testing" "time" @@ - ctx := context.Background() + ctx := t.Context() @@ - ctx := context.Background() + ctx := t.Context() @@ - ctx := context.Background() + ctx := t.Context()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/subscription/service/change_test.go` around lines 50 - 57, Tests in change_test.go create a root context using context.Background() even though the test harness's t *testing.T is in scope; replace each "ctx := context.Background()" with "ctx := t.Context()" (these occurrences create the local ctx variable in the subtests) so the context is tied to the test lifecycle, and then remove the now-unused "context" import from the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@openmeter/productcatalog/subscription/service/change_test.go`:
- Around line 50-57: Tests in change_test.go create a root context using
context.Background() even though the test harness's t *testing.T is in scope;
replace each "ctx := context.Background()" with "ctx := t.Context()" (these
occurrences create the local ctx variable in the subtests) so the context is
tied to the test lifecycle, and then remove the now-unused "context" import from
the file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0ea79d09-36cd-4213-a79d-2116edfbb112
⛔ Files ignored due to path filters (7)
api/client/go/client.gen.gois excluded by!api/client/**api/client/javascript/src/client/schemas.tsis excluded by!api/client/**api/client/javascript/src/zod/index.tsis excluded by!api/client/**api/client/python/openmeter/_generated/models/_models.pyis excluded by!**/_generated/**,!api/client/**api/openapi.cloud.yamlis excluded by!**/openapi.cloud.yamlapi/openapi.yamlis excluded by!**/openapi.yamlapi/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (24)
api/api.gen.goapi/spec/packages/aip/src/billing/tax.tspapi/spec/packages/legacy/src/productcatalog/tax.tspapi/v3/api.gen.goapi/v3/handlers/billingprofiles/convert.gen.goapi/v3/handlers/billingprofiles/convert.goopenmeter/billing/service/profile.goopenmeter/productcatalog/addon/service/addon.goopenmeter/productcatalog/addon/service/taxcode_test.goopenmeter/productcatalog/http/mapping.goopenmeter/productcatalog/plan/service/plan.goopenmeter/productcatalog/plan/service/taxcode_test.goopenmeter/productcatalog/subscription/service/change_test.goopenmeter/productcatalog/subscription/service/migrate_test.goopenmeter/productcatalog/subscription/service/plan_test.goopenmeter/productcatalog/tax.goopenmeter/productcatalog/tax_test.goopenmeter/subscription/addon/diff/restore_test.goopenmeter/subscription/service/service_test.goopenmeter/subscription/service/synchelpers.goopenmeter/subscription/workflow/service/addon_test.goopenmeter/subscription/workflow/service/subscription_test.goopenmeter/taxcode/taxcode.gotest/billing/taxcode_dual_write_test.go
💤 Files with no reviewable changes (1)
- api/v3/handlers/billingprofiles/convert.go
✅ Files skipped from review due to trivial changes (3)
- openmeter/productcatalog/subscription/service/plan_test.go
- openmeter/productcatalog/subscription/service/migrate_test.go
- api/v3/handlers/billingprofiles/convert.gen.go
🚧 Files skipped from review as they are similar to previous changes (10)
- api/spec/packages/legacy/src/productcatalog/tax.tsp
- api/spec/packages/aip/src/billing/tax.tsp
- openmeter/productcatalog/http/mapping.go
- openmeter/subscription/workflow/service/addon_test.go
- openmeter/billing/service/profile.go
- openmeter/productcatalog/plan/service/taxcode_test.go
- openmeter/productcatalog/addon/service/taxcode_test.go
- openmeter/productcatalog/addon/service/addon.go
- openmeter/productcatalog/tax.go
- openmeter/subscription/workflow/service/subscription_test.go
tothandras
approved these changes
Apr 29, 2026
This was referenced Apr 30, 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
3 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
New Features
Deprecated
Tests