feat(taxcode): prevent deletion of plan-referenced tax codes and validate tax codes on plan publish#4557
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@greptileai review this draft |
borbelyr-kong
added
release-note/misc
Greptile SummaryThis PR adds two complementary safeguards around tax code lifecycle: (1) a namespace-scoped filter was missing from the
Confidence Score: 4/5Safe to merge after addressing the empty-id error-classification edge case; the core delete-prevention and publish-time validation logic is correct. The namespace fix in the PreDelete hook and the publish-time tax code validation are implemented correctly and consistently follow the existing FeatureResolver pattern. Two small gaps worth noting: the concrete resolver doesn't guard against an empty id string, causing a system-level validation error to surface instead of a tidy not-found response; and the unit-test stub ignores the id argument, leaving the lookup-key path untested. Neither blocks the core flows. openmeter/productcatalog/taxcoderesolver/resolver.go — missing empty-id guard; openmeter/productcatalog/taxcoderesolver_test.go — stub resolver does not assert on the id passed to it. Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func (s stubTaxCodeResolver) Namespace() string { return s.namespace } | ||
|
|
||
| func (s stubTaxCodeResolver) ResolveTaxCode(_ context.Context, _ string) (*taxcode.TaxCode, error) { | ||
| return s.result, s.err | ||
| } |
There was a problem hiding this comment.
id argument — test can't verify correct lookup key
stubTaxCodeResolver.ResolveTaxCode discards the id parameter and always returns the preset result/err. This means the tests cannot catch a bug where the wrong field (e.g. rc.TaxConfig.TaxCodeID vs. some other field) is passed to the resolver. Adding an expectedID string field to the stub and asserting on it (or using require.Equal(t, taxCodeID, capturedID)) would make the tests meaningful for the lookup-key path.
| func (r *resolver) ResolveTaxCode(ctx context.Context, namespace string, id string) (*taxcode.TaxCode, error) { | ||
| if namespace == "" { | ||
| return nil, errors.New("namespace is not set") | ||
| } |
There was a problem hiding this comment.
id produces a misleading system error instead of a user-friendly not-found error
When TaxCodeID is a non-nil pointer to an empty string, GetTaxCodeInput.Validate() returns ErrResourceIDEmpty — a validation issue that is neither IsGenericNotFoundError nor IsTaxCodeNotFoundError. The caller in ValidateRateCardsWithTaxCodes therefore falls into the generic "failed to resolve tax code for ratecard: ..." branch rather than surfacing ErrRateCardTaxCodeNotFound. An early guard here keeps the error classification consistent.
| func (r *resolver) ResolveTaxCode(ctx context.Context, namespace string, id string) (*taxcode.TaxCode, error) { | |
| if namespace == "" { | |
| return nil, errors.New("namespace is not set") | |
| } | |
| func (r *resolver) ResolveTaxCode(ctx context.Context, namespace string, id string) (*taxcode.TaxCode, error) { | |
| if namespace == "" { | |
| return nil, errors.New("namespace is not set") | |
| } | |
| if id == "" { | |
| return nil, errors.New("id is not set") | |
| } |
| if tc == nil || tc.DeletedAt != nil { | ||
| errs = append(errs, models.ErrorWithFieldPrefix(rateCardFieldSelector, ErrRateCardTaxCodeNotFound)) | ||
| } |
There was a problem hiding this comment.
tc == nil branch is unreachable with the concrete resolver
resolver.ResolveTaxCode either returns an error or &tc (a non-nil pointer). A (nil, nil) return is impossible from the production implementation, so tc == nil can never be true here. The DeletedAt check alone is sufficient. Since the NamespacedTaxCodeResolver interface doesn't prohibit (nil, nil) from other implementations, keeping the guard is defensible — but a comment explaining the intent would help future readers.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openmeter/productcatalog/taxcoderesolver_test.go (1)
84-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
t.Context()instead ofcontext.Background()in this test.Line 84 should use the test-scoped context so cancellation/deadline stays tied to the test harness.
🔧 Proposed change
- ctx := context.Background() + ctx := t.Context()As per coding guidelines, "Use
t.Context()in Go tests when atesting.Tortesting.TBis available instead of introducingcontext.Background()to keep cancellation tied to the test harness."🤖 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 `@openmeter/productcatalog/taxcoderesolver_test.go` around lines 84 - 85, Replace the `context.Background()` call with `t.Context()` in the ctx variable initialization to ensure the context is properly tied to the test harness for correct cancellation and deadline management. The ctx variable assignment should use t.Context() instead of context.Background() to align with the coding guidelines for Go tests.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 `@openmeter/productcatalog/taxcoderesolver_test.go`:
- Around line 84-85: Replace the `context.Background()` call with `t.Context()`
in the ctx variable initialization to ensure the context is properly tied to the
test harness for correct cancellation and deadline management. The ctx variable
assignment should use t.Context() instead of context.Background() to align with
the coding guidelines for Go tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0ec3f2f6-65e0-432f-97b6-52cd84a58039
📒 Files selected for processing (17)
app/common/productcatalog.gocmd/billing-worker/wire_gen.gocmd/jobs/internal/wire_gen.gocmd/server/wire_gen.goopenmeter/productcatalog/errors.goopenmeter/productcatalog/plan.goopenmeter/productcatalog/plan/service/plan.goopenmeter/productcatalog/plan/service/service.goopenmeter/productcatalog/ratecard.goopenmeter/productcatalog/taxcoderesolver.goopenmeter/productcatalog/taxcoderesolver/resolver.goopenmeter/productcatalog/taxcoderesolver_test.goopenmeter/productcatalog/testutils/env.goopenmeter/subscription/testutils/service.goopenmeter/taxcode/service/hooks/planvalidator.gotest/billing/subscription_suite.gotest/customer/testenv.go
borbelyr-kong
force-pushed
the
feat/taxcode-plan-publish-validation
branch
2 times, most recently
from
7a906df to
2f9c5ab
Compare
…date tax codes on plan publish
…cessor and rate-card tax code guards with tests
borbelyr-kong
force-pushed
the
feat/taxcode-plan-publish-validation
branch
from
2f9c5ab to
3a58a03
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
openmeter/productcatalog/plan/service/taxcode_test.go (1)
892-893: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSmall nudge: derive from
t.Context().You can build the cancelable context off the test context so cancellation stays tied to the harness (and
t.Context()is already canceled at cleanup, so this stays nicely scoped):♻️ Suggested tweak
- ctx, cancel := context.WithCancel(context.Background()) + ctx, cancel := context.WithCancel(t.Context()) defer cancel()As per coding guidelines: "Use
t.Context()in Go tests when atesting.Tortesting.TBis available instead of introducingcontext.Background()".🤖 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 `@openmeter/productcatalog/plan/service/taxcode_test.go` around lines 892 - 893, The test context setup is using context.Background() directly instead of the test harness context. Update the cancelable context creation in taxcode_test.go to derive from t.Context() so it stays scoped to the test lifecycle, keeping the existing cancel behavior while tying cancellation to the test cleanup.Source: Coding guidelines
openmeter/productcatalog/taxcoderesolver_test.go (1)
84-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTiny one: prefer
t.Context()here.Since
tis right there,t.Context()keeps cancellation tied to the test harness. Quick swap:♻️ Suggested tweak
- ctx := context.Background() + ctx := t.Context()As per coding guidelines: "Use
t.Context()in Go tests when atesting.Tortesting.TBis available instead of introducingcontext.Background()".🤖 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 `@openmeter/productcatalog/taxcoderesolver_test.go` at line 84, The test setup currently creates a standalone context with context.Background() instead of using the test-scoped context. Update the test in taxcoderesolver_test.go to use t.Context() wherever the local testing.T is available, so cancellation and deadlines stay tied to the test harness; this should be applied at the ctx initialization near the TaxCodeResolver test setup.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 `@openmeter/productcatalog/plan/service/taxcode_test.go`:
- Around line 892-893: The test context setup is using context.Background()
directly instead of the test harness context. Update the cancelable context
creation in taxcode_test.go to derive from t.Context() so it stays scoped to the
test lifecycle, keeping the existing cancel behavior while tying cancellation to
the test cleanup.
In `@openmeter/productcatalog/taxcoderesolver_test.go`:
- Line 84: The test setup currently creates a standalone context with
context.Background() instead of using the test-scoped context. Update the test
in taxcoderesolver_test.go to use t.Context() wherever the local testing.T is
available, so cancellation and deadlines stay tied to the test harness; this
should be applied at the ctx initialization near the TaxCodeResolver test setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2eb85a87-7d87-4680-bbb4-0807a71c8fa2
📒 Files selected for processing (17)
app/common/productcatalog.gocmd/billing-worker/wire_gen.gocmd/jobs/internal/wire_gen.gocmd/server/wire_gen.goopenmeter/productcatalog/errors.goopenmeter/productcatalog/plan.goopenmeter/productcatalog/plan/service/plan.goopenmeter/productcatalog/plan/service/service.goopenmeter/productcatalog/plan/service/taxcode_test.goopenmeter/productcatalog/ratecard.goopenmeter/productcatalog/taxcoderesolver.goopenmeter/productcatalog/taxcoderesolver/resolver.goopenmeter/productcatalog/taxcoderesolver_test.goopenmeter/productcatalog/testutils/env.goopenmeter/subscription/testutils/service.gotest/billing/subscription_suite.gotest/customer/testenv.go
✅ Files skipped from review due to trivial changes (2)
- cmd/billing-worker/wire_gen.go
- cmd/jobs/internal/wire_gen.go
🚧 Files skipped from review as they are similar to previous changes (11)
- openmeter/productcatalog/taxcoderesolver.go
- test/billing/subscription_suite.go
- openmeter/productcatalog/errors.go
- openmeter/productcatalog/plan/service/service.go
- cmd/server/wire_gen.go
- openmeter/productcatalog/plan.go
- openmeter/productcatalog/testutils/env.go
- test/customer/testenv.go
- openmeter/productcatalog/ratecard.go
- openmeter/productcatalog/plan/service/plan.go
- app/common/productcatalog.go
1 participant
@coderabbitai
Summary by CodeRabbit
New Features
Bug Fixes
Tests