From 2ab07195013605c3db2fb8e3b729b860bb3b4484 Mon Sep 17 00:00:00 2001 From: rhymes Date: Mon, 7 Dec 2020 18:55:41 +0100 Subject: [PATCH] Use custom, dynamic, error pages (#11744) * Use ErrorsController to render error pages * Hide GitHub bug report prompt from anything other than dev.to and forem.dev * Update service worker to use 404 and 500 * Other fixes --- app/controllers/errors_controller.rb | 27 ++++++++++ app/models/comment.rb | 2 +- .../errors/internal_server_error.html.erb | 14 ++++- .../views/errors/not_found.html.erb | 0 .../views/errors/service_unavailable.html.erb | 0 .../errors/unprocessable_entity.html.erb | 1 - app/views/service_worker/index.js.erb | 16 +++--- config/application.rb | 4 ++ config/routes.rb | 6 +++ spec/requests/errors_request_spec.rb | 53 +++++++++++++++++++ 10 files changed, 111 insertions(+), 12 deletions(-) create mode 100644 app/controllers/errors_controller.rb rename public/500.html => app/views/errors/internal_server_error.html.erb (82%) rename public/404.html => app/views/errors/not_found.html.erb (100%) rename public/503.html => app/views/errors/service_unavailable.html.erb (100%) rename public/422.html => app/views/errors/unprocessable_entity.html.erb (97%) create mode 100644 spec/requests/errors_request_spec.rb diff --git a/app/controllers/errors_controller.rb b/app/controllers/errors_controller.rb new file mode 100644 index 000000000..65781559f --- /dev/null +++ b/app/controllers/errors_controller.rb @@ -0,0 +1,27 @@ +class ErrorsController < ApplicationController + GITHUB_BUG_REPORT_DOMAINS = ["dev.to", "forem.dev"].freeze + GITHUB_BUG_REPORT_URL = "https://github.com/forem/forem/issues/new?template=bug_report.md".freeze + + # HTTP 404 - Not Found - https://httpstatuses.com/400 + def not_found + render status: :not_found + end + + # HTTP 422 - Unprocessable Entity - https://httpstatuses.com/422 + def unprocessable_entity + render status: :unprocessable_entity + end + + # HTTP 500 - Internal Server Error - https://httpstatuses.com/500 + def internal_server_error + @github_bug_report_url = GITHUB_BUG_REPORT_URL + @display_github_bug_report_url = SiteConfig.app_domain.in?(GITHUB_BUG_REPORT_DOMAINS) + + render status: :internal_server_error + end + + # HTTP 503 - Service Unavailable - https://httpstatuses.com/503 + def service_unavailable + render status: :service_unavailable + end +end diff --git a/app/models/comment.rb b/app/models/comment.rb index 06f4b80dc..ae597f8d9 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -75,7 +75,7 @@ class Comment < ApplicationRecord def path "/#{user.username}/comment/#{id_code_generated}" rescue StandardError - "/404.html" + "/404" end def parent_or_root_article diff --git a/public/500.html b/app/views/errors/internal_server_error.html.erb similarity index 82% rename from public/500.html rename to app/views/errors/internal_server_error.html.erb index 0d40386e9..8fc95f755 100644 --- a/public/500.html +++ b/app/views/errors/internal_server_error.html.erb @@ -62,7 +62,6 @@ -
@@ -70,7 +69,18 @@

Ooooops...

You have encountered an error.

-

Our team has been notified. Please feel welcome to also submit a public bug report.

We apologize for any inconvenience.

+ +

+ Our team has been notified. + + <% if @display_github_bug_report_url %> + Please feel welcome to also submit a public + bug report. + <% end %> + +

+ We apologize for any inconvenience. +

diff --git a/public/404.html b/app/views/errors/not_found.html.erb similarity index 100% rename from public/404.html rename to app/views/errors/not_found.html.erb diff --git a/public/503.html b/app/views/errors/service_unavailable.html.erb similarity index 100% rename from public/503.html rename to app/views/errors/service_unavailable.html.erb diff --git a/public/422.html b/app/views/errors/unprocessable_entity.html.erb similarity index 97% rename from public/422.html rename to app/views/errors/unprocessable_entity.html.erb index a21f82b3b..8c5e0802b 100644 --- a/public/422.html +++ b/app/views/errors/unprocessable_entity.html.erb @@ -55,7 +55,6 @@ -

The change you wanted was rejected.

diff --git a/app/views/service_worker/index.js.erb b/app/views/service_worker/index.js.erb index 4ec5a783b..d6940167f 100644 --- a/app/views/service_worker/index.js.erb +++ b/app/views/service_worker/index.js.erb @@ -15,12 +15,12 @@ event.waitUntil( caches.open(staticCacheName) .then(cache => cache.addAll([ - "/shell_top", // head, top bar, inline styles - "/shell_bottom", // footer - "/async_info/shell_version", // For comparing changes in the shell. Should be incremented with style changes. - "/404.html", // Not found page - "/500.html", // Error page - "/offline.html" //Offline page + "/shell_top", // head, top bar, inline styles + "/shell_bottom", // footer + "/async_info/shell_version", // For comparing changes in the shell. Should be incremented with style changes. + "/404", // Not found page + "/500", // Error page + "/offline.html" // Offline page ])) ); }); @@ -54,10 +54,10 @@ const endFetch = Promise.resolve(cachedShellBottom); const middleFetch = fetch(url).then(response => { if (!response.ok && response.status === 404) { - return caches.match('/404.html'); + return caches.match('/404'); } if (!response.ok && response.status != 404) { - return caches.match('/500.html'); + return caches.match('/500'); } return response; }).catch(err => caches.match('/offline.html')); diff --git a/config/application.rb b/config/application.rb index 5fbab3654..3b1316797 100644 --- a/config/application.rb +++ b/config/application.rb @@ -67,6 +67,10 @@ module PracticalDeveloper # To improve security, Rails embeds the purpose and expiry metadata inside encrypted or signed cookies value. config.action_dispatch.use_cookies_with_metadata = false + # Sets the exceptions application invoked by the ShowException middleware when an exception happens + # Defaults to ActionDispatch::PublicExceptions.new(Rails.public_path) + config.exceptions_app = routes # use ErrorsController to handle error pages + # After-initialize checker to add routes to reserved words config.after_initialize do Rails.application.reload_routes! diff --git a/config/routes.rb b/config/routes.rb index dd44e41a7..0fdc4e46c 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -1,6 +1,12 @@ # rubocop:disable Metrics/BlockLength Rails.application.routes.draw do + # errors routes + match "/404", to: "errors#not_found", via: :all, as: :errors_not_found + match "/422", to: "errors#unprocessable_entity", via: :all, as: :errors_unprocessable_entity + match "/500", to: "errors#internal_server_error", via: :all, as: :errors_internal_server_error + match "/503", to: "errors#service_unavailable", via: :all, as: :errors_service_unavailable + use_doorkeeper do controllers tokens: "oauth/tokens" end diff --git a/spec/requests/errors_request_spec.rb b/spec/requests/errors_request_spec.rb new file mode 100644 index 000000000..7cb5ac107 --- /dev/null +++ b/spec/requests/errors_request_spec.rb @@ -0,0 +1,53 @@ +require "rails_helper" + +RSpec.describe "Errors", type: :request do + describe "GET /404" do + it "returns not found error" do + get errors_not_found_path + + expect(response).to have_http_status(:not_found) + end + end + + describe "GET /422" do + it "returns unprocessable entity error" do + get errors_unprocessable_entity_path + + expect(response).to have_http_status(:unprocessable_entity) + end + end + + describe "GET /500" do + it "returns internal server error" do + get errors_internal_server_error_path + + expect(response).to have_http_status(:internal_server_error) + end + + it "does not include the prompt to report on Forem GitHub" do + allow(SiteConfig).to receive(:app_domain).and_return("example.com") + + get errors_internal_server_error_path + + expect(response).to have_http_status(:internal_server_error) + expect(response.body).not_to include("github.com/forem/forem/issues") + end + + it "includes the prompt to report on Forem GitHub for DEV" do + allow(SiteConfig).to receive(:app_domain).and_return("dev.to") + + get errors_internal_server_error_path + + expect(response).to have_http_status(:internal_server_error) + expect(response.body).to include("github.com/forem/forem/issues") + end + end + + describe "GET /503" do + it "returns service unavailable error" do + get errors_service_unavailable_path + + expect(response).to have_http_status(:service_unavailable) + end + end +end