diff --git a/app/controllers/snaptrade_items_controller.rb b/app/controllers/snaptrade_items_controller.rb index f02d5a155..6fab6b0ec 100644 --- a/app/controllers/snaptrade_items_controller.rb +++ b/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, diff --git a/app/models/snaptrade_item.rb b/app/models/snaptrade_item.rb index f99c763f6..367836e42 100644 --- a/app/models/snaptrade_item.rb +++ b/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, "" ]) } diff --git a/test/controllers/snaptrade_items_controller_test.rb b/test/controllers/snaptrade_items_controller_test.rb index 18048ac2d..06c4d54e9 100644 --- a/test/controllers/snaptrade_items_controller_test.rb +++ b/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)