refactor: simplify invoicing api #2912
Conversation
📝 Walkthrough## Walkthrough
This change removes all support for flat fee invoice lines across the codebase and schemas, unifying invoice line handling around usage-based lines. It consolidates and renames related types, updates OpenAPI and TypeScript schemas, and refactors mapping and validation logic to reject flat fee lines, resulting in a single, detailed invoice line model.
## Changes
| Files/Paths | Change Summary |
|-------------------------------------------------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------|
| api/client/javascript/src/client/schemas.ts | Removed all flat fee invoice line types; added unified `InvoiceDetailedLine` and related types; updated `InvoiceLine` to a single detailed interface representing usage-based lines only; corrected document ref enum typo. |
| api/openapi.cloud.yaml<br>api/openapi.yaml | Removed flat fee line schemas; added `InvoiceDetailedLine` and related types; unified `InvoiceLine` schema; updated related input and simulation schemas; corrected document ref enum typo; moved related schemas to end of components. |
| api/spec/src/billing/invoices.tsp<br>api/spec/src/billing/invoices/invoice.tsp<br>api/spec/src/billing/invoices/docref.tsp | Removed discriminated unions for invoice lines and simulation lines; introduced unified models; corrected enum typo; deprecated and renamed related types and enums; simplified simulation line model. |
| openmeter/billing/httpdriver/deprecations.go<br>openmeter/billing/httpdriver/deprecations_test.go | Removed flat fee rate card deprecation logic and tests; refactored usage-based rate card logic to be generic for invoice lines; renamed structs, methods, and functions accordingly. |
| openmeter/billing/httpdriver/invoice.go | Updated mapping function to use new invoice line mapping logic (`mapInvoiceLineToAPI`). |
| openmeter/billing/httpdriver/invoiceline.go | Removed all flat fee line mapping, merging, and validation; consolidated logic to usage-based lines only; updated error handling to reject flat fee lines; removed discriminator-based branching; simplified merging and update logic. |
## Possibly related PRs
- [openmeterio/openmeter#2886](https://github.com/openmeterio/openmeter/pull/2886): Implements a usage-based flat fee line type to replace traditional flat fee lines, addressing invoice line type hierarchy and data model unification.
- [openmeterio/openmeter#2898](https://github.com/openmeterio/openmeter/pull/2898): Introduces feature-flagged helper functions for usage-based flat fee lines alongside legacy flat fee lines during migration, related to the transition from legacy flat fee lines.
- [openmeterio/openmeter#2873](https://github.com/openmeterio/openmeter/pull/2873): Adds sorting functionality for invoice lines, including flat fee lines, focusing on ordering rather than schema unification.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
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
turip
force-pushed
the
refactor/invocing-api
branch
4 times, most recently
from
4752aa9 to
46d128e
Compare
turip
force-pushed
the
refactor/invocing-api
branch
2 times, most recently
from
8108616 to
803c1b7
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: 6
♻️ Duplicate comments (1)
openmeter/billing/httpdriver/deprecations.go (1)
152-152: Same missing import issue affects this line.This line also uses
lo.FromPtrand will fail due to the missing import fixed above.🧰 Tools
🪛 GitHub Actions: CI
[error] 120-152: Golangci-lint: undefined identifier 'lo' used in multiple locations
🧹 Nitpick comments (14)
api/client/javascript/src/client/schemas.ts (1)
5354-5355: Clarify the comment to reflect the new architecture.The comment refers to "manually added fee" which could be misleading since flat fee support is being removed. Consider updating it to reflect that this represents detailed line items within usage-based lines.
- /** @description InvoiceDetailedLine represents a line item that is sold to the customer as a manually added fee. */ + /** @description InvoiceDetailedLine represents a detailed line item within a usage-based invoice line. */api/spec/src/billing/invoices/invoice.tsp (2)
423-423: Update outdated comment to reflect unified invoice line structure.The comment references "UBP invoice line" but since all invoice lines are now usage-based with the unified structure, this distinction is no longer relevant.
- * InvoiceLineReplaceUpdate represents the update model for an UBP invoice line. + * InvoiceLineReplaceUpdate represents the update model for an invoice line.
442-442: Simplify comment to remove redundant "usage-based" specification.Since all invoice lines now follow the usage-based structure with detailed children, the explicit mention of "usage-based" is redundant.
- * InvoicePendingLineCreate represents the create model for an invoice line that is sold to the customer based on usage. + * InvoicePendingLineCreate represents the create model for an invoice line.api/openapi.cloud.yaml (6)
15781-15788: Refine cost category description
TheInvoiceDetailedLineCostCategorydescription contains a duplicated "due to". Consider rephrasing to:
"Determines whether the flat fee is regular or commitment-based."
15789-15796: Add examples to InvoiceDetailedLine definitions
While the new descriptions clarify purpose, including anexampleblock forInvoiceDetailedLine,InvoiceDetailedLineCostCategory, andInvoiceDetailedLineRateCardwill improve client-side schema generation and documentation.
15879-15902: Add examples for new InvoiceLine fields
Includingname,description, andmetadatain theInvoiceLineschema is helpful, but addingexamplevalues for these properties will greatly aid API consumers and automated clients.
15950-15957: Fix typo in discounts description
Change"Discounts detailes applied to this line."to"Discounts details applied to this line."for correct spelling.
15972-15977: Mark deprecatedtaxConfigas read-only
SincetaxConfigis deprecated and not intended for client input, consider addingreadOnly: trueto align it with other deprecated output fields.
16230-16233: Grammar in update schema description
InInvoiceLineReplaceUpdate, change"an UBP invoice line"to"a UBP invoice line"for proper English usage.api/openapi.yaml (5)
16266-16290: Fix formatting and grammar inInvoiceGenericDocumentRefdescription
The Markdown list underdescriptionis missing hyphens and contains a typo (it's→its).- Omitted fields: - period: Tax period in which the referred document had an effect required by some tax regimes and formats. - stamps: Seals of approval from other organisations that may need to be listed. - ext: Extensions for additional codes that may be required. + Omitted fields: + - period: Tax period in which the referred document had an effect required by some tax regimes and formats. + - stamps: Seals of approval from other organisations that may need to be listed. + - ext: Extensions for additional codes that may be required. ... - title: InvoiceGenericDocumentRef is used to describe an existing document or a specific part of it's contents. + title: InvoiceGenericDocumentRef is used to describe an existing document or a specific part of its contents.
16420-16424: Consider usingconstfor the single-valuedtype
When a property only permits one literal (usage_based),constin OpenAPI is more explicit than a single-itemenum.
16617-16618: Optional: Remove the deprecatedtypeproperty
Since this schema no longer supports multiple line types, thetypefield is now redundant and could be removed to simplify the spec.
16862-16863: Remove the now-unusedInvoiceLineTypesenum
This schema is fully deprecated and no longer referenced; it can be safely deleted to clean up the spec.
666-666: Add missing operation descriptions for Spectral compliance
Spectral warnings cite several endpoints that need non-emptydescriptionfields:
/api/v1/apps/custom-invoicing/{invoiceId}/draft/synchronized.post/api/v1/apps/custom-invoicing/{invoiceId}/issuing/synchronized.post/api/v1/apps/custom-invoicing/{invoiceId}/payment/status.post/api/v1/meters/{meterIdOrSlug}/query.post/api/v1/subscriptions.post/api/v1/subscriptions/{subscriptionId}.getAlso applies to: 731-731, 796-796, 5554-5554, 9242-9242, 9310-9310
🧰 Tools
🪛 GitHub Actions: CI
[warning] 666-666: Spectral: Operation "description" must be present and non-empty string at path /api/v1/apps/custom-invoicing/{invoiceId}/draft/synchronized.post
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
api/client/javascript/src/client/schemas.ts(9 hunks)api/openapi.cloud.yaml(9 hunks)api/openapi.yaml(8 hunks)api/spec/src/billing/invoices.tsp(1 hunks)api/spec/src/billing/invoices/invoice.tsp(8 hunks)openmeter/billing/httpdriver/deprecations.go(6 hunks)openmeter/billing/httpdriver/deprecations_test.go(3 hunks)openmeter/billing/httpdriver/invoice.go(1 hunks)openmeter/billing/httpdriver/invoiceline.go(13 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
api/client/javascript/src/client/schemas.ts (2)
api/api.gen.go (3)
InvoiceDetailedLine(3442-3530)InvoiceDetailedLineCostCategory(3537-3537)InvoiceDetailedLineRateCard(3540-3556)api/client/go/client.gen.go (3)
InvoiceDetailedLine(3143-3231)InvoiceDetailedLineCostCategory(3238-3238)InvoiceDetailedLineRateCard(3241-3257)
🪛 GitHub Actions: CI
api/openapi.yaml
[warning] 666-666: Spectral: Operation "description" must be present and non-empty string at path /api/v1/apps/custom-invoicing/{invoiceId}/draft/synchronized.post
[warning] 731-731: Spectral: Operation "description" must be present and non-empty string at path /api/v1/apps/custom-invoicing/{invoiceId}/issuing/synchronized.post
[warning] 796-796: Spectral: Operation "description" must be present and non-empty string at path /api/v1/apps/custom-invoicing/{invoiceId}/payment/status.post
[warning] 5554-5554: Spectral: Operation "description" must be present and non-empty string at path /api/v1/meters/{meterIdOrSlug}/query.post
[warning] 9242-9242: Spectral: Operation "description" must be present and non-empty string at path /api/v1/subscriptions.post
[warning] 9310-9310: Spectral: Operation "description" must be present and non-empty string at path /api/v1/subscriptions/{subscriptionId}.get
openmeter/billing/httpdriver/deprecations.go
[error] 120-152: Golangci-lint: undefined identifier 'lo' used in multiple locations
🔇 Additional comments (17)
openmeter/billing/httpdriver/invoice.go (1)
559-559:✅ Verification successful
Verify the renamed mapping function exists and maintains compatibility.
The function call has been changed from
mapBillingLineToAPItomapInvoiceLineToAPI. Ensure this new function exists, has the same signature, and provides equivalent functionality.
🏁 Script executed:
#!/bin/bash # Verify that mapInvoiceLineToAPI function exists and check for any remaining references to mapBillingLineToAPI echo "Searching for mapInvoiceLineToAPI function definition..." ast-grep --pattern 'func mapInvoiceLineToAPI($$$) ($$$) { $$$ }' echo "Checking for any remaining references to old function name..." rg "mapBillingLineToAPI" -A 2 -B 2Length of output: 6600
Confirmation: Renamed mapping function exists and all references updated
mapInvoiceLineToAPIis defined in openmeter/billing/httpdriver/invoiceline.go (lines 274–351) with the signature
func mapInvoiceLineToAPI(line *billing.Line) (api.InvoiceLine, error)- No occurrences of the old
mapBillingLineToAPIremain in the codebaseNo further action required.
openmeter/billing/httpdriver/deprecations_test.go (1)
30-90: LGTM! Consistent renaming for unified invoice line handling.The function and type name changes from usage-based specific naming to generic invoice line naming are consistent with the refactoring goals of unifying invoice line handling. The removal of flat fee line tests aligns with the removal of flat fee support mentioned in the AI summary.
api/spec/src/billing/invoices.tsp (1)
566-594: Approve the API simplification but note the breaking change.The consolidation from a discriminated union to a single
InvoiceSimulationLinemodel significantly simplifies the API by removing the complexity of handling separate flat fee and usage-based line types. This aligns well with the PR objective of simplifying the invoicing API.Note: This represents a breaking change for API clients that previously relied on the discriminated union. Ensure appropriate migration documentation and versioning strategies are in place.
openmeter/billing/httpdriver/deprecations.go (1)
16-155: Approve the renaming for unified invoice line handling.The renaming from usage-based specific types and functions to generic invoice line naming is consistent with the refactoring goals. The removal of flat fee support and consolidation into unified invoice line handling simplifies the codebase.
🧰 Tools
🪛 GitHub Actions: CI
[error] 120-152: Golangci-lint: undefined identifier 'lo' used in multiple locations
api/client/javascript/src/client/schemas.ts (1)
5624-5644: Well-structured consolidation of invoice line types.The implementation correctly consolidates the invoice line structure with usage-based lines containing detailed child lines. The quantity fields provide comprehensive tracking of usage with and without discounts, including support for progressive billing scenarios.
api/spec/src/billing/invoices/invoice.tsp (1)
456-461: Well-executed refactoring to unify invoice line structure.The refactoring successfully consolidates the invoice line models into a coherent hierarchical structure where:
- All invoice lines are usage-based with potential detailed children
- Clear deprecation messages guide migration
- Renamed models (
InvoiceDetailedLine,InvoiceDetailedLineRateCard, etc.) better reflect their purpose- The parent-child relationship between usage-based lines and detailed lines is intuitive
This simplification will improve API usability and maintainability.
Also applies to: 537-539, 661-691, 735-824
api/openapi.cloud.yaml (5)
15628-15631: Review required fields for InvoiceDetailedLine
The newInvoiceDetailedLineschema currently only listsnameas required. Please verify that all necessary properties (e.g.,rateCard,category, pricing fields) are marked as required according to your design.
15758-15763: Mark deprecatedflat_feetype as read-only
AddingreadOnly: trueto the deprecatedtype: flat_feefield correctly prevents clients from sending it.
15834-15834: Approve rate card schema description
The description forInvoiceDetailedLineRateCardclearly communicates its intent in the flat-fee context.
16426-16502: Ensure required parameters for pending line creation
TheInvoicePendingLineCreateschema currently lacksquantity, which is generally required for usage-based lines. Please verify ifquantity(and any other fields likefeatureKeyorprice) should be marked as required here.
16736-16736: Approve simulation line description
The updated description forInvoiceSimulationLineclearly reflects its role as input to the simulation endpoint.openmeter/billing/httpdriver/invoiceline.go (3)
274-351: Well-implemented type validation and mapping.The function correctly validates that lines must be usage-based and properly maps all usage-based fields. Good defensive programming with the type check at the beginning.
208-262:❌ Incorrect review comment
Inconsistent handling of flat fee types in detailed line mapping.
The
mapDetailedLineToAPIfunction still maps to flat fee types and accessesline.FlatFeefields, which contradicts the goal of removing flat fee support. This function appears to handle child lines that are still expected to be flat fee type.If flat fee support is being fully removed, this function needs refactoring. If child lines are still allowed to be flat fee type, consider adding a comment explaining this design decision.
🏁 Script executed:
#!/bin/bash # Check if there are other references to FlatFee fields or types in the codebase rg -A 3 "FlatFee" --type go # Check for InvoiceDetailedLineTypeFlatFee usage ast-grep --pattern 'api.InvoiceDetailedLineTypeFlatFee'Length of output: 66351
mapDetailedLineToAPI intentionally handles flat fee lines
Our repository still includes extensive FlatFee support—migrations, tests, HTTP mappings, and core types all reference FlatFee. Removing flat-fee handling here would break the existing detailed-line API. The mapping in
mapDetailedLineToAPIis therefore correct and should remain unchanged.Likely an incorrect or invalid review comment.
108-113:❌ Incorrect review comment
Move flat fee validation before service call to prevent invalid line creation.
The flat fee validation happens after
h.service.CreatePendingInvoiceLineshas already been called, which means invalid lines might already be persisted. This validation should occur before the service call or be handled within the service layer.Consider moving this validation to before line 93 or ensuring the service layer handles this validation:
func(ctx context.Context, request CreatePendingLineRequest) (CreatePendingLineResponse, error) { + // Validate before service call + if h.featureSwitches.UseUsageBasedFlatFeeLines { + for _, line := range request.Lines { + if line.Type == billing.InvoiceLineTypeFee { + return CreatePendingLineResponse{}, billing.ValidationError{ + Err: fmt.Errorf("flat fee lines are not supported"), + } + } + } + } + res, err := h.service.CreatePendingInvoiceLines(ctx, request) if err != nil { return CreatePendingLineResponse{}, fmt.Errorf("failed to create invoice lines: %w", err) }
🏁 Script executed:
#!/bin/bash # Show context around the service call in the HTTP driver rg -n -C5 "CreatePendingInvoiceLines" openmeter/billing/httpdriver/invoiceline.go # Show location of flat fee validation in the same file rg -n -C5 "if .*Type == billing.InvoiceLineTypeFee" openmeter/billing/httpdriver/invoiceline.goLength of output: 2907
Flat‐fee validation already occurs before the service call
The validation loop in the first handler function (lines 73–80) rejects flat‐fee lines when
UseUsageBasedFlatFeeLinesis enabled and returns aValidationError, preventingh.service.CreatePendingInvoiceLinesfrom ever being invoked with unsupported lines.• File: openmeter/billing/httpdriver/invoiceline.go
– Lines 73–80: early flat‐fee check in the mapping function
– Line 93: service call only happens after mapping succeedsNo change needed; the suggested move is redundant.
Likely an incorrect or invalid review comment.
api/openapi.yaml (3)
15992-16199: ApproveInvoiceDetailedLineschema
The newInvoiceDetailedLineobject and its supporting schemas appear well-defined and align with the PR goal of unifying flat-fee lines into a detailed line model.
16722-16725: Approve changes toInvoiceLineReplaceUpdaterequired fields
Ensuringname,period, andinvoiceAtare required aligns with the new unified update model.
17298-17298: Approve newInvoiceSimulationLinedescription
Inline documentation for the simulation schema is concise and accurate.
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
api/client/javascript/src/client/schemas.ts (1)
5354-5645: 💡 Verification agent🧩 Analysis chain
Verify API consumers are prepared for breaking changes.
This refactoring removes all flat fee invoice line types (InvoiceFlatFeeLine, InvoiceFlatFeeLineReplaceUpdate, etc.) which is a breaking change. While deprecated fields provide some backward compatibility, consumers will need to:
- Migrate from removed flat fee types to the new InvoiceDetailedLine structure
- Update code expecting discriminated unions to handle the unified model
- Adapt to usage-based lines (InvoiceLine) having children of type InvoiceDetailedLine
Please ensure that:
- API migration documentation is provided
- Deprecation notices include clear migration paths
- API versioning strategy accommodates these breaking changes
🌐 Web query:
What are the best practices for handling breaking changes in TypeScript API schemas while maintaining backward compatibility?💡 Result:
Best Practices for Handling Breaking Changes in TypeScript API Schemas While Maintaining Backward Compatibility
Effectively managing breaking changes in TypeScript API schemas is critical for minimizing disruption to API consumers and ensuring a smooth evolution of your API. Here are the best practices, with a focus on backward compatibility:
Prefer Additive Changes Over Modifying or Removing Existing Features
- Whenever possible, add new optional properties or methods instead of altering or removing existing ones. This ensures clients using older versions do not encounter runtime or compile-time errors[8].
- For example, if you need to remove a property, first mark it as optional and deprecated, giving consumers time to adapt before it is removed completely in a future major version[3].
Use Options Objects Instead of Positional Parameters
- Designing functions to accept a single options object (e.g.,
function foo(options: FooOptions)) simplifies API evolution and backward compatibility. You can easily add new options as optional properties without impacting existing consumers, whereas changing positional parameters is more error-prone[3].Employ the Data Transfer Object (DTO) Pattern for External Data
- DTOs help decouple API schemas from your application's internal models. Use mapping functions to transform between versions, making it easier to support multiple API versions or partial changes without affecting the rest of your codebase. Mark fields as optional to handle differences between versions gracefully[6].
Implement API Versioning
- When breaking changes are unavoidable, introduce new API versions (e.g.,
/v1/endpointand/v2/endpoint). Maintain old versions for a transition period, allowing consumers to migrate at their own pace[7][9][10].- Communicate versioning policies clearly and maintain multiple active versions when necessary.
Test for Breaking Changes at the Type Level
- Use TypeScript interfaces to define the public API. Write automated tests to ensure that instances of the new API remain assignable to the previous version's types. This allows breaking changes to be detected at build time, reducing the risk of runtime issues[5].
Utilize Semantic Versioning
- Adopt semantic versioning (MAJOR.MINOR.PATCH). Increment the major version for breaking changes, minor for backward-compatible functionality, and patch for bug fixes. This communicates the nature of changes to your consumers[10].
- Document all changes in changelogs and notify users of upcoming breaking changes in advance.
Deprecate Before Removing
- Use JSDoc
@deprecatedtags or similar mechanisms to highlight deprecated fields, properties, or methods. Announce their planned removal in release notes and documentation so that consumers have time to adjust[3].Automate Compatibility Testing
- Integrate compatibility checks into your CI/CD pipeline. Run tests to ensure the new schema can still be consumed safely by clients expecting the older version, and alert when compatibility is broken[7].
Communicate Clearly and Set Deprecation Timelines
- Maintain thorough documentation detailing API changes and version differences. Announce major changes and deprecations well in advance, providing migration guides and timelines for retiring old versions[8][10].
Summary Table: Key Practices
Practice Why It Matters Additive Changes Prevents breaking existing consumers Use Options Objects Simplifies adding new features/fields DTO Pattern Decouples data models, eases multi-version support API Versioning Enables parallel support for breaking changes Type-Level Compatibility Tests Detects breaking changes early Semantic Versioning Communicates impact of changes to consumers Deprecation Notices Warns consumers before removal Automated Backward Compatibility Tests Ensures ongoing support for old clients Clear Communication Minimizes surprise, eases transitions By rigorously following these practices, TypeScript API maintainers can evolve their codebases while minimizing disruption and ensuring a stable, predictable experience for API consumers[3][5][6][7][8][9][10].
Citations:
- 1: https://github.com/microsoft/TypeScript/wiki/API-Breaking-Changes
- 2: https://github.com/microsoft/TypeScript/wiki/Breaking-Changes
- 3: https://michaelscodingspot.com/typescript-api-change/
- 4: https://javascript-conference.com/blog/typescript-5/
- 5: https://lostintime.dev/2021/01/02/typescript-api-breaking-changes.html
- 6: https://codewithstyle.info/typescript-dto/
- 7: https://www.xmatters.com/blog/api-versioning-strategies
- 8: https://endgrate.com/blog/api-versioning-best-practices-for-backward-compatibility
- 9: https://hackernoon.com/hacking-backward-and-forward-compatible-application-releases
- 10: https://daily.dev/blog/api-versioning-strategies-best-practices-guide
Add a clear migration guide and bump major version for InvoiceLine overhaul
To minimize disruption for API consumers and follow best practices for breaking changes, please:
- Document the change in your changelog (e.g. v2.0.0):
- Clearly state that all flat-fee line types have been removed in favor of
InvoiceDetailedLinechildren onInvoiceLine.- Provide ↔ “before & after” examples mapping old models (e.g.,
InvoiceFlatFeeLine) to the new schema.- Update JSDoc
@deprecatedtags on removed/legacy fields to include migration hints, e.g.:/** * @deprecated Use `InvoiceDetailedLine` instead. Will be removed in v3.0.0. */ taxConfig?: components['schemas']['TaxConfig']- Introduce or update a dedicated Migration Guide in your docs (Markdown or site) covering:
- How to transform existing code consuming the flat-fee types into the new discriminated-union model.
- Handling of
children: InvoiceDetailedLine[]underInvoiceLine.- Bump the client library’s major version per semantic versioning to signal this breaking change.
- Add automated type‐level compatibility tests in CI to detect regressions for consumers still on the old model.
These steps will ensure a smooth migration path, clear communication, and maintain backward compatibility for a transition period.
♻️ Duplicate comments (4)
api/client/javascript/src/client/schemas.ts (1)
3229-3229: Typo fix applied correctly.The previously identified typo in the enum value has been fixed.
api/openapi.yaml (1)
16990-16994:⚠️ Potential issueRequire
rateCardinInvoicePendingLineCreate.
A rate card is mandatory for usage-based lines; please add- rateCardto therequiredlist.required: - name - period - invoiceAt + - rateCardapi/openapi.cloud.yaml (1)
12779-12779: Typo fixed – duplicate past comment
This corrects the enum entry from"credit_node_original_invoice"to"credit_note_original_invoice".openmeter/billing/httpdriver/invoiceline.go (1)
596-596: Past review comment has been resolved.The line type is now correctly set to
billing.InvoiceLineTypeUsageBasedinstead of the previously incorrectbilling.InvoiceLineTypeFee. This fixes the inconsistency that was flagged in the previous review.
🧹 Nitpick comments (3)
api/openapi.cloud.yaml (1)
15796-15797: Refine grammar inInvoiceDetailedLineCostCategorydescription
The phrase “determines if the flat fee is a regular fee due to use due to a commitment” is unclear. Consider rewriting, e.g.:Determines whether the flat fee applies as a regular charge or under a predefined commitment.
openmeter/billing/httpdriver/invoiceline.go (2)
184-206: Clarify the business logic for usage-based children validation.The validation at lines 194-196 rejects usage-based lines as children, which appears to enforce that usage-based lines can only have flat fee detail children. This seems intentional but could benefit from a comment explaining the business rule.
+ // Usage-based lines can only have flat fee detail children, not other usage-based lines if child.Type == billing.InvoiceLineTypeUsageBased { return nil, fmt.Errorf("usage based lines are not supported as children of usage based lines") }
619-688: Complex merging logic is well-structured but consider refactoring.The merging function handles multiple complex validation scenarios (discount updates, period changes, progressive billing constraints). While the implementation appears correct, the function is quite large and handles multiple responsibilities.
Consider extracting smaller validation functions for better maintainability:
validateDiscountUpdatesForProgressiveBillingvalidatePeriodChangesForSplitLinesapplyLineUpdatesThe validation logic itself is comprehensive and handles important edge cases properly, particularly the progressive billing discount constraint and split line period change restrictions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
api/client/javascript/src/client/schemas.ts(10 hunks)api/openapi.cloud.yaml(10 hunks)api/openapi.yaml(10 hunks)api/spec/src/billing/invoices/docref.tsp(1 hunks)openmeter/billing/httpdriver/deprecations.go(6 hunks)openmeter/billing/httpdriver/invoiceline.go(13 hunks)
✅ Files skipped from review due to trivial changes (1)
- api/spec/src/billing/invoices/docref.tsp
🚧 Files skipped from review as they are similar to previous changes (1)
- openmeter/billing/httpdriver/deprecations.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
api/client/javascript/src/client/schemas.ts (2)
api/api.gen.go (3)
InvoiceDetailedLine(3442-3530)InvoiceDetailedLineCostCategory(3537-3537)InvoiceDetailedLineRateCard(3540-3556)api/client/go/client.gen.go (3)
InvoiceDetailedLine(3143-3231)InvoiceDetailedLineCostCategory(3238-3238)InvoiceDetailedLineRateCard(3241-3257)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: Artifacts / Benthos Collector Container image
- GitHub Check: Artifacts / Container image
- GitHub Check: Test
- GitHub Check: Quickstart
- GitHub Check: Migration Checks
- GitHub Check: Lint
- GitHub Check: E2E
- GitHub Check: Developer environment
- GitHub Check: Commit hooks
- GitHub Check: Build
- GitHub Check: CI
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (go)
🔇 Additional comments (24)
api/client/javascript/src/client/schemas.ts (9)
5354-5358: New InvoiceDetailedLine type introduced correctly.The new type definition aligns with the PR objective to simplify the invoicing API by consolidating line item types.
5437-5441: Deprecated type field maintains backward compatibility.The fixed type value of 'flat_fee' in InvoiceDetailedLine is appropriately marked as deprecated, providing a transition path while the main invoice lines are consolidated to usage-based only.
5459-5529: Schema definitions are well-structured and consistent.The new type definitions for InvoiceDetailedLineRateCard, InvoiceDetailedLineCostCategory, and document references are properly structured. The InvoiceLine type (formerly InvoiceUsageBasedLine) correctly represents the consolidated usage-based line model.
5540-5595: InvoiceLine fields are comprehensive and well-documented.The field definitions include proper TypeScript types, readonly modifiers where appropriate, and clear deprecation notices for legacy fields like taxConfig.
5600-5645: Usage-based line structure properly supports the simplified API.The InvoiceLine type correctly represents usage-based lines with:
- Fixed type 'usage_based' (appropriately deprecated)
- Children array for nested InvoiceDetailedLine items
- Comprehensive quantity tracking fields for various billing scenarios
- Proper deprecation of legacy fields in favor of the rateCard approach
5709-5776: Update and reference types are well-designed.The InvoiceLineReplaceUpdate type properly supports both updates and creation of new lines with its optional id field. The status enum and subscription reference types provide necessary relationships and state management.
5893-5939: Pending line creation type is consistent with the schema design.The InvoicePendingLineCreate type properly supports creating new usage-based lines with all necessary fields and maintains backward compatibility through deprecated fields.
6031-6038: Simulation type correctly limited to usage-based lines.The InvoiceSimulationLine type aligns with the simplified API by supporting only usage-based line simulations.
10370-10384: Export declarations are complete and correct.All new types including InvoiceDetailedLine, InvoiceDetailedLineCostCategory, and InvoiceDetailedLineRateCard are properly exported for use by API consumers.
api/openapi.yaml (8)
12943-12943: Corrected typo in enum value.
The enum now correctly usescredit_note_original_invoiceinstead of the misspelled variant.
15992-16199: IntroduceInvoiceDetailedLineschema.
The new flat-fee line model is well-defined with required fields, readOnly/deprecated flags on legacy properties, and supporting sub-schemas (InvoiceDetailedLineRateCard,InvoiceDetailedLineCostCategory).
16255-16257: Corrected typo inInvoiceDocumentRefTypeenum.
The value now properly readscredit_note_original_invoice.
16418-16481: Deprecate legacy fields and consolidateInvoiceLine.
The schema correctly limitstypetousage_based, deprecatespriceandfeatureKey, and introduceschildren(nestedInvoiceDetailedLine) for flat-fee details. Please verify whetherpriceshould also be markedreadOnly: truefor consistency with other deprecated properties.
16617-16618: Deprecatetypein update model.
Markingtypeas deprecated and readOnly in the replace/update schema prevents clients from changing a line’s type.
16722-16725: Require core fields inInvoiceLineReplaceUpdate.
Addingname,period, andinvoiceAttorequiredensures that updates cannot omit essential data.
16862-16863: DeprecateInvoiceLineTypesenum.
Flagging this enum as deprecated guides clients toward the unified line models.
17298-17298: UpdateInvoiceSimulationLinedescription.
The description now clearly explains that this schema represents a usage-based line for the simulation endpoint.api/openapi.cloud.yaml (4)
15835-15844: Verify completeness ofInvoiceDocumentRefTypeenum
TheInvoiceDocumentRefTypeonly includescredit_note_original_invoice. Confirm whether other reference types (e.g.,invoice,credit_note) should be supported or if this is intentional.
16023-16028: Confirmchildrenusage and constraints
Thechildrenarray now nestsInvoiceDetailedLine. Verify if additional constraints (e.g.,minItems,uniqueItems) or write-permissions are needed. Otherwise, this addition aligns with the unified schema.
16429-16432: Review required fields forInvoicePendingLineCreate
Addingname,period, andinvoiceAtas required aligns with the unified model’s expectations. No additional required fields appear missing.
16736-16736: Add description forInvoiceSimulationLine
This clarifies the schema’s intent and matches the level of documentation in related models.openmeter/billing/httpdriver/invoiceline.go (3)
108-113: Consistent validation for flat fee rejection.The validation logic correctly rejects flat fee lines in the response mapping, which aligns with the broader removal of flat fee support from the API.
274-277: Good defensive validation for line type consistency.The validation ensures only usage-based lines are processed by this function, which is consistent with the refactoring goals and provides clear error messages.
662-668: Robust validation for progressive billing discount updates.The validation correctly prevents discount updates on progressively billed lines, with clear error messaging. The comment block (lines 653-660) provides excellent context for why this restriction exists and potential future solutions.
turip
added
area/api
release-note/deprecation
turip
force-pushed
the
refactor/invocing-api
branch
from
a235340 to
fcb42c7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
openmeter/billing/httpdriver/invoiceline.go (1)
208-262:⚠️ Potential issueAdd null safety check for FlatFee fields.
The function accesses
line.FlatFeefields without verifying thatline.FlatFeeis not nil or that the line type is actually a flat fee line. This could cause a panic if called with unexpected line types.Add a safety check at the beginning of the function:
func mapDetailedLineToAPI(line *billing.Line) (api.InvoiceDetailedLine, error) { + if line.Type != billing.InvoiceLineTypeFee || line.FlatFee == nil { + return api.InvoiceDetailedLine{}, fmt.Errorf("line is not a flat fee line [line=%s, type=%s]", line.ID, line.Type) + } + discountsAPI, err := mapDiscountsToAPI(line.Discounts)
♻️ Duplicate comments (6)
api/openapi.cloud.yaml (5)
15784-15788: Makecategorywritable
MarkingcategoryasreadOnlyprevents client-specified cost categories. It should be writable in requests.
15879-15890: Remove deprecatedtypefromrequired
Thetypefield is fixed tousage_basedand marked deprecated; keeping it inrequiredis unnecessary. Please drop it from theInvoiceLinerequired list.
16189-16193: Remove deprecatedtaxConfigfrom replace update model
ThetaxConfigproperty remains inInvoiceLineReplaceUpdateeven though it’s deprecated. It should be removed to clean up the schema.
15628-15631: 🛠️ Refactor suggestionEnforce essential required properties
InvoiceDetailedLinecurrently only requiresname. To ensure data integrity, consider marking critical fields likeid,price,rateCard, and timestamps (e.g.,createdAt,updatedAt) as required.
15758-15763:⚠️ Potential issueRemove residual
flat_feeenum entry
Theflat_feevalue remains in thetypeenum but should be fully removed to honor the shift toward usage-based-only invoice lines.api/openapi.yaml (1)
16266-16290: Missingidin InvoiceGenericDocumentRef & fix title typo
As previously noted,InvoiceGenericDocumentRefrequires anidproperty (e.g.$ref: '#/components/schemas/IDResource') to uniquely identify documents. Also correct the title from "part of it's contents" to "part of its contents."
🧹 Nitpick comments (6)
api/openapi.cloud.yaml (3)
15795-15797: Refine description clarity
TheInvoiceDetailedLineCostCategorydescription is awkward (“due to use due to a commitment”). Rephrase to:
“Determines whether the flat fee is a regular usage fee or a commitment fee.”
16002-16003: Typo insubscriptiondescription
The description reads “subscritpions” instead of “subscriptions.” Please correct the spelling.
16451-16455: Consider removing deprecatedtaxConfigfrom create model
SincetaxConfigis marked deprecated, you may want to drop it fromInvoicePendingLineCreateto streamline the payload.api/openapi.yaml (3)
16065-16073: Fix typo in discounts description
Change "Discounts detailes applied to this line." to "Discounts details applied to this line."
16115-16119: Correct typo and grammar in subscription description
Update "subscritpions" to "subscriptions" and adjust grammar, e.g., "The subscription references related to this line."
16160-16161: Clarify InvoiceDetailedLineCostCategory description
Revise to "Determines if the flat fee is a regular fee or a commitment fee." for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
api/client/javascript/src/client/schemas.ts(10 hunks)api/openapi.cloud.yaml(10 hunks)api/openapi.yaml(10 hunks)api/spec/src/billing/invoices.tsp(1 hunks)api/spec/src/billing/invoices/docref.tsp(1 hunks)api/spec/src/billing/invoices/invoice.tsp(8 hunks)openmeter/billing/httpdriver/deprecations.go(6 hunks)openmeter/billing/httpdriver/deprecations_test.go(3 hunks)openmeter/billing/httpdriver/invoice.go(1 hunks)openmeter/billing/httpdriver/invoiceline.go(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- openmeter/billing/httpdriver/invoice.go
- api/spec/src/billing/invoices/docref.tsp
- api/spec/src/billing/invoices.tsp
- openmeter/billing/httpdriver/deprecations_test.go
- openmeter/billing/httpdriver/deprecations.go
- api/spec/src/billing/invoices/invoice.tsp
🧰 Additional context used
🧬 Code Graph Analysis (1)
api/client/javascript/src/client/schemas.ts (2)
api/api.gen.go (3)
InvoiceDetailedLine(3442-3530)InvoiceDetailedLineCostCategory(3537-3537)InvoiceDetailedLineRateCard(3540-3556)api/client/go/client.gen.go (3)
InvoiceDetailedLine(3143-3231)InvoiceDetailedLineCostCategory(3238-3238)InvoiceDetailedLineRateCard(3241-3257)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: Artifacts / Container image
- GitHub Check: Artifacts / Benthos Collector Container image
- GitHub Check: CI
- GitHub Check: Commit hooks
- GitHub Check: E2E
- GitHub Check: Developer environment
- GitHub Check: Quickstart
- GitHub Check: Migration Checks
- GitHub Check: Build
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: Analyze (go)
🔇 Additional comments (37)
api/client/javascript/src/client/schemas.ts (12)
3229-3229: Typo fix correctly applied.The enum value has been corrected from 'credit_node_original_invoice' to 'credit_note_original_invoice', addressing the previous review comment.
5354-5355: Clear documentation for the new unified detailed line type.The description accurately reflects that
InvoiceDetailedLinerepresents manually added fees, which aligns with the refactoring to consolidate flat fee functionality into detailed lines.
5437-5441: Proper deprecation marking for legacy flat fee type.The
type: 'flat_fee'field is correctly marked as deprecated, maintaining backward compatibility while signaling the transition to the new unified model.
5459-5497: Well-structured rate card model for detailed lines.The
InvoiceDetailedLineRateCardprovides comprehensive pricing configuration including tax config, discounts, and quantity handling, supporting the unified detailed line approach.
5504-5504: Enum value correctly matches the fixed type.The
InvoiceDocumentRefTypeenum value properly uses 'credit_note_original_invoice', consistent with the typo fix in the first hunk.
5517-5524: Generic document reference type provides good extensibility.The
InvoiceGenericDocumentReftype is well-designed with optional reason and description fields, supporting flexible document referencing needs.
5525-5526: Clear distinction in the unified line model.The comment and type name clearly indicate that
InvoiceLinenow represents usage-based lines specifically, which aligns with the refactoring to separate usage-based and detailed line types.
5624-5624: Proper parent-child relationship established.The
childrenfield linking toInvoiceDetailedLine[]correctly implements the new hierarchical model where usage-based lines can contain detailed line children.
5709-5762: Consistent update model for usage-based lines.The
InvoiceLineReplaceUpdatetype maintains consistency with the mainInvoiceLineschema while properly supporting update operations with optional ID for new line creation.
5893-5939: Unified pending line creation model.The
InvoicePendingLineCreatetype correctly focuses on usage-based lines only, consistent with the overall refactoring approach to unify line handling.
6034-6035: Simulation interface aligned with usage-based model.The
InvoiceSimulationLinetype correctly represents usage-based lines for simulation purposes, maintaining consistency with the unified schema approach.
10373-10377: Proper type exports for the new detailed line types.The exported types correctly include all the new
InvoiceDetailedLinerelated types, ensuring they're available for external consumption.api/openapi.cloud.yaml (6)
12779-12779: Fix typo in enum value
Corrects the invoice document reference enum from"credit_node_original_invoice"to"credit_note_original_invoice", aligning with the intended naming.
15781-15782: Consolidate rate card via$ref
Switching toallOf: ['$ref: InvoiceDetailedLineRateCard']DRYs up the schema and centralizes rate card definitions—good improvement.
16023-16028: Introducechildrenproperty for detailed lines
NestingInvoiceDetailedLineitems under achildrenarray enhances the unified invoice line model.
16429-16431: Verify required properties for pending line creation
InvoicePendingLineCreateonly requiresname,period, andinvoiceAt. For usage-based lines, shouldprice(orrateCard) also be mandatory? Please confirm this intent.
16736-16736: Add descriptive summary forInvoiceSimulationLine
Clarifying that this schema is the input model for usage-based line simulations improves readability.
15834-15851:✅ Verification successful
New document reference schemas and enum typo correction
IntroducingInvoiceDocumentRefviaCreditNoteOriginalInvoiceRef, correctingInvoiceDocumentRefTypeenum, and definingInvoiceExpandconsolidates document references cleanly.Verify removal of the old
"credit_node_original_invoice"value everywhere:
🏁 Script executed:
rg -n "credit_node_original_invoice" api/openapi.cloud.yamlLength of output: 59
🏁 Script executed:
#!/bin/bash # Search for any remaining occurrences of the typo across the entire repository rg -n "credit_node_original_invoice"Length of output: 36
Verification complete – typo removed across the repository
No instances of"credit_node_original_invoice"remain after a full-text search. The newInvoiceDocumentRef, correctedInvoiceDocumentRefTypeenum, andInvoiceExpanddefinitions are clean and ready to merge.api/openapi.yaml (12)
12943-12943: Corrected enum typo
credit_note_original_invoicefixes a typo in the enum for original invoice credit note. Nice catch.
15992-16006: InvoiceDetailedLine schema required fields look comprehensive
The newInvoiceDetailedLineincludes all essential metadata and lifecycle fields.
16257-16257: Corrected enum typo
Fixedcredit_note_original_invoiceinInvoiceDocumentRefTypeto match referenced types.
16418-16436: Deprecate legacy fields in InvoiceLine
Fieldstype,price, andfeatureKeyare correctly marked as deprecated and readOnly forInvoiceLine, consolidating around usage-based lines.
16437-16443: Introducechildrenfor detailed flat fee lines
The newchildren: InvoiceDetailedLine[]property allows nesting manual fee lines under usage-based items. Looks good.
16444-16450: Update rateCard reference
rateCardnow points toInvoiceUsageBasedRateCard, aligning with the unified usage-based model.
16450-16458: VerifyquantityandmeteredQuantitysemantics
EnsurequantityandmeteredQuantityreadOnly values align with business logic for billed versus metered usage.
16616-16618: Deprecate type field in update schemas
Marking thetypefield as deprecated and readOnly here aligns with the shift to usage-based-only models.
16719-16784: SimplifyInvoiceLineReplaceUpdateto usage-based lines
The update model now only requiresname,period,invoiceAtand deprecates legacy fields. The optionalidproperty allows adding or updating lines within a single request.
16862-16863: Deprecate globalInvoiceLineTypesenum
LegacyInvoiceLineTypesis now marked deprecated; ensure all references are migrated.
16990-17049: RefineInvoicePendingLineCreatefor usage-based lines
Removedtypediscriminator and deprecated legacy fields, consolidating the create model around usage-based invoices.
17298-17298: UpdateInvoiceSimulationLinedescription
Clarifies that simulation lines now only support usage-based items; no type discriminator.openmeter/billing/httpdriver/invoiceline.go (7)
107-114: LGTM! Proper validation for flat fee line rejection.The validation correctly rejects flat fee lines with a clear error message, aligning with the goal of removing flat fee support from the main invoice line handling.
184-206: LGTM! Correct business logic for line hierarchy.The validation properly prevents usage-based lines from having usage-based children, maintaining the intended architecture where main lines are usage-based and children are detailed (flat fee) lines.
274-351: LGTM! Proper type validation and mapping for usage-based lines.The function correctly validates that only usage-based lines are processed and properly maps them to the API format. The children mapping to detailed lines is also correctly implemented.
510-573: LGTM! Correct simulation line mapping with proper quantity handling.The function properly maps simulation lines to usage-based entities with appropriate quantity calculation logic for different price types.
575-617: LGTM! Correct line creation from replace update.The function properly creates usage-based lines with the correct type and structure from update requests.
619-688: LGTM! Comprehensive merge validation with proper business rules.The function correctly validates line type consistency and implements appropriate business rules for discount updates and period changes on special line types.
690-767: LGTM! Proper validation integration in line merging logic.The function correctly integrates flat fee line validation at appropriate points in the merge process, maintaining consistency with the refactoring goals.
Summary by CodeRabbit