diff --git a/app/controllers/simplefin_items_controller.rb b/app/controllers/simplefin_items_controller.rb index 03a5322de..b3e02a6be 100644 --- a/app/controllers/simplefin_items_controller.rb +++ b/app/controllers/simplefin_items_controller.rb @@ -21,43 +21,21 @@ class SimplefinItemsController < ApplicationController return render_error(t(".errors.blank_token"), context: :edit) if setup_token.blank? begin - # Create new SimpleFin item data with updated token - updated_item = Current.family.create_simplefin_item!( - setup_token: setup_token, - item_name: @simplefin_item.name + # Validate token shape early so the user gets immediate feedback. + claim_url = Base64.decode64(setup_token) + URI.parse(claim_url) + + # Updating a SimpleFin connection can involve network retries/backoff and account import. + # Do it asynchronously so web requests aren't blocked by retry sleeps. + SimplefinConnectionUpdateJob.perform_later( + family_id: Current.family.id, + old_simplefin_item_id: @simplefin_item.id, + setup_token: setup_token ) - # Ensure new simplefin_accounts are created & have account_id set - updated_item.import_latest_simplefin_data - - # Transfer accounts from old item to new item - ActiveRecord::Base.transaction do - @simplefin_item.simplefin_accounts.each do |old_account| - if old_account.account.present? - # Find matching account in new item by account_id - new_account = updated_item.simplefin_accounts.find_by(account_id: old_account.account_id) - if new_account - # Transfer the account directly to the new SimpleFin account - # This will automatically break the old association - old_account.account.update!(simplefin_account_id: new_account.id) - end - end - end - - # Mark old item for deletion - @simplefin_item.destroy_later - end - - # Clear any requires_update status on new item - updated_item.update!(status: :good) - if turbo_frame_request? - @simplefin_items = Current.family.simplefin_items.ordered - render turbo_stream: turbo_stream.replace( - "simplefin-providers-panel", - partial: "settings/providers/simplefin_panel", - locals: { simplefin_items: @simplefin_items } - ) + flash.now[:notice] = t(".success") + render turbo_stream: Array(flash_notification_stream_items) else redirect_to accounts_path, notice: t(".success"), status: :see_other end @@ -157,12 +135,16 @@ class SimplefinItemsController < ApplicationController end def setup_accounts - @simplefin_accounts = @simplefin_item.simplefin_accounts.includes(:account).where(accounts: { id: nil }) + # Only show unlinked accounts - check both legacy FK and AccountProvider + @simplefin_accounts = @simplefin_item.simplefin_accounts + .left_joins(:account, :account_provider) + .where(accounts: { id: nil }, account_providers: { id: nil }) @account_type_options = [ [ "Skip this account", "skip" ], [ "Checking or Savings Account", "Depository" ], [ "Credit Card", "CreditCard" ], [ "Investment Account", "Investment" ], + [ "Crypto Account", "Crypto" ], [ "Loan or Mortgage", "Loan" ], [ "Other Asset", "OtherAsset" ] ] @@ -208,6 +190,11 @@ class SimplefinItemsController < ApplicationController label: "Loan Type:", options: Loan::SUBTYPES.map { |k, v| [ v[:long], k ] } }, + "Crypto" => { + label: nil, + options: [], + message: "Crypto accounts track cryptocurrency holdings." + }, "OtherAsset" => { label: nil, options: [], @@ -225,8 +212,8 @@ class SimplefinItemsController < ApplicationController @simplefin_item.update!(sync_start_date: params[:sync_start_date]) end - # Valid account types for this provider (plus OtherAsset which SimpleFIN UI allows) - valid_types = Provider::SimplefinAdapter.supported_account_types + [ "OtherAsset" ] + # Valid account types for this provider (plus Crypto and OtherAsset which SimpleFIN UI allows) + valid_types = Provider::SimplefinAdapter.supported_account_types + [ "Crypto", "OtherAsset" ] created_accounts = [] skipped_count = 0 @@ -269,6 +256,8 @@ class SimplefinItemsController < ApplicationController selected_subtype ) simplefin_account.update!(account: account) + # Also create AccountProvider for consistency with the new linking system + simplefin_account.ensure_account_provider! created_accounts << account end diff --git a/app/javascript/controllers/account_type_selector_controller.js b/app/javascript/controllers/account_type_selector_controller.js index 1f794ef0e..4504c41b7 100644 --- a/app/javascript/controllers/account_type_selector_controller.js +++ b/app/javascript/controllers/account_type_selector_controller.js @@ -42,4 +42,27 @@ export default class extends Controller { } } } + + clearWarning(event) { + // When user selects a subtype value, clear all warning styling + const select = event.target + if (select.value) { + // Clear the subtype dropdown warning + const warningContainer = select.closest('.ring-2') + if (warningContainer) { + warningContainer.classList.remove('ring-2', 'ring-warning/50', 'rounded-md', 'p-2', '-m-2') + const warningText = warningContainer.querySelector('.text-warning') + if (warningText) { + warningText.remove() + } + } + + // Clear the parent card's warning border + const card = this.element.closest('.border-2.border-warning') + if (card) { + card.classList.remove('border-2', 'border-warning', 'bg-warning/5') + card.classList.add('border', 'border-primary') + } + } + } } \ No newline at end of file diff --git a/app/jobs/simplefin_connection_update_job.rb b/app/jobs/simplefin_connection_update_job.rb new file mode 100644 index 000000000..850eaac8b --- /dev/null +++ b/app/jobs/simplefin_connection_update_job.rb @@ -0,0 +1,167 @@ +class SimplefinConnectionUpdateJob < ApplicationJob + queue_as :high_priority + + # Disable automatic retries for this job since the setup token is single-use. + # If the token claim succeeds but import fails, retrying would fail at claim. + discard_on Provider::Simplefin::SimplefinError do |job, error| + Rails.logger.error( + "SimplefinConnectionUpdateJob discarded: #{error.class} - #{error.message} " \ + "(family_id=#{job.arguments.first[:family_id]}, old_item_id=#{job.arguments.first[:old_simplefin_item_id]})" + ) + end + + def perform(family_id:, old_simplefin_item_id:, setup_token:) + family = Family.find(family_id) + old_item = family.simplefin_items.find(old_simplefin_item_id) + + # Step 1: Claim the token and create the new item. + # This is the critical step - if it fails, we can safely retry. + # If it succeeds, the token is consumed and we must not retry the claim. + updated_item = family.create_simplefin_item!( + setup_token: setup_token, + item_name: old_item.name + ) + + # Step 2: Import accounts from SimpleFin. + # If this fails, we have an orphaned item but the token is already consumed. + # We handle this gracefully by marking the item and continuing. + begin + updated_item.import_latest_simplefin_data + rescue => e + Rails.logger.error( + "SimplefinConnectionUpdateJob: import failed for new item #{updated_item.id}: " \ + "#{e.class} - #{e.message}. Item created but may need manual sync." + ) + # Mark the item as needing attention but don't fail the job entirely. + # The item exists and can be synced manually later. + updated_item.update!(status: :requires_update) + # Still proceed to transfer accounts and schedule old item deletion + end + + # Step 3: Transfer account links from old to new item. + # This is idempotent and safe to retry. + # Check for linked accounts via BOTH legacy FK and AccountProvider. + ActiveRecord::Base.transaction do + old_item.simplefin_accounts.includes(:account, account_provider: :account).each do |old_account| + # Get the linked account via either system + linked_account = old_account.current_account + next unless linked_account.present? + + new_simplefin_account = find_matching_simplefin_account(old_account, updated_item.simplefin_accounts) + next unless new_simplefin_account + + # Update legacy FK + linked_account.update!(simplefin_account_id: new_simplefin_account.id) + + # Also migrate AccountProvider if it exists + if old_account.account_provider.present? + old_account.account_provider.update!( + provider_type: "SimplefinAccount", + provider_id: new_simplefin_account.id + ) + else + # Create AccountProvider for consistency + new_simplefin_account.ensure_account_provider! + end + end + end + + # Schedule deletion outside transaction to avoid race condition where + # the job is enqueued even if the transaction rolls back + old_item.destroy_later + + # Only mark as good if import succeeded (status wasn't set to requires_update above) + updated_item.update!(status: :good) unless updated_item.requires_update? + end + + private + # Find a matching SimpleFin account in the new item's accounts. + # Uses a multi-tier matching strategy: + # 1. Exact account_id match (preferred) + # 2. Fingerprint match (name + institution + account_type) + # 3. Fuzzy name match with same institution (fallback) + def find_matching_simplefin_account(old_account, new_accounts) + exact_match = new_accounts.find_by(account_id: old_account.account_id) + return exact_match if exact_match + + old_fingerprint = account_fingerprint(old_account) + fingerprint_match = new_accounts.find { |new_account| account_fingerprint(new_account) == old_fingerprint } + return fingerprint_match if fingerprint_match + + old_institution = extract_institution_id(old_account) + old_name_normalized = normalize_account_name(old_account.name) + + new_accounts.find do |new_account| + new_institution = extract_institution_id(new_account) + new_name_normalized = normalize_account_name(new_account.name) + + next false unless old_institution.present? && old_institution == new_institution + + names_similar?(old_name_normalized, new_name_normalized) + end + end + + def account_fingerprint(simplefin_account) + institution_id = extract_institution_id(simplefin_account) + name_normalized = normalize_account_name(simplefin_account.name) + account_type = simplefin_account.account_type.to_s.downcase + + "#{institution_id}:#{name_normalized}:#{account_type}" + end + + def extract_institution_id(simplefin_account) + org_data = simplefin_account.org_data + return nil unless org_data.is_a?(Hash) + + org_data["id"] || org_data["domain"] || org_data["name"]&.downcase&.gsub(/\s+/, "_") + end + + def normalize_account_name(name) + return "" if name.blank? + + name.to_s + .downcase + .gsub(/[^a-z0-9]/, "") + end + + def names_similar?(name1, name2) + return false if name1.blank? || name2.blank? + + return true if name1 == name2 + return true if name1.include?(name2) || name2.include?(name1) + + longer = [ name1.length, name2.length ].max + return false if longer == 0 + + # Use Levenshtein distance for more accurate similarity + distance = levenshtein_distance(name1, name2) + similarity = 1.0 - (distance.to_f / longer) + similarity >= 0.8 + end + + # Compute Levenshtein edit distance between two strings + def levenshtein_distance(s1, s2) + m, n = s1.length, s2.length + return n if m.zero? + return m if n.zero? + + # Use a single array and update in place for memory efficiency + prev_row = (0..n).to_a + curr_row = [] + + (1..m).each do |i| + curr_row[0] = i + (1..n).each do |j| + cost = s1[i - 1] == s2[j - 1] ? 0 : 1 + curr_row[j] = [ + prev_row[j] + 1, # deletion + curr_row[j - 1] + 1, # insertion + prev_row[j - 1] + cost # substitution + ].min + end + prev_row, curr_row = curr_row, prev_row + end + + prev_row[n] + end +end diff --git a/app/models/account/provider_import_adapter.rb b/app/models/account/provider_import_adapter.rb index 83f483fac..30d5c93f4 100644 --- a/app/models/account/provider_import_adapter.rb +++ b/app/models/account/provider_import_adapter.rb @@ -75,6 +75,7 @@ class Account::ProviderImportAdapter existing = entry.transaction.extra || {} incoming = extra.is_a?(Hash) ? extra.deep_stringify_keys : {} entry.transaction.extra = existing.deep_merge(incoming) + entry.transaction.save! end entry.save! entry diff --git a/app/models/account_provider.rb b/app/models/account_provider.rb index bfb39f508..5cc8dbc70 100644 --- a/app/models/account_provider.rb +++ b/app/models/account_provider.rb @@ -2,6 +2,8 @@ class AccountProvider < ApplicationRecord belongs_to :account belongs_to :provider, polymorphic: true + has_many :holdings, dependent: :nullify + validates :account_id, uniqueness: { scope: :provider_type } validates :provider_id, uniqueness: { scope: :provider_type } diff --git a/app/models/provider/simplefin.rb b/app/models/provider/simplefin.rb index 8fff2a297..5ceb2ecad 100644 --- a/app/models/provider/simplefin.rb +++ b/app/models/provider/simplefin.rb @@ -9,6 +9,22 @@ class Provider::Simplefin headers "User-Agent" => "Sure Finance SimpleFin Client" default_options.merge!(verify: true, ssl_verify_mode: OpenSSL::SSL::VERIFY_PEER, timeout: 120) + # Retry configuration for transient network failures + MAX_RETRIES = 3 + INITIAL_RETRY_DELAY = 2 # seconds + MAX_RETRY_DELAY = 30 # seconds + + # Errors that are safe to retry (transient network issues) + RETRYABLE_ERRORS = [ + SocketError, + Net::OpenTimeout, + Net::ReadTimeout, + Errno::ECONNRESET, + Errno::ECONNREFUSED, + Errno::ETIMEDOUT, + EOFError + ].freeze + def initialize end @@ -16,7 +32,11 @@ class Provider::Simplefin # Decode the base64 setup token to get the claim URL claim_url = Base64.decode64(setup_token) - response = HTTParty.post(claim_url) + # Use retry logic for transient network failures during token claim + # Claim should be fast; keep request-path latency bounded. + response = with_retries("POST /claim", max_retries: 1, sleep: false) do + HTTParty.post(claim_url, timeout: 15) + end case response.code when 200 @@ -49,19 +69,12 @@ class Provider::Simplefin accounts_url = "#{access_url}/accounts" accounts_url += "?#{URI.encode_www_form(query_params)}" unless query_params.empty? - # The access URL already contains HTTP Basic Auth credentials - begin - response = HTTParty.get(accounts_url) - rescue SocketError, Net::OpenTimeout, Net::ReadTimeout => e - Rails.logger.error "SimpleFin API: GET /accounts failed: #{e.class}: #{e.message}" - raise SimplefinError.new("Exception during GET request: #{e.message}", :request_failed) - rescue => e - Rails.logger.error "SimpleFin API: Unexpected error during GET /accounts: #{e.class}: #{e.message}" - raise SimplefinError.new("Exception during GET request: #{e.message}", :request_failed) + # Use retry logic with exponential backoff for transient network failures + response = with_retries("GET /accounts") do + HTTParty.get(accounts_url) end - case response.code when 200 JSON.parse(response.body, symbolize_names: true) @@ -72,6 +85,12 @@ class Provider::Simplefin raise SimplefinError.new("Access URL is no longer valid", :access_forbidden) when 402 raise SimplefinError.new("Payment required to access this account", :payment_required) + when 429 + Rails.logger.warn "SimpleFin API: Rate limited - #{response.body}" + raise SimplefinError.new("SimpleFin rate limit exceeded. Please try again later.", :rate_limited) + when 500..599 + Rails.logger.error "SimpleFin API: Server error - Code: #{response.code}, Body: #{response.body}" + raise SimplefinError.new("SimpleFin server error (#{response.code}). Please try again later.", :server_error) else Rails.logger.error "SimpleFin API: Unexpected response - Code: #{response.code}, Body: #{response.body}" raise SimplefinError.new("Failed to fetch accounts: #{response.code} #{response.message} - #{response.body}", :fetch_failed) @@ -97,4 +116,55 @@ class Provider::Simplefin @error_type = error_type end end + + private + + # Execute a block with retry logic and exponential backoff for transient network errors. + # This helps handle temporary network issues that cause autosync failures while + # manual sync (with user retry) succeeds. + def with_retries(operation_name, max_retries: MAX_RETRIES, sleep: true) + retries = 0 + + begin + yield + rescue *RETRYABLE_ERRORS => e + retries += 1 + + if retries <= max_retries + delay = calculate_retry_delay(retries) + Rails.logger.warn( + "SimpleFin API: #{operation_name} failed (attempt #{retries}/#{max_retries}): " \ + "#{e.class}: #{e.message}. Retrying in #{delay}s..." + ) + Kernel.sleep(delay) if sleep && delay.to_f.positive? + retry + else + Rails.logger.error( + "SimpleFin API: #{operation_name} failed after #{max_retries} retries: " \ + "#{e.class}: #{e.message}" + ) + raise SimplefinError.new( + "Network error after #{max_retries} retries: #{e.message}", + :network_error + ) + end + rescue SimplefinError => e + # Preserve original error type and message. + raise + rescue => e + # Non-retryable errors are logged and re-raised immediately + Rails.logger.error "SimpleFin API: #{operation_name} failed with non-retryable error: #{e.class}: #{e.message}" + raise SimplefinError.new("Exception during #{operation_name}: #{e.message}", :request_failed) + end + end + + # Calculate delay with exponential backoff and jitter + def calculate_retry_delay(retry_count) + # Exponential backoff: 2^retry * initial_delay + base_delay = INITIAL_RETRY_DELAY * (2 ** (retry_count - 1)) + # Add jitter (0-25% of base delay) to prevent thundering herd + jitter = base_delay * rand * 0.25 + # Cap at max delay + [ base_delay + jitter, MAX_RETRY_DELAY ].min + end end diff --git a/app/models/simplefin/account_type_mapper.rb b/app/models/simplefin/account_type_mapper.rb index e01f4adb0..18eb8cbcf 100644 --- a/app/models/simplefin/account_type_mapper.rb +++ b/app/models/simplefin/account_type_mapper.rb @@ -12,6 +12,11 @@ module Simplefin CREDIT_NAME_KEYWORDS = /\b(credit|card)\b/i.freeze CREDIT_BRAND_KEYWORDS = /\b(visa|mastercard|amex|american express|discover|apple card|freedom unlimited|quicksilver)\b/i.freeze LOAN_KEYWORDS = /\b(loan|mortgage|heloc|line of credit|loc)\b/i.freeze + CHECKING_KEYWORDS = /\b(checking|chequing|dda|demand deposit)\b/i.freeze + SAVINGS_KEYWORDS = /\b(savings|sav|money market|mma|high.yield)\b/i.freeze + CRYPTO_KEYWORDS = /\b(bitcoin|btc|ethereum|eth|crypto|cryptocurrency|litecoin|dogecoin|solana)\b/i.freeze + # "Cash" as a standalone name (not "cash back", "cash rewards", etc.) + CASH_ACCOUNT_PATTERN = /\A\s*cash\s*\z/i.freeze # Explicit investment subtype tokens mapped to known SUBTYPES keys EXPLICIT_INVESTMENT_TOKENS = { @@ -53,18 +58,23 @@ module Simplefin end end - # 1) Holdings present => Investment (high confidence) + # 1) Crypto keywords → Crypto account (check before holdings since crypto accounts may have holdings) + if CRYPTO_KEYWORDS.match?(nm) + return Inference.new(accountable_type: "Crypto", confidence: :high) + end + + # 2) Holdings present => Investment (high confidence) if holdings_present # Do not guess generic retirement; explicit tokens handled above return Inference.new(accountable_type: "Investment", subtype: nil, confidence: :high) end - # 2) Name suggests LOAN (high confidence) + # 3) Name suggests LOAN (high confidence) if LOAN_KEYWORDS.match?(nm) return Inference.new(accountable_type: "Loan", confidence: :high) end - # 3) Credit card signals + # 4) Credit card signals # - Name contains credit/card (medium to high) # - Card brands (Visa/Mastercard/Amex/Discover/Apple Card) → high # - Or negative balance with available-balance present (medium) @@ -76,14 +86,26 @@ module Simplefin return Inference.new(accountable_type: "CreditCard", confidence: :high) end - # 4) Retirement keywords without holdings still point to Investment (retirement) + # 5) Retirement keywords without holdings still point to Investment (retirement) if RETIREMENT_KEYWORDS.match?(nm) # If the name contains 'brokerage', avoid forcing retirement subtype subtype = BROKERAGE_KEYWORD.match?(nm) ? nil : "retirement" return Inference.new(accountable_type: "Investment", subtype: subtype, confidence: :high) end - # 5) Default + # 6) Checking/Savings/Cash accounts (high confidence when name explicitly says so) + if CHECKING_KEYWORDS.match?(nm) + return Inference.new(accountable_type: "Depository", subtype: "checking", confidence: :high) + end + if SAVINGS_KEYWORDS.match?(nm) + return Inference.new(accountable_type: "Depository", subtype: "savings", confidence: :high) + end + # "Cash" as a standalone account name (like Cash App's "Cash" account) → checking + if CASH_ACCOUNT_PATTERN.match?(nm) + return Inference.new(accountable_type: "Depository", subtype: "checking", confidence: :high) + end + + # 7) Default - unknown account type, let user decide Inference.new(accountable_type: "Depository", confidence: :low) end end diff --git a/app/models/simplefin_account.rb b/app/models/simplefin_account.rb index 3961bea63..d505d41e9 100644 --- a/app/models/simplefin_account.rb +++ b/app/models/simplefin_account.rb @@ -80,7 +80,7 @@ class SimplefinAccount < ApplicationRecord end def parse_currency(currency_value) - return "USD" if currency_value.nil? + return "USD" if currency_value.blank? # SimpleFin currency can be a 3-letter code or a URL for custom currencies if currency_value.start_with?("http") diff --git a/app/models/simplefin_account/investments/holdings_processor.rb b/app/models/simplefin_account/investments/holdings_processor.rb index ae7723206..052c28cb5 100644 --- a/app/models/simplefin_account/investments/holdings_processor.rb +++ b/app/models/simplefin_account/investments/holdings_processor.rb @@ -9,13 +9,29 @@ class SimplefinAccount::Investments::HoldingsProcessor holdings_data.each do |simplefin_holding| begin - symbol = simplefin_holding["symbol"] + symbol = simplefin_holding["symbol"].presence holding_id = simplefin_holding["id"] + description = simplefin_holding["description"].to_s.strip Rails.logger.debug({ event: "simplefin.holding.start", sfa_id: simplefin_account.id, account_id: account&.id, id: holding_id, symbol: symbol, raw: simplefin_holding }.to_json) - unless symbol.present? && holding_id.present? - Rails.logger.debug({ event: "simplefin.holding.skip", reason: "missing_symbol_or_id", id: holding_id, symbol: symbol }.to_json) + unless holding_id.present? + Rails.logger.debug({ event: "simplefin.holding.skip", reason: "missing_id", id: holding_id, symbol: symbol }.to_json) + next + end + + # If symbol is missing but we have a description, create a synthetic ticker + # This allows tracking holdings like 401k funds that don't have standard symbols + # Append a hash suffix to ensure uniqueness for similar descriptions + if symbol.blank? && description.present? + normalized = description.gsub(/[^a-zA-Z0-9]/, "_").upcase.truncate(24, omission: "") + hash_suffix = Digest::MD5.hexdigest(description)[0..4].upcase + symbol = "CUSTOM:#{normalized}_#{hash_suffix}" + Rails.logger.info("SimpleFin: using synthetic ticker #{symbol} for holding #{holding_id} (#{description})") + end + + unless symbol.present? + Rails.logger.debug({ event: "simplefin.holding.skip", reason: "no_symbol_or_description", id: holding_id }.to_json) next end @@ -57,7 +73,7 @@ class SimplefinAccount::Investments::HoldingsProcessor security: security, quantity: qty, amount: computed_amount, - currency: simplefin_holding["currency"] || "USD", + currency: simplefin_holding["currency"].presence || "USD", date: holding_date, price: price, cost_basis: cost_basis, @@ -101,14 +117,23 @@ class SimplefinAccount::Investments::HoldingsProcessor if !sym.include?(":") && (is_crypto_account || is_crypto_symbol || mentions_crypto) sym = "CRYPTO:#{sym}" end + + # Custom tickers (from holdings without symbols) should always be offline + is_custom = sym.start_with?("CUSTOM:") + # Use Security::Resolver to find or create the security, but be resilient begin + if is_custom + # Skip resolver for custom tickers - create offline security directly + raise "Custom ticker - skipping resolver" + end Security::Resolver.new(sym).resolve rescue => e # If provider search fails or any unexpected error occurs, fall back to an offline security - Rails.logger.warn "SimpleFin: resolver failed for symbol=#{sym}: #{e.class} - #{e.message}; falling back to offline security" + Rails.logger.warn "SimpleFin: resolver failed for symbol=#{sym}: #{e.class} - #{e.message}; falling back to offline security" unless is_custom Security.find_or_initialize_by(ticker: sym).tap do |sec| sec.offline = true if sec.respond_to?(:offline) && sec.offline != true + sec.name = description.presence if sec.name.blank? && description.present? sec.save! if sec.changed? end end diff --git a/app/models/simplefin_entry/processor.rb b/app/models/simplefin_entry/processor.rb index de0802c90..4ef68449e 100644 --- a/app/models/simplefin_entry/processor.rb +++ b/app/models/simplefin_entry/processor.rb @@ -34,12 +34,13 @@ class SimplefinEntry::Processor # Include provider-supplied extra hash if present sf["extra"] = data[:extra] if data[:extra].is_a?(Hash) - # Pending detection: honor provider flag or infer from missing/zero posted with present transacted_at - posted_val = data[:posted] - posted_missing = posted_val.blank? || posted_val == 0 || posted_val == "0" - if ActiveModel::Type::Boolean.new.cast(data[:pending]) || (posted_missing && data[:transacted_at].present?) + # Pending detection: only use explicit provider flag + # We always set the key (true or false) to ensure deep_merge overwrites any stale value + if ActiveModel::Type::Boolean.new.cast(data[:pending]) sf["pending"] = true Rails.logger.debug("SimpleFIN: flagged pending transaction #{external_id}") + else + sf["pending"] = false end # FX metadata: when tx currency differs from account currency @@ -87,7 +88,7 @@ class SimplefinEntry::Processor elsif description.present? description else - data[:memo] || "Unknown transaction" + data[:memo] || I18n.t("transactions.unknown_name") end end @@ -142,7 +143,7 @@ class SimplefinEntry::Processor def posted_date val = data[:posted] - # Treat 0 / "0" as missing to avoid Unix epoch 1970-01-01 for pendings + # Treat 0 / "0" as missing to avoid Unix epoch 1970-01-01 return nil if val == 0 || val == "0" Simplefin::DateUtils.parse_provider_date(val) end diff --git a/app/models/simplefin_item.rb b/app/models/simplefin_item.rb index cfad9c19d..15d2ea2d5 100644 --- a/app/models/simplefin_item.rb +++ b/app/models/simplefin_item.rb @@ -56,7 +56,10 @@ class SimplefinItem < ApplicationRecord end def process_accounts - simplefin_accounts.joins(:account).each do |simplefin_account| + # Process accounts linked via BOTH legacy FK and AccountProvider + simplefin_accounts.includes(:account, account_provider: :account).each do |simplefin_account| + # Only process if there's a linked account (via either system) + next unless simplefin_account.current_account.present? SimplefinAccount::Processor.new(simplefin_account).process end end @@ -192,6 +195,58 @@ class SimplefinItem < ApplicationRecord end end + # Detect if sync data appears stale (no new transactions for extended period) + # Returns a hash with :stale (boolean) and :message (string) if stale + def stale_sync_status + return { stale: false } unless last_synced_at.present? + + # Check if last sync was more than 3 days ago + days_since_sync = (Date.current - last_synced_at.to_date).to_i + if days_since_sync > 3 + return { + stale: true, + days_since_sync: days_since_sync, + message: "Last successful sync was #{days_since_sync} days ago. Your SimpleFin connection may need attention." + } + end + + # Check if linked accounts have recent transactions + linked_accounts = accounts + return { stale: false } if linked_accounts.empty? + + # Find the most recent transaction date across all linked accounts + latest_transaction_date = Entry.where(account_id: linked_accounts.map(&:id)) + .where(entryable_type: "Transaction") + .maximum(:date) + + if latest_transaction_date.present? + days_since_transaction = (Date.current - latest_transaction_date).to_i + if days_since_transaction > 14 + return { + stale: true, + days_since_transaction: days_since_transaction, + message: "No new transactions in #{days_since_transaction} days. Check your SimpleFin dashboard to ensure your bank connections are active." + } + end + end + + { stale: false } + end + + # Check if the SimpleFin connection needs user attention + def needs_attention? + requires_update? || stale_sync_status[:stale] || pending_account_setup? + end + + # Get a summary of issues requiring attention + def attention_summary + issues = [] + issues << "Connection needs update" if requires_update? + issues << stale_sync_status[:message] if stale_sync_status[:stale] + issues << "Accounts need setup" if pending_account_setup? + issues + end + private def remove_simplefin_item # SimpleFin doesn't require server-side cleanup like Plaid diff --git a/app/models/simplefin_item/importer.rb b/app/models/simplefin_item/importer.rb index d7d7e7226..eb4eb942b 100644 --- a/app/models/simplefin_item/importer.rb +++ b/app/models/simplefin_item/importer.rb @@ -15,11 +15,23 @@ class SimplefinItem::Importer Rails.logger.info "SimplefinItem::Importer - last_synced_at: #{simplefin_item.last_synced_at.inspect}" Rails.logger.info "SimplefinItem::Importer - sync_start_date: #{simplefin_item.sync_start_date.inspect}" + # Clear stale error and reconciliation stats from previous syncs at the start of a full import + # This ensures the UI doesn't show outdated warnings from old sync runs + if sync.respond_to?(:sync_stats) + sync.update_columns(sync_stats: { + "cleared_at" => Time.current.iso8601, + "import_started" => true + }) + end + begin # Defensive guard: If last_synced_at is set but there are linked accounts # with no transactions captured yet (typical after a balances-only run), # force the first full run to use chunked history to backfill. - linked_accounts = simplefin_item.simplefin_accounts.joins(:account) + # + # Check for linked accounts via BOTH legacy FK (accounts.simplefin_account_id) AND + # the new AccountProvider system. An account is "linked" if either association exists. + linked_accounts = simplefin_item.simplefin_accounts.select { |sfa| sfa.current_account.present? } no_txns_yet = linked_accounts.any? && linked_accounts.all? { |sfa| sfa.raw_transactions_payload.blank? } if simplefin_item.last_synced_at.nil? || no_txns_yet @@ -569,7 +581,18 @@ class SimplefinItem::Importer end end - attrs[:raw_transactions_payload] = best_by_key.values + merged_transactions = best_by_key.values + attrs[:raw_transactions_payload] = merged_transactions + + # NOTE: Reconciliation disabled - it analyzes the SimpleFin API response + # which only contains ~90 days of history, creating misleading "gap" warnings + # that don't reflect actual database state. Re-enable if we improve it to + # compare against database transactions instead of just the API response. + # begin + # reconcile_transactions(simplefin_account, merged_transactions) + # rescue => e + # Rails.logger.warn("SimpleFin: reconciliation failed for sfa=#{simplefin_account.id || account_id}: #{e.class} - #{e.message}") + # end end # Track whether incoming holdings are new/changed so we can materialize and refresh balances @@ -783,4 +806,164 @@ class SimplefinItem::Importer # Default to 7 days buffer for subsequent syncs 7 end + + # Transaction reconciliation: detect potential data gaps or missing transactions + # This helps identify when SimpleFin may not be returning complete data + def reconcile_transactions(simplefin_account, new_transactions) + return if new_transactions.blank? + + account_id = simplefin_account.account_id + existing_transactions = simplefin_account.raw_transactions_payload.to_a + reconciliation = { account_id: account_id, issues: [] } + + # 1. Check for unexpected transaction count drops + # If we previously had more transactions and now have fewer (after merge), + # something may have been removed upstream + if existing_transactions.any? + existing_count = existing_transactions.size + new_count = new_transactions.size + + # After merging, we should have at least as many as before + # A significant drop (>10%) could indicate data loss + if new_count < existing_count + drop_pct = ((existing_count - new_count).to_f / existing_count * 100).round(1) + if drop_pct > 10 + reconciliation[:issues] << { + type: "transaction_count_drop", + message: "Transaction count dropped from #{existing_count} to #{new_count} (#{drop_pct}% decrease)", + severity: drop_pct > 25 ? "high" : "medium" + } + end + end + end + + # 2. Detect gaps in transaction history + # Look for periods with no transactions that seem unusual + gaps = detect_transaction_gaps(new_transactions) + if gaps.any? + reconciliation[:issues] += gaps.map do |gap| + { + type: "transaction_gap", + message: "No transactions between #{gap[:start_date]} and #{gap[:end_date]} (#{gap[:days]} days)", + severity: gap[:days] > 30 ? "high" : "medium", + gap_start: gap[:start_date], + gap_end: gap[:end_date], + gap_days: gap[:days] + } + end + end + + # 3. Check for stale data (most recent transaction is old) + latest_tx_date = extract_latest_transaction_date(new_transactions) + if latest_tx_date.present? + days_since_latest = (Date.current - latest_tx_date).to_i + if days_since_latest > 7 + reconciliation[:issues] << { + type: "stale_transactions", + message: "Most recent transaction is #{days_since_latest} days old", + severity: days_since_latest > 14 ? "high" : "medium", + latest_date: latest_tx_date.to_s, + days_stale: days_since_latest + } + end + end + + # 4. Check for duplicate transaction IDs (data integrity issue) + duplicate_ids = find_duplicate_transaction_ids(new_transactions) + if duplicate_ids.any? + reconciliation[:issues] << { + type: "duplicate_ids", + message: "Found #{duplicate_ids.size} duplicate transaction ID(s)", + severity: "low", + duplicate_count: duplicate_ids.size + } + end + + # Record reconciliation results in stats + if reconciliation[:issues].any? + stats["reconciliation"] ||= {} + stats["reconciliation"][account_id] = reconciliation + + # Count issues by severity + high_severity = reconciliation[:issues].count { |i| i[:severity] == "high" } + medium_severity = reconciliation[:issues].count { |i| i[:severity] == "medium" } + + if high_severity > 0 + stats["reconciliation_warnings"] = stats.fetch("reconciliation_warnings", 0) + high_severity + Rails.logger.warn("SimpleFin reconciliation: #{high_severity} high-severity issue(s) for account #{account_id}") + + ActiveSupport::Notifications.instrument( + "simplefin.reconciliation_warning", + item_id: simplefin_item.id, + account_id: account_id, + issues: reconciliation[:issues] + ) + end + + if medium_severity > 0 + stats["reconciliation_notices"] = stats.fetch("reconciliation_notices", 0) + medium_severity + end + + persist_stats! + end + + reconciliation + end + + # Detect gaps in transaction history (periods with no activity) + def detect_transaction_gaps(transactions) + return [] if transactions.blank? || transactions.size < 2 + + # Extract and sort transaction dates + dates = transactions.map do |tx| + t = tx.with_indifferent_access + posted = t[:posted] + next nil if posted.blank? || posted.to_i <= 0 + Time.at(posted.to_i).to_date + end.compact.uniq.sort + + return [] if dates.size < 2 + + gaps = [] + min_gap_days = 14 # Only report gaps of 2+ weeks + + dates.each_cons(2) do |earlier, later| + gap_days = (later - earlier).to_i + if gap_days >= min_gap_days + gaps << { + start_date: earlier.to_s, + end_date: later.to_s, + days: gap_days + } + end + end + + # Limit to top 3 largest gaps to avoid noise + gaps.sort_by { |g| -g[:days] }.first(3) + end + + # Extract the most recent transaction date + def extract_latest_transaction_date(transactions) + return nil if transactions.blank? + + latest_timestamp = transactions.map do |tx| + t = tx.with_indifferent_access + posted = t[:posted] + posted.to_i if posted.present? && posted.to_i > 0 + end.compact.max + + latest_timestamp ? Time.at(latest_timestamp).to_date : nil + end + + # Find duplicate transaction IDs + def find_duplicate_transaction_ids(transactions) + return [] if transactions.blank? + + ids = transactions.map do |tx| + t = tx.with_indifferent_access + t[:id] || t[:fitid] + end.compact + + ids.group_by(&:itself).select { |_, v| v.size > 1 }.keys + end end diff --git a/app/models/simplefin_item/syncer.rb b/app/models/simplefin_item/syncer.rb index b00d40028..28bc6816a 100644 --- a/app/models/simplefin_item/syncer.rb +++ b/app/models/simplefin_item/syncer.rb @@ -10,7 +10,15 @@ class SimplefinItem::Syncer # can review and manually link accounts first. This mirrors the historical flow # users expect: initial 7-day balances snapshot, then full chunked history after linking. begin - if simplefin_item.simplefin_accounts.joins(:account).count == 0 + # Check for linked accounts via BOTH legacy FK (accounts.simplefin_account_id) AND + # the new AccountProvider system. An account is "linked" if either association exists. + linked_via_legacy = simplefin_item.simplefin_accounts.joins(:account).count + linked_via_provider = simplefin_item.simplefin_accounts.joins(:account_provider).count + total_linked = simplefin_item.simplefin_accounts.select { |sfa| sfa.current_account.present? }.count + + Rails.logger.info("SimplefinItem::Syncer - linked check: legacy=#{linked_via_legacy}, provider=#{linked_via_provider}, total=#{total_linked}") + + if total_linked == 0 sync.update!(status_text: "Discovering accounts (balances only)...") if sync.respond_to?(:status_text) # Pre-mark the sync as balances_only for runtime only (no persistence) begin @@ -52,8 +60,9 @@ class SimplefinItem::Syncer finalize_setup_counts(sync) # Process transactions/holdings only for linked accounts - linked_accounts = simplefin_item.simplefin_accounts.joins(:account) - if linked_accounts.any? + # Check both legacy FK and AccountProvider associations + linked_simplefin_accounts = simplefin_item.simplefin_accounts.select { |sfa| sfa.current_account.present? } + if linked_simplefin_accounts.any? sync.update!(status_text: "Processing transactions and holdings...") if sync.respond_to?(:status_text) simplefin_item.process_accounts @@ -77,7 +86,11 @@ class SimplefinItem::Syncer def finalize_setup_counts(sync) sync.update!(status_text: "Checking account configuration...") if sync.respond_to?(:status_text) total_accounts = simplefin_item.simplefin_accounts.count - linked_accounts = simplefin_item.simplefin_accounts.joins(:account) + + # Count linked accounts using both legacy FK and AccountProvider associations + linked_count = simplefin_item.simplefin_accounts.count { |sfa| sfa.current_account.present? } + + # Unlinked = no legacy FK AND no AccountProvider unlinked_accounts = simplefin_item.simplefin_accounts .left_joins(:account, :account_provider) .where(accounts: { id: nil }, account_providers: { id: nil }) @@ -93,7 +106,7 @@ class SimplefinItem::Syncer existing = (sync.sync_stats || {}) setup_stats = { "total_accounts" => total_accounts, - "linked_accounts" => linked_accounts.count, + "linked_accounts" => linked_count, "unlinked_accounts" => unlinked_accounts.count } sync.update!(sync_stats: existing.merge(setup_stats)) @@ -185,7 +198,8 @@ class SimplefinItem::Syncer window_start = sync.created_at || 30.minutes.ago window_end = Time.current - account_ids = simplefin_item.simplefin_accounts.joins(:account).pluck("accounts.id") + # Get account IDs via BOTH legacy FK and AccountProvider to ensure we capture all linked accounts + account_ids = simplefin_item.simplefin_accounts.filter_map { |sfa| sfa.current_account&.id } return {} if account_ids.empty? tx_scope = Entry.where(account_id: account_ids, source: "simplefin", entryable_type: "Transaction") @@ -193,14 +207,16 @@ class SimplefinItem::Syncer tx_updated = tx_scope.where(updated_at: window_start..window_end).where.not(created_at: window_start..window_end).count tx_seen = tx_imported + tx_updated - holdings_scope = Holding.where(account_id: account_ids) - holdings_processed = holdings_scope.where(created_at: window_start..window_end).count + # Count holdings from raw_holdings_payload (what the sync found) rather than + # the database. Holdings are applied asynchronously via SimplefinHoldingsApplyJob, + # so database counts would always be 0 at this point. + holdings_found = simplefin_item.simplefin_accounts.sum { |sfa| Array(sfa.raw_holdings_payload).size } { "tx_imported" => tx_imported, "tx_updated" => tx_updated, "tx_seen" => tx_seen, - "holdings_processed" => holdings_processed, + "holdings_found" => holdings_found, "window_start" => window_start, "window_end" => window_end } diff --git a/app/views/rules/index.html.erb b/app/views/rules/index.html.erb index 23613b959..f40020abe 100644 --- a/app/views/rules/index.html.erb +++ b/app/views/rules/index.html.erb @@ -117,12 +117,12 @@ <% @recent_runs.each do |run| %> - + "> <%= run.executed_at.strftime("%b %d, %Y %I:%M %p") %> - + "> <%= t("rules.recent_runs.execution_types.#{run.execution_type}") %> diff --git a/app/views/simplefin_items/_simplefin_item.html.erb b/app/views/simplefin_items/_simplefin_item.html.erb index 680a89d04..c4cee3b88 100644 --- a/app/views/simplefin_items/_simplefin_item.html.erb +++ b/app/views/simplefin_items/_simplefin_item.html.erb @@ -16,9 +16,31 @@ <% end %> + <%# Compute unlinked count early for badge display %> + <% header_unlinked_count = if defined?(@simplefin_unlinked_count_map) && @simplefin_unlinked_count_map + @simplefin_unlinked_count_map[simplefin_item.id] || 0 + else + begin + simplefin_item.simplefin_accounts + .left_joins(:account, :account_provider) + .where(accounts: { id: nil }, account_providers: { id: nil }) + .count + rescue => e + 0 + end + end %> +
<%= tag.p simplefin_item.name, class: "font-medium text-primary" %> + <% if header_unlinked_count.to_i > 0 %> + <%= link_to setup_accounts_simplefin_item_path(simplefin_item), + data: { turbo_frame: :modal }, + class: "inline-flex items-center gap-1 px-2 py-0.5 rounded-full text-xs font-medium bg-warning/10 text-warning hover:bg-warning/20 transition-colors" do %> + <%= icon "alert-circle", size: "xs" %> + <%= header_unlinked_count %> <%= header_unlinked_count == 1 ? "account" : "accounts" %> need setup + <% end %> + <% end %> <% if simplefin_item.scheduled_for_deletion? %>

<%= t(".deletion_in_progress") %>

<% end %> @@ -77,6 +99,11 @@ <%= icon "alert-triangle", size: "sm", color: "warning" %> <%= tag.span t(".requires_update") %>
+ <% elsif (stale_status = simplefin_item.stale_sync_status)[:stale] %> +
+ <%= icon "alert-circle", size: "sm", color: "warning" %> + <%= tag.span stale_status[:message], class: "text-sm" %> +
<% elsif simplefin_item.rate_limited_message.present? %>
<%= icon "clock", size: "sm", color: "warning" %> @@ -125,8 +152,6 @@ ) %> <% end %> - - <%= render DS::Menu.new do |menu| %> <% menu.with_item( variant: "button", @@ -146,7 +171,6 @@ <%= render "accounts/index/account_groups", accounts: simplefin_item.accounts %> <% end %> - <%# Sync summary (collapsible) Prefer controller-provided map; fallback to latest sync stats so Turbo broadcasts can render the summary without requiring a full page refresh. %> @@ -192,23 +216,28 @@

Holdings

- Processed: <%= stats["holdings_processed"].to_i %> + Found: <%= stats["holdings_found"].to_i %>

Health

-
- <% if stats["rate_limited"].present? || stats["rate_limited_at"].present? %> - <% ts = stats["rate_limited_at"] %> - <% ago = (ts.present? ? (begin; time_ago_in_words(Time.parse(ts)); rescue StandardError; nil; end) : nil) %> - Rate limited <%= ago ? "(#{ago} ago)" : "recently" %> - <% end %> - <% total_errors = stats["total_errors"].to_i %> - <% if total_errors > 0 %> - Errors: <%= total_errors %> - <% else %> - Errors: 0 - <% end %> +
+
+ <% if stats["rate_limited"].present? || stats["rate_limited_at"].present? %> + <% ts = stats["rate_limited_at"] %> + <% ago = (ts.present? ? (begin; time_ago_in_words(Time.parse(ts)); rescue StandardError; nil; end) : nil) %> + Rate limited <%= ago ? "(#{ago} ago)" : "recently" %> + <% end %> + <% total_errors = stats["total_errors"].to_i %> + <% import_started = stats["import_started"].present? %> + <% if total_errors > 0 %> + Errors: <%= total_errors %> + <% elsif import_started %> + Errors: 0 + <% else %> + Errors: 0 + <% end %> +
diff --git a/app/views/simplefin_items/_subtype_select.html.erb b/app/views/simplefin_items/_subtype_select.html.erb index f5c30548f..9899cf423 100644 --- a/app/views/simplefin_items/_subtype_select.html.erb +++ b/app/views/simplefin_items/_subtype_select.html.erb @@ -1,27 +1,27 @@ - - +
- <%= icon "calendar", size: "sm", class: "text-primary mt-0.5 flex-shrink-0" %> + <%= icon "clock", size: "sm", class: "text-primary mt-0.5 flex-shrink-0" %>
-

- Historical Data Range: +

+ Transaction History: +

+

+ SimpleFin typically provides 60-90 days of transaction history, depending on your bank. + After initial setup, new transactions will sync automatically going forward. + Historical data availability varies by institution and account type.

- <%= form.date_field :sync_start_date, - label: "Start syncing transactions from:", - value: @simplefin_item.sync_start_date || 1.year.ago.to_date, - min: 3.years.ago.to_date, - max: Date.current, - class: "w-full max-w-xs", - help_text: "Select how far back you want to sync transaction history. Maximum 3 years of history available." %>
<% @simplefin_accounts.each do |simplefin_account| %> -
+ <% inferred = @inferred_map[simplefin_account.id] || {} %> + <% selected_type = inferred[:confidence] == :high ? inferred[:type] : "skip" %> + <%# Check if this account needs user attention (type selected but subtype missing) %> + <% types_with_subtypes = %w[Depository Investment Loan] %> + <% needs_subtype_attention = selected_type != "skip" && types_with_subtypes.include?(selected_type) && inferred[:subtype].blank? %> + +
">

@@ -78,8 +82,6 @@
<%= label_tag "account_types[#{simplefin_account.id}]", "Account Type:", class: "block text-sm font-medium text-primary mb-2" %> - <% inferred = @inferred_map[simplefin_account.id] || {} %> - <% selected_type = inferred[:confidence] == :high ? inferred[:type] : "skip" %> <%= select_tag "account_types[#{simplefin_account.id}]", options_for_select(@account_type_options, selected_type), { 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", diff --git a/config/locales/views/transactions/en.yml b/config/locales/views/transactions/en.yml index e66407248..4572ed8c6 100644 --- a/config/locales/views/transactions/en.yml +++ b/config/locales/views/transactions/en.yml @@ -1,6 +1,7 @@ --- en: transactions: + unknown_name: Unknown transaction form: account: Account account_prompt: Select an Account diff --git a/test/controllers/simplefin_items_controller_test.rb b/test/controllers/simplefin_items_controller_test.rb index 43a100d88..1242dd771 100644 --- a/test/controllers/simplefin_items_controller_test.rb +++ b/test/controllers/simplefin_items_controller_test.rb @@ -1,6 +1,7 @@ require "test_helper" class SimplefinItemsControllerTest < ActionDispatch::IntegrationTest + include ActiveJob::TestHelper fixtures :users, :families setup do sign_in users(:family_admin) @@ -154,22 +155,20 @@ class SimplefinItemsControllerTest < ActionDispatch::IntegrationTest test "should update simplefin item with valid token" do @simplefin_item.update!(status: :requires_update) - # Mock the SimpleFin provider to prevent real API calls - mock_provider = mock() - mock_provider.expects(:claim_access_url).with("valid_token").returns("https://example.com/new_access") - mock_provider.expects(:get_accounts).returns({ accounts: [] }).at_least_once - Provider::Simplefin.expects(:new).returns(mock_provider).at_least_once + token = Base64.strict_encode64("https://example.com/claim") - # Let the real create_simplefin_item! method run - don't mock it + SimplefinConnectionUpdateJob.expects(:perform_later).with( + family_id: @family.id, + old_simplefin_item_id: @simplefin_item.id, + setup_token: token + ).once patch simplefin_item_url(@simplefin_item), params: { - simplefin_item: { setup_token: "valid_token" } + simplefin_item: { setup_token: token } } assert_redirected_to accounts_path assert_equal "SimpleFin connection updated.", flash[:notice] - @simplefin_item.reload - assert @simplefin_item.scheduled_for_deletion? end test "should handle update with invalid token" do @@ -186,6 +185,8 @@ class SimplefinItemsControllerTest < ActionDispatch::IntegrationTest test "should transfer accounts when updating simplefin item token" do @simplefin_item.update!(status: :requires_update) + token = Base64.strict_encode64("https://example.com/claim") + # Create old SimpleFin accounts linked to Maybe accounts old_simplefin_account1 = @simplefin_item.simplefin_accounts.create!( name: "Test Checking", @@ -228,7 +229,7 @@ class SimplefinItemsControllerTest < ActionDispatch::IntegrationTest # Mock only the external API calls, let business logic run mock_provider = mock() - mock_provider.expects(:claim_access_url).with("valid_token").returns("https://example.com/new_access") + mock_provider.expects(:claim_access_url).with(token).returns("https://example.com/new_access") mock_provider.expects(:get_accounts).returns({ accounts: [ { @@ -251,10 +252,13 @@ class SimplefinItemsControllerTest < ActionDispatch::IntegrationTest }).at_least_once Provider::Simplefin.expects(:new).returns(mock_provider).at_least_once - # Perform the update - patch simplefin_item_url(@simplefin_item), params: { - simplefin_item: { setup_token: "valid_token" } - } + # Perform the update (async job), but execute enqueued jobs inline so we can + # assert the link transfers. + perform_enqueued_jobs(only: SimplefinConnectionUpdateJob) do + patch simplefin_item_url(@simplefin_item), params: { + simplefin_item: { setup_token: token } + } + end assert_redirected_to accounts_path assert_equal "SimpleFin connection updated.", flash[:notice] @@ -279,11 +283,7 @@ class SimplefinItemsControllerTest < ActionDispatch::IntegrationTest assert_equal new_sf_account1.id, maybe_account1.simplefin_account_id assert_equal new_sf_account2.id, maybe_account2.simplefin_account_id - # Verify old SimpleFin accounts no longer reference Maybe accounts - old_simplefin_account1.reload - old_simplefin_account2.reload - assert_nil old_simplefin_account1.current_account - assert_nil old_simplefin_account2.current_account + # The old item will be deleted asynchronously; until then, legacy links should be moved. # Verify old SimpleFin item is scheduled for deletion @simplefin_item.reload @@ -293,6 +293,8 @@ class SimplefinItemsControllerTest < ActionDispatch::IntegrationTest test "should handle partial account matching during token update" do @simplefin_item.update!(status: :requires_update) + token = Base64.strict_encode64("https://example.com/claim") + # Create old SimpleFin account old_simplefin_account = @simplefin_item.simplefin_accounts.create!( name: "Test Checking", @@ -316,19 +318,19 @@ class SimplefinItemsControllerTest < ActionDispatch::IntegrationTest # Mock only the external API calls, let business logic run mock_provider = mock() - mock_provider.expects(:claim_access_url).with("valid_token").returns("https://example.com/new_access") + mock_provider.expects(:claim_access_url).with(token).returns("https://example.com/new_access") # Return empty accounts list to simulate account was removed from bank mock_provider.expects(:get_accounts).returns({ accounts: [] }).at_least_once Provider::Simplefin.expects(:new).returns(mock_provider).at_least_once # Perform update - patch simplefin_item_url(@simplefin_item), params: { - simplefin_item: { setup_token: "valid_token" } - } + perform_enqueued_jobs(only: SimplefinConnectionUpdateJob) do + patch simplefin_item_url(@simplefin_item), params: { + simplefin_item: { setup_token: token } + } + end - assert_response :redirect - uri2 = URI(response.redirect_url) - assert_equal "/accounts", uri2.path + assert_redirected_to accounts_path # Verify Maybe account still linked to old SimpleFin account (no transfer occurred) maybe_account.reload @@ -450,30 +452,27 @@ class SimplefinItemsControllerTest < ActionDispatch::IntegrationTest test "update redirects to accounts after setup without forcing a modal" do @simplefin_item.update!(status: :requires_update) - # Mock provider to return one account so updated_item creates SFAs - mock_provider = mock() - mock_provider.expects(:claim_access_url).with("valid_token").returns("https://example.com/new_access") - mock_provider.expects(:get_accounts).returns({ - accounts: [ - { id: "sf_auto_open_1", name: "Auto Open Checking", type: "depository", currency: "USD", balance: 100, transactions: [] } - ] - }).at_least_once - Provider::Simplefin.expects(:new).returns(mock_provider).at_least_once + token = Base64.strict_encode64("https://example.com/claim") - patch simplefin_item_url(@simplefin_item), params: { simplefin_item: { setup_token: "valid_token" } } + SimplefinConnectionUpdateJob.expects(:perform_later).with( + family_id: @family.id, + old_simplefin_item_id: @simplefin_item.id, + setup_token: token + ).once - assert_response :redirect - uri = URI(response.redirect_url) - assert_equal "/accounts", uri.path + patch simplefin_item_url(@simplefin_item), params: { simplefin_item: { setup_token: token } } + + assert_redirected_to accounts_path end test "create does not auto-open when no candidates or unlinked" do # Mock provider interactions for item creation (no immediate account import on create) mock_provider = mock() - mock_provider.expects(:claim_access_url).with("valid_token").returns("https://example.com/new_access") + token = Base64.strict_encode64("https://example.com/claim") + mock_provider.expects(:claim_access_url).with(token).returns("https://example.com/new_access") Provider::Simplefin.expects(:new).returns(mock_provider).at_least_once - post simplefin_items_url, params: { simplefin_item: { setup_token: "valid_token" } } + post simplefin_items_url, params: { simplefin_item: { setup_token: token } } assert_response :redirect uri = URI(response.redirect_url) @@ -485,12 +484,15 @@ class SimplefinItemsControllerTest < ActionDispatch::IntegrationTest test "update does not auto-open when no SFAs present" do @simplefin_item.update!(status: :requires_update) - mock_provider = mock() - mock_provider.expects(:claim_access_url).with("valid_token").returns("https://example.com/new_access") - mock_provider.expects(:get_accounts).returns({ accounts: [] }).at_least_once - Provider::Simplefin.expects(:new).returns(mock_provider).at_least_once + token = Base64.strict_encode64("https://example.com/claim") - patch simplefin_item_url(@simplefin_item), params: { simplefin_item: { setup_token: "valid_token" } } + SimplefinConnectionUpdateJob.expects(:perform_later).with( + family_id: @family.id, + old_simplefin_item_id: @simplefin_item.id, + setup_token: token + ).once + + patch simplefin_item_url(@simplefin_item), params: { simplefin_item: { setup_token: token } } assert_response :redirect uri = URI(response.redirect_url) diff --git a/test/models/provider/simplefin_test.rb b/test/models/provider/simplefin_test.rb new file mode 100644 index 000000000..e0cbaee23 --- /dev/null +++ b/test/models/provider/simplefin_test.rb @@ -0,0 +1,144 @@ +require "test_helper" + +class Provider::SimplefinTest < ActiveSupport::TestCase + setup do + @provider = Provider::Simplefin.new + @access_url = "https://example.com/simplefin/access" + end + + test "retries on Net::ReadTimeout and succeeds on retry" do + # First call raises timeout, second call succeeds + mock_response = OpenStruct.new(code: 200, body: '{"accounts": []}') + + HTTParty.expects(:get) + .times(2) + .raises(Net::ReadTimeout.new("Connection timed out")) + .then.returns(mock_response) + + # Stub sleep to avoid actual delays in tests + @provider.stubs(:sleep) + + result = @provider.get_accounts(@access_url) + assert_equal({ accounts: [] }, result) + end + + test "retries on Net::OpenTimeout and succeeds on retry" do + mock_response = OpenStruct.new(code: 200, body: '{"accounts": []}') + + HTTParty.expects(:get) + .times(2) + .raises(Net::OpenTimeout.new("Connection timed out")) + .then.returns(mock_response) + + @provider.stubs(:sleep) + + result = @provider.get_accounts(@access_url) + assert_equal({ accounts: [] }, result) + end + + test "retries on SocketError and succeeds on retry" do + mock_response = OpenStruct.new(code: 200, body: '{"accounts": []}') + + HTTParty.expects(:get) + .times(2) + .raises(SocketError.new("Failed to open TCP connection")) + .then.returns(mock_response) + + @provider.stubs(:sleep) + + result = @provider.get_accounts(@access_url) + assert_equal({ accounts: [] }, result) + end + + test "raises SimplefinError after max retries exceeded" do + HTTParty.expects(:get) + .times(4) # Initial + 3 retries + .raises(Net::ReadTimeout.new("Connection timed out")) + + @provider.stubs(:sleep) + + error = assert_raises(Provider::Simplefin::SimplefinError) do + @provider.get_accounts(@access_url) + end + + assert_equal :network_error, error.error_type + assert_match(/Network error after 3 retries/, error.message) + end + + test "does not retry on non-retryable errors" do + HTTParty.expects(:get) + .times(1) + .raises(ArgumentError.new("Invalid argument")) + + error = assert_raises(Provider::Simplefin::SimplefinError) do + @provider.get_accounts(@access_url) + end + + assert_equal :request_failed, error.error_type + end + + test "handles HTTP 429 rate limit response" do + mock_response = OpenStruct.new(code: 429, body: "Rate limit exceeded") + + HTTParty.expects(:get).returns(mock_response) + + error = assert_raises(Provider::Simplefin::SimplefinError) do + @provider.get_accounts(@access_url) + end + + assert_equal :rate_limited, error.error_type + assert_match(/rate limit exceeded/i, error.message) + end + + test "handles HTTP 500 server error response" do + mock_response = OpenStruct.new(code: 500, body: "Internal Server Error") + + HTTParty.expects(:get).returns(mock_response) + + error = assert_raises(Provider::Simplefin::SimplefinError) do + @provider.get_accounts(@access_url) + end + + assert_equal :server_error, error.error_type + end + + test "claim_access_url retries on network errors" do + setup_token = Base64.encode64("https://example.com/claim") + mock_response = OpenStruct.new(code: 200, body: "https://example.com/access") + + HTTParty.expects(:post) + .times(2) + .raises(Net::ReadTimeout.new("Connection timed out")) + .then.returns(mock_response) + + @provider.stubs(:sleep) + + result = @provider.claim_access_url(setup_token) + assert_equal "https://example.com/access", result + end + + test "exponential backoff delay increases with retries" do + provider = Provider::Simplefin.new + + # Access private method for testing + delay1 = provider.send(:calculate_retry_delay, 1) + delay2 = provider.send(:calculate_retry_delay, 2) + delay3 = provider.send(:calculate_retry_delay, 3) + + # Delays should increase (accounting for jitter) + # Base delays: 2, 4, 8 seconds (with up to 25% jitter) + assert delay1 >= 2 && delay1 <= 2.5, "First retry delay should be ~2s" + assert delay2 >= 4 && delay2 <= 5, "Second retry delay should be ~4s" + assert delay3 >= 8 && delay3 <= 10, "Third retry delay should be ~8s" + end + + test "retry delay is capped at MAX_RETRY_DELAY" do + provider = Provider::Simplefin.new + + # Test with a high retry count that would exceed max delay + delay = provider.send(:calculate_retry_delay, 10) + + assert delay <= Provider::Simplefin::MAX_RETRY_DELAY, + "Delay should be capped at MAX_RETRY_DELAY (#{Provider::Simplefin::MAX_RETRY_DELAY}s)" + end +end diff --git a/test/models/simplefin_entry/processor_test.rb b/test/models/simplefin_entry/processor_test.rb index abea68a91..2717f6bb5 100644 --- a/test/models/simplefin_entry/processor_test.rb +++ b/test/models/simplefin_entry/processor_test.rb @@ -53,7 +53,9 @@ class SimplefinEntry::ProcessorTest < ActiveSupport::TestCase assert_equal "Order #1234", sf["description"] assert_equal({ "category" => "restaurants", "check_number" => nil }, sf["extra"]) end - test "flags pending transaction when posted is nil and transacted_at present" do + test "does not flag pending when posted is nil but provider pending flag not set" do + # Previously we inferred pending from missing posted date, but this was too aggressive - + # some providers don't supply posted dates even for settled transactions tx = { id: "tx_pending_1", amount: "-20.00", @@ -70,7 +72,7 @@ class SimplefinEntry::ProcessorTest < ActiveSupport::TestCase entry = @account.entries.find_by!(external_id: "simplefin_tx_pending_1", source: "simplefin") sf = entry.transaction.extra.fetch("simplefin") - assert_equal true, sf["pending"], "expected pending flag to be true" + assert_equal false, sf["pending"], "expected pending flag to be false when provider doesn't explicitly set pending" end test "captures FX metadata when tx currency differs from account currency" do