Simplify transfer auto matching with RejectedTransfer model

maybe-finance · Jan 25, 2025 · 2e57c67 · 2e57c67
1 parent 22456ff
commit 2e57c67
Show file tree

Hide file tree

Showing 18 changed files with 206 additions and 73 deletions.
diff --git a/app/controllers/transfers_controller.rb b/app/controllers/transfers_controller.rb
@@ -38,11 +38,14 @@ def create
   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

diff --git a/app/models/account.rb b/app/models/account.rb
@@ -159,4 +159,61 @@ def update_balance!(balance)
         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'")
+      .order("date_diff ASC") # Closest matches first
+  end
+
+  def auto_match_transfers!
+    # Exclude already matched transfers
+    candidates_scope = transfer_match_candidates.where(existing_transfers: { id: nil }, 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 |pm|
+        next if used_transaction_ids.include?(pm.inflow_transaction_id) ||
+               used_transaction_ids.include?(pm.outflow_transaction_id)
+
+        Transfer.create!(
+          inflow_transaction_id: pm.inflow_transaction_id,
+          outflow_transaction_id: pm.outflow_transaction_id,
+        )
+
+        used_transaction_ids << pm.inflow_transaction_id
+        used_transaction_ids << pm.outflow_transaction_id
+      end
+    end
+  end
 end
diff --git a/app/models/account/entry.rb b/app/models/account/entry.rb
@@ -77,12 +77,20 @@ def display_name
   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

diff --git a/app/models/account/syncer.rb b/app/models/account/syncer.rb
@@ -5,7 +5,7 @@ def initialize(account, start_date: nil)
   end
 
   def run
-    Transfer.auto_match_for_account(account)
+    account.auto_match_transfers!
 
     holdings = sync_holdings
     balances = sync_balances(holdings)

diff --git a/app/models/account/transaction.rb b/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 @@ def transfer
   end
 
   def transfer?
-    transfer.present? && transfer.status != "rejected"
+    transfer.present?
   end
 end
diff --git a/app/models/rejected_transfer.rb b/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
diff --git a/app/models/transfer.rb b/app/models/transfer.rb
@@ -2,7 +2,7 @@ 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" }
 
   validate :transfer_has_different_accounts
   validate :transfer_has_opposite_amounts
@@ -41,44 +41,19 @@ def from_accounts(from_account:, to_account:, date:, amount:)
         status: "confirmed"
       )
     end
+  end
 
-    def auto_match_for_account(account)
-      Transfer.transaction do
-        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
-          )
-        ").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 })
-
-        matches.each do |match|
-          Transfer.find_or_create_by!(
-            inflow_transaction_id: match.inflow_transaction_id,
-            outflow_transaction_id: match.outflow_transaction_id,
-          )
-        end
-      end
+  def reject!
+    Transfer.transaction do
+      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
     inflow_transaction.entry.sync_account_later
     outflow_transaction.entry.sync_account_later

diff --git a/app/views/account/transactions/_transaction_category.html.erb b/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 %>

diff --git a/app/views/account/transactions/_transfer_match.html.erb b/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" %>

diff --git a/app/views/account/transfer_matches/new.html.erb b/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>

diff --git a/app/views/transfers/_transfer.html.erb b/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" %>

diff --git a/app/views/transfers/update.turbo_stream.erb b/app/views/transfers/update.turbo_stream.erb
@@ -1,17 +1,19 @@
-<%= turbo_stream.replace @transfer %>
+<% unless @transfer.destroyed? %>
+  <%= turbo_stream.replace @transfer %>
 
-<%= turbo_stream.replace "category_menu_account_entry_#{@transfer.inflow_transaction.entry.id}",
+  <%= turbo_stream.replace "category_menu_account_entry_#{@transfer.inflow_transaction.entry.id}",
   partial: "account/transactions/transaction_category",
   locals: { entry: @transfer.inflow_transaction.entry } %>
 
-<%= turbo_stream.replace "category_menu_account_entry_#{@transfer.outflow_transaction.entry.id}",
+  <%= turbo_stream.replace "category_menu_account_entry_#{@transfer.outflow_transaction.entry.id}",
   partial: "account/transactions/transaction_category",
   locals: { entry: @transfer.outflow_transaction.entry } %>
 
-<%= turbo_stream.replace "transfer_match_account_entry_#{@transfer.inflow_transaction.entry.id}",
+  <%= turbo_stream.replace "transfer_match_account_entry_#{@transfer.inflow_transaction.entry.id}",
   partial: "account/transactions/transfer_match",
   locals: { entry: @transfer.inflow_transaction.entry } %>
 
-<%= turbo_stream.replace "transfer_match_account_entry_#{@transfer.outflow_transaction.entry.id}",
+  <%= turbo_stream.replace "transfer_match_account_entry_#{@transfer.outflow_transaction.entry.id}",
   partial: "account/transactions/transfer_match",
   locals: { entry: @transfer.outflow_transaction.entry } %>
+<% end %>
diff --git a/config/locales/models/transfer/en.yml b/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}

diff --git a/db/migrate/20250124224316_create_rejected_transfers.rb b/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
diff --git a/db/schema.rb b/db/schema.rb
diff --git a/test/controllers/transfers_controller_test.rb b/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