Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Hidden] Display a friendly message when a background report times out #10644

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .env.development
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,9 @@ OFN_REDIS_URL="redis://localhost:6379/1"
OFN_REDIS_JOBS_URL="redis://localhost:6379/2"

SITE_URL="0.0.0.0:3000"

# Deactivate rack-timeout in development.
# https://github.com/zombocom/rack-timeout#configuring
RACK_TIMEOUT_SERVICE_TIMEOUT="0"
RACK_TIMEOUT_WAIT_TIMEOUT="0"
RACK_TIMEOUT_WAIT_OVERTIME="0"
2 changes: 1 addition & 1 deletion Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ gem 'gmaps4rails'
gem 'mimemagic', '> 0.3.5'
gem 'paper_trail', '~> 12.1'
gem 'rack-rewrite'
gem 'rack-timeout'
gem 'roadie-rails'

gem 'hiredis'
Expand Down Expand Up @@ -141,7 +142,6 @@ gem "private_address_check"

group :production, :staging do
gem 'ddtrace'
gem 'rack-timeout'
gem 'sd_notify' # For better Systemd process management. Used by Puma.
end

Expand Down
26 changes: 24 additions & 2 deletions app/controllers/admin/reports_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ def show
else
show_report
end
rescue Timeout::Error
render_timeout_error
end

private
Expand All @@ -36,6 +38,7 @@ def export_report

def show_report
assign_view_data
@table = render_report_as(:html) if render_data?
render "show"
end

Expand All @@ -45,7 +48,6 @@ def assign_view_data
@report_subtype = report_subtype
@report_title = report_title
@rendering_options = rendering_options
@table = render_report_as(:html) if render_data?
@data = Reporting::FrontendData.new(spree_current_user)
end

Expand All @@ -58,13 +60,33 @@ def render_report_as(format)
job = ReportJob.perform_later(
report_class, spree_current_user, params, format
)
sleep 1 until job.done?
Timeout.timeout(max_wait_time) do
sleep 1 until job.done?
end

# This result has been rendered by Rails in safe mode already.
job.result.html_safe # rubocop:disable Rails/OutputSafety
else
@report.render_as(format)
end
end

def render_timeout_error
assign_view_data
@error = ".report_taking_longer"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious if there's any reason to translate in the view rather than here? I think we can access the translate helper in the controller.

This looks nice and clean, but I was thinking we usually expect translate keys to be with the t() method call, so if we ever need to rely on that pattern (eg when searching the codebase or perform mass find/replace), this could slow us down or cause us to miss it.

It's a really minor point but I was curious :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious if there's any reason to translate in the view rather than here?

Yes, Rails supports translations in the view better.

  • We can use the lazy loading with the prefixed dot.
  • We can simply use t instead of I18n.t.
  • Views are concerned with how things are displayed. An error code can be translated or a different error code could trigger a modal and that should be the concern of the view, not the controller, I think.

You got a good point about the disassociation of translation key and the t call as well. And I'm not sure what to suggest about that. If we want to translate in the view then we could do something like t(".report_taking_longer") if @error == :report_taking_longer but that seems cumbersome, too. So we can just translate in the controller and add an apt translation scope. My pattern is a bit closer to the translations of errors on models. There's a default structure of error messages on ActiveRecord errors. Maybe the path will become more clear later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good points.

render "show"
end

def max_wait_time
# This value is used by rack-timeout and nginx, usually 30 seconds in
# staging and production:
server_timeout = ENV.fetch("RACK_TIMEOUT_SERVICE_TIMEOUT", "15").to_f

# Zero disables the timeout:
return 0 if server_timeout.zero?

# We want to time out earlier than nginx:
server_timeout - 2.seconds
end
end
end
3 changes: 2 additions & 1 deletion app/views/admin/reports/show.html.haml
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
.report__header.print-hidden
- if @report.message.present?
%p.report__message= @report.message
- if request.post?
- if request.post? && !@error
%button.btn-print.icon-print{ onclick: "window.print()"}= t(:report_print)

= t(@error) if @error
= @table
5 changes: 5 additions & 0 deletions config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1438,6 +1438,11 @@ en:
pack_by_customer: Pack By Customer
pack_by_supplier: Pack By Supplier
pack_by_product: Pack By Product
show:
report_taking_longer: >
Sorry, this report took too long to process.
It may contain a lot of data or we are busy with other reports.
You can try again later.
revenues_by_hub:
name: Revenues By Hub
description: Revenues by hub
Expand Down
13 changes: 13 additions & 0 deletions spec/system/admin/reports_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,19 @@
click_button "Go"
expect(page).to have_content "EMAIL FIRST NAME"
end

it "displays a friendly timeout message" do
ActiveJob::Base.queue_adapter.perform_enqueued_jobs = false
login_as_admin_and_visit admin_report_path(
report_type: :customers, report_subtype: :mailing_list
)
expect(ENV).to receive(:fetch).with("RACK_TIMEOUT_SERVICE_TIMEOUT", "15")
.and_return("-1") # Negative values time out immediately.

click_button "Go"

expect(page).to have_content "this report took too long"
end
end

describe "Can access Customers reports and generate customers report" do
Expand Down