fix(tax-code): fix tax code delete#4555
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds tax-code lifecycle hooks, blocks deletion when a plan references a tax code, adds TaxCodes filtering to plan listing, removes the delete feature flag, and rejects soft-deleted tax codes in organization-default tax code updates. ChangesTax code deletion guard via plan-reference hooks
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@greptileai review this draft |
Greptile SummaryThis PR enables tax code deletion by removing the feature-flag guard and introducing a
Confidence Score: 4/5Safe to merge with awareness of the open soft-delete visibility regression in GetTaxCode — all new enforcement logic is sound, but fetching a tax code by ID now returns deleted records to any caller that does not add its own IsDeleted guard. The adapter's GetTaxCode no longer filters out soft-deleted records, and the public service GetTaxCode method passes through without adding the guard. Every call site introduced in this PR adds an explicit IsDeleted check, but pre-existing callers (including the GET /taxcodes/{id} API handler) have not been updated, so they will return deleted records instead of 404. The rest of the change — the plan hook, the org-defaults hardening, the ListPlans filter — is well-constructed and tested. openmeter/taxcode/adapter/taxcode.go and openmeter/taxcode/service/taxcode.go — the removal of DeletedAtIsNil from GetTaxCode needs a corresponding guard in the service-level GetTaxCode to avoid exposing soft-deleted records through the public API. Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
openmeter/taxcode/service/hooks/planvalidator.go (1)
26-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign
Validate()with the repo’s validation aggregation convention.This method returns on first error. In this codebase,
Validate() erroris expected to aggregate errors and returnmodels.NewNillableGenericValidationError(errors.Join(errs...)).♻️ Suggested refactor
import ( "context" + "errors" "fmt" @@ func (e PlanValidatorHookConfig) Validate() error { + var errs []error + if e.PlanService == nil { - return fmt.Errorf("plan service is required") + errs = append(errs, fmt.Errorf("plan service is required")) } - return nil + return models.NewNillableGenericValidationError(errors.Join(errs...)) }As per coding guidelines
For Go Validate() error methods, prefer collecting all validation issues into var errs []error and returning models.NewNillableGenericValidationError(errors.Join(errs...)).🤖 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/taxcode/service/hooks/planvalidator.go` around lines 26 - 32, The Validate() method in PlanValidatorHookConfig currently returns immediately on the first error instead of aggregating all validation errors as per the codebase convention. Refactor the method to collect validation errors in a slice (var errs []error) rather than returning early. When the PlanService field is nil, append the error to the errs slice and continue with any other validations. Finally, return models.NewNillableGenericValidationError(errors.Join(errs...)) to aggregate and return all collected errors together.Source: Coding guidelines
openmeter/taxcode/service/hooks/planvalidator_test.go (1)
61-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNice coverage on the core flow—please add the two missing error-path tests.
Would be great to add subtests for:
NewPlanValidatorHookwithPlanService: nilPreDeletewhenListPlansreturns an errorThat closes coverage for all new branches in this file.
As per path instructions
**/*_test.go: Make sure the tests are comprehensive and cover the changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/taxcode/service/hooks/planvalidator_test.go` around lines 61 - 115, The TestPlanValidatorHook_PreDelete test function is missing coverage for error paths. Add two new subtests: one that calls NewPlanValidatorHook with a nil PlanService value in the PlanValidatorHookConfig to verify it properly handles and returns an error, and another subtest that uses a stubPlanService configured to return an error from ListPlans (by adding an error field to the stub) and verifies that PreDelete properly propagates that error. These error-path tests will close the coverage gaps for all branches in the hook implementation.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openmeter/taxcode/adapter/taxcode.go`:
- Around line 113-116: The GetTaxCode adapter query (lines 113-116) is missing
the DeletedAtIsNil() filter that all other adapter queries like
GetTaxCodeByAppMapping and ListTaxCodes include, allowing it to unexpectedly
return soft-deleted records. Fix this by adding an IncludeDeleted field to the
GetTaxCodeInput struct, then conditionally apply the DeletedAtIsNil() filter in
the GetTaxCode adapter method based on this field (defaulting to false),
ensuring callers can explicitly opt-in to retrieving soft-deleted records.
Alternatively, add DeletedAtIsNil() unconditionally to the GetTaxCode query and
refactor DeleteTaxCode.PostDelete to use a separate internal query method that
explicitly handles soft-deleted records to maintain consistency across all
adapter queries.
In `@openmeter/taxcode/service/hooks/planvalidator.go`:
- Around line 54-59: The pre-delete integrity check for tax codes uses a
hard-coded Status slice that excludes PlanStatusInvalid and will regress when
new plan statuses are introduced. Instead of explicitly listing specific
statuses in the Status field, modify the query to include all possible plan
statuses or query without status filtering (or filter only for deleted plans if
applicable) so that the integrity check catches references in plans with any
status, including invalid and future statuses.
---
Nitpick comments:
In `@openmeter/taxcode/service/hooks/planvalidator_test.go`:
- Around line 61-115: The TestPlanValidatorHook_PreDelete test function is
missing coverage for error paths. Add two new subtests: one that calls
NewPlanValidatorHook with a nil PlanService value in the PlanValidatorHookConfig
to verify it properly handles and returns an error, and another subtest that
uses a stubPlanService configured to return an error from ListPlans (by adding
an error field to the stub) and verifies that PreDelete properly propagates that
error. These error-path tests will close the coverage gaps for all branches in
the hook implementation.
In `@openmeter/taxcode/service/hooks/planvalidator.go`:
- Around line 26-32: The Validate() method in PlanValidatorHookConfig currently
returns immediately on the first error instead of aggregating all validation
errors as per the codebase convention. Refactor the method to collect validation
errors in a slice (var errs []error) rather than returning early. When the
PlanService field is nil, append the error to the errs slice and continue with
any other validations. Finally, return
models.NewNillableGenericValidationError(errors.Join(errs...)) to aggregate and
return all collected errors together.
🪄 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: e988c293-4668-4ac6-b727-1f60ca7ec354
📒 Files selected for processing (16)
api/v3/handlers/taxcodes/delete.goapp/common/taxcode.gocmd/server/wire.gocmd/server/wire_gen.goopenmeter/productcatalog/plan/adapter/adapter_test.goopenmeter/productcatalog/plan/adapter/plan.goopenmeter/productcatalog/plan/service.goopenmeter/productcatalog/tax_test.goopenmeter/server/server_test.goopenmeter/taxcode/adapter/taxcode.goopenmeter/taxcode/errors.goopenmeter/taxcode/service.goopenmeter/taxcode/service/hooks/planvalidator.goopenmeter/taxcode/service/hooks/planvalidator_test.goopenmeter/taxcode/service/service.goopenmeter/taxcode/service/taxcode.go
💤 Files with no reviewable changes (1)
- api/v3/handlers/taxcodes/delete.go
| Where( | ||
| taxcodedb.Namespace(input.Namespace), | ||
| taxcodedb.ID(input.ID), | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect TaxCode service methods, especially GetTaxCode and delete flow
ast-grep outline openmeter/taxcode/service/taxcode.go --view expanded
# 2) Find all GetTaxCode call sites in taxcode service + v3 taxcode handlers
rg -n --type=go '\bGetTaxCode\s*\(' openmeter/taxcode openmeter/api/v3/handlers/taxcodes
# 3) Check whether deleted-row guards still exist in service/handler paths
rg -n --type=go 'DeletedAt|IncludeDeleted|IsDeleted|NotFound' openmeter/taxcode openmeter/api/v3/handlers/taxcodesRepository: openmeterio/openmeter
Length of output: 2389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Get the full GetTaxCode adapter implementation (lines 106-130)
sed -n '106,130p' openmeter/taxcode/adapter/taxcode.go
# 2) Get the DeleteTaxCode service implementation, especially the refetch at line 182
sed -n '149,195p' openmeter/taxcode/service/taxcode.go
# 3) Check UpdateTaxCode service method for soft-delete guards
sed -n '34,67p' openmeter/taxcode/service/taxcode.go
# 4) Look for any NotFound or deletion-aware logic in the service
rg -n --type=go 'ErrNotFound|NotFound|soft.*delete' openmeter/taxcode/service/ -A 2Repository: openmeterio/openmeter
Length of output: 6314
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Look at adapter.DeleteTaxCode implementation
sed -n '127,150p' openmeter/taxcode/adapter/taxcode.go
# 2) Check ListTaxCodes for soft-delete filtering
sed -n '60,90p' openmeter/taxcode/adapter/taxcode.go
# 3) Search for test coverage on GetTaxCode post-deletion
rg -n --type=go 'GetTaxCode.*after.*[dD]elete|[dD]elete.*GetTaxCode' openmeter/taxcode -A 5
# 4) Check if taxcodedb schema has DeletedAt and how it's used
rg -n --type=go 'DeletedAt|WithDeletedAt' openmeter/taxcode/adapterRepository: openmeterio/openmeter
Length of output: 3311
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if GetTaxCodeInput has any IncludeDeleted parameter
rg -n --type=go 'type GetTaxCodeInput' openmeter/taxcode -A 10
# Verify the complete DeleteTaxCode adapter to see the soft-delete operation
sed -n '200,220p' openmeter/taxcode/adapter/taxcode.go
# Check if there are any tests specifically for getting a deleted tax code
rg -n --type=go 'Delete.*GetTaxCode|GetTaxCode.*[Dd]elete' openmeter/taxcode --type=go -B 2 -A 8Repository: openmeterio/openmeter
Length of output: 1235
Add an IncludeDeleted parameter to GetTaxCodeInput or refactor DeleteTaxCode.PostDelete to avoid exposing soft-deleted records.
The GetTaxCode adapter (lines 113–116) queries only by namespace+ID and lacks the DeletedAtIsNil() filter that all other adapter queries use (e.g., GetTaxCodeByAppMapping, UpdateTaxCode, ListTaxCodes). This creates an asymmetry: GetTaxCode can return soft-deleted records, but callers have no way to opt into this behavior since GetTaxCodeInput has no IncludeDeleted field.
The refetch in DeleteTaxCode.PostDelete (service line 182) relies on this unprotected access, but regular reads—like UpdateTaxCode calling GetTaxCode at service line 40—could also unexpectedly receive deleted records. Either add explicit opt-in via IncludeDeleted or refactor the delete hook pattern to preserve consistency with the rest of the adapter.
🤖 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/taxcode/adapter/taxcode.go` around lines 113 - 116, The GetTaxCode
adapter query (lines 113-116) is missing the DeletedAtIsNil() filter that all
other adapter queries like GetTaxCodeByAppMapping and ListTaxCodes include,
allowing it to unexpectedly return soft-deleted records. Fix this by adding an
IncludeDeleted field to the GetTaxCodeInput struct, then conditionally apply the
DeletedAtIsNil() filter in the GetTaxCode adapter method based on this field
(defaulting to false), ensuring callers can explicitly opt-in to retrieving
soft-deleted records. Alternatively, add DeletedAtIsNil() unconditionally to the
GetTaxCode query and refactor DeleteTaxCode.PostDelete to use a separate
internal query method that explicitly handles soft-deleted records to maintain
consistency across all adapter queries.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
openmeter/taxcode/service/hooks/planhook.go (1)
52-82: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftThe status filtering gap flagged in the previous review is still present.
The previous review correctly identified that the hard-coded status list excludes
PlanStatusInvalid, which means a tax code can be deleted even when invalid plans reference it. Additionally, this explicit list will need manual updates when new plan statuses are introduced.🤖 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/taxcode/service/hooks/planhook.go` around lines 52 - 82, In the PreDelete method of the planHook struct, the Status filter is using a hard-coded list that excludes PlanStatusInvalid and will require manual updates when new statuses are added. Either add PlanStatusInvalid to the Status filter list to include invalid plans in the reference check, or remove the Status filter entirely if the intent is to verify that no plans of any status reference the tax code being deleted. This ensures all referenced plans are detected regardless of their status.
🤖 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.
Duplicate comments:
In `@openmeter/taxcode/service/hooks/planhook.go`:
- Around line 52-82: In the PreDelete method of the planHook struct, the Status
filter is using a hard-coded list that excludes PlanStatusInvalid and will
require manual updates when new statuses are added. Either add PlanStatusInvalid
to the Status filter list to include invalid plans in the reference check, or
remove the Status filter entirely if the intent is to verify that no plans of
any status reference the tax code being deleted. This ensures all referenced
plans are detected regardless of their status.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9b801067-be76-44bb-94a3-abeb7bee6e98
📒 Files selected for processing (7)
app/common/taxcode.gocmd/server/wire.gocmd/server/wire_gen.goopenmeter/taxcode/service/hooks/planhook.goopenmeter/taxcode/service/hooks/planhook_test.goopenmeter/taxcode/service/organizationdefaulttaxcodes.goopenmeter/taxcode/service/organizationdefaulttaxcodes_test.go
✅ Files skipped from review due to trivial changes (2)
- openmeter/taxcode/service/hooks/planhook_test.go
- cmd/server/wire_gen.go
…on update, and use a service-backed plan hook test
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openmeter/taxcode/service/hooks/planhook_test.go (1)
48-137: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNice, this reads really cleanly! The given/when/then comments make the two scenarios super easy to follow, and reusing the real
pctestutilsenvironment gives you genuine end-to-end confidence in the hook path. 👍One small thought for later: both subtests share the same
nsand run sequentially, so they're correctly relying on not.Parallel(). If anyone adds parallelism down the road, the shared namespace (and the plan created in the first subtest) would leak across cases. A tiny// note: subtests share ns; do not run in parallelwould save a future reader a head-scratch.🤖 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/taxcode/service/hooks/planhook_test.go` around lines 48 - 137, The shared namespace setup in TestPlanHookPreDelete means both subtests depend on the same state and must not be parallelized; add a brief note near the shared ns setup or test body making that constraint explicit. Use the TestPlanHookPreDelete subtests and the shared ns variable as the reference point so future changes don’t introduce t.Parallel() or other parallel execution that would leak plan/tax code state between cases.
🤖 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/taxcode/service/hooks/planhook_test.go`:
- Around line 48-137: The shared namespace setup in TestPlanHookPreDelete means
both subtests depend on the same state and must not be parallelized; add a brief
note near the shared ns setup or test body making that constraint explicit. Use
the TestPlanHookPreDelete subtests and the shared ns variable as the reference
point so future changes don’t introduce t.Parallel() or other parallel execution
that would leak plan/tax code state between cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4b811fdd-7359-453d-9583-eeb5833ae377
📒 Files selected for processing (4)
openmeter/taxcode/service/hooks/planhook.goopenmeter/taxcode/service/hooks/planhook_test.goopenmeter/taxcode/service/organizationdefaulttaxcodes.goopenmeter/taxcode/service/taxcode.go
🚧 Files skipped from review as they are similar to previous changes (2)
- openmeter/taxcode/service/organizationdefaulttaxcodes.go
- openmeter/taxcode/service/hooks/planhook.go
2 participants
@coderabbitai @greptileai review this draft
Summary by CodeRabbit