From e7e3b95feea8694e4bbbd1cd3897a78e4c65d4a8 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 27 Apr 2018 16:05:04 +0100 Subject: [PATCH] Store info about files as S3 metadata MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Storing things in the session is proving buggy – we still have one user (that we know about) where the session data isn’t getting written, so they’re blocked from uploading a file. Since all the info we’re storing in the session is about the file, it makes sense to store it with the file. This commit only does the writing of the metadata, once we’re sure this is working we can do subsequent work to read it back, and remove reliance on the session. --- app/main/s3_client.py | 40 ++++++++++++++++++++++++------- app/main/views/send.py | 9 ++++++- tests/app/main/test_s3_client.py | 21 +++++++++++++++- tests/app/main/views/test_send.py | 25 +++++++++++++++++++ tests/conftest.py | 5 ++++ 5 files changed, 90 insertions(+), 10 deletions(-) diff --git a/app/main/s3_client.py b/app/main/s3_client.py index cbcbb0357..474848c0a 100644 --- a/app/main/s3_client.py +++ b/app/main/s3_client.py @@ -15,6 +15,17 @@ def get_s3_object(bucket_name, filename): return s3.Object(bucket_name, filename) +def get_csv_location(service_id, upload_id): + return ( + current_app.config['CSV_UPLOAD_BUCKET_NAME'], + FILE_LOCATION_STRUCTURE.format(service_id, upload_id), + ) + + +def get_csv_upload(service_id, upload_id): + return get_s3_object(*get_csv_location(service_id, upload_id)) + + def delete_s3_object(filename): bucket_name = current_app.config['LOGO_UPLOAD_BUCKET_NAME'] get_s3_object(bucket_name, filename).delete() @@ -39,20 +50,20 @@ def get_temp_truncated_filename(filename, user_id): def s3upload(service_id, filedata, region): upload_id = str(uuid.uuid4()) - upload_file_name = FILE_LOCATION_STRUCTURE.format(service_id, upload_id) - utils_s3upload(filedata=filedata['data'], - region=region, - bucket_name=current_app.config['CSV_UPLOAD_BUCKET_NAME'], - file_location=upload_file_name) + bucket_name, file_location = get_csv_location(service_id, upload_id) + utils_s3upload( + filedata=filedata['data'], + region=region, + bucket_name=bucket_name, + file_location=file_location, + ) return upload_id def s3download(service_id, upload_id): contents = '' try: - bucket_name = current_app.config['CSV_UPLOAD_BUCKET_NAME'] - upload_file_name = FILE_LOCATION_STRUCTURE.format(service_id, upload_id) - key = get_s3_object(bucket_name, upload_file_name) + key = get_csv_upload(service_id, upload_id) contents = key.get()['Body'].read().decode('utf-8') except botocore.exceptions.ClientError as e: current_app.logger.error("Unable to download s3 file {}".format( @@ -121,3 +132,16 @@ def delete_temp_file(filename): raise ValueError('Not a temp file: {}'.format(filename)) delete_s3_object(filename) + + +def set_metadata_on_csv_upload(service_id, upload_id, **kwargs): + get_csv_upload( + service_id, upload_id + ).copy_from( + CopySource='{}/{}'.format(*get_csv_location(service_id, upload_id)), + ServerSideEncryption='AES256', + Metadata={ + key: str(value) for key, value in kwargs.items() + }, + MetadataDirective='REPLACE', + ) diff --git a/app/main/views/send.py b/app/main/views/send.py index 18b1e7183..dd9d22231 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -38,7 +38,7 @@ from app.main.forms import ( SetSenderForm, get_placeholder_form_instance, ) -from app.main.s3_client import s3download, s3upload +from app.main.s3_client import s3download, s3upload, set_metadata_on_csv_upload from app.template_previews import TemplatePreview, get_page_count_for_letter from app.utils import ( Spreadsheet, @@ -549,6 +549,13 @@ def _check_messages(service_id, template_id, upload_id, preview_row, letters_as_ session['file_uploads'][upload_id]['notification_count'] = len(recipients) session['file_uploads'][upload_id]['template_id'] = str(template_id) session['file_uploads'][upload_id]['valid'] = True + set_metadata_on_csv_upload( + service_id, + upload_id, + notification_count=len(recipients), + template_id=str(template_id), + valid=True, + ) else: session['file_uploads'].pop(upload_id) diff --git a/tests/app/main/test_s3_client.py b/tests/app/main/test_s3_client.py index 3c9ad5bec..db05ee128 100644 --- a/tests/app/main/test_s3_client.py +++ b/tests/app/main/test_s3_client.py @@ -1,5 +1,5 @@ from collections import namedtuple -from unittest.mock import call +from unittest.mock import Mock, call import pytest @@ -10,6 +10,7 @@ from app.main.s3_client import ( delete_temp_files_created_by, get_temp_truncated_filename, persist_logo, + set_metadata_on_csv_upload, upload_logo, ) @@ -94,3 +95,21 @@ def test_does_not_delete_non_temp_file(client, mocker, fake_uuid): assert mocked_delete_s3_object.called_with_args(filename) assert str(error.value) == 'Not a temp file: {}'.format(filename) + + +def test_sets_metadata(client, mocker): + mocked_s3_object = Mock() + mocked_get_s3_object = mocker.patch( + 'app.main.s3_client.get_csv_upload', + return_value=mocked_s3_object, + ) + + set_metadata_on_csv_upload('1234', '5678', foo='bar', baz=True) + + mocked_get_s3_object.assert_called_once_with('1234', '5678') + mocked_s3_object.copy_from.assert_called_once_with( + CopySource='test-notifications-csv-upload/service-1234-notify/5678.csv', + Metadata={'baz': 'True', 'foo': 'bar'}, + MetadataDirective='REPLACE', + ServerSideEncryption='AES256', + ) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 36386fd0f..15b2fb5e5 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -565,6 +565,7 @@ def test_upload_valid_csv_shows_preview_and_table( mock_get_service_template_with_placeholders, mock_get_users_by_service, mock_get_detailed_service_for_today, + mock_s3_set_metadata, fake_uuid, extra_args, expected_link_in_first_row, @@ -592,6 +593,14 @@ def test_upload_valid_csv_shows_preview_and_table( **extra_args ) + mock_s3_set_metadata.assert_called_once_with( + SERVICE_ONE_ID, + fake_uuid, + notification_count=3, + template_id=fake_uuid, + valid=True, + ) + assert page.h1.text.strip() == 'Preview of Two week reminder' assert page.select_one('.sms-message-recipient').text.strip() == expected_recipient assert page.select_one('.sms-message-wrapper').text.strip() == expected_message @@ -702,6 +711,7 @@ def test_404_for_previewing_a_row_out_of_range( mock_get_service_template_with_placeholders, mock_get_users_by_service, mock_get_detailed_service_for_today, + mock_s3_set_metadata, fake_uuid, row_index, expected_status, @@ -1464,6 +1474,7 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers( mock_get_users_by_service, mock_get_detailed_service_for_today, mock_get_live_service, + mock_s3_set_metadata, service_one, fake_uuid, mock_s3_upload, @@ -1489,6 +1500,14 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers( assert sess['file_uploads'][fake_uuid]['template_id'] == fake_uuid assert sess['file_uploads'][fake_uuid]['valid'] is True + mock_s3_set_metadata.assert_called_once_with( + SERVICE_ONE_ID, + fake_uuid, + notification_count=53, + template_id=fake_uuid, + valid=True, + ) + content = response.get_data(as_text=True) assert response.status_code == 200 assert '07700 900701' in content @@ -1543,6 +1562,7 @@ def test_test_message_can_only_be_sent_now( mock_s3_download, mock_get_users_by_service, mock_get_detailed_service_for_today, + mock_s3_set_metadata, fake_uuid ): with logged_in_client.session_transaction() as session: @@ -1957,6 +1977,7 @@ def test_check_messages_back_link( mock_has_permissions, mock_get_detailed_service_for_today, mock_s3_download, + mock_s3_set_metadata, fake_uuid, mocker, template_mock, @@ -2138,6 +2159,7 @@ def test_check_messages_shows_trial_mode_error_for_letters( mock_has_permissions, mock_get_users_by_service, mock_get_detailed_service_for_today, + mock_s3_set_metadata, fake_uuid, mocker, service_mock, @@ -2265,6 +2287,7 @@ def test_generate_test_letter_doesnt_block_in_trial_mode( fake_uuid, mock_get_users_by_service, mock_get_detailed_service_for_today, + mock_s3_set_metadata, ): mocker.patch('app.main.views.send.s3download', return_value=""" @@ -2712,6 +2735,7 @@ def test_reply_to_is_previewed_if_chosen( mocker, mock_get_service_email_template, mock_s3_download, + mock_s3_set_metadata, mock_get_users_by_service, mock_get_detailed_service_for_today, get_default_reply_to_email_address, @@ -2760,6 +2784,7 @@ def test_sms_sender_is_previewed( mocker, mock_get_service_template, mock_s3_download, + mock_s3_set_metadata, mock_get_users_by_service, mock_get_detailed_service_for_today, get_default_sms_sender, diff --git a/tests/conftest.py b/tests/conftest.py index a506122f2..e14f9c34c 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2027,6 +2027,11 @@ def mock_s3_download(mocker, content=None): return mocker.patch('app.main.views.send.s3download', side_effect=_download) +@pytest.fixture(scope='function') +def mock_s3_set_metadata(mocker, content=None): + return mocker.patch('app.main.views.send.set_metadata_on_csv_upload') + + @pytest.fixture(scope='function') def sample_invite(mocker, service_one, status='pending'): id_ = str(generate_uuid())