feat(productcatalog): eager load tax_code on plan rate cards and expo…#3945
Merged
borbelyr-kong merged 16 commits intomainfrom Mar 24, 2026
Merged
feat(productcatalog): eager load tax_code on plan rate cards and expo…#3945borbelyr-kong merged 16 commits intomainfrom
borbelyr-kong merged 16 commits intomainfrom
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:
📝 WalkthroughWalkthroughIntegrates TaxCode support across product catalog and subscription flows: adds TaxCode service methods, resolves/creates tax codes during plan/addon create/update, persists/backfills tax data on rate cards, wires the TaxCode dependency through app initialization, and adds extensive tests for dual-write and backfill behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PlanService as Plan Service
participant TaxCodeSvc as Tax Code Service
participant Adapter as Rate Card Adapter
participant DB as Database
Client->>PlanService: CreatePlan(phases with RateCards)
PlanService->>PlanService: resolveFeatures()
PlanService->>PlanService: resolveTaxCodes()
PlanService->>TaxCodeSvc: GetOrCreateByAppMapping(namespace, AppTypeStripe, stripeCode)
TaxCodeSvc->>DB: Query TaxCode by app_mappings
alt TaxCode exists
DB-->>TaxCodeSvc: TaxCode
else Not found
TaxCodeSvc->>DB: Create TaxCode with AppMapping
DB-->>TaxCodeSvc: New TaxCode
end
TaxCodeSvc-->>PlanService: TaxCode (with ID)
PlanService->>Adapter: Persist RateCard (TaxCodeID, TaxBehavior)
Adapter->>DB: INSERT/UPDATE with dedicated columns
DB-->>Adapter: Success
Adapter-->>PlanService: Success
PlanService-->>Client: Plan with resolved TaxCodeID
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
borbelyr-kong
added
the
release-note/ignore
borbelyr-kong
force-pushed
the
feat/tax-codes-v3-add-fk-keys
branch
from
360fb25 to
47540a1
Compare
turip
reviewed
Mar 20, 2026
…se in RateCardMeta
…rate card write path
…umn and TaxCode entity on plan rate card read
…Stripe tax codes on plan creation
borbelyr-kong
force-pushed
the
feat/tax-codes-double-write-plan
branch
from
ebf9126 to
b9a9453
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
openmeter/productcatalog/ratecard.go (1)
142-181:⚠️ Potential issue | 🟡 Minor
Equal()doesn't compare the newTaxCodefield.The
Equal()method compares all other fields inRateCardMetabut omits the newTaxCodefield. If two rate cards should be considered different when their tax codes differ, this needs updating.If the intentional behavior is to ignore
TaxCodeduring equality checks (e.g., because it's only for display/eager loading), adding a comment would clarify that decision.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/ratecard.go` around lines 142 - 181, The Equal method on RateCardMeta omits the new TaxCode field; update RateCardMeta.Equal to compare TaxCode (use the same pointer-safe comparison style as FeatureKey/FeatureID, e.g., lo.FromPtr(r.TaxCode) vs lo.FromPtr(v.TaxCode)) and return false if they differ, or if the intended semantics are to ignore TaxCode add a clarifying comment above RateCardMeta.Equal explaining why it is excluded; reference the RateCardMeta.Equal method and the TaxCode field when making the change.openmeter/productcatalog/tax.go (1)
88-99:⚠️ Potential issue | 🟡 MinorKeep the cloning pattern consistent across all pointer fields.
Good catch on deep-copying
TaxCodeID! ThoughBehavioris also a pointer field, it's assigned directly instead of being deep-copied likeStripe. For consistency and maintainability, all pointer fields should follow the same cloning pattern.🛠️ Suggested fix
func (c TaxConfig) Clone() TaxConfig { - out := TaxConfig{ - Behavior: c.Behavior, - } + out := TaxConfig{} + + if c.Behavior != nil { + out.Behavior = lo.ToPtr(*c.Behavior) + } if c.Stripe != nil { out.Stripe = lo.ToPtr(c.Stripe.Clone()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/tax.go` around lines 88 - 99, The Clone method for TaxConfig assigns the pointer field Behavior directly rather than deep-copying it like Stripe and TaxCodeID; update TaxConfig.Clone to deep-copy Behavior as well (e.g. in the same pattern used for Stripe/TaxCodeID: check if c.Behavior != nil and set out.Behavior to a new pointer holding a cloned or dereferenced value using lo.ToPtr or the Behavior.Clone() method as appropriate) so all pointer fields are cloned consistently in TaxConfig.Clone.openmeter/productcatalog/plan/adapter/mapping.go (1)
339-341:⚠️ Potential issue | 🟡 MinorError variable may be nil in this error message.
The
errvariable referenced here comes from theParsePtrOrNil()call on line 300. If that succeeded but we hit this default case,errwill benil, leading to a confusing error message likeinvalid RateCard type foo: <nil>.🐛 Suggested fix
default: - return nil, fmt.Errorf("invalid RateCard type %s: %w", r.Type, err) + return nil, fmt.Errorf("invalid RateCard type %s", r.Type) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/plan/adapter/mapping.go` around lines 339 - 341, The default error return in the switch uses err (from ParsePtrOrNil) which may be nil, producing messages like "invalid RateCard type foo: <nil>"; update the default case in mapping.go to avoid appending a nil error—e.g., check if err != nil and return fmt.Errorf("invalid RateCard type %s: %w", r.Type, err) otherwise return fmt.Errorf("invalid RateCard type %s", r.Type); ensure you reference the ParsePtrOrNil result and avoid shadowing the err variable.
🧹 Nitpick comments (5)
openmeter/taxcode/service.go (1)
125-171: Consider consolidating duplicate input types.
GetTaxCodeByAppMappingInputandGetOrCreateByAppMappingInputhave identical structures and validation logic. You could define a shared base type or use a single input type with a type alias if the semantic distinction matters:type AppMappingLookupInput struct { Namespace string AppType app.AppType TaxCode string } type GetTaxCodeByAppMappingInput = AppMappingLookupInput type GetOrCreateByAppMappingInput = AppMappingLookupInputThat said, keeping them separate is totally fine if you anticipate them diverging in the future.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/taxcode/service.go` around lines 125 - 171, Replace the duplicated structs and validation by introducing a single shared type (e.g., AppMappingLookupInput) containing Namespace, AppType and TaxCode, move the Validate() implementation to that type (Validate on AppMappingLookupInput) and then create type aliases GetTaxCodeByAppMappingInput = AppMappingLookupInput and GetOrCreateByAppMappingInput = AppMappingLookupInput so existing code referring to GetTaxCodeByAppMappingInput and GetOrCreateByAppMappingInput continues to work; ensure you remove the duplicate Validate methods for the old names and keep references to AppType.Validate() and models.NewNillableGenericValidationError(errors.Join(...)) unchanged.api/v3/handlers/billingprofiles/convert.go (1)
69-71: FIXME noted — consider tracking this.The goverter ignore for
TaxCodeIDmakes sense as a temporary measure. Just a friendly nudge to ensure this gets tracked (issue/ticket) so it doesn't get forgotten once the OpenAPI schema is updated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/billingprofiles/convert.go` around lines 69 - 71, The goverter ignore on TaxCodeID in the ConvertBillingTaxConfigToTaxConfig declaration is a temporary workaround; create and reference a tracking ticket/issue for removing the "// goverter:ignore TaxCodeID" once the OpenAPI schema is updated, then update this comment to include the issue number (e.g., "TODO: remove goverter:ignore TaxCodeID — tracked in ISSUE-1234") and add a brief note in the function comment for ConvertBillingTaxConfigToTaxConfig so reviewers can find and remove the ignore when the schema change lands.openmeter/productcatalog/plan/service/service_test.go (1)
214-216: Consider adding a nil check for robustness.The extraction logic assumes
TaxConfigandTaxCodeIDare populated. While the test setup guarantees this, a quickrequire.NotNilwould make the test more self-documenting and catch regressions early if the tax code resolution ever fails silently.taxConfig := draftPlanV1.Phases[0].RateCards[0].AsMeta().TaxConfig require.NotNil(t, taxConfig, "TaxConfig should be populated after plan creation") require.NotNil(t, taxConfig.TaxCodeID, "TaxCodeID should be resolved during plan creation") taxCodeID := taxConfig.TaxCodeID🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/plan/service/service_test.go` around lines 214 - 216, Add explicit nil assertions before extracting TaxCodeID from draftPlanV1 to make the test robust: retrieve taxConfig via draftPlanV1.Phases[0].RateCards[0].AsMeta().TaxConfig, call require.NotNil(t, taxConfig, "TaxConfig should be populated after plan creation") and require.NotNil(t, taxConfig.TaxCodeID, "TaxCodeID should be resolved during plan creation"), then assign taxCodeID := taxConfig.TaxCodeID; this ensures TaxConfig and TaxCodeID are validated before use in the test.openmeter/productcatalog/plan/service/taxcode_test.go (1)
23-38: Consider adding test coverage forUsageBasedRateCard.The current tests comprehensively cover
FlatFeeRateCardscenarios, but the production code inresolveTaxCodeshas different handling forUsageBasedRateCard(BillingCadence is required and non-pointer). Adding a test case with a usage-based rate card + tax config would validate that code path.Something like:
t.Run("UsageBasedWithStripeCode", func(t *testing.T) { ubp := &productcatalog.UsageBasedRateCard{ RateCardMeta: productcatalog.RateCardMeta{ Key: features[0].Key, Name: features[0].Name, TaxConfig: &productcatalog.TaxConfig{ Stripe: &productcatalog.StripeTaxConfig{Code: "txcd_ubp_test"}, }, // ... price config }, BillingCadence: MonthPeriod, // non-pointer } // ... test assertions })Based on learnings: In
productcatalog.UsageBasedRateCard, theBillingCadencefield is a non-pointerisodate.Period, while inproductcatalog.FlatFeeRateCard,BillingCadenceis a pointer type.Also applies to: 67-337
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/plan/service/taxcode_test.go` around lines 23 - 38, Tests only exercise FlatFeeRateCard but production resolveTaxCodes also handles UsageBasedRateCard (with non-pointer BillingCadence); add a test covering UsageBasedRateCard to validate that path. Create a helper similar to newTestFlatRateCard (e.g., newTestUsageBasedRateCard) that returns a *productcatalog.UsageBasedRateCard with RateCardMeta including a TaxConfig (StripeTaxConfig.Code set) and a non-pointer BillingCadence (e.g., MonthPeriod), then add a t.Run case (e.g., "UsageBasedWithStripeCode") asserting resolveTaxCodes returns the expected stripe tax code mapping for that feature; reference resolveTaxCodes, productcatalog.UsageBasedRateCard, RateCardMeta, and StripeTaxConfig in the new test.openmeter/productcatalog/plan/service/plan.go (1)
185-240: Consider extracting the rate card rebuild logic to reduce duplication.The rate card type-switching and rebuild pattern (lines 212-236) duplicates what's already in
resolveFeatures(lines 120-142, 154-176). The existing FIXME on line 37 mentions this is "a bit brittle."This isn't blocking, but consolidating into a shared helper (e.g.,
rebuildRateCardWithMeta(rc RateCard, meta RateCardMeta) (RateCard, error)) would reduce duplication and make future rate card type additions easier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/plan/service/plan.go` around lines 185 - 240, The rate-card rebuild switch in resolveTaxCodes duplicates logic from resolveFeatures; extract that logic into a shared helper (e.g., rebuildRateCardWithMeta(rc productcatalog.RateCard, meta productcatalog.RateCardMeta) (productcatalog.RateCard, error)) that inspects rc.Type(), uses rc.GetBillingCadence() when needed, validates required billing cadence for UsageBasedRateCardType, constructs and returns the appropriate productcatalog.FlatFeeRateCard or productcatalog.UsageBasedRateCard with the provided meta, and returns an error for unsupported types or missing cadence; then call this helper from both resolveTaxCodes and resolveFeatures and keep the existing rc.Merge(newRC) call and error handling.
🤖 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/ratecard.go`:
- Around line 83-84: The RateCard.Clone() method currently omits the newly added
TaxCode field so cloned RateCard instances lose the eagerly-loaded tax
reference; update the Clone() implementation(s) that copy RateCard data (look
for the Clone method on RateCard and any helper clone functions in the 93-140
region) to also copy the TaxCode pointer (preserving nil vs non-nil) into the
cloned struct, or add a clarifying comment in Clone() that omitting TaxCode is
intentional if it must remain read-only/eager-only.
In `@openmeter/productcatalog/tax_test.go`:
- Around line 156-191: Add a test case that exercises TaxConfig.Clone(): create
a TaxConfig with pointer fields (e.g., TaxCodeID), call Clone() and assert the
cloned value is equal via Equal(), then mutate the clone (change the TaxCodeID)
and assert the original is unchanged and pointers are not the same; place this
alongside the existing Equal()/MergeTaxConfigs() cases so the test suite catches
any shallow-copy regressions in TaxConfig.Clone().
In `@openmeter/taxcode/service/taxcode.go`:
- Around line 58-87: The code in GetOrCreateByAppMapping calls
s.adapter.GetTaxCodeByAppMapping and treats any error as "not found" — change
this so you only proceed to create when the error is an explicit not-found; if
err != nil and !models.IsNotFoundError(err) (or the project’s equivalent
not-found checker) return the error immediately, otherwise continue to build the
key and call s.adapter.CreateTaxCode; keep the existing concurrent-create
conflict handling using models.IsGenericConflictError and the subsequent
GetTaxCodeByAppMapping call.
---
Outside diff comments:
In `@openmeter/productcatalog/plan/adapter/mapping.go`:
- Around line 339-341: The default error return in the switch uses err (from
ParsePtrOrNil) which may be nil, producing messages like "invalid RateCard type
foo: <nil>"; update the default case in mapping.go to avoid appending a nil
error—e.g., check if err != nil and return fmt.Errorf("invalid RateCard type %s:
%w", r.Type, err) otherwise return fmt.Errorf("invalid RateCard type %s",
r.Type); ensure you reference the ParsePtrOrNil result and avoid shadowing the
err variable.
In `@openmeter/productcatalog/ratecard.go`:
- Around line 142-181: The Equal method on RateCardMeta omits the new TaxCode
field; update RateCardMeta.Equal to compare TaxCode (use the same pointer-safe
comparison style as FeatureKey/FeatureID, e.g., lo.FromPtr(r.TaxCode) vs
lo.FromPtr(v.TaxCode)) and return false if they differ, or if the intended
semantics are to ignore TaxCode add a clarifying comment above
RateCardMeta.Equal explaining why it is excluded; reference the
RateCardMeta.Equal method and the TaxCode field when making the change.
In `@openmeter/productcatalog/tax.go`:
- Around line 88-99: The Clone method for TaxConfig assigns the pointer field
Behavior directly rather than deep-copying it like Stripe and TaxCodeID; update
TaxConfig.Clone to deep-copy Behavior as well (e.g. in the same pattern used for
Stripe/TaxCodeID: check if c.Behavior != nil and set out.Behavior to a new
pointer holding a cloned or dereferenced value using lo.ToPtr or the
Behavior.Clone() method as appropriate) so all pointer fields are cloned
consistently in TaxConfig.Clone.
---
Nitpick comments:
In `@api/v3/handlers/billingprofiles/convert.go`:
- Around line 69-71: The goverter ignore on TaxCodeID in the
ConvertBillingTaxConfigToTaxConfig declaration is a temporary workaround; create
and reference a tracking ticket/issue for removing the "// goverter:ignore
TaxCodeID" once the OpenAPI schema is updated, then update this comment to
include the issue number (e.g., "TODO: remove goverter:ignore TaxCodeID —
tracked in ISSUE-1234") and add a brief note in the function comment for
ConvertBillingTaxConfigToTaxConfig so reviewers can find and remove the ignore
when the schema change lands.
In `@openmeter/productcatalog/plan/service/plan.go`:
- Around line 185-240: The rate-card rebuild switch in resolveTaxCodes
duplicates logic from resolveFeatures; extract that logic into a shared helper
(e.g., rebuildRateCardWithMeta(rc productcatalog.RateCard, meta
productcatalog.RateCardMeta) (productcatalog.RateCard, error)) that inspects
rc.Type(), uses rc.GetBillingCadence() when needed, validates required billing
cadence for UsageBasedRateCardType, constructs and returns the appropriate
productcatalog.FlatFeeRateCard or productcatalog.UsageBasedRateCard with the
provided meta, and returns an error for unsupported types or missing cadence;
then call this helper from both resolveTaxCodes and resolveFeatures and keep the
existing rc.Merge(newRC) call and error handling.
In `@openmeter/productcatalog/plan/service/service_test.go`:
- Around line 214-216: Add explicit nil assertions before extracting TaxCodeID
from draftPlanV1 to make the test robust: retrieve taxConfig via
draftPlanV1.Phases[0].RateCards[0].AsMeta().TaxConfig, call require.NotNil(t,
taxConfig, "TaxConfig should be populated after plan creation") and
require.NotNil(t, taxConfig.TaxCodeID, "TaxCodeID should be resolved during plan
creation"), then assign taxCodeID := taxConfig.TaxCodeID; this ensures TaxConfig
and TaxCodeID are validated before use in the test.
In `@openmeter/productcatalog/plan/service/taxcode_test.go`:
- Around line 23-38: Tests only exercise FlatFeeRateCard but production
resolveTaxCodes also handles UsageBasedRateCard (with non-pointer
BillingCadence); add a test covering UsageBasedRateCard to validate that path.
Create a helper similar to newTestFlatRateCard (e.g., newTestUsageBasedRateCard)
that returns a *productcatalog.UsageBasedRateCard with RateCardMeta including a
TaxConfig (StripeTaxConfig.Code set) and a non-pointer BillingCadence (e.g.,
MonthPeriod), then add a t.Run case (e.g., "UsageBasedWithStripeCode") asserting
resolveTaxCodes returns the expected stripe tax code mapping for that feature;
reference resolveTaxCodes, productcatalog.UsageBasedRateCard, RateCardMeta, and
StripeTaxConfig in the new test.
In `@openmeter/taxcode/service.go`:
- Around line 125-171: Replace the duplicated structs and validation by
introducing a single shared type (e.g., AppMappingLookupInput) containing
Namespace, AppType and TaxCode, move the Validate() implementation to that type
(Validate on AppMappingLookupInput) and then create type aliases
GetTaxCodeByAppMappingInput = AppMappingLookupInput and
GetOrCreateByAppMappingInput = AppMappingLookupInput so existing code referring
to GetTaxCodeByAppMappingInput and GetOrCreateByAppMappingInput continues to
work; ensure you remove the duplicate Validate methods for the old names and
keep references to AppType.Validate() and
models.NewNillableGenericValidationError(errors.Join(...)) unchanged.
🪄 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: 76d2fa06-08c7-4b11-96ce-1bad47181fca
📒 Files selected for processing (29)
api/v3/handlers/billingprofiles/convert.goapp/common/productcatalog.gocmd/billing-worker/wire.gocmd/billing-worker/wire_gen.gocmd/jobs/internal/wire.gocmd/jobs/internal/wire_gen.gocmd/server/wire_gen.goopenmeter/productcatalog/plan/adapter/mapping.goopenmeter/productcatalog/plan/adapter/phase.goopenmeter/productcatalog/plan/adapter/plan.goopenmeter/productcatalog/plan/service/plan.goopenmeter/productcatalog/plan/service/service.goopenmeter/productcatalog/plan/service/service_test.goopenmeter/productcatalog/plan/service/taxcode_test.goopenmeter/productcatalog/ratecard.goopenmeter/productcatalog/tax.goopenmeter/productcatalog/tax_test.goopenmeter/productcatalog/testutils/env.goopenmeter/server/server_test.goopenmeter/subscription/addon/diff/apply_test.goopenmeter/subscription/testutils/service.goopenmeter/taxcode/adapter/mapping.goopenmeter/taxcode/adapter/taxcode.goopenmeter/taxcode/repository.goopenmeter/taxcode/service.goopenmeter/taxcode/service/taxcode.goopenmeter/taxcode/taxcode.gotest/billing/subscription_suite.gotest/customer/testenv.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/adapter/taxconfig.go`:
- Around line 11-39: BackfillTaxConfig must also populate the TaxCodeID when
rebuilding a nil cfg: in BackfillTaxConfig, after creating cfg (cfg =
&productcatalog.TaxConfig{}) and before returning, set cfg.TaxCodeID (if it's
empty/nil) from the provided taxcode object (use tc.GetId() when tc != nil),
similar to how cfg.Behavior and cfg.Stripe are set so the backfill path
preserves the TaxCodeID field consistent with the dual-write path.
In `@openmeter/productcatalog/addon/adapter/mapping.go`:
- Around line 102-115: The FromPlanRateCardRow path never backfills legacy tax
fields from the eagerly-loaded TaxCode like the addon path does; update the
FromPlanRateCardRow handling to call r.Edges.TaxCodeOrErr(), map it via
taxcodeadapter.MapTaxCodeFromEntity, set meta.TaxCode, and then call
productcatalogadapter.BackfillTaxConfig(meta.TaxConfig, r.TaxBehavior,
meta.TaxCode) (same sequence as in the addon branch) so plan-phase rate cards
receive the same backfilled TaxConfig and TaxCode values.
In `@openmeter/productcatalog/addon/service/addon.go`:
- Around line 503-506: In UpdateAddon, move the call to s.resolveTaxCodes(ctx,
params.Namespace, params.RateCards) so it runs after the existing add-on is
loaded and after the draft-only/status validation (i.e., after the code that
enforces update allowed only for draft status), not before; this prevents
running the get-or-create write on failing/unauthorized updates and avoids
creating orphan tax codes when the update is rejected. Ensure resolveTaxCodes
remains using params.Namespace and params.RateCards and only executes once the
status check on the loaded add-on passes.
In `@openmeter/productcatalog/addon/service/taxcode_test.go`:
- Around line 527-533: The test is missing an assertion for the backfilled
TaxCodeID; update the test block that reads tc := backfillRC.AsMeta().TaxConfig
to also require/not-nil and assert the expected TaxCodeID value (e.g.,
require.NotNil or require.Equal on tc.TaxCodeID) so the dedicated-column read
path preserves TaxConfig.TaxCodeID; reference the existing variables backfillRC,
tc, and TaxConfig.TaxCodeID when adding the assertion.
🪄 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: 5a0996d7-7bdd-4ea3-b897-1a83eb8c662c
📒 Files selected for processing (22)
app/common/productcatalog.gocmd/billing-worker/wire_gen.gocmd/jobs/internal/wire_gen.gocmd/server/wire_gen.goopenmeter/productcatalog/adapter/taxconfig.goopenmeter/productcatalog/addon/adapter/addon.goopenmeter/productcatalog/addon/adapter/mapping.goopenmeter/productcatalog/addon/service/addon.goopenmeter/productcatalog/addon/service/service.goopenmeter/productcatalog/addon/service/service_test.goopenmeter/productcatalog/addon/service/taxcode_test.goopenmeter/productcatalog/plan/adapter/mapping.goopenmeter/productcatalog/plan/adapter/plan.goopenmeter/productcatalog/plan/service/taxcode_test.goopenmeter/productcatalog/ratecard.goopenmeter/productcatalog/tax.goopenmeter/productcatalog/tax_test.goopenmeter/productcatalog/testutils/env.goopenmeter/subscription/addon/repo/subscriptionaddon.goopenmeter/subscription/testutils/service.gotest/billing/subscription_suite.gotest/customer/testenv.go
✅ Files skipped from review due to trivial changes (3)
- cmd/billing-worker/wire_gen.go
- cmd/jobs/internal/wire_gen.go
- cmd/server/wire_gen.go
🚧 Files skipped from review as they are similar to previous changes (4)
- openmeter/productcatalog/plan/adapter/plan.go
- openmeter/productcatalog/plan/adapter/mapping.go
- app/common/productcatalog.go
- openmeter/productcatalog/plan/service/taxcode_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openmeter/productcatalog/addon/service/taxcode_test.go (1)
433-443: Consider assertingTaxCodeIDfor completeness.The test verifies
BehaviorandStripeare preserved on read-back, but doesn't checkTaxCodeID. Since this test covers the full round-trip (create → get), assertingTaxCodeIDwould ensure the resolution path is fully validated.🧪 Suggested assertion to add
tc := getFirstAddonRCTaxConfig(t, fetched) require.NotNil(t, tc) + +// TaxCodeID should be present +require.NotNil(t, tc.TaxCodeID, "TaxCodeID must be resolved and preserved") // Behavior should be present require.NotNil(t, tc.Behavior)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/addon/service/taxcode_test.go` around lines 433 - 443, The test checks Behavior and Stripe on the read-back tax config but omits asserting the TaxCodeID; update the test after calling getFirstAddonRCTaxConfig (variable tc) to assert that tc.TaxCodeID is present and equals the expected tax code identifier used when creating the resource (e.g., assert.NotNil(tc.TaxCodeID) and assert.Equal(t, expectedTaxCodeID, tc.TaxCodeID) where expectedTaxCodeID is the tax code ID variable from the create/setup step).
🤖 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/taxcode_test.go`:
- Around line 433-443: The test checks Behavior and Stripe on the read-back tax
config but omits asserting the TaxCodeID; update the test after calling
getFirstAddonRCTaxConfig (variable tc) to assert that tc.TaxCodeID is present
and equals the expected tax code identifier used when creating the resource
(e.g., assert.NotNil(tc.TaxCodeID) and assert.Equal(t, expectedTaxCodeID,
tc.TaxCodeID) where expectedTaxCodeID is the tax code ID variable from the
create/setup step).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5b745e11-2fcc-4347-94b5-4db7707ddf5c
📒 Files selected for processing (5)
openmeter/productcatalog/adapter/taxconfig.goopenmeter/productcatalog/addon/service/taxcode_test.goopenmeter/productcatalog/plan/service/taxcode_test.goopenmeter/productcatalog/testutils/env.goopenmeter/subscription/testutils/service.go
🚧 Files skipped from review as they are similar to previous changes (2)
- openmeter/productcatalog/adapter/taxconfig.go
- openmeter/productcatalog/plan/service/taxcode_test.go
turip
reviewed
Mar 24, 2026
Comment on lines
+62
to
+64
| if err == nil { | ||
| return tc, nil | ||
| } |
Member
There was a problem hiding this comment.
Suggested change
| if err == nil { | |
| return tc, nil | |
| } | |
| if err != nil && !errors.Is(err, taxcode.ErrTaxCodeNotFound) { | |
| return taxcode.TaxCode{}, err | |
| } | |
| if err == nil { // If taxcode is returned let's just return it to the caller | |
| return tc, nil | |
| } | |
It's better as if there is any other error except of the not found we are propagating that downstream.
Also generally speaking in go you almost always follow each call returning an err with the if err != nil (think as those are always coupled).
Comment on lines
+126
to
+128
| if err := input.Validate(); err != nil { | ||
| return taxcode.TaxCode{}, err | ||
| } |
Member
There was a problem hiding this comment.
nit: you can do input validation before entutils.TransactingRepo / transaction.Run (it's a bit more efficient)
Contributor
Author
There was a problem hiding this comment.
Changed everywhere in the file to be consistent.
|
|
||
| // GetOrCreateByAppMapping looks up a TaxCode by its app mapping. If none exists, | ||
| // it creates one with a key derived from the app-specific code. | ||
| func (s *service) GetOrCreateByAppMapping(ctx context.Context, input taxcode.GetOrCreateByAppMappingInput) (taxcode.TaxCode, error) { |
Member
There was a problem hiding this comment.
Please validate the input before passing downstream in case the implementation changes later.
| }) | ||
| } | ||
|
|
||
| func (s *service) GetTaxCodeByAppMapping(ctx context.Context, input taxcode.GetTaxCodeByAppMappingInput) (taxcode.TaxCode, error) { |
Member
There was a problem hiding this comment.
nit: you can consider validating the input here even if the adapter does that (when the service is a simple passthru it's not that important)
Contributor
Author
There was a problem hiding this comment.
Changed everywhere in the file to be consistent.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
openmeter/taxcode/errors.go (1)
60-71: Optional cleanup: extract shared not-found issue builder.Line [61] and Line [67] repeat the same base chain (
ErrTaxCodeNotFound.WithPathString(...)) and only differ by attrs. A tiny helper would keep this easier to extend and reduce drift.💡 Small refactor sketch
+func newTaxCodeNotFoundIssue(path string) models.ValidationIssue { + return ErrTaxCodeNotFound.WithPathString(path) +} + func NewTaxCodeNotFoundError(id string) error { - return ErrTaxCodeNotFound. - WithPathString("id"). + return newTaxCodeNotFoundIssue("id"). WithAttr("id", id) } func NewTaxCodeByAppMappingNotFoundError(appType, taxCode string) error { - return ErrTaxCodeNotFound. - WithPathString("app_mappings"). + return newTaxCodeNotFoundIssue("app_mappings"). WithAttr("app_type", appType). WithAttr("tax_code", taxCode) }As per coding guidelines, "In general when reviewing the Golang code make readability and maintainability a priority, even potentially suggest restructuring the code to improve them."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/taxcode/errors.go` around lines 60 - 71, Extract the repeated base builder into a small helper (e.g., buildTaxCodeNotFound or taxCodeNotFoundWithPath) that takes the differing path string and returns ErrTaxCodeNotFound.WithPathString(path), then use that helper inside NewTaxCodeNotFoundError and NewTaxCodeByAppMappingNotFoundError to chain WithAttr calls; reference ErrTaxCodeNotFound, NewTaxCodeNotFoundError and NewTaxCodeByAppMappingNotFoundError to locate the current duplication and replace the duplicated WithPathString(...) start of the chain with the helper call.openmeter/productcatalog/addon/adapter/mapping.go (1)
102-115: Consider extracting the tax-code hydrate/backfill path into a shared helper.
FromAddonRateCardRowandFromPlanRateCardRownow repeat the sameTaxCodeOrErr→MapTaxCodeFromEntity→BackfillTaxConfigflow. A small helper would keep the addon and plan mappers from drifting the next time tax handling changes.As per coding guidelines,
**/*.go: In general when reviewing the Golang code make readability and maintainability a priority, even potentially suggest restructuring the code to improve them.Also applies to: 302-315
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/addon/adapter/mapping.go` around lines 102 - 115, Both FromAddonRateCardRow and FromPlanRateCardRow duplicate the TaxCode hydrate/backfill flow; extract that into a shared helper (e.g., hydrateAndBackfillTaxForRateCard) that accepts the rate card row (the object with Edges.TaxCodeOrErr and TaxBehavior) and a pointer to meta (with TaxConfig/TaxCode) and performs: call TaxCodeOrErr, map via taxcodeadapter.MapTaxCodeFromEntity, set meta.TaxCode, then call productcatalogadapter.BackfillTaxConfig(meta.TaxConfig, r.TaxBehavior, meta.TaxCode); update both FromAddonRateCardRow and FromPlanRateCardRow to call this helper and propagate any errors returned.
🤖 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/taxcode_test.go`:
- Around line 63-449: The tests in TestAddonTaxCodeDualWrite only assert the
returned TaxConfig and TaxCode entity but don't verify the dedicated DB columns;
after each CreateAddon and UpdateAddon call (e.g., within the "Create" and
"Update" subtests such as "StripeCodeOnly", "ChangeStripeCode",
"RemoveTaxConfig", etc.), perform a direct read of the persisted AddonRateCard
row (use the same DB accessor used elsewhere or a helper that queries the
AddonRateCard table for the addon—refer to env.Addon.CreateAddon /
env.Addon.UpdateAddon and the rate card keys) and assert that the dedicated
columns tax_code_id and tax_behavior (or the DB column names used by
asAddonRateCardRow) contain the expected values (nil or the TaxCode ID and
expected behavior enum) to ensure the dedicated columns—not just JSONB—were
written.
---
Nitpick comments:
In `@openmeter/productcatalog/addon/adapter/mapping.go`:
- Around line 102-115: Both FromAddonRateCardRow and FromPlanRateCardRow
duplicate the TaxCode hydrate/backfill flow; extract that into a shared helper
(e.g., hydrateAndBackfillTaxForRateCard) that accepts the rate card row (the
object with Edges.TaxCodeOrErr and TaxBehavior) and a pointer to meta (with
TaxConfig/TaxCode) and performs: call TaxCodeOrErr, map via
taxcodeadapter.MapTaxCodeFromEntity, set meta.TaxCode, then call
productcatalogadapter.BackfillTaxConfig(meta.TaxConfig, r.TaxBehavior,
meta.TaxCode); update both FromAddonRateCardRow and FromPlanRateCardRow to call
this helper and propagate any errors returned.
In `@openmeter/taxcode/errors.go`:
- Around line 60-71: Extract the repeated base builder into a small helper
(e.g., buildTaxCodeNotFound or taxCodeNotFoundWithPath) that takes the differing
path string and returns ErrTaxCodeNotFound.WithPathString(path), then use that
helper inside NewTaxCodeNotFoundError and NewTaxCodeByAppMappingNotFoundError to
chain WithAttr calls; reference ErrTaxCodeNotFound, NewTaxCodeNotFoundError and
NewTaxCodeByAppMappingNotFoundError to locate the current duplication and
replace the duplicated WithPathString(...) start of the chain with the helper
call.
🪄 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: c094852f-7245-476a-a1d3-affa0003b4bc
📒 Files selected for processing (6)
openmeter/productcatalog/addon/adapter/mapping.goopenmeter/productcatalog/addon/service/taxcode_test.goopenmeter/productcatalog/plan/adapter/mapping.goopenmeter/taxcode/adapter/taxcode.goopenmeter/taxcode/errors.goopenmeter/taxcode/service/taxcode.go
🚧 Files skipped from review as they are similar to previous changes (3)
- openmeter/taxcode/adapter/taxcode.go
- openmeter/productcatalog/plan/adapter/mapping.go
- openmeter/taxcode/service/taxcode.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
openmeter/productcatalog/addon/service/taxcode_test.go (1)
482-495:⚠️ Potential issue | 🟡 MinorPlease assert
TaxCodeIDon theGetAddonreadback too.This subtest proves Stripe/Behavior survived the fetch, but it never checks the fetched
TaxConfig.TaxCodeID. If the adapter stops wiring that field on reads, this path still stays green because the raw DB assertion is separate from the returned payload.🧪 Minimal assertion to add
tc := getFirstAddonRCTaxConfig(t, fetched) require.NotNil(t, tc) + require.NotNil(t, tc.TaxCodeID, "TaxCodeID must be present on readback") // Behavior should be present require.NotNil(t, tc.Behavior) assert.Equal(t, productcatalog.ExclusiveTaxBehavior, *tc.Behavior) @@ tcEntity13, err := findAddonTaxCodeByStripeCode(t, ctx, env.TaxCode, namespace, "txcd_13000001") require.NoError(t, err) + assert.Equal(t, tcEntity13.ID, *tc.TaxCodeID) assertAddonRCDBCols(t, ctx, env, fetched.ID, features[0].Key, lo.ToPtr(tcEntity13.ID), lo.ToPtr(productcatalog.ExclusiveTaxBehavior))As per coding guidelines,
**/*_test.go: "Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/addon/service/taxcode_test.go` around lines 482 - 495, The test is missing an assertion that the fetched Addon TaxConfig's TaxCodeID is populated; after retrieving tc via getFirstAddonRCTaxConfig and finding tcEntity13 with findAddonTaxCodeByStripeCode, add an assertion that tc.TaxCodeID is non-nil and equals the ID of tcEntity13 (use the same pointer/ID comparison style as assertAddonRCDBCols or require/assert helpers used elsewhere) so the readback verifies TaxCodeID wiring in the returned payload.
🧹 Nitpick comments (1)
openmeter/productcatalog/addon/service/taxcode_test.go (1)
84-112: Might be worth extracting the shared meter/feature bootstrap.The same env + namespace + meter/feature setup shows up in all three top-level tests. A small helper returning
env,namespace, andfeatureswould keep each suite focused on the tax behavior it’s actually validating. As per coding guidelines,**/*.go: "In general when reviewing the Golang code make readability and maintainability a priority, even potentially suggest restructuring the code to improve them."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/addon/service/taxcode_test.go` around lines 84 - 112, Extract the repeated test setup into a helper (e.g., setupTestEnvFeatures) that creates and returns the TestEnv, namespace and []feature.Feature: call pctestutils.NewTestEnv, defer/cleanup env.Close, call env.DBSchemaMigrate, create namespace via pctestutils.NewTestNamespace, replace meters with env.Meter.ReplaceMeters(pctestutils.NewTestMeters(...)), list meters via env.Meter.ListMeters with meter.ListMetersParams, then create features by calling env.Feature.CreateFeature for each pctestutils.NewTestFeatureFromMeter result and return the env, namespace and features slice; update each top-level test to call this helper and remove the duplicated lines.
🤖 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/taxcode_test.go`:
- Around line 313-461: Add a new subtest under the existing "Update" suite
(e.g., t.Run("TaxBehaviorOnlyUpdate", ...)) that creates an addon with a
RateCard containing a Stripe TaxConfig (Stripe.Code set, TaxBehavior nil), then
calls env.Addon.UpdateAddon with the same Stripe.Code but sets TaxBehavior to a
value (and another subcase clearing it), and asserts that
getFirstAddonRCTaxConfig returns the same TaxCodeID (unchanged) and same
Stripe.Code, while the tax_behavior column is updated as expected using
assertAddonRCDBCols; use env.Addon.CreateAddon, env.Addon.UpdateAddon,
getFirstAddonRCTaxConfig, findAddonTaxCodeByStripeCode and assertAddonRCDBCols
to verify TaxCode entity still exists and only the tax_behavior column changed.
---
Duplicate comments:
In `@openmeter/productcatalog/addon/service/taxcode_test.go`:
- Around line 482-495: The test is missing an assertion that the fetched Addon
TaxConfig's TaxCodeID is populated; after retrieving tc via
getFirstAddonRCTaxConfig and finding tcEntity13 with
findAddonTaxCodeByStripeCode, add an assertion that tc.TaxCodeID is non-nil and
equals the ID of tcEntity13 (use the same pointer/ID comparison style as
assertAddonRCDBCols or require/assert helpers used elsewhere) so the readback
verifies TaxCodeID wiring in the returned payload.
---
Nitpick comments:
In `@openmeter/productcatalog/addon/service/taxcode_test.go`:
- Around line 84-112: Extract the repeated test setup into a helper (e.g.,
setupTestEnvFeatures) that creates and returns the TestEnv, namespace and
[]feature.Feature: call pctestutils.NewTestEnv, defer/cleanup env.Close, call
env.DBSchemaMigrate, create namespace via pctestutils.NewTestNamespace, replace
meters with env.Meter.ReplaceMeters(pctestutils.NewTestMeters(...)), list meters
via env.Meter.ListMeters with meter.ListMetersParams, then create features by
calling env.Feature.CreateFeature for each pctestutils.NewTestFeatureFromMeter
result and return the env, namespace and features slice; update each top-level
test to call this helper and remove the duplicated lines.
🪄 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: 35f73f31-2b71-42e5-a122-945c955f5ce0
📒 Files selected for processing (2)
openmeter/productcatalog/addon/service/taxcode_test.goopenmeter/productcatalog/plan/service/taxcode_test.go
✅ Files skipped from review due to trivial changes (1)
- openmeter/productcatalog/plan/service/taxcode_test.go
Comment on lines
+313
to
+461
| t.Run("Update", func(t *testing.T) { | ||
| t.Run("AddTaxConfig", func(t *testing.T) { | ||
| // Create addon without TaxConfig | ||
| input := newTestAddonInput(t, namespace, newTestAddonFlatRateCard(features[0], nil)) | ||
| input.Key = "addon-update-add-tax" | ||
| input.Name = "Update Add Tax" | ||
|
|
||
| a, err := env.Addon.CreateAddon(ctx, input) | ||
| require.NoError(t, err) | ||
|
|
||
| tc := getFirstAddonRCTaxConfig(t, a) | ||
| assert.Nil(t, tc) | ||
|
|
||
| // Update to add TaxConfig with Stripe code | ||
| updatedRateCards := productcatalog.RateCards{ | ||
| newTestAddonFlatRateCard(features[0], &productcatalog.TaxConfig{ | ||
| Stripe: &productcatalog.StripeTaxConfig{Code: "txcd_70000001"}, | ||
| }), | ||
| } | ||
|
|
||
| updated, err := env.Addon.UpdateAddon(ctx, addon.UpdateAddonInput{ | ||
| NamespacedID: a.NamespacedID, | ||
| RateCards: &updatedRateCards, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| tc = getFirstAddonRCTaxConfig(t, updated) | ||
| require.NotNil(t, tc) | ||
| require.NotNil(t, tc.Stripe) | ||
| assert.Equal(t, "txcd_70000001", tc.Stripe.Code) | ||
| require.NotNil(t, tc.TaxCodeID, "TaxCodeID must be populated after update") | ||
|
|
||
| tcEntity, err := findAddonTaxCodeByStripeCode(t, ctx, env.TaxCode, namespace, "txcd_70000001") | ||
| require.NoError(t, err) | ||
| assert.Equal(t, *tc.TaxCodeID, tcEntity.ID) | ||
|
|
||
| assertAddonRCDBCols(t, ctx, env, updated.ID, features[0].Key, lo.ToPtr(tcEntity.ID), nil) | ||
| }) | ||
|
|
||
| t.Run("ChangeStripeCode", func(t *testing.T) { | ||
| input := newTestAddonInput(t, namespace, newTestAddonFlatRateCard(features[0], &productcatalog.TaxConfig{ | ||
| Stripe: &productcatalog.StripeTaxConfig{Code: "txcd_80000001"}, | ||
| })) | ||
| input.Key = "addon-update-change-code" | ||
| input.Name = "Update Change Code" | ||
|
|
||
| a, err := env.Addon.CreateAddon(ctx, input) | ||
| require.NoError(t, err) | ||
|
|
||
| oldTC := getFirstAddonRCTaxConfig(t, a) | ||
| require.NotNil(t, oldTC) | ||
| require.NotNil(t, oldTC.TaxCodeID) | ||
| oldTaxCodeID := *oldTC.TaxCodeID | ||
|
|
||
| // Update to different stripe code | ||
| updatedRateCards := productcatalog.RateCards{ | ||
| newTestAddonFlatRateCard(features[0], &productcatalog.TaxConfig{ | ||
| Stripe: &productcatalog.StripeTaxConfig{Code: "txcd_90000001"}, | ||
| }), | ||
| } | ||
|
|
||
| updated, err := env.Addon.UpdateAddon(ctx, addon.UpdateAddonInput{ | ||
| NamespacedID: a.NamespacedID, | ||
| RateCards: &updatedRateCards, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| newTC := getFirstAddonRCTaxConfig(t, updated) | ||
| require.NotNil(t, newTC) | ||
| require.NotNil(t, newTC.Stripe) | ||
| assert.Equal(t, "txcd_90000001", newTC.Stripe.Code) | ||
| require.NotNil(t, newTC.TaxCodeID) | ||
|
|
||
| assert.NotEqual(t, oldTaxCodeID, *newTC.TaxCodeID, "new stripe code must create a new TaxCode entity") | ||
|
|
||
| // Old TaxCode entity should still exist | ||
| _, err = findAddonTaxCodeByStripeCode(t, ctx, env.TaxCode, namespace, "txcd_80000001") | ||
| assert.NoError(t, err, "old TaxCode entity should still exist") | ||
|
|
||
| newTCEntity, err := findAddonTaxCodeByStripeCode(t, ctx, env.TaxCode, namespace, "txcd_90000001") | ||
| require.NoError(t, err) | ||
| assertAddonRCDBCols(t, ctx, env, updated.ID, features[0].Key, lo.ToPtr(newTCEntity.ID), nil) | ||
| }) | ||
|
|
||
| t.Run("RemoveTaxConfig", func(t *testing.T) { | ||
| input := newTestAddonInput(t, namespace, newTestAddonFlatRateCard(features[0], &productcatalog.TaxConfig{ | ||
| Stripe: &productcatalog.StripeTaxConfig{Code: "txcd_11000001"}, | ||
| })) | ||
| input.Key = "addon-update-remove-tax" | ||
| input.Name = "Update Remove Tax" | ||
|
|
||
| a, err := env.Addon.CreateAddon(ctx, input) | ||
| require.NoError(t, err) | ||
|
|
||
| tc := getFirstAddonRCTaxConfig(t, a) | ||
| require.NotNil(t, tc) | ||
| require.NotNil(t, tc.TaxCodeID) | ||
|
|
||
| // Update to remove TaxConfig | ||
| updatedRateCards := productcatalog.RateCards{ | ||
| newTestAddonFlatRateCard(features[0], nil), | ||
| } | ||
|
|
||
| updated, err := env.Addon.UpdateAddon(ctx, addon.UpdateAddonInput{ | ||
| NamespacedID: a.NamespacedID, | ||
| RateCards: &updatedRateCards, | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| tc = getFirstAddonRCTaxConfig(t, updated) | ||
| assert.Nil(t, tc, "TaxConfig should be nil after removal") | ||
|
|
||
| // TaxCode entity should still exist (orphaned, not deleted) | ||
| _, err = findAddonTaxCodeByStripeCode(t, ctx, env.TaxCode, namespace, "txcd_11000001") | ||
| assert.NoError(t, err, "TaxCode entity should not be deleted") | ||
|
|
||
| assertAddonRCDBCols(t, ctx, env, updated.ID, features[0].Key, nil, nil) | ||
| }) | ||
|
|
||
| t.Run("MetadataOnlyUpdate", func(t *testing.T) { | ||
| input := newTestAddonInput(t, namespace, newTestAddonFlatRateCard(features[0], &productcatalog.TaxConfig{ | ||
| Stripe: &productcatalog.StripeTaxConfig{Code: "txcd_12000001"}, | ||
| })) | ||
| input.Key = "addon-update-metadata-only" | ||
| input.Name = "Update Metadata Only" | ||
|
|
||
| a, err := env.Addon.CreateAddon(ctx, input) | ||
| require.NoError(t, err) | ||
|
|
||
| tc := getFirstAddonRCTaxConfig(t, a) | ||
| require.NotNil(t, tc) | ||
| require.NotNil(t, tc.TaxCodeID) | ||
| originalTaxCodeID := *tc.TaxCodeID | ||
|
|
||
| // Update only addon name, no ratecards | ||
| updated, err := env.Addon.UpdateAddon(ctx, addon.UpdateAddonInput{ | ||
| NamespacedID: a.NamespacedID, | ||
| Name: lo.ToPtr("Updated Name"), | ||
| }) | ||
| require.NoError(t, err) | ||
|
|
||
| tc = getFirstAddonRCTaxConfig(t, updated) | ||
| require.NotNil(t, tc) | ||
| require.NotNil(t, tc.TaxCodeID) | ||
| assert.Equal(t, originalTaxCodeID, *tc.TaxCodeID, "TaxCodeID should be unchanged on metadata-only update") | ||
|
|
||
| assertAddonRCDBCols(t, ctx, env, updated.ID, features[0].Key, lo.ToPtr(originalTaxCodeID), nil) | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
Please add an update case that only changes TaxBehavior.
Right now the update coverage exercises TaxCodeID churn and whole-config removal, but not the case where the Stripe code stays the same and only tax_behavior is added, changed, or cleared. A regression in the dedicated-column update path would still slip through. As per coding guidelines, **/*_test.go: "Make sure the tests are comprehensive and cover the changes. Keep a strong focus on unit tests and in-code integration tests."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/productcatalog/addon/service/taxcode_test.go` around lines 313 -
461, Add a new subtest under the existing "Update" suite (e.g.,
t.Run("TaxBehaviorOnlyUpdate", ...)) that creates an addon with a RateCard
containing a Stripe TaxConfig (Stripe.Code set, TaxBehavior nil), then calls
env.Addon.UpdateAddon with the same Stripe.Code but sets TaxBehavior to a value
(and another subcase clearing it), and asserts that getFirstAddonRCTaxConfig
returns the same TaxCodeID (unchanged) and same Stripe.Code, while the
tax_behavior column is updated as expected using assertAddonRCDBCols; use
env.Addon.CreateAddon, env.Addon.UpdateAddon, getFirstAddonRCTaxConfig,
findAddonTaxCodeByStripeCode and assertAddonRCDBCols to verify TaxCode entity
still exists and only the tax_behavior column changed.
turip
approved these changes
Mar 24, 2026
Comment on lines
+73
to
76
| func IsTaxCodeNotFoundError(err error) bool { | ||
| var vi models.ValidationIssue | ||
| return errors.As(err, &vi) && vi.Code() == ErrCodeTaxCodeNotFound | ||
| } |
Member
There was a problem hiding this comment.
I think this is equivalent of errors.Is(err, ErrTaxCodeNotFound), but if it works I am fine with this.
This was referenced Mar 27, 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
New Features
Tests