Add tests for SnapTrade error handling and refine unlink behavior (#931)

- Introduced new tests to cover SnapTrade decryption and connection errors in `SnaptradeItemsControllerTest`.
- Updated error messages for improved user clarity.
- Modified `unlink` functionality to preserve `SnaptradeAccount` records while ensuring proper detachment of associated holdings.

app/controllers/accounts_controller.rb

@@ -116,14 +116,11 @@ class AccountsController < ApplicationController
         # Capture provider accounts before clearing links (so we can destroy them)
         simplefin_account_to_destroy = @account.simplefin_account
 
-        # Capture SnaptradeAccounts linked via AccountProvider
-        # Destroying them will trigger delete_snaptrade_connection callback to free connection slots
-        snaptrade_accounts_to_destroy = @account.account_providers
-          .where(provider_type: "SnaptradeAccount")
-          .map { |ap| SnaptradeAccount.find_by(id: ap.provider_id) }
-          .compact
-
         # Remove new system links (account_providers join table)
+        # SnaptradeAccount records are preserved (not destroyed) so users can relink later.
+        # This follows the Plaid pattern where the provider account survives as "unlinked".
+        # SnapTrade has limited connection slots (5 free), so preserving the record avoids
+        # wasting a slot on reconnect.
         @account.account_providers.destroy_all
 
         # Remove legacy system links (foreign keys)
@@ -135,11 +132,6 @@ class AccountsController < ApplicationController
         # - SimplefinAccount only caches API data which is regenerated on reconnect
         # - If user reconnects SimpleFin later, a new SimplefinAccount will be created
         simplefin_account_to_destroy&.destroy!
-
-        # Destroy SnaptradeAccount records to free up SnapTrade connection slots
-        # The before_destroy callback will delete the connection from SnapTrade API
-        # if no other accounts share the same authorization
-        snaptrade_accounts_to_destroy.each(&:destroy!)
       end
 
       redirect_to accounts_path, notice: t("accounts.unlink.success")

app/controllers/snaptrade_items_controller.rb

@@ -106,27 +106,17 @@ class SnaptradeItemsController < ApplicationController
 
   # Redirect user to SnapTrade connection portal
   def connect
-    # Ensure user is registered first
-    unless @snaptrade_item.user_registered?
-      begin
-        @snaptrade_item.ensure_user_registered!
-      rescue => e
-        Rails.logger.error "SnapTrade registration error: #{e.class} - #{e.message}\n#{e.backtrace&.first(5)&.join("\n")}"
-        redirect_to settings_providers_path, alert: t(".registration_failed", message: e.message)
-        return
-      end
-    end
+    @snaptrade_item.ensure_user_registered! unless @snaptrade_item.user_registered?
 
-    # Get the connection portal URL - include item ID in callback for proper routing
     redirect_url = callback_snaptrade_items_url(item_id: @snaptrade_item.id)
-
-    begin
-      portal_url = @snaptrade_item.connection_portal_url(redirect_url: redirect_url)
-      redirect_to portal_url, allow_other_host: true
-    rescue => e
-      Rails.logger.error "SnapTrade connection portal error: #{e.class} - #{e.message}\n#{e.backtrace&.first(5)&.join("\n")}"
-      redirect_to settings_providers_path, alert: t(".portal_error", message: e.message)
-    end
+    portal_url = @snaptrade_item.connection_portal_url(redirect_url: redirect_url)
+    redirect_to portal_url, allow_other_host: true
+  rescue ActiveRecord::Encryption::Errors::Decryption => e
+    Rails.logger.error "SnapTrade decryption error for item #{@snaptrade_item.id}: #{e.class} - #{e.message}\n#{e.backtrace&.first(5)&.join("\n")}"
+    redirect_to settings_providers_path, alert: t(".decryption_failed")
+  rescue => e
+    Rails.logger.error "SnapTrade connection error: #{e.class} - #{e.message}\n#{e.backtrace&.first(5)&.join("\n")}"
+    redirect_to settings_providers_path, alert: t(".connection_failed", message: e.message)
   end
 
   # Handle callback from SnapTrade after user connects brokerage

app/views/snaptrade_items/_connections_list.html.erb

@@ -53,7 +53,10 @@
                 <span class="w-1.5 h-1.5 rounded-full <%= account[:linked] ? "bg-success" : "bg-warning" %>"></span>
                 <%= account[:name] %>
                 <% unless account[:linked] %>
-                  <span class="text-warning">(<%= t("providers.snaptrade.needs_linking") %>)</span>
+                  <%= link_to "(#{t('providers.snaptrade.needs_linking')})",
+                        setup_accounts_snaptrade_item_path(snaptrade_item),
+                        class: "text-warning hover:text-primary underline",
+                        data: { turbo_frame: :modal } %>
                 <% end %>
               </li>
             <% end %>

config/locales/views/snaptrade_items/en.yml

@@ -8,8 +8,8 @@ en:
     destroy:
       success: "Scheduled SnapTrade connection for deletion."
     connect:
-      registration_failed: "Failed to register with SnapTrade: %{message}"
-      portal_error: "Failed to connect to SnapTrade: %{message}"
+      decryption_failed: "Unable to read SnapTrade credentials. Please delete and recreate this connection."
+      connection_failed: "Failed to connect to SnapTrade: %{message}"
     callback:
       success: "Brokerage connected! Please select which accounts to link."
       no_item: "SnapTrade configuration not found."

test/controllers/accounts_controller_test.rb

@@ -165,6 +165,51 @@ class AccountsControllerTest < ActionDispatch::IntegrationTest
     assert_redirected_to account_url(@account)
     assert_equal "Account is already linked to a provider", flash[:alert]
   end
+
+  test "unlink preserves SnaptradeAccount record" do
+    snaptrade_account = snaptrade_accounts(:fidelity_401k)
+    investment = accounts(:investment)
+    AccountProvider.create!(account: investment, provider: snaptrade_account)
+    investment.reload
+
+    assert investment.linked?
+
+    delete unlink_account_url(investment)
+    investment.reload
+
+    assert_not investment.linked?
+    assert_redirected_to accounts_path
+    # SnaptradeAccount should still exist (not destroyed)
+    assert SnaptradeAccount.exists?(snaptrade_account.id), "SnaptradeAccount should be preserved after unlink"
+    # But AccountProvider should be gone
+    assert_not AccountProvider.exists?(provider_type: "SnaptradeAccount", provider_id: snaptrade_account.id)
+  end
+
+  test "unlink does not enqueue SnapTrade cleanup job" do
+    snaptrade_account = snaptrade_accounts(:fidelity_401k)
+    investment = accounts(:investment)
+    AccountProvider.create!(account: investment, provider: snaptrade_account)
+    investment.reload
+
+    assert_no_enqueued_jobs(only: SnaptradeConnectionCleanupJob) do
+      delete unlink_account_url(investment)
+    end
+  end
+
+  test "unlink detaches holdings from SnapTrade provider" do
+    snaptrade_account = snaptrade_accounts(:fidelity_401k)
+    investment = accounts(:investment)
+    ap = AccountProvider.create!(account: investment, provider: snaptrade_account)
+
+    # Assign a holding to this provider
+    holding = holdings(:one)
+    holding.update!(account_provider: ap)
+
+    delete unlink_account_url(investment)
+    holding.reload
+
+    assert_nil holding.account_provider_id, "Holding should be detached from provider after unlink"
+  end
 end
 
 class AccountsControllerSimplefinCtaTest < ActionDispatch::IntegrationTest

test/controllers/snaptrade_items_controller_test.rb

@@ -0,0 +1,41 @@
+require "test_helper"
+
+class SnaptradeItemsControllerTest < ActionDispatch::IntegrationTest
+  setup do
+    sign_in @user = users(:family_admin)
+    @snaptrade_item = snaptrade_items(:configured_item)
+  end
+
+  test "connect handles decryption error gracefully" do
+    SnaptradeItem.any_instance
+      .stubs(:user_registered?)
+      .raises(ActiveRecord::Encryption::Errors::Decryption.new("cannot decrypt"))
+
+    get connect_snaptrade_item_url(@snaptrade_item)
+
+    assert_redirected_to settings_providers_path
+    assert_match(/Unable to read SnapTrade credentials/, flash[:alert])
+  end
+
+  test "connect handles general error gracefully" do
+    SnaptradeItem.any_instance
+      .stubs(:user_registered?)
+      .raises(StandardError.new("something broke"))
+
+    get connect_snaptrade_item_url(@snaptrade_item)
+
+    assert_redirected_to settings_providers_path
+    assert_match(/Failed to connect/, flash[:alert])
+  end
+
+  test "connect redirects to portal when successful" do
+    portal_url = "https://app.snaptrade.com/portal/test123"
+
+    SnaptradeItem.any_instance.stubs(:user_registered?).returns(true)
+    SnaptradeItem.any_instance.stubs(:connection_portal_url).returns(portal_url)
+
+    get connect_snaptrade_item_url(@snaptrade_item)
+
+    assert_redirected_to portal_url
+  end
+end