refactor: invoice pending lines#3880
Conversation
turip
changed the title
📝 WalkthroughWalkthroughRefactors the gathering invoice pending lines workflow by introducing two new public methods—PrepareBillableLines and CreateStandardInvoiceFromGatheringLines—with supporting types. Replaces prior per-currency direct handling with a two-step approach: prepare billable lines grouped by currency, then create standard invoices per currency. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
openmeter/billing/service/gatheringinvoicependinglines.go (2)
109-126: Duplicate validation and namespace lockdown checks.Lines 110-120 duplicate the exact same validation and
fsNamespaceLockdowncheck fromInvoicePendingLines(lines 48-58). When called viaInvoicePendingLines, these run twice. This is expected for a standalone public method, but wanted to call it out — if there's ever a hot-path concern, a privateprepareBillableLinesInternalwithout the redundant checks could help. Not urgent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/service/gatheringinvoicependinglines.go` around lines 109 - 126, Duplicate input validation and fsNamespaceLockdown checks occur in PrepareBillableLines and InvoicePendingLines causing redundant work; extract the core work into a private helper (e.g., prepareBillableLinesInternal) that accepts the already-validated input and performs the transactionForInvoiceManipulation callback logic, then have the public PrepareBillableLines perform the validation and lockdown checks and call the internal helper, and update InvoicePendingLines to call the internal helper directly (or to call the public method only if you intentionally want revalidation); reference PrepareBillableLines, InvoicePendingLines, transactionForInvoiceManipulation and the new prepareBillableLinesInternal to locate where to move the logic.
660-714: Consider passing pre-fetched profile and feature meters to reduce redundant DB calls in this hot path.When
CreateStandardInvoiceFromGatheringLinesis invoked fromInvoicePendingLineswith multiple currencies, bothGetCustomerOverrideandresolveFeatureMetersare called again even though they were already fetched inPrepareBillableLines. For N currencies, this meansGetCustomerOverridegets hit N+1 times andresolveFeatureMetersgets hit 2N times.This is a reasonable trade-off for keeping the method independently callable, but in this hot path you could accept optional profile and feature meters parameters. When present, skip the redundant fetches; when absent, fetch as usual. That way callers like
InvoicePendingLinescan pass them through and avoid the extra DB round-trips.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/service/gatheringinvoicependinglines.go` around lines 660 - 714, CreateStandardInvoiceFromGatheringLines currently re-fetches GetCustomerOverride and resolveFeatureMeters on a hot path causing N+1/2N DB calls; make them optional inputs so callers can supply pre-fetched data. Extend billing.CreateStandardInvoiceFromGatheringLinesInput to include optional fields (e.g., PreFetchedProfile *billing.CustomerOverride and PreFetchedFeatureMeters type matching resolveFeatureMeters output), then in CreateStandardInvoiceFromGatheringLines use in.PreFetchedProfile when non-nil instead of calling GetCustomerOverride and use in.PreFetchedFeatureMeters when present instead of calling resolveFeatureMeters; leave existing fetch logic as the fallback. Update hot-path callers (notably InvoicePendingLines and PrepareBillableLines flows) to pass through the already-fetched profile and featureMeters when available.openmeter/billing/service.go (1)
103-104: Minor inconsistency: pointer return type vs. value returns in same interface.The other
StandardInvoiceServicemethods (UpdateStandardInvoice,GetStandardInvoiceById,ListStandardInvoices) all returnStandardInvoiceby value, but this new method returns*StandardInvoice. This isn't a bug, but it makes the interface inconsistent and forces callers to handle the pointer differently.Was this intentional (e.g., to signal a nil result case)? Looking at the implementation in
gatheringinvoicependinglines.go, it always returns a non-nil result on success, so a value return would align better with the rest of the interface.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/service.go` around lines 103 - 104, The interface method CreateStandardInvoiceFromGatheringLines returns *StandardInvoice while other methods (UpdateStandardInvoice, GetStandardInvoiceById, ListStandardInvoices) return StandardInvoice by value; change the interface signature to return (StandardInvoice, error) and update the implementation in gatheringinvoicependinglines.go to match that value return (modify the function signature and ensure it returns a non-pointer StandardInvoice on success), and update any callers to accept a value instead of a pointer.
🤖 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.go`:
- Around line 103-104: The interface method
CreateStandardInvoiceFromGatheringLines returns *StandardInvoice while other
methods (UpdateStandardInvoice, GetStandardInvoiceById, ListStandardInvoices)
return StandardInvoice by value; change the interface signature to return
(StandardInvoice, error) and update the implementation in
gatheringinvoicependinglines.go to match that value return (modify the function
signature and ensure it returns a non-pointer StandardInvoice on success), and
update any callers to accept a value instead of a pointer.
In `@openmeter/billing/service/gatheringinvoicependinglines.go`:
- Around line 109-126: Duplicate input validation and fsNamespaceLockdown checks
occur in PrepareBillableLines and InvoicePendingLines causing redundant work;
extract the core work into a private helper (e.g., prepareBillableLinesInternal)
that accepts the already-validated input and performs the
transactionForInvoiceManipulation callback logic, then have the public
PrepareBillableLines perform the validation and lockdown checks and call the
internal helper, and update InvoicePendingLines to call the internal helper
directly (or to call the public method only if you intentionally want
revalidation); reference PrepareBillableLines, InvoicePendingLines,
transactionForInvoiceManipulation and the new prepareBillableLinesInternal to
locate where to move the logic.
- Around line 660-714: CreateStandardInvoiceFromGatheringLines currently
re-fetches GetCustomerOverride and resolveFeatureMeters on a hot path causing
N+1/2N DB calls; make them optional inputs so callers can supply pre-fetched
data. Extend billing.CreateStandardInvoiceFromGatheringLinesInput to include
optional fields (e.g., PreFetchedProfile *billing.CustomerOverride and
PreFetchedFeatureMeters type matching resolveFeatureMeters output), then in
CreateStandardInvoiceFromGatheringLines use in.PreFetchedProfile when non-nil
instead of calling GetCustomerOverride and use in.PreFetchedFeatureMeters when
present instead of calling resolveFeatureMeters; leave existing fetch logic as
the fallback. Update hot-path callers (notably InvoicePendingLines and
PrepareBillableLines flows) to pass through the already-fetched profile and
featureMeters when available.
turip
deleted the
refactor/allow-invoice-pending-lines-steps-from-other-services
branch
2 participants
Overview
This approach gives us more granular control about the invoice pending lines flow when executed in external services.
Summary by CodeRabbit
Refactor
Tests