chore: add AllowAnnotations bypass

[borbelyr-kong]

Summary by CodeRabbit

• New Features
  • System-managed tax codes can now be updated and deleted when annotations are enabled, providing greater flexibility in managing tax code configurations.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR introduces an AllowAnnotations bypass flag that lets update and delete operations proceed on system-managed tax codes, which previously always returned a conflict. A Goverter directive prevents the flag from being included in API conversion logic.

Changes

Cohort / File(s)	Summary
API Conversion Handling api/v3/handlers/taxcodes/convert.go	Added Goverter directive to ignore inputOptions during conversion generation for FromAPIUpsertTaxCodeRequest.
Input Type Extensions openmeter/taxcode/service.go	Introduced inputOptions struct with AllowAnnotations bool field and embedded it into both UpdateTaxCodeInput and DeleteTaxCodeInput.
Service Logic openmeter/taxcode/service/taxcode.go	Modified UpdateTaxCode and DeleteTaxCode to permit operations on system-managed entities when AllowAnnotations is true; also refactored DeleteTaxCode lookup to use explicit GetTaxCodeInput construction.
Test Coverage openmeter/taxcode/service/taxcode_test.go	Added UpdateIsByPassed and DeleteIsByPassed test cases to verify bypass behavior with AllowAnnotations = true.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

• feat(taxcode): add annotations #4061: Introduces the core annotations feature that this PR builds upon with the AllowAnnotations bypass mechanism.

Suggested reviewers

• chrisgacsal

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: adding an AllowAnnotations bypass mechanism for tax codes across multiple files.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch chore/tax-code-annotation-bypass

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (2)

openmeter/taxcode/service.go (1)

22-24: Consider documenting AllowAnnotations intent inline

Tiny doc comments here would make the internal-only bypass intent clearer and help prevent accidental broad use later.

💡 Suggested tweak

 type inputOptions struct {
+	// AllowAnnotations bypasses system-managed guards for trusted/internal callers.
+	// Public API converters intentionally do not map this option.
 	AllowAnnotations bool
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@openmeter/taxcode/service.go` around lines 22 - 24, Add a short doc comment
above the inputOptions struct and its AllowAnnotations field explaining its
intent as an internal-only bypass (e.g., to permit non-public annotation
handling or testing), who may set it, and warnings against broad external use;
update the comments for inputOptions and AllowAnnotations so future readers know
it is not part of the public API and when it is safe to enable.

openmeter/taxcode/service/taxcode_test.go (1)

73-80: Consider asserting post-delete state too

Small improvement: after successful delete, fetch the tax code and assert not found. That makes this test resilient against future no-op delete behavior.

✅ Possible test strengthening

 		t.Run("DeleteIsByPassed", func(t *testing.T) {
 			input := taxcode.DeleteTaxCodeInput{
 				NamespacedID: models.NamespacedID{Namespace: ns, ID: tc.ID},
 			}
 			input.AllowAnnotations = true
 			err := env.Service.DeleteTaxCode(t.Context(), input)
 			require.NoError(t, err)
+
+			_, err = env.Service.GetTaxCode(t.Context(), taxcode.GetTaxCodeInput{
+				NamespacedID: models.NamespacedID{Namespace: ns, ID: tc.ID},
+			})
+			require.Error(t, err)
+			assert.True(t, taxcode.IsTaxCodeNotFoundError(err))
 		})

As per coding guidelines: **/*_test.go: Make sure the tests are comprehensive and cover the changes.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@openmeter/taxcode/service/taxcode_test.go` around lines 73 - 80, The test
case "DeleteIsByPassed" currently only calls env.Service.DeleteTaxCode with a
DeleteTaxCodeInput (setting AllowAnnotations) and asserts no error; enhance it
by attempting to retrieve the deleted tax code afterwards and asserting it's not
found: call the service read/get method (e.g., env.Service.GetTaxCode or
equivalent read function that accepts models.NamespacedID or similar) using the
same NamespacedID (models.NamespacedID{Namespace: ns, ID: tc.ID}) and assert
that it returns a not-found error (or nil result and appropriate error) instead
of a successful object, updating assertions accordingly to ensure delete had
effect.

🤖 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/taxcode/service.go`:
- Around line 22-24: Add a short doc comment above the inputOptions struct and
its AllowAnnotations field explaining its intent as an internal-only bypass
(e.g., to permit non-public annotation handling or testing), who may set it, and
warnings against broad external use; update the comments for inputOptions and
AllowAnnotations so future readers know it is not part of the public API and
when it is safe to enable.

In `@openmeter/taxcode/service/taxcode_test.go`:
- Around line 73-80: The test case "DeleteIsByPassed" currently only calls
env.Service.DeleteTaxCode with a DeleteTaxCodeInput (setting AllowAnnotations)
and asserts no error; enhance it by attempting to retrieve the deleted tax code
afterwards and asserting it's not found: call the service read/get method (e.g.,
env.Service.GetTaxCode or equivalent read function that accepts
models.NamespacedID or similar) using the same NamespacedID
(models.NamespacedID{Namespace: ns, ID: tc.ID}) and assert that it returns a
not-found error (or nil result and appropriate error) instead of a successful
object, updating assertions accordingly to ensure delete had effect.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 76ea7455-1604-4fa8-b2f9-ef4063a5e73a

📥 Commits

Reviewing files that changed from the base of the PR and between 171cbc2 and 943bb28.

📒 Files selected for processing (4)

• api/v3/handlers/taxcodes/convert.go
• openmeter/taxcode/service.go
• openmeter/taxcode/service/taxcode.go
• openmeter/taxcode/service/taxcode_test.go