Fix price lookup logic for holdings sync

app/controllers/concerns/auto_sync.rb

@@ -13,6 +13,7 @@ module AutoSync
 
     def family_needs_auto_sync?
       return false unless Current.family.present?
+      return false unless Current.family.accounts.active.any?
 
       (Current.family.last_synced_at&.to_date || 1.day.ago) < Date.current
     end

app/models/account/balance/base_calculator.rb

@@ -18,7 +18,7 @@ class Account::Balance::BaseCalculator
 
     def build_balance(date, cash_balance, holdings_value)
       Account::Balance.new(
-        account: account,
+        account_id: account.id,
         date: date,
         balance: holdings_value + cash_balance,
         cash_balance: cash_balance,

app/models/account/balance/syncer.rb

@@ -16,7 +16,7 @@ class Account::Balance::Syncer
 
       purge_stale_balances
 
-      unless strategy == :reverse
+      if strategy == :forward
         update_account_info
       end
     end

app/models/account/holding/base_calculator.rb

@@ -7,8 +7,8 @@ class Account::Holding::BaseCalculator
 
   def calculate
     Rails.logger.tagged(self.class.name) do
-      calculate_holdings
-      Account::Holding.gapfill(@holdings)
+      holdings = calculate_holdings
+      Account::Holding.gapfill(holdings)
     end
   end
 
@@ -41,18 +41,22 @@ class Account::Holding::BaseCalculator
     end
 
     def build_holdings(portfolio, date)
-      Rails.logger.info "Generating holdings for #{portfolio.size} securities on #{date}"
-
       portfolio.map do |security_id, qty|
         price = portfolio_cache.get_price(security_id, date)
 
-        account.holdings.build(
+        if price.nil?
+          Rails.logger.warn "No price found for security #{security_id} on #{date}"
+          next
+        end
+
+        Account::Holding.new(
+          account_id: account.id,
           security_id: security_id,
           date: date,
           qty: qty,
-          price: price,
-          currency: account.currency,
-          amount: qty * price
+          price: price.price,
+          currency: price.currency,
+          amount: qty * price.price
         )
       end.compact
     end

app/models/account/holding/forward_calculator.rb

@@ -1,15 +1,21 @@
 class Account::Holding::ForwardCalculator < Account::Holding::BaseCalculator
   private
+    def portfolio_cache
+      @portfolio_cache ||= Account::Holding::PortfolioCache.new(account)
+    end
+
     def calculate_holdings
       current_portfolio = generate_starting_portfolio
       next_portfolio = {}
-      @holdings = []
+      holdings = []
 
       account.start_date.upto(Date.current).each do |date|
         trades = portfolio_cache.get_trades(date: date)
         next_portfolio = transform_portfolio(current_portfolio, trades, direction: :forward)
-        @holdings += build_holdings(next_portfolio, date)
+        holdings += build_holdings(next_portfolio, date)
         current_portfolio = next_portfolio
       end
+
+      holdings
     end
 end

app/models/account/holding/gapfillable.rb

@@ -19,7 +19,8 @@ module Account::Holding::Gapfillable
             previous_holding = holding
           else
             # Create a new holding based on the previous day's data
-            filled_holdings << account.holdings.build(
+            filled_holdings << Account::Holding.new(
+              account: previous_holding.account,
               security: previous_holding.security,
               date: date,
               qty: previous_holding.qty,

app/models/account/holding/portfolio_cache.rb

@@ -1,5 +1,5 @@
 class Account::Holding::PortfolioCache
-  attr_reader :account
+  attr_reader :account, :use_holdings
 
   class SecurityNotFound < StandardError
     def initialize(security_id, account_id)
@@ -7,8 +7,9 @@ class Account::Holding::PortfolioCache
     end
   end
 
-  def initialize(account)
+  def initialize(account, use_holdings: false)
     @account = account
+    @use_holdings = use_holdings
     load_prices
   end
 
@@ -22,22 +23,22 @@ class Account::Holding::PortfolioCache
 
   def get_price(security_id, date)
     security = @security_cache[security_id]
-
     raise SecurityNotFound.new(security_id, account.id) unless security
 
-    price = security[:prices].find { |p| p.date == date }
+    price = security[:prices].select { |p| p.price.date == date }.min_by(&:priority)&.price
 
-    # We prefer to use prices from our data provider.  But if the provider doesn't have an EOD price
-    # for this security, we search through the account's trades and use the "spot" price at the time of
-    # the most recent trade for that day's holding.  This is not as accurate, but it allows users to define
-    # what we call "offline" securities (which is essential given we cannot get prices for all securities globally)
-    if price.blank?
-      converted_price = most_recent_trade_price(security_id, date)
-    else
-      converted_price = Money.new(price.price, price.currency).exchange_to(account.currency, fallback_rate: 1).amount
-    end
+    return nil unless price
 
-    converted_price
+    price_money = Money.new(price.price, price.currency)
+
+    converted_amount = price_money.exchange_to(account.currency, fallback_rate: 1).amount
+
+    Security::Price.new(
+      security_id: security_id,
+      date: price.date,
+      price: converted_amount,
+      currency: price.currency
+    )
   end
 
   def get_securities
@@ -45,47 +46,86 @@ class Account::Holding::PortfolioCache
   end
 
   private
+    PriceWithPriority = Data.define(:price, :priority)
+
     def trades
       @trades ||= account.entries.includes(entryable: :security).account_trades.chronological.to_a
     end
 
-    def most_recent_trade_price(security_id, date)
-      first_trade = trades.select { |t| t.entryable.security_id == security_id }.min_by(&:date)
-      most_recent_trade = trades.select { |t| t.entryable.security_id == security_id && t.date <= date }.max_by(&:date)
-
-      if most_recent_trade
-        most_recent_trade.entryable.price
-      else
-        first_trade.entryable.price
-      end
+    def holdings
+      @holdings ||= account.holdings.chronological.to_a
     end
 
+    def collect_unique_securities
+      unique_securities_from_trades = trades.map(&:entryable).map(&:security).uniq
+
+      return unique_securities_from_trades unless use_holdings
+
+      unique_securities_from_holdings = holdings.map(&:security).uniq
+
+      (unique_securities_from_trades + unique_securities_from_holdings).uniq
+    end
+
+    # Loads all known prices for all securities in the account with priority based on source:
+    # 1 - DB or provider prices
+    # 2 - Trade prices
+    # 3 - Holding prices
     def load_prices
       @security_cache = {}
-
-      # Get securities from trades and current holdings.  We need them from holdings
-      # because for linked accounts, our provider gives us holding data that may not
-      # exist solely in the trades history.
-      securities = trades.map(&:entryable).map(&:security).uniq
-      securities += account.holdings.where(date: Date.current).map(&:security)
-      securities.uniq!
+      securities = collect_unique_securities
 
       Rails.logger.info "Preloading #{securities.size} securities for account #{account.id}"
 
       securities.each do |security|
         Rails.logger.info "Loading security: ID=#{security.id} Ticker=#{security.ticker}"
 
-        fetched_prices = Security::Price.find_prices(
+        # Highest priority prices
+        db_or_provider_prices = Security::Price.find_prices(
           security: security,
           start_date: account.start_date,
           end_date: Date.current
-        )
+        ).map do |price|
+          PriceWithPriority.new(
+            price: price,
+            priority: 1
+          )
+        end
 
-        Rails.logger.info "Found #{fetched_prices.size} prices for security #{security.id}"
+        # Medium priority prices from trades
+        trade_prices = trades
+          .select { |t| t.entryable.security_id == security.id }
+          .map do |trade|
+            PriceWithPriority.new(
+              price: Security::Price.new(
+                security: security,
+                price: trade.entryable.price,
+                currency: trade.entryable.currency,
+                date: trade.date
+              ),
+              priority: 2
+            )
+          end
+
+        # Low priority prices from holdings (if applicable)
+        holding_prices = if use_holdings
+          holdings.select { |h| h.security_id == security.id }.map do |holding|
+            PriceWithPriority.new(
+              price: Security::Price.new(
+                security: security,
+                price: holding.price,
+                currency: holding.currency,
+                date: holding.date
+              ),
+              priority: 3
+            )
+          end
+        else
+          []
+        end
 
         @security_cache[security.id] = {
           security: security,
-          prices: fetched_prices
+          prices: db_or_provider_prices + trade_prices + holding_prices
         }
       end
     end

app/models/account/holding/reverse_calculator.rb

@@ -1,17 +1,26 @@
 class Account::Holding::ReverseCalculator < Account::Holding::BaseCalculator
   private
+    # Reverse calculators will use the existing holdings as a source of security ids and prices
+    # since it is common for a provider to supply "current day" holdings but not all the historical
+    # trades that make up those holdings.
+    def portfolio_cache
+      @portfolio_cache ||= Account::Holding::PortfolioCache.new(account, use_holdings: true)
+    end
+
     def calculate_holdings
       current_portfolio = generate_starting_portfolio
       previous_portfolio = {}
 
-      @holdings = []
+      holdings = []
 
-      Date.current.downto(account.start_date).map do |date|
+      Date.current.downto(account.start_date).each do |date|
         today_trades = portfolio_cache.get_trades(date: date)
         previous_portfolio = transform_portfolio(current_portfolio, today_trades, direction: :reverse)
-        @holdings += build_holdings(current_portfolio, date)
+        holdings += build_holdings(current_portfolio, date)
         current_portfolio = previous_portfolio
       end
+
+      holdings
     end
 
     # Since this is a reverse sync, we start with today's holdings

app/models/account/holding/syncer.rb

@@ -2,15 +2,15 @@ class Account::Holding::Syncer
   def initialize(account, strategy:)
     @account = account
     @strategy = strategy
-    @securities_cache = {}
   end
 
   def sync_holdings
     calculate_holdings
+
     Rails.logger.info("Persisting #{@holdings.size} holdings")
     persist_holdings
 
-    unless strategy == :reverse
+    if strategy == :forward
       purge_stale_holdings
     end
 
@@ -18,7 +18,7 @@ class Account::Holding::Syncer
   end
 
   private
-    attr_reader :account, :securities_cache, :strategy
+    attr_reader :account, :strategy
 
     def calculate_holdings
       @holdings = calculator.calculate
@@ -30,7 +30,7 @@ class Account::Holding::Syncer
       account.holdings.upsert_all(
         @holdings.map { |h| h.attributes
                .slice("date", "currency", "qty", "price", "amount", "security_id")
-               .merge("updated_at" => current_time) },
+               .merge("account_id" => account.id, "updated_at" => current_time) },
         unique_by: %i[account_id security_id date currency]
       )
     end

config/environments/development.rb

@@ -84,4 +84,7 @@ Rails.application.configure do
 
   # Allow connection from any host in development
   config.hosts = nil
+
+  # Set log level
+  config.log_level = :info
 end

test/models/account/holding/forward_calculator_test.rb

@@ -98,7 +98,6 @@ class Account::Holding::ForwardCalculatorTest < ActiveSupport::TestCase
     create_trade(offline_security, qty: 1, date: 1.day.ago.to_date, price: 100, account: @account)
 
     expected = [
-      Account::Holding.new(security: offline_security, date: 4.days.ago.to_date, qty: 0, price: 90, amount: 0),
       Account::Holding.new(security: offline_security, date: 3.days.ago.to_date, qty: 1, price: 90, amount: 90),
       Account::Holding.new(security: offline_security, date: 2.days.ago.to_date, qty: 1, price: 90, amount: 90),
       Account::Holding.new(security: offline_security, date: 1.day.ago.to_date, qty: 2, price: 100, amount: 200),

test/models/account/holding/portfolio_cache_test.rb

@@ -0,0 +1,63 @@
+require "test_helper"
+
+class Account::Holding::PortfolioCacheTest < ActiveSupport::TestCase
+  include Account::EntriesTestHelper
+
+  setup do
+    # Prices, highest to lowest priority
+    @db_price = 210
+    @provider_price = 220
+    @trade_price = 200
+    @holding_price = 250
+
+    @account = families(:empty).accounts.create!(name: "Test Brokerage", balance: 10000, currency: "USD", accountable: Investment.new)
+    @test_security = Security.create!(name: "Test Security", ticker: "TEST")
+
+    @trade = create_trade(@test_security, account: @account, qty: 1, date: Date.current, price: @trade_price)
+    @holding = Account::Holding.create!(security: @test_security, account: @account, date: Date.current, qty: 1, price: @holding_price, amount: @holding_price, currency: "USD")
+    Security::Price.create!(security: @test_security, date: Date.current, price: @db_price)
+  end
+
+  test "gets price from DB if available" do
+    cache = Account::Holding::PortfolioCache.new(@account)
+
+    assert_equal @db_price, cache.get_price(@test_security.id, Date.current).price
+  end
+
+  test "if no price in DB, try fetching from provider" do
+    Security::Price.destroy_all
+    Security::Price.expects(:find_prices)
+                   .with(security: @test_security, start_date: @account.start_date, end_date: Date.current)
+                   .returns([
+                     Security::Price.new(security: @test_security, date: Date.current, price: @provider_price, currency: "USD")
+                   ])
+
+    cache = Account::Holding::PortfolioCache.new(@account)
+
+    assert_equal @provider_price, cache.get_price(@test_security.id, Date.current).price
+  end
+
+  test "if no price from db or provider, try getting the price from trades" do
+    Security::Price.destroy_all # No DB prices
+    Security::Price.expects(:find_prices)
+                   .with(security: @test_security, start_date: @account.start_date, end_date: Date.current)
+                   .returns([]) # No provider prices
+
+    cache = Account::Holding::PortfolioCache.new(@account)
+
+    assert_equal @trade_price, cache.get_price(@test_security.id, Date.current).price
+  end
+
+  test "if no price from db, provider, or trades, search holdings" do
+    Security::Price.destroy_all # No DB prices
+    Security::Price.expects(:find_prices)
+                   .with(security: @test_security, start_date: @account.start_date, end_date: Date.current)
+                   .returns([]) # No provider prices
+
+    @account.entries.destroy_all # No prices from trades
+
+    cache = Account::Holding::PortfolioCache.new(@account, use_holdings: true)
+
+    assert_equal @holding_price, cache.get_price(@test_security.id, Date.current).price
+  end
+end