fix(ledger): nit comments from #4731 - #4839
Conversation
mark-vass-konghq
added
release-note/feature
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds JSON and text unmarshalling for ChangesCurrency handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
openmeter/billing/charges/creditpurchase/settlement.go (1)
335-352: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the nullable settlement-currency contract.
GetCurrencynow returns(nil, nil)for promotional settlements, returns a non-nil pointer for invoice and external settlements, and returns errors for invalid or incomplete state. Add a Go doc comment that explains these semantics and why promotional settlements have no fiat currency.Suggested documentation
+// GetCurrency returns the fiat currency for invoice and external settlements. +// It returns nil, nil for promotional settlements, which have no fiat +// settlement currency, and returns an error for invalid or incomplete state. func (s Settlement) GetCurrency() (*currencyx.FiatCode, error) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/billing/charges/creditpurchase/settlement.go` around lines 335 - 352, Add a Go doc comment directly above Settlement.GetCurrency describing that it returns a non-nil fiat currency for invoice and external settlements, returns nil with no error for promotional settlements because they have no fiat currency, and returns an error for invalid or incomplete settlement state.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openmeter/currencies/currency_test.go`:
- Around line 163-165: Strengthen the test around
CurrencyReference.UnmarshalText by asserting that the unmarshaled reference is
resolved and retains the original custom precision, in addition to the existing
equality check. Update the test using the visible reference and unmarshaled
symbols so a regression dropping the custom resolution snapshot fails.
---
Nitpick comments:
In `@openmeter/billing/charges/creditpurchase/settlement.go`:
- Around line 335-352: Add a Go doc comment directly above
Settlement.GetCurrency describing that it returns a non-nil fiat currency for
invoice and external settlements, returns nil with no error for promotional
settlements because they have no fiat currency, and returns an error for invalid
or incomplete settlement state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e23147cd-a2a2-4e64-b19b-0b958e60c70b
📒 Files selected for processing (50)
api/v3/handlers/customers/charges/convert_test.goapi/v3/handlers/customers/credits/convert_test.goopenmeter/billing/charges/creditpurchase/charge_test.goopenmeter/billing/charges/creditpurchase/service/external_test.goopenmeter/billing/charges/creditpurchase/service/promotional_test.goopenmeter/billing/charges/creditpurchase/settlement.goopenmeter/billing/charges/flatfee/adapter/detailedline_test.goopenmeter/billing/charges/flatfee/adapter/intentoverride_test.goopenmeter/billing/charges/flatfee/adapter/realizationrun_test.goopenmeter/billing/charges/flatfee/charge_test.goopenmeter/billing/charges/lineage/lineage_test.goopenmeter/billing/charges/service/base_test.goopenmeter/billing/charges/service/creditpurchase_test.goopenmeter/billing/charges/service/invoicable_test.goopenmeter/billing/charges/service/lineage_test.goopenmeter/billing/charges/service/taxcode_test.goopenmeter/billing/charges/service/truncation_test.goopenmeter/billing/charges/usagebased/adapter/detailedline_test.goopenmeter/billing/charges/usagebased/adapter/intentoverride_test.goopenmeter/billing/charges/usagebased/charge_test.goopenmeter/billing/charges/usagebased/service/creditheninvoice_test.goopenmeter/billing/charges/usagebased/service/creditsonly_test.goopenmeter/billing/charges/usagebased/service/rating/service_test.goopenmeter/billing/charges/usagebased/service/rating/subtract/subtract_test.goopenmeter/billing/charges/usagebased/service/rating/testutils/testutils.goopenmeter/billing/charges/usagebased/service/run/payment_test.goopenmeter/billing/worker/subscriptionsync/service/reconciler/patchcharge_test.goopenmeter/billing/worker/subscriptionsync/service/sync_credittheninvoice_test.goopenmeter/currencies/adapter/currencies_test.goopenmeter/currencies/currency.goopenmeter/currencies/currency_test.goopenmeter/currencies/currencyresolver/resolver_test.goopenmeter/currencies/service/service_test.goopenmeter/currencies/testutils/currency.goopenmeter/currencies/testutils/currency/currency.goopenmeter/currencies/testutils/env/env.goopenmeter/ledger/chargeadapter/creditpurchase.goopenmeter/ledger/chargeadapter/creditpurchase_customcurrency_test.goopenmeter/ledger/chargeadapter/creditpurchase_test.goopenmeter/ledger/chargeadapter/flatfee_test.goopenmeter/ledger/chargeadapter/usagebased_customcurrency_test.goopenmeter/ledger/chargeadapter/usagebased_test.goopenmeter/ledger/collector/collection_fbo_test.goopenmeter/ledger/customerbalance/creditvoid_customcurrency_test.goopenmeter/ledger/customerbalance/expired_loader_test.goopenmeter/ledger/customerbalance/service_test.goopenmeter/ledger/customerbalance/testenv_test.goopenmeter/ledger/recognizer/service_test.gotest/app/stripe/invoice_credits_test.gotest/credits/base.go
💤 Files with no reviewable changes (1)
- openmeter/currencies/testutils/currency/currency.go
| @@ -1,4 +1,4 @@ | |||
| package testutils | |||
There was a problem hiding this comment.
why do we need the subpackage?
There was a problem hiding this comment.
To keep currencies/testutils lightweight. It is imported widely for pure currency fixtures, while NewTestEnv pulls in the DB, adapter, and service setup and is only used by the currencies package tests
There was a problem hiding this comment.
testutils generally arent lightweight and arent meant to be lightweight, are you concerned with this because of test-compile times? those are bad as but thats a more complex topic
| return reference, nil | ||
| } | ||
|
|
||
| func parseCurrencyReference(value []byte) (CurrencyReference, error) { |
There was a problem hiding this comment.
i think there's no point in having this separate if you've implemented UnmarshalText
| var serialized struct { | ||
| Code currencyx.Code `json:"code"` | ||
| CustomCurrencyID *string `json:"custom_currency_id,omitempty"` | ||
| } |
There was a problem hiding this comment.
I do not think you need this. You can unmarshalJSON directly to CurrencyReference as the JSON encoder will skip the private fields anyway.
| return reference, nil | ||
| } | ||
|
|
||
| func parseCurrencyReference(value []byte) (CurrencyReference, error) { |
There was a problem hiding this comment.
I think the implementation of parseCurrencyReference should be moved to UnmarshaText method, no need for a separate parser helper. The ParseCurrencyReference could simple construct an empty CurrecnyReference and unmarshal the byte stream to it. So the parseCurrencyReference feels redundant to me.
a80e04e to
fc4a965
Compare
fc4a965 to
4c9fb8d
Compare
| if settlementCurrency == nil { | ||
| return "", fmt.Errorf("settlement currency is required for a custom currency purchase") | ||
| } | ||
|
|
||
| return currencyx.Code(settlementCurrency), nil | ||
| return currencyx.Code(lo.FromPtr(settlementCurrency)), nil |
There was a problem hiding this comment.
| if settlementCurrency == nil { | |
| return "", fmt.Errorf("settlement currency is required for a custom currency purchase") | |
| } | |
| return currencyx.Code(settlementCurrency), nil | |
| return currencyx.Code(lo.FromPtr(settlementCurrency)), nil | |
| sc := lo.FromPtr(settlementCurrency) | |
| if sc == "" { | |
| return "", fmt.Errorf("settlement currency is required for a custom currency purchase") | |
| } | |
| return currencyx.Code(sc), nil |
There was a problem hiding this comment.
Also we might want to validate the currencyx.Code before returning it.
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Greptile Summary
The PR refines currency serialization and settlement-currency handling while consolidating currency test helpers.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR S[Credit purchase settlement] --> G[GetCurrency] G -->|Invoice or external| F[Fiat currency pointer] G -->|Promotional| N[nil currency] F --> P[Payment and cost-basis ledger routing] N --> C[Promotional credit issuance without payment currency]Reviews (5): Last reviewed commit: "fix: settlementcurrency validation" | Re-trigger Greptile
Context used: