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 <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>

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

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
     )

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

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)

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