From bb7e9726d3cab72248ecebdf135bb6e0aa14faf9 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Mon, 7 Jan 2019 12:02:24 +0000 Subject: [PATCH 1/3] Stop showing validation-failed letters as cancelled in table Changed the table for displaying all notifications to show letters which have the status of 'validation-failed' as 'Validation failed' instead of 'Cancelled'. The individual notification page for a letter which has failed validation has not been changed since this already has a description (letter has content outside the printable area). --- app/__init__.py | 3 ++- app/templates/components/table.html | 4 ++-- tests/app/main/views/test_activity.py | 4 ++-- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 0221981a8..75837af07 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -373,7 +373,8 @@ def format_notification_status(status, template_type): 'pending-virus-check': 'Pending virus check', 'virus-scan-failed': 'Virus detected', 'returned-letter': 'Delivered', - 'cancelled': 'Cancelled,' + 'cancelled': 'Cancelled,', + 'validation-failed': 'Validation failed', } }[template_type].get(status, status) diff --git a/app/templates/components/table.html b/app/templates/components/table.html index cb5622c60..b4863e947 100644 --- a/app/templates/components/table.html +++ b/app/templates/components/table.html @@ -157,12 +157,12 @@ {% if notification.status|format_notification_status_as_url(notification.notification_type) %} {% endif %} - {% if notification['notification_type'] != "letter" or notification.status == 'virus-scan-failed' %} + {% if notification['notification_type'] != "letter" or notification.status in ('virus-scan-failed', 'validation-failed') %} {{ notification.status|format_notification_status( notification.template.template_type ) }} {% endif %} - {% if notification.notification_type == "letter" and notification.status in ['permanent-failure', 'validation-failed', 'cancelled'] %} + {% if notification.notification_type == "letter" and notification.status in ['permanent-failure', 'cancelled'] %} Cancelled {% endif %} {% if notification.status|format_notification_status_as_url(notification.notification_type) %} diff --git a/tests/app/main/views/test_activity.py b/tests/app/main/views/test_activity.py index b6c2f1a52..3e4fb7e10 100644 --- a/tests/app/main/views/test_activity.py +++ b/tests/app/main/views/test_activity.py @@ -602,10 +602,10 @@ def test_big_numbers_and_search_dont_show_for_letters( ('sms', 'delivered', 'Delivered 27 September at 5:31pm', True), ('letter', 'delivered', '27 September at 5:30pm', True), ('letter', 'permanent-failure', 'Cancelled 27 September at 5:31pm', False), - ('letter', 'validation-failed', 'Cancelled 27 September at 5:30pm', False), + ('letter', 'validation-failed', 'Validation failed 27 September at 5:30pm', False), ] ) -def test_sending_status_hint_does_not_include_status_for_letters( +def test_sending_status_hint_displays_correctly_on_notifications_page( client_request, service_one, active_user_with_permissions, From da50f7753857924982d6b3a0a934ee62a5ee509d Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Tue, 8 Jan 2019 17:34:57 +0000 Subject: [PATCH 2/3] Show error message if precompiled PDF cannot be opened If PDF files have a validation error which means that they can't be opened by PyPDF2 we would previously show the 500 status error page. We now catch PyPDF2.utils.PdfReadErrors so that we can display a custom error message on the notification page instead. --- app/main/views/notifications.py | 11 +++++-- .../invalid_precompiled_letter.html | 17 ++++++++++ tests/app/main/views/test_notifications.py | 31 +++++++++++++++++++ 3 files changed, 57 insertions(+), 2 deletions(-) create mode 100644 app/templates/views/notifications/invalid_precompiled_letter.html diff --git a/app/main/views/notifications.py b/app/main/views/notifications.py index 5b2a2d99e..1277da379 100644 --- a/app/main/views/notifications.py +++ b/app/main/views/notifications.py @@ -24,6 +24,7 @@ from notifications_utils.letter_timings import ( ) from notifications_utils.pdf import pdf_page_count from notifications_utils.timezones import utc_string_to_aware_gmt_datetime +from PyPDF2.utils import PdfReadError from app import ( _format_datetime_short, @@ -58,8 +59,14 @@ def view_notification(service_id, notification_id): personalisation = get_all_personalisation_from_notification(notification) if notification['template']['is_precompiled_letter']: - file_contents = view_letter_notification_as_preview(service_id, notification_id, "pdf") - page_count = pdf_page_count(io.BytesIO(file_contents)) + try: + file_contents = view_letter_notification_as_preview(service_id, notification_id, "pdf") + page_count = pdf_page_count(io.BytesIO(file_contents)) + except PdfReadError: + return render_template( + 'views/notifications/invalid_precompiled_letter.html', + created_at=notification['created_at'] + ) else: page_count = get_page_count_for_letter(notification['template'], values=personalisation) diff --git a/app/templates/views/notifications/invalid_precompiled_letter.html b/app/templates/views/notifications/invalid_precompiled_letter.html new file mode 100644 index 000000000..de26ec624 --- /dev/null +++ b/app/templates/views/notifications/invalid_precompiled_letter.html @@ -0,0 +1,17 @@ +{% extends "withnav_template.html" %} + +{% block service_page_title %} + Letter +{% endblock %} + +{% block maincolumn_content %} + +

Letter

+ +

+ Provided as PDF on {{ created_at|format_datetime_short }} +

+

+ Couldn’t read this file +

+{% endblock %} diff --git a/tests/app/main/views/test_notifications.py b/tests/app/main/views/test_notifications.py index 353c6bc94..c27a601bf 100644 --- a/tests/app/main/views/test_notifications.py +++ b/tests/app/main/views/test_notifications.py @@ -7,6 +7,7 @@ import pytest from flask import url_for from freezegun import freeze_time from notifications_python_client.errors import APIError +from PyPDF2.utils import PdfReadError from app.main.views.notifications import get_letter_printing_statement from tests.conftest import ( @@ -398,6 +399,36 @@ def test_should_show_preview_error_image_letter_notification_on_preview_error( assert response.get_data(as_text=True) == 'preview error image' +def test_notifification_page_shows_error_message_if_precompiled_letter_cannot_be_opened( + client_request, + mocker, + fake_uuid, +): + mock_get_notification( + mocker, + fake_uuid, + notification_status='validation-failed', + template_type='letter', + is_precompiled_letter=True, + ) + mocker.patch( + 'app.main.views.notifications.view_letter_notification_as_preview', + side_effect=PdfReadError() + ) + mocker.patch( + 'app.main.views.notifications.pdf_page_count', + side_effect=PdfReadError() + ) + page = client_request.get( + 'main.view_notification', + service_id=SERVICE_ONE_ID, + notification_id=fake_uuid, + ) + + error_message = page.find('p', class_='notification-status-cancelled').text + assert normalize_spaces(error_message) == "Couldn’t read this file" + + def test_should_404_for_unknown_extension( client_request, fake_uuid, From 1c6a71700d1ed85879acd161ffdff7229910ddb9 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Wed, 9 Jan 2019 13:09:51 +0000 Subject: [PATCH 3/3] Update error messages for validation-failed precompiled letters --- .../views/notifications/invalid_precompiled_letter.html | 2 +- app/templates/views/notifications/notification.html | 5 ++--- tests/app/main/views/test_notifications.py | 4 ++-- 3 files changed, 5 insertions(+), 6 deletions(-) diff --git a/app/templates/views/notifications/invalid_precompiled_letter.html b/app/templates/views/notifications/invalid_precompiled_letter.html index de26ec624..5e924c3e8 100644 --- a/app/templates/views/notifications/invalid_precompiled_letter.html +++ b/app/templates/views/notifications/invalid_precompiled_letter.html @@ -12,6 +12,6 @@ Provided as PDF on {{ created_at|format_datetime_short }}

- Couldn’t read this file + Validation failed – this isn’t a PDF file that Notify can read

{% endblock %} diff --git a/app/templates/views/notifications/notification.html b/app/templates/views/notifications/notification.html index 1149ef9db..cab6d4765 100644 --- a/app/templates/views/notifications/notification.html +++ b/app/templates/views/notifications/notification.html @@ -16,7 +16,7 @@

{% if is_precompiled_letter %} - Provided as PDF, sent + Provided as PDF {% else %} {% if help %} ‘{{ template.name }}’ @@ -43,8 +43,7 @@

{% elif notification_status == 'validation-failed' %}

- Cancelled {{ updated_at|format_datetime_short }} - (letter has content outside the printable area) + Validation failed – content is outside the printable area

{% else %}

diff --git a/tests/app/main/views/test_notifications.py b/tests/app/main/views/test_notifications.py index c27a601bf..b9f3a93de 100644 --- a/tests/app/main/views/test_notifications.py +++ b/tests/app/main/views/test_notifications.py @@ -202,7 +202,7 @@ def test_notification_page_shows_page_for_letter_notification( ), ( 'validation-failed', - 'Cancelled 1 January at 1:02am (letter has content outside the printable area)', + 'Validation failed – content is outside the printable area', ), )) @freeze_time("2016-01-01 01:01") @@ -426,7 +426,7 @@ def test_notifification_page_shows_error_message_if_precompiled_letter_cannot_be ) error_message = page.find('p', class_='notification-status-cancelled').text - assert normalize_spaces(error_message) == "Couldn’t read this file" + assert normalize_spaces(error_message) == "Validation failed – this isn’t a PDF file that Notify can read" def test_should_404_for_unknown_extension(