feat(billing): dual-write tax code IDs and snapshot TaxCode entities into invoice lines#4034
Merged
borbelyr-kong merged 11 commits intomainfrom Apr 1, 2026
Merged
Conversation
…into invoice lines
borbelyr-kong
added
the
release-note/ignore
Contributor
|
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 (6)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughWires a TaxCodeService into billing, adds tax-code resolution and stamping across profiles, customer overrides, invoices, and invoice calculation; updates adapters to eager-load TaxCode edges; extends productcatalog tax model/tests for TaxCode snapshots; refines API goverter ignore comments for tax fields. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Billing as Billing Service
participant TaxSvc as TaxCode Service
participant Adapter as Billing Adapter
participant DB as Database
participant Calc as Invoice Calculator
Client->>Billing: CreateProfile(with DefaultTaxConfig)
Billing->>TaxSvc: resolveDefaultTaxCode(Stripe code)
TaxSvc->>DB: GetOrCreateByAppMapping(Stripe code)
DB-->>TaxSvc: TaxCode entity
TaxSvc-->>Billing: TaxCode (ID)
Billing->>Adapter: CreateProfile(with TaxCodeID)
Adapter->>DB: Persist profile + TaxCodeID
DB-->>Adapter: OK
Client->>Billing: CreateInvoice(lines)
Billing->>Billing: resolveTaxCodes(scan invoice & lines)
Billing->>TaxSvc: GetOrCreateByAppMapping / GetTaxCodeByAppMapping
TaxSvc->>DB: Fetch/Ensure TaxCode entities
DB-->>TaxSvc: Map of TaxCode entities
Billing->>Calc: Calculate(invoice, deps{TaxCodes})
Calc->>Calc: SnapshotTaxConfigIntoLines (stamp TaxCodeID & TaxCode)
Calc-->>Billing: Invoice with stamped lines
Billing->>Adapter: CreateInvoice(stamped)
Adapter->>DB: Persist invoice + stamped tax refs
DB-->>Adapter: OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openmeter/productcatalog/tax.go (1)
46-72:⚠️ Potential issue | 🟠 Major
TaxCodeis missing fromTaxConfig.Equal().The new snapshot field never participates in equality, but invoice-line derived equality in
openmeter/billing/derived.gen.gostill routes through this method. A renamed or remapped stamped tax code would currently look unchanged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/tax.go` around lines 46 - 72, The Equal method for TaxConfig omits the TaxCode field, so update TaxConfig.Equal to include nil-checks and value comparison for the TaxCode pointer (mirroring the pattern used for Behavior and TaxCodeID): check for one-side nil, return false if so, then compare the dereferenced values and return false if they differ, before finally returning c.Stripe.Equal(v.Stripe); target the TaxConfig.Equal function and the TaxCode field when making this change.
🧹 Nitpick comments (1)
openmeter/billing/service/taxcode.go (1)
35-46: Potential N+1 database pattern worth noting.Each unique Stripe code triggers a separate
GetOrCreateByAppMappingcall (line 37), which per context snippet 1 involves a DB lookup (and potentially an insert). For most invoices with a handful of distinct tax codes, this is probably fine. But if an invoice has many unique codes, this becomes N sequential DB round-trips.If this ever becomes a bottleneck, you could add a batch lookup method to the tax code service. For now, given typical invoice shapes, this is likely acceptable — just flagging it for future awareness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/service/taxcode.go` around lines 35 - 46, The loop in taxcode.go that calls s.taxCodeService.GetOrCreateByAppMapping for each stripeCode can produce N+1 DB calls; replace this by adding a batch method on the tax code service (e.g., TaxCodeService.GetOrCreateByAppMappingBatch or GetByAppMappings) that accepts a slice of stripe codes/inputs and returns a map[string]invoicecalc.TaxCode (or list with keys), then change the code that builds result (the current make(invoicecalc.TaxCodes, len(seen)) section and the loop) to call that batch method once and populate result from the returned map; ensure the batch implementation performs a single lookup for existing mappings and a single bulk insert/upsert for missing mappings to avoid sequential DB round-trips.
🤖 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/billing/adapter/profile.go`:
- Around line 109-113: After saving the profile (the Save(ctx) result from the
cmd built with SetNillableTaxCodeID and DefaultTaxConfig), re-load the saved
entity with the tax-code edge so the returned object has Edges.TaxCode
populated; specifically, after cmd.Save(ctx) perform a follow-up query using
WithTaxCode() (or hydrate the edge) and return that re-queried object instead of
the raw Save() result. Apply the same pattern in CreateProfile, UpdateProfile,
and CreateInvoice so DefaultTaxConfig.TaxCode is available immediately after
write operations.
In `@openmeter/billing/service/customeroverride.go`:
- Around line 28-30: The call to resolveDefaultTaxCode (which calls
GetOrCreateByAppMapping) performs a write outside the upsert transaction, so
move tax-code resolution into the same DB transaction: either invoke
resolveDefaultTaxCode from inside transaction.Run where the override upsert is
performed or change taxCodeService/GetOrCreateByAppMapping to accept the
transaction-bound context/tx so it joins the same transaction; alternatively, if
you cannot join the tx, perform compensating cleanup (delete the created tax
code) on upsert failure. Ensure you update references to resolveDefaultTaxCode,
transaction.Run, taxCodeService, and GetOrCreateByAppMapping accordingly so the
tax-code creation participates in the same transaction as the customer override
upsert.
In `@openmeter/billing/service/invoice.go`:
- Around line 243-252: resolveTaxCodes currently calls GetOrCreateByAppMapping
(in taxcode.go) which performs writes during preview/simulation flows; change
resolveTaxCodes to use a read-only lookup (e.g., GetTaxCodeByAppMapping) so it
does not create records, or otherwise detect a preview/simulate context and
avoid calling GetOrCreateByAppMapping; update callers such as
calculateGatheringInvoiceAsStandardInvoice and SimulateInvoice to handle missing
tax codes gracefully (returning an error or using a default/null tax code) and
ensure tax-code creation is only performed in real write flows.
In `@openmeter/billing/service/profile.go`:
- Around line 634-646: The helper resolveDefaultTaxCode currently calls
taxCodeService.GetOrCreateByAppMapping even when taxConfig.Stripe.Code is an
empty string; update resolveDefaultTaxCode to treat an empty Stripe.Code as
"no-op" by returning nil before calling s.taxCodeService.GetOrCreateByAppMapping
so it skips resolution for empty codes (check taxConfig.Stripe.Code and return
nil), leaving existing behavior for non-empty codes and existing TaxCodeID
unchanged; refer to the resolveDefaultTaxCode function and
s.taxCodeService.GetOrCreateByAppMapping / taxConfig.Stripe.Code symbols when
making the change.
---
Outside diff comments:
In `@openmeter/productcatalog/tax.go`:
- Around line 46-72: The Equal method for TaxConfig omits the TaxCode field, so
update TaxConfig.Equal to include nil-checks and value comparison for the
TaxCode pointer (mirroring the pattern used for Behavior and TaxCodeID): check
for one-side nil, return false if so, then compare the dereferenced values and
return false if they differ, before finally returning c.Stripe.Equal(v.Stripe);
target the TaxConfig.Equal function and the TaxCode field when making this
change.
---
Nitpick comments:
In `@openmeter/billing/service/taxcode.go`:
- Around line 35-46: The loop in taxcode.go that calls
s.taxCodeService.GetOrCreateByAppMapping for each stripeCode can produce N+1 DB
calls; replace this by adding a batch method on the tax code service (e.g.,
TaxCodeService.GetOrCreateByAppMappingBatch or GetByAppMappings) that accepts a
slice of stripe codes/inputs and returns a map[string]invoicecalc.TaxCode (or
list with keys), then change the code that builds result (the current
make(invoicecalc.TaxCodes, len(seen)) section and the loop) to call that batch
method once and populate result from the returned map; ensure the batch
implementation performs a single lookup for existing mappings and a single bulk
insert/upsert for missing mappings to avoid sequential DB round-trips.
🪄 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: fbdae860-69d9-4aae-87ad-5b66aa30488f
📒 Files selected for processing (20)
api/v3/handlers/billingprofiles/convert.goapp/common/billing.gocmd/billing-worker/wire_gen.gocmd/jobs/internal/wire_gen.gocmd/server/wire_gen.goopenmeter/billing/adapter/customeroverride.goopenmeter/billing/adapter/invoice.goopenmeter/billing/adapter/invoicelinesplitgroup.goopenmeter/billing/adapter/profile.goopenmeter/billing/adapter/stdinvoicelines.goopenmeter/billing/service/customeroverride.goopenmeter/billing/service/gatheringinvoicependinglines.goopenmeter/billing/service/invoice.goopenmeter/billing/service/invoicecalc/calculator.goopenmeter/billing/service/invoicecalc/taxconfig.goopenmeter/billing/service/profile.goopenmeter/billing/service/service.goopenmeter/billing/service/stdinvoicestate.goopenmeter/billing/service/taxcode.goopenmeter/productcatalog/tax.go
turip
reviewed
Mar 30, 2026
|
|
||
| if cfg := input.Invoicing.DefaultTaxConfig; cfg != nil { | ||
| if cfg.TaxCodeID != nil { | ||
| update = update.SetNillableTaxCodeID(cfg.TaxCodeID) |
Member
There was a problem hiding this comment.
Suggested change
| update = update.SetNillableTaxCodeID(cfg.TaxCodeID) | |
| update = update.SetOrClearTaxCodeID(cfg.TaxCodeID) |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
openmeter/billing/adapter/profile.go (1)
109-113:⚠️ Potential issue | 🟡 MinorHydrate
TaxCodeon write responses too.Small follow-up: both helpers still return the raw
Save()result, soCreateProfileandUpdateProfilemap a workflow config with no loadedTaxCodeedge. That means the new backfill branch is skipped on the object returned right after a write, and clients can see different tax fields until the next read. Re-query withWithTaxCode()before attaching the workflow config to the response.Also applies to: 381-401
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/adapter/profile.go` around lines 109 - 113, The Save() result is returned without the TaxCode edge hydrated, causing CreateProfile/UpdateProfile to skip the backfill branch; after calling cmd.Save(ctx) re-query the saved entity with WithTaxCode() (e.g., use the profile's ID to fetch via Query().WithTaxCode() or use the builder's equivalent) and attach that fully loaded entity (with TaxCode) before mapping/attaching the workflow config; apply the same change in the other copy of this block around the 381-401 region.
🤖 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/billing/service/profile.go`:
- Around line 85-87: The demotion path still writes legacy default profiles via
demoteDefaultProfile (which calls s.adapter.UpdateProfile) without resolving
DefaultTaxConfig into TaxCodeID, so legacy DefaultTaxConfig.Stripe.Code can
remain; update demoteDefaultProfile (and the other write paths referenced around
the create/update flows) to call s.resolveDefaultTaxCode(ctx, input.Namespace,
profile.WorkflowConfig.Invoicing.DefaultTaxConfig) before calling
s.adapter.UpdateProfile (or otherwise ensure resolveDefaultTaxCode is invoked on
the profile being written) so any Stripe.Code is translated into TaxCodeID prior
to persisting the demoted/updated profile.
---
Duplicate comments:
In `@openmeter/billing/adapter/profile.go`:
- Around line 109-113: The Save() result is returned without the TaxCode edge
hydrated, causing CreateProfile/UpdateProfile to skip the backfill branch; after
calling cmd.Save(ctx) re-query the saved entity with WithTaxCode() (e.g., use
the profile's ID to fetch via Query().WithTaxCode() or use the builder's
equivalent) and attach that fully loaded entity (with TaxCode) before
mapping/attaching the workflow config; apply the same change in the other copy
of this block around the 381-401 region.
🪄 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: 4e36e4be-aa34-44d2-9399-3bdf4f80f3a9
📒 Files selected for processing (7)
openmeter/billing/adapter/customeroverride.goopenmeter/billing/adapter/profile.goopenmeter/billing/service/customeroverride.goopenmeter/billing/service/invoice.goopenmeter/billing/service/profile.goopenmeter/billing/service/stdinvoicestate.goopenmeter/billing/service/taxcode.go
✅ Files skipped from review due to trivial changes (2)
- openmeter/billing/service/invoice.go
- openmeter/billing/adapter/customeroverride.go
🚧 Files skipped from review as they are similar to previous changes (2)
- openmeter/billing/service/customeroverride.go
- openmeter/billing/service/taxcode.go
Comment on lines
+85
to
+87
| if err := s.resolveDefaultTaxCode(ctx, input.Namespace, input.WorkflowConfig.Invoicing.DefaultTaxConfig); err != nil { | ||
| return nil, err | ||
| } |
Contributor
There was a problem hiding this comment.
Cover the default-profile demotion write path as well.
Nice addition overall, but this only covers the explicit create/update flows. demoteDefaultProfile() still writes the previous default profile via s.adapter.UpdateProfile() on Lines 466-469 without running the resolver, so a legacy DefaultTaxConfig.Stripe.Code can stay without TaxCodeID even after that write. That leaves the new dual-write incomplete for the old profile, which may still stay in service for pinned customers.
💡 One small way to close the gap
defaultProfile.Default = false
+
+ if err := s.resolveDefaultTaxCode(ctx, ns, defaultProfile.WorkflowConfig.Invoicing.DefaultTaxConfig); err != nil {
+ return nil, err
+ }
updatedBaseProfile, err := s.adapter.UpdateProfile(ctx, billing.UpdateProfileAdapterInput{Also applies to: 287-294, 634-654
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/billing/service/profile.go` around lines 85 - 87, The demotion path
still writes legacy default profiles via demoteDefaultProfile (which calls
s.adapter.UpdateProfile) without resolving DefaultTaxConfig into TaxCodeID, so
legacy DefaultTaxConfig.Stripe.Code can remain; update demoteDefaultProfile (and
the other write paths referenced around the create/update flows) to call
s.resolveDefaultTaxCode(ctx, input.Namespace,
profile.WorkflowConfig.Invoicing.DefaultTaxConfig) before calling
s.adapter.UpdateProfile (or otherwise ensure resolveDefaultTaxCode is invoked on
the profile being written) so any Stripe.Code is translated into TaxCodeID prior
to persisting the demoted/updated profile.
Contributor
Author
There was a problem hiding this comment.
The finding is invalid.
demoteDefaultProfile calls GetDefaultProfile → uses WithWorkflowConfig(workflowConfigWithTaxCode) which eager-loads the TaxCode edge → mapWorkflowConfigFromDB calls BackfillTaxConfig, which sets DefaultTaxConfig.TaxCodeID = &tc.ID from the DB row.
Contributor
There was a problem hiding this comment.
You're right, my apologies for the noise! The read path already handles it:
GetDefaultProfile → WithWorkflowConfig(workflowConfigWithTaxCode) eager-loads the TaxCode edge → mapWorkflowConfigFromDB calls BackfillTaxConfig which stamps TaxCodeID from the DB row. Since resolveDefaultTaxCode is a no-op when TaxCodeID is already set, calling it in demoteDefaultProfile would be redundant.
✏️ Learnings added
Learnt from: borbelyr-kong
Repo: openmeterio/openmeter PR: 4034
File: openmeter/billing/service/profile.go:85-87
Timestamp: 2026-03-31T11:55:01.002Z
Learning: In `openmeter/billing/service/profile.go`, the `demoteDefaultProfile` function does NOT need an explicit call to `resolveDefaultTaxCode` before writing. When it calls `GetDefaultProfile`, the adapter uses `WithWorkflowConfig(workflowConfigWithTaxCode)` which eager-loads the `TaxCode` edge; `mapWorkflowConfigFromDB` then calls `BackfillTaxConfig` to stamp `DefaultTaxConfig.TaxCodeID` from the DB row. Since `resolveDefaultTaxCode` is idempotent (no-op when `TaxCodeID != nil`), `TaxCodeID` is already populated by the time `adapter.UpdateProfile` is called. Do not flag the absence of `resolveDefaultTaxCode` in `demoteDefaultProfile` as a missing dual-write guard.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: turip
Repo: openmeterio/openmeter PR: 4024
File: openmeter/billing/worker/subscriptionsync/service/reconciler/patch.go:108-124
Timestamp: 2026-03-30T08:14:27.578Z
Learning: In `openmeter/billing/worker/subscriptionsync/service/reconciler/patch.go`, `ResolveDefaultCollection` intentionally routes all non-flat credit-only prices (i.e., anything that is not `productcatalog.FlatPriceType`) to `usageBasedChargeCollection` via the default branch. This flat-vs-everything-else split is a deliberate, consistent convention across the codebase. Do not flag the absence of an explicit unit-price case or the use of a catch-all default as a missing fail-fast guard.
Learnt from: turip
Repo: openmeterio/openmeter PR: 4023
File: openmeter/billing/worker/subscriptionsync/service/reconciler/patchinvoicelinehierarchy.go:112-127
Timestamp: 2026-03-28T18:44:25.086Z
Learning: In `openmeter/billing/worker/subscriptionsync/service/reconciler/patchinvoicelinehierarchy.go`, the `AddShrink` and `AddExtend` methods do NOT need to clear `DeletedAt` on the split line group (`updatedGroup.DeletedAt = nil`) when reviving managed child lines. Whole group delete/restore is not a scenario that occurs in this system, so this is intentional and not a missing guard. Do not flag the absence of group-level `DeletedAt` revival alongside managed child reactivation in this file.
Learnt from: turip
Repo: openmeterio/openmeter PR: 4023
File: openmeter/billing/worker/subscriptionsync/service/reconciler/patch.go:71-79
Timestamp: 2026-03-28T18:24:57.361Z
Learning: In `openmeter/billing/worker/subscriptionsync/service/reconciler/patch.go`, the `patchCollectionRouter.GetCollectionFor` method intentionally returns `nil` for the default branch of the `item.Type()` switch. All `billing.LineOrHierarchyType` values (`LineOrHierarchyTypeLine` and `LineOrHierarchyTypeHierarchy`) are explicitly handled; the default branch is considered unreachable and the nil/panic behavior on an unknown type is an accepted, deliberate design decision backed by extensive testing. Do not flag this as a missing explicit error path.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
openmeter/productcatalog/tax_test.go (2)
537-542: Consider addingt.Parallel()for subtests.For consistency and faster test runs as the suite grows, you might want to add parallel execution:
for _, tt := range tests { + tt := tt // capture range variable t.Run(tt.name, func(t *testing.T) { + t.Parallel() got := BackfillTaxConfig(tt.cfg, tt.taxBehavior, tt.tc) assert.Equal(t, tt.want, got) }) }Same could apply to other table-driven tests in this file. Totally optional though—the tests are quick as-is.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/tax_test.go` around lines 537 - 542, Add parallel execution to the table-driven subtests by calling t.Parallel() as the first statement inside the t.Run closure (the anonymous func in the loop that invokes BackfillTaxConfig); ensure any shared mutable state used by the test loop is properly isolated (e.g., capture loop variable into a local var) before calling t.Parallel() so BackfillTaxConfig tests run safely in parallel.
431-543: Comprehensive test coverage for BackfillTaxConfig!The table-driven approach is clean and the cases align well with the implementation contract:
- Nil handling
- No-op when TaxCode lacks Stripe mapping
- Field backfilling for empty configs
- "Don't overwrite" behavior for existing fields
One optional edge case you could consider adding:
🔧 Optional: Test TaxCode with empty ID
The implementation guards with
tc.ID != ""before settingTaxCodeID. You could add a case to verify this edge case is handled:+ { + name: "tc with empty ID does not set TaxCodeID", + cfg: nil, + taxBehavior: nil, + tc: &taxcode.TaxCode{ + NamespacedID: models.NamespacedID{Namespace: "ns", ID: ""}, + AppMappings: taxcode.TaxCodeAppMappings{ + {AppType: app.AppTypeStripe, TaxCode: "txcd_10000000"}, + }, + }, + want: &TaxConfig{ + Stripe: &StripeTaxConfig{Code: "txcd_10000000"}, + }, + },This is a nit since empty IDs are unlikely in practice, but it documents the behavior explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/productcatalog/tax_test.go` around lines 431 - 543, Add a table-driven test case to TestBackfillTaxConfig that verifies a taxcode.TaxCode with an empty ID does not set TaxCodeID: call BackfillTaxConfig with tc := newTaxCode("", "txcd_10000000") (or construct a taxcode.TaxCode with NamespacedID.ID == "") and assert the resulting *TaxConfig has Stripe populated but TaxCodeID remains nil; reference the existing test harness and helper newTaxCode, and place the case alongside the other test entries to document the guarded behavior.test/billing/tax_test.go (1)
280-282: Consider addingTaxCodeIDandTaxCodeassertions for detailed lines.The parent split line gets full tax code verification (ID + entity snapshot), but the detailed line only checks
BehaviorandStripe. If detailed lines should also have the stampedTaxCodeIDandTaxCodeentity (per the calculator flow in context snippet 1), you might want to add those assertions here for completeness.That said, if detailed lines intentionally don't get the entity snapshot, this is fine as-is!
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/billing/tax_test.go` around lines 280 - 282, The test currently asserts only TaxConfig.Behavior and TaxConfig.Stripe on ubpDetailedLine but omits verifying that detailed lines also received the stamped TaxCodeID and TaxCode entity; update the assertions in this test to include checks that ubpDetailedLine.TaxConfig.TaxCodeID equals the expected taxConfig.TaxCodeID and that ubpDetailedLine.TaxConfig.TaxCode (or the TaxCode snapshot field used in your structs) is non-nil and matches the expected TaxCode values, so add assertions referencing ubpDetailedLine.TaxConfig.TaxCodeID and ubpDetailedLine.TaxConfig.TaxCode (and compare against taxConfig.TaxCodeID / taxConfig.TaxCode) to mirror the parent split line coverage.openmeter/billing/service/invoicecalc/taxconfig_test.go (1)
194-208: Standard test execution pattern.The loop handles both success and error cases cleanly. Since all current test cases expect success (
wantNoErr: true), you might consider simplifying by removing the error case branch if errors aren't expected from this function. But keeping it is fine for future-proofing if error cases are added later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/service/invoicecalc/taxconfig_test.go` around lines 194 - 208, The test loop currently branches on tt.wantNoErr after calling SnapshotTaxConfigIntoLines; since all existing cases set wantNoErr=true, simplify by removing the else branch and always assert no error (require.NoError(t, err)), then proceed to validate lines via invoice.Lines.OrEmpty and compare TaxConfig; update the loop body around SnapshotTaxConfigIntoLines to remove the require.Error/return branch tied to tt.wantNoErr (or alternatively keep the branching only if you add negative cases later).
🤖 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/billing/service/invoicecalc/taxconfig_test.go`:
- Around line 194-208: The test loop currently branches on tt.wantNoErr after
calling SnapshotTaxConfigIntoLines; since all existing cases set wantNoErr=true,
simplify by removing the else branch and always assert no error
(require.NoError(t, err)), then proceed to validate lines via
invoice.Lines.OrEmpty and compare TaxConfig; update the loop body around
SnapshotTaxConfigIntoLines to remove the require.Error/return branch tied to
tt.wantNoErr (or alternatively keep the branching only if you add negative cases
later).
In `@openmeter/productcatalog/tax_test.go`:
- Around line 537-542: Add parallel execution to the table-driven subtests by
calling t.Parallel() as the first statement inside the t.Run closure (the
anonymous func in the loop that invokes BackfillTaxConfig); ensure any shared
mutable state used by the test loop is properly isolated (e.g., capture loop
variable into a local var) before calling t.Parallel() so BackfillTaxConfig
tests run safely in parallel.
- Around line 431-543: Add a table-driven test case to TestBackfillTaxConfig
that verifies a taxcode.TaxCode with an empty ID does not set TaxCodeID: call
BackfillTaxConfig with tc := newTaxCode("", "txcd_10000000") (or construct a
taxcode.TaxCode with NamespacedID.ID == "") and assert the resulting *TaxConfig
has Stripe populated but TaxCodeID remains nil; reference the existing test
harness and helper newTaxCode, and place the case alongside the other test
entries to document the guarded behavior.
In `@test/billing/tax_test.go`:
- Around line 280-282: The test currently asserts only TaxConfig.Behavior and
TaxConfig.Stripe on ubpDetailedLine but omits verifying that detailed lines also
received the stamped TaxCodeID and TaxCode entity; update the assertions in this
test to include checks that ubpDetailedLine.TaxConfig.TaxCodeID equals the
expected taxConfig.TaxCodeID and that ubpDetailedLine.TaxConfig.TaxCode (or the
TaxCode snapshot field used in your structs) is non-nil and matches the expected
TaxCode values, so add assertions referencing
ubpDetailedLine.TaxConfig.TaxCodeID and ubpDetailedLine.TaxConfig.TaxCode (and
compare against taxConfig.TaxCodeID / taxConfig.TaxCode) to mirror the parent
split line coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cb57fd1a-bebb-4c2b-8a9e-149f6565e96e
📒 Files selected for processing (8)
openmeter/billing/service/invoicecalc/taxconfig_test.goopenmeter/productcatalog/tax_test.gotest/app/testenv.gotest/billing/suite.gotest/billing/tax_test.gotest/billing/taxcode_dual_write_test.gotest/customer/testenv.gotest/subscription/framework_test.go
turip
reviewed
Mar 31, 2026
| out.QuantitySnapshotedAt = lo.ToPtr(now) | ||
| out.Lines = billing.NewStandardInvoiceLines(inScopeLines) | ||
|
|
||
| taxCodes, err := s.resolveTaxCodes(ctx, invoice.Namespace, out, true) |
Member
There was a problem hiding this comment.
Suggested change
| taxCodes, err := s.resolveTaxCodes(ctx, invoice.Namespace, out, true) | |
| taxCodes, err := s.resolveTaxCodes(ctx, resolveTaxCodesInput{ | |
| Namespace: invoice.namespace, | |
| Invoice: out, | |
| ReadOnly: true, | |
| }) |
Please don't use naked bools as function arguments as it's hard to read. Instead use this input structure.
Or if you want you can use the following format:
taxCodes, err := s.resolveTaxCodex(ctx, namespace, out, withReadOnlyTaxCode())
Where:
type Options struct {
ReadOnly bool
}
type resolveOption() func(*Options)
func withReadonlyTaxCode() resolveOption {
return func(o *Options) {
o.ReadOnly = true
}
}
func (s *Service) resolveTaxCodes(ctx context.Context, namespace string, invoice *billing.StandardInvoice, opts ...resolveOptions) (invoicecalc.TaxCodes, error) {
options := Options{}
for _, opt := range opts {
opt(&options)
}
I would use the first as it's easier to do.
| // resolveDefaultTaxCode resolves the TaxCode entity for a DefaultTaxConfig's Stripe.Code and | ||
| // stamps TaxCodeID back onto the pointed-to config before it is persisted. Idempotent: no-op | ||
| // when taxConfig is nil, TaxCodeID is already set, or there is no Stripe code. | ||
| func (s *Service) resolveDefaultTaxCode(ctx context.Context, namespace string, taxConfig *productcatalog.TaxConfig) error { |
Member
There was a problem hiding this comment.
changing a billing profile or customer override from one Stripe tax code to a different one can leave the old TaxCodeID FK in place while persisting the new Stripe code.
openmeter/billing/service/profile.go:637 returns early whenever TaxCodeID is already set, and both update paths call that helper on caller-supplied config before persisting it:
openmeter/billing/service/profile.go:287 and openmeter/billing/
service/customeroverride.go:28.
That is a realistic read-modify-write flow, because the service now returns TaxCodeID on reads.
If a caller fetches a profile/override, changes only Stripe.Code, and sends it back, the stale FK is retained. The inconsistency then survives reads because BackfillTaxConfig only fills missing fields, and invoice snapshotting will even preserve the stale TaxCodeID while stamping a TaxCode entity for the new code openmeter/billing/service/invoicecalc/taxconfig.go:25.
The new tests cover clearing the code and idempotent same-code updates, but not changing txcd_A -> txcd_B on an already-stamped config test/billing/taxcode_dual_write_test.go:119.
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 `@openmeter/billing/service/profile.go`:
- Around line 639-654: The resolver currently returns early when taxConfig or
taxConfig.Stripe or taxConfig.Stripe.Code is empty, leaving a stale TaxCodeID;
update resolveDefaultTaxCode to explicitly clear taxConfig.TaxCodeID (set to
nil) when Stripe is nil or Stripe.Code is empty, then continue (or return) so
that removing the Stripe code removes the FK; keep the existing logic that sets
TaxCodeID when a code exists (the block using
s.taxCodeService.GetOrCreateByAppMapping and lo.ToPtr(tc.ID)).
In `@test/billing/taxcode_dual_write_test.go`:
- Around line 119-151: The test TestProfileUpdateClearsTaxCodeFK only replaces
DefaultTaxConfig with a fresh struct; add a round-trip clear case that mimics a
real read-modify-write: after provisioning the profile (where
WorkflowConfig.Invoicing.DefaultTaxConfig.Stripe.Code and TaxCodeID are
populated), fetch the profile via BillingService.GetDefaultProfile (or
equivalent), modify only the Stripe field (set Stripe to nil or clear
Stripe.Code) on the fetched profile.BaseProfile, then call
BillingService.UpdateProfile with that BaseProfile and assert that
SetOrClearTaxCodeID logic clears cfg.TaxCodeID and leaves cfg.Stripe nil; also
add the same round-trip variant for the other clear-path test near
TestProfileUpdateStripeCodeChangeUpdatesFK (lines ~241-286) to cover the branch
where the FK was already populated and only Stripe is removed.
🪄 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: 4e46aa7b-33a5-4f20-87b5-244a5644efbc
📒 Files selected for processing (5)
openmeter/billing/service/invoice.goopenmeter/billing/service/profile.goopenmeter/billing/service/stdinvoicestate.goopenmeter/billing/service/taxcode.gotest/billing/taxcode_dual_write_test.go
✅ Files skipped from review due to trivial changes (1)
- openmeter/billing/service/taxcode.go
🚧 Files skipped from review as they are similar to previous changes (1)
- openmeter/billing/service/invoice.go
turip
approved these changes
Apr 1, 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
Bug Fixes