From bf695972e46003dc9bd1df748bac85134acde574 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Tue, 29 Oct 2024 14:55:46 -0400 Subject: [PATCH] Remove missing prices issue (#1390) --- app/models/account/holding/syncer.rb | 2 - app/models/concerns/issuable.rb | 6 -- app/models/issue/prices_missing.rb | 33 ---------- ...41029184115_remove_prices_missing_issue.rb | 9 +++ db/schema.rb | 4 +- test/models/account/holding/syncer_test.rb | 2 - test/models/account_test.rb | 8 --- test/models/issue/prices_missing_test.rb | 61 ------------------- 8 files changed, 11 insertions(+), 114 deletions(-) delete mode 100644 app/models/issue/prices_missing.rb create mode 100644 db/migrate/20241029184115_remove_prices_missing_issue.rb delete mode 100644 test/models/issue/prices_missing_test.rb diff --git a/app/models/account/holding/syncer.rb b/app/models/account/holding/syncer.rb index 5dfa7644..9db844f0 100644 --- a/app/models/account/holding/syncer.rb +++ b/app/models/account/holding/syncer.rb @@ -68,8 +68,6 @@ class Account::Holding::Syncer price = get_cached_price(ticker, date) || trade_price - account.observe_missing_price(ticker:, date:) unless price - account.holdings.build \ date: date, security_id: holding[:security_id], diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 8ffe87d3..f295332f 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -33,12 +33,6 @@ module Issuable ) end - def observe_missing_price(ticker:, date:) - issue = issues.find_or_create_by(type: Issue::PricesMissing.name, resolved_at: nil) - issue.append_missing_price(ticker, date) - issue.save! - end - def highest_priority_issue issues.active.ordered.first end diff --git a/app/models/issue/prices_missing.rb b/app/models/issue/prices_missing.rb deleted file mode 100644 index 6218a7de..00000000 --- a/app/models/issue/prices_missing.rb +++ /dev/null @@ -1,33 +0,0 @@ -class Issue::PricesMissing < Issue - store_accessor :data, :missing_prices - - after_initialize :initialize_missing_prices - - validates :missing_prices, presence: true, allow_blank: true - - def append_missing_price(ticker, date) - missing_prices[ticker] ||= [] - missing_prices[ticker] << date unless missing_prices[ticker].include?(date.to_s) - end - - def stale? - stale = true - - missing_prices.each do |ticker, dates| - next unless issuable.owns_ticker?(ticker) - - oldest_date = dates.min.to_date - expected_price_count = (oldest_date..Date.current).count - prices = Security::Price.find_prices(ticker: ticker, start_date: oldest_date) - stale = false if prices.count < expected_price_count - end - - stale - end - - private - - def initialize_missing_prices - self.missing_prices ||= {} - end -end diff --git a/db/migrate/20241029184115_remove_prices_missing_issue.rb b/db/migrate/20241029184115_remove_prices_missing_issue.rb new file mode 100644 index 00000000..535698c6 --- /dev/null +++ b/db/migrate/20241029184115_remove_prices_missing_issue.rb @@ -0,0 +1,9 @@ +class RemovePricesMissingIssue < ActiveRecord::Migration[7.2] + def up + execute "DELETE FROM issues WHERE type = 'Issue::PricesMissing'" + end + + def down + # Cannot restore deleted issues since we don't have the original data + end +end diff --git a/db/schema.rb b/db/schema.rb index 5b2b4310..119c1b1d 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.2].define(version: 2024_10_25_182612) do +ActiveRecord::Schema[7.2].define(version: 2024_10_29_184115) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" enable_extension "plpgsql" @@ -119,7 +119,7 @@ ActiveRecord::Schema[7.2].define(version: 2024_10_25_182612) do t.boolean "is_active", default: true, null: false t.date "last_sync_date" t.uuid "institution_id" - t.virtual "classification", type: :string, as: "\nCASE\n WHEN ((accountable_type)::text = ANY (ARRAY[('Loan'::character varying)::text, ('CreditCard'::character varying)::text, ('OtherLiability'::character varying)::text])) THEN 'liability'::text\n ELSE 'asset'::text\nEND", stored: true + t.virtual "classification", type: :string, as: "\nCASE\n WHEN ((accountable_type)::text = ANY ((ARRAY['Loan'::character varying, 'CreditCard'::character varying, 'OtherLiability'::character varying])::text[])) THEN 'liability'::text\n ELSE 'asset'::text\nEND", stored: true t.uuid "import_id" t.string "mode" t.index ["accountable_id", "accountable_type"], name: "index_accounts_on_accountable_id_and_accountable_type" diff --git a/test/models/account/holding/syncer_test.rb b/test/models/account/holding/syncer_test.rb index 1b422cb9..a0c63fd6 100644 --- a/test/models/account/holding/syncer_test.rb +++ b/test/models/account/holding/syncer_test.rb @@ -85,8 +85,6 @@ class Account::Holding::SyncerTest < ActiveSupport::TestCase .once .returns(fetched_prices) - @account.expects(:observe_missing_price).with(ticker: "AMZN", date: Date.current).once - run_sync_for(@account) assert_holdings(expected) diff --git a/test/models/account_test.rb b/test/models/account_test.rb index 18346e15..3da01860 100644 --- a/test/models/account_test.rb +++ b/test/models/account_test.rb @@ -89,12 +89,4 @@ class AccountTest < ActiveSupport::TestCase assert_equal 10, account.holding_qty(security, date: 1.day.ago.to_date) assert_equal 0, account.holding_qty(security, date: 2.days.ago.to_date) end - - test "can observe missing price" do - account = accounts(:investment) - - assert_difference -> { account.issues.count } do - account.observe_missing_price(ticker: "AAPL", date: Date.current) - end - end end diff --git a/test/models/issue/prices_missing_test.rb b/test/models/issue/prices_missing_test.rb deleted file mode 100644 index 5c6a5502..00000000 --- a/test/models/issue/prices_missing_test.rb +++ /dev/null @@ -1,61 +0,0 @@ -require "test_helper" - -class Issue::PricesMissingTest < ActiveSupport::TestCase - setup do - @issue = Issue::PricesMissing.new - @account = Account.new(id: "123abc") - @issue.issuable = @account - end - - test "stale? returns false when no prices are found" do - @issue.append_missing_price("AAPL", (Date.current - 5.days).to_s) - - Security::Price.expects(:find_prices).returns([]) - @account.expects(:owns_ticker?).with("AAPL").returns(true) - - assert_not @issue.stale? - end - - test "stale? returns true when all expected prices are found" do - start_date = Date.current - 5.days - @issue.append_missing_price("AAPL", start_date.to_s) - - expected_prices = (start_date..Date.current).map { |date| { date: date } } - Security::Price.expects(:find_prices).returns(expected_prices) - @account.expects(:owns_ticker?).with("AAPL").returns(true) - - assert @issue.stale? - end - - test "stale? returns false when some prices are missing" do - start_date = Date.current - 5.days - @issue.append_missing_price("AAPL", start_date.to_s) - - incomplete_prices = (start_date..Date.current - 2.days).map { |date| { date: date } } - Security::Price.expects(:find_prices).returns(incomplete_prices) - @account.expects(:owns_ticker?).with("AAPL").returns(true) - - assert_not @issue.stale? - end - - test "stale? returns true when the account doesn't own the ticker" do - @issue.append_missing_price("AAPL", (Date.current - 5.days).to_s) - - @account.expects(:owns_ticker?).with("AAPL").returns(false) - - assert @issue.stale? - end - - test "stale? handles multiple tickers correctly" do - @issue.append_missing_price("AAPL", (Date.current - 5.days).to_s) - @issue.append_missing_price("GOOGL", (Date.current - 3.days).to_s) - - @account.expects(:owns_ticker?).with("AAPL").returns(true) - @account.expects(:owns_ticker?).with("GOOGL").returns(true) - - Security::Price.expects(:find_prices).with(ticker: "AAPL", start_date: Date.current - 5.days).returns([]) - Security::Price.expects(:find_prices).with(ticker: "GOOGL", start_date: Date.current - 3.days).returns([]) - - assert_not @issue.stale? - end -end