Conversation
GAlexIHU
added
the
release-note/ignore
📝 WalkthroughWalkthroughThis PR extends credit transactions with an optional Changes
Sequence DiagramsequenceDiagram
participant Client as API Client
participant Handler as API Handler
participant Service as Credit Service
participant ChargeService as Charge Service
participant Ledger as Ledger
Client->>Handler: List Credit Transactions
Handler->>Service: ListCreditTransactions()
Service->>Ledger: Fetch ledger transactions
Ledger-->>Service: Return transactions
Service->>Service: Extract charge IDs from annotations
Service->>ChargeService: GetByIDs(charge IDs)
ChargeService-->>Service: Return charge details
Service->>Service: applyChargeMetadataToCreditTransactions()
Note over Service: Populate Name and Description
Service-->>Handler: Return enriched transactions
Handler->>Handler: Map Description to API response
Handler-->>Client: Return API transactions with descriptions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 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: 2
🧹 Nitpick comments (3)
openmeter/ledger/customerbalance/transactions_test.go (1)
74-124: Lovely focused test — consider adding the error path too.The happy path and the "no charge ID annotation" path are both covered nicely. One gap worth filling while you're in here: what happens when
ChargesService.GetByIDsreturns an error? Given the current silent-swallow behavior inapplyChargeMetadataToCreditTransactions, a regression that accidentally starts clobberingName/Descriptionon error would slip through.A tiny extra case using a stub that returns an error from
GetByIDsand asserting items stay untouched (Name == "IssueCustomerReceivableTemplate",Description == nil) would lock down the contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/customerbalance/transactions_test.go` around lines 74 - 124, Add a new sub-test that covers the error path for applyChargeMetadataToCreditTransactions: create a Service whose ChargesService stub (implementing GetByIDs) returns an error, call Service.applyChargeMetadataToCreditTransactions with an item that has ledger.AnnotationChargeID set and a pre-existing Name ("IssueCustomerReceivableTemplate") and nil Description, and assert after the call that the item's Name remains "IssueCustomerReceivableTemplate" and Description is still nil; ensure the stub is wired into the same test harness (e.g., alongside staticChargeService) so the failing GetByIDs path is exercised and items are not mutated on error.openmeter/ledger/customerbalance/testenv_test.go (1)
369-423:GetByIDsis almost a carbon copy ofListChargesbelow it — worth a tiny helper?The split into flat-fee/usage-based IDs, the two service fetches, building the
chargesByIDmap, and the ordered rebuild are duplicated pretty much verbatim betweenGetByIDs(lines 369–423) andListCharges(lines 425–483). A small private helper likehydrateChargesBySearchItems(ctx, items, input.Namespace, input.Expands)would collapse both into a few lines.Also minor: neither switch handles
chargemeta.ChargeTypeCreditPurchase. That's fine today because this test env only creates flat-fee and usage-based charges, but if a future test seeds a credit-purchase charge and queries through thischargeStore, it'll silently disappear from the results. Worth a// only flat-fee and usage-based supported by this test envcomment if not hydrating them here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/customerbalance/testenv_test.go` around lines 369 - 423, GetByIDs duplicates the flat-fee/usage-based split, service fetches, map hydration, and ordered rebuild logic found in ListCharges; extract this into a small private helper (e.g., hydrateChargesBySearchItems(ctx context.Context, items []chargemeta.SearchItem, namespace string, expands []string) (charges.Charges, error)) and call it from both GetByIDs and ListCharges to remove the duplication; ensure the helper performs the ID partitioning (using chargemeta.ChargeTypeFlatFee and chargemeta.ChargeTypeUsageBased), calls flatFeeService.GetByIDs and usageBasedService.GetByIDs, builds the chargesByID map with charges.NewCharge, and reorders results to match the input search items; also add a brief comment in the helper noting that ChargeTypeCreditPurchase is intentionally not handled in this test env so such items would be skipped.openmeter/ledger/customerbalance/transactions.go (1)
265-311: Small readability nit: two passes overitemscould be one.Right now you iterate
itemsonce to collect charge IDs (lines 265–268) and again to apply metadata (lines 297–310). It's readable as-is, but since you're already cachingchargeIDFromAnnotations(item.Annotations)once per item in both loops, you could stash per-index charge IDs in a slice during the first pass and avoid re-computing. Purely optional — the current shape is easy to follow.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/ledger/customerbalance/transactions.go` around lines 265 - 311, The code does two passes over items calling chargeIDFromAnnotations twice; change the first pass (where chargeIDs is built via lo.FilterMap) to also record per-index charge IDs (e.g., a slice or map like chargeIDByIndex[int] = string) so you can reuse those values in the second loop instead of recomputing; keep the existing flow that builds chargeIDs, calls s.ChargesService.GetByIDs, fills chargeDisplayByID, and then iterate items using the cached chargeIDByIndex value to look up metadata and set items[i].Name/Description.
🤖 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/ledger/customerbalance/transactions.go`:
- Around line 128-131: Call site s.applyChargeMetadataToCreditTransactions
currently ignores failures because applyChargeMetadataToCreditTransactions
returns no error; update the function signature to return error (func
applyChargeMetadataToCreditTransactions(... ) error), update its internal calls
to return errors, and change this call site to handle the returned error (either
propagate it from the surrounding function or log-and-continue per the sibling
comment). Ensure you update any callers of
applyChargeMetadataToCreditTransactions and tests accordingly, and choose
consistent behavior (fail hard by returning the error up or swallow with a
structured log) for observability.
- Around line 274-280: The code silently swallows errors from
ChargesService.GetByIDs (and similarly from GetChargeID and
chargeDisplayMetadataFromCharge) which results in blank transaction
names/descriptions; modify the error handling in the block that calls
s.ChargesService.GetByIDs (and the subsequent GetChargeID /
chargeDisplayMetadataFromCharge calls used by
creditTransactionFromLedgerTransaction) to surface failures: either return the
error up to the caller (so ListCreditTransactions can fail) or log the error
(use the service logger at warn/error) and attach a fallback marker to
transaction Name/Description so callers know data is incomplete; update the call
sites in the GetByIDs handling and the code path in
creditTransactionFromLedgerTransaction to propagate or log errors consistently.
---
Nitpick comments:
In `@openmeter/ledger/customerbalance/testenv_test.go`:
- Around line 369-423: GetByIDs duplicates the flat-fee/usage-based split,
service fetches, map hydration, and ordered rebuild logic found in ListCharges;
extract this into a small private helper (e.g., hydrateChargesBySearchItems(ctx
context.Context, items []chargemeta.SearchItem, namespace string, expands
[]string) (charges.Charges, error)) and call it from both GetByIDs and
ListCharges to remove the duplication; ensure the helper performs the ID
partitioning (using chargemeta.ChargeTypeFlatFee and
chargemeta.ChargeTypeUsageBased), calls flatFeeService.GetByIDs and
usageBasedService.GetByIDs, builds the chargesByID map with charges.NewCharge,
and reorders results to match the input search items; also add a brief comment
in the helper noting that ChargeTypeCreditPurchase is intentionally not handled
in this test env so such items would be skipped.
In `@openmeter/ledger/customerbalance/transactions_test.go`:
- Around line 74-124: Add a new sub-test that covers the error path for
applyChargeMetadataToCreditTransactions: create a Service whose ChargesService
stub (implementing GetByIDs) returns an error, call
Service.applyChargeMetadataToCreditTransactions with an item that has
ledger.AnnotationChargeID set and a pre-existing Name
("IssueCustomerReceivableTemplate") and nil Description, and assert after the
call that the item's Name remains "IssueCustomerReceivableTemplate" and
Description is still nil; ensure the stub is wired into the same test harness
(e.g., alongside staticChargeService) so the failing GetByIDs path is exercised
and items are not mutated on error.
In `@openmeter/ledger/customerbalance/transactions.go`:
- Around line 265-311: The code does two passes over items calling
chargeIDFromAnnotations twice; change the first pass (where chargeIDs is built
via lo.FilterMap) to also record per-index charge IDs (e.g., a slice or map like
chargeIDByIndex[int] = string) so you can reuse those values in the second loop
instead of recomputing; keep the existing flow that builds chargeIDs, calls
s.ChargesService.GetByIDs, fills chargeDisplayByID, and then iterate items using
the cached chargeIDByIndex value to look up metadata and set
items[i].Name/Description.
🪄 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
Run ID: 560a0754-4a6e-4af7-8438-ecd9e2d6b1d9
📒 Files selected for processing (6)
api/v3/handlers/customers/credits/convert.goapi/v3/handlers/customers/credits/list_transactions_test.goopenmeter/ledger/customerbalance/service.goopenmeter/ledger/customerbalance/testenv_test.goopenmeter/ledger/customerbalance/transactions.goopenmeter/ledger/customerbalance/transactions_test.go
| } | ||
|
|
||
| s.applyChargeMetadataToCreditTransactions(ctx, input.CustomerID.Namespace, items) | ||
|
|
There was a problem hiding this comment.
Heads up: applyChargeMetadataToCreditTransactions has no return value.
Tiny thing — since the function signature returns nothing, the current call site is fine, but it means the caller can't react to enrichment failures even if you later decide to add observability. If you do plumb in an error return (per the sibling comment), you'll want to decide here whether to fail hard or just log-and-continue.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/ledger/customerbalance/transactions.go` around lines 128 - 131,
Call site s.applyChargeMetadataToCreditTransactions currently ignores failures
because applyChargeMetadataToCreditTransactions returns no error; update the
function signature to return error (func
applyChargeMetadataToCreditTransactions(... ) error), update its internal calls
to return errors, and change this call site to handle the returned error (either
propagate it from the surrounding function or log-and-continue per the sibling
comment). Ensure you update any callers of
applyChargeMetadataToCreditTransactions and tests accordingly, and choose
consistent behavior (fail hard by returning the error up or swallow with a
structured log) for observability.
| chargeEntities, err := s.ChargesService.GetByIDs(ctx, charges.GetByIDsInput{ | ||
| Namespace: namespace, | ||
| IDs: chargeIDs, | ||
| }) | ||
| if err != nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
Silent swallow on GetByIDs failure leaves users staring at nameless transactions.
If ChargesService.GetByIDs errors out (timeout, DB hiccup, etc.), every returned transaction ends up with Name == "" and Description == nil — and the caller has no idea anything went wrong. Since creditTransactionFromLedgerTransaction now initializes Name: "" (line 220) and relies entirely on this step to populate it, a transient failure quietly degrades the UX.
A couple of options depending on how strict you want to be:
- Log the error (even at warn) so operators can diagnose missing names.
- Or propagate the error and let
ListCreditTransactionsdecide whether to fail the request.
At minimum, the same concern applies to lines 284–287 and 289–292 where GetChargeID and chargeDisplayMetadataFromCharge errors are also swallowed — those indicate real data inconsistencies worth surfacing in logs.
💡 Sketch: add a logger and warn on failures
chargeEntities, err := s.ChargesService.GetByIDs(ctx, charges.GetByIDsInput{
Namespace: namespace,
IDs: chargeIDs,
})
if err != nil {
+ s.Logger.WarnContext(ctx, "failed to fetch charges for credit transaction metadata",
+ "namespace", namespace, "error", err)
return
}(would require plumbing a *slog.Logger through Config/Service).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openmeter/ledger/customerbalance/transactions.go` around lines 274 - 280, The
code silently swallows errors from ChargesService.GetByIDs (and similarly from
GetChargeID and chargeDisplayMetadataFromCharge) which results in blank
transaction names/descriptions; modify the error handling in the block that
calls s.ChargesService.GetByIDs (and the subsequent GetChargeID /
chargeDisplayMetadataFromCharge calls used by
creditTransactionFromLedgerTransaction) to surface failures: either return the
error up to the caller (so ListCreditTransactions can fail) or log the error
(use the service logger at warn/error) and attach a fallback marker to
transaction Name/Description so callers know data is incomplete; update the call
sites in the GetByIDs handling and the code path in
creditTransactionFromLedgerTransaction to propagate or log errors consistently.
2 participants
Overview
Fixes #(issue)
Notes for reviewer
Summary by CodeRabbit
New Features
Tests