From 387fcfda3fec26c9b05539c9128dc39904239480 Mon Sep 17 00:00:00 2001
From: David McDonald <david.mcdonald@digital.cabinet-office.gov.uk>
Date: Mon, 18 May 2020 15:44:20 +0100
Subject: [PATCH] Move recipient decoding into LetterMetadata class

This is for consistency with how we do it for filenames in the previous
commit and moves the decoding into the `LetterMetadata` class for
abstracting this behaviour.

Small refactor of the LetterMetadata class needed to handle None case as
recipient can be None.
---
 app/main/views/uploads.py                |  3 ---
 app/s3_client/s3_letter_upload_client.py |  9 +++++----
 tests/app/main/views/test_uploads.py     | 14 ++++++++------
 3 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/app/main/views/uploads.py b/app/main/views/uploads.py
index 263fd43bf..d2ee17de2 100644
--- a/app/main/views/uploads.py
+++ b/app/main/views/uploads.py
@@ -1,7 +1,6 @@
 import base64
 import itertools
 import json
-import urllib
 import uuid
 from io import BytesIO
 from zipfile import BadZipFile
@@ -192,14 +191,12 @@ def _get_error_from_upload_form(form_errors):
 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
diff --git a/app/s3_client/s3_letter_upload_client.py b/app/s3_client/s3_letter_upload_client.py
index 0f2e5a587..2e3ff08cb 100644
--- a/app/s3_client/s3_letter_upload_client.py
+++ b/app/s3_client/s3_letter_upload_client.py
@@ -45,15 +45,16 @@ def upload_letter_to_s3(
 
 
 class LetterMetadata:
-    KEYS_TO_DECODE = ["filename"]
+    KEYS_TO_DECODE = ["filename", "recipient"]
 
     def __init__(self, metadata):
         self._metadata = metadata
 
     def get(self, key, default=None):
-        if key in self.KEYS_TO_DECODE:
-            return urllib.parse.unquote(self._metadata.get(key, default))
-        return self._metadata.get(key, default)
+        value = self._metadata.get(key, default)
+        if value and key in self.KEYS_TO_DECODE:
+            value = urllib.parse.unquote(value)
+        return value
 
 
 def get_letter_pdf_and_metadata(service_id, file_id):
diff --git a/tests/app/main/views/test_uploads.py b/tests/app/main/views/test_uploads.py
index b9ac34222..49f8b784e 100644
--- a/tests/app/main/views/test_uploads.py
+++ b/tests/app/main/views/test_uploads.py
@@ -1,5 +1,4 @@
 import re
-import urllib
 import uuid
 from io import BytesIO
 from unittest.mock import ANY, Mock
@@ -455,12 +454,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=LetterMetadata({
         'filename': 'my_encoded_filename%C2%A3.pdf',
         'page_count': '1',
         'status': 'valid',
-        'recipient': urllib.parse.quote(recipient)
+        'recipient': 'Bugs Bunny%0A123 Big Hole%0DLooney Town'  # 'Bugs Bunny%0A123 Big Hole\rLooney Town' url encoded
     }))
 
     service_one['restricted'] = False
@@ -655,8 +653,12 @@ def test_send_uploaded_letter_when_metadata_states_pdf_is_invalid(mocker, servic
     mock_send = mocker.patch('app.main.views.uploads.notification_api_client.send_precompiled_letter')
     mocker.patch(
         'app.main.views.uploads.get_letter_metadata',
-        return_value=LetterMetadata({'filename': 'my_file.pdf', 'page_count': '3', 'status': 'invalid',
-                      'message': 'error', 'invalid_pages': '[1]'})
+        return_value=LetterMetadata(
+            {
+                'filename': 'my_file.pdf', 'page_count': '3', 'status': 'invalid',
+                'message': 'error', 'invalid_pages': '[1]'
+            }
+        )
     )
 
     service_one['permissions'] = ['letter', 'upload_letters']
@@ -680,7 +682,7 @@ def test_send_uploaded_letter_when_metadata_states_pdf_is_invalid(mocker, servic
     ('', ''),
 ])
 def test_format_recipient(original_address, expected_address):
-    assert format_recipient(urllib.parse.quote(original_address)) == expected_address
+    assert format_recipient(original_address) == expected_address
 
 
 @pytest.mark.parametrize('user', (