mirror of
https://github.com/we-promise/sure.git
synced 2026-04-19 20:14:08 +00:00
Add stale SimpleFin account detection and improve unlink cleanup (#574)
* Add stale account detection and handling in SimpleFin setup - Introduced UI for managing stale accounts during SimpleFin setup. - Added logic to detect accounts no longer provided by SimpleFin. - Implemented actions to delete, move transactions, or skip stale accounts. - Updated `simplefin_items_controller` with stale account processing and handling. - Enhanced tests to validate stale account scenarios, including detection, deletion, moving transactions, and skipping. * Update SimpleFin to SimpleFIN in locale file Signed-off-by: Juan José Mata <jjmata@jjmata.com> * Silly changes break things ... Signed-off-by: Juan José Mata <jjmata@jjmata.com> * Refactor stale account processing and UI handling - Moved `target_account.sync_later` to execute after commit for proper recalculation of balances. - Added additional safeguard in JavaScript to check for `moveRadioTarget` before updating target visibility. * More silly capitalization changes * Enhance stale account action handling in SimpleFIN setup - Introduced `permitted_stale_account_actions` to validate and permit nested `stale_account_actions` parameters. - Updated `complete_account_setup` to use the new method for safer processing. - Corrected capitalization in SimpleFIN update success and error messages. * Add error tracking and UI feedback for stale account actions - Updated `process_stale_account_actions` to track errors for delete and move actions. - Enhanced UI to display success and error messages for stale account processing. - Implemented destruction of conflicting transfers during account move to maintain data integrity. * Refactor transfer destruction and improve SimpleFIN account setup messages - Updated `simplefin_items_controller` to use `find_each(&:destroy!)` for transfer deletions, ensuring callbacks are invoked. - Enhanced localization for success messages in account creation to handle singular and plural cases. --------- Signed-off-by: Juan José Mata <jjmata@jjmata.com> Co-authored-by: Josh Waldrep <joshua.waldrep5+github@gmail.com> Co-authored-by: Juan José Mata <jjmata@jjmata.com>
This commit is contained in:
LPW
@@ -141,11 +141,27 @@ class AccountsController < ApplicationController
|
||||
|
||||
begin
|
||||
Account.transaction do
|
||||
# Detach holdings from provider links before destroying them
|
||||
provider_link_ids = @account.account_providers.pluck(:id)
|
||||
if provider_link_ids.any?
|
||||
Holding.where(account_provider_id: provider_link_ids).update_all(account_provider_id: nil)
|
||||
end
|
||||
|
||||
# Capture SimplefinAccount before clearing FK (so we can destroy it)
|
||||
simplefin_account_to_destroy = @account.simplefin_account
|
||||
|
||||
# Remove new system links (account_providers join table)
|
||||
@account.account_providers.destroy_all
|
||||
|
||||
# Remove legacy system links (foreign keys)
|
||||
@account.update!(plaid_account_id: nil, simplefin_account_id: nil)
|
||||
|
||||
# Destroy the SimplefinAccount record so it doesn't cause stale account issues
|
||||
# This is safe because:
|
||||
# - Account data (transactions, holdings, balances) lives on the Account, not SimplefinAccount
|
||||
# - 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!
|
||||
end
|
||||
|
||||
redirect_to accounts_path, notice: t("accounts.unlink.success")
|
||||
|
||||
@@ -201,17 +201,33 @@ class SimplefinItemsController < ApplicationController
|
||||
message: "No additional options needed for Other Assets."
|
||||
}
|
||||
}
|
||||
|
||||
# Detect stale accounts: linked in DB but no longer in upstream SimpleFin API
|
||||
@stale_simplefin_accounts = detect_stale_simplefin_accounts
|
||||
if @stale_simplefin_accounts.any?
|
||||
# Build list of target accounts for "move transactions to" dropdown
|
||||
# Only show accounts from this SimpleFin connection (excluding stale ones)
|
||||
stale_account_ids = @stale_simplefin_accounts.map { |sfa| sfa.current_account&.id }.compact
|
||||
@target_accounts = @simplefin_item.accounts
|
||||
.reject { |acct| stale_account_ids.include?(acct.id) }
|
||||
.sort_by(&:name)
|
||||
end
|
||||
end
|
||||
|
||||
def complete_account_setup
|
||||
account_types = params[:account_types] || {}
|
||||
account_subtypes = params[:account_subtypes] || {}
|
||||
stale_account_actions = permitted_stale_account_actions
|
||||
|
||||
# Update sync start date from form
|
||||
if params[:sync_start_date].present?
|
||||
@simplefin_item.update!(sync_start_date: params[:sync_start_date])
|
||||
end
|
||||
|
||||
# Process stale account actions first
|
||||
stale_results = process_stale_account_actions(stale_account_actions)
|
||||
stale_action_errors = stale_results[:errors] || []
|
||||
|
||||
# Valid account types for this provider (plus Crypto and OtherAsset which SimpleFIN UI allows)
|
||||
valid_types = Provider::SimplefinAdapter.supported_account_types + [ "Crypto", "OtherAsset" ]
|
||||
|
||||
@@ -275,6 +291,17 @@ class SimplefinItemsController < ApplicationController
|
||||
else
|
||||
flash[:notice] = t(".no_accounts")
|
||||
end
|
||||
|
||||
# Add stale account results to flash
|
||||
if stale_results[:deleted] > 0 || stale_results[:moved] > 0
|
||||
stale_message = t(".stale_accounts_processed", deleted: stale_results[:deleted], moved: stale_results[:moved])
|
||||
flash[:notice] = [ flash[:notice], stale_message ].compact.join(" ")
|
||||
end
|
||||
|
||||
# Warn about any stale account action failures
|
||||
if stale_action_errors.any?
|
||||
flash[:alert] = t(".stale_accounts_errors", count: stale_action_errors.size)
|
||||
end
|
||||
if turbo_frame_request?
|
||||
# Recompute data needed by Accounts#index partials
|
||||
@manual_accounts = Account.uncached {
|
||||
@@ -451,6 +478,24 @@ class SimplefinItemsController < ApplicationController
|
||||
params.require(:simplefin_item).permit(:setup_token, :sync_start_date)
|
||||
end
|
||||
|
||||
def permitted_stale_account_actions
|
||||
return {} unless params[:stale_account_actions].is_a?(ActionController::Parameters)
|
||||
|
||||
# Permit the nested structure: stale_account_actions[simplefin_account_id][action|target_account_id]
|
||||
params[:stale_account_actions].to_unsafe_h.each_with_object({}) do |(simplefin_account_id, action_params), result|
|
||||
next unless simplefin_account_id.present? && action_params.is_a?(Hash)
|
||||
|
||||
# Validate simplefin_account_id is a valid UUID format to prevent injection
|
||||
next unless simplefin_account_id.to_s.match?(/\A[0-9a-f-]+\z/i)
|
||||
|
||||
permitted = {}
|
||||
permitted[:action] = action_params[:action] if %w[delete move skip].include?(action_params[:action])
|
||||
permitted[:target_account_id] = action_params[:target_account_id] if action_params[:target_account_id].present?
|
||||
|
||||
result[simplefin_account_id] = permitted if permitted[:action].present?
|
||||
end
|
||||
end
|
||||
|
||||
def render_error(message, setup_token = nil, context: :new)
|
||||
if context == :edit
|
||||
# Keep the persisted record and assign the token for re-render
|
||||
@@ -472,4 +517,106 @@ class SimplefinItemsController < ApplicationController
|
||||
render context, status: :unprocessable_entity
|
||||
end
|
||||
end
|
||||
|
||||
# Detect stale SimpleFin accounts: linked in DB but no longer in upstream API
|
||||
def detect_stale_simplefin_accounts
|
||||
# Get upstream account IDs from the last sync's raw_payload
|
||||
raw_payload = @simplefin_item.raw_payload
|
||||
return [] if raw_payload.blank?
|
||||
|
||||
upstream_ids = raw_payload.with_indifferent_access[:accounts]&.map { |a| a[:id].to_s } || []
|
||||
return [] if upstream_ids.empty?
|
||||
|
||||
# Find SimplefinAccounts that are linked but not in upstream
|
||||
@simplefin_item.simplefin_accounts
|
||||
.includes(:account, account_provider: :account)
|
||||
.select { |sfa| sfa.current_account.present? && !upstream_ids.include?(sfa.account_id) }
|
||||
end
|
||||
|
||||
# Process user-selected actions for stale accounts
|
||||
def process_stale_account_actions(stale_actions)
|
||||
results = { deleted: 0, moved: 0, skipped: 0, errors: [] }
|
||||
return results if stale_actions.blank?
|
||||
|
||||
stale_actions.each do |simplefin_account_id, action_params|
|
||||
action = action_params[:action]
|
||||
next if action.blank? || action == "skip"
|
||||
|
||||
sfa = @simplefin_item.simplefin_accounts.find_by(id: simplefin_account_id)
|
||||
next unless sfa
|
||||
|
||||
account = sfa.current_account
|
||||
next unless account
|
||||
|
||||
case action
|
||||
when "delete"
|
||||
if handle_stale_account_delete(sfa, account)
|
||||
results[:deleted] += 1
|
||||
else
|
||||
results[:errors] << { account: account.name, action: "delete" }
|
||||
end
|
||||
when "move"
|
||||
target_account_id = action_params[:target_account_id]
|
||||
if target_account_id.present? && handle_stale_account_move(sfa, account, target_account_id)
|
||||
results[:moved] += 1
|
||||
else
|
||||
results[:errors] << { account: account.name, action: "move" }
|
||||
end
|
||||
else
|
||||
results[:skipped] += 1
|
||||
end
|
||||
end
|
||||
|
||||
results
|
||||
end
|
||||
|
||||
def handle_stale_account_delete(simplefin_account, account)
|
||||
ActiveRecord::Base.transaction do
|
||||
# Destroy the Account (cascades to entries/holdings)
|
||||
account.destroy!
|
||||
# Destroy the SimplefinAccount
|
||||
simplefin_account.destroy!
|
||||
end
|
||||
true
|
||||
rescue => e
|
||||
Rails.logger.error("Failed to delete stale account: #{e.class} - #{e.message}")
|
||||
false
|
||||
end
|
||||
|
||||
def handle_stale_account_move(simplefin_account, source_account, target_account_id)
|
||||
target_account = @simplefin_item.accounts.find { |acct| acct.id.to_s == target_account_id.to_s }
|
||||
return false unless target_account
|
||||
|
||||
ActiveRecord::Base.transaction do
|
||||
# Handle transfers that would become invalid after moving entries.
|
||||
# Transfers linking source entries to target entries would end up with both
|
||||
# entries in the same account, violating transfer_has_different_accounts validation.
|
||||
source_entry_ids = source_account.entries.pluck(:id)
|
||||
target_entry_ids = target_account.entries.pluck(:id)
|
||||
|
||||
if source_entry_ids.any? && target_entry_ids.any?
|
||||
# Find and destroy transfers between source and target accounts
|
||||
# Use find_each + destroy! to invoke Transfer's custom destroy! callbacks
|
||||
# which reset transaction kinds to "standard"
|
||||
Transfer.where(inflow_transaction_id: source_entry_ids, outflow_transaction_id: target_entry_ids)
|
||||
.or(Transfer.where(inflow_transaction_id: target_entry_ids, outflow_transaction_id: source_entry_ids))
|
||||
.find_each(&:destroy!)
|
||||
end
|
||||
|
||||
# Move all entries to target account
|
||||
source_account.entries.update_all(account_id: target_account.id)
|
||||
|
||||
# Destroy the now-empty source account
|
||||
source_account.destroy!
|
||||
# Destroy the SimplefinAccount
|
||||
simplefin_account.destroy!
|
||||
end
|
||||
|
||||
# Trigger sync on target account to recalculate balances (after commit)
|
||||
target_account.sync_later
|
||||
true
|
||||
rescue => e
|
||||
Rails.logger.error("Failed to move transactions from stale account: #{e.class} - #{e.message}")
|
||||
false
|
||||
end
|
||||
end
|
||||
|
||||
@@ -0,0 +1,25 @@
|
||||
import { Controller } from "@hotwired/stimulus"
|
||||
|
||||
export default class extends Controller {
|
||||
static targets = ["moveRadio", "targetSelect"]
|
||||
static values = { accountId: String }
|
||||
|
||||
connect() {
|
||||
this.updateTargetVisibility()
|
||||
}
|
||||
|
||||
updateTargetVisibility() {
|
||||
if (!this.hasTargetSelectTarget || !this.hasMoveRadioTarget) return
|
||||
|
||||
const moveRadio = this.moveRadioTarget
|
||||
const targetSelect = this.targetSelectTarget
|
||||
|
||||
if (moveRadio?.checked) {
|
||||
targetSelect.disabled = false
|
||||
targetSelect.classList.remove("opacity-50", "cursor-not-allowed")
|
||||
} else {
|
||||
targetSelect.disabled = true
|
||||
targetSelect.classList.add("opacity-50", "cursor-not-allowed")
|
||||
}
|
||||
}
|
||||
}
|
||||
65
app/views/simplefin_items/_stale_account_row.html.erb
Normal file
65
app/views/simplefin_items/_stale_account_row.html.erb
Normal file
@@ -0,0 +1,65 @@
|
||||
<% account = simplefin_account.current_account %>
|
||||
<% transaction_count = account&.entries&.where(entryable_type: "Transaction")&.count || 0 %>
|
||||
|
||||
<div class="rounded-lg p-4 border border-warning/50 bg-warning/5 mb-4"
|
||||
data-controller="stale-account-action"
|
||||
data-stale-account-action-account-id-value="<%= simplefin_account.id %>">
|
||||
<div class="flex items-start justify-between mb-4">
|
||||
<div>
|
||||
<h3 class="font-medium text-primary">
|
||||
<%= simplefin_account.name %>
|
||||
<% if simplefin_account.org_data.present? && simplefin_account.org_data['name'].present? %>
|
||||
<span class="text-secondary">• <%= simplefin_account.org_data["name"] %></span>
|
||||
<% end %>
|
||||
</h3>
|
||||
<p class="text-sm text-secondary">
|
||||
<%= number_to_currency(simplefin_account.current_balance || 0, unit: simplefin_account.currency || "USD") %>
|
||||
<span class="mx-1">•</span>
|
||||
<%= t("simplefin_items.setup_accounts.stale_accounts.transaction_count", count: transaction_count) %>
|
||||
</p>
|
||||
</div>
|
||||
</div>
|
||||
|
||||
<fieldset class="space-y-3">
|
||||
<legend class="text-sm text-secondary mb-2">
|
||||
<%= t("simplefin_items.setup_accounts.stale_accounts.action_prompt") %>
|
||||
</legend>
|
||||
|
||||
<label class="flex items-center gap-2 cursor-pointer">
|
||||
<%= radio_button_tag "stale_account_actions[#{simplefin_account.id}][action]",
|
||||
"delete",
|
||||
false,
|
||||
class: "form-radio accent-primary",
|
||||
data: { action: "change->stale-account-action#updateTargetVisibility" } %>
|
||||
<span class="text-sm text-primary"><%= t("simplefin_items.setup_accounts.stale_accounts.action_delete") %></span>
|
||||
</label>
|
||||
|
||||
<% if target_accounts&.any? %>
|
||||
<div class="flex flex-col gap-2">
|
||||
<label class="flex items-center gap-2 cursor-pointer">
|
||||
<%= radio_button_tag "stale_account_actions[#{simplefin_account.id}][action]",
|
||||
"move",
|
||||
false,
|
||||
class: "form-radio accent-primary",
|
||||
data: { action: "change->stale-account-action#updateTargetVisibility",
|
||||
stale_account_action_target: "moveRadio" } %>
|
||||
<span class="text-sm text-primary"><%= t("simplefin_items.setup_accounts.stale_accounts.action_move") %></span>
|
||||
</label>
|
||||
<%= select_tag "stale_account_actions[#{simplefin_account.id}][target_account_id]",
|
||||
options_from_collection_for_select(target_accounts, :id, :name),
|
||||
class: "appearance-none bg-container border border-primary rounded-md px-2 py-1 text-sm text-primary focus:border-primary focus:ring-1 focus:ring-primary focus:outline-none ml-6 max-w-[200px] truncate disabled:opacity-50 disabled:cursor-not-allowed",
|
||||
disabled: true,
|
||||
data: { stale_account_action_target: "targetSelect" } %>
|
||||
</div>
|
||||
<% end %>
|
||||
|
||||
<label class="flex items-center gap-2 cursor-pointer">
|
||||
<%= radio_button_tag "stale_account_actions[#{simplefin_account.id}][action]",
|
||||
"skip",
|
||||
true,
|
||||
class: "form-radio accent-primary",
|
||||
data: { action: "change->stale-account-action#updateTargetVisibility" } %>
|
||||
<span class="text-sm text-primary"><%= t("simplefin_items.setup_accounts.stale_accounts.action_skip") %></span>
|
||||
</label>
|
||||
</fieldset>
|
||||
</div>
|
||||
@@ -99,6 +99,29 @@
|
||||
</div>
|
||||
</div>
|
||||
<% end %>
|
||||
|
||||
<% if @stale_simplefin_accounts&.any? %>
|
||||
<!-- Stale Accounts Section -->
|
||||
<div class="border-t border-primary mt-6 pt-6">
|
||||
<div class="bg-warning/10 border border-warning rounded-lg p-4 mb-4">
|
||||
<div class="flex items-start gap-3">
|
||||
<%= icon "alert-triangle", size: "sm", class: "text-warning mt-0.5 flex-shrink-0" %>
|
||||
<div>
|
||||
<p class="text-sm text-primary font-medium mb-1">
|
||||
<%= t("simplefin_items.setup_accounts.stale_accounts.title") %>
|
||||
</p>
|
||||
<p class="text-xs text-secondary">
|
||||
<%= t("simplefin_items.setup_accounts.stale_accounts.description") %>
|
||||
</p>
|
||||
</div>
|
||||
</div>
|
||||
</div>
|
||||
|
||||
<% @stale_simplefin_accounts.each do |simplefin_account| %>
|
||||
<%= render "stale_account_row", simplefin_account: simplefin_account, target_accounts: @target_accounts %>
|
||||
<% end %>
|
||||
</div>
|
||||
<% end %>
|
||||
</div>
|
||||
|
||||
<div class="flex gap-3">
|
||||
|
||||
Reference in New Issue
Block a user