diff --git a/app/jobs/simplefin_connection_update_job.rb b/app/jobs/simplefin_connection_update_job.rb index 850eaac8b..db4593762 100644 --- a/app/jobs/simplefin_connection_update_job.rb +++ b/app/jobs/simplefin_connection_update_job.rb @@ -2,166 +2,30 @@ 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. + # If the token claim succeeds but sync 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]})" + "(family_id=#{job.arguments.first[:family_id]}, 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) + simplefin_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 1: Claim the new token and update the existing item's access_url. + # This preserves all existing account linkages - no need to transfer anything. + simplefin_item.update_access_url!(setup_token: setup_token) + + # Step 2: Sync the item to import fresh data. + # The existing repair_stale_linkages logic handles cases where SimpleFIN + # account IDs changed (e.g., user re-added institution in SimpleFIN Bridge). + simplefin_item.sync_later + + Rails.logger.info( + "SimplefinConnectionUpdateJob: Successfully updated SimplefinItem #{simplefin_item.id} " \ + "with new access_url for family #{family_id}" ) - - # 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/simplefin_item.rb b/app/models/simplefin_item.rb index baab600b5..daabf99b3 100644 --- a/app/models/simplefin_item.rb +++ b/app/models/simplefin_item.rb @@ -55,6 +55,20 @@ class SimplefinItem < ApplicationRecord SimplefinItem::Importer.new(self, simplefin_provider: simplefin_provider, sync: sync).import end + # Update the access_url by claiming a new setup token. + # This is used when reconnecting an existing SimpleFIN connection. + # Unlike create_simplefin_item!, this updates in-place, preserving all account linkages. + def update_access_url!(setup_token:) + new_access_url = simplefin_provider.claim_access_url(setup_token) + + update!( + access_url: new_access_url, + status: :good + ) + + self + end + def process_accounts # Process accounts linked via BOTH legacy FK and AccountProvider # Use direct query to ensure fresh data from DB, bypassing any association cache diff --git a/app/models/simplefin_item/importer.rb b/app/models/simplefin_item/importer.rb index 8d43993f8..c465bf951 100644 --- a/app/models/simplefin_item/importer.rb +++ b/app/models/simplefin_item/importer.rb @@ -45,6 +45,11 @@ class SimplefinItem::Importer Rails.logger.info "SimplefinItem::Importer - Using REGULAR SYNC (last_synced_at=#{simplefin_item.last_synced_at&.strftime('%Y-%m-%d %H:%M')})" import_regular_sync end + + # Reset status to good if no auth errors occurred in this sync. + # This allows the item to recover automatically when a bank's auth issue is resolved + # in SimpleFIN Bridge, without requiring the user to manually reconnect. + maybe_clear_requires_update_status rescue RateLimitedError => e stats["rate_limited"] = true stats["rate_limited_at"] = Time.current.iso8601 @@ -321,6 +326,21 @@ class SimplefinItem::Importer sync.update_columns(sync_stats: merged) # avoid callbacks/validations during tight loops end + # Reset status to good if no auth errors occurred in this sync. + # This allows automatic recovery when a bank's auth issue is resolved in SimpleFIN Bridge. + def maybe_clear_requires_update_status + return unless simplefin_item.requires_update? + + auth_errors = stats.dig("error_buckets", "auth").to_i + if auth_errors.zero? + simplefin_item.update!(status: :good) + Rails.logger.info( + "SimpleFIN: cleared requires_update status for item ##{simplefin_item.id} " \ + "(no auth errors in this sync)" + ) + end + end + def import_with_chunked_history # SimpleFin's actual limit is 60 days (not 365 as documented) # Use 60-day chunks to stay within limits diff --git a/test/controllers/simplefin_items_controller_test.rb b/test/controllers/simplefin_items_controller_test.rb index f702a0006..c7e039583 100644 --- a/test/controllers/simplefin_items_controller_test.rb +++ b/test/controllers/simplefin_items_controller_test.rb @@ -182,20 +182,21 @@ class SimplefinItemsControllerTest < ActionDispatch::IntegrationTest assert_includes response.body, I18n.t("simplefin_items.update.errors.blank_token", default: "Please enter a SimpleFIN setup token") end - test "should transfer accounts when updating simplefin item token" do + test "should update simplefin item access_url in place preserving account linkages" do @simplefin_item.update!(status: :requires_update) + original_item_id = @simplefin_item.id token = Base64.strict_encode64("https://example.com/claim") - # Create old SimpleFIN accounts linked to Maybe accounts - old_simplefin_account1 = @simplefin_item.simplefin_accounts.create!( + # Create SimpleFIN accounts linked to Maybe accounts + simplefin_account1 = @simplefin_item.simplefin_accounts.create!( name: "Test Checking", account_id: "sf_account_123", currency: "USD", current_balance: 1000, account_type: "depository" ) - old_simplefin_account2 = @simplefin_item.simplefin_accounts.create!( + simplefin_account2 = @simplefin_item.simplefin_accounts.create!( name: "Test Savings", account_id: "sf_account_456", currency: "USD", @@ -211,7 +212,7 @@ class SimplefinItemsControllerTest < ActionDispatch::IntegrationTest currency: "USD", accountable_type: "Depository", accountable: Depository.create!(subtype: "checking"), - simplefin_account_id: old_simplefin_account1.id + simplefin_account_id: simplefin_account1.id ) maybe_account2 = Account.create!( family: @family, @@ -220,40 +221,19 @@ class SimplefinItemsControllerTest < ActionDispatch::IntegrationTest currency: "USD", accountable_type: "Depository", accountable: Depository.create!(subtype: "savings"), - simplefin_account_id: old_simplefin_account2.id + simplefin_account_id: simplefin_account2.id ) - # Update old SimpleFIN accounts to reference the Maybe accounts - old_simplefin_account1.update!(account: maybe_account1) - old_simplefin_account2.update!(account: maybe_account2) + # Update SimpleFIN accounts to reference the Maybe accounts + simplefin_account1.update!(account: maybe_account1) + simplefin_account2.update!(account: maybe_account2) - # Mock only the external API calls, let business logic run + # Mock only the external API calls mock_provider = mock() mock_provider.expects(:claim_access_url).with(token).returns("https://example.com/new_access") - mock_provider.expects(:get_accounts).returns({ - accounts: [ - { - id: "sf_account_123", - name: "Test Checking", - type: "depository", - currency: "USD", - balance: 1000, - transactions: [] - }, - { - id: "sf_account_456", - name: "Test Savings", - type: "depository", - currency: "USD", - balance: 5000, - transactions: [] - } - ] - }).at_least_once Provider::Simplefin.expects(:new).returns(mock_provider).at_least_once - # Perform the update (async job), but execute enqueued jobs inline so we can - # assert the link transfers. + # Perform the update - job updates access_url and enqueues sync perform_enqueued_jobs(only: SimplefinConnectionUpdateJob) do patch simplefin_item_url(@simplefin_item), params: { simplefin_item: { setup_token: token } @@ -263,40 +243,33 @@ class SimplefinItemsControllerTest < ActionDispatch::IntegrationTest assert_redirected_to accounts_path assert_equal "SimpleFIN connection updated.", flash[:notice] - # Verify accounts were transferred to new SimpleFIN accounts - assert Account.exists?(maybe_account1.id), "maybe_account1 should still exist" - assert Account.exists?(maybe_account2.id), "maybe_account2 should still exist" + # Verify the same SimpleFIN item was updated (not a new one created) + @simplefin_item.reload + assert_equal original_item_id, @simplefin_item.id + assert_equal "https://example.com/new_access", @simplefin_item.access_url + assert_equal "good", @simplefin_item.status + # Verify no duplicate SimpleFIN items were created + assert_equal 1, @family.simplefin_items.count + + # Verify account linkages remain intact maybe_account1.reload maybe_account2.reload + assert_equal simplefin_account1.id, maybe_account1.simplefin_account_id + assert_equal simplefin_account2.id, maybe_account2.simplefin_account_id - # Find the new SimpleFIN item that was created - new_simplefin_item = @family.simplefin_items.where.not(id: @simplefin_item.id).first - assert_not_nil new_simplefin_item, "New SimpleFIN item should have been created" - - new_sf_account1 = new_simplefin_item.simplefin_accounts.find_by(account_id: "sf_account_123") - new_sf_account2 = new_simplefin_item.simplefin_accounts.find_by(account_id: "sf_account_456") - - assert_not_nil new_sf_account1, "New SimpleFIN account with ID sf_account_123 should exist" - assert_not_nil new_sf_account2, "New SimpleFIN account with ID sf_account_456 should exist" - - assert_equal new_sf_account1.id, maybe_account1.simplefin_account_id - assert_equal new_sf_account2.id, maybe_account2.simplefin_account_id - - # 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 - assert @simplefin_item.scheduled_for_deletion? + # Verify item is NOT scheduled for deletion (we updated it, not replaced it) + assert_not @simplefin_item.scheduled_for_deletion? end - test "should handle partial account matching during token update" do + test "should preserve account linkages when reconnecting even if accounts change" do @simplefin_item.update!(status: :requires_update) + original_item_id = @simplefin_item.id token = Base64.strict_encode64("https://example.com/claim") - # Create old SimpleFIN account - old_simplefin_account = @simplefin_item.simplefin_accounts.create!( + # Create SimpleFIN account linked to Maybe account + simplefin_account = @simplefin_item.simplefin_accounts.create!( name: "Test Checking", account_id: "sf_account_123", currency: "USD", @@ -312,18 +285,16 @@ class SimplefinItemsControllerTest < ActionDispatch::IntegrationTest currency: "USD", accountable_type: "Depository", accountable: Depository.create!(subtype: "checking"), - simplefin_account_id: old_simplefin_account.id + simplefin_account_id: simplefin_account.id ) - old_simplefin_account.update!(account: maybe_account) + simplefin_account.update!(account: maybe_account) - # Mock only the external API calls, let business logic run + # Mock only the external API calls mock_provider = mock() 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 + # Perform update - job updates access_url and enqueues sync perform_enqueued_jobs(only: SimplefinConnectionUpdateJob) do patch simplefin_item_url(@simplefin_item), params: { simplefin_item: { setup_token: token } @@ -332,15 +303,19 @@ class SimplefinItemsControllerTest < ActionDispatch::IntegrationTest assert_redirected_to accounts_path - # Verify Maybe account still linked to old SimpleFIN account (no transfer occurred) - maybe_account.reload - old_simplefin_account.reload - assert_equal old_simplefin_account.id, maybe_account.simplefin_account_id - assert_equal maybe_account, old_simplefin_account.current_account - - # Old item still scheduled for deletion + # Verify item was updated in place @simplefin_item.reload - assert @simplefin_item.scheduled_for_deletion? + assert_equal original_item_id, @simplefin_item.id + assert_equal "https://example.com/new_access", @simplefin_item.access_url + + # Verify account linkage remains intact (linkage preserved regardless of sync results) + maybe_account.reload + simplefin_account.reload + assert_equal simplefin_account.id, maybe_account.simplefin_account_id + assert_equal maybe_account, simplefin_account.current_account + + # Item is NOT scheduled for deletion (we updated it, not replaced it) + assert_not @simplefin_item.scheduled_for_deletion? end test "select_existing_account renders empty-state modal when no simplefin accounts exist" do