From 8b4bb64a23f263c731a274ffc0192eae027185ce Mon Sep 17 00:00:00 2001 From: Lobster Date: Fri, 17 Jul 2026 16:49:31 -0400 Subject: [PATCH] fix(lunchflow): prune orphaned accounts deleted upstream (#1861) (#1886) * fix(lunchflow): prune orphaned accounts deleted upstream (#1861) Accounts deleted in Lunch Flow lingered in Sure as unlinked LunchflowAccount records, permanently pinning the item to "Need setup". The importer created/updated accounts returned by the API but never removed records for accounts that disappeared upstream. Add prune_orphaned_lunchflow_accounts, mirroring SimpleFin's prune_orphaned_simplefin_accounts: after importing, delete LunchflowAccount records whose account_id is no longer returned upstream and that are not linked to an Account via AccountProvider. Linked accounts are kept so the prune never cascade-destroys a user's Account. The prune is guarded to a non-empty upstream list so a transient empty/failed response can't wipe out all accounts. Fixes #1861 * fix(lunchflow): prune NULL-id orphans + consistent import return shape (#1886) * fix(lunchflow): make orphan prune resilient to destroy failures Wrap the per-record destroy in begin/rescue so a single failed prune doesn't abort the whole import. Pruning runs before transaction fetch, so an unhandled error would have blocked transaction syncing entirely. Matches the importer's existing continue-on-error convention. * fix(lunchflow): only count pruned accounts when destroy succeeds destroy returns false (without raising) when a before_destroy callback halts deletion; the prior code incremented the pruned count regardless, which could inflate it. Increment only on success and log when halted. --- app/models/lunchflow_item/importer.rb | 68 +++++++++- .../importer_orphan_prune_test.rb | 121 ++++++++++++++++++ 2 files changed, 187 insertions(+), 2 deletions(-) create mode 100644 test/models/lunchflow_item/importer_orphan_prune_test.rb diff --git a/app/models/lunchflow_item/importer.rb b/app/models/lunchflow_item/importer.rb index cdfd60433..0a6003947 100644 --- a/app/models/lunchflow_item/importer.rb +++ b/app/models/lunchflow_item/importer.rb @@ -15,7 +15,16 @@ class LunchflowItem::Importer accounts_data = fetch_accounts_data unless accounts_data Rails.logger.error "LunchflowItem::Importer - Failed to fetch accounts data for item #{lunchflow_item.id}" - return { success: false, error: "Failed to fetch accounts data", accounts_imported: 0, transactions_imported: 0 } + return { + success: false, + error: "Failed to fetch accounts data", + accounts_updated: 0, + accounts_created: 0, + accounts_failed: 0, + accounts_pruned: 0, + transactions_imported: 0, + transactions_failed: 0 + } end # Store raw payload @@ -30,6 +39,7 @@ class LunchflowItem::Importer accounts_updated = 0 accounts_created = 0 accounts_failed = 0 + accounts_pruned = 0 if accounts_data[:accounts].present? # Get linked lunchflow account IDs (ones actually imported/used by the user) @@ -73,9 +83,16 @@ class LunchflowItem::Importer end end end + + # Remove records for accounts that no longer exist upstream so they don't + # linger as unlinked "Need setup" accounts (#1861). Guarded to a non-empty + # upstream list so a transient empty/failed response can't wipe out all + # accounts. + upstream_account_ids = accounts_data[:accounts].filter_map { |a| a[:id].to_s.presence } + accounts_pruned = prune_orphaned_lunchflow_accounts(upstream_account_ids) end - Rails.logger.info "LunchflowItem::Importer - Updated #{accounts_updated} accounts, created #{accounts_created} new (#{accounts_failed} failed)" + Rails.logger.info "LunchflowItem::Importer - Updated #{accounts_updated} accounts, created #{accounts_created} new (#{accounts_failed} failed), pruned #{accounts_pruned}" # Step 3: Fetch transactions only for linked accounts with active status transactions_imported = 0 @@ -103,6 +120,7 @@ class LunchflowItem::Importer accounts_updated: accounts_updated, accounts_created: accounts_created, accounts_failed: accounts_failed, + accounts_pruned: accounts_pruned, transactions_imported: transactions_imported, transactions_failed: transactions_failed } @@ -110,6 +128,52 @@ class LunchflowItem::Importer private + # Removes LunchflowAccount records that no longer exist upstream and are not + # linked to any Account, so accounts deleted in Lunch Flow stop lingering as + # unlinked records that pin the item to "Need setup" (#1861). Mirrors + # SimpleFin's prune_orphaned_simplefin_accounts. LunchFlow linkage is only + # via AccountProvider (no legacy FK), so a present account_provider means keep. + # + # account_id is nullable on lunchflow_accounts. A NULL account_id can never + # match an upstream id, and `where.not(account_id: upstream_account_ids)` + # alone would silently drop those rows (SQL `NULL NOT IN (...)` is never + # TRUE). We explicitly OR them back in so a NULL-id unlinked record — which + # has no upstream identity and would otherwise pin the item to "Need setup" + # forever — is also pruned. The per-record account_provider guard below still + # protects any linked record regardless of its account_id. + def prune_orphaned_lunchflow_accounts(upstream_account_ids) + return 0 if upstream_account_ids.blank? + + scope = lunchflow_item.lunchflow_accounts.includes(:account_provider) + orphaned = scope + .where.not(account_id: upstream_account_ids) + .or(scope.where(account_id: nil)) + + pruned = 0 + orphaned.each do |lunchflow_account| + if lunchflow_account.account_provider.present? + Rails.logger.info "LunchflowItem::Importer - Keeping stale LunchflowAccount id=#{lunchflow_account.id} account_id=#{lunchflow_account.account_id} (still linked to Account)" + next + end + + begin + Rails.logger.info "LunchflowItem::Importer - Pruning orphaned LunchflowAccount id=#{lunchflow_account.id} account_id=#{lunchflow_account.account_id} (no longer exists upstream)" + if lunchflow_account.destroy + pruned += 1 + else + # A before_destroy callback halted deletion — don't inflate the count. + Rails.logger.warn "LunchflowItem::Importer - Destroy halted for LunchflowAccount id=#{lunchflow_account.id}; not counting as pruned" + end + rescue => e + # Don't let one failed destroy abort the whole import (transactions are + # fetched in a later step). Mirrors the importer's continue-on-error style. + Rails.logger.error "LunchflowItem::Importer - Failed to prune LunchflowAccount id=#{lunchflow_account.id}: #{e.message}" + end + end + + pruned + end + def fetch_accounts_data begin accounts_data = lunchflow_provider.get_accounts diff --git a/test/models/lunchflow_item/importer_orphan_prune_test.rb b/test/models/lunchflow_item/importer_orphan_prune_test.rb new file mode 100644 index 000000000..909624460 --- /dev/null +++ b/test/models/lunchflow_item/importer_orphan_prune_test.rb @@ -0,0 +1,121 @@ +require "test_helper" + +class LunchflowItem::ImporterOrphanPruneTest < ActiveSupport::TestCase + setup do + @family = families(:dylan_family) + @item = LunchflowItem.create!( + family: @family, + name: "Test Lunchflow", + api_key: "test_key_123", + status: :good + ) + @importer = LunchflowItem::Importer.new(@item, lunchflow_provider: mock()) + end + + test "prunes orphaned unlinked LunchflowAccount no longer returned upstream" do + orphan = @item.lunchflow_accounts.create!( + account_id: "acct-old", + name: "Deleted Account", + currency: "USD" + ) + + pruned = @importer.send(:prune_orphaned_lunchflow_accounts, [ "acct-new" ]) + + assert_equal 1, pruned + assert_nil LunchflowAccount.find_by(id: orphan.id), "orphaned unlinked account should be deleted" + end + + test "keeps unlinked LunchflowAccount that is still returned upstream" do + still_present = @item.lunchflow_accounts.create!( + account_id: "acct-keep", + name: "Active Account", + currency: "USD" + ) + + pruned = @importer.send(:prune_orphaned_lunchflow_accounts, [ "acct-keep" ]) + + assert_equal 0, pruned + assert_not_nil LunchflowAccount.find_by(id: still_present.id) + end + + test "does not prune a LunchflowAccount linked via AccountProvider" do + linked = @item.lunchflow_accounts.create!( + account_id: "acct-linked", + name: "Linked Account", + currency: "USD" + ) + account = @family.accounts.create!( + name: "Linked Checking", + balance: 100, + currency: "USD", + accountable: Depository.new(subtype: "checking") + ) + AccountProvider.create!(account: account, provider: linked) + + # Even though it's gone upstream, a linked account must be kept (deleting it + # would cascade-destroy the AccountProvider and orphan the user's Account). + pruned = @importer.send(:prune_orphaned_lunchflow_accounts, [ "acct-other" ]) + + assert_equal 0, pruned + assert_not_nil LunchflowAccount.find_by(id: linked.id), "linked account should not be deleted" + end + + test "blank upstream list is a no-op so transient failures cannot wipe accounts" do + orphan = @item.lunchflow_accounts.create!( + account_id: "acct-old", + name: "Account", + currency: "USD" + ) + + assert_equal 0, @importer.send(:prune_orphaned_lunchflow_accounts, []) + assert_not_nil LunchflowAccount.find_by(id: orphan.id), "must not prune when upstream list is empty" + end + + test "prunes multiple orphaned unlinked accounts" do + orphan1 = @item.lunchflow_accounts.create!(account_id: "old-1", name: "One", currency: "USD") + orphan2 = @item.lunchflow_accounts.create!(account_id: "old-2", name: "Two", currency: "USD") + kept = @item.lunchflow_accounts.create!(account_id: "new-1", name: "Three", currency: "USD") + + pruned = @importer.send(:prune_orphaned_lunchflow_accounts, [ "new-1" ]) + + assert_equal 2, pruned + assert_nil LunchflowAccount.find_by(id: orphan1.id) + assert_nil LunchflowAccount.find_by(id: orphan2.id) + assert_not_nil LunchflowAccount.find_by(id: kept.id) + end + + test "prunes an unlinked LunchflowAccount with a nil account_id" do + # account_id is nullable; a NULL id can never match upstream and would be + # silently skipped by a plain `where.not(account_id: ...)` (SQL three-valued + # logic). Such an unlinked record is a genuine orphan and must be pruned. + orphan = @item.lunchflow_accounts.create!( + account_id: nil, + name: "Never received an upstream id", + currency: "USD" + ) + + pruned = @importer.send(:prune_orphaned_lunchflow_accounts, [ "acct-new" ]) + + assert_equal 1, pruned + assert_nil LunchflowAccount.find_by(id: orphan.id), "nil account_id orphan should be pruned" + end + + test "import returns accounts_pruned in its result and prunes orphans end-to-end" do + orphan = @item.lunchflow_accounts.create!( + account_id: "acct-old", + name: "Deleted Account", + currency: "USD" + ) + + provider = mock() + provider.stubs(:get_accounts).returns( + accounts: [ { id: "acct-new", name: "New Account", currency: "USD" } ] + ) + + importer = LunchflowItem::Importer.new(@item, lunchflow_provider: provider) + result = importer.import + + assert_equal 1, result[:accounts_pruned] + assert_nil LunchflowAccount.find_by(id: orphan.id), "orphan should be pruned through import" + end +end