From fa0248056dabb29693e190c6faedbdedc64b9d82 Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Fri, 28 Feb 2025 11:35:10 -0500 Subject: [PATCH] Show UI warning to user when they need provider data but have not setup Synth yet (#1926) * Simplify provider concerns * Update tests * Add UI warning for missing Synth key if family requires external data --- app/controllers/securities_controller.rb | 2 +- app/jobs/fetch_security_info_job.rb | 4 +- app/models/account/data_enricher.rb | 4 +- app/models/account/entry.rb | 2 +- app/models/account/entry/provided.rb | 11 ++++++ app/models/concerns/providable.rb | 35 ------------------ app/models/concerns/synthable.rb | 37 +++++++++++++++++++ app/models/exchange_rate/provided.rb | 15 ++++---- app/models/family.rb | 29 ++++++++------- app/models/security.rb | 17 +-------- app/models/security/price/provided.rb | 15 +++++--- app/models/security/provided.rb | 26 +++++++++++++ app/models/trade_import.rb | 17 ++------- app/models/upgrader/provided.rb | 5 ++- .../accounts/_account_sidebar_tabs.html.erb | 20 ++++++++++ .../configurations/_trade_import.html.erb | 5 +-- .../hostings/_synth_settings.html.erb | 9 ++++- test/models/exchange_rate_test.rb | 16 +++++--- test/models/security/price_test.rb | 17 ++++++--- test/models/trade_import_test.rb | 30 +++++---------- test/system/imports_test.rb | 2 + test/system/trades_test.rb | 2 +- 22 files changed, 184 insertions(+), 136 deletions(-) create mode 100644 app/models/account/entry/provided.rb delete mode 100644 app/models/concerns/providable.rb create mode 100644 app/models/concerns/synthable.rb create mode 100644 app/models/security/provided.rb diff --git a/app/controllers/securities_controller.rb b/app/controllers/securities_controller.rb index 5be3cbd9..6cfe9fd0 100644 --- a/app/controllers/securities_controller.rb +++ b/app/controllers/securities_controller.rb @@ -3,7 +3,7 @@ class SecuritiesController < ApplicationController query = params[:q] return render json: [] if query.blank? || query.length < 2 || query.length > 100 - @securities = Security.search({ + @securities = Security.search_provider({ search: query, country: params[:country_code] == "US" ? "US" : nil }) diff --git a/app/jobs/fetch_security_info_job.rb b/app/jobs/fetch_security_info_job.rb index aa64c169..484a47e1 100644 --- a/app/jobs/fetch_security_info_job.rb +++ b/app/jobs/fetch_security_info_job.rb @@ -2,7 +2,7 @@ class FetchSecurityInfoJob < ApplicationJob queue_as :latency_low def perform(security_id) - return unless Security.security_info_provider.present? + return unless Security.provider.present? security = Security.find(security_id) @@ -12,7 +12,7 @@ class FetchSecurityInfoJob < ApplicationJob params[:mic_code] = security.exchange_mic if security.exchange_mic.present? params[:operating_mic] = security.exchange_operating_mic if security.exchange_operating_mic.present? - security_info_response = Security.security_info_provider.fetch_security_info(**params) + security_info_response = Security.provider.fetch_security_info(**params) security.update( name: security_info_response.info.dig("name") diff --git a/app/models/account/data_enricher.rb b/app/models/account/data_enricher.rb index 59979df0..8d07eff8 100644 --- a/app/models/account/data_enricher.rb +++ b/app/models/account/data_enricher.rb @@ -1,6 +1,4 @@ class Account::DataEnricher - include Providable - attr_reader :account def initialize(account) @@ -37,7 +35,7 @@ class Account::DataEnricher candidates.each do |entry| begin - info = self.class.synth_provider.enrich_transaction(entry.name).info + info = entry.fetch_enrichment_info next unless info.present? diff --git a/app/models/account/entry.rb b/app/models/account/entry.rb index b53db19b..25065efd 100644 --- a/app/models/account/entry.rb +++ b/app/models/account/entry.rb @@ -1,5 +1,5 @@ class Account::Entry < ApplicationRecord - include Monetizable + include Monetizable, Provided monetize :amount diff --git a/app/models/account/entry/provided.rb b/app/models/account/entry/provided.rb new file mode 100644 index 00000000..c18654c9 --- /dev/null +++ b/app/models/account/entry/provided.rb @@ -0,0 +1,11 @@ +module Account::Entry::Provided + extend ActiveSupport::Concern + + include Synthable + + def fetch_enrichment_info + return nil unless synth_client.present? + + synth_client.enrich_transaction(name).info + end +end diff --git a/app/models/concerns/providable.rb b/app/models/concerns/providable.rb deleted file mode 100644 index 996efff8..00000000 --- a/app/models/concerns/providable.rb +++ /dev/null @@ -1,35 +0,0 @@ -# `Providable` serves as an extension point for integrating multiple providers. -# For an example of a multi-provider, multi-concept implementation, -# see: https://github.com/maybe-finance/maybe/pull/561 - -module Providable - extend ActiveSupport::Concern - - class_methods do - def security_prices_provider - synth_provider - end - - def security_info_provider - synth_provider - end - - def exchange_rates_provider - synth_provider - end - - def git_repository_provider - Provider::Github.new - end - - def synth_provider - api_key = self_hosted? ? Setting.synth_api_key : ENV["SYNTH_API_KEY"] - api_key.present? ? Provider::Synth.new(api_key) : nil - end - - private - def self_hosted? - Rails.application.config.app_mode.self_hosted? - end - end -end diff --git a/app/models/concerns/synthable.rb b/app/models/concerns/synthable.rb new file mode 100644 index 00000000..51adcade --- /dev/null +++ b/app/models/concerns/synthable.rb @@ -0,0 +1,37 @@ +module Synthable + extend ActiveSupport::Concern + + class_methods do + def synth_usage + synth_client&.usage + end + + def synth_overage? + synth_usage&.usage&.utilization.to_i >= 100 + end + + def synth_healthy? + synth_client&.healthy? + end + + def synth_client + api_key = ENV.fetch("SYNTH_API_KEY", Setting.synth_api_key) + + return nil unless api_key.present? + + Provider::Synth.new(api_key) + end + end + + def synth_client + self.class.synth_client + end + + def synth_usage + self.class.synth_usage + end + + def synth_overage? + self.class.synth_overage? + end +end diff --git a/app/models/exchange_rate/provided.rb b/app/models/exchange_rate/provided.rb index d1e2aea2..d010ff98 100644 --- a/app/models/exchange_rate/provided.rb +++ b/app/models/exchange_rate/provided.rb @@ -1,19 +1,18 @@ module ExchangeRate::Provided extend ActiveSupport::Concern - include Providable + include Synthable class_methods do - def provider_healthy? - exchange_rates_provider.present? && exchange_rates_provider.healthy? + def provider + synth_client end private - def fetch_rates_from_provider(from:, to:, start_date:, end_date: Date.current, cache: false) - return [] unless exchange_rates_provider.present? + return [] unless provider.present? - response = exchange_rates_provider.fetch_exchange_rates \ + response = provider.fetch_exchange_rates \ from: from, to: to, start_date: start_date, @@ -38,9 +37,9 @@ module ExchangeRate::Provided end def fetch_rate_from_provider(from:, to:, date:, cache: false) - return nil unless exchange_rates_provider.present? + return nil unless provider.present? - response = exchange_rates_provider.fetch_exchange_rate \ + response = provider.fetch_exchange_rate \ from: from, to: to, date: date diff --git a/app/models/family.rb b/app/models/family.rb index ff2c8b07..ffb14a7d 100644 --- a/app/models/family.rb +++ b/app/models/family.rb @@ -1,5 +1,5 @@ class Family < ApplicationRecord - include Providable, Plaidable, Syncable, AutoTransferMatchable + include Synthable, Plaidable, Syncable, AutoTransferMatchable DATE_FORMATS = [ [ "MM-DD-YYYY", "%m-%d-%Y" ], @@ -92,22 +92,25 @@ class Family < ApplicationRecord ).link_token end - def synth_usage - self.class.synth_provider&.usage - end - - def synth_overage? - self.class.synth_provider&.usage&.utilization.to_i >= 100 - end - - def synth_valid? - self.class.synth_provider&.healthy? - end - def subscribed? stripe_subscription_status == "active" end + def requires_data_provider? + # If family has any trades, they need a provider for historical prices + return true if trades.any? + + # If family has any accounts not denominated in the family's currency, they need a provider for historical exchange rates + return true if accounts.where.not(currency: self.currency).any? + + # If family has any entries in different currencies, they need a provider for historical exchange rates + uniq_currencies = entries.pluck(:currency).uniq + return true if uniq_currencies.count > 1 + return true if uniq_currencies.first != self.currency + + false + end + def primary_user users.order(:created_at).first end diff --git a/app/models/security.rb b/app/models/security.rb index 991ac202..6d94c798 100644 --- a/app/models/security.rb +++ b/app/models/security.rb @@ -1,5 +1,5 @@ class Security < ApplicationRecord - include Providable + include Provided before_save :upcase_ticker @@ -9,21 +9,6 @@ class Security < ApplicationRecord validates :ticker, presence: true validates :ticker, uniqueness: { scope: :exchange_operating_mic, case_sensitive: false } - class << self - def provider - security_prices_provider - end - - def search(query) - security_prices_provider.search_securities( - query: query[:search], - dataset: "limited", - country_code: query[:country], - exchange_operating_mic: query[:exchange_operating_mic] - ).securities.map { |attrs| new(**attrs) } - end - end - def current_price @current_price ||= Security::Price.find_price(security: self, date: Date.current) return nil if @current_price.nil? diff --git a/app/models/security/price/provided.rb b/app/models/security/price/provided.rb index e2a99774..aed56702 100644 --- a/app/models/security/price/provided.rb +++ b/app/models/security/price/provided.rb @@ -1,16 +1,19 @@ module Security::Price::Provided extend ActiveSupport::Concern - include Providable + include Synthable class_methods do - private + def provider + synth_client + end + private def fetch_price_from_provider(security:, date:, cache: false) - return nil unless security_prices_provider.present? + return nil unless provider.present? return nil unless security.has_prices? - response = security_prices_provider.fetch_security_prices \ + response = provider.fetch_security_prices \ ticker: security.ticker, mic_code: security.exchange_mic, start_date: date, @@ -31,11 +34,11 @@ module Security::Price::Provided end def fetch_prices_from_provider(security:, start_date:, end_date:, cache: false) - return [] unless security_prices_provider.present? + return [] unless provider.present? return [] unless security return [] unless security.has_prices? - response = security_prices_provider.fetch_security_prices \ + response = provider.fetch_security_prices \ ticker: security.ticker, mic_code: security.exchange_mic, start_date: start_date, diff --git a/app/models/security/provided.rb b/app/models/security/provided.rb new file mode 100644 index 00000000..4a4fd6a5 --- /dev/null +++ b/app/models/security/provided.rb @@ -0,0 +1,26 @@ +module Security::Provided + extend ActiveSupport::Concern + + include Synthable + + class_methods do + def provider + synth_client + end + + def search_provider(query) + response = provider.search_securities( + query: query[:search], + dataset: "limited", + country_code: query[:country], + exchange_operating_mic: query[:exchange_operating_mic] + ) + + if response.success? + response.securities.map { |attrs| new(**attrs) } + else + [] + end + end + end +end diff --git a/app/models/trade_import.rb b/app/models/trade_import.rb index ddaad904..549c9093 100644 --- a/app/models/trade_import.rb +++ b/app/models/trade_import.rb @@ -75,10 +75,7 @@ class TradeImport < Import return internal_security if internal_security.present? # If security prices provider isn't properly configured or available, create with nil exchange_operating_mic - provider = Security.security_prices_provider - unless provider.present? && provider.respond_to?(:search_securities) - return Security.find_or_create_by!(ticker: ticker, exchange_operating_mic: nil) - end + return Security.find_or_create_by!(ticker: ticker, exchange_operating_mic: nil) unless Security.provider.present? # Cache provider responses so that when we're looping through rows and importing, # we only hit our provider for the unique combinations of ticker / exchange_operating_mic @@ -86,18 +83,10 @@ class TradeImport < Import @provider_securities_cache ||= {} provider_security = @provider_securities_cache[cache_key] ||= begin - response = provider.search_securities( + Security.search_provider( query: ticker, exchange_operating_mic: exchange_operating_mic - ) - - if !response || !response.success? || !response.securities || response.securities.empty? - nil - else - response.securities.first - end - rescue => e - nil + ).first end return Security.find_or_create_by!(ticker: ticker, exchange_operating_mic: nil) if provider_security.nil? diff --git a/app/models/upgrader/provided.rb b/app/models/upgrader/provided.rb index 4b518e51..fc1e65b7 100644 --- a/app/models/upgrader/provided.rb +++ b/app/models/upgrader/provided.rb @@ -1,11 +1,14 @@ module Upgrader::Provided extend ActiveSupport::Concern - include Providable class_methods do private def fetch_latest_upgrade_candidates_from_provider git_repository_provider.fetch_latest_upgrade_candidates end + + def git_repository_provider + Provider::Github.new + end end end diff --git a/app/views/accounts/_account_sidebar_tabs.html.erb b/app/views/accounts/_account_sidebar_tabs.html.erb index d42fab86..bb409492 100644 --- a/app/views/accounts/_account_sidebar_tabs.html.erb +++ b/app/views/accounts/_account_sidebar_tabs.html.erb @@ -1,5 +1,25 @@ <%# locals: (family:) %> +<% if family.requires_data_provider? && family.synth_client.nil? %> +
+ +
+ <%= icon "triangle-alert", size: "sm" %> +

Missing historical data

+
+ + <%= lucide_icon "chevron-down", class: "text-yellow-600 group-open:transform group-open:rotate-180 w-5" %> + +
+

Maybe uses Synth API to fetch historical exchange rates, security prices, and more. This data is required to calculate accurate historical account balances.

+ +

+ <%= link_to "Add your Synth API key here.", settings_hosting_path, class: "text-yellow-600 underline" %> +

+
+
+<% end %> +
<%= form.select :name_col_label, import.csv_headers, { include_blank: "Leave empty", label: "Name (optional)" } %> - <% if Security.security_prices_provider.nil? %> + <% unless Security.provider %>

- Note: The Synth provider is not configured. Exchange validation is disabled. - Securities will be created without exchange validation, and price history will not be available. + Note: The security prices provider is not configured. Your trade imports will work, but Maybe will not backfill price history. Please go to your settings to configure this.

<% end %> diff --git a/app/views/settings/hostings/_synth_settings.html.erb b/app/views/settings/hostings/_synth_settings.html.erb index 9a4e9dd4..4ee9c182 100644 --- a/app/views/settings/hostings/_synth_settings.html.erb +++ b/app/views/settings/hostings/_synth_settings.html.erb @@ -1,7 +1,11 @@

<%= t(".title") %>

-

<%= t(".description") %>

+ <% if ENV["SYNTH_API_KEY"].present? %> +

You have successfully configured your Synth API key through the SYNTH_API_KEY environment variable.

+ <% else %> +

<%= t(".description") %>

+ <% end %>
<%= styled_form_with model: Setting.new, @@ -15,7 +19,8 @@ label: t(".label"), type: "password", placeholder: t(".placeholder"), - value: Setting.synth_api_key, + value: ENV.fetch("SYNTH_API_KEY", Setting.synth_api_key), + disabled: ENV["SYNTH_API_KEY"].present?, container_class: @synth_usage.present? && !@synth_usage.success? ? "border-red-500" : "", data: { "auto-submit-form-target": "auto" } %> <% end %> diff --git a/test/models/exchange_rate_test.rb b/test/models/exchange_rate_test.rb index 1a705a5b..7bc7ba61 100644 --- a/test/models/exchange_rate_test.rb +++ b/test/models/exchange_rate_test.rb @@ -5,14 +5,16 @@ class ExchangeRateTest < ActiveSupport::TestCase setup do @provider = mock - ExchangeRate.stubs(:exchange_rates_provider).returns(@provider) + ExchangeRate.stubs(:provider).returns(@provider) end test "exchange rate provider nil if no api key configured" do - ExchangeRate.unstub(:exchange_rates_provider) + ExchangeRate.unstub(:provider) + + Setting.stubs(:synth_api_key).returns(nil) with_env_overrides SYNTH_API_KEY: nil do - assert_not ExchangeRate.exchange_rates_provider + assert_not ExchangeRate.provider end end @@ -42,7 +44,9 @@ class ExchangeRateTest < ActiveSupport::TestCase end test "nil if rate is not found in DB and provider is disabled" do - ExchangeRate.unstub(:exchange_rates_provider) + ExchangeRate.unstub(:provider) + + Setting.stubs(:synth_api_key).returns(nil) with_env_overrides SYNTH_API_KEY: nil do assert_not ExchangeRate.find_rate(from: "USD", to: "EUR", date: Date.current) @@ -102,7 +106,9 @@ class ExchangeRateTest < ActiveSupport::TestCase end test "returns empty array if no rates found in DB or provider" do - ExchangeRate.unstub(:exchange_rates_provider) + ExchangeRate.unstub(:provider) + + Setting.stubs(:synth_api_key).returns(nil) with_env_overrides SYNTH_API_KEY: nil do assert_equal [], ExchangeRate.find_rates(from: "USD", to: "JPY", start_date: 10.days.ago.to_date) diff --git a/test/models/security/price_test.rb b/test/models/security/price_test.rb index d5021242..66b60469 100644 --- a/test/models/security/price_test.rb +++ b/test/models/security/price_test.rb @@ -5,14 +5,16 @@ class Security::PriceTest < ActiveSupport::TestCase setup do @provider = mock - Security::Price.stubs(:security_prices_provider).returns(@provider) + Security::Price.stubs(:provider).returns(@provider) end test "security price provider nil if no api key provided" do - Security::Price.unstub(:security_prices_provider) + Security::Price.unstub(:provider) + + Setting.stubs(:synth_api_key).returns(nil) with_env_overrides SYNTH_API_KEY: nil do - assert_not Security::Price.security_prices_provider + assert_not Security::Price.provider end end @@ -60,7 +62,10 @@ class Security::PriceTest < ActiveSupport::TestCase end test "returns nil if price not found in DB and provider disabled" do - Security::Price.unstub(:security_prices_provider) + Security::Price.unstub(:provider) + + Setting.stubs(:synth_api_key).returns(nil) + security = Security.new(ticker: "NVDA") with_env_overrides SYNTH_API_KEY: nil do @@ -105,7 +110,9 @@ class Security::PriceTest < ActiveSupport::TestCase end test "returns empty array if no prices found in DB or from provider" do - Security::Price.unstub(:security_prices_provider) + Security::Price.unstub(:provider) + + Setting.stubs(:synth_api_key).returns(nil) with_env_overrides SYNTH_API_KEY: nil do assert_equal [], Security::Price.find_prices(security: Security.new(ticker: "NVDA"), start_date: 10.days.ago.to_date, end_date: Date.current) diff --git a/test/models/trade_import_test.rb b/test/models/trade_import_test.rb index 2900034e..f0ee3e68 100644 --- a/test/models/trade_import_test.rb +++ b/test/models/trade_import_test.rb @@ -12,30 +12,20 @@ class TradeImportTest < ActiveSupport::TestCase # Create an existing AAPL security with no exchange_operating_mic aapl = Security.create!(ticker: "AAPL", exchange_operating_mic: nil) - provider = mock - # We should only hit the provider for GOOGL since AAPL already exists - provider.expects(:search_securities).with( + Security.expects(:search_provider).with( query: "GOOGL", exchange_operating_mic: "XNAS" - ).returns( - OpenStruct.new( - securities: [ - { - ticker: "GOOGL", - name: "Google Inc.", - country_code: "US", - exchange_mic: "XNGS", - exchange_operating_mic: "XNAS", - exchange_acronym: "NGS" - } - ], - success?: true, - raw_response: nil + ).returns([ + Security.new( + ticker: "GOOGL", + name: "Google Inc.", + country_code: "US", + exchange_mic: "XNGS", + exchange_operating_mic: "XNAS", + exchange_acronym: "NGS" ) - ).once - - Security.stubs(:security_prices_provider).returns(provider) + ]).once import = <<~CSV date,ticker,qty,price,currency,account,name,exchange_operating_mic diff --git a/test/system/imports_test.rb b/test/system/imports_test.rb index 5c936f93..85c4707c 100644 --- a/test/system/imports_test.rb +++ b/test/system/imports_test.rb @@ -52,6 +52,8 @@ class ImportsTest < ApplicationSystemTestCase end test "trade import" do + Security.stubs(:search_provider).returns([]) + visit new_import_path click_on "Import investments" diff --git a/test/system/trades_test.rb b/test/system/trades_test.rb index ab0c6109..a1f19139 100644 --- a/test/system/trades_test.rb +++ b/test/system/trades_test.rb @@ -10,7 +10,7 @@ class TradesTest < ApplicationSystemTestCase visit_account_portfolio - Security.stubs(:search).returns([ + Security.stubs(:search_provider).returns([ Security.new( ticker: "AAPL", name: "Apple Inc.",