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!(