QT/sure
1
0
Fork 0
mirror of https://github.com/we-promise/sure.git synced 2026-04-17 11:04:14 +00:00
Code Issues Packages Projects Releases Wiki Activity

Fix Broken Account Re-linking Feature (#469)

Browse Source 
* Update SimpleFIN relinking flow and enhance duplicate account handling

- Updated logic to allow relinking of SimpleFIN accounts while preserving legacy mappings.
- Introduced clean-up logic to hide orphaned duplicate accounts after relinking.
- Enhanced UI to display current mappings for linked accounts.
- Improved test coverage for relinking scenarios and SimpleFIN account visibility.

* Localize SimpleFIN account selection messages and remove hardcoded text

- Added translations for user-facing messages in `select_existing_account` flow (`pt-BR` and `en` locales).
- Replaced hardcoded strings in the view with localized keys.

* Localize Enable Banking and SimpleFIN account linking messages; add support for investment accounts.

- Added translations for Enable Banking and SimpleFIN account linking flows.
- Updated views and controllers to replace hardcoded strings with localized keys.
- Introduced support for investment accounts in `Provider::LunchflowAdapter`.
- Enhanced relinking logic for SimpleFIN accounts and improved test coverage for related scenarios.

---------

Co-authored-by: Josh Waldrep <joshua.waldrep5+github@gmail.com>
This commit is contained in:
LPW
2025-12-20 15:18:55 -05:00
committed by GitHub
parent 5c293b532a
commit e9dbf5f4e7
11 changed files with 188 additions and 32 deletions
Download Patch File Download Diff File Expand all files Collapse all files

15
app/controllers/enable_banking_items_controller.rb
View File

@@ -430,7 +430,7 @@ class EnableBankingItemsController < ApplicationController
# Guard: only manual accounts can be linked (no existing provider links or legacy IDs)
if @account.account_providers.any? || @account.plaid_account_id.present? || @account.simplefin_account_id.present?
flash[:alert] = "Only manual accounts can be linked"
flash[:alert] = t("enable_banking_items.link_existing_account.errors.only_manual")
if turbo_frame_request?
return render turbo_stream: Array(flash_notification_stream_items)
else
@@ -441,7 +441,7 @@ class EnableBankingItemsController < ApplicationController
# Verify the Enable Banking account belongs to this family's Enable Banking items
unless enable_banking_account.enable_banking_item.present? &&
Current.family.enable_banking_items.include?(enable_banking_account.enable_banking_item)
flash[:alert] = "Invalid Enable Banking account selected"
flash[:alert] = t("enable_banking_items.link_existing_account.errors.invalid_enable_banking_account")
if turbo_frame_request?
render turbo_stream: Array(flash_notification_stream_items)
else
@@ -466,9 +466,12 @@ class EnableBankingItemsController < ApplicationController
# follows the chosen account.
if previous_account && previous_account.id != @account.id && previous_account.family_id == @account.family_id
begin
previous_account.disable!
# Disabled accounts still appear (greyed-out) in the manual list after a full refresh.
# Use the app's standard deletion path (async) so the duplicate disappears and the
# "pending_deletion" state remains truthful in the UI.
previous_account.destroy_later if previous_account.may_mark_for_deletion?
rescue => e
Rails.logger.warn("Failed to disable orphaned account #{previous_account.id}: #{e.class} - #{e.message}")
Rails.logger.warn("Failed to cleanup orphaned account #{previous_account.id}: #{e.class} - #{e.message}")
end
end
end
@@ -489,7 +492,7 @@ class EnableBankingItemsController < ApplicationController
@enable_banking_items = Current.family.enable_banking_items.ordered.includes(:syncs)
build_enable_banking_maps_for(@enable_banking_items)
flash[:notice] = "Account successfully linked to Enable Banking"
flash[:notice] = t("enable_banking_items.link_existing_account.success")
@account.reload
manual_accounts_stream = if @manual_accounts.any?
turbo_stream.update(
@@ -513,7 +516,7 @@ class EnableBankingItemsController < ApplicationController
turbo_stream.replace("modal", view_context.turbo_frame_tag("modal"))
] + Array(flash_notification_stream_items)
else
redirect_to accounts_path(cache_bust: SecureRandom.hex(6)), notice: "Account successfully linked to Enable Banking", status: :see_other
redirect_to accounts_path(cache_bust: SecureRandom.hex(6)), notice: t("enable_banking_items.link_existing_account.success"), status: :see_other
end
end

39
app/controllers/simplefin_items_controller.rb
View File

@@ -323,12 +323,20 @@ class SimplefinItemsController < ApplicationController
def select_existing_account
@account = Current.family.accounts.find(params[:account_id])
# Filter out SimpleFIN accounts that are already linked to any account
# (either via account_provider or legacy account association)
# Allow explicit relinking by listing all available SimpleFIN accounts for the family.
# The UI will surface the current mapping (if any), and the action will move the link.
@available_simplefin_accounts = Current.family.simplefin_items
.includes(:simplefin_accounts)
.includes(simplefin_accounts: [ :account, { account_provider: :account } ])
.flat_map(&:simplefin_accounts)
.reject { |sfa| sfa.account_provider.present? || sfa.account.present? }
# After provider setup, SFAs may already have an AccountProvider (linked to the freshly
# created duplicate accounts). During relink, we need to show those SFAs until the legacy
# link (`Account.simplefin_account_id`) has been cleared.
#
# Eligibility rule:
# - Show SFAs that are still legacy-linked (`sfa.account.present?`) => candidates to move.
# - Show SFAs that are fully unlinked (no legacy account and no account_provider) => candidates to link.
# - Hide SFAs that are linked via AccountProvider but no longer legacy-linked => already relinked.
.select { |sfa| sfa.account.present? || sfa.account_provider.nil? }
.sort_by { |sfa| sfa.updated_at || sfa.created_at }
.reverse
@@ -342,7 +350,7 @@ class SimplefinItemsController < ApplicationController
# Guard: only manual accounts can be linked (no existing provider links or legacy IDs)
if @account.account_providers.any? || @account.plaid_account_id.present? || @account.simplefin_account_id.present?
flash[:alert] = "Only manual accounts can be linked"
flash[:alert] = t("simplefin_items.link_existing_account.errors.only_manual")
if turbo_frame_request?
return render turbo_stream: Array(flash_notification_stream_items)
else
@@ -352,7 +360,7 @@ class SimplefinItemsController < ApplicationController
# Verify the SimpleFIN account belongs to this family's SimpleFIN items
unless Current.family.simplefin_items.include?(simplefin_account.simplefin_item)
flash[:alert] = "Invalid SimpleFIN account selected"
flash[:alert] = t("simplefin_items.link_existing_account.errors.invalid_simplefin_account")
if turbo_frame_request?
render turbo_stream: Array(flash_notification_stream_items)
else
@@ -364,9 +372,10 @@ class SimplefinItemsController < ApplicationController
# Relink behavior: detach any legacy link and point provider link at the chosen account
Account.transaction do
simplefin_account.lock!
# Clear legacy association if present
if simplefin_account.account_id.present?
simplefin_account.update!(account_id: nil)
# Clear legacy association if present (Account.simplefin_account_id)
if (legacy_account = simplefin_account.account)
legacy_account.update!(simplefin_account_id: nil)
end
# Upsert the AccountProvider mapping deterministically
@@ -382,9 +391,13 @@ class SimplefinItemsController < ApplicationController
if previous_account && previous_account.id != @account.id && previous_account.family_id == @account.family_id
begin
previous_account.reload
# Only disable if the previous account is truly orphaned (no other provider links)
# Only hide if the previous account is truly orphaned (no other provider links)
if previous_account.account_providers.none?
previous_account.disable!
# Disabled accounts still appear (greyed-out) in the manual list; for relink
# consolidation we want the duplicate to disappear from the UI.
# Use the app's standard deletion path (async) so the "pending_deletion" state
# remains truthful in the UI.
previous_account.destroy_later if previous_account.may_mark_for_deletion?
else
Rails.logger.info("Skipped disabling account ##{previous_account.id} after relink because it still has active provider links")
end
@@ -410,7 +423,7 @@ class SimplefinItemsController < ApplicationController
@simplefin_items = Current.family.simplefin_items.ordered.includes(:syncs)
build_simplefin_maps_for(@simplefin_items)
flash[:notice] = "Account successfully linked to SimpleFIN"
flash[:notice] = t("simplefin_items.link_existing_account.success")
@account.reload
manual_accounts_stream = if @manual_accounts.any?
turbo_stream.update(
@@ -434,7 +447,7 @@ class SimplefinItemsController < ApplicationController
turbo_stream.replace("modal", view_context.turbo_frame_tag("modal"))
] + Array(flash_notification_stream_items)
else
redirect_to accounts_path(cache_bust: SecureRandom.hex(6)), notice: "Account successfully linked to SimpleFIN", status: :see_other
redirect_to accounts_path(cache_bust: SecureRandom.hex(6)), notice: t("simplefin_items.link_existing_account.success"), status: :see_other
end
end

2
app/models/provider/lunchflow_adapter.rb
View File

@@ -7,7 +7,7 @@ class Provider::LunchflowAdapter < Provider::Base
# Define which account types this provider supports
def self.supported_account_types
%w[Depository CreditCard Loan]
%w[Depository CreditCard Loan Investment]
end
# Returns connection configurations for this provider

6
app/models/simplefin_item/unlinking.rb
View File

@@ -39,8 +39,10 @@ module SimplefinItem::Unlinking
end
# Legacy FK fallback: ensure any legacy link is cleared
if sfa.account_id.present?
sfa.update!(account: nil)
# NOTE: `SimplefinAccount#account_id` is the provider's external identifier.
# The legacy link is `Account.simplefin_account_id` (accessible via `sfa.account`).
if sfa.account.present?
sfa.account.update!(simplefin_account_id: nil)
end
end
rescue => e

11
app/views/simplefin_items/_simplefin_item.html.erb
View File

@@ -147,8 +147,15 @@
<% end %>
<%# Sync summary (collapsible) %>
<% stats = (@simplefin_sync_stats_map || {})[simplefin_item.id] || {} %>
<%# Sync summary (collapsible)
Prefer controller-provided map; fallback to latest sync stats so Turbo broadcasts
can render the summary without requiring a full page refresh. %>
<% stats = if defined?(@simplefin_sync_stats_map) && @simplefin_sync_stats_map
@simplefin_sync_stats_map[simplefin_item.id] || {}
else
# `latest_sync` is private on Syncable; access via association for broadcast renders.
simplefin_item.syncs.ordered.first&.sync_stats || {}
end %>
<% if stats.present? %>
<details class="group bg-surface rounded-lg border border-surface-inset/50">
<summary class="flex items-center justify-between gap-2 p-3 cursor-pointer">

9
app/views/simplefin_items/select_existing_account.html.erb
View File

@@ -6,10 +6,10 @@
<% dialog.with_body do %>
<% if @available_simplefin_accounts.blank? %>
<div class="p-4 text-sm text-secondary">
<p class="mb-2">All SimpleFIN accounts appear to be linked already.</p>
<p class="mb-2"><%= t("simplefin_items.select_existing_account.no_accounts_found") %></p>
<ul class="list-disc list-inside space-y-1">
<li>If you just connected or synced, try again after the sync completes.</li>
<li>To link a different account, first unlink it from the account’s actions menu.</li>
<li><%= t("simplefin_items.select_existing_account.wait_for_sync") %></li>
<li><%= t("simplefin_items.select_existing_account.check_provider_health") %></li>
</ul>
</div>
<% else %>
@@ -24,6 +24,9 @@
<span class="text-xs text-secondary">
<%= sfa.currency %> • Balance: <%= number_to_currency((sfa.current_balance || sfa.available_balance || 0), unit: sfa.currency) %>
</span>
<% if sfa.current_account.present? %>
<span class="text-xs text-secondary"><%= t("simplefin_items.select_existing_account.currently_linked_to", account_name: sfa.current_account.name) %></span>
<% end %>
</div>
</label>
<% end %>

8
config/locales/views/enable_banking_items/en.yml Normal file
View File

@@ -0,0 +1,8 @@
---
en:
enable_banking_items:
link_existing_account:
success: Account successfully linked to Enable Banking
errors:
only_manual: Only manual accounts can be linked
invalid_enable_banking_account: Invalid Enable Banking account selected

13
config/locales/views/simplefin_items/en.yml
View File

@@ -57,4 +57,15 @@ en:
title: "Link %{account_name} to SimpleFIN"
description: Select a SimpleFIN account to link to your existing account
cancel: Cancel
link_account: Link account
link_account: Link account
no_accounts_found: No SimpleFIN accounts found for this family.
wait_for_sync: If you just connected or synced, try again after the sync completes.
unlink_to_move: To move a link, first unlink it from the account’s actions menu.
all_accounts_already_linked: All SimpleFIN accounts appear to be linked already.
currently_linked_to: "Currently linked to: %{account_name}"
link_existing_account:
success: Account successfully linked to SimpleFIN
errors:
only_manual: Only manual accounts can be linked
invalid_simplefin_account: Invalid SimpleFIN account selected

4
config/locales/views/simplefin_items/pt-BR.yml
View File

@@ -58,3 +58,7 @@ pt-BR:
description: Selecione uma conta do SimpleFIN para vincular à sua conta existente
cancel: Cancelar
link_account: Vincular conta
no_accounts_found: Nenhuma conta SimpleFIN encontrada para esta família.
wait_for_sync: Se você acabou de conectar ou sincronizar, tente novamente após a conclusão da sincronização.
check_provider_health: Verifique se sua conexão SimpleFIN está ativa em Configurações → Provedores.
currently_linked_to: "Atualmente vinculada a: %{account_name}"

106
test/controllers/simplefin_items_controller_test.rb
View File

@@ -95,16 +95,56 @@ class SimplefinItemsControllerTest < ActionDispatch::IntegrationTest
assert_response :see_other
# Reload and assert: account A should still be enabled (not disabled) because it has another provider link
# Reload and assert: account A should not be hidden 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"
refute_equal "pending_deletion", account_a.status, "previous account should not be hidden 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 "relink hides a previously linked orphaned duplicate account" do
account_a = Account.create!(
family: @family,
name: "Duplicate A",
balance: 0,
currency: "USD",
accountable_type: "Depository",
accountable: Depository.create!(subtype: "checking")
)
account_b = Account.create!(
family: @family,
name: "Target B",
balance: 0,
currency: "USD",
accountable_type: "Depository",
accountable: Depository.create!(subtype: "savings")
)
sfa = @simplefin_item.simplefin_accounts.create!(
name: "SF For Duplicate",
account_id: "sf_dup_1",
account_type: "depository",
currency: "USD",
current_balance: 0
)
AccountProvider.create!(account: account_a, provider: sfa)
post link_existing_account_simplefin_items_path, params: {
account_id: account_b.id,
simplefin_account_id: sfa.id
}
assert_response :see_other
account_a.reload
assert_equal "pending_deletion", account_a.status, "expected orphaned duplicate to be hidden after relink"
end
test "should get edit" do
@simplefin_item.update!(status: :requires_update)
get edit_simplefin_item_url(@simplefin_item)
@@ -301,12 +341,70 @@ class SimplefinItemsControllerTest < ActionDispatch::IntegrationTest
assert @simplefin_item.scheduled_for_deletion?
end
test "select_existing_account renders empty-state modal when no available simplefin accounts" do
test "select_existing_account renders empty-state modal when no simplefin accounts exist" do
account = accounts(:depository)
get select_existing_account_simplefin_items_url(account_id: account.id)
assert_response :success
assert_includes @response.body, "All SimpleFIN accounts appear to be linked already."
assert_includes @response.body, "No SimpleFIN accounts found for this family."
end
test "select_existing_account lists simplefin accounts even when they are already linked" do
account = accounts(:depository)
sfa = @simplefin_item.simplefin_accounts.create!(
name: "Linked SF",
account_id: "sf_linked_123",
currency: "USD",
current_balance: 10,
account_type: "depository"
)
linked_account = Account.create!(
family: @family,
name: "Existing Linked Account",
currency: "USD",
balance: 0,
accountable_type: "Depository",
accountable: Depository.create!(subtype: "checking")
)
# Model the pre-relink state: the provider account is linked to a newly set up duplicate
# via the legacy FK, and may also have an AccountProvider.
linked_account.update!(simplefin_account_id: sfa.id)
sfa.update!(account: linked_account)
AccountProvider.create!(account: linked_account, provider: sfa)
get select_existing_account_simplefin_items_url(account_id: account.id)
assert_response :success
assert_includes @response.body, "Linked SF"
assert_includes @response.body, "Currently linked to: Existing Linked Account"
end
test "select_existing_account hides simplefin accounts after they have been relinked" do
account = accounts(:depository)
sfa = @simplefin_item.simplefin_accounts.create!(
name: "Relinked SF",
account_id: "sf_relinked_123",
currency: "USD",
current_balance: 10,
account_type: "depository"
)
# Simulate post-relink state: legacy link cleared, AccountProvider exists.
linked_account = Account.create!(
family: @family,
name: "Final Linked Account",
currency: "USD",
balance: 0,
accountable_type: "Depository",
accountable: Depository.create!(subtype: "checking")
)
AccountProvider.create!(account: linked_account, provider: sfa)
get select_existing_account_simplefin_items_url(account_id: account.id)
assert_response :success
refute_includes @response.body, "Relinked SF"
end
test "destroy should unlink provider links and legacy fk" do
# Create SFA and linked Account with AccountProvider

7
test/models/provider/lunchflow_adapter_test.rb Normal file
View File

@@ -0,0 +1,7 @@
require "test_helper"
class Provider::LunchflowAdapterTest < ActiveSupport::TestCase
test "supports Investment accounts" do
assert_includes Provider::LunchflowAdapter.supported_account_types, "Investment"
end
end