diff --git a/app/__init__.py b/app/__init__.py index 6ceb1fdbe..ac6386e02 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -1,4 +1,3 @@ -import itertools import os import re import urllib @@ -616,8 +615,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) @@ -628,15 +629,10 @@ def register_errorhandlers(application): # noqa (C901 too complex) error.message )) error_code = error.status_code - if error_code == 400: - if isinstance(error.message, str): - msg = [error.message] - else: - msg = list(itertools.chain(*[error.message[x] for x in error.message.keys()])) - resp = make_response(render_template("error/400.html", message=msg)) - return useful_headers_after_request(resp) - elif error_code not in [401, 404, 403, 410]: - # probably a 500 or 503 + if error_code not in [401, 404, 403, 410]: + # probably a 500 or 503. + # it might be a 400, which we should handle as if it's an internal server error. If the API might + # legitimately return a 400, we should handle that within the view or the client that calls it. application.logger.exception("API {} failed with status {} message {}".format( error.response.url if error.response else 'unknown', error.status_code, @@ -646,8 +642,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): @@ -690,19 +688,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, )