mirror of
https://github.com/maybe-finance/maybe.git
synced 2025-07-18 20:59:39 +02:00
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
This commit is contained in:
Zach Gollwitzer
GitHub
parent
e60b5df442
commit
8db95623cf
8 changed files with 281 additions and 39 deletions
50
test/models/holding/portfolio_snapshot_test.rb
Normal file
50
test/models/holding/portfolio_snapshot_test.rb
Normal file
|
|
@ -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
|
||||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue