Conversation
WalkthroughA defensive nil-check is added to the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #588 +/- ##
==========================================
- Coverage 63.06% 63.05% -0.01%
==========================================
Files 771 771
Lines 34108 34110 +2
==========================================
- Hits 21509 21507 -2
- Misses 11245 11247 +2
- Partials 1354 1356 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
internal/connectors/plugins/public/atlar/balances.go(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: fguery
Repo: formancehq/payments PR: 427
File: internal/connectors/plugins/public/qonto/balances.go:20-55
Timestamp: 2025-05-15T16:35:29.660Z
Learning: The Qonto connector plugin has unit tests for fetchNextBalances in internal/connectors/plugins/public/qonto/balances_test.go, covering success cases, missing FromPayload, invalid JSON handling, and other error conditions.
📚 Learning: 2025-05-15T16:35:29.660Z
Learnt from: fguery
Repo: formancehq/payments PR: 427
File: internal/connectors/plugins/public/qonto/balances.go:20-55
Timestamp: 2025-05-15T16:35:29.660Z
Learning: The Qonto connector plugin has unit tests for fetchNextBalances in internal/connectors/plugins/public/qonto/balances_test.go, covering success cases, missing FromPayload, invalid JSON handling, and other error conditions.
Applied to files:
internal/connectors/plugins/public/atlar/balances.go
📚 Learning: 2024-12-12T12:45:27.164Z
Learnt from: laouji
Repo: formancehq/payments PR: 193
File: internal/connectors/plugins/public/dummypay/client/client.go:104-131
Timestamp: 2024-12-12T12:45:27.164Z
Learning: The `Balance` struct in `internal/connectors/plugins/public/dummypay/client/client.go` is only used in tests, so changing `AmountInMinors` from `int64` to `*big.Int` is not necessary.
Applied to files:
internal/connectors/plugins/public/atlar/balances.go
📚 Learning: 2025-11-06T15:50:27.524Z
Learnt from: paul-nicolas
Repo: formancehq/payments PR: 580
File: internal/connectors/engine/workflow/send_events.go:69-75
Timestamp: 2025-11-06T15:50:27.524Z
Learning: In the payments codebase, the BankAccount model (internal/models/bank_accounts.go) does not have a ConnectorID field. Bank accounts are global entities not scoped to individual connectors, so when sending bank account events via SendEventsRequest, the ConnectorID field should be nil/omitted.
Applied to files:
internal/connectors/plugins/public/atlar/balances.go
📚 Learning: 2025-08-08T13:46:21.578Z
Learnt from: paul-nicolas
Repo: formancehq/payments PR: 509
File: internal/connectors/plugins/public/powens/client/transactions.go:77-91
Timestamp: 2025-08-08T13:46:21.578Z
Learning: In formancehq/payments Powens client (file: internal/connectors/plugins/public/powens/client/transactions.go), keep single-layout time parsing for Transaction fields (Date: time.DateOnly, DateTime: time.RFC3339, LastUpdate: time.DateTime); no need for multi-layout fallbacks per maintainer preference (paul-nicolas) in PR #509.
Applied to files:
internal/connectors/plugins/public/atlar/balances.go
| // https://docs.atlar.com/docs/deprecations#v1-accountbalance | ||
| // some accounts no longer return balance: we'll need to upgrade to API v2 to get those balances | ||
| // but for now we can avoid a panic | ||
| if account.Payload == nil || account.Payload.Balance == nil { | ||
| return models.FetchNextBalancesResponse{}, err | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
LGTM: Defensive nil-check prevents panic.
The nil-check correctly prevents a panic when balance data is unavailable in API v1 responses. Returning an empty balance list (with nil error) when balance is missing is a reasonable defensive approach.
Verify that this implementation matches the v2 approach referenced in the PR description (formancehq/stack#1967). Based on learnings, run the following to check for unit tests covering this nil-balance scenario:
🏁 Script executed:
#!/bin/bash
# Check for unit tests in the atlar balances_test.go file
fd -t f 'balances_test\.go$' internal/connectors/plugins/public/atlar/ --exec cat {}Length of output: 2804
:::
I need to verify the v2 implementation and compare with the Qonto pattern to assess if the nil-balance scenario test coverage is adequate.
🏁 Script executed:
#!/bin/bash
# Search for v2 implementation in internal/connectors/plugins/public/atlar/
# to see if there's a v2 version that handles balances
fd -t f '\.go$' internal/connectors/plugins/public/atlar/ -exec grep -l "v2\|V2" {} \;Length of output: 200
🏁 Script executed:
#!/bin/bash
# Look at Qonto implementation mentioned in learnings for comparison
# Find the Qonto fetchNextBalances function and its nil-balance handling
rg -n -A 20 'func.*fetchNextBalances' internal/connectors/plugins/public/qonto/balances.goLength of output: 837
:::
🏁 Script executed:
#!/bin/bash
# Look at full Qonto balances.go to see nil-balance handling pattern
cat internal/connectors/plugins/public/qonto/balances.goLength of output: 1740
🏁 Script executed:
#!/bin/bash
# Check atlar balances.go lines 33-44 to see what would panic without nil-check
rg -n -A 15 'if account.Payload == nil' internal/connectors/plugins/public/atlar/balances.goLength of output: 593
🏁 Script executed:
#!/bin/bash
# Search for v2 balances in atlar directory (no syntax errors)
find internal/connectors/plugins/public/atlar/ -name '*balances*'Length of output: 110
Missing test case: nil-balance scenario not covered.
The code correctly prevents panic when balance is nil (lines 26-31), but the test suite lacks coverage for this case. Lines 33-44 would dereference nil without this check, so the guard is necessary. However:
- Test gap: All 3 test cases pass non-nil balance or fail earlier. No test for when
account.Payload.Balance == nil. - Behavior concern: Returns empty
FetchNextBalancesResponse{}witherr=nil(silent success), differing from Qonto's pattern of returning errors for missing required data.
Add a test case:
It("should return empty balance when payload balance is nil", func(...) {
req := models.FetchNextBalancesRequest{
PageSize: 60,
FromPayload: []byte(`{"reference": "test"}`),
}
m.EXPECT().GetV1AccountsID(...).Return(
&accounts.GetV1AccountsIDOK{
Payload: &atlar_models.Account{Balance: nil},
},
nil,
)
resp, err := plg.FetchNextBalances(ctx, req)
Expect(err).To(BeNil())
Expect(resp.Balances).To(HaveLen(0))
})
Verify this aligns with v2 behavior referenced in the PR.
🤖 Prompt for AI Agents
internal/connectors/plugins/public/atlar/balances.go lines 26-31: the guard
prevents a panic when account.Payload.Balance == nil but there is no unit test
for that path; add a new test case that mocks GetV1AccountsID to return a
payload with Balance: nil and then calls plg.FetchNextBalances with a
FromPayload (e.g. {"reference":"test"}), asserting that err == nil and
resp.Balances has length 0; place the test alongside the existing
FetchNextBalances tests, set up the mock expectation (returning
&accounts.GetV1AccountsIDOK{Payload: &atlar_models.Account{Balance: nil}}, nil),
and run assertions to ensure behavior stays as an empty response rather than an
error (and note in test comment to revisit when upgrading to API v2).
fguery
left a comment
fguery
left a comment
There was a problem hiding this comment.
Add the test if you can, but regardless LGTM
|
The v1 API will be decomissioned soon so I don't really want to add a new test for this. Let's add tests when we build the v2 API integration. |
2 participants
Making the same change we made for payments v2 here: formancehq/stack#1967