From 09dae9255aa9c6c8dc78d07b2822f83c935d2e95 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Fri, 16 May 2025 13:58:31 -0400 Subject: [PATCH] Add back price and exchange rate syncs to account syncs --- app/models/account.rb | 2 +- app/models/account/convertible.rb | 28 ----- app/models/account/market_data_syncer.rb | 82 ++++++++++++++ app/models/account/syncer.rb | 15 +++ app/models/balance/syncer.rb | 2 - test/models/account/convertible_test.rb | 52 --------- .../models/account/market_data_syncer_test.rb | 107 ++++++++++++++++++ 7 files changed, 205 insertions(+), 83 deletions(-) delete mode 100644 app/models/account/convertible.rb create mode 100644 app/models/account/market_data_syncer.rb delete mode 100644 test/models/account/convertible_test.rb create mode 100644 test/models/account/market_data_syncer_test.rb diff --git a/app/models/account.rb b/app/models/account.rb index 8c74b83e..13734071 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -1,5 +1,5 @@ class Account < ApplicationRecord - include Syncable, Monetizable, Chartable, Linkable, Convertible, Enrichable + include Syncable, Monetizable, Chartable, Linkable, Enrichable validates :name, :balance, :currency, presence: true diff --git a/app/models/account/convertible.rb b/app/models/account/convertible.rb deleted file mode 100644 index 07e32bd5..00000000 --- a/app/models/account/convertible.rb +++ /dev/null @@ -1,28 +0,0 @@ -module Account::Convertible - extend ActiveSupport::Concern - - def sync_required_exchange_rates - unless requires_exchange_rates? - Rails.logger.info("No exchange rate sync needed for account #{id}") - return - end - - affected_row_count = ExchangeRate.sync_provider_rates( - from: currency, - to: target_currency, - start_date: start_date, - end_date: Date.current - ) - - Rails.logger.info("Synced #{affected_row_count} exchange rates for account #{id}") - end - - private - def target_currency - family.currency - end - - def requires_exchange_rates? - currency != target_currency - end -end diff --git a/app/models/account/market_data_syncer.rb b/app/models/account/market_data_syncer.rb new file mode 100644 index 00000000..b223d229 --- /dev/null +++ b/app/models/account/market_data_syncer.rb @@ -0,0 +1,82 @@ +class Account::MarketDataSyncer + attr_reader :account + + def initialize(account) + @account = account + end + + def sync_market_data + sync_exchange_rates + sync_security_prices + end + + private + def sync_exchange_rates + return unless needs_exchange_rates? + return unless ExchangeRate.provider + + pair_dates = {} + + # 1. ENTRY-BASED PAIRS – currencies that differ from the account currency + account.entries + .where.not(currency: account.currency) + .group(:currency) + .minimum(:date) + .each do |source_currency, date| + key = [ source_currency, account.currency ] + pair_dates[key] = [ pair_dates[key], date ].compact.min + end + + # 2. ACCOUNT-BASED PAIR – convert the account currency to the family currency (if different) + if foreign_account? + key = [ account.currency, account.family.currency ] + pair_dates[key] = [ pair_dates[key], account.start_date ].compact.min + end + + pair_dates.each do |(source, target), start_date| + ExchangeRate.sync_provider_rates( + from: source, + to: target, + start_date: start_date, + end_date: Date.current + ) + end + end + + def sync_security_prices + return unless Security.provider + + account_securities = account.trades.map(&:security).uniq + + return if account_securities.empty? + + account_securities.each do |security| + security.sync_provider_prices( + start_date: first_required_price_date(security), + end_date: Date.current + ) + + security.sync_provider_details + end + end + + # Calculates the first date we require a price for the given security scoped to this account + def first_required_price_date(security) + account.trades.with_entry + .where(security: security) + .where(entries: { account_id: account.id }) + .minimum("entries.date") + end + + def needs_exchange_rates? + has_multi_currency_entries? || foreign_account? + end + + def has_multi_currency_entries? + account.entries.where.not(currency: account.currency).exists? + end + + def foreign_account? + account.currency != account.family.currency + end +end diff --git a/app/models/account/syncer.rb b/app/models/account/syncer.rb index 7e5fabcd..de63f5e8 100644 --- a/app/models/account/syncer.rb +++ b/app/models/account/syncer.rb @@ -7,6 +7,7 @@ class Account::Syncer def perform_sync(sync) Rails.logger.info("Processing balances (#{account.linked? ? 'reverse' : 'forward'})") + sync_market_data sync_balances end @@ -19,4 +20,18 @@ class Account::Syncer strategy = account.linked? ? :reverse : :forward Balance::Syncer.new(account, strategy: strategy).sync_balances end + + # Syncs all the exchange rates + security prices this account needs to display historical chart data + # + # This is a *supplemental* sync. The daily market data sync should have already populated + # a majority or all of this data, so this is often a no-op. + # + # We rescue errors here because if this operation fails, we don't want to fail the entire sync since + # we have reasonable fallbacks for missing market data. + def sync_market_data + Account::MarketDataSyncer.new(account).sync_market_data + rescue => e + Rails.logger.error("Error syncing market data for account #{account.id}: #{e.message}") + Sentry.capture_exception(e) + end end diff --git a/app/models/balance/syncer.rb b/app/models/balance/syncer.rb index 362b87aa..890bb5f9 100644 --- a/app/models/balance/syncer.rb +++ b/app/models/balance/syncer.rb @@ -19,8 +19,6 @@ class Balance::Syncer if strategy == :forward update_account_info end - - account.sync_required_exchange_rates end end diff --git a/test/models/account/convertible_test.rb b/test/models/account/convertible_test.rb deleted file mode 100644 index 246ee6a9..00000000 --- a/test/models/account/convertible_test.rb +++ /dev/null @@ -1,52 +0,0 @@ -require "test_helper" -require "ostruct" - -class Account::ConvertibleTest < ActiveSupport::TestCase - include EntriesTestHelper, ProviderTestHelper - - setup do - @family = families(:empty) - @family.update!(currency: "USD") - - # Foreign account (currency is not in the family's primary currency, so it will require exchange rates for net worth rollups) - @account = @family.accounts.create!(name: "Test Account", currency: "EUR", balance: 10000, accountable: Depository.new) - - @provider = mock - ExchangeRate.stubs(:provider).returns(@provider) - end - - test "syncs required exchange rates for an account" do - create_valuation(account: @account, date: 1.day.ago.to_date, amount: 9500, currency: "EUR") - - # Since we had a valuation 1 day ago, this account starts 2 days ago and needs daily exchange rates looking forward - assert_equal 2.days.ago.to_date, @account.start_date - - ExchangeRate.delete_all - - provider_response = provider_success_response( - [ - OpenStruct.new(from: "EUR", to: "USD", date: 2.days.ago.to_date, rate: 1.1), - OpenStruct.new(from: "EUR", to: "USD", date: 1.day.ago.to_date, rate: 1.2), - OpenStruct.new(from: "EUR", to: "USD", date: Date.current, rate: 1.3) - ] - ) - - @provider.expects(:fetch_exchange_rates) - .with(from: "EUR", to: "USD", start_date: 2.days.ago.to_date - 5.days, end_date: Date.current) - .returns(provider_response) - - assert_difference "ExchangeRate.count", 3 do - @account.sync_required_exchange_rates - end - end - - test "does not sync rates for a domestic account" do - @account.update!(currency: "USD") - - @provider.expects(:fetch_exchange_rates).never - - assert_no_difference "ExchangeRate.count" do - @account.sync_required_exchange_rates - end - end -end diff --git a/test/models/account/market_data_syncer_test.rb b/test/models/account/market_data_syncer_test.rb new file mode 100644 index 00000000..596798f5 --- /dev/null +++ b/test/models/account/market_data_syncer_test.rb @@ -0,0 +1,107 @@ +require "test_helper" +require "ostruct" + +class Account::MarketDataSyncerTest < ActiveSupport::TestCase + include ProviderTestHelper + + PROVIDER_BUFFER = 5.days + + setup do + # Ensure a clean slate for deterministic assertions + Security::Price.delete_all + ExchangeRate.delete_all + Trade.delete_all + Holding.delete_all + Security.delete_all + Entry.delete_all + + @provider = mock("provider") + Provider::Registry.any_instance + .stubs(:get_provider) + .with(:synth) + .returns(@provider) + end + + test "syncs required exchange rates for a foreign-currency account" do + family = Family.create!(name: "Smith", currency: "USD") + + account = family.accounts.create!( + name: "Chequing", + currency: "CAD", + balance: 100, + accountable: Depository.new + ) + + # Seed a rate for the first required day so that the syncer only needs the next day forward + existing_date = account.start_date + ExchangeRate.create!(from_currency: "CAD", to_currency: "USD", date: existing_date, rate: 2.0) + + expected_start_date = (existing_date + 1.day) - PROVIDER_BUFFER + end_date = Date.current.in_time_zone("America/New_York").to_date + + @provider.expects(:fetch_exchange_rates) + .with(from: "CAD", + to: "USD", + start_date: expected_start_date, + end_date: end_date) + .returns(provider_success_response([ + OpenStruct.new(from: "CAD", to: "USD", date: existing_date, rate: 1.5) + ])) + + before = ExchangeRate.count + Account::MarketDataSyncer.new(account).sync_market_data + after = ExchangeRate.count + + assert_operator after, :>, before, "Should insert at least one new exchange-rate row" + end + + test "syncs security prices for securities traded by the account" do + family = Family.create!(name: "Smith", currency: "USD") + + account = family.accounts.create!( + name: "Brokerage", + currency: "USD", + balance: 0, + accountable: Investment.new + ) + + security = Security.create!(ticker: "AAPL", exchange_operating_mic: "XNAS") + + trade_date = 10.days.ago.to_date + trade = Trade.new(security: security, qty: 1, price: 100, currency: "USD") + + account.entries.create!( + name: "Buy AAPL", + date: trade_date, + amount: 100, + currency: "USD", + entryable: trade + ) + + expected_start_date = trade_date - PROVIDER_BUFFER + end_date = Date.current.in_time_zone("America/New_York").to_date + + @provider.expects(:fetch_security_prices) + .with(symbol: security.ticker, + exchange_operating_mic: security.exchange_operating_mic, + start_date: expected_start_date, + end_date: end_date) + .returns(provider_success_response([ + OpenStruct.new(security: security, + date: trade_date, + price: 100, + currency: "USD") + ])) + + @provider.stubs(:fetch_security_info) + .with(symbol: security.ticker, exchange_operating_mic: security.exchange_operating_mic) + .returns(provider_success_response(OpenStruct.new(name: "Apple", logo_url: "logo"))) + + # Ignore exchange-rate calls for this test + @provider.stubs(:fetch_exchange_rates).returns(provider_success_response([])) + + Account::MarketDataSyncer.new(account).sync_market_data + + assert_equal 1, Security::Price.where(security: security, date: trade_date).count + end +end