From ad4de99f1ad9f11b5ef1326c750ae96797b16ead Mon Sep 17 00:00:00 2001 From: Jakub Kottnauer Date: Wed, 24 Apr 2024 21:55:52 +0200 Subject: [PATCH] Add partial account sync support (#653) * Add partial account sync support * Fix indentation * Use historical balance as base when doing partial sync * Simplify controller crud tests * Fix linter errors --- app/controllers/transactions_controller.rb | 9 +- app/controllers/valuations_controller.rb | 17 +- app/jobs/account_sync_job.rb | 4 +- app/models/account/balance/calculator.rb | 8 +- app/models/account/syncable.rb | 10 +- .../transactions_controller_test.rb | 31 ++++ .../controllers/valuations_controller_test.rb | 51 +++++- .../models/account/balance/calculator_test.rb | 148 ++++++++++-------- test/models/account/syncable_test.rb | 93 ++++++++--- 9 files changed, 260 insertions(+), 111 deletions(-) diff --git a/app/controllers/transactions_controller.rb b/app/controllers/transactions_controller.rb index 1fdfa33d..46bdcfb9 100644 --- a/app/controllers/transactions_controller.rb +++ b/app/controllers/transactions_controller.rb @@ -75,7 +75,7 @@ class TransactionsController < ApplicationController respond_to do |format| if @transaction.save - @transaction.account.sync_later + @transaction.account.sync_later(@transaction.date) format.html { redirect_to transactions_url, notice: t(".success") } else format.html { render :new, status: :unprocessable_entity } @@ -85,8 +85,9 @@ class TransactionsController < ApplicationController def update respond_to do |format| + sync_start_date = [ @transaction.date, Date.parse(transaction_params[:date]) ].compact.min if @transaction.update(transaction_params) - @transaction.account.sync_later + @transaction.account.sync_later(sync_start_date) format.html { redirect_to transaction_url(@transaction), notice: t(".success") } format.turbo_stream do @@ -102,8 +103,10 @@ class TransactionsController < ApplicationController end def destroy + @account = @transaction.account + sync_start_date = @account.transactions.where("date < ?", @transaction.date).order(date: :desc).first&.date @transaction.destroy! - @transaction.account.sync_later + @account.sync_later(sync_start_date) respond_to do |format| format.html { redirect_to transactions_url, notice: t(".success") } diff --git a/app/controllers/valuations_controller.rb b/app/controllers/valuations_controller.rb index 71b72f35..c77d7067 100644 --- a/app/controllers/valuations_controller.rb +++ b/app/controllers/valuations_controller.rb @@ -1,11 +1,12 @@ class ValuationsController < ApplicationController + before_action :set_valuation, only: %i[ edit update destroy ] def create @account = Current.family.accounts.find(params[:account_id]) # TODO: placeholder logic until we have a better abstraction for trends @valuation = @account.valuations.new(valuation_params.merge(currency: Current.family.currency)) if @valuation.save - @valuation.account.sync_later + @valuation.account.sync_later(@valuation.date) respond_to do |format| format.html { redirect_to account_path(@account), notice: "Valuation created" } @@ -24,13 +25,12 @@ class ValuationsController < ApplicationController end def edit - @valuation = Valuation.find(params[:id]) end def update - @valuation = Valuation.find(params[:id]) + sync_start_date = [ @valuation.date, Date.parse(valuation_params[:date]) ].compact.min if @valuation.update(valuation_params) - @valuation.account.sync_later + @valuation.account.sync_later(sync_start_date) redirect_to account_path(@valuation.account), notice: "Valuation updated" else @@ -42,10 +42,10 @@ class ValuationsController < ApplicationController end def destroy - @valuation = Valuation.find(params[:id]) @account = @valuation.account + sync_start_date = @account.valuations.where("date < ?", @valuation.date).order(date: :desc).first&.date @valuation.destroy! - @account.sync_later + @account.sync_later(sync_start_date) respond_to do |format| format.html { redirect_to account_path(@account), notice: "Valuation deleted" } @@ -59,6 +59,11 @@ class ValuationsController < ApplicationController end private + # Use callbacks to share common setup or constraints between actions. + def set_valuation + @valuation = Valuation.find(params[:id]) + end + def valuation_params params.require(:valuation).permit(:date, :value) end diff --git a/app/jobs/account_sync_job.rb b/app/jobs/account_sync_job.rb index 8b9ac828..4464ef4b 100644 --- a/app/jobs/account_sync_job.rb +++ b/app/jobs/account_sync_job.rb @@ -1,7 +1,7 @@ class AccountSyncJob < ApplicationJob queue_as :default - def perform(account) - account.sync + def perform(account, start_date = nil) + account.sync(start_date) end end diff --git a/app/models/account/balance/calculator.rb b/app/models/account/balance/calculator.rb index 2f7e0d26..eaa59734 100644 --- a/app/models/account/balance/calculator.rb +++ b/app/models/account/balance/calculator.rb @@ -25,7 +25,7 @@ class Account::Balance::Calculator prior_balance = current_balance - { date: date, balance: current_balance, currency: @account.currency, updated_at: Time.current } + { date:, balance: current_balance, currency: @account.currency, updated_at: Time.current } end @daily_balances = [ @@ -92,11 +92,15 @@ class Account::Balance::Calculator end def implied_start_balance + if @calc_start_date > @account.effective_start_date + return @account.balance_on(@calc_start_date) + end + oldest_valuation_date = normalized_valuations.first&.dig("date") oldest_transaction_date = normalized_transactions.first&.dig("date") oldest_entry_date = [ oldest_valuation_date, oldest_transaction_date ].compact.min - if oldest_entry_date == oldest_valuation_date + if oldest_entry_date.present? && oldest_entry_date == oldest_valuation_date oldest_valuation = normalized_valuations.find { |v| v["date"] == oldest_valuation_date } oldest_valuation["value"].to_d else diff --git a/app/models/account/syncable.rb b/app/models/account/syncable.rb index 19c15337..1afd18c8 100644 --- a/app/models/account/syncable.rb +++ b/app/models/account/syncable.rb @@ -1,16 +1,18 @@ module Account::Syncable extend ActiveSupport::Concern - def sync_later - AccountSyncJob.perform_later self + def sync_later(start_date = nil) + AccountSyncJob.perform_later(self, start_date) end - def sync + def sync(start_date = nil) update!(status: "syncing") sync_exchange_rates - calculator = Account::Balance::Calculator.new(self) + calc_start_date = start_date - 1.day if start_date.present? && self.balance_on(start_date - 1.day).present? + + calculator = Account::Balance::Calculator.new(self, { calc_start_date: }) calculator.calculate self.balances.upsert_all(calculator.daily_balances, unique_by: :index_account_balances_on_account_id_date_currency_unique) self.balances.where("date < ?", effective_start_date).delete_all diff --git a/test/controllers/transactions_controller_test.rb b/test/controllers/transactions_controller_test.rb index c57f1403..11326f7f 100644 --- a/test/controllers/transactions_controller_test.rb +++ b/test/controllers/transactions_controller_test.rb @@ -4,6 +4,7 @@ class TransactionsControllerTest < ActionDispatch::IntegrationTest setup do sign_in @user = users(:family_admin) @transaction = transactions(:checking_one) + @account = @transaction.account end test "should get index" do @@ -31,6 +32,12 @@ class TransactionsControllerTest < ActionDispatch::IntegrationTest assert_redirected_to transactions_url end + test "create should sync account with correct start date" do + assert_enqueued_with(job: AccountSyncJob, args: [ @account, @transaction.date ]) do + post transactions_url, params: { transaction: { account_id: @transaction.account_id, amount: @transaction.amount, currency: @transaction.currency, date: @transaction.date, name: @transaction.name } } + end + end + test "creation preserves decimals" do assert_difference("Transaction.count") do post transactions_url, params: { transaction: { @@ -91,6 +98,18 @@ class TransactionsControllerTest < ActionDispatch::IntegrationTest assert_redirected_to transaction_url(@transaction) end + test "update should sync account with correct start date" do + new_date = @transaction.date - 1.day + assert_enqueued_with(job: AccountSyncJob, args: [ @account, new_date ]) do + patch transaction_url(@transaction), params: { transaction: { account_id: @transaction.account_id, amount: @transaction.amount, currency: @transaction.currency, date: new_date, name: @transaction.name } } + end + + new_date = @transaction.reload.date + 1.day + assert_enqueued_with(job: AccountSyncJob, args: [ @account, @transaction.date ]) do + patch transaction_url(@transaction), params: { transaction: { account_id: @transaction.account_id, amount: @transaction.amount, currency: @transaction.currency, date: new_date, name: @transaction.name } } + end + end + test "should destroy transaction" do assert_difference("Transaction.count", -1) do delete transaction_url(@transaction) @@ -98,4 +117,16 @@ class TransactionsControllerTest < ActionDispatch::IntegrationTest assert_redirected_to transactions_url end + + test "destroy should sync account with correct start date" do + first, second = @transaction.account.transactions.order(:date).all + + assert_enqueued_with(job: AccountSyncJob, args: [ @account, first.date ]) do + delete transaction_url(second) + end + + assert_enqueued_with(job: AccountSyncJob, args: [ @account, nil ]) do + delete transaction_url(first) + end + end end diff --git a/test/controllers/valuations_controller_test.rb b/test/controllers/valuations_controller_test.rb index c0f367ac..2c422408 100644 --- a/test/controllers/valuations_controller_test.rb +++ b/test/controllers/valuations_controller_test.rb @@ -3,7 +3,8 @@ require "test_helper" class ValuationsControllerTest < ActionDispatch::IntegrationTest setup do sign_in @user = users(:family_admin) - @account = accounts(:checking) + @valuation = valuations(:savings_one) + @account = @valuation.account end test "new" do @@ -11,9 +12,55 @@ class ValuationsControllerTest < ActionDispatch::IntegrationTest assert_response :success end - test "create" do + test "should create valuation" do assert_difference("Valuation.count") do post account_valuations_url(@account), params: { valuation: { value: 1, date: Date.current, type: "Appraisal" } } end end + + test "create should sync account with correct start date" do + date = Date.current - 1.day + + assert_enqueued_with(job: AccountSyncJob, args: [ @account, date ]) do + post account_valuations_url(@account), params: { valuation: { value: 2, date:, type: "Appraisal" } } + end + end + + test "should update valuation" do + date = @valuation.date + patch valuation_url(@valuation), params: { valuation: { account_id: @valuation.account_id, value: 1, date:, type: "Appraisal" } } + assert_redirected_to account_path(@valuation.account) + end + + test "update should sync account with correct start date" do + new_date = @valuation.date - 1.day + assert_enqueued_with(job: AccountSyncJob, args: [ @account, new_date ]) do + patch valuation_url(@valuation), params: { valuation: { account_id: @valuation.account_id, value: @valuation.value, date: new_date, type: "Appraisal" } } + end + + new_date = @valuation.reload.date + 1.day + assert_enqueued_with(job: AccountSyncJob, args: [ @account, @valuation.date ]) do + patch valuation_url(@valuation), params: { valuation: { account_id: @valuation.account_id, value: @valuation.value, date: new_date, type: "Appraisal" } } + end + end + + test "should destroy valuation" do + assert_difference("Valuation.count", -1) do + delete valuation_url(@valuation) + end + + assert_redirected_to account_path(@account) + end + + test "destroy should sync account with correct start date" do + first, second = @account.valuations.order(:date).all + + assert_enqueued_with(job: AccountSyncJob, args: [ @account, first.date ]) do + delete valuation_url(second) + end + + assert_enqueued_with(job: AccountSyncJob, args: [ @account, nil ]) do + delete valuation_url(first) + end + end end diff --git a/test/models/account/balance/calculator_test.rb b/test/models/account/balance/calculator_test.rb index ab631f8f..05a9abaf 100644 --- a/test/models/account/balance/calculator_test.rb +++ b/test/models/account/balance/calculator_test.rb @@ -2,97 +2,111 @@ require "test_helper" require "csv" class Account::Balance::CalculatorTest < ActiveSupport::TestCase - # See: https://docs.google.com/spreadsheets/d/18LN5N-VLq4b49Mq1fNwF7_eBiHSQB46qQduRtdAEN98/edit?usp=sharing - setup do - @expected_balances = CSV.read("test/fixtures/account/expected_balances.csv", headers: true).map do |row| - { - "date" => (Date.current + row["date_offset"].to_i.days).to_date, - "collectable" => row["collectable"], - "checking" => row["checking"], - "savings_with_valuation_overrides" => row["savings_with_valuation_overrides"], - "credit_card" => row["credit_card"], - "multi_currency" => row["multi_currency"], + # See: https://docs.google.com/spreadsheets/d/18LN5N-VLq4b49Mq1fNwF7_eBiHSQB46qQduRtdAEN98/edit?usp=sharing + setup do + @expected_balances = CSV.read("test/fixtures/account/expected_balances.csv", headers: true).map do |row| + { + "date" => (Date.current + row["date_offset"].to_i.days).to_date, + "collectable" => row["collectable"], + "checking" => row["checking"], + "savings_with_valuation_overrides" => row["savings_with_valuation_overrides"], + "credit_card" => row["credit_card"], + "multi_currency" => row["multi_currency"], - # Balances should be calculated for all currencies of an account - "eur_checking_eur" => row["eur_checking_eur"], - "eur_checking_usd" => row["eur_checking_usd"] - } - end + # Balances should be calculated for all currencies of an account + "eur_checking_eur" => row["eur_checking_eur"], + "eur_checking_usd" => row["eur_checking_usd"] + } end + end - test "syncs account with only valuations" do - account = accounts(:collectable) + test "syncs account with only valuations" do + account = accounts(:collectable) - calculator = Account::Balance::Calculator.new(account) - calculator.calculate + calculator = Account::Balance::Calculator.new(account) + calculator.calculate - expected = @expected_balances.map { |row| row["collectable"].to_d } - actual = calculator.daily_balances.map { |b| b[:balance] } + expected = @expected_balances.map { |row| row["collectable"].to_d } + actual = calculator.daily_balances.map { |b| b[:balance] } - assert_equal expected, actual - end + assert_equal expected, actual + end - test "syncs account with only transactions" do - account = accounts(:checking) + test "syncs account with only transactions" do + account = accounts(:checking) - calculator = Account::Balance::Calculator.new(account) - calculator.calculate + calculator = Account::Balance::Calculator.new(account) + calculator.calculate - expected = @expected_balances.map { |row| row["checking"].to_d } - actual = calculator.daily_balances.map { |b| b[:balance] } + expected = @expected_balances.map { |row| row["checking"].to_d } + actual = calculator.daily_balances.map { |b| b[:balance] } - assert_equal expected, actual - end + assert_equal expected, actual + end - test "syncs account with both valuations and transactions" do - account = accounts(:savings_with_valuation_overrides) + test "syncs account with both valuations and transactions" do + account = accounts(:savings_with_valuation_overrides) - calculator = Account::Balance::Calculator.new(account) - calculator.calculate + calculator = Account::Balance::Calculator.new(account) + calculator.calculate - expected = @expected_balances.map { |row| row["savings_with_valuation_overrides"].to_d } - actual = calculator.daily_balances.map { |b| b[:balance] } + expected = @expected_balances.map { |row| row["savings_with_valuation_overrides"].to_d } + actual = calculator.daily_balances.map { |b| b[:balance] } - assert_equal expected, actual - end + assert_equal expected, actual + end - test "syncs liability account" do - account = accounts(:credit_card) + test "syncs liability account" do + account = accounts(:credit_card) - calculator = Account::Balance::Calculator.new(account) - calculator.calculate + calculator = Account::Balance::Calculator.new(account) + calculator.calculate - expected = @expected_balances.map { |row| row["credit_card"].to_d } - actual = calculator.daily_balances.map { |b| b[:balance] } + expected = @expected_balances.map { |row| row["credit_card"].to_d } + actual = calculator.daily_balances.map { |b| b[:balance] } - assert_equal expected, actual - end + assert_equal expected, actual + end - test "syncs foreign currency account" do - account = accounts(:eur_checking) - calculator = Account::Balance::Calculator.new(account) - calculator.calculate + test "syncs foreign currency account" do + account = accounts(:eur_checking) + calculator = Account::Balance::Calculator.new(account) + calculator.calculate - # Calculator should calculate balances in both account and family currency - expected_eur_balances = @expected_balances.map { |row| row["eur_checking_eur"].to_d } - expected_usd_balances = @expected_balances.map { |row| row["eur_checking_usd"].to_d } + # Calculator should calculate balances in both account and family currency + expected_eur_balances = @expected_balances.map { |row| row["eur_checking_eur"].to_d } + expected_usd_balances = @expected_balances.map { |row| row["eur_checking_usd"].to_d } - actual_eur_balances = calculator.daily_balances.select { |b| b[:currency] == "EUR" }.sort_by { |b| b[:date] }.map { |b| b[:balance] } - actual_usd_balances = calculator.daily_balances.select { |b| b[:currency] == "USD" }.sort_by { |b| b[:date] }.map { |b| b[:balance] } + actual_eur_balances = calculator.daily_balances.select { |b| b[:currency] == "EUR" }.sort_by { |b| b[:date] }.map { |b| b[:balance] } + actual_usd_balances = calculator.daily_balances.select { |b| b[:currency] == "USD" }.sort_by { |b| b[:date] }.map { |b| b[:balance] } - assert_equal expected_eur_balances, actual_eur_balances - assert_equal expected_usd_balances, actual_usd_balances - end + assert_equal expected_eur_balances, actual_eur_balances + assert_equal expected_usd_balances, actual_usd_balances + end - test "syncs multi currency account" do - account = accounts(:multi_currency) - calculator = Account::Balance::Calculator.new(account) - calculator.calculate + test "syncs multi currency account" do + account = accounts(:multi_currency) + calculator = Account::Balance::Calculator.new(account) + calculator.calculate - expected_balances = @expected_balances.map { |row| row["multi_currency"].to_d } + expected_balances = @expected_balances.map { |row| row["multi_currency"].to_d } - actual_balances = calculator.daily_balances.map { |b| b[:balance] } + actual_balances = calculator.daily_balances.map { |b| b[:balance] } - assert_equal expected_balances, actual_balances - end + assert_equal expected_balances, actual_balances + end + + test "syncs with overridden start date" do + account = accounts(:multi_currency) + account.sync + calc_start_date = 10.days.ago.to_date + calculator = Account::Balance::Calculator.new(account, { calc_start_date: }) + calculator.calculate + + expected_balances = @expected_balances.filter { |row| row["date"] >= calc_start_date }.map { |row| row["multi_currency"].to_d } + + actual_balances = calculator.daily_balances.map { |b| b[:balance] } + + assert_equal expected_balances, actual_balances + end end diff --git a/test/models/account/syncable_test.rb b/test/models/account/syncable_test.rb index b99295ad..67dafe6d 100644 --- a/test/models/account/syncable_test.rb +++ b/test/models/account/syncable_test.rb @@ -1,38 +1,81 @@ require "test_helper" class Account::SyncableTest < ActiveSupport::TestCase - test "account has no balances until synced" do - account = accounts(:savings_with_valuation_overrides) + test "account has no balances until synced" do + account = accounts(:savings_with_valuation_overrides) - assert_equal 0, account.balances.count - end + assert_equal 0, account.balances.count + end - test "account has balances after syncing" do - account = accounts(:savings_with_valuation_overrides) - account.sync + test "account has balances after syncing" do + account = accounts(:savings_with_valuation_overrides) + account.sync - assert_equal 31, account.balances.count - end + assert_equal 31, account.balances.count + end - test "foreign currency account has balances in each currency after syncing" do - account = accounts(:eur_checking) - account.sync + test "partial sync with missing historical balances performs a full sync" do + account = accounts(:savings_with_valuation_overrides) + account.sync 10.days.ago.to_date - assert_equal 62, account.balances.count - assert_equal 31, account.balances.where(currency: "EUR").count - assert_equal 31, account.balances.where(currency: "USD").count - end + assert_equal 31, account.balances.count + end - test "stale balances are purged after syncing" do - account = accounts(:savings_with_valuation_overrides) + test "balances are updated after syncing" do + account = accounts(:savings_with_valuation_overrides) + balance_date = 10.days.ago + account.balances.create!(date: balance_date, balance: 1000) + account.sync - # Create old, stale balances that should be purged (since they are before account start date) - account.balances.create!(date: 1.year.ago, balance: 1000) - account.balances.create!(date: 2.years.ago, balance: 2000) - account.balances.create!(date: 3.years.ago, balance: 3000) + assert_equal 19500, account.balances.find_by(date: balance_date)[:balance] + end - account.sync + test "balances before sync start date are not updated after syncing" do + account = accounts(:savings_with_valuation_overrides) + balance_date = 10.days.ago + account.balances.create!(date: balance_date, balance: 1000) + account.sync 5.days.ago.to_date - assert_equal 31, account.balances.count - end + assert_equal 1000, account.balances.find_by(date: balance_date)[:balance] + end + + test "balances after sync start date are updated after syncing" do + account = accounts(:savings_with_valuation_overrides) + balance_date = 10.days.ago + account.balances.create!(date: balance_date, balance: 1000) + account.sync 20.days.ago.to_date + + assert_equal 19500, account.balances.find_by(date: balance_date)[:balance] + end + + test "balance on the sync date is updated after syncing" do + account = accounts(:savings_with_valuation_overrides) + balance_date = 5.days.ago + account.balances.create!(date: balance_date, balance: 1000) + account.sync balance_date.to_date + + assert_equal 19700, account.balances.find_by(date: balance_date)[:balance] + end + + test "foreign currency account has balances in each currency after syncing" do + account = accounts(:eur_checking) + account.sync + + assert_equal 62, account.balances.count + assert_equal 31, account.balances.where(currency: "EUR").count + assert_equal 31, account.balances.where(currency: "USD").count + end + + test "stale balances are purged after syncing" do + account = accounts(:savings_with_valuation_overrides) + + # Create old, stale balances that should be purged (since they are before account start date) + account.balances.create!(date: 1.year.ago, balance: 1000) + account.balances.create!(date: 2.years.ago, balance: 2000) + account.balances.create!(date: 3.years.ago, balance: 3000) + + account.sync + + assert_equal 31, account.balances.count + end end