Add RejectedTransfer model, simplify auto matching (#1690)

* Allow transfers to match when inflow is after outflow

* Simplify transfer auto matching with RejectedTransfer model

* Validations

* Reset migrations

app/controllers/transfers_controller.rb

@@ -38,11 +38,14 @@ class TransfersController < ApplicationController
   end
 
   def update
-    Transfer.transaction do
-      @transfer.update!(transfer_update_params.except(:category_id))
-      @transfer.outflow_transaction.update!(category_id: transfer_update_params[:category_id])
+    if transfer_update_params[:status] == "rejected"
+      @transfer.reject!
+    elsif transfer_update_params[:status] == "confirmed"
+      @transfer.confirm!
     end
 
+    @transfer.outflow_transaction.update!(category_id: transfer_update_params[:category_id])
+
     respond_to do |format|
       format.html { redirect_back_or_to transactions_url, notice: t(".success") }
       format.turbo_stream

app/models/account.rb

@@ -159,4 +159,62 @@ class Account < ApplicationRecord
         entryable: Account::Valuation.new
     end
   end
+
+  def transfer_match_candidates
+    Account::Entry.select([
+      "inflow_candidates.entryable_id as inflow_transaction_id",
+      "outflow_candidates.entryable_id as outflow_transaction_id",
+      "ABS(inflow_candidates.date - outflow_candidates.date) as date_diff"
+    ]).from("account_entries inflow_candidates")
+      .joins("
+        JOIN account_entries outflow_candidates ON (
+          inflow_candidates.amount < 0 AND
+          outflow_candidates.amount > 0 AND
+          inflow_candidates.amount = -outflow_candidates.amount AND
+          inflow_candidates.currency = outflow_candidates.currency AND
+          inflow_candidates.account_id <> outflow_candidates.account_id AND
+          inflow_candidates.date BETWEEN outflow_candidates.date - 4 AND outflow_candidates.date + 4
+        )
+      ").joins("
+        LEFT JOIN transfers existing_transfers ON (
+          existing_transfers.inflow_transaction_id = inflow_candidates.entryable_id OR
+          existing_transfers.outflow_transaction_id = outflow_candidates.entryable_id
+        )
+      ")
+      .joins("LEFT JOIN rejected_transfers ON (
+        rejected_transfers.inflow_transaction_id = inflow_candidates.entryable_id AND
+        rejected_transfers.outflow_transaction_id = outflow_candidates.entryable_id
+      )")
+      .joins("JOIN accounts inflow_accounts ON inflow_accounts.id = inflow_candidates.account_id")
+      .joins("JOIN accounts outflow_accounts ON outflow_accounts.id = outflow_candidates.account_id")
+      .where("inflow_accounts.family_id = ? AND outflow_accounts.family_id = ?", self.family_id, self.family_id)
+      .where("inflow_candidates.entryable_type = 'Account::Transaction' AND outflow_candidates.entryable_type = 'Account::Transaction'")
+      .where(existing_transfers: { id: nil })
+      .order("date_diff ASC") # Closest matches first
+  end
+
+  def auto_match_transfers!
+    # Exclude already matched transfers
+    candidates_scope = transfer_match_candidates.where(rejected_transfers: { id: nil })
+
+    # Track which transactions we've already matched to avoid duplicates
+    used_transaction_ids = Set.new
+
+    candidates = []
+
+    Transfer.transaction do
+      candidates_scope.each do |match|
+        next if used_transaction_ids.include?(match.inflow_transaction_id) ||
+               used_transaction_ids.include?(match.outflow_transaction_id)
+
+        Transfer.create!(
+          inflow_transaction_id: match.inflow_transaction_id,
+          outflow_transaction_id: match.outflow_transaction_id,
+        )
+
+        used_transaction_ids << match.inflow_transaction_id
+        used_transaction_ids << match.outflow_transaction_id
+      end
+    end
+  end
 end

app/models/account/entry.rb

@@ -77,12 +77,20 @@ class Account::Entry < ApplicationRecord
   end
 
   def transfer_match_candidates
-    account.family.entries
-          .where.not(account_id: account_id)
-          .where.not(id: id)
-          .where(amount: -amount)
-          .where(currency: currency)
-          .where(date: (date - 4.days)..(date + 4.days))
+    candidates_scope = account.transfer_match_candidates
+
+    candidates_scope = if amount.negative?
+      candidates_scope.where("inflow_candidates.entryable_id = ?", entryable_id)
+    else
+      candidates_scope.where("outflow_candidates.entryable_id = ?", entryable_id)
+    end
+
+    candidates_scope.map do |pm|
+      Transfer.new(
+        inflow_transaction_id: pm.inflow_transaction_id,
+        outflow_transaction_id: pm.outflow_transaction_id,
+      )
+    end
   end
 
   class << self

app/models/account/syncer.rb

@@ -5,7 +5,7 @@ class Account::Syncer
   end
 
   def run
-    Transfer.auto_match_for_account(account)
+    account.auto_match_transfers!
 
     holdings = sync_holdings
     balances = sync_balances(holdings)

app/models/account/transaction.rb

@@ -9,6 +9,10 @@ class Account::Transaction < ApplicationRecord
   has_one :transfer_as_inflow, class_name: "Transfer", foreign_key: "inflow_transaction_id", dependent: :destroy
   has_one :transfer_as_outflow, class_name: "Transfer", foreign_key: "outflow_transaction_id", dependent: :destroy
 
+  # We keep track of rejected transfers to avoid auto-matching them again
+  has_one :rejected_transfer_as_inflow, class_name: "RejectedTransfer", foreign_key: "inflow_transaction_id", dependent: :destroy
+  has_one :rejected_transfer_as_outflow, class_name: "RejectedTransfer", foreign_key: "outflow_transaction_id", dependent: :destroy
+
   accepts_nested_attributes_for :taggings, allow_destroy: true
 
   scope :active, -> { where(excluded: false) }
@@ -24,6 +28,6 @@ class Account::Transaction < ApplicationRecord
   end
 
   def transfer?
-    transfer.present? && transfer.status != "rejected"
+    transfer.present?
   end
 end

app/models/rejected_transfer.rb

@@ -0,0 +1,4 @@
+class RejectedTransfer < ApplicationRecord
+  belongs_to :inflow_transaction, class_name: "Account::Transaction"
+  belongs_to :outflow_transaction, class_name: "Account::Transaction"
+end

app/models/transfer.rb

@@ -2,13 +2,15 @@ class Transfer < ApplicationRecord
   belongs_to :inflow_transaction, class_name: "Account::Transaction"
   belongs_to :outflow_transaction, class_name: "Account::Transaction"
 
-  enum :status, { pending: "pending", confirmed: "confirmed", rejected: "rejected" }
+  enum :status, { pending: "pending", confirmed: "confirmed" }
+
+  validates :inflow_transaction_id, uniqueness: true
+  validates :outflow_transaction_id, uniqueness: true
 
   validate :transfer_has_different_accounts
   validate :transfer_has_opposite_amounts
   validate :transfer_within_date_range
   validate :transfer_has_same_family
-  validate :inflow_on_or_after_outflow
 
   class << self
     def from_accounts(from_account:, to_account:, date:, amount:)
@@ -42,43 +44,17 @@ class Transfer < ApplicationRecord
         status: "confirmed"
       )
     end
+  end
 
-    def auto_match_for_account(account)
-      matches = Account::Entry.select([
-        "inflow_candidates.entryable_id as inflow_transaction_id",
-        "outflow_candidates.entryable_id as outflow_transaction_id"
-      ]).from("account_entries inflow_candidates")
-        .joins("
-          JOIN account_entries outflow_candidates ON (
-            inflow_candidates.amount < 0 AND
-            outflow_candidates.amount > 0 AND
-            inflow_candidates.amount = -outflow_candidates.amount AND
-            inflow_candidates.currency = outflow_candidates.currency AND
-            inflow_candidates.account_id <> outflow_candidates.account_id AND
-            inflow_candidates.date BETWEEN outflow_candidates.date - 4 AND outflow_candidates.date + 4 AND
-            inflow_candidates.date >= outflow_candidates.date
-          )
-        ").joins("
-          LEFT JOIN transfers existing_transfers ON (
-            existing_transfers.inflow_transaction_id = inflow_candidates.entryable_id OR
-            existing_transfers.outflow_transaction_id = outflow_candidates.entryable_id
-          )
-        ")
-        .joins("JOIN accounts inflow_accounts ON inflow_accounts.id = inflow_candidates.account_id")
-        .joins("JOIN accounts outflow_accounts ON outflow_accounts.id = outflow_candidates.account_id")
-        .where("inflow_accounts.family_id = ? AND outflow_accounts.family_id = ?", account.family_id, account.family_id)
-        .where("inflow_candidates.entryable_type = 'Account::Transaction' AND outflow_candidates.entryable_type = 'Account::Transaction'")
-        .where(existing_transfers: { id: nil })
-
+  def reject!
     Transfer.transaction do
-        matches.each do |match|
-          Transfer.create!(
-            inflow_transaction_id: match.inflow_transaction_id,
-            outflow_transaction_id: match.outflow_transaction_id,
-          )
-        end
+      RejectedTransfer.find_or_create_by!(inflow_transaction_id: inflow_transaction_id, outflow_transaction_id: outflow_transaction_id)
+      destroy!
     end
   end
+
+  def confirm!
+    update!(status: "confirmed")
   end
 
   def sync_account_later
@@ -119,11 +95,6 @@ class Transfer < ApplicationRecord
   end
 
   private
-    def inflow_on_or_after_outflow
-      return unless inflow_transaction.present? && outflow_transaction.present?
-      errors.add(:base, :inflow_must_be_on_or_after_outflow) if inflow_transaction.entry.date < outflow_transaction.entry.date
-    end
-
     def transfer_has_different_accounts
       return unless inflow_transaction.present? && outflow_transaction.present?
       errors.add(:base, :must_be_from_different_accounts) if inflow_transaction.entry.account == outflow_transaction.entry.account

app/views/account/transactions/_transaction_category.html.erb

@@ -1,7 +1,7 @@
 <%# locals: (entry:) %>
 
 <div id="<%= dom_id(entry, "category_menu") %>">
-  <% if entry.account_transaction.transfer&.categorizable? || entry.account_transaction.transfer.nil? || entry.account_transaction.transfer&.rejected? %>
+  <% if entry.account_transaction.transfer&.categorizable? || entry.account_transaction.transfer.nil? %>
     <%= render "categories/menu", transaction: entry.account_transaction %>
   <% else %>
     <%= render "categories/badge", category: entry.account_transaction.transfer&.payment? ? payment_category :  transfer_category %>

app/views/account/transactions/_transfer_match.html.erb

@@ -19,6 +19,7 @@
 
     <%= button_to transfer_path(entry.account_transaction.transfer, transfer: { status: "rejected" }),
                     method: :patch,
+                    data: { turbo: false },
                     class: "text-gray-500 hover:text-gray-800 flex items-center justify-center",
                     title: "Reject match" do %>
       <%= lucide_icon "x", class: "w-4 h-4 text-gray-400 hover:text-gray-600" %>

app/views/account/transfer_matches/new.html.erb

@@ -23,7 +23,7 @@
               ) %>
         <% else %>
           <%= render "account/transfer_matches/matching_fields",
-                      form: f, entry: @entry, candidates: @transfer_match_candidates, accounts: @accounts %>
+                      form: f, entry: @entry, candidates: @transfer_match_candidates.map { |pm| pm.outflow_transaction.entry }, accounts: @accounts %>
         <% end %>
       </div>
     </section>
@@ -50,7 +50,7 @@
               ) %>
         <% else %>
           <%= render "account/transfer_matches/matching_fields",
-                      form: f, entry: @entry, candidates: @transfer_match_candidates, accounts: @accounts %>
+                      form: f, entry: @entry, candidates: @transfer_match_candidates.map { |pm| pm.inflow_transaction.entry }, accounts: @accounts %>
         <% end %>
       </div>
     </section>

app/views/transfers/_transfer.html.erb

@@ -24,10 +24,6 @@
                   <span title="<%= transfer.payment? ? "Payment" : "Transfer" %> is confirmed">
                     <%= lucide_icon "link-2", class: "w-4 h-4 text-indigo-600" %>
                   </span>
-                <% elsif transfer.status == "rejected" %>
-                  <span class="inline-flex items-center rounded-full bg-red-50 px-2 py-0.5 text-xs font-medium text-red-700">
-                    Rejected
-                  </span>
                 <% else %>
                   <span class="inline-flex items-center rounded-full bg-indigo-50 px-2 py-0.5 text-xs font-medium text-indigo-700">
                     Auto-matched
@@ -42,6 +38,7 @@
 
                   <%= button_to transfer_path(transfer, transfer: { status: "rejected" }),
                     method: :patch,
+                    data: { turbo: false },
                     class: "text-gray-500 hover:text-gray-800 flex items-center justify-center",
                     title: "Reject match" do %>
                     <%= lucide_icon "x", class: "w-4 h-4 text-gray-400 hover:text-gray-600" %>

app/views/transfers/update.turbo_stream.erb

@@ -1,3 +1,4 @@
+<% unless @transfer.destroyed? %>
   <%= turbo_stream.replace @transfer %>
 
   <%= turbo_stream.replace "category_menu_account_entry_#{@transfer.inflow_transaction.entry.id}",
@@ -15,3 +16,4 @@
   <%= turbo_stream.replace "transfer_match_account_entry_#{@transfer.outflow_transaction.entry.id}",
   partial: "account/transactions/transfer_match",
   locals: { entry: @transfer.outflow_transaction.entry } %>
+<% end %>

config/locales/models/transfer/en.yml

@@ -13,7 +13,8 @@ en:
                 amounts
               must_have_single_currency: Transfer must have a single currency
               must_be_from_same_family: Transfer must be from the same family
-              inflow_must_be_on_or_after_outflow: Inflow transaction must be on or after outflow transaction
+              inflow_cannot_be_in_multiple_transfers: Inflow transaction cannot be part of multiple transfers
+              outflow_cannot_be_in_multiple_transfers: Outflow transaction cannot be part of multiple transfers
   transfer:
     name: Transfer to %{to_account}
     payment_name: Payment to %{to_account}

db/migrate/20250124224316_create_rejected_transfers.rb

@@ -0,0 +1,44 @@
+class CreateRejectedTransfers < ActiveRecord::Migration[7.2]
+  def change
+    create_table :rejected_transfers, id: :uuid do |t|
+      t.references :inflow_transaction, null: false, foreign_key: { to_table: :account_transactions }, type: :uuid
+      t.references :outflow_transaction, null: false, foreign_key: { to_table: :account_transactions }, type: :uuid
+      t.timestamps
+    end
+
+    add_index :rejected_transfers, [ :inflow_transaction_id, :outflow_transaction_id ], unique: true
+
+    reversible do |dir|
+      dir.up do
+        execute <<~SQL
+          INSERT INTO rejected_transfers (inflow_transaction_id, outflow_transaction_id, created_at, updated_at)
+          SELECT
+            inflow_transaction_id,
+            outflow_transaction_id,
+            created_at,
+            updated_at
+          FROM transfers
+          WHERE status = 'rejected'
+        SQL
+
+        execute <<~SQL
+          DELETE FROM transfers
+          WHERE status = 'rejected'
+        SQL
+      end
+
+      dir.down do
+        execute <<~SQL
+          INSERT INTO transfers (inflow_transaction_id, outflow_transaction_id, status, created_at, updated_at)
+          SELECT
+            inflow_transaction_id,
+            outflow_transaction_id,
+            'rejected',
+            created_at,
+            updated_at
+          FROM rejected_transfers
+        SQL
+      end
+    end
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[7.2].define(version: 2025_01_20_210449) do
+ActiveRecord::Schema[7.2].define(version: 2025_01_24_224316) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "pgcrypto"
   enable_extension "plpgsql"
@@ -527,6 +527,16 @@ ActiveRecord::Schema[7.2].define(version: 2025_01_20_210449) do
     t.string "area_unit"
   end
 
+  create_table "rejected_transfers", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t|
+    t.uuid "inflow_transaction_id", null: false
+    t.uuid "outflow_transaction_id", null: false
+    t.datetime "created_at", null: false
+    t.datetime "updated_at", null: false
+    t.index ["inflow_transaction_id", "outflow_transaction_id"], name: "idx_on_inflow_transaction_id_outflow_transaction_id_412f8e7e26", unique: true
+    t.index ["inflow_transaction_id"], name: "index_rejected_transfers_on_inflow_transaction_id"
+    t.index ["outflow_transaction_id"], name: "index_rejected_transfers_on_outflow_transaction_id"
+  end
+
   create_table "securities", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t|
     t.string "ticker"
     t.string "name"
@@ -691,6 +701,8 @@ ActiveRecord::Schema[7.2].define(version: 2025_01_20_210449) do
   add_foreign_key "merchants", "families"
   add_foreign_key "plaid_accounts", "plaid_items"
   add_foreign_key "plaid_items", "families"
+  add_foreign_key "rejected_transfers", "account_transactions", column: "inflow_transaction_id"
+  add_foreign_key "rejected_transfers", "account_transactions", column: "outflow_transaction_id"
   add_foreign_key "security_prices", "securities"
   add_foreign_key "sessions", "impersonation_sessions", column: "active_impersonator_session_id"
   add_foreign_key "sessions", "users"

test/controllers/transfers_controller_test.rb

@@ -25,8 +25,8 @@ class TransfersControllerTest < ActionDispatch::IntegrationTest
     end
   end
 
-  test "can destroy transfer" do
-    assert_difference -> { Transfer.count } => -1, -> { Account::Transaction.count } => 0 do
+  test "soft deletes transfer" do
+    assert_difference -> { Transfer.count }, -1 do
       delete transfer_url(transfers(:one))
     end
   end

test/models/account_test.rb

@@ -1,7 +1,7 @@
 require "test_helper"
 
 class AccountTest < ActiveSupport::TestCase
-  include SyncableInterfaceTest
+  include SyncableInterfaceTest, Account::EntriesTestHelper
 
   setup do
     @account = @syncable = accounts(:depository)
@@ -65,4 +65,38 @@ class AccountTest < ActiveSupport::TestCase
       assert_equal 0, @account.series(currency: "NZD").values.count
     end
   end
+
+  test "auto-matches transfers" do
+    outflow_entry = create_transaction(date: 1.day.ago.to_date, account: @account, amount: 500)
+    inflow_entry = create_transaction(date: Date.current, account: accounts(:credit_card), amount: -500)
+
+    assert_difference -> { Transfer.count } => 1 do
+      @account.auto_match_transfers!
+    end
+  end
+
+  # In this scenario, our matching logic should find 4 potential matches.  These matches should be ranked based on
+  # days apart, then de-duplicated so that we aren't auto-matching the same transaction across multiple transfers.
+  test "when 2 options exist, only auto-match one at a time, ranked by days apart" do
+    yesterday_outflow = create_transaction(date: 1.day.ago.to_date, account: @account, amount: 500)
+    yesterday_inflow = create_transaction(date: 1.day.ago.to_date, account: accounts(:credit_card), amount: -500)
+
+    today_outflow = create_transaction(date: Date.current, account: @account, amount: 500)
+    today_inflow = create_transaction(date: Date.current, account: accounts(:credit_card), amount: -500)
+
+    assert_difference -> { Transfer.count } => 2 do
+      @account.auto_match_transfers!
+    end
+  end
+
+  test "does not auto-match any transfers that have been rejected by user already" do
+    outflow = create_transaction(date: Date.current, account: @account, amount: 500)
+    inflow = create_transaction(date: Date.current, account: accounts(:credit_card), amount: -500)
+
+    RejectedTransfer.create!(inflow_transaction_id: inflow.entryable_id, outflow_transaction_id: outflow.entryable_id)
+
+    assert_no_difference -> { Transfer.count } do
+      @account.auto_match_transfers!
+    end
+  end
 end

test/models/transfer_test.rb

@@ -14,15 +14,6 @@ class TransferTest < ActiveSupport::TestCase
     end
   end
 
-  test "auto matches transfers" do
-    outflow_entry = create_transaction(date: 1.day.ago.to_date, account: accounts(:depository), amount: 500)
-    inflow_entry = create_transaction(date: Date.current, account: accounts(:credit_card), amount: -500)
-
-    assert_difference -> { Transfer.count } => 1 do
-      Transfer.auto_match_for_account(accounts(:depository))
-    end
-  end
-
   test "transfer has different accounts, opposing amounts, and within 4 days of each other" do
     outflow_entry = create_transaction(date: 1.day.ago.to_date, account: accounts(:depository), amount: 500)
     inflow_entry = create_transaction(date: Date.current, account: accounts(:credit_card), amount: -500)
@@ -131,4 +122,16 @@ class TransferTest < ActiveSupport::TestCase
       transfer.save!
     end
   end
+
+  test "transaction can only belong to one transfer" do
+    outflow_entry = create_transaction(date: Date.current, account: accounts(:depository), amount: 500)
+    inflow_entry1 = create_transaction(date: Date.current, account: accounts(:credit_card), amount: -500)
+    inflow_entry2 = create_transaction(date: Date.current, account: accounts(:credit_card), amount: -500)
+
+    Transfer.create!(inflow_transaction: inflow_entry1.account_transaction, outflow_transaction: outflow_entry.account_transaction)
+
+    assert_raises ActiveRecord::RecordInvalid do
+      Transfer.create!(inflow_transaction: inflow_entry2.account_transaction, outflow_transaction: outflow_entry.account_transaction)
+    end
+  end
 end