fix(enable-banking): refactor error handling and add missing GIN index#1432
fix(enable-banking): refactor error handling and add missing GIN index#1432jjmata merged 6 commits intowe-promise:mainfrom
Conversation
|
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:
📝 WalkthroughWalkthroughReplaces message-parsing with exception-type-based error classification in the Enable Banking importer; returns I18n error keys for sync failures, updates session/item state for auth/not-found errors, adds EN/FR translations, adds a concurrent GIN index on identification_hashes, and adds tests for importer error handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Importer as Importer (EnableBankingItem::Importer)
participant Provider as Provider Client
participant Session as Session Store
participant Account as EnableBankingAccount
Importer->>Provider: fetch_session_data
alt EnableBankingError (error_type: :unauthorized/:not_found)
Provider-->>Importer: EnableBankingError(error_type)
Importer->>Session: mark session invalid / update state
Importer->>Account: set item status -> requires_update
Importer-->>Client: return I18n key "enable_banking_items.errors.session_invalid"
else Network/socket/timeout error
Provider-->>Importer: Errno / Timeout / SocketError
Importer-->>Client: return I18n key "enable_banking_items.errors.network_unreachable"
else Other API error
Provider-->>Importer: EnableBankingError(error_type: other)
Importer-->>Client: return I18n key "enable_banking_items.errors.api_error"
end
loop per-account
Importer->>Provider: fetch_and_store_transactions / fetch_and_update_balance
alt Provider error
Provider-->>Importer: localized error key via handle_sync_error
Importer->>Importer: set `@sync_error` / promote_session_invalid if applicable
else success
Provider-->>Importer: transactions/balance updated
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c04d7e82e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![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 (1)
app/models/enable_banking_item/importer.rb (1)
120-126: Optional: avoid rebuilding static exception list per call.
network_errorsis constant data; moving it to a frozen class constant avoids repeated allocations in error-heavy paths.♻️ Suggested refactor
class EnableBankingItem::Importer + NETWORK_ERROR_CLASSES = [ + ::SocketError, + ::Errno::ECONNREFUSED, + ::Timeout::Error, + ::Net::ReadTimeout, + ::Net::OpenTimeout + ].freeze + private def localized_error_message(exception) # Check the underlying cause first, then the exception itself exceptions = [ exception.cause, exception ].compact - network_errors = [ - ::SocketError, - ::Errno::ECONNREFUSED, - ::Timeout::Error, - ::Net::ReadTimeout, - ::Net::OpenTimeout - ] - - if exceptions.any? { |ex| network_errors.any? { |err| ex.is_a?(err) } } + if exceptions.any? { |ex| NETWORK_ERROR_CLASSES.any? { |err| ex.is_a?(err) } } I18n.t("enable_banking_items.errors.network_unreachable", default: "The banking service is temporarily unreachable. Please try again later.")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/enable_banking_item/importer.rb` around lines 120 - 126, The temp variable network_errors is being rebuilt per call; instead define a frozen class constant (e.g. NETWORK_ERRORS = [::SocketError, ::Errno::ECONNREFUSED, ::Timeout::Error, ::Net::ReadTimeout, ::Net::OpenTimeout].freeze) on the Importer class and replace uses of the local network_errors with that constant (or self.class::NETWORK_ERRORS) so the array is allocated once and reused in error-heavy paths. Ensure the constant name is uppercased and frozen to prevent accidental mutation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@db/migrate/20260411082125_add_gin_index_to_enable_banking_accounts_identification_hashes.rb`:
- Line 5: The file ends with the migration class closure but is missing a final
newline which causes a lint failure; open the migration class
AddGinIndexToEnableBankingAccountsIdentificationHashes (in
db/migrate/20260411082125_add_gin_index_to_enable_banking_accounts_identification_hashes.rb)
and add a single trailing newline character after the final `end` so the file
ends with a newline.
- Line 3: This migration currently calls add_index :enable_banking_accounts,
:identification_hashes without using the concurrent algorithm; update the
migration by adding disable_ddl_transaction! at the top of the file and change
the add_index invocation to use algorithm: :concurrently (i.e., add_index
:enable_banking_accounts, :identification_hashes, using: :gin, algorithm:
:concurrently) so the GIN index is created without blocking writes; ensure the
migration does not rely on being wrapped in a transaction after adding
disable_ddl_transaction!.
---
Nitpick comments:
In `@app/models/enable_banking_item/importer.rb`:
- Around line 120-126: The temp variable network_errors is being rebuilt per
call; instead define a frozen class constant (e.g. NETWORK_ERRORS =
[::SocketError, ::Errno::ECONNREFUSED, ::Timeout::Error, ::Net::ReadTimeout,
::Net::OpenTimeout].freeze) on the Importer class and replace uses of the local
network_errors with that constant (or self.class::NETWORK_ERRORS) so the array
is allocated once and reused in error-heavy paths. Ensure the constant name is
uppercased and frozen to prevent accidental mutation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e1007ba0-479a-47f4-bfd2-8d82a4a30655
📒 Files selected for processing (5)
app/models/enable_banking_item/importer.rbconfig/locales/views/enable_banking_items/en.ymlconfig/locales/views/enable_banking_items/fr.ymldb/migrate/20260411082125_add_gin_index_to_enable_banking_accounts_identification_hashes.rbdb/schema.rb
db/migrate/20260411082125_add_gin_index_to_enable_banking_accounts_identification_hashes.rb
Outdated
Show resolved
Hide resolved
db/migrate/20260411082125_add_gin_index_to_enable_banking_accounts_identification_hashes.rb
Outdated
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/models/enable_banking_item/importer.rb`:
- Around line 124-139: The localized_error_message method currently groups
Provider::EnableBanking::EnableBankingError into api_error even though error
types like :unauthorized and :not_found should map to the
reauthorization/reconnect flow; update localized_error_message to check for
Provider::EnableBanking::EnableBankingError with error_type in [:unauthorized,
:not_found] first and return the same I18n key/message used by the
requires_update path (so callers get the reconnect/session-expired message),
then fall back to the existing network_unreachable / api_error / unexpected
branches; reference the localized_error_message method and
Provider::EnableBanking::EnableBankingError.error_type when making this change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 60dac73d-e9cf-4791-b8ba-255f1099ee02
📒 Files selected for processing (1)
app/models/enable_banking_item/importer.rb
github-project-automation
bot
moved this to Todo
in Latest release train (v0.7.0) - April 2026
jjmata
left a comment
jjmata
left a comment
There was a problem hiding this comment.
Here's the Opus 4.6 review, YMMV!
Context
PR #1432 (enable-banking-improvements by @boul2gom) does two things:
- Refactor
EnableBankingItem::Importererror handling — replaces fragile message-string parsing (msg.include?("getaddrinfo"), etc.) with exception-type classification that returns I18n keys (network_unreachable,api_error,unexpected). Adds en/fr translations. The refactor also propagates localized messages up throughfetch_and_store_transactions(previously these rescue blocks returnede.messageraw, bypassing any localization). - Add a concurrent GIN index on
enable_banking_accounts.identification_hashes— optimizes theWHERE identification_hashes @> ?query infind_enable_banking_account_by_hash(app/models/enable_banking_item/importer.rb:434-437).
The PR already went through two bot-review rounds: a Codex finding about :request_failed misclassification was fixed, and CodeRabbit's missing-disable_ddl_transaction!/missing-newline findings were fixed. One CodeRabbit comment remains unresolved (the :unauthorized/:not_found issue below), and this is the most important thing the human review should weigh in on.
The user asked me to review this PR. My deliverable is review feedback — no code changes.
Review findings
Overall: approve-with-comments
The refactor is a clear win. String-matching on exception messages is brittle (locale-dependent, version-dependent, and silently breaks when a gem changes wording); switching to type-based classification plus a frozen NETWORK_ERRORS constant is the right call. The GIN index is correctly created with disable_ddl_transaction! + algorithm: :concurrently and genuinely targets a JSONB @> query that exists in importer.rb:436, so it will actually be used.
Blocking / Major — :unauthorized and :not_found get the wrong user message
File: app/models/enable_banking_item/importer.rb, new localized_error_message method (lines ~124-139 in PR).
Problem: The method classifies any Provider::EnableBanking::EnableBankingError that isn't :request_failed/:timeout as api_error → "A communication error occurred with the bank." But Provider::EnableBanking#handle_response raises :unauthorized on HTTP 401 (app/models/provider/enable_banking.rb:232) and :not_found on HTTP 404 (line 236). These are session-expiration / reconnect signals, not communication errors — fetch_session_data itself already recognizes this and calls enable_banking_item.update!(status: :requires_update) at line 137, and the short-circuit path at line 16 uses the copy "Session expired or invalid".
Impact: When a user's Enable Banking session actually expires, they see "A communication error occurred with the bank" in the UI while the item is silently marked requires_update. They have no idea they need to reconnect. This was the original state before the PR, too, but the PR touches exactly this code path and should fix it while it's here.
Suggested fix (to leave as a review comment):
ruby
def localized_error_message(exception)
exceptions = [ exception.cause, exception ].compact
provider_error = exceptions.find { |ex| ex.is_a?(Provider::EnableBanking::EnableBankingError) }
is_network_error = exceptions.any? do |ex|
NETWORK_ERRORS.any? { |err| ex.is_a?(err) } ||
(ex.is_a?(Provider::EnableBanking::EnableBankingError) && [ :request_failed, :timeout ].include?(ex.error_type))
end
if provider_error && [ :unauthorized, :not_found ].include?(provider_error.error_type)
I18n.t("enable_banking_items.errors.session_invalid", default: "Session expired. Please reconnect your bank.")
elsif is_network_error
I18n.t("enable_banking_items.errors.network_unreachable", default: "The banking service is temporarily unreachable. Please try again later.")
elsif provider_error
I18n.t("enable_banking_items.errors.api_error", default: "A communication error occurred with the bank.")
else
I18n.t("enable_banking_items.errors.unexpected", default: "An unexpected error occurred during synchronization.")
end
end
Plus a new session_invalid key in both config/locales/views/enable_banking_items/en.yml and fr.yml.
This is the same finding CodeRabbit raised at #1432 (comment) and it hasn't been addressed yet.
Minor — transactions path doesn't set requires_update
File: app/models/enable_banking_item/importer.rb, fetch_and_store_transactions rescue (lines ~295-300 in PR).
Observation: get_account_transactions can also raise :unauthorized (same HTTP handler in the provider). fetch_session_data sets status: :requires_update on that error type, but fetch_and_store_transactions does not — it just logs and returns { success: false, error: ... }. If a session expires mid-sync (after the session call succeeded but before transactions are fetched), the item is never flagged for reconnection and the user keeps getting silent failures.
This is pre-existing behavior, not introduced by this PR, but the PR is the natural place to address it since it's already touching these rescue blocks. Acceptable as a follow-up — mention it but do not block.
Nit — I18n default: drift hazard
Each I18n.t call passes a default: that duplicates the YAML string. If someone updates the YAML without updating the Ruby default (or vice versa), a YAML lookup miss will silently return different copy than a successful one. Low-impact; flag only if it comes up naturally.
No test coverage for the refactor
There are no Minitest tests under test/models/enable_banking_item/ covering localized_error_message or the rescue branches. Per CLAUDE.md's "test critical paths" philosophy this is borderline — error classification is a real decision path that's already had one regression caught in review this PR — but it's not blocking. Could be noted as an ask-nicely.
Things verified as good, no action needed
- GIN index migration format —
disable_ddl_transaction!+algorithm: :concurrently, correct Rails pattern. - GIN index is actually used —
importer.rb:436doeswhere("identification_hashes @> ?", [hash_value.to_s].to_json), which is exactly a GIN-accelerated JSONB containment query. schema.rbversion bump (2026_04_10_114435→2026_04_11_082125) and new index line match the migration.NETWORK_ERRORSfrozen constant addresses Codex's earlier nit about re-allocating per call.- Both
en.ymlandfr.ymlgot parallel updates with matching key structures. :request_failed/:timeoutEnableBankingError values are correctly surfaced as network errors (this was the fix from Codex round 1).
Recommended action
Post one GitHub review on the PR with:
- State:
COMMENT(not approve, not request-changes — the blocking finding is already a CodeRabbit thread that just needs a human nudge). - Body: Summarize the overall-good verdict, then:
-
Endorse CodeRabbit's unresolved thread about
:unauthorized/:not_found→ provide the fix snippet.- Note the transactions-path
requires_updategap as a suggested follow-up. - Note the missing test coverage as a soft ask.
Do not leave separate inline review comments for the nits. Do not approve.
Files touched by this PR (no edits from me)
app/models/enable_banking_item/importer.rb— the main refactor target.config/locales/views/enable_banking_items/en.yml,config/locales/views/enable_banking_items/fr.yml— newerrors.*keys.db/migrate/20260411082125_add_gin_index_to_enable_banking_accounts_identification_hashes.rb— new migration.db/schema.rb— regenerated.
Related code read during review (not modified):
app/models/provider/enable_banking.rb:220-265—handle_response+EnableBankingErrorclass.app/models/enable_banking_item/importer.rb:1-50, 116-146, 430-440— entry point, session error path, hash lookup query.
Verification
This is review-only, no implementation. To confirm the blocking finding is valid, exercise:
Stub
enable_banking_provider.get_sessionto raiseProvider::EnableBanking::EnableBankingError.new("Invalid credentials", :unauthorized)and callimporter.import. Expected (after fix): resulterrorcontains the localized session-expired string, not "communication error". - Note the transactions-path
-
Same stub for
get_account_transactions— check whetherenable_banking_item.statustransitions torequires_update(it should, after the follow-up).
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/models/enable_banking_item/importer.rb`:
- Around line 79-80: The current use of ||= for `@sync_error` preserves the first
error and prevents later :session_invalid outcomes from replacing earlier
generic network/API errors; change the assignments in Importer (the occurrences
around the `@sync_error` ||= handle_sync_error(e) lines and the other spots you
noted) so that you compute new_error = handle_sync_error(e) and then set
`@sync_error` = new_error if new_error == :session_invalid or if `@sync_error` is
nil — otherwise keep the existing error; alternatively implement a small helper
(e.g., promote_session_invalid(existing, new)) and use `@sync_error` =
promote_session_invalid(`@sync_error`, new_error) to ensure :session_invalid
always wins over earlier errors.
- Around line 212-215: fetch_and_update_balance currently rescues
Provider::EnableBanking::EnableBankingError, sets `@sync_error` via
handle_sync_error and logs, but lets the import proceed as success; change the
behavior so a balance-only failure marks the import as failed: after setting
`@sync_error` (and logging) either re-raise the exception or return a falsy value,
and update the caller that runs fetch_and_update_balance (the same method that
calls fetch_and_store_transactions) to treat a falsy return or rescued exception
as a failure (set success: false). Reference: fetch_and_update_balance,
fetch_and_store_transactions, `@sync_error`, handle_sync_error,
enable_banking_account — ensure the outer import flow checks `@sync_error` or the
method return and sets the overall result to failed when a
Provider::EnableBanking::EnableBankingError occurs.
In `@test/models/enable_banking_item/importer_error_handling_test.rb`:
- Around line 6-15: The test setup uses EnableBankingItem.create! with an
invalid enum value status: :active which causes creation to fail; update the
create! call in the test to use a valid enum (e.g., status: :good or status:
:requires_update) so EnableBankingItem.create! succeeds, referencing the
EnableBankingItem model and the create! invocation in the test fixture.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f63f01f6-6038-4c4e-9f87-aa5b6bcf4862
📒 Files selected for processing (4)
app/models/enable_banking_item/importer.rbconfig/locales/views/enable_banking_items/en.ymlconfig/locales/views/enable_banking_items/fr.ymltest/models/enable_banking_item/importer_error_handling_test.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- config/locales/views/enable_banking_items/fr.yml
- config/locales/views/enable_banking_items/en.yml
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/models/enable_banking_item/importer_error_handling_test.rb (1)
17-17: UseOpenStructfor the mock provider to match test conventions.Line 17 uses
mock(). Repository guidance prefersOpenStructfor mock instances unless a dedicated mock class is needed.♻️ Suggested refactor
+require "ostruct" require "test_helper" @@ - `@mock_provider` = mock() + `@mock_provider` = OpenStruct.newAs per coding guidelines "Always prefer
OpenStructwhen creating mock instances, or in complex cases, a mock class".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/models/enable_banking_item/importer_error_handling_test.rb` at line 17, Replace the generic mock() with an OpenStruct instance for `@mock_provider` to follow test conventions: require 'ostruct' if not already required, change `@mock_provider` = mock() to `@mock_provider` = OpenStruct.new and set any expected attributes or methods on that OpenStruct used by the importer (e.g., status, id, name) so tests exercise real-like object behavior; update references in the test to use `@mock_provider`'s attributes rather than mock expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/models/enable_banking_item/importer_error_handling_test.rb`:
- Line 17: Replace the generic mock() with an OpenStruct instance for
`@mock_provider` to follow test conventions: require 'ostruct' if not already
required, change `@mock_provider` = mock() to `@mock_provider` = OpenStruct.new and
set any expected attributes or methods on that OpenStruct used by the importer
(e.g., status, id, name) so tests exercise real-like object behavior; update
references in the test to use `@mock_provider`'s attributes rather than mock
expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 384d27fc-4009-4a2b-955d-e61971c2e5c8
📒 Files selected for processing (2)
app/models/enable_banking_item/importer.rbtest/models/enable_banking_item/importer_error_handling_test.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- app/models/enable_banking_item/importer.rb
github-project-automation
bot
moved this from Todo
to Done
in Latest release train (v0.7.0) - April 2026
2 participants
Summary by CodeRabbit
Bug Fixes
Localization
Chores
Tests