fix: send first-time SnapTrade users to connect flow (#1613)

* fix: route unregistered SnapTrade users to connect flow

* test: fix snaptrade controller sign-out helper

* fix: prefer active registered snaptrade items

* test: avoid Current.family outside request cycle

* fix: preserve snaptrade resume flow

* fix: read snaptrade resume session with indifferent keys

---------

Co-authored-by: SureBot <sure-bot@we-promise.com>

app/controllers/snaptrade_items_controller.rb

@@ -132,12 +132,19 @@ class SnaptradeItemsController < ApplicationController
     snaptrade_item = Current.family.snaptrade_items.find_by(id: params[:item_id])
 
     if snaptrade_item
-      # Trigger a sync to fetch the newly connected accounts
       snaptrade_item.sync_later unless snaptrade_item.syncing?
-      # Redirect to accounts page - user can click "accounts need setup" badge
-      # when sync completes. This avoids the auto-refresh loop issues.
-      redirect_to accounts_path, notice: t(".success")
+
+      stored_return_to, stored_accountable_type = clear_snaptrade_resume_context
+      return_to = params[:return_to].presence || stored_return_to
+      accountable_type = params[:accountable_type].presence || stored_accountable_type
+
+      if return_to == "setup_accounts"
+        redirect_to setup_accounts_snaptrade_item_path(snaptrade_item, accountable_type: accountable_type.presence), notice: t(".success")
+      else
+        redirect_to accounts_path, notice: t(".success")
+      end
     else
+      clear_snaptrade_resume_context
       redirect_to settings_providers_path, alert: t(".no_item")
     end
   end
@@ -340,24 +347,35 @@ class SnaptradeItemsController < ApplicationController
   # Collection actions for account linking flow
 
   def preload_accounts
-    snaptrade_item = Current.family.snaptrade_items.first
-    if snaptrade_item
+    snaptrade_item = current_snaptrade_item
+    unless snaptrade_item
+      redirect_to settings_providers_path, alert: t(".not_configured", default: "SnapTrade is not configured.")
+      return
+    end
+
+    if snaptrade_item.user_registered?
       snaptrade_item.sync_later unless snaptrade_item.syncing?
       redirect_to setup_accounts_snaptrade_item_path(snaptrade_item)
     else
-      redirect_to settings_providers_path, alert: t(".not_configured", default: "SnapTrade is not configured.")
+      redirect_to connect_snaptrade_item_path(snaptrade_item)
     end
   end
 
   def select_accounts
     @accountable_type = params[:accountable_type]
     @return_to = params[:return_to]
-    snaptrade_item = Current.family.snaptrade_items.first
+    snaptrade_item = current_snaptrade_item
 
-    if snaptrade_item
+    unless snaptrade_item
+      redirect_to settings_providers_path, alert: t(".not_configured", default: "SnapTrade is not configured.")
+      return
+    end
+
+    if snaptrade_item.user_registered?
       redirect_to setup_accounts_snaptrade_item_path(snaptrade_item, accountable_type: @accountable_type, return_to: @return_to)
     else
-      redirect_to settings_providers_path, alert: t(".not_configured", default: "SnapTrade is not configured.")
+      store_snaptrade_resume_context(return_to: @return_to, accountable_type: @accountable_type)
+      redirect_to connect_snaptrade_item_path(snaptrade_item)
     end
   end
 
@@ -368,7 +386,7 @@ class SnaptradeItemsController < ApplicationController
   def select_existing_account
     @account_id = params[:account_id]
     @account = Current.family.accounts.find_by(id: @account_id)
-    snaptrade_item = Current.family.snaptrade_items.first
+    snaptrade_item = current_snaptrade_item
 
     if snaptrade_item && @account
       @snaptrade_accounts = snaptrade_item.snaptrade_accounts
@@ -417,6 +435,26 @@ class SnaptradeItemsController < ApplicationController
       @snaptrade_item = Current.family.snaptrade_items.find(params[:id])
     end
 
+    def current_snaptrade_item
+      active_items = Current.family.snaptrade_items.active
+
+      active_items.syncable.ordered.first ||
+        active_items.credentials_configured.ordered.first ||
+        active_items.ordered.first
+    end
+
+    def store_snaptrade_resume_context(return_to:, accountable_type:)
+      session[:snaptrade_resume] = {
+        return_to: return_to,
+        accountable_type: accountable_type
+      }
+    end
+
+    def clear_snaptrade_resume_context
+      resume = (session.delete(:snaptrade_resume) || {}).with_indifferent_access
+      [ resume[:return_to], resume[:accountable_type] ]
+    end
+
     def snaptrade_item_params
       params.require(:snaptrade_item).permit(
         :name,

app/models/snaptrade_item.rb

@@ -35,6 +35,7 @@ class SnaptradeItem < ApplicationRecord
   has_many :linked_accounts, through: :snaptrade_accounts
 
   scope :active, -> { where(scheduled_for_deletion: false) }
+  scope :credentials_configured, -> { active.where.not(client_id: [ nil, "" ]).where.not(consumer_key: [ nil, "" ]) }
   # Syncable = active + fully configured (user registered with SnapTrade API)
   # Items without user registration will fail sync, so exclude them from auto-sync
   scope :syncable, -> { active.where.not(snaptrade_user_id: [ nil, "" ]).where.not(snaptrade_user_secret: [ nil, "" ]) }

test/controllers/snaptrade_items_controller_test.rb

@@ -6,6 +6,12 @@ class SnaptradeItemsControllerTest < ActionDispatch::IntegrationTest
     @snaptrade_item = snaptrade_items(:configured_item)
   end
 
+  def sign_out
+    @user.sessions.each do |session|
+      delete session_path(session)
+    end
+  end
+
   test "connect handles decryption error gracefully" do
     SnaptradeItem.any_instance
       .stubs(:user_registered?)
@@ -39,6 +45,78 @@ class SnaptradeItemsControllerTest < ActionDispatch::IntegrationTest
     assert_redirected_to portal_url
   end
 
+  test "select_accounts redirects unregistered users into connect flow" do
+    sign_out
+    sign_in @user = users(:empty)
+    snaptrade_item = snaptrade_items(:pending_registration_item)
+
+    get select_accounts_snaptrade_items_url, params: { accountable_type: "Investment", return_to: "setup_accounts" }
+
+    assert_redirected_to connect_snaptrade_item_path(snaptrade_item)
+  end
+
+  test "callback resumes setup flow after first-time connect detour" do
+    sign_out
+    sign_in @user = users(:empty)
+    snaptrade_item = snaptrade_items(:pending_registration_item)
+
+    assert_difference "Sync.count", 1 do
+      get select_accounts_snaptrade_items_url, params: { accountable_type: "Investment", return_to: "setup_accounts" }
+      assert_redirected_to connect_snaptrade_item_path(snaptrade_item)
+
+      get callback_snaptrade_items_url, params: { item_id: snaptrade_item.id }
+    end
+
+    assert_redirected_to setup_accounts_snaptrade_item_path(snaptrade_item, accountable_type: "Investment")
+  end
+
+  test "select_accounts redirects registered users to setup flow" do
+    get select_accounts_snaptrade_items_url, params: { accountable_type: "Investment", return_to: "/accounts" }
+
+    assert_redirected_to setup_accounts_snaptrade_item_path(@snaptrade_item, accountable_type: "Investment", return_to: "/accounts")
+  end
+
+  test "preload_accounts redirects unregistered users into connect flow" do
+    sign_out
+    sign_in @user = users(:empty)
+    snaptrade_item = snaptrade_items(:pending_registration_item)
+
+    assert_no_difference "Sync.count" do
+      get preload_accounts_snaptrade_items_url
+    end
+
+    assert_redirected_to connect_snaptrade_item_path(snaptrade_item)
+  end
+
+  test "preload_accounts redirects registered users to setup flow and queues sync" do
+    assert_difference "Sync.count", 1 do
+      get preload_accounts_snaptrade_items_url
+    end
+
+    assert_redirected_to setup_accounts_snaptrade_item_path(@snaptrade_item)
+  end
+
+  test "entry routing prefers a registered active item over a pending one" do
+    pending_item = @user.family.snaptrade_items.create!(
+      name: "Pending Registration",
+      client_id: "pending_client_id",
+      consumer_key: "pending_consumer_key",
+      status: :good,
+      scheduled_for_deletion: false,
+      pending_account_setup: true
+    )
+
+    get select_accounts_snaptrade_items_url, params: { accountable_type: "Investment", return_to: "/accounts" }
+    assert_redirected_to setup_accounts_snaptrade_item_path(@snaptrade_item, accountable_type: "Investment", return_to: "/accounts")
+
+    assert_difference "Sync.count", 1 do
+      get preload_accounts_snaptrade_items_url
+    end
+    assert_redirected_to setup_accounts_snaptrade_item_path(@snaptrade_item)
+
+    assert_not pending_item.user_registered?
+  end
+
   test "setup_accounts shows linkable investment and crypto accounts in dropdown" do
     get setup_accounts_snaptrade_item_url(@snaptrade_item)
 
@@ -70,6 +148,30 @@ class SnaptradeItemsControllerTest < ActionDispatch::IntegrationTest
     assert_match accounts(:crypto).name, response.body
   end
 
+  test "select_existing_account prefers registered active item over pending one" do
+    pending_item = @user.family.snaptrade_items.create!(
+      name: "Pending Registration",
+      client_id: "pending_client_id",
+      consumer_key: "pending_consumer_key",
+      status: :good,
+      scheduled_for_deletion: false,
+      pending_account_setup: true
+    )
+    pending_item.snaptrade_accounts.create!(
+      snaptrade_account_id: "pending_snaptrade_account",
+      name: "Pending Brokerage Account",
+      brokerage_name: "Pending Broker",
+      currency: "USD",
+      current_balance: 0
+    )
+
+    get select_existing_account_snaptrade_items_url, params: { account_id: accounts(:investment).id }
+
+    assert_response :success
+    assert_includes response.body, snaptrade_accounts(:fidelity_401k).name
+    refute_includes response.body, "Pending Brokerage Account"
+  end
+
   test "link_existing_account links account to snaptrade_account" do
     account = accounts(:investment)
     snaptrade_account = snaptrade_accounts(:fidelity_401k)