Improve account transaction, trade, and valuation editing and sync experience (#1506)

* Consolidate entry controller logic

* Transaction builder

* Update trades controller to use new params

* Load account charts in turbo frames, fix PG overflow

* Consolidate tests

* Tests passing

* Remove unused code

* Add client side trade form validations

test/controllers/account/entries_controller_test.rb

@@ -3,63 +3,11 @@ require "test_helper"
 class Account::EntriesControllerTest < ActionDispatch::IntegrationTest
   setup do
     sign_in @user = users(:family_admin)
-    @transaction = account_entries :transaction
-    @valuation = account_entries :valuation
-    @trade = account_entries :trade
+    @entry = account_entries(:transaction)
   end
 
-  # =================
-  # Shared
-  # =================
-
-  test "should destroy entry" do
-    [ @transaction, @valuation, @trade ].each do |entry|
-      assert_difference -> { Account::Entry.count } => -1, -> { entry.entryable_class.count } => -1 do
-        delete account_entry_url(entry.account, entry)
-      end
-
-      assert_redirected_to account_url(entry.account)
-      assert_enqueued_with(job: SyncJob)
-    end
+  test "gets index" do
+    get account_entries_path(account_id: @entry.account.id)
+    assert_response :success
   end
-
-  test "gets show" do
-    [ @transaction, @valuation, @trade ].each do |entry|
-      get account_entry_url(entry.account, entry)
-      assert_response :success
-    end
-  end
-
-  test "gets edit" do
-    [ @valuation ].each do |entry|
-      get edit_account_entry_url(entry.account, entry)
-      assert_response :success
-    end
-  end
-
-  test "can update generic entry" do
-    [ @transaction, @valuation, @trade ].each do |entry|
-      assert_no_difference_in_entries do
-        patch account_entry_url(entry.account, entry), params: {
-          account_entry: {
-            name: "Name",
-            date: Date.current,
-            currency: "USD",
-            amount: 100
-          }
-        }
-      end
-
-      assert_redirected_to account_entry_url(entry.account, entry)
-      assert_enqueued_with(job: SyncJob)
-    end
-  end
-
-  private
-
-    # Simple guard to verify that nested attributes are passed the record ID to avoid new creation of record
-    # See `update_only` option of accepts_nested_attributes_for
-    def assert_no_difference_in_entries(&block)
-      assert_no_difference [ "Account::Entry.count", "Account::Transaction.count", "Account::Valuation.count" ], &block
-    end
 end

test/controllers/account/holdings_controller_test.rb

@@ -8,12 +8,12 @@ class Account::HoldingsControllerTest < ActionDispatch::IntegrationTest
   end
 
   test "gets holdings" do
-    get account_holdings_url(@account)
+    get account_holdings_url(account_id: @account.id)
     assert_response :success
   end
 
   test "gets holding" do
-    get account_holding_path(@account, @holding)
+    get account_holding_path(@holding)
 
     assert_response :success
   end
@@ -21,10 +21,10 @@ class Account::HoldingsControllerTest < ActionDispatch::IntegrationTest
   test "destroys holding and associated entries" do
     assert_difference -> { Account::Holding.count } => -1,
                       -> { Account::Entry.count } => -1 do
-      delete account_holding_path(@account, @holding)
+      delete account_holding_path(@holding)
     end
 
-    assert_redirected_to account_holdings_path(@account)
-    assert_empty @account.entries.where(entryable: @account.trades.where(security: @holding.security))
+    assert_redirected_to account_path(@holding.account)
+    assert_empty @holding.account.entries.where(entryable: @holding.account.trades.where(security: @holding.security))
   end
 end

test/controllers/account/trades_controller_test.rb

@@ -1,19 +1,36 @@
 require "test_helper"
 
 class Account::TradesControllerTest < ActionDispatch::IntegrationTest
+  include EntryableResourceInterfaceTest
+
   setup do
     sign_in @user = users(:family_admin)
-    @entry = account_entries :trade
+    @entry = account_entries(:trade)
   end
 
-  test "should get index" do
-    get account_trades_url(@entry.account)
-    assert_response :success
-  end
+  test "updates trade entry" do
+    assert_no_difference [ "Account::Entry.count", "Account::Trade.count" ] do
+      patch account_trade_url(@entry), params: {
+        account_entry: {
+          currency: "USD",
+          entryable_attributes: {
+            id: @entry.entryable_id,
+            qty: 20,
+            price: 20
+          }
+        }
+      }
+    end
 
-  test "should get new" do
-    get new_account_trade_url(@entry.account)
-    assert_response :success
+    @entry.reload
+
+    assert_enqueued_with job: SyncJob
+
+    assert_equal 20, @entry.account_trade.qty
+    assert_equal 20, @entry.account_trade.price
+    assert_equal "USD", @entry.currency
+
+    assert_redirected_to account_url(@entry.account)
   end
 
   test "creates deposit entry" do
@@ -22,9 +39,10 @@ class Account::TradesControllerTest < ActionDispatch::IntegrationTest
     assert_difference -> { Account::Entry.count } => 2,
                       -> { Account::Transaction.count } => 2,
                       -> { Account::Transfer.count } => 1 do
-      post account_trades_url(@entry.account), params: {
+      post account_trades_url, params: {
         account_entry: {
-          type: "transfer_in",
+          account_id: @entry.account_id,
+          type: "deposit",
           date: Date.current,
           amount: 10,
           currency: "USD",
@@ -42,9 +60,10 @@ class Account::TradesControllerTest < ActionDispatch::IntegrationTest
     assert_difference -> { Account::Entry.count } => 2,
                       -> { Account::Transaction.count } => 2,
                       -> { Account::Transfer.count } => 1 do
-      post account_trades_url(@entry.account), params: {
+      post account_trades_url, params: {
         account_entry: {
-          type: "transfer_out",
+          account_id: @entry.account_id,
+          type: "withdrawal",
           date: Date.current,
           amount: 10,
           currency: "USD",
@@ -60,9 +79,10 @@ class Account::TradesControllerTest < ActionDispatch::IntegrationTest
     assert_difference -> { Account::Entry.count } => 1,
                       -> { Account::Transaction.count } => 1,
                       -> { Account::Transfer.count } => 0 do
-      post account_trades_url(@entry.account), params: {
+      post account_trades_url, params: {
         account_entry: {
-          type: "transfer_out",
+          account_id: @entry.account_id,
+          type: "withdrawal",
           date: Date.current,
           amount: 10,
           currency: "USD"
@@ -79,8 +99,9 @@ class Account::TradesControllerTest < ActionDispatch::IntegrationTest
 
   test "creates interest entry" do
     assert_difference [ "Account::Entry.count", "Account::Transaction.count" ], 1 do
-      post account_trades_url(@entry.account), params: {
+      post account_trades_url, params: {
         account_entry: {
+          account_id: @entry.account_id,
           type: "interest",
           date: Date.current,
           amount: 10,
@@ -97,13 +118,15 @@ class Account::TradesControllerTest < ActionDispatch::IntegrationTest
 
   test "creates trade buy entry" do
     assert_difference [ "Account::Entry.count", "Account::Trade.count", "Security.count" ], 1 do
-      post account_trades_url(@entry.account), params: {
+      post account_trades_url, params: {
         account_entry: {
+          account_id: @entry.account_id,
           type: "buy",
           date: Date.current,
           ticker: "NVDA (NASDAQ)",
           qty: 10,
-          price: 10
+          price: 10,
+          currency: "USD"
         }
       }
     end
@@ -112,15 +135,16 @@ class Account::TradesControllerTest < ActionDispatch::IntegrationTest
 
     assert created_entry.amount.positive?
     assert created_entry.account_trade.qty.positive?
-    assert_equal "Transaction created successfully.", flash[:notice]
+    assert_equal "Entry created", flash[:notice]
     assert_enqueued_with job: SyncJob
-    assert_redirected_to @entry.account
+    assert_redirected_to account_url(created_entry.account)
   end
 
   test "creates trade sell entry" do
     assert_difference [ "Account::Entry.count", "Account::Trade.count" ], 1 do
-      post account_trades_url(@entry.account), params: {
+      post account_trades_url, params: {
         account_entry: {
+          account_id: @entry.account_id,
           type: "sell",
           ticker: "AAPL (NYSE)",
           date: Date.current,
@@ -135,8 +159,8 @@ class Account::TradesControllerTest < ActionDispatch::IntegrationTest
 
     assert created_entry.amount.negative?
     assert created_entry.account_trade.qty.negative?
-    assert_equal "Transaction created successfully.", flash[:notice]
+    assert_equal "Entry created", flash[:notice]
     assert_enqueued_with job: SyncJob
-    assert_redirected_to @entry.account
+    assert_redirected_to account_url(created_entry.account)
   end
 end

test/controllers/account/transactions_controller_test.rb

@@ -1,40 +1,117 @@
 require "test_helper"
 
 class Account::TransactionsControllerTest < ActionDispatch::IntegrationTest
+  include EntryableResourceInterfaceTest
+
   setup do
     sign_in @user = users(:family_admin)
-    @entry = account_entries :transaction
+    @entry = account_entries(:transaction)
   end
 
-  test "should get index" do
-    get account_transactions_url(@entry.account)
-    assert_response :success
-  end
-
-  test "update" do
-    assert_no_difference [ "Account::Entry.count", "Account::Transaction.count" ] do
-      patch account_transaction_url(@entry.account, @entry), params: {
+  test "creates with transaction details" do
+    assert_difference [ "Account::Entry.count", "Account::Transaction.count" ], 1 do
+      post account_transactions_url, params: {
         account_entry: {
-          name: "Name",
+          account_id: @entry.account_id,
+          name: "New transaction",
           date: Date.current,
           currency: "USD",
           amount: 100,
-          nature: "income",
-          entryable_type: @entry.entryable_type,
+          nature: "inflow",
           entryable_attributes: {
-            id: @entry.entryable_id,
             tag_ids: [ Tag.first.id, Tag.second.id ],
             category_id: Category.first.id,
-            merchant_id: Merchant.first.id,
-            notes: "test notes",
-            excluded: false
+            merchant_id: Merchant.first.id
           }
         }
       }
     end
 
-    assert_equal "Transaction updated successfully.", flash[:notice]
-    assert_redirected_to account_entry_url(@entry.account, @entry)
+    created_entry = Account::Entry.order(:created_at).last
+
+    assert_redirected_to account_url(created_entry.account)
+    assert_equal "Entry created", flash[:notice]
     assert_enqueued_with(job: SyncJob)
   end
+
+  test "updates with transaction details" do
+    assert_no_difference [ "Account::Entry.count", "Account::Transaction.count" ] do
+      patch account_transaction_url(@entry), params: {
+        account_entry: {
+          name: "Updated name",
+          date: Date.current,
+          currency: "USD",
+          amount: 100,
+          nature: "inflow",
+          entryable_type: @entry.entryable_type,
+          notes: "test notes",
+          excluded: false,
+          entryable_attributes: {
+            id: @entry.entryable_id,
+            tag_ids: [ Tag.first.id, Tag.second.id ],
+            category_id: Category.first.id,
+            merchant_id: Merchant.first.id
+          }
+        }
+      }
+    end
+
+    @entry.reload
+
+    assert_equal "Updated name", @entry.name
+    assert_equal Date.current, @entry.date
+    assert_equal "USD", @entry.currency
+    assert_equal -100, @entry.amount
+    assert_equal [ Tag.first.id, Tag.second.id ], @entry.entryable.tag_ids.sort
+    assert_equal Category.first.id, @entry.entryable.category_id
+    assert_equal Merchant.first.id, @entry.entryable.merchant_id
+    assert_equal "test notes", @entry.notes
+    assert_equal false, @entry.excluded
+
+    assert_equal "Entry updated", flash[:notice]
+    assert_redirected_to account_url(@entry.account)
+    assert_enqueued_with(job: SyncJob)
+  end
+
+  test "can destroy many transactions at once" do
+    transactions = @user.family.entries.account_transactions
+    delete_count = transactions.size
+
+    assert_difference([ "Account::Transaction.count", "Account::Entry.count" ], -delete_count) do
+      post bulk_delete_account_transactions_url, params: {
+        bulk_delete: {
+          entry_ids: transactions.pluck(:id)
+        }
+      }
+    end
+
+    assert_redirected_to transactions_url
+    assert_equal "#{delete_count} transactions deleted", flash[:notice]
+  end
+
+  test "can update many transactions at once" do
+    transactions = @user.family.entries.account_transactions
+
+    assert_difference [ "Account::Entry.count", "Account::Transaction.count" ], 0 do
+      post bulk_update_account_transactions_url, params: {
+        bulk_update: {
+          entry_ids: transactions.map(&:id),
+          date: 1.day.ago.to_date,
+          category_id: Category.second.id,
+          merchant_id: Merchant.second.id,
+          notes: "Updated note"
+        }
+      }
+    end
+
+    assert_redirected_to transactions_url
+    assert_equal "#{transactions.count} transactions updated", flash[:notice]
+
+    transactions.reload.each do |transaction|
+      assert_equal 1.day.ago.to_date, transaction.date
+      assert_equal Category.second, transaction.account_transaction.category
+      assert_equal Merchant.second, transaction.account_transaction.merchant
+      assert_equal "Updated note", transaction.notes
+    end
+  end
 end

test/controllers/account/valuations_controller_test.rb

@@ -1,36 +1,11 @@
 require "test_helper"
 
 class Account::ValuationsControllerTest < ActionDispatch::IntegrationTest
+  include EntryableResourceInterfaceTest
+
   setup do
     sign_in @user = users(:family_admin)
-    @entry = account_entries :valuation
-  end
-
-  test "should get index" do
-    get account_valuations_url(@entry.account)
-    assert_response :success
-  end
-
-  test "should get new" do
-    get new_account_valuation_url(@entry.account)
-    assert_response :success
-  end
-
-  test "create" do
-    assert_difference [ "Account::Entry.count", "Account::Valuation.count" ], 1 do
-      post account_valuations_url(@entry.account), params: {
-        account_entry: {
-          name: "Manual valuation",
-          amount: 19800,
-          date: Date.current,
-          currency: "USD"
-        }
-      }
-    end
-
-    assert_equal "Valuation created successfully.", flash[:notice]
-    assert_enqueued_with job: SyncJob
-    assert_redirected_to account_valuations_path(@entry.account)
+    @entry = account_entries(:valuation)
   end
 
   test "error when valuation already exists for date" do
@@ -44,7 +19,43 @@ class Account::ValuationsControllerTest < ActionDispatch::IntegrationTest
       }
     end
 
-    assert_equal "Date has already been taken", flash[:alert]
-    assert_redirected_to @entry.account
+    assert_response :unprocessable_entity
+  end
+
+  test "creates entry with basic attributes" do
+    assert_difference [ "Account::Entry.count", "Account::Valuation.count" ], 1 do
+      post account_valuations_url, params: {
+        account_entry: {
+          name: "New entry",
+          amount: 10000,
+          currency: "USD",
+          date: Date.current,
+          account_id: @entry.account_id
+        }
+      }
+    end
+
+    created_entry = Account::Entry.order(created_at: :desc).first
+
+    assert_enqueued_with job: SyncJob
+
+    assert_redirected_to account_url(created_entry.account)
+  end
+
+  test "updates entry with basic attributes" do
+    assert_no_difference [ "Account::Entry.count", "Account::Valuation.count" ] do
+      patch account_valuation_url(@entry), params: {
+        account_entry: {
+          name: "Updated entry",
+          amount: 20000,
+          currency: "USD",
+          date: Date.current
+        }
+      }
+    end
+
+    assert_enqueued_with job: SyncJob
+
+    assert_redirected_to account_url(@entry.account)
   end
 end

test/controllers/transactions_controller_test.rb

@@ -8,83 +8,6 @@ class TransactionsControllerTest < ActionDispatch::IntegrationTest
     @transaction = account_entries(:transaction)
   end
 
-  test "should get new" do
-    get new_transaction_url
-    assert_response :success
-  end
-
-  test "prefills account_id" do
-    get new_transaction_url(account_id: @transaction.account.id)
-    assert_response :success
-    assert_select "option[selected][value='#{@transaction.account.id}']"
-  end
-
-  test "should create transaction" do
-    account = @user.family.accounts.first
-    entry_params = {
-      account_id: account.id,
-      amount: 100.45,
-      currency: "USD",
-      date: Date.current,
-      name: "Test transaction",
-      entryable_type: "Account::Transaction",
-      entryable_attributes: { category_id: categories(:food_and_drink).id }
-    }
-
-    assert_difference [ "Account::Entry.count", "Account::Transaction.count" ], 1 do
-      post transactions_url, params: { account_entry: entry_params }
-    end
-
-    assert_equal entry_params[:amount].to_d, Account::Transaction.order(created_at: :desc).first.entry.amount
-    assert_equal "New transaction created successfully", flash[:notice]
-    assert_enqueued_with(job: SyncJob)
-    assert_redirected_to account_url(account)
-  end
-
-  test "expenses are positive" do
-    assert_difference([ "Account::Transaction.count", "Account::Entry.count" ], 1) do
-      post transactions_url, params: {
-        account_entry: {
-          nature: "expense",
-          account_id: @transaction.account_id,
-          amount: @transaction.amount,
-          currency: @transaction.currency,
-          date: @transaction.date,
-          name: @transaction.name,
-          entryable_type: "Account::Transaction",
-          entryable_attributes: {}
-        }
-      }
-    end
-
-    created_entry = Account::Entry.order(created_at: :desc).first
-
-    assert_redirected_to account_url(@transaction.account)
-    assert created_entry.amount.positive?, "Amount should be positive"
-  end
-
-  test "incomes are negative" do
-    assert_difference("Account::Transaction.count") do
-      post transactions_url, params: {
-        account_entry: {
-          nature: "income",
-          account_id: @transaction.account_id,
-          amount: @transaction.amount,
-          currency: @transaction.currency,
-          date: @transaction.date,
-          name: @transaction.name,
-          entryable_type: "Account::Transaction",
-          entryable_attributes: { category_id: categories(:food_and_drink).id }
-        }
-      }
-    end
-
-    created_entry = Account::Entry.order(created_at: :desc).first
-
-    assert_redirected_to account_url(@transaction.account)
-    assert created_entry.amount.negative?, "Amount should be negative"
-  end
-
   test "transaction count represents filtered total" do
     family = families(:empty)
     sign_in family.users.first
@@ -135,46 +58,4 @@ class TransactionsControllerTest < ActionDispatch::IntegrationTest
 
     assert_dom "#" + dom_id(sorted_transactions.last), count: 1
   end
-
-  test "can destroy many transactions at once" do
-    transactions = @user.family.entries.account_transactions
-    delete_count = transactions.size
-
-    assert_difference([ "Account::Transaction.count", "Account::Entry.count" ], -delete_count) do
-      post bulk_delete_transactions_url, params: {
-        bulk_delete: {
-          entry_ids: transactions.pluck(:id)
-        }
-      }
-    end
-
-    assert_redirected_to transactions_url
-    assert_equal "#{delete_count} transactions deleted", flash[:notice]
-  end
-
-  test "can update many transactions at once" do
-    transactions = @user.family.entries.account_transactions
-
-    assert_difference [ "Account::Entry.count", "Account::Transaction.count" ], 0 do
-      post bulk_update_transactions_url, params: {
-        bulk_update: {
-          entry_ids: transactions.map(&:id),
-          date: 1.day.ago.to_date,
-          category_id: Category.second.id,
-          merchant_id: Merchant.second.id,
-          notes: "Updated note"
-        }
-      }
-    end
-
-    assert_redirected_to transactions_url
-    assert_equal "#{transactions.count} transactions updated", flash[:notice]
-
-    transactions.reload.each do |transaction|
-      assert_equal 1.day.ago.to_date, transaction.date
-      assert_equal Category.second, transaction.account_transaction.category
-      assert_equal Merchant.second, transaction.account_transaction.merchant
-      assert_equal "Updated note", transaction.notes
-    end
-  end
 end