From 4d3d9d10dfe26aa5eec8dee0b101c1d126a105a1 Mon Sep 17 00:00:00 2001 From: LPW Date: Mon, 15 Dec 2025 03:47:16 -0500 Subject: [PATCH] Address remaining CodeRabbit comments from PR #267 #351 (#451) * Address remaining CodeRabbit comments from PR #267 This commit addresses the remaining unresolved code review comments: 1. Fix down migration in drop_was_merged_from_transactions.rb - Add null: false, default: false constraints to match original column - Ensures proper rollback compatibility 2. Fix bare rescue in maps_helper.rb compute_duplicate_only_flag - Replace bare rescue with rescue StandardError => e - Add proper logging for debugging - Follows Ruby best practices by being explicit about exception handling These changes improve code quality and follow Rails/Ruby best practices. * Refactor `SimplefinItemsController` and add tests for balances sync and account relinking behavior - Replaced direct sync execution with `SyncJob` for asynchronous handling of balances sync. - Updated account relinking logic to prevent disabling accounts with other active provider links. - Removed unused `compute_relink_candidates` method. - Added tests to verify `balances` action enqueues `SyncJob` and relinking respects account-provider relationships. * Refactor balances sync to use runtime-only `balances_only` flag - Replaced persistent `sync_stats` usage with runtime `balances_only?` predicate via `define_singleton_method`. - Updated `SimplefinItemsController` `balances` action to pass `balances_only` flag to `SyncJob`. - Enhanced `SyncJob` to attach transient `balances_only?` flag for execution. - Adjusted `SimplefinItem::Syncer` logic to rely on the runtime `balances_only?` method. - Updated controller tests to validate runtime flag usage in `SyncJob`. --------- Co-authored-by: Claude Co-authored-by: Josh Waldrep --- .../concerns/simplefin_items/maps_helper.rb | 3 +- app/controllers/simplefin_items_controller.rb | 122 ++---------------- app/jobs/sync_job.rb | 10 +- app/models/simplefin_item/syncer.rb | 13 +- ...85320_drop_was_merged_from_transactions.rb | 4 +- .../simplefin_items_controller_test.rb | 77 +++++++++++ 6 files changed, 110 insertions(+), 119 deletions(-) diff --git a/app/controllers/concerns/simplefin_items/maps_helper.rb b/app/controllers/concerns/simplefin_items/maps_helper.rb index 8067cb504..70495b377 100644 --- a/app/controllers/concerns/simplefin_items/maps_helper.rb +++ b/app/controllers/concerns/simplefin_items/maps_helper.rb @@ -87,7 +87,8 @@ module SimplefinItems end end errs.present? && errs.all? { |m| m.to_s.downcase.include?("duplicate upstream account detected") } - rescue + rescue StandardError => e + Rails.logger.warn("SimpleFin maps: compute_duplicate_only_flag failed: #{e.class} - #{e.message}") false end end diff --git a/app/controllers/simplefin_items_controller.rb b/app/controllers/simplefin_items_controller.rb index dd5c5eea4..858771d8f 100644 --- a/app/controllers/simplefin_items_controller.rb +++ b/app/controllers/simplefin_items_controller.rb @@ -146,8 +146,9 @@ class SimplefinItemsController < ApplicationController # Starts a balances-only sync for this SimpleFin item def balances - sync = @simplefin_item.syncs.create!(status: :pending, sync_stats: { "balances_only" => true }) - SimplefinItem::Syncer.new(@simplefin_item).perform_sync(sync) + # Create a Sync and enqueue it to run asynchronously with a runtime-only flag + sync = @simplefin_item.syncs.create!(status: :pending) + SyncJob.perform_later(sync, balances_only: true) respond_to do |format| format.html { redirect_back_or_to accounts_path } @@ -379,7 +380,17 @@ class SimplefinItemsController < ApplicationController # visible manual list. This mirrors the unified flow expectation that the provider # follows the chosen account. if previous_account && previous_account.id != @account.id && previous_account.family_id == @account.family_id - previous_account.disable! rescue nil + begin + previous_account.reload + # Only disable if the previous account is truly orphaned (no other provider links) + if previous_account.account_providers.none? + previous_account.disable! + else + Rails.logger.info("Skipped disabling account ##{previous_account.id} after relink because it still has active provider links") + end + rescue => e + Rails.logger.warn("Failed disabling-orphan check for account ##{previous_account&.id}: #{e.class} - #{e.message}") + end end end @@ -430,111 +441,6 @@ class SimplefinItemsController < ApplicationController private - NAME_NORM_RE = /\s+/.freeze - - - def normalize_name(str) - s = str.to_s.downcase.strip - return s if s.empty? - s.gsub(NAME_NORM_RE, " ") - end - - def compute_relink_candidates - # Best-effort dedup before building candidates - @simplefin_item.dedup_simplefin_accounts! rescue nil - - family = @simplefin_item.family - manuals = Account.visible_manual.where(family_id: family.id).to_a - - # Evaluate only one SimpleFin account per upstream account_id (prefer linked, else newest) - grouped = @simplefin_item.simplefin_accounts.group_by(&:account_id) - sfas = grouped.values.map { |list| list.find { |s| s.current_account.present? } || list.max_by(&:updated_at) } - - Rails.logger.info("SimpleFin compute_relink_candidates: manuals=#{manuals.size} sfas=#{sfas.size} (item_id=#{@simplefin_item.id})") - - used_manual_ids = Set.new - pairs = [] - - sfas.each do |sfa| - next if sfa.name.blank? - # Heuristics (with ambiguity guards): last4 > balance ±0.01 > name - raw = (sfa.raw_payload || {}).with_indifferent_access - sfa_last4 = raw[:mask] || raw[:last4] || raw[:"last-4"] || raw[:"account_number_last4"] - sfa_last4 = sfa_last4.to_s.strip.presence - sfa_balance = (sfa.current_balance || sfa.available_balance).to_d rescue 0.to_d - - chosen = nil - reason = nil - - # 1) last4 match: compute all candidates not yet used - if sfa_last4.present? - last4_matches = manuals.reject { |a| used_manual_ids.include?(a.id) }.select do |a| - a_last4 = nil - %i[mask last4 number_last4 account_number_last4].each do |k| - if a.respond_to?(k) - val = a.public_send(k) - a_last4 = val.to_s.strip.presence if val.present? - break if a_last4 - end - end - a_last4.present? && a_last4 == sfa_last4 - end - # Ambiguity guard: skip if multiple matches - if last4_matches.size == 1 - cand = last4_matches.first - # Conflict guard: if both have balances and differ wildly, skip - begin - ab = (cand.balance || cand.cash_balance || 0).to_d - if sfa_balance.nonzero? && ab.nonzero? && (ab - sfa_balance).abs > BigDecimal("1.00") - cand = nil - end - rescue - # ignore balance parsing errors - end - if cand - chosen = cand - reason = "last4" - end - end - end - - # 2) balance proximity - if chosen.nil? && sfa_balance.nonzero? - balance_matches = manuals.reject { |a| used_manual_ids.include?(a.id) }.select do |a| - begin - ab = (a.balance || a.cash_balance || 0).to_d - (ab - sfa_balance).abs <= BigDecimal("0.01") - rescue - false - end - end - if balance_matches.size == 1 - chosen = balance_matches.first - reason = "balance" - end - end - - # 3) exact normalized name - if chosen.nil? - name_matches = manuals.reject { |a| used_manual_ids.include?(a.id) }.select { |a| normalize_name(a.name) == normalize_name(sfa.name) } - if name_matches.size == 1 - chosen = name_matches.first - reason = "name" - end - end - - if chosen - used_manual_ids << chosen.id - pairs << { sfa_id: sfa.id, sfa_name: sfa.name, manual_id: chosen.id, manual_name: chosen.name, reason: reason } - end - end - - Rails.logger.info("SimpleFin compute_relink_candidates: built #{pairs.size} pairs (item_id=#{@simplefin_item.id})") - - # Return without the reason field to the view - pairs.map { |p| p.slice(:sfa_id, :sfa_name, :manual_id, :manual_name) } - end - def set_simplefin_item @simplefin_item = Current.family.simplefin_items.find(params[:id]) end diff --git a/app/jobs/sync_job.rb b/app/jobs/sync_job.rb index 0d9b848c0..0c8c3922c 100644 --- a/app/jobs/sync_job.rb +++ b/app/jobs/sync_job.rb @@ -1,7 +1,15 @@ class SyncJob < ApplicationJob queue_as :high_priority - def perform(sync) + # Accept a runtime-only flag to influence sync behavior without persisting config + def perform(sync, balances_only: false) + # Attach a transient predicate for this execution only + begin + sync.define_singleton_method(:balances_only?) { balances_only } + rescue => e + Rails.logger.warn("SyncJob: failed to attach balances_only? flag: #{e.class} - #{e.message}") + end + sync.perform end end diff --git a/app/models/simplefin_item/syncer.rb b/app/models/simplefin_item/syncer.rb index 997d684ab..b00d40028 100644 --- a/app/models/simplefin_item/syncer.rb +++ b/app/models/simplefin_item/syncer.rb @@ -12,12 +12,11 @@ class SimplefinItem::Syncer begin if simplefin_item.simplefin_accounts.joins(:account).count == 0 sync.update!(status_text: "Discovering accounts (balances only)...") if sync.respond_to?(:status_text) - # Pre-mark the sync as balances_only so downstream completion code does not - # bump last_synced_at. The importer also sets this flag, but setting it here - # guarantees the guard is present even if the importer exits early. - if sync.respond_to?(:sync_stats) - existing = (sync.sync_stats || {}) - sync.update_columns(sync_stats: existing.merge("balances_only" => true)) + # Pre-mark the sync as balances_only for runtime only (no persistence) + begin + sync.define_singleton_method(:balances_only?) { true } + rescue => e + Rails.logger.warn("SimplefinItem::Syncer: failed to attach balances_only? flag: #{e.class} - #{e.message}") end SimplefinItem::Importer.new(simplefin_item, simplefin_provider: simplefin_item.simplefin_provider, sync: sync).import_balances_only finalize_setup_counts(sync) @@ -30,7 +29,7 @@ class SimplefinItem::Syncer end # Balances-only fast path - if sync.respond_to?(:sync_stats) && (sync.sync_stats || {})["balances_only"] + if sync.respond_to?(:balances_only?) && sync.balances_only? sync.update!(status_text: "Refreshing balances only...") if sync.respond_to?(:status_text) begin # Use the Importer to run balances-only path diff --git a/db/migrate/20251103185320_drop_was_merged_from_transactions.rb b/db/migrate/20251103185320_drop_was_merged_from_transactions.rb index 6adb411ce..8992230ca 100644 --- a/db/migrate/20251103185320_drop_was_merged_from_transactions.rb +++ b/db/migrate/20251103185320_drop_was_merged_from_transactions.rb @@ -7,9 +7,9 @@ class DropWasMergedFromTransactions < ActiveRecord::Migration[7.2] end def down - # Recreate the column for rollback compatibility + # Recreate the column for rollback compatibility with original constraints unless column_exists?(:transactions, :was_merged) - add_column :transactions, :was_merged, :boolean + add_column :transactions, :was_merged, :boolean, null: false, default: false end end end diff --git a/test/controllers/simplefin_items_controller_test.rb b/test/controllers/simplefin_items_controller_test.rb index a44f39e04..e4fd74b36 100644 --- a/test/controllers/simplefin_items_controller_test.rb +++ b/test/controllers/simplefin_items_controller_test.rb @@ -28,6 +28,83 @@ class SimplefinItemsControllerTest < ActionDispatch::IntegrationTest assert_redirected_to accounts_path end + test "balances enqueues SyncJob and returns sync id as JSON" do + # Expect a Sync to be enqueued via SyncJob + SyncJob.expects(:perform_later).with do |sync, opts| + sync.is_a?(Sync) && opts.is_a?(Hash) && opts[:balances_only] == true + end.once + + post balances_simplefin_item_url(@simplefin_item, format: :json) + + assert_response :success + body = JSON.parse(@response.body) + assert_equal true, body["ok"], "expected ok: true" + assert body["sync_id"].present?, "expected sync_id to be present" + end + + test "relink does not disable a previously linked account that still has other provider links" do + # Create two manual accounts A and B + account_a = Account.create!( + family: @family, + name: "Manual A", + balance: 0, + currency: "USD", + accountable_type: "Depository", + accountable: Depository.create!(subtype: "checking") + ) + + account_b = Account.create!( + family: @family, + name: "Manual B", + balance: 0, + currency: "USD", + accountable_type: "Depository", + accountable: Depository.create!(subtype: "savings") + ) + + # Create a SimpleFIN account under the same item + sfa_primary = SimplefinAccount.create!( + simplefin_item: @simplefin_item, + name: "SF A", + account_id: "sf_a", + account_type: "depository", + currency: "USD", + current_balance: 0 + ) + + # Link the primary SimpleFIN provider to account A via AccountProvider (legacy link cleared by action) + AccountProvider.create!(account: account_a, provider: sfa_primary) + + # Also link a different provider TYPE (Plaid) to account A so it is NOT orphaned + plaid_item = PlaidItem.create!(family: @family, name: "Plaid Conn", access_token: "test-token", plaid_id: "test-plaid-id") + plaid_acct = PlaidAccount.create!( + plaid_item: plaid_item, + plaid_id: "test-plaid-acct", + name: "Plaid A", + plaid_type: "depository", + currency: "USD", + current_balance: 0 + ) + AccountProvider.create!(account: account_a, provider: plaid_acct) + + # Perform relink: point sfa_primary at account B + post link_existing_account_simplefin_items_path, params: { + account_id: account_b.id, + simplefin_account_id: sfa_primary.id + } + + assert_response :see_other + + # Reload and assert: account A should still be enabled (not disabled) because it has another provider link + account_a.reload + assert account_a.account_providers.any?, "expected previous account to still have provider links" + refute account_a.disabled?, "previous account should not be disabled when still linked to other providers" + + # And the AccountProvider for sfa_primary should now point to account B + ap = AccountProvider.find_by(provider: sfa_primary) + assert_equal account_b.id, ap.account_id + end + test "should get edit" do @simplefin_item.update!(status: :requires_update) get edit_simplefin_item_url(@simplefin_item)