From 6f67827f145ef952d967f524f52270a622d877c9 Mon Sep 17 00:00:00 2001 From: Josh Pigford Date: Mon, 26 May 2025 20:05:16 -0500 Subject: [PATCH] Implement error handling and logging for sparkline and series methods - Added rescue blocks to handle exceptions in the Accounts and AccountableSparklines controllers, logging errors and rendering error partials. - Enhanced error handling in the Account::Chartable and Balance::ChartSeriesBuilder models, logging specific error messages for series generation failures. - Updated the accounts view to include a timeout for Turbo frame loading. - Added a test to ensure graceful handling of sparkline errors in the AccountsController. In reference to bug #2315 --- .../accountable_sparklines_controller.rb | 3 ++ app/controllers/accounts_controller.rb | 3 ++ .../turbo_frame_timeout_controller.js | 42 +++++++++++++++++++ app/models/account/chartable.rb | 10 +++++ app/models/balance/chart_series_builder.rb | 12 ++++++ .../accountable_sparklines/_error.html.erb | 8 ++++ .../accounts/_accountable_group.html.erb | 4 +- app/views/accounts/_sparkline_error.html.erb | 8 ++++ test/controllers/accounts_controller_test.rb | 31 +++++++++++--- 9 files changed, 113 insertions(+), 8 deletions(-) create mode 100644 app/javascript/controllers/turbo_frame_timeout_controller.js create mode 100644 app/views/accountable_sparklines/_error.html.erb create mode 100644 app/views/accounts/_sparkline_error.html.erb diff --git a/app/controllers/accountable_sparklines_controller.rb b/app/controllers/accountable_sparklines_controller.rb index bebc211f..832b0a09 100644 --- a/app/controllers/accountable_sparklines_controller.rb +++ b/app/controllers/accountable_sparklines_controller.rb @@ -17,6 +17,9 @@ class AccountableSparklinesController < ApplicationController end render layout: false + rescue => e + Rails.logger.error "Accountable sparkline error for #{@accountable&.name}: #{e.message}" + render partial: "accountable_sparklines/error", layout: false end private diff --git a/app/controllers/accounts_controller.rb b/app/controllers/accounts_controller.rb index 904be2b5..d3fbb00e 100644 --- a/app/controllers/accounts_controller.rb +++ b/app/controllers/accounts_controller.rb @@ -24,6 +24,9 @@ class AccountsController < ApplicationController def sparkline render layout: false + rescue => e + Rails.logger.error "Sparkline error for account #{@account.id}: #{e.message}" + render partial: "accounts/sparkline_error", layout: false end private diff --git a/app/javascript/controllers/turbo_frame_timeout_controller.js b/app/javascript/controllers/turbo_frame_timeout_controller.js new file mode 100644 index 00000000..a81b3dde --- /dev/null +++ b/app/javascript/controllers/turbo_frame_timeout_controller.js @@ -0,0 +1,42 @@ +import { Controller } from "@hotwired/stimulus" + +// Connects to data-controller="turbo-frame-timeout" +export default class extends Controller { + static values = { timeout: { type: Number, default: 10000 } } + + connect() { + this.timeoutId = setTimeout(() => { + this.handleTimeout() + }, this.timeoutValue) + + // Listen for successful frame loads to clear timeout + this.element.addEventListener("turbo:frame-load", this.clearTimeout.bind(this)) + } + + disconnect() { + this.clearTimeout() + } + + clearTimeout() { + if (this.timeoutId) { + clearTimeout(this.timeoutId) + this.timeoutId = null + } + } + + handleTimeout() { + // Replace loading content with error state + this.element.innerHTML = ` +
+
+ + + + + +
+

Timeout

+
+ ` + } +} \ No newline at end of file diff --git a/app/models/account/chartable.rb b/app/models/account/chartable.rb index 000fc3de..304fcbbd 100644 --- a/app/models/account/chartable.rb +++ b/app/models/account/chartable.rb @@ -29,5 +29,15 @@ module Account::Chartable Rails.cache.fetch(cache_key) do balance_series end + rescue => e + Rails.logger.error "Sparkline series error for account #{id}: #{e.message}" + # Return empty series as fallback + Series.new( + start_date: 30.days.ago.to_date, + end_date: Date.current, + interval: "1 day", + values: [], + favorable_direction: favorable_direction + ) end end diff --git a/app/models/balance/chart_series_builder.rb b/app/models/balance/chart_series_builder.rb index d350b29d..0c367685 100644 --- a/app/models/balance/chart_series_builder.rb +++ b/app/models/balance/chart_series_builder.rb @@ -9,14 +9,23 @@ class Balance::ChartSeriesBuilder def balance_series build_series_for(:balance) + rescue => e + Rails.logger.error "Balance series error: #{e.message} for accounts #{@account_ids}" + raise end def cash_balance_series build_series_for(:cash_balance) + rescue => e + Rails.logger.error "Cash balance series error: #{e.message} for accounts #{@account_ids}" + raise end def holdings_balance_series build_series_for(:holdings_balance) + rescue => e + Rails.logger.error "Holdings balance series error: #{e.message} for accounts #{@account_ids}" + raise end private @@ -61,6 +70,9 @@ class Balance::ChartSeriesBuilder sign_multiplier: sign_multiplier } ]) + rescue => e + Rails.logger.error "Query data error: #{e.message} for accounts #{account_ids}, period #{period.start_date} to #{period.end_date}" + raise end # Since the query aggregates the *net* of assets - liabilities, this means that if we're looking at diff --git a/app/views/accountable_sparklines/_error.html.erb b/app/views/accountable_sparklines/_error.html.erb new file mode 100644 index 00000000..70ce745e --- /dev/null +++ b/app/views/accountable_sparklines/_error.html.erb @@ -0,0 +1,8 @@ +<%= turbo_frame_tag "#{params[:accountable_type]}_sparkline" do %> +
+
+ <%= icon("alert-triangle", size: "sm", class: "text-warning") %> +
+

Error

+
+<% end %> \ No newline at end of file diff --git a/app/views/accounts/_accountable_group.html.erb b/app/views/accounts/_accountable_group.html.erb index b5393856..b4495afc 100644 --- a/app/views/accounts/_accountable_group.html.erb +++ b/app/views/accounts/_accountable_group.html.erb @@ -14,7 +14,7 @@ <% else %> <%= tag.p format_money(account_group.total_money), class: "text-sm font-medium text-primary" %> - <%= turbo_frame_tag "#{account_group.key}_sparkline", src: accountable_sparkline_path(account_group.key), loading: "lazy" do %> + <%= turbo_frame_tag "#{account_group.key}_sparkline", src: accountable_sparkline_path(account_group.key), loading: "lazy", data: { controller: "turbo-frame-timeout", turbo_frame_timeout_timeout_value: 10000 } do %>
@@ -50,7 +50,7 @@ <% else %>
<%= tag.p format_money(account.balance_money), class: "text-sm font-medium text-primary whitespace-nowrap" %> - <%= turbo_frame_tag dom_id(account, :sparkline), src: sparkline_account_path(account), loading: "lazy" do %> + <%= turbo_frame_tag dom_id(account, :sparkline), src: sparkline_account_path(account), loading: "lazy", data: { controller: "turbo-frame-timeout", turbo_frame_timeout_timeout_value: 10000 } do %>
diff --git a/app/views/accounts/_sparkline_error.html.erb b/app/views/accounts/_sparkline_error.html.erb new file mode 100644 index 00000000..2f813a0d --- /dev/null +++ b/app/views/accounts/_sparkline_error.html.erb @@ -0,0 +1,8 @@ +<%= turbo_frame_tag dom_id(@account, :sparkline) do %> +
+
+ <%= icon("alert-triangle", size: "sm", class: "text-warning") %> +
+

Error

+
+<% end %> \ No newline at end of file diff --git a/test/controllers/accounts_controller_test.rb b/test/controllers/accounts_controller_test.rb index a3d827e8..e81cf7d0 100644 --- a/test/controllers/accounts_controller_test.rb +++ b/test/controllers/accounts_controller_test.rb @@ -6,13 +6,32 @@ class AccountsControllerTest < ActionDispatch::IntegrationTest @account = accounts(:depository) end - test "new" do - get new_account_path - assert_response :ok + test "should get index" do + get accounts_url + assert_response :success end - test "can sync an account" do - post sync_account_path(@account) - assert_redirected_to account_path(@account) + test "should sync account" do + post sync_account_url(@account) + assert_redirected_to account_url(@account) + end + + test "should get chart" do + get chart_account_url(@account) + assert_response :success + end + + test "should get sparkline" do + get sparkline_account_url(@account) + assert_response :success + end + + test "should handle sparkline errors gracefully" do + # Mock an error in the sparkline_series method + @account.stubs(:sparkline_series).raises(StandardError.new("Test error")) + + get sparkline_account_url(@account) + assert_response :success + assert_match /Error/, response.body end end