Conversation
📝 WalkthroughWalkthroughThis set of changes introduces asynchronous invoice synchronization capabilities and refines the expansion and state management logic in the billing and custom invoicing modules. Key updates include the addition of new interfaces and methods for syncing draft and issuing invoices, validation and merging logic for invoice results, and structured input types for sync operations. The invoice expansion mechanism is simplified by always resolving workflow apps, removing the need for explicit expansion flags. State machine transitions for invoices are now guarded by checks to determine if synchronous advancement is allowed, with logging for errors. Test suites are expanded to cover the new asynchronous flows and integration points, including full invoicing lifecycles with and without sync hooks. Changes
Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (10)
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-invoicing
branch
5 times, most recently
from
3f889f8 to
e7735e6
Compare
turip
force-pushed
the
feat/custom-invoicing
branch
from
e7735e6 to
246123a
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: 6
🔭 Outside diff range comments (1)
openmeter/billing/app.go (1)
214-259: 🛠️ Refactor suggestionDiscount‑ID merge is skipped when no line IDs are present & misspelled variable
lineDiscountExternalIDsare merged only when at least onelineExternalIDexists, due to the outerif len(r.GetLineExternalIDs()) > 0 { … }guard.
This means discount‑IDs supplied without line‑IDs are ignored.
Additionally, the local variable is misspelled (dicountIDToExternalID).- // Let's merge the line IDs - if len(r.GetLineExternalIDs()) > 0 { - flattenedLines := invoice.FlattenLinesByID() - - // Merge the line IDs - for lineID, externalID := range r.GetLineExternalIDs() { - ... - } - - // Let's merge the line discount IDs - dicountIDToExternalID := r.GetLineDiscountExternalIDs() - - for _, line := range flattenedLines { - ... - } - } + // Build a reusable flattened view + flattenedLines := invoice.FlattenLinesByID() + + // 1. Merge line IDs (if any) + for lineID, externalID := range r.GetLineExternalIDs() { + ... + } + + // 2. Merge discount IDs (independent from line IDs) + discountIDToExternalID := r.GetLineDiscountExternalIDs() + for _, line := range flattenedLines { + ... + }This change:
- Ensures discount IDs are always merged when provided.
- Fixes the typo
discountIDToExternalID.
🧹 Nitpick comments (9)
openmeter/app/custominvoicing/service/sync.go (4)
23-30: Avoid copy‑pasting the timestamp metadata block
SyncDraftInvoiceandSyncIssuingInvoiceboth create a one‑element map whose sole purpose is to stamp the current time. Duplicating that logic increases the maintenance burden and risks drifting key names or the time format in the future.-return s.billingService.SyncDraftInvoice(ctx, billing.SyncDraftInvoiceInput{ - InvoiceID: input.InvoiceID, - UpsertInvoiceResults: input.UpsertInvoiceResults, - AdditionalMetadata: map[string]string{ - appcustominvoicing.MetadataKeyDraftSyncedAt: clock.Now().Format(time.RFC3339), - }, -}) +return s.billingService.SyncDraftInvoice(ctx, billing.SyncDraftInvoiceInput{ + InvoiceID: input.InvoiceID, + UpsertInvoiceResults: input.UpsertInvoiceResults, + AdditionalMetadata: buildTimestampMetadata(appcustominvoicing.MetadataKeyDraftSyncedAt), +}) + +func buildTimestampMetadata(key string) map[string]string { + return map[string]string{key: clock.Now().Format(time.RFC3339)} +}Encapsulating the logic makes future changes (e.g. switching to RFC 3339Nano or injecting a clock for tests) a one‑liner.
37-44: Same timestamp helper can be reused here
SyncIssuingInvoicere‑implements the exact same pattern as above. Re‑use the helper to keep behaviour consistent and remove duplication.
51-69: Skip the extra read: reuse the invoice returned byTriggerInvoice(if available)Inside the transaction we first call
TriggerInvoice, then immediately issue a second read withGetInvoiceByID.
IfTriggerInvoicealready returns the updated invoice (or could be changed to do so) we could avoid:
- an extra database round‑trip, and
- the possibility of reading a stale snapshot under weaker isolation levels.
Consider extending the billing‑service interface to return the mutated invoice and remove this follow‑up query.
71-81: Minor: redundant length check beforelo.Filter
lo.Filteron an empty slice is cheap and already returns an empty slice, so the explicitlen(invoice.ValidationIssues) > 0guard is unnecessary.openmeter/app/custominvoicing/app.go (1)
118-148: Duplicate gating logic – extract a helper
CanDraftSyncAdvanceandCanIssuingSyncAdvanceshare the same three‑step pattern: feature‑flag check ➜ nil‑metadata guard ➜ key lookup. Extracting the common part removes 15+ duplicated lines and ensures any future change (e.g. additional metadata checks) stays in sync.test/app/custominvoicing/invocing_test.go (3)
305-307: Test description does not match the assertionThe sub‑test name says “uncollectible state”, but the code triggers
billing.TriggerPaidand expectsInvoiceStatusPaid.- s.Run("invoice can be transitioned to uncollectible state", func() { + s.Run("invoice can be transitioned to paid state", func() {Keeping names in sync with behaviour prevents confusion when reading test logs.
397-416: Stale comment + sub‑test labelThis flow no longer enters
draft.syncing; it short‑circuits topayment_processing.pendingbecause both hooks are disabled.- s.Run("invoice can be created and will end up in draft.syncing state", func() { + s.Run("invoice can be created and will end up in payment_processing.pending state", func() { ... - // We end up in payment_processing.pending state because we don't have a draft sync hook + // We end up in payment_processing.pending state because no draft/issuing sync hooks are enabled
113-117: Flaky tests: avoid realtime.Now()Tests rely on wall‑clock time (
now := time.Now()), which can cause flakes when run near DST changes or leap seconds.
Using a fixed time (e.g.time.Date(2023, 1, 2, …, time.UTC)) or a stubbed clock keeps the suite deterministic.openmeter/billing/app.go (1)
62-74: Minor: consider exportinglineDiscountExternalIDsor providing a copy
GetLineDiscountExternalIDs()returns the internal map directly.
Callers may inadvertently mutate the internal state.
You could either:
- Return a copy of the map, or
- Document the mutability expectation.
A simple, low‑cost fix:
func (u *UpsertResults) GetLineDiscountExternalIDs() map[string]string { cp := make(map[string]string, len(u.lineDiscountExternalIDs)) for k, v := range u.lineDiscountExternalIDs { cp[k] = v } return cp }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
api/spec/src/billing/invoices.tsp(1 hunks)app/common/app.go(1 hunks)openmeter/app/custominvoicing/app.go(3 hunks)openmeter/app/custominvoicing/factory.go(5 hunks)openmeter/app/custominvoicing/service.go(2 hunks)openmeter/app/custominvoicing/service/service.go(4 hunks)openmeter/app/custominvoicing/service/sync.go(1 hunks)openmeter/app/custominvoicing/sync.go(1 hunks)openmeter/billing/app.go(7 hunks)openmeter/billing/httpdriver/invoice.go(1 hunks)openmeter/billing/invoice.go(1 hunks)openmeter/billing/service.go(2 hunks)openmeter/billing/service/invoice.go(2 hunks)openmeter/billing/service/invoiceapp.go(2 hunks)openmeter/billing/service/invoicestate.go(8 hunks)openmeter/server/server_test.go(1 hunks)test/app/custominvoicing/invocing_test.go(1 hunks)test/app/stripe/invoice_test.go(1 hunks)test/billing/invoice_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
app/common/app.go (2)
app/common/billing.go (1)
BillingService(40-89)openmeter/app/stripe/service.go (1)
BillingService(44-50)
openmeter/app/custominvoicing/service/service.go (2)
openmeter/billing/service.go (1)
Service(9-19)openmeter/app/service.go (1)
AppService(13-36)
openmeter/billing/httpdriver/invoice.go (3)
openmeter/billing/service/lineservice/service.go (1)
Lines(193-193)api/api.gen.go (2)
InvoiceExpandLines(359-359)InvoiceExpandPreceding(360-360)api/client/go/client.gen.go (2)
InvoiceExpandLines(322-322)InvoiceExpandPreceding(323-323)
openmeter/billing/service/invoice.go (2)
openmeter/ent/db/app.go (3)
Apps(339-339)App(18-46)App(133-150)openmeter/billing/profile.go (1)
ProfileApps(261-265)
openmeter/billing/app.go (6)
openmeter/billing/invoice.go (2)
Invoice(329-340)InvoiceID(207-207)api/api.gen.go (1)
Invoice(3146-3254)api/client/go/client.gen.go (1)
Invoice(2850-2958)openmeter/app/custominvoicing/sync.go (2)
SyncDraftInvoiceInput(11-14)SyncIssuingInvoiceInput(30-33)pkg/models/validator.go (1)
Validate(16-26)pkg/models/error.go (1)
NewNillableGenericValidationError(129-135)
openmeter/app/custominvoicing/sync.go (5)
openmeter/billing/invoice.go (1)
InvoiceID(207-207)openmeter/billing/app.go (4)
UpsertInvoiceResult(76-76)SyncDraftInvoiceInput(271-275)FinalizeInvoiceResult(82-86)SyncIssuingInvoiceInput(305-309)pkg/models/validator.go (1)
Validate(16-26)pkg/models/error.go (1)
NewNillableGenericValidationError(129-135)openmeter/billing/invoicestate.go (1)
InvoiceTrigger(10-10)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Test
- 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 (46)
api/spec/src/billing/invoices.tsp (1)
415-419: Well-documented deprecation with clear rationale.The deprecation comment clearly explains that workflow apps are now always expanded, which helps API consumers understand why they shouldn't use this enum value. This change maintains backward compatibility while moving toward a more consistent approach.
app/common/app.go (1)
128-128: Added required BillingService dependency to the custom invoicing app factory.This change properly injects the BillingService dependency into the custom invoicing app factory, which is needed to support the new custom invoicing capabilities like invoice synchronization and workflow integration. The pattern follows existing conventions in the codebase.
test/app/stripe/invoice_test.go (1)
776-776: Improved API design using method instead of standalone function.The code now uses the
MergeIntoInvoicemethod on theUpsertInvoiceResulttype rather than a standalone function. This change follows good object-oriented design principles by encapsulating the merge logic in the result type.openmeter/billing/invoice.go (1)
214-218: Removed WorkflowApps field to enforce always-on expansion.This change simplifies the invoice expansion mechanism by removing the ability to toggle workflow apps expansion. This aligns with the API changes that deprecated the corresponding enum value, making the system more consistent by always resolving workflow apps.
test/billing/invoice_test.go (2)
399-399: Test case name updated to reflect streamlined workflow app expansionThe test name was updated from "Expand scenarios - no expand" to simply "Expand scenarios", reflecting that workflow apps are now always expanded regardless of the expansion flags. This aligns with the broader change in the codebase where workflow apps are consistently resolved for all invoices.
421-424: Now asserting workflow apps are always populatedThese new assertions confirm that workflow apps and their components (Tax, Invoicing, Payment) are always properly resolved, even when no explicit expansion flags are set. This is an important validation ensuring the new expected behavior works correctly.
openmeter/billing/httpdriver/invoice.go (1)
603-605: Removed WorkflowApps from expansion flagsThe WorkflowApps field has been removed from the InvoiceExpand struct, aligning with the architectural decision to always expand workflow apps regardless of the provided expansion flags. This simplifies the API and ensures consistent behavior across the application.
openmeter/billing/service/invoice.go (2)
57-66: Simplified workflow apps resolutionThe conditional check for
invoice.ExpandedFields.WorkflowAppshas been removed, making theresolveWorkflowAppsmethod always resolve and assign apps toinvoice.Workflow.Apps. This ensures workflow apps are consistently available throughout the invoice lifecycle, simplifying the codebase and reducing potential bugs from inconsistent app availability.
323-327: Now resolving workflow apps earlier in the invoice creation processAdded explicit workflow app resolution before associating invoice lines, ensuring apps are available for downstream operations like
CanDraftSyncAdvancechecks. This is a critical improvement that ensures custom invoicing hooks have access to the workflow apps they need to operate properly.openmeter/server/server_test.go (2)
1443-1445: Implementation of SyncDraftInvoice in NoopBillingService looks good.This method properly implements the billing.Service interface for testing by providing a no-op implementation of the SyncDraftInvoice method that returns an empty invoice and nil error.
1447-1449: Implementation of SyncIssuingInvoice in NoopBillingService looks good.This method properly implements the billing.Service interface for testing by providing a no-op implementation of the SyncIssuingInvoice method that returns an empty invoice and nil error.
openmeter/app/custominvoicing/service.go (3)
7-7: Added billing package import to support the new sync functionality.The import of the billing package is necessary to reference billing types in the new SyncService interface.
13-13: Service interface now embeds SyncService interface.This change extends the Service interface to include sync capabilities by embedding the newly defined SyncService interface.
29-34: Added SyncService interface with invoice synchronization methods.The new SyncService interface defines methods for:
- Synchronizing draft invoices
- Synchronizing issuing invoices
- Handling payment triggers
This interface extension supports the custom invoicing app's ability to interact with external systems at different stages of the invoice lifecycle.
openmeter/app/custominvoicing/service/service.go (5)
9-9: Added billing package import to support integration with billing service.This import is required for the new billing service dependency in the Service struct.
19-20: Added billing service as a dependency to the Service struct.The Service struct now includes the billing service as a dependency, which will be used to implement the newly introduced SyncService interface methods.
27-28: Added BillingService to the Config struct.The Config struct now includes a BillingService field to pass the billing service dependency during service initialization.
44-46: Added validation for billing service dependency.The Validate method now checks that the BillingService is not nil, ensuring that this required dependency is properly provided during service initialization.
57-60: Updated Service initialization to include billing service.The New constructor now properly initializes the Service struct with the billing service from the Config.
openmeter/billing/service.go (3)
18-18: Renamed ConfigIntrospectionService to ConfigService for better clarity.The embedded interface was renamed from ConfigIntrospectionService to ConfigService, which provides a clearer and more concise name while maintaining the same functionality.
83-85: Added async synchronization methods to InvoiceAppService interface.Two new methods for asynchronous invoice synchronization have been added:
- SyncDraftInvoice - For synchronizing draft invoices
- SyncIssuingInvoice - For synchronizing invoices that are being issued
These methods enable integration with external systems at critical points in the invoice lifecycle.
88-88: Renamed interface to match its usage in the Service interface.The interface name has been updated from ConfigIntrospectionService to ConfigService to maintain consistency with its reference on line 18.
openmeter/app/custominvoicing/factory.go (6)
8-8: Import of billing package addedThe billing package is now correctly imported to support the integration of billing service into the custom invoicing app factory.
51-51: Added billing service dependency to Factory structThe Factory struct now includes a billing service field, enabling invoice synchronization capabilities in the custom invoicing app.
57-57: Added billing service dependency to FactoryConfig structThe billing service field is correctly added to the configuration struct for factory initialization.
69-71: Updated validation to require billing serviceThe validation now properly checks for a non-nil billing service, ensuring that the factory has all required dependencies.
84-84: Assigning billing service in factory constructorThe billing service is correctly assigned from the config to the factory instance.
109-109: Passing billing service to App instanceThe billing service is now correctly passed to the App struct, enabling the app to implement the new invoicing interfaces.
openmeter/billing/service/invoiceapp.go (5)
5-5: Added fmt importThe fmt package is now imported to support error formatting in new validation methods.
63-66: Structured input type for synchronized invoice editingThe new
syncEditInvoiceInputstruct provides a clear interface for the internal edit operation, with fields for the sync input and expected invoice state.
68-115: Added core invoice synchronization methodThe
syncEditInvoicemethod provides transactional synchronization for invoices with proper validation, state checking, metadata merging, and state machine advancement.Key features:
- Validates input before proceeding
- Ensures invoice is in the expected state
- Merges synchronization data into the invoice
- Updates metadata as needed
- Advances the state machine until stable
- Wraps errors as validation errors
This implementation correctly handles the complex workflow of synchronizing invoices with external systems.
117-122: Added method for synchronizing draft invoicesThe
SyncDraftInvoicemethod correctly delegates to the internal sync method with the appropriate expected state (InvoiceStatusDraftSyncing).
124-129: Added method for synchronizing issuing invoicesThe
SyncIssuingInvoicemethod correctly delegates to the internal sync method with the appropriate expected state (InvoiceStatusIssuingSyncing).openmeter/app/custominvoicing/sync.go (4)
1-9: New package with imports for invoice synchronizationThis file correctly establishes a new module for handling invoice synchronization with appropriate imports:
- errors: For joining multiple validation errors
- fmt: For error message formatting
- billing: For invoice-related types
- models: For validation error handling
30-47: Input structure for issuing invoice synchronizationThe
SyncIssuingInvoiceInputstruct and validation are well implemented:
- Appropriate fields for issuing invoice synchronization
- Validation ensuring invoice ID is valid and finalize result is present
- Proper error handling with joined errors and standard wrapping
Similar to the draft input, this lacks the
AdditionalMetadatafield found in the billing package equivalent.
49-66: Input structure for payment trigger handlingThe
HandlePaymentTriggerInputstruct and validation are appropriate:
- Contains necessary fields for invoice ID and trigger
- Validates both required fields
- Uses consistent error handling pattern with other structs
This provides a clean interface for payment triggers in the custom invoicing system.
11-28:Details
✅ Verification successful
Input structure for draft invoice synchronization
The
SyncDraftInvoiceInputstruct and its validation method are well-designed:
- Clear struct fields capturing necessary data for draft invoice sync
- Proper validation logic checking for required fields
- Appropriate error wrapping using
models.NewNillableGenericValidationErrorHowever, I notice that unlike the
billing.SyncDraftInvoiceInputfrom the relevant code snippets, this struct doesn't include anAdditionalMetadatafield. This might be intentional, but should be verified.
🏁 Script executed:
#!/bin/bash # Check how the AdditionalMetadata is handled between these structs # Look for where SyncDraftInvoiceInput is used rg -A 10 "SyncDraftInvoiceInput" --type goLength of output: 8400
Omitting AdditionalMetadata in the app‐level input is intentional
The
SyncDraftInvoiceInputinopenmeter/app/custominvoicing/sync.goonly models the user‑provided fields. Any required metadata (e.g.DraftSyncedAt) is injected inopenmeter/app/custominvoicing/service/sync.gowhen constructing the corebilling.SyncDraftInvoiceInput.No changes needed.
openmeter/billing/service/invoicestate.go (8)
7-7: Added slog import for structured loggingThe slog package is now imported to support structured logging in the state machine operations.
24-24: Added Logger field to InvoiceStateMachineThe state machine now has structured logging capability, enabling better error reporting during state transitions.
113-113: Added guard conditions for draft sync advancementThe state machine now checks if draft sync can advance through the
canDraftSyncAdvancemethod before transitioning to the next state, supporting asynchronous workflows.Also applies to: 120-120
177-180: Added guard for issuing sync advancementThe state machine transition from issuing syncing to issued state is now conditionally guarded based on the
canIssuingSyncAdvancemethod, properly supporting async workflows.
246-246: Logger initialization in WithInvoiceStateMachineThe state machine's logger is now properly initialized from the service's logger, ensuring consistent logging throughout the workflow.
514-514: Simplified result merging in mergeUpsertInvoiceResultThe method now directly delegates to the result's
MergeIntoInvoicemethod, promoting consistent behavior and reducing code duplication.
567-569: Improved error handling in finalizeInvoiceThe method now directly uses the result's
MergeIntoInvoicemethod with proper error handling, making the code more consistent with other merging logic.
606-630: Implemented guards for async invoice processingThese two new methods -
canDraftSyncAdvanceandcanIssuingSyncAdvance- are well-designed:
- They check if the invoicing app implements the
InvoicingAppAsyncSyncerinterface- They properly query the app to determine if synchronous advancement is allowed
- They include appropriate error handling and logging
- They default to allowing advancement if the app doesn't implement the interface
These guards are essential for supporting asynchronous invoice workflows and are implemented in a non-breaking manner.
openmeter/app/custominvoicing/app.go (1)
80-109:FinalizeInvoice– guard against concurrent updates & lowercase draft prefix
The check
strings.HasPrefix(invoice.Number, "DRAFT-")is case‑sensitive; external systems might store"draft-XXX".
Consider normalising to upper‑case before the prefix test.A concurrent finalisation may already have assigned a real invoice number between the read and the update.
A last‑write‑wins update would silently overwrite the correct number.
Adding an optimistic lock (e.g.WHERE version = ?) or re‑reading inside the transaction could mitigate this.Both issues are edge‑cases but can result in duplicate invoice numbers.
3 participants
Overview
Implement the basic backend logic for custom invoicing. Validates the hook support, plus the basic use-case of just setting the payment status.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation