feat: move legacy splitline handling to lineengine#4750
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| func (s *stubRatingService) GenerateProgressiveBilledDetailedLines(in billingrating.ProgressiveBilledLineAccessor, opts ...billingrating.GenerateDetailedLinesOption) (billingrating.GenerateDetailedLinesResult, error) { | ||
| return billingrating.GenerateDetailedLinesResult{}, fmt.Errorf("charges rating must always use the non-progressive billed rating engine") | ||
| } | ||
|
|
There was a problem hiding this comment.
GenerateDetailedLines now returns an empty result instead of s.result
Changing return s.result, nil to return billingrating.GenerateDetailedLinesResult{}, nil means test assertions that previously inspected the returned detailed lines from the charges path will now see an empty result. If any test in this package verifies output content from the stub (e.g., total amounts or generated line shapes), those assertions pass trivially with zero values rather than catching regressions. Double-check whether any test case in engine_test.go reads the result of GenerateDetailedLines calls.
Prompt To Fix With AI
This is a comment left during a code review.
Path: openmeter/billing/charges/usagebased/service/rating/delta/engine_test.go
Line: 344-348
Comment:
**Stub `GenerateDetailedLines` now returns an empty result instead of `s.result`**
Changing `return s.result, nil` to `return billingrating.GenerateDetailedLinesResult{}, nil` means test assertions that previously inspected the returned detailed lines from the charges path will now see an empty result. If any test in this package verifies output content from the stub (e.g., total amounts or generated line shapes), those assertions pass trivially with zero values rather than catching regressions. Double-check whether any test case in `engine_test.go` reads the result of `GenerateDetailedLines` calls.
How can I resolve this? If you propose a fix, please make it concise.
| func (h SplitLineHierarchy) Lines() []LineHeaderAccessor { | ||
| lines := lo.Map(h.StandardLines, func(line StandardLine, _ int) LineHeaderAccessor { | ||
| return line | ||
| }) | ||
|
|
||
| if h.GatheringLine != nil { | ||
| lines = append(lines, *h.GatheringLine) | ||
| } | ||
| return lines | ||
| } | ||
|
|
||
| type SumNetAmountInput struct { | ||
| PeriodEndLTE time.Time | ||
| IncludeCharges bool | ||
| } | ||
|
|
||
| // SumNetAmount returns the sum of the net amount (pre-tax) of the progressive billed line and its children | ||
| // containing the values for all lines whose period's end is <= in.UpTo and are not deleted or not part of | ||
| // an invoice that has been deleted. | ||
| // As gathering lines do not represent any kind of actual charge, they are not included in the sum. | ||
| func (h *SplitLineHierarchy) SumNetAmount(in SumNetAmountInput) (alpacadecimal.Decimal, error) { | ||
| netAmount := alpacadecimal.Zero | ||
|
|
||
| err := h.ForEachStandardLine(ForEachStandardLineInput{ | ||
| PeriodEndLTE: in.PeriodEndLTE, | ||
| Callback: func(line StandardLine) error { | ||
| netAmount = netAmount.Add(line.Totals.Amount) | ||
|
|
||
| if in.IncludeCharges { | ||
| netAmount = netAmount.Add(line.Totals.ChargesTotal) | ||
| } | ||
|
|
||
| return nil | ||
| }, | ||
| }) | ||
| if err != nil { | ||
| return alpacadecimal.Zero, err | ||
| } | ||
|
|
||
| return netAmount, nil | ||
| } |
There was a problem hiding this comment.
ForEachStandardLine drops the deleted-invoice status check present in the old ForEachChild
The previous ForEachChild explicitly skipped lines from invoices with Status == StandardInvoiceStatusDeleted. The new ForEachStandardLine only checks line.Invoice.DeletedAt != nil. InvoiceHeader has no Status field, so that check is gone. If an invoice transitions to a deleted status without setting DeletedAt, lines from logically deleted invoices will be included in SumNetAmount, causing progressive billing to under-charge by counting already-deleted prior-period amounts.
Prompt To Fix With AI
This is a comment left during a code review.
Path: openmeter/billing/invoicing/legacy/splitlinegroup/invoicelinesplitgroup.go
Line: 200-240
Comment:
**`ForEachStandardLine` drops the deleted-invoice status check present in the old `ForEachChild`**
The previous `ForEachChild` explicitly skipped lines from invoices with `Status == StandardInvoiceStatusDeleted`. The new `ForEachStandardLine` only checks `line.Invoice.DeletedAt != nil`. `InvoiceHeader` has no `Status` field, so that check is gone. If an invoice transitions to a deleted status without setting `DeletedAt`, lines from logically deleted invoices will be included in `SumNetAmount`, causing progressive billing to under-charge by counting already-deleted prior-period amounts.
How can I resolve this? If you propose a fix, please make it concise.1 participant
Summary
openmeter/billing/invoicing/legacy/splitlinegroupStack
Draft state
This is intentionally a mid-refactor draft. It is known not to build, and no build-error fixes were applied while reconstructing the stack.
Validation
Greptile Summary
This PR moves the legacy split-line-group domain types, adapter interface, and service interface out of the top-level
billingpackage into a dedicatedopenmeter/billing/invoicing/legacy/splitlinegrouppackage, and rewires the line engine to carry split-line hierarchy as an explicit wrapper (StandardLineWithSplitLineHierarchy) rather than embedding it inStandardLine.SplitLineHierarchyis redesigned with typedStandardLines []StandardLineandGatheringLine *GatheringLinefields (replacing the genericLinesWithInvoiceHeaders), and a newLineHeaderAccessorinterface unifies read-only access across both line types.GenerateProgressiveBilledDetailedLinesmethod for the legacy line-engine path, whileGenerateDetailedLinesis guarded against receiving a progressive accessor to prevent accidental overcharging.ResolveSplitLineGroupHeadersis refactored to return[]StandardLineWithSplitLineHierarchyinstead of mutating lines in place, andLoader/Updatergain an explicitSplitLineGroupServicedependency separated fromBillingService.Confidence Score: 3/5
The PR is an intentionally non-building mid-refactor draft; AddShrink and AddExtend in patchinvoicelinehierarchy.go were not migrated to the new interface, and two test helpers have logic gaps that could mask real regressions once the build is restored.
AddShrink and AddExtend still use the old billing.LineWithInvoiceHeader field accessors on what is now a LineHeaderAccessor interface — these methods need to be redesigned, not just re-spelled. The test helper RunCalculationTestCase silently discards tc.UnitConfig after removing unitConfigLineAccessor. The charges-path stub in engine_test.go returns an empty result instead of s.result, which can hide assertion failures. The removal of SnapshotLineQuantity from updateMutableStandardInvoice lacks an explanation of where re-snapshotting now occurs in the mutable invoice update path.
patchinvoicelinehierarchy.go (AddShrink and AddExtend methods), rating/service/testutil/ubptest.go (UnitConfig handling), charges/usagebased/service/rating/delta/engine_test.go (stub result change), invoiceupdater/invoiceupdate.go (SnapshotLineQuantity removal)
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD subgraph lineengine["lineengine package"] E[Engine] SLG[splitlinegroup.go\nResolveSplitLineGroupHeaders] SI[stdinvoice.go\nbuildStandardInvoiceLinesWithQuantitySnapshot] QS[quantitysnapshot.go\nSnapshotLineQuantities] SL[stdline.go\nStandardLineWithSplitLineHierarchy] end subgraph splitlinegroup["invoicing/legacy/splitlinegroup"] SLH[SplitLineHierarchy\nStandardLines + GatheringLine] LHA[LineHeaderAccessor interface] SLGrp[SplitLineGroup] Svc[Service interface] end subgraph rating["rating/service"] DL[detailedline.go] GPBDL[GenerateProgressiveBilledDetailedLines] GDL[GenerateDetailedLines\nnon-progressive only] end subgraph reconciler["subscriptionsync/reconciler"] IU[invoiceupdater] PH[patchinvoicelinehierarchy\nAddDelete ✓\nAddShrink ⚠\nAddExtend ⚠] Loader[persistedstate/Loader] end E --> SLG SLG --> SL SL --> SLH SI --> SLG SI --> QS SI --> DL DL --> GPBDL DL --> GDL SLH --> LHA SLH --> SLGrp Loader -->|splitLineGroupService| Svc IU -->|splitLineGroupService| Svc PH --> SLH%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD subgraph lineengine["lineengine package"] E[Engine] SLG[splitlinegroup.go\nResolveSplitLineGroupHeaders] SI[stdinvoice.go\nbuildStandardInvoiceLinesWithQuantitySnapshot] QS[quantitysnapshot.go\nSnapshotLineQuantities] SL[stdline.go\nStandardLineWithSplitLineHierarchy] end subgraph splitlinegroup["invoicing/legacy/splitlinegroup"] SLH[SplitLineHierarchy\nStandardLines + GatheringLine] LHA[LineHeaderAccessor interface] SLGrp[SplitLineGroup] Svc[Service interface] end subgraph rating["rating/service"] DL[detailedline.go] GPBDL[GenerateProgressiveBilledDetailedLines] GDL[GenerateDetailedLines\nnon-progressive only] end subgraph reconciler["subscriptionsync/reconciler"] IU[invoiceupdater] PH[patchinvoicelinehierarchy\nAddDelete ✓\nAddShrink ⚠\nAddExtend ⚠] Loader[persistedstate/Loader] end E --> SLG SLG --> SL SL --> SLH SI --> SLG SI --> QS SI --> DL DL --> GPBDL DL --> GDL SLH --> LHA SLH --> SLGrp Loader -->|splitLineGroupService| Svc IU -->|splitLineGroupService| Svc PH --> SLHComments Outside Diff (4)
openmeter/billing/worker/subscriptionsync/service/reconciler/patchinvoicelinehierarchy.go, line 94-133 (link)AddShrink— still uses old field accessorsAddShrinkwas only partially migrated. After the call toexistingHierarchy.Lines()(which now returns[]LineHeaderAccessor), the loop body still accesseschild.Line.*andchild.Invoice.*— fields that no longer exist on theLineHeaderAccessorinterface. Critically, the method also callschild.Line.CloneWithoutChildren(),child.Line.UpdateServicePeriod(), and castschild.Invoiceto check invoice type, all of which require the concretebilling.LineWithInvoiceHeadertype. Beyond the compile failure, theLineHeaderAccessorinterface is intentionally read-only header data, so the mutation logic (SetDeletedAt,UpdateServicePeriod,InvoiceAtAccessor) needs to be re-thought for this path rather than just swapping field access.Prompt To Fix With AI
openmeter/billing/worker/subscriptionsync/service/reconciler/patchinvoicelinehierarchy.go, line 164-195 (link)AddExtendnot migrated — still uses oldLinesslice typeAddExtendstill accessesexistingHierarchy.Linesdirectly as the oldbilling.LinesWithInvoiceHeadersslice (type[]billing.LineWithInvoiceHeader), usesbilling.LineWithInvoiceHeaderin theslices.SortFunccomparator, and callslines[…].Line.CloneWithoutChildren()andlines[…].Invoice.AsInvoice().Type(). None of these exist on the new typed fields (StandardLines []StandardLine,GatheringLine *GatheringLine). UnlikeAddDeleteandAddShrinkwhich were at least partially migrated togroup.Lines(), this method was not touched at all and needs equivalent migration plus logic to handle the typedStandardLines/GatheringLinesplit.Prompt To Fix With AI
openmeter/billing/rating/service/testutil/ubptest.go, line 56-160 (link)CalculationTestCase.UnitConfigsilently ignored after removingunitConfigLineAccessorunitConfigLineAccessorwas removed along with the branch that wrapped the line accessor whentc.UnitConfig != nil. TheCalculationTestCase.UnitConfigfield still exists in the struct, so callers can set it — butRunCalculationTestCasenow silently ignores it and always callsGenerateProgressiveBilledDetailedLineswith the bare line. Any test that relies on unit-config-aware rounding routed through this helper will compute with a zero unit config, producing incorrect assertions without any compile-time or runtime warning.Prompt To Fix With AI
openmeter/billing/worker/subscriptionsync/service/reconciler/invoiceupdater/invoiceupdate.go, line 366-377 (link)SnapshotLineQuantitycall removed without comment or replacement inupdateMutableStandardInvoiceThe block that called
u.quantitySnapshotter.SnapshotLineQuantity(...)and replacedtargetStandardLinewith the re-snapshotted line was deleted entirely. This means that when a subscription sync applies a period or metadata change to a mutable standard invoice line, the metered quantity is no longer re-calculated before the line is put back into the invoice. Clarify where (or whether) re-snapshotting now happens for this code path.Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "feat: move legacy splitline handling to ..." | Re-trigger Greptile