refactor: reimplement min max commitments #2627
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a variety of changes across the system. The modifications include updates to function signatures and error handling in invoice upsert operations, refactoring of invoice mapping functions, and the addition of new methods for handling progressive line hierarchies. Several pricing modules have been simplified by removing commitment and discount logic, and new mutator structures have been introduced for post-calculation processing. Test cases and a SQL migration script have been updated accordingly. Changes
Possibly related PRs
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 🔧 SQLFluff (3.3.0)tools/migrate/migrations/20250408162413_child-uniq-id-rename.up.sqlUser Error: No dialect was specified. You must configure a dialect or specify one on the command line using --dialect after the command. Available dialects: ✨ 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:
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/reimplement-min-max-commitments
branch
4 times, most recently
from
86d8aa1 to
20ca5d0
Compare
turip
changed the title
turip
force-pushed
the
refactor/reimplement-min-max-commitments
branch
from
20ca5d0 to
6f994ee
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (15)
openmeter/billing/service/lineservice/pricepackage.go (1)
46-46: Consider returning an empty slice instead ofnil.
Although returning(nil, nil)is valid, returning an empty slice (e.g.,newDetailedLinesInput{}) may prevent potential nil-slice checks downstream and can simplify iteration logic.- return nil, nil + return newDetailedLinesInput{}, nilopenmeter/productcatalog/price.go (1)
366-381: Return an error or at least log unknown types.
While returning an emptyCommitmentsfor unsupported price types is safe, consider logging or returning an explicit error for unknown price types to avoid silent failures.case PackagePriceType: return p.packagePrice.Commitments default: - return Commitments{} + // Log or handle unknown types to avoid silent failures. + // return an empty struct or an error as per design decisions return Commitments{}openmeter/billing/service/lineservice/priceunit_test.go (3)
179-179: TrackPreviouslyBilledAmount: ensure correct usage in test.The introduction of
previousBilledAmountis useful for progressive billing scenarios. Verify that downstream tests or calculations incorporate this value accurately to prevent off-by-one or partial usage errors.
2303-2303: Testing line count assertion.Verifying the invoice has 4 lines is a solid checkpoint. Consider adding more descriptive messages or intermediate checks for clarity on which lines are expected.
2361-2362: Invoice app testing comment for discounts.Refreshing documentation is always helpful for future maintainers. Possibly expand the comment to outline expected discount interactions in test results.
openmeter/billing/adapter/invoice.go (1)
683-693: Consider adding null check for BillingAddressThe code assumes
BillingAddressis never nil when mapping customer address fields. While this might be correct based on your data model, consider adding a nil check to prevent potential panic if the address could be nil in some scenarios.- BillingAddress: &models.Address{ - Country: invoice.CustomerAddressCountry, - PostalCode: invoice.CustomerAddressPostalCode, - City: invoice.CustomerAddressCity, - State: invoice.CustomerAddressState, - Line1: invoice.CustomerAddressLine1, - Line2: invoice.CustomerAddressLine2, - PhoneNumber: invoice.CustomerAddressPhoneNumber, - }, + BillingAddress: func() *models.Address { + if invoice.CustomerAddressCountry == nil && invoice.CustomerAddressPostalCode == nil && + invoice.CustomerAddressCity == nil && invoice.CustomerAddressState == nil && + invoice.CustomerAddressLine1 == nil && invoice.CustomerAddressLine2 == nil && + invoice.CustomerAddressPhoneNumber == nil { + return nil + } + return &models.Address{ + Country: invoice.CustomerAddressCountry, + PostalCode: invoice.CustomerAddressPostalCode, + City: invoice.CustomerAddressCity, + State: invoice.CustomerAddressState, + Line1: invoice.CustomerAddressLine1, + Line2: invoice.CustomerAddressLine2, + PhoneNumber: invoice.CustomerAddressPhoneNumber, + } + }(),openmeter/billing/service/lineservice/pricemutate.go (1)
5-35: Well-structured design for pricing mutatorsThis is a well-designed implementation that follows the decorator pattern. The
priceMutatorwraps an existingPricerand enhances it with post-calculation mutations, while still implementing the same interface. The type assertion at line 14 is a good practice to verify interface conformance.Consider adding a constructor function to ensure proper initialization:
// NewPriceMutator creates a new priceMutator with the given Pricer and PostCalculationMutators func NewPriceMutator(pricer Pricer, mutators ...PostCalculationMutator) *priceMutator { return &priceMutator{ Pricer: pricer, PostCalculation: mutators, } }openmeter/app/sandbox/mock.go (1)
79-79: Updated to properly propagate errors from callbackThe
UpsertInvoicemethod now correctly returns both the result and error from the callback, enabling proper error propagation through the system.Consider adding additional protection against the
MustGet()call:-return m.upsertInvoiceCallback.MustGet()(invoice) +if m.upsertInvoiceCallback.IsPresent() && m.upsertInvoiceCallback.MustGet() != nil { + return m.upsertInvoiceCallback.MustGet()(invoice) +} + +// Fallback to default behavior with no error +return billing.NewUpsertInvoiceResult(), nilopenmeter/billing/invoicelineprogressive.go (1)
41-70: Comprehensive implementation for summing progressive line amountsThe
SumNetAmountmethod correctly implements the logic to calculate the sum of net amounts for progressive billing lines, addressing the PR objective of accurately calculating min/max commitments. It properly filters out lines based on period end date, deletion status, and invoice status.Consider adding a nil check for the line period to handle potential edge cases:
-if child.Line.Period.End.After(in.UpTo) { +if child.Line.Period == nil || child.Line.Period.End.After(in.UpTo) {openmeter/billing/service/lineservice/usagebasedline.go (2)
37-39: Correct the typographical error in the comment.There's a minor spelling mistake: "child uniqe reference ids" should be "child unique reference IDs." Also, consider opening a dedicated issue or adding a code reference to track the migrations discussed in this TODO.
251-259: Check that all pricers implement the Pricer interface correctly.Each assignment (
flatPricer{},unitPricer{}, etc.) relies on these structs adhering to thePricerinterface. Since these lines are critical switch outcomes, ensure thorough unit testing for each pricer to confirm compliance with expected behaviors.openmeter/billing/service/lineservice/commitments.go (1)
94-112: Handle progressive line hierarchy gracefully.
getPreviouslyBilledAmountreturns an error if the line is declared progressive but has no hierarchy. This safeguard is important. If there are advanced use cases (e.g., partial progressive states), consider whether a more nuanced fallback is ever required. Current error handling seems sufficient unless future requirements change.openmeter/billing/adapter/invoicelineprogressive.go (3)
15-18: Clarify whether multi-level hierarchies are supported.The current documentation suggests handling of a “progressive line hierarchy” but the implementation stores only a single parent and direct children, effectively working at a single depth. If multi-level hierarchies (child-of-child lines) are intended long-term, consider clarifying or extending this function to recursively handle deeper nesting.
79-91: Consider extracting invoice loading logic.Currently, a single query loads parent lines, child lines, and their edges (flat fee, usage-based, discounts, invoice). While this simplifies code, it may be inefficient if certain fields are rarely needed. The TODO hint about separate queries is a valid optimization for large datasets.
118-146: Handle multi-parent scenarios or circular references.Your
buildProgressiveLineHierarchyfunction prevents a second time encountering the same parent line as a root. However, there is no guard against cycles (e.g., a line that eventually references itself up the chain). If cyclical or multi-parent references might occur (even if unintended) in the database, consider adding defensive checks or constraints to ensure data integrity.
📜 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 (27)
openmeter/app/sandbox/mock.go(2 hunks)openmeter/billing/adapter/invoice.go(2 hunks)openmeter/billing/adapter/invoicelineprogressive.go(1 hunks)openmeter/billing/adapter/invoicelines.go(1 hunks)openmeter/billing/invoice.go(1 hunks)openmeter/billing/invoiceline.go(3 hunks)openmeter/billing/invoicelinediscount.go(4 hunks)openmeter/billing/invoicelineprogressive.go(1 hunks)openmeter/billing/service/invoice.go(3 hunks)openmeter/billing/service/invoicestate.go(3 hunks)openmeter/billing/service/lineservice/commitments.go(1 hunks)openmeter/billing/service/lineservice/pricedynamic.go(1 hunks)openmeter/billing/service/lineservice/pricedynamic_test.go(1 hunks)openmeter/billing/service/lineservice/pricegraduatedtiered.go(0 hunks)openmeter/billing/service/lineservice/pricegraduatedtiered_test.go(4 hunks)openmeter/billing/service/lineservice/pricemutate.go(1 hunks)openmeter/billing/service/lineservice/pricepackage.go(1 hunks)openmeter/billing/service/lineservice/pricepackage_test.go(1 hunks)openmeter/billing/service/lineservice/priceunit.go(1 hunks)openmeter/billing/service/lineservice/priceunit_test.go(3 hunks)openmeter/billing/service/lineservice/pricevolumetiered.go(1 hunks)openmeter/billing/service/lineservice/pricevolumetiered_test.go(2 hunks)openmeter/billing/service/lineservice/usagebasedline.go(2 hunks)openmeter/billing/service/lineservice/usagebasedline_test.go(4 hunks)openmeter/productcatalog/price.go(2 hunks)test/billing/invoice_test.go(12 hunks)tools/migrate/migrations/20250408162413_child-uniq-id-rename.up.sql(1 hunks)
💤 Files with no reviewable changes (1)
- openmeter/billing/service/lineservice/pricegraduatedtiered.go
🧰 Additional context used
🧬 Code Definitions (16)
openmeter/billing/service/lineservice/pricevolumetiered_test.go (2)
openmeter/billing/service/lineservice/usagebasedline.go (1)
MinSpendChildUniqueReferenceID(21-21)openmeter/billing/invoiceline.go (1)
Period(82-85)
openmeter/billing/service/lineservice/pricedynamic.go (3)
openmeter/ent/db/billinginvoiceflatfeelineconfig/where.go (1)
PerUnitAmount(74-76)openmeter/billing/service/lineservice/usagebasedline.go (1)
UsageChildUniqueReferenceID(20-20)openmeter/productcatalog/price.go (1)
InArrearsPaymentTerm(21-21)
openmeter/billing/service/lineservice/priceunit.go (3)
openmeter/ent/db/billinginvoiceflatfeelineconfig/where.go (1)
PerUnitAmount(74-76)openmeter/billing/service/lineservice/usagebasedline.go (1)
UnitPriceUsageChildUniqueReferenceID(27-27)openmeter/productcatalog/price.go (1)
InArrearsPaymentTerm(21-21)
openmeter/billing/service/lineservice/priceunit_test.go (1)
openmeter/billing/service/lineservice/usagebasedline.go (1)
MinSpendChildUniqueReferenceID(21-21)
openmeter/billing/service/lineservice/usagebasedline_test.go (6)
openmeter/billing/invoiceline.go (3)
Price(695-695)Line(278-292)LineBase(127-161)openmeter/productcatalog/price.go (1)
Price(84-91)openmeter/billing/invoicelineprogressive.go (2)
InvoiceLineProgressiveHierarchy(22-25)InvoiceLineWithInvoiceBase(10-13)openmeter/billing/totals.go (1)
Totals(9-26)openmeter/ent/db/billinginvoiceline/where.go (1)
Amount(102-104)openmeter/ent/db/billinginvoice/where.go (1)
Amount(165-167)
openmeter/billing/service/lineservice/pricegraduatedtiered_test.go (3)
openmeter/ent/db/billinginvoiceflatfeelineconfig/where.go (1)
PerUnitAmount(74-76)openmeter/billing/invoiceline.go (1)
Period(82-85)openmeter/billing/service/lineservice/usagebasedline.go (1)
MinSpendChildUniqueReferenceID(21-21)
openmeter/billing/service/invoice.go (2)
openmeter/billing/invoice.go (1)
Invoice(331-342)api/api.gen.go (1)
Invoice(2865-2973)
openmeter/billing/adapter/invoicelineprogressive.go (5)
openmeter/billing/invoiceline.go (1)
Line(278-292)openmeter/ent/db/billinginvoiceline/where.go (2)
ParentLineID(142-144)ParentLineIDIn(933-935)pkg/slicesx/map.go (1)
MapWithErr(23-47)openmeter/billing/invoicelineprogressive.go (2)
InvoiceLineWithInvoiceBase(10-13)InvoiceLineProgressiveHierarchy(22-25)openmeter/billing/invoice.go (1)
Invoice(331-342)
openmeter/app/sandbox/mock.go (3)
openmeter/billing/invoice.go (1)
Invoice(331-342)api/api.gen.go (1)
Invoice(2865-2973)api/client/go/client.gen.go (1)
Invoice(2605-2713)
test/billing/invoice_test.go (4)
openmeter/billing/invoice.go (1)
Invoice(331-342)openmeter/billing/invoiceline.go (2)
InvoiceLineTypeFee(29-29)InvoiceLineTypeUsageBased(31-31)openmeter/billing/validationissue.go (1)
ValidationIssues(142-142)openmeter/billing/totals.go (1)
Totals(9-26)
openmeter/billing/adapter/invoicelines.go (3)
openmeter/billing/invoice.go (1)
Invoice(331-342)api/api.gen.go (1)
Invoice(2865-2973)api/client/go/client.gen.go (1)
Invoice(2605-2713)
openmeter/billing/adapter/invoice.go (1)
openmeter/billing/invoice.go (6)
InvoiceBase(258-297)InvoiceCustomer(516-522)CustomerUsageAttribution(509-509)InvoiceExternalIDs(471-474)InvoiceExpand(213-224)Invoice(331-342)
openmeter/billing/service/lineservice/pricemutate.go (1)
openmeter/billing/service/lineservice/pricer.go (2)
Pricer(23-29)PricerCalculateInput(16-21)
openmeter/billing/invoicelineprogressive.go (3)
openmeter/billing/invoiceline.go (2)
Line(278-292)Period(82-85)openmeter/billing/invoice.go (3)
Invoice(331-342)InvoiceBase(258-297)InvoiceStatusDeleted(88-88)openmeter/billing/totals.go (1)
Totals(9-26)
openmeter/billing/service/invoicestate.go (2)
openmeter/billing/invoicestate.go (1)
TriggerFailed(20-20)openmeter/billing/invoice.go (1)
InvoiceStatusDraftInvalid(79-79)
openmeter/billing/service/lineservice/commitments.go (6)
openmeter/billing/service/lineservice/pricemutate.go (1)
PostCalculationMutator(10-12)openmeter/billing/service/lineservice/pricer.go (1)
PricerCalculateInput(16-21)openmeter/billing/invoiceline.go (3)
Price(695-695)Period(82-85)FlatFeeCategoryCommitment(248-248)openmeter/productcatalog/price.go (1)
Price(84-91)openmeter/billing/service/lineservice/usagebasedline.go (1)
MinSpendChildUniqueReferenceID(21-21)openmeter/billing/invoicelineprogressive.go (1)
SumNetAmountInput(36-39)
🔇 Additional comments (70)
tools/migrate/migrations/20250408162413_child-uniq-id-rename.up.sql (1)
1-3: Well-structured migration for standardizing commitment identifiers.This SQL statement correctly unifies various min-spend identifiers into a single standardized value, which aligns with the PR objective of simplifying the handling of unique reference IDs across price types.
openmeter/billing/invoice.go (1)
295-296: Good documentation for future improvements.The TODO comment clearly documents a potential future enhancement to include totals in the
InvoiceBasestruct.openmeter/billing/service/lineservice/pricedynamic_test.go (1)
180-180: Added support for previous billed amount in max spend test.This change adds tracking for previously billed amounts, which is necessary for accurately calculating remaining commitments in progressive billing scenarios.
openmeter/billing/service/lineservice/pricepackage_test.go (1)
235-235: Proper test enhancement for max spend calculation.Similar to the changes in the dynamic pricing tests, this added field enables proper testing of maximum spend commitment calculations when there are previously billed amounts.
openmeter/billing/service/lineservice/pricedynamic.go (2)
28-36: Simplified pricing calculation aligns with mutator architecture.The implementation has been streamlined to focus exclusively on the core usage calculation, removing all min/max commitment application logic. This aligns with the PR's objective of moving commitment handling into distinct Mutators.
39-39: LGTM - Clear early return path.This simplified return statement improves code clarity for the no-usage scenario, separating it from the main calculation path.
openmeter/billing/service/lineservice/pricevolumetiered_test.go (2)
258-260: Updated to use standardized min-spend reference ID.The test has been updated to use the consolidated
MinSpendChildUniqueReferenceIDconstant instead of the price-type specific ID, and adds a period reference to properly test the behavior with progressive billing.
317-319: Consistent period and reference ID update.This change follows the same pattern as the previous test case, ensuring consistent reference ID usage and adding the period information needed for the new progressive billing implementation.
openmeter/billing/service/lineservice/pricevolumetiered.go (1)
38-38: Simplified capacity allocation reflects architectural changes.The capacity reduction from 4 to 2 elements reflects the removal of min/max commitment logic, which now happens in dedicated mutators after the pricing calculation is complete.
openmeter/billing/service/lineservice/pricegraduatedtiered_test.go (6)
164-165: Added previousBilledAmount to support progressive billing.This addition properly tests the scenario where flat fees have already been billed in previous periods, which is essential for the PR's objective of correctly handling progressive billing line hierarchies.
172-174: Enhanced min-spend test case with period information.The updated test now includes period information and uses the standardized reference ID, ensuring proper testing of the new commitment handling logic.
195-196: Consistent previousBilledAmount setup for second test case.This ensures the test appropriately accounts for amounts already billed in previous periods, which is crucial for correctly calculating commitment differentials.
201-203: Standardized period and reference ID patterns.This follows the same pattern as the previous test case, maintaining consistency across all tests involving minimum spend calculations.
238-238: Changed line mode to test mid-period processing.This change from
singlePerPeriodLineModetomidPeriodSplitLineModeprovides better test coverage for progressive billing scenarios, addressing a key focus of the PR.
249-250: Added detailed previousBilledAmount calculation.The comment explains exactly how the $160 value is derived, which makes the test more maintainable and self-documenting.
openmeter/billing/service/invoicestate.go (3)
70-70: State machine enhanced with new transition pathAdding a permit for the failed event from the DraftCreated state provides a direct path to transition to an invalid state when validation issues are detected early in the invoice lifecycle.
377-388: LGTM: Implementation of the TriggerFailed methodThis new method provides a clean way to handle state transitions when validation errors occur. The implementation follows the pattern established in
FireAndActivateand properly handles errors.
523-523: Good to make this explicit with a commentThe comment clarifies the reason for saving the invoice at this stage, ensuring downstream apps have all necessary IDs available.
openmeter/billing/service/lineservice/usagebasedline_test.go (3)
36-36: LGTM: New test case field for supporting progressive billingAdding the
previousBilledAmountfield to the test cases supports testing how previously billed amounts affect commitment calculations.
71-87: Good implementation of progressive billing test hierarchyThe implementation of
fakeHierarchyproperly structures a mock hierarchy with a root line and a child line. The comment on line 79 clearly explains why the period is unset in the implementation.
99-99: Correctly applied hierarchy to split linesThe hierarchy is properly assigned to both types of split lines, enabling accurate testing of commitment calculations across progressive billings.
Also applies to: 109-109
openmeter/billing/service/lineservice/priceunit.go (2)
22-31: Simplified calculation logicThe calculation logic has been simplified by directly returning the detailed line input without commitment calculations. This aligns with moving commitment handling to dedicated mutators as mentioned in the PR objectives.
33-33: Simplified nil return caseThe return statement when conditions aren't met has been simplified, maintaining the same functional behavior but with cleaner code.
openmeter/billing/invoiceline.go (3)
285-287: Added support for progressive line hierarchiesThe addition of the
ProgressiveLineHierarchyfield enables the invoice line to maintain references to hierarchical relationships, which is crucial for properly calculating min/max commitments across progressive billings.
313-313: Properly clearing hierarchy in CloneWithoutDependenciesSetting
ProgressiveLineHierarchyto nil when cloning without dependencies ensures that no unintended references are maintained, preventing potential memory leaks or incorrect calculations.
407-409: Consistent cloning of ProgressiveLineHierarchyThe implementation properly handles cloning the hierarchy only if it exists, using a pointer to the cloned hierarchy. This maintains consistency with the rest of the cloning mechanisms in the codebase.
openmeter/billing/service/lineservice/pricepackage.go (1)
35-43: Simplified return structure looks good.
The new approach of returning a single-element slice whentoBeBilledPackagesis non-zero is clear and straightforward. It cleanly encodes the additional billed packages as a single line.openmeter/billing/adapter/invoicelines.go (1)
439-450: Fetching lines and expanding hierarchy is well-structured.
This flow properly checks for errors when fetching lines before expanding them, ensuring the invoice lines are consistently populated for subsequent calculations.openmeter/productcatalog/price.go (1)
76-80: New interface method improves commitment access.
DefiningGetCommitments()in the interface clarifies how commitments can be retrieved for each price type in a uniform way.openmeter/billing/service/lineservice/priceunit_test.go (11)
45-45: Use ofMinSpendChildUniqueReferenceIDis consistent with the refactor.This change aligns the identifier with the single constant defined for minimum spend ID handling and is consistent throughout the codebase.
89-89: Consistent naming for minimum spend child ID.Good job ensuring uniform usage of
MinSpendChildUniqueReferenceID. This fosters better code readability and maintainability.
2070-2070: Mock callback for UpsertInvoice.Defining a mocked callback function to test the invoice upsert logic seems correct. This approach enhances the test’s scope by verifying how invoices are processed during upsert operations.
2077-2079: Error check for empty line ID.If a line ID is empty, the method returns an error. Confirm that this behavior is desired and no legitimate scenario expects a missing line ID (e.g., newly created lines). You might also want to add explicit logging of the error for easier debugging.
2103-2103: Validation issues length check.Ensuring zero validation issues here helps confirm the tested path remains clean and error-free.
2156-2157: Clarifying comment on previously invoiced line deletion.These comments provide helpful context explaining why discounts are reset after a line is removed. Good documentation for future readers.
2178-2178: Revised amount to 3450.Double-check that 3450 is the correct calculation for the scenario. If it represents usage or tier sums, ensure matching logic is tested thoroughly.
2180-2180: Revised total to 3450.Corresponding to the updated amount, confirm that the final total computation matches the expected discount and charges flow.
2293-2294: Additional usage events for multiple meters.Adding events for “tiered-graduated” and “flat-per-unit” ensures coverage of combined scenarios. This improves test robustness for progressive and final invoice calculations.
2331-2338: Excluding plain parent lines from final invoice output.Ensuring that certain lines (like split parents or cleared lines) are not included in the final results can avoid confusion. This seems correct for the requested functionality.
2363-2383: Validation of line external IDs and discount external IDs.These checks confirm that lines and discounts properly map back to external systems. It’s a thorough verification step that can help ensure correctness in integrated environments.
openmeter/billing/service/invoice.go (3)
293-293: Switched to storing full invoice objects increatedInvoices.Retaining a full
billing.Invoiceinstead of just an ID offers more flexibility for subsequent processing and error handling.
334-334: Refactor to calls.updateInvoice(ctx, invoice).Saving the invoice immediately after creation ensures consistency between the adapter’s stored data and the business logic. This looks like a good improvement to prevent stale invoice states.
339-339: Appending updated invoice instead of ID.Maintaining the full
savedInvoiceobject increatedInvoicesaligns with the new structure, enabling more robust checks later in the process.openmeter/billing/adapter/invoice.go (2)
654-709: Good refactoring to separate base invoice mapping logicThe extraction of
mapInvoiceBaseFromDBfunction improves code organization by isolating the common base invoice mapping logic. This makes the code more maintainable and reduces duplication.
711-766: New line hierarchy expansion supports progressive billingThe updated
mapInvoiceFromDBfunction now callsexpandProgressiveLineHierarchyto properly handle progressive billing line hierarchies. This change aligns with the PR objective to "fetch all previously billed lines for progressively billed invoices to accurately calculate min/max commitments."The integration looks good, but ensure comprehensive testing of this functionality, especially for edge cases where invoices have been deleted or have validation errors.
openmeter/billing/service/lineservice/pricemutate.go (2)
16-31: Robust implementation of post-calculation mutationsThe
Calculatemethod properly handles the execution flow by first calculating with the base pricer and then applying each mutator in sequence, stopping on the first error. This approach aligns with the PR objective of "moving the Commitment handling into distinct Mutators" and "allowing for a clearer specification of the order in which discounts and commitments are applied."
33-35: Simple delegation pattern for CanBeInvoicedAsOfGood use of delegation pattern to forward the
CanBeInvoicedAsOfcall to the embeddedPricer, maintaining interface compliance without duplicating logic.openmeter/app/sandbox/mock.go (1)
20-20: Function signature changed to include error handlingThe
InvoiceUpsertCallbacktype now returns an error, which is a breaking change but improves error handling. This aligns with the PR objective of ensuring "if an invoice encounters validation errors during creation, the state machine will transition the invoice to a failed state."Note that any existing implementations of this callback type will need to be updated to include an error return.
openmeter/billing/invoicelineprogressive.go (3)
10-20: Good design for linking invoice lines with their invoice baseThe
InvoiceLineWithInvoiceBasestruct and itsClonemethod are well-designed for maintaining the association between a line and its parent invoice. Note that only theLineis cloned, while theInvoicereference is copied as-is, which seems intentional but should be verified.
22-34: Clear hierarchy structure for progressive billingThe
InvoiceLineProgressiveHierarchystruct provides a clear representation of the hierarchical relationship between invoice lines. TheClonemethod properly creates deep copies of both the root and all children lines.
36-39: Well-defined input parameters for sum calculationThe
SumNetAmountInputstruct clearly defines the parameters needed for calculating the sum of net amounts, with a cutoff time and an option to include charges.openmeter/billing/service/lineservice/usagebasedline.go (2)
247-247: Variable initialization looks fine.Declaring the
basePricervariable here is straightforward and sets the stage for more complex logic below. No issues spotted.
264-271: Validate the mutator ordering in post-calculation.Applying the
maxAmountCommitmentMutatorbefore theminAmountCommitmentMutatorcould lead to edge cases if the maximum spend discounts reduce the final amount below the intended minimum threshold. Consider verifying logic correctness or adding tests to confirm the correct interplay of max/min commitments.openmeter/billing/service/lineservice/commitments.go (2)
1-11: File structure and imports look appropriate.The package, imports, and type definitions form a clean setup for commitment mutators. This is a good foundation with no immediate red flags regarding naming or struct organization.
58-92: Double-check scaling of maximum amount discount across multiple lines.When iterating through
pricerResultinmaxAmountCommitmentMutator, each line’s discount is applied based on the pre-line total. This is generally correct, but consider if multiple consecutive lines could cause incremental overshoot or repeated discount logic. Testing or verifying the cumulative totals logic would be prudent.openmeter/billing/invoicelinediscount.go (6)
4-4: Import usage is correct.Adding
"encoding/json"is required for custom Marshalling/Unmarshalling. No issues identified here.
294-295: Interfaces for JSON (un)marshalling properly declared.Exposing
json.Marshalerandjson.Unmarshalerin the interface is a solid approach for custom serialization of discounts. This ensures consistent usage across implementations.
337-367: Custom MarshalJSON logic is straightforward.Good use of a temporary struct to embed the discount fields based on the discount type. Returning an error for unknown types provides clarity. This approach is well-structured for extension if more discount types are added later.
369-400: Symmetrical UnmarshalJSON logic is robust.The switch on
serde.Typehelps parse different discount structures. Returning meaningful errors on unknown types or deserialization failures supports maintainability and debugging. This aligns well with the MarshalJSON approach.
409-409: Pointer-based initialization in NewLineDiscountFrom.By storing discounts in pointer form, subsequent in-place mutations become possible, which aligns with usage patterns like progressive invoice building. This ensures that ID reuse and other mutators can work seamlessly on each discount instance.
Also applies to: 415-415, 419-419, 423-423, 428-428
538-538: Pointer receiver in Mutate enables in-place transformations.Switching to
func (i *lineDiscount) Mutate(...)clarifies the intention to mutate the instance. Verify concurrency assumptions if multiple goroutines can access the same discount, but otherwise, this approach is suitable for enabling dynamic discount modifications.openmeter/billing/adapter/invoicelineprogressive.go (2)
33-43: Validate parent ID usage.When fetching lines for the parent IDs, you rely on storing child line ID → parent ID in
lineIDsToParentIDs. Although it works well for single-level hierarchies, be mindful if future requirements allow nested child lines. You may need to gather deeper ancestry or handle repeated references to the same parent in expansions.
44-60: Comprehensive validation is a good practice.Your validation steps ensure that each child line exists and is valid. This protects against referencing orphaned or erroneous lines. The approach is commendable and reduces edge-case failures in subsequent logic.
test/billing/invoice_test.go (7)
2070-2078: No issues with checking for empty line IDs
This ensures each line has a valid identifier in the test scenario.
2103-2103: Appropriate assertion for absence of validation issues
Verifying that the invoice contains no validation issues is a useful test guard.
2156-2157: Clear commentary on discount behavior after line deletion
These added comments help clarify why no discounts apply for a deleted line scenario.
2188-2189: Test expectations updated for correct invoice totals
These new values likely reflect updated calculation logic. No concerns with the change.
2263-2280: Mock setup and upsert logic look consistent
Enabling sandbox mocks, validating the invoice as nil, and assigning external IDs to lines and discounts all appear correct for test coverage.
2293-2293: Adding 'flat-per-unit' event increases test coverage
Injecting more usage data helps validate the invoice’s handling of additional events.
2450-2453: Updated invoice totals align with new calculations
These new field values (amount, charges, discounts, total) appear consistent with the revised logic.
| // Let's check if the invoice has any validation issues due to the recalculation and make sure we are not executing the | ||
| // state machine on top of the failed invoice. | ||
| if invoice.HasCriticalValidationIssues() { | ||
| invoice, err := s.withLockedInvoiceStateMachine(ctx, withLockedStateMachineInput{ | ||
| InvoiceID: invoice.InvoiceID(), | ||
| Callback: func(ctx context.Context, sm *InvoiceStateMachine) error { | ||
| return sm.TriggerFailed(ctx) | ||
| }, | ||
| }) | ||
| if err != nil { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handling critical validation issues before state machine triggers.
It’s commendable to check for critical validation issues and move the invoice into a failed state. However, consider refactoring to separate the detection of these issues from the invoice creation flow, so the logic remains simpler and more modular.
| type minAmountCommitmentMutator struct{} | ||
|
|
||
| var _ PostCalculationMutator = (*minAmountCommitmentMutator)(nil) | ||
|
|
||
| func (m *minAmountCommitmentMutator) Mutate(i PricerCalculateInput, pricerResult newDetailedLinesInput) (newDetailedLinesInput, error) { | ||
| commitments := i.line.UsageBased.Price.GetCommitments() | ||
|
|
||
| if commitments.MinimumAmount == nil { | ||
| return pricerResult, nil | ||
| } | ||
|
|
||
| // Minimum amount commitments are always applied to the last line in period | ||
| if !i.IsLastInPeriod() { | ||
| return pricerResult, nil | ||
| } | ||
|
|
||
| previouslyBilledAmount, err := getPreviouslyBilledAmount(i) | ||
| if err != nil { | ||
| return pricerResult, err | ||
| } | ||
|
|
||
| totalBilledAmount := pricerResult.Sum(i.currency).Add(previouslyBilledAmount) | ||
|
|
||
| if totalBilledAmount.LessThan(*commitments.MinimumAmount) { | ||
| minimumSpendAmount := i.currency.RoundToPrecision(commitments.MinimumAmount.Sub(totalBilledAmount)) | ||
|
|
||
| // Minimum spend is always billed for the whole period, this is a noop if we are not using progressive billing | ||
| period := i.line.Period | ||
| if i.line.ParentLine != nil { | ||
| period = i.line.ParentLine.Period | ||
| } | ||
|
|
||
| pricerResult = append(pricerResult, newDetailedLineInput{ | ||
| Name: fmt.Sprintf("%s: minimum spend", i.line.Name), | ||
| Quantity: alpacadecimal.NewFromFloat(1), | ||
| PerUnitAmount: minimumSpendAmount, | ||
| ChildUniqueReferenceID: MinSpendChildUniqueReferenceID, | ||
| PaymentTerm: productcatalog.InArrearsPaymentTerm, | ||
| Category: billing.FlatFeeCategoryCommitment, | ||
| Period: &period, | ||
| }) | ||
| } | ||
|
|
||
| return pricerResult, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure minimum amount logic accounts for partial periods.
The minAmountCommitmentMutator checks for the last line in the period and then enforces the minimum. For progressive billing scenarios, confirm if partial billing periods or intermediate invoice corrections handle min-commit logic properly. Adding or expanding test cases would help validate partial billing corner cases.
2 participants
Overview
This patch moves the Commitment handling into separate Mutators, so that we can clearly state the order of discounts/commitments applied:
Later we can add the usage discount as a PreCalculation step, and the percentage discount as a PostCalculation step before the commitment parts.
Progressive billing line hierarchies
Previously, as we didn't have discounts, we were relying on split lines on the meter's value for the line's pre-period values to calculate min/max commitments.
Now that we have discounts and number discounts applied to the lines, this will not cut it, so instead we fetch all the previously billed lines for progressively billed lines and calculate the min/max commitment based on that.
This also solves a bug, that previously if a progressively billed invoice was deleted, the min/max commitment still assumed that the invoice was paid for.
Bug fixes included
If an invoice has validation errors during creation the state machine won't try to calculate the invoice, but rather it would move it to failed state. This helps troubleshooting invoice creation issues.
Add the JSON marshaler to the invoice discounts, as the invoice discount tests depend on the JSON parser for pretty formatted outputs.
ChildUniqeReferenceID changes
For discounts we previously had per price type uniq reference IDs. This patch unifies it to a single ID for simplicity's sake.
Summary by CodeRabbit
New Features
Improvements
Chores