diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 147078e5c..5b457c3f8 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -513,7 +513,7 @@ def add_preview_of_content_to_notifications(notifications): ) else: if notification['template']['is_precompiled_letter']: - notification['template']['subject'] = 'Provided as PDF' + notification['template']['subject'] = notification['client_reference'] yield dict( preview_of_content=( WithSubjectTemplate( diff --git a/app/main/views/uploads.py b/app/main/views/uploads.py index 84cac95ad..8be0aa4dc 100644 --- a/app/main/views/uploads.py +++ b/app/main/views/uploads.py @@ -1,5 +1,6 @@ import base64 import json +import urllib import uuid from io import BytesIO @@ -168,6 +169,25 @@ def _get_error_from_upload_form(form_errors): return error +def format_recipient(address): + ''' + To format the recipient we need to: + - decode, address is url encoded + - remove new line characters + - remove whitespace around the lines + - join the address lines, separated by a comma + ''' + if not address: + return address + address = urllib.parse.unquote(address) + stripped_address_lines_no_trailing_commas = [ + line.lstrip().rstrip(' ,') + for line in address.splitlines() if line + ] + one_line_address = ', '.join(stripped_address_lines_no_trailing_commas) + return one_line_address + + @main.route("/services//preview-letter/") @user_has_permissions('send_messages') def uploaded_letter_preview(service_id, file_id): @@ -179,7 +199,7 @@ def uploaded_letter_preview(service_id, file_id): status = metadata.get('status') error_shortcode = metadata.get('message') invalid_pages = metadata.get('invalid_pages') - recipient = metadata.get('recipient') + recipient = format_recipient(metadata.get('recipient')) if invalid_pages: invalid_pages = json.loads(invalid_pages) @@ -246,11 +266,12 @@ def send_uploaded_letter(service_id): postage = form.postage.data metadata = get_letter_metadata(service_id, file_id) filename = metadata.get('filename') + recipient_address = metadata.get('recipient') if metadata.get('status') != 'valid': abort(403) - notification_api_client.send_precompiled_letter(service_id, filename, file_id, postage) + notification_api_client.send_precompiled_letter(service_id, filename, file_id, postage, recipient_address) return redirect(url_for( '.view_notification', diff --git a/app/notify_client/notification_api_client.py b/app/notify_client/notification_api_client.py index a6982b553..87898fe55 100644 --- a/app/notify_client/notification_api_client.py +++ b/app/notify_client/notification_api_client.py @@ -59,11 +59,12 @@ class NotificationApiClient(NotifyAdminAPIClient): data = _attach_current_user(data) return self.post(url='/service/{}/send-notification'.format(service_id), data=data) - def send_precompiled_letter(self, service_id, filename, file_id, postage): + def send_precompiled_letter(self, service_id, filename, file_id, postage, recipient_address): data = { 'filename': filename, 'file_id': file_id, 'postage': postage, + 'recipient_address': recipient_address } data = _attach_current_user(data) return self.post(url='/service/{}/send-pdf-letter'.format(service_id), data=data) diff --git a/app/s3_client/s3_letter_upload_client.py b/app/s3_client/s3_letter_upload_client.py index adff787e9..bf263fb6f 100644 --- a/app/s3_client/s3_letter_upload_client.py +++ b/app/s3_client/s3_letter_upload_client.py @@ -1,9 +1,9 @@ import json +import urllib from boto3 import resource from flask import current_app from notifications_utils.s3 import s3upload as utils_s3upload -from notifications_utils.sanitise_text import SanitiseASCII def get_transient_letter_file_location(service_id, upload_id): @@ -31,7 +31,7 @@ def upload_letter_to_s3( if invalid_pages: metadata['invalid_pages'] = json.dumps(invalid_pages) if recipient: - metadata['recipient'] = format_recipient(recipient) + metadata['recipient'] = urllib.parse.quote(recipient) utils_s3upload( filedata=data, @@ -59,20 +59,3 @@ def get_letter_metadata(service_id, file_id): s3_object = s3.Object(current_app.config['TRANSIENT_UPLOADED_LETTERS'], file_location).get() return s3_object['Metadata'] - - -def format_recipient(address): - ''' - To format the recipient we need to: - - remove new line characters - - remove whitespace around the lines - - join the address lines, separated by a comma - - convert the string to ASCII (S3 metadata must be stored as ASCII) - ''' - stripped_address_lines_no_trailing_commas = [ - line.lstrip().rstrip(' ,') - for line in address.splitlines() if line - ] - one_line_address = ', '.join(stripped_address_lines_no_trailing_commas) - - return SanitiseASCII.encode(one_line_address) diff --git a/app/templates/views/activity/notifications.html b/app/templates/views/activity/notifications.html index d2b8e3465..09ab70657 100644 --- a/app/templates/views/activity/notifications.html +++ b/app/templates/views/activity/notifications.html @@ -17,9 +17,9 @@ ) %} {% call row_heading() %} {% if item.status in ('pending-virus-check', 'virus-scan-failed') %} - {{ item.to }} + Checking {% else %} - {{ item.to }} + {{ item.to.split('\n')[0] }} {% endif %}

{{ item.preview_of_content }} diff --git a/tests/app/main/views/test_activity.py b/tests/app/main/views/test_activity.py index b2c7fe550..5781148db 100644 --- a/tests/app/main/views/test_activity.py +++ b/tests/app/main/views/test_activity.py @@ -265,15 +265,19 @@ def test_download_not_available_to_users_without_dashboard( def test_letters_with_status_virus_scan_failed_shows_a_failure_description( mocker, + active_user_with_permissions, client_request, service_one, mock_get_service_statistics, mock_get_service_data_retention, mock_get_api_keys, ): - notifications = create_notifications(template_type='letter', status='virus-scan-failed', is_precompiled_letter=True) - mocker.patch('app.notification_api_client.get_notifications_for_service', return_value=notifications) - + mock_get_notifications( + mocker, + active_user_with_permissions, + is_precompiled_letter=True, + noti_status='virus-scan-failed' + ) page = client_request.get( 'main.view_notifications', service_id=service_one['id'], @@ -563,6 +567,7 @@ def test_html_contains_notification_id( def test_html_contains_links_for_failed_notifications( client_request, + active_user_with_permissions, mock_get_service_statistics, mock_get_service_data_retention, mock_get_no_api_keys, diff --git a/tests/app/main/views/test_uploads.py b/tests/app/main/views/test_uploads.py index da54ac3dd..490adb1b3 100644 --- a/tests/app/main/views/test_uploads.py +++ b/tests/app/main/views/test_uploads.py @@ -1,9 +1,11 @@ +import urllib from unittest.mock import Mock import pytest from flask import make_response, url_for from requests import RequestException +from app.main.views.uploads import format_recipient from app.utils import normalize_spaces from tests.conftest import SERVICE_ONE_ID @@ -350,8 +352,11 @@ def test_uploaded_letter_preview( fake_uuid, ): mocker.patch('app.main.views.uploads.service_api_client') + recipient = 'Bugs Bunny\n123 Big Hole\rLooney Town' mocker.patch('app.main.views.uploads.get_letter_metadata', return_value={ - 'filename': 'my_letter.pdf', 'page_count': '1', 'status': 'valid'}) + 'filename': 'my_letter.pdf', 'page_count': '1', 'status': 'valid', + 'recipient': urllib.parse.quote(recipient) + }) service_one['restricted'] = False client_request.login(active_user_with_permissions, service=service_one) @@ -455,7 +460,7 @@ def test_uploaded_letter_preview_image_does_not_show_overlay_if_no_content_outsi def test_send_uploaded_letter_sends_letter_and_redirects_to_notification_page(mocker, service_one, client_request): - metadata = {'filename': 'my_file.pdf', 'page_count': '1', 'status': 'valid'} + metadata = {'filename': 'my_file.pdf', 'page_count': '1', 'status': 'valid', 'recipient': 'address'} mocker.patch('app.main.views.uploads.get_letter_pdf_and_metadata', return_value=('file', metadata)) mock_send = mocker.patch('app.main.views.uploads.notification_api_client.send_precompiled_letter') @@ -475,7 +480,7 @@ def test_send_uploaded_letter_sends_letter_and_redirects_to_notification_page(mo _external=True ) ) - mock_send.assert_called_once_with(SERVICE_ONE_ID, 'my_file.pdf', file_id, 'first') + mock_send.assert_called_once_with(SERVICE_ONE_ID, 'my_file.pdf', file_id, 'first', 'address') @pytest.mark.parametrize('permissions', [ @@ -522,3 +527,14 @@ def test_send_uploaded_letter_when_metadata_states_pdf_is_invalid(mocker, servic _expected_status=403 ) assert not mock_send.called + + +@pytest.mark.parametrize('original_address,expected_address', [ + ('The Queen, Buckingham Palace, SW1 1AA', 'The Queen, Buckingham Palace, SW1 1AA'), + ('The Queen Buckingham Palace SW1 1AA', 'The Queen Buckingham Palace SW1 1AA'), + ('The Queen,\nBuckingham Palace,\r\nSW1 1AA', 'The Queen, Buckingham Palace, SW1 1AA'), + ('The Queen ,,\nBuckingham Palace,\rSW1 1AA,', 'The Queen, Buckingham Palace, SW1 1AA'), + (' The Queen\n Buckingham Palace\n SW1 1AA', 'The Queen, Buckingham Palace, SW1 1AA'), +]) +def test_format_recipient(original_address, expected_address): + assert format_recipient(urllib.parse.quote(original_address)) == expected_address diff --git a/tests/app/notify_client/test_notification_client.py b/tests/app/notify_client/test_notification_client.py index 5df1a97b1..5741fe219 100644 --- a/tests/app/notify_client/test_notification_client.py +++ b/tests/app/notify_client/test_notification_client.py @@ -65,7 +65,8 @@ def test_send_precompiled_letter(mocker, logged_in_client, active_user_with_perm 'abcd-1234', 'my_file.pdf', 'file-ID', - 'second' + 'second', + 'Bugs Bunny, 12 Hole Avenue, Looney Town' ) mock_post.assert_called_once_with( url='/service/abcd-1234/send-pdf-letter', @@ -74,6 +75,7 @@ def test_send_precompiled_letter(mocker, logged_in_client, active_user_with_perm 'file_id': 'file-ID', 'created_by': active_user_with_permissions['id'], 'postage': 'second', + 'recipient_address': 'Bugs Bunny, 12 Hole Avenue, Looney Town', } ) diff --git a/tests/app/s3_client/test_s3_letter_upload_client.py b/tests/app/s3_client/test_s3_letter_upload_client.py index 0c65dcc77..e03aae794 100644 --- a/tests/app/s3_client/test_s3_letter_upload_client.py +++ b/tests/app/s3_client/test_s3_letter_upload_client.py @@ -1,27 +1,29 @@ -import pytest +import urllib + from flask import current_app -from app.s3_client.s3_letter_upload_client import ( - format_recipient, - upload_letter_to_s3, -) +from app.s3_client.s3_letter_upload_client import upload_letter_to_s3 def test_upload_letter_to_s3(mocker): s3_mock = mocker.patch('app.s3_client.s3_letter_upload_client.utils_s3upload') + recipient = 'Bugs Bunny\n123 Big Hole\nLooney Town' upload_letter_to_s3( 'pdf_data', file_location='service_id/upload_id.pdf', status='valid', page_count=3, - filename='my_doc') + filename='my_doc', + recipient=recipient + ) s3_mock.assert_called_once_with( bucket_name=current_app.config['TRANSIENT_UPLOADED_LETTERS'], file_location='service_id/upload_id.pdf', filedata='pdf_data', - metadata={'status': 'valid', 'page_count': '3', 'filename': 'my_doc'}, + metadata={'status': 'valid', 'page_count': '3', 'filename': 'my_doc', + 'recipient': urllib.parse.quote(recipient)}, region=current_app.config['AWS_REGION'] ) @@ -51,15 +53,3 @@ def test_upload_letter_to_s3_with_message_and_invalid_pages(mocker): }, region=current_app.config['AWS_REGION'] ) - - -@pytest.mark.parametrize('original_address,expected_address', [ - ('The Queen, Buckingham Palace, SW1 1AA', 'The Queen, Buckingham Palace, SW1 1AA'), - ('The Queen Buckingham Palace SW1 1AA', 'The Queen Buckingham Palace SW1 1AA'), - ('The Queen,\nBuckingham Palace,\r\nSW1 1AA', 'The Queen, Buckingham Palace, SW1 1AA'), - ('The Queen ,,\nBuckingham Palace,\rSW1 1AA,', 'The Queen, Buckingham Palace, SW1 1AA'), - (' The Queen\n Buckingham Palace\n SW1 1AA', 'The Queen, Buckingham Palace, SW1 1AA'), - ("The ’Queen\n Buckingham Palace\n SW1 1AA", "The 'Queen, Buckingham Palace, SW1 1AA"), -]) -def test_format_recipient(original_address, expected_address): - assert format_recipient(original_address) == expected_address