From 6995ddc8f5a71a5af8d4bafaebe34713718ac530 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 10 Nov 2017 10:10:34 +0000 Subject: [PATCH 1/2] =?UTF-8?q?Don=E2=80=99t=20list=20all=20letter=20addre?= =?UTF-8?q?ss=20cols=20if=20some=20missing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If you’ve spelt ‘postcode’ wrong, or missed only ‘address_line_2’ then it’s pretty noisy to be told that your file needs columns called address line 1, address line 2, and postcode. It’s better to be specific about which column you need to fix in order to get past this error. As a principle, we’ve found it better to tell get people to fix one error at a time, rather than overwhelm them with a list of errors to correct – this is why we split the recipient column errors out separately in the first place. --- app/templates/views/check/column-errors.html | 5 ++++- tests/app/main/views/test_send.py | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/app/templates/views/check/column-errors.html b/app/templates/views/check/column-errors.html index 4b3d788ae..553449600 100644 --- a/app/templates/views/check/column-errors.html +++ b/app/templates/views/check/column-errors.html @@ -48,7 +48,10 @@ {% elif not recipients.has_recipient_columns %}

- Your file needs {{ required_recipient_columns | formatted_list( + Your file needs {{ ( + recipients.missing_column_headers + if template.template_type == 'letter' else required_recipient_columns + ) | formatted_list( prefix='a column called', prefix_plural='columns called' ) }} diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 33363bd60..7b9cb45ec 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -1883,7 +1883,7 @@ def test_check_messages_column_error_doesnt_show_optional_columns( ) assert normalize_spaces(page.select_one('.banner-dangerous').text) == ( - 'Your file needs columns called ‘address line 1’, ‘address line 2’ and ‘postcode’ ' + 'Your file needs a column called ‘postcode’ ' 'Right now it has columns called ‘address_line_1’, ‘address_line_2’ and ‘foo’. ' 'Skip to file contents' ) From 23a1c388f3f718922ba0289c0010065017275a31 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 10 Nov 2017 08:42:49 +0000 Subject: [PATCH 2/2] Add a link to download a PDF of a sent letter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is useful for service teams to keep a copy for their records. The letter won’t be available in Notify once the 7 day retention period has passed. --- app/main/views/notifications.py | 16 ++-- .../views/notifications/notification.html | 3 + tests/app/main/views/test_notifications.py | 76 ++++++++++++++++--- 3 files changed, 79 insertions(+), 16 deletions(-) diff --git a/app/main/views/notifications.py b/app/main/views/notifications.py index cb52d5b34..5b448e534 100644 --- a/app/main/views/notifications.py +++ b/app/main/views/notifications.py @@ -1,5 +1,6 @@ # -*- coding: utf-8 -*- from flask import ( + abort, render_template, jsonify, request, @@ -51,9 +52,10 @@ def view_notification(service_id, notification_id): notification['template'], current_service, letter_preview_url=url_for( - '.view_letter_notification_as_image', + '.view_letter_notification_as_preview', service_id=service_id, notification_id=notification_id, + filetype='png', ), show_recipient=True, redact_missing_personalisation=True, @@ -87,10 +89,13 @@ def view_notification(service_id, notification_id): ) -@main.route("/services//notification/.png") +@main.route("/services//notification/.") @login_required @user_has_permissions('view_activity', admin_override=True) -def view_letter_notification_as_image(service_id, notification_id): +def view_letter_notification_as_preview(service_id, notification_id, filetype): + + if filetype not in ('pdf', 'png'): + abort(404) notification = notification_api_client.get_notification(service_id, notification_id) @@ -98,15 +103,16 @@ def view_letter_notification_as_image(service_id, notification_id): notification['template'], current_service, letter_preview_url=url_for( - '.view_letter_notification_as_image', + '.view_letter_notification_as_preview', service_id=service_id, notification_id=notification_id, + filetype='png', ), ) template.values = notification['personalisation'] - return TemplatePreview.from_utils_template(template, 'png', page=request.args.get('page')) + return TemplatePreview.from_utils_template(template, filetype, page=request.args.get('page')) @main.route("/services//notification/.json") diff --git a/app/templates/views/notifications/notification.html b/app/templates/views/notifications/notification.html index 7c1592cfc..f0ef58dba 100644 --- a/app/templates/views/notifications/notification.html +++ b/app/templates/views/notifications/notification.html @@ -34,6 +34,9 @@

Estimated delivery date: {{ estimated_letter_delivery_date|string|format_date_short }}

+

+ Download as a PDF +

{% endif %} {{ template|string }} diff --git a/tests/app/main/views/test_notifications.py b/tests/app/main/views/test_notifications.py index d86507872..ff2e94890 100644 --- a/tests/app/main/views/test_notifications.py +++ b/tests/app/main/views/test_notifications.py @@ -1,5 +1,6 @@ from freezegun import freeze_time from flask import url_for +from functools import partial import pytest from notifications_utils.template import LetterImageTemplate @@ -131,10 +132,14 @@ def test_notification_page_shows_status_of_letter_notification( assert page.select('p.notification-status') == [] +@pytest.mark.parametrize('filetype', [ + 'pdf', 'png' +]) def test_should_show_image_of_letter_notification( logged_in_client, fake_uuid, - mocker + mocker, + filetype, ): mock_get_notification(mocker, fake_uuid, template_type='letter') @@ -145,15 +150,29 @@ def test_should_show_image_of_letter_notification( ) response = logged_in_client.get(url_for( - 'main.view_letter_notification_as_image', + 'main.view_letter_notification_as_preview', service_id=SERVICE_ONE_ID, notification_id=fake_uuid, + filetype=filetype )) assert response.status_code == 200 assert response.get_data(as_text=True) == 'foo' assert isinstance(mocked_preview.call_args[0][0], LetterImageTemplate) - assert mocked_preview.call_args[0][1] == 'png' + assert mocked_preview.call_args[0][1] == filetype + + +def test_should_404_for_unknown_extension( + client_request, + fake_uuid, +): + client_request.get( + 'main.view_letter_notification_as_preview', + service_id=SERVICE_ONE_ID, + notification_id=fake_uuid, + filetype='docx', + _expected_status=404, + ) @pytest.mark.parametrize('service_permissions, template_type, link_expected', [ @@ -182,17 +201,52 @@ def test_notification_page_has_link_to_send_another_for_sms( ) last_paragraph = page.select('main p')[-1] + conversation_link = url_for( + '.conversation', + service_id=SERVICE_ONE_ID, + notification_id=fake_uuid, + _anchor='n{}'.format(fake_uuid), + ) if link_expected: assert normalize_spaces(last_paragraph.text) == ( 'See all text messages sent to this phone number' ) - assert last_paragraph.select_one('a')['href'] == url_for( - '.conversation', - service_id=SERVICE_ONE_ID, - notification_id=fake_uuid, - _anchor='n{}'.format(fake_uuid), - ) + assert last_paragraph.select_one('a')['href'] == conversation_link else: - # covers ‘Delivered’, ‘Expected delivery date’ - assert 'deliver' in normalize_spaces(last_paragraph.text).lower() + assert conversation_link not in str(page.select_one('main')) + + +@pytest.mark.parametrize('template_type, expected_link', [ + ('email', lambda notification_id: None), + ('sms', lambda notification_id: None), + ('letter', partial( + url_for, + 'main.view_letter_notification_as_preview', + service_id=SERVICE_ONE_ID, + filetype='pdf' + )), +]) +def test_notification_page_has_link_to_download_letter( + client_request, + mocker, + fake_uuid, + service_one, + template_type, + expected_link, +): + + mock_get_notification(mocker, fake_uuid, template_type=template_type) + + page = client_request.get( + 'main.view_notification', + service_id=SERVICE_ONE_ID, + notification_id=fake_uuid, + ) + + try: + download_link = page.select_one('a[download]')['href'] + except TypeError: + download_link = None + + assert download_link == expected_link(notification_id=fake_uuid)