Transfer and Payment auto-matching, model and UI improvements (#1585)

* Transfer data model migration

* Transfers and payment modeling and UI improvements

* Fix CI

* Transfer matching flow

* Better UI for transfers

* Auto transfer matching, approve, reject flow

* Mark transfers created from form as confirmed

* Account filtering

* Excluded rejected transfers from calculations

* Calculation tweaks with transfer exclusions

* Clean up migration

test/controllers/account/trades_controller_test.rb

@@ -38,7 +38,7 @@ class Account::TradesControllerTest < ActionDispatch::IntegrationTest
 
     assert_difference -> { Account::Entry.count } => 2,
                       -> { Account::Transaction.count } => 2,
-                      -> { Account::Transfer.count } => 1 do
+                      -> { Transfer.count } => 1 do
       post account_trades_url, params: {
         account_entry: {
           account_id: @entry.account_id,
@@ -59,7 +59,7 @@ class Account::TradesControllerTest < ActionDispatch::IntegrationTest
 
     assert_difference -> { Account::Entry.count } => 2,
                       -> { Account::Transaction.count } => 2,
-                      -> { Account::Transfer.count } => 1 do
+                      -> { Transfer.count } => 1 do
       post account_trades_url, params: {
         account_entry: {
           account_id: @entry.account_id,
@@ -78,7 +78,7 @@ class Account::TradesControllerTest < ActionDispatch::IntegrationTest
   test "deposit and withdrawal has optional transfer account" do
     assert_difference -> { Account::Entry.count } => 1,
                       -> { Account::Transaction.count } => 1,
-                      -> { Account::Transfer.count } => 0 do
+                      -> { Transfer.count } => 0 do
       post account_trades_url, params: {
         account_entry: {
           account_id: @entry.account_id,
@@ -93,7 +93,6 @@ class Account::TradesControllerTest < ActionDispatch::IntegrationTest
     created_entry = Account::Entry.order(created_at: :desc).first
 
     assert created_entry.amount.positive?
-    assert created_entry.marked_as_transfer
     assert_redirected_to @entry.account
   end
 

test/controllers/account/transactions_controller_test.rb

@@ -74,7 +74,7 @@ class Account::TransactionsControllerTest < ActionDispatch::IntegrationTest
   end
 
   test "can destroy many transactions at once" do
-    transactions = @user.family.entries.account_transactions
+    transactions = @user.family.entries.account_transactions.incomes_and_expenses
     delete_count = transactions.size
 
     assert_difference([ "Account::Transaction.count", "Account::Entry.count" ], -delete_count) do

test/controllers/account/transfer_matches_controller_test.rb

@@ -0,0 +1,42 @@
+require "test_helper"
+
+class Account::TransferMatchesControllerTest < ActionDispatch::IntegrationTest
+  include Account::EntriesTestHelper
+
+  setup do
+    sign_in @user = users(:family_admin)
+  end
+
+  test "matches existing transaction and creates transfer" do
+    inflow_transaction = create_transaction(amount: 100, account: accounts(:depository))
+    outflow_transaction = create_transaction(amount: -100, account: accounts(:investment))
+
+    assert_difference "Transfer.count", 1 do
+      post account_transaction_transfer_match_path(inflow_transaction), params: {
+        transfer_match: {
+          method: "existing",
+          matched_entry_id: outflow_transaction.id
+        }
+      }
+    end
+
+    assert_redirected_to transactions_url
+    assert_equal "Transfer created", flash[:notice]
+  end
+
+  test "creates transfer for target account" do
+    inflow_transaction = create_transaction(amount: 100, account: accounts(:depository))
+
+    assert_difference [ "Transfer.count", "Account::Entry.count", "Account::Transaction.count" ], 1 do
+      post account_transaction_transfer_match_path(inflow_transaction), params: {
+        transfer_match: {
+          method: "new",
+          target_account_id: accounts(:investment).id
+        }
+      }
+    end
+
+    assert_redirected_to transactions_url
+    assert_equal "Transfer created", flash[:notice]
+  end
+end

test/controllers/transfers_controller_test.rb

@@ -1,19 +1,19 @@
 require "test_helper"
 
-class Account::TransfersControllerTest < ActionDispatch::IntegrationTest
+class TransfersControllerTest < ActionDispatch::IntegrationTest
   setup do
     sign_in users(:family_admin)
   end
 
   test "should get new" do
-    get new_account_transfer_url
+    get new_transfer_url
     assert_response :success
   end
 
   test "can create transfers" do
-    assert_difference "Account::Transfer.count", 1 do
-      post account_transfers_url, params: {
-        account_transfer: {
+    assert_difference "Transfer.count", 1 do
+      post transfers_url, params: {
+        transfer: {
           from_account_id: accounts(:depository).id,
           to_account_id: accounts(:credit_card).id,
           date: Date.current,
@@ -26,8 +26,8 @@ class Account::TransfersControllerTest < ActionDispatch::IntegrationTest
   end
 
   test "can destroy transfer" do
-    assert_difference -> { Account::Transfer.count } => -1, -> { Account::Transaction.count } => -2 do
-      delete account_transfer_url(account_transfers(:one))
+    assert_difference -> { Transfer.count } => -1, -> { Account::Transaction.count } => 0 do
+      delete transfer_url(transfers(:one))
     end
   end
 end

test/fixtures/account/entries.yml

@@ -31,8 +31,6 @@ transfer_out:
   amount: 100
   currency: USD
   account: depository
-  marked_as_transfer: true
-  transfer: one
   entryable_type: Account::Transaction
   entryable: transfer_out
 
@@ -42,7 +40,5 @@ transfer_in:
   amount: -100
   currency: USD
   account: credit_card
-  marked_as_transfer: true
-  transfer: one
   entryable_type: Account::Transaction
   entryable: transfer_in

test/fixtures/account/transfers.yml

@@ -1 +0,0 @@
-one: { }

test/fixtures/transfers.yml

@@ -0,0 +1,3 @@
+one:
+  inflow_transaction: transfer_in
+  outflow_transaction: transfer_out

test/models/account/transfer_test.rb

@@ -1,69 +0,0 @@
-require "test_helper"
-
-class Account::TransferTest < ActiveSupport::TestCase
-  setup do
-    @outflow = account_entries(:transfer_out)
-    @inflow = account_entries(:transfer_in)
-  end
-
-  test "transfer valid if it has inflow and outflow from different accounts for the same amount" do
-    transfer = Account::Transfer.create! entries: [ @inflow, @outflow ]
-
-    assert transfer.valid?
-  end
-
-  test "transfer must have 2 transactions" do
-    invalid_transfer_1 = Account::Transfer.new entries: [ @outflow ]
-    invalid_transfer_2 = Account::Transfer.new entries: [ @inflow, @outflow, account_entries(:transaction) ]
-
-    assert invalid_transfer_1.invalid?
-    assert invalid_transfer_2.invalid?
-  end
-
-  test "transfer cannot have 2 transactions from the same account" do
-    account = accounts(:depository)
-
-    inflow = account.entries.create! \
-      date: Date.current,
-      name: "Inflow",
-      amount: -100,
-      currency: "USD",
-      marked_as_transfer: true,
-      entryable: Account::Transaction.new
-
-    outflow = account.entries.create! \
-      date: Date.current,
-      name: "Outflow",
-      amount: 100,
-      currency: "USD",
-      marked_as_transfer: true,
-      entryable: Account::Transaction.new
-
-    assert_raise ActiveRecord::RecordInvalid do
-      Account::Transfer.create! entries: [ inflow, outflow ]
-    end
-  end
-
-  test "all transfer transactions must be marked as transfers" do
-    @inflow.update! marked_as_transfer: false
-
-    assert_raise ActiveRecord::RecordInvalid do
-      Account::Transfer.create! entries: [ @inflow, @outflow ]
-    end
-  end
-
-  test "single-currency transfer transactions must net to zero" do
-    @outflow.update! amount: 105
-
-    assert_raises ActiveRecord::RecordInvalid do
-      Account::Transfer.create! entries: [ @inflow, @outflow ]
-    end
-  end
-
-  test "multi-currency transfer transactions do not have to net to zero" do
-    @outflow.update! amount: 105, currency: "EUR"
-    transfer = Account::Transfer.create! entries: [ @inflow, @outflow ]
-
-    assert transfer.valid?
-  end
-end

test/models/family_test.rb

@@ -120,11 +120,9 @@ class FamilyTest < ActiveSupport::TestCase
 
   test "calculates rolling transaction totals" do
     account = create_account(balance: 1000, accountable: Depository.new)
-    liability_account = create_account(balance: 1000, accountable: Loan.new)
     create_transaction(account: account, date: 2.days.ago.to_date, amount: -500)
     create_transaction(account: account, date: 1.day.ago.to_date, amount: 100)
     create_transaction(account: account, date: Date.current, amount: 20)
-    create_transaction(account: liability_account, date: 2.days.ago.to_date, amount: -333)
 
     snapshot = @family.snapshot_transactions
 

test/models/transfer_test.rb

@@ -0,0 +1,100 @@
+require "test_helper"
+
+class TransferTest < ActiveSupport::TestCase
+  include Account::EntriesTestHelper
+
+  setup do
+    @outflow = account_transactions(:transfer_out)
+    @inflow = account_transactions(:transfer_in)
+  end
+
+  test "transfer has different accounts, opposing amounts, and within 4 days of each other" do
+    outflow_entry = create_transaction(date: Date.current, account: accounts(:depository), amount: 500)
+    inflow_entry = create_transaction(date: 1.day.ago.to_date, account: accounts(:credit_card), amount: -500)
+
+    assert_difference -> { Transfer.count } => 1 do
+      Transfer.create!(
+        inflow_transaction: inflow_entry.account_transaction,
+        outflow_transaction: outflow_entry.account_transaction,
+      )
+    end
+  end
+
+  test "transfer cannot have 2 transactions from the same account" do
+    outflow_entry = create_transaction(date: Date.current, account: accounts(:depository), amount: 500)
+    inflow_entry = create_transaction(date: 1.day.ago.to_date, account: accounts(:depository), amount: -500)
+
+    transfer = Transfer.new(
+      inflow_transaction: inflow_entry.account_transaction,
+      outflow_transaction: outflow_entry.account_transaction,
+    )
+
+    assert_no_difference -> { Transfer.count } do
+      transfer.save
+    end
+
+    assert_equal "Transfer must have different accounts", transfer.errors.full_messages.first
+  end
+
+  test "Transfer transactions must have opposite amounts" do
+    outflow_entry = create_transaction(date: Date.current, account: accounts(:depository), amount: 500)
+    inflow_entry = create_transaction(date: Date.current, account: accounts(:credit_card), amount: -400)
+
+    transfer = Transfer.new(
+      inflow_transaction: inflow_entry.account_transaction,
+      outflow_transaction: outflow_entry.account_transaction,
+    )
+
+    assert_no_difference -> { Transfer.count } do
+      transfer.save
+    end
+
+    assert_equal "Transfer transactions must have opposite amounts", transfer.errors.full_messages.first
+  end
+
+  test "transfer dates must be within 4 days of each other" do
+    outflow_entry = create_transaction(date: Date.current, account: accounts(:depository), amount: 500)
+    inflow_entry = create_transaction(date: 5.days.ago.to_date, account: accounts(:credit_card), amount: -500)
+
+    transfer = Transfer.new(
+      inflow_transaction: inflow_entry.account_transaction,
+      outflow_transaction: outflow_entry.account_transaction,
+    )
+
+    assert_no_difference -> { Transfer.count } do
+      transfer.save
+    end
+
+    assert_equal "Transfer transaction dates must be within 4 days of each other", transfer.errors.full_messages.first
+  end
+
+  test "from_accounts converts amounts to the to_account's currency" do
+    accounts(:depository).update!(currency: "EUR")
+
+    eur_account = accounts(:depository).reload
+    usd_account = accounts(:credit_card)
+
+    ExchangeRate.create!(
+      from_currency: "EUR",
+      to_currency: "USD",
+      rate: 1.1,
+      date: Date.current,
+    )
+
+    transfer = Transfer.from_accounts(
+      from_account: eur_account,
+      to_account: usd_account,
+      date: Date.current,
+      amount: 500,
+    )
+
+    assert_equal 500, transfer.outflow_transaction.entry.amount
+    assert_equal "EUR", transfer.outflow_transaction.entry.currency
+    assert_equal -550, transfer.inflow_transaction.entry.amount
+    assert_equal "USD", transfer.inflow_transaction.entry.currency
+
+    assert_difference -> { Transfer.count } => 1 do
+      transfer.save!
+    end
+  end
+end

test/system/transactions_test.rb

@@ -210,7 +210,7 @@ class TransactionsTest < ApplicationSystemTestCase
     end
 
     def number_of_transactions_on_page
-      [ @user.family.entries.without_transfers.count, @page_size ].min
+      [ @user.family.entries.count, @page_size ].min
     end
 
     def all_transactions_checkbox

test/system/transfers_test.rb

@@ -19,71 +19,13 @@ class TransfersTest < ApplicationSystemTestCase
 
     select checking_name, from: "From"
     select savings_name, from: "To"
-    fill_in "account_transfer[amount]", with: 500
+    fill_in "transfer[amount]", with: 500
     fill_in "Date", with: transfer_date
 
     click_button "Create transfer"
 
     within "#entry-group-" + transfer_date.to_s do
-      assert_text "Transfer from"
+      assert_text "Payment to"
     end
   end
-
-  test "can match 2 transactions and create a transfer" do
-    transfer_date = Date.current
-    outflow = accounts(:depository).entries.create! \
-      name: "Outflow from checking account",
-      date: transfer_date,
-      amount: 100,
-      currency: "USD",
-      entryable: Account::Transaction.new
-
-    inflow = accounts(:credit_card).entries.create! \
-      name: "Inflow to cc account",
-      date: transfer_date,
-      amount: -100,
-      currency: "USD",
-      entryable: Account::Transaction.new
-
-    visit transactions_url
-
-    transaction_entry_checkbox(inflow).check
-    transaction_entry_checkbox(outflow).check
-
-    bulk_transfer_action_button.click
-
-    click_on "Mark as transfers"
-
-    within "#entry-group-" + transfer_date.to_s do
-      assert_text "Outflow"
-      assert_text "Inflow"
-    end
-  end
-
-  test "can mark a single transaction as a transfer" do
-    txn = @user.family.entries.account_transactions.reverse_chronological.first
-
-    within "#" + dom_id(txn) do
-      assert_text txn.account_transaction.category.name || "Uncategorized"
-    end
-
-    transaction_entry_checkbox(txn).check
-
-    bulk_transfer_action_button.click
-    click_on "Mark as transfers"
-
-    within "#" + dom_id(txn) do
-      assert_no_text "Uncategorized"
-    end
-  end
-
-  private
-
-    def transaction_entry_checkbox(transaction_entry)
-      find("#" + dom_id(transaction_entry, "selection"))
-    end
-
-    def bulk_transfer_action_button
-      find("#bulk-transfer-btn")
-    end
 end