Match Plaid holding values on current day (#2212)

* Match Plaid holding values on current day

* Fix chart timezone issue

* Add timezone tests for syncs

* Hide sidebars on trades test

.gitignore

@@ -69,3 +69,5 @@ coverage
 node_modules
 
 compose.yml
+
+plaid_test_accounts/

app/controllers/accountable_sparklines_controller.rb

@@ -7,6 +7,7 @@ class AccountableSparklinesController < ApplicationController
               .where(accountable_type: @accountable.name)
               .balance_series(
                 currency: family.currency,
+                timezone: family.timezone,
                 favorable_direction: @accountable.favorable_direction
               )
     end

app/javascript/controllers/time_series_chart_controller.js

@@ -1,6 +1,8 @@
 import { Controller } from "@hotwired/stimulus";
 import * as d3 from "d3";
 
+const parseLocalDate = d3.timeParse("%Y-%m-%d");
+
 export default class extends Controller {
   static values = {
     data: Object,
@@ -51,10 +53,12 @@ export default class extends Controller {
 
   _normalizeDataPoints() {
     this._normalDataPoints = (this.dataValue.values || []).map((d) => ({
-      date: new Date(`${d.date}T00:00:00Z`),
+      date: parseLocalDate(d.date),
       date_formatted: d.date_formatted,
       trend: d.trend,
     }));
+
+    console.log(this._normalDataPoints);
   }
 
   _rememberInitialContainerSize() {
@@ -185,7 +189,7 @@ export default class extends Controller {
             this._normalDataPoints[this._normalDataPoints.length - 1].date,
           ])
           .tickSize(0)
-          .tickFormat(d3.utcFormat("%b %d, %Y")),
+          .tickFormat(d3.timeFormat("%b %d, %Y")),
       )
       .select(".domain")
       .remove();

app/models/account/chartable.rb

@@ -2,7 +2,7 @@ module Account::Chartable
   extend ActiveSupport::Concern
 
   class_methods do
-    def balance_series(currency:, period: Period.last_30_days, favorable_direction: "up", view: :balance, interval: nil)
+    def balance_series(currency:, period: Period.last_30_days, favorable_direction: "up", view: :balance, interval: nil, timezone: nil)
       raise ArgumentError, "Invalid view type" unless [ :balance, :cash_balance, :holdings_balance ].include?(view.to_sym)
 
       series_interval = interval || period.interval
@@ -51,7 +51,7 @@ module Account::Chartable
           WITH dates as (
             SELECT generate_series(DATE :start_date, DATE :end_date, :interval::interval)::date as date
             UNION DISTINCT
-            SELECT CURRENT_DATE -- Ensures we always end on current date, regardless of interval
+            SELECT :end_date::date AS date -- Ensures we always end on user's "today" date, regardless of interval
           )
           SELECT
             d.date,
@@ -132,7 +132,8 @@ module Account::Chartable
       period: period,
       view: view,
       interval: interval,
-      favorable_direction: favorable_direction
+      favorable_direction: favorable_direction,
+      timezone: family.timezone
     )
   end
 

app/models/assistant/function/get_accounts.rb

@@ -33,7 +33,7 @@ class Assistant::Function::GetAccounts < Assistant::Function
     def historical_balances(account)
       start_date = [ account.start_date, 5.years.ago.to_date ].max
       period = Period.custom(start_date: start_date, end_date: Date.current)
-      balance_series = account.balance_series(period: period, interval: "1 month")
+      balance_series = account.balance_series(period: period, interval: "1 month", timezone: family.timezone)
 
       to_ai_time_series(balance_series)
     end

app/models/assistant/function/get_balance_sheet.rb

@@ -55,6 +55,7 @@ class Assistant::Function::GetBalanceSheet < Assistant::Function
           period: period,
           interval: "1 month",
           favorable_direction: "up",
+          timezone: family.timezone
         )
 
         to_ai_time_series(balance_series)

app/models/balance_sheet.rb

@@ -69,7 +69,7 @@ class BalanceSheet
   end
 
   def net_worth_series(period: Period.last_30_days)
-    active_accounts.balance_series(currency: currency, period: period, favorable_direction: "up")
+    active_accounts.balance_series(currency: currency, period: period, favorable_direction: "up", timezone: family.timezone)
   end
 
   def currency

app/models/holding/base_calculator.rb

@@ -40,12 +40,11 @@ class Holding::BaseCalculator
       new_quantities
     end
 
-    def build_holdings(portfolio, date)
+    def build_holdings(portfolio, date, price_source: nil)
       portfolio.map do |security_id, qty|
-        price = portfolio_cache.get_price(security_id, date)
+        price = portfolio_cache.get_price(security_id, date, source: price_source)
 
         if price.nil?
-          Rails.logger.warn "No price found for security #{security_id} on #{date}"
           next
         end
 

app/models/holding/portfolio_cache.rb

@@ -21,11 +21,15 @@ class Holding::PortfolioCache
     end
   end
 
-  def get_price(security_id, date)
+  def get_price(security_id, date, source: nil)
     security = @security_cache[security_id]
     raise SecurityNotFound.new(security_id, account.id) unless security
 
-    price = security[:prices].select { |p| p.price.date == date }.min_by(&:priority)&.price
+    if source.present?
+      price = security[:prices].select { |p| p.price.date == date && p.source == source }.min_by(&:priority)&.price
+    else
+      price = security[:prices].select { |p| p.price.date == date }.min_by(&:priority)&.price
+    end
 
     return nil unless price
 
@@ -46,7 +50,7 @@ class Holding::PortfolioCache
   end
 
   private
-    PriceWithPriority = Data.define(:price, :priority)
+    PriceWithPriority = Data.define(:price, :priority, :source)
 
     def trades
       @trades ||= account.entries.includes(entryable: :security).trades.chronological.to_a
@@ -86,7 +90,8 @@ class Holding::PortfolioCache
         db_prices = security.prices.where(date: account.start_date..Date.current).map do |price|
           PriceWithPriority.new(
             price: price,
-            priority: 1
+            priority: 1,
+            source: "db"
           )
         end
 
@@ -101,7 +106,8 @@ class Holding::PortfolioCache
                 currency: trade.entryable.currency,
                 date: trade.date
               ),
-              priority: 2
+              priority: 2,
+              source: "trade"
             )
           end
 
@@ -115,7 +121,8 @@ class Holding::PortfolioCache
                 currency: holding.currency,
                 date: holding.date
               ),
-              priority: 3
+              priority: 3,
+              source: "holding"
             )
           end
         else

app/models/holding/reverse_calculator.rb

@@ -16,7 +16,9 @@ class Holding::ReverseCalculator < Holding::BaseCalculator
       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)
+
+        # If current day, always use holding prices (since that's what Plaid gives us).  For historical values, use market data (since Plaid doesn't supply historical prices)
+        holdings += build_holdings(current_portfolio, date, price_source: date == Date.current ? "holding" : nil)
         current_portfolio = previous_portfolio
       end
 

app/views/onboardings/show.html.erb

@@ -1,4 +1,4 @@
-<%= content_for :prev_nav do  %>
+<%= content_for :prev_nav do %>
   <%= image_tag "logomark-color.svg", class: "w-10 h-10" %>
 <% end %>
 
@@ -38,8 +38,7 @@
             <%= family_form.select :country,
               country_options,
               { label: "Country" },
-              required: true
-            %>
+              required: true %>
           <% end %>
         </div>
       <% end %>

test/models/balance/forward_calculator_test.rb

@@ -13,6 +13,23 @@ class Balance::ForwardCalculatorTest < ActiveSupport::TestCase
     )
   end
 
+  test "balance generation respects user timezone and last generated date is current user date" do
+    # Simulate user in EST timezone
+    Time.zone = "America/New_York"
+
+    # Set current time to 1am UTC on Jan 5, 2025
+    # This would be 8pm EST on Jan 4, 2025 (user's time, and the last date we should generate balances for)
+    travel_to Time.utc(2025, 01, 05, 1, 0, 0)
+
+    # Create a valuation for Jan 3, 2025
+    create_valuation(account: @account, date: "2025-01-03", amount: 17000)
+
+    expected = [ [ "2025-01-02", 0 ], [ "2025-01-03", 17000 ], [ "2025-01-04", 17000 ] ]
+    calculated = Balance::ForwardCalculator.new(@account).calculate
+
+    assert_equal expected, calculated.map { |b| [ b.date.to_s, b.balance ] }
+  end
+
   # When syncing forwards, we don't care about the account balance.  We generate everything based on entries, starting from 0.
   test "no entries sync" do
     assert_equal 0, @account.balances.count
@@ -71,4 +88,42 @@ class Balance::ForwardCalculatorTest < ActiveSupport::TestCase
 
     assert_equal expected, calculated
   end
+
+  test "holdings and trades sync" do
+    aapl = securities(:aapl)
+
+    # Account starts at a value of $5000
+    create_valuation(account: @account, date: 2.days.ago.to_date, amount: 5000)
+
+    # Share purchase reduces cash balance by $1000, but keeps overall balance same
+    create_trade(aapl, account: @account, qty: 10, date: 1.day.ago.to_date, price: 100)
+
+    Holding.create!(date: 1.day.ago.to_date, account: @account, security: aapl, qty: 10, price: 100, amount: 1000, currency: "USD")
+    Holding.create!(date: Date.current, account: @account, security: aapl, qty: 10, price: 100, amount: 1000, currency: "USD")
+
+    # Given constant prices, overall balance (account value) should be constant
+    # (the single trade doesn't affect balance; it just alters cash vs. holdings composition)
+    expected = [ 0, 5000, 5000, 5000 ]
+    calculated = Balance::ForwardCalculator.new(@account).calculate.sort_by(&:date).map(&:balance)
+
+    assert_equal expected, calculated
+  end
+
+  # Balance calculator is entirely reliant on HoldingCalculator and respects whatever holding records it creates.
+  test "holdings are additive to total balance" do
+    aapl = securities(:aapl)
+
+    # Account starts at a value of $5000
+    create_valuation(account: @account, date: 2.days.ago.to_date, amount: 5000)
+
+    # Even though there are no trades in the history, the calculator will still add the holdings to the total balance
+    Holding.create!(date: 1.day.ago.to_date, account: @account, security: aapl, qty: 10, price: 100, amount: 1000, currency: "USD")
+    Holding.create!(date: Date.current, account: @account, security: aapl, qty: 10, price: 100, amount: 1000, currency: "USD")
+
+    # Start at zero, then valuation of $5000, then tack on $1000 of holdings for remaining 2 days
+    expected = [ 0, 5000, 6000, 6000 ]
+    calculated = Balance::ForwardCalculator.new(@account).calculate.sort_by(&:date).map(&:balance)
+
+    assert_equal expected, calculated
+  end
 end

test/models/balance/reverse_calculator_test.rb

@@ -23,6 +23,22 @@ class Balance::ReverseCalculatorTest < ActiveSupport::TestCase
     assert_equal expected, calculated.map(&:balance)
   end
 
+  test "balance generation respects user timezone and last generated date is current user date" do
+    # Simulate user in EST timezone
+    Time.zone = "America/New_York"
+
+    # Set current time to 1am UTC on Jan 5, 2025
+    # This would be 8pm EST on Jan 4, 2025 (user's time, and the last date we should generate balances for)
+    travel_to Time.utc(2025, 01, 05, 1, 0, 0)
+
+    create_valuation(account: @account, date: "2025-01-03", amount: 17000)
+
+    expected = [ [ "2025-01-02", 17000 ], [ "2025-01-03", 17000 ], [ "2025-01-04", @account.balance ] ]
+    calculated = Balance::ReverseCalculator.new(@account).calculate
+
+    assert_equal expected, calculated.sort_by(&:date).map { |b| [ b.date.to_s, b.balance ] }
+  end
+
   test "valuations sync" do
     create_valuation(account: @account, date: 4.days.ago.to_date, amount: 17000)
     create_valuation(account: @account, date: 2.days.ago.to_date, amount: 19000)
@@ -56,4 +72,52 @@ class Balance::ReverseCalculatorTest < ActiveSupport::TestCase
 
     assert_equal expected, calculated
   end
+
+  # When syncing backwards, trades from the past should NOT affect the current balance or previous balances.
+  # They should only affect the *cash* component of the historical balances
+  test "holdings and trades sync" do
+    aapl = securities(:aapl)
+
+    # Account starts with $20,000 total value, $19,000 cash, $1,000 in holdings
+    @account.update!(cash_balance: 19000, balance: 20000)
+
+    # Bought 10 AAPL shares 1 day ago, so cash is $19,000, $1,000 in holdings, total value is $20,000
+    create_trade(aapl, account: @account, qty: 10, date: 1.day.ago.to_date, price: 100)
+
+    Holding.create!(date: Date.current, account: @account, security: aapl, qty: 10, price: 100, amount: 1000, currency: "USD")
+    Holding.create!(date: 1.day.ago.to_date, account: @account, security: aapl, qty: 10, price: 100, amount: 1000, currency: "USD")
+
+    # Given constant prices, overall balance (account value) should be constant
+    # (the single trade doesn't affect balance; it just alters cash vs. holdings composition)
+    expected = [ 20000, 20000, 20000 ]
+    calculated = Balance::ReverseCalculator.new(@account).calculate.sort_by(&:date).map(&:balance)
+
+    assert_equal expected, calculated
+  end
+
+  # A common scenario with Plaid is they'll give us holding records for today, but no trade history for some of them.
+  # This is because they only supply 2 years worth of historical data.  Our system must properly handle this.
+  test "properly calculates balances when a holding has no trade history" do
+    aapl = securities(:aapl)
+    msft = securities(:msft)
+
+    # Account starts with $20,000 total value, $19,000 cash, $1,000 in holdings ($500 AAPL, $500 MSFT)
+    @account.update!(cash_balance: 19000, balance: 20000)
+
+    # A holding *with* trade history (5 shares of AAPL, purchased 1 day ago, results in 2 holdings)
+    Holding.create!(date: Date.current, account: @account, security: aapl, qty: 5, price: 100, amount: 500, currency: "USD")
+    Holding.create!(date: 1.day.ago.to_date, account: @account, security: aapl, qty: 5, price: 100, amount: 500, currency: "USD")
+    create_trade(aapl, account: @account, qty: 5, date: 1.day.ago.to_date, price: 100)
+
+    # A holding *without* trade history (5 shares of MSFT, no trade history, results in 1 holding)
+    # We assume if no history is provided, this holding has existed since beginning of account
+    Holding.create!(date: Date.current, account: @account, security: msft, qty: 5, price: 100, amount: 500, currency: "USD")
+    Holding.create!(date: 1.day.ago.to_date, account: @account, security: msft, qty: 5, price: 100, amount: 500, currency: "USD")
+    Holding.create!(date: 2.days.ago.to_date, account: @account, security: msft, qty: 5, price: 100, amount: 500, currency: "USD")
+
+    expected = [ 20000, 20000, 20000 ]
+    calculated = Balance::ReverseCalculator.new(@account).calculate.sort_by(&:date).map(&:balance)
+
+    assert_equal expected, calculated
+  end
 end

test/models/holding/forward_calculator_test.rb

@@ -18,6 +18,26 @@ class Holding::ForwardCalculatorTest < ActiveSupport::TestCase
     assert_equal [], calculated
   end
 
+  test "holding generation respects user timezone and last generated date is current user date" do
+    # Simulate user in EST timezone
+    Time.zone = "America/New_York"
+
+    # Set current time to 1am UTC on Jan 5, 2025
+    # This would be 8pm EST on Jan 4, 2025 (user's time, and the last date we should generate holdings for)
+    travel_to Time.utc(2025, 01, 05, 1, 0, 0)
+
+    voo = Security.create!(ticker: "VOO", name: "Vanguard S&P 500 ETF")
+    Security::Price.create!(security: voo, date: "2025-01-02", price: 500)
+    Security::Price.create!(security: voo, date: "2025-01-03", price: 500)
+    Security::Price.create!(security: voo, date: "2025-01-04", price: 500)
+    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::ForwardCalculator.new(@account).calculate
+
+    assert_equal expected, calculated.map { |b| [ b.date.to_s, b.amount ] }
+  end
+
   test "forward portfolio calculation" do
     load_prices
 

test/models/holding/reverse_calculator_test.rb

@@ -18,6 +18,30 @@ class Holding::ReverseCalculatorTest < ActiveSupport::TestCase
     assert_equal [], calculated
   end
 
+  test "holding generation respects user timezone and last generated date is current user date" do
+    # Simulate user in EST timezone
+    Time.zone = "America/New_York"
+
+    # Set current time to 1am UTC on Jan 5, 2025
+    # This would be 8pm EST on Jan 4, 2025 (user's time, and the last date we should generate holdings for)
+    travel_to Time.utc(2025, 01, 05, 1, 0, 0)
+
+    voo = Security.create!(ticker: "VOO", name: "Vanguard S&P 500 ETF")
+    Security::Price.create!(security: voo, date: "2025-01-02", price: 500)
+    Security::Price.create!(security: voo, date: "2025-01-03", price: 500)
+    Security::Price.create!(security: voo, date: "2025-01-04", price: 500)
+
+    # Today's holdings (provided)
+    @account.holdings.create!(security: voo, date: "2025-01-04", qty: 10, price: 500, amount: 5000, currency: "USD")
+
+    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
+
+    assert_equal expected, calculated.sort_by(&:date).map { |b| [ b.date.to_s, b.amount ] }
+  end
+
   # Should be able to handle this case, although we should not be reverse-syncing an account without provided current day holdings
   test "reverse portfolio with trades but without current day holdings" do
     voo = Security.create!(ticker: "VOO", name: "Vanguard S&P 500 ETF")
@@ -85,6 +109,46 @@ class Holding::ReverseCalculatorTest < ActiveSupport::TestCase
     end
   end
 
+  # For a reverse sync, Plaid will provide today's holdings + prices.  We need to match those exactly so balances match in net worth rollups.
+  test "current day holdings always match provided holdings and prices" do
+    # Provider gives us total value of $10,000 ($5,000 cash, $5,000 in holdings)
+    @account.update!(balance: 10000, cash_balance: 5000)
+
+    wmt = Security.create!(ticker: "WMT", name: "Walmart Inc.")
+    create_trade(wmt, qty: 50, date: 1.day.ago.to_date, price: 98, account: @account)
+
+    @account.holdings.create!(
+      date: Date.current,
+      price: 100,
+      qty: 50,
+      amount: 5000,
+      currency: "USD",
+      security: wmt
+    )
+
+    Security::Price.create!(security: wmt, date: Date.current, price: 102) # This price should be ignored on current day
+    Security::Price.create!(security: wmt, date: 1.day.ago, price: 98) # This price will be used for historical holding calculation
+    Security::Price.create!(security: wmt, date: 2.days.ago, price: 95) # This price will be used for historical holding calculation
+
+    expected = [
+      Holding.new(security: wmt, date: 2.days.ago.to_date, qty: 0, price: 95, amount: 0), # Uses market price, empty holding
+      Holding.new(security: wmt, date: 1.day.ago.to_date, qty: 50, price: 98, amount: 4900), # Uses market price
+      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
+
+    assert_equal expected.length, calculated.length
+
+    expected.each do |expected_entry|
+      calculated_entry = calculated.find { |c| c.security == expected_entry.security && c.date == expected_entry.date }
+
+      assert_equal expected_entry.qty, calculated_entry.qty, "Qty mismatch for #{expected_entry.security.ticker} on #{expected_entry.date}"
+      assert_equal expected_entry.price, calculated_entry.price, "Price mismatch for #{expected_entry.security.ticker} on #{expected_entry.date}"
+      assert_equal expected_entry.amount, calculated_entry.amount, "Amount mismatch for #{expected_entry.security.ticker} on #{expected_entry.date}"
+    end
+  end
+
   private
     def assert_holdings(expected, calculated)
       expected.each do |expected_entry|

test/system/trades_test.rb

@@ -6,6 +6,8 @@ class TradesTest < ApplicationSystemTestCase
   setup do
     sign_in @user = users(:family_admin)
 
+    @user.update!(show_sidebar: false, show_ai_sidebar: false)
+
     @account = accounts(:investment)
 
     visit_account_portfolio