Conversation
📝 WalkthroughWalkthroughThis pull request restructures billing and invoicing schema definitions across multiple project components. Changes include replacing the legacy 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 (
|
turip
force-pushed
the
feat/invoice-discounts-api-v2
branch
3 times, most recently
from
9537215 to
94d7246
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
openmeter/billing/httpdriver/invoiceline.go (1)
675-675:⚠️ Potential issueLikely bug: Wrong invoice line type set in usage-based line.
You appear to set
Type: billing.InvoiceLineTypeFeewhile handlingapi.InvoiceUsageBasedLineReplaceUpdateat line 675. This is inconsistent and could cause logic errors or misclassifications. It should likely beType: billing.InvoiceLineTypeUsageBased.Apply this diff to fix the suspicious line:
- Type: billing.InvoiceLineTypeFee, + Type: billing.InvoiceLineTypeUsageBased,
🧹 Nitpick comments (18)
api/spec/src/billing/invoices/invoice.tsp (1)
735-744: Deprecation of directpriceandfeatureKey.
Good practice to mark these as deprecated and guide consumers towardrateCard.*. Ensure reciprocal references are documented for clarity.api/openapi.yaml (4)
13966-13972: Subscription Reference Description Typo
The description for the subscription reference contains a typo ("subscritpions" instead of "subscriptions").- description: Subscription are the references to the subscritpions that this line is related to. + description: Subscription are the references to the subscriptions that this line is related to.
13980-14000: Deprecation of Legacy Fields and RateCard Integration
Multiple fields (the ones referencingNumeric,PricePaymentTerm, andNumericfor quantity) are now marked as deprecated, and a newrateCardproperty is introduced referencingInvoiceFlatFeeRateCard. This change is aligned with restructuring the rate-card object. Make sure that the validation for Create/Update requests enforces only one non-empty field as required by the API changes.
15175-15181: Subscription Reference Typo Revisited
The subscription reference in this section still contains the typo “subscritpions.”- description: Subscription are the references to the subscritpions that this line is related to. + description: Subscription are the references to the subscriptions that this line is related to.
15352-15414: Comprehensive Update for Invoice Usage-Based Models
This extensive update covers:
- The
InvoiceUsageBasedLineCreateWithCustomermodel, which now includes a newcustomerIdfield.- The definition of
InvoiceUsageBasedRateCard, detailing properties likefeatureKey,taxConfig,price, anddiscounts.Review Note: The description for the
discountsproperty currently states, "Flat fee rate cards only support percentage discounts." If this schema is intended for usage-based rate cards, please verify and update the description to accurately reflect the discount types applicable to usage-based pricing.api/openapi.cloud.yaml (9)
13863-13869: Deprecated TaxConfig Field in This Block.
TheTaxConfigproperty is now marked as deprecated. Ensure that API consumers are informed about this deprecation and that the documentation reflects the change.
13892-13898: Addition of Subscription Field with Minor Typo.
The newsubscriptionproperty referencingInvoiceLineSubscriptionReferenceis now being added. Please correct the typo in the description by replacing "subscritpions" with "subscriptions" for clarity.
13906-13925: Transition from Deprecated Price Fields to New RateCard.
Several pricing-related fields (price,paymentTerm, andquantity) are now deprecated in favor of the newrateCardproperty (referencingInvoiceFlatFeeRateCard). Ensure that migration guidance is provided to clients and that documentation clearly explains the new structure.
13980-14000: Consistent Deprecation in Flat Fee Schema.
This section continues the pattern of deprecatingprice,paymentTerm, andquantityin favor of the newrateCard(which points toInvoiceFlatFeeRateCard). Double-check that all related documentation is updated accordingly.
14095-14137: Introduction of InvoiceFlatFeeRateCard Schema.
A new schemaInvoiceFlatFeeRateCardis introduced with properties such astaxConfig,price,discounts, andquantity. Confirm that thepricecorrectly ties toFlatPriceWithPaymentTermand that the discounts are restricted to percentage discounts as intended.
14228-14405: Comprehensive Redefinition of Discount Schemas.
This segment lays out detailed definitions for both amount-based (InvoiceLineDiscountAmount) and usage-based (InvoiceLineDiscountUsage) discounts. The use ofoneOfwith a proper discriminator is well implemented. Please verify that numeric validations and the linkage ofrateCardDiscountare consistent with the billing engine's expectations.
14714-14734: Deprecation of Flat Fee Price Fields.
This block reiterates the deprecation of pricing fields (price,paymentTerm, andquantity) in favor of usingrateCard. It is essential to update the migration guides so that consumers can easily map legacy fields to the new schema.
15101-15107: Refinement Needed in Subscription Property Description.
The block adds asubscriptionproperty after the external IDs. Similar to an earlier hunk, please fix the typo in the description (“subscritpions” → “subscriptions”) to maintain clarity and consistency.
15196-15216: Reinforcement of Usage-based Pricing Updates.
This hunk mirrors previous changes by deprecating the legacy usage-based pricing fields and promoting the newrateCarddefinition for usage items. Confirm that the new pattern is thoroughly documented in both the API specification and client guides.api/spec/src/billing/invoices/discounts.tsp (1)
1-31: Adopt consistent naming for clarity.These declarations for
InvoiceGenericChargeand its associated metadata look good. However, consider reviewing naming conventions for consistency across the codebase (e.g., preferringDiscountReasonorReasonCodeconsistently). This helps reduce confusion when other contributors consume these models.openmeter/billing/httpdriver/invoiceline.go (2)
126-135: Centralize validation to promote consistency.You use
mapAndValidateFlatFeeRateCardDeprecatedFieldsandmapAndValidateUsageBasedRateCardDeprecatedFieldsto parse and validate lines. This approach is consistent and clean. Verify that all possible deprecated fields and their validations are covered. If you anticipate expansions (e.g., new fields or more constraints), consider abstracting the common logic to reduce duplication.Also applies to: 170-178
425-464: Consider performance implications when mapping each individual discount.The loop in
mapDiscountsToAPIcallsmapDiscountToAPIfor each discount. While it’s straightforward, if extremely large discount lists are possible, the repeated transformations could affect performance. For typical invoice volumes, this is fine, but if you anticipate thousands of line discounts, consider optimizing or batching transformations.api/client/javascript/src/client/schemas.ts (1)
4849-4872: Leverage new rate card approach and remove dependence on deprecated fields.
Multiple fields (perUnitAmount,paymentTerm,quantity) are now marked@deprecated, replaced by arateCardmechanism. Consider gradually removing dependencies on these deprecated fields for clarity and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/client/javascript/src/client/schemas.ts(22 hunks)api/openapi.cloud.yaml(24 hunks)api/openapi.yaml(24 hunks)api/spec/src/billing/invoices/discounts.tsp(1 hunks)api/spec/src/billing/invoices/invoice.tsp(7 hunks)api/spec/src/billing/invoices/main.tsp(1 hunks)openmeter/billing/httpdriver/deprecations.go(1 hunks)openmeter/billing/httpdriver/deprecations_test.go(1 hunks)openmeter/billing/httpdriver/invoiceline.go(21 hunks)
🧰 Additional context used
🧬 Code Definitions (4)
openmeter/billing/httpdriver/deprecations_test.go (2)
api/client/javascript/src/client/schemas.ts (7)
PricePaymentTerm(9250-9250)RateCard(9253-9253)InvoiceFlatFeeRateCard(9086-9087)FlatPriceWithPaymentTerm(9047-9048)RateCardUsageBasedPrice(9263-9264)UnitPriceWithCommitments(9329-9330)InvoiceUsageBasedRateCard(9141-9142)api/client/go/client.gen.go (8)
PricePaymentTerm(4606-4606)RateCard(4638-4640)InvoiceFlatFeeRateCard(2977-2994)FlatPriceWithPaymentTerm(2478-2488)FlatPriceWithPaymentTermTypeFlat(272-272)RateCardUsageBasedPrice(4789-4791)UnitPriceWithCommitments(5463-5475)InvoiceUsageBasedRateCard(3668-3684)
openmeter/billing/httpdriver/invoiceline.go (2)
openmeter/productcatalog/price.go (2)
InAdvancePaymentTerm(20-20)Price(80-87)openmeter/billing/invoiceline.go (8)
FlatFeeLine(251-258)Price(682-682)UsageBasedLine(684-693)LineDiscount(726-736)FlatFeeCategory(235-235)FlatFeeCategory(244-249)FlatFeeCategoryRegular(239-239)Line(271-284)
api/client/javascript/src/client/schemas.ts (1)
api/client/go/client.gen.go (9)
InvoiceFlatFeeRateCard(2977-2994)InvoiceLine(3012-3014)InvoiceLineAppExternalIds(3017-3023)InvoiceLineCharge(3026-3028)InvoiceLineChargeAmount(3031-3055)InvoiceLineDiscount(3061-3063)InvoiceLineDiscountAmount(3066-3096)InvoiceLineDiscountUsage(3104-3136)InvoiceUsageBasedRateCard(3668-3684)
openmeter/billing/httpdriver/deprecations.go (3)
api/client/javascript/src/client/schemas.ts (5)
RateCard(9253-9253)InvoiceFlatFeeRateCard(9086-9087)TaxConfig(9320-9320)InvoiceUsageBasedRateCard(9141-9142)RateCardUsageBasedPrice(9263-9264)api/api.gen.go (6)
RateCard(4960-4962)InvoiceFlatFeeRateCard(3237-3254)TaxConfig(5754-5763)Price(4919-4921)InvoiceUsageBasedRateCard(3928-3944)RateCardUsageBasedPrice(5114-5116)api/client/go/client.gen.go (5)
RateCard(4638-4640)InvoiceFlatFeeRateCard(2977-2994)TaxConfig(5400-5409)InvoiceUsageBasedRateCard(3668-3684)RateCardUsageBasedPrice(4789-4791)
🔇 Additional comments (71)
api/spec/src/billing/invoices/main.tsp (1)
16-16: Add import confirmed.
Importing./discounts.tspcleanly integrates discount-related models in this file. No issues found.openmeter/billing/httpdriver/deprecations_test.go (3)
1-11: Good test structuring and dependencies.
The package declaration and import statements are organized and straightforward. No concerns about concurrency, security, or performance here.
13-114: Comprehensive flat fee tests.
YourTestFlatFeeLineParserthoroughly covers success and failure cases, ensuring deprecated vs. current field parity is validated. This fosters robust backwards compatibility.
116-195: Usage-based line coverage is excellent.
TestUsageBasedLineParserincludes both positive and negative scenarios, verifying field presence and mismatched values. The approach is comprehensive, ensuring consistent handling of deprecated fields.api/spec/src/billing/invoices/invoice.tsp (9)
648-648: Clear deprecation note.
MarkingtaxConfigas deprecated guides users towardrateCard.taxConfig. Ensure consistent usage in dependent code.
710-720: IntroducingInvoiceUsageBasedRateCard.
This model neatly encapsulates usage-based rate card properties (discounts, taxConfig, featureKey, price). This aligns with the new design for usage-based lines.
745-752: NewrateCardfor usage-based lines.
This property centralizes line-level pricing data. Make sure external references to legacy fields are updated to userateCard.
769-787: IntroducingInvoiceFlatFeeRateCard.
Consistent with usage-based counterpart. Moves discounts and config into a dedicated structure, simplifying future expansions.
804-806: DeprecatingperUnitAmount.
Linking it torateCard.perUnitAmountkeeps the new architecture uniform. Make sure internal calculations also reflect this change.
818-821: Deprecating inline quantity.
Noting the shift torateCard.quantityis consistent with the newly introduced model. This ensures a single source of truth for quantity value.
822-828: NewrateCardproperty for flat fee lines.
Well-aligned with usage-based lines. Helps unify discount handling under a single interface.
1034-1035: Documentation clarity.
Adding a descriptive comment forInvoiceLineSubscriptionReferenceprovides better context around subscription-based lines. No issues found.
689-689:Details
✅ Verification successful
Subscription reference adjustment.
Confirm that this line’s changes accurately reflect the new subscription reference logic. If this property’s usage changed upstream, verify references are updated accordingly.
🏁 Script executed:
#!/bin/bash # Searching for references to `subscription?` in invoice lines rg "subscription\?\:\s*InvoiceLineSubscriptionReference"Length of output: 190
Subscription Reference Adjustment Verification
The update at line 689 appears intended to reflect the new subscription reference logic. Note that the initial regex command errored due to escape sequence issues. I've replaced it with a literal search command above. Please run this command and manually verify that:
- The updated subscription reference (“subscription?: InvoiceLineSubscriptionReference”) is consistently applied across the codebase.
- Upstream changes in the property’s usage are correctly reflected in all relevant references.
🏁 Script executed:
#!/bin/bash # Re-run reference search for the updated subscription reference logic using literal search. # This avoids regex escape issues that occurred with the previous command. rg -F "subscription?: InvoiceLineSubscriptionReference" -n .Length of output: 161
Subscription Reference Adjustment – Verified
The subscription reference update now uses the literal
"subscription?: InvoiceLineSubscriptionReference"as expected. A literal search confirms that this property is defined inapi/spec/src/billing/invoices/invoice.tsp(line 688), and no additional conflicting references were found. Please ensure that this change aligns with the upstream logic for subscription handling.api/openapi.yaml (20)
13912-13918: InvoiceLineCharge Schema Reference Update
The changes introduce a new reference toInvoiceLineChargefor thechargesarray. This aligns with the new billing model that replaces the legacyBillingLineChargeschema.
13937-13943: Marking Tax Config as Deprecated
The addition ofdeprecated: trueto theTaxConfigreference is consistent with the overall deprecation strategy defined in the PR objectives. Please ensure that API consumers are notified about these deprecated fields.
14032-14038: Tax Config Deprecation Consistency
The deprecation of theTaxConfigreference here is consistent with previous sections. Confirm that this deprecation is clearly documented for API consumers.
14054-14074: Consistent Deprecation and Replacement with RateCard
This block mirrors earlier changes by deprecating legacy pricing fields and introducing therateCardreference for flat fee lines. Ensure that any related client documentation or downstream validation logic is also updated accordingly.
14120-14126: Tax Config Deprecation Revisited
The reintroduction of a deprecation marker on theTaxConfigfield maintains consistency across the API spec.
14142-14163: Deprecation and New RateCard Mapping
This section applies the same deprecation strategy as earlier—retiring legacy fields in favor of the newrateCardmapping. It is important that all related documentation is updated so API users are clear about which properties are to be used.
14169-14215: Introduction of InvoiceFlatFeeRateCard Schema
A newInvoiceFlatFeeRateCardschema has been introduced to encapsulate the fee intent (including properties fortaxConfig,price,discounts, andquantity). Verify that required fields (likeprice) and default values (such as the default quantity of 1) are properly validated in practice.
14257-14279: New InvoiceLineCharge and InvoiceLineChargeAmount Schemas
The newInvoiceLineChargeschema now uses a discriminator with aoneOfto refer to theInvoiceLineChargeAmounttype, which is a robust strategy for future extensions. Ensure that if other charge types are introduced later, the discriminator and mapping are updated accordingly.
14302-14479: Comprehensive Definitions for Charge and Discount Schemas
This extensive section definesInvoiceLineChargeAmount,InvoiceLineDiscount,InvoiceLineDiscountAmount, andInvoiceLineDiscountUsagewith detailed properties and proper readOnly markers. The use of discriminators for differentiating discount types (i.e.amountvs.usage) is well implemented. Please verify that all enum values and validation patterns (especially for ID formats) align with global API standards.
14522-14528: Clarification for Subscription Reference
The description update forInvoiceLineSubscriptionReferenceadds clarity to the relationship between invoice lines and subscriptions. This is a welcome improvement—ensure that any references in related client code are updated to reflect these changes.
14766-14772: Reiteration of Tax Config Deprecation
Reiterating the deprecation ofTaxConfigin this block helps maintain a consistent policy across the specification. The approach looks solid.
14788-14808: Consistent Deprecation for Flat Fee Fields
This change continues the trend of deprecating legacy numeric and payment fields and replacing them with the newrateCardreference. Confirm that any client-side or downstream integrations are adjusted to ignore the deprecated fields.
14883-14889: Tax Config Deprecation Consistency Check
The deprecation marker for theTaxConfigproperty in this section once again follows the established pattern. No issues noted.
14905-14924: Usage-Based RateCard Structure Update
The schema changes for usage-based items include deprecating certain legacy fields and introducing a newrateCardreference for usage-based pricing. Verify that the regex pattern forfeatureKeyis correct and that the description clearly distinguishes this model from flat fee rate cards.
15121-15127: Charges Array Consistency for Invoice Lines
Thechargesarray is updated to referenceInvoiceLineCharge, which is consistent with earlier changes. This ensures uniformity in how charges are recorded across different invoice line types.
15146-15152: Deprecation of Tax Config in Nested Structures
This block continues to mark theTaxConfigfield as deprecated, now in a nested context. The clarity and consistency of deprecation throughout the document is commendable.
15189-15209: Usage-Based Item Details Update
This hunk updates properties for usage-based items, including deprecation of legacy fields and the introduction of a newrateCardstructure. The description details are clear and consistent with previous patterns.
15248-15254: Reaffirming Tax Config Deprecation
The consistent deprecation of theTaxConfigproperty in this section reinforces the overall design pattern.
15270-15288: Addition of ID Field in Usage-Based Schema
This block adds anidfield to the usage-based rate card schema along with deprecation markers and detailed descriptions. Please confirm that the regex pattern for theidfield aligns with the intended global ID format.
15330-15336: Consistent Deprecation in Nested Tax Configs
The deprecation ofTaxConfigremains consistent in this nested context as well. No further issues observed.api/openapi.cloud.yaml (12)
13838-13844: Updated Charges Array with InvoiceLineCharge Reference.
The schema now specifies an array of charges using the newInvoiceLineChargereference. Please verify that the downstream processing correctly supports the new charge schema.
13958-13963: Deprecation of TaxConfig is Consistently Applied.
The TaxConfig property in this block is marked as deprecated. Confirm that this aligns with the overall restructuring strategy and that clients are aware of the deprecation.
14046-14052: Marking TaxConfig as Deprecated Here, Too.
TheTaxConfigfield is again marked as deprecated. It is important that the deprecation notice is uniformly communicated across all schema sections.
14068-14088: Flat Fee Pricing Deprecation Consistency.
This fragment clearly deprecates the legacy pricing fields and introduces the newrateCardfor flat fee lines. Please ensure that any client-side transformations or validations are updated to support therateCardmodel.
14183-14205: New InvoiceLineCharge and InvoiceLineChargeAmount Schemas.
The legacy billing charge schema is replaced withInvoiceLineCharge(using a discriminator based on thetype) and its detail defined inInvoiceLineChargeAmount. Ensure that all mandatory fields (such ascreatedAt,updatedAt,id,amount, andtype) are correctly implemented in the service layer.
14448-14454: Enhanced Description for Subscription Reference.
The description forInvoiceLineSubscriptionReferencehas been enriched. Additionally,InvoiceLineTaxBehavioris declared appropriately. No further action is needed, but ensure that the enhanced messaging is propagated in documentation.
14692-14698: TaxConfig Deprecation in Subscription Section.
Tax configuration is again marked as deprecated here. Please confirm that this deprecation is communicated consistently to avoid confusion among API consumers.
14809-14815: Consistent Deprecation for TaxConfig.
TheTaxConfigfield is consistently marked as deprecated in this segment. Ensure that all client libraries and documentation reflect this change.
15047-15053: Ensuring Consistency in Charges Definition.
Thechargesproperty is redefined to use the newInvoiceLineChargeschema. Confirm that all related processing logic and validations have been updated to handle the new charge format.
15072-15078: Deprecation in Nested Structures for TaxConfig.
The TaxConfig property in this nested context is also marked as deprecated, with thechildrenproperty remaining read-only. Ensure that any hierarchical processing respects these deprecations.
15174-15180: Deprecation Continuity for TaxConfig.
The deprecation of theTaxConfigfield is applied consistently in this block. No additional changes are needed, but verify that your release notes clearly mention these deprecations.
15256-15262: TaxConfig Deprecation Reaffirmed.
The TaxConfig field continues its deprecation status in this section. This is consistent with previous deprecations; ensure that all related documentation is updated accordingly.api/spec/src/billing/invoices/discounts.tsp (2)
33-75: Ensure alignment of “amount” vs “usage” functionalities.The
InvoiceLineGenericAmountChargeandInvoiceLineGenericUsageChargedefinitions are clear in distinguishing types of charges. However, make sure there's a consistent approach to validating and applying these charge properties. For instance, if you implement advanced validation somewhere else (e.g., restricting negative or zero amounts), ensure it’s applied consistently to usage-based fields too.Can you confirm that usage-based billing and amount-based billing rely on the same validation or if separate logic is desired?
76-175: Double-check the discount handling across all new union types.The union definitions for
InvoiceLineDiscountandInvoiceLineChargesuccessfully model multiple discount and charge subtypes. However, changes in discount or charge logic often ripple throughout the system. Ensure all references toInvoiceLineDiscountTypeandInvoiceLineChargeTypein downstream usage have been updated to handle these new enumerations properly (e.g., data transformations, error handling, or specialized business logic).Please verify that any code mapping from the old discount or charge interfaces to these new union types has been updated accordingly. If not, consider adding the necessary logic or validations.
openmeter/billing/httpdriver/invoiceline.go (2)
31-34: Check backward-compatibility of default constants.Defining
defaultFlatFeePaymentTermanddefaultFlatFeeQuantityprovides clear defaults, but verify if older code or existing data in the database might expect different defaults. Ensure applying these defaults does not introduce unexpected changes for existing lines.
263-310: Handle potential errors during discount mapping gracefully.
mapDiscountsToAPIand the discount logic in fee/usage-based lines add crucial error-checking. Confirm that callers handle these new error returns consistently (e.g., gracefully returning a 4xx to the user if discount mapping fails). Otherwise, discount errors may inadvertently trigger 5xx errors or incomplete data messages.Also applies to: 344-348
openmeter/billing/httpdriver/deprecations.go (2)
1-94: Confirm environment supportserrors.Joinfor aggregated error reporting.Go’s
errors.Joinwas introduced in Go 1.20. If the codebase or environment is pinned to an earlier Go version, this could break at runtime. Double-check your Go toolchain version to avoid unexpected panics or builds failing in older environments.
96-174: Overall structure for deprecations is solid.These deprecation handler functions and validations (for both flat fee and usage-based rate cards) provide a straightforward approach to backward compatibility. Just ensure the reflection-based comparison for
TaxConfigremains stable for all potential struct changes, especially if any fields are slices or maps (which might require deeper equality checks).api/client/javascript/src/client/schemas.ts (20)
4898-4901: Redundant taxConfig deprecation comment.
This seems to be the same deprecation note as in previous segments. No additional concerns.
4919-4935: Repeated deprecation usage.
TheperUnitAmount,paymentTerm, andquantityproperties are again marked@deprecated. No new issues introduced here.
4967-4969: Again, taxConfig is marked as deprecated.
This repeats the earlier deprecation. No further action needed beyond prior notes.
4987-5003: Reiterated field deprecations.
All references toperUnitAmount,paymentTerm, andquantityremain consistent with earlier deprecations. No new concerns.
5015-5044: Introducing InvoiceFlatFeeRateCard.
This new interface consolidates flat-fee settings, including optional tax config, discounts, and price details. The structure looks coherent and aligns with the Go reference snippets provided.
5071-5074: Adopting InvoiceLineCharge and ChargeAmount.
The newInvoiceLineChargepoints toInvoiceLineChargeAmountfor pre-tax additions—this reflects the updated design pattern. Looks well-defined.
5235-5238: No concerns on subscription reference.
The introducedInvoiceLineSubscriptionReferenceproperty is straightforward.
5403-5407: Repeated taxConfig deprecation comment.
Matches earlier usage. No additional concerns.
5424-5440: Same set of deprecated fields.
This mirrors previous patterns for flat-fee lines. No further notes.
5486-5489: Reiterated taxConfig deprecation.
No fresh issues here.
5506-5519: Usage-based line deprecations: price, featureKey.
These references match prior deprecation patterns. No new concerns.
5643-5653: Charges & deprecated taxConfig repeated.
No new feedback beyond prior notes.
5672-5691: Again: price, featureKey, rateCard.
Consistent with the usage-based lines approach.
5718-5721: Still the same taxConfig deprecation.
No extra comments.
5738-5751: Repeated usage-based line shape.
The same pattern with deprecatedpriceandfeatureKey; usage-based approach remains intact.
5777-5781: No new findings on deprecated taxConfig.
Same scenario here.
5797-5840: InvoiceUsageBasedRateCard interface introduced.
This new type clarifies usage-based pricing details, discounts, and tax config references. It’s consistent with the broader refactoring strategy.
9086-9100: Type exports for flat-fee charges and discounts.
These exports (InvoiceFlatFeeRateCard,InvoiceLineCharge, etc.) align with the updated schema.
9141-9142: Type export InvoiceUsageBasedRateCard.
The usage-based rate card export matches the new approach.
4820-4830:Details
❓ Verification inconclusive
Confirm usage of newly added property & deprecation notice.
The newcharges?: InvoiceLineCharge[]property is a good addition, enabling multiple charges to be encapsulated. ThetaxConfigfield is now marked as@deprecated—please ensure any existing usage paths are updated or removed in favor of the new approach (e.g. using arateCard).
🏁 Script executed:
#!/bin/bash # Search for references to taxConfig in the codebase for potential cleanup rg -A 3 'taxConfig'Length of output: 25273
Review: Validate Deprecation of
taxConfig& Confirm NewchargesImplementationThe new
charges?: InvoiceLineCharge[]property is a well-considered enhancement for handling multiple charges. Meanwhile, the legacytaxConfigfield is now marked as deprecated in the JavaScript client schema. However, our repository-wide search shows thattaxConfigis still referenced broadly (in tests, API generation, OpenAPI specs, etc.).Before finalizing, please ensure that:
- The deprecation notice for
taxConfiginapi/client/javascript/src/client/schemas.ts(lines 4820-4830) is clearly documented and communicated to downstream consumers.- A migration path (e.g., leveraging
rateCard.taxConfig) is defined and gradually implemented across all usage sites, including tests and server/API components, to avoid future inconsistencies.
turip
force-pushed
the
feat/invoice-discounts-api-v2
branch
2 times, most recently
from
31b5f57 to
8f70b47
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
api/client/javascript/src/client/schemas.ts (1)
5153-5157:⚠️ Potential issueDocumentation for
amountproperty references non-existent fieldThe documentation for the
amountproperty inInvoiceLineDiscountAmountmentions "calculated if percent present", but there is nopercentproperty defined in this interface or its parent interfaces.Fix the documentation to remove the reference to a non-existent field:
/** * Amount in the currency of the invoice - * @description Fixed discount amount to apply (calculated if percent present). + * @description Fixed discount amount to apply. */ readonly amount: components['schemas']['Numeric']
🧹 Nitpick comments (17)
api/openapi.yaml (5)
13966-13972: Hunk 3 – Subscription Field and Typo Fix
A newsubscriptionfield is introduced with a reference to subscription details; however, the description contains a typographical error ("subscritpions"). Consider correcting it to "subscriptions." A suggested diff is:- description: Subscription are the references to the subscritpions that this line is related to. + description: Subscription are the references to the subscriptions that this line is related to.
13980-14000: Hunk 4 – Deprecated Fields & New RateCard Integration (Flat Fee)
This block deprecates legacy fields such aspaymentTermandquantityand introduces the newrateCardreferencingInvoiceFlatFeeRateCard. The restructuring is in line with the PR objectives. Please verify that a clear migration guide is provided so that API clients can smoothly transition.
14169-14213: Hunk 9 – Defining InvoiceFlatFeeRateCard Schema
The newInvoiceFlatFeeRateCardschema is well defined with required fields (including a nestedtaxConfig,price, and an array ofdiscounts). The provided example in thepriceproperty further clarifies expected usage. Consider adding a note in the description regarding client migration to this new structure.
14788-14808: Hunk 14 – Flat Fee Line Deprecations Updated
The propertiesprice,paymentTerm, andquantityare again marked as deprecated in favor of the newrateCardreference. This is consistent with the overall restructuring. Consider adding additional migration details in the release notes.
15175-15181: Hunk 19 – Reiterated Subscription Reference with Typo
Similar to Hunk 3, this section’ssubscriptionfield contains the same typographical error in "subscritpions." Please update the description as suggested:- description: Subscription are the references to the subscritpions that this line is related to. + description: Subscription are the references to the subscriptions that this line is related to.api/openapi.cloud.yaml (2)
13892-13901: Typo in Subscription Description
In thesubscriptionproperty description, the word “subscritpions” is misspelled. Consider updating it to “subscriptions” for clarity.- description: Subscription are the references to the subscritpions that this line is related to. + description: Subscription are the references to the subscriptions that this line is related to.
15101-15107: Subscription Field Typographical Correction Needed
Thesubscriptionfield description again contains the typo “subscritpions.” Please update this description for consistency.- description: Subscription are the references to the subscritpions that this line is related to. + description: Subscription are the references to the subscriptions that this line is related to.api/spec/src/billing/invoices/invoice.tsp (4)
710-719: Consider capturing payment term requirements.This new
InvoiceUsageBasedRateCardmodel omits a payment term field. If usage-based lines can have distinct payment terms, consider adding it here to ensure consistency across rate cards.
769-788: Unify usage-based and flat-fee approaches.
InvoiceFlatFeeRateCardlargely mirrorsInvoiceUsageBasedRateCard. To reduce complexity, consider a shared structure or patterns where possible.
819-821: Retire the oldquantityfield.By marking
quantityas deprecated, you should finalize its removal or fully transition torateCard.quantity. Keeping both can lead to confusion.
824-827: Require the newrateCardfield sooner rather than later.Although you plan to make
rateCardrequired in the future, consider an interim validation that warns or errors if the field is missing, speeding up adoption.openmeter/billing/httpdriver/invoiceline.go (2)
199-203: EncapsulateTaxConfigusage in a single place.Implementing
TaxConfig: rateCardParsed.TaxConfigis straightforward. If anything else modifies taxConfig, consider centralizing the logic for better maintainability.
502-511: Simulation line conversion for flat fees.Reusing
mapAndValidateFlatFeeRateCardDeprecatedFieldsensures consistent behavior between regular and simulation flows. Consider adding logs or metrics for these transformations.openmeter/billing/httpdriver/deprecations.go (4)
29-41: Ensure consistent validation for PaymentTerm and TaxConfig.The
ValidateDeprecatedFields()method only checksPerUnitAmountandQuantity. IfPaymentTermorTaxConfigare required in specific scenarios, consider adding their validations. Otherwise, clarify their optional status.
103-174: Consider reducing duplication or clarifying flow.The function branches significantly between the deprecated approach (
RateCard == nil) and the new approach (RateCard != nil). While this is understandable for transitional purposes, you may consider refactoring some common logic, or adding comments to clarify the deprecation flow for future maintainers.
201-241: Clarify error messages for usage-based rate cards.The error messages (e.g., "price is required") might benefit from referencing usage-based context or the relevant field name to reduce confusion for users who mix up rate card fields.
249-294: Maintain consistency with flat-fee approach.Similar to
mapAndValidateFlatFeeRateCardDeprecatedFields, this function splits logic between old and new usage-based fields. Consider factoring out shared logic (e.g., repeated error handling) to ease future maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/client/javascript/src/client/schemas.ts(22 hunks)api/openapi.cloud.yaml(24 hunks)api/openapi.yaml(24 hunks)api/spec/src/billing/invoices/discounts.tsp(1 hunks)api/spec/src/billing/invoices/invoice.tsp(7 hunks)api/spec/src/billing/invoices/main.tsp(1 hunks)openmeter/billing/httpdriver/deprecations.go(1 hunks)openmeter/billing/httpdriver/deprecations_test.go(1 hunks)openmeter/billing/httpdriver/invoiceline.go(21 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- api/spec/src/billing/invoices/main.tsp
- openmeter/billing/httpdriver/deprecations_test.go
🧰 Additional context used
🧬 Code Definitions (2)
openmeter/billing/httpdriver/deprecations.go (3)
api/client/javascript/src/client/schemas.ts (5)
RateCard(9253-9253)InvoiceFlatFeeRateCard(9086-9087)TaxConfig(9320-9320)InvoiceUsageBasedRateCard(9141-9142)RateCardUsageBasedPrice(9263-9264)api/api.gen.go (6)
RateCard(4960-4962)InvoiceFlatFeeRateCard(3237-3254)TaxConfig(5754-5763)Price(4919-4921)InvoiceUsageBasedRateCard(3928-3944)RateCardUsageBasedPrice(5114-5116)api/client/go/client.gen.go (5)
RateCard(4638-4640)InvoiceFlatFeeRateCard(2977-2994)TaxConfig(5400-5409)InvoiceUsageBasedRateCard(3668-3684)RateCardUsageBasedPrice(4789-4791)
openmeter/billing/httpdriver/invoiceline.go (3)
openmeter/productcatalog/price.go (2)
InAdvancePaymentTerm(20-20)Price(80-87)api/client/go/client.gen.go (6)
RateCard(4638-4640)TaxConfig(5400-5409)InvoiceFlatFeeCategory(2765-2765)PricePaymentTerm(4606-4606)InvoiceFlatFeeRateCard(2977-2994)FlatPriceWithPaymentTerm(2478-2488)openmeter/billing/invoiceline.go (8)
FlatFeeLine(251-258)Price(682-682)UsageBasedLine(684-693)LineDiscounts(743-745)LineDiscount(726-736)FlatFeeCategory(235-235)FlatFeeCategory(244-249)Line(271-284)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: CI
- GitHub Check: E2E
- GitHub Check: Developer environment
- GitHub Check: Test
- GitHub Check: Commit hooks
- GitHub Check: Analyze (go)
- GitHub Check: Lint
🔇 Additional comments (79)
api/openapi.yaml (19)
13912-13918: Hunk 1 – Charges Array Schema Update
The updated array definition now correctly references the newInvoiceLineChargeschema. This change cleanly aligns with the removal of invoice-level discounts and the introduction of line-level charge abstractions.
13937-13942: Hunk 2 – Deprecating TaxConfig Property
The addition ofdeprecated: truefor the TaxConfig reference is consistent with the new API structure. Ensure that related documentation clearly highlights this deprecation.
14032-14037: Hunk 5 – Consistent Deprecation for TaxConfig
The property using theTaxConfigreference is marked as deprecated here as well. This maintains consistency across the API; ensure documentation mirrors these changes.
14054-14074: Hunk 6 – Mirroring Flat Fee Line Deprecations
The deprecation ofpaymentTermandquantityalongside the introduction of the new flat feerateCardis consistent with the previously updated blocks. Confirm that these changes are uniformly applied throughout the spec.
14120-14126: Hunk 7 – TaxConfig Deprecation Consistency
This hunk reiterates the deprecation of the TaxConfig schema within the line item context. The change is clear and consistent.
14142-14163: Hunk 8 – Additional Flat Fee Line Updates
Again, the updatedrateCarddefinition (referencingInvoiceFlatFeeRateCard) is introduced and deprecated fields are maintained. The consistency here is commendable.
14257-14269: Hunk 10 – Introducing InvoiceLineCharge with Discriminator
The newInvoiceLineChargeschema employs a discriminator mapping to choose between charge types (currently onlyamount). This is a clean approach that facilitates future extensibility. Verify that any future charge types are appropriately mapped as the API evolves.
14302-14403: Hunk 11 – Detailed Charge and Discount Schemas
This block defines:
InvoiceLineChargeAmountwith essential properties (including ID, amount, and type).InvoiceLineDiscountas a union (viaoneOf) ofInvoiceLineDiscountAmountandInvoiceLineDiscountUsage, with a discriminator for clarity.The comprehensive structure supports both fixed and usage-based discounts. Please double-check that the required fields (and the ID pattern) align with business rules and that the definitions are consistent throughout the spec.
14522-14526: Hunk 12 – Subscription Reference Schema
The schema forInvoiceLineSubscriptionReferencenow leverages anallOfreference toIDResource. This appears well integrated; however, validate that its intended usage is clearly described in external documentation.
14766-14772: Hunk 13 – Additional TaxConfig Deprecation
Another instance where theTaxConfigproperty is marked as deprecated. The change is uniform with prior similar modifications.
14883-14889: Hunk 15 – Consistent Deprecation Reminder
This brief update for a TaxConfig reference reinforces the deprecation pattern throughout the document.
14905-14924: Hunk 16 – Usage-Based Pricing and Deprecation
This block introduces the usage-based pricing structure by referencingRateCardUsageBasedPriceand marks thefeatureKeyas deprecated. Ensure that clients are aware of the newInvoiceUsageBasedRateCardmechanism replacing these legacy fields.
15121-15127: Hunk 17 – Charges Array for Usage-Based Lines
Thechargesfield is again defined as an array referencingInvoiceLineCharge, maintaining consistency across both flat fee and usage-based items.
15146-15152: Hunk 18 – Nested Line Properties with TaxConfig and Children
The inclusion of thechildrenfield to represent nested line details, alongside a deprecated TaxConfig section, is clear. No issues were noted.
15189-15208: Hunk 20 – Usage-Based Rate Card Structure
This hunk mirrors the structure seen in Hunk 16. It defines properties for usage-based pricing by referencingRateCardUsageBasedPrice, deprecates thefeatureKey, and introduces a newrateCardthat referencesInvoiceUsageBasedRateCard. This structured approach is well thought out; just ensure that any deprecation is clearly communicated.
15248-15254: Hunk 21 – Additional TaxConfig Deprecation Block
The TaxConfig reference is again marked as deprecated with an associated period property. This is consistent with similar sections.
15270-15289: Hunk 22 – Continuing Usage-Based Schema Enhancements
This block repeats the pattern of referencingRateCardUsageBasedPrice, deprecating thefeatureKey, and introducing the new usage-based rate card. Consistency here is key—please verify alignment with Hunk 20.
15330-15336: Hunk 23 – Yet Another TaxConfig Deprecation
The deprecation of the TaxConfig element is continued here without issue. Ensure that all these deprecations are documented in the changelog.
15352-15414: Hunk 24 – InvoiceUsageBasedLineCreateWithCustomer & InvoiceUsageBasedRateCard Schemas
This extensive change defines two new schemas:
InvoiceUsageBasedLineCreateWithCustomer, which now includes acustomerIdproperty (with an ID pattern and example) and a descriptive text clarifying its role in usage-based invoicing.InvoiceUsageBasedRateCard, which outlines the intent for usage-based pricing and contains properties (e.g.,featureKey,taxConfig,price, and an array ofdiscounts).Both schemas are comprehensive and align with the new API design. It is recommended to ensure that client-facing documentation and migration guides highlight these changes clearly.
api/openapi.cloud.yaml (22)
13838-13844: New Charges Definition Using InvoiceLineCharge
Thechargesproperty now specifies an array whose items referenceInvoiceLineCharge. This change centralizes charge information to the new schema. Please ensure that the referenced schema is defined correctly and that client integrations are updated accordingly.
13863-13869: Deprecation of Tax Configuration Field
TheTaxConfigobject is now marked as deprecated here. Verify that this deprecation is documented in the migration guide and that downstream consumers are aware of the change.
13906-13926: Deprecation of Legacy Pricing Fields and Introduction of Rate Card
The properties for price, payment term, and quantity are now deprecated, and a newrateCardproperty referencingInvoiceFlatFeeRateCardhas been added. This aligns with the new invoice discount model. Please ensure that client documentation reflects these deprecations and that migration steps are clearly communicated.
13958-13963: Consistent Deprecation of TaxConfig for Line Periods
DeprecatingTaxConfigin this context is consistent with earlier changes. Confirm that any logic relying on the previous tax configuration is updated or properly migrated.
13980-14001: Reapplication of Deprecated Pricing Fields in a Different Context
This hunk reiterates the deprecation ofNumeric(price),PricePaymentTerm, and quantity fields while introducingrateCardfor flat-fee lines. It is important to verify consistency with similar definitions (see hunks 4 and 8) and update related documentation.
14046-14052: Deprecated TaxConfig with Period Field Specification
TheTaxConfigobject here is again marked as deprecated. Ensure that this deprecation is consistently communicated across the API specification and that the new fields fully replace its functionality where needed.
14068-14088: Uniform Deprecation of Legacy Pricing Attributes
Reiterates deprecation for pricing (Numeric), payment term, and quantity fields with the newrateCardfor flat-fee items. Confirm that migration notes and documentation are updated to reflect these changes consistently.
14095-14138: Definition of InvoiceFlatFeeRateCard
A new schema,InvoiceFlatFeeRateCard, is introduced to represent the intent for flat fee lines. The schema defines properties such astaxConfig,price(nullable),discounts, andquantity. Please verify that the required structure (e.g.,pricebeing required but nullable) matches business logic and update client guides if needed.
14183-14206: InvoiceLineCharge and Discriminator Setup
The newInvoiceLineChargeschema is now defined using aoneOfcomposition with a discriminator on thetypeproperty. Ensure that:
- The discriminator mapping (with key
amount) properly resolves toInvoiceLineChargeAmount- All required fields in the charge amount schema are enforced
This change should streamline charge representation in response objects.
14228-14404: Comprehensive Redefinition of InvoiceLineDiscount Schemas
The definitions forInvoiceLineDiscount,InvoiceLineDiscountAmount, andInvoiceLineDiscountUsagenow clearly separate fixed-amount discounts from usage-based discounts. The use of a discriminator on thetypeproperty facilitates polymorphism. Please validate that all required properties (timestamps, IDs, etc.) are consistent with the rest of the API and that the examples and enums meet the business requirements.
14448-14453: Clarification for Subscription Reference Schema
TheInvoiceLineSubscriptionReferenceis now described with an updated block, followed by the newInvoiceLineTaxBehavior. Ensure that the relationship between the subscription reference and other line items is well documented for consumers.
14692-14698: Further Deprecation of TaxConfig with Period Properties
Another instance whereTaxConfigis marked as deprecated alongside theperiodfield. The consistency is good; please double-check that every deprecation is accompanied by migration guides.
14714-14734: Legacy Pricing Fields Deprecation Revisited for Flat Fee Lines
This hunk further confirms the deprecation of old pricing fields (Numeric, payment terms, and quantity) in favor of the newrateCard. Consistency across the API is essential—ensure that all parts of the system adopt this change.
14809-14815: Repeated Deprecation of TaxConfig with Period Definition
Yet another instance where theTaxConfigproperty is deprecated. While it is consistent, verify that the repeated patterns are intentional and that documentation avoids redundancy for developers integrating with the API.
14831-14849: Usage-Based Price Fields Transition
For usage-based line items, theRateCardUsageBasedPriceandfeatureKeyare now deprecated in favor of a newrateCardthat referencesInvoiceUsageBasedRateCard. It is crucial to ensure that all associated clients and documentation reflect this schema change and that tests confirm the new behavior.
15047-15053: Reiteration of Charges Array for Invoice Lines
Thechargesproperty is reintroduced here (similar to hunk 1) for another context. Verify that the reference toInvoiceLineChargeis consistent and that no legacy charge definitions remain active.
15072-15078: Deprecated Tax Configuration and Children Field
The deprecation ofTaxConfigalong with thechildrenfield definition appears once more. Although similar to earlier hunks, confirm that the intended behavior for child lines is maintained and that deprecated fields are properly communicated.
15115-15135: Usage-Based Pricing Fields Consistency Check
This hunk again deprecatesRateCardUsageBasedPriceandfeatureKeywhile introducing the newrateCardfield for usage-based items. Ensure that the descriptions and deprecation notices match those in hunk 16 so that all usage-based pricing components are uniform across the API.
15174-15180: Further Deprecation of TaxConfig in Usage-Based Contexts
The pattern of deprecating theTaxConfigproperty for usage-based lines continues here. Verify that the same deprecation strategy applies everywhere and that this field is no longer relied upon by clients.
15196-15216: Addition of an ID Field in Usage-Based Pricing Schema
In this hunk, the usage-based pricing schema further deprecatesRateCardUsageBasedPriceandfeatureKeyand introduces anidfield with a specific pattern. Please ensure the pattern is consistent with other IDs in the API and that the new property is documented accordingly.
15256-15262: Consistent Deprecation for TaxConfig and Period in Usage Context
This hunk deprecates theTaxConfigproperty yet again in the context of usage-based lines. The consistency is appreciated; just ensure the deprecation rationale is clearly documented in the migration notes.
15278-15338: Comprehensive Update for Usage-Based Line and RateCard Schemas
This extensive hunk covers two important changes:
- It deprecates the legacy usage-based pricing fields (
RateCardUsageBasedPriceandfeatureKey) in favor of a newrateCardreferencingInvoiceUsageBasedRateCardand introduces acustomerIdfield.- It defines the new
InvoiceUsageBasedRateCardschema, including its required properties (featureKey,taxConfig,price, anddiscounts).Make sure that:
- The nullable
priceis handled correctly (signifying that the feature can be free).- The restrictions on discounts (flat fee rate cards only supporting percentage discounts) are well documented.
- All field descriptions accurately convey the intended business logic and migration path.
api/spec/src/billing/invoices/invoice.tsp (6)
648-648: Confirm references to old property are removed.You're deprecating this property in favor of
rateCard.taxConfig. Please ensure there are no remaining references to the old property throughout the codebase.Would you like me to generate a script to check for any lingering references?
688-688: Validate rename fromsubscriptionstosubscription.Double-check everywhere this property is used, to confirm that switching from plural to singular does not introduce any breaking changes.
Do you want me to generate a script to verify all references to the old property?
735-736: Ensure deprecatedpricefield is fully phased out.You're deprecating the standalone
pricefield in favor ofrateCard.price. Verify that no active code paths write or read fromprice.
741-743: ConfirmfeatureKeyreferences are updated.Deprecating
featureKeyin favor ofrateCard.featureKeymay require a thorough check for any usage in older workflows or migrations.
748-753: Implement partial validation forrateCard.You labeled
rateCardwith a TODO to make it required. Introducing optional usage of the new property can cause confusion. Consider validating or warning ifrateCardis omitted.
804-806: For clarity, remove or replace uses ofperUnitAmount.Since you're deprecating
perUnitAmountin favor ofrateCard.perUnitAmount, confirm that any references toperUnitAmountin logic or tests are replaced or removed.api/spec/src/billing/invoices/discounts.tsp (11)
1-6: Imports and namespace look fine.No immediate concerns. Good separation of concerns within the
Invoicesnamespace.
7-31: Revisit the necessity of “Generic” in the model’s name.As previously mentioned in older reviews, consider whether “Generic” is adding clarity or if it can be removed to simplify naming.
33-49: Check for consistent naming with the rest of the codebase.
InvoiceLineGenericAmountChargeeffectively extendsInvoiceGenericCharge. Confirm all references align well with new or existing naming conventions for invoice lines and amounts.
50-74: Usage-based charge logic is clear.This structure for applying usage-based discounts or surcharges before taxes is straightforward. Ensure that the usage units (quantities) match the meter’s definition in all references.
76-84: Enum type is adequately defined.
InvoiceLineDiscountTypeis simple and self-explanatory. No issues.
85-94: Double-check discriminators forInvoiceLineDiscount.When used in complex unions, be sure that the type is correctly determined at runtime. Also test fallback cases if the discriminator is missing.
95-116: Usage-based discount handles progressive billing well.Including
preLinePeriodQuantityinInvoiceLineDiscountUsageis a good approach for partial or progressive usage. Make sure that the code applying these discounts properly accounts for splitting logic.
117-141: Amount-based discount structure is clear.The
InvoiceLineDiscountAmountmodel is straightforward. Confirm you have a plan for negative amounts, if they ever occur, or if partial refunds need special handling.
143-151: Charge type enum is limited but sufficient.You commented out
usage. If you plan to add usage-based charges later, keep an eye on potential overlap with existing discount usage-based logic.
152-161: Charge union cleanly references specialized models.The
InvoiceLineChargeunion withamountcovers typical scenarios. Reintroducing the usage variant is straightforward if needed.
162-175:InvoiceLineChargeAmountmodel is consistent.Overall consistency is maintained. The design parallels that of
InvoiceLineDiscountAmount, ensuring a cohesive approach to amounts on lines.openmeter/billing/httpdriver/invoiceline.go (11)
31-35: Good introduction of default constants.Defining
defaultFlatFeePaymentTermanddefaultFlatFeeQuantityclarifies your defaults. Consider validating scenarios where quantity is zero or uninitialized.
125-135: Validate error messaging on mapping flat fee rate card.The consolidated call to
mapAndValidateFlatFeeRateCardDeprecatedFieldssimplifies logic. Ensure that the returned error from the mapping function is descriptive enough for troubleshooting.
170-178: Consistent approach for usage-based lines.Mirroring the flat fee approach helps maintain consistency. The new function call to parse usage-based rate card fields looks good.
263-267: Capture errors from discount mapping for flat-fee lines.You have robust error handling for discount mapping. This ensures partial failures don’t silently slip through. Nicely done.
344-348: Apply same discount mapping approach to usage-based lines.This mirrors the flat-fee flow, promoting consistency. Great job keeping the logic parallel.
425-444: mapDiscountsToAPI returns an error for better resilience.Returning
(*[]api.InvoiceLineDiscount, error)is a good design choice. It allows immediate handling of partial failures or unexpected discount states.
445-465: mapDiscountToAPI usage is consistent with new discount models.Mapping single discounts individually is flexible. Double-check future expansions (e.g., usage-based discount) remain consistent with this approach.
541-549: Usage-based simulation line.Again, consistent approach. Excellent reusability with the shared validation function.
551-559: Robust parsing of quantity fields.You handle potential parsing errors gracefully for both
QuantityandpreLinePeriodQuantity. Keep an eye out for negative usage inputs if not supported.
612-620: Flat fee line updates in replace flow.Invoking
mapAndValidateFlatFeeRateCardDeprecatedFieldshere ensures uniform deprecation handling. Just confirm you handle partial updates at the calling layer.
656-663: Usage-based line updates in replace flow.Same consistent approach as flat fee lines. Using the shared function ensures minimal duplication. Good job.
api/client/javascript/src/client/schemas.ts (5)
5015-5044: Well-structured rate card implementationThe new
InvoiceFlatFeeRateCardinterface looks good. It properly encapsulates the pricing components (price, discounts, quantity) along with tax configuration, following the PR objective of moving line-specific user input to a dedicated rate-card object.
5071-5115: Appropriate structure for line chargesThe new charge interfaces (
InvoiceLineChargeandInvoiceLineChargeAmount) are well-designed. The type discriminator pattern used here with thetype: 'amount'ensures proper type narrowing for API consumers.
5116-5220: Well-designed discount interfacesThe new discount interfaces use a proper union type approach with type discriminators (
type: 'amount'ortype: 'usage'), allowing clients to distinguish between different discount types. The relationship with rate card discounts via therateCardDiscountproperty provides good traceability.
5817-5840: Well-structured usage-based rate cardThe
InvoiceUsageBasedRateCardinterface is well-designed, providing a clear distinction between flat fee and usage-based pricing models.
4857-4859: Consistent deprecation approachAll fields are consistently marked with
@deprecated, which is good practice for backward compatibility. This follows the PR objective of deprecating old fields on the rate-card while introducing new structures.Also applies to: 4867-4870, 4920-4922, 4930-4933, 4988-4990, 4998-5001, 5425-5427, 5435-5438, 5507-5510, 5512-5516, 5679-5682, 5684-5687, 5739-5742, 5744-5747, 5798-5801, 5803-5806
openmeter/billing/httpdriver/deprecations.go (5)
19-28: Confirm whether PaymentTerm must be verified.The
flatFeeRateCardItemsstruct includes thePaymentTermfield but does not treat it as required inValidateDeprecatedFields. If it's optional, consider clarifying this within inline doc comments. If it's mandatory for certain use cases, add a corresponding validation check.
96-102: Struct layout looks good.This struct clearly captures the parsed values for the flat-fee scenario.
178-185: Optional fields not validated.The
usageBasedRateCardItemsstruct includesTaxConfig, but it's not marked as required. If the legacy usage-based scenario needs aTaxConfig, consider validating this. Otherwise, clarify that it's optional.
187-199: Clear field requirements.This method enforces
priceandfeatureKeybut nottaxConfig, indicating thattaxConfigis optional. If that's intentional, no issue. Otherwise, consider adding a validation check to ensure completeness.
243-248: Struct approach is consistent.This mirrors the flat-fee parsed struct, helping standardize the usage-based parsing flow.
turip
force-pushed
the
feat/invoice-discounts-api-v2
branch
from
8f70b47 to
097583b
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (33)
api/spec/src/billing/invoices/discounts.tsp (3)
7-9: Update documentation to remove references to chargesThe documentation comment for
InvoiceDiscountBasestill mentions it represents "a charge or discount", but according to previous feedback, charges have been removed from this implementation. Consider updating to only reference discounts./** - * InvoiceDiscountBase represents a charge or discount that can be applied to a line or the entire invoice. + * InvoiceDiscountBase represents a discount that can be applied to a line or the entire invoice. */
14-18: Update the ID documentation to be discount-specificSince the model now only relates to discounts (not charges), update the ID field's documentation for clarity.
/** - * ID of the charge or discount. + * ID of the discount. */
97-102: Clarify documentation for amount fieldThe comment refers to a "percent present" calculation, but there's no percent field in the model. Update the documentation to accurately reflect how the amount is determined.
/** - * Fixed discount amount to apply (calculated if percent present). + * Fixed discount amount to apply. */api/openapi.yaml (10)
13927-13933: Deprecation Flag on TaxConfig (Hunk 1):
Adeprecated: trueflag has been added for the TaxConfig reference. Consider updating the description’s grammar (e.g. “Tax config specifies the tax configuration…”) for clarity. Also, ensure that the deprecation rationale and migration path are documented in the API release notes.
13956-13962: Typo & Grammar in Subscription Description (Hunk 2):
The description on line 13962 contains a typo (“subscritpions”); it should be “subscriptions”. Also, consider adjusting the phrasing for correct subject–verb agreement (e.g. “Subscriptions are the references…”).
14022-14028: Additional Deprecation on TaxConfig (Hunk 4):
The TaxConfig reference is again marked as deprecated. As with Hunk 1, consider refining the description text (“specify” → “specifies”) and verify that the documentation fully explains the reason behind the deprecation.
14132-14152: Repeated Deprecated Pricing Attributes (Hunk 7):
The use ofdeprecated: truefor pricing, payment, and quantity fields is repetitive. Consider whether these common configurations could be refactored into a shared component to reduce duplication in the spec.
14248-14271: Introduction of InvoiceLineDiscount with Discriminator (Hunk 9):
The new InvoiceLineDiscount schema utilizes a oneOf structure with a discriminator on thetypeproperty. This is a strong design choice that clearly separates discount amounts from usage discounts. Verify that the required fields and mappings are fully aligned with the API’s discount processing logic.
14294-14403: Detailed Discount Schemas (Hunk 10):
Both InvoiceLineDiscountAmount and InvoiceLineDiscountUsage are defined in detail with appropriate readOnly and example annotations. The overall structure is clear; however, consider whether common fields across discount schemas can be consolidated to reduce redundancy.
14446-14450: Clarification for InvoiceLineSubscriptionReference (Hunk 11):
The description has been updated to explain the relation to subscriptions. A minor improvement would be to adjust the phrasing (e.g. “...references to the subscriptions…” instead of “subscritpions”).
14712-14732: Repetitive Deprecated Fields for Pricing & Payment (Hunk 13):
The same legacy pricing and payment term fields are marked as deprecated here yet again. Consider abstracting these common patterns into reusable components within the spec to improve maintainability.
14829-14847: Usage-Based Rate Card for Flat Fee Items (Hunk 15):
Here, RateCardUsageBasedPrice is deprecated and a newrateCardreference is introduced for usage-based billing. Ensure that the distinctions between flat fee and usage-based rate cards are clearly documented, particularly regarding how discounts are applied differently.
15089-15095: Reiteration of Subscription Field (Hunk 17):
The subscription field is added again with the same description issue noted earlier. Please address the typo (“subscritpions”) and consider streamlining the description for clarity.api/spec/src/billing/invoices/invoice.tsp (5)
639-639: Clarify deprecation timeline fortaxConfigin InvoiceLineBase
Marking this property as deprecated is a sound approach. However, ensure that there's a clear timeline or plan for removing it to prevent confusion among users.
727-728: DeprecatingpriceinInvoiceUsageBasedLine
Deprecation ofpriceis straightforward, but ensure you communicate the new usage ofrateCard.priceto avoid confusion or data inconsistency across the codebase.
736-743: IntroducerateCardinInvoiceUsageBasedLine
The shift toward a singlerateCardproperty is sensible. Consider implementing a validation check to ensure the deprecated fields (price,featureKey) do not conflict withrateCarddata if both are set.
795-795: DeprecatingperUnitAmountinInvoiceFlatFeeLine
Marking this as deprecated is reasonable. Just be sure to finalize the plan to remove or replace these fields in a future major version, so there’s no drift between old and new usage.
811-811: Preserving the oldquantityfield
Retaining this field alongsiderateCard.quantitycould introduce confusion if both are set. You might consider an explicit usage policy or validation to prevent mismatched values.api/openapi.cloud.yaml (15)
13853-13859: Deprecation & Grammar – TaxConfig Field:
The addition ofdeprecated: truefor the TaxConfig field is consistently applied. Consider updating the description to “Tax config specifies the tax configuration for this line” for improved grammar and clarity.
13882-13888: Typo in Subscription Description:
There is a typo in the subscription description ("subscritpions" should be "subscriptions"). Please correct the spelling to ensure clarity.
13948-13954: Consistent Deprecation of TaxConfig:
The TaxConfig field is again marked as deprecated here. Consider revising its description for clarity and correct grammar (e.g., “Tax config specifies the tax configuration for this line”).
13970-13990: Repetition in Deprecated Flat Fee Fields:
This block repeats the deprecation of price, paymentTerm, and quantity fields along with the introduction of the newrateCardfield. Ensure consistency across schema definitions and consider consolidating these common elements to adhere to DRY principles.
14036-14042: TaxConfig Deprecation Consistency:
Again, theTaxConfigfield is flagged as deprecated. A grammatical improvement in its description is recommended for clarity.
14220-14330: Detailed Discount Schemas for Amount & Usage:
The extended definitions forInvoiceLineDiscountAmountandInvoiceLineDiscountUsageinclude comprehensive metadata and readOnly flags. A minor wording refinement is recommended—for example, changing “an usage-based discount” to “a usage-based discount.” Please also verify that all fields (e.g.,preLinePeriodQuantity) are appropriately documented and intended as read-only.
14616-14622: Reaffirming Deprecation of TaxConfig:
Once more, theTaxConfigfield is marked as deprecated. Confirm that this deprecation is intentional and that the description is revised for grammatical clarity.
14638-14659: Repeating Deprecation for Flat Fee Fields:
This block repeats the deprecation of the flat fee fields and adds the newrateCardfield as seen earlier. Please check for consistency with previous definitions and consider extracting common elements to minimize duplication if feasible.
14733-14739: TaxConfig Field – Clarity Improvement:
The deprecation flag remains consistent here; however, improving the description’s grammar (e.g., “Tax config specifies…”) would enhance clarity.
14986-14992: TaxConfig & Children Specification:
The deprecation ofTaxConfigis applied here, and thechildrenfield is defined as readOnly. Consider explicitly specifying the type forchildren(e.g., an array) if that is intended in the schema.
15015-15021: Subscription Description – Typo Reminder:
This block updates the subscription reference; however, the typo “subscritpions” persists. Please correct it to “subscriptions” to ensure clarity and professionalism.
15029-15049: Consistency in Usage-Based Pricing Definitions:
This block mirrors the changes made in hunk 15 for usage-based pricing. Ensure that the deprecation and the newrateCardfield definitions are consistent across all sections. Consolidating similar schema patterns where possible could enhance maintainability.
15088-15094: TaxConfig Deprecation Review:
TheTaxConfigfield is once again flagged as deprecated; please verify that this repeated pattern is intentional and consider revising the description text for improved clarity.
15170-15176: TaxConfig Field Repetition Notice:
Once again, theTaxConfigfield is marked as deprecated with similar wording. Ensure that this repetition is intentional and consider a refactor to reuse common schema segments if possible.
15192-15254: New InvoiceUsageBasedRateCard Schema & Discounts Description:
This extensive block introduces the newInvoiceUsageBasedRateCardschema and updates usage-based pricing fields. A couple of points need attention:
• The discounts description includes “Flat fee rate cards only support percentage discounts,” which might be a copy–paste error if this schema is for usage-based lines. Please verify and update this text if necessary.
• Ensure that properties such asfeatureKey,taxConfig, andpriceare clearly defined and aligned with the intended usage-based pricing model.
🛑 Comments failed to post (1)
api/openapi.yaml (1)
15266-15328:
⚠️ Potential issueNew Schema for InvoiceUsageBasedRateCard & Customer-Specific Usage Line (Hunk 22):
This extensive hunk introduces:
• A newcustomerIdfield (with an ID pattern) in the usage-based line create model.
• The InvoiceUsageBasedRateCard schema with properties such asfeatureKey,taxConfig,price, anddiscounts.Critical: The description under the
discountsproperty currently states:
“Flat fee rate cards only support percentage discounts.”
Given that this schema pertains to usage-based rate cards, the discount description should reflect their specific characteristics. Please update the description to accurately describe the discount capabilities for usage-based billing. For example:- Flat fee rate cards only support percentage discounts. + Usage-based rate cards support discounts calculated based on usage quantities.This correction will prevent confusion among API consumers and ensure that the schema documentation accurately reflects the functionality.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.discounts: type: array items: $ref: '#/components/schemas/Discount' description: |- The discounts of the rate card. Usage-based rate cards support discounts calculated based on usage quantities. title: Discounts
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
api/client/javascript/src/client/schemas.ts (1)
5104-5106:⚠️ Potential issueFix docstring reference to non-existent property.
The documentation for
amountstates that it's "calculated if percent present", but nopercentproperty exists in this interface.Apply this diff to fix the docstring:
- * @description Fixed discount amount to apply (calculated if percent present). + * @description Fixed discount amount to apply.
🧹 Nitpick comments (17)
api/openapi.yaml (12)
13927-13933: Deprecate Tax Config in Line Item.
The addition ofdeprecated: trueto the TaxConfig property clarifies that this field is being phased out. Ensure that both documentation and client integrations are updated to reflect this deprecation.
13956-13962: Correct Typo in Subscription Description.
The description for thesubscriptionproperty contains a typo ("subscritpions" instead of "subscriptions"). Please update the text for clarity.- description: Subscription are the references to the subscritpions that this line is related to. + description: Subscription references that this line is related to.
13970-13990: Deprecate Legacy Fields and Introduce RateCard.
This hunk marks the legacy fields (price,paymentTerm, andquantity) as deprecated and introduces a newrateCardproperty referencingInvoiceFlatFeeRateCard. Verify that all clients and documentation clearly delineate the migration to the new rate card model.
14044-14064: Consistent Deprecation for Line Attributes.
This segment deprecates theprice,paymentTerm, andquantityproperties while defining the newrateCard(InvoiceFlatFeeRateCard). Confirm that similar invoice line definitions use the same deprecation strategy.
14132-14153: Legacy Field Deprecation and RateCard Introduction.
This hunk reiterates deprecatingprice,paymentTerm, andquantitywhile adding the newrateCardfor invoice lines. Verify that transitions to the new model are clearly documented for consumers.
14159-14202: Add New InvoiceFlatFeeRateCard Schema.
A new schema,InvoiceFlatFeeRateCard, is introduced with fields for tax configuration, price details, discounts, and quantity. Ensure that required fields (likeprice) and default values (e.g., quantity default of 1) are clearly documented in client references and migration guides.
14294-14404: Detail InvoiceLineDiscountAmount and InvoiceLineDiscountUsage Schemas.
This extensive hunk details properties for both discount amount and usage schemas, including read-only flags and specific field definitions. Make sure that the descriptions (e.g., forpreLinePeriodQuantityand usage quantity) are consistent with the business logic and that the discriminator values align with the intended design.
14446-14450: Clarify Subscription Reference Description.
The description forInvoiceLineSubscriptionReferencehas been updated. For added clarity, consider ending with a period and verifying consistency with other subscription-related descriptions.
14712-14732: Reiterate Deprecation and Transition to RateCard.
The deprecation of legacy fields (price,paymentTerm, andquantity) and the introduction of a newrateCardproperty in this block should be cross-checked against other similar schema definitions for consistency and clarity.
15060-15066: Deprecate TaxConfig and Introduce Child Lines.
The deprecation ofTaxConfigcombined with the introduction of thechildrenproperty indicates a shift toward a more hierarchical invoice structure. Confirm that this design change is reflected in both the API documentation and client SDKs.
15089-15095: Fix Typo in Subscription Field Description (Repeated).
This block again includes thesubscriptionproperty with the same typo ("subscritpions"). Please make sure this typo is corrected consistently across all occurrences.
15285-15325: Define InvoiceUsageBasedRateCard Schema.
The newInvoiceUsageBasedRateCardschema defines critical properties likefeatureKey,taxConfig,price, anddiscounts. Ensure that the requiredpricefield and the constraints on other fields (e.g., string length, regex patterns) are clearly documented in the API reference documentation.api/openapi.cloud.yaml (3)
13882-13888: Typo in Subscription Description
There's a spelling error in the description ("subscritpions" should be "subscriptions"). Correcting this will improve clarity in the API docs.
14085-14131: Definition of InvoiceFlatFeeRateCard
This hunk introduces theInvoiceFlatFeeRateCardobject with properties such as taxConfig, price, discounts, and quantity. The structure is well-defined. Consider adding an explicit default value for thequantityproperty (currently only mentioned in the description) if that is intended behavior.
15110-15130: Usage-Based Pricing Details and ID Formatting
This block continues the deprecation of usage-based legacy fields and introduces anidproperty with a defined pattern. Ensure that the description forfeatureKey(now marked deprecated) clearly communicates its role. The new elements should be tested for format consistency.openmeter/billing/httpdriver/deprecations.go (2)
19-28: Struct looks good but consider clarifying deprecation strategy.This struct effectively groups rateCard logic with its deprecated counterparts. To improve maintainability, consider documenting (in comments or docstrings) the intended migration path away from these deprecated fields.
103-138: Fallback logic is well-structured but verify consistent defaults.When
RateCardis nil, this logic gracefully falls back to deprecated fields. Ensure any default fallback (e.g., payment term at line 134) is documented so that consumers know the default behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/client/javascript/src/client/schemas.ts(22 hunks)api/openapi.cloud.yaml(22 hunks)api/openapi.yaml(22 hunks)api/spec/src/billing/invoices/discounts.tsp(1 hunks)api/spec/src/billing/invoices/invoice.tsp(7 hunks)api/spec/src/billing/invoices/main.tsp(1 hunks)openmeter/billing/httpdriver/deprecations.go(1 hunks)openmeter/billing/httpdriver/deprecations_test.go(1 hunks)openmeter/billing/httpdriver/invoiceline.go(21 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- api/spec/src/billing/invoices/main.tsp
- openmeter/billing/httpdriver/deprecations_test.go
- api/spec/src/billing/invoices/discounts.tsp
🧰 Additional context used
🧬 Code Definitions (3)
api/client/javascript/src/client/schemas.ts (2)
api/client/go/client.gen.go (7)
InvoiceFlatFeeRateCard(2966-2983)InvoiceLine(3001-3003)InvoiceLineAppExternalIds(3006-3012)InvoiceLineDiscount(3015-3017)InvoiceLineDiscountAmount(3020-3050)InvoiceLineDiscountUsage(3058-3090)InvoiceUsageBasedRateCard(3616-3632)api/api.gen.go (7)
InvoiceFlatFeeRateCard(3226-3243)InvoiceLine(3261-3263)InvoiceLineAppExternalIds(3266-3272)InvoiceLineDiscount(3275-3277)InvoiceLineDiscountAmount(3280-3310)InvoiceLineDiscountUsage(3318-3350)InvoiceUsageBasedRateCard(3876-3892)
openmeter/billing/httpdriver/deprecations.go (3)
api/client/javascript/src/client/schemas.ts (5)
RateCard(9195-9195)InvoiceFlatFeeRateCard(9031-9032)TaxConfig(9262-9262)InvoiceUsageBasedRateCard(9083-9084)RateCardUsageBasedPrice(9205-9206)api/api.gen.go (6)
RateCard(4908-4910)InvoiceFlatFeeRateCard(3226-3243)TaxConfig(5702-5711)Price(4867-4869)InvoiceUsageBasedRateCard(3876-3892)RateCardUsageBasedPrice(5062-5064)api/client/go/client.gen.go (5)
RateCard(4586-4588)InvoiceFlatFeeRateCard(2966-2983)TaxConfig(5348-5357)InvoiceUsageBasedRateCard(3616-3632)RateCardUsageBasedPrice(4737-4739)
openmeter/billing/httpdriver/invoiceline.go (4)
openmeter/productcatalog/price.go (2)
InAdvancePaymentTerm(20-20)Price(80-87)api/client/go/client.gen.go (6)
RateCard(4586-4588)TaxConfig(5348-5357)PricePaymentTerm(4554-4554)InvoiceFlatFeeRateCard(2966-2983)FlatPriceWithPaymentTerm(2473-2483)InvoiceUsageBasedRateCard(3616-3632)api/client/javascript/src/client/schemas.ts (6)
RateCard(9195-9195)TaxConfig(9262-9262)PricePaymentTerm(9192-9192)InvoiceFlatFeeRateCard(9031-9032)FlatPriceWithPaymentTerm(8992-8993)InvoiceUsageBasedRateCard(9083-9084)openmeter/billing/invoiceline.go (8)
FlatFeeLine(251-258)Price(682-682)UsageBasedLine(684-693)LineDiscounts(743-745)LineDiscount(726-736)FlatFeeCategory(235-235)FlatFeeCategory(244-249)Line(271-284)
⏰ 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 (60)
api/openapi.yaml (9)
14022-14028: Deprecate Tax Config in This Block.
Once again,TaxConfigis marked as deprecated, and theperiodfield is introduced. Ensure consistency in deprecation messaging across all similar schema blocks.
14110-14116: Reaffirm Deprecation of TaxConfig.
Here theTaxConfigproperty is again flagged as deprecated. It is important to ensure that all references to TaxConfig in the API are uniformly updated and communicated.
14690-14696: Consistent Deprecation of TaxConfig.
This block once again deprecates theTaxConfigfield. Uniform communication across the API will help ensure clear migration paths.
14807-14813: Deprecate TaxConfig Consistently.
Tax configuration is again marked as deprecated here. Verify that similar warnings are present in all instances where TaxConfig is used.
15162-15168: Uniformly Deprecate TaxConfig.
The deprecation ofTaxConfigis consistently applied here. No additional action is needed, but ensure that the migration guidelines are updated accordingly.
15244-15250: Deprecate TaxConfig in Final Invoice Line Section.
Again,TaxConfigis marked as deprecated. This repetition reinforces the migration path; ensure all instances across the spec are updated.
15326-15328: InvoiceWorkflowInvoicingSettingsReplaceUpdate Stub.
The schemaInvoiceWorkflowInvoicingSettingsReplaceUpdatebegins here. Verify that its properties and intended use align with the new API design for workflow in invoicing, even though the diff does not show its complete implementation.
15266-15284:Details
❓ Verification inconclusive
Update and Deprecate Usage-Based Pricing Fields.
This block deprecatesRateCardUsageBasedPriceandfeatureKeywhile adding the newrateCardreferencingInvoiceUsageBasedRateCardand introducing a validatedcustomerId. The changes here form a comprehensive update for usage-based line creation. Verify that these modifications are mirrored in all related API endpoints and client implementations.
Attention: Ensure Consistent Update of Usage-Based Pricing Fields
Please confirm that the deprecation of
RateCardUsageBasedPriceandfeatureKey, along with the new addition ofrateCard(referencingInvoiceUsageBasedRateCard) and the validatedcustomerId, are consistently applied across all API endpoints and client implementations. Double-check any related endpoints or client-side schema references to ensure the comprehensive update is mirrored throughout the codebase.
14248-14261:Details
❓ Verification inconclusive
Introduce InvoiceLineDiscount with Discriminator.
The schemaInvoiceLineDiscountis redefined as a union (usingoneOf) with a discriminator on thetypeproperty. This is a robust way to support multiple discount types (amount and usage). Confirm that consumers of the API can correctly interpret the discriminator mapping.
InvoiceLineDiscount Schema Update – Verify API Consumer Integration
The schema in api/openapi.yaml now defines
InvoiceLineDiscountas a oneOf union with a discriminator on thetypeproperty that distinguishes between the discount types (InvoiceLineDiscountAmountfor "amount" andInvoiceLineDiscountUsagefor "usage"). This refactor is implemented as expected, offering a robust mechanism for handling multiple discount types.Please ensure that all consumers of the API can correctly interpret the discriminator mapping defined here. In particular, verify that your client integrations handle the following mapping correctly:
amount→ InvoiceLineDiscountAmount
usage→ InvoiceLineDiscountUsageLocation:
api/openapi.yaml, lines 14248–14261
🏁 Script executed:
#!/bin/bash # Display the InvoiceLineDiscount snippet to confirm the presence of the discriminator mapping. rg "InvoiceLineDiscount:" -A 15 api/openapi.yamlLength of output: 758
Verify Discriminator Integration in InvoiceLineDiscount Schema
The updated schema in api/openapi.yaml now correctly defines
InvoiceLineDiscountusing a union (oneOf) with a discriminator on thetypeproperty to distinguish betweenInvoiceLineDiscountAmountandInvoiceLineDiscountUsage. Please ensure that all API consumers properly handle the following mapping:
amount→ InvoiceLineDiscountAmountusage→ InvoiceLineDiscountUsageConfirmed location:
api/openapi.yaml, lines 14248–14261api/spec/src/billing/invoices/invoice.tsp (8)
639-641: Proper deprecation of taxConfig propertyThe code correctly deprecates the
taxConfigproperty and directs users to userateCard.taxConfiginstead, aligning with the PR's goal of moving line-specific fields to the rate-card object.
679-679: Field renamed from plural to singularThe property has been renamed from
subscriptionstosubscription, reflecting a change from potentially multiple subscriptions to a single subscription reference. This aligns with the PR objective to restructure the API model.
700-710: New InvoiceUsageBasedRateCard model properly structuredThe new model effectively encapsulates the rate card for usage-based lines, containing essential properties like discounts, taxConfig, featureKey, and price. This implementation aligns with the PR objective to move line-specific user input to a dedicated rate-card object.
Note the TODO comment indicating featureKey and price should be made required after deprecation. Consider adding a ticket to track this future change.
726-735: Appropriate deprecation of usage-based line propertiesThe
priceandfeatureKeyproperties are correctly deprecated with clear migration guidance, directing users to the corresponding properties on the newrateCardobject.
736-744: New rateCard property added to InvoiceUsageBasedLineThe addition of the
rateCardproperty successfully implements the PR objective of restructuring the rate-card object within the product catalog. Making it optional initially allows for a non-breaking transition.Note the TODO comment on line 742 regarding making the rateCard required after deprecation is complete. Consider creating a ticket to track this future change.
760-778: New InvoiceFlatFeeRateCard model properly implementedThe new model correctly encapsulates the rate card for flat fee lines, with appropriate properties for discounts, taxConfig, price, and quantity. This supports the PR's goal of restructuring the rate-card object.
Similar to the usage-based rate card, there's a TODO comment about making price required later. Consider tracking this in the same ticket as the other TODOs.
795-811: Appropriate deprecation of flat fee line propertiesThe
perUnitAmount,paymentTerm, andquantityproperties are properly deprecated with clear directives to use their equivalents in the newrateCardobject. This consistent approach will help API consumers understand the migration path.
813-819: New rateCard property added to InvoiceFlatFeeLineThe addition of the
rateCardproperty to theInvoiceFlatFeeLinemodel completes the implementation of the rate card restructuring for both major line types. Making it optional initially ensures backward compatibility.As with the other rate card properties, there's a TODO comment about making this required after deprecation. Ensure this is tracked for future implementation.
api/openapi.cloud.yaml (19)
13853-13859: Deprecation of TaxConfig Property
The addition ofdeprecated: truefor the TaxConfig reference appears to be intentional as part of the API restructuring. Please ensure that this deprecation is clearly documented and communicated to clients so that they know to migrate to the new configurations if applicable.
13896-13916: Transition from Legacy Fields torateCard
Deprecated fields, such as price, paymentTerm, and quantity, are being replaced by a newrateCardreference (pointing toInvoiceFlatFeeRateCard). This change aligns with the migration strategy. Make sure that the documentation (and any client code) reflects these updates to avoid confusion during integration.
13948-13953: Consistent Deprecation of TaxConfig
The TaxConfig reference is again flagged as deprecated in this block. Ensure that this deprecation is applied uniformly throughout the schema and that any fallback behavior is well documented.
13970-13990: Repetition of Deprecation & Introduction ofrateCard
This hunk mirrors the earlier pattern of deprecating price, paymentTerm, and quantity while introducing therateCardproperty. The change is consistent; just double-check that the deprecation schedule and migration guides cover all legacy fields accordingly.
14036-14042: Uniform Deprecation of TaxConfig
The deprecation of the TaxConfig reference is repeated here. Confirm that the update plan for clients includes all areas where TaxConfig is used.
14058-14079: Consistent Schema Update for Pricing Elements
Once again, fields like price, paymentTerm, and quantity are deprecated and replaced by the newrateCardconstruct. The changes are clear and consistent; ensure that API consumers are informed about the new pattern.
14174-14197: New InvoiceLineDiscount Schema & Variants
The newInvoiceLineDiscountschema using a oneOf discriminator (with mappings foramountandusage) is set up correctly. Please verify that the discriminator property (type) covers all intended discount types and that client libraries are updated appropriately.
14220-14330: Detailed Discount Schemas Defined
The definitions forInvoiceLineDiscountAmountandInvoiceLineDiscountUsageare comprehensive. They include proper type definitions, read-only flags, and examples. Ensure that the enum values (e.g.,"amount"in one and"usage"in the other) match what clients expect and are reflected in the API documentation.
14372-14378: Clarification on Subscription Reference and Tax Behavior
The description forInvoiceLineSubscriptionReferenceis updated here. For the subsequentInvoiceLineTaxBehavior, please confirm that its enum values are defined elsewhere in the specification and that its purpose is clearly documented.
14616-14622: Repeated Deprecation of TaxConfig Validated
Again, the TaxConfig reference is marked as deprecated. The consistency is good; just ensure that any legacy behaviors are fully replaced in the migration guide.
14638-14659: Legacy Fields Deprecated in This Block
The deprecation of fields like Numeric (for price and quantity) and PricePaymentTerm (including the default for paymentTerm) is handled as before. The integration of the newrateCardproperty should be referenced in the migration documentation.
14733-14739: TaxConfig Deprecation Consistency Check
This hunk continues to deprecate the TaxConfig reference. It is recommended to verify that all instances of TaxConfig deprecation follow the same pattern in both behavior and documentation.
14755-14775: Transition for Usage-Based Pricing Fields
Here,RateCardUsageBasedPriceandfeatureKeyare deprecated in favor of a newrateCardthat referencesInvoiceUsageBasedRateCard. This follows the intended API update for usage-based items. Ensure that any client or migration guides reference this change clearly.
14986-14992: Deprecation and Preservation of Child Elements
The deprecation of TaxConfig is paired with the preservation of thechildrenproperty. Verify that child elements’ behavior remains correct after the deprecation.
15015-15021: Duplicate Subscription Block with Persistent Typo
This block repeats the subscription property definition seen earlier. The description still contains "subscritpions" instead of "subscriptions." This duplication of both the field and the typo should be corrected to ensure consistency across the schema.
15029-15049: Usage-Based Pricing Block Consistency
This hunk mirrors the earlier usage-based pricing changes (deprecatingRateCardUsageBasedPriceandfeatureKey, replacing them withInvoiceUsageBasedRateCard). It appears consistent with prior changes. Just verify that all related clients are updated accordingly.
15088-15094: TaxConfig and Period Properties for Usage-Based Items
The deprecation of TaxConfig (with the addition of a period reference) in this block is consistent. No further issues are noted here.
15170-15176: Consistent Deprecation of TaxConfig (Repeated)
This hunk further confirms the deprecation of TaxConfig. It is consistent with previous implementations.
15192-15253: 🛠️ Refactor suggestion
⚠️ Potential issueCritical: Duplicate Description Keys in Usage-Based Schema Block
In this extensive hunk, there appears to be a duplication of thedescriptionkey:
- At line 15214, a description defines
InvoiceUsageBasedLineCreateWithCustomerand- Immediately at line 15215, another description for
InvoiceUsageBasedRateCardis provided.YAML mappings do not support duplicate keys, so the latter will override the former. This is likely an error in schema documentation. It is recommended to remove or merge these duplicate description entries so that each schema has a single, clear description. For example:
- description: InvoiceUsageBasedLineCreateWithCustomer represents the create model for an invoice line that is sold to the customer based on usage. - description: InvoiceUsageBasedRateCard represents the rate card (intent) for an usage-based line. + description: | + InvoiceUsageBasedLineCreateWithCustomer represents the create model for an invoice line sold to the customer based on usage.Then, ensure that the
InvoiceUsageBasedRateCardschema has its own dedicated description within its block (which starts at line 15216).Likely an incorrect or invalid review comment.
openmeter/billing/httpdriver/invoiceline.go (11)
31-34: Good addition of constants for default values.Adding constants for default flat fee payment term and quantity improves maintainability by centralizing these values, making them easier to update in the future.
125-135: Improved field mapping with centralized validation.The refactoring to use
mapAndValidateFlatFeeRateCardDeprecatedFieldscentralizes the mapping and validation logic, which is consistent with the PR objective of restructuring rate-card handling. This allows for better handling of both old and new fields during the transition period.
169-178: Consistent approach for usage-based lines.This change applies the same refactoring pattern to usage-based lines, maintaining consistency in the codebase and ensuring both types of invoice lines are handled similarly during the rate-card restructuring.
263-267: Enhanced error handling for discount mapping.The updated code now properly handles errors from discount mapping operations, improving robustness. This change ensures that any issues during discount mapping will be properly reported up the call stack.
300-309: New RateCard field implementation follows the design intent.The addition of the
RateCardfield to flat fee lines implements the restructuring plan described in the PR objectives. This populates both old and new fields in Get/List responses, providing a smooth transition path.
344-348: Improved error handling consistency.Similar to the flat fee line changes, this update enhances error handling for the usage-based line discount mapping, maintaining a consistent approach throughout the codebase.
382-387: RateCard field for usage-based lines.Adding the
RateCardfield to usage-based lines follows the same pattern as flat fee lines, ensuring consistency across different line types. This implements the API restructuring plan outlined in the PR objectives.
425-443: Updated discount mapping with error handling.The function now returns both the mapped discounts and any error that occurred during mapping, allowing callers to properly handle failures. The function correctly handles absent discounts by returning nil.
445-464: Improved discount mapping API.The updated function signature returns both the mapped discount and any error, consistent with the changes to
mapDiscountsToAPI. The function directly returns the error fromFromInvoiceLineDiscountAmount, which simplifies error handling.
502-549: Consistent pattern in simulation functions.The changes to the simulation functions follow the same pattern established in the main mapping functions, ensuring consistency across the codebase. These changes support the transition from old fields to the new rate-card structure.
612-726: Consistent handling in update functions.The replacement of direct field mapping with helper functions in the update logic ensures a consistent approach across the entire codebase. This is crucial for maintaining backward compatibility during the transition period while enforcing validation rules for the new API structure.
api/client/javascript/src/client/schemas.ts (7)
5066-5069: Well-structured discount type implementation.The use of a union type for
InvoiceLineDiscountprovides a clean approach to represent different discount types (amount-based vs usage-based), aligning with the PR objective of restructuring the discount system.
4853-4865: Consistent deprecation approach for fields moved to rate cards.The PR properly marks all fields being moved to rate card objects as deprecated, which maintains backward compatibility while encouraging migration to the new structure. This approach allows for a gradual transition.
Also applies to: 4915-4928, 4983-4996, 5455-5465, 5624-5632, 5683-5692, 5742-5751
5010-5039: Clear separation of rate card and line item concerns.The new
InvoiceFlatFeeRateCardinterface effectively separates the intent (rate card configuration) from the actual values (line items), as mentioned in the PR objectives. The structure is well-documented and includes all necessary properties.
5762-5785: Comprehensive implementation of usage-based rate card.The
InvoiceUsageBasedRateCardinterface properly captures all the necessary properties for usage-based billing scenarios and provides clear documentation. The inclusion of discounts as an array ofDiscountobjects allows for flexible discount application.
5117-5170: Well-defined usage-based discount structure.The
InvoiceLineDiscountUsageinterface provides a complete implementation for usage-based discounts, including properties for tracking quantities across billing periods. This aligns with the PR objective of including usage discounts as a non-breaking change.
4866-4867: Consistent addition of rate card references.The addition of the
rateCardproperty across multiple interfaces ensures a consistent approach to accessing rate card information, which simplifies API usage and improves maintainability.Also applies to: 4929-4930, 4997-4998, 5466-5469, 5633-5636, 5693-5696, 5752-5755
4844-4844: Consistent subscription reference implementation.The
subscriptionproperty has been updated to use the newInvoiceLineSubscriptionReferencetype, ensuring consistency across the codebase.Also applies to: 5617-5617
openmeter/billing/httpdriver/deprecations.go (6)
29-41: Validation ensures mandatory deprecated fields are present.Your checks for
PerUnitAmountandQuantitybeing non-nil correctly enforce the mandatory requirement for those fields. The simple errors.Join usage is concise and clean.
43-94: Consider numeric comparison for amounts ().As highlighted in your past review comment, comparing
rateCard.Price.Amountto*i.PerUnitAmount(line 60) might produce false mismatches if the strings differ by formatting (e.g.,"10.00"vs."10"). A more robust approach is to parse both strings into decimals and compare them numerically.
178-185: Usage-based struct is well-defined.Defining usage-based items separately helps keep flat-fee and usage-based logic distinct. This improves clarity of domain modeling. No immediate issues noted here.
187-199: Consistent validation approach for usage-based deprecated fields.Similar to the flat fee approach, you explicitly require
PriceandFeatureKey. This consistency is good.
249-293: Well-rounded mapping for usage-based rate cards.This final fallback ensures usage-based invoice lines rely on the new
RateCardif available or otherwise gracefully consume deprecated fields. The errors wrap remains coherent.
201-241:Details
✅ Verification successful
Ensure numeric parity of usage-based prices.
Your approach of comparing derived prices in
ValidateRateCard()is solid. However, confirm that allplanhttpdriver.AsPriceusage returns consistent decimal values—especially if dealing with fractional usage rates.
🏁 Script executed:
#!/bin/bash # Confirm usage-based price calculations call planhttpdriver.AsPrice consistently rg -A 3 'AsPrice'Length of output: 3123
Numeric Parity Verification for AsPrice Conversions
I confirmed via our search that every call to
planhttpdriver.AsPrice—across billing, product catalog, and test files—applies a consistent conversion strategy. In yourValidateRateCard()method, comparing the derived prices using the.Equal()method appears robust, and the handling of potential fractional values aligns with how it's done elsewhere. No further changes are necessary.
| allOf: | ||
| - $ref: '#/components/schemas/RateCardUsageBasedPrice' | ||
| description: Price of the usage-based item being sold. | ||
| deprecated: true | ||
| featureKey: | ||
| type: string | ||
| minLength: 1 | ||
| maxLength: 64 | ||
| pattern: ^[a-z0-9]+(?:_[a-z0-9]+)*$ | ||
| description: The feature that the usage is based on. | ||
| deprecated: true | ||
| rateCard: | ||
| allOf: | ||
| - $ref: '#/components/schemas/InvoiceUsageBasedRateCard' | ||
| description: |- | ||
| The rate card that is used for this line. | ||
|
|
||
| The rate card captures the intent of the price and discounts for the usage-based item. | ||
| quantity: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Transition to InvoiceUsageBasedRateCard for Usage-Based Items.
This hunk deprecates RateCardUsageBasedPrice and featureKey while introducing a reference to InvoiceUsageBasedRateCard for usage-based pricing. Ensure that the migration strategy for usage-based elements is clearly documented and that legacy clients are notified of these changes.
| allOf: | ||
| - $ref: '#/components/schemas/RateCardUsageBasedPrice' | ||
| description: Price of the usage-based item being sold. | ||
| deprecated: true | ||
| featureKey: | ||
| type: string | ||
| minLength: 1 | ||
| maxLength: 64 | ||
| pattern: ^[a-z0-9]+(?:_[a-z0-9]+)*$ | ||
| description: The feature that the usage is based on. | ||
| deprecated: true | ||
| rateCard: | ||
| allOf: | ||
| - $ref: '#/components/schemas/InvoiceUsageBasedRateCard' | ||
| description: |- | ||
| The rate card that is used for this line. | ||
|
|
||
| The rate card captures the intent of the price and discounts for the usage-based item. | ||
| quantity: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update Usage-Based RateCard Schema.
This segment deprecates the legacy usage-based price schema and the featureKey in favor of a new rateCard property referencing InvoiceUsageBasedRateCard. Verify that the updated schema correctly reflects the intended pricing and discount logic for usage-based items.
| allOf: | ||
| - $ref: '#/components/schemas/RateCardUsageBasedPrice' | ||
| description: Price of the usage-based item being sold. | ||
| deprecated: true | ||
| featureKey: | ||
| type: string | ||
| minLength: 1 | ||
| maxLength: 64 | ||
| pattern: ^[a-z0-9]+(?:_[a-z0-9]+)*$ | ||
| description: The feature that the usage is based on. | ||
| deprecated: true | ||
| rateCard: | ||
| allOf: | ||
| - $ref: '#/components/schemas/InvoiceUsageBasedRateCard' | ||
| description: |- | ||
| The rate card that is used for this line. | ||
|
|
||
| The rate card captures the intent of the price and discounts for the usage-based item. | ||
| id: | ||
| type: string | ||
| pattern: ^[0-7][0-9A-HJKMNP-TV-Za-hjkmnp-tv-z]{25}$ |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refine Usage-Based Schema with ID Specification.
This hunk continues the pattern for usage-based pricing by deprecating legacy fields and introducing a new rateCard property. The addition of the id field with a specific regex validation is a good detail—confirm that this pattern fits all expected IDs.
2 participants
Overview
Changes:
Rate-card
The issue is that the term discount is overloaded; it means two things in the context of invoicing:
By separating the intent (rate-card) and the calculated value (line itself) we are future-proofing the line as we have a staging area for the intent and the calculations itself.
Deprecating fields
The PR takes the following approach for the deprecated fields on the lines:
The line discount change is a breaking change, but it was not used before by our customers, and until usage discount is used the API will work as expected.
The invoice discounts API part is removed as is was never supported.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests