feat: add gathering/standard invoice update functions #3860
+914
−294
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
📝 WalkthroughWalkthroughThis PR introduces a type-discriminated invoice update system, splitting the generic UpdateInvoice method into UpdateStandardInvoice and UpdateGatheringInvoice flows. It expands invoice interfaces with generic line handling (SetLines, GetGenericLines), adds new adapter methods (GetInvoiceType, GetGatheringInvoiceById), and updates invoice-to-API mapping to dispatch by invoice type. Call sites throughout tests and services are migrated to use the appropriate type-specific APIs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Service
participant Adapter
participant DB
participant TypeSystem
Client->>Service: UpdateInvoice(ctx, UpdateInvoiceInput)
Service->>Adapter: GetInvoiceType(invoiceID)
Adapter->>DB: Query invoice status
DB-->>Adapter: Return status
Adapter-->>Service: Return InvoiceType
alt Type == Gathering
Service->>Service: UpdateGatheringInvoice(ctx, EditFn)
else Type == Standard
Service->>Service: UpdateStandardInvoice(ctx, EditFn)
end
Service->>Service: executeTriggerOnInvoice with edit callback
Service->>DB: Update invoice with EditFn result
DB-->>Service: Updated invoice
Service-->>Client: Return Invoice
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@openmeter/billing/httpdriver/invoiceline.go`:
- Around line 790-854: mergeGatheringLineFromInvoiceLineReplaceUpdate currently
assigns existing.Price = lo.FromPtr(rateCardParsed.Price) without handling a nil
rateCardParsed.Price, which will overwrite the value with zero instead of
preserving or rejecting it; update the function
mergeGatheringLineFromInvoiceLineReplaceUpdate to check rateCardParsed.Price for
nil and either preserve existing.Price when nil or return a
billing.ValidationError (consistent with
gatheringLineFromInvoiceLineReplaceUpdate behavior), referencing
rateCardParsed.Price and existing.Price to locate the change.
In `@openmeter/billing/service/gatheringinvoice.go`:
- Around line 209-213: GetGatheringInvoiceById is missing input validation like
other methods; call input.Validate() at the start of
Service.GetGatheringInvoiceById and return an empty billing.GatheringInvoice
plus the error if validation fails, before invoking transaction.Run with
s.adapter and s.adapter.GetGatheringInvoiceById, so the method follows the same
validation pattern as ListGatheringInvoices and UpdateGatheringInvoice.
In `@openmeter/billing/service/invoice.go`:
- Around line 820-899: UpdateInvoice ignores input.IncludeDeletedLines so edit
callbacks can't see deleted lines; pass input.IncludeDeletedLines through into
the type-specific update calls by adding IncludeDeletedLines:
input.IncludeDeletedLines to the UpdateGatheringInvoice call
(UpdateGatheringInvoiceInput) and to the UpdateStandardInvoice call
(UpdateStandardInvoiceInput), and also include input.IncludeDeletedLines when
refetching the invoice (e.g., the s.adapter.GetGatheringInvoiceById /
GetStandardInvoiceById call) if those retrieval inputs support that flag so the
returned invoice matches the requested visibility.
🧹 Nitpick comments (4)
openmeter/billing/httpdriver/invoiceline.go (1)
923-968: Gathering line merge flow looks solid, with one note on duplication.The logic here parallels
mergeStandardInvoiceLinesFromAPIclosely, which is fine given the different underlying types. The absence ofSnapshotLineQuantitycalls is correct for gathering invoices since they don't need quantity snapshots.One thing: the two
merge*InvoiceLinesFromAPIfunctions share the same pattern (group by ID → iterate → create-or-merge → mark deletions). If this pattern grows more complex over time, consider extracting the shared orchestration via generics. Not urgent though — the current approach is clear and type-safe.openmeter/billing/adapter/invoice.go (1)
890-894: Useinvoice.Typedirectly instead of inferring from status.The invoice entity already stores a
Typefield (set during creation, validated as immutable). Inferring the type frominvoice.Status == billing.StandardInvoiceStatusGatheringduplicates that source of truth. Other parts of the codebase useinvoice.Typedirectly (e.g., line 282 in gatheringinvoice.go and line 645 in adapter/invoice.go), so using it here would be more consistent and robust.test/billing/invoice_test.go (1)
773-782: Add a test for the genericUpdateInvoicedispatcherRight now the test exercises
UpdateStandardInvoicedirectly, which is great. But the genericUpdateInvoicemethod—which dispatches to eitherUpdateStandardInvoiceorUpdateGatheringInvoicebased on invoice type—doesn't have direct test coverage. This dispatcher has real logic (type checking, EditFn wrapping), so a focused test hitting the StandardInvoice path throughUpdateInvoicewould help keep the public API from drifting.openmeter/billing/httpdriver/invoice.go (1)
505-523: Avoid the extra customer override lookup for standard invoices. UpdateInvoice now always fetches overrides, but the standard mapper doesn’t need them; branching by invoice type keeps the standard path lean and saves a DB roundtrip.💡 Suggested refactor
- genericInvoice, err := invoice.AsGenericInvoice() - if err != nil { - return UpdateInvoiceResponse{}, fmt.Errorf("converting invoice to generic invoice: %w", err) - } - - // TODO: For the V3 api let's make sure that we don't return gathering invoice customer data (or even gathering invoices) - mergedProfile, err := h.service.GetCustomerOverride(ctx, billing.GetCustomerOverrideInput{ - Customer: genericInvoice.GetCustomerID(), - Expand: billing.CustomerOverrideExpand{ - Customer: true, - Apps: true, - }, - }) - if err != nil { - return UpdateInvoiceResponse{}, fmt.Errorf("failed to get customer override: %w", err) - } - - return MapInvoiceToAPI(invoice, mergedProfile.Customer, mergedProfile.MergedProfile) + if invoice.Type() == billing.InvoiceTypeStandard { + stdInvoice, err := invoice.AsStandardInvoice() + if err != nil { + return UpdateInvoiceResponse{}, fmt.Errorf("converting invoice to standard invoice: %w", err) + } + + return MapStandardInvoiceToAPI(stdInvoice) + } + + genericInvoice, err := invoice.AsGenericInvoice() + if err != nil { + return UpdateInvoiceResponse{}, fmt.Errorf("converting invoice to generic invoice: %w", err) + } + + // TODO: For the V3 api let's make sure that we don't return gathering invoice customer data (or even gathering invoices) + mergedProfile, err := h.service.GetCustomerOverride(ctx, billing.GetCustomerOverrideInput{ + Customer: genericInvoice.GetCustomerID(), + Expand: billing.CustomerOverrideExpand{ + Customer: true, + Apps: true, + }, + }) + if err != nil { + return UpdateInvoiceResponse{}, fmt.Errorf("failed to get customer override: %w", err) + } + + return MapInvoiceToAPI(invoice, mergedProfile.Customer, mergedProfile.MergedProfile)
Comment on lines
+790
to
+854
| func mergeGatheringLineFromInvoiceLineReplaceUpdate(existing billing.GatheringLine, line api.InvoiceLineReplaceUpdate) (billing.GatheringLine, error) { | ||
| old, err := existing.Clone() | ||
| if err != nil { | ||
| return billing.GatheringLine{}, fmt.Errorf("cloning existing line: %w", err) | ||
| } | ||
|
|
||
| rateCardParsed, err := mapAndValidateInvoiceLineRateCardDeprecatedFields(invoiceLineRateCardItems{ | ||
| RateCard: line.RateCard, | ||
| Price: line.Price, | ||
| TaxConfig: line.TaxConfig, | ||
| FeatureKey: line.FeatureKey, | ||
| }) | ||
| if err != nil { | ||
| return billing.GatheringLine{}, billing.ValidationError{ | ||
| Err: fmt.Errorf("failed to map usage based line: %w", err), | ||
| } | ||
| } | ||
|
|
||
| existing.Metadata = lo.FromPtrOr(line.Metadata, api.Metadata(existing.Metadata)) | ||
| existing.Name = line.Name | ||
| existing.Description = line.Description | ||
|
|
||
| existing.ServicePeriod.From = line.Period.From.Truncate(streaming.MinimumWindowSizeDuration) | ||
| existing.ServicePeriod.To = line.Period.To.Truncate(streaming.MinimumWindowSizeDuration) | ||
| existing.InvoiceAt = line.InvoiceAt.Truncate(streaming.MinimumWindowSizeDuration) | ||
|
|
||
| existing.TaxConfig = rateCardParsed.TaxConfig | ||
| existing.Price = lo.FromPtr(rateCardParsed.Price) | ||
| existing.FeatureKey = rateCardParsed.FeatureKey | ||
|
|
||
| // Rate card discounts are not allowed to be updated on a progressively billed line (e.g. if there is | ||
| // already a partial invoice created), as we might go short on the discount quantity. | ||
| // | ||
| // If this is ever requested: | ||
| // - we should introduce the concept of a "discount pool" that is shared across invoices and | ||
| // - editing the discount edits the pool | ||
| // - editing requires that the discount pool's quantity cannot be less than the already used | ||
| // quantity. | ||
|
|
||
| if existing.SplitLineGroupID != nil && rateCardParsed.Discounts.Usage != nil && existing.RateCardDiscounts.Usage != nil { | ||
| if !equal.PtrEqual(rateCardParsed.Discounts.Usage, existing.RateCardDiscounts.Usage) { | ||
| return billing.GatheringLine{}, billing.ValidationError{ | ||
| Err: fmt.Errorf("line[%s]: %w", existing.ID, billing.ErrInvoiceLineProgressiveBillingUsageDiscountUpdateForbidden), | ||
| } | ||
| } | ||
| } | ||
|
|
||
| existing.RateCardDiscounts = rateCardParsed.Discounts | ||
|
|
||
| if !old.Equal(existing) { | ||
| existing.ManagedBy = billing.ManuallyManagedLine | ||
| } | ||
|
|
||
| // We are not allowing period change for split lines (or their children), as that would mess up the | ||
| // calculation logic and/or we would need to update multiple invoices to correct all the references. | ||
| // | ||
| // Deletion is allowed. | ||
| if old.SplitLineGroupID != nil && !old.ServicePeriod.Equal(existing.ServicePeriod) { | ||
| return billing.GatheringLine{}, billing.ValidationError{ | ||
| Err: fmt.Errorf("line[%s]: %w", existing.ID, billing.ErrInvoiceLineNoPeriodChangeForSplitLine), | ||
| } | ||
| } | ||
|
|
||
| return existing, nil | ||
| } |
Contributor
There was a problem hiding this comment.
Missing nil check for Price in the gathering line merge path.
In gatheringLineFromInvoiceLineReplaceUpdate (Line 691-695), you guard against a nil price and return a ValidationError. However, in mergeGatheringLineFromInvoiceLineReplaceUpdate, Line 817 does existing.Price = lo.FromPtr(rateCardParsed.Price) without a nil check. If rateCardParsed.Price is nil, this silently sets Price to its zero value rather than preserving the existing price or returning an error.
The standard counterpart at Line 750 (existing.UsageBased.Price = rateCardParsed.Price) gets away with it because Price is a pointer there. Since GatheringLineBase.Price is a value type, the behavior differs.
Proposed fix
existing.TaxConfig = rateCardParsed.TaxConfig
- existing.Price = lo.FromPtr(rateCardParsed.Price)
+ if rateCardParsed.Price != nil {
+ existing.Price = lo.FromPtr(rateCardParsed.Price)
+ }
existing.FeatureKey = rateCardParsed.FeatureKeyOr, if a price should always be required on update (consistent with the create path), add a validation error for nil:
+ if rateCardParsed.Price == nil {
+ return billing.GatheringLine{}, billing.ValidationError{
+ Err: fmt.Errorf("price is required for usage based lines"),
+ }
+ }
+
existing.TaxConfig = rateCardParsed.TaxConfig
existing.Price = lo.FromPtr(rateCardParsed.Price)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func mergeGatheringLineFromInvoiceLineReplaceUpdate(existing billing.GatheringLine, line api.InvoiceLineReplaceUpdate) (billing.GatheringLine, error) { | |
| old, err := existing.Clone() | |
| if err != nil { | |
| return billing.GatheringLine{}, fmt.Errorf("cloning existing line: %w", err) | |
| } | |
| rateCardParsed, err := mapAndValidateInvoiceLineRateCardDeprecatedFields(invoiceLineRateCardItems{ | |
| RateCard: line.RateCard, | |
| Price: line.Price, | |
| TaxConfig: line.TaxConfig, | |
| FeatureKey: line.FeatureKey, | |
| }) | |
| if err != nil { | |
| return billing.GatheringLine{}, billing.ValidationError{ | |
| Err: fmt.Errorf("failed to map usage based line: %w", err), | |
| } | |
| } | |
| existing.Metadata = lo.FromPtrOr(line.Metadata, api.Metadata(existing.Metadata)) | |
| existing.Name = line.Name | |
| existing.Description = line.Description | |
| existing.ServicePeriod.From = line.Period.From.Truncate(streaming.MinimumWindowSizeDuration) | |
| existing.ServicePeriod.To = line.Period.To.Truncate(streaming.MinimumWindowSizeDuration) | |
| existing.InvoiceAt = line.InvoiceAt.Truncate(streaming.MinimumWindowSizeDuration) | |
| existing.TaxConfig = rateCardParsed.TaxConfig | |
| existing.Price = lo.FromPtr(rateCardParsed.Price) | |
| existing.FeatureKey = rateCardParsed.FeatureKey | |
| // Rate card discounts are not allowed to be updated on a progressively billed line (e.g. if there is | |
| // already a partial invoice created), as we might go short on the discount quantity. | |
| // | |
| // If this is ever requested: | |
| // - we should introduce the concept of a "discount pool" that is shared across invoices and | |
| // - editing the discount edits the pool | |
| // - editing requires that the discount pool's quantity cannot be less than the already used | |
| // quantity. | |
| if existing.SplitLineGroupID != nil && rateCardParsed.Discounts.Usage != nil && existing.RateCardDiscounts.Usage != nil { | |
| if !equal.PtrEqual(rateCardParsed.Discounts.Usage, existing.RateCardDiscounts.Usage) { | |
| return billing.GatheringLine{}, billing.ValidationError{ | |
| Err: fmt.Errorf("line[%s]: %w", existing.ID, billing.ErrInvoiceLineProgressiveBillingUsageDiscountUpdateForbidden), | |
| } | |
| } | |
| } | |
| existing.RateCardDiscounts = rateCardParsed.Discounts | |
| if !old.Equal(existing) { | |
| existing.ManagedBy = billing.ManuallyManagedLine | |
| } | |
| // We are not allowing period change for split lines (or their children), as that would mess up the | |
| // calculation logic and/or we would need to update multiple invoices to correct all the references. | |
| // | |
| // Deletion is allowed. | |
| if old.SplitLineGroupID != nil && !old.ServicePeriod.Equal(existing.ServicePeriod) { | |
| return billing.GatheringLine{}, billing.ValidationError{ | |
| Err: fmt.Errorf("line[%s]: %w", existing.ID, billing.ErrInvoiceLineNoPeriodChangeForSplitLine), | |
| } | |
| } | |
| return existing, nil | |
| } | |
| func mergeGatheringLineFromInvoiceLineReplaceUpdate(existing billing.GatheringLine, line api.InvoiceLineReplaceUpdate) (billing.GatheringLine, error) { | |
| old, err := existing.Clone() | |
| if err != nil { | |
| return billing.GatheringLine{}, fmt.Errorf("cloning existing line: %w", err) | |
| } | |
| rateCardParsed, err := mapAndValidateInvoiceLineRateCardDeprecatedFields(invoiceLineRateCardItems{ | |
| RateCard: line.RateCard, | |
| Price: line.Price, | |
| TaxConfig: line.TaxConfig, | |
| FeatureKey: line.FeatureKey, | |
| }) | |
| if err != nil { | |
| return billing.GatheringLine{}, billing.ValidationError{ | |
| Err: fmt.Errorf("failed to map usage based line: %w", err), | |
| } | |
| } | |
| existing.Metadata = lo.FromPtrOr(line.Metadata, api.Metadata(existing.Metadata)) | |
| existing.Name = line.Name | |
| existing.Description = line.Description | |
| existing.ServicePeriod.From = line.Period.From.Truncate(streaming.MinimumWindowSizeDuration) | |
| existing.ServicePeriod.To = line.Period.To.Truncate(streaming.MinimumWindowSizeDuration) | |
| existing.InvoiceAt = line.InvoiceAt.Truncate(streaming.MinimumWindowSizeDuration) | |
| if rateCardParsed.Price == nil { | |
| return billing.GatheringLine{}, billing.ValidationError{ | |
| Err: fmt.Errorf("price is required for usage based lines"), | |
| } | |
| } | |
| existing.TaxConfig = rateCardParsed.TaxConfig | |
| existing.Price = lo.FromPtr(rateCardParsed.Price) | |
| existing.FeatureKey = rateCardParsed.FeatureKey | |
| // Rate card discounts are not allowed to be updated on a progressively billed line (e.g. if there is | |
| // already a partial invoice created), as we might go short on the discount quantity. | |
| // | |
| // If this is ever requested: | |
| // - we should introduce the concept of a "discount pool" that is shared across invoices and | |
| // - editing the discount edits the pool | |
| // - editing requires that the discount pool's quantity cannot be less than the already used | |
| // quantity. | |
| if existing.SplitLineGroupID != nil && rateCardParsed.Discounts.Usage != nil && existing.RateCardDiscounts.Usage != nil { | |
| if !equal.PtrEqual(rateCardParsed.Discounts.Usage, existing.RateCardDiscounts.Usage) { | |
| return billing.GatheringLine{}, billing.ValidationError{ | |
| Err: fmt.Errorf("line[%s]: %w", existing.ID, billing.ErrInvoiceLineProgressiveBillingUsageDiscountUpdateForbidden), | |
| } | |
| } | |
| } | |
| existing.RateCardDiscounts = rateCardParsed.Discounts | |
| if !old.Equal(existing) { | |
| existing.ManagedBy = billing.ManuallyManagedLine | |
| } | |
| // We are not allowing period change for split lines (or their children), as that would mess up the | |
| // calculation logic and/or we would need to update multiple invoices to correct all the references. | |
| // | |
| // Deletion is allowed. | |
| if old.SplitLineGroupID != nil && !old.ServicePeriod.Equal(existing.ServicePeriod) { | |
| return billing.GatheringLine{}, billing.ValidationError{ | |
| Err: fmt.Errorf("line[%s]: %w", existing.ID, billing.ErrInvoiceLineNoPeriodChangeForSplitLine), | |
| } | |
| } | |
| return existing, nil | |
| } |
🤖 Prompt for AI Agents
In `@openmeter/billing/httpdriver/invoiceline.go` around lines 790 - 854,
mergeGatheringLineFromInvoiceLineReplaceUpdate currently assigns existing.Price
= lo.FromPtr(rateCardParsed.Price) without handling a nil rateCardParsed.Price,
which will overwrite the value with zero instead of preserving or rejecting it;
update the function mergeGatheringLineFromInvoiceLineReplaceUpdate to check
rateCardParsed.Price for nil and either preserve existing.Price when nil or
return a billing.ValidationError (consistent with
gatheringLineFromInvoiceLineReplaceUpdate behavior), referencing
rateCardParsed.Price and existing.Price to locate the change.
Comment on lines
+209
to
+213
| func (s *Service) GetGatheringInvoiceById(ctx context.Context, input billing.GetGatheringInvoiceByIdInput) (billing.GatheringInvoice, error) { | ||
| return transaction.Run(ctx, s.adapter, func(ctx context.Context) (billing.GatheringInvoice, error) { | ||
| return s.adapter.GetGatheringInvoiceById(ctx, input) | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the structure of the gatheringinvoice.go file
cat -n openmeter/billing/service/gatheringinvoice.go | head -100Repository: openmeterio/openmeter
Length of output: 3993
🏁 Script executed:
# Also check what's around lines 209-213 to see GetGatheringInvoiceById
cat -n openmeter/billing/service/gatheringinvoice.go | sed -n '200,220p'Repository: openmeterio/openmeter
Length of output: 617
🏁 Script executed:
# Check lines around 20 to see ListGatheringInvoices
cat -n openmeter/billing/service/gatheringinvoice.go | sed -n '15,35p'Repository: openmeterio/openmeter
Length of output: 1082
🏁 Script executed:
# Check lines around 92 to see UpdateGatheringInvoice
cat -n openmeter/billing/service/gatheringinvoice.go | sed -n '85,105p'Repository: openmeterio/openmeter
Length of output: 873
🏁 Script executed:
# Search for GetGatheringInvoiceByIdInput definition and its Validate method
rg -n "type GetGatheringInvoiceByIdInput" --type=goRepository: openmeterio/openmeter
Length of output: 149
🏁 Script executed:
# Search for Validate method on GetGatheringInvoiceByIdInput
rg -n "func.*\*GetGatheringInvoiceByIdInput.*Validate" --type=goRepository: openmeterio/openmeter
Length of output: 47
🏁 Script executed:
# Also search more broadly for Validate methods on input types
rg -n "\.Validate\(\)" openmeter/billing/service/gatheringinvoice.go --type=goRepository: openmeterio/openmeter
Length of output: 250
🏁 Script executed:
# Get the definition of GetGatheringInvoiceByIdInput
cat -n openmeter/billing/gatheringinvoice.go | sed -n '820,850p'Repository: openmeterio/openmeter
Length of output: 988
🏁 Script executed:
# Search for any Validate method on input types in the billing package
rg -n "func.*Validate.*error" openmeter/billing/ --type=go | grep -i "input"Repository: openmeterio/openmeter
Length of output: 6342
Add input validation to match the pattern used elsewhere in this file.
GetGatheringInvoiceByIdInput has a Validate() method that checks both the Invoice and Expand fields. Since ListGatheringInvoices (line 20) and UpdateGatheringInvoice (line 92) both validate their inputs before proceeding, GetGatheringInvoiceById should do the same for consistency.
func (s *Service) GetGatheringInvoiceById(ctx context.Context, input billing.GetGatheringInvoiceByIdInput) (billing.GatheringInvoice, error) {
if err := input.Validate(); err != nil {
return billing.GatheringInvoice{}, err
}
return transaction.Run(ctx, s.adapter, func(ctx context.Context) (billing.GatheringInvoice, error) {
return s.adapter.GetGatheringInvoiceById(ctx, input)
})
}🤖 Prompt for AI Agents
In `@openmeter/billing/service/gatheringinvoice.go` around lines 209 - 213,
GetGatheringInvoiceById is missing input validation like other methods; call
input.Validate() at the start of Service.GetGatheringInvoiceById and return an
empty billing.GatheringInvoice plus the error if validation fails, before
invoking transaction.Run with s.adapter and s.adapter.GetGatheringInvoiceById,
so the method follows the same validation pattern as ListGatheringInvoices and
UpdateGatheringInvoice.
Comment on lines
+820
to
+899
| func (s *Service) UpdateInvoice(ctx context.Context, input billing.UpdateInvoiceInput) (billing.Invoice, error) { | ||
| if err := input.Validate(); err != nil { | ||
| return billing.Invoice{}, billing.ValidationError{ | ||
| Err: err, | ||
| } | ||
| } | ||
| return transaction.Run(ctx, s.adapter, func(ctx context.Context) (billing.Invoice, error) { | ||
| invoiceType, err := s.adapter.GetInvoiceType(ctx, input.Invoice) | ||
| if err != nil { | ||
| return billing.Invoice{}, fmt.Errorf("getting invoice type: %w", err) | ||
| } | ||
|
|
||
| if invoiceType == billing.InvoiceTypeGathering { | ||
| err := s.UpdateGatheringInvoice(ctx, billing.UpdateGatheringInvoiceInput{ | ||
| Invoice: input.Invoice, | ||
| EditFn: func(invoice *billing.GatheringInvoice) error { | ||
| if invoice == nil { | ||
| return fmt.Errorf("invoice is nil") | ||
| } | ||
|
|
||
| editedInvoice, err := input.EditFn(billing.NewInvoice(*invoice)) | ||
| if err != nil { | ||
| return fmt.Errorf("editing invoice: %w", err) | ||
| } | ||
|
|
||
| editedGatheringInvoice, err := editedInvoice.AsGatheringInvoice() | ||
| if err != nil { | ||
| return fmt.Errorf("converting invoice to gathering invoice: %w", err) | ||
| } | ||
|
|
||
| *invoice = editedGatheringInvoice | ||
|
|
||
| return nil | ||
| }, | ||
| }) | ||
| if err != nil { | ||
| return billing.Invoice{}, fmt.Errorf("updating gathering invoice: %w", err) | ||
| } | ||
|
|
||
| gatheringInvoice, err := s.adapter.GetGatheringInvoiceById(ctx, billing.GetGatheringInvoiceByIdInput{ | ||
| Invoice: input.Invoice, | ||
| Expand: billing.GatheringInvoiceExpands{ | ||
| billing.GatheringInvoiceExpandLines, | ||
| billing.GatheringInvoiceExpandAvailableActions, | ||
| }, | ||
| }) | ||
| if err != nil { | ||
| return billing.Invoice{}, fmt.Errorf("fetching gathering invoice: %w", err) | ||
| } | ||
|
|
||
| return billing.NewInvoice(gatheringInvoice), nil | ||
| } | ||
|
|
||
| // Standard invoice | ||
| standardInvoice, err := s.UpdateStandardInvoice(ctx, billing.UpdateStandardInvoiceInput{ | ||
| Invoice: input.Invoice, | ||
| EditFn: func(invoice *billing.StandardInvoice) error { | ||
| if invoice == nil { | ||
| return fmt.Errorf("invoice is nil") | ||
| } | ||
|
|
||
| editedInvoice, err := input.EditFn(billing.NewInvoice(*invoice)) | ||
| if err != nil { | ||
| return fmt.Errorf("editing invoice: %w", err) | ||
| } | ||
|
|
||
| editedStandardInvoice, err := editedInvoice.AsStandardInvoice() | ||
| if err != nil { | ||
| return fmt.Errorf("converting invoice to standard invoice: %w", err) | ||
| } | ||
|
|
||
| *invoice = editedStandardInvoice | ||
|
|
||
| return nil | ||
| }, | ||
| }) | ||
| if err != nil { | ||
| return billing.Invoice{}, fmt.Errorf("updating standard invoice: %w", err) | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Propagate IncludeDeletedLines into the type-specific update calls
UpdateInvoiceInput.IncludeDeletedLines is currently ignored, so edit callbacks won’t see deleted lines even when requested. That breaks parity with the type-specific APIs.
🛠️ Proposed fix
- err := s.UpdateGatheringInvoice(ctx, billing.UpdateGatheringInvoiceInput{
- Invoice: input.Invoice,
+ err := s.UpdateGatheringInvoice(ctx, billing.UpdateGatheringInvoiceInput{
+ Invoice: input.Invoice,
+ IncludeDeletedLines: input.IncludeDeletedLines,
EditFn: func(invoice *billing.GatheringInvoice) error {
if invoice == nil {
return fmt.Errorf("invoice is nil")
}
@@
- standardInvoice, err := s.UpdateStandardInvoice(ctx, billing.UpdateStandardInvoiceInput{
- Invoice: input.Invoice,
+ standardInvoice, err := s.UpdateStandardInvoice(ctx, billing.UpdateStandardInvoiceInput{
+ Invoice: input.Invoice,
+ IncludeDeletedLines: input.IncludeDeletedLines,
EditFn: func(invoice *billing.StandardInvoice) error {
if invoice == nil {
return fmt.Errorf("invoice is nil")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (s *Service) UpdateInvoice(ctx context.Context, input billing.UpdateInvoiceInput) (billing.Invoice, error) { | |
| if err := input.Validate(); err != nil { | |
| return billing.Invoice{}, billing.ValidationError{ | |
| Err: err, | |
| } | |
| } | |
| return transaction.Run(ctx, s.adapter, func(ctx context.Context) (billing.Invoice, error) { | |
| invoiceType, err := s.adapter.GetInvoiceType(ctx, input.Invoice) | |
| if err != nil { | |
| return billing.Invoice{}, fmt.Errorf("getting invoice type: %w", err) | |
| } | |
| if invoiceType == billing.InvoiceTypeGathering { | |
| err := s.UpdateGatheringInvoice(ctx, billing.UpdateGatheringInvoiceInput{ | |
| Invoice: input.Invoice, | |
| EditFn: func(invoice *billing.GatheringInvoice) error { | |
| if invoice == nil { | |
| return fmt.Errorf("invoice is nil") | |
| } | |
| editedInvoice, err := input.EditFn(billing.NewInvoice(*invoice)) | |
| if err != nil { | |
| return fmt.Errorf("editing invoice: %w", err) | |
| } | |
| editedGatheringInvoice, err := editedInvoice.AsGatheringInvoice() | |
| if err != nil { | |
| return fmt.Errorf("converting invoice to gathering invoice: %w", err) | |
| } | |
| *invoice = editedGatheringInvoice | |
| return nil | |
| }, | |
| }) | |
| if err != nil { | |
| return billing.Invoice{}, fmt.Errorf("updating gathering invoice: %w", err) | |
| } | |
| gatheringInvoice, err := s.adapter.GetGatheringInvoiceById(ctx, billing.GetGatheringInvoiceByIdInput{ | |
| Invoice: input.Invoice, | |
| Expand: billing.GatheringInvoiceExpands{ | |
| billing.GatheringInvoiceExpandLines, | |
| billing.GatheringInvoiceExpandAvailableActions, | |
| }, | |
| }) | |
| if err != nil { | |
| return billing.Invoice{}, fmt.Errorf("fetching gathering invoice: %w", err) | |
| } | |
| return billing.NewInvoice(gatheringInvoice), nil | |
| } | |
| // Standard invoice | |
| standardInvoice, err := s.UpdateStandardInvoice(ctx, billing.UpdateStandardInvoiceInput{ | |
| Invoice: input.Invoice, | |
| EditFn: func(invoice *billing.StandardInvoice) error { | |
| if invoice == nil { | |
| return fmt.Errorf("invoice is nil") | |
| } | |
| editedInvoice, err := input.EditFn(billing.NewInvoice(*invoice)) | |
| if err != nil { | |
| return fmt.Errorf("editing invoice: %w", err) | |
| } | |
| editedStandardInvoice, err := editedInvoice.AsStandardInvoice() | |
| if err != nil { | |
| return fmt.Errorf("converting invoice to standard invoice: %w", err) | |
| } | |
| *invoice = editedStandardInvoice | |
| return nil | |
| }, | |
| }) | |
| if err != nil { | |
| return billing.Invoice{}, fmt.Errorf("updating standard invoice: %w", err) | |
| } | |
| func (s *Service) UpdateInvoice(ctx context.Context, input billing.UpdateInvoiceInput) (billing.Invoice, error) { | |
| if err := input.Validate(); err != nil { | |
| return billing.Invoice{}, billing.ValidationError{ | |
| Err: err, | |
| } | |
| } | |
| return transaction.Run(ctx, s.adapter, func(ctx context.Context) (billing.Invoice, error) { | |
| invoiceType, err := s.adapter.GetInvoiceType(ctx, input.Invoice) | |
| if err != nil { | |
| return billing.Invoice{}, fmt.Errorf("getting invoice type: %w", err) | |
| } | |
| if invoiceType == billing.InvoiceTypeGathering { | |
| err := s.UpdateGatheringInvoice(ctx, billing.UpdateGatheringInvoiceInput{ | |
| Invoice: input.Invoice, | |
| IncludeDeletedLines: input.IncludeDeletedLines, | |
| EditFn: func(invoice *billing.GatheringInvoice) error { | |
| if invoice == nil { | |
| return fmt.Errorf("invoice is nil") | |
| } | |
| editedInvoice, err := input.EditFn(billing.NewInvoice(*invoice)) | |
| if err != nil { | |
| return fmt.Errorf("editing invoice: %w", err) | |
| } | |
| editedGatheringInvoice, err := editedInvoice.AsGatheringInvoice() | |
| if err != nil { | |
| return fmt.Errorf("converting invoice to gathering invoice: %w", err) | |
| } | |
| *invoice = editedGatheringInvoice | |
| return nil | |
| }, | |
| }) | |
| if err != nil { | |
| return billing.Invoice{}, fmt.Errorf("updating gathering invoice: %w", err) | |
| } | |
| gatheringInvoice, err := s.adapter.GetGatheringInvoiceById(ctx, billing.GetGatheringInvoiceByIdInput{ | |
| Invoice: input.Invoice, | |
| Expand: billing.GatheringInvoiceExpands{ | |
| billing.GatheringInvoiceExpandLines, | |
| billing.GatheringInvoiceExpandAvailableActions, | |
| }, | |
| }) | |
| if err != nil { | |
| return billing.Invoice{}, fmt.Errorf("fetching gathering invoice: %w", err) | |
| } | |
| return billing.NewInvoice(gatheringInvoice), nil | |
| } | |
| // Standard invoice | |
| standardInvoice, err := s.UpdateStandardInvoice(ctx, billing.UpdateStandardInvoiceInput{ | |
| Invoice: input.Invoice, | |
| IncludeDeletedLines: input.IncludeDeletedLines, | |
| EditFn: func(invoice *billing.StandardInvoice) error { | |
| if invoice == nil { | |
| return fmt.Errorf("invoice is nil") | |
| } | |
| editedInvoice, err := input.EditFn(billing.NewInvoice(*invoice)) | |
| if err != nil { | |
| return fmt.Errorf("editing invoice: %w", err) | |
| } | |
| editedStandardInvoice, err := editedInvoice.AsStandardInvoice() | |
| if err != nil { | |
| return fmt.Errorf("converting invoice to standard invoice: %w", err) | |
| } | |
| *invoice = editedStandardInvoice | |
| return nil | |
| }, | |
| }) | |
| if err != nil { | |
| return billing.Invoice{}, fmt.Errorf("updating standard invoice: %w", err) | |
| } |
🤖 Prompt for AI Agents
In `@openmeter/billing/service/invoice.go` around lines 820 - 899, UpdateInvoice
ignores input.IncludeDeletedLines so edit callbacks can't see deleted lines;
pass input.IncludeDeletedLines through into the type-specific update calls by
adding IncludeDeletedLines: input.IncludeDeletedLines to the
UpdateGatheringInvoice call (UpdateGatheringInvoiceInput) and to the
UpdateStandardInvoice call (UpdateStandardInvoiceInput), and also include
input.IncludeDeletedLines when refetching the invoice (e.g., the
s.adapter.GetGatheringInvoiceById / GetStandardInvoiceById call) if those
retrieval inputs support that flag so the returned invoice matches the requested
visibility.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
wip: working on self review
Notes for reviewer
Summary by CodeRabbit
New Features
Refactor