From d83250c7c2608d2a6f1d4e0dd5d983d46ac430e3 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 20 Feb 2020 17:46:48 +0000 Subject: [PATCH] remove the generic 400 error page handler it just shows a h1, so isn't helpful for people. We can re-use the 500 error page, which includes instructings "Try again later" and instructions on what to do next (check the status page, email notify support). this required refactoring to ensure we can show the 500 error page while still returning the required status code --- app/__init__.py | 27 +++++++++++---------------- app/main/views/uploads.py | 1 - app/templates/error/400.html | 11 ----------- tests/app/main/test_errorhandlers.py | 8 ++++---- tests/app/main/views/test_uploads.py | 1 + 5 files changed, 16 insertions(+), 32 deletions(-) delete mode 100644 app/templates/error/400.html diff --git a/app/__init__.py b/app/__init__.py index 10c82a8a1..11e7f72c0 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -1,4 +1,3 @@ -import itertools import os import urllib from datetime import datetime, timedelta, timezone @@ -602,8 +601,10 @@ def useful_headers_after_request(response): def register_errorhandlers(application): # noqa (C901 too complex) - def _error_response(error_code): - resp = make_response(render_template("error/{0}.html".format(error_code)), error_code) + def _error_response(error_code, error_page_template=None): + if error_page_template is None: + error_page_template = error_code + resp = make_response(render_template("error/{0}.html".format(error_page_template)), error_code) return useful_headers_after_request(resp) @application.errorhandler(HTTPError) @@ -627,8 +628,10 @@ def register_errorhandlers(application): # noqa (C901 too complex) return _error_response(error_code) @application.errorhandler(400) - def handle_400(error): - return _error_response(400) + def handle_client_error(error): + # This is tripped if we call `abort(400)`. + application.logger.exception('Unhandled 400 client error') + return _error_response(400, error_page_template=500) @application.errorhandler(410) def handle_gone(error): @@ -671,19 +674,11 @@ def register_errorhandlers(application): # noqa (C901 too complex) u'csrf.invalid_token: Aborting request, user_id: {user_id}', extra={'user_id': session['user_id']}) - resp = make_response(render_template( - "error/400.html", - message=['Something went wrong, please go back and try again.'] - ), 400) - return useful_headers_after_request(resp) + return _error_response(400, error_page_template=500) @application.errorhandler(405) - def handle_405(error): - resp = make_response(render_template( - "error/400.html", - message=['Something went wrong, please go back and try again.'] - ), 405) - return useful_headers_after_request(resp) + def handle_method_not_allowed(error): + return _error_response(405, error_page_template=500) @application.errorhandler(WerkzeugHTTPException) def handle_http_error(error): diff --git a/app/main/views/uploads.py b/app/main/views/uploads.py index 60c28c894..332b6f9aa 100644 --- a/app/main/views/uploads.py +++ b/app/main/views/uploads.py @@ -234,7 +234,6 @@ def uploaded_letter_preview(service_id, file_id): @main.route("/services//preview-letter-image/") @user_has_permissions('send_messages') def view_letter_upload_as_preview(service_id, file_id): - try: page = int(request.args.get('page')) except ValueError: diff --git a/app/templates/error/400.html b/app/templates/error/400.html deleted file mode 100644 index f874031c1..000000000 --- a/app/templates/error/400.html +++ /dev/null @@ -1,11 +0,0 @@ -{% extends "withoutnav_template.html" %} -{% block per_page_title %}Bad request{% endblock %} -{% block maincolumn_content %} -
-
-

- {{ message|join(",")}} -

-
-
-{% endblock %} diff --git a/tests/app/main/test_errorhandlers.py b/tests/app/main/test_errorhandlers.py index 6c52612bb..eeb6bac00 100644 --- a/tests/app/main/test_errorhandlers.py +++ b/tests/app/main/test_errorhandlers.py @@ -51,8 +51,8 @@ def test_csrf_returns_400(logged_in_client, mocker): assert response.status_code == 400 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - assert page.h1.string.strip() == 'Something went wrong, please go back and try again.' - assert page.title.string.strip() == 'Bad request – GOV.UK Notify' + assert page.h1.string.strip() == 'Sorry, there’s a problem with GOV.UK Notify' + assert page.title.string.strip() == 'Sorry, there’s a problem with the service – GOV.UK Notify' def test_csrf_redirects_to_sign_in_page_if_not_signed_in(client, mocker): @@ -70,5 +70,5 @@ def test_405_returns_something_went_wrong_page(client, mocker): assert response.status_code == 405 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - assert page.h1.string.strip() == 'Something went wrong, please go back and try again.' - assert page.title.string.strip() == 'Bad request – GOV.UK Notify' + assert page.h1.string.strip() == 'Sorry, there’s a problem with GOV.UK Notify' + assert page.title.string.strip() == 'Sorry, there’s a problem with the service – GOV.UK Notify' diff --git a/tests/app/main/views/test_uploads.py b/tests/app/main/views/test_uploads.py index b4d1c9804..081e2fe35 100644 --- a/tests/app/main/views/test_uploads.py +++ b/tests/app/main/views/test_uploads.py @@ -504,6 +504,7 @@ def test_uploaded_letter_preview_image_400s_for_bad_page_type( file_id=fake_uuid, service_id=SERVICE_ONE_ID, page='foo', + _test_page_title=False, _expected_status=400, )