refactor: totals mixins v2#3940
Conversation
This patch moves the totals mixin to a seperate package while allowing adapters to reuse the set/map functions.
📝 WalkthroughWalkthroughThis PR refactors the Totals type out of the billing package into a dedicated Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 unit tests (beta)
📝 Coding Plan
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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openmeter/billing/service/lineservice/detailedlines.go (1)
202-231: Variable shadows package name - consider renaming for clarity.The local variable
totalson line 210 shadows the importedtotalspackage. While this works correctly (all subsequent uses liketotals.CalculateTotal()refer to the local struct's method, not a package function), it can be confusing to readers.🔧 Suggested rename for clarity
-func calculateDetailedLineTotals(line billing.DetailedLine) (totals.Totals, error) { +func calculateDetailedLineTotals(line billing.DetailedLine) (totals.Totals, error) { // Calculate the line totals calc, err := line.Currency.Calculator() if err != nil { return totals.Totals{}, err } // Calculate the line totals - totals := totals.Totals{ + result := totals.Totals{ DiscountsTotal: line.AmountDiscounts.SumAmount(calc), CreditsTotal: line.CreditsApplied.SumAmount(calc), // TODO[OM-979]: implement taxes TaxesInclusiveTotal: alpacadecimal.Zero, TaxesExclusiveTotal: alpacadecimal.Zero, TaxesTotal: alpacadecimal.Zero, } amount := calc.RoundToPrecision(line.PerUnitAmount.Mul(line.Quantity)) switch line.Category { case billing.FlatFeeCategoryCommitment: - totals.ChargesTotal = amount + result.ChargesTotal = amount default: - totals.Amount = amount + result.Amount = amount } - totals.Total = totals.CalculateTotal() + result.Total = result.CalculateTotal() - return totals, nil + return result, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/service/lineservice/detailedlines.go` around lines 202 - 231, In calculateDetailedLineTotals, the local variable named totals shadows the imported totals package; rename the local variable (e.g., lineTotals or totalsStruct) and update all references inside calculateDetailedLineTotals (fields like DiscountsTotal, CreditsTotal, ChargesTotal/Amount/Total and the call to CalculateTotal()) to use the new name so there is no confusion between the package and the local struct.
🤖 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/lineservice/detailedlines.go`:
- Around line 202-231: In calculateDetailedLineTotals, the local variable named
totals shadows the imported totals package; rename the local variable (e.g.,
lineTotals or totalsStruct) and update all references inside
calculateDetailedLineTotals (fields like DiscountsTotal, CreditsTotal,
ChargesTotal/Amount/Total and the call to CalculateTotal()) to use the new name
so there is no confusion between the package and the local struct.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b660ba5f-1451-44b1-858c-397df6217745
⛔ Files ignored due to path filters (1)
openmeter/ent/db/entmixinaccessor.gois excluded by!**/ent/db/**
📒 Files selected for processing (25)
openmeter/billing/adapter/invoice.goopenmeter/billing/adapter/stdinvoicelinemapper.goopenmeter/billing/adapter/stdinvoicelines.goopenmeter/billing/charges/adapter/mapper.goopenmeter/billing/charges/adapter/stdinvoice.goopenmeter/billing/charges/chargeflatfee.goopenmeter/billing/charges/stdinvoice.goopenmeter/billing/derived.gen.goopenmeter/billing/httpdriver/invoice.goopenmeter/billing/invoicedetailedline.goopenmeter/billing/models/totals/mixin.goopenmeter/billing/models/totals/model.goopenmeter/billing/service/invoicecalc/details.goopenmeter/billing/service/lineservice/detailedlines.goopenmeter/billing/service/lineservice/linebase.goopenmeter/billing/service/lineservice/service.goopenmeter/billing/service/lineservice/usagebasedline_test.goopenmeter/billing/stdinvoice.goopenmeter/billing/stdinvoiceline.goopenmeter/ent/entc.goopenmeter/ent/schema/billing.goopenmeter/ent/schema/charges.gopkg/framework/entutils/entmixinaccessor/entmixinaccessor.gopkg/framework/entutils/entmixinaccessor/mixinaccessor.tpltest/billing/invoice_test.go
Overview
This patch moves the totals mixin to a seperate package while allowing adapters to reuse the set/map functions.
The motivation is twofold:
Notes for reviewer
Summary by CodeRabbit
Release Notes