From c274c5d8bbce80a50a378dca16a72a6562193aaa Mon Sep 17 00:00:00 2001 From: Guillem Arias Fauste Date: Wed, 3 Jun 2026 00:04:32 +0200 Subject: [PATCH] fix(recurring): match transfer pairs so Cleaner stops mis-retiring transfers (#2110) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #1590. Implements Option A (the proper fix), replacing the interim skip. A recurring transfer's name is seeded as "Transfer to {dest}", but future occurrences carry arbitrary names (user free-text, importer wording, the auto-matcher), so the name-based matching_transactions returned [] and the Cleaner retired still-active transfers at the 6-month threshold. main worked around this by skipping transfer rows entirely (Option B) — which also meant a genuinely-stopped transfer never got retired. matching_transactions now detects the Transfer *pair* for transfer rows: an outflow on the source account paired with an inflow on the destination account, within the usual amount/cadence window. The Cleaner no longer skips transfers: - a transfer whose pair still occurs keeps surfacing recent matches → stays active - a transfer whose pair has stopped → correctly retired The amount / day-of-month scopes are extracted and shared between the name-based and pair-based paths. The Identifier's separate transfer skip (auto-identifying pairs from history) is intentionally untouched — that's the out-of-scope feature the issue defers. --- app/models/recurring_transaction.rb | 71 ++++++++++++++------- app/models/recurring_transaction/cleaner.rb | 11 ++-- test/models/recurring_transaction_test.rb | 60 +++++++++++++++-- 3 files changed, 106 insertions(+), 36 deletions(-) diff --git a/app/models/recurring_transaction.rb b/app/models/recurring_transaction.rb index 090d31549..f05df6fcb 100644 --- a/app/models/recurring_transaction.rb +++ b/app/models/recurring_transaction.rb @@ -260,29 +260,15 @@ class RecurringTransaction < ApplicationRecord # Find matching transactions for this recurring pattern def matching_transactions - # For manual recurring with amount variance, match within range - # For automatic recurring, match exact amount - base = account.present? ? account.entries : family.entries + # Recurring transfers can't be matched by single-account name/amount — + # future occurrences carry arbitrary names — so match the Transfer pair. + return transfer_matching_transactions if transfer? - entries = if manual? && has_amount_variance? - base - .where(entryable_type: "Transaction") - .where(currency: currency) - .where("entries.amount BETWEEN ? AND ?", expected_amount_min, expected_amount_max) - .where("EXTRACT(DAY FROM entries.date) BETWEEN ? AND ?", - [ expected_day_of_month - 2, 1 ].max, - [ expected_day_of_month + 2, 31 ].min) - .order(date: :desc) - else - base - .where(entryable_type: "Transaction") - .where(currency: currency) - .where("entries.amount = ?", amount) - .where("EXTRACT(DAY FROM entries.date) BETWEEN ? AND ?", - [ expected_day_of_month - 2, 1 ].max, - [ expected_day_of_month + 2, 31 ].min) - .order(date: :desc) - end + # Amount/cadence-scoped Transaction entries on this account (or family). + base = account.present? ? account.entries : family.entries + entries = day_of_month_scope( + amount_window_scope(base.where(entryable_type: "Transaction").where(currency: currency)) + ).order(date: :desc) # Filter by merchant or name if merchant_id.present? @@ -401,6 +387,47 @@ class RecurringTransaction < ApplicationRecord end private + # Issue #1590: a recurring transfer's future occurrences rarely share the + # seed's name (user free-text, importer wording, the auto-matcher's + # "Transfer to ..."), so name-based matching returns [] and the Cleaner + # would wrongly inactivate a still-active transfer. Match the Transfer + # *pair* instead — an outflow on the source account paired with an inflow + # on the destination account, within the usual amount/cadence window — and + # return the outflow entries (the occurrence-date carrier, consistent with + # create_from_transfer). + def transfer_matching_transactions + return Entry.none unless account && destination_account + + outflow_entries = day_of_month_scope( + amount_window_scope(account.entries.where(entryable_type: "Transaction").where(currency: currency)) + ).order(date: :desc) + + paired_outflow_transaction_ids = Transfer + .where(outflow_transaction_id: outflow_entries.select(:entryable_id)) + .where(inflow_transaction_id: + destination_account.entries.where(entryable_type: "Transaction").select(:entryable_id)) + .pluck(:outflow_transaction_id) + + outflow_entries.where(entryable_id: paired_outflow_transaction_ids) + end + + # Transaction entries whose amount fits the pattern: exact, or within the + # configured variance band for manual recurring rows. + def amount_window_scope(relation) + if manual? && has_amount_variance? + relation.where("entries.amount BETWEEN ? AND ?", expected_amount_min, expected_amount_max) + else + relation.where("entries.amount = ?", amount) + end + end + + # Entries whose day-of-month lands within ±2 days of the expected day. + def day_of_month_scope(relation) + relation.where("EXTRACT(DAY FROM entries.date) BETWEEN ? AND ?", + [ expected_day_of_month - 2, 1 ].max, + [ expected_day_of_month + 2, 31 ].min) + end + def monetizable_currency currency end diff --git a/app/models/recurring_transaction/cleaner.rb b/app/models/recurring_transaction/cleaner.rb index dd22dcb89..ec5dfd917 100644 --- a/app/models/recurring_transaction/cleaner.rb +++ b/app/models/recurring_transaction/cleaner.rb @@ -9,18 +9,15 @@ class RecurringTransaction # Mark recurring transactions as inactive if they haven't occurred recently # Uses 2 months for automatic recurring, 6 months for manual recurring. # - # Transfer rows (destination_account_id present) are skipped: their - # `matching_transactions` helper looks at single-account name/amount - # which never matches a Transfer pair, so the Cleaner would - # incorrectly mark a still-recurring transfer inactive at the - # 6-month threshold. Issue #1590 tracks pair-detection-aware - # matching for recurring transfers. + # Transfer rows (destination_account_id present) are included: as of issue + # #1590, `matching_transactions` detects the Transfer pair, so a still-active + # transfer keeps surfacing recent matches and stays active, while one whose + # pair has genuinely stopped is correctly retired. def cleanup_stale_transactions stale_count = 0 family.recurring_transactions .active - .where(destination_account_id: nil) .find_each do |recurring_transaction| next unless recurring_transaction.should_be_inactive? diff --git a/test/models/recurring_transaction_test.rb b/test/models/recurring_transaction_test.rb index 20c8a28ae..75d8ce596 100644 --- a/test/models/recurring_transaction_test.rb +++ b/test/models/recurring_transaction_test.rb @@ -998,12 +998,9 @@ class RecurringTransactionTest < ActiveSupport::TestCase assert_not RecurringTransaction.exists?(rt.id) end - test "Cleaner skips recurring transfers so they aren't mistakenly marked inactive" do - # `matching_transactions` is single-account name/amount-based and never - # matches a Transfer pair, so without the skip the recurring transfer - # would flip to inactive at the 6-month threshold even when the user - # is still doing the transfer monthly. Issue #1590 tracks the proper - # pair-detection fix. + test "Cleaner keeps a recurring transfer active when its pair still occurs (issue #1590)" do + # The seed name rarely matches future occurrences, so pair detection (not + # name matching) is what keeps a live transfer active past the threshold. rt = @family.recurring_transactions.create!( account: @account, destination_account: accounts(:credit_card), name: "Transfer to CC", amount: 250, currency: "USD", @@ -1012,12 +1009,61 @@ class RecurringTransactionTest < ActiveSupport::TestCase next_expected_date: 5.days.from_now.to_date, manual: true ) - assert rt.should_be_inactive?, "guard sanity: row would be marked inactive without the skip" + assert rt.should_be_inactive?, "guard sanity: stale last_occurrence_date" + + # A fresh transfer pair this cycle, carrying a *different* free-text name. + date = 1.month.ago.beginning_of_month + 4.days # day-of-month 5 + outflow = @account.entries.create!( + date: date, amount: 250, currency: "USD", name: "rent transfer", + entryable: Transaction.new(kind: "funds_movement") + ) + inflow = accounts(:credit_card).entries.create!( + date: date, amount: -250, currency: "USD", name: "rent transfer", + entryable: Transaction.new(kind: "funds_movement") + ) + Transfer.create!(outflow_transaction: outflow.entryable, inflow_transaction: inflow.entryable) RecurringTransaction.cleanup_stale_for(@family) assert_equal "active", rt.reload.status end + test "Cleaner retires a recurring transfer whose pair has stopped" do + # No matching Transfer pair → genuinely stale → should be retired. This is + # the correctness the pair-detection (vs the old blanket skip) buys us. + rt = @family.recurring_transactions.create!( + account: @account, destination_account: accounts(:credit_card), + name: "Transfer to CC", amount: 250, currency: "USD", + expected_day_of_month: 5, + last_occurrence_date: 7.months.ago.to_date, + next_expected_date: 5.days.from_now.to_date, + manual: true + ) + + RecurringTransaction.cleanup_stale_for(@family) + assert_equal "inactive", rt.reload.status + end + + test "matching_transactions finds the transfer pair regardless of occurrence name" do + rt = @family.recurring_transactions.create!( + account: @account, destination_account: accounts(:credit_card), + name: "Transfer to CC", amount: 250, currency: "USD", + expected_day_of_month: 5, last_occurrence_date: Date.current, + next_expected_date: 1.month.from_now.to_date, manual: true + ) + date = Date.current.beginning_of_month + 4.days # day-of-month 5 + outflow = @account.entries.create!( + date: date, amount: 250, currency: "USD", name: "an importer's wording", + entryable: Transaction.new(kind: "funds_movement") + ) + inflow = accounts(:credit_card).entries.create!( + date: date, amount: -250, currency: "USD", name: "an importer's wording", + entryable: Transaction.new(kind: "funds_movement") + ) + Transfer.create!(outflow_transaction: outflow.entryable, inflow_transaction: inflow.entryable) + + assert_includes rt.matching_transactions.map(&:id), outflow.id + end + test "Identifier#update_manual_recurring_transactions skips recurring transfers" do # Same reasoning as the Cleaner skip. Without the guard, the helper # would call find_matching_transaction_entries (single-account, by