From 8db95623cf9ca54f47126944c7a28660ae358384 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Thu, 26 Jun 2025 16:57:17 -0400 Subject: [PATCH] Handle holding quantity generation for reverse syncs correctly when not all holdings are generated for current day (#2417) * Handle reverse calculator starting portfolio generation correctly * Fix current_holdings to handle different dates and hide zero quantities - Use DISTINCT ON to get most recent holding per security instead of assuming same date - Filter out zero quantity holdings from UI display - Maintain cash display regardless of zero balance - Use single efficient query with proper Rails syntax * Continue to process holdings even if one is not resolvable * Lint fixes --- app/models/account.rb | 9 +- app/models/holding/materializer.rb | 3 +- app/models/holding/portfolio_snapshot.rb | 32 ++++ app/models/holding/reverse_calculator.rb | 26 +-- .../investments/holdings_processor.rb | 19 +- .../models/holding/portfolio_snapshot_test.rb | 50 ++++++ .../models/holding/reverse_calculator_test.rb | 19 +- .../investments/holdings_processor_test.rb | 162 +++++++++++++++++- 8 files changed, 281 insertions(+), 39 deletions(-) create mode 100644 app/models/holding/portfolio_snapshot.rb create mode 100644 test/models/holding/portfolio_snapshot_test.rb diff --git a/app/models/account.rb b/app/models/account.rb index b1e2c80c..a87c9fa1 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -82,7 +82,14 @@ class Account < ApplicationRecord end def current_holdings - holdings.where(currency: currency, date: holdings.maximum(:date)).order(amount: :desc) + holdings.where(currency: currency) + .where.not(qty: 0) + .where( + id: holdings.select("DISTINCT ON (security_id) id") + .where(currency: currency) + .order(:security_id, date: :desc) + ) + .order(amount: :desc) end def update_with_sync!(attributes) diff --git a/app/models/holding/materializer.rb b/app/models/holding/materializer.rb index e4931128..6c1e8db1 100644 --- a/app/models/holding/materializer.rb +++ b/app/models/holding/materializer.rb @@ -52,7 +52,8 @@ class Holding::Materializer def calculator if strategy == :reverse - Holding::ReverseCalculator.new(account) + portfolio_snapshot = Holding::PortfolioSnapshot.new(account) + Holding::ReverseCalculator.new(account, portfolio_snapshot: portfolio_snapshot) else Holding::ForwardCalculator.new(account) end diff --git a/app/models/holding/portfolio_snapshot.rb b/app/models/holding/portfolio_snapshot.rb new file mode 100644 index 00000000..0c512873 --- /dev/null +++ b/app/models/holding/portfolio_snapshot.rb @@ -0,0 +1,32 @@ +# Captures the most recent holding quantities for each security in an account's portfolio. +# Returns a portfolio hash compatible with the reverse calculator's format. +class Holding::PortfolioSnapshot + attr_reader :account + + def initialize(account) + @account = account + end + + # Returns a hash of {security_id => qty} representing today's starting portfolio. + # Includes all securities from trades (with 0 qty if no holdings exist). + def to_h + @portfolio ||= build_portfolio + end + + private + def build_portfolio + # Start with all securities from trades initialized to 0 + portfolio = account.trades + .pluck(:security_id) + .uniq + .each_with_object({}) { |security_id, hash| hash[security_id] = 0 } + + # Get the most recent holding for each security and update quantities + account.holdings + .select("DISTINCT ON (security_id) security_id, qty") + .order(:security_id, date: :desc) + .each { |holding| portfolio[holding.security_id] = holding.qty } + + portfolio + end +end diff --git a/app/models/holding/reverse_calculator.rb b/app/models/holding/reverse_calculator.rb index f52184d7..656fc0d9 100644 --- a/app/models/holding/reverse_calculator.rb +++ b/app/models/holding/reverse_calculator.rb @@ -1,8 +1,9 @@ class Holding::ReverseCalculator - attr_reader :account + attr_reader :account, :portfolio_snapshot - def initialize(account) + def initialize(account, portfolio_snapshot:) @account = account + @portfolio_snapshot = portfolio_snapshot end def calculate @@ -21,7 +22,8 @@ class Holding::ReverseCalculator end def calculate_holdings - current_portfolio = generate_starting_portfolio + # Start with the portfolio snapshot passed in from the materializer + current_portfolio = portfolio_snapshot.to_h previous_portfolio = {} holdings = [] @@ -38,24 +40,6 @@ class Holding::ReverseCalculator holdings end - def empty_portfolio - securities = portfolio_cache.get_securities - securities.each_with_object({}) { |security, hash| hash[security.id] = 0 } - end - - # Since this is a reverse sync, we start with today's holdings - def generate_starting_portfolio - holding_quantities = empty_portfolio - - todays_holdings = account.holdings.where(date: Date.current) - - todays_holdings.each do |holding| - holding_quantities[holding.security_id] = holding.qty - end - - holding_quantities - end - def transform_portfolio(previous_portfolio, trade_entries, direction: :forward) new_quantities = previous_portfolio.dup diff --git a/app/models/plaid_account/investments/holdings_processor.rb b/app/models/plaid_account/investments/holdings_processor.rb index cfaaa5b3..8dac6bae 100644 --- a/app/models/plaid_account/investments/holdings_processor.rb +++ b/app/models/plaid_account/investments/holdings_processor.rb @@ -8,11 +8,14 @@ class PlaidAccount::Investments::HoldingsProcessor holdings.each do |plaid_holding| resolved_security_result = security_resolver.resolve(plaid_security_id: plaid_holding["security_id"]) - return unless resolved_security_result.security.present? + next unless resolved_security_result.security.present? + + security = resolved_security_result.security + holding_date = plaid_holding["institution_price_as_of"] || Date.current holding = account.holdings.find_or_initialize_by( - security: resolved_security_result.security, - date: Date.current, + security: security, + date: holding_date, currency: plaid_holding["iso_currency_code"] ) @@ -22,7 +25,15 @@ class PlaidAccount::Investments::HoldingsProcessor amount: plaid_holding["quantity"] * plaid_holding["institution_price"] ) - holding.save! + ActiveRecord::Base.transaction do + holding.save! + + # Delete all holdings for this security after the institution price date + account.holdings + .where(security: security) + .where("date > ?", holding_date) + .destroy_all + end end end diff --git a/test/models/holding/portfolio_snapshot_test.rb b/test/models/holding/portfolio_snapshot_test.rb new file mode 100644 index 00000000..624e6086 --- /dev/null +++ b/test/models/holding/portfolio_snapshot_test.rb @@ -0,0 +1,50 @@ +require "test_helper" + +class Holding::PortfolioSnapshotTest < ActiveSupport::TestCase + include EntriesTestHelper + setup do + @account = accounts(:investment) + @aapl = securities(:aapl) + @msft = securities(:msft) + end + + test "captures the most recent holding quantities for each security" do + # Clear any existing data + @account.holdings.destroy_all + @account.entries.destroy_all + + # Create some trades to establish which securities are in the portfolio + create_trade(@aapl, account: @account, qty: 10, price: 100, date: 5.days.ago) + create_trade(@msft, account: @account, qty: 30, price: 200, date: 5.days.ago) + + # Create holdings for AAPL at different dates + @account.holdings.create!(security: @aapl, date: 3.days.ago, qty: 10, price: 100, amount: 1000, currency: "USD") + @account.holdings.create!(security: @aapl, date: 1.day.ago, qty: 20, price: 150, amount: 3000, currency: "USD") + + # Create holdings for MSFT at different dates + @account.holdings.create!(security: @msft, date: 5.days.ago, qty: 30, price: 200, amount: 6000, currency: "USD") + @account.holdings.create!(security: @msft, date: 2.days.ago, qty: 40, price: 250, amount: 10000, currency: "USD") + + snapshot = Holding::PortfolioSnapshot.new(@account) + portfolio = snapshot.to_h + + assert_equal 2, portfolio.size + assert_equal 20, portfolio[@aapl.id] + assert_equal 40, portfolio[@msft.id] + end + + test "includes securities from trades with zero quantities when no holdings exist" do + # Clear any existing data + @account.holdings.destroy_all + @account.entries.destroy_all + + # Create a trade to establish AAPL is in the portfolio + create_trade(@aapl, account: @account, qty: 10, price: 100, date: 5.days.ago) + + snapshot = Holding::PortfolioSnapshot.new(@account) + portfolio = snapshot.to_h + + assert_equal 1, portfolio.size + assert_equal 0, portfolio[@aapl.id] + end +end diff --git a/test/models/holding/reverse_calculator_test.rb b/test/models/holding/reverse_calculator_test.rb index 785d7b94..f2cec5fd 100644 --- a/test/models/holding/reverse_calculator_test.rb +++ b/test/models/holding/reverse_calculator_test.rb @@ -14,7 +14,8 @@ class Holding::ReverseCalculatorTest < ActiveSupport::TestCase end test "no holdings" do - calculated = Holding::ReverseCalculator.new(@account).calculate + empty_snapshot = OpenStruct.new(to_h: {}) + calculated = Holding::ReverseCalculator.new(@account, portfolio_snapshot: empty_snapshot).calculate assert_equal [], calculated end @@ -36,7 +37,9 @@ class Holding::ReverseCalculatorTest < ActiveSupport::TestCase create_trade(voo, qty: 10, date: "2025-01-03", price: 500, account: @account) expected = [ [ "2025-01-02", 0 ], [ "2025-01-03", 5000 ], [ "2025-01-04", 5000 ] ] - calculated = Holding::ReverseCalculator.new(@account).calculate + # Mock snapshot with the holdings we created + snapshot = OpenStruct.new(to_h: { voo.id => 10 }) + calculated = Holding::ReverseCalculator.new(@account, portfolio_snapshot: snapshot).calculate assert_equal expected, calculated.sort_by(&:date).map { |b| [ b.date.to_s, b.amount ] } end @@ -50,7 +53,9 @@ class Holding::ReverseCalculatorTest < ActiveSupport::TestCase create_trade(voo, qty: -10, date: Date.current, price: 470, account: @account) - calculated = Holding::ReverseCalculator.new(@account).calculate + # Mock empty portfolio since no current day holdings + snapshot = OpenStruct.new(to_h: { voo.id => 0 }) + calculated = Holding::ReverseCalculator.new(@account, portfolio_snapshot: snapshot).calculate assert_equal 2, calculated.length end @@ -96,7 +101,9 @@ class Holding::ReverseCalculatorTest < ActiveSupport::TestCase Holding.new(security: @amzn, date: Date.current, qty: 0, price: 200, amount: 0) ] - calculated = Holding::ReverseCalculator.new(@account).calculate + # Mock snapshot with today's portfolio from load_today_portfolio + snapshot = OpenStruct.new(to_h: { @voo.id => 10, @wmt.id => 100, @amzn.id => 0 }) + calculated = Holding::ReverseCalculator.new(@account, portfolio_snapshot: snapshot).calculate assert_equal expected.length, calculated.length @@ -136,7 +143,9 @@ class Holding::ReverseCalculatorTest < ActiveSupport::TestCase Holding.new(security: wmt, date: Date.current, qty: 50, price: 100, amount: 5000) # Uses holding price, not market price ] - calculated = Holding::ReverseCalculator.new(@account).calculate + # Mock snapshot with WMT holding from the test setup + snapshot = OpenStruct.new(to_h: { wmt.id => 50 }) + calculated = Holding::ReverseCalculator.new(@account, portfolio_snapshot: snapshot).calculate assert_equal expected.length, calculated.length diff --git a/test/models/plaid_account/investments/holdings_processor_test.rb b/test/models/plaid_account/investments/holdings_processor_test.rb index ac5b5895..3aa797da 100644 --- a/test/models/plaid_account/investments/holdings_processor_test.rb +++ b/test/models/plaid_account/investments/holdings_processor_test.rb @@ -14,6 +14,13 @@ class PlaidAccount::Investments::HoldingsProcessorTest < ActiveSupport::TestCase "security_id" => "123", "quantity" => 100, "institution_price" => 100, + "iso_currency_code" => "USD", + "institution_price_as_of" => 1.day.ago.to_date + }, + { + "security_id" => "456", + "quantity" => 200, + "institution_price" => 200, "iso_currency_code" => "USD" } ], @@ -32,18 +39,159 @@ class PlaidAccount::Investments::HoldingsProcessorTest < ActiveSupport::TestCase ) ) + @security_resolver.expects(:resolve) + .with(plaid_security_id: "456") + .returns( + OpenStruct.new( + security: securities(:aapl), + cash_equivalent?: false, + brokerage_cash?: false + ) + ) + processor = PlaidAccount::Investments::HoldingsProcessor.new(@plaid_account, security_resolver: @security_resolver) - assert_difference "Holding.count" do + assert_difference "Holding.count", 2 do processor.process end - holding = Holding.order(created_at: :desc).first + holdings = Holding.where(account: @plaid_account.account).order(:date) - assert_equal 100, holding.qty - assert_equal 100, holding.price - assert_equal "USD", holding.currency - assert_equal securities(:aapl), holding.security - assert_equal Date.current, holding.date + assert_equal 100, holdings.first.qty + assert_equal 100, holdings.first.price + assert_equal "USD", holdings.first.currency + assert_equal securities(:aapl), holdings.first.security + assert_equal 1.day.ago.to_date, holdings.first.date + + assert_equal 200, holdings.second.qty + assert_equal 200, holdings.second.price + assert_equal "USD", holdings.second.currency + assert_equal securities(:aapl), holdings.second.security + assert_equal Date.current, holdings.second.date + end + + # When Plaid provides holdings data, it includes an "institution_price_as_of" date + # which represents when the holdings were last updated. Any holdings in our database + # after this date are now stale and should be deleted, as the Plaid data is the + # authoritative source of truth for the current holdings. + test "deletes stale holdings per security based on institution price date" do + account = @plaid_account.account + + # Create a third security for testing + third_security = Security.create!(ticker: "GOOGL", name: "Google", exchange_operating_mic: "XNAS", country_code: "US") + + # Scenario 3: AAPL has a stale holding that should be deleted + stale_aapl_holding = account.holdings.create!( + security: securities(:aapl), + date: Date.current, + qty: 80, + price: 180, + amount: 14400, + currency: "USD" + ) + + # Plaid returns 3 holdings with different scenarios + test_investments_payload = { + securities: [], + holdings: [ + # Scenario 1: Current date holding (no deletions needed) + { + "security_id" => "current", + "quantity" => 50, + "institution_price" => 50, + "iso_currency_code" => "USD", + "institution_price_as_of" => Date.current + }, + # Scenario 2: Yesterday's holding with no future holdings + { + "security_id" => "clean", + "quantity" => 75, + "institution_price" => 75, + "iso_currency_code" => "USD", + "institution_price_as_of" => 1.day.ago.to_date + }, + # Scenario 3: Yesterday's holding with stale future holding + { + "security_id" => "stale", + "quantity" => 100, + "institution_price" => 100, + "iso_currency_code" => "USD", + "institution_price_as_of" => 1.day.ago.to_date + } + ], + transactions: [] + } + + @plaid_account.update!(raw_investments_payload: test_investments_payload) + + # Mock security resolver for all three securities + @security_resolver.expects(:resolve) + .with(plaid_security_id: "current") + .returns(OpenStruct.new(security: securities(:msft), cash_equivalent?: false, brokerage_cash?: false)) + + @security_resolver.expects(:resolve) + .with(plaid_security_id: "clean") + .returns(OpenStruct.new(security: third_security, cash_equivalent?: false, brokerage_cash?: false)) + + @security_resolver.expects(:resolve) + .with(plaid_security_id: "stale") + .returns(OpenStruct.new(security: securities(:aapl), cash_equivalent?: false, brokerage_cash?: false)) + + processor = PlaidAccount::Investments::HoldingsProcessor.new(@plaid_account, security_resolver: @security_resolver) + processor.process + + # Should have created 3 new holdings + assert_equal 3, account.holdings.count + + # Scenario 3: Should have deleted the stale AAPL holding + assert_not account.holdings.exists?(stale_aapl_holding.id) + + # Should have the correct holdings from Plaid + assert account.holdings.exists?(security: securities(:msft), date: Date.current, qty: 50) + assert account.holdings.exists?(security: third_security, date: 1.day.ago.to_date, qty: 75) + assert account.holdings.exists?(security: securities(:aapl), date: 1.day.ago.to_date, qty: 100) + end + + test "continues processing other holdings when security resolution fails" do + test_investments_payload = { + securities: [], + holdings: [ + { + "security_id" => "fail", + "quantity" => 100, + "institution_price" => 100, + "iso_currency_code" => "USD" + }, + { + "security_id" => "success", + "quantity" => 200, + "institution_price" => 200, + "iso_currency_code" => "USD" + } + ], + transactions: [] + } + + @plaid_account.update!(raw_investments_payload: test_investments_payload) + + # First security fails to resolve + @security_resolver.expects(:resolve) + .with(plaid_security_id: "fail") + .returns(OpenStruct.new(security: nil)) + + # Second security succeeds + @security_resolver.expects(:resolve) + .with(plaid_security_id: "success") + .returns(OpenStruct.new(security: securities(:aapl))) + + processor = PlaidAccount::Investments::HoldingsProcessor.new(@plaid_account, security_resolver: @security_resolver) + + # Should create only 1 holding (the successful one) + assert_difference "Holding.count", 1 do + processor.process + end + + # Should have created the successful holding + assert @plaid_account.account.holdings.exists?(security: securities(:aapl), qty: 200) end end