From be85010ed25342821c357cddeb4475d878553452 Mon Sep 17 00:00:00 2001 From: Ben Thorner <ben.thorner@digital.cabinet-office.gov.uk> Date: Tue, 12 May 2020 16:15:25 +0100 Subject: [PATCH 1/5] Add missing pages for known errors Previously the use of Slimmer meant that all unexpected responses were treated as '500 Internal Server Error', which is incorrect and misleading, since the fault is often with the client. This adds new error pages to cover all public error responses in the last 30 days. Using our logs, we found the following unhandled errors: - 499 is unnecessary, as the client closed the request - 405 can be similar to our current 403 - 401 can be similar to our current 403 - 429 is for rate limiting (new page) - 409 is unnecessary, as it's internal only (conflict) - 426 is only handled at the Nginx level --- app/views/root/401.html.erb | 6 ++++++ app/views/root/405.html.erb | 7 +++++++ app/views/root/429.html.erb | 7 +++++++ 3 files changed, 20 insertions(+) create mode 100644 app/views/root/401.html.erb create mode 100644 app/views/root/405.html.erb create mode 100644 app/views/root/429.html.erb diff --git a/app/views/root/401.html.erb b/app/views/root/401.html.erb new file mode 100644 index 000000000..601484320 --- /dev/null +++ b/app/views/root/401.html.erb @@ -0,0 +1,6 @@ +<%= render partial: "error_page", locals: { + title: "Unauthorized - 401", + heading: "You are not permitted to access this page", + intro: '<p>This page is restricted to certain users only.</p>', + status_code: 401 +} %> diff --git a/app/views/root/405.html.erb b/app/views/root/405.html.erb new file mode 100644 index 000000000..033f9e290 --- /dev/null +++ b/app/views/root/405.html.erb @@ -0,0 +1,7 @@ +<%= render partial: "error_page", locals: { + title: "Method Not Allowed - 405", + heading: "You are not permitted to perform this action", + intro: '<p>This page does not support the action you requested.</p>', + status_code: 405 +} %> + diff --git a/app/views/root/429.html.erb b/app/views/root/429.html.erb new file mode 100644 index 000000000..c3f21169f --- /dev/null +++ b/app/views/root/429.html.erb @@ -0,0 +1,7 @@ +<%= render partial: "error_page", locals: { + title: "Sorry, there was a problem", + heading: "Sorry, there was a problem", + intro: '<p>You have made too many requests to this page.</p> + <p>Try again later.</p>', + status_code: 429 +} %> From 59805f43c20e3b0add00fc55d622f7bab03d523c Mon Sep 17 00:00:00 2001 From: Ben Thorner <ben.thorner@digital.cabinet-office.gov.uk> Date: Wed, 13 May 2020 14:40:44 +0100 Subject: [PATCH 2/5] Move showing status code out of the title Previously we appended the status code onto the end of the browser title for some of the static error pages, which was inconsistent, and also restricted us from improving the content of these titles. This moves the status code into a visually separate area of the screen, so it can be quoted for reference. --- app/assets/stylesheets/error-page-print.scss | 1 + app/assets/stylesheets/error-page.scss | 1 + app/views/root/400.html.erb | 2 +- app/views/root/401.html.erb | 2 +- app/views/root/403.html.erb | 2 +- app/views/root/404.html.erb | 2 +- app/views/root/405.html.erb | 2 +- app/views/root/406.html.erb | 2 +- app/views/root/410.html.erb | 2 +- app/views/root/_error_page.html.erb | 4 ++++ test/integration/templates/error_4xx_test.rb | 5 ++--- test/integration/templates/error_5xx_test.rb | 5 ++--- 12 files changed, 17 insertions(+), 13 deletions(-) diff --git a/app/assets/stylesheets/error-page-print.scss b/app/assets/stylesheets/error-page-print.scss index 6c495388d..6a746f354 100644 --- a/app/assets/stylesheets/error-page-print.scss +++ b/app/assets/stylesheets/error-page-print.scss @@ -8,3 +8,4 @@ $is-print: true; @import "govuk_publishing_components/component_support"; @import "govuk_publishing_components/components/print/button"; @import "govuk_publishing_components/components/print/feedback"; +@import "govuk_publishing_components/components/_warning-text"; diff --git a/app/assets/stylesheets/error-page.scss b/app/assets/stylesheets/error-page.scss index f93aff394..a5a5cf543 100644 --- a/app/assets/stylesheets/error-page.scss +++ b/app/assets/stylesheets/error-page.scss @@ -6,3 +6,4 @@ $govuk-use-legacy-palette: false; @import "govuk_publishing_components/components/_cookie-banner"; @import "govuk_publishing_components/components/_button"; @import "govuk_publishing_components/components/_feedback"; +@import "govuk_publishing_components/components/_warning-text"; diff --git a/app/views/root/400.html.erb b/app/views/root/400.html.erb index 324cba261..59259439c 100644 --- a/app/views/root/400.html.erb +++ b/app/views/root/400.html.erb @@ -1,5 +1,5 @@ <%= render partial: "error_page", locals: { - title: "Bad request - 400", + title: "Bad request", heading: "Bad request", intro: '', status_code: 400 diff --git a/app/views/root/401.html.erb b/app/views/root/401.html.erb index 601484320..e5af575bf 100644 --- a/app/views/root/401.html.erb +++ b/app/views/root/401.html.erb @@ -1,5 +1,5 @@ <%= render partial: "error_page", locals: { - title: "Unauthorized - 401", + title: "Unauthorized", heading: "You are not permitted to access this page", intro: '<p>This page is restricted to certain users only.</p>', status_code: 401 diff --git a/app/views/root/403.html.erb b/app/views/root/403.html.erb index 5952dbf38..b9a00efca 100644 --- a/app/views/root/403.html.erb +++ b/app/views/root/403.html.erb @@ -1,5 +1,5 @@ <%= render partial: "error_page", locals: { - title: "Forbidden - 403", + title: "Forbidden", heading: "You are not permitted to see this page", intro: '<p>This page is restricted to certain users only.</p>', status_code: 403 diff --git a/app/views/root/404.html.erb b/app/views/root/404.html.erb index d52d97a94..5509e572f 100644 --- a/app/views/root/404.html.erb +++ b/app/views/root/404.html.erb @@ -1,5 +1,5 @@ <%= render partial: "error_page", locals: { - title: "Page not found - 404", + title: "Page not found", heading: "Page not found", intro: '<p>If you entered a web address please check it was correct.</p> diff --git a/app/views/root/405.html.erb b/app/views/root/405.html.erb index 033f9e290..d9277f5e7 100644 --- a/app/views/root/405.html.erb +++ b/app/views/root/405.html.erb @@ -1,5 +1,5 @@ <%= render partial: "error_page", locals: { - title: "Method Not Allowed - 405", + title: "Method not allowed", heading: "You are not permitted to perform this action", intro: '<p>This page does not support the action you requested.</p>', status_code: 405 diff --git a/app/views/root/406.html.erb b/app/views/root/406.html.erb index 3a2ca800f..1096436c9 100644 --- a/app/views/root/406.html.erb +++ b/app/views/root/406.html.erb @@ -1,5 +1,5 @@ <%= render partial: "error_page", locals: { - title: "Page not found - 406", + title: "Page not found", heading: "The page you are looking for can't be found", intro: '<p>Please check that you have entered the correct web address, or try using the search box above or explore <a class="govuk-link" href="/">GOV.UK</a> to find the information you need.</p>', status_code: 406 diff --git a/app/views/root/410.html.erb b/app/views/root/410.html.erb index f5b7e0a63..685f58e5a 100644 --- a/app/views/root/410.html.erb +++ b/app/views/root/410.html.erb @@ -1,5 +1,5 @@ <%= render partial: "error_page", locals: { - title: "Page no longer here - 410", + title: "Page no longer here", heading: "The page you are looking for is no longer here", intro: '<p>It may have been moved or replaced.</p> <p>Please check that you have entered the correct web address, or try using the search box above or explore <a class="govuk-link" href="/">GOV.UK</a> to find the information you need.</p>', diff --git a/app/views/root/_error_page.html.erb b/app/views/root/_error_page.html.erb index 64e5d4d95..6cb232bed 100644 --- a/app/views/root/_error_page.html.erb +++ b/app/views/root/_error_page.html.erb @@ -27,6 +27,10 @@ <div class="article-container"> <article role="article" class="group"> <div class="inner"> + <%= render "govuk_publishing_components/components/warning_text", { + text: "Status code: #{status_code}" + } %> + <% if local_assigns.include?(:intro) %> <%= raw intro %> <% else %> diff --git a/test/integration/templates/error_4xx_test.rb b/test/integration/templates/error_4xx_test.rb index 86f5a579a..0c208dfdc 100644 --- a/test/integration/templates/error_4xx_test.rb +++ b/test/integration/templates/error_4xx_test.rb @@ -6,8 +6,7 @@ class Error4XXTest < ActionDispatch::IntegrationTest assert page.has_selector?("body.mainstream.error") within "head", visible: :all do - assert page.has_selector?("title", text: "Page not found - 404 - GOV.UK", visible: :all) - + assert page.has_selector?("title", text: "Page not found - GOV.UK", visible: :all) assert page.has_selector?("link[href='/static/static.css']", visible: :all) end @@ -21,11 +20,11 @@ class Error4XXTest < ActionDispatch::IntegrationTest within "#wrapper" do assert page.has_selector?("h1", text: "Page not found") + assert page.has_selector?(".govuk-warning-text__text", text: "Status code: 404", visible: :all) end within "footer" do assert page.has_selector?(".footer-categories") - assert page.has_selector?(".footer-meta") end end diff --git a/test/integration/templates/error_5xx_test.rb b/test/integration/templates/error_5xx_test.rb index 402147ef4..f92251246 100644 --- a/test/integration/templates/error_5xx_test.rb +++ b/test/integration/templates/error_5xx_test.rb @@ -6,8 +6,7 @@ class Error5XXTest < ActionDispatch::IntegrationTest assert page.has_selector?("body.mainstream.error") within "head", visible: :all do - assert page.has_selector?("title", text: "Sorry, we're experiencing technical difficulties (500 error) - GOV.UK", visible: :all) - + assert page.has_selector?("title", text: "Sorry, we're experiencing technical difficulties - GOV.UK", visible: :all) assert page.has_selector?("link[href='/static/static.css']", visible: :all) end @@ -21,11 +20,11 @@ class Error5XXTest < ActionDispatch::IntegrationTest within "#wrapper" do assert page.has_selector?("h1", text: "Sorry, we're experiencing technical difficulties") + assert page.has_selector?(".govuk-warning-text__text", text: "Status code: 500", visible: :all) end within "footer" do assert page.has_selector?(".footer-categories") - assert page.has_selector?(".footer-meta") end end From 40f0e301a9e452768b89af65fa6dd5751420b875 Mon Sep 17 00:00:00 2001 From: Ben Thorner <ben.thorner@digital.cabinet-office.gov.uk> Date: Wed, 13 May 2020 14:46:30 +0100 Subject: [PATCH 3/5] Make 5xx errors consistent with 4xx errors Previously we defined content for 4xx errors explicitly in each template, and the content for 5xx errors using conditionals in the 'error_page' partial, which was confusing. This duplicates the 5xx content into each error template, noting that the duplication is incidental, and making this change allows us to iterate it. --- app/views/root/500.html.erb | 7 ++++++- app/views/root/501.html.erb | 7 ++++++- app/views/root/503.html.erb | 7 ++++++- app/views/root/504.html.erb | 7 ++++++- app/views/root/_error_page.html.erb | 13 ++----------- 5 files changed, 26 insertions(+), 15 deletions(-) diff --git a/app/views/root/500.html.erb b/app/views/root/500.html.erb index 5bfecfae5..fae1ec2bf 100644 --- a/app/views/root/500.html.erb +++ b/app/views/root/500.html.erb @@ -1 +1,6 @@ -<%= render partial: "error_page", locals: {status_code: 500} %> +<%= render partial: "error_page", locals: { + title: "Sorry, we're experiencing technical difficulties", + heading: "Sorry, we're experiencing technical difficulties", + intro: "<p>Please try again in a few moments.</p>", + status_code: 500 +} %> diff --git a/app/views/root/501.html.erb b/app/views/root/501.html.erb index e08aa4a2f..d2f20ec4b 100644 --- a/app/views/root/501.html.erb +++ b/app/views/root/501.html.erb @@ -1 +1,6 @@ -<%= render partial: "error_page", locals: {status_code: 501} %> +<%= render partial: "error_page", locals: { + title: "Sorry, we're experiencing technical difficulties", + heading: "Sorry, we're experiencing technical difficulties", + intro: "<p>Please try again in a few moments.</p>", + status_code: 501 +} %> diff --git a/app/views/root/503.html.erb b/app/views/root/503.html.erb index cee06cdb3..a4297ad23 100644 --- a/app/views/root/503.html.erb +++ b/app/views/root/503.html.erb @@ -1 +1,6 @@ -<%= render partial: "error_page", locals: {status_code: 503} %> +<%= render partial: "error_page", locals: { + title: "Sorry, we're experiencing technical difficulties", + heading: "Sorry, we're experiencing technical difficulties", + intro: "<p>Please try again in a few moments.</p>", + status_code: 503 +} %> diff --git a/app/views/root/504.html.erb b/app/views/root/504.html.erb index 02fce4d2f..ae42e4b3a 100644 --- a/app/views/root/504.html.erb +++ b/app/views/root/504.html.erb @@ -1 +1,6 @@ -<%= render partial: "error_page", locals: {status_code: 504} %> +<%= render partial: "error_page", locals: { + title: "Sorry, we're experiencing technical difficulties", + heading: "Sorry, we're experiencing technical difficulties", + intro: "<p>Please try again in a few moments.</p>", + status_code: 504 +} %> diff --git a/app/views/root/_error_page.html.erb b/app/views/root/_error_page.html.erb index 6cb232bed..0084b88fb 100644 --- a/app/views/root/_error_page.html.erb +++ b/app/views/root/_error_page.html.erb @@ -17,11 +17,7 @@ <header class="page-header group"> <div> - <% if local_assigns.include?(:heading) %> - <h1><%= heading %></h1> - <% else %> - <h1>Sorry, we're experiencing technical difficulties</h1> - <% end %> + <h1><%= heading %></h1> </div> </header> <div class="article-container"> @@ -31,12 +27,7 @@ text: "Status code: #{status_code}" } %> - <% if local_assigns.include?(:intro) %> - <%= raw intro %> - <% else %> - <p>Please try again in a few moments.</p> - <% end %> - + <%= raw intro %> <p>You can <a class="govuk-link" href="/coronavirus">find coronavirus information</a> on GOV.UK.</p> </div> </article> From 4182a77747044a8ef362f48870c4b3768740c458 Mon Sep 17 00:00:00 2001 From: Ben Thorner <ben.thorner@digital.cabinet-office.gov.uk> Date: Wed, 13 May 2020 14:48:52 +0100 Subject: [PATCH 4/5] Allow users to give feedback on 5xx pages It's unclear why we should not offer this facility, when it may help us improve our error pages. --- app/views/root/_error_page.html.erb | 3 --- 1 file changed, 3 deletions(-) diff --git a/app/views/root/_error_page.html.erb b/app/views/root/_error_page.html.erb index 0084b88fb..cf7177963 100644 --- a/app/views/root/_error_page.html.erb +++ b/app/views/root/_error_page.html.erb @@ -14,7 +14,6 @@ <% content_for :wrapper_content do %> <main id="content" role="main" class="group"> - <header class="page-header group"> <div> <h1><%= heading %></h1> @@ -33,9 +32,7 @@ </article> </div> -<% if status_code.in?(400..499) %> <%= render "govuk_publishing_components/components/feedback" %> -<% end %> <% if status_code %> <script type="text/javascript"> From 405b02dfee9be07b0e840c2a1bb6f2d943fa3fd5 Mon Sep 17 00:00:00 2001 From: Ben Thorner <ben.thorner@digital.cabinet-office.gov.uk> Date: Wed, 13 May 2020 14:50:42 +0100 Subject: [PATCH 5/5] Remove redundant ERB conditional All uses of the 'error_page' partial include a 'status_code'. --- app/views/root/_error_page.html.erb | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/views/root/_error_page.html.erb b/app/views/root/_error_page.html.erb index cf7177963..d322b8daa 100644 --- a/app/views/root/_error_page.html.erb +++ b/app/views/root/_error_page.html.erb @@ -34,11 +34,9 @@ <%= render "govuk_publishing_components/components/feedback" %> -<% if status_code %> <script type="text/javascript"> window.httpStatusCode = '<%= status_code %>'; </script> -<% end %> </main> <% end %>