From a542047581ccb15c9ee0b5de9019b1f2ee32dd43 Mon Sep 17 00:00:00 2001
From: Katie Smith <katie.smith@digital.cabinet-office.gov.uk>
Date: Fri, 8 Nov 2019 09:56:59 +0000
Subject: [PATCH] Show recipient when about to send uploaded letter

The recipient of the letter now displays at the bottom of the page when
previewing a valid letter. The template preview `/precompiled/sanitise`
endpoint returns the address, but we format it to display on a single
line with commas between each line. We also need to convert the
recipient address to ASCII so that it can be stored as S3 metadata.
---
 app/main/views/uploads.py                     | 10 ++++--
 app/s3_client/s3_letter_upload_client.py      | 32 ++++++++++++++++++-
 app/templates/views/uploads/preview.html      |  4 +++
 tests/app/main/views/test_uploads.py          | 21 ++++++++----
 .../s3_client/test_s3_letter_upload_client.py | 18 ++++++++++-
 5 files changed, 75 insertions(+), 10 deletions(-)

diff --git a/app/main/views/uploads.py b/app/main/views/uploads.py
index c97919062..5054cc4d7 100644
--- a/app/main/views/uploads.py
+++ b/app/main/views/uploads.py
@@ -92,14 +92,18 @@ def upload_letter(service_id):
             else:
                 raise ex
         else:
+            response = response.json()
+            recipient = response['recipient_address']
             status = 'valid'
-            file_contents = base64.b64decode(response.json()['file'].encode())
+            file_contents = base64.b64decode(response['file'].encode())
+
             upload_letter_to_s3(
                 file_contents,
                 file_location=file_location,
                 status=status,
                 page_count=page_count,
-                filename=original_filename)
+                filename=original_filename,
+                recipient=recipient)
 
         return redirect(
             url_for(
@@ -147,6 +151,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')
 
     if invalid_pages:
         invalid_pages = json.loads(invalid_pages)
@@ -180,6 +185,7 @@ def uploaded_letter_preview(service_id, file_id):
         message=error_message,
         error_code=error_shortcode,
         form=form,
+        recipient=recipient,
     )
 
 
diff --git a/app/s3_client/s3_letter_upload_client.py b/app/s3_client/s3_letter_upload_client.py
index 1a5bb4611..adff787e9 100644
--- a/app/s3_client/s3_letter_upload_client.py
+++ b/app/s3_client/s3_letter_upload_client.py
@@ -3,13 +3,24 @@ import json
 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):
     return 'service-{}/{}.pdf'.format(service_id, upload_id)
 
 
-def upload_letter_to_s3(data, *, file_location, status, page_count, filename, message=None, invalid_pages=None):
+def upload_letter_to_s3(
+    data,
+    *,
+    file_location,
+    status,
+    page_count,
+    filename,
+    message=None,
+    invalid_pages=None,
+    recipient=None
+):
     metadata = {
         'status': status,
         'page_count': str(page_count),
@@ -19,6 +30,8 @@ def upload_letter_to_s3(data, *, file_location, status, page_count, filename, me
         metadata['message'] = message
     if invalid_pages:
         metadata['invalid_pages'] = json.dumps(invalid_pages)
+    if recipient:
+        metadata['recipient'] = format_recipient(recipient)
 
     utils_s3upload(
         filedata=data,
@@ -46,3 +59,20 @@ 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/uploads/preview.html b/app/templates/views/uploads/preview.html
index 55f65e055..be7a7abe7 100644
--- a/app/templates/views/uploads/preview.html
+++ b/app/templates/views/uploads/preview.html
@@ -32,6 +32,10 @@
 
     {% if status == 'valid' and current_service.live %}
     <div class="js-stick-at-bottom-when-scrolling">
+      <p>
+        Recipient: {{ recipient }}
+      </p>
+
       <form method="post" enctype="multipart/form-data" action="{{url_for(
           'main.send_uploaded_letter',
           service_id=current_service.id,
diff --git a/tests/app/main/views/test_uploads.py b/tests/app/main/views/test_uploads.py
index 6882c0f43..b2ee537fc 100644
--- a/tests/app/main/views/test_uploads.py
+++ b/tests/app/main/views/test_uploads.py
@@ -55,11 +55,14 @@ def test_post_upload_letter_redirects_for_valid_file(mocker, active_user_with_pe
     antivirus_mock = mocker.patch('app.main.views.uploads.antivirus_client.scan', return_value=True)
     mocker.patch(
         'app.main.views.uploads.sanitise_letter',
-        return_value=Mock(content='The sanitised content', json=lambda: {'file': 'VGhlIHNhbml0aXNlZCBjb250ZW50'})
+        return_value=Mock(
+            content='The sanitised content',
+            json=lambda: {'file': 'VGhlIHNhbml0aXNlZCBjb250ZW50', 'recipient_address': 'The Queen'}
+        )
     )
     mock_s3 = mocker.patch('app.main.views.uploads.upload_letter_to_s3')
     mocker.patch('app.main.views.uploads.get_letter_metadata', return_value={
-        'filename': 'tests/test_pdf_files/one_page_pdf.pdf', 'page_count': '1', 'status': 'valid'})
+        'filename': 'tests/test_pdf_files/one_page_pdf.pdf', 'page_count': '1', 'status': 'valid', 'recipient': 'The Queen'})
     mocker.patch('app.main.views.uploads.service_api_client.get_precompiled_template')
 
     service_one['restricted'] = False
@@ -79,9 +82,11 @@ def test_post_upload_letter_redirects_for_valid_file(mocker, active_user_with_pe
         file_location='service-{}/fake-uuid.pdf'.format(SERVICE_ONE_ID),
         status='valid',
         page_count=1,
-        filename='tests/test_pdf_files/one_page_pdf.pdf'
+        filename='tests/test_pdf_files/one_page_pdf.pdf',
+        recipient='The Queen',
     )
 
+    assert 'The Queen' in page.find('div', class_='js-stick-at-bottom-when-scrolling').text
     assert page.find('h1').text == 'tests/test_pdf_files/one_page_pdf.pdf'
     assert not page.find(id='validation-error-message')
 
@@ -105,12 +110,15 @@ def test_post_upload_letter_shows_letter_preview_for_valid_file(
     mocker.patch('app.main.views.uploads.antivirus_client.scan', return_value=True)
     mocker.patch(
         'app.main.views.uploads.sanitise_letter',
-        return_value=Mock(content='The sanitised content', json=lambda: {'file': 'VGhlIHNhbml0aXNlZCBjb250ZW50'})
+        return_value=Mock(
+            content='The sanitised content',
+            json=lambda: {'file': 'VGhlIHNhbml0aXNlZCBjb250ZW50', 'recipient_address': 'The Queen'}
+        )
     )
     mocker.patch('app.main.views.uploads.upload_letter_to_s3')
     mocker.patch('app.main.views.uploads.pdf_page_count', return_value=3)
     mocker.patch('app.main.views.uploads.get_letter_metadata', return_value={
-        'filename': 'tests/test_pdf_files/one_page_pdf.pdf', 'page_count': '3', 'status': 'valid'})
+        'filename': 'tests/test_pdf_files/one_page_pdf.pdf', 'page_count': '3', 'status': 'valid', 'recipient': 'The Queen'})
     mocker.patch('app.main.views.uploads.service_api_client.get_precompiled_template', return_value=letter_template)
 
     service_one['restricted'] = False
@@ -265,7 +273,7 @@ def test_post_upload_letter_shows_letter_preview_for_invalid_file(mocker, client
     mocker.patch('app.main.views.uploads.upload_letter_to_s3')
     mock_sanitise_response = Mock()
     mock_sanitise_response.raise_for_status.side_effect = RequestException(response=Mock(status_code=400))
-    mock_sanitise_response.json = lambda: {"message": "template preview error"}
+    mock_sanitise_response.json = lambda: {"message": "template preview error", "recipient_address": "The Queen"}
     mocker.patch('app.main.views.uploads.sanitise_letter', return_value=mock_sanitise_response)
     mocker.patch('app.main.views.uploads.service_api_client.get_precompiled_template', return_value=letter_template)
     mocker.patch('app.main.views.uploads.get_letter_metadata', return_value={
@@ -280,6 +288,7 @@ def test_post_upload_letter_shows_letter_preview_for_invalid_file(mocker, client
             _follow_redirects=True,
         )
 
+    assert 'The Queen' not in page.text
     assert len(page.select('.letter-postage')) == 0
 
     letter_images = page.select('main img')
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 72d2891f7..0c65dcc77 100644
--- a/tests/app/s3_client/test_s3_letter_upload_client.py
+++ b/tests/app/s3_client/test_s3_letter_upload_client.py
@@ -1,6 +1,10 @@
+import pytest
 from flask import current_app
 
-from app.s3_client.s3_letter_upload_client import upload_letter_to_s3
+from app.s3_client.s3_letter_upload_client import (
+    format_recipient,
+    upload_letter_to_s3,
+)
 
 
 def test_upload_letter_to_s3(mocker):
@@ -47,3 +51,15 @@ 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