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/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/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/app/templates/views/notifications/invalid_precompiled_letter.html b/app/templates/views/notifications/invalid_precompiled_letter.html new file mode 100644 index 000000000..5e924c3e8 --- /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 }} +

+

+ 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_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, diff --git a/tests/app/main/views/test_notifications.py b/tests/app/main/views/test_notifications.py index 353c6bc94..b9f3a93de 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 ( @@ -201,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") @@ -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) == "Validation failed – this isn’t a PDF file that Notify can read" + + def test_should_404_for_unknown_extension( client_request, fake_uuid,