diff --git a/.gitignore b/.gitignore index 010505c1..eeb5e8f2 100644 --- a/.gitignore +++ b/.gitignore @@ -68,4 +68,6 @@ coverage # Ignore node related files node_modules -compose.yml \ No newline at end of file +compose.yml + +plaid_test_accounts/ \ No newline at end of file diff --git a/app/controllers/accountable_sparklines_controller.rb b/app/controllers/accountable_sparklines_controller.rb index 17892479..505700b3 100644 --- a/app/controllers/accountable_sparklines_controller.rb +++ b/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 diff --git a/app/javascript/controllers/time_series_chart_controller.js b/app/javascript/controllers/time_series_chart_controller.js index 57cac880..abec91e3 100644 --- a/app/javascript/controllers/time_series_chart_controller.js +++ b/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(); diff --git a/app/models/account/chartable.rb b/app/models/account/chartable.rb index d9a6c44b..8a34c25d 100644 --- a/app/models/account/chartable.rb +++ b/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 diff --git a/app/models/assistant/function/get_accounts.rb b/app/models/assistant/function/get_accounts.rb index b912d81d..38db5758 100644 --- a/app/models/assistant/function/get_accounts.rb +++ b/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 diff --git a/app/models/assistant/function/get_balance_sheet.rb b/app/models/assistant/function/get_balance_sheet.rb index 81afeccb..930eda6d 100644 --- a/app/models/assistant/function/get_balance_sheet.rb +++ b/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) diff --git a/app/models/balance_sheet.rb b/app/models/balance_sheet.rb index c289f86f..fce184b2 100644 --- a/app/models/balance_sheet.rb +++ b/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 diff --git a/app/models/holding/base_calculator.rb b/app/models/holding/base_calculator.rb index 47178d8b..d9b85d03 100644 --- a/app/models/holding/base_calculator.rb +++ b/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 diff --git a/app/models/holding/portfolio_cache.rb b/app/models/holding/portfolio_cache.rb index e8d3fcec..2d67a1d8 100644 --- a/app/models/holding/portfolio_cache.rb +++ b/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 diff --git a/app/models/holding/reverse_calculator.rb b/app/models/holding/reverse_calculator.rb index f3996e5f..62e2dc95 100644 --- a/app/models/holding/reverse_calculator.rb +++ b/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 diff --git a/app/views/onboardings/show.html.erb b/app/views/onboardings/show.html.erb index f5e5a5f9..43510796 100644 --- a/app/views/onboardings/show.html.erb +++ b/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 %> <% end %> diff --git a/test/models/balance/forward_calculator_test.rb b/test/models/balance/forward_calculator_test.rb index 7cc84738..24aecdb5 100644 --- a/test/models/balance/forward_calculator_test.rb +++ b/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 diff --git a/test/models/balance/reverse_calculator_test.rb b/test/models/balance/reverse_calculator_test.rb index 358d7313..29711702 100644 --- a/test/models/balance/reverse_calculator_test.rb +++ b/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 diff --git a/test/models/holding/forward_calculator_test.rb b/test/models/holding/forward_calculator_test.rb index f05e67fc..7a8aaa53 100644 --- a/test/models/holding/forward_calculator_test.rb +++ b/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 diff --git a/test/models/holding/reverse_calculator_test.rb b/test/models/holding/reverse_calculator_test.rb index 4a13e71f..ec9dc204 100644 --- a/test/models/holding/reverse_calculator_test.rb +++ b/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| diff --git a/test/system/trades_test.rb b/test/system/trades_test.rb index 1222dfe7..930ab296 100644 --- a/test/system/trades_test.rb +++ b/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