Conversation
📝 WalkthroughWalkthroughCredit purchase intent validation now checks that settlement currency matches the intent's currency for invoice and external settlement types. When a mismatch is detected, the validation fails with an error. A comprehensive test validates both settlement type variants. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
openmeter/billing/charges/creditpurchase/charge.go (2)
148-150: Heads-up: theEffectiveAtearly-return drops accumulatederrs.Not introduced by this PR, but now that
errsaccumulates currency-mismatch errors right above, returning a bareerrors.New(...)here means a caller who sets bothEffectiveAtand a mismatched settlement currency will only see the "effective at is not yet supported" message. Might be worth appending toerrsand falling through to the finalNewNillableGenericValidationErrorfor consistency with the rest ofValidate().♻️ Suggested tweak
- if i.EffectiveAt != nil { - return errors.New("effective at is not yet supported") - } + if i.EffectiveAt != nil { + errs = append(errs, errors.New("effective at is not yet supported")) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/creditpurchase/charge.go` around lines 148 - 150, The current Validate() branch returns a fresh error when i.EffectiveAt != nil, which discards any previously appended validation errors in errs (e.g., currency mismatch); instead append a descriptive error about EffectiveAt to errs (same type/format used elsewhere) and let execution fall through to the final NewNillableGenericValidationError call so all accumulated errs are returned together; update the logic around the EffectiveAt check in Validate() to push the error into errs (referencing the errs variable and the EffectiveAt field) rather than returning immediately.
131-146: Nice currency guard — consider DRY-ing the two arms.The two
casebodies are nearly identical. Since bothInvoiceSettlementandExternalSettlementembedGenericSettlement(which carriesCurrency), you could collapse this into a single accessor/helper and avoid the copy-paste. Totally optional, just a readability nit.♻️ One possible shape
- switch i.Settlement.Type() { - case SettlementTypeInvoice: - settlement, err := i.Settlement.AsInvoiceSettlement() - if err != nil { - errs = append(errs, fmt.Errorf("settlement: %w", err)) - } else if settlement.Currency != i.Currency { - errs = append(errs, fmt.Errorf("settlement currency %q must match credit currency %q", settlement.Currency, i.Currency)) - } - case SettlementTypeExternal: - settlement, err := i.Settlement.AsExternalSettlement() - if err != nil { - errs = append(errs, fmt.Errorf("settlement: %w", err)) - } else if settlement.Currency != i.Currency { - errs = append(errs, fmt.Errorf("settlement currency %q must match credit currency %q", settlement.Currency, i.Currency)) - } - } + if settlementCurrency, ok := i.Settlement.GenericCurrency(); ok && settlementCurrency != i.Currency { + errs = append(errs, fmt.Errorf("settlement currency %q must match credit currency %q", settlementCurrency, i.Currency)) + }(where
GenericCurrency()returns the embeddedGenericSettlement.Currencyfor invoice/external and("", false)for promotional.)As per coding guidelines: "In general when reviewing the Golang code make readability and maintainability a priority".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/creditpurchase/charge.go` around lines 131 - 146, Collapse the duplicated currency checks in the switch by introducing a single accessor that extracts the embedded GenericSettlement.Currency (e.g., add a method like GenericCurrency() on the settlement type/interface that returns (string, bool) or similar) and then replace the two nearly identical branches for SettlementTypeInvoice and SettlementTypeExternal with one shared path that calls i.Settlement.GenericCurrency(), checks the boolean, and compares the returned currency to i.Currency, appending the same error messages (fmt.Errorf("settlement: %w") for accessor errors and the currency mismatch message) so the logic in the switch no longer duplicates AsInvoiceSettlement/AsExternalSettlement handling.openmeter/billing/charges/service/creditpurchase_test.go (1)
99-99: Prefers.T().Context()overcontext.Background()in new tests.Small nit, but the guideline asks us to tie test contexts to the test harness lifecycle for new test code.
- ctx := context.Background() + ctx := s.T().Context()As per coding guidelines: "Use
t.Context()when atesting.Tortesting.TBis available instead of introducingcontext.Background()to keep cancellation and test-scoped lifecycle tied to the test harness".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/service/creditpurchase_test.go` at line 99, Replace the test-local context creation using context.Background() with the test harness context by using the suite's testing.T context; specifically change the usage that creates the variable ctx (currently set from context.Background()) to obtain it from s.T().Context() so the test context is tied to the test lifecycle (look for the ctx := context.Background() in creditpurchase_test.go and replace with ctx from s.T().Context()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@openmeter/billing/charges/creditpurchase/charge.go`:
- Around line 148-150: The current Validate() branch returns a fresh error when
i.EffectiveAt != nil, which discards any previously appended validation errors
in errs (e.g., currency mismatch); instead append a descriptive error about
EffectiveAt to errs (same type/format used elsewhere) and let execution fall
through to the final NewNillableGenericValidationError call so all accumulated
errs are returned together; update the logic around the EffectiveAt check in
Validate() to push the error into errs (referencing the errs variable and the
EffectiveAt field) rather than returning immediately.
- Around line 131-146: Collapse the duplicated currency checks in the switch by
introducing a single accessor that extracts the embedded
GenericSettlement.Currency (e.g., add a method like GenericCurrency() on the
settlement type/interface that returns (string, bool) or similar) and then
replace the two nearly identical branches for SettlementTypeInvoice and
SettlementTypeExternal with one shared path that calls
i.Settlement.GenericCurrency(), checks the boolean, and compares the returned
currency to i.Currency, appending the same error messages
(fmt.Errorf("settlement: %w") for accessor errors and the currency mismatch
message) so the logic in the switch no longer duplicates
AsInvoiceSettlement/AsExternalSettlement handling.
In `@openmeter/billing/charges/service/creditpurchase_test.go`:
- Line 99: Replace the test-local context creation using context.Background()
with the test harness context by using the suite's testing.T context;
specifically change the usage that creates the variable ctx (currently set from
context.Background()) to obtain it from s.T().Context() so the test context is
tied to the test lifecycle (look for the ctx := context.Background() in
creditpurchase_test.go and replace with ctx from s.T().Context()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 08bada66-ce95-4482-a05e-92fcc8a5fffa
📒 Files selected for processing (2)
openmeter/billing/charges/creditpurchase/charge.goopenmeter/billing/charges/service/creditpurchase_test.go
2 participants
Overview
Forbid purchasing credits for a given fiat currency from a different currency.
Notes for reviewer
Summary by CodeRabbit
Release Notes