feat: charges example handlers for flat fee/credit purchase#3900
feat: charges example handlers for flat fee/credit purchase#3900
Conversation
📝 WalkthroughWalkthroughThis PR introduces a set of new interfaces and types to the charges package for handling billing lifecycle events. It defines handlers for credit purchases and flat fees, along with supporting types for credit realizations and ledger transaction references, establishing the foundation for event-driven billing operations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
openmeter/billing/charges/credits.go (4)
83-97: Loop includes index 0 redundantly.
lastRealizedPeriodis initialized withr[0], then the loop starts from index 0 again. Starting from index 1 would be slightly cleaner:Proposed tweak
lastRealizedPeriod := r[0].ServicePeriod - for _, realization := range r { + for _, realization := range r[1:] { if realization.ServicePeriod.To.After(lastRealizedPeriod.To) { lastRealizedPeriod = realization.ServicePeriod } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/credits.go` around lines 83 - 97, In CreditRealizations.LastRealizedPeriod(), avoid comparing r[0] against itself by starting the range from the second element: initialize lastRealizedPeriod with r[0].ServicePeriod as you do now, then iterate from index 1 (or skip the first element in the loop) when scanning realizations so you only compare distinct entries; update the loop over r (or use a for i := 1; i < len(r); i++ loop) and keep the existing comparison logic that updates lastRealizedPeriod when realization.ServicePeriod.To is after lastRealizedPeriod.To.
13-20: TODO on line 17 — adding ledger transaction IDs.Just noting the TODO for adding ledger transaction ID(s) to
CreditRealizationCreateInput. Happy to help sketch that out if you'd like.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/credits.go` around lines 13 - 20, The struct CreditRealizationCreateInput needs a field to carry ledger transaction ID(s); add a new optional field (e.g., LedgerTransactionIDs []string `json:"ledgerTransactionIds,omitempty"`) to CreditRealizationCreateInput so callers can attach one or more ledger transaction identifiers, include a short comment above the field explaining purpose, and update any JSON marshalling/validation logic that consumes CreditRealizationCreateInput to handle the new ledger transaction IDs (or switch the field type to []uuid.UUID if your codebase uses UUID types).
75-81:RealizedUnsettledAmountsums all realizations, not just unsettled ones.The method name implies it filters for unsettled realizations, but it sums every realization's
Amountunconditionally. If the intent is that this type only ever contains unsettled realizations, it'd be worth a brief doc comment to clarify that assumption. Otherwise, the name could be simplified to something likeTotalRealizedAmount.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/credits.go` around lines 75 - 81, RealizedUnsettledAmount currently sums every realization in CreditRealizations (sum = sum.Add(realization.Amount)) but its name implies it should only include unsettled items; fix by either (A) filtering to include only unsettled realizations (e.g., check a boolean like realization.Settled or realization.IsSettled and only add when not settled) so RealizedUnsettledAmount matches its name, or (B) if the slice truly only contains unsettled realizations, rename the method to TotalRealizedAmount and add a short doc comment on CreditRealizations/TotalRealizedAmount clarifying the invariant; update callers accordingly.
47-59: Consider validating the embeddedNamespacedIDinCreditRealization.Validate()for consistency.
ReversedCreditRealizationvalidates itsRealizationID(amodels.NamespacedID), butCreditRealizationskips validation of its embeddedNamespacedID. Even if the ID is system-assigned and always valid at this point, adding validation would make the validation logic consistent across both types and align with the pattern used elsewhere in the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/credits.go` around lines 47 - 59, CreditRealization.Validate() currently skips validating the embedded NamespacedID; update CreditRealization.Validate to call the embedded NamespacedID's Validate method (e.g., r.NamespacedID.Validate()) and append any returned error to errs with contextual wrapping (for example: fmt.Errorf("namespaced id: %w", err)) to match the pattern used in ReversedCreditRealization.Validate and keep validation consistent with other types.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openmeter/billing/charges/chargeflatfee.go`:
- Around line 112-115: The JSON tag for PreTaxTotalAmount on the
OnFlatFeeAssignedToInvoiceInput struct is inconsistent (json:"totalAmount") —
update the tag to accurately reflect the field's semantics (for example
json:"preTaxTotalAmount") so serialized JSON matches the Go field name; modify
the struct definition for OnFlatFeeAssignedToInvoiceInput and its
PreTaxTotalAmount field to use the new tag and run tests/linting to ensure no
other code relies on the old "totalAmount" key.
In `@openmeter/billing/charges/credits.go`:
- Around line 29-31: Update the error text to reflect that zero is allowed:
where the code checks i.Amount.IsNegative() and appends fmt.Errorf("amount must
be positive"), change the message to something like "amount cannot be negative"
so the error accurately describes the validation (reference the
i.Amount.IsNegative() check and the appended errs entry).
---
Nitpick comments:
In `@openmeter/billing/charges/credits.go`:
- Around line 83-97: In CreditRealizations.LastRealizedPeriod(), avoid comparing
r[0] against itself by starting the range from the second element: initialize
lastRealizedPeriod with r[0].ServicePeriod as you do now, then iterate from
index 1 (or skip the first element in the loop) when scanning realizations so
you only compare distinct entries; update the loop over r (or use a for i := 1;
i < len(r); i++ loop) and keep the existing comparison logic that updates
lastRealizedPeriod when realization.ServicePeriod.To is after
lastRealizedPeriod.To.
- Around line 13-20: The struct CreditRealizationCreateInput needs a field to
carry ledger transaction ID(s); add a new optional field (e.g.,
LedgerTransactionIDs []string `json:"ledgerTransactionIds,omitempty"`) to
CreditRealizationCreateInput so callers can attach one or more ledger
transaction identifiers, include a short comment above the field explaining
purpose, and update any JSON marshalling/validation logic that consumes
CreditRealizationCreateInput to handle the new ledger transaction IDs (or switch
the field type to []uuid.UUID if your codebase uses UUID types).
- Around line 75-81: RealizedUnsettledAmount currently sums every realization in
CreditRealizations (sum = sum.Add(realization.Amount)) but its name implies it
should only include unsettled items; fix by either (A) filtering to include only
unsettled realizations (e.g., check a boolean like realization.Settled or
realization.IsSettled and only add when not settled) so RealizedUnsettledAmount
matches its name, or (B) if the slice truly only contains unsettled
realizations, rename the method to TotalRealizedAmount and add a short doc
comment on CreditRealizations/TotalRealizedAmount clarifying the invariant;
update callers accordingly.
- Around line 47-59: CreditRealization.Validate() currently skips validating the
embedded NamespacedID; update CreditRealization.Validate to call the embedded
NamespacedID's Validate method (e.g., r.NamespacedID.Validate()) and append any
returned error to errs with contextual wrapping (for example:
fmt.Errorf("namespaced id: %w", err)) to match the pattern used in
ReversedCreditRealization.Validate and keep validation consistent with other
types.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
openmeter/billing/charges/chargecreditpurchase.goopenmeter/billing/charges/chargeflatfee.goopenmeter/billing/charges/credits.goopenmeter/billing/charges/ledger.go
| type OnFlatFeeAssignedToInvoiceInput struct { | ||
| Charge FlatFeeCharge `json:"charge"` | ||
| PreTaxTotalAmount alpacadecimal.Decimal `json:"totalAmount"` | ||
| } |
There was a problem hiding this comment.
JSON tag "totalAmount" doesn't match the field name PreTaxTotalAmount.
The Go field name communicates "pre-tax" semantics, but the JSON tag just says "totalAmount". This mismatch could confuse consumers of the serialized form — they'd see totalAmount and might not realize it's specifically the pre-tax amount. Consider aligning them, e.g., json:"preTaxTotalAmount".
Proposed fix
type OnFlatFeeAssignedToInvoiceInput struct {
Charge FlatFeeCharge `json:"charge"`
- PreTaxTotalAmount alpacadecimal.Decimal `json:"totalAmount"`
+ PreTaxTotalAmount alpacadecimal.Decimal `json:"preTaxTotalAmount"`
}📝 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.
| type OnFlatFeeAssignedToInvoiceInput struct { | |
| Charge FlatFeeCharge `json:"charge"` | |
| PreTaxTotalAmount alpacadecimal.Decimal `json:"totalAmount"` | |
| } | |
| type OnFlatFeeAssignedToInvoiceInput struct { | |
| Charge FlatFeeCharge `json:"charge"` | |
| PreTaxTotalAmount alpacadecimal.Decimal `json:"preTaxTotalAmount"` | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/billing/charges/chargeflatfee.go` around lines 112 - 115, The JSON
tag for PreTaxTotalAmount on the OnFlatFeeAssignedToInvoiceInput struct is
inconsistent (json:"totalAmount") — update the tag to accurately reflect the
field's semantics (for example json:"preTaxTotalAmount") so serialized JSON
matches the Go field name; modify the struct definition for
OnFlatFeeAssignedToInvoiceInput and its PreTaxTotalAmount field to use the new
tag and run tests/linting to ensure no other code relies on the old
"totalAmount" key.
| if i.Amount.IsNegative() { | ||
| errs = append(errs, fmt.Errorf("amount must be positive")) | ||
| } |
There was a problem hiding this comment.
Error message says "must be positive" but zero is allowed.
The check i.Amount.IsNegative() permits zero amounts, but the error message "amount must be positive" implies zero would be rejected. If zero is intentionally valid, the message should say "amount cannot be negative" (or similar) to match the actual behavior.
Proposed fix
if i.Amount.IsNegative() {
- errs = append(errs, fmt.Errorf("amount must be positive"))
+ errs = append(errs, fmt.Errorf("amount cannot be negative"))
}📝 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.
| if i.Amount.IsNegative() { | |
| errs = append(errs, fmt.Errorf("amount must be positive")) | |
| } | |
| if i.Amount.IsNegative() { | |
| errs = append(errs, fmt.Errorf("amount cannot be negative")) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/billing/charges/credits.go` around lines 29 - 31, Update the error
text to reflect that zero is allowed: where the code checks
i.Amount.IsNegative() and appends fmt.Errorf("amount must be positive"), change
the message to something like "amount cannot be negative" so the error
accurately describes the validation (reference the i.Amount.IsNegative() check
and the appended errs entry).
1 participant
Overview
This patch defines the interfaces for credit purchase and flat fee creation. This encapsulates some of the common lifecycle events, but things like voiding is not captured yet, as they are not implemented on billing side.
Summary by CodeRabbit