feat: custom invoicing api #2712
Conversation
📝 Walkthrough## Walkthrough
This change introduces comprehensive support for custom invoicing synchronization and payment status updates. It adds new OpenAPI endpoints, data models, and specification interfaces for handling draft and finalized invoice synchronization as well as payment status updates. The implementation includes new HTTP handlers, error handling, and mapping logic to connect API requests to internal billing operations. The router and server initialization are updated to integrate the custom invoicing service and its handlers. Validation logic ensures that only invoices associated with the custom invoicing app are processed. Supporting changes refactor mapping utilities for invoices and profiles to standalone functions.
## Changes
| File(s) | Change Summary |
|----------------------------------------------------------------------------------------------|---------------|
| `api/openapi.yaml`, `api/spec/src/app/custominvoicing.tsp` | Added new OpenAPI endpoints and data models for custom invoicing draft/finalized synchronization and payment status updates. Introduced corresponding API specification interface and models. |
| `openmeter/app/custominvoicing/httpdriver/custominvoicing.go`, `openmeter/app/custominvoicing/httpdriver/handler.go`, `openmeter/app/custominvoicing/httpdriver/errors.go`, `openmeter/app/custominvoicing/httpdriver/mapper.go` | Introduced HTTP driver package for custom invoicing with handlers for draft/finalized invoice synchronization and payment status updates, error encoding, handler interface, and mapping functions from API to internal models. |
| `openmeter/app/custominvoicing/service/sync.go` | Added invoice validation method to ensure operations only on custom invoicing app invoices. Updated service methods to use this validation. |
| `openmeter/billing/app.go` | Extended `SyncInput` interface and related structs to support invoice-level validation via a new method and validator field. |
| `openmeter/billing/httpdriver/invoice.go`, `openmeter/billing/httpdriver/profile.go` | Refactored invoice and profile mapping methods to standalone functions for broader reuse. |
| `openmeter/server/router/appcustominvoicing.go` | Added HTTP router methods for new custom invoicing endpoints, delegating to the corresponding handlers. |
| `openmeter/server/router/router.go` | Integrated custom invoicing service and handler into the router configuration, initialization, and validation. |
| `cmd/server/main.go` | Passed the custom invoicing service to the router configuration. |
| `openmeter/billing/adapter.go` | Corrected spelling of interface name from `CustomerSyncronizationAdapter` to `CustomerSynchronizationAdapter`. |
| `openmeter/billing/worker/subscription/sync_test.go` | Fixed test method name spelling from `TestDiscountSyncronization` to `TestDiscountSynchronization`. |
## Possibly related PRs
- [openmeterio/openmeter#2705](https://github.com/openmeterio/openmeter/pull/2705): Introduces backend support for custom invoicing, including hook validation and payment status handling, which is extended and completed by this PR with full API and HTTP driver implementation.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 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
turip
force-pushed
the
feat/custom-invocing-api
branch
4 times, most recently
from
361edb6 to
a2569c0
Compare
turip
added
release-note/feature
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (17)
openmeter/server/router/router.go (1)
214-214: Fix typo in handler field name.There's a spelling mistake in the field name:
appCustomInvocingHandlershould beappCustomInvoicingHandler(missing an 'i').- appCustomInvocingHandler appcustominvoicinghttpdriver.Handler + appCustomInvoicingHandler appcustominvoicinghttpdriver.Handleropenmeter/app/custominvoicing/httpdriver/handler.go (2)
19-21: Fix typo in method names.The method names contain a typo: "Syncronized" should be "Synchronized".
type AppHandler interface { - DraftSyncronized() DraftSyncronizedHandler - IssuingSyncronized() IssuingSyncronizedHandler + DraftSynchronized() DraftSynchronizedHandler + IssuingSynchronized() IssuingSynchronizedHandler UpdatePaymentStatus() UpdatePaymentStatusHandler }
33-40: Consider more specific error handling for namespace resolution.The current implementation returns a generic 500 Internal Server Error when the namespace is missing. Consider a more specific error message or a different status code depending on the context (e.g., 400 Bad Request if the namespace should have been provided by the client).
func (h *handler) resolveNamespace(ctx context.Context) (string, error) { ns, ok := h.namespaceDecoder.GetNamespace(ctx) if !ok { - return "", commonhttp.NewHTTPError(http.StatusInternalServerError, errors.New("internal server error")) + return "", commonhttp.NewHTTPError(http.StatusBadRequest, errors.New("namespace not provided or invalid")) } return ns, nil }openmeter/server/router/appcustominvoicing.go (1)
9-31: Improve handler documentation to describe functionalityThe comments above each handler only document the HTTP method and path, without explaining what each endpoint actually does functionally.
Consider enhancing comments to include a brief functional description of each endpoint, for example:
-// Submit draft syncronization results -// (POST /api/v1/apps/custom-invoicing/{invoiceId}/draft/syncronized) +// AppCustomInvoicingDraftSynchronized handles submission of draft invoice synchronization results. +// It acknowledges that a draft invoice has been successfully synchronized with an external system. +// (POST /api/v1/apps/custom-invoicing/{invoiceId}/draft/synchronized)openmeter/app/custominvoicing/httpdriver/mapper.go (3)
62-68: Extract magic string format as a constantThe hardcoded string format
trigger_%sinmapPaymentTriggerFromAPIshould be extracted as a constant for better maintainability.+const ( + // PaymentTriggerPrefix is prepended to API payment triggers to form internal billing triggers + PaymentTriggerPrefix = "trigger_" +) func mapPaymentTriggerFromAPI(in api.CustomInvoicingPaymentTrigger) (billing.InvoiceTrigger, error) { if in == "" { return "", models.NewGenericValidationError(fmt.Errorf("payment trigger is required")) } - return fmt.Sprintf("trigger_%s", in), nil + return billing.InvoiceTrigger(PaymentTriggerPrefix + string(in)), nil }This makes the code more maintainable if the prefix needs to change in the future and clarifies its purpose.
11-39: Add input validation for line IDs in mapUpsertInvoiceResultFromAPIThe function adds line external IDs and line discount external IDs without validating the input values. Consider adding validation to ensure data integrity.
Consider adding validation to ensure the line IDs and external IDs are not empty before adding them to the result:
if in.LineExternalIds != nil { for _, line := range *in.LineExternalIds { + if line.LineId == "" || line.ExternalId == "" { + // Skip empty values or consider returning an error + continue + } res.AddLineExternalID(line.LineId, line.ExternalId) } } if in.LineDiscountExternalIds != nil { for _, lineDiscount := range *in.LineDiscountExternalIds { + if lineDiscount.LineDiscountId == "" || lineDiscount.ExternalId == "" { + // Skip empty values or consider returning an error + continue + } res.AddLineDiscountExternalID(lineDiscount.LineDiscountId, lineDiscount.ExternalId) } }This would prevent adding empty or invalid mappings to the result object.
1-69: Consider adding comprehensive error handlingThe mapper functions currently have limited error handling, with only
mapPaymentTriggerFromAPIreturning errors. Consider extending error handling to the other mapper functions as well.Consider modifying the function signatures to return errors for validation failures:
-func mapUpsertInvoiceResultFromAPI(in *api.CustomInvoicingSyncResult) *billing.UpsertInvoiceResult { +func mapUpsertInvoiceResultFromAPI(in *api.CustomInvoicingSyncResult) (*billing.UpsertInvoiceResult, error) { if in == nil { - return nil + return nil, nil } // ... existing code ... - return res + return res, nil } -func mapFinalizeInvoiceResultFromAPI(in api.CustomInvoicingFinalizedRequest) *billing.FinalizeInvoiceResult { +func mapFinalizeInvoiceResultFromAPI(in *api.CustomInvoicingFinalizedRequest) (*billing.FinalizeInvoiceResult, error) { // ... updated code with nil check ... - return res + return res, nil }This would provide a consistent pattern across all mapper functions and allow for validation errors to be returned.
api/openapi.yaml (5)
668-733: Typo in endpoint naming: “syncronized”/“syncronization” is misspelled
The/draft/syncronizedpath segment, operationId (appCustomInvoicingDraftSyncronized), and summary (Submit draft syncronization results) use “syncronized”/“syncronization” instead of “synchronized”/“synchronization”. Consider renaming to:
- Path:
/api/v1/apps/custom-invoicing/{invoiceId}/draft/synchronized- operationId:
appCustomInvoicingDraftSynchronized- summary: “Submit draft synchronization results”
This will improve readability and align with standard spelling.
734-798: Consistent spelling update needed on issuing endpoint
Similar to the draft endpoint,/issuing/syncronized, operationIdappCustomInvoicingIssuingSyncronized, and summary “Submit issuing syncronization results” should use “synchronized” and “synchronization”. Please update these identifiers and descriptive text accordingly.
799-863: Consider adding explicit 404 and 422 responses
The/payment/statusendpoint currently defines 204, 400, 401, 403, 412, 500, and 503 responses. For completeness and better client feedback, consider adding:
404 Not Foundwhen theinvoiceIddoes not correspond to an existing invoice.422 Unprocessable Entityfor semantic validation failures in the request body.
This will make error handling more robust and consistent across all custom‑invoicing operations.
12671-12786: Schema definitions: fix spelling and strengthen validation
- The schema name
CustomInvoicingDraftSyncronizedRequestand its description use “syncronization”—should be “synchronization”.- The top‑level descriptions for
CustomInvoicingSyncResult(“Information to syncronize the invoice”) and related types also misspell “synchronize”.- None of the new request schemas declare
requiredproperties beyond the root objects. If certain fields (e.g.invoicinginDraftSyncronizedRequestor bothinvoicingandpaymentinCustomInvoicingFinalizedRequest) are mandatory, consider adding arequiredarray to enforce those constraints.- It may also be helpful to provide example values for key properties (e.g.
invoiceNumber,externalId) to improve the OpenAPI documentation.
Please update spelling and any schema-level requirements as needed.
12799-12811: Grammar and consistency in payment‑status request description
The description underCustomInvoicingUpdatePaymentStatusRequestreads:
“Can be used to manipulate invoice's payment status (when custominvoicing app is being used).”
For clarity and consistency:
- Add a space in “custom invoicing”.
- Rephrase to: “Use this endpoint to update the payment status of an invoice managed by the custom invoicing app.”
This small change will enhance readability in the generated API docs.api/spec/src/app/custominvoicing.tsp (2)
186-197: Propagate rename to finalized request modelFollow‑up to previous comment:
CustomInvoicingFinalizedRequestembedsCustomInvoicingFinalizedInvoicingRequest, but the surrounding docs still say syncronization. Update docs and (after rename) double‑check generated OpenAPI contains the corrected wording.
200-224: Enum value “Void” is ambiguous – consider “Voided” or “Cancel”When an invoice is invalidated the common terminology is voided (past‑tense) or canceled.
UsingVoidrisks confusion with the Go zero‑type and may be unclear to API consumers.- Void: "void", + Voided: "voided",If you keep the current choice, add clarifying docs (
/** Invoice has been voided */).openmeter/app/custominvoicing/httpdriver/custominvoicing.go (3)
33-45: Lost error context frommapUpsertInvoiceResultFromAPIIf
mapUpsertInvoiceResultFromAPIcan fail (e.g., invalid external‑ID mapping), that error is silently ignored because the function only returns a value. Align it withmapPaymentTriggerFromAPIand make it(Result, error)so the decoder can surface validation issues early.- UpsertInvoiceResults: mapUpsertInvoiceResultFromAPI(body.Invoicing), + results, err := mapUpsertInvoiceResultFromAPI(body.Invoicing) + if err != nil { + return DraftSyncronizedRequest{}, fmt.Errorf("invalid invoicing mapping: %w", err) + } + UpsertInvoiceResults: results,
84-96: Similar missing error‑handling in issuing handler
mapFinalizeInvoiceResultFromAPImay fail for the same reasons (invalid number format, etc.). Propagate its error as shown above to avoid passing malformed data into the service layer.
158-163: Mapping failure bubbles raw internal error to client
billinghttpdriver.MapInvoiceToAPIcan return rich internal errors. Passing them directly to the client may leak implementation details.
Wrap with a domain‑level error (ErrMappingInvoice) and leterrorEncoder()translate it to a sanitized 500.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
api/openapi.yaml(3 hunks)api/spec/src/app/custominvoicing.tsp(2 hunks)cmd/server/main.go(1 hunks)openmeter/app/custominvoicing/httpdriver/custominvoicing.go(1 hunks)openmeter/app/custominvoicing/httpdriver/errors.go(1 hunks)openmeter/app/custominvoicing/httpdriver/handler.go(1 hunks)openmeter/app/custominvoicing/httpdriver/mapper.go(1 hunks)openmeter/app/custominvoicing/service/sync.go(5 hunks)openmeter/billing/app.go(4 hunks)openmeter/billing/httpdriver/invoice.go(7 hunks)openmeter/billing/httpdriver/profile.go(2 hunks)openmeter/server/router/appcustominvoicing.go(1 hunks)openmeter/server/router/router.go(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (7)
openmeter/billing/httpdriver/profile.go (2)
openmeter/billing/profile.go (1)
ProfileApps(261-265)api/api.gen.go (1)
BillingProfileAppsOrReference(1301-1303)
cmd/server/main.go (3)
openmeter/ent/schema/appcustominvoicing.go (5)
AppCustomInvoicing(15-17)AppCustomInvoicing(19-25)AppCustomInvoicing(27-32)AppCustomInvoicing(34-38)AppCustomInvoicing(40-51)openmeter/ent/db/predicate/predicate.go (1)
AppCustomInvoicing(41-41)app/common/app.go (1)
AppRegistry(138-143)
openmeter/app/custominvoicing/httpdriver/handler.go (3)
openmeter/app/custominvoicing/httpdriver/custominvoicing.go (3)
DraftSyncronizedHandler(22-22)IssuingSyncronizedHandler(73-73)UpdatePaymentStatusHandler(124-124)pkg/framework/transport/httptransport/options.go (1)
HandlerOption(19-21)pkg/framework/commonhttp/errors.go (1)
NewHTTPError(35-41)
openmeter/billing/app.go (5)
openmeter/billing/invoice.go (2)
Invoice(329-340)InvoiceID(207-207)api/client/javascript/src/client/schemas.ts (1)
Invoice(9922-9922)api/api.gen.go (1)
Invoice(3240-3348)api/client/go/client.gen.go (1)
Invoice(2850-2958)openmeter/app/custominvoicing/sync.go (2)
SyncDraftInvoiceInput(11-14)SyncIssuingInvoiceInput(30-33)
openmeter/app/custominvoicing/httpdriver/errors.go (3)
pkg/framework/commonhttp/errors.go (1)
HandleErrorIfTypeMatches(62-75)openmeter/billing/errors.go (2)
EncodeValidationIssues(97-113)AppError(142-146)api/api.gen.go (1)
ValidationIssue(6658-6688)
openmeter/app/custominvoicing/httpdriver/mapper.go (4)
api/api.gen.go (4)
CustomInvoicingSyncResult(1984-2002)InvoiceNumber(3893-3893)CustomInvoicingFinalizedRequest(1952-1958)CustomInvoicingPaymentTrigger(1979-1979)openmeter/billing/app.go (2)
FinalizeInvoiceResult(82-86)NewFinalizeInvoiceResult(88-90)openmeter/billing/invoicestate.go (1)
InvoiceTrigger(10-10)pkg/models/error.go (1)
NewGenericValidationError(138-140)
openmeter/app/custominvoicing/httpdriver/custominvoicing.go (6)
api/api.gen.go (4)
Invoice(3240-3348)CustomInvoicingDraftSyncronizedRequest(1929-1932)CustomInvoicingFinalizedRequest(1952-1958)CustomInvoicingUpdatePaymentStatusRequest(2015-2018)pkg/framework/transport/httptransport/argshandler.go (2)
HandlerWithArgs(11-14)NewHandlerWithArgs(18-29)openmeter/billing/httpdriver/invoice.go (1)
MapInvoiceToAPI(470-563)pkg/framework/commonhttp/encoder.go (1)
JSONResponseEncoderWithStatus(64-68)pkg/framework/transport/httptransport/options.go (3)
AppendOptions(81-83)WithOperationName(41-47)WithErrorEncoder(35-39)openmeter/app/custominvoicing/sync.go (1)
HandlePaymentTriggerInput(49-52)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Test
- GitHub Check: Quickstart
- GitHub Check: Lint
- GitHub Check: E2E
- GitHub Check: Developer environment
- GitHub Check: CI
- GitHub Check: Commit hooks
- GitHub Check: Build
- GitHub Check: Analyze (go)
🔇 Additional comments (24)
cmd/server/main.go (1)
154-154: Integration of AppCustomInvoicing looks good.The addition of the
AppCustomInvoicingfield to the router config properly integrates the custom invoicing service with the router, following the same pattern used for other app services likeAppStripe.openmeter/billing/httpdriver/profile.go (2)
440-440: Method call updated correctly.This line appropriately updates the call to use the new standalone function instead of the handler method.
462-495: Good refactoring of mapProfileAppsToAPI to a standalone function.This refactoring improves code reusability by converting a handler method to a standalone function. Since the function doesn't use any handler state, this is a safe change that allows the mapping logic to be used by other components (like the new custom invoicing handlers) without requiring access to a handler instance.
openmeter/billing/httpdriver/invoice.go (7)
89-89: Method call updated correctly.This line properly updates the call to use the new standalone function instead of the handler method.
152-152: Method call updated correctly.This line properly updates the call to use the new standalone function instead of the handler method.
208-208: Method call updated correctly.This line properly updates the call to use the new standalone function instead of the handler method.
289-289: Method call updated correctly.This line properly updates the call to use the new standalone function instead of the handler method.
384-384: Method call updated correctly.This line properly updates the call to use the new standalone function instead of the handler method.
459-459: Method call updated correctly.This line properly updates the call to use the new standalone function instead of the handler method.
470-563: Good refactoring of mapInvoiceToAPI to a standalone exported function.This refactoring improves code reusability by converting a handler method to a standalone function and exporting it (capitalized name). Since the function doesn't use any handler state, this is a safe change that allows the mapping logic to be used by other components (like the new custom invoicing handlers) without requiring access to a handler instance.
The function also correctly updates the internal call to use the standalone
mapProfileAppsToAPIfunction that was refactored in profile.go.openmeter/server/router/router.go (4)
16-17: Import statements added correctly.The new imports for the custom invoicing packages are added appropriately.
87-87: Config struct field added correctly.The
AppCustomInvoicingfield has been added to theConfigstruct, allowing the custom invoicing service to be passed to the router.
141-143: Validation added correctly.Proper validation has been added to ensure the required custom invoicing service is provided, following the same pattern as other service validations.
335-340: Handler initialization looks good.The custom invoicing handler is initialized with the appropriate dependencies (service, namespace decoder, error handler) following the same pattern as other handlers in the router.
Note: Remember to update the variable name in the initialization to match the corrected field name if you fix the typo.
openmeter/app/custominvoicing/httpdriver/errors.go (1)
12-21: Well-structured error handling for HTTP responses.The error encoder correctly maps different error types to appropriate HTTP status codes, providing consistent error handling across the custom invoicing HTTP endpoints. This approach ensures that clients receive meaningful responses when errors occur.
openmeter/app/custominvoicing/service/sync.go (3)
31-31: Good implementation of validator integration.Using the
ValidateInvoiceAppas theInvoiceValidatorensures that only invoices associated with the custom invoicing app are processed, enhancing security.
46-46: Consistent validator integration across methods.The validator is consistently applied to both draft and issuing invoice synchronization, ensuring uniform validation behavior.
75-77: Good integration of app validation in payment trigger handling.The payment trigger handler now correctly validates that the invoice is associated with the custom invoicing app before processing payment triggers, maintaining consistency with other methods.
openmeter/billing/app.go (4)
264-264: Good interface extension for invoice validation.Adding
ValidateWithInvoiceto theSyncInputinterface allows for additional validation logic to be applied to invoices during synchronization.
276-277: Clean implementation of validator function field.The addition of the
InvoiceValidatorfunction field provides a flexible way to inject validation logic.
312-318: Well-implemented validation method.The implementation of
ValidateWithInvoicecorrectly handles the case when no validator is provided, making it safe to use even when validation is not required.
326-327: Consistent pattern across structs.Both
SyncDraftInvoiceInputandSyncIssuingInvoiceInputstructs follow the same pattern for validator implementation, ensuring consistency in behavior.Also applies to: 363-369
openmeter/app/custominvoicing/httpdriver/handler.go (1)
42-52: Clean dependency injection pattern.The constructor properly initializes the handler with its dependencies and optional configuration, following good practices for dependency injection.
openmeter/app/custominvoicing/httpdriver/custominvoicing.go (1)
127-152: Body size is unbounded – addhttp.MaxBytesReaderfor basic DoS protectionThe JSON decoders read the entire body; a malicious client could send an enormous payload and exhaust memory.
Wrap the request body first:r.Body = http.MaxBytesReader(w, r.Body, 1<<20) // 1 MiB, tune as neededand surface a 413 error if the limit is exceeded. Reuse a common middleware if one already exists in the framework.
[offer_security_advice]
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
api/spec/src/app/custominvoicing.tsp (3)
50-56: Spelling error in API endpoint definitionThe endpoint route, operationId, method name, and request type all contain the misspelling "Syncronized" instead of "Synchronized".
This misspelling exists across multiple elements:
@route("/api/v1/apps/custom-invoicing/{invoiceId}/draft/synchronized")@operationId("appCustomInvoicingDraftSyncronized")draftSyncronizedmethod nameCustomInvoicingDraftSyncronizedRequest(line 158)Since this API will be part of the public contract, consistently fixing this spelling is critical before release.
157-158: Misspelled model nameThe model name contains the same misspelling "Syncronized" instead of "Synchronized".
- @friendlyName("CustomInvoicingDraftSyncronizedRequest") - model CustomInvoicingDraftSyncronizedRequest { + @friendlyName("CustomInvoicingDraftSynchronizedRequest") + model CustomInvoicingDraftSynchronizedRequest {This needs to be fixed consistently throughout the API specification.
59-65: 🛠️ Refactor suggestionMethod name inconsistent with endpoint path
The endpoint path contains "issuing/synchronized" but the method is named
finalized, creating inconsistency.Rename the method to match the pattern used in the route:
- finalized( + issuingSynchronized(This makes the API more consistent and easier to understand, with methods that clearly reflect their endpoint paths.
🧹 Nitpick comments (2)
api/spec/src/app/custominvoicing.tsp (2)
77-79: Fix typo in documentation commentThe comment contains the same misspelling "syncronize" instead of "synchronize".
- /** - * Information to syncronize the invoice. + /** + * Information to synchronize the invoice.
187-197: Model name could be more specificThe model
CustomInvoicingFinalizedRequestcombines both invoicing and payment aspects, but its name doesn't reflect this dual purpose.Consider renaming to better describe its purpose:
- @friendlyName("CustomInvoicingFinalizedRequest") - model CustomInvoicingFinalizedRequest { + @friendlyName("CustomInvoicingIssuingSynchronizedRequest") + model CustomInvoicingIssuingSynchronizedRequest {This would maintain consistency with the naming pattern of other request models and clearly indicate its purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
api/openapi.yaml(3 hunks)api/spec/src/app/custominvoicing.tsp(2 hunks)openmeter/app/custominvoicing/httpdriver/custominvoicing.go(1 hunks)openmeter/app/custominvoicing/service/sync.go(5 hunks)openmeter/billing/adapter.go(2 hunks)openmeter/billing/worker/subscription/sync_test.go(1 hunks)openmeter/server/router/appcustominvoicing.go(1 hunks)openmeter/server/router/router.go(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- openmeter/billing/worker/subscription/sync_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- openmeter/server/router/router.go
- openmeter/server/router/appcustominvoicing.go
- openmeter/app/custominvoicing/service/sync.go
- api/openapi.yaml
- openmeter/app/custominvoicing/httpdriver/custominvoicing.go
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Quickstart
- GitHub Check: E2E
- GitHub Check: CI
- GitHub Check: Lint
- GitHub Check: Developer environment
- GitHub Check: Commit hooks
- GitHub Check: Build
- GitHub Check: Test
- GitHub Check: Analyze (go)
🔇 Additional comments (3)
openmeter/billing/adapter.go (1)
21-21: Approved: Fixed spelling of interface name.The correction from "CustomerSyncronization" to "CustomerSynchronization" improves code consistency and readability. This spelling fix aligns with the custom invoicing functionality being introduced.
Also applies to: 51-51
api/spec/src/app/custominvoicing.tsp (2)
202-210: Payment trigger enum looks goodThe payment trigger enum provides a comprehensive set of status options that cover the common payment scenarios.
217-223: Well-structured payment status request modelThe payment status update request has a clear, simple design with a required trigger field.
turip
force-pushed
the
feat/custom-invocing-api
branch
from
36d9d9f to
4606eb3
Compare
turip
force-pushed
the
feat/custom-invocing-api
branch
from
4606eb3 to
8507a03
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
api/spec/src/app/custominvoicing.tsp (3)
51-56:⚠️ Potential issueSpelling error in operation ID persists
The operation ID
appCustomInvoicingDraftSyncronizedand method namedraftSyncronizedcontain a misspelling of "synchronized". This matches earlier feedback that hasn't been addressed.- @operationId("appCustomInvoicingDraftSyncronized") - draftSyncronized( + @operationId("appCustomInvoicingDraftSynchronized") + draftSynchronized(
157-158:⚠️ Potential issueModel name inherits the spelling error
CustomInvoicingDraftSyncronizedRequestcarries the same misspelling.- @friendlyName("CustomInvoicingDraftSyncronizedRequest") - model CustomInvoicingDraftSyncronizedRequest { + @friendlyName("CustomInvoicingDraftSynchronizedRequest") + model CustomInvoicingDraftSynchronizedRequest {
60-65:⚠️ Potential issueMethod name inconsistent with operation ID
The operation ID
appCustomInvoicingIssuingSyncronized(with misspelling) doesn't match the method namefinalized. This creates confusion and inconsistency in the API.Either rename the method to match the operation ID (but with correct spelling):
- @operationId("appCustomInvoicingIssuingSyncronized") - finalized( + @operationId("appCustomInvoicingIssuingSynchronized") + issuingSynchronized(Or keep the method name and adjust the operation ID:
- @operationId("appCustomInvoicingIssuingSyncronized") - finalized( + @operationId("appCustomInvoicingFinalized") + finalized(
🧹 Nitpick comments (5)
api/openapi.yaml (1)
12799-12811: Minor: Improve grammar in UpdatePaymentStatus request descriptionThe description block:
description: |- Update payment status request. Can be used to manipulate invoice's payment status (when custominvoicing app is being used).Consider adjusting for clarity and consistency, e.g.:
description: |- Request to update the payment status of an invoice. Applicable only for invoices managed by the custom invoicing app.This enhances readability and aligns with other schema descriptions.
api/spec/src/app/custominvoicing.tsp (4)
78-81: Fix spelling in documentationThe same spelling error appears in the documentation comments.
-/** - * Information to syncronize the invoice. - * - * Can be used to store external app's IDs on the invoice or lines. - */ +/** + * Information to synchronize the invoice. + * + * Can be used to store external app's IDs on the invoice or lines. + */
47-75: Inconsistent HTTP response documentationThe interface methods return
void | CommonErrors, but there's no documentation describing which HTTP status codes are returned for different scenarios or what kinds of common errors might be encountered. Consider adding@statusCodeannotations to document the expected response codes.For example:
@post @route("/api/v1/apps/custom-invoicing/{invoiceId}/draft/synchronized") @operationId("appCustomInvoicingDraftSyncronized") @summary("Submit draft synchronization results") +@doc("Synchronizes a draft invoice with an external system") +@returns(204, "Draft synchronized successfully") +@returns(400, "Invalid request", CommonErrors.BadRequestError) +@returns(404, "Invoice not found", CommonErrors.NotFoundError) draftSyncronized(
183-185: Document default invoice number formatThe comment mentions "INV- prefix" but doesn't specify the full format (e.g., sequential numbering, date-based, etc.). This could be confusing for API consumers.
/** * Information to finalize the invoice. * - * If invoicing.invoiceNumber is not set, then a new invoice number will be generated (INV- prefix). + * If invoicing.invoiceNumber is not set, then a new invoice number will be generated + * with INV- prefix followed by a sequential number (e.g., INV-00001). */
67-74: Improve payment trigger documentationThe
CustomInvoicingPaymentTriggerenum defines several payment states, but there's no documentation about when to use each trigger or what effects they have on the invoice. This makes the API harder to use correctly.Consider adding more detailed documentation for each enum value:
enum CustomInvoicingPaymentTrigger { + /** + * Mark the invoice as paid. This will update the payment status and record the payment date. + */ Paid: "paid", + /** + * Record that a payment attempt failed. The invoice remains unpaid and can be retried. + */ PaymentFailed: "payment_failed", + /** + * Mark the payment as uncollectible. This is a terminal state indicating + * the payment cannot be collected and should be written off. + */ PaymentUncollectible: "payment_uncollectible", // ... similar documentation for other valuesAlso applies to: 199-223
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
api/openapi.yaml(3 hunks)api/spec/src/app/custominvoicing.tsp(2 hunks)cmd/server/main.go(1 hunks)openmeter/app/custominvoicing/httpdriver/custominvoicing.go(1 hunks)openmeter/app/custominvoicing/httpdriver/errors.go(1 hunks)openmeter/app/custominvoicing/httpdriver/handler.go(1 hunks)openmeter/app/custominvoicing/httpdriver/mapper.go(1 hunks)openmeter/app/custominvoicing/service/sync.go(5 hunks)openmeter/billing/adapter.go(2 hunks)openmeter/billing/app.go(4 hunks)openmeter/billing/httpdriver/invoice.go(7 hunks)openmeter/billing/httpdriver/profile.go(2 hunks)openmeter/billing/worker/subscription/sync_test.go(1 hunks)openmeter/server/router/appcustominvoicing.go(1 hunks)openmeter/server/router/router.go(5 hunks)openmeter/server/server_test.go(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- openmeter/billing/worker/subscription/sync_test.go
🚧 Files skipped from review as they are similar to previous changes (12)
- cmd/server/main.go
- openmeter/billing/adapter.go
- openmeter/billing/httpdriver/invoice.go
- openmeter/server/router/router.go
- openmeter/app/custominvoicing/httpdriver/errors.go
- openmeter/app/custominvoicing/httpdriver/handler.go
- openmeter/billing/app.go
- openmeter/app/custominvoicing/service/sync.go
- openmeter/app/custominvoicing/httpdriver/mapper.go
- openmeter/server/router/appcustominvoicing.go
- openmeter/billing/httpdriver/profile.go
- openmeter/app/custominvoicing/httpdriver/custominvoicing.go
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: E2E
- GitHub Check: CI
- GitHub Check: Developer environment
- GitHub Check: Commit hooks
- GitHub Check: Lint
- GitHub Check: Analyze (go)
- GitHub Check: Test
- GitHub Check: Build
🔇 Additional comments (4)
openmeter/server/server_test.go (4)
23-23: Addition of a new import for custom invoicing integration.The import of the custom invoicing package follows the existing code organization pattern, maintaining proper alphabetical ordering with other app-related imports.
467-467: Service instantiation matches existing test setup patterns.The addition of the NoopAppCustomInvoicingService instance follows the established pattern for other service mocks in the test setup.
494-494: Router configuration correctly integrates the new custom invoicing service.The AppCustomInvoicing field is properly set in the router configuration, ensuring the new service is accessible to the router handlers.
1008-1022: Well-structured implementation of the custom invoicing service interface.The NoopAppCustomInvoicingService properly implements the appcustominvoicing.SyncService interface with appropriate no-op methods that return empty objects and nil errors, consistent with other no-op service implementations in this file.
turip
force-pushed
the
feat/custom-invocing-api
branch
from
aca8ad8 to
b571ba2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
api/spec/src/app/custominvoicing.tsp (5)
199-210: Document valid payment status transitionsThe payment trigger enum defines possible states but doesn't indicate which transitions between states are valid. This was mentioned in a previous review and remains unaddressed.
Add documentation that explicitly states which transitions are allowed. For example, can an invoice go from "void" to "paid"? This information is crucial for API consumers to understand the valid workflow states.
/** * Payment trigger to execute on a finalized invoice. + * + * Valid transitions: + * - Any state → Paid: Marks the invoice as paid + * - Any state → PaymentFailed: Indicates a payment attempt failed + * - Any state → PaymentUncollectible: Marks the invoice as uncollectible + * - Any state → PaymentOverdue: Marks the invoice as overdue + * - Any state → ActionRequired: Indicates customer action is needed + * - Any state → Void: Voids the invoice (note: voided invoices cannot transition to other states) */If there are invalid transitions, those should be documented as well. For example: "Voided invoices cannot transition to any other state."
157-158:⚠️ Potential issueFix model name to correct spelling of "synchronized"
The model name contains a misspelling of "synchronized" as "syncronized", which was noted in previous reviews. This is a public API type and the spelling should be consistent.
- @friendlyName("CustomInvoicingDraftSyncronizedRequest") - model CustomInvoicingDraftSyncronizedRequest { + @friendlyName("CustomInvoicingDraftSynchronizedRequest") + model CustomInvoicingDraftSynchronizedRequest {Don't forget to update references to this model elsewhere in the codebase.
55-55:⚠️ Potential issueUpdate parameter type to match renamed model
The body parameter references the misspelled model type and should be updated when fixing the model name.
- @body body: CustomInvoicingDraftSyncronizedRequest, + @body body: CustomInvoicingDraftSynchronizedRequest,
62-65: 🛠️ Refactor suggestionRename method for consistency with route and operationId
The method name
finalizeddoesn't follow the same pattern as the route and operationId, which useIssuingSynchronized. This inconsistency could lead to confusion when working with this API.- finalized( + issuingSynchronized( @path invoiceId: ULID, @body body: CustomInvoicingFinalizedRequest, ): void | CommonErrors;
53-56:⚠️ Potential issueFix method name to match route and operationId
The method name
draftSyncronizedis misspelled and inconsistent with the correctly spelled route at line 50 (/draft/synchronized) and operationId at line 51 (appCustomInvoicingDraftSynchronized).- draftSyncronized( + draftSynchronized( @path invoiceId: ULID, @body body: CustomInvoicingDraftSyncronizedRequest, ): void | CommonErrors;
🧹 Nitpick comments (2)
api/spec/src/app/custominvoicing.tsp (2)
218-223: Enhance payment status update model with additional fieldsThe current payment status update model only supports changing the status via a trigger, but it might be useful to provide additional context.
Consider enhancing this model with optional fields that provide more information about the payment status change:
model CustomInvoicingUpdatePaymentStatusRequest { /** * The trigger to be executed on the invoice. */ trigger: CustomInvoicingPaymentTrigger; + + /** + * Optional timestamp when the payment status changed. + * If not provided, the current time will be used. + */ + statusChangedAt?: DateTime; + + /** + * Optional reason for the status change, especially useful for failures. + */ + statusChangeReason?: string; + + /** + * Optional reference ID from the external payment system. + */ + externalPaymentId?: string; }
47-75: Consider adding query endpoints for custom invoicing statusThe interface currently only provides ways to update custom invoicing status, but lacks query capabilities to fetch the current status.
Consider adding GET endpoints to retrieve the current status of an invoice in the external system:
@get @route("/api/v1/apps/custom-invoicing/{invoiceId}") @operationId("appCustomInvoicingGetStatus") @summary("Get custom invoicing status for an invoice") getStatus( @path invoiceId: ULID, ): CustomInvoicingStatus | CommonErrors;This would allow clients to query the current state without having to retrieve the full invoice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
api/openapi.yaml(3 hunks)api/spec/src/app/custominvoicing.tsp(2 hunks)openmeter/billing/README.md(1 hunks)openmeter/billing/app.go(5 hunks)openmeter/billing/worker/subscription/handlers.go(1 hunks)openmeter/billing/worker/subscription/sync_test.go(2 hunks)openmeter/server/router/appcustominvoicing.go(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- openmeter/billing/worker/subscription/handlers.go
- openmeter/billing/README.md
- openmeter/billing/worker/subscription/sync_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- openmeter/billing/app.go
- api/openapi.yaml
- openmeter/server/router/appcustominvoicing.go
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Quickstart
- GitHub Check: E2E
- GitHub Check: CI
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: Developer environment
- GitHub Check: Analyze (go)
- GitHub Check: Commit hooks
- GitHub Check: Build
🔇 Additional comments (2)
api/spec/src/app/custominvoicing.tsp (2)
1-14: LGTM: Imports and using statementsThe imports and using statements are comprehensive and correctly organized for the custom invoicing functionality.
24-45: LGTM: CustomInvoicingApp model extensionsThe additions to the CustomInvoicingApp model with hooks for draft synchronization and issuing synchronization are well-documented and provide clear functionality for controlling invoice progression.
2 participants
Overview
Adds the APIs required for custom invoicing based setups.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor