Initial enable banking implementation #382
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds an "Enable Banking" integration: new AR models, import/process/sync pipeline, provider HTTP client and adapter, controller/routes for OAuth and account setup, views/partials, a DB migration, and small updates to Accounts and Settings controllers/views. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant Controller as EnableBankingItemsController
participant ProviderClient as Provider::EnableBanking
participant EnableAPI as Enable Banking API
participant Item as EnableBankingItem
participant Syncer as EnableBankingItem::Syncer
participant Importer as EnableBankingItem::Importer
Note over User,Browser: User starts bank connect
Browser->>Controller: POST /enable_banking_items/:id/authorize (aspsp_name)
Controller->>ProviderClient: start_authorization(aspsp_name, redirect_url, state)
ProviderClient->>EnableAPI: request auth
EnableAPI-->>ProviderClient: auth redirect
ProviderClient-->>Controller: redirect_url
Controller-->>Browser: redirect user to bank
Note over EnableAPI,Controller: Bank redirects back with code
Browser->>Controller: GET /enable_banking_items/callback?code=...
Controller->>ProviderClient: create_session(code)
ProviderClient->>EnableAPI: exchange code
EnableAPI-->>ProviderClient: session payload
ProviderClient-->>Controller: session created
Controller->>Item: complete_authorization(code/session)
Controller->>Syncer: trigger perform_sync
Syncer->>ProviderClient: get_session / get_account_balances / get_account_transactions
ProviderClient->>EnableAPI: fetch data
EnableAPI-->>ProviderClient: payloads
Syncer->>Importer: import(session_payload)
Importer->>Importer: upsert accounts -> spawn processors -> import transactions
Importer-->>Syncer: import result
Syncer-->>Controller: sync complete
Controller-->>Browser: update UI (Turbo/TurboStream)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 13
🧹 Nitpick comments (22)
app/models/enable_banking_account/transactions/processor.rb (2)
28-30: Consider distinguishing "skipped" from "failed" for nil results.Per the
EnableBankingEntry::Processor(lines 18-22), returningnilindicates the transaction was skipped because there's no linked account—an expected condition, not a failure. Counting it asfailedwith error message "No linked account" may inflate failure metrics.Consider tracking a separate
skipped_countor only incrementingfailed_countfor actual exceptions:if result.nil? - failed_count += 1 - errors << { index: index, transaction_id: transaction_data[:transaction_id], error: "No linked account" } + # Transaction skipped - no linked account (expected for unlinked accounts) + Rails.logger.debug "EnableBankingAccount::Transactions::Processor - Skipped transaction #{transaction_data[:transaction_id]} (no linked account)" else imported_count += 1 end
36-36: Consider usingwith_indifferent_accessfor cleaner key access.The transaction data access pattern is verbose. If
raw_transactions_payloadstores hashes, converting once would simplify:- transaction_id = transaction_data.try(:[], :transaction_id) || transaction_data.try(:[], "transaction_id") || "unknown" + transaction_id = transaction_data.with_indifferent_access[:transaction_id] || "unknown"This aligns with the pattern in
EnableBankingEntry::Processor(line 60).app/models/enable_banking_entry/processor.rb (2)
76-80: Hardcoded strings should use i18n.Per coding guidelines, user-facing strings must use localization. These fallback names may appear in the UI:
counterparty = if amount_value.negative? - creditor.presence || "Unknown Recipient" + creditor.presence || I18n.t("enable_banking.transaction.unknown_recipient") else - debtor.presence || "Unknown Sender" + debtor.presence || I18n.t("enable_banking.transaction.unknown_sender") end
37-39: Consider preserving the original exception cause.Wrapping exceptions loses the original backtrace. While the error is logged beforehand, consider using
raise StandardError, "...", e.backtraceor Ruby 2.1+ cause chaining for better debugging:rescue ActiveRecord::RecordInvalid, ActiveRecord::RecordNotSaved => e Rails.logger.error "EnableBankingEntry::Processor - Failed to save transaction #{external_id}: #{e.message}" - raise StandardError.new("Failed to import transaction: #{e.message}") + raise StandardError, "Failed to import transaction: #{e.message}", cause: eapp/views/settings/providers/show.html.erb (1)
30-34: Consider adding collapsible options for consistency.The Enable Banking section is missing
collapsible: true, open: falseoptions that are present in the SimpleFIN and Lunch Flow sections (lines 17, 23). Additionally, consider using "Enable Banking" (two words with space) for the title to match the "Lunch Flow" and "SimpleFIN" naming style.Apply this diff for consistency:
-<%= settings_section title: "EnableBanking" do %> +<%= settings_section title: "Enable Banking", collapsible: true, open: false do %> <turbo-frame id="enable_banking-providers-panel"> <%= render "settings/providers/enable_banking_panel" %> </turbo-frame> <% end %>app/views/enable_banking_items/setup_accounts.html.erb (1)
10-19: Clarify form submission behavior withlocal: trueandturbo_frame.The form has
local: true(disables Turbo for the form) but also specifiesturbo_frame: "_top". This combination may be redundant sincelocal: truealready bypasses Turbo Drive. If you want a full page redirect after submission,local: truealone suffices. If you want Turbo to handle it but target the top frame, removelocal: true.If the intent is a full page navigation after form submission:
- local: true, data: { controller: "loading-button", action: "submit->loading-button#showLoading", loading_button_loading_text_value: "Creating Accounts...", turbo_frame: "_top" },Or if you truly want to bypass Turbo entirely, remove the
turbo_framedata attribute.app/models/enable_banking_item/syncer.rb (2)
8-18: Duplicate session validation.Session validity is checked here (line 10) and again inside
import_latest_enable_banking_data(see relevant code snippet lines 104-107). While defensive, this duplication means the session is validated twice. Consider documenting that this is intentional for early-fail behavior, or removing one check.If you want to keep both checks, add a comment explaining the early-fail strategy:
def perform_sync(sync) - # Check if session is valid before syncing + # Early-fail check: validate session before any work. + # Note: import_latest_enable_banking_data also validates, but we check here + # to provide immediate feedback and avoid unnecessary API setup. unless enable_banking_item.session_valid?
24-24: Simplify chained joins.The query chains
.joins(:account_provider).joins(:account)which creates two separate INNER JOINs. This is functionally correct but could be simplified for readability.-linked_accounts = enable_banking_item.enable_banking_accounts.joins(:account_provider).joins(:account).merge(Account.visible) +linked_accounts = enable_banking_item.enable_banking_accounts + .joins(:account_provider, :account) + .merge(Account.visible)app/models/provider/enable_banking_adapter.rb (1)
40-59: Consider extracting repeated metadata retrieval pattern.The
institution_domain,institution_name, andinstitution_urlmethods all repeat the same pattern of fetching and checkingprovider_account.institution_metadata. Consider extracting to a memoized helper method.+ def institution_metadata_hash + @institution_metadata_hash ||= provider_account.institution_metadata + end + def institution_domain - metadata = provider_account.institution_metadata - return nil unless metadata.present? - - metadata["domain"] + return nil unless institution_metadata_hash.present? + institution_metadata_hash["domain"] end def institution_name - metadata = provider_account.institution_metadata - return nil unless metadata.present? - - metadata["name"] || metadata["aspsp_name"] || item&.aspsp_name + return nil unless institution_metadata_hash.present? + institution_metadata_hash["name"] || institution_metadata_hash["aspsp_name"] || item&.aspsp_name end def institution_url - metadata = provider_account.institution_metadata - return nil unless metadata.present? - - metadata["url"] || item&.institution_url + return nil unless institution_metadata_hash.present? + institution_metadata_hash["url"] || item&.institution_url endapp/models/enable_banking_account/processor.rb (1)
32-36: Redundant nil check for current_account.The
current_account.blank?check on line 33 is redundant since theprocessmethod already returns early ifcurrent_accountis not present (lines 11-14). This dead code path will never execute.def process_account! - if enable_banking_account.current_account.blank? - Rails.logger.error("Enable Banking account #{enable_banking_account.id} has no associated Account") - return - end - account = enable_banking_account.current_account balance = enable_banking_account.current_balance || 0app/views/settings/providers/_enable_banking_panel.html.erb (2)
87-90: Use functional design tokens instead of raw color values.Per coding guidelines, prefer functional design system tokens (e.g.,
bg-primary,text-on-primary) over raw Tailwind colors likebg-gray-900,hover:bg-gray-800,bg-warning. Referencemaybe-design-system.cssfor available tokens.Also applies to: 124-142
39-71: Consider extracting country options to a constant or helper.The hardcoded country list could be moved to a model constant (e.g.,
EnableBankingItem::SUPPORTED_COUNTRIES) or a helper method for better maintainability and reusability.app/controllers/enable_banking_items_controller.rb (1)
305-324: Consider wrapping account creation loop in a transaction.The loop creates multiple accounts and AccountProviders. If one fails partway through, the system could be left in an inconsistent state with some accounts created and others not. Consider wrapping in a transaction for atomicity.
+ ActiveRecord::Base.transaction do account_types.each do |enable_banking_account_id, selected_type| enable_banking_account = @enable_banking_item.enable_banking_accounts.find(enable_banking_account_id) # ... rest of loop end + endapp/models/provider/enable_banking.rb (2)
175-193: JWT generated on every request - consider caching for performance.A new JWT is generated for every API call via
auth_headers. While the 1-hour expiry (line 188) is reasonable, generating JWTs is computationally expensive due to RSA signing. For high-frequency sync operations, consider caching the JWT until close to expiry.def generate_jwt @jwt_cache ||= {} now = Time.current.to_i # Return cached JWT if it has more than 5 minutes remaining if @jwt_cache[:token] && @jwt_cache[:exp] > now + 300 return @jwt_cache[:token] end exp = now + 3600 # ... generate new JWT ... @jwt_cache = { token: token, exp: exp } token end
209-221: Consider sanitizing response body in error messages.Error messages include the raw response body (e.g., line 209, 217, 221). If the API returns sensitive information in error responses, this could be logged or exposed. Consider truncating or sanitizing the body before including in exceptions.
+ def sanitize_response_body(body, max_length: 500) + return "" if body.blank? + body.to_s.truncate(max_length) + end + when 400 - raise EnableBankingError.new("Bad request to Enable Banking API: #{response.body}", :bad_request) + raise EnableBankingError.new("Bad request to Enable Banking API: #{sanitize_response_body(response.body)}", :bad_request)app/models/enable_banking_account.rb (3)
7-9: Redundant association definition.Both
accountandlinked_accountare defined with identicalthrough: :account_provider, source: :accountconfiguration. Consider removinglinked_accountif it serves the same purpose, or document why both are needed.# New association through account_providers has_one :account_provider, as: :provider, dependent: :destroy has_one :account, through: :account_provider, source: :account - has_one :linked_account, through: :account_provider, source: :account
26-34: Extract duplicate account_id_data parsing logic.The logic for extracting
account_id_datafromraw_account_id(lines 26-34) is duplicated inbuild_account_name(lines 66-73). Extract this into a private helper method.+ def extract_account_id_data(snapshot) + raw_account_id = snapshot[:account_id] + if raw_account_id.is_a?(Hash) + raw_account_id + elsif raw_account_id.is_a?(Array) && raw_account_id.first.is_a?(Hash) + raw_account_id.find { |item| item[:iban].present? } || {} + else + {} + end + endThen use
account_id_data = extract_account_id_data(snapshot)in bothupsert_enable_banking_snapshot!andbuild_account_name.
54-60: Inconsistent persistence pattern.This method uses
assign_attributes+save!whileupsert_enable_banking_snapshot!usesupdate!. Consider using consistent patterns across similar methods for maintainability.def upsert_enable_banking_transactions_snapshot!(transactions_snapshot) - assign_attributes( - raw_transactions_payload: transactions_snapshot - ) - - save! + update!(raw_transactions_payload: transactions_snapshot) endapp/models/enable_banking_item/importer.rb (2)
109-112: Consider logging exception class in broad rescue.The broad
rescue => eon line 109 could catch unexpected errors. Includinge.classin the log message (like line 110 does for transactions) would improve debugging.rescue => e - Rails.logger.error "EnableBankingItem::Importer - Unexpected error fetching session: #{e.class} - #{e.message}" + Rails.logger.error "EnableBankingItem::Importer - Unexpected error fetching session: #{e.class} - #{e.message}\n#{e.backtrace&.first(5)&.join("\n")}" nil end
114-122: Redundantsave!call afterupsert_enable_banking_snapshot!.The
upsert_enable_banking_snapshot!method (in EnableBankingAccount) already callsupdate!which persists the record. The subsequentsave!on line 121 is redundant.def import_account(account_data) uid = account_data[:uid] enable_banking_account = enable_banking_item.enable_banking_accounts.find_by(uid: uid.to_s) return unless enable_banking_account enable_banking_account.upsert_enable_banking_snapshot!(account_data) - enable_banking_account.save! endapp/models/enable_banking_item.rb (2)
169-199: Consider extracting status message generation.The status message logic (lines 179-185 and 191-197) is duplicated. Consider extracting to a private helper for DRY.
+ private + + def format_sync_status(total, linked, unlinked) + if total == 0 + "No accounts found" + elsif unlinked == 0 + "#{linked} #{'account'.pluralize(linked)} synced" + else + "#{linked} synced, #{unlinked} need setup" + end + end
246-257: UseTime.zone.parsefor timezone-aware parsing.
Time.parsereturns a local time. UsingTime.zone.parseensures consistency with Rails' timezone handling.def parse_session_expiry(session_result) # Enable Banking sessions typically last 90 days # The exact expiry depends on the ASPSP consent if session_result[:access].present? && session_result[:access][:valid_until].present? - Time.parse(session_result[:access][:valid_until]) + Time.zone.parse(session_result[:access][:valid_until]) else 90.days.from_now end
| state: @enable_banking_item.id | ||
| ) | ||
|
|
||
| redirect_to redirect_url, allow_other_host: true |
There was a problem hiding this comment.
Address Brakeman warning: Possible unprotected redirect.
The redirect_to redirect_url, allow_other_host: true on lines 124 and 185 triggers Brakeman's unprotected redirect warning. The redirect_url comes from the Enable Banking API response. While this is typical for OAuth flows, consider validating that the URL matches expected Enable Banking domains before redirecting.
+ ALLOWED_REDIRECT_HOSTS = ["enablebanking.com"].freeze
+
+ def validate_redirect_url!(url)
+ uri = URI.parse(url)
+ unless ALLOWED_REDIRECT_HOSTS.any? { |host| uri.host&.end_with?(host) }
+ raise Provider::EnableBanking::EnableBankingError.new("Invalid redirect URL", :invalid_redirect)
+ end
+ url
+ end
# In authorize action:
- redirect_to redirect_url, allow_other_host: true
+ redirect_to validate_redirect_url!(redirect_url), allow_other_host: trueAlso applies to: 185-185
🧰 Tools
🪛 GitHub Actions: Pull Request
[warning] 124-124: Brakeman warning: Possible unprotected redirect. (redirect_to) in the application.
🤖 Prompt for AI Agents
In app/controllers/enable_banking_items_controller.rb around lines 124 and 185,
the unguarded redirect_to using redirect_url (from the Enable Banking API)
triggers a Brakeman possible unprotected redirect warning; validate the
redirect_url before calling redirect_to by parsing it into a URI and confirming
its scheme is http or https and its host matches a whitelist of expected Enable
Banking domains (e.g., enable-banking domains you trust); only call redirect_to
redirect_url, allow_other_host: true when validation passes, otherwise redirect
to a safe internal fallback or render an error, and optionally log the invalid
URL for investigation.
| enable_banking_item = Current.family.enable_banking_items.where.not(session_id: nil).first | ||
|
|
||
| unless enable_banking_item.present? | ||
| redirect_to settings_providers_path, alert: t(".no_session", default: "No active Enable Banking connection. Please connect a bank first.") | ||
| return | ||
| end |
There was a problem hiding this comment.
Non-deterministic query when multiple Enable Banking items exist.
Using .first without ordering on line 202 could return different items if multiple items with active sessions exist. Consider making this deterministic with .order(:created_at).first or using @enable_banking_item from params instead.
- enable_banking_item = Current.family.enable_banking_items.where.not(session_id: nil).first
+ enable_banking_item = Current.family.enable_banking_items.where.not(session_id: nil).order(:created_at).first📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| enable_banking_item = Current.family.enable_banking_items.where.not(session_id: nil).first | |
| unless enable_banking_item.present? | |
| redirect_to settings_providers_path, alert: t(".no_session", default: "No active Enable Banking connection. Please connect a bank first.") | |
| return | |
| end | |
| enable_banking_item = Current.family.enable_banking_items.where.not(session_id: nil).order(:created_at).first | |
| unless enable_banking_item.present? | |
| redirect_to settings_providers_path, alert: t(".no_session", default: "No active Enable Banking connection. Please connect a bank first.") | |
| return | |
| end |
🤖 Prompt for AI Agents
In app/controllers/enable_banking_items_controller.rb around lines 202 to 207,
the query uses .first on
Current.family.enable_banking_items.where.not(session_id: nil) which is
non-deterministic when multiple active sessions exist; make it deterministic by
ordering (e.g. .order(:created_at).first or .order(created_at: :desc).first
depending on desired semantics) or by selecting the specific item from params
(e.g. use params[:id] to find the exact enable_banking_item) and then proceed
with the present? check and redirect logic accordingly.
| def log_invalid_currency(currency_value) | ||
| Rails.logger.warn("Invalid currency code '#{currency_value}' in Enable Banking transaction #{external_id}, falling back to account currency") | ||
| end |
There was a problem hiding this comment.
Unused method log_invalid_currency.
This method is defined but never called. Either remove it or integrate it with the currency method to log when an invalid currency falls back to account currency or EUR.
def currency
tx_amount = data[:transaction_amount] || {}
- parse_currency(tx_amount[:currency]) || parse_currency(data[:currency]) || account&.currency || "EUR"
+ parsed = parse_currency(tx_amount[:currency]) || parse_currency(data[:currency])
+ if parsed
+ parsed
+ else
+ log_invalid_currency(tx_amount[:currency] || data[:currency])
+ account&.currency || "EUR"
+ end
endCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/models/enable_banking_entry/processor.rb around lines 146–148, the helper
method log_invalid_currency is defined but never used; either remove it or call
it from the currency resolution logic so an invalid currency code is logged when
you fall back to the account currency or EUR. Update the currency method to
detect an invalid currency_value, invoke log_invalid_currency(currency_value) at
that point (or inline the same Rails.logger.warn message), and ensure the log
contains the invalid value and external_id; if you choose to remove the method,
delete the unused definition and add a one-line warning in the currency fallback
branch instead.
| begin | ||
| # For string UIDs, we don't have account data to update - skip the import_account call | ||
| # The account data will be fetched via balances/transactions endpoints | ||
| if account_data.is_a?(Hash) | ||
| import_account(account_data) | ||
| end | ||
| accounts_updated += 1 | ||
| rescue => e | ||
| accounts_failed += 1 | ||
| Rails.logger.error "EnableBankingItem::Importer - Failed to update account #{uid}: #{e.message}" | ||
| end | ||
| end |
There was a problem hiding this comment.
Incorrect counter increment for string UIDs.
When account_data is a string (line 41-42), import_account is skipped but accounts_updated is still incremented (line 60). This inflates the success count for accounts that weren't actually updated.
begin
# For string UIDs, we don't have account data to update - skip the import_account call
# The account data will be fetched via balances/transactions endpoints
if account_data.is_a?(Hash)
import_account(account_data)
+ accounts_updated += 1
end
- accounts_updated += 1
rescue => e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| begin | |
| # For string UIDs, we don't have account data to update - skip the import_account call | |
| # The account data will be fetched via balances/transactions endpoints | |
| if account_data.is_a?(Hash) | |
| import_account(account_data) | |
| end | |
| accounts_updated += 1 | |
| rescue => e | |
| accounts_failed += 1 | |
| Rails.logger.error "EnableBankingItem::Importer - Failed to update account #{uid}: #{e.message}" | |
| end | |
| end | |
| begin | |
| # For string UIDs, we don't have account data to update - skip the import_account call | |
| # The account data will be fetched via balances/transactions endpoints | |
| if account_data.is_a?(Hash) | |
| import_account(account_data) | |
| accounts_updated += 1 | |
| end | |
| rescue => e | |
| accounts_failed += 1 | |
| Rails.logger.error "EnableBankingItem::Importer - Failed to update account #{uid}: #{e.message}" | |
| end |
🤖 Prompt for AI Agents
In app/models/enable_banking_item/importer.rb around lines 54–65, the code
unconditionally increments accounts_updated even when account_data is a string
and import_account is skipped; move the accounts_updated += 1 so it only runs
after a successful import_account call (e.g., place the increment inside the if
account_data.is_a?(Hash) block immediately after import_account), keep the
rescue behaviour to increment accounts_failed and log the error, and ensure you
don't count skipped string UIDs as updated.
| # Paginate through all transactions | ||
| loop do | ||
| transactions_data = enable_banking_provider.get_account_transactions( | ||
| account_id: enable_banking_account.uid, | ||
| date_from: start_date, | ||
| continuation_key: continuation_key | ||
| ) | ||
|
|
||
| transactions = transactions_data[:transactions] || [] | ||
| all_transactions.concat(transactions) | ||
|
|
||
| continuation_key = transactions_data[:continuation_key] | ||
| break if continuation_key.blank? | ||
| end |
There was a problem hiding this comment.
Add pagination safety limit to prevent infinite loops.
The pagination loop relies on continuation_key being blank to exit. If the API misbehaves and returns the same key repeatedly, this could loop indefinitely. Consider adding a maximum iteration safeguard.
all_transactions = []
continuation_key = nil
+ max_pages = 100 # Safety limit
+ page_count = 0
# Paginate through all transactions
loop do
+ page_count += 1
+ if page_count > max_pages
+ Rails.logger.warn "EnableBankingItem::Importer - Pagination limit reached for account #{enable_banking_account.uid}"
+ break
+ end
+
transactions_data = enable_banking_provider.get_account_transactions(
account_id: enable_banking_account.uid,
date_from: start_date,
continuation_key: continuation_key
)🤖 Prompt for AI Agents
In app/models/enable_banking_item/importer.rb around lines 157 to 170, the
pagination loop can infinite-loop if the provider returns the same
continuation_key repeatedly; add a max-iteration safeguard (e.g. MAX_PAGES = 100
or configurable) and a counter initialized before the loop, increment it each
iteration, and break/raise/log a clear error when the counter exceeds the max to
stop the loop; also consider logging the last continuation_key and response for
debugging when aborting.
| (enable_banking_account.name.downcase.include?("checking") ? "checking" : | ||
| enable_banking_account.name.downcase.include?("savings") ? "savings" : "") : "" %> | ||
| <%= select_tag "account_subtypes[#{enable_banking_account.id}]", | ||
| options_for_select([["Select #{account_type == 'Depository' ? 'subtype' : 'type'}", ""]] + subtype_config[:options], selected_value), |
There was a problem hiding this comment.
Use i18n for user-facing dropdown text.
The hardcoded "Select subtype/type" text should use the t() helper for proper localization support, per the coding guideline: "All user-facing strings must use localization (i18n) via the t() helper."
Apply this diff:
<%= select_tag "account_subtypes[#{enable_banking_account.id}]",
- options_for_select([["Select #{account_type == 'Depository' ? 'subtype' : 'type'}", ""]] + subtype_config[:options], selected_value),
+ options_for_select([[t(".select_option", type: (account_type == 'Depository' ? t(".subtype") : t(".type"))), ""]] + subtype_config[:options], selected_value),
{ class: "appearance-none bg-container border border-primary rounded-md px-3 py-2 text-sm leading-6 text-primary focus:border-primary focus:ring-1 focus:ring-primary focus:outline-none w-full" } %>Add to locale file:
en:
enable_banking_items:
subtype_select:
select_option: "Select %{type}"
subtype: "subtype"
type: "type"🤖 Prompt for AI Agents
In app/views/enable_banking_items/_subtype_select.html.erb around line 9, the
dropdown label uses a hardcoded "Select subtype/type" string; replace it with
the i18n t() helper using the provided keys
(enable_banking_items.subtype_select.select_option and
enable_banking_items.subtype_select.subtype or .type) and interpolate the
correct type label (based on account_type == 'Depository' ?
t('enable_banking_items.subtype_select.subtype') :
t('enable_banking_items.subtype_select.type')) so options_for_select uses t(...)
instead of the literal string, and add the given locale entries to the en locale
file.
| <% if aspsp[:logo].present? %> | ||
| <img src="<%= aspsp[:logo] %>" alt="<%= aspsp[:name] %>" class="w-10 h-10 rounded object-contain"> | ||
| <% else %> |
There was a problem hiding this comment.
🧩 Analysis chain
Script executed:
#!/bin/bash
# Find the controller handling select_bank action
fd -t f "enable_banking_items_controller" --type rb
# Search for @aspsps assignment
rg -n "@aspsps\s*=" --type=rb
# Search for select_bank action
rg -n "def select_bank" --type=rbRepository: we-promise/sure
<shell_result>
Repository: we-promise/sure
Script execution failed with unrecoverable error: [CAPABILITY:RUN_SHELL_SCRIPT] Failed to clone repository we-promise/sure: Clone operation failed
</shell_result>
Enable Banking API logo URL source documentation
<web_search_result>
Do you mean the Enable (enablebanking.com) API, or a different "Enable Banking" product? Which documentation or endpoint (logo_url field, bank list, or app branding) do you need the logo URL source for?
</web_search_result>
Rails ERB img src attribute XSS security sanitization
<web_search_result>
Short answer
- Rails' sanitize helpers strip dangerous attributes (e.g., onerror) but will allow an img tag and its src value unless you restrict it. The sanitizer also removes href/src values using clearly unsafe protocols like javascript:. [1][2][3]
Risks to watch for
- src can still be abused (data:, base64-encoded HTML, or unusual schemes) and historically rails-html-sanitizer had protocol-related issues and CVEs when configurations or allowed-tags were changed; rails-ujs has also had DOM XSS issues (contenteditable + data-* attributes). Upgrade and be cautious. [3][4]
Practical protections (recommended)
- Do not use raw/html_safe with user-supplied HTML. Escape by default. [1][5]
- Sanitize user HTML when you must render it: sanitize(user_html) and explicitly pass tags/attributes if you need them. [1]
- Validate or whitelist img src URLs server-side before rendering. Example approach:
- Parse and check scheme: allow only http, https (and optionally data:image for images) using URI.parse and permit-list the scheme.
- Reject or rewrite values with scheme == "javascript" or unexpected data payloads.
Example (illustrative):
uri = URI.parse(src) rescue nil
allowed = %w(http https)
safe = uri && allowed.include?(uri.scheme)
- If you must allow rich pasted HTML or contenteditable areas, upgrade Rails to fixed versions and/or avoid enabling rails-ujs behavior on contenteditable elements. [4]
- Use a CSP (Content Security Policy) to mitigate impact of injected scripts.
References
- Rails sanitize docs (Action View Helpers — sanitize). [1]
- ActionView sanitize helper comment showing protocol stripping for href/src. [3]
- rails-html-sanitizer / protocol-related CVEs and guidance. [2]
- rails-ujs DOM XSS advisory (CVE-2023-23913) and upgrade/workaround recommendation. [4]
If you want, I can: (A) give a tiny helper method you can drop into a Rails app to validate/whitelist img src values, or (B) scan a code example to point out XSS risks.
</web_search_result>
Based on my research, I can now provide a more informed assessment:
Key Findings:
- Enable Banking API (enablebanking.com) is a legitimate, trusted banking data aggregation service
- Rails img src security: Rails ERB does NOT automatically sanitize
srcattributes by default. While Rails' sanitizer removesjavascript:protocol, it allows standard URLs including potentially maliciousdata:or base64-encoded URLs - Best practice: Validate/whitelist image URLs server-side before rendering
Context for this review:
- The
aspsp[:logo]data comes from the Enable Banking API (trusted source), NOT user input - While the URL is rendered without explicit sanitization, the lower risk comes from the trusted API source
- However, the code doesn't follow Rails security best practices for URL validation
Here is the rewritten review comment:
Consider validating Enable Banking API logo URLs before rendering.
The bank logo URL from aspsp[:logo] is rendered directly in the src attribute without validation. While the data comes from the trusted Enable Banking API (not user input), Rails security best practices recommend validating or whitelisting image URLs server-side before rendering them. This mitigates risk against potential API response tampering or accidental malformed URLs.
Consider adding a helper method to validate the URL scheme (allowing only http and https) before rendering:
def safe_image_url(url)
uri = URI.parse(url) rescue nil
return url if uri && %w(http https).include?(uri.scheme)
nil
endThen use <img src="<%= safe_image_url(aspsp[:logo]) %>" ...> in the template.
🤖 Prompt for AI Agents
In app/views/enable_banking_items/select_bank.html.erb around lines 25–27, the
template renders aspsp[:logo] directly into the img src without validating the
URL; add server-side validation/whitelisting for the logo URL (allow only http
and https schemes), implement a helper that parses the URL and returns it only
if it has an allowed scheme (otherwise return nil or a safe placeholder), and
update the template to use that helper so the img tag is only rendered with a
validated URL or a fallback.
| <% dialog.with_header(title: "Set Up Your Enable Banking Accounts") do %> | ||
| <div class="flex items-center gap-2"> | ||
| <%= icon "building-2", class: "text-primary" %> | ||
| <span class="text-primary">Choose the correct account types for your imported accounts</span> | ||
| </div> | ||
| <% end %> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Hardcoded strings should use i18n.
Multiple user-facing strings in this view are hardcoded in English. As per coding guidelines, all user-facing strings must use localization.
Key strings needing i18n:
- Dialog title: "Set Up Your Enable Banking Accounts"
- Info header: "Choose the correct account type..."
- Account type descriptions (lines 30-34)
- Field labels and help text
- Button text: "Create Accounts", "Cancel"
Example:
<% dialog.with_header(title: t(".title")) do %>| <div class="prose prose-sm text-secondary"> | ||
| <p class="text-primary font-medium">Setup instructions:</p> | ||
| <ol> | ||
| <li>Visit your <a href="https://enablebanking.com" target="_blank" rel="noopener noreferrer" class="link">Enable Banking</a> developer account to get your credentials</li> | ||
| <li>Select your country code from the dropdown below</li> | ||
| <li>Enter your Application ID and paste your Client Certificate (including the private key)</li> | ||
| <li>Click Save Configuration, then use "Add Connection" to link your bank</li> | ||
| </ol> | ||
|
|
||
| <p class="text-primary font-medium">Field descriptions:</p> | ||
| <ul> | ||
| <li><strong>Country Code:</strong> ISO 3166-1 alpha-2 country code (e.g., GB, DE, FR) - determines available banks</li> | ||
| <li><strong>Application ID:</strong> The ID generated in your Enable Banking developer account</li> | ||
| <li><strong>Client Certificate:</strong> The certificate generated when you created your application (must include the private key)</li> | ||
| </ul> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
User-facing strings should use i18n localization.
Per coding guidelines, all user-facing strings must use the t() helper. The setup instructions and field descriptions are hardcoded in English.
Consider extracting these strings to locale files:
<p class="text-primary font-medium"><%= t(".setup_instructions") %></p>
<ol>
<li><%= t(".instruction_step_1_html", link: link_to("Enable Banking", "https://enablebanking.com", target: "_blank", rel: "noopener noreferrer", class: "link")) %></li>
<li><%= t(".instruction_step_2") %></li>
<!-- etc. -->
</ol>| t.string :status, default: "good" | ||
| t.boolean :scheduled_for_deletion, default: false | ||
| t.boolean :pending_account_setup, default: false |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add null constraints to columns with defaults.
The status, scheduled_for_deletion, and pending_account_setup columns have default values but are missing null: false constraints. Per the coding guideline: "Use simple validations (null checks, unique indexes) in database layer," these should be non-nullable for data integrity.
Apply this diff:
- t.string :status, default: "good"
- t.boolean :scheduled_for_deletion, default: false
- t.boolean :pending_account_setup, default: false
+ t.string :status, default: "good", null: false
+ t.boolean :scheduled_for_deletion, default: false, null: false
+ t.boolean :pending_account_setup, default: false, null: falseBased on learnings, consider consistency across all similar integrations (Plaid, SimpleFin, Enable Banking) and propose comprehensive changes in a separate PR if needed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| t.string :status, default: "good" | |
| t.boolean :scheduled_for_deletion, default: false | |
| t.boolean :pending_account_setup, default: false | |
| t.string :status, default: "good", null: false | |
| t.boolean :scheduled_for_deletion, default: false, null: false | |
| t.boolean :pending_account_setup, default: false, null: false |
|
Added support for multiple connections. 
|
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (6)
app/views/enable_banking_items/_enable_banking_item.html.erb (2)
23-46: Hardcoded strings should use i18n.Multiple user-facing strings are hardcoded (e.g., "Deletion in progress", "Syncing...", "Re-authorization required", "Never synced"). Per coding guidelines, use the
t()helper for all user-facing strings.
89-92: Move query logic out of the view template.The
left_joins(:account_provider).where(account_providers: { id: nil }).countquery belongs in the model. Per coding guidelines: "Keep domain logic out of view templates."app/controllers/enable_banking_items_controller.rb (3)
124-124: Address Brakeman warning: Possible unprotected redirect.The redirect URL comes from the Enable Banking API. Validate that it matches expected Enable Banking domains before redirecting.
197-197: Address Brakeman warning for reauthorization redirect.Same unprotected redirect issue as Line 124.
214-214: Non-deterministic query when multiple Enable Banking items exist.Using
.firstwithout ordering could return different items. Add.order(:created_at).firstfor deterministic behavior.app/views/settings/providers/_enable_banking_panel.html.erb (1)
2-17: User-facing strings should use i18n localization.Setup instructions and field descriptions are hardcoded in English. Use the
t()helper per coding guidelines.
🧹 Nitpick comments (7)
app/controllers/enable_banking_items_controller.rb (3)
3-3: Consider using a cryptographic state token for CSRF mitigation.Skipping CSRF verification for the OAuth callback is necessary, but using
@enable_banking_item.idas the state (Line 121) is guessable. Consider using a cryptographically random token stored in the session or database to validate the callback origin.# In authorize action: - state: @enable_banking_item.id + state: SecureRandom.urlsafe_base64(32) # Store state in enable_banking_item.authorization_state
128-131: Consider logging backtrace for unexpected errors.The bare
rescue => ecatches allStandardErrorsubclasses, which is appropriate for user-facing error handling, but the log message omits the backtrace, making debugging harder.rescue => e - Rails.logger.error "Unexpected error in authorize: #{e.class}: #{e.message}" + Rails.logger.error "Unexpected error in authorize: #{e.class}: #{e.message}\n#{e.backtrace&.first(10)&.join("\n")}" redirect_to settings_providers_path, alert: t(".unexpected_error", default: "An unexpected error occurred. Please try again.")
271-277: Account type options should use i18n.These user-facing strings are hardcoded. Per coding guidelines, use the
t()helper.@account_type_options = [ [ t(".account_types.checking_or_savings"), "Depository" ], [ t(".account_types.credit_card"), "CreditCard" ], # ... ]app/views/settings/providers/_enable_banking_panel.html.erb (4)
100-101: Use design system tokens for button styling.Per coding guidelines, prefer functional design tokens instead of raw Tailwind color values like
bg-gray-900.- class: "inline-flex items-center justify-center rounded-lg px-4 py-2 text-sm font-medium text-white bg-gray-900 hover:bg-gray-800 focus:outline-none focus:ring-2 focus:ring-gray-900 focus:ring-offset-2 transition-colors" + class: "btn btn--primary"Or use the appropriate design system classes from
maybe-design-system.css.
121-136: Multiple hardcoded strings need i18n.Lines 121 ("Connected Bank"), 123 ("Unknown"), 129-130 ("Connection", "Session expired..."), 135-136 ("Configured", "Ready to connect...") should use the
t()helper.
143-169: Action button text should use i18n.Button labels "Sync", "Re-authorize", "Connect Bank", "Remove" on lines 147, 154, 160, 168 are hardcoded.
177-183: "Add Connection" button styling and text need updates.Use design system tokens instead of
bg-gray-900and localize "Add Connection" text.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/controllers/enable_banking_items_controller.rb(1 hunks)app/views/accounts/index.html.erb(2 hunks)app/views/enable_banking_items/_enable_banking_item.html.erb(1 hunks)app/views/settings/providers/_enable_banking_panel.html.erb(1 hunks)config/routes.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/views/accounts/index.html.erb
- config/routes.rb
🧰 Additional context used
📓 Path-based instructions (21)
{app/**/*.{erb,html},app/javascript/**/*.js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Prefer Hotwire-first approach: use Turbo + Stimulus for reactive UI without heavy JavaScript
Files:
app/views/enable_banking_items/_enable_banking_item.html.erbapp/views/settings/providers/_enable_banking_panel.html.erb
app/views/**/_*.html.erb
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use partials only for simple, static HTML with minimal logic in specific contexts
app/views/**/_*.html.erb: Use underscore prefix for partial files (e.g., _trend_change.html.erb, _form_errors.html.erb, _sync_indicator.html.erb)
Place context-specific partials in relevant controller view directory (e.g., accounts/_account_sidebar_tabs.html.erb)
Files:
app/views/enable_banking_items/_enable_banking_item.html.erbapp/views/settings/providers/_enable_banking_panel.html.erb
app/views/**/*.erb
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
app/views/**/*.erb: Always use theiconhelper for icons in views, never uselucide_icondirectly
Use Tailwind design tokens (e.g., text-primary, bg-container, border-secondary) instead of raw color names
Pass data to Stimulus controllers via data-*-value attributes instead of inline JavaScript
Prefer semantic HTML elements (dialog, details, summary) over JavaScript components
Use Turbo Frames for page sections instead of client-side solutions
Use server-side formatting for currencies, numbers, and dates instead of client-sideViews should use ERB and avoid heavy logic; prefer helpers and components instead (ERB checked by
erb-lintper.erb_lint.yml)
Files:
app/views/enable_banking_items/_enable_banking_item.html.erbapp/views/settings/providers/_enable_banking_panel.html.erb
**/*.{css,erb}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use Tailwind CSS v4.x with custom design system for styling
Files:
app/views/enable_banking_items/_enable_banking_item.html.erbapp/views/settings/providers/_enable_banking_panel.html.erb
**/*.{rb,erb,html.erb}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{rb,erb,html.erb}: UseCurrent.userfor the current user context in controllers and views. Do NOT usecurrent_user.
UseCurrent.familyfor the current family context. Do NOT usecurrent_family.
Files:
app/views/enable_banking_items/_enable_banking_item.html.erbapp/views/settings/providers/_enable_banking_panel.html.erbapp/controllers/enable_banking_items_controller.rb
**/*.{erb,html,vue,jsx,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{erb,html,vue,jsx,tsx}: Use Tailwind CSS v4.x with custom design system defined inapp/assets/tailwind/maybe-design-system.css. Always use functional tokens (e.g.,text-primarynottext-white).
Prefer semantic HTML elements over JavaScript components (e.g., use<dialog>for modals,<details><summary>for disclosures).
Files:
app/views/enable_banking_items/_enable_banking_item.html.erbapp/views/settings/providers/_enable_banking_panel.html.erb
**/*.{erb,html.erb}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{erb,html.erb}: Useiconhelper for icons, never uselucide_icondirectly.
Leverage Turbo frames for page sections over client-side solutions.
Pass data to Stimulus controllers viadata-*-valueattributes, not inline JavaScript.
**/*.{erb,html.erb}: Use ViewComponents when: element has complex logic or styling patterns, will be reused across multiple views/contexts, needs structured styling with variants/sizes, requires interactive behavior or Stimulus controllers, has configurable slots or complex APIs, or needs accessibility features or ARIA support
Use Partials when: element is primarily static HTML with minimal logic, used in only one or few specific contexts, is simple template content, doesn't need variants/sizes/complex configuration, or is more about content organization than reusable functionality
Prefer components over partials: if a component is available in app/components, use it; if not, look for a partial; if neither exists, decide between component or partial based on complexity and reusability criteria
Keep domain logic out of views: compute values like button classes, conditional logic, and data transformations in the component file, not the template file
Always use the declarative approach when integrating Stimulus controllers in views: the ERB template should declare what happens using data-* attributes, and the Stimulus controller should respond
Component controllers in app/components/ should only be used within their component templates; global controllers in app/javascript/controllers/ can be used across any view
Pass data from Rails to Stimulus using data-*-value attributes, not inline JavaScript
Files:
app/views/enable_banking_items/_enable_banking_item.html.erbapp/views/settings/providers/_enable_banking_panel.html.erb
**/*.{erb,html.erb,rb}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{erb,html.erb,rb}: All user-facing strings must use localization (i18n) via thet()helper. Update locale files for each new or changed element.
Use server-side formatting for currencies, numbers, and dates. Do not format in JavaScript.
Files:
app/views/enable_banking_items/_enable_banking_item.html.erbapp/views/settings/providers/_enable_banking_panel.html.erbapp/controllers/enable_banking_items_controller.rb
app/views/**/_*.{erb,html.erb}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Partials when element is primarily static HTML with minimal logic, used in only one or few contexts, is simple template content, or doesn't need variants/sizes/complex configuration.
Files:
app/views/enable_banking_items/_enable_banking_item.html.erbapp/views/settings/providers/_enable_banking_panel.html.erb
{app/components/**/*.html.erb,app/views/**/*.{erb,html.erb}}
📄 CodeRabbit inference engine (CLAUDE.md)
Keep domain logic out of view templates. Logic belongs in component files, not template files.
Files:
app/views/enable_banking_items/_enable_banking_item.html.erbapp/views/settings/providers/_enable_banking_panel.html.erb
**/*.erb
📄 CodeRabbit inference engine (CLAUDE.md)
Run
bundle exec erb_lint ./app/**/*.erb -afor ERB linting with auto-correct before pull requests.
Files:
app/views/enable_banking_items/_enable_banking_item.html.erbapp/views/settings/providers/_enable_banking_panel.html.erb
**/*.{rb,erb}
📄 CodeRabbit inference engine (.cursor/rules/general-rules.mdc)
**/*.{rb,erb}: UseCurrent.userfor accessing the current user. Do NOT usecurrent_user
UseCurrent.familyfor accessing the current family. Do NOT usecurrent_family
Files:
app/views/enable_banking_items/_enable_banking_item.html.erbapp/views/settings/providers/_enable_banking_panel.html.erbapp/controllers/enable_banking_items_controller.rb
**/*.html.erb
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
**/*.html.erb: Prefer native HTML elements over JavaScript-based components. Use semantic HTML: for modals,for disclosures
Leverage Turbo frames to break up pages instead of JavaScript-driven client-side solutions
Use theiconhelper from application_helper.rb for icons. Never uselucide_iconhelper directly
Files:
app/views/enable_banking_items/_enable_banking_item.html.erbapp/views/settings/providers/_enable_banking_panel.html.erb
**/*.{rb,js,erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Format currencies, numbers, dates, and other values server-side, then pass to Stimulus controllers for display only
Files:
app/views/enable_banking_items/_enable_banking_item.html.erbapp/views/settings/providers/_enable_banking_panel.html.erbapp/controllers/enable_banking_items_controller.rb
**/*.{rb,html.erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Use ActiveRecord validations for complex validations and business logic. Simple validations may be mirrored in ActiveRecord for form error handling convenience, but prioritize client-side form validation when possible
Files:
app/views/enable_banking_items/_enable_banking_item.html.erbapp/views/settings/providers/_enable_banking_panel.html.erbapp/controllers/enable_banking_items_controller.rb
{app/views/**,app/helpers/**,app/javascript/controllers/**}
📄 CodeRabbit inference engine (.cursor/rules/ui-ux-design-guidelines.mdc)
{app/views/**,app/helpers/**,app/javascript/controllers/**}: Reference maybe-design-system.css for base primitives, functional tokens, and component tokens before writing styles
Prefer using functional design system tokens (e.g., text-primary, bg-container, border-primary) from maybe-design-system.css instead of raw Tailwind color values
Files:
app/views/enable_banking_items/_enable_banking_item.html.erbapp/views/settings/providers/_enable_banking_panel.html.erb
{app/views/**,app/helpers/**}
📄 CodeRabbit inference engine (.cursor/rules/ui-ux-design-guidelines.mdc)
Always generate semantic HTML
Files:
app/views/enable_banking_items/_enable_banking_item.html.erbapp/views/settings/providers/_enable_banking_panel.html.erb
**/*.rb
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rb: Application supports two modes: 'managed' and 'self_hosted' via Rails.application.config.app_mode
Use Current.user and Current.family instead of current_user / current_family for authentication context
Optimize database queries with proper indexes to prevent N+1 queries using includes/joins
**/*.rb: Use Rails pluralization in i18n:t("transactions.count", count: @transactions.count).
Runbin/rubocop -f github -afor Ruby linting with auto-correct before pull requests.
**/*.rb: Ruby code should use 2-space indent,snake_casefor methods and variables, andCamelCasefor classes and modules, following Rails conventions for folders and file names
Prefer environment variables over hard-coded values for configuration
Files:
app/controllers/enable_banking_items_controller.rb
app/controllers/**/*.rb
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use Rails strong parameters and CSRF protection throughout the application
app/controllers/**/*.rb: Use strong parameters and CSRF protection throughout the application.
Use query params for state over localStorage/sessions.
Files:
app/controllers/enable_banking_items_controller.rb
app/**/*.{rb,js,ts,jsx,tsx,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Code should be organized in
app/directory (Rails MVC, services, jobs, mailers, components), with JS inapp/javascript/, and styles/assets inapp/assets/(Tailwind, images, fonts)
Files:
app/controllers/enable_banking_items_controller.rb
app/**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Run
bin/brakemansecurity scan before major PRs to check for static analysis of common Rails issues
Files:
app/controllers/enable_banking_items_controller.rb
🧠 Learnings (31)
📓 Common learnings
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: db/migrate/20250825224717_add_enable_banking_domain.rb:3-34
Timestamp: 2025-09-13T12:23:31.324Z
Learning: When suggesting database schema improvements for banking integrations, consider consistency across all similar integrations (Plaid, SimpleFin, Enable Banking) and propose comprehensive changes in a separate PR rather than making changes to individual integrations in isolation.
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account.rb:9-20
Timestamp: 2025-09-13T11:33:54.159Z
Learning: The EnableBankingAccount model uses a `has_balance` validation method that ensures at least one balance value (current_balance or available_balance) is present, similar to the Plaid integration pattern. This means balance values should not be nil and the validation expects actual balance data from the API.
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account.rb:9-20
Timestamp: 2025-09-13T11:33:54.159Z
Learning: The EnableBankingAccount model uses a `has_balance` validation method that ensures at least one balance value (current_balance or available_balance) is present, similar to the Plaid integration pattern. This means balance values should not be nil and the validation expects actual balance data from the API.
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:55:43.046Z
Learning: Use Plaid integration for real-time bank account syncing via `PlaidItem`, `Sync` models, and background jobs for data updates.
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method in EnableBankingAccount::Processor follows the same pattern as the Plaid integration, using OpenStruct without explicit requires and returning balance values as-is without BigDecimal coercion for consistency across banking integrations.
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/controllers/enable_banking_items_controller.rb:48-62
Timestamp: 2025-09-13T11:27:55.271Z
Learning: The `create_enable_banking_item!` method in `Family::EnableBankingConnectable` uses `find_or_create_by(id: enable_banking_id)` to either find an existing EnableBankingItem or create a new one, so it automatically handles updates to existing items without creating duplicates.
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method pattern is consistent across banking integrations in the codebase. Both PlaidAccount::Processor and EnableBankingAccount::Processor use the same approach: `balance = account.current_balance || account.available_balance || 0` followed by `OpenStruct.new(balance: balance, cash_balance: balance)` without explicit requires, and include identical comments about not distinguishing cash vs non-cash balances for non-investment accounts.
📚 Learning: 2025-09-13T11:27:55.271Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/controllers/enable_banking_items_controller.rb:48-62
Timestamp: 2025-09-13T11:27:55.271Z
Learning: The `create_enable_banking_item!` method in `Family::EnableBankingConnectable` uses `find_or_create_by(id: enable_banking_id)` to either find an existing EnableBankingItem or create a new one, so it automatically handles updates to existing items without creating duplicates.
Applied to files:
app/views/enable_banking_items/_enable_banking_item.html.erbapp/views/settings/providers/_enable_banking_panel.html.erbapp/controllers/enable_banking_items_controller.rb
📚 Learning: 2025-11-24T16:57:20.307Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-11-24T16:57:20.307Z
Learning: Applies to app/views/**/_*.html.erb : Place context-specific partials in relevant controller view directory (e.g., accounts/_account_sidebar_tabs.html.erb)
Applied to files:
app/views/enable_banking_items/_enable_banking_item.html.erb
📚 Learning: 2025-09-23T22:23:22.963Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-09-23T22:23:22.963Z
Learning: Applies to app/views/**/*.html.erb : Use partials when content is primarily static, simple, context-specific, and without variants or complex configuration
Applied to files:
app/views/enable_banking_items/_enable_banking_item.html.erb
📚 Learning: 2025-11-24T16:55:43.046Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:55:43.046Z
Learning: Applies to app/views/**/_*.{erb,html.erb} : Use Partials when element is primarily static HTML with minimal logic, used in only one or few contexts, is simple template content, or doesn't need variants/sizes/complex configuration.
Applied to files:
app/views/enable_banking_items/_enable_banking_item.html.erb
📚 Learning: 2025-11-24T16:55:43.046Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:55:43.046Z
Learning: Applies to config/locales/**/*.yml : Use hierarchical i18n keys by feature: `accounts.index.title`, `transactions.form.amount_label`. Use `t()` helper for all user-facing strings with interpolation for dynamic content.
Applied to files:
app/views/enable_banking_items/_enable_banking_item.html.erbapp/views/settings/providers/_enable_banking_panel.html.erb
📚 Learning: 2025-11-24T16:56:13.394Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-conventions.mdc:0-0
Timestamp: 2025-11-24T16:56:13.394Z
Learning: Applies to **/*.html.erb : Prefer native HTML elements over JavaScript-based components. Use semantic HTML: <dialog> for modals, <details><summary> for disclosures
Applied to files:
app/views/enable_banking_items/_enable_banking_item.html.erbapp/views/settings/providers/_enable_banking_panel.html.erb
📚 Learning: 2025-11-24T16:57:20.307Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-11-24T16:57:20.307Z
Learning: Applies to **/*.{erb,html.erb} : Use Partials when: element is primarily static HTML with minimal logic, used in only one or few specific contexts, is simple template content, doesn't need variants/sizes/complex configuration, or is more about content organization than reusable functionality
Applied to files:
app/views/enable_banking_items/_enable_banking_item.html.erb
📚 Learning: 2025-11-24T16:54:59.185Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T16:54:59.185Z
Learning: Applies to app/views/**/_*.html.erb : Use partials only for simple, static HTML with minimal logic in specific contexts
Applied to files:
app/views/enable_banking_items/_enable_banking_item.html.erb
📚 Learning: 2025-11-24T16:54:59.185Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T16:54:59.185Z
Learning: Applies to app/views/**/*.erb : Prefer semantic HTML elements (dialog, details, summary) over JavaScript components
Applied to files:
app/views/enable_banking_items/_enable_banking_item.html.erb
📚 Learning: 2025-11-24T16:57:20.307Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-11-24T16:57:20.307Z
Learning: Applies to app/views/**/_*.html.erb : Use underscore prefix for partial files (e.g., _trend_change.html.erb, _form_errors.html.erb, _sync_indicator.html.erb)
Applied to files:
app/views/enable_banking_items/_enable_banking_item.html.erb
📚 Learning: 2025-11-24T16:55:43.046Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:55:43.046Z
Learning: Applies to **/*.{erb,html.erb,rb} : All user-facing strings must use localization (i18n) via the `t()` helper. Update locale files for each new or changed element.
Applied to files:
app/views/enable_banking_items/_enable_banking_item.html.erbapp/views/settings/providers/_enable_banking_panel.html.erb
📚 Learning: 2025-10-07T09:51:02.265Z
Learnt from: jjmata
Repo: we-promise/sure PR: 189
File: app/views/password_resets/new.html.erb:14-14
Timestamp: 2025-10-07T09:51:02.265Z
Learning: In the we-promise/sure repository, use i18n methods (like `t()` or `I18n.t`) for all user-facing strings to maintain proper internationalization infrastructure. While translations should be provided, extensive localization work is not a priority at this stage of development.
Applied to files:
app/views/enable_banking_items/_enable_banking_item.html.erbapp/views/settings/providers/_enable_banking_panel.html.erb
📚 Learning: 2025-10-01T11:40:36.711Z
Learnt from: jjmata
Repo: we-promise/sure PR: 173
File: app/mailers/email_confirmation_mailer.rb:9-9
Timestamp: 2025-10-01T11:40:36.711Z
Learning: In the we-promise/sure repository, i18n (internationalization) is used throughout the application for translations. When making branding configurable (e.g., product names), it's appropriate to add parameters like `product: product_name` to existing `t()` translation calls rather than replacing them with hardcoded strings.
Applied to files:
app/views/enable_banking_items/_enable_banking_item.html.erbapp/views/settings/providers/_enable_banking_panel.html.erb
📚 Learning: 2025-11-24T16:55:43.046Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:55:43.046Z
Learning: Applies to **/*.rb : Use Rails pluralization in i18n: `t("transactions.count", count: transactions.count)`.
Applied to files:
app/views/enable_banking_items/_enable_banking_item.html.erbapp/views/settings/providers/_enable_banking_panel.html.erb
📚 Learning: 2025-11-24T16:56:13.394Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-conventions.mdc:0-0
Timestamp: 2025-11-24T16:56:13.394Z
Learning: Applies to **/*.{rb,js,erb} : Format currencies, numbers, dates, and other values server-side, then pass to Stimulus controllers for display only
Applied to files:
app/views/enable_banking_items/_enable_banking_item.html.erbapp/views/settings/providers/_enable_banking_panel.html.erb
📚 Learning: 2025-11-24T16:54:59.185Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T16:54:59.185Z
Learning: Applies to app/views/**/*.erb : Use server-side formatting for currencies, numbers, and dates instead of client-side
Applied to files:
app/views/enable_banking_items/_enable_banking_item.html.erbapp/views/settings/providers/_enable_banking_panel.html.erb
📚 Learning: 2025-11-24T16:57:20.307Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-11-24T16:57:20.307Z
Learning: Applies to **/*.{erb,html.erb} : Keep domain logic out of views: compute values like button classes, conditional logic, and data transformations in the component file, not the template file
Applied to files:
app/views/enable_banking_items/_enable_banking_item.html.erb
📚 Learning: 2025-09-23T22:23:22.963Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-09-23T22:23:22.963Z
Learning: Applies to app/{views,components}/**/*.html.erb : Keep domain logic out of ERB templates; compute classes and logic in component/Ruby code, not inline in the template
Applied to files:
app/views/enable_banking_items/_enable_banking_item.html.erb
📚 Learning: 2025-11-24T16:57:34.442Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-24T16:57:34.442Z
Learning: Applies to app/views/**/*.erb : Views should use ERB and avoid heavy logic; prefer helpers and components instead (ERB checked by `erb-lint` per `.erb_lint.yml`)
Applied to files:
app/views/enable_banking_items/_enable_banking_item.html.erb
📚 Learning: 2025-11-24T16:55:43.046Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:55:43.046Z
Learning: Applies to {app/components/**/*.html.erb,app/views/**/*.{erb,html.erb}} : Keep domain logic out of view templates. Logic belongs in component files, not template files.
Applied to files:
app/views/enable_banking_items/_enable_banking_item.html.erb
📚 Learning: 2025-11-24T16:54:59.185Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T16:54:59.185Z
Learning: Applies to **/*.rb : Optimize database queries with proper indexes to prevent N+1 queries using includes/joins
Applied to files:
app/views/enable_banking_items/_enable_banking_item.html.erb
📚 Learning: 2025-11-24T16:56:13.394Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-conventions.mdc:0-0
Timestamp: 2025-11-24T16:56:13.394Z
Learning: Use Rails concerns for code organization around model 'traits' and shared functionality, not just for moving code to another location. Prefer models answering questions about themselves with methods like account.balance_series over service-like patterns
Applied to files:
app/views/enable_banking_items/_enable_banking_item.html.erb
📚 Learning: 2025-09-13T11:36:45.479Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method pattern is consistent across banking integrations in the codebase. Both PlaidAccount::Processor and EnableBankingAccount::Processor use the same approach: `balance = account.current_balance || account.available_balance || 0` followed by `OpenStruct.new(balance: balance, cash_balance: balance)` without explicit requires, and include identical comments about not distinguishing cash vs non-cash balances for non-investment accounts.
Applied to files:
app/views/enable_banking_items/_enable_banking_item.html.erbapp/controllers/enable_banking_items_controller.rb
📚 Learning: 2025-11-24T16:55:43.046Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:55:43.046Z
Learning: Applies to **/*.{erb,html.erb,rb} : Use server-side formatting for currencies, numbers, and dates. Do not format in JavaScript.
Applied to files:
app/views/settings/providers/_enable_banking_panel.html.erb
📚 Learning: 2025-11-24T16:55:43.046Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:55:43.046Z
Learning: Applies to app/models/**/*.rb : All monetary values stored in base currency (user's primary currency). Use `Money` objects for currency conversion and formatting.
Applied to files:
app/views/settings/providers/_enable_banking_panel.html.erb
📚 Learning: 2025-11-24T16:55:43.046Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:55:43.046Z
Learning: Use Plaid integration for real-time bank account syncing via `PlaidItem`, `Sync` models, and background jobs for data updates.
Applied to files:
app/controllers/enable_banking_items_controller.rb
📚 Learning: 2025-11-24T16:55:43.046Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:55:43.046Z
Learning: Applies to app/controllers/**/*.rb : Use strong parameters and CSRF protection throughout the application.
Applied to files:
app/controllers/enable_banking_items_controller.rb
📚 Learning: 2025-09-13T11:36:45.479Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method in EnableBankingAccount::Processor follows the same pattern as the Plaid integration, using OpenStruct without explicit requires and returning balance values as-is without BigDecimal coercion for consistency across banking integrations.
Applied to files:
app/controllers/enable_banking_items_controller.rb
📚 Learning: 2025-11-24T16:54:59.185Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T16:54:59.185Z
Learning: Applies to app/controllers/**/*.rb : Use Rails strong parameters and CSRF protection throughout the application
Applied to files:
app/controllers/enable_banking_items_controller.rb
📚 Learning: 2025-09-13T11:33:54.159Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account.rb:9-20
Timestamp: 2025-09-13T11:33:54.159Z
Learning: The EnableBankingAccount model uses a `has_balance` validation method that ensures at least one balance value (current_balance or available_balance) is present, similar to the Plaid integration pattern. This means balance values should not be nil and the validation expects actual balance data from the API.
Applied to files:
app/controllers/enable_banking_items_controller.rb
🧬 Code graph analysis (1)
app/controllers/enable_banking_items_controller.rb (2)
app/models/enable_banking_item.rb (4)
revoke_session(225-242)credentials_configured?(41-43)start_authorization(59-77)complete_authorization(80-98)app/models/provider/enable_banking.rb (2)
get_aspsps(21-31)start_authorization(40-63)
🪛 GitHub Actions: Pull Request
app/controllers/enable_banking_items_controller.rb
[warning] 124-124: Brakeman: Possible unprotected redirect. Code: redirect_to(Current.family.enable_banking_items.find(params[:id]).start_authorization(...), :allow_other_host => true)
[warning] 197-197: Brakeman: Possible unprotected redirect. Code: redirect_to(Current.family.enable_banking_items.find(params[:id]).start_authorization(...), :allow_other_host => true)
🔇 Additional comments (8)
app/views/enable_banking_items/_enable_banking_item.html.erb (2)
1-17: LGTM - Header structure is clean and semantic.The local declaration is proper, and the header section uses semantic HTML with
<details>and<summary>elements following coding guidelines. The conditional logo/initials fallback is well-implemented.
52-79: Action buttons implementation looks correct.The conditional rendering based on
requires_update?and the development-mode refresh are appropriate. Theturbo: falsefor OAuth redirect andCustomConfirm.for_resource_deletionfor deletion are good patterns.app/controllers/enable_banking_items_controller.rb (6)
5-37: LGTM - Create action is well-structured.Uses
Current.familycorrectly, applies strong parameters, and handles both Turbo frame and regular requests with appropriate flash messages usingt()helper.
39-68: Update action follows correct patterns.Consistent with create action, uses strong parameters and handles Turbo/non-Turbo responses appropriately.
70-85: Destroy and sync actions are well-implemented.The destroy action properly revokes the session before scheduling deletion. The sync action correctly prevents duplicate jobs by checking
syncing?first.
87-106: Select bank action handles errors appropriately.Validates credentials first, catches API errors gracefully with logging, and falls back to an empty list. The
layout: falseis correct for modal rendering.
134-174: Callback action handles OAuth flow correctly.The error parameter handling, state validation, and authorization completion flow are well-structured. Consider the earlier suggestion about using cryptographic state tokens for additional security.
348-372: Private methods are well-structured.Strong parameters include appropriate fields, and the callback URL generation correctly handles development vs production environments.
| client_certificate: @enable_banking_item.client_certificate | ||
| ) | ||
|
|
||
| redirect_to select_bank_enable_banking_item_path(new_item), data: { turbo_frame: "modal" } |
There was a problem hiding this comment.
The data: { turbo_frame: "modal" } on redirect_to has no effect.
Turbo frame data attributes only work on links and forms, not server-side redirects. The browser will follow the redirect normally.
- redirect_to select_bank_enable_banking_item_path(new_item), data: { turbo_frame: "modal" }
+ redirect_to select_bank_enable_banking_item_path(new_item)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| redirect_to select_bank_enable_banking_item_path(new_item), data: { turbo_frame: "modal" } | |
| redirect_to select_bank_enable_banking_item_path(new_item) |
🤖 Prompt for AI Agents
In app/controllers/enable_banking_items_controller.rb around line 185, the
controller is calling redirect_to with data: { turbo_frame: "modal" } which has
no effect because Turbo frame attributes only apply to links/forms in the
browser, not server-side redirects; remove the data: { turbo_frame: "modal" }
from the redirect and either (a) if you want to open/update a modal frame as a
result of this action, respond with a Turbo Stream (render a turbo_stream
response that targets the "modal" frame and replaces/updates its contents) or
(b) keep the redirect but ensure the originating link/form includes
data-turbo-frame="modal" so the browser issues a frame-targeted request; pick
the appropriate approach and implement the corresponding server response
(turbo_stream render for modal update, or plain redirect without data).
| selected_uids.each do |uid| | ||
| enable_banking_account = enable_banking_item.enable_banking_accounts.find_by(uid: uid) | ||
| next unless enable_banking_account | ||
|
|
||
| # Check if already linked | ||
| if enable_banking_account.account_provider.present? | ||
| already_linked_accounts << enable_banking_account.name | ||
| next | ||
| end | ||
|
|
||
| # Create the internal Account | ||
| account = Account.create_and_sync( | ||
| family: Current.family, | ||
| name: enable_banking_account.name, | ||
| balance: enable_banking_account.current_balance || 0, | ||
| currency: enable_banking_account.currency || "EUR", | ||
| accountable_type: accountable_type, | ||
| accountable_attributes: {} | ||
| ) | ||
|
|
||
| # Link account to enable_banking_account via account_providers | ||
| AccountProvider.create!( | ||
| account: account, | ||
| provider: enable_banking_account | ||
| ) | ||
|
|
||
| created_accounts << account | ||
| end |
There was a problem hiding this comment.
Wrap account creation loop in a database transaction.
If AccountProvider.create! fails midway through the loop, some accounts will be created without their provider links, leaving the database in an inconsistent state.
+ ActiveRecord::Base.transaction do
selected_uids.each do |uid|
enable_banking_account = enable_banking_item.enable_banking_accounts.find_by(uid: uid)
next unless enable_banking_account
# ... rest of loop
end
+ end🤖 Prompt for AI Agents
In app/controllers/enable_banking_items_controller.rb around lines 224 to 251,
the loop creating Account and AccountProvider should be executed inside an
ActiveRecord transaction so partial failures don't leave created accounts
without provider links; wrap the selected_uids.each block in a transaction (e.g.
ActiveRecord::Base.transaction do ... end), ensure account creation raises on
failure (use a bang or raise if create_and_sync returns a failed record) so the
transaction will roll back, and keep AccountProvider.create! as-is so any
exception will abort and rollback the whole operation.
| account_types.each do |enable_banking_account_id, selected_type| | ||
| enable_banking_account = @enable_banking_item.enable_banking_accounts.find(enable_banking_account_id) | ||
| selected_subtype = account_subtypes[enable_banking_account_id] | ||
|
|
||
| # Default subtype for CreditCard since it only has one option | ||
| selected_subtype = "credit_card" if selected_type == "CreditCard" && selected_subtype.blank? | ||
|
|
||
| # Create account with user-selected type and subtype | ||
| account = Account.create_from_enable_banking_account( | ||
| enable_banking_account, | ||
| selected_type, | ||
| selected_subtype | ||
| ) | ||
|
|
||
| # Link account via AccountProvider | ||
| AccountProvider.create!( | ||
| account: account, | ||
| provider: enable_banking_account | ||
| ) | ||
| end |
There was a problem hiding this comment.
Wrap account setup loop in a database transaction.
Similar to link_accounts, if account creation or provider linking fails partway through, the database will be left in an inconsistent state with some accounts created but not all.
+ ActiveRecord::Base.transaction do
account_types.each do |enable_banking_account_id, selected_type|
# ... account creation logic
end
+ end🤖 Prompt for AI Agents
In app/controllers/enable_banking_items_controller.rb around lines 317 to 336,
the account creation and AccountProvider linking loop must be wrapped in a
database transaction so partial failures don't leave the DB in an inconsistent
state; surround the loop with an ActiveRecord transaction (e.g.,
ActiveRecord::Base.transaction do ... end) and ensure failures raise to trigger
rollback by using bang methods or raising if create_from_enable_banking_account
returns an invalid record, so any exception during account creation or
AccountProvider.create! will roll back all changes.
| <% | ||
| enable_banking_item = Current.family.enable_banking_items.first_or_initialize(name: "Enable Banking Connection") | ||
| is_new_record = enable_banking_item.new_record? | ||
| # Check if there are any authenticated connections (have session_id) | ||
| has_authenticated_connections = Current.family.enable_banking_items.where.not(session_id: nil).exists? | ||
| %> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move query logic to the controller.
The first_or_initialize and where.not(...).exists? queries are domain logic that should be computed in the controller and passed as locals.
# In controller:
@enable_banking_item = Current.family.enable_banking_items.first_or_initialize(name: "Enable Banking Connection")
@has_authenticated_connections = Current.family.enable_banking_items.where.not(session_id: nil).exists?🤖 Prompt for AI Agents
In app/views/settings/providers/_enable_banking_panel.html.erb around lines 26
to 31 the view is performing ActiveRecord queries (first_or_initialize and
where.not(...).exists?) that belong in the controller; move these queries into
the corresponding controller action (e.g. set @enable_banking_item =
Current.family.enable_banking_items.first_or_initialize(name: "Enable Banking
Connection") and @has_authenticated_connections =
Current.family.enable_banking_items.where.not(session_id: nil).exists?) and pass
them to the view (as instance vars or locals), then update the partial to
reference those variables and remove the query logic from the template; ensure
the controller always sets those variables (or provide safe nil/defaults) so the
view does not run queries.
| <% | ||
| # Find the first item with valid session to use for "Add Connection" button | ||
| item_for_new_connection = items.find(&:session_valid?) | ||
| # Check if any item needs initial connection (configured but no session yet) | ||
| item_needing_connection = items.find { |i| !i.session_valid? && !i.session_expired? } | ||
| %> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move item filtering logic to the controller.
The items.find(&:session_valid?) and items.find { |i| ... } logic should be computed in the controller and passed as locals.
# In controller:
@item_for_new_connection = items.find(&:session_valid?)
@item_needing_connection = items.find { |i| !i.session_valid? && !i.session_expired? }🤖 Prompt for AI Agents
In app/views/settings/providers/_enable_banking_panel.html.erb around lines 108
to 113, the view currently runs items.find(&:session_valid?) and items.find {
|i| !i.session_valid? && !i.session_expired? } — move this filtering into the
controller so the view receives precomputed values; in the controller compute
@item_for_new_connection = items.find(&:session_valid?) and
@item_needing_connection = items.find { |i| !i.session_valid? &&
!i.session_expired? } (or pass them as locals when rendering the partial), then
remove the inline find calls from the partial and reference the passed-in
variables instead.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
app/views/enable_banking_items/setup_accounts.html.erb (1)
2-7: Hardcoded strings should use i18n.As previously noted, multiple user-facing strings are hardcoded. Per coding guidelines, all user-facing strings must use
t()helper with corresponding locale entries.Also applies to: 26-35, 45-54, 63-68, 82-83, 105-105, 113-113
app/models/enable_banking_entry/processor.rb (1)
142-149: Wirelog_invalid_currencyintocurrencyfallback logic
log_invalid_currencyis still unused; this was flagged previously. You can keep the helper and improve observability by invoking it when both transaction-level and entry-level currency codes failparse_currency, while preserving the existing fallback behavior:- def currency - tx_amount = data[:transaction_amount] || {} - parse_currency(tx_amount[:currency]) || parse_currency(data[:currency]) || account&.currency || "EUR" - end - - def log_invalid_currency(currency_value) - Rails.logger.warn("Invalid currency code '#{currency_value}' in Enable Banking transaction #{external_id}, falling back to account currency") - end + def currency + tx_amount = data[:transaction_amount] || {} + tx_currency = tx_amount[:currency] + entry_currency = data[:currency] + + if tx_currency.present? + parsed = parse_currency(tx_currency) + return parsed if parsed + + log_invalid_currency(tx_currency) + end + + if entry_currency.present? + parsed = parse_currency(entry_currency) + return parsed if parsed + + log_invalid_currency(entry_currency) + end + + account&.currency || "EUR" + end + + def log_invalid_currency(currency_value) + Rails.logger.warn( + "Invalid currency code '#{currency_value}' in Enable Banking transaction #{external_id}, falling back to account currency" + ) + endThis keeps the resolution order (
transaction_amount.currency→data.currency→account→"EUR"), logs truly invalid codes, and avoids silently swallowing bad data.
🧹 Nitpick comments (3)
app/models/enable_banking_account.rb (2)
6-17: Redundant associations and method.
linked_account(line 9) andcurrent_account(lines 15-17) both resolve to the sameaccountassociation. If this redundancy is intentional for API consistency with other banking integrations, consider adding a comment explaining the purpose. Otherwise, remove the duplicates.
19-43: Consider extracting the type mappings to a frozen constant.The
type_mappingshash is recreated on every method call. For marginal efficiency and clearer documentation, extract it to a class constant.+ # Map PSD2 cash_account_type codes to user-friendly names + # Based on ISO 20022 External Cash Account Type codes + ACCOUNT_TYPE_MAPPINGS = { + "CACC" => "Current/Checking Account", + "SVGS" => "Savings Account", + "CARD" => "Card Account", + "CRCD" => "Credit Card", + "LOAN" => "Loan Account", + "MORT" => "Mortgage Account", + "ODFT" => "Overdraft Account", + "CASH" => "Cash Account", + "TRAN" => "Transacting Account", + "SALA" => "Salary Account", + "MOMA" => "Money Market Account", + "NREX" => "Non-Resident External Account", + "TAXE" => "Tax Account", + "TRAS" => "Cash Trading Account", + "ONDP" => "Overnight Deposit" + }.freeze + - # Map PSD2 cash_account_type codes to user-friendly names - # Based on ISO 20022 External Cash Account Type codes def account_type_display return nil unless account_type.present? - - type_mappings = { - "CACC" => "Current/Checking Account", - ... - } - - type_mappings[account_type.upcase] || account_type.titleize + ACCOUNT_TYPE_MAPPINGS[account_type.upcase] || account_type.titleize endapp/models/enable_banking_entry/processor.rb (1)
63-67: Clarify external ID missing-field error message
external_idcorrectly falls back toentry_reference, but the raised error only mentionstransaction_idwhen both are missing. Consider updating the message so it reflects both accepted fields, e.g.:- raise ArgumentError, "Enable Banking transaction missing required field 'transaction_id'" unless id + raise ArgumentError, "Enable Banking transaction missing required field 'transaction_id' or 'entry_reference'" unless idThis will make debugging malformed payloads easier when only
entry_referenceis relevant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/models/enable_banking_account.rb(1 hunks)app/models/enable_banking_account/processor.rb(1 hunks)app/models/enable_banking_entry/processor.rb(1 hunks)app/views/enable_banking_items/setup_accounts.html.erb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/models/enable_banking_account/processor.rb
🧰 Additional context used
📓 Path-based instructions (20)
{app/**/*.{erb,html},app/javascript/**/*.js}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Prefer Hotwire-first approach: use Turbo + Stimulus for reactive UI without heavy JavaScript
Files:
app/views/enable_banking_items/setup_accounts.html.erb
app/views/**/*.erb
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
app/views/**/*.erb: Always use theiconhelper for icons in views, never uselucide_icondirectly
Use Tailwind design tokens (e.g., text-primary, bg-container, border-secondary) instead of raw color names
Pass data to Stimulus controllers via data-*-value attributes instead of inline JavaScript
Prefer semantic HTML elements (dialog, details, summary) over JavaScript components
Use Turbo Frames for page sections instead of client-side solutions
Use server-side formatting for currencies, numbers, and dates instead of client-sideViews should use ERB and avoid heavy logic; prefer helpers and components instead (ERB checked by
erb-lintper.erb_lint.yml)
Files:
app/views/enable_banking_items/setup_accounts.html.erb
**/*.{css,erb}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use Tailwind CSS v4.x with custom design system for styling
Files:
app/views/enable_banking_items/setup_accounts.html.erb
**/*.{rb,erb,html.erb}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{rb,erb,html.erb}: UseCurrent.userfor the current user context in controllers and views. Do NOT usecurrent_user.
UseCurrent.familyfor the current family context. Do NOT usecurrent_family.
Files:
app/views/enable_banking_items/setup_accounts.html.erbapp/models/enable_banking_account.rbapp/models/enable_banking_entry/processor.rb
**/*.{erb,html,vue,jsx,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{erb,html,vue,jsx,tsx}: Use Tailwind CSS v4.x with custom design system defined inapp/assets/tailwind/maybe-design-system.css. Always use functional tokens (e.g.,text-primarynottext-white).
Prefer semantic HTML elements over JavaScript components (e.g., use<dialog>for modals,<details><summary>for disclosures).
Files:
app/views/enable_banking_items/setup_accounts.html.erb
**/*.{erb,html.erb}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{erb,html.erb}: Useiconhelper for icons, never uselucide_icondirectly.
Leverage Turbo frames for page sections over client-side solutions.
Pass data to Stimulus controllers viadata-*-valueattributes, not inline JavaScript.
**/*.{erb,html.erb}: Use ViewComponents when: element has complex logic or styling patterns, will be reused across multiple views/contexts, needs structured styling with variants/sizes, requires interactive behavior or Stimulus controllers, has configurable slots or complex APIs, or needs accessibility features or ARIA support
Use Partials when: element is primarily static HTML with minimal logic, used in only one or few specific contexts, is simple template content, doesn't need variants/sizes/complex configuration, or is more about content organization than reusable functionality
Prefer components over partials: if a component is available in app/components, use it; if not, look for a partial; if neither exists, decide between component or partial based on complexity and reusability criteria
Keep domain logic out of views: compute values like button classes, conditional logic, and data transformations in the component file, not the template file
Always use the declarative approach when integrating Stimulus controllers in views: the ERB template should declare what happens using data-* attributes, and the Stimulus controller should respond
Component controllers in app/components/ should only be used within their component templates; global controllers in app/javascript/controllers/ can be used across any view
Pass data from Rails to Stimulus using data-*-value attributes, not inline JavaScript
Files:
app/views/enable_banking_items/setup_accounts.html.erb
**/*.{erb,html.erb,rb}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{erb,html.erb,rb}: All user-facing strings must use localization (i18n) via thet()helper. Update locale files for each new or changed element.
Use server-side formatting for currencies, numbers, and dates. Do not format in JavaScript.
Files:
app/views/enable_banking_items/setup_accounts.html.erbapp/models/enable_banking_account.rbapp/models/enable_banking_entry/processor.rb
{app/components/**/*.html.erb,app/views/**/*.{erb,html.erb}}
📄 CodeRabbit inference engine (CLAUDE.md)
Keep domain logic out of view templates. Logic belongs in component files, not template files.
Files:
app/views/enable_banking_items/setup_accounts.html.erb
**/*.erb
📄 CodeRabbit inference engine (CLAUDE.md)
Run
bundle exec erb_lint ./app/**/*.erb -afor ERB linting with auto-correct before pull requests.
Files:
app/views/enable_banking_items/setup_accounts.html.erb
**/*.{rb,erb}
📄 CodeRabbit inference engine (.cursor/rules/general-rules.mdc)
**/*.{rb,erb}: UseCurrent.userfor accessing the current user. Do NOT usecurrent_user
UseCurrent.familyfor accessing the current family. Do NOT usecurrent_family
Files:
app/views/enable_banking_items/setup_accounts.html.erbapp/models/enable_banking_account.rbapp/models/enable_banking_entry/processor.rb
**/*.html.erb
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
**/*.html.erb: Prefer native HTML elements over JavaScript-based components. Use semantic HTML: for modals,for disclosures
Leverage Turbo frames to break up pages instead of JavaScript-driven client-side solutions
Use theiconhelper from application_helper.rb for icons. Never uselucide_iconhelper directly
Files:
app/views/enable_banking_items/setup_accounts.html.erb
**/*.{rb,js,erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Format currencies, numbers, dates, and other values server-side, then pass to Stimulus controllers for display only
Files:
app/views/enable_banking_items/setup_accounts.html.erbapp/models/enable_banking_account.rbapp/models/enable_banking_entry/processor.rb
**/*.{rb,html.erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Use ActiveRecord validations for complex validations and business logic. Simple validations may be mirrored in ActiveRecord for form error handling convenience, but prioritize client-side form validation when possible
Files:
app/views/enable_banking_items/setup_accounts.html.erbapp/models/enable_banking_account.rbapp/models/enable_banking_entry/processor.rb
{app/views/**,app/helpers/**,app/javascript/controllers/**}
📄 CodeRabbit inference engine (.cursor/rules/ui-ux-design-guidelines.mdc)
{app/views/**,app/helpers/**,app/javascript/controllers/**}: Reference maybe-design-system.css for base primitives, functional tokens, and component tokens before writing styles
Prefer using functional design system tokens (e.g., text-primary, bg-container, border-primary) from maybe-design-system.css instead of raw Tailwind color values
Files:
app/views/enable_banking_items/setup_accounts.html.erb
{app/views/**,app/helpers/**}
📄 CodeRabbit inference engine (.cursor/rules/ui-ux-design-guidelines.mdc)
Always generate semantic HTML
Files:
app/views/enable_banking_items/setup_accounts.html.erb
**/*.rb
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rb: Application supports two modes: 'managed' and 'self_hosted' via Rails.application.config.app_mode
Use Current.user and Current.family instead of current_user / current_family for authentication context
Optimize database queries with proper indexes to prevent N+1 queries using includes/joins
**/*.rb: Use Rails pluralization in i18n:t("transactions.count", count: @transactions.count).
Runbin/rubocop -f github -afor Ruby linting with auto-correct before pull requests.
**/*.rb: Ruby code should use 2-space indent,snake_casefor methods and variables, andCamelCasefor classes and modules, following Rails conventions for folders and file names
Prefer environment variables over hard-coded values for configuration
Files:
app/models/enable_banking_account.rbapp/models/enable_banking_entry/processor.rb
app/models/**/*.rb
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
app/models/**/*.rb: Keep business logic in models using skinny controllers and fat models pattern
Store all monetary values in base currency (user's primary currency) and use Money objects for conversion
Use ActiveRecord validations for form convenience and complex business logic validations
Use Rails concerns and POROs for organizing business logic instead of services folder
app/models/**/*.rb: All monetary values stored in base currency (user's primary currency). UseMoneyobjects for currency conversion and formatting.
Place business logic inapp/models/folder. Avoid creatingapp/services/folder. Use Rails concerns and POROs for organization.
Models should answer questions about themselves using instance methods: useaccount.balance_seriesnotAccountSeries.new(account).call.
Files:
app/models/enable_banking_account.rbapp/models/enable_banking_entry/processor.rb
{app/models/**/*.rb,db/migrate/**/*.rb}
📄 CodeRabbit inference engine (CLAUDE.md)
Simple validations (null checks, unique indexes) should be in the database. ActiveRecord validations for form convenience (prefer client-side when possible). Complex validations and business logic in ActiveRecord.
Files:
app/models/enable_banking_account.rbapp/models/enable_banking_entry/processor.rb
app/**/*.{rb,js,ts,jsx,tsx,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Code should be organized in
app/directory (Rails MVC, services, jobs, mailers, components), with JS inapp/javascript/, and styles/assets inapp/assets/(Tailwind, images, fonts)
Files:
app/models/enable_banking_account.rbapp/models/enable_banking_entry/processor.rb
app/**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Run
bin/brakemansecurity scan before major PRs to check for static analysis of common Rails issues
Files:
app/models/enable_banking_account.rbapp/models/enable_banking_entry/processor.rb
🧠 Learnings (28)
📓 Common learnings
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: db/migrate/20250825224717_add_enable_banking_domain.rb:3-34
Timestamp: 2025-09-13T12:23:31.324Z
Learning: When suggesting database schema improvements for banking integrations, consider consistency across all similar integrations (Plaid, SimpleFin, Enable Banking) and propose comprehensive changes in a separate PR rather than making changes to individual integrations in isolation.
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:55:43.046Z
Learning: Use Plaid integration for real-time bank account syncing via `PlaidItem`, `Sync` models, and background jobs for data updates.
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method in EnableBankingAccount::Processor follows the same pattern as the Plaid integration, using OpenStruct without explicit requires and returning balance values as-is without BigDecimal coercion for consistency across banking integrations.
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/controllers/enable_banking_items_controller.rb:48-62
Timestamp: 2025-09-13T11:27:55.271Z
Learning: The `create_enable_banking_item!` method in `Family::EnableBankingConnectable` uses `find_or_create_by(id: enable_banking_id)` to either find an existing EnableBankingItem or create a new one, so it automatically handles updates to existing items without creating duplicates.
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account.rb:9-20
Timestamp: 2025-09-13T11:33:54.159Z
Learning: The EnableBankingAccount model uses a `has_balance` validation method that ensures at least one balance value (current_balance or available_balance) is present, similar to the Plaid integration pattern. This means balance values should not be nil and the validation expects actual balance data from the API.
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account.rb:9-20
Timestamp: 2025-09-13T11:33:54.159Z
Learning: The EnableBankingAccount model uses a `has_balance` validation method that ensures at least one balance value (current_balance or available_balance) is present, similar to the Plaid integration pattern. This means balance values should not be nil and the validation expects actual balance data from the API.
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method pattern is consistent across banking integrations in the codebase. Both PlaidAccount::Processor and EnableBankingAccount::Processor use the same approach: `balance = account.current_balance || account.available_balance || 0` followed by `OpenStruct.new(balance: balance, cash_balance: balance)` without explicit requires, and include identical comments about not distinguishing cash vs non-cash balances for non-investment accounts.
📚 Learning: 2025-09-23T22:23:22.963Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-09-23T22:23:22.963Z
Learning: Applies to app/{views,components}/**/*.html.erb : Integrate Stimulus declaratively in ERB: templates declare data-controller/actions/targets; controllers respond to those declarations
Applied to files:
app/views/enable_banking_items/setup_accounts.html.erb
📚 Learning: 2025-11-24T16:57:20.307Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-11-24T16:57:20.307Z
Learning: Applies to app/views/**/_*.html.erb : Place context-specific partials in relevant controller view directory (e.g., accounts/_account_sidebar_tabs.html.erb)
Applied to files:
app/views/enable_banking_items/setup_accounts.html.erb
📚 Learning: 2025-09-23T22:22:00.149Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/stimulus_conventions.mdc:0-0
Timestamp: 2025-09-23T22:22:00.149Z
Learning: Applies to app/{views,components}/**/*.html.erb : In ERB views, wire Stimulus interactions declaratively using data-action to call controller methods
Applied to files:
app/views/enable_banking_items/setup_accounts.html.erb
📚 Learning: 2025-09-13T11:27:55.271Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/controllers/enable_banking_items_controller.rb:48-62
Timestamp: 2025-09-13T11:27:55.271Z
Learning: The `create_enable_banking_item!` method in `Family::EnableBankingConnectable` uses `find_or_create_by(id: enable_banking_id)` to either find an existing EnableBankingItem or create a new one, so it automatically handles updates to existing items without creating duplicates.
Applied to files:
app/views/enable_banking_items/setup_accounts.html.erbapp/models/enable_banking_account.rbapp/models/enable_banking_entry/processor.rb
📚 Learning: 2025-09-23T22:23:22.963Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-09-23T22:23:22.963Z
Learning: Applies to app/views/**/*.html.erb : Use ViewComponents when elements have complex logic/styling, are reused across contexts, need variants/slots/accessibility, or require Stimulus interactivity
Applied to files:
app/views/enable_banking_items/setup_accounts.html.erb
📚 Learning: 2025-11-24T16:57:20.307Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-11-24T16:57:20.307Z
Learning: Applies to **/*.{erb,html.erb} : Always use the declarative approach when integrating Stimulus controllers in views: the ERB template should declare what happens using data-* attributes, and the Stimulus controller should respond
Applied to files:
app/views/enable_banking_items/setup_accounts.html.erb
📚 Learning: 2025-11-24T16:55:43.046Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:55:43.046Z
Learning: Applies to config/locales/**/*.yml : Use hierarchical i18n keys by feature: `accounts.index.title`, `transactions.form.amount_label`. Use `t()` helper for all user-facing strings with interpolation for dynamic content.
Applied to files:
app/views/enable_banking_items/setup_accounts.html.erb
📚 Learning: 2025-11-24T16:56:13.394Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-conventions.mdc:0-0
Timestamp: 2025-11-24T16:56:13.394Z
Learning: Applies to **/*.html.erb : Prefer native HTML elements over JavaScript-based components. Use semantic HTML: <dialog> for modals, <details><summary> for disclosures
Applied to files:
app/views/enable_banking_items/setup_accounts.html.erb
📚 Learning: 2025-11-24T16:57:20.307Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-11-24T16:57:20.307Z
Learning: Applies to **/*.{erb,html.erb} : Use ViewComponents when: element has complex logic or styling patterns, will be reused across multiple views/contexts, needs structured styling with variants/sizes, requires interactive behavior or Stimulus controllers, has configurable slots or complex APIs, or needs accessibility features or ARIA support
Applied to files:
app/views/enable_banking_items/setup_accounts.html.erb
📚 Learning: 2025-09-23T22:23:22.963Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/view_conventions.mdc:0-0
Timestamp: 2025-09-23T22:23:22.963Z
Learning: Applies to app/{views,components}/**/*.html.erb : Pass data from Rails to Stimulus using data-*-value attributes, not inline JavaScript
Applied to files:
app/views/enable_banking_items/setup_accounts.html.erb
📚 Learning: 2025-11-24T16:55:43.046Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:55:43.046Z
Learning: Applies to **/*.{erb,html.erb,rb} : All user-facing strings must use localization (i18n) via the `t()` helper. Update locale files for each new or changed element.
Applied to files:
app/views/enable_banking_items/setup_accounts.html.erb
📚 Learning: 2025-10-07T09:51:02.265Z
Learnt from: jjmata
Repo: we-promise/sure PR: 189
File: app/views/password_resets/new.html.erb:14-14
Timestamp: 2025-10-07T09:51:02.265Z
Learning: In the we-promise/sure repository, use i18n methods (like `t()` or `I18n.t`) for all user-facing strings to maintain proper internationalization infrastructure. While translations should be provided, extensive localization work is not a priority at this stage of development.
Applied to files:
app/views/enable_banking_items/setup_accounts.html.erb
📚 Learning: 2025-11-24T16:54:59.185Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T16:54:59.185Z
Learning: Applies to app/views/**/*.erb : Use server-side formatting for currencies, numbers, and dates instead of client-side
Applied to files:
app/views/enable_banking_items/setup_accounts.html.erb
📚 Learning: 2025-10-01T11:40:36.711Z
Learnt from: jjmata
Repo: we-promise/sure PR: 173
File: app/mailers/email_confirmation_mailer.rb:9-9
Timestamp: 2025-10-01T11:40:36.711Z
Learning: In the we-promise/sure repository, i18n (internationalization) is used throughout the application for translations. When making branding configurable (e.g., product names), it's appropriate to add parameters like `product: product_name` to existing `t()` translation calls rather than replacing them with hardcoded strings.
Applied to files:
app/views/enable_banking_items/setup_accounts.html.erb
📚 Learning: 2025-11-24T16:55:43.046Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:55:43.046Z
Learning: Applies to **/*.rb : Use Rails pluralization in i18n: `t("transactions.count", count: transactions.count)`.
Applied to files:
app/views/enable_banking_items/setup_accounts.html.erb
📚 Learning: 2025-11-24T16:56:13.394Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-conventions.mdc:0-0
Timestamp: 2025-11-24T16:56:13.394Z
Learning: Applies to **/*.{rb,js,erb} : Format currencies, numbers, dates, and other values server-side, then pass to Stimulus controllers for display only
Applied to files:
app/views/enable_banking_items/setup_accounts.html.erb
📚 Learning: 2025-09-13T11:36:45.479Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method pattern is consistent across banking integrations in the codebase. Both PlaidAccount::Processor and EnableBankingAccount::Processor use the same approach: `balance = account.current_balance || account.available_balance || 0` followed by `OpenStruct.new(balance: balance, cash_balance: balance)` without explicit requires, and include identical comments about not distinguishing cash vs non-cash balances for non-investment accounts.
Applied to files:
app/views/enable_banking_items/setup_accounts.html.erbapp/models/enable_banking_account.rbapp/models/enable_banking_entry/processor.rb
📚 Learning: 2025-09-13T11:33:54.159Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account.rb:9-20
Timestamp: 2025-09-13T11:33:54.159Z
Learning: The EnableBankingAccount model uses a `has_balance` validation method that ensures at least one balance value (current_balance or available_balance) is present, similar to the Plaid integration pattern. This means balance values should not be nil and the validation expects actual balance data from the API.
Applied to files:
app/models/enable_banking_account.rbapp/models/enable_banking_entry/processor.rb
📚 Learning: 2025-11-24T16:56:30.657Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-11-24T16:56:30.657Z
Learning: Account model is a Rails delegated type with subtypes: Depository, Investment, Crypto, Property, Vehicle, OtherAsset (assets) and CreditCard, Loan, OtherLiability (liabilities)
Applied to files:
app/models/enable_banking_account.rb
📚 Learning: 2025-09-13T11:36:45.479Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method in EnableBankingAccount::Processor follows the same pattern as the Plaid integration, using OpenStruct without explicit requires and returning balance values as-is without BigDecimal coercion for consistency across banking integrations.
Applied to files:
app/models/enable_banking_account.rbapp/models/enable_banking_entry/processor.rb
📚 Learning: 2025-11-24T16:55:43.046Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:55:43.046Z
Learning: Use Plaid integration for real-time bank account syncing via `PlaidItem`, `Sync` models, and background jobs for data updates.
Applied to files:
app/models/enable_banking_account.rb
📚 Learning: 2025-09-13T12:23:31.324Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: db/migrate/20250825224717_add_enable_banking_domain.rb:3-34
Timestamp: 2025-09-13T12:23:31.324Z
Learning: When suggesting database schema improvements for banking integrations, consider consistency across all similar integrations (Plaid, SimpleFin, Enable Banking) and propose comprehensive changes in a separate PR rather than making changes to individual integrations in isolation.
Applied to files:
app/models/enable_banking_account.rb
📚 Learning: 2025-11-24T16:56:30.657Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-11-24T16:56:30.657Z
Learning: Entry model is a Rails delegated type with three subtypes: Valuation (absolute account value), Transaction (balance modification), and Trade (buy/sell for investment accounts)
Applied to files:
app/models/enable_banking_account.rb
📚 Learning: 2025-11-24T16:56:30.657Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-11-24T16:56:30.657Z
Learning: Syncable models (Account, PlaidItem, Family) must implement sync operations tracked via Sync records that log status, errors, and maintain an audit trail
Applied to files:
app/models/enable_banking_account.rb
📚 Learning: 2025-11-24T16:55:43.046Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:55:43.046Z
Learning: Applies to app/models/**/*.rb : All monetary values stored in base currency (user's primary currency). Use `Money` objects for currency conversion and formatting.
Applied to files:
app/models/enable_banking_account.rbapp/models/enable_banking_entry/processor.rb
📚 Learning: 2025-11-24T16:54:59.185Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T16:54:59.185Z
Learning: Applies to app/models/**/*.rb : Store all monetary values in base currency (user's primary currency) and use Money objects for conversion
Applied to files:
app/models/enable_banking_account.rbapp/models/enable_banking_entry/processor.rb
📚 Learning: 2025-11-24T16:56:30.657Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-design.mdc:0-0
Timestamp: 2025-11-24T16:56:30.657Z
Learning: Account sync must perform transfer auto-matching, calculate daily balance records from start_date to Date.current, calculate holdings, and optionally enrich transaction data
Applied to files:
app/models/enable_banking_entry/processor.rb
🧬 Code graph analysis (1)
app/models/enable_banking_entry/processor.rb (2)
app/models/enable_banking_item/importer.rb (1)
initialize(4-7)app/models/account/provider_import_adapter.rb (1)
import_transaction(21-82)
🔇 Additional comments (7)
app/models/enable_banking_account.rb (2)
80-86: LGTM!The method correctly uses
save!to match the bang method naming convention, ensuring failures are raised. The two-step approach withassign_attributesis fine here.
112-114: I'll verify the review comment by investigating thelog_invalid_currencymethod and how it's called in the codebase.
<function_calls>
#!/bin/bashSearch for CurrencyNormalizable concern and log_invalid_currency calls
echo "=== Searching for CurrencyNormalizable concern ==="
fd -e rb | xargs rg -l "CurrencyNormalizable" | head -20echo -e "\n=== Searching for log_invalid_currency method calls ==="
rg -n "log_invalid_currency" --type ruby -A 2 -B 2echo -e "\n=== Checking enable_banking_account.rb for currency setup ==="
cat -n app/models/enable_banking_account.rb | grep -A 5 -B 5 "currency|CurrencyNormalizable"
</function_calls>app/views/enable_banking_items/setup_accounts.html.erb (4)
9-19: Declarative Stimulus integration is correctly implemented.The form properly uses data attributes to wire up the
loading-buttoncontroller withsubmit->loading-button#showLoadingaction. Theturbo_frame: "_top"correctly ensures the response replaces the full page.
48-54: Date field implementation follows best practices.Server-side date handling with proper
min/maxconstraints is correct. The default of 3 months with a maximum of 1 year aligns with reasonable sync limits.
80-97: Stimulus controller integration is well-structured.The
account-type-selectorcontroller is properly scoped per account usingdata-account-type-selector-account-id-value. The declarative action binding (change->account-type-selector#updateSubtype) and target (subtypeContainer) follow conventions.
103-117: Button and link components properly configured.The
DS::ButtonandDS::Linkcomponents are used correctly with appropriate variants. The Stimulus target binding (loading_button_target: "button") properly connects to the loading-button controller.app/models/enable_banking_entry/processor.rb (1)
18-45: Transaction import flow and error handling look solidThe guard for missing
account, the single entry point viaimport_adapter.import_transaction, and the tieredrescuelogic (argument vs. AR vs. generic errors) are all appropriate and consistent with theAccount::ProviderImportAdaptercontract. No changes needed here.
| class EnableBankingAccount < ApplicationRecord | ||
| include CurrencyNormalizable | ||
|
|
||
| belongs_to :enable_banking_item | ||
|
|
||
| # New association through account_providers | ||
| has_one :account_provider, as: :provider, dependent: :destroy | ||
| has_one :account, through: :account_provider, source: :account | ||
| has_one :linked_account, through: :account_provider, source: :account | ||
|
|
||
| validates :name, :currency, presence: true | ||
| validates :uid, presence: true, uniqueness: { scope: :enable_banking_item_id } |
There was a problem hiding this comment.
🧩 Analysis chain
Script executed:
#!/bin/bash
# Search for PlaidAccount model and check for has_balance validation
fd -t f "plaid_account.rb" -x cat -n {}Repository: we-promise/sure
<shell_result>
Repository: we-promise/sure
Script execution failed with unrecoverable error: [CAPABILITY:RUN_SHELL_SCRIPT] Failed to clone repository we-promise/sure: Clone operation failed
</shell_result>
Script executed:
[skip_cloning]
# Search for documentation or references about has_balance validation in banking integrations
curl -s https://api.github.com/repos/we-promise/sure/contents/app/models/plaid_account.rb 2>/dev/null || echo "API call failed"Repository: we-promise/sure
<shell_result>
Repository: we-promise/sure
Script execution failed with unrecoverable error: [CAPABILITY:RUN_SHELL_SCRIPT] Failed to clone repository we-promise/sure: Clone operation failed
</shell_result>
Premature approval without complete verification; potential missing balance validation.
The review comment contains a structural contradiction: it includes while simultaneously requesting verification via. More critically, the learnings confirm that EnableBankingAccount should implement a has_balance validation method (referenced at lines 9-20), but this validation is absent from the 12-line snippet shown.
The snippet displays only the class header and basic validations (name, currency, uid) but does not show the has_balance validation mentioned in the learnings. Since the learnings reference lines 9-20 of this same file and the snippet only shows lines 1-12, the validation may exist elsewhere in the model, or it may be missing entirely.
The review should not approve the code until:
- Verification confirms whether
has_balancevalidation is present in the full model - If missing, this validation must be added to match the Plaid integration pattern documented in the learnings
| raw_account_id = snapshot[:account_id] | ||
| account_id_data = if raw_account_id.is_a?(Hash) | ||
| raw_account_id | ||
| elsif raw_account_id.is_a?(Array) && raw_account_id.first.is_a?(Hash) | ||
| # If it's an array of hashes, find the one with iban | ||
| raw_account_id.find { |item| item[:iban].present? } || {} | ||
| else | ||
| {} | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract duplicated account_id parsing logic.
The logic for extracting account_id_data from raw_account_id (lines 52-60) is duplicated in build_account_name (lines 92-99). Extract this to a private helper method to adhere to DRY and prevent future inconsistencies.
+ private
+
+ def extract_account_id_data(snapshot)
+ raw_account_id = snapshot[:account_id]
+ if raw_account_id.is_a?(Hash)
+ raw_account_id
+ elsif raw_account_id.is_a?(Array) && raw_account_id.first.is_a?(Hash)
+ raw_account_id.find { |item| item[:iban].present? } || {}
+ else
+ {}
+ end
+ endThen use it in both places:
# In upsert_enable_banking_snapshot!
account_id_data = extract_account_id_data(snapshot)
# In build_account_name
account_id_data = extract_account_id_data(snapshot)Also applies to: 92-99
🤖 Prompt for AI Agents
In app/models/enable_banking_account.rb around lines 52-60 and 92-99, the logic
that extracts account_id_data from snapshot[:account_id] is duplicated; extract
this into a private helper method (e.g. extract_account_id_data(snapshot)) that
returns the hash when snapshot[:account_id] is a Hash, returns the first hash
containing an :iban when it's an Array, or returns {} otherwise, then replace
the existing duplicated blocks in upsert_enable_banking_snapshot! and
build_account_name to call this helper; add the new private method near other
private methods and ensure tests/usage still pass.
| <% if enable_banking_account.current_balance.present? %> | ||
| <p>Balance: <%= number_to_currency(enable_banking_account.current_balance, unit: enable_banking_account.currency) %></p> | ||
| <% end %> |
There was a problem hiding this comment.
Incorrect number_to_currency usage with currency code.
The unit: option expects a currency symbol (e.g., "$", "€"), not a currency code (e.g., "EUR", "USD"). This will display "Balance: EUR100.00" instead of "Balance: €100.00".
Per coding guidelines, monetary values should use Money objects for proper currency conversion and formatting. Consider using a helper that handles currency codes correctly.
- <p>Balance: <%= number_to_currency(enable_banking_account.current_balance, unit: enable_banking_account.currency) %></p>
+ <p>Balance: <%= format_money(Money.new(enable_banking_account.current_balance * 100, enable_banking_account.currency)) %></p>Or verify how other banking integrations format balances in views:
#!/bin/bash
# Check how PlaidAccount balances are formatted in views
rg -n "current_balance|format_money|number_to_currency" --type erb -A2 -B2🤖 Prompt for AI Agents
In app/views/enable_banking_items/setup_accounts.html.erb around lines 73–75,
number_to_currency is being passed a currency code (e.g., "USD"/"EUR") as the
unit which prints "EUR100.00" instead of a symbol; change the view to format
balances via a Money-aware helper or a Money object so the correct currency
symbol and formatting are used. Replace the direct number_to_currency call with
the project’s money formatting helper (or construct a Money.new/monetize value
with the account’s cents and currency code and call its formatting helper),
ensuring you pass the amount in smallest units (cents) and use the helper that
resolves the currency code to the proper symbol and locale-aware formatting.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
app/controllers/enable_banking_items_controller.rb (5)
176-186: Remove Turbo frame data fromredirect_to; it has no effect.
redirect_to select_bank_enable_banking_item_path(new_item), data: { turbo_frame: "modal" }won’t target a Turbo frame—data-turbo-frameonly affects links/forms in the browser, not server-side redirects. Either:
- Drop the
dataoption and rely on the original link/form havingdata-turbo-frame="modal", or- Respond with a Turbo Stream that updates the modal frame instead of redirecting.
Minimal fix:
- redirect_to select_bank_enable_banking_item_path(new_item), data: { turbo_frame: "modal" } + redirect_to select_bank_enable_banking_item_path(new_item)
109-132: Validate external redirect URL before callingredirect_to(Brakeman warning).
redirect_urlhere comes from the Enable Banking API and is used withredirect_to redirect_url, allow_other_host: true, which Brakeman correctly flags as an unprotected redirect. Add a strict allowlist + scheme check and reuse it for bothauthorizeandreauthorize.Example:
+ ALLOWED_REDIRECT_HOSTS = %w[enablebanking.com sandbox.enablebanking.com].freeze + + private + + def validate_redirect_url!(url) + uri = URI.parse(url) + unless uri.is_a?(URI::HTTP) && ALLOWED_REDIRECT_HOSTS.any? { |host| uri.host&.end_with?(host) } + Rails.logger.warn("Rejected Enable Banking redirect URL: #{url}") + raise Provider::EnableBanking::EnableBankingError.new("Invalid redirect URL", :invalid_redirect) + end + url + rescue URI::InvalidURIError + raise Provider::EnableBanking::EnableBankingError.new("Invalid redirect URL", :invalid_redirect) + end- redirect_to redirect_url, allow_other_host: true + redirect_to validate_redirect_url!(redirect_url), allow_other_host: trueAlso apply the same helper in
reauthorize.
188-202: Apply redirect URL validation inreauthorizeas well.Same untrusted
redirect_urlpattern as inauthorize; make sure thevalidate_redirect_url!helper (or equivalent) is used here too so both code paths satisfy Brakeman and your security posture.- redirect_to redirect_url, allow_other_host: true + redirect_to validate_redirect_url!(redirect_url), allow_other_host: true
204-263: Multi-connection semantics and data consistency inlink_accounts.Two related points:
Picking the item with
.firstis ambiguous with multiple connections.
enable_banking_item = Current.family.enable_banking_items.where.not(session_id: nil).firstbecomes non-deterministic once there are multiple active sessions, which undercuts the “multiple connections” goal. Prefer tying this action to a specific item (e.g., member route +before_action :set_enable_banking_item) or passing an explicitenable_banking_item_idparam.Example (if
link_accountsis made a member action):
- before_action :set_enable_banking_item, only: [ :update, :destroy, :sync, :select_bank, :authorize, :reauthorize, :setup_accounts, :complete_account_setup, :new_connection ]
- before_action :set_enable_banking_item, only: [ :update, :destroy, :sync, :select_bank, :authorize, :reauthorize, :setup_accounts, :complete_account_setup, :new_connection, :link_accounts ]
```diff
- enable_banking_item = Current.family.enable_banking_items.where.not(session_id: nil).first
- unless enable_banking_item.present?
- unless @enable_banking_item.session_id.present?
redirect_to settings_providers_path, alert: t(".no_session", default: "No active Enable Banking connection. Please connect a bank first.")
return
end2. **Wrap account creation/linking in a transaction.** If `Account.create_and_sync` or `AccountProvider.create!` fails mid-loop, you’ll end up with partially-created accounts and broken provider links. Wrap `selected_uids.each` in an `ActiveRecord::Base.transaction` and make sure failures raise so the whole operation rolls back. Example: ```diff - selected_uids.each do |uid| + ActiveRecord::Base.transaction do + selected_uids.each do |uid| # existing loop body... - created_accounts << account - end + created_accounts << account + end + end
309-365: Wrap account setup loop in a transaction to avoid partial state.
complete_account_setupcreates accounts andAccountProviderlinks inside a plainaccount_types.eachloop. IfAccount.create_from_enable_banking_accountorAccountProvider.create!raises for any item, some accounts will be created while others are not, leaving inconsistent state.Wrap the loop in a transaction and ensure failures raise:
- account_types.each do |enable_banking_account_id, selected_type| + ActiveRecord::Base.transaction do + account_types.each do |enable_banking_account_id, selected_type| # existing loop body... - created_count += 1 - end + created_count += 1 + end + endIf
create_from_enable_banking_accountcan return invalid records without raising, consider switching to a bang variant or manually raising on failure so the transaction rolls back.
🧹 Nitpick comments (2)
app/controllers/enable_banking_items_controller.rb (2)
39-68: Create/update flows are consistent but could be DRY’d up.
createandupdateduplicate the same turbo_stream panel refresh and error-handling logic. Consider extracting a small private helper (e.g.render_providers_panel(success: ..., error_message: ...)) to keep these actions lean and reduce maintenance surface.
135-174: State handling is functional; consider opaque tokens for defense in depth.Using
stateas the rawenable_banking_item.idand scoping lookup throughCurrent.familyis workable, but it does expose predictable IDs in the OAuth state parameter. If you want to tighten this further, consider issuing a random, opaque state token tied to the item (or session) and validating that instead of a bare id.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/controllers/enable_banking_items_controller.rb(1 hunks)app/views/enable_banking_items/setup_accounts.html.erb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/views/enable_banking_items/setup_accounts.html.erb
🧰 Additional context used
📓 Path-based instructions (9)
**/*.rb
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rb: Application supports two modes: 'managed' and 'self_hosted' via Rails.application.config.app_mode
Use Current.user and Current.family instead of current_user / current_family for authentication context
Optimize database queries with proper indexes to prevent N+1 queries using includes/joins
**/*.rb: Use Rails pluralization in i18n:t("transactions.count", count: @transactions.count).
Runbin/rubocop -f github -afor Ruby linting with auto-correct before pull requests.
**/*.rb: Ruby code should use 2-space indent,snake_casefor methods and variables, andCamelCasefor classes and modules, following Rails conventions for folders and file names
Prefer environment variables over hard-coded values for configuration
Files:
app/controllers/enable_banking_items_controller.rb
app/controllers/**/*.rb
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use Rails strong parameters and CSRF protection throughout the application
app/controllers/**/*.rb: Use strong parameters and CSRF protection throughout the application.
Use query params for state over localStorage/sessions.
Files:
app/controllers/enable_banking_items_controller.rb
**/*.{rb,erb,html.erb}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{rb,erb,html.erb}: UseCurrent.userfor the current user context in controllers and views. Do NOT usecurrent_user.
UseCurrent.familyfor the current family context. Do NOT usecurrent_family.
Files:
app/controllers/enable_banking_items_controller.rb
**/*.{erb,html.erb,rb}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{erb,html.erb,rb}: All user-facing strings must use localization (i18n) via thet()helper. Update locale files for each new or changed element.
Use server-side formatting for currencies, numbers, and dates. Do not format in JavaScript.
Files:
app/controllers/enable_banking_items_controller.rb
**/*.{rb,erb}
📄 CodeRabbit inference engine (.cursor/rules/general-rules.mdc)
**/*.{rb,erb}: UseCurrent.userfor accessing the current user. Do NOT usecurrent_user
UseCurrent.familyfor accessing the current family. Do NOT usecurrent_family
Files:
app/controllers/enable_banking_items_controller.rb
**/*.{rb,js,erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Format currencies, numbers, dates, and other values server-side, then pass to Stimulus controllers for display only
Files:
app/controllers/enable_banking_items_controller.rb
**/*.{rb,html.erb}
📄 CodeRabbit inference engine (.cursor/rules/project-conventions.mdc)
Use ActiveRecord validations for complex validations and business logic. Simple validations may be mirrored in ActiveRecord for form error handling convenience, but prioritize client-side form validation when possible
Files:
app/controllers/enable_banking_items_controller.rb
app/**/*.{rb,js,ts,jsx,tsx,css,scss}
📄 CodeRabbit inference engine (AGENTS.md)
Code should be organized in
app/directory (Rails MVC, services, jobs, mailers, components), with JS inapp/javascript/, and styles/assets inapp/assets/(Tailwind, images, fonts)
Files:
app/controllers/enable_banking_items_controller.rb
app/**/*.rb
📄 CodeRabbit inference engine (AGENTS.md)
Run
bin/brakemansecurity scan before major PRs to check for static analysis of common Rails issues
Files:
app/controllers/enable_banking_items_controller.rb
🧠 Learnings (13)
📓 Common learnings
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: db/migrate/20250825224717_add_enable_banking_domain.rb:3-34
Timestamp: 2025-09-13T12:23:31.324Z
Learning: When suggesting database schema improvements for banking integrations, consider consistency across all similar integrations (Plaid, SimpleFin, Enable Banking) and propose comprehensive changes in a separate PR rather than making changes to individual integrations in isolation.
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:55:43.046Z
Learning: Use Plaid integration for real-time bank account syncing via `PlaidItem`, `Sync` models, and background jobs for data updates.
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/controllers/enable_banking_items_controller.rb:48-62
Timestamp: 2025-09-13T11:27:55.271Z
Learning: The `create_enable_banking_item!` method in `Family::EnableBankingConnectable` uses `find_or_create_by(id: enable_banking_id)` to either find an existing EnableBankingItem or create a new one, so it automatically handles updates to existing items without creating duplicates.
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method in EnableBankingAccount::Processor follows the same pattern as the Plaid integration, using OpenStruct without explicit requires and returning balance values as-is without BigDecimal coercion for consistency across banking integrations.
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account.rb:9-20
Timestamp: 2025-09-13T11:33:54.159Z
Learning: The EnableBankingAccount model uses a `has_balance` validation method that ensures at least one balance value (current_balance or available_balance) is present, similar to the Plaid integration pattern. This means balance values should not be nil and the validation expects actual balance data from the API.
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account.rb:9-20
Timestamp: 2025-09-13T11:33:54.159Z
Learning: The EnableBankingAccount model uses a `has_balance` validation method that ensures at least one balance value (current_balance or available_balance) is present, similar to the Plaid integration pattern. This means balance values should not be nil and the validation expects actual balance data from the API.
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method pattern is consistent across banking integrations in the codebase. Both PlaidAccount::Processor and EnableBankingAccount::Processor use the same approach: `balance = account.current_balance || account.available_balance || 0` followed by `OpenStruct.new(balance: balance, cash_balance: balance)` without explicit requires, and include identical comments about not distinguishing cash vs non-cash balances for non-investment accounts.
📚 Learning: 2025-09-13T11:27:55.271Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/controllers/enable_banking_items_controller.rb:48-62
Timestamp: 2025-09-13T11:27:55.271Z
Learning: The `create_enable_banking_item!` method in `Family::EnableBankingConnectable` uses `find_or_create_by(id: enable_banking_id)` to either find an existing EnableBankingItem or create a new one, so it automatically handles updates to existing items without creating duplicates.
Applied to files:
app/controllers/enable_banking_items_controller.rb
📚 Learning: 2025-09-13T11:36:45.479Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method in EnableBankingAccount::Processor follows the same pattern as the Plaid integration, using OpenStruct without explicit requires and returning balance values as-is without BigDecimal coercion for consistency across banking integrations.
Applied to files:
app/controllers/enable_banking_items_controller.rb
📚 Learning: 2025-11-24T16:55:43.046Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:55:43.046Z
Learning: Use Plaid integration for real-time bank account syncing via `PlaidItem`, `Sync` models, and background jobs for data updates.
Applied to files:
app/controllers/enable_banking_items_controller.rb
📚 Learning: 2025-11-24T16:55:43.046Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:55:43.046Z
Learning: Applies to app/controllers/**/*.rb : Use strong parameters and CSRF protection throughout the application.
Applied to files:
app/controllers/enable_banking_items_controller.rb
📚 Learning: 2025-09-13T11:33:54.159Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account.rb:9-20
Timestamp: 2025-09-13T11:33:54.159Z
Learning: The EnableBankingAccount model uses a `has_balance` validation method that ensures at least one balance value (current_balance or available_balance) is present, similar to the Plaid integration pattern. This means balance values should not be nil and the validation expects actual balance data from the API.
Applied to files:
app/controllers/enable_banking_items_controller.rb
📚 Learning: 2025-09-13T11:36:45.479Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: app/models/enable_banking_account/processor.rb:72-79
Timestamp: 2025-09-13T11:36:45.479Z
Learning: The balance_calculator method pattern is consistent across banking integrations in the codebase. Both PlaidAccount::Processor and EnableBankingAccount::Processor use the same approach: `balance = account.current_balance || account.available_balance || 0` followed by `OpenStruct.new(balance: balance, cash_balance: balance)` without explicit requires, and include identical comments about not distinguishing cash vs non-cash balances for non-investment accounts.
Applied to files:
app/controllers/enable_banking_items_controller.rb
📚 Learning: 2025-11-24T16:54:59.185Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T16:54:59.185Z
Learning: Applies to app/views/**/*.erb : Use Turbo Frames for page sections instead of client-side solutions
Applied to files:
app/controllers/enable_banking_items_controller.rb
📚 Learning: 2025-11-24T16:55:43.046Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:55:43.046Z
Learning: Applies to **/*.{erb,html.erb} : Leverage Turbo frames for page sections over client-side solutions.
Applied to files:
app/controllers/enable_banking_items_controller.rb
📚 Learning: 2025-11-24T16:56:13.394Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .cursor/rules/project-conventions.mdc:0-0
Timestamp: 2025-11-24T16:56:13.394Z
Learning: Applies to **/*.html.erb : Leverage Turbo frames to break up pages instead of JavaScript-driven client-side solutions
Applied to files:
app/controllers/enable_banking_items_controller.rb
📚 Learning: 2025-11-24T16:54:59.185Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T16:54:59.185Z
Learning: Applies to {app/**/*.{erb,html},app/javascript/**/*.js} : Prefer Hotwire-first approach: use Turbo + Stimulus for reactive UI without heavy JavaScript
Applied to files:
app/controllers/enable_banking_items_controller.rb
📚 Learning: 2025-11-24T16:55:43.046Z
Learnt from: CR
Repo: we-promise/sure PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-24T16:55:43.046Z
Learning: Applies to **/*.{erb,html.erb} : Pass data to Stimulus controllers via `data-*-value` attributes, not inline JavaScript.
Applied to files:
app/controllers/enable_banking_items_controller.rb
📚 Learning: 2025-09-13T12:23:31.324Z
Learnt from: alessiocappa
Repo: we-promise/sure PR: 141
File: db/migrate/20250825224717_add_enable_banking_domain.rb:3-34
Timestamp: 2025-09-13T12:23:31.324Z
Learning: When suggesting database schema improvements for banking integrations, consider consistency across all similar integrations (Plaid, SimpleFin, Enable Banking) and propose comprehensive changes in a separate PR rather than making changes to individual integrations in isolation.
Applied to files:
app/controllers/enable_banking_items_controller.rb
🪛 GitHub Actions: Pull Request
app/controllers/enable_banking_items_controller.rb
[warning] 124-124: Brakeman warning: Possible unprotected redirect. Code: redirect_to(Current.family.enable_banking_items.find(params[:id]).start_authorization(:aspsp_name => params[:aspsp_name], :redirect_url => enable_banking_callback_url, :state => Current.family.enable_banking_items.find(params[:id]).id), :allow_other_host => true)
[warning] 197-197: Brakeman warning: Possible unprotected redirect. Code: redirect_to(Current.family.enable_banking_items.find(params[:id]).start_authorization(:aspsp_name => Current.family.enable_banking_items.find(params[:id]).aspsp_name, :redirect_url => enable_banking_callback_url, :state => Current.family.enable_banking_items.find(params[:id]).id), :allow_other_host => true)
🔇 Additional comments (3)
app/controllers/enable_banking_items_controller.rb (3)
76-85: Sync trigger behavior looks correct.Guarding on
syncing?before callingsync_laterkeeps the action idempotent-ish and therespond_toblock covers HTML/JSON cleanly.
87-106: Bank selection flow and error handling look solid.
credentials_configured?guard, API error rescue with logging, and always assigning@aspsps(falling back to[]) make the modal predictable for the view.
369-390: Strong params and callback URL helper look aligned with conventions.
enable_banking_item_paramspermits only the expected attributes, andenable_banking_callback_urlcleanly separates production vs development (viaDEV_WEBHOOKS_URL), which should play well with tools like ngrok.
| def create | ||
| @enable_banking_item = Current.family.enable_banking_items.build(enable_banking_item_params) | ||
| @enable_banking_item.name ||= "Enable Banking Connection" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Localize default connection name instead of hard‑coding.
Line 7 and the new_connection action (line 179) set a user-facing name "Enable Banking Connection" directly. This should go through I18n to follow project guidelines and allow translation.
Example:
- @enable_banking_item.name ||= "Enable Banking Connection"
+ @enable_banking_item.name ||= t(".default_name", default: "Enable Banking Connection")- name: "Enable Banking Connection",
+ name: t(".default_name", default: "Enable Banking Connection"),Also applies to: 176-183
| # Show setup accounts modal | ||
| def setup_accounts | ||
| @enable_banking_accounts = @enable_banking_item.enable_banking_accounts | ||
| .left_joins(:account_provider) | ||
| .where(account_providers: { id: nil }) | ||
|
|
||
| @account_type_options = [ | ||
| [ "Skip this account", "skip" ], | ||
| [ "Checking or Savings Account", "Depository" ], | ||
| [ "Credit Card", "CreditCard" ], | ||
| [ "Investment Account", "Investment" ], | ||
| [ "Loan or Mortgage", "Loan" ], | ||
| [ "Other Asset", "OtherAsset" ] | ||
| ] | ||
|
|
||
| @subtype_options = { | ||
| "Depository" => { | ||
| label: "Account Subtype:", | ||
| options: Depository::SUBTYPES.map { |k, v| [ v[:long], k ] } | ||
| }, | ||
| "CreditCard" => { | ||
| label: "", | ||
| options: [], | ||
| message: "Credit cards will be automatically set up as credit card accounts." | ||
| }, | ||
| "Investment" => { | ||
| label: "Investment Type:", | ||
| options: Investment::SUBTYPES.map { |k, v| [ v[:long], k ] } | ||
| }, | ||
| "Loan" => { | ||
| label: "Loan Type:", | ||
| options: Loan::SUBTYPES.map { |k, v| [ v[:long], k ] } | ||
| }, | ||
| "OtherAsset" => { | ||
| label: nil, | ||
| options: [], | ||
| message: "Other assets will be set up as general assets." | ||
| } | ||
| } | ||
|
|
||
| render layout: false | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Localize all account type/subtype labels and messages.
@account_type_options and @subtype_options currently embed user-facing English strings directly. These should go through I18n so they can be translated and adjusted consistently.
Example sketch:
- @account_type_options = [
- [ "Skip this account", "skip" ],
- [ "Checking or Savings Account", "Depository" ],
- [ "Credit Card", "CreditCard" ],
- [ "Investment Account", "Investment" ],
- [ "Loan or Mortgage", "Loan" ],
- [ "Other Asset", "OtherAsset" ]
- ]
+ @account_type_options = [
+ [ t(".account_types.skip", default: "Skip this account"), "skip" ],
+ [ t(".account_types.depository", default: "Checking or Savings Account"), "Depository" ],
+ [ t(".account_types.credit", default: "Credit Card"), "CreditCard" ],
+ [ t(".account_types.investment", default: "Investment Account"), "Investment" ],
+ [ t(".account_types.loan", default: "Loan or Mortgage"), "Loan" ],
+ [ t(".account_types.other", default: "Other Asset"), "OtherAsset" ]
+ ]Similarly for label and message entries in @subtype_options.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/controllers/enable_banking_items_controller.rb around lines 265 to 306,
the account type and subtype labels/messages are hard-coded in English; replace
these literal strings with I18n lookups (e.g.
I18n.t('enable_banking.account_types.skip') etc.) for every user-facing string
in @account_type_options and every label/message and displayed option text
inside @subtype_options, and update the option value arrays to map translated
display text to the existing keys; ensure keys are namespaced and add
corresponding entries to the locale yml files so all labels, option texts and
messages are localizable.
| value: @enable_banking_item.sync_start_date || 3.months.ago.to_date, | ||
| min: 1.year.ago.to_date, | ||
| max: Date.current, | ||
| class: "w-full max-w-xs", |
There was a problem hiding this comment.
I noticed that this field is not properly displayed in dark mode. I've fixed it by using the following classes here: w-full max-w-xs rounded-md border border-primary px-3 py-2 text-sm bg-container-inset text-primary", which are the same used somewhere else in another date field.
| label: "Client Certificate (with Private Key)", | ||
| placeholder: "-----BEGIN PRIVATE KEY-----\n...\n-----END PRIVATE KEY-----\n-----BEGIN CERTIFICATE-----\n...\n-----END CERTIFICATE-----", | ||
| rows: 6, | ||
| class: "font-mono text-xs", |
There was a problem hiding this comment.
Also this one is not visible in dark mode. I've fixed it by using the following classes: form-field__input font-mono text-xs
| ["Sweden (SE)", "SE"], | ||
| ["United Kingdom (GB)", "GB"] | ||
| ], enable_banking_item.country_code), | ||
| { include_blank: "Select country..." }, |
There was a problem hiding this comment.
The label wasn’t visible on this field so I suppose the property label: true should be added. Setting it like this: { label: true, include_blank: “Select country…” } works perfectly.
| ["United Kingdom (GB)", "GB"] | ||
| ], enable_banking_item.country_code), | ||
| { include_blank: "Select country..." }, | ||
| { label: "Country", class: "form-select", disabled: has_authenticated_connections && !is_new_record } %> |
There was a problem hiding this comment.
Using the form-field__input class instead of form-select ensures the field is displayed correctly and aligned with the rest of the elements.
| iban = account_id_data[:iban] || snapshot[:iban] | ||
|
|
||
| if snapshot[:name].present? | ||
| snapshot[:name] |
There was a problem hiding this comment.
The name field actually stores the account holder’s name while the “account name” is stored in product at least in my real account. I’m not sure whether it’s worth using the value of product as the name or a combination of both. It certainly provides more information about the account than just the holder’s name; this is at least what I’ve implemented.
There was a problem hiding this comment.
on the sandbox, this is the only available field in the api. annoying
"name": "Rabobank Nederland B.V.",
"usage": null,
"details": null,
"product": null,
|
@sokie, thanks a lot for this fantastic work! I really appreciate it! 🙏🏻 I quickly tested it using a fake account and my real one and everything seems to be working well so far. I also liked the extra setup step where you can confirm or update the mapping provided by Enable Banking. This is something I wanted to do too and it’s great to see it in your PR. I’ve left a few comments about data mapping and some small UI fixes. Let me know if you agree with those suggestions. |
3 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.