From ba80d5b7cd56e34a699f2f0b349efb60e893e83a Mon Sep 17 00:00:00 2001 From: David McDonald Date: Mon, 18 May 2020 15:15:54 +0100 Subject: [PATCH] Introduce abstraction for s3 metadata S3 metadata only supports ascii characters. Whenever we save data to it we need to make sure we encode it to save it and then decode it to display it again to users. This abstraction will act as the place for that decoding to happen so the rest of the code in our views doesn't need to care about the encoding abstraction. --- app/s3_client/s3_letter_upload_client.py | 17 ++++++++++--- tests/app/main/views/test_uploads.py | 31 ++++++++++++------------ 2 files changed, 30 insertions(+), 18 deletions(-) diff --git a/app/s3_client/s3_letter_upload_client.py b/app/s3_client/s3_letter_upload_client.py index bf263fb6f..e5ddad695 100644 --- a/app/s3_client/s3_letter_upload_client.py +++ b/app/s3_client/s3_letter_upload_client.py @@ -48,9 +48,20 @@ def get_letter_pdf_and_metadata(service_id, file_id): s3_object = s3.Object(current_app.config['TRANSIENT_UPLOADED_LETTERS'], file_location).get() pdf = s3_object['Body'].read() - metadata = s3_object['Metadata'] - return pdf, metadata + return pdf, LetterMetadata(s3_object['Metadata']) + + +class LetterMetadata: + KEYS_TO_DECODE = [] + + 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) def get_letter_metadata(service_id, file_id): @@ -58,4 +69,4 @@ def get_letter_metadata(service_id, file_id): s3 = resource('s3') s3_object = s3.Object(current_app.config['TRANSIENT_UPLOADED_LETTERS'], file_location).get() - return s3_object['Metadata'] + return LetterMetadata(s3_object['Metadata']) diff --git a/tests/app/main/views/test_uploads.py b/tests/app/main/views/test_uploads.py index 98246762a..c77eb9bc6 100644 --- a/tests/app/main/views/test_uploads.py +++ b/tests/app/main/views/test_uploads.py @@ -10,6 +10,7 @@ from freezegun import freeze_time from requests import RequestException from app.main.views.uploads import format_recipient +from app.s3_client.s3_letter_upload_client import LetterMetadata from app.utils import normalize_spaces from tests.conftest import ( SERVICE_ONE_ID, @@ -166,12 +167,12 @@ def test_post_upload_letter_redirects_for_valid_file( ) ) mock_s3 = mocker.patch('app.main.views.uploads.upload_letter_to_s3') - mocker.patch('app.main.views.uploads.get_letter_metadata', return_value={ + mocker.patch('app.main.views.uploads.get_letter_metadata', return_value=LetterMetadata({ '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 @@ -232,12 +233,12 @@ def test_post_upload_letter_shows_letter_preview_for_valid_file( ) 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={ + mocker.patch('app.main.views.uploads.get_letter_metadata', return_value=LetterMetadata({ '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 @@ -351,9 +352,9 @@ def test_post_upload_letter_with_invalid_file(mocker, client_request, fake_uuid) } 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') - mocker.patch('app.main.views.uploads.get_letter_metadata', return_value={ + mocker.patch('app.main.views.uploads.get_letter_metadata', return_value=LetterMetadata({ 'filename': 'tests/test_pdf_files/one_page_pdf.pdf', 'page_count': '1', 'status': 'invalid', - 'message': 'content-outside-printable-area', 'invalid_pages': '[1]'}) + 'message': 'content-outside-printable-area', 'invalid_pages': '[1]'})) with open('tests/test_pdf_files/one_page_pdf.pdf', 'rb') as file: file_contents = file.read() @@ -395,9 +396,9 @@ def test_post_upload_letter_shows_letter_preview_for_invalid_file(mocker, client 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={ + mocker.patch('app.main.views.uploads.get_letter_metadata', return_value=LetterMetadata({ 'filename': 'tests/test_pdf_files/one_page_pdf.pdf', 'page_count': '1', 'status': 'invalid', - 'message': 'template-preview-error'}) + 'message': 'template-preview-error'})) with open('tests/test_pdf_files/one_page_pdf.pdf', 'rb') as file: page = client_request.post( @@ -455,10 +456,10 @@ def test_uploaded_letter_preview( ): 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={ + mocker.patch('app.main.views.uploads.get_letter_metadata', return_value=LetterMetadata({ '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) @@ -485,8 +486,8 @@ def test_uploaded_letter_preview_does_not_show_send_button_if_service_in_trial_m fake_uuid, ): mocker.patch('app.main.views.uploads.service_api_client') - mocker.patch('app.main.views.uploads.get_letter_metadata', return_value={ - 'filename': 'my_letter.pdf', 'page_count': '1', 'status': 'valid', 'recipient': 'The Queen'}) + mocker.patch('app.main.views.uploads.get_letter_metadata', return_value=LetterMetadata({ + 'filename': 'my_letter.pdf', 'page_count': '1', 'status': 'valid', 'recipient': 'The Queen'})) # client_request uses service_one, which is in trial mode page = client_request.get( @@ -603,7 +604,7 @@ def test_uploaded_letter_preview_image_400s_for_bad_page_type( 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', 'recipient': 'address'} + metadata = LetterMetadata({'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') @@ -656,8 +657,8 @@ 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={'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']