Security resolver and health checker (#2281)

* Setup health check

* Security health checker cron

* Use resolver throughout codebase

* Use resolver for trade builder

* Add security health checks to schedule

* Handle no provider

* Lint fixes

test/models/security/health_checker_test.rb

@@ -0,0 +1,158 @@
+require "test_helper"
+
+class Security::HealthCheckerTest < ActiveSupport::TestCase
+  include ProviderTestHelper
+
+  setup do
+    # Clean slate
+    Holding.destroy_all
+    Trade.destroy_all
+    Security::Price.delete_all
+    Security.delete_all
+
+    @provider = mock
+    Security.stubs(:provider).returns(@provider)
+
+    # Brand new, no health check has been run yet
+    @new_security = Security.create!(
+      ticker: "NEW",
+      offline: false,
+      last_health_check_at: nil
+    )
+
+    # New security, offline
+    # This will be checked, but unless it gets a price, we keep it offline
+    @new_offline_security = Security.create!(
+      ticker: "NEW_OFFLINE",
+      offline: true,
+      last_health_check_at: nil
+    )
+
+    # Online, recently checked, healthy
+    @healthy_security = Security.create!(
+      ticker: "HEALTHY",
+      offline: false,
+      last_health_check_at: 2.hours.ago
+    )
+
+    # Online, due for a health check
+    @due_for_check_security = Security.create!(
+      ticker: "DUE",
+      offline: false,
+      last_health_check_at: Security::HealthChecker::HEALTH_CHECK_INTERVAL.ago - 1.day
+    )
+
+    # Offline, recently checked (keep offline, don't check)
+    @offline_security = Security.create!(
+      ticker: "OFFLINE",
+      offline: true,
+      last_health_check_at: 20.days.ago
+    )
+
+    # Currently offline, but has had no health check and actually has prices (needs to convert to "online")
+    @offline_never_checked_with_prices = Security.create!(
+      ticker: "OFFLINE_NEVER_CHECKED",
+      offline: true,
+      last_health_check_at: nil
+    )
+  end
+
+  test "any security without a health check runs" do
+    to_check = Security.where(last_health_check_at: nil).or(Security.where(last_health_check_at: ..Security::HealthChecker::HEALTH_CHECK_INTERVAL.ago))
+    Security::HealthChecker.any_instance.expects(:run_check).times(to_check.count)
+    Security::HealthChecker.check_all
+  end
+
+  test "offline security with no health check that fails stays offline" do
+    hc = Security::HealthChecker.new(@new_offline_security)
+
+    @provider.expects(:fetch_security_price)
+      .with(
+        symbol: @new_offline_security.ticker,
+        exchange_operating_mic: @new_offline_security.exchange_operating_mic,
+        date: Date.current
+      )
+      .returns(
+        provider_error_response(StandardError.new("No prices found"))
+      )
+      .once
+
+    hc.run_check
+
+    assert_equal 1, @new_offline_security.failed_fetch_count
+    assert @new_offline_security.offline?
+  end
+
+  test "after enough consecutive health check failures, security goes offline and prices are deleted" do
+    # Create one test price
+    Security::Price.create!(
+      security: @due_for_check_security,
+      date: Date.current,
+      price: 100,
+      currency: "USD"
+    )
+
+    hc = Security::HealthChecker.new(@due_for_check_security)
+
+    @provider.expects(:fetch_security_price)
+      .with(
+        symbol: @due_for_check_security.ticker,
+        exchange_operating_mic: @due_for_check_security.exchange_operating_mic,
+        date: Date.current
+      )
+      .returns(provider_error_response(StandardError.new("No prices found")))
+      .times(Security::HealthChecker::MAX_CONSECUTIVE_FAILURES + 1)
+
+    Security::HealthChecker::MAX_CONSECUTIVE_FAILURES.times do
+      hc.run_check
+    end
+
+    refute @due_for_check_security.offline?
+    assert_equal 1, @due_for_check_security.prices.count
+
+    # We've now exceeded the max consecutive failures, so the security should be marked offline
+    hc.run_check
+    assert @due_for_check_security.offline?
+    assert_equal 0, @due_for_check_security.prices.count
+  end
+
+  test "failure incrementor increases for each health check failure" do
+    hc = Security::HealthChecker.new(@due_for_check_security)
+
+    @provider.expects(:fetch_security_price)
+      .with(
+        symbol: @due_for_check_security.ticker,
+        exchange_operating_mic: @due_for_check_security.exchange_operating_mic,
+        date: Date.current
+      )
+      .returns(provider_error_response(StandardError.new("No prices found")))
+      .twice
+
+    hc.run_check
+    assert_equal 1, @due_for_check_security.failed_fetch_count
+
+    hc.run_check
+    assert_equal 2, @due_for_check_security.failed_fetch_count
+  end
+
+  test "failure incrementor resets to 0 when health check succeeds" do
+    hc = Security::HealthChecker.new(@offline_never_checked_with_prices)
+
+    @provider.expects(:fetch_security_price)
+      .with(
+        symbol: @offline_never_checked_with_prices.ticker,
+        exchange_operating_mic: @offline_never_checked_with_prices.exchange_operating_mic,
+        date: Date.current
+      )
+      .returns(provider_success_response(OpenStruct.new(price: 100, date: Date.current, currency: "USD")))
+      .once
+
+    assert @offline_never_checked_with_prices.offline?
+
+    hc.run_check
+
+    refute @offline_never_checked_with_prices.offline?
+    assert_equal 0, @offline_never_checked_with_prices.failed_fetch_count
+    assert_nil @offline_never_checked_with_prices.failed_fetch_at
+  end
+end

test/models/security/price_test.rb

@@ -43,7 +43,7 @@ class Security::PriceTest < ActiveSupport::TestCase
     with_provider_response = provider_error_response(StandardError.new("Test error"))
 
     @provider.expects(:fetch_security_price)
-             .with(security, date: Date.current)
+             .with(symbol: security.ticker, exchange_operating_mic: security.exchange_operating_mic, date: Date.current)
              .returns(with_provider_response)
 
     assert_not @security.find_or_fetch_price(date: Date.current)
@@ -52,7 +52,7 @@ class Security::PriceTest < ActiveSupport::TestCase
   private
     def expect_provider_price(security:, price:, date:)
       @provider.expects(:fetch_security_price)
-               .with(security, date: date)
+               .with(symbol: security.ticker, exchange_operating_mic: security.exchange_operating_mic, date: date)
                .returns(provider_success_response(price))
     end
 

test/models/security/resolver_test.rb

@@ -0,0 +1,78 @@
+require "test_helper"
+
+class Security::ResolverTest < ActiveSupport::TestCase
+  setup do
+    @provider = mock
+    Security.stubs(:provider).returns(@provider)
+  end
+
+  test "resolves DB security" do
+    # Given an existing security in the DB that exactly matches the lookup params
+    db_security = Security.create!(ticker: "TSLA", exchange_operating_mic: "XNAS", country_code: "US")
+
+    # The resolver should return the DB record and never hit the provider
+    Security.expects(:search_provider).never
+
+    resolved = Security::Resolver.new("TSLA", exchange_operating_mic: "XNAS", country_code: "US").resolve
+
+    assert_equal db_security, resolved
+  end
+
+  test "resolves exact provider match" do
+    # Provider returns multiple results, one of which exactly matches symbol + exchange (and country)
+    exact_match = Security.new(ticker: "NVDA", exchange_operating_mic: "XNAS", country_code: "US")
+    near_miss   = Security.new(ticker: "NVDA", exchange_operating_mic: "XNYS", country_code: "US")
+
+    Security.expects(:search_provider)
+            .with("NVDA", exchange_operating_mic: "XNAS", country_code: "US")
+            .returns([ near_miss, exact_match ])
+
+    assert_difference "Security.count", 1 do
+      resolved = Security::Resolver.new("NVDA", exchange_operating_mic: "XNAS", country_code: "US").resolve
+
+      assert resolved.persisted?
+      assert_equal "NVDA", resolved.ticker
+      assert_equal "XNAS", resolved.exchange_operating_mic
+      assert_equal "US",   resolved.country_code
+      refute resolved.offline, "Exact provider matches should not be marked offline"
+    end
+  end
+
+  test "resolves close provider match" do
+    # No exact match – resolver should choose the most relevant close match based on exchange + country ranking
+    preferred = Security.new(ticker: "TEST1", exchange_operating_mic: "XNAS", country_code: "US")
+    other     = Security.new(ticker: "TEST2", exchange_operating_mic: "XNYS", country_code: "GB")
+
+    # Return in reverse-priority order to prove the sorter works
+    Security.expects(:search_provider)
+            .with("TEST", exchange_operating_mic: "XNAS")
+            .returns([ other, preferred ])
+
+    assert_difference "Security.count", 1 do
+      resolved = Security::Resolver.new("TEST", exchange_operating_mic: "XNAS").resolve
+
+      assert resolved.persisted?
+      assert_equal "TEST1", resolved.ticker
+      assert_equal "XNAS",  resolved.exchange_operating_mic
+      assert_equal "US",    resolved.country_code
+      refute resolved.offline, "Provider matches should not be marked offline"
+    end
+  end
+
+  test "resolves offline security" do
+    Security.expects(:search_provider).returns([])
+
+    assert_difference "Security.count", 1 do
+      resolved = Security::Resolver.new("FOO").resolve
+
+      assert resolved.persisted?, "Offline security should be saved"
+      assert_equal "FOO", resolved.ticker
+      assert resolved.offline, "Offline securities should be flagged offline"
+    end
+  end
+
+  test "returns nil when symbol blank" do
+    assert_raises(ArgumentError) { Security::Resolver.new(nil).resolve }
+    assert_raises(ArgumentError) { Security::Resolver.new("").resolve }
+  end
+end

test/models/trade_import_test.rb

@@ -11,23 +11,24 @@ class TradeImportTest < ActiveSupport::TestCase
   end
 
   test "imports trades and accounts" do
-    # Create an existing AAPL security with no exchange_operating_mic
-    aapl = Security.create!(ticker: "AAPL", exchange_operating_mic: nil)
+    aapl_resolver = mock
+    googl_resolver = mock
 
-    # We should only hit the provider for GOOGL since AAPL already exists
-    Security.expects(:search_provider).with(
-      "GOOGL",
-      exchange_operating_mic: "XNAS"
-    ).returns([
-      Security.new(
-        ticker: "GOOGL",
-        name: "Google Inc.",
-        country_code: "US",
-        exchange_mic: "XNGS",
-        exchange_operating_mic: "XNAS",
-        exchange_acronym: "NGS"
-      )
-    ]).once
+    Security::Resolver.expects(:new)
+                      .with("AAPL", exchange_operating_mic: nil)
+                      .returns(aapl_resolver)
+                      .once
+
+    Security::Resolver.expects(:new)
+                      .with("GOOGL", exchange_operating_mic: "XNAS")
+                      .returns(googl_resolver)
+                      .once
+
+    aapl = securities(:aapl)
+    googl = Security.create!(ticker: "GOOGL", exchange_operating_mic: "XNAS")
+
+    aapl_resolver.stubs(:resolve).returns(aapl)
+    googl_resolver.stubs(:resolve).returns(googl)
 
     import = <<~CSV
       date,ticker,qty,price,currency,account,name,exchange_operating_mic
@@ -55,19 +56,10 @@ class TradeImportTest < ActiveSupport::TestCase
 
     assert_difference -> { Entry.count } => 2,
                       -> { Trade.count } => 2,
-                      -> { Security.count } => 1,
                       -> { Account.count } => 1 do
       @import.publish
     end
 
     assert_equal "complete", @import.status
-
-    # Verify the securities were created/updated correctly
-    aapl.reload
-    assert_nil aapl.exchange_operating_mic
-
-    googl = Security.find_by(ticker: "GOOGL")
-    assert_equal "XNAS", googl.exchange_operating_mic
-    assert_equal "XNGS", googl.exchange_mic
   end
 end