diff --git a/Gemfile.lock b/Gemfile.lock index 8b6d1b77e..915cf08fe 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -441,7 +441,7 @@ GEM ostruct (0.6.2) pagy (9.3.5) parallel (1.27.0) - parser (3.3.8.0) + parser (3.3.10.2) ast (~> 2.4.1) racc pdf-reader (2.15.1) diff --git a/app/controllers/holdings_controller.rb b/app/controllers/holdings_controller.rb index f8ba811cc..24f7c9726 100644 --- a/app/controllers/holdings_controller.rb +++ b/app/controllers/holdings_controller.rb @@ -1,11 +1,12 @@ class HoldingsController < ApplicationController - before_action :set_holding, only: %i[show update destroy unlock_cost_basis remap_security reset_security] + before_action :set_holding, only: %i[show update destroy unlock_cost_basis remap_security reset_security sync_prices] def index @account = Current.family.accounts.find(params[:account_id]) end def show + @last_price_updated = @holding.security.prices.maximum(:updated_at) end def update @@ -70,6 +71,13 @@ class HoldingsController < ApplicationController return end + # The user explicitly selected this security from provider search results, + # so we know the provider can handle it. Bring it back online if it was + # previously marked offline (e.g. by a failed QIF import resolution). + if new_security.offline? + new_security.update!(offline: false, failed_fetch_count: 0, failed_fetch_at: nil) + end + @holding.remap_security!(new_security) flash[:notice] = t(".success") @@ -79,6 +87,44 @@ class HoldingsController < ApplicationController end end + def sync_prices + security = @holding.security + + if security.offline? + redirect_to account_path(@holding.account, tab: "holdings"), + alert: t("holdings.sync_prices.unavailable") + return + end + + prices_updated, @provider_error = security.import_provider_prices( + start_date: 31.days.ago.to_date, + end_date: Date.current, + clear_cache: true + ) + security.import_provider_details + + @last_price_updated = @holding.security.prices.maximum(:updated_at) + + if prices_updated == 0 + @provider_error = @provider_error.presence || t("holdings.sync_prices.provider_error") + respond_to do |format| + format.html { redirect_to account_path(@holding.account, tab: "holdings"), alert: @provider_error } + format.turbo_stream + end + return + end + + strategy = @holding.account.linked? ? :reverse : :forward + Balance::Materializer.new(@holding.account, strategy: strategy, security_ids: [ @holding.security_id ]).materialize_balances + @holding.reload + @last_price_updated = @holding.security.prices.maximum(:updated_at) + + respond_to do |format| + format.html { redirect_to account_path(@holding.account, tab: "holdings"), notice: t("holdings.sync_prices.success") } + format.turbo_stream + end + end + def reset_security @holding.reset_security_to_provider! flash[:notice] = t(".success") diff --git a/app/models/balance/materializer.rb b/app/models/balance/materializer.rb index c6501ffa1..e4715c021 100644 --- a/app/models/balance/materializer.rb +++ b/app/models/balance/materializer.rb @@ -1,9 +1,10 @@ class Balance::Materializer - attr_reader :account, :strategy + attr_reader :account, :strategy, :security_ids - def initialize(account, strategy:) + def initialize(account, strategy:, security_ids: nil) @account = account @strategy = strategy + @security_ids = security_ids end def materialize_balances @@ -24,7 +25,7 @@ class Balance::Materializer private def materialize_holdings - @holdings = Holding::Materializer.new(account, strategy: strategy).materialize_holdings + @holdings = Holding::Materializer.new(account, strategy: strategy, security_ids: security_ids).materialize_holdings end def update_account_info diff --git a/app/models/holding/forward_calculator.rb b/app/models/holding/forward_calculator.rb index ce490acba..ecb59e826 100644 --- a/app/models/holding/forward_calculator.rb +++ b/app/models/holding/forward_calculator.rb @@ -1,8 +1,9 @@ class Holding::ForwardCalculator attr_reader :account - def initialize(account) + def initialize(account, security_ids: nil) @account = account + @security_ids = security_ids # Track cost basis per security: { security_id => { total_cost: BigDecimal, total_qty: BigDecimal } } @cost_basis_tracker = Hash.new { |h, k| h[k] = { total_cost: BigDecimal("0"), total_qty: BigDecimal("0") } } end @@ -27,7 +28,7 @@ class Holding::ForwardCalculator private def portfolio_cache - @portfolio_cache ||= Holding::PortfolioCache.new(account) + @portfolio_cache ||= Holding::PortfolioCache.new(account, security_ids: @security_ids) end def empty_portfolio @@ -55,6 +56,8 @@ class Holding::ForwardCalculator def build_holdings(portfolio, date, price_source: nil) portfolio.map do |security_id, qty| + next if @security_ids && !@security_ids.include?(security_id) + price = portfolio_cache.get_price(security_id, date, source: price_source) if price.nil? diff --git a/app/models/holding/materializer.rb b/app/models/holding/materializer.rb index 7359a2099..582bc743b 100644 --- a/app/models/holding/materializer.rb +++ b/app/models/holding/materializer.rb @@ -1,9 +1,10 @@ # "Materializes" holdings (similar to a DB materialized view, but done at the app level) # into a series of records we can easily query and join with other data. class Holding::Materializer - def initialize(account, strategy:) + def initialize(account, strategy:, security_ids: nil) @account = account @strategy = strategy + @security_ids = security_ids end def materialize_holdings @@ -12,7 +13,7 @@ class Holding::Materializer Rails.logger.info("Persisting #{@holdings.size} holdings") persist_holdings - if strategy == :forward + if strategy == :forward && security_ids.nil? purge_stale_holdings end @@ -28,7 +29,7 @@ class Holding::Materializer end private - attr_reader :account, :strategy + attr_reader :account, :strategy, :security_ids def calculate_holdings @holdings = calculator.calculate @@ -164,9 +165,9 @@ class Holding::Materializer def calculator if strategy == :reverse portfolio_snapshot = Holding::PortfolioSnapshot.new(account) - Holding::ReverseCalculator.new(account, portfolio_snapshot: portfolio_snapshot) + Holding::ReverseCalculator.new(account, portfolio_snapshot: portfolio_snapshot, security_ids: security_ids) else - Holding::ForwardCalculator.new(account) + Holding::ForwardCalculator.new(account, security_ids: security_ids) end end end diff --git a/app/models/holding/portfolio_cache.rb b/app/models/holding/portfolio_cache.rb index 9ffed15b4..6763d1fd1 100644 --- a/app/models/holding/portfolio_cache.rb +++ b/app/models/holding/portfolio_cache.rb @@ -7,9 +7,10 @@ class Holding::PortfolioCache end end - def initialize(account, use_holdings: false) + def initialize(account, use_holdings: false, security_ids: nil) @account = account @use_holdings = use_holdings + @security_ids = security_ids load_prices end @@ -62,10 +63,12 @@ class Holding::PortfolioCache def collect_unique_securities unique_securities_from_trades = trades.map(&:entryable).map(&:security).uniq + unique_securities_from_trades = unique_securities_from_trades.select { |s| @security_ids.include?(s.id) } if @security_ids return unique_securities_from_trades unless use_holdings unique_securities_from_holdings = holdings.map(&:security).uniq + unique_securities_from_holdings = unique_securities_from_holdings.select { |s| @security_ids.include?(s.id) } if @security_ids (unique_securities_from_trades + unique_securities_from_holdings).uniq end diff --git a/app/models/holding/reverse_calculator.rb b/app/models/holding/reverse_calculator.rb index 2a4ea0375..d9ed2efe0 100644 --- a/app/models/holding/reverse_calculator.rb +++ b/app/models/holding/reverse_calculator.rb @@ -1,9 +1,10 @@ class Holding::ReverseCalculator attr_reader :account, :portfolio_snapshot - def initialize(account, portfolio_snapshot:) + def initialize(account, portfolio_snapshot:, security_ids: nil) @account = account @portfolio_snapshot = portfolio_snapshot + @security_ids = security_ids end def calculate @@ -19,7 +20,7 @@ class Holding::ReverseCalculator # since it is common for a provider to supply "current day" holdings but not all the historical # trades that make up those holdings. def portfolio_cache - @portfolio_cache ||= Holding::PortfolioCache.new(account, use_holdings: true) + @portfolio_cache ||= Holding::PortfolioCache.new(account, use_holdings: true, security_ids: @security_ids) end def calculate_holdings @@ -57,6 +58,8 @@ class Holding::ReverseCalculator def build_holdings(portfolio, date, price_source: nil) portfolio.map do |security_id, qty| + next if @security_ids && !@security_ids.include?(security_id) + price = portfolio_cache.get_price(security_id, date, source: price_source) if price.nil? diff --git a/app/models/security/price/importer.rb b/app/models/security/price/importer.rb index bc5840c0c..9d57332b6 100644 --- a/app/models/security/price/importer.rb +++ b/app/models/security/price/importer.rb @@ -4,6 +4,8 @@ class Security::Price::Importer PROVISIONAL_LOOKBACK_DAYS = 7 + attr_reader :provider_error + def initialize(security:, security_provider:, start_date:, end_date:, clear_cache: false) @security = security @security_provider = security_provider @@ -130,6 +132,7 @@ class Security::Price::Importer scope.set_context("security", { id: security.id, start_date: start_date, end_date: end_date }) end + @provider_error = error_message {} end end diff --git a/app/models/security/provided.rb b/app/models/security/provided.rb index b80046483..e412244a9 100644 --- a/app/models/security/provided.rb +++ b/app/models/security/provided.rb @@ -114,13 +114,14 @@ module Security::Provided return 0 end - Security::Price::Importer.new( + importer = Security::Price::Importer.new( security: self, security_provider: provider, start_date: start_date, end_date: end_date, clear_cache: clear_cache - ).import_provider_prices + ) + [ importer.import_provider_prices, importer.provider_error ] end private diff --git a/app/views/holdings/show.html.erb b/app/views/holdings/show.html.erb index 927d0b9a2..c94582471 100644 --- a/app/views/holdings/show.html.erb +++ b/app/views/holdings/show.html.erb @@ -35,7 +35,7 @@ @@ -66,7 +66,7 @@
<%= t(".current_market_price_label") %>
-
+
<% begin %> <%= @holding.security.current_price ? format_money(@holding.security.current_price) : t(".unknown") %> <% rescue ActiveRecord::RecordInvalid %> @@ -78,6 +78,10 @@ <% end %>
+
+
<%= t(".shares_label") %>
+
<%= format_quantity(@holding.qty) %>
+
<%= t(".portfolio_weight_label") %>
<%= @holding.weight ? number_to_percentage(@holding.weight, precision: 2) : t(".unknown") %>
@@ -171,6 +175,17 @@
+
<%= t(".book_value_label") %>
+
+ <% book_value = @holding.avg_cost ? @holding.avg_cost * @holding.qty : nil %> + <%= book_value ? format_money(book_value) : t(".unknown") %> +
+
+
+
<%= t(".market_value_label") %>
+
<%= format_money(@holding.amount_money) %>
+
+
<%= t(".total_return_label") %>
<% if @holding.trend %>
@@ -214,7 +229,7 @@
<% end %> - <% if @holding.cost_basis_locked? || @holding.security_remapped? || @holding.account.can_delete_holdings? %> + <% if @holding.cost_basis_locked? || @holding.security_remapped? || @holding.account.can_delete_holdings? || !@holding.security.offline? %> <% dialog.with_section(title: t(".settings"), open: true) do %>
<% if @holding.security_remapped? %> @@ -234,6 +249,26 @@ } } %>
<% end %> + <% unless @holding.security.offline? %> +
+
+

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

+

+ <%= t(".last_price_update") %>: <%= @last_price_updated ? l(@last_price_updated, format: :long) : t(".never") %> +

+
+ <%= button_to t(".market_data_sync_button"), + sync_prices_holding_path(@holding), + method: :post, + class: "inline-flex items-center gap-1 px-3 py-2 rounded-lg text-sm font-medium text-primary bg-gray-200 hover:bg-gray-300 theme-dark:bg-gray-700 theme-dark:hover:bg-gray-600", + data: { loading_button_target: "button" }, + form: { data: { + controller: "loading-button", + action: "submit->loading-button#showLoading", + loading_button_loading_text_value: t(".syncing") + } } %> +
+ <% end %> <% if @holding.cost_basis_locked? %>
diff --git a/app/views/holdings/sync_prices.turbo_stream.erb b/app/views/holdings/sync_prices.turbo_stream.erb new file mode 100644 index 000000000..f2e253d91 --- /dev/null +++ b/app/views/holdings/sync_prices.turbo_stream.erb @@ -0,0 +1,56 @@ +<% unless @provider_error %> + <%= turbo_stream.replace dom_id(@holding, :current_market_price) do %> +
+ <% begin %> + <%= @holding.security.current_price ? format_money(@holding.security.current_price) : t("holdings.show.unknown") %> + <% rescue ActiveRecord::RecordInvalid %> + <%= t("holdings.show.unknown") %> + <% rescue StandardError => e %> + <% logger.error "Error fetching current price for security #{@holding.security.id}: #{e.message}" %> + <% logger.error e.backtrace.first(5).join("\n") %> + <%= t("holdings.show.unknown") %> + <% end %> +
+ <% end %> + <%= turbo_stream.replace dom_id(@holding, :market_value) do %> +
+
<%= t("holdings.show.market_value_label") %>
+
<%= format_money(@holding.amount_money) %>
+
+ <% end %> + <%= turbo_stream.replace dom_id(@holding, :total_return) do %> +
+
<%= t("holdings.show.total_return_label") %>
+ <% if @holding.trend %> +
+ <%= render("shared/trend_change", trend: @holding.trend) %> +
+ <% else %> +
<%= t("holdings.show.unknown") %>
+ <% end %> +
+ <% end %> +<% end %> +<%= turbo_stream.replace dom_id(@holding, :market_data_section) do %> +
+
+

<%= t("holdings.show.market_data_label") %>

+

+ <%= t("holdings.show.last_price_update") %>: <%= @last_price_updated ? l(@last_price_updated, format: :long) : t("holdings.show.never") %> +

+ <% if @provider_error %> +

<%= @provider_error %>

+ <% end %> +
+ <%= button_to t("holdings.show.market_data_sync_button"), + sync_prices_holding_path(@holding), + method: :post, + class: "inline-flex items-center gap-1 px-3 py-2 rounded-lg text-sm font-medium text-primary bg-gray-200 hover:bg-gray-300 theme-dark:bg-gray-700 theme-dark:hover:bg-gray-600", + data: { loading_button_target: "button" }, + form: { data: { + controller: "loading-button", + action: "submit->loading-button#showLoading", + loading_button_loading_text_value: t("holdings.show.syncing") + } } %> +
+<% end %> diff --git a/config/locales/views/holdings/de.yml b/config/locales/views/holdings/de.yml index 0d5bb4219..7fdd2ca74 100644 --- a/config/locales/views/holdings/de.yml +++ b/config/locales/views/holdings/de.yml @@ -32,4 +32,7 @@ de: ticker_label: Ticker trade_history_entry: "%{qty} Anteile von %{security} zu %{price}" total_return_label: Gesamtrendite + shares_label: Anteile + book_value_label: Buchwert + market_value_label: Marktwert unknown: Unbekannt diff --git a/config/locales/views/holdings/en.yml b/config/locales/views/holdings/en.yml index a14efe9b6..4fe40c9e6 100644 --- a/config/locales/views/holdings/en.yml +++ b/config/locales/views/holdings/en.yml @@ -15,6 +15,10 @@ en: security_not_found: Could not find the selected security. reset_security: success: Security reset to provider value. + sync_prices: + success: Market data synced successfully. + unavailable: Market data sync is not available for offline securities. + provider_error: Could not fetch latest prices. Please try again in a few minutes. errors: security_collision: "Cannot remap: you already have a holding for %{ticker} on %{date}." cost_basis_sources: @@ -82,3 +86,11 @@ en: unlock_cost_basis: Unlock unlock_confirm_title: Unlock cost basis? unlock_confirm_body: This will allow the cost basis to be updated by provider syncs or trade calculations. + shares_label: Shares + book_value_label: Book Value + market_value_label: Market Value + market_data_label: Market data + market_data_sync_button: Refresh + last_price_update: Last price update + syncing: Syncing... + never: Never diff --git a/config/locales/views/holdings/es.yml b/config/locales/views/holdings/es.yml index bc17c9aab..9ebbce072 100644 --- a/config/locales/views/holdings/es.yml +++ b/config/locales/views/holdings/es.yml @@ -47,6 +47,10 @@ es: missing_price_tooltip: description: Esta inversión tiene valores faltantes y no pudimos calcular su rendimiento o valor. missing_data: Datos faltantes + sync_prices: + success: Datos de mercado sincronizados correctamente. + unavailable: La sincronización de datos de mercado no está disponible para valores fuera de línea. + provider_error: No se pudieron obtener los precios más recientes. Inténtalo de nuevo en unos minutos. show: avg_cost_label: Costo promedio current_market_price_label: Precio de mercado actual @@ -74,6 +78,14 @@ es: ticker_label: Ticker trade_history_entry: "%{qty} acciones de %{security} a %{price}" total_return_label: Rendimiento total + shares_label: Acciones + book_value_label: Valor en libros + market_value_label: Valor de mercado + market_data_label: Datos de mercado + market_data_sync_button: Actualizar + last_price_update: Última actualización de precio + syncing: Sincronizando... + never: Nunca unknown: Desconocido cost_basis_locked_label: La base de costes está bloqueada cost_basis_locked_description: La base de costes establecida manualmente no cambiará con las sincronizaciones. diff --git a/config/locales/views/holdings/fr.yml b/config/locales/views/holdings/fr.yml index 09c96ef83..2959132a1 100644 --- a/config/locales/views/holdings/fr.yml +++ b/config/locales/views/holdings/fr.yml @@ -33,4 +33,7 @@ fr: ticker_label: Ticker trade_history_entry: "%{qty} actions de %{security} à %{price}" total_return_label: Rendement total + shares_label: Actions + book_value_label: Valeur comptable + market_value_label: Valeur marchande unknown: Inconnu diff --git a/config/routes.rb b/config/routes.rb index f5e2fb767..514cece15 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -247,6 +247,7 @@ Rails.application.routes.draw do post :unlock_cost_basis patch :remap_security post :reset_security + post :sync_prices end end resources :trades, only: %i[show new create update destroy] do diff --git a/test/controllers/holdings_controller_test.rb b/test/controllers/holdings_controller_test.rb index a73680d54..dfa52d499 100644 --- a/test/controllers/holdings_controller_test.rb +++ b/test/controllers/holdings_controller_test.rb @@ -61,4 +61,61 @@ class HoldingsControllerTest < ActionDispatch::IntegrationTest assert_equal 50.0, @holding.cost_basis.to_f assert_equal "manual", @holding.cost_basis_source end + + test "remap_security brings offline security back online" do + # Given: the target security is marked offline (e.g. created by a failed QIF import) + msft = securities(:msft) + msft.update!(offline: true, failed_fetch_count: 3) + + # When: user explicitly selects it from the provider search and saves + patch remap_security_holding_path(@holding), params: { security_id: "MSFT|XNAS" } + + # Then: the security is brought back online and the holding is remapped + assert_redirected_to account_path(@holding.account, tab: "holdings") + @holding.reload + msft.reload + assert_equal msft.id, @holding.security_id + assert_not msft.offline? + assert_equal 0, msft.failed_fetch_count + end + + test "sync_prices redirects with alert for offline security" do + @holding.security.update!(offline: true) + + post sync_prices_holding_path(@holding) + + assert_redirected_to account_path(@holding.account, tab: "holdings") + assert_equal I18n.t("holdings.sync_prices.unavailable"), flash[:alert] + end + + test "sync_prices syncs market data and redirects with notice" do + Security.any_instance.expects(:import_provider_prices).with( + start_date: 31.days.ago.to_date, + end_date: Date.current, + clear_cache: true + ).returns([ 31, nil ]) + Security.any_instance.stubs(:import_provider_details) + materializer = mock("materializer") + materializer.expects(:materialize_balances).once + Balance::Materializer.expects(:new).with( + @holding.account, + strategy: :forward, + security_ids: [ @holding.security_id ] + ).returns(materializer) + + post sync_prices_holding_path(@holding) + + assert_redirected_to account_path(@holding.account, tab: "holdings") + assert_equal I18n.t("holdings.sync_prices.success"), flash[:notice] + end + + test "sync_prices shows provider error inline when provider returns no prices" do + Security.any_instance.stubs(:import_provider_prices).returns([ 0, "Yahoo Finance rate limit exceeded" ]) + Security.any_instance.stubs(:import_provider_details) + + post sync_prices_holding_path(@holding) + + assert_redirected_to account_path(@holding.account, tab: "holdings") + assert_equal "Yahoo Finance rate limit exceeded", flash[:alert] + end end diff --git a/test/controllers/settings/hostings_controller_test.rb b/test/controllers/settings/hostings_controller_test.rb index bd02b321a..f211e07bf 100644 --- a/test/controllers/settings/hostings_controller_test.rb +++ b/test/controllers/settings/hostings_controller_test.rb @@ -201,16 +201,18 @@ class Settings::HostingsControllerTest < ActionDispatch::IntegrationTest test "disconnect external assistant clears settings and resets type" do with_self_hosting do - Setting.external_assistant_url = "https://agent.example.com/v1/chat" - Setting.external_assistant_token = "token" - Setting.external_assistant_agent_id = "finance-bot" - users(:family_admin).family.update!(assistant_type: "external") + with_env_overrides("EXTERNAL_ASSISTANT_URL" => nil, "EXTERNAL_ASSISTANT_TOKEN" => nil) do + Setting.external_assistant_url = "https://agent.example.com/v1/chat" + Setting.external_assistant_token = "token" + Setting.external_assistant_agent_id = "finance-bot" + users(:family_admin).family.update!(assistant_type: "external") - delete disconnect_external_assistant_settings_hosting_url + delete disconnect_external_assistant_settings_hosting_url - assert_redirected_to settings_hosting_url - assert_not Assistant::External.configured? - assert_equal "builtin", users(:family_admin).family.reload.assistant_type + assert_redirected_to settings_hosting_url + assert_not Assistant::External.configured? + assert_equal "builtin", users(:family_admin).family.reload.assistant_type + end end ensure Setting.external_assistant_url = nil