Merge pull request #2644 from alphagov/show-friendly-error-for-pdf-failures

Show better error message if precompiled letters can't be opened

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)
 

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)
 

app/templates/components/table.html

@@ -157,12 +157,12 @@
       {% if notification.status|format_notification_status_as_url(notification.notification_type) %}
         <a href="{{ 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) %}

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 %}
+
+    <h1 class="heading-large">Letter</h1>
+
+    <p>
+      Provided as PDF on {{ created_at|format_datetime_short }}
+    </p>
+    <p class="notification-status-cancelled">
+      Validation failed – this isn’t a PDF file that Notify can read
+    </p>
+{% endblock %}

app/templates/views/notifications/notification.html

@@ -16,7 +16,7 @@
 
     <p>
       {% if is_precompiled_letter %}
-      Provided as PDF, sent
+      Provided as PDF
       {% else %}
         {% if help %}
           ‘{{ template.name }}’
@@ -43,8 +43,7 @@
         </p>
       {% elif notification_status == 'validation-failed' %}
         <p class="notification-status-cancelled">
-          Cancelled {{ updated_at|format_datetime_short }}
-          (letter has content outside the printable area)
+          Validation failed – content is outside the printable area
         </p>
       {% else %}
         <p>

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,

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,