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 <noreply@anthropic.com>
Co-authored-by: Josh Waldrep <joshua.waldrep5+github@gmail.com>

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

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 })
+    # Create a Sync and enqueue it to run asynchronously with a runtime-only flag
-    SimplefinItem::Syncer.new(@simplefin_item).perform_sync(sync)
+    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

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

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
+        # Pre-mark the sync as balances_only for runtime only (no persistence)
-        # bump last_synced_at. The importer also sets this flag, but setting it here
+        begin
-        # guarantees the guard is present even if the importer exits early.
+          sync.define_singleton_method(:balances_only?) { true }
-        if sync.respond_to?(:sync_stats)
+        rescue => e
-          existing = (sync.sync_stats || {})
+          Rails.logger.warn("SimplefinItem::Syncer: failed to attach balances_only? flag: #{e.class} - #{e.message}")
-          sync.update_columns(sync_stats: existing.merge("balances_only" => true))
         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

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

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)