refactor: pricer billable period cleanup #3843
Conversation
📝 WalkthroughWalkthroughReworks invoice billability: introduces generic billable-period helpers, changes pricer APIs to return ClosedPeriod, adds StandardLine accessors, centralizes invoicability checks (hasInvoicableLines), and refactors gathering/splitting to operate on *billing.StandardLine and explicit billable periods. Changes
Sequence Diagram(s)mermaid Client->>InvoiceSvc: Request invoice pending lines (currency, asOf) Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
turip
force-pushed
the
refactor/pricer
branch
from
1a4d417 to
c0837ac
Compare
turip
force-pushed
the
refactor/pricer
branch
from
c0837ac to
a4b7ca5
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@openmeter/billing/service/gatheringinvoicependinglines.go`:
- Around line 403-410: The call to gatherInScopeLines is missing the AsOf from
the input, so gatherInScopeLines will receive a zero-time and mis-filter lines;
update the gatherInScopeLines invocation (the gatherInScopeLineInput being
constructed where GatheringInvoicesByCurrency is set for in.Invoice.Currency) to
include AsOf: in.AsOf (matching the hasInvoicableLinesInput validation), so the
struct passed to gatherInScopeLines carries the correct AsOf value.
- Around line 136-140: The check `len(invoicesByCurrency) == 0` inside the loop
is unreachable; instead verify whether the current currency's in-scope lines are
empty. Update the condition inside the loop that uses inScopeLines (from
gatherInScopeLines) to `len(inScopeLines) == 0` (or the equivalent per-currency
slice/map) so you bail with the billing.ValidationError when there are no lines
for that currency; locate the check near the variables invoicesByCurrency and
inScopeLines in the function that gathers invoice pending lines and replace the
invariant global check with the per-currency inScopeLines length check.
🧹 Nitpick comments (4)
openmeter/billing/service/lineservice/usagebasedline.go (1)
78-82: Minor: Consider improving the error message identifier.For flat price lines without a feature key,
line.GetFeatureKey()returns an empty string, which would produce an error like"price is nil for line[]". Consider using a different identifier (like line ID if available through the interface) or handling the empty case.💡 Possible improvement
func newPricerFor(line PriceAccessor) (Pricer, error) { price := line.GetPrice() if price == nil { - return nil, fmt.Errorf("price is nil for line[%s]", line.GetFeatureKey()) + featureKey := line.GetFeatureKey() + if featureKey == "" { + return nil, fmt.Errorf("price is nil for line") + } + return nil, fmt.Errorf("price is nil for line[%s]", featureKey) }openmeter/billing/service/lineservice/billableperiod.go (1)
79-121: Minor: Redundant price nil check.
newPricerFor(line 80) already validates that price is not nil and returns an error. The check at lines 85-88 is therefore redundant, though harmless. You could remove it or keep it as defensive coding—up to you.openmeter/billing/service/lineservice/pricer.go (1)
61-77: Consider addingValidate()calls toCanBeInvoicedAsOfimplementations for consistency and defensive programming.The
Validate()method onCanBeInvoicedAsOfInputchecks for required fields (Line, FeatureMeters, AsOf), but it's not currently called in any of the pricer implementations. While implementations do perform ad-hoc validation on the fields they depend on, adding a defensivein.Validate()call at the start of eachCanBeInvoicedAsOfmethod would catch programming errors earlier and align with the codebase's broader pattern of validating inputs at entry points.openmeter/billing/service/gatheringinvoicependinglines.go (1)
137-139: Minor inconsistency: Pointer vs value forValidationError.This line uses
&billing.ValidationError{}(pointer) while other occurrences in this file (lines 48, 54, 231, 344) usebilling.ValidationError{}(value). Both work correctly as errors, but for consistency, consider using the same style throughout.Suggested fix for consistency
- return nil, &billing.ValidationError{ + return nil, billing.ValidationError{ Err: billing.ErrInvoiceCreateNoLines, }
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openmeter/billing/service/gatheringinvoicependinglines.go (1)
652-669:⚠️ Potential issue | 🟠 MajorAvoid billing a pre‑split line when the truncated period is empty.
WhenpreSplitAtLineEmptyis true, the line is markedDeletedAtbut still returned and later queued for billing (because onlynilis skipped). That can invoice empty/deleted lines. Consider returningnilforPreSplitAtLine(or filtering deleted lines inprepareLinesToBill).🛠️ Suggested fix
- preSplitAtLine := line + preSplitAtLine := line @@ - if preSplitAtLineEmpty { - line.DeletedAt = lo.ToPtr(clock.Now()) - } else { + if preSplitAtLineEmpty { + line.DeletedAt = lo.ToPtr(clock.Now()) + preSplitAtLine = nil + } else { if err := preSplitAtLine.Validate(); err != nil { return res, fmt.Errorf("validating pre split line: %w", err) } } @@ return splitGatheringInvoiceLineResult{ PreSplitAtLine: preSplitAtLine, PostSplitAtLine: postSplitAtLine, GatheringInvoice: gatheringInvoice, }, nil
🤖 Fix all issues with AI agents
In `@openmeter/billing/service/gatheringinvoicependinglines.go`:
- Around line 379-431: hasInvoicableLines always calls gatherInScopeLines with
ProgressiveBilling=false, causing wrong filtering for customers with progressive
billing; add a ProgressiveBilling bool to hasInvoicableLinesInput, propagate it
through hasInvoicableLines into the gatherInScopeLines call (set the
gatherInScopeLineInput.ProgressiveBilling to in.ProgressiveBilling), and update
the caller at invoice.go to pass the resolved ProgressiveBilling value (follow
the InvoicePendingLines pattern for resolving from customerProfile/override).
Ensure hasInvoicableLinesInput.Validate still runs and accepts the new field (no
additional validation needed).
In `@openmeter/billing/service/lineservice/pricer.go`:
- Around line 92-116: In ProgressiveBillingMeteredPricer.CanBeInvoicedAsOf
update the returned ClosedPeriod when progressive billing is enabled so the To
value is capped to the line service period end: compute a cappedTo = min(asOf,
period.To) and return &timeutil.ClosedPeriod{From: period.From, To: cappedTo,}
(preserving existing truncation and the earlier After check); this prevents the
returned period from extending past period.To and avoids downstream
splitAt/out-of-period errors.
🧹 Nitpick comments (2)
openmeter/billing/worker/subscriptionsync/service/sync_test.go (1)
3326-3345: Drop the duplicates.NoError(err)to reduce noise.
You already assert the same thing a few lines later, so this extra check doesn’t add signal.🧹 Suggested cleanup
- s.NoError(err) - updatedLineFromEditedInvoice := s.getLineByChildID(editedInvoice, childUniqueReferenceID) s.NotNil(updatedLineFromEditedInvoice.DeletedAt) s.Equal(billing.ManuallyManagedLine, updatedLineFromEditedInvoice.ManagedBy)openmeter/billing/service/lineservice/billableperiod.go (1)
90-109: Skip meter lookup when progressive billing is off.
Right now we always callisDependingOnIncreaseOnlyMetersfor non-flat prices, even ifProgressiveBillingis already false. Short‑circuiting avoids extra lookups (and potential errors) on the non‑progressive path.♻️ Suggested refactor
- meterTypeAllowsProgressiveBilling := false - if price.Type() != productcatalog.FlatPriceType { - isDependingOnIncreaseOnlyMeters, err := isDependingOnIncreaseOnlyMeters(CanBeInvoicedAsOfInput{ - AsOf: in.AsOf, - ProgressiveBilling: in.ProgressiveBilling, - Line: in.Line, - FeatureMeters: in.FeatureMeters, - }) - if err != nil { - return nil, err - } - - meterTypeAllowsProgressiveBilling = isDependingOnIncreaseOnlyMeters - } + meterTypeAllowsProgressiveBilling := false + if in.ProgressiveBilling && price.Type() != productcatalog.FlatPriceType { + isDependingOnIncreaseOnlyMeters, err := isDependingOnIncreaseOnlyMeters(CanBeInvoicedAsOfInput{ + AsOf: in.AsOf, + ProgressiveBilling: in.ProgressiveBilling, + Line: in.Line, + FeatureMeters: in.FeatureMeters, + }) + if err != nil { + return nil, err + } + + meterTypeAllowsProgressiveBilling = isDependingOnIncreaseOnlyMeters + }
2 participants
Overview
Update pricer to be able to report the billable periods without depending on any specific line type.
Fix an issue that progressive billing might need to bill some in arrears UBP lines in case of subscriptions with smaller billing period than the subscription's billing period.
Notes for reviewer
Summary by CodeRabbit
Improvements
Bug Fixes