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 Plus 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.
0b740b7 to
e105b4c
Compare
8b49204 to
09f5e35
Compare
09f5e35 to
5423012
Compare
| if line.AsInvoiceLine().Type() == billing.InvoiceLineTypeGathering { | ||
| existingGatheringLine, err := line.AsInvoiceLine().AsGatheringLine() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("converting existing line to gathering line: %w", err) | ||
| } | ||
|
|
||
| if existingGatheringLine.SplitLineHierarchy != nil { | ||
| hierarchy, err := existingGatheringLine.SplitLineHierarchy.Clone() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("cloning split line hierarchy: %w", err) | ||
| } | ||
|
|
||
| updatedGatheringLine, err := updatedLine.AsInvoiceLine().AsGatheringLine() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("converting updated line to gathering line: %w", err) | ||
| } | ||
|
|
||
| updatedGatheringLine.SplitLineHierarchy = &hierarchy | ||
| updatedLine = updatedGatheringLine.AsGenericLine() | ||
| } |
There was a problem hiding this comment.
Apply references billing.GatheringLine.SplitLineHierarchy removed in this same PR
billing.GatheringLine.SplitLineHierarchy and billing.SplitLineHierarchy (from invoicelinesplitgroup.go) were both deleted in this PR, but the newly-added PatchLineUpdate.Apply still references them at lines 120–132. This causes an immediate compile failure in code that was just introduced, independent of the other known draft-state errors. More importantly, the intent — preserving the split-line hierarchy reference across a clone/update of a gathering line — has no replacement mechanism now that the field is gone. Any caller that updates a gathering line via Apply will silently lose the hierarchy reference unless the method is updated to carry it through the new splitlinegroup package types.
Prompt To Fix With AI
This is a comment left during a code review.
Path: openmeter/billing/worker/subscriptionsync/service/reconciler/invoiceupdater/patch.go
Line: 114-133
Comment:
**`Apply` references `billing.GatheringLine.SplitLineHierarchy` removed in this same PR**
`billing.GatheringLine.SplitLineHierarchy` and `billing.SplitLineHierarchy` (from `invoicelinesplitgroup.go`) were both deleted in this PR, but the newly-added `PatchLineUpdate.Apply` still references them at lines 120–132. This causes an immediate compile failure in code that was just introduced, independent of the other known draft-state errors. More importantly, the intent — preserving the split-line hierarchy reference across a clone/update of a gathering line — has no replacement mechanism now that the field is gone. Any caller that updates a gathering line via `Apply` will silently lose the hierarchy reference unless the method is updated to carry it through the new `splitlinegroup` package types.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.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 is an intentional mid-refactor draft (acknowledged as non-building) that extracts all legacy split-line-group domain types, adapter, and service from the top-level
billingpackage into a dedicatedopenmeter/billing/invoicing/legacy/splitlinegrouppackage, and wires the legacy line-engine path through those new boundaries.billing.SplitLineHierarchy,SplitLineGroup, and all related types are deleted frombilling/invoicelinesplitgroup.goand re-introduced undersplitlinegroup/, where the heterogeneousLines []LineWithInvoiceHeaderslice is replaced with typedStandardLines []StandardLineandGatheringLine *GatheringLinefields backed by the new read-onlyLineHeaderAccessorinterface.PatchLineUpdateswitches from carrying a fullGenericInvoiceLinesnapshot to an intent-basedNewUpdateLinePatchInput(only the fields being changed), with anApplymethod that merges the diff onto the existing line.SplitLineHierarchyis deleted from bothbilling.StandardLineandbilling.GatheringLine; the newly-addedPatchLineUpdate.Applystill referencesbilling.GatheringLine.SplitLineHierarchyin the same PR, leaving the gathering-line hierarchy-preservation path without an implementation.Confidence Score: 3/5
AddShrinkandAddExtendinpatchinvoicelinehierarchy.gostill reference old field shapes,updateMutableStandardInvoiceno longer snapshots quantities after applying line updates, and the newly introducedPatchLineUpdate.Applyimmediately references abilling.GatheringLine.SplitLineHierarchyfield that was deleted in the same PR — leaving gathering-line hierarchy preservation without any implementation. Thelineenginewiring changes and thesplitlinegrouppackage extraction are structurally sound, but the PR cannot be validated until the compilation failures and the brokenApplylogic are resolved.Important Files Changed
SplitLineHierarchynow holds typedStandardLines/GatheringLinefields replacing the old heterogeneousLines []LineWithInvoiceHeader.ForEachStandardLinedrops the status-based deleted-invoice guard that existed in the oldForEachChild(already flagged).LineHeaderAccessorinterface and associated lightweightStandardLine/GatheringLine/InvoiceHeaderheader structs. Clean boundary:GetInvoiceID()returnsmodels.NamespacedID, which differs frombilling.GenericInvoiceLine.GetInvoiceID() string— callers must use.IDto extract the string (correctly done inAddDelete, not yet fixed inAddShrink).DeleteSplitLineGroupnow correctly validates bothStandardLinesand the optionalGatheringLinebefore deletion — the previously flagged gap appears addressed by this revision.GenericInvoiceLinetarget state with intent-basedNewUpdateLinePatchInput. The newApplymethod referencesbilling.GatheringLine.SplitLineHierarchywhich was removed in this same PR, leaving hierarchy-preservation for gathering-line updates without an implementation path.LineHeaderAccessor.AddDeleteis correctly migrated;AddShrinkswitches loop source toLines()but its body still useschild.Line.*(pre-existing flagged issue).AddExtendis entirely unmigrated, still references the removedexistingHierarchy.Linesfield andbilling.LineWithInvoiceHeadertype (pre-existing flagged issue).ResolveSplitLineGroupHeadersandcalculateLinesbefore returning fromOnCollectionCompleted.LineCalculatorinterface assertion was dropped; intent is clear but verification depends oncalculateLinesbeing present elsewhere.GenericInvoiceLinetoPatchLineUpdate.updateMutableStandardInvoiceno longer callsSnapshotLineQuantityafter applying the update (pre-existing flagged issue).updateImmutableInvoiceretains quantity snapshotting for period-change validation.billingServiceandsplitLineGroupServiceinterfaces;normalizePersistedSplitLineHierarchynow mutatesStandardLines[i].ServicePeriodandGatheringLine.InvoiceAtdirectly (value semantics) instead of through interface accessors — correctly reflects the new typed fields.SplitLineHierarchyfield andSetSplitLineHierarchyfrombilling.GatheringLine; also removesGatheringInvoiceExpandSplitLineHierarchy. AddsGatheringLineWithInvoiceHeadertransport type. Removal of the hierarchy field is self-consistent here but creates a dangling reference in the newpatch.go.SplitLineHierarchyfield,SetSplitLineHierarchy,GetProgressivelyBilledServicePeriod,GetPreviouslyBilledAmount,IsProgressivelyBilled, andWithoutSplitLineHierarchyfromStandardLine. AddsStandardLineWithInvoiceHeader. Responsibility for progressive billing math is moving to thesplitlinegrouppackage.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD subgraph OLD["Before: billing package"] OA["billing.SplitLineHierarchy"] --> OB["Lines - heterogeneous slice"] OD["billing.GatheringLine\n+ SplitLineHierarchy field"] --> OA OE["billing.StandardLine\n+ SplitLineHierarchy field"] --> OA end subgraph NEW["After: splitlinegroup package"] NA["splitlinegroup.SplitLineHierarchy"] --> NB["StandardLines typed slice"] NA --> NC["GatheringLine pointer"] NB --> ND["LineHeaderAccessor\nread-only interface"] NC --> ND NE["splitlinegroup.Service"] --> NF["splitlinegroup.Adapter"] end subgraph ENGINE["Line Engine"] LE["OnCollectionCompleted"] --> RS["ResolveSplitLineGroupHeaders"] RS --> SQ["SnapshotLineQuantities"] SQ --> CL["calculateLines"] end subgraph UPDATER["Subscription Sync Updater"] PU["PatchLineUpdate.Apply\nreferences removed field WARN"] --> OD UPS["updateMutableStandardInvoice\nno SnapshotLineQuantity WARN"] --> CL end OLD -.->|"migrated to"| NEW NEW --> ENGINE NEW --> UPDATERPrompt To Fix All With AI
Reviews (3): Last reviewed commit: "fix: self-review" | Re-trigger Greptile