refactor: split lines #2961
refactor: split lines #2961
Conversation
📝 WalkthroughWalkthroughThis change removes the legacy "split" invoice line status and parent/child split line model, replacing it with a new "split line group" structure for usage-based billing lines. It introduces new domain types, interfaces, service methods, adapter logic, and migration scripts to support split line groups and their hierarchies. Corresponding updates are made to code, tests, and database schema to use split line groups, with removals of the old progressive/split line logic. Changes
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (11)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
turip
force-pushed
the
refactor/split-lines-v2
branch
from
a822096 to
6c2adcd
Compare
turip
force-pushed
the
refactor/split-lines-v2
branch
5 times, most recently
from
9153163 to
39f9d3c
Compare
turip
force-pushed
the
refactor/split-lines-v2-ddl
branch
from
a83f392 to
9871466
Compare
turip
force-pushed
the
refactor/split-lines-v2
branch
4 times, most recently
from
069387e to
fc1d56e
Compare
turip
force-pushed
the
refactor/split-lines-v2
branch
from
fc1d56e to
fa40904
Compare
turip
force-pushed
the
refactor/split-lines-v2
branch
from
fa40904 to
5cfe0c8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 7
🔭 Outside diff range comments (2)
tools/migrate/testdata/billing/removesplitlines/sqlc/queries.sql (1)
11-15:⚠️ Potential issueAdd missing semicolon to SQL query.
The
CountLinesByStatusTypequery is missing a semicolon at the end.SELECT status, type, count(*) FROM public.billing_invoice_lines - GROUP BY status, type; + GROUP BY status, type;openmeter/billing/service/lineservice/linebase.go (1)
176-212: 🛠️ Refactor suggestionAddress the design concern about mutation vs cloning
The TODO comment raises a valid concern about the Update method mutating the receiver while returning it as if it's a new object. This could lead to confusion about ownership and side effects.
Consider either:
- Making this method clearly mutate in-place and return nothing, or
- Cloning the line before updating and returning the clone
For consistency with immutable patterns:
func (l lineBase) Update(in UpdateInput) Line { - // TODO[later]: Either we should clone and update the clone or we should not return the Line as if that's a new - // object. + // Clone to maintain immutability + clonedLine := l.line.CloneWithoutDependencies() + clonedLine.ID = l.line.ID + clonedLine.CreatedAt = l.line.CreatedAt + clonedLine.UpdatedAt = l.line.UpdatedAt - if !in.PeriodStart.IsZero() { - l.line.Period.Start = in.PeriodStart - } + if !in.PeriodStart.IsZero() { + clonedLine.Period.Start = in.PeriodStart + } // ... apply other updates to clonedLine ... // Let's ignore the error here as we don't allow for any type updates - svc, _ := l.service.FromEntity(l.line) + svc, _ := l.service.FromEntity(clonedLine) return svc }
♻️ Duplicate comments (1)
openmeter/billing/invoicelinesplitgroup.go (1)
285-340: Consider interface-based design for LineOrHierarchyThe struct-based discriminated union pattern works, but an interface-based approach might provide better extensibility and clearer API boundaries.
🧹 Nitpick comments (12)
openmeter/billing/service/lineservice/pricer.go (1)
66-74: Update comment to reflect new split line group terminologyThe logic change from
ParentLineIDtoSplitLineGroupIDcorrectly maintains the invoicing restriction, but the comment on line 66 still references "parent line" terminology.Consider updating the comment to reflect the new split line group model:
- // Invoicing a line that has a parent line is not supported, as that's a progressive billing use-case + // Invoicing a line that belongs to a split line group is not supported, as that's a progressive billing use-caseopenmeter/billing/service/lineservice/commitments.go (2)
40-46: Update error message terminology for consistency.The logic correctly uses the new split line group model, but the error message still references "progressive billed line" instead of "split line group".
- return pricerResult, fmt.Errorf("line[%s] does not have a split line hierarchy, but is a progressive billed line", i.line.ID) + return pricerResult, fmt.Errorf("line[%s] does not have a split line hierarchy, but has a split line group ID", i.line.ID)
99-114: Update error message terminology and approve the changes.The function correctly uses the new split line hierarchy model. However, the error message should be updated for consistency.
- return alpacadecimal.Zero, fmt.Errorf("line[%s] does not have a progressive line hierarchy, but is a progressive billed line", l.line.ID) + return alpacadecimal.Zero, fmt.Errorf("line[%s] does not have a split line hierarchy, but has a split line group ID", l.line.ID)openmeter/billing/service/lineservice/discountusage.go (1)
130-131: Update error message to reflect the new terminology.The error message still mentions "progressive billed line" which is inconsistent with the new split line group model.
- return alpacadecimal.Zero, errors.New("no line hierarchy is available for a progressive billed line") + return alpacadecimal.Zero, errors.New("no line hierarchy is available for a split line")openmeter/billing/service/invoicelinesplitgroup.go (1)
19-29: Fix grammatical error in comment.The possessive form should be "its" not "it's".
- // Let's load the split line group and validate that all of it's children are also deleted + // Let's load the split line group and validate that all of its children are also deletedAlso applies to line 29:
- // Let's validate that all of it's children are also deleted + // Let's validate that all of its children are also deletedopenmeter/billing/adapter/invoicelines.go (2)
141-143: Track the TODO for nullable field handling.The TODO indicates that all nullable fields must be listed explicitly in the upsert operation. This could lead to bugs if new nullable fields are added to the schema without updating this code.
Would you like me to open an issue to track implementing a more robust solution that automatically handles all nullable fields?
615-617: Enhance error message with specific overlapping IDs.The error message could be more helpful by including which entities have the overlapping IDs.
- return nil, fmt.Errorf("overlapping childUniqueReferenceID: %v", overlappingChildUniqueReferenceIDs) + return nil, fmt.Errorf("found overlapping childUniqueReferenceID between split line groups and individual lines: %v", overlappingChildUniqueReferenceIDs)test/billing/invoice_test.go (1)
922-939: Fix typo in parameter name.The function logic is correct and properly implements the split line group model, but there's a typo in the parameter name.
-func (s *InvoicingTestSuite) lineInSameSplitLineGroup(lines []*billing.Line, shiblingLineID string) *billing.Line { +func (s *InvoicingTestSuite) lineInSameSplitLineGroup(lines []*billing.Line, siblingLineID string) *billing.Line { s.T().Helper() for _, line := range lines { if line.SplitLineHierarchy == nil { continue } for _, child := range line.SplitLineHierarchy.Lines { - if child.Line.ID == shiblingLineID { + if child.Line.ID == siblingLineID { return line } } }tools/migrate/testdata/billing/removesplitlines/db/queries.sql.go (2)
29-50: Remove redundantrows.Close()call.The
defer rows.Close()statement on line 34 already ensures the rows are closed. The explicit call on line 43 is redundant.} - if err := rows.Close(); err != nil { - return nil, err - } if err := rows.Err(); err != nil {
132-192: Remove redundantrows.Close()call.Similar to the previous function, the
defer rows.Close()statement already ensures cleanup.} - if err := rows.Close(); err != nil { - return nil, err - } if err := rows.Err(); err != nil {openmeter/billing/adapter/invoicelinesplitgroup.go (1)
223-225: Update outdated comment referencing progressive line hierarchy.The comment still references the old "progressive line hierarchy" model. Update it to reflect the new split line hierarchy model.
-// expandSplitLineHierarchy expands the given lines with their progressive line hierarchy +// expandSplitLineHierarchy expands the given lines with their split line hierarchy // This is done by fetching all the lines that are children of the given lines parent lines and then building // the hierarchy.openmeter/billing/worker/subscription/invoiceupdate.go (1)
205-207: Improve error message clarity for missing child reference ID.The current error message could be confusing when
ChildUniqueReferenceIDis nil, as it will display the string "nil" rather than indicating absence.- return fmt.Errorf("line[%s/%s] not found in the invoice, cannot update", targetState.ID, lo.FromPtrOr(targetState.ChildUniqueReferenceID, "nil")) + return fmt.Errorf("line[%s] not found in the invoice, cannot update", targetState.ID)Alternatively, if the child reference ID is important for debugging:
if targetState.ChildUniqueReferenceID != nil { return fmt.Errorf("line[%s/%s] not found in the invoice, cannot update", targetState.ID, *targetState.ChildUniqueReferenceID) } return fmt.Errorf("line[%s] not found in the invoice, cannot update", targetState.ID)
🛑 Comments failed to post (7)
tools/migrate/testdata/billing/README.md (1)
15-15:
⚠️ Potential issueFix typo and verify command syntax.
There's a typo: "specifiy" should be "specify". Additionally, the command appears to mix
-n(schema names) and-t(table names) flags, which may not work as intended.Apply this diff to fix the typo and clarify the command:
- - You can also use the `-n` flag to specifiy individual schemas: `pg_dump -Ox -s -t '*billing*' 'postgres://pgtdbuser:[email protected]:5432/testdb_tpl_92d6cc3e2b7979388fd8f7b12aad9c7b_inst_4710adae?sslmode=disable'` + - You can also use the `-n` flag to specify individual schemas: `pg_dump -Ox -s -n '*billing*' 'postgres://pgtdbuser:[email protected]:5432/testdb_tpl_92d6cc3e2b7979388fd8f7b12aad9c7b_inst_4710adae?sslmode=disable'`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.- You can also use the `-n` flag to specify individual schemas: `pg_dump -Ox -s -n '*billing*' 'postgres://pgtdbuser:[email protected]:5432/testdb_tpl_92d6cc3e2b7979388fd8f7b12aad9c7b_inst_4710adae?sslmode=disable'`🤖 Prompt for AI Agents
In tools/migrate/testdata/billing/README.md at line 15, fix the typo by changing "specifiy" to "specify". Also, correct the command by using the `-n` flag properly for specifying schemas instead of mixing it with the `-t` flag for tables. Update the example command to use `-n '*billing*'` if the intention is to filter schemas, ensuring the syntax matches pg_dump's expected usage.tools/migrate/migrations/20250609204117_billing-migrate-split-line-groups.up.sql (2)
53-55:
⚠️ Potential issueAdd validation to ensure parent_line_id references a valid split line.
The migration assumes that
parent_line_idpoints to a split line, but doesn't verify this. This could lead to data integrity issues if the assumption is incorrect.Consider adding a validation step or JOIN condition to ensure the referenced parent line exists and has the expected status:
UPDATE public.billing_invoice_lines -SET split_line_group_id = parent_line_id, parent_line_id = NULL -WHERE "type" = 'usage_based' and "status" = 'valid' and "parent_line_id" IS NOT NULL; +SET split_line_group_id = parent_line_id, parent_line_id = NULL +WHERE "type" = 'usage_based' + AND "status" = 'valid' + AND "parent_line_id" IS NOT NULL + AND EXISTS ( + SELECT 1 FROM public.billing_invoice_split_line_groups + WHERE id = billing_invoice_lines.parent_line_id + );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.UPDATE public.billing_invoice_lines SET split_line_group_id = parent_line_id, parent_line_id = NULL WHERE "type" = 'usage_based' AND "status" = 'valid' AND "parent_line_id" IS NOT NULL AND EXISTS ( SELECT 1 FROM public.billing_invoice_split_line_groups WHERE id = billing_invoice_lines.parent_line_id );🤖 Prompt for AI Agents
In tools/migrate/migrations/20250609204117_billing-migrate-split-line-groups.up.sql around lines 53 to 55, the UPDATE statement sets split_line_group_id based on parent_line_id without verifying that parent_line_id references a valid split line. To fix this, modify the UPDATE to include a JOIN or EXISTS clause that ensures parent_line_id points to a line with the expected status and type, thereby validating the reference before updating to maintain data integrity.
1-60:
⚠️ Potential issueWrap migration in a transaction for atomicity.
The migration performs three critical steps without transaction wrapping. If any step fails, the database could be left in an inconsistent state.
Consider wrapping the entire migration in a transaction:
+BEGIN; + -- Step 1: convert existing split lines into split line groups INSERT INTO public.billing_invoice_split_line_groups ... -- Step 3: delete the split lines DELETE FROM public.billing_invoice_lines WHERE "type" = 'usage_based' and "status" = 'split'; + +COMMIT;Additionally, consider adding a corresponding
.down.sqlfile for rollback capability.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.BEGIN; -- Step 1: convert existing split lines into split line groups INSERT INTO public.billing_invoice_split_line_groups ( id, namespace, metadata, created_at, updated_at, deleted_at, name, description, service_period_start, service_period_end, currency, tax_config, unique_reference_id, ratecard_discounts, feature_key, price, subscription_id, subscription_phase_id, subscription_item_id ) SELECT l.id, l.namespace, l.metadata, l.created_at, l.updated_at, l.deleted_at, l.name, l.description, l.period_start, l.period_end, l.currency, l.tax_config, l.child_unique_reference_id, l.ratecard_discounts, u.feature_key, u.price, l.subscription_id, l.subscription_phase_id, l.subscription_item_id FROM public.billing_invoice_lines l JOIN public.billing_invoice_usage_based_line_configs u ON l.usage_based_line_config_id = u.id WHERE l.type = 'usage_based' AND l.status = 'split'; -- Step 2: Associate existing lines referencing the split lines to the split line group UPDATE public.billing_invoice_lines SET split_line_group_id = parent_line_id, parent_line_id = NULL WHERE "type" = 'usage_based' AND "status" = 'valid' AND parent_line_id IS NOT NULL; -- Step 3: delete the split lines DELETE FROM public.billing_invoice_lines WHERE "type" = 'usage_based' AND "status" = 'split'; COMMIT;🤖 Prompt for AI Agents
In tools/migrate/migrations/20250609204117_billing-migrate-split-line-groups.up.sql lines 1 to 60, the migration executes multiple critical steps without wrapping them in a transaction, risking partial application and inconsistent database state if a failure occurs. Wrap the entire migration script in a BEGIN and COMMIT transaction block to ensure atomicity. Additionally, create a corresponding down migration script to allow rollback of these changes if needed.openmeter/billing/service.go (1)
52-55:
⚠️ Potential issueMissing GetSplitLineGroup method
According to the AI summary, the
SplitLineGroupServiceinterface should include aGetSplitLineGroupmethod, but it's not present here. This method would be needed for retrieving split line group details.Consider adding:
GetSplitLineGroup(ctx context.Context, input GetSplitLineGroupInput) (SplitLineGroup, error)🤖 Prompt for AI Agents
In openmeter/billing/service.go around lines 52 to 55, the SplitLineGroupService interface is missing the GetSplitLineGroup method required for retrieving split line group details. Add the method signature GetSplitLineGroup(ctx context.Context, input GetSplitLineGroupInput) (SplitLineGroup, error) to the interface to provide this functionality.openmeter/billing/invoiceline.go (1)
305-307: 💡 Verification agent
🧩 Analysis chain
JSON tag inconsistency with field name.
The field name changed from
ProgressiveLineHierarchytoSplitLineHierarchy, but the JSON tag remainsprogressiveLineHierarchy. This inconsistency could cause confusion and might indicate a backward compatibility concern that should be documented.
🏁 Script executed:
#!/bin/bash # Description: Check if there are any API clients or tests that depend on the 'progressiveLineHierarchy' JSON field name # Search for references to the progressiveLineHierarchy JSON field in test files and API docs rg -A 3 -B 3 'progressiveLineHierarchy' --type go --type json --type yaml --type mdLength of output: 589
Document backward‐compatibility of JSON tag
The
SplitLineHierarchyfield intentionally keeps thejson:"progressiveLineHierarchy,omitempty"tag to preserve API compatibility with existing clients. To avoid confusion, please add a comment noting the legacy name.• File:
openmeter/billing/invoiceline.go
• Lines: ~305Suggested diff:
- SplitLineHierarchy *SplitLineHierarchy `json:"progressiveLineHierarchy,omitempty"` + // SplitLineHierarchy was renamed from ProgressiveLineHierarchy; + // JSON tag is preserved for backward compatibility with existing clients. + SplitLineHierarchy *SplitLineHierarchy `json:"progressiveLineHierarchy,omitempty"`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.Children LineChildren `json:"children,omitempty"` ParentLine *Line `json:"parent,omitempty"` + // SplitLineHierarchy was renamed from ProgressiveLineHierarchy; + // JSON tag is preserved for backward compatibility with existing clients. SplitLineHierarchy *SplitLineHierarchy `json:"progressiveLineHierarchy,omitempty"`🤖 Prompt for AI Agents
In openmeter/billing/invoiceline.go around lines 305 to 307, the JSON tag for the SplitLineHierarchy field is still "progressiveLineHierarchy" to maintain backward compatibility, but this is inconsistent with the field name. Add a comment above this field explaining that the JSON tag uses the legacy name intentionally to preserve API compatibility and avoid confusion for future maintainers.openmeter/billing/invoicelinesplitgroup.go (2)
173-183:
⚠️ Potential issueMissing NamespacedID in Clone method
The
Clonemethod is missing theNamespacedIDfield, which could lead to incorrect behavior when cloning split line groups.func (i SplitLineGroup) Clone() SplitLineGroup { return SplitLineGroup{ ManagedModel: i.ManagedModel, + NamespacedID: i.NamespacedID, SplitLineGroupMutableFields: i.SplitLineGroupMutableFields.Clone(), Price: i.Price, FeatureKey: i.FeatureKey, Subscription: i.Subscription, Currency: i.Currency, UniqueReferenceID: i.UniqueReferenceID, } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func (i SplitLineGroup) Clone() SplitLineGroup { return SplitLineGroup{ ManagedModel: i.ManagedModel, NamespacedID: i.NamespacedID, SplitLineGroupMutableFields: i.SplitLineGroupMutableFields.Clone(), Price: i.Price, FeatureKey: i.FeatureKey, Subscription: i.Subscription, Currency: i.Currency, UniqueReferenceID: i.UniqueReferenceID, } }🤖 Prompt for AI Agents
In openmeter/billing/invoicelinesplitgroup.go around lines 173 to 183, the Clone method is missing the NamespacedID field, which should be included to ensure the cloned SplitLineGroup is complete and behaves correctly. Add the NamespacedID field to the returned SplitLineGroup struct, assigning it from the original instance.
74-112:
⚠️ Potential issueRemove duplicate validation of RatecardDiscounts
The
RatecardDiscountsare validated twice - once at line 81 throughSplitLineGroupMutableFields.ValidateForPrice()and again at line 107.func (i SplitLineGroupCreate) Validate() error { var errs []error if i.Namespace == "" { errs = append(errs, errors.New("namespace is required")) } if err := i.SplitLineGroupMutableFields.ValidateForPrice(i.Price); err != nil { errs = append(errs, err) } if i.Price == nil { errs = append(errs, errors.New("price is required")) } else { if err := i.Price.Validate(); err != nil { errs = append(errs, err) } } if i.Subscription != nil { if err := i.Subscription.Validate(); err != nil { errs = append(errs, err) } } if i.Currency == "" { errs = append(errs, errors.New("currency is required")) } if i.UniqueReferenceID != nil && *i.UniqueReferenceID == "" { errs = append(errs, errors.New("unique reference id is required")) } - if err := i.RatecardDiscounts.ValidateForPrice(i.Price); err != nil { - errs = append(errs, err) - } - return errors.Join(errs...) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func (i SplitLineGroupCreate) Validate() error { var errs []error if i.Namespace == "" { errs = append(errs, errors.New("namespace is required")) } if err := i.SplitLineGroupMutableFields.ValidateForPrice(i.Price); err != nil { errs = append(errs, err) } if i.Price == nil { errs = append(errs, errors.New("price is required")) } else { if err := i.Price.Validate(); err != nil { errs = append(errs, err) } } if i.Subscription != nil { if err := i.Subscription.Validate(); err != nil { errs = append(errs, err) } } if i.Currency == "" { errs = append(errs, errors.New("currency is required")) } if i.UniqueReferenceID != nil && *i.UniqueReferenceID == "" { errs = append(errs, errors.New("unique reference id is required")) } return errors.Join(errs...) }🤖 Prompt for AI Agents
In openmeter/billing/invoicelinesplitgroup.go between lines 74 and 112, the RatecardDiscounts validation is duplicated: it is called once inside SplitLineGroupMutableFields.ValidateForPrice() around line 81 and again explicitly at line 107. To fix this, remove the explicit call to i.RatecardDiscounts.ValidateForPrice(i.Price) at line 107 to avoid redundant validation.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
openmeter/billing/worker/subscription/invoiceupdate.go (1)
206-206: Consider using a more descriptive nil representation.Using "nil" as a string might be ambiguous in logs. Consider using "" or "N/A" for better clarity.
-return fmt.Errorf("line[%s/%s] not found in the invoice, cannot update", targetState.ID, lo.FromPtrOr(targetState.ChildUniqueReferenceID, "nil")) +return fmt.Errorf("line[%s/%s] not found in the invoice, cannot update", targetState.ID, lo.FromPtrOr(targetState.ChildUniqueReferenceID, "<none>"))openmeter/billing/invoicelinesplitgroup.go (1)
216-218: Update outdated comment referencing progressive lines.The comment still references "progressive billed line" but the code now uses split line hierarchies.
-// SumNetAmount returns the sum of the net amount (pre-tax) of the progressive billed line and its children +// SumNetAmount returns the sum of the net amount (pre-tax) of the split line hierarchy 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.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tools/migrate/migrations/atlas.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
openmeter/billing/adapter/invoicelinesplitgroup.go(1 hunks)openmeter/billing/discount.go(1 hunks)openmeter/billing/invoiceline.go(9 hunks)openmeter/billing/invoicelinesplitgroup.go(1 hunks)openmeter/billing/service/invoicelinesplitgroup.go(1 hunks)openmeter/billing/service/lineservice/commitments.go(2 hunks)openmeter/billing/service/lineservice/linebase.go(7 hunks)openmeter/billing/service/lineservice/meters.go(5 hunks)openmeter/billing/service/lineservice/usagebasedline_test.go(3 hunks)openmeter/billing/worker/subscription/invoiceupdate.go(1 hunks)openmeter/billing/worker/subscription/patch.go(1 hunks)openmeter/billing/worker/subscription/sync.go(11 hunks)openmeter/billing/worker/subscription/sync_test.go(6 hunks)openmeter/ent/db/billinginvoicesplitlinegroup_create.go(2 hunks)openmeter/ent/db/billinginvoicesplitlinegroup_update.go(0 hunks)openmeter/ent/db/setorclear.go(0 hunks)openmeter/ent/schema/billing.go(1 hunks)test/billing/invoice_test.go(10 hunks)
💤 Files with no reviewable changes (2)
- openmeter/ent/db/setorclear.go
- openmeter/ent/db/billinginvoicesplitlinegroup_update.go
✅ Files skipped from review due to trivial changes (1)
- openmeter/billing/service/invoicelinesplitgroup.go
🚧 Files skipped from review as they are similar to previous changes (11)
- openmeter/billing/discount.go
- openmeter/ent/schema/billing.go
- openmeter/billing/service/lineservice/usagebasedline_test.go
- openmeter/billing/service/lineservice/meters.go
- openmeter/billing/invoiceline.go
- openmeter/billing/worker/subscription/sync_test.go
- openmeter/billing/adapter/invoicelinesplitgroup.go
- test/billing/invoice_test.go
- openmeter/ent/db/billinginvoicesplitlinegroup_create.go
- openmeter/billing/service/lineservice/commitments.go
- openmeter/billing/service/lineservice/linebase.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
openmeter/billing/worker/subscription/patch.go (2)
openmeter/billing/invoiceline.go (2)
Line(298-312)LineID(21-21)openmeter/billing/worker/subscription/sync.go (1)
Handler(66-73)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: CI
- GitHub Check: E2E
- GitHub Check: Lint
- GitHub Check: Test
- GitHub Check: Analyze (go)
🔇 Additional comments (14)
openmeter/billing/worker/subscription/invoiceupdate.go (2)
26-70: Well-structured patch application logic!The method follows a clear flow with proper separation of concerns and comprehensive error handling.
90-142: Clean patch parsing implementation!Good use of type assertions and clear error messages for each patch operation type.
openmeter/billing/worker/subscription/patch.go (2)
10-50: Excellent refactoring to structured patch types!The new structure provides better type safety and clearer separation between different patch operations.
142-180: Well-implemented polymorphic delete patch generation!Good handling of both line and hierarchy types with proper filtering of already deleted items.
openmeter/billing/invoicelinesplitgroup.go (1)
27-49: Good validation implementation with proper error aggregation!Nice use of
errors.Joinfor collecting multiple validation errors.openmeter/billing/worker/subscription/sync.go (9)
147-150: LGTM: Customer ID construction is correct.The customer ID construction follows the established pattern and is properly scoped for the billing service operations.
154-156: LGTM: Billing profile validation is appropriate.The customer override retrieval serves as a proper validation step before proceeding with the synchronization.
170-184: LGTM: Clear separation of concerns in the synchronization flow.The refactored flow properly separates plan calculation, patch generation, and patch application phases, which improves maintainability and testability.
187-202: LGTM: Unified patch application approach.The introduction of
InvoiceUpdaterfor applying all patches at once is a good architectural improvement over the previous manual per-invoice approach.
208-217: LGTM: Type refactoring aligns with the polymorphic approach.The migration from concrete
*billing.Linetypes to polymorphicbilling.LineOrHierarchyproperly supports both single lines and split line groups.
257-262: LGTM: Proper polymorphic interface usage.The filtering and mapping logic correctly uses the
ChildUniqueReferenceID()method from the polymorphic interface to handle both lines and hierarchies uniformly.
310-350: LGTM: Well-structured patch generation logic.The extracted
getPatchesFromPlanfunction properly handles both deletion and update scenarios, with appropriate error handling and null line filtering for zero-amount lines.
555-584: LGTM: Proper polymorphic dispatch pattern.The switch-based dispatch to handle both
LineOrHierarchyTypeLineandLineOrHierarchyTypeHierarchycases follows good polymorphic design patterns with appropriate error handling.
586-646: LGTM: Comprehensive single line update logic.The function properly handles managed line detection, period adjustments, flat fee prorating, and edge cases like empty usage-based periods. The change detection pattern is efficient.
Overview
Previously, the line object was used for 4 distinct reasons:
This patch moves the split lines from the gathering invoice into a separate entity. This is required as this will allow us to:
The existing tests are passing, the only test removed is the adapter test that handled split line updates.
The migration is validated using the SQLc based fixture setup to ensure that we are correctly migrating to the new setup.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Database Migration
Documentation
Tests