Optimize and Fix provider price fetches for sold securities and batch queries

app/models/account/market_data_importer.rb

@@ -45,34 +45,52 @@ def import_exchange_rates
   def import_security_prices
     return unless Security.provider
 
-    account_securities = (account.trades.map(&:security) + account.current_holdings.map(&:security)).uniq
+    current_security_ids = account.current_holdings.pluck(:security_id).to_set
+    traded_security_ids  = account.trades.pluck(:security_id).uniq
 
-    return if account_securities.empty?
+    all_security_ids = (current_security_ids | traded_security_ids)
+    return if all_security_ids.empty?
 
-    account_securities.each do |security|
-      security.import_provider_prices(
-        start_date: first_required_price_date(security),
-        end_date: Date.current
-      )
+    securities = Security.online.where(id: all_security_ids).index_by(&:id)
+
+    start_dates       = batch_first_required_price_dates(all_security_ids)
+
+    # For securities no longer held, cap end_date at the last holding date so
+    # all_prices_exist? stays stable and we don't call the provider every sync.
+    historical_ids    = traded_security_ids - current_security_ids.to_a
+    last_holding_date = account.holdings
+                               .where(security_id: historical_ids)
+                               .group(:security_id)
+                               .maximum(:date)
+
+    all_security_ids.each do |security_id|
+      security = securities[security_id]
+      next unless security
+
+      end_date = current_security_ids.include?(security_id) ? Date.current : (last_holding_date[security_id] || Date.current)
 
+      security.import_provider_prices(start_date: start_dates[security_id], end_date: end_date)
       security.import_provider_details
     end
   end
 
   private
-    # Calculates the first date we require a price for the given security scoped to this account
-    def first_required_price_date(security)
-      trade_start_date = account.trades.with_entry
-                              .where(security: security)
-                              .where(entries: { account_id: account.id })
-                              .minimum("entries.date")
-
-      holding_start_date =
-        if account.holdings.where(security: security).where.not(account_provider_id: nil).exists?
-          account.start_date
-        end
-
-      [ trade_start_date, holding_start_date ].compact.min
+    # Replaces 2-queries-per-security with 3 queries total.
+    def batch_first_required_price_dates(security_ids)
+      # account.trades is a has_many :through :entries, so entries is already joined
+      trade_start_dates = account.trades.group(:security_id).minimum("entries.date")
+
+      provider_holding_security_ids = account.holdings
+                                             .where(security_id: security_ids)
+                                             .where.not(account_provider_id: nil)
+                                             .pluck(:security_id)
+                                             .to_set
+
+      security_ids.each_with_object({}) do |security_id, hash|
+        trade_date   = trade_start_dates[security_id]
+        holding_date = provider_holding_security_ids.include?(security_id) ? account.start_date : nil
+        hash[security_id] = [ trade_date, holding_date ].compact.min || account.start_date
+      end
     end
 
     def needs_exchange_rates?

test/models/account/market_data_importer_test.rb

@@ -112,6 +112,56 @@ class Account::MarketDataImporterTest < ActiveSupport::TestCase
     assert_equal 1, Security::Price.where(security: security, date: trade_date).count
   end
 
+  test "caps end_date at last holding date for securities no longer held" do
+    family = Family.create!(name: "Smith", currency: "USD")
+
+    account = family.accounts.create!(
+      name: "Brokerage",
+      currency: "USD",
+      balance: 0,
+      accountable: Investment.new
+    )
+
+    current_sec    = Security.create!(ticker: "CURR", exchange_operating_mic: "XNAS")
+    historical_sec = Security.create!(ticker: "HIST", exchange_operating_mic: "XNAS")
+
+    trade_date  = 30.days.ago.to_date
+    sold_date   = 5.days.ago.to_date
+
+    [ current_sec, historical_sec ].each do |sec|
+      trade = Trade.new(security: sec, qty: 10, price: 100, currency: "USD", investment_activity_label: "Buy")
+      account.entries.create!(name: "Buy #{sec.ticker}", date: trade_date, amount: 1000, currency: "USD", entryable: trade)
+    end
+
+    # Current: most-recent holding has qty > 0 — shows up in current_holdings
+    account.holdings.create!(security: current_sec, date: Date.current, qty: 10, price: 110, amount: 1100, currency: "USD")
+
+    # Historical: most-recent holding has qty == 0 (sold) — excluded from current_holdings
+    account.holdings.create!(security: historical_sec, date: 10.days.ago.to_date, qty: 10, price: 105, amount: 1050, currency: "USD")
+    account.holdings.create!(security: historical_sec, date: sold_date, qty: 0, price: 0, amount: 0, currency: "USD")
+
+    expected_start_date = trade_date - SECURITY_PRICE_BUFFER
+
+    @provider.expects(:fetch_security_prices)
+             .with(symbol: current_sec.ticker,
+                   exchange_operating_mic: current_sec.exchange_operating_mic,
+                   start_date: expected_start_date,
+                   end_date: Date.current.in_time_zone("America/New_York").to_date)
+             .returns(provider_success_response([]))
+
+    @provider.expects(:fetch_security_prices)
+             .with(symbol: historical_sec.ticker,
+                   exchange_operating_mic: historical_sec.exchange_operating_mic,
+                   start_date: expected_start_date,
+                   end_date: sold_date)
+             .returns(provider_success_response([]))
+
+    @provider.stubs(:fetch_security_info).returns(provider_success_response(OpenStruct.new(name: "Test", logo_url: nil)))
+    @provider.stubs(:fetch_exchange_rates).returns(provider_success_response([]))
+
+    Account::MarketDataImporter.new(account).import_all
+  end
+
   test "handles provider error response gracefully for security prices" do
     family = Family.create!(name: "Smith", currency: "USD")