From bf3ce5d79b7a363fc6a031cafa96fb0d23dc4084 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Wed, 23 Jul 2025 18:07:52 -0400 Subject: [PATCH] Balance series uses new component fields --- app/models/account/reconciliation_manager.rb | 4 +- app/models/balance/chart_series_builder.rb | 122 +++++++++--------- app/models/balance/materializer.rb | 22 +++- .../account/reconciliation_manager_test.rb | 24 ++-- .../balance/chart_series_builder_test.rb | 30 +++-- test/models/balance/materializer_test.rb | 41 +++--- test/support/balance_test_helper.rb | 72 +++++++++++ 7 files changed, 191 insertions(+), 124 deletions(-) create mode 100644 test/support/balance_test_helper.rb diff --git a/app/models/account/reconciliation_manager.rb b/app/models/account/reconciliation_manager.rb index aac821b2..6fadcfa1 100644 --- a/app/models/account/reconciliation_manager.rb +++ b/app/models/account/reconciliation_manager.rb @@ -82,8 +82,8 @@ class Account::ReconciliationManager balance_record = account.balances.find_by(date: date, currency: account.currency) { - cash_balance: balance_record&.cash_balance, - balance: balance_record&.balance + cash_balance: balance_record&.end_cash_balance, + balance: balance_record&.end_balance } end end diff --git a/app/models/balance/chart_series_builder.rb b/app/models/balance/chart_series_builder.rb index 0c367685..dad10817 100644 --- a/app/models/balance/chart_series_builder.rb +++ b/app/models/balance/chart_series_builder.rb @@ -8,21 +8,21 @@ class Balance::ChartSeriesBuilder end def balance_series - build_series_for(:balance) + build_series_for(:end_balance) rescue => e Rails.logger.error "Balance series error: #{e.message} for accounts #{@account_ids}" raise end def cash_balance_series - build_series_for(:cash_balance) + build_series_for(:end_cash_balance) rescue => e Rails.logger.error "Cash balance series error: #{e.message} for accounts #{@account_ids}" raise end def holdings_balance_series - build_series_for(:holdings_balance) + build_series_for(:end_holdings_balance) rescue => e Rails.logger.error "Holdings balance series error: #{e.message} for accounts #{@account_ids}" raise @@ -37,13 +37,20 @@ class Balance::ChartSeriesBuilder def build_series_for(column) values = query_data.map do |datum| + # Map column names to their start equivalents + previous_column = case column + when :end_balance then :start_balance + when :end_cash_balance then :start_cash_balance + when :end_holdings_balance then :start_holdings_balance + end + Series::Value.new( date: datum.date, date_formatted: I18n.l(datum.date, format: :long), value: Money.new(datum.send(column), currency), trend: Trend.new( current: Money.new(datum.send(column), currency), - previous: Money.new(datum.send("previous_#{column}"), currency), + previous: Money.new(datum.send(previous_column), currency), favorable_direction: favorable_direction ) ) @@ -88,66 +95,57 @@ class Balance::ChartSeriesBuilder WITH dates AS ( SELECT generate_series(DATE :start_date, DATE :end_date, :interval::interval)::date AS date UNION DISTINCT - SELECT :end_date::date -- Pass in date to ensure timezone-aware "today" date - ), aggregated_balances AS ( - SELECT - d.date, - -- Total balance (assets positive, liabilities negative) - SUM( - CASE WHEN accounts.classification = 'asset' - THEN COALESCE(last_bal.balance, 0) - ELSE -COALESCE(last_bal.balance, 0) - END * COALESCE(er.rate, 1) * :sign_multiplier::integer - ) AS balance, - -- Cash-only balance - SUM( - CASE WHEN accounts.classification = 'asset' - THEN COALESCE(last_bal.cash_balance, 0) - ELSE -COALESCE(last_bal.cash_balance, 0) - END * COALESCE(er.rate, 1) * :sign_multiplier::integer - ) AS cash_balance, - -- Holdings value (balance ‑ cash) - SUM( - CASE WHEN accounts.classification = 'asset' - THEN COALESCE(last_bal.balance, 0) - COALESCE(last_bal.cash_balance, 0) - ELSE 0 - END * COALESCE(er.rate, 1) * :sign_multiplier::integer - ) AS holdings_balance - FROM dates d - JOIN accounts ON accounts.id = ANY(array[:account_ids]::uuid[]) - - -- Last observation carried forward (LOCF), use the most recent balance on or before the chart date - LEFT JOIN LATERAL ( - SELECT b.balance, b.cash_balance - FROM balances b - WHERE b.account_id = accounts.id - AND b.date <= d.date - ORDER BY b.date DESC - LIMIT 1 - ) last_bal ON TRUE - - -- Last observation carried forward (LOCF), use the most recent exchange rate on or before the chart date - LEFT JOIN LATERAL ( - SELECT er.rate - FROM exchange_rates er - WHERE er.from_currency = accounts.currency - AND er.to_currency = :target_currency - AND er.date <= d.date - ORDER BY er.date DESC - LIMIT 1 - ) er ON TRUE - GROUP BY d.date + SELECT :end_date::date -- Ensure end date is included ) SELECT - date, - balance, - cash_balance, - holdings_balance, - COALESCE(LAG(balance) OVER (ORDER BY date), 0) AS previous_balance, - COALESCE(LAG(cash_balance) OVER (ORDER BY date), 0) AS previous_cash_balance, - COALESCE(LAG(holdings_balance) OVER (ORDER BY date), 0) AS previous_holdings_balance - FROM aggregated_balances - ORDER BY date + d.date, + -- Use flows_factor: already handles asset (+1) vs liability (-1) + COALESCE(SUM(last_bal.end_balance * last_bal.flows_factor * COALESCE(er.rate, 1) * :sign_multiplier::integer), 0) AS end_balance, + COALESCE(SUM(last_bal.end_cash_balance * last_bal.flows_factor * COALESCE(er.rate, 1) * :sign_multiplier::integer), 0) AS end_cash_balance, + -- Holdings only for assets (flows_factor = 1) + COALESCE(SUM( + CASE WHEN last_bal.flows_factor = 1 + THEN last_bal.end_non_cash_balance + ELSE 0 + END * COALESCE(er.rate, 1) * :sign_multiplier::integer + ), 0) AS end_holdings_balance, + -- Previous balances + COALESCE(SUM(last_bal.start_balance * last_bal.flows_factor * COALESCE(er.rate, 1) * :sign_multiplier::integer), 0) AS start_balance, + COALESCE(SUM(last_bal.start_cash_balance * last_bal.flows_factor * COALESCE(er.rate, 1) * :sign_multiplier::integer), 0) AS start_cash_balance, + COALESCE(SUM( + CASE WHEN last_bal.flows_factor = 1 + THEN last_bal.start_non_cash_balance + ELSE 0 + END * COALESCE(er.rate, 1) * :sign_multiplier::integer + ), 0) AS start_holdings_balance + FROM dates d + CROSS JOIN accounts + LEFT JOIN LATERAL ( + SELECT b.end_balance, + b.end_cash_balance, + b.end_non_cash_balance, + b.start_balance, + b.start_cash_balance, + b.start_non_cash_balance, + b.flows_factor + FROM balances b + WHERE b.account_id = accounts.id + AND b.date <= d.date + ORDER BY b.date DESC + LIMIT 1 + ) last_bal ON TRUE + LEFT JOIN LATERAL ( + SELECT er.rate + FROM exchange_rates er + WHERE er.from_currency = accounts.currency + AND er.to_currency = :target_currency + AND er.date <= d.date + ORDER BY er.date DESC + LIMIT 1 + ) er ON TRUE + WHERE accounts.id = ANY(array[:account_ids]::uuid[]) + GROUP BY d.date + ORDER BY d.date SQL end end diff --git a/app/models/balance/materializer.rb b/app/models/balance/materializer.rb index 17a950f7..c6501ffa 100644 --- a/app/models/balance/materializer.rb +++ b/app/models/balance/materializer.rb @@ -28,9 +28,20 @@ class Balance::Materializer end def update_account_info - calculated_balance = @balances.sort_by(&:date).last&.balance || 0 - calculated_holdings_value = @holdings.select { |h| h.date == Date.current }.sum(&:amount) || 0 - calculated_cash_balance = calculated_balance - calculated_holdings_value + # Query fresh balance from DB to get generated column values + current_balance = account.balances + .where(currency: account.currency) + .order(date: :desc) + .first + + if current_balance + calculated_balance = current_balance.end_balance + calculated_cash_balance = current_balance.end_cash_balance + else + # Fallback if no balance exists + calculated_balance = 0 + calculated_cash_balance = 0 + end Rails.logger.info("Balance update: cash=#{calculated_cash_balance}, total=#{calculated_balance}") @@ -61,7 +72,10 @@ class Balance::Materializer end def purge_stale_balances - deleted_count = account.balances.delete_by("date < ?", account.start_date) + sorted_balances = @balances.sort_by(&:date) + oldest_calculated_balance_date = sorted_balances.first&.date + newest_calculated_balance_date = sorted_balances.last&.date + deleted_count = account.balances.delete_by("date < ? OR date > ?", oldest_calculated_balance_date, newest_calculated_balance_date) Rails.logger.info("Purged #{deleted_count} stale balances") if deleted_count > 0 end diff --git a/test/models/account/reconciliation_manager_test.rb b/test/models/account/reconciliation_manager_test.rb index 794c2cc5..fe5257e9 100644 --- a/test/models/account/reconciliation_manager_test.rb +++ b/test/models/account/reconciliation_manager_test.rb @@ -1,18 +1,15 @@ require "test_helper" class Account::ReconciliationManagerTest < ActiveSupport::TestCase + include BalanceTestHelper + setup do @account = accounts(:investment) @manager = Account::ReconciliationManager.new(@account) end test "new reconciliation" do - @account.balances.create!( - date: Date.current, - balance: 1000, - cash_balance: 500, - currency: @account.currency - ) + create_balance(account: @account, date: Date.current, balance: 1000, cash_balance: 500) result = @manager.reconcile_balance(balance: 1200, date: Date.current) @@ -24,7 +21,7 @@ class Account::ReconciliationManagerTest < ActiveSupport::TestCase end test "updates existing reconciliation without date change" do - @account.balances.create!(date: Date.current, balance: 1000, cash_balance: 500, currency: @account.currency) + create_balance(account: @account, date: Date.current, balance: 1000, cash_balance: 500) # Existing reconciliation entry existing_entry = @account.entries.create!(name: "Test", amount: 1000, date: Date.current, entryable: Valuation.new(kind: "reconciliation"), currency: @account.currency) @@ -39,8 +36,8 @@ class Account::ReconciliationManagerTest < ActiveSupport::TestCase end test "updates existing reconciliation with date and amount change" do - @account.balances.create!(date: 5.days.ago, balance: 1000, cash_balance: 500, currency: @account.currency) - @account.balances.create!(date: Date.current, balance: 1200, cash_balance: 700, currency: @account.currency) + create_balance(account: @account, date: 5.days.ago, balance: 1000, cash_balance: 500) + create_balance(account: @account, date: Date.current, balance: 1200, cash_balance: 700) # Existing reconciliation entry (5 days ago) existing_entry = @account.entries.create!(name: "Test", amount: 1000, date: 5.days.ago, entryable: Valuation.new(kind: "reconciliation"), currency: @account.currency) @@ -63,12 +60,7 @@ class Account::ReconciliationManagerTest < ActiveSupport::TestCase end test "handles date conflicts" do - @account.balances.create!( - date: Date.current, - balance: 1000, - cash_balance: 1000, - currency: @account.currency - ) + create_balance(account: @account, date: Date.current, balance: 1000, cash_balance: 1000) # Existing reconciliation entry @account.entries.create!( @@ -89,7 +81,7 @@ class Account::ReconciliationManagerTest < ActiveSupport::TestCase end test "dry run does not persist account" do - @account.balances.create!(date: Date.current, balance: 1000, cash_balance: 500, currency: @account.currency) + create_balance(account: @account, date: Date.current, balance: 1000, cash_balance: 500) assert_no_difference "Valuation.count" do @manager.reconcile_balance(balance: 1200, date: Date.current, dry_run: true) diff --git a/test/models/balance/chart_series_builder_test.rb b/test/models/balance/chart_series_builder_test.rb index 80d2467f..fbf5019f 100644 --- a/test/models/balance/chart_series_builder_test.rb +++ b/test/models/balance/chart_series_builder_test.rb @@ -1,6 +1,8 @@ require "test_helper" class Balance::ChartSeriesBuilderTest < ActiveSupport::TestCase + include BalanceTestHelper + setup do end @@ -9,9 +11,9 @@ class Balance::ChartSeriesBuilderTest < ActiveSupport::TestCase account.balances.destroy_all # With gaps - account.balances.create!(date: 3.days.ago.to_date, balance: 1000, currency: "USD") - account.balances.create!(date: 1.day.ago.to_date, balance: 1100, currency: "USD") - account.balances.create!(date: Date.current, balance: 1200, currency: "USD") + create_balance(account: account, date: 3.days.ago.to_date, balance: 1000) + create_balance(account: account, date: 1.day.ago.to_date, balance: 1100) + create_balance(account: account, date: Date.current, balance: 1200) builder = Balance::ChartSeriesBuilder.new( account_ids: [ account.id ], @@ -38,9 +40,9 @@ class Balance::ChartSeriesBuilderTest < ActiveSupport::TestCase account = accounts(:depository) account.balances.destroy_all - account.balances.create!(date: 2.days.ago.to_date, balance: 1000, currency: "USD") - account.balances.create!(date: 1.day.ago.to_date, balance: 1100, currency: "USD") - account.balances.create!(date: Date.current, balance: 1200, currency: "USD") + create_balance(account: account, date: 2.days.ago.to_date, balance: 1000) + create_balance(account: account, date: 1.day.ago.to_date, balance: 1100) + create_balance(account: account, date: Date.current, balance: 1200) builder = Balance::ChartSeriesBuilder.new( account_ids: [ account.id ], @@ -68,13 +70,13 @@ class Balance::ChartSeriesBuilderTest < ActiveSupport::TestCase Balance.destroy_all - asset_account.balances.create!(date: 3.days.ago.to_date, balance: 500, currency: "USD") - asset_account.balances.create!(date: 1.day.ago.to_date, balance: 1000, currency: "USD") - asset_account.balances.create!(date: Date.current, balance: 1000, currency: "USD") + create_balance(account: asset_account, date: 3.days.ago.to_date, balance: 500) + create_balance(account: asset_account, date: 1.day.ago.to_date, balance: 1000) + create_balance(account: asset_account, date: Date.current, balance: 1000) - liability_account.balances.create!(date: 3.days.ago.to_date, balance: 200, currency: "USD") - liability_account.balances.create!(date: 2.days.ago.to_date, balance: 200, currency: "USD") - liability_account.balances.create!(date: Date.current, balance: 100, currency: "USD") + create_balance(account: liability_account, date: 3.days.ago.to_date, balance: 200) + create_balance(account: liability_account, date: 2.days.ago.to_date, balance: 200) + create_balance(account: liability_account, date: Date.current, balance: 100) builder = Balance::ChartSeriesBuilder.new( account_ids: [ asset_account.id, liability_account.id ], @@ -98,8 +100,8 @@ class Balance::ChartSeriesBuilderTest < ActiveSupport::TestCase account = accounts(:credit_card) account.balances.destroy_all - account.balances.create!(date: 1.day.ago.to_date, balance: 1000, currency: "USD") - account.balances.create!(date: Date.current, balance: 500, currency: "USD") + create_balance(account: account, date: 1.day.ago.to_date, balance: 1000) + create_balance(account: account, date: Date.current, balance: 500) builder = Balance::ChartSeriesBuilder.new( account_ids: [ account.id ], diff --git a/test/models/balance/materializer_test.rb b/test/models/balance/materializer_test.rb index 0715ed63..01d34769 100644 --- a/test/models/balance/materializer_test.rb +++ b/test/models/balance/materializer_test.rb @@ -2,6 +2,7 @@ require "test_helper" class Balance::MaterializerTest < ActiveSupport::TestCase include EntriesTestHelper + include BalanceTestHelper setup do @account = families(:empty).accounts.create!( @@ -16,8 +17,6 @@ class Balance::MaterializerTest < ActiveSupport::TestCase test "syncs balances" do Holding::Materializer.any_instance.expects(:materialize_holdings).returns([]).once - @account.expects(:start_date).returns(2.days.ago.to_date) - expected_balances = [ Balance.new( date: 1.day.ago.to_date, @@ -62,29 +61,13 @@ class Balance::MaterializerTest < ActiveSupport::TestCase assert_balance_fields_persisted(expected_balances) end - test "purges stale balances and holdings" do - # Balance before start date is stale - @account.expects(:start_date).returns(2.days.ago.to_date).twice - - stale_balance = Balance.new( - date: 3.days.ago.to_date, - balance: 10000, - cash_balance: 10000, - currency: "USD", - start_cash_balance: 0, - start_non_cash_balance: 0, - cash_inflows: 0, - cash_outflows: 0, - non_cash_inflows: 0, - non_cash_outflows: 0, - net_market_flows: 0, - cash_adjustments: 10000, - non_cash_adjustments: 0, - flows_factor: 1 - ) + test "purges stale balances outside calculated range" do + # Create existing balances that will be stale + stale_old = create_balance(account: @account, date: 5.days.ago.to_date, balance: 5000) + stale_future = create_balance(account: @account, date: 2.days.from_now.to_date, balance: 15000) + # Calculator will return balances for only these dates expected_balances = [ - stale_balance, Balance.new( date: 2.days.ago.to_date, balance: 10000, @@ -136,13 +119,19 @@ class Balance::MaterializerTest < ActiveSupport::TestCase ] Balance::ForwardCalculator.any_instance.expects(:calculate).returns(expected_balances) + Holding::Materializer.any_instance.expects(:materialize_holdings).returns([]).once - assert_difference "@account.balances.count", 3 do + # Should end up with 3 balances (stale ones deleted, new ones created) + assert_difference "@account.balances.count", 1 do Balance::Materializer.new(@account, strategy: :forward).materialize_balances end - # Only non-stale balances should be persisted and checked - assert_balance_fields_persisted(expected_balances.reject { |b| b.date < 2.days.ago.to_date }) + # Verify stale balances were deleted + assert_nil @account.balances.find_by(id: stale_old.id) + assert_nil @account.balances.find_by(id: stale_future.id) + + # Verify expected balances were persisted + assert_balance_fields_persisted(expected_balances) end private diff --git a/test/support/balance_test_helper.rb b/test/support/balance_test_helper.rb new file mode 100644 index 00000000..e9eed0d7 --- /dev/null +++ b/test/support/balance_test_helper.rb @@ -0,0 +1,72 @@ +module BalanceTestHelper + def create_balance(account:, date:, balance:, cash_balance: nil, **attributes) + # If cash_balance is not provided, default to entire balance being cash + cash_balance ||= balance + + # Calculate non-cash balance + non_cash_balance = balance - cash_balance + + # Set default component values that will generate the desired end_balance + # flows_factor should be 1 for assets, -1 for liabilities + flows_factor = account.classification == "liability" ? -1 : 1 + + defaults = { + date: date, + balance: balance, + cash_balance: cash_balance, + currency: account.currency, + start_cash_balance: cash_balance, + start_non_cash_balance: non_cash_balance, + cash_inflows: 0, + cash_outflows: 0, + non_cash_inflows: 0, + non_cash_outflows: 0, + net_market_flows: 0, + cash_adjustments: 0, + non_cash_adjustments: 0, + flows_factor: flows_factor + } + + account.balances.create!(defaults.merge(attributes)) + end + + def create_balance_with_flows(account:, date:, start_balance:, end_balance:, + cash_portion: 1.0, cash_flow: 0, non_cash_flow: 0, + market_flow: 0, **attributes) + # Calculate cash and non-cash portions + start_cash = start_balance * cash_portion + start_non_cash = start_balance * (1 - cash_portion) + + # Calculate adjustments needed to reach end_balance + expected_end_cash = start_cash + cash_flow + expected_end_non_cash = start_non_cash + non_cash_flow + market_flow + expected_total = expected_end_cash + expected_end_non_cash + + # Calculate adjustments if end_balance doesn't match expected + total_adjustment = end_balance - expected_total + cash_adjustment = cash_portion * total_adjustment + non_cash_adjustment = (1 - cash_portion) * total_adjustment + + # flows_factor should be 1 for assets, -1 for liabilities + flows_factor = account.classification == "liability" ? -1 : 1 + + defaults = { + date: date, + balance: end_balance, + cash_balance: expected_end_cash + cash_adjustment, + currency: account.currency, + start_cash_balance: start_cash, + start_non_cash_balance: start_non_cash, + cash_inflows: cash_flow > 0 ? cash_flow : 0, + cash_outflows: cash_flow < 0 ? -cash_flow : 0, + non_cash_inflows: non_cash_flow > 0 ? non_cash_flow : 0, + non_cash_outflows: non_cash_flow < 0 ? -non_cash_flow : 0, + net_market_flows: market_flow, + cash_adjustments: cash_adjustment, + non_cash_adjustments: non_cash_adjustment, + flows_factor: flows_factor + } + + account.balances.create!(defaults.merge(attributes)) + end +end