From d0bb1a31e8fa107a62f1e9d8db37899ee70fdff3 Mon Sep 17 00:00:00 2001 From: GFR Date: Sun, 16 Aug 2026 09:30:44 +0200 Subject: [PATCH] fix(recurring): include amount in manual recurring duplicate check (#2972) * fix(recurring): include amount in manual recurring duplicate check TransactionsController#mark_as_recurring blocked a second manual recurring transaction whenever an existing one shared the same account + payee name/merchant + currency, even when the amount differed -- stricter than the DB unique indexes (idx_recurring_txns_acct_name / idx_recurring_txns_acct_merchant), RecurringTransaction::Identifier's own grouping key, and the equivalent check already used in TransfersController#mark_as_recurring. Add amount to the duplicate lookup so two distinct recurring payments to the same payee at different amounts are both allowed, while an exact duplicate is still blocked. Also rescue ActiveRecord::RecordNotUnique around the create call so a race between the pre-check and the DB constraint (e.g. a double-submit) surfaces the same friendly "already exists" message instead of a generic error, mirroring the existing race-handling pattern in RecurringTransaction::Identifier. Fixes #2936 Co-Authored-By: Claude Sonnet 5 * fix(recurring): don't blend distinct charge amounts into variance band Once two manual recurring rows with the same payee/different amounts can coexist (this PR), RecurringTransaction.create_from_transaction's variance-band discovery still matched historical entries only by account/payee/currency/day-window -- never by amount -- so it could blend genuinely unrelated charges (e.g. a fee + a due from the same merchant, same day) into one row's expected_amount_min/max/avg. Flagged by Codex review on this PR. Confirmed this is not hypothetical: two real production transactions (3.00 and 19.68, same merchant, same day) got blended into a single recurring row showing a fabricated "11.34" projected amount that matches neither real transaction. The same unfiltered matching independently exists in RecurringTransaction::Identifier#manual_recurring_matches_entry?, which periodically re-derives every manual recurring row's variance after each sync (via IdentifyRecurringTransactionsJob). Both call sites needed the fix together, or the job would silently re-blend amounts on the next sync. Add RecurringTransaction.amount_within_variance_band?(candidate, anchor, ratio: 2) -- a candidate only counts as "the same fluctuating payment" if it's within 2x (double/half) of the anchor. Anchored on the target amount (not pairwise) so unrelated charges can't chain together; ratio-based (not %-of-target-with-floor) so it's scale-invariant and handles signed (expense) amounts correctly. Threshold checked against real data: existing variance test fixtures sit at ~1.2-1.3x (must stay included), the real corrupted case sits at ~6.6x (must be excluded) -- 2x leaves comfortable margin on both sides. Wire this into find_matching_transaction_entries/ find_matching_transaction_amounts (SQL-level filter, same pattern as the existing day-of-month bounds) and into manual_recurring_matches_entry?. amount_window_scope/ matching_transactions and create_from_transfer need no changes -- confirmed by reading: the former only consumes an already-computed band, the latter never does variance discovery at all. Does not touch any already-corrupted production data -- deliberately out of scope, discussed separately. Co-Authored-By: Claude Sonnet 5 --------- Co-authored-by: Claude Sonnet 5 --- app/controllers/transactions_controller.rb | 16 ++- app/models/recurring_transaction.rb | 27 ++++- .../recurring_transaction/identifier.rb | 6 + .../transactions_controller_test.rb | 109 ++++++++++++++++++ test/models/recurring_transaction_test.rb | 95 +++++++++++++++ 5 files changed, 250 insertions(+), 3 deletions(-) diff --git a/app/controllers/transactions_controller.rb b/app/controllers/transactions_controller.rb index d462d98bb..ef8f3d000 100644 --- a/app/controllers/transactions_controller.rb +++ b/app/controllers/transactions_controller.rb @@ -353,11 +353,15 @@ class TransactionsController < ApplicationController return unless require_account_permission!(transaction.entry.account) - # Check if a recurring transaction already exists for this pattern + # Check if a recurring transaction already exists for this pattern. + # Amount is included so two distinct recurring payments with the same + # payee but different amounts aren't treated as duplicates (matches the + # DB uniqueness scope and RecurringTransaction::Identifier's grouping key). existing = Current.family.recurring_transactions.find_by( account_id: transaction.entry.account_id, merchant_id: transaction.merchant_id, name: transaction.merchant_id.present? ? nil : transaction.entry.name, + amount: transaction.entry.amount, currency: transaction.entry.currency, manual: true ) @@ -384,6 +388,16 @@ class TransactionsController < ApplicationController redirect_back_or_to transactions_path end end + rescue ActiveRecord::RecordNotUnique + # Another request created the same (account, name/merchant, amount, + # currency) pattern between the check above and this create — the DB + # unique index is the authoritative backstop for that race. + respond_to do |format| + format.html do + flash[:alert] = t("recurring_transactions.already_exists") + redirect_back_or_to transactions_path + end + end rescue StandardError => e respond_to do |format| format.html do diff --git a/app/models/recurring_transaction.rb b/app/models/recurring_transaction.rb index f05df6fcb..29ec28caf 100644 --- a/app/models/recurring_transaction.rb +++ b/app/models/recurring_transaction.rb @@ -138,6 +138,21 @@ class RecurringTransaction < ApplicationRecord ) end + # A candidate amount only counts as "the same fluctuating payment" as the + # anchor amount if it's within this ratio (2x = may double or halve). + # Anchored on the target amount (not pairwise) so unrelated charges can't + # chain together, and expressed as a ratio (not a %-of-target-with-floor) + # so it's scale-invariant and handles negative (expense) amounts correctly + # via the signed min/max bounds below. + AMOUNT_VARIANCE_RATIO = 2 + + def self.amount_within_variance_band?(candidate_amount, anchor_amount, ratio: AMOUNT_VARIANCE_RATIO) + return candidate_amount == anchor_amount if anchor_amount.zero? + + low, high = [ anchor_amount / ratio, anchor_amount * ratio ].minmax + candidate_amount.between?(low, high) + end + # Create a manual recurring transaction from an existing transaction # Automatically calculates amount variance from past 6 months of matching transactions def self.create_from_transaction(transaction, date_variance: 2) @@ -152,6 +167,7 @@ class RecurringTransaction < ApplicationRecord name: transaction.merchant_id.present? ? nil : entry.name, currency: entry.currency, expected_day: expected_day, + amount: entry.amount, lookback_months: 6, account: entry.account ) @@ -194,8 +210,9 @@ class RecurringTransaction < ApplicationRecord end # Find matching transaction entries for variance calculation - def self.find_matching_transaction_entries(family:, merchant_id:, name:, currency:, expected_day:, lookback_months: 6, account: nil) + def self.find_matching_transaction_entries(family:, merchant_id:, name:, currency:, expected_day:, amount:, lookback_months: 6, account: nil) lookback_date = lookback_months.months.ago.to_date + amount_low, amount_high = [ amount / AMOUNT_VARIANCE_RATIO, amount * AMOUNT_VARIANCE_RATIO ].minmax entries = (account.present? ? account.entries : family.entries) .where(entryable_type: "Transaction") @@ -204,6 +221,11 @@ class RecurringTransaction < ApplicationRecord .where("EXTRACT(DAY FROM entries.date) BETWEEN ? AND ?", [ expected_day - 2, 1 ].max, [ expected_day + 2, 31 ].min) + # Only entries whose amount is within the variance band of the target + # amount count as "the same fluctuating payment" — otherwise unrelated + # charges that happen to share a merchant/day get averaged together + # (see issue #2936 follow-up). + .where("entries.amount BETWEEN ? AND ?", amount_low, amount_high) .order(date: :desc) # Filter by merchant or name @@ -219,13 +241,14 @@ class RecurringTransaction < ApplicationRecord end # Find matching transaction amounts for variance calculation - def self.find_matching_transaction_amounts(family:, merchant_id:, name:, currency:, expected_day:, lookback_months: 6, account: nil) + def self.find_matching_transaction_amounts(family:, merchant_id:, name:, currency:, expected_day:, amount:, lookback_months: 6, account: nil) matching_entries = find_matching_transaction_entries( family: family, merchant_id: merchant_id, name: name, currency: currency, expected_day: expected_day, + amount: amount, lookback_months: lookback_months, account: account ) diff --git a/app/models/recurring_transaction/identifier.rb b/app/models/recurring_transaction/identifier.rb index 9a443df3d..03a49dea3 100644 --- a/app/models/recurring_transaction/identifier.rb +++ b/app/models/recurring_transaction/identifier.rb @@ -244,6 +244,12 @@ class RecurringTransaction def manual_recurring_matches_entry?(recurring, entry) return false unless entry.currency == recurring.currency return false if recurring.account_id.present? && entry.account_id != recurring.account_id + # Anchor on the row's stable, user-set seed amount (not + # expected_amount_avg, which the very corruption we're guarding + # against here could already have skewed) so unrelated charges that + # happen to share a merchant/day don't get averaged in (issue #2936 + # follow-up). + return false unless RecurringTransaction.amount_within_variance_band?(entry.amount, recurring.amount) expected_day = [ recurring.expected_day_of_month, entry.date.end_of_month.day ].min day = entry.date.day diff --git a/test/controllers/transactions_controller_test.rb b/test/controllers/transactions_controller_test.rb index f3c5763ec..6de5f9b81 100644 --- a/test/controllers/transactions_controller_test.rb +++ b/test/controllers/transactions_controller_test.rb @@ -400,6 +400,115 @@ end assert_equal "A manual recurring transaction already exists for this pattern", flash[:alert] end + test "mark_as_recurring allows a second manual recurring transaction with same merchant but different amount" do + family = families(:empty) + sign_in users(:empty) + account = family.accounts.create! name: "Test", balance: 0, currency: "USD", accountable: Depository.new + merchant = family.merchants.create! name: "Test Merchant" + entry = create_transaction(account: account, amount: 34, merchant: merchant) + transaction = entry.entryable + + # Existing manual recurring row for the same merchant, but a different amount + family.recurring_transactions.create!( + account: account, + merchant: merchant, + amount: 12, + currency: entry.currency, + expected_day_of_month: entry.date.day, + last_occurrence_date: entry.date, + next_expected_date: 1.month.from_now, + status: "active", + manual: true, + occurrence_count: 1 + ) + + assert_difference "family.recurring_transactions.count", 1 do + post mark_as_recurring_transaction_path(transaction) + end + + assert_redirected_to transactions_path + assert_equal "Transaction marked as recurring", flash[:notice] + end + + test "mark_as_recurring allows a second manual recurring transaction with same name but different amount" do + family = families(:empty) + sign_in users(:empty) + account = family.accounts.create! name: "Test", balance: 0, currency: "USD", accountable: Depository.new + entry = create_transaction(account: account, name: "Example Payee", amount: 34) + transaction = entry.entryable + + # Existing manual recurring row for the same payee name, but a different amount + family.recurring_transactions.create!( + account: account, + name: "Example Payee", + amount: 12, + currency: entry.currency, + expected_day_of_month: entry.date.day, + last_occurrence_date: entry.date, + next_expected_date: 1.month.from_now, + status: "active", + manual: true, + occurrence_count: 1 + ) + + assert_difference "family.recurring_transactions.count", 1 do + post mark_as_recurring_transaction_path(transaction) + end + + assert_redirected_to transactions_path + assert_equal "Transaction marked as recurring", flash[:notice] + end + + test "mark_as_recurring shows alert if recurring transaction with same name and amount already exists" do + family = families(:empty) + sign_in users(:empty) + account = family.accounts.create! name: "Test", balance: 0, currency: "USD", accountable: Depository.new + entry = create_transaction(account: account, name: "Example Payee", amount: 100) + transaction = entry.entryable + + family.recurring_transactions.create!( + account: account, + name: "Example Payee", + amount: entry.amount, + currency: entry.currency, + expected_day_of_month: entry.date.day, + last_occurrence_date: entry.date, + next_expected_date: 1.month.from_now, + status: "active", + manual: true, + occurrence_count: 1 + ) + + assert_no_difference "RecurringTransaction.count" do + post mark_as_recurring_transaction_path(transaction) + end + + assert_redirected_to transactions_path + assert_equal "A manual recurring transaction already exists for this pattern", flash[:alert] + end + + test "mark_as_recurring shows already-exists alert when a concurrent request wins the race" do + family = families(:empty) + sign_in users(:empty) + account = family.accounts.create! name: "Test", balance: 0, currency: "USD", accountable: Depository.new + merchant = family.merchants.create! name: "Test Merchant" + entry = create_transaction(account: account, amount: 100, merchant: merchant) + transaction = entry.entryable + + # Simulate another request creating the identical pattern between our + # pre-check and our create call. + RecurringTransaction.expects(:create_from_transaction).raises( + ActiveRecord::RecordNotUnique.new("duplicate key value violates unique constraint") + ) + + assert_no_difference "RecurringTransaction.count" do + post mark_as_recurring_transaction_path(transaction) + end + + assert_redirected_to transactions_path + assert_equal "A manual recurring transaction already exists for this pattern", flash[:alert] + end + test "mark_as_recurring handles validation errors gracefully" do family = families(:empty) sign_in users(:empty) diff --git a/test/models/recurring_transaction_test.rb b/test/models/recurring_transaction_test.rb index 75d8ce596..4b9422218 100644 --- a/test/models/recurring_transaction_test.rb +++ b/test/models/recurring_transaction_test.rb @@ -457,6 +457,53 @@ class RecurringTransactionTest < ActiveSupport::TestCase assert recurring.next_expected_date >= Date.current end + test "create_from_transaction does not blend a distinct same-day charge type into the variance band" do + # Mirrors a real production case: two genuinely different charges from + # the same merchant, same day (a small fee alongside a larger due), + # ~6.5x apart -- not one fluctuating payment. + fee_transaction = Transaction.create!(merchant: @merchant, category: categories(:food_and_drink)) + fee_entry = @account.entries.create!( + date: 1.month.ago.beginning_of_month + 14.days, + amount: 3.00, + currency: "USD", + name: "Test Transaction", + entryable: fee_transaction + ) + + due_transaction = Transaction.create!(merchant: @merchant, category: categories(:food_and_drink)) + @account.entries.create!( + date: 1.month.ago.beginning_of_month + 14.days, + amount: 19.68, + currency: "USD", + name: "Test Transaction", + entryable: due_transaction + ) + + recurring = RecurringTransaction.create_from_transaction(fee_entry.transaction) + + assert_equal 3.00, recurring.expected_amount_min + assert_equal 3.00, recurring.expected_amount_max + assert_equal 3.00, recurring.expected_amount_avg + assert_equal 1, recurring.occurrence_count + end + + test "amount_within_variance_band? allows up to 2x and excludes beyond" do + assert RecurringTransaction.amount_within_variance_band?(199, 100) + assert_not RecurringTransaction.amount_within_variance_band?(201, 100) + assert RecurringTransaction.amount_within_variance_band?(50, 100) # halved is still within band + assert_not RecurringTransaction.amount_within_variance_band?(49, 100) + end + + test "amount_within_variance_band? handles a zero anchor without dividing by zero" do + assert RecurringTransaction.amount_within_variance_band?(0, 0) + assert_not RecurringTransaction.amount_within_variance_band?(5, 0) + end + + test "amount_within_variance_band? does not match across a sign mismatch" do + assert_not RecurringTransaction.amount_within_variance_band?(50, -50) + assert_not RecurringTransaction.amount_within_variance_band?(-10, 100) + end + test "matching_transactions with amount variance matches within range" do # Create manual recurring with variance for day 15 of the month recurring = @family.recurring_transactions.create!( @@ -610,6 +657,54 @@ class RecurringTransactionTest < ActiveSupport::TestCase assert manual_recurring.occurrence_count > 1 end + test "identify_patterns_for does not re-blend a distinct charge type into an existing manual recurring transaction" do + # Mirrors the real production corruption: a manual recurring row seeded + # at 3.00 must not have its variance widened by a same-day, same-merchant + # 19.68 entry when the periodic identification job runs. + manual_recurring = @family.recurring_transactions.create!( + account: @account, + merchant: @merchant, + amount: 3.00, + currency: "USD", + expected_day_of_month: 15, + last_occurrence_date: 3.months.ago, + next_expected_date: 1.month.from_now, + status: "active", + manual: true, + occurrence_count: 1, + expected_amount_min: 3.00, + expected_amount_max: 3.00, + expected_amount_avg: 3.00 + ) + + fee_transaction = Transaction.create!(merchant: @merchant, category: categories(:food_and_drink)) + @account.entries.create!( + date: 1.month.ago.beginning_of_month + 14.days, + amount: 3.00, + currency: "USD", + name: "Test Transaction", + entryable: fee_transaction + ) + + due_transaction = Transaction.create!(merchant: @merchant, category: categories(:food_and_drink)) + @account.entries.create!( + date: 1.month.ago.beginning_of_month + 14.days, + amount: 19.68, + currency: "USD", + name: "Test Transaction", + entryable: due_transaction + ) + + assert_no_difference "@family.recurring_transactions.count" do + RecurringTransaction.identify_patterns_for!(@family) + end + + manual_recurring.reload + assert_equal 3.00, manual_recurring.expected_amount_min + assert_equal 3.00, manual_recurring.expected_amount_max + assert_equal 3.00, manual_recurring.expected_amount_avg + end + test "cleaner does not delete manual recurring transactions" do # Create inactive manual recurring manual_recurring = @family.recurring_transactions.create!(