fix(enable_banking): match bank list search against BIC, not just name#1874
fix(enable_banking): match bank list search against BIC, not just name#1874dripsmvcp wants to merge 4 commits into
Conversation
Bank-search filter on the Enable Banking bank-selection modal only indexed `aspsp[:name]`, so users searching by BIC code (e.g. `INGDDEFF`) got no results even when the bank was rendered in the list. Switch the per-item data attribute to a `name + BIC` haystack and read from it in the Stimulus controller, so either token matches. Refs we-promise#1814
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughView renders a combined ChangesBank search with BIC
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/javascript/controllers/bank_search_controller.js (1)
11-11: ⚡ Quick winConsider using
toLowerCase()instead oftoLocaleLowerCase().The
toLocaleLowerCase()method is locale-sensitive and can produce unexpected results in certain locales (e.g., Turkish locale lowercases 'I' to 'ı' instead of 'i'). Since bank names and BIC codes are not locale-specific data,toLowerCase()would provide more predictable, locale-independent behavior and better align with the intent of case-insensitive matching.🔄 Proposed change
- const haystack = (item.dataset.bankSearch ?? "").toLocaleLowerCase(); + const haystack = (item.dataset.bankSearch ?? "").toLowerCase();Also update line 7 for consistency:
- const query = this.inputTarget.value.toLocaleLowerCase().trim(); + const query = this.inputTarget.value.toLowerCase().trim();🤖 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 `@app/javascript/controllers/bank_search_controller.js` at line 11, Replace the locale-sensitive toLocaleLowerCase() call on the bank search string with toLowerCase() to ensure locale-independent, predictable case folding: update the expression that assigns haystack from item.dataset.bankSearch to use toLowerCase() instead of toLocaleLowerCase(); also make the similar change for the other search-normalization call referenced in the file (the comparable dataset/variable used on line 7) so both sides of the case-insensitive comparison use toLowerCase().
🤖 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.
Nitpick comments:
In `@app/javascript/controllers/bank_search_controller.js`:
- Line 11: Replace the locale-sensitive toLocaleLowerCase() call on the bank
search string with toLowerCase() to ensure locale-independent, predictable case
folding: update the expression that assigns haystack from
item.dataset.bankSearch to use toLowerCase() instead of toLocaleLowerCase();
also make the similar change for the other search-normalization call referenced
in the file (the comparable dataset/variable used on line 7) so both sides of
the case-insensitive comparison use toLowerCase().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 156cb804-e022-47dd-9651-1a109c45a975
📒 Files selected for processing (3)
app/javascript/controllers/bank_search_controller.jsapp/views/enable_banking_items/select_bank.html.erbtest/controllers/enable_banking_items_controller_test.rb
|
@jjmata @dripsmvcp - a few additional thoughts:
|
|
Clean, well-scoped fix. Renaming The test is good — it exercises the rendered HTML attribute content directly, which is the right level for this kind of view+controller integration. One note on the scope clarification in the PR description: the comment about ING Germany being absent from the upstream list entirely (rather than just not searchable) is a useful distinction. Good to document that this PR addresses the UX gap (BIC search returns no results) without claiming to fix the upstream availability issue. That context will be useful for anyone following up on #1814. Generated by Claude Code |
|
Thanks @jjmata for the review |
Summary
aspsp[:name], so users searching by BIC (e.g.INGDDEFF) got an empty result even when the bank was visibly rendered in the list. This change extends the search haystack to include the BIC alongside the name.Root Cause
app/views/enable_banking_items/select_bank.html.erb:31rendered each list row withdata-bank-name="<%= aspsp[:name].downcase(:fold) %>", and the Stimulus controller atapp/javascript/controllers/bank_search_controller.js:11substring-matched the user's query against that single field. The BIC was shown in the row's body but was never part of what the filter looked at.Fix
data-bank-search="<name> <bic>"(case-folded, blanks dropped) on each item.dataset.bankSearch.data-bank-namehad exactly one consumer (this controller), renamed in lockstep.Scope note
Issue #1814 describes ING Germany as missing from the list entirely. The Sure code path does not filter ASPSPs —
provider.get_aspsps(country:)results are passed through unmodified — so a bank being absent reflects what Enable Banking's/aspsps?country=DEreturned for that application, not an app-side allowlist. This PR addresses one concrete UX gap the report calls out (BIC search returning no results) without claiming to make a truly-absent bank appear; the latter is an Enable Banking dashboard / upstream concern.Test plan
New regression test:
test/controllers/enable_banking_items_controller_test.rb— mocksProvider::EnableBanking#get_aspspsto return ING-DiBa with BICINGDDEFF, asserts the rendered list row'sdata-bank-searchattribute contains bothingddeffanding-diba ag.Test fails before the fix, passes after:
bin/rubocop -f github -aclean on the new Ruby test filebundle exec erb_lint ./app/views/enable_banking_items/select_bank.html.erb -acleannpm run lint(biome) clean on the changed Stimulus controllerbin/brakeman --no-pager— 0 security warningsManual UI smoke: open the Enable Banking bank-selection modal, type a BIC substring, confirm the matching bank stays visible
Refs #1814
Summary by CodeRabbit