Merge pull request #3320 from alphagov/remove-generic-400-error-page

remove admin 400 error handler

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):
+    def _error_response(error_code, error_page_template=None):
-        resp = make_response(render_template("error/{0}.html".format(error_code)), error_code)
+        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 error_code not in [401, 404, 403, 410]:
-            if isinstance(error.message, str):
+            # probably a 500 or 503.
-                msg = [error.message]
+            # it might be a 400, which we should handle as if it's an internal server error. If the API might
-            else:
+            # legitimately return a 400, we should handle that within the view or the client that calls it.
-                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
             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):
+    def handle_client_error(error):
-        return _error_response(400)
+        # 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(
+        return _error_response(400, error_page_template=500)
-            "error/400.html",
-            message=['Something went wrong, please go back and try again.']
-        ), 400)
-        return useful_headers_after_request(resp)
 
     @application.errorhandler(405)
-    def handle_405(error):
+    def handle_method_not_allowed(error):
-        resp = make_response(render_template(
+        return _error_response(405, error_page_template=500)
-            "error/400.html",
-            message=['Something went wrong, please go back and try again.']
-        ), 405)
-        return useful_headers_after_request(resp)
 
     @application.errorhandler(WerkzeugHTTPException)
     def handle_http_error(error):

app/main/views/uploads.py

@@ -234,7 +234,6 @@ def uploaded_letter_preview(service_id, file_id):
 @main.route("/services/<uuid:service_id>/preview-letter-image/<uuid:file_id>")
 @user_has_permissions('send_messages')
 def view_letter_upload_as_preview(service_id, file_id):
-
     try:
         page = int(request.args.get('page'))
     except ValueError:

app/templates/error/400.html

@@ -1,11 +0,0 @@
-{% extends "withoutnav_template.html" %}
-{% block per_page_title %}Bad request{% endblock %}
-{% block maincolumn_content %}
-<div class="grid-row">
-    <div class="column-two-thirds">
-      <h1 class="heading-medium">
-            {{ message|join(",")}}
-        </h1>
-    </div>
-</div>
-{% endblock %}

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.h1.string.strip() == 'Sorry, there’s a problem with GOV.UK Notify'
-    assert page.title.string.strip() == 'Bad request – 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.h1.string.strip() == 'Sorry, there’s a problem with GOV.UK Notify'
-    assert page.title.string.strip() == 'Bad request – GOV.UK Notify'
+    assert page.title.string.strip() == 'Sorry, there’s a problem with the service – GOV.UK Notify'

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,
     )