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.

Sign up for GitHub

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

AO3-6911 Add error page for Rack::Timeout::RequestTimeoutException #5074

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

AO3-6911 Add error page for Rack::Timeout::RequestTimeoutException #5074

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c420089
AO3-6911: add timeout page as catch to Rack::Timeout::RequestTimeoutE…
boyer-victor Feb 20, 2025
165261d
AO3-6911: prefer double quotes, add i18n support
boyer-victor Feb 21, 2025
3f2d882
Update spec/controllers/errors_controller_spec.rb
boyer-victor Feb 24, 2025
d507d2a
AO3-6911: prefer lazy loaded locale, fix route
boyer-victor Feb 24, 2025
ec7dbd7
AO3-6911: return 504
boyer-victor Feb 25, 2025
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 app/controllers/application_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,12 @@ def raise_not_found
redirect_to '/404'
end

rescue_from Rack::Timeout::RequestTimeoutException, with: :raise_timeout

def raise_timeout
redirect_to "/timeout"
end

helper :all # include all helpers, all the time

include HtmlCleaner
Expand Down
4 changes: 4 additions & 0 deletions app/controllers/errors_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,8 @@ class ErrorsController < ApplicationController
def auth_error
@page_subtitle = "Auth Error"
end

def timeout_error
@page_subtitle = "Timeout Error"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please also i18n this. Here is an example:

otwarchive/app/controllers/home_controller.rb

Line 16 in 775ed13

@page_subtitle = t(".page_title")
+

otwarchive/config/locales/controllers/en.yml

Line 100 in 775ed13

page_title: Privacy Policy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Apologies, I am used to a different pattern and did not look carefully enough at the locale specs for this project. I updated auth_error as well.

end
end
3 changes: 3 additions & 0 deletions app/views/errors/timeout.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
<h2 class="heading"><%= t("errors.timeout.title") %></h2>
<h3 class="heading"><%= t("errors.timeout.subtitle") %></h3>
<p><%= t("errors.timeout.html", contact_support_link: link_to(ts("contact Support"), new_feedback_report_path)) %></p>

Check failure on line 3 in app/views/errors/timeout.html.erb

View workflow job for this annotation

GitHub Actions / ERB Lint runner 

[] reported by reviewdog 🐶
Missing a trailing newline at the end of the file.

Raw Output:
app/views/errors/timeout.html.erb:3:118: Missing a trailing newline at the end of the file.

Check failure on line 3 in app/views/errors/timeout.html.erb

View workflow job for this annotation

GitHub Actions / ERB Lint runner 

[] reported by reviewdog 🐶
I18n/DeprecatedHelper: Prefer Rails built-in `t` helper over `ts` and move the text into the yml file. `ts` is not actually translatable. For more information, refer to https://github.com/otwcode/otwarchive/wiki/Internationalization-(i18n)-Standards

Raw Output:
app/views/errors/timeout.html.erb:3:62: I18n/DeprecatedHelper: Prefer Rails built-in `t` helper over `ts` and move the text into the yml file. `ts` is not actually translatable. For more information, refer to https://github.com/otwcode/otwarchive/wiki/Internationalization-(i18n)-Standards
5 changes: 5 additions & 0 deletions config/locales/views/en.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -611,6 +611,11 @@ en:
restricted_html: A translation of [Restricted Work] by %{creator_link}
revealed_html: A translation of %{work_link} by %{creator_link}
unrevealed: A translation of a work in an unrevealed collection
errors:
timeout:
title: Timeout
subtitle: The page was responding too slowly. Please try again after a few minutes.
html: If you still get this error after a few tries, please %{contact_support_link}.
feedbacks:
new:
abuse:
Expand Down
1 change: 1 addition & 0 deletions config/routes.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
get '/422', to: 'errors#422'
get '/500', to: 'errors#500'
get '/auth_error', to: 'errors#auth_error'
get "/timeout", to: "errors#timeout"
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to have the name of the controller action in the to option, and then lets match the auth_error for the get

Suggested change
get "/timeout", to: "errors#timeout"
get "/timeout_error", to: "errors#timeout_error"

The reason the test didn't fail is because by default, even if there is no method in the controller, Rails renders the HTML template with the action name, which is timeout.html.erb. The html.erb should also be renamed to timeout_error.html.erb to match the controller (and the auth error).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tricky, I would prefer just a failure than a default loading that is non-obvious. Thanks for pointing it out, resolved.


#### DOWNLOADS ####

Expand Down
16 changes: 16 additions & 0 deletions spec/controllers/errors_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,4 +28,20 @@
expect(response.header["Content-Type"]).to eq("text/html; charset=utf-8")
end
end

describe "auth_error" do
it "returns an HTML auth error page" do
get :auth_error
expect(response.status).to eq(200)
expect(response.header["Content-Type"]).to eq("text/html; charset=utf-8")
end
end

describe "timeout" do
it "returns an HTML timeout error page" do
get :timeout
expect(response.status).to eq(200)
expect(response.header["Content-Type"]).to eq("text/html; charset=utf-8")
end
end
end
Loading