From 1b6ce6af4574e23464dd231b40778b25986663da Mon Sep 17 00:00:00 2001 From: Zach Gollwitzer Date: Fri, 16 Aug 2024 16:08:27 -0400 Subject: [PATCH] Improved UI warning states for holdings with missing data (#1098) * Fix security price issue flow * Fix tooltip positioning and add tooltip for missing holding data * Fix tooltip controller error with stale arrow target * Lint fixes --- .../controllers/tooltip_controller.js | 89 ++++++++++--------- app/models/account.rb | 8 +- app/models/account/sync.rb | 2 + app/models/issue/prices_missing.rb | 11 +++ app/views/account/holdings/_holding.html.erb | 14 +-- .../holdings/_missing_price_tooltip.html.erb | 11 +++ app/views/accounts/_tooltip.html.erb | 5 +- .../issue/_request_synth_data_action.html.erb | 12 +++ .../exchange_rates_missings/show.html.erb | 13 +-- app/views/issue/prices_missings/show.html.erb | 11 +++ app/views/issues/_issue.html.erb | 5 +- app/views/shared/_drawer.html.erb | 8 +- config/locales/models/issue/en.yml | 1 + config/locales/views/account/holdings/en.yml | 4 + test/models/account_test.rb | 8 ++ test/system/tooltips_test.rb | 10 ++- 16 files changed, 139 insertions(+), 73 deletions(-) create mode 100644 app/views/account/holdings/_missing_price_tooltip.html.erb create mode 100644 app/views/issue/_request_synth_data_action.html.erb create mode 100644 app/views/issue/prices_missings/show.html.erb diff --git a/app/javascript/controllers/tooltip_controller.js b/app/javascript/controllers/tooltip_controller.js index cad419a1d6c..7013607f369 100644 --- a/app/javascript/controllers/tooltip_controller.js +++ b/app/javascript/controllers/tooltip_controller.js @@ -4,11 +4,11 @@ import { flip, shift, offset, - arrow + autoUpdate } from '@floating-ui/dom'; export default class extends Controller { - static targets = ["arrow", "tooltip"]; + static targets = ["tooltip"]; static values = { placement: { type: String, default: "top" }, offset: { type: Number, default: 10 }, @@ -17,58 +17,67 @@ export default class extends Controller { }; connect() { - this.element.addEventListener("mouseenter", this.showTooltip); - this.element.addEventListener("mouseleave", this.hideTooltip); - this.element.addEventListener("focus", this.showTooltip); - this.element.addEventListener("blur", this.hideTooltip); - }; + this._cleanup = null; + this.boundUpdate = this.update.bind(this); + this.startAutoUpdate(); + this.addEventListeners(); + } + + disconnect() { + this.removeEventListeners(); + this.stopAutoUpdate(); + } + + addEventListeners() { + this.element.addEventListener("mouseenter", this.show); + this.element.addEventListener("mouseleave", this.hide); + } - showTooltip = () => { + removeEventListeners() { + this.element.removeEventListener("mouseenter", this.show); + this.element.removeEventListener("mouseleave", this.hide); + } + + show = () => { this.tooltipTarget.style.display = 'block'; - this.#update(); - }; + this.update(); // Ensure immediate update when shown + } - hideTooltip = () => { - this.tooltipTarget.style.display = ''; - }; + hide = () => { + this.tooltipTarget.style.display = 'none'; + } - disconnect() { - this.element.removeEventListener("mouseenter", this.showTooltip); - this.element.removeEventListener("mouseleave", this.hideTooltip); - this.element.removeEventListener("focus", this.showTooltip); - this.element.removeEventListener("blur", this.hideTooltip); - }; + startAutoUpdate() { + if (!this._cleanup) { + this._cleanup = autoUpdate( + this.element, + this.tooltipTarget, + this.boundUpdate + ); + } + } - #update() { + stopAutoUpdate() { + if (this._cleanup) { + this._cleanup(); + this._cleanup = null; + } + } + + update() { + // Update position even if not visible, to ensure correct positioning when shown computePosition(this.element, this.tooltipTarget, { placement: this.placementValue, middleware: [ offset({ mainAxis: this.offsetValue, crossAxis: this.crossAxisValue, alignmentAxis: this.alignmentAxisValue }), flip(), - shift({ padding: 5 }), - arrow({ element: this.arrowTarget }), + shift({ padding: 5 }) ], }).then(({ x, y, placement, middlewareData }) => { Object.assign(this.tooltipTarget.style, { left: `${x}px`, top: `${y}px`, }); - - const { x: arrowX, y: arrowY } = middlewareData.arrow; - const staticSide = { - top: 'bottom', - right: 'left', - bottom: 'top', - left: 'right', - }[placement.split('-')[0]]; - - Object.assign(this.arrowTarget.style, { - left: arrowX != null ? `${arrowX}px` : '', - top: arrowY != null ? `${arrowY}px` : '', - right: '', - bottom: '', - [staticSide]: '-4px', - }); }); - }; -} + } +} \ No newline at end of file diff --git a/app/models/account.rb b/app/models/account.rb index 9490f0f316e..9f17f756382 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -75,9 +75,11 @@ def create_with_optional_start_balance!(attributes:, start_date: nil, start_bala end end - def alert - latest_sync = syncs.latest - [ latest_sync&.error, *latest_sync&.warnings ].compact.first + def owns_ticker?(ticker) + security_id = Security.find_by(ticker: ticker)&.id + entries.account_trades + .joins("JOIN account_trades ON account_entries.entryable_id = account_trades.id") + .where(account_trades: { security_id: security_id }).any? end def favorable_direction diff --git a/app/models/account/sync.rb b/app/models/account/sync.rb index adc26843ac1..28ae251a9cd 100644 --- a/app/models/account/sync.rb +++ b/app/models/account/sync.rb @@ -25,6 +25,8 @@ def run rescue StandardError => error account.observe_unknown_issue(error) fail! error + + raise error if Rails.env.development? end private diff --git a/app/models/issue/prices_missing.rb b/app/models/issue/prices_missing.rb index 823c1d19767..6cb014afb35 100644 --- a/app/models/issue/prices_missing.rb +++ b/app/models/issue/prices_missing.rb @@ -1,6 +1,8 @@ class Issue::PricesMissing < Issue store_accessor :data, :missing_prices + after_initialize :initialize_missing_prices + validates :missing_prices, presence: true def append_missing_price(ticker, date) @@ -10,7 +12,10 @@ def append_missing_price(ticker, date) def stale? stale = true + missing_prices.each do |ticker, dates| + next unless issuable.owns_ticker?(ticker) + oldest_date = dates.min expected_price_count = (oldest_date..Date.current).count prices = Security::Price.find_prices(ticker: ticker, start_date: oldest_date) @@ -19,4 +24,10 @@ def stale? stale end + + private + + def initialize_missing_prices + self.missing_prices ||= {} + end end diff --git a/app/views/account/holdings/_holding.html.erb b/app/views/account/holdings/_holding.html.erb index fc4db7f5c43..83f842541f4 100644 --- a/app/views/account/holdings/_holding.html.erb +++ b/app/views/account/holdings/_holding.html.erb @@ -4,9 +4,13 @@
<%= render "shared/circle_logo", name: holding.name || "H" %> -
+
<%= link_to holding.name || holding.ticker, account_holding_path(holding.account, holding), data: { turbo_frame: :drawer }, class: "hover:underline" %> - <%= tag.p holding.ticker, class: "text-gray-500 text-xs uppercase" %> + <% if holding.amount %> + <%= tag.p holding.ticker, class: "text-gray-500 text-xs uppercase" %> + <% else %> + <%= render "missing_price_tooltip" %> + <% end %>
@@ -15,7 +19,7 @@ <%= render "shared/progress_circle", progress: holding.weight, text_class: "text-blue-500" %> <%= tag.p number_to_percentage(holding.weight, precision: 1) %> <% else %> - <%= tag.p "?", class: "text-gray-500" %> + <%= tag.p "--", class: "text-gray-500 mb-5" %> <% end %>
@@ -28,7 +32,7 @@ <% if holding.amount_money %> <%= tag.p format_money holding.amount_money %> <% else %> - <%= tag.p "?", class: "text-gray-500" %> + <%= tag.p "--", class: "text-gray-500" %> <% end %> <%= tag.p t(".shares", qty: number_with_precision(holding.qty, precision: 1)), class: "font-normal text-gray-500" %>
@@ -38,7 +42,7 @@ <%= tag.p format_money(holding.trend.value), style: "color: #{holding.trend.color};" %> <%= tag.p "(#{number_to_percentage(holding.trend.percent, precision: 1)})", style: "color: #{holding.trend.color};" %> <% else %> - <%= tag.p "?", class: "text-gray-500" %> + <%= tag.p "--", class: "text-gray-500 mb-4" %> <% end %> diff --git a/app/views/account/holdings/_missing_price_tooltip.html.erb b/app/views/account/holdings/_missing_price_tooltip.html.erb new file mode 100644 index 00000000000..933a4355f39 --- /dev/null +++ b/app/views/account/holdings/_missing_price_tooltip.html.erb @@ -0,0 +1,11 @@ +
+
+ <%= lucide_icon "info", class: "w-4 h-4 shrink-0" %> + <%= tag.span t(".missing_data"), class: "font-normal text-xs" %> +
+ +
diff --git a/app/views/accounts/_tooltip.html.erb b/app/views/accounts/_tooltip.html.erb index 6a985793a9d..952d2ad18a4 100644 --- a/app/views/accounts/_tooltip.html.erb +++ b/app/views/accounts/_tooltip.html.erb @@ -1,7 +1,7 @@ <%# locals: (account:) -%> -
+
<%= lucide_icon("info", class: "w-4 h-4 shrink-0 text-gray-500") %> -
-
diff --git a/app/views/issue/_request_synth_data_action.html.erb b/app/views/issue/_request_synth_data_action.html.erb new file mode 100644 index 00000000000..a805524b227 --- /dev/null +++ b/app/views/issue/_request_synth_data_action.html.erb @@ -0,0 +1,12 @@ +

The Synth data provider could not find the requested data.

+ +

We are actively developing Synth to be a low cost and easy to use data provider. You can help us improve Synth by + requesting the data you need.

+ +

Please post in our <%= link_to "Discord server", "https://link.maybe.co/discord", target: "_blank" %> with the + following information:

+ + diff --git a/app/views/issue/exchange_rates_missings/show.html.erb b/app/views/issue/exchange_rates_missings/show.html.erb index 5504ebae789..65c624edcb1 100644 --- a/app/views/issue/exchange_rates_missings/show.html.erb +++ b/app/views/issue/exchange_rates_missings/show.html.erb @@ -7,16 +7,5 @@ <% end %> <%= content_for :action do %> -

The Synth data provider could not find the requested data.

- -

We are actively developing Synth to be a low cost and easy to use data provider. You can help us improve Synth by - requesting the data you need.

- -

Please post in our <%= link_to "Discord server", "https://link.maybe.co/discord", target: "_blank" %> with the - following information:

- - + <%= render "issue/request_synth_data_action" %> <% end %> diff --git a/app/views/issue/prices_missings/show.html.erb b/app/views/issue/prices_missings/show.html.erb new file mode 100644 index 00000000000..c71e7bff028 --- /dev/null +++ b/app/views/issue/prices_missings/show.html.erb @@ -0,0 +1,11 @@ +<%= content_for :title, @issue.title %> + +<%= content_for :description do %> +

Some stock prices are missing for this account.

+ + 
<%= JSON.pretty_generate(@issue.data) %>
+<% end %> + +<%= content_for :action do %> + <%= render "issue/request_synth_data_action" %> +<% end %> diff --git a/app/views/issues/_issue.html.erb b/app/views/issues/_issue.html.erb index cc80fbb65a3..cabb360cdbc 100644 --- a/app/views/issues/_issue.html.erb +++ b/app/views/issues/_issue.html.erb @@ -1,14 +1,15 @@ <%# locals: (issue:) %> <% priority_class = issue.critical? || issue.error? ? "bg-error/5" : "bg-warning/5" %> +<% text_class = issue.critical? || issue.error? ? "text-error" : "text-warning" %> <%= tag.div class: "flex gap-6 items-center rounded-xl px-4 py-3 #{priority_class}" do %> -
+
<%= lucide_icon("alert-octagon", class: "w-5 h-5 shrink-0") %>

<%= issue.title %>

- <%= link_to "Troubleshoot", issue_path(issue), class: "text-red-500 font-medium hover:underline", data: { turbo_frame: :drawer } %> + <%= link_to "Troubleshoot", issue_path(issue), class: "#{text_class} font-medium hover:underline", data: { turbo_frame: :drawer } %>
<% end %> diff --git a/app/views/shared/_drawer.html.erb b/app/views/shared/_drawer.html.erb index ebe70cf67c6..68aa2db5077 100644 --- a/app/views/shared/_drawer.html.erb +++ b/app/views/shared/_drawer.html.erb @@ -1,12 +1,12 @@ <%= turbo_frame_tag "drawer" do %> - -
-
+ +
+
<%= lucide_icon("x", class: "w-5 h-5 shrink-0") %>
-
+
<%= content %>
diff --git a/config/locales/models/issue/en.yml b/config/locales/models/issue/en.yml index b23fac8f2f2..9c0938b4d74 100644 --- a/config/locales/models/issue/en.yml +++ b/config/locales/models/issue/en.yml @@ -4,4 +4,5 @@ en: models: issue/exchange_rate_provider_missing: Exchange rate provider missing issue/exchange_rates_missing: Exchange rates missing + issue/missing_prices: Missing prices issue/unknown: Unknown issue occurred diff --git a/config/locales/views/account/holdings/en.yml b/config/locales/views/account/holdings/en.yml index 35a7b59af8f..500bdd87cea 100644 --- a/config/locales/views/account/holdings/en.yml +++ b/config/locales/views/account/holdings/en.yml @@ -15,6 +15,10 @@ en: no_holdings: No holdings to show. return: total return weight: weight + missing_price_tooltip: + description: This investment has missing values and we could not calculate + its returns or value. + missing_data: Missing data show: history: History overview: Overview diff --git a/test/models/account_test.rb b/test/models/account_test.rb index 3da0186073b..18346e15723 100644 --- a/test/models/account_test.rb +++ b/test/models/account_test.rb @@ -89,4 +89,12 @@ 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/system/tooltips_test.rb b/test/system/tooltips_test.rb index e9de040c383..7666269cf8f 100644 --- a/test/system/tooltips_test.rb +++ b/test/system/tooltips_test.rb @@ -11,9 +11,11 @@ class TooltipsTest < ApplicationSystemTestCase test "can see account information tooltip" do visit account_path(@account) - find('[data-controller="tooltip"]').hover - assert find("#tooltip", visible: true) - within "#tooltip" do + tooltip_element = find('[data-controller="tooltip"]') + tooltip_element.hover + tooltip_contents = find('[data-tooltip-target="tooltip"]') + assert tooltip_contents.visible? + within tooltip_contents do assert_text I18n.t("accounts.tooltip.total_value_tooltip") assert_text I18n.t("accounts.tooltip.holdings") assert_text format_money(@account.investment.holdings_value, precision: 0) @@ -21,6 +23,6 @@ class TooltipsTest < ApplicationSystemTestCase assert_text format_money(@account.balance_money, precision: 0) end find("body").click - assert find("#tooltip", visible: false) + assert find('[data-tooltip-target="tooltip"]', visible: false) end end