refactor: billing pricer interface #2624
Conversation
📝 WalkthroughWalkthroughThis pull request introduces several new pricing implementations and refactors within the billing module. Multiple pricer types—including dynamic, flat, graduated tiered, unit, volume tiered, and package pricers—are added with their corresponding tests. A common Pricer interface is defined along with progressive and non-progressive billing variants. Additionally, the usage-based line calculations are refactored to delegate pricing to the appropriate pricer, and invoice tests are updated to reflect changes in billing support. A new error for unsupported progressive billing is also introduced. 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 ✨ Finishing Touches
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (24)
openmeter/billing/service/lineservice/pricedynamic.go (1)
1-53: Overall implementation of the dynamic pricer looks solid.
The logic for calculating the period amount and applying commitments is clearly structured. One optional consideration is to guard against scenarios wheredynamicPrice.Multipliermight be zero or negative (if that is possible) to avoid unintended zero or negative charges. Otherwise, the code appears correct.test/billing/invoice_test.go (1)
2290-2291: Consider removing or converting debug statements.
s.DebugDumpInvoicecan be helpful during development, but consider replacing it with structured logging or removing it if it’s no longer needed.openmeter/billing/service/lineservice/pricetiered.go (1)
35-49: Consider extracting common conversion logic.While the implementation is correct, there's duplicate code between
CalculateandCanBeInvoicedAsOffor price conversion and switch logic. Consider extracting this common functionality to reduce duplication.+func (p tieredPricer) convertAndRoute(l usageBasedLine, handler func(mode productcatalog.TieredPriceMode) (bool, error)) (bool, error) { + price, err := l.line.UsageBased.Price.AsTiered() + if err != nil { + return false, fmt.Errorf("converting price to tiered price: %w", err) + } + + switch price.Mode { + case productcatalog.VolumeTieredPrice: + return handler(productcatalog.VolumeTieredPrice) + case productcatalog.GraduatedTieredPrice: + return handler(productcatalog.GraduatedTieredPrice) + default: + return false, fmt.Errorf("unsupported tiered price mode: %s", price.Mode) + } +} + func (p tieredPricer) CanBeInvoicedAsOf(l usageBasedLine, asOf time.Time) (bool, error) { - price, err := l.line.UsageBased.Price.AsTiered() - if err != nil { - return false, fmt.Errorf("converting price to tiered price: %w", err) - } - - switch price.Mode { - case productcatalog.VolumeTieredPrice: - return p.volume.CanBeInvoicedAsOf(l, asOf) - case productcatalog.GraduatedTieredPrice: - return p.graduated.CanBeInvoicedAsOf(l, asOf) - default: - return false, fmt.Errorf("unsupported tiered price mode: %s", price.Mode) - } + return p.convertAndRoute(l, func(mode productcatalog.TieredPriceMode) (bool, error) { + if mode == productcatalog.VolumeTieredPrice { + return p.volume.CanBeInvoicedAsOf(l, asOf) + } + return p.graduated.CanBeInvoicedAsOf(l, asOf) + }) }Note: This suggestion requires adapting
Calculatemethod as well but increases complexity for relatively small methods, so it's optional.openmeter/billing/service/lineservice/priceflat.go (1)
17-47: Consider adding a comment for the default return case.The implementation correctly handles in-advance payments for the first line in a period and non-advance payments for the last line. However, when neither condition is met, the method returns
nil, nilwithout explanation.Consider adding a comment before the final return statement to explain the intended behavior:
} + // For other cases (e.g., mid-period lines for in-advance payments or non-last lines for in-arrears payments), + // we don't generate any line items as they will be handled by either the first or last line in the period. return nil, nilopenmeter/billing/service/lineservice/priceunit.go (1)
9-11: Consider adding a descriptive comment for the unitPricer structA brief comment explaining the purpose of this struct and how it relates to the
ProgressiveBillingPricerwould improve code clarity.+// unitPricer implements the Pricer interface for unit-based pricing calculations. +// It embeds ProgressiveBillingPricer to inherit progressive billing behavior. type unitPricer struct { ProgressiveBillingPricer }openmeter/billing/service/lineservice/pricepacakge_test.go (1)
1-2: Filename has a typoThere's a typo in the filename "pricepacakge_test.go" - it should be "pricepackage_test.go".
openmeter/billing/service/lineservice/pricevolumetiered.go (2)
27-31: Consider adding logging or supplementary details for early returns.When
!l.IsLastInPeriod(), the function returns(nil, nil)with a brief comment about progressive billing not being supported. It may improve maintainability to log or provide more context in the error message or comment to clarify that no billing occurs for partial periods.
66-84: Extract repeated discount logic for maintainability.Lines 66-84 replicate discounting logic similar to lines 47-64. Consider extracting a helper function to implement ApplyDiscountForMaxSpend, to adhere to DRY and improve maintainability.
openmeter/billing/service/lineservice/pricepacakage.go (1)
60-68: Guard against zero or invalid package sizes.If
packagePrice.QuantityPerPackageis zero or invalid,qty.Div(packageSize)could lead to a runtime error or an unexpected result. Consider validating the package size to avoid division by zero.openmeter/billing/service/lineservice/usagebasedline_test.go (1)
91-94: Expand test coverage for edge cases.Currently, the test sets usage from
tc.usage. Consider adding cases for negative usage or extremely large values to ensure robust error checking and behavior.openmeter/billing/service/lineservice/usagebasedline.go (5)
94-97: HandlingCanBeInvoicedAsOferrors
The error check pattern is consistent. You may consider adding context in the error log for traceability, although it's optional.
215-223: Sequential calls to compute and instantiate detailed lines
The two-step approach (calculate & then instantiate lines) is straightforward. Ensure intermediate results are not needed elsewhere to reduce complexity.
225-228: Reusing child line IDs
Good approach to maintain continuity in child line references. RenamingchildrenWithIDReuseto a more descriptive term might improve clarity (e.g.,reusedChildLines).
247-266: Switch-based pricer instantiation
The switch is fine. If price types grow, consider a map-based or factory-based approach for maintainability. Otherwise, no blocking issues.
268-281: Delegation to pricer
Cleanly delegates final calculation to the pricer. Logging the calculation outcome might aid debugging, but it’s optional.openmeter/billing/service/lineservice/pricegraduatedtiered_test.go (2)
16-115: Test:TestTieredGraduatedCalculation
This suite comprehensively checks various mid- and last-in-period scenarios, ensuring correct usage of tiered pricing. Consider verifying edge cases with zero usage, negative usage (if ever possible), and extremely large usage for extra coverage.
147-221: Minimum spend scenarios
The tests confirm proper line creation for minimum spends with zero usage as well as partial usage. This coverage is strong. Make sure you also cover boundary conditions around exactly meeting the minimum amount.openmeter/billing/service/lineservice/pricer.go (2)
15-20:PricerCalculateInputfields
Encapsulating usage-based line, pre-line usage, and line usage sets a clear structure for pricer computations. Ensure that conversion/truncation logic (if needed) is handled consistently.
30-38:ProgressiveBillingPricer
Allows invoicing if current time is on or after start time. This is consistent with typical partial billing logic. Consider clarifying whether partial usage queries need additional checks here, though.openmeter/billing/service/lineservice/pricegraduatedtiered.go (5)
12-14: Validate embedding ofProgressiveBillingPricer.
EmbeddingProgressiveBillingPricerhere might expose methods or properties that aren’t strictly needed bygraduatedTieredPricer. Ensure you want this “is-a” relationship rather than a “has-a.”
18-110: Revisit complex inline logic inCalculatemethod.
TheCalculatemethod is quite comprehensive, directly handling multiple pricing scenarios (e.g., usage price, flat price, discount application, minimum spend). While it appears functional, consider splitting out some logic (e.g., discount logic or fee logic) to smaller helper functions to improve maintainability and testability.
113-120: Enhance struct documentation fortierRange.
Clarify the meaning ofAtTierBoundaryand whetherFromQtyis inclusive or exclusive in a doc comment. This will help future maintainers.
122-128: Document thetierCallbackInputfields.
Adding doc comments for fields likeQuantityandPreviousTotalAmountwould help clarify their roles during tier processing.
223-326: Fix minor spelling and consider modularizing callback flow.
Line 281 in the comment says “cummulative” instead of “cumulative.” Also, the tier callback flow is carefully integrated, but you might reduce complexity by modularizing repeated logic branches (e.g., repeated rounding calls).Apply this diff to fix the spelling:
-// Now that we have the ranges, let's iterate over the ranges and calculate the cummulative total amount +// Now that we have the ranges, let's iterate over the ranges and calculate the cumulative total amount
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
openmeter/billing/errors.go(1 hunks)openmeter/billing/service/lineservice/pricedynamic.go(1 hunks)openmeter/billing/service/lineservice/pricedynamic_test.go(1 hunks)openmeter/billing/service/lineservice/priceflat.go(1 hunks)openmeter/billing/service/lineservice/priceflat_test.go(1 hunks)openmeter/billing/service/lineservice/pricegraduatedtiered.go(1 hunks)openmeter/billing/service/lineservice/pricegraduatedtiered_test.go(1 hunks)openmeter/billing/service/lineservice/pricepacakage.go(1 hunks)openmeter/billing/service/lineservice/pricepacakge_test.go(1 hunks)openmeter/billing/service/lineservice/pricer.go(1 hunks)openmeter/billing/service/lineservice/pricetiered.go(1 hunks)openmeter/billing/service/lineservice/priceunit.go(1 hunks)openmeter/billing/service/lineservice/priceunit_test.go(1 hunks)openmeter/billing/service/lineservice/pricevolumetiered.go(1 hunks)openmeter/billing/service/lineservice/pricevolumetiered_test.go(1 hunks)openmeter/billing/service/lineservice/usagebasedline.go(3 hunks)openmeter/billing/service/lineservice/usagebasedline_test.go(1 hunks)test/billing/invoice_test.go(6 hunks)
🧰 Additional context used
🧬 Code Definitions (17)
openmeter/billing/errors.go (1)
openmeter/billing/validationissue.go (1)
NewValidationError(75-81)
openmeter/billing/service/lineservice/pricetiered.go (2)
openmeter/billing/service/lineservice/pricer.go (2)
Pricer(22-28)PricerCalculateInput(15-20)openmeter/productcatalog/price.go (2)
VolumeTieredPrice(448-448)GraduatedTieredPrice(450-450)
test/billing/invoice_test.go (4)
pkg/slicesx/map.go (1)
Map(6-19)openmeter/billing/invoiceline.go (2)
Line(278-291)Period(82-85)openmeter/billing/errors.go (1)
ErrInvoiceProgressiveBillingNotSupported(38-38)api/client/go/client.gen.go (1)
Period(4325-4331)
openmeter/billing/service/lineservice/priceunit.go (4)
openmeter/billing/service/lineservice/pricer.go (4)
ProgressiveBillingPricer(30-30)ProgressiveBillingPricer(32-38)Pricer(22-28)PricerCalculateInput(15-20)openmeter/ent/db/billinginvoiceflatfeelineconfig/where.go (1)
PerUnitAmount(74-76)openmeter/billing/service/lineservice/usagebasedline.go (2)
UnitPriceUsageChildUniqueReferenceID(27-27)UnitPriceMinSpendChildUniqueReferenceID(28-28)openmeter/productcatalog/price.go (2)
InArrearsPaymentTerm(21-21)Commitments(687-693)
openmeter/billing/service/lineservice/pricepacakge_test.go (5)
openmeter/productcatalog/price.go (2)
NewPriceFrom(338-360)Commitments(687-693)openmeter/billing/service/lineservice/usagebasedline.go (2)
MinSpendChildUniqueReferenceID(21-21)UsageChildUniqueReferenceID(20-20)openmeter/billing/invoiceline.go (1)
FlatFeeCategoryCommitment(248-248)openmeter/productcatalog/discount.go (1)
Discounts(346-346)openmeter/billing/invoicelinediscount.go (5)
NewLineDiscounts(625-627)AmountLineDiscount(100-108)LineDiscountBase(62-69)LineMaximumSpendReferenceID(21-21)LineDiscountReasonMaximumSpend(27-27)
openmeter/billing/service/lineservice/priceunit_test.go (5)
openmeter/productcatalog/price.go (2)
NewPriceFrom(338-360)Commitments(687-693)openmeter/billing/service/lineservice/usagebasedline.go (2)
UnitPriceMinSpendChildUniqueReferenceID(28-28)UnitPriceUsageChildUniqueReferenceID(27-27)openmeter/billing/invoiceline.go (1)
FlatFeeCategoryCommitment(248-248)openmeter/productcatalog/discount.go (1)
Discounts(346-346)openmeter/billing/invoicelinediscount.go (6)
NewLineDiscounts(625-627)NewLineDiscountFrom(334-361)AmountLineDiscount(100-108)LineDiscountBase(62-69)LineMaximumSpendReferenceID(21-21)LineDiscountReasonMaximumSpend(27-27)
openmeter/billing/service/lineservice/priceflat_test.go (2)
openmeter/productcatalog/price.go (3)
NewPriceFrom(338-360)InAdvancePaymentTerm(20-20)InArrearsPaymentTerm(21-21)openmeter/billing/service/lineservice/usagebasedline.go (1)
FlatPriceChildUniqueReferenceID(25-25)
openmeter/billing/service/lineservice/priceflat.go (3)
openmeter/billing/service/lineservice/pricer.go (4)
NonProgressiveBillingPricer(40-40)NonProgressiveBillingPricer(42-58)Pricer(22-28)PricerCalculateInput(15-20)openmeter/productcatalog/price.go (2)
InAdvancePaymentTerm(20-20)InArrearsPaymentTerm(21-21)openmeter/billing/service/lineservice/usagebasedline.go (1)
FlatPriceChildUniqueReferenceID(25-25)
openmeter/billing/service/lineservice/pricedynamic.go (4)
openmeter/billing/service/lineservice/pricer.go (4)
ProgressiveBillingPricer(30-30)ProgressiveBillingPricer(32-38)Pricer(22-28)PricerCalculateInput(15-20)openmeter/ent/db/billinginvoiceflatfeelineconfig/where.go (1)
PerUnitAmount(74-76)openmeter/billing/service/lineservice/usagebasedline.go (2)
UsageChildUniqueReferenceID(20-20)MinSpendChildUniqueReferenceID(21-21)openmeter/productcatalog/price.go (2)
InArrearsPaymentTerm(21-21)Commitments(687-693)
openmeter/billing/service/lineservice/pricedynamic_test.go (5)
openmeter/productcatalog/price.go (2)
NewPriceFrom(338-360)Commitments(687-693)openmeter/billing/service/lineservice/usagebasedline.go (3)
DecimalOne(42-42)MinSpendChildUniqueReferenceID(21-21)UsageChildUniqueReferenceID(20-20)openmeter/billing/invoiceline.go (1)
FlatFeeCategoryCommitment(248-248)openmeter/productcatalog/discount.go (1)
Discounts(346-346)openmeter/billing/invoicelinediscount.go (6)
NewLineDiscounts(625-627)NewLineDiscountFrom(334-361)AmountLineDiscount(100-108)LineDiscountBase(62-69)LineMaximumSpendReferenceID(21-21)LineDiscountReasonMaximumSpend(27-27)
openmeter/billing/service/lineservice/pricevolumetiered.go (5)
openmeter/billing/service/lineservice/pricer.go (4)
NonProgressiveBillingPricer(40-40)NonProgressiveBillingPricer(42-58)Pricer(22-28)PricerCalculateInput(15-20)openmeter/productcatalog/price.go (1)
VolumeTieredPrice(448-448)openmeter/billing/errors.go (2)
ErrInvoiceLineVolumeSplitNotSupported(45-45)ErrInvoiceLineMissingOpenEndedTier(47-47)openmeter/billing/service/lineservice/usagebasedline.go (3)
VolumeFlatPriceChildUniqueReferenceID(33-33)VolumeUnitPriceChildUniqueReferenceID(34-34)VolumeMinSpendChildUniqueReferenceID(35-35)openmeter/billing/invoiceline.go (1)
FlatFeeCategoryCommitment(248-248)
openmeter/billing/service/lineservice/pricepacakage.go (3)
openmeter/billing/service/lineservice/pricer.go (4)
ProgressiveBillingPricer(30-30)ProgressiveBillingPricer(32-38)Pricer(22-28)PricerCalculateInput(15-20)openmeter/billing/service/lineservice/usagebasedline.go (3)
UsageChildUniqueReferenceID(20-20)MinSpendChildUniqueReferenceID(21-21)DecimalOne(42-42)openmeter/productcatalog/price.go (1)
InArrearsPaymentTerm(21-21)
openmeter/billing/service/lineservice/pricevolumetiered_test.go (3)
openmeter/productcatalog/price.go (3)
PriceTierFlatPrice(651-654)PriceTierUnitPrice(670-673)NewPriceFrom(338-360)openmeter/billing/service/lineservice/usagebasedline.go (3)
VolumeFlatPriceChildUniqueReferenceID(33-33)VolumeUnitPriceChildUniqueReferenceID(34-34)VolumeMinSpendChildUniqueReferenceID(35-35)openmeter/billing/invoicelinediscount.go (3)
NewLineDiscounts(625-627)NewLineDiscountFrom(334-361)AmountLineDiscount(100-108)
openmeter/billing/service/lineservice/usagebasedline_test.go (3)
openmeter/ent/db/billinginvoicelinediscount/where.go (2)
Quantity(116-118)PreLinePeriodQuantity(121-123)openmeter/ent/db/billinginvoiceline/where.go (1)
Quantity(168-170)openmeter/ent/db/billinginvoiceusagebasedlineconfig/where.go (1)
PreLinePeriodQuantity(78-80)
openmeter/billing/service/lineservice/pricegraduatedtiered_test.go (4)
openmeter/productcatalog/price.go (5)
PriceTierFlatPrice(651-654)PriceTierUnitPrice(670-673)NewPriceFrom(338-360)GraduatedTieredPrice(450-450)Commitments(687-693)openmeter/billing/service/lineservice/usagebasedline.go (1)
GraduatedMinSpendChildUniqueReferenceID(39-39)openmeter/billing/invoiceline.go (1)
FlatFeeCategoryCommitment(248-248)openmeter/billing/invoicelinediscount.go (3)
NewLineDiscounts(625-627)NewLineDiscountFrom(334-361)AmountLineDiscount(100-108)
openmeter/billing/service/lineservice/usagebasedline.go (1)
openmeter/billing/service/lineservice/pricer.go (2)
PricerCalculateInput(15-20)Pricer(22-28)
openmeter/billing/service/lineservice/pricegraduatedtiered.go (4)
openmeter/billing/service/lineservice/pricer.go (4)
ProgressiveBillingPricer(30-30)ProgressiveBillingPricer(32-38)Pricer(22-28)PricerCalculateInput(15-20)openmeter/productcatalog/price.go (2)
GraduatedTieredPrice(450-450)InArrearsPaymentTerm(21-21)openmeter/billing/service/lineservice/usagebasedline.go (3)
GraduatedTieredPriceUsageChildUniqueReferenceID(37-37)GraduatedTieredFlatPriceChildUniqueReferenceID(38-38)GraduatedMinSpendChildUniqueReferenceID(39-39)openmeter/billing/invoiceline.go (1)
FlatFeeCategoryCommitment(248-248)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Test
- GitHub Check: Quickstart
- GitHub Check: Lint
- GitHub Check: E2E
- GitHub Check: Developer environment
- GitHub Check: CI
- GitHub Check: Commit hooks
- GitHub Check: Build
- GitHub Check: Analyze (go)
🔇 Additional comments (51)
openmeter/billing/errors.go (1)
35-42: New invoice-related error constants look consistent.
These additions align well with existing validation error patterns and naming schemes. It would be good to ensure they are covered by relevant unit or integration tests, but there are no immediate issues from a correctness or maintainability standpoint.test/billing/invoice_test.go (8)
1811-1811: Confirmed correct line count check.
This test assertion correctly verifies the expected number of lines.
1819-1820: Helpful clarifying comment.
These new comments make it clearer which items are meant to be invoiced in arrears.
1951-1951: Appropriate error handling for unsupported progressive billing.
Switching the requirement to checkErrInvoiceProgressiveBillingNotSupportedis consistent with the new logic introduced in the pricer.
1970-1970: Correct assertion verifying updated lines.
Ensuring that exactly two lines remain is consistent with scenarios where one line was removed or updated earlier in the test.
2108-2108: Good test coverage for multiple line items.
This assertion helps confirm that two lines appear in this scenario.
2116-2117: Additional clarifying comments regarding invoice lines in arrears.
No functional changes here, but these comments improve overall readability.
2303-2305: Clear explanation of non-prorated flat fees.
These comments provide a concise rationale for why certain lines do not receive a parent reference.
2320-2320: Straightforward iteration update.
This loop restricts processing to the relevant lines, reflecting the new logic surrounding usage-based lines. Looks good.openmeter/billing/service/lineservice/pricetiered.go (2)
10-15: Good implementation of the router pattern for tiered pricing.The
tieredPricerstruct elegantly routes pricing calculations to the appropriate pricer based on the pricing mode. This design aligns well with the PR's objective to break down the monolithic UBP line into smaller, more manageable components.
19-33: Clean implementation with proper error handling.The
Calculatemethod correctly:
- Converts the price to tiered format
- Routes to the appropriate pricer based on the mode
- Returns meaningful error messages for both conversion failures and unsupported modes
The router pattern implementation is consistent with the PR objectives of creating more modular pricing components.
openmeter/billing/service/lineservice/priceflat_test.go (6)
10-32: Good test coverage for flat price with no usage.The test properly validates the expected behavior when a flat price is applied with no usage in an in-advance payment scenario. The test structure is clear and consistent with the overall testing approach.
34-54: Appropriate test for in-advance flat pricing with usage.This test correctly verifies that usage doesn't affect flat pricing when payment term is in advance. The flat price is still applied regardless of usage amount, which is the expected behavior.
56-68: Good test for mid-period behavior with in-advance pricing.This test verifies an important edge case: when using in-advance payment terms in the middle of a period, no detailed lines should be created. This aligns with the expectation that in-advance charges only apply to the first line of a period.
70-90: Proper handling of in-arrears flat pricing.The test correctly validates the behavior for in-arrears flat pricing in a single period line scenario. This test case is particularly relevant to the PR objective of addressing the issue where UBP FlatFee lines were incorrectly classified as progressively billable.
92-104: Important edge case for in-arrears mid-period lines.This test verifies that flat fees with in-arrears payment terms are not billed in mid-period lines, which is consistent with the expected behavior that they should only be billed in the last period.
106-126: Comprehensive test case for last-period in-arrears flat pricing.This test completes the coverage by verifying that in-arrears flat prices are applied in the last period line. The comprehensive test suite effectively covers all the timing scenarios for flat pricing, which is important for the refactoring goals of this PR.
openmeter/billing/service/lineservice/pricedynamic_test.go (6)
12-24: Good baseline test for dynamic pricing with no usage.The test correctly verifies that when there's no usage with a dynamic price (and no minimum spend), no detailed lines are created. This establishes the baseline behavior for dynamic pricing.
26-51: Proper verification of minimum spend commitments.This test ensures that minimum spend is honored even when there's no usage, which is a critical aspect of commitment-based pricing. The test properly validates the creation of a line item for minimum spend with the correct payment term (in arrears) and the appropriate billing category.
53-95: Good tests for minimum spend timing logic.These two tests comprehensively verify the timing aspects of minimum spend commitments:
- Mid-period lines should not bill minimum spend
- Only the last line in a period should bill minimum spend
This behavior is essential for preventing duplicate billing of commitments and ensures the PR objective of correctly handling billing timing.
97-138: Thorough testing of usage-based billing.These tests properly verify that usage is billed correctly regardless of the line position within a period. The tests check that:
- The amount is calculated correctly using the multiplier
- The line is categorized as usage
- The payment term is in arrears
These tests ensure the core usage-based pricing functionality works as expected.
140-163: Good verification of maximum spend limit not reached.This test ensures that when a maximum spend is set but not reached, the billing behaves normally with no discount applied. This is an important scenario to test as it validates that maximum spend limits don't affect normal billing when they aren't triggered.
165-201: Comprehensive test for maximum spend discount application.This test thoroughly validates the complex scenario where maximum spend is hit:
- Pre-line period usage is tracked
- Line period usage is calculated
- The total exceeds the maximum spend limit
- A discount is applied with the correct amount and metadata
This test ensures the pricing module correctly implements maximum spend caps, which is a critical billing component.
openmeter/billing/service/lineservice/priceflat.go (2)
10-13: Clear indication of future enhancement for pro-rating support.The TODO comment indicates awareness that full pro-rating support will require changing to
ProgressiveBillingPricer. This aligns with the PR objective of addressing the issue where UBP FlatFee lines were incorrectly classified as progressively billable.
17-47: Good implementation of the flat pricing logic.The
Calculatemethod properly implements the two key scenarios for flat pricing:
- In-advance payments are only billed on the first line of a period
- In-arrears payments are only billed on the last line of a period
This correctly addresses the issue mentioned in the PR objectives where flat fee lines needed special handling.
openmeter/billing/service/lineservice/priceunit_test.go (1)
1-200: Great test coverage for unit price calculation!The test cases comprehensively cover key pricing scenarios including:
- Zero usage cases
- Minimum spend handling across different line positions
- Usage calculation with various quantities
- Maximum spend thresholds and discount application
The test structure is clean and follows best practices with descriptive test names and clear expected outputs for each case.
openmeter/billing/service/lineservice/priceunit.go (1)
15-48: Clear implementation of unit price calculation logicThe Calculate method correctly:
- Extracts the unit price from the line configuration
- Handles usage-based pricing when quantities are present
- Properly calculates previous period billing
- Applies commitments (min/max spend) appropriately
The error handling is thorough with meaningful error messages.
openmeter/billing/service/lineservice/pricepacakge_test.go (2)
13-254: Comprehensive test suite for package pricingThe test cases are well-structured and cover essential pricing scenarios:
- Empty usage cases
- Minimum spend validation across different line positions
- Various package quantity calculations
- Maximum spend thresholds and discount application
The test cases properly validate that packages are calculated correctly based on usage quantities.
257-265: Good unit testing for the getNumberOfPackages methodThe test verifies multiple edge cases for package quantity calculation:
- Zero quantity (0 packages)
- Partial package (1 package)
- Exact package size (1 package)
- Package overflow (2 packages)
This ensures the core package calculation logic works correctly.
openmeter/billing/service/lineservice/pricevolumetiered_test.go (5)
14-40: Well-structured test tiers definitionThe test tiers are clearly defined with a mix of flat and unit pricing across different quantity thresholds:
- Tier 1: Flat price of 100 for quantities up to 5
- Tier 2: Flat price of 150 for quantities up to 10
- Tier 3: Unit price of 10 for quantities up to 15
- Tier 4: Unit price of 5 for all quantities above 15
This provides a good foundation for testing various tier calculation scenarios.
42-228: Thorough testing of volume-based tier selectionThe tests effectively verify:
- Correct tier selection based on usage quantity
- Proper boundary condition handling at tier transitions
- Appropriate billing format (flat vs unit price) based on tier type
All tests appropriately validate the expected behavior for each scenario.
232-346: Comprehensive minimum spend validation testsThe test cases verify that:
- Minimum spend is honored when usage is below threshold
- Additional commitment line is created when needed
- Appropriate calculation of delta between actual usage and minimum spend
The tests cover various edge cases including exact minimum spend matches.
349-407: Solid maximum spend limit testingThe tests effectively verify:
- Maximum spend is properly enforced
- Discounts are correctly applied when usage exceeds maximum spend
- Appropriate handling of maximum spend thresholds that don't align with tier boundaries
The discount calculation and application logic is thoroughly tested.
410-470: Good unit tests for tier selection logicThe
TestFindTierForQuantityfunction effectively validates that:
- Correct tier is selected for quantities within various tier ranges
- Tier boundaries are handled properly (e.g., quantity exactly at the boundary)
- Large quantities beyond defined thresholds select the final tier
This ensures the core tier selection logic works as expected.
openmeter/billing/service/lineservice/pricevolumetiered.go (1)
1-13: Well-structured initialization ofvolumeTieredPricer.Indices and imports appear consistent, and embedding
NonProgressiveBillingPriceraligns with the intended design.openmeter/billing/service/lineservice/pricepacakage.go (1)
19-22: Good error handling.Converting to a package price is clearly guarded by proper error handling, improving reliability.
openmeter/billing/service/lineservice/usagebasedline.go (4)
88-92: Use ofgetPricer()for progressive billing
No immediate issues. This approach cleanly separates the logic for pricer retrieval and allows for structured error handling.
99-102: Returnnil, nilinstead of raising an error
Returningnil, nilis appropriate if the calling code interprets “cannot be invoiced” as a non-error scenario. Confirm that upstream callers expect this behavior and handle it properly.
105-105: Refined comment
The inline comment is clear, no concerns.
235-245: Validation for required quantities
The check ensures quantity fields are set before pricer calculations. Consider returning a custom error type if you want more specific parsing upstream, but this is acceptable.openmeter/billing/service/lineservice/pricegraduatedtiered_test.go (5)
1-14: File-level setup and imports
Test file is well-structured with relevant imports and packages. No issues spotted here.
117-146: Test: verifying multiple tier usage in mid period
Logic covers partial usage crossing tier boundaries. The approach looks robust. No further issues noted.
223-283: Maximum spend scenarios
Tests correctly validate discount application once usage crosses the maximum spend threshold. The discount item lines are well-structured. Great coverage of boundary cross.
286-547: Test:TestTieredPriceCalculator- tier calculations
Extensive coverage of tier-by-tier partial usage (and no usage) scenarios. The usage of callback mocks ensures logic is validated thoroughly. The dynamic tier boundary validations are clear.
549-593: Helper functions and mock callbacks
Good separation of concerns for finalizing amounts, introspecting tiers, and verifying callback logic. This ensures reliable debugging of tier transitions.openmeter/billing/service/lineservice/pricer.go (3)
1-13: Added type definitions
ThePricerCapabilitiesstruct is straightforward; no concerns. ThePricerCalculateInputstruct is well-defined for usage-based lines.
22-28:Pricerinterface
Defines the contract for progressive or non-progressive pricers, enabling flexible expansions. The minimal interface is appropriate.
40-58:NonProgressiveBillingPricer
The handling of parent lines as an error scenario clarifies the “no progressive billing” stance. This logic is correct and returns a specialized error as needed.openmeter/billing/service/lineservice/pricegraduatedtiered.go (3)
1-2: File structure and naming look good.
The package declaration and blank line follow standard Go conventions. No issues here.
144-170: Robust validation checks.
TheValidate()method thoroughly checks for malformed input (negative quantities, currency presence, price mode). This reduces unexpected runtime errors. Good job.
172-221: Keep an eye on boundary splits insplitTierRangeAtBoundary.
The logic for splitting ranges is intricate but appears consistent with your tiering approach. Add unit tests for edge cases wherefrom == toor nearly so, ensuring boundary conditions behave correctly.Would you like a script to locate or generate tests specifically addressing boundary splits?
turip
force-pushed
the
refactor/billing-pricer-interface
branch
from
8fe18e2 to
b7c7703
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (12)
openmeter/billing/service/lineservice/pricevolumetiered.go (2)
33-35: Check error message consistency.
billing.ErrInvoiceLineVolumeSplitNotSupportedreferences "graduated" in its message, but here it's used for volume-tiered pricing. Consider updating the error definition or message to avoid confusion.- return nil, billing.ErrInvoiceLineVolumeSplitNotSupported + return nil, billing.ErrInvoiceLineVolumeSplitNotSupported // modify error message to reflect volume-tiered
48-65: Validate discount application order for flat and unit lines.
Both flat and unit price lines apply maximum spend discounts sequentially, possibly causing different discount amounts depending on line order. If the discount logic for multiple lines is intended to prioritize certain charges first, consider clarifying or enforcing an explicit order.Also applies to: 67-85
openmeter/billing/service/lineservice/usagebasedline.go (1)
88-105: Ensure pricer compatibility with meter checks.
Afterpricer.CanBeInvoicedAsOfreturns true, you further validate the meter. Confirm that any decision from the pricer is consistent with the meter logic (e.g., if the pricer says “invoice now” but the meter forbids it). Consider logging or clarifying any conflicting signals.openmeter/billing/service/lineservice/usagebasedline_test.go (5)
1811-1811: Enhance test assertion messages.While
require.Len(s.T(), invoiceLines, 2)is good for correctness, adding a descriptive message parameter (e.g.,"expected two invoice lines at mid-period") can ease debugging when the assertion fails.
1819-1820: Fix spelling in comment.There is a small spelling issue: "arreas" should be "arrears".
Apply this diff to correct the spelling:
-// - the volume item as that must be invoiced in arreas -// - the flat per usage item as that is invoiced in arreas (no pro-rating) +// - the volume item as that must be invoiced in arrears +// - the flat per usage item as that is invoiced in arrears (no pro-rating)
1970-1970: Optional: Add assertion message.
require.Len(s.T(), updatedInvoice.Lines.MustGet(), 2)could include a description (e.g.,"updated invoice should carry two line items") for clarity.
2108-2108: Optional: Add assertion message.Adding a small description, like
"should have exactly two invoice lines at this stage", would improve test clarity here as well.
2290-2291: Consider removing or gating debugging statements.
s.DebugDumpInvoice("end of period invoice", out[0])is helpful for diagnostics but might clutter logs in production. If feasible, consider wrapping it with a debug flag or removing it once the code is stable.openmeter/billing/service/lineservice/pricegraduatedtiered.go (4)
226-327: Consider breaking down the complex tiered price calculator methodThe
tieredPriceCalculatormethod is quite long and handles multiple responsibilities. Consider breaking it down into smaller, more focused methods to improve readability and maintainability.For example, you could extract the range creation logic (lines 232-280) and the range processing logic (lines 282-318) into separate methods:
-func (p graduatedTieredPricer) tieredPriceCalculator(in tieredPriceCalculatorInput) error { +func (p graduatedTieredPricer) tieredPriceCalculator(in tieredPriceCalculatorInput) error { // Note: this is not the most efficient algorithm, but it is at least pseudo-readable if err := in.Validate(); err != nil { return err } + + qtyRanges := p.createTierRanges(in) + + if in.IntrospectRangesFn != nil { + in.IntrospectRangesFn(qtyRanges) + } + + total, err := p.processTierRanges(in, qtyRanges) + if err != nil { + return err + } + + if in.FinalizerFn != nil { + if err := in.FinalizerFn(total); err != nil { + return err + } + } + + return nil +} + +func (p graduatedTieredPricer) createTierRanges(in tieredPriceCalculatorInput) []tierRange { // Let's break up the tiers and the input data into a sequence of periods, for easier processing // Invariant of the qtyRanges: // - Non overlapping ranges // - The ranges are sorted by the from quantity // - There is always one range for which range.From == in.FromQty // - There is always one range for which range.ToQty == in.ToQty qtyRanges := make([]tierRange, 0, len(in.TieredPrice.Tiers)+2) previousTierQty := alpacadecimal.Zero for idx, tier := range in.TieredPrice.WithSortedTiers().Tiers { if previousTierQty.GreaterThanOrEqual(in.ToQty) { // We already have enough data to bill for this tiered price break } // Given that the previous tier's max qty was less than then in.ToQty, toQty will fall into the // open ended tier, so we can safely use it as the upper bound tierUpperBound := in.ToQty if tier.UpToAmount != nil { tierUpperBound = *tier.UpToAmount } input := tierRange{ Tier: tier, TierIndex: idx, AtTierBoundary: true, FromQty: previousTierQty, ToQty: tierUpperBound, } qtyRanges = append(qtyRanges, p.splitTierRangeAtBoundary(in.FromQty, in.ToQty, input)...) previousTierQty = tierUpperBound } if in.ToQty.Equal(alpacadecimal.Zero) { // We need to add the first range, in case there's a flat price component qtyRanges = append(qtyRanges, tierRange{ Tier: in.TieredPrice.Tiers[0], TierIndex: 0, AtTierBoundary: true, FromQty: alpacadecimal.Zero, ToQty: alpacadecimal.Zero, }) } + + return qtyRanges +} + +func (p graduatedTieredPricer) processTierRanges(in tieredPriceCalculatorInput, qtyRanges []tierRange) (alpacadecimal.Decimal, error) { // Now that we have the ranges, let's iterate over the ranges and calculate the cummulative total amount // and call the callback for each in-scope range total := alpacadecimal.Zero shouldEmitCallbacks := false for _, qtyRange := range qtyRanges { if qtyRange.FromQty.Equal(in.FromQty) { shouldEmitCallbacks = true } if shouldEmitCallbacks && in.TierCallbackFn != nil { err := in.TierCallbackFn(tierCallbackInput{ Tier: qtyRange.Tier, TierIndex: qtyRange.TierIndex, Quantity: qtyRange.ToQty.Sub(qtyRange.FromQty), PreviousTotalAmount: total, AtTierBoundary: qtyRange.AtTierBoundary, }) if err != nil { - return err + return alpacadecimal.Zero, err } } // Let's update totals if qtyRange.Tier.FlatPrice != nil && qtyRange.AtTierBoundary { total = total.Add(in.Currency.RoundToPrecision(qtyRange.Tier.FlatPrice.Amount)) } if qtyRange.Tier.UnitPrice != nil { total = total.Add(in.Currency.RoundToPrecision(qtyRange.ToQty.Sub(qtyRange.FromQty).Mul(qtyRange.Tier.UnitPrice.Amount))) } // We should only calculate totals up to in.ToQty (given tiers are open-ended we cannot have a full upper bound // either ways) if qtyRange.ToQty.GreaterThanOrEqual(in.ToQty) { break } } - - if in.FinalizerFn != nil { - if err := in.FinalizerFn(total); err != nil { - return err - } - } - - return nil + return total, nil }
31-106: Consider returning detailed lines from tieredPriceCalculator instead of using callbacksThe current approach using callback functions to build the
outslice makes the code harder to follow. Consider refactoring to have thetieredPriceCalculatorreturn the detailed lines directly.This would result in cleaner, more straightforward code and improve testability.
29-29: Consider increasing the initial capacity of the output sliceThe current allocation assumes at most one line per tier, but you may add more (unit price, flat price, and minimum spend). Consider increasing the capacity estimation to avoid reallocations.
-out := make(newDetailedLinesInput, 0, len(price.Tiers)) +out := make(newDetailedLinesInput, 0, len(price.Tiers)*2+1) // Account for unit price, flat price per tier, plus min spend
173-222: Splitting tier ranges looks good but needs more commentsThe
splitTierRangeAtBoundarymethod is complex but appears correct. Consider adding more detailed comments to explain the splitting logic, especially the conditions that trigger the splits.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
openmeter/billing/errors.go(1 hunks)openmeter/billing/service/lineservice/pricedynamic.go(1 hunks)openmeter/billing/service/lineservice/pricedynamic_test.go(1 hunks)openmeter/billing/service/lineservice/priceflat.go(1 hunks)openmeter/billing/service/lineservice/priceflat_test.go(1 hunks)openmeter/billing/service/lineservice/pricegraduatedtiered.go(1 hunks)openmeter/billing/service/lineservice/pricegraduatedtiered_test.go(1 hunks)openmeter/billing/service/lineservice/pricepackage.go(1 hunks)openmeter/billing/service/lineservice/pricepackage_test.go(1 hunks)openmeter/billing/service/lineservice/pricer.go(1 hunks)openmeter/billing/service/lineservice/pricetiered.go(1 hunks)openmeter/billing/service/lineservice/priceunit.go(1 hunks)openmeter/billing/service/lineservice/priceunit_test.go(1 hunks)openmeter/billing/service/lineservice/pricevolumetiered.go(1 hunks)openmeter/billing/service/lineservice/pricevolumetiered_test.go(1 hunks)openmeter/billing/service/lineservice/usagebasedline.go(3 hunks)openmeter/billing/service/lineservice/usagebasedline_test.go(1 hunks)test/billing/invoice_test.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- openmeter/billing/errors.go
- openmeter/billing/service/lineservice/priceflat.go
- openmeter/billing/service/lineservice/pricedynamic.go
- openmeter/billing/service/lineservice/priceunit.go
- openmeter/billing/service/lineservice/priceflat_test.go
- openmeter/billing/service/lineservice/pricetiered.go
- openmeter/billing/service/lineservice/pricegraduatedtiered_test.go
🧰 Additional context used
🧬 Code Definitions (10)
openmeter/billing/service/lineservice/pricedynamic_test.go (5)
openmeter/productcatalog/price.go (1)
NewPriceFrom(338-360)openmeter/billing/service/lineservice/usagebasedline.go (3)
DecimalOne(42-42)MinSpendChildUniqueReferenceID(21-21)UsageChildUniqueReferenceID(20-20)openmeter/billing/invoiceline.go (1)
FlatFeeCategoryCommitment(248-248)openmeter/productcatalog/discount.go (1)
Discounts(346-346)openmeter/billing/invoicelinediscount.go (6)
NewLineDiscounts(625-627)NewLineDiscountFrom(334-361)AmountLineDiscount(100-108)LineDiscountBase(62-69)LineMaximumSpendReferenceID(21-21)LineDiscountReasonMaximumSpend(27-27)
openmeter/billing/service/lineservice/pricepackage_test.go (5)
openmeter/productcatalog/price.go (1)
NewPriceFrom(338-360)openmeter/billing/service/lineservice/usagebasedline.go (2)
MinSpendChildUniqueReferenceID(21-21)UsageChildUniqueReferenceID(20-20)openmeter/billing/invoiceline.go (1)
FlatFeeCategoryCommitment(248-248)openmeter/productcatalog/discount.go (1)
Discounts(346-346)openmeter/billing/invoicelinediscount.go (6)
NewLineDiscounts(625-627)NewLineDiscountFrom(334-361)AmountLineDiscount(100-108)LineDiscountBase(62-69)LineMaximumSpendReferenceID(21-21)LineDiscountReasonMaximumSpend(27-27)
openmeter/billing/service/lineservice/pricevolumetiered_test.go (4)
openmeter/productcatalog/price.go (4)
PriceTierFlatPrice(651-654)PriceTierUnitPrice(670-673)NewPriceFrom(338-360)Commitments(687-693)openmeter/billing/service/lineservice/usagebasedline.go (3)
VolumeFlatPriceChildUniqueReferenceID(33-33)VolumeUnitPriceChildUniqueReferenceID(34-34)VolumeMinSpendChildUniqueReferenceID(35-35)openmeter/billing/invoiceline.go (1)
FlatFeeCategoryCommitment(248-248)openmeter/billing/invoicelinediscount.go (3)
NewLineDiscounts(625-627)AmountLineDiscount(100-108)LineDiscountBase(62-69)
openmeter/billing/service/lineservice/priceunit_test.go (5)
openmeter/productcatalog/price.go (2)
NewPriceFrom(338-360)Commitments(687-693)openmeter/billing/service/lineservice/usagebasedline.go (2)
UnitPriceMinSpendChildUniqueReferenceID(28-28)UnitPriceUsageChildUniqueReferenceID(27-27)openmeter/billing/invoiceline.go (1)
FlatFeeCategoryCommitment(248-248)openmeter/productcatalog/discount.go (1)
Discounts(346-346)openmeter/billing/invoicelinediscount.go (6)
NewLineDiscounts(625-627)NewLineDiscountFrom(334-361)AmountLineDiscount(100-108)LineDiscountBase(62-69)LineMaximumSpendReferenceID(21-21)LineDiscountReasonMaximumSpend(27-27)
openmeter/billing/service/lineservice/pricepackage.go (3)
openmeter/billing/service/lineservice/pricer.go (2)
Pricer(23-29)PricerCalculateInput(16-21)openmeter/billing/service/lineservice/usagebasedline.go (3)
UsageChildUniqueReferenceID(20-20)MinSpendChildUniqueReferenceID(21-21)DecimalOne(42-42)openmeter/productcatalog/price.go (2)
InArrearsPaymentTerm(21-21)Commitments(687-693)
openmeter/billing/service/lineservice/pricegraduatedtiered.go (4)
openmeter/billing/service/lineservice/pricer.go (4)
ProgressiveBillingPricer(31-31)ProgressiveBillingPricer(33-39)Pricer(23-29)PricerCalculateInput(16-21)openmeter/productcatalog/price.go (1)
GraduatedTieredPrice(450-450)openmeter/billing/service/lineservice/usagebasedline.go (3)
GraduatedTieredPriceUsageChildUniqueReferenceID(37-37)GraduatedTieredFlatPriceChildUniqueReferenceID(38-38)GraduatedMinSpendChildUniqueReferenceID(39-39)openmeter/billing/invoiceline.go (1)
FlatFeeCategoryCommitment(248-248)
openmeter/billing/service/lineservice/usagebasedline.go (1)
openmeter/billing/service/lineservice/pricer.go (2)
PricerCalculateInput(16-21)Pricer(23-29)
openmeter/billing/service/lineservice/usagebasedline_test.go (3)
openmeter/ent/db/billinginvoicelinediscount/where.go (2)
Quantity(116-118)PreLinePeriodQuantity(121-123)openmeter/ent/db/billinginvoiceline/where.go (1)
Quantity(168-170)openmeter/ent/db/billinginvoiceusagebasedlineconfig/where.go (1)
PreLinePeriodQuantity(78-80)
openmeter/billing/service/lineservice/pricevolumetiered.go (5)
openmeter/billing/service/lineservice/pricer.go (4)
NonProgressiveBillingPricer(41-41)NonProgressiveBillingPricer(43-59)Pricer(23-29)PricerCalculateInput(16-21)openmeter/productcatalog/price.go (1)
VolumeTieredPrice(448-448)openmeter/billing/errors.go (2)
ErrInvoiceLineVolumeSplitNotSupported(45-45)ErrInvoiceLineMissingOpenEndedTier(47-47)openmeter/billing/service/lineservice/usagebasedline.go (3)
VolumeFlatPriceChildUniqueReferenceID(33-33)VolumeUnitPriceChildUniqueReferenceID(34-34)VolumeMinSpendChildUniqueReferenceID(35-35)openmeter/billing/invoiceline.go (1)
FlatFeeCategoryCommitment(248-248)
openmeter/billing/service/lineservice/pricer.go (2)
openmeter/ent/db/billinginvoiceline/where.go (1)
ParentLineID(142-144)openmeter/billing/errors.go (2)
ValidationError(116-116)ErrInvoiceProgressiveBillingNotSupported(38-38)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: E2E
- GitHub Check: CI
- GitHub Check: Developer environment
- GitHub Check: Analyze (go)
🔇 Additional comments (70)
openmeter/billing/service/lineservice/pricedynamic_test.go (8)
14-25: Test case properly handles no usage scenarioThe test correctly verifies that when there is no usage, the expected output is an empty detailed lines input, which is an important base case for the dynamic pricing logic.
28-52: Minimum spend commitment correctly enforcedThis test validates that the minimum spend is honored even when there is no usage, which is a critical business rule. The expected output correctly includes a line item for the minimum spend with appropriate reference ID and payment term.
55-69: Mid-period behavior with minimum spend is validatedThe test correctly verifies that minimum spend is not billed for mid-period lines, which aligns with the expected billing behavior where minimum spend commitments are only evaluated at the end of the period.
72-96: Last-in-period handling of minimum spend is verifiedThis test ensures that minimum spend is properly applied for the last line in a period, even with no usage, which is an important edge case for commitment billing.
99-118: Usage billing accurately testedThe test verifies that usage is calculated correctly with the multiplier applied. The expected per unit amount of 13.33 matches the calculation of 10 * 1.33333, demonstrating proper rounding behavior.
120-139: Mid-period usage billing verifiedThis test ensures that usage is billed correctly regardless of line position, which is important for consistent billing across split periods.
142-164: Maximum spend limit respected when not reachedThe test validates that when a maximum spend is set but not hit, billing proceeds normally without any discounts applied.
166-201: Maximum spend discount correctly appliedThis test comprehensively verifies the complex scenario where maximum spend is hit and a discount is applied. The calculation considers both the current period usage and pre-period usage to determine when the maximum spend cap is reached.
openmeter/billing/service/lineservice/priceunit_test.go (8)
13-25: LGTM: Unit price calculation tests cover essential scenariosThis test function follows the same pattern as the dynamic pricing tests, ensuring consistent testing across different pricing models, and properly handles the no usage case.
28-52: Minimum spend validation for unit pricesTest properly verifies that minimum spend commitment is honored even when there's no usage, which is an important business rule for unit pricing.
55-69: Mid-period line behavior with minimum spendThe test correctly verifies that minimum spend is not billed for mid-period lines, ensuring proper billing behavior for split billing periods.
72-96: Last-in-period behavior with minimum spendThis test confirms that minimum spend is properly applied for the last line in a period, which is essential for validating end-of-period commitment calculations.
99-118: Unit price usage billing verifiedTest correctly validates that when usage is present, it's multiplied by the unit price (100 * 10 = 1000) and billed properly. The quantity field reflects the actual usage amount.
120-139: Mid-period usage billingThis test ensures that usage is billed consistently in mid-period line modes, which is important for split billing periods.
142-164: Maximum spend validation when not reachedTest correctly verifies that when maximum spend is set but not hit, billing proceeds normally without discounts.
166-200: Maximum spend discount properly calculatedThis test validates that when maximum spend is exceeded, the appropriate discount is applied. The calculation properly accounts for both current and previous line period usage to determine when the cap is reached.
openmeter/billing/service/lineservice/pricepackage_test.go (11)
14-27: LGTM: Package pricing tests for no usage scenarioTest correctly verifies the base case where there's no usage, resulting in no detailed line items.
30-55: Package pricing with minimum spend commitmentTest properly validates that minimum spend commitment is honored for package pricing when there's no usage, following the same pattern as other pricing types.
58-73: Mid-period behavior with minimum spendThis test ensures that minimum spend is not billed for mid-period lines in package pricing, consistent with other pricing types.
76-101: Last-in-period with minimum spendTest verifies that minimum spend is properly applied for the last line in a period, maintaining consistency with other pricing models.
104-124: Basic package usage billingThis test validates that when usage exactly matches the package size (10), one package is billed at the correct amount (15).
126-146: Mid-line package billingTest confirms that package pricing works properly for mid-period line modes with full package usage.
148-168: Partial package usage billingThis test validates an important edge case where usage (5) is less than the package size (10), verifying that a full package is still billed even with partial usage.
170-192: Package billing with prior usageTest ensures that package billing correctly accounts for previous usage in the period, which is important for maintaining consistent package billing across split periods.
195-218: Maximum spend handling when not reachedThis test verifies that maximum spend limits don't affect package pricing when the limit isn't reached.
220-255: Maximum spend discount correctly appliedThis test comprehensively validates the complex scenario where package usage exceeds the maximum spend limit, ensuring the proper discount is applied to cap the total cost.
258-266: Package calculation unit testThis test thoroughly verifies the
getNumberOfPackageshelper function with various edge cases, including zero quantity, values below package size, exactly at package size, and values requiring multiple packages.openmeter/billing/service/lineservice/pricevolumetiered_test.go (17)
14-41: LGTM: Comprehensive test tier definitionThe test tiers are well-defined to cover multiple scenarios: flat pricing in early tiers and unit pricing in later tiers with various thresholds.
43-55: Mid-period tiered volume testThis test verifies that no billing occurs for mid-period lines, which is consistent with the expected behavior for tiered volume pricing.
57-77: Last-in-period with no usageThe test correctly validates that when there's no usage in the last line of a period, the flat price for the first tier is applied.
79-97: Unit price in first tierThis test verifies that when only unit pricing is defined (no flat price) and there's no usage, no billing occurs, which is the expected behavior.
99-119: Tier 1 mid usageThe test confirms that usage within the first tier is properly billed using the flat price for that tier.
121-141: Tier 1 at upper boundaryThis test validates that usage exactly at the upper boundary of tier 1 (5 units) is still billed using tier 1's flat price.
143-163: Tier 3 near upper boundaryTest verifies that usage near the upper boundary of tier 3 is billed correctly using the unit price for that tier.
165-185: Tier 3 at exact boundaryThis test validates the edge case where usage is exactly at the upper boundary of tier 3, ensuring it's still billed in tier 3.
187-207: Tier 4 just over boundaryTest confirms that usage just above the tier 3 boundary (16 units) is correctly billed using tier 4's unit price.
209-229: Tier 4 with high usageThis test validates that high usage volumes are properly billed using the appropriate tier (tier 4 in this case).
233-264: Minimum spend with tiered pricingTest verifies that minimum spend commitments are properly applied with tiered pricing, including calculating the difference between tier cost and minimum commitment.
266-289: Usage exceeding minimum spendThis test ensures that when usage exceeds the minimum spend commitment, no additional minimum spend line is added.
291-322: Usage below minimum spendThe test validates that when usage is below the minimum spend commitment, an additional line is added to meet the commitment amount.
324-347: Usage exactly at minimum spendThis test verifies the edge case where usage exactly matches the minimum spend commitment, ensuring no additional minimum spend line is added.
350-373: Maximum spend at tier boundaryThe test validates that when maximum spend exactly matches a tier boundary, pricing is capped appropriately without discounts.
375-408: Maximum spend not aligned with tier boundaryThis test verifies the complex case where maximum spend is not aligned with tier boundaries, ensuring the appropriate discount is applied to cap the total cost.
411-471: Helper function for tier identificationThis test thoroughly validates the
findTierForQuantityhelper function across multiple scenarios, including tier boundaries and values in different ranges, ensuring the correct tier is identified for various usage amounts.openmeter/billing/service/lineservice/pricevolumetiered.go (2)
28-31: Clarify partial billing logic.
Returningnil, nilwhen!l.IsLastInPeriod()means partial usage in mid-period is neither billed nor tracked, which might be acceptable for non-progressive billing strategies. However, confirm this aligns with your business requirements so that no usage is lost inadvertently.
112-125: Ensure infinite tier coverage is properly enforced.
By design, the last tier should haveUpToAmount == nil. If it's missing, an error is returned. This seems correct for volume pricing. However, if there's a scenario in which the last tier incorrectly includes an upper bound, no tier might match large quantities. Ensure the product catalog always includes an open-ended final tier.openmeter/billing/service/lineservice/usagebasedline.go (3)
235-245: Initialization of pricer input looks solid.
The straightforward check for nil quantities before buildingPricerCalculateInputensures no hidden nil-pointer issues.
247-265: Confirm usage ofvolumeTieredPricer.
Althoughpricevolumetiered.godefinesvolumeTieredPricer, thisgetPricer()switch does not reference it. If volume-tiered pricing is expected to appear as a distinct type from standard tiered pricing, you may need an explicit case. Otherwise, ensuretieredPricerindeed covers volume-tiered logic.
268-280: Centralized calculation approach is clean.
This method delegates the entire logic to the pricer’sCalculatefunction, improving modularity and clarity.openmeter/billing/service/lineservice/usagebasedline_test.go (4)
91-95: Consider validatingtc.usagebefore assignment.Here, you're directly assigning the usage-based quantities from
tc.usagebefore invokingcalculateDetailedLines(). It might be prudent to verify thattc.usageis not nil or contains valid data to avoid unexpected nil pointer dereferences. If there's any chance thattc.usagecan be invalid, consider adding defensive checks or explicit test cases for zero/negative usage scenarios.
1951-1951: Good coverage check for unsupported progress billing.Verifying that
ErrInvoiceProgressiveBillingNotSupportedis returned ensures the new validation logic works as intended.
2303-2305: Comment clarity acknowledged.Your comment clarifies why there's no proration for flat prices. The subsequent line assignment to
flatPerUsagealigns with the explanation. This is consistent and readable.
2320-2320: Loop usage looks good.Iterating over
[flatPerUnit, tieredGraduated]is straightforward for the final checks. The usage inside the loop appears consistent with your billing logic.openmeter/billing/service/lineservice/pricer.go (6)
1-9: File structure and imports look clean.The new file sets a clear boundary for pricer logic. The imports are minimal and relevant, with no obvious redundancies.
11-14:PricerCapabilitiesis succinct and purposeful.Exposing a boolean flag for progressive billing is straightforward. The naming is direct, avoiding confusion about usage.
16-21:PricerCalculateInputnaming is clear.Structuring the line’s pre-period and line-period quantities alongside the embedded
usageBasedLineclarifies input data for pricers.
23-29: Interface design is concise and future-proof.Defining
CalculateandCanBeInvoicedAsOfsets a clear contract. This clean separation of responsibilities should aid maintainability.
31-39: Progressive pricer logic partially implemented.
CanBeInvoicedAsOfcorrectly blocks invoicing ifasOfprecedes the line’s period start. However, note that theCalculatemethod is not shown here. If it's implemented elsewhere, ensure integration is tested. Otherwise, consider adding an explicit implementation or a stub to prevent runtime errors.
41-59: Non-progressive pricer logic is consistent with new billing constraints.We see the correct error return for parent lines indicating progressive billing is unsupported. As with the progressive pricer, ensure the
Calculatemethod is fully implemented or stubbed in the appropriate location to satisfy thePricerinterface fully.openmeter/billing/service/lineservice/pricegraduatedtiered.go (5)
13-17: Good implementation of the Pricer interfaceThe code correctly defines the
graduatedTieredPricerstruct and confirms it implements thePricerinterface with a compile-time check, which is a great practice to ensure interface compliance.
19-112: The Calculate method implementation looks correctThe overall flow of the
Calculatemethod is appropriate. It properly converts the price to a tiered price, validates the price mode, and delegates to the tiered price calculator.
145-171: Thorough validation of input parametersThe validation method is well-implemented with meaningful error messages for each validation check.
66-68:Details
❓ Verification inconclusive
Verify the flat price billing logic
The comment states "Flat price is always billed for the whole tier when we are crossing the tier boundary", but the code checks both
in.AtTierBoundaryandshouldFirstFlatLineBeBilled. Please verify if this is the intended behavior, as it may lead to unexpected results in edge cases.
🏁 Script executed:
#!/bin/bash # Search for other implementations or tests of flat price billing logic rg -A 5 "flat[_ ]?price.*bill" --type goLength of output: 657
Verify Flat Price Billing Logic
The current implementation in
openmeter/billing/service/lineservice/pricegraduatedtiered.go(lines 66–68) checks for bothin.AtTierBoundaryandshouldFirstFlatLineBeBilledalong with the existence ofin.Tier.FlatPrice. Test cases inpricegraduatedtiered_test.goindicate that this combined condition is intentional to ensure that the flat price is billed only once when the tier boundary is crossed. However, the inline comment stating "Flat price is always billed for the whole tier when we are crossing the tier boundary" may be misleading since the billing only occurs ifshouldFirstFlatLineBeBilledis true.
- Verify that the additional
shouldFirstFlatLineBeBilledflag is indeed meant to gate the flat price billing.- If this behavior is intended, consider updating the comment to clarify that billing applies only when both conditions are met (i.e., to prevent duplicate billing in edge cases).
267-276:Details
❓ Verification inconclusive
Special handling for zero quantity
This special case handles a scenario where
in.ToQtyis zero. It's important to verify this is the correct behavior and that it's properly tested, especially since this appears to be the only place that explicitly handles zero quantities.
🏁 Script executed:
#!/bin/bash # Search for tests covering zero quantity scenarios rg -A 10 "Zero|zero.*quantity" --type goLength of output: 88608
Review Zero Quantity Handling in Price Graduated Tiered Logic
The special handling for when
in.ToQtyis zero (lines 267–276 inopenmeter/billing/service/lineservice/pricegraduatedtiered.go) remains the only explicit treatment of a zero quantity in this module. A search for tests using the “Zero” keyword returned many hits across the codebase (including tests likeTestEmptyInvoiceGenerationZeroUsageandTestEmptyInvoiceGenerationZeroPricein the billing tests), but none appear to directly target this exact behavior.
- Action Required:
Please verify that the scenario wherein.ToQtyis zero is explicitly covered—either by dedicating a test for this edge case or by confirming that existing tests indeed exercise this code path.test/billing/invoice_test.go (6)
1811-1812: Updated test expectations for progressive invoicingThis change reduces the expected number of invoice lines from 3 to 2, correctly reflecting that the flat per usage item (which is in arrears) should no longer be included in progressive billing.
1819-1820: Improved test documentation for billing behaviorThis comment clearly explains why the flat per usage item is excluded from progressive billing - because it's invoiced in arrears without pro-rating, which aligns with the PR objective of correctly classifying UBP FlatFee lines.
1820-1825: Enhanced assertion logic for invoice line validationChanged from
NotContainstoElementsMatchfor a more precise verification of exactly which lines should be present in the invoice, improving the test's reliability.
1951-1952: Updated error verification for progressive billing supportThe error checking now correctly uses the new
ErrInvoiceProgressiveBillingNotSupportederror instead of the previousErrInvoiceLinesNotBillable, properly reflecting the new validation behavior for progressive billing.
1970-1971: Consistent line count assertion for updated invoiceUpdated the expected line count to 2 to maintain consistency with the earlier change to line count expectations.
2290-2291: Added debug output for improved test diagnosticsAdded debug dump of the invoice to aid in troubleshooting and understanding the test behavior.
turip
force-pushed
the
refactor/billing-pricer-interface
branch
from
b7c7703 to
3d893b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
openmeter/billing/service/lineservice/usagebasedline.go (1)
215-245: 💡 Verification agent🧩 Analysis chain
Validate potential negative usage.
Currently,asPricerCalculateInputonly checks if usage is nil, not if it’s negative. If negative usage is impossible, consider asserting it in your validation. Otherwise, handle it carefully (e.g., convert to zero or return an error).Run the following script to see if negative usage can come from other code paths that call
SnapshotQuantity:
🏁 Script executed:
#!/bin/bash # Searching codebase for suspicious usage assignments rg -A 3 "UsageBased" | rg -i "quantity.*="Length of output: 1261
Action Required: Validate and Handle Negative Usage Values
The function
asPricerCalculateInputinopenmeter/billing/service/lineservice/usagebasedline.gocurrently only checks fornilvalues for the usage-based quantities. However, based on our search, there’s no evident mechanism preventing negative values from being assigned (e.g., vialo.ToPtr(usage.LinePeriodQty)). If negative usage is not a valid state, please enhance the validation to assert non-negativity (or otherwise handle negative values, such as converting them to zero or returning an error). For instance, consider updating the function to:
- Verify that both
*l.line.UsageBased.Quantityand*l.line.UsageBased.PreLinePeriodQuantityare non-negative.- Include unit tests to confirm that negative values are either rejected or handled as intended.
♻️ Duplicate comments (2)
openmeter/billing/service/lineservice/pricepackage.go (2)
34-46: Guard against negative “toBeBilledPackages.”
If postLinePeriodPackages < preLinePeriodPackages, thentoBeBilledPackagescan become negative, potentially creating a negative quantity line entry, which may be disallowed. Consider returning an error or capping the value at zero if usage legitimately decreases.
61-69: Prevent division by zero ingetNumberOfPackages.
No checks guardpackageSizefrom being zero or negative, which can lead to a panic. As done in earlier pricer implementations, add a guard clause to handle zero or negativepackageSizeaccordingly.
🧹 Nitpick comments (10)
openmeter/billing/service/lineservice/priceflat.go (2)
14-14: Consider linking the TODO with an issue or future plan.
The comment at line 14 indicates an upcoming need to handle progressive billing logic. Having a clear reference or issue ID ensures that the future refactor won’t be overlooked.Would you like me to open a follow-up issue to track this enhancement?
20-24: Append context to the error message.
When wrapping the error at line 23, consider appending some metadata (e.g., line ID, underlying price data) to aid in troubleshooting.-return nil, fmt.Errorf("converting price to flat price: %w", err) +return nil, fmt.Errorf("converting price to flat price (%s, line: %s): %w", + l.line.Name, l.line.ID, err)openmeter/billing/service/lineservice/pricepackage_test.go (1)
258-266: Consider boundary tests for negative usage or extremely large usage.
While the existing tests are thorough for typical usage ranges, additional tests covering edge cases (e.g., accidental negative usage, extremely large usage) could bolster your confidence in the pricing logic.openmeter/billing/service/lineservice/pricevolumetiered.go (3)
28-31: Confirm unsupported progressive billing for volume-tiered pricing.
Returning no error but also no lines whenIsLastInPeriod()is false may be confusing to future maintainers. Document this clearly in related docstrings or comments to clarify why partial billing is disregarded.
33-36: Raise an explicit error or logging warning for partial usage.
Instead of silently returning an error only ifpreLinePeriodQtyis non-zero, consider alerting that volume-tiered pricing does not support partial usage if the code ever attempts it.
107-124: Handle multiple tiers if future requirements expand.
Currently,findTierForQuantityreturns only a single tier. If future product requirements necessitate multi-tier splits within a single period, you might need a more advanced approach.openmeter/billing/service/lineservice/usagebasedline.go (1)
88-103: Consider adding debug logs or comments for pricer eligibility.
Whenpricer.CanBeInvoicedAsOfreturns!canBeInvoiced, the code silently skips invoicing with anil, nilreturn. Adding logs or clarifying comments could help future maintainers understand why invoicing was skipped.openmeter/billing/service/lineservice/usagebasedline_test.go (1)
91-99: Add coverage for edge usage values.
The test sets positive usage but does not verify behavior with zero or negative usage. If such edge cases can occur, add test scenarios to ensurecalculateDetailedLineshandles them consistently.test/billing/invoice_test.go (1)
1819-1820: Correct spelling of "arreas" → "arrears"
There is a minor misspelling in these comments.-// - the volume item as that must be invoiced in arreas -// - the flat per usage item as that is invoiced in arreas (no pro-rating) +// - the volume item as that must be invoiced in arrears +// - the flat per usage item as that is invoiced in arrears (no pro-rating)openmeter/billing/service/lineservice/pricegraduatedtiered.go (1)
174-224: Splitting tier ranges at boundaries
Splitting the usage range at tier boundaries helps produce clear sub-ranges for partial usage. The approach is correct but keep an eye on large usage inputs—extensive splitting might be less efficient.Potentially consider an early-exit if usage surpasses an upper limit.
+// Example early exit check: +if from >= in.ToQty { + return append(res, pendingLine) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
openmeter/billing/errors.go(1 hunks)openmeter/billing/service/lineservice/pricedynamic.go(1 hunks)openmeter/billing/service/lineservice/pricedynamic_test.go(1 hunks)openmeter/billing/service/lineservice/priceflat.go(1 hunks)openmeter/billing/service/lineservice/priceflat_test.go(1 hunks)openmeter/billing/service/lineservice/pricegraduatedtiered.go(1 hunks)openmeter/billing/service/lineservice/pricegraduatedtiered_test.go(1 hunks)openmeter/billing/service/lineservice/pricepackage.go(1 hunks)openmeter/billing/service/lineservice/pricepackage_test.go(1 hunks)openmeter/billing/service/lineservice/pricer.go(1 hunks)openmeter/billing/service/lineservice/pricetiered.go(1 hunks)openmeter/billing/service/lineservice/priceunit.go(1 hunks)openmeter/billing/service/lineservice/priceunit_test.go(1 hunks)openmeter/billing/service/lineservice/pricevolumetiered.go(1 hunks)openmeter/billing/service/lineservice/pricevolumetiered_test.go(1 hunks)openmeter/billing/service/lineservice/usagebasedline.go(3 hunks)openmeter/billing/service/lineservice/usagebasedline_test.go(1 hunks)test/billing/invoice_test.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- openmeter/billing/service/lineservice/priceflat_test.go
- openmeter/billing/errors.go
- openmeter/billing/service/lineservice/priceunit.go
- openmeter/billing/service/lineservice/pricedynamic.go
- openmeter/billing/service/lineservice/priceunit_test.go
- openmeter/billing/service/lineservice/pricedynamic_test.go
- openmeter/billing/service/lineservice/pricetiered.go
- openmeter/billing/service/lineservice/pricevolumetiered_test.go
- openmeter/billing/service/lineservice/pricer.go
🧰 Additional context used
🧬 Code Definitions (8)
openmeter/billing/service/lineservice/pricepackage.go (3)
openmeter/billing/service/lineservice/pricer.go (2)
Pricer(23-29)PricerCalculateInput(16-21)openmeter/billing/service/lineservice/usagebasedline.go (3)
UsageChildUniqueReferenceID(20-20)MinSpendChildUniqueReferenceID(21-21)DecimalOne(42-42)openmeter/productcatalog/price.go (1)
InArrearsPaymentTerm(21-21)
openmeter/billing/service/lineservice/pricevolumetiered.go (5)
openmeter/billing/service/lineservice/pricer.go (4)
NonProgressiveBillingPricer(41-41)NonProgressiveBillingPricer(43-59)Pricer(23-29)PricerCalculateInput(16-21)openmeter/productcatalog/price.go (2)
VolumeTieredPrice(448-448)InArrearsPaymentTerm(21-21)openmeter/billing/errors.go (2)
ErrInvoiceLineVolumeSplitNotSupported(45-45)ErrInvoiceLineMissingOpenEndedTier(47-47)openmeter/billing/service/lineservice/usagebasedline.go (3)
VolumeFlatPriceChildUniqueReferenceID(33-33)VolumeUnitPriceChildUniqueReferenceID(34-34)VolumeMinSpendChildUniqueReferenceID(35-35)openmeter/billing/invoiceline.go (1)
FlatFeeCategoryCommitment(248-248)
openmeter/billing/service/lineservice/pricegraduatedtiered.go (4)
openmeter/billing/service/lineservice/pricer.go (4)
ProgressiveBillingPricer(31-31)ProgressiveBillingPricer(33-39)Pricer(23-29)PricerCalculateInput(16-21)openmeter/productcatalog/price.go (2)
GraduatedTieredPrice(450-450)InArrearsPaymentTerm(21-21)openmeter/billing/service/lineservice/usagebasedline.go (3)
GraduatedTieredPriceUsageChildUniqueReferenceID(37-37)GraduatedTieredFlatPriceChildUniqueReferenceID(38-38)GraduatedMinSpendChildUniqueReferenceID(39-39)openmeter/billing/invoiceline.go (1)
FlatFeeCategoryCommitment(248-248)
openmeter/billing/service/lineservice/priceflat.go (4)
openmeter/billing/service/lineservice/pricer.go (4)
NonProgressiveBillingPricer(41-41)NonProgressiveBillingPricer(43-59)Pricer(23-29)PricerCalculateInput(16-21)openmeter/productcatalog/price.go (3)
PaymentTermType(24-24)InAdvancePaymentTerm(20-20)InArrearsPaymentTerm(21-21)openmeter/billing/errors.go (1)
ValidationError(116-116)openmeter/billing/service/lineservice/usagebasedline.go (1)
FlatPriceChildUniqueReferenceID(25-25)
openmeter/billing/service/lineservice/usagebasedline.go (1)
openmeter/billing/service/lineservice/pricer.go (2)
PricerCalculateInput(16-21)Pricer(23-29)
openmeter/billing/service/lineservice/pricepackage_test.go (5)
openmeter/productcatalog/price.go (2)
NewPriceFrom(338-360)Commitments(687-693)openmeter/billing/service/lineservice/usagebasedline.go (2)
MinSpendChildUniqueReferenceID(21-21)UsageChildUniqueReferenceID(20-20)openmeter/billing/invoiceline.go (1)
FlatFeeCategoryCommitment(248-248)openmeter/productcatalog/discount.go (1)
Discounts(346-346)openmeter/billing/invoicelinediscount.go (6)
NewLineDiscounts(625-627)NewLineDiscountFrom(334-361)AmountLineDiscount(100-108)LineDiscountBase(62-69)LineMaximumSpendReferenceID(21-21)LineDiscountReasonMaximumSpend(27-27)
openmeter/billing/service/lineservice/pricegraduatedtiered_test.go (4)
openmeter/productcatalog/price.go (4)
PriceTierFlatPrice(651-654)PriceTierUnitPrice(670-673)GraduatedTieredPrice(450-450)Commitments(687-693)openmeter/billing/service/lineservice/usagebasedline.go (1)
GraduatedMinSpendChildUniqueReferenceID(39-39)openmeter/billing/invoiceline.go (1)
FlatFeeCategoryCommitment(248-248)openmeter/billing/invoicelinediscount.go (3)
NewLineDiscounts(625-627)NewLineDiscountFrom(334-361)AmountLineDiscount(100-108)
openmeter/billing/service/lineservice/usagebasedline_test.go (3)
openmeter/ent/db/billinginvoicelinediscount/where.go (2)
Quantity(116-118)PreLinePeriodQuantity(121-123)openmeter/ent/db/billinginvoiceline/where.go (1)
Quantity(168-170)openmeter/ent/db/billinginvoiceusagebasedlineconfig/where.go (1)
PreLinePeriodQuantity(78-80)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Artifacts / Container image
- GitHub Check: Lint
- GitHub Check: CI
- GitHub Check: Quickstart
- GitHub Check: E2E
- GitHub Check: Developer environment
- GitHub Check: Commit hooks
- GitHub Check: Build
- GitHub Check: Test
- GitHub Check: Analyze (go)
- GitHub Check: Migration Checks
🔇 Additional comments (18)
openmeter/billing/service/lineservice/priceflat.go (1)
57-57:Details
❓ Verification inconclusive
Clarify return of nil lines for non-first and non-last periods.
Returning(nil, nil)is valid if the code intentionally avoids generating lines for partial or middle-of-the-period usage. Confirm whether this design meets long-term requirements for partial billing cycles.
🏁 Script executed:
#!/bin/bash # Check references to partial-billing usage with `flatPricer` across the codebase. rg -A 5 '[Ff]latPricer'Length of output: 1633
Clarify and Document the Partial Billing Cycle Behavior
The
flatPricer.Calculatemethod currently returns(nil, nil)for non-first and non-last periods, indicating that no billing lines are generated for intermediate periods. This appears to be intentional—especially given the TODO comment suggesting the future introduction of a ProgressiveBillingPricer to support full pro-rating. Please update the inline documentation to explicitly state that this behavior is by design under the current model and note the potential future requirements for partial billing cycles.openmeter/billing/service/lineservice/pricepackage_test.go (1)
14-255: Robust test coverage for package pricing scenarios.
These sub-tests comprehensively validate various usage and commitment scenarios, including zero usage, min spend, max spend, and partial usage. The naming and structure of these tests make them easy to read and maintain.openmeter/billing/service/lineservice/usagebasedline.go (1)
247-280: Looks good for pricer selection and calculation flow.
ThegetPricerandcalculateDetailedLinesfunctions correctly delegate logic to the new pricer implementations and handle errors cleanly.test/billing/invoice_test.go (6)
1811-1811: Confirm reduced line count expectation
This test now expects 2 lines instead of the previously higher count. Ensure all code paths align with this updated logic and that other tests or references are consistent with the refined invoice line structure.
1951-1951: Use of new error constant for unsupported progressive billing
This aligns correctly with the updated logic for progressive billing.
1971-1971: Reduced line count check
Similar to the prior assertion, ensure the new expected line count of 2 is fully tested and validated against the new logic.
2108-2108: Consistent line count check
Confirm that the invoice creation logic consistently produces 2 lines under this flow.
2291-2292: Debugging test output and updated line count
The additional debug dump can help trace flows at the end of the billing period. The assertion for 4 lines appears consistent with the extended scenario coverage.
2303-2305: Clarifying comment on pro-ration
These remarks clarify that flat prices are retained without partial splits. Looks consistent with the intended design.openmeter/billing/service/lineservice/pricegraduatedtiered_test.go (4)
1-15: Package and imports
All necessary test dependencies (testify, lo, require, etc.) are correctly imported, and the package naming follows standard conventions.
17-116: Graduated tier test scenarios
The test cases comprehensively cover several usage patterns, checking both no usage and multi-tier usage. This is good coverage for graduated tiered pricing. Ensure minimal spend, maximum spend, or discount scenarios are also tested in these segments.
224-284: Maximum spend discount checks
These tests validate overage discount logic when usage surpasses the configured spend threshold. The approach to add line discounts looks correct and ensures no over-billing.
287-549: Tier callback mechanism & finalizer
The callback-based approach for tier boundaries, along with finalizer checks, is well-structured. It promotes clear testing of boundary conditions and accumulative totals. Keep verifying edge cases (like negative or extremely large quantities) if relevant.openmeter/billing/service/lineservice/pricegraduatedtiered.go (5)
1-15: Struct definition and base pricer embedding
EmbeddingProgressiveBillingPriceringraduatedTieredPriceris an organized approach. It keeps high-level progressive billing logic accessible while focusing the graduated tier logic here.
19-27: Validation on tiered price mode
The check ensures the price’s mode is indeedGraduatedTieredPrice. This guards against accidental usage of other pricing modes.
31-61: Callback-based tier processing
The callback function building each tier’s detailed line, along with maximum spend handling, is both readable and extensible. Continue to ensure concurrency safety if multi-threaded usage is anticipated.
63-106: Minimum spend finalizer
Applying the minimum spend as a final step when the total is below a threshold is well-handled. This logic is crucial for ensuring customers meet their committed spend.
225-326: Tiered price calculator
Iterating through sorted tiers, aggregating sub-ranges, and applying the finalizer is clean. The break condition at line 317 prevents unnecessary processing, which benefits performance.
turip
force-pushed
the
refactor/billing-pricer-interface
branch
from
3d893b9 to
8448c9c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
openmeter/billing/service/lineservice/priceflat.go (5)
13-16: Ensure proper documentation for structThe
flatPricerstruct lacks godoc-style documentation explaining its purpose and usage. Consider adding comments that describe what this struct does and how it fits into the billing system.+// flatPricer implements the Pricer interface for flat pricing calculations. +// It currently uses NonProgressiveBillingPricer as its base implementation. type flatPricer struct { // TODO[later]: when we introduce full pro-rating support this should be ProgressiveBillingPricer NonProgressiveBillingPricer }
20-24: Add godoc-style documentation for the Calculate methodThe
Calculatemethod lacks proper documentation explaining its purpose, parameters, and return values.+// Calculate determines the detailed lines for a flat price based on the payment term. +// It returns appropriate detailed line inputs or an error if the price cannot be calculated. func (p flatPricer) Calculate(l PricerCalculateInput) (newDetailedLinesInput, error) { flatPrice, err := l.line.UsageBased.Price.AsFlat() if err != nil { return nil, fmt.Errorf("converting price to flat price: %w", err) }
26-32: Consider using a cleaner approach for payment term validationThe payment term validation could be simplified by directly checking against the valid terms instead of using
slices.Contains.- if !slices.Contains( - []productcatalog.PaymentTermType{productcatalog.InAdvancePaymentTerm, productcatalog.InArrearsPaymentTerm}, - flatPrice.PaymentTerm) { + if flatPrice.PaymentTerm != productcatalog.InAdvancePaymentTerm && + flatPrice.PaymentTerm != productcatalog.InArrearsPaymentTerm { return nil, billing.ValidationError{ Err: fmt.Errorf("flat price payment term %s is not supported", flatPrice.PaymentTerm), } }
34-55: Consider refactoring duplicated code in switch statementThere's significant duplication between the two case blocks. You could extract the common parts to reduce repetition.
- switch { - case flatPrice.PaymentTerm == productcatalog.InAdvancePaymentTerm && l.IsFirstInPeriod(): - return newDetailedLinesInput{ - { - Name: l.line.Name, - Quantity: alpacadecimal.NewFromFloat(1), - PerUnitAmount: flatPrice.Amount, - ChildUniqueReferenceID: FlatPriceChildUniqueReferenceID, - PaymentTerm: productcatalog.InAdvancePaymentTerm, - }, - }, nil - case flatPrice.PaymentTerm == productcatalog.InArrearsPaymentTerm && l.IsLastInPeriod(): - return newDetailedLinesInput{ - { - Name: l.line.Name, - Quantity: alpacadecimal.NewFromFloat(1), - PerUnitAmount: flatPrice.Amount, - ChildUniqueReferenceID: FlatPriceChildUniqueReferenceID, - PaymentTerm: productcatalog.InArrearsPaymentTerm, - }, - }, nil - } + var shouldCharge bool + var paymentTerm productcatalog.PaymentTermType + + switch flatPrice.PaymentTerm { + case productcatalog.InAdvancePaymentTerm: + shouldCharge = l.IsFirstInPeriod() + paymentTerm = productcatalog.InAdvancePaymentTerm + case productcatalog.InArrearsPaymentTerm: + shouldCharge = l.IsLastInPeriod() + paymentTerm = productcatalog.InArrearsPaymentTerm + } + + if shouldCharge { + return newDetailedLinesInput{ + { + Name: l.line.Name, + Quantity: alpacadecimal.NewFromFloat(1), + PerUnitAmount: flatPrice.Amount, + ChildUniqueReferenceID: FlatPriceChildUniqueReferenceID, + PaymentTerm: paymentTerm, + }, + }, nil + }
14-14: Consider adding a ticket reference to the TODO commentThe TODO comment would be more useful with a reference to a ticket or issue number for tracking.
- // TODO[later]: when we introduce full pro-rating support this should be ProgressiveBillingPricer + // TODO[later,TICKET-XYZ]: when we introduce full pro-rating support this should be ProgressiveBillingPricer
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openmeter/billing/service/lineservice/priceflat.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
openmeter/billing/service/lineservice/priceflat.go (4)
openmeter/billing/service/lineservice/pricer.go (4)
NonProgressiveBillingPricer(41-41)NonProgressiveBillingPricer(43-59)Pricer(23-29)PricerCalculateInput(16-21)openmeter/productcatalog/price.go (3)
PaymentTermType(24-24)InAdvancePaymentTerm(20-20)InArrearsPaymentTerm(21-21)openmeter/billing/errors.go (1)
ValidationError(116-116)openmeter/billing/service/lineservice/usagebasedline.go (1)
FlatPriceChildUniqueReferenceID(25-25)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: E2E
- GitHub Check: CI
- GitHub Check: Developer environment
- GitHub Check: Quickstart
- GitHub Check: Test
- GitHub Check: Migration Checks
- GitHub Check: Lint
- GitHub Check: Commit hooks
- GitHub Check: Analyze (javascript)
- GitHub Check: Build
- GitHub Check: Analyze (go)
🔇 Additional comments (1)
openmeter/billing/service/lineservice/priceflat.go (1)
18-18: Good usage of interface compliance checkUsing a compile-time check to ensure that
flatPricerimplements thePricerinterface is a good practice.
This patch splits the monolithic ubp line into smaller pricers allowing to compose the types going forward. The patch also fixes an issue, that UBP FlatFee lines were considered progressively billable (this is only an issue if the line is in arrears) which is a rare use-case.
turip
force-pushed
the
refactor/billing-pricer-interface
branch
from
8448c9c to
74689c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
openmeter/billing/service/lineservice/pricepackage_test.go (1)
258-266: Duplicate comment: Ensure checks against zero or negative packageSize
This line references thegetNumberOfPackagesfunction, which can potentially divide by zero ifpackageSizeis zero or negative. A similar comment was shared in a past review.openmeter/billing/service/lineservice/pricepackage.go (2)
34-36: Duplicate comment: Guard against negativetoBeBilledPackages
IfpostLinePeriodPackages < preLinePeriodPackages, thentoBeBilledPackagesbecomes negative. Confirm whether usage can legitimately decrease and, if so, handle negative results appropriately. This was raised in a previous review.
61-69: Duplicate comment: Prevent division by zero ingetNumberOfPackages
No check exists forpackageSizebeing zero or negative, which could lead to division by zero. Consider adding appropriate validation as previously suggested.openmeter/billing/service/lineservice/pricegraduatedtiered.go (2)
19-112: MissingbilledAmountupdate after adding flat price.When adding the flat price line at lines 66–85, the code appends the new flat line item but does not update
billedAmountaccordingly. This risks undercounting total spend when both unit price and flat price exist in the same tier.
224-328: Add safeguard for empty tiers.As highlighted in a past review, ensure you handle the scenario where
in.TieredPrice.Tiersmight be empty to avoid potential panics or unexpected behavior.
🧹 Nitpick comments (3)
openmeter/billing/service/lineservice/pricepackage_test.go (1)
14-256: Consider adding negative usage scenario to broaden test coverage
These tests appear robust and thorough for typical usage patterns. However, it's unclear how the system should respond to negative usage values or other edge cases (e.g., usage decreasing between lines). Adding such scenarios could help ensure full coverage.openmeter/billing/service/lineservice/usagebasedline.go (1)
88-92: Consider wrapping the error for additional context.When
getPricer()fails, returning the raw error might obscure the source. Consider wrapping it (e.g.,fmt.Errorf("progressive billing: %w", err)) to improve traceability.openmeter/billing/service/lineservice/pricegraduatedtiered.go (1)
173-222: Range-splitting logic is clear.The function neatly partitions quantity ranges at the needed boundaries for tier calculations. Recommended to ensure thorough unit tests reflect off-by-one or boundary conditions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
openmeter/billing/errors.go(1 hunks)openmeter/billing/service/lineservice/pricedynamic.go(1 hunks)openmeter/billing/service/lineservice/pricedynamic_test.go(1 hunks)openmeter/billing/service/lineservice/priceflat.go(1 hunks)openmeter/billing/service/lineservice/priceflat_test.go(1 hunks)openmeter/billing/service/lineservice/pricegraduatedtiered.go(1 hunks)openmeter/billing/service/lineservice/pricegraduatedtiered_test.go(1 hunks)openmeter/billing/service/lineservice/pricepackage.go(1 hunks)openmeter/billing/service/lineservice/pricepackage_test.go(1 hunks)openmeter/billing/service/lineservice/pricer.go(1 hunks)openmeter/billing/service/lineservice/pricetiered.go(1 hunks)openmeter/billing/service/lineservice/priceunit.go(1 hunks)openmeter/billing/service/lineservice/priceunit_test.go(1 hunks)openmeter/billing/service/lineservice/pricevolumetiered.go(1 hunks)openmeter/billing/service/lineservice/pricevolumetiered_test.go(1 hunks)openmeter/billing/service/lineservice/usagebasedline.go(3 hunks)openmeter/billing/service/lineservice/usagebasedline_test.go(1 hunks)test/billing/invoice_test.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (12)
- openmeter/billing/errors.go
- openmeter/billing/service/lineservice/pricedynamic.go
- openmeter/billing/service/lineservice/priceunit_test.go
- openmeter/billing/service/lineservice/pricedynamic_test.go
- openmeter/billing/service/lineservice/priceunit.go
- openmeter/billing/service/lineservice/priceflat.go
- openmeter/billing/service/lineservice/pricetiered.go
- openmeter/billing/service/lineservice/pricegraduatedtiered_test.go
- openmeter/billing/service/lineservice/pricevolumetiered_test.go
- openmeter/billing/service/lineservice/pricer.go
- openmeter/billing/service/lineservice/priceflat_test.go
- test/billing/invoice_test.go
🧰 Additional context used
🧬 Code Definitions (6)
openmeter/billing/service/lineservice/pricepackage_test.go (5)
openmeter/productcatalog/price.go (2)
NewPriceFrom(338-360)Commitments(687-693)openmeter/billing/service/lineservice/usagebasedline.go (2)
MinSpendChildUniqueReferenceID(21-21)UsageChildUniqueReferenceID(20-20)openmeter/billing/invoiceline.go (1)
FlatFeeCategoryCommitment(248-248)openmeter/productcatalog/discount.go (1)
Discounts(346-346)openmeter/billing/invoicelinediscount.go (6)
NewLineDiscounts(625-627)NewLineDiscountFrom(334-361)AmountLineDiscount(100-108)LineDiscountBase(62-69)LineMaximumSpendReferenceID(21-21)LineDiscountReasonMaximumSpend(27-27)
openmeter/billing/service/lineservice/pricevolumetiered.go (5)
openmeter/billing/service/lineservice/pricer.go (4)
NonProgressiveBillingPricer(41-41)NonProgressiveBillingPricer(43-59)Pricer(23-29)PricerCalculateInput(16-21)openmeter/productcatalog/price.go (2)
VolumeTieredPrice(448-448)InArrearsPaymentTerm(21-21)openmeter/billing/errors.go (2)
ErrInvoiceLineVolumeSplitNotSupported(45-45)ErrInvoiceLineMissingOpenEndedTier(47-47)openmeter/billing/service/lineservice/usagebasedline.go (3)
VolumeFlatPriceChildUniqueReferenceID(33-33)VolumeUnitPriceChildUniqueReferenceID(34-34)VolumeMinSpendChildUniqueReferenceID(35-35)openmeter/billing/invoiceline.go (1)
FlatFeeCategoryCommitment(248-248)
openmeter/billing/service/lineservice/pricepackage.go (3)
openmeter/billing/service/lineservice/pricer.go (2)
Pricer(23-29)PricerCalculateInput(16-21)openmeter/billing/service/lineservice/usagebasedline.go (3)
UsageChildUniqueReferenceID(20-20)MinSpendChildUniqueReferenceID(21-21)DecimalOne(42-42)openmeter/productcatalog/price.go (2)
InArrearsPaymentTerm(21-21)Commitments(687-693)
openmeter/billing/service/lineservice/usagebasedline_test.go (3)
openmeter/ent/db/billinginvoicelinediscount/where.go (2)
Quantity(116-118)PreLinePeriodQuantity(121-123)openmeter/ent/db/billinginvoiceline/where.go (1)
Quantity(168-170)openmeter/ent/db/billinginvoiceusagebasedlineconfig/where.go (1)
PreLinePeriodQuantity(78-80)
openmeter/billing/service/lineservice/pricegraduatedtiered.go (4)
openmeter/billing/service/lineservice/pricer.go (4)
ProgressiveBillingPricer(31-31)ProgressiveBillingPricer(33-39)Pricer(23-29)PricerCalculateInput(16-21)openmeter/productcatalog/price.go (2)
GraduatedTieredPrice(450-450)InArrearsPaymentTerm(21-21)openmeter/billing/service/lineservice/usagebasedline.go (3)
GraduatedTieredPriceUsageChildUniqueReferenceID(37-37)GraduatedTieredFlatPriceChildUniqueReferenceID(38-38)GraduatedMinSpendChildUniqueReferenceID(39-39)openmeter/billing/invoiceline.go (1)
FlatFeeCategoryCommitment(248-248)
openmeter/billing/service/lineservice/usagebasedline.go (2)
openmeter/billing/service/lineservice/pricer.go (2)
PricerCalculateInput(16-21)Pricer(23-29)openmeter/productcatalog/price.go (6)
Price(80-87)FlatPriceType(41-41)UnitPriceType(42-42)TieredPriceType(43-43)PackagePriceType(45-45)DynamicPriceType(44-44)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Quickstart
- GitHub Check: E2E
- GitHub Check: CI
- GitHub Check: Developer environment
- GitHub Check: Test
- GitHub Check: Commit hooks
- GitHub Check: Migration Checks
- GitHub Check: Lint
- GitHub Check: Analyze (javascript)
- GitHub Check: Build
- GitHub Check: Analyze (go)
🔇 Additional comments (14)
openmeter/billing/service/lineservice/pricevolumetiered.go (2)
18-31: Logic for skipping volume-tiered billing on non-last lines is correct
No concerns found. This design aligns with the notion that volume-tiered pricing is billed once at the end of the period.
112-124: Consider additional tests for missing or improperly sorted tiers
The code expects the last tier to have an infinite upper bound (nil). If a price definition lacks this final open-ended tier, the function returns an error. Ensure the test suite covers this scenario thoroughly.openmeter/billing/service/lineservice/usagebasedline.go (6)
94-102: Behavior on non-invoiceable scenario looks correct.If the pricer explicitly indicates the line is not invoiceable, returning
nil, nilis a clean way to skip further processing.
105-105: Comment matches logic.This comment clarifies the intent to confirm progressive billing support in downstream logic. No concerns here.
215-215: Invocation of your newcalculateDetailedLines()method is properly placed.This is a straightforward call right after snapshotting. Looks consistent with the surrounding flow.
235-246: Good use of a dedicated helper to build pricer input.Your
asPricerCalculateInputmethod ensures consistency by validating and returning the required parameters. This fosters clarity and reduces duplication.
247-264: Clean switch-based logic for pricer retrieval.This approach is easy to extend if more pricing models are introduced. Error handling for unsupported types is also appropriate.
268-280: Modular approach enhances maintainability.Delegating the calculation to the pricer captures the new architecture’s intent. This function is concise and clear.
openmeter/billing/service/lineservice/usagebasedline_test.go (1)
91-95: Properly assigning usage before calculation.Directly setting
QuantityandPreLinePeriodQuantityon the line clarifies test setup and ensures test logic accurately simulates real usage data.openmeter/billing/service/lineservice/pricegraduatedtiered.go (5)
1-15: File-level overview.Introducing a graduated tiered pricer under the
lineservicepackage is consistent with your new modular pricing architecture. The initial definitions (imports, struct, interface conformance) appear clear.
114-121: Straightforward tierRange struct.Holds the tier metadata and bounding quantities for partial usage in progressive billing. Looks accurate and meets the needed data shape.
123-129: tierCallbackInput captures crucial tier context.This struct is well-formed for passing partial usage and prior total to the callback function, promoting clarity in tier-based calculations.
131-143: Well-defined tieredPriceCalculatorInput.Validation fields (e.g.
FromQty,ToQty,Currency) and callback wrappers ensure structured data flow. No issues noted.
145-171: Robust validation.Checks mode, currency, and quantity boundaries. This guards well against invalid tiered pricing setups.
2 participants
Overview
The only logic change included is the addition of the Pricer interface, other than that the logic parts are copy paste from the old implementation to prevent anything from breaking.
This patch splits the monolithic ubp line into smaller pricers allowing to compose the types going forward.
The patch also fixes an issue, that UBP FlatFee lines were considered progressively billable (this is only an issue if the line is in arrears which is a rare use-case).
Summary by CodeRabbit
New Features
Bug Fixes