From 03f2368debae21d75d9d1d9adec8909e98ab6fb2 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 13 Mar 2020 13:53:18 +0000 Subject: [PATCH] Use the correct bucket for storing contact lists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We don’t want to muddy them up with the normal CSV uploads. I’ve tried to reuse the existing S3 code where possible because it’s well tested. Buckets have already been created. --- app/config.py | 6 +++ app/main/views/send.py | 2 +- app/main/views/uploads.py | 19 ++++----- app/models/contact_list.py | 50 +++++++++++++++++++++-- app/models/service.py | 4 -- app/s3_client/s3_csv_client.py | 35 ++++++++++------ app/utils.py | 7 ++++ tests/app/main/views/test_uploads.py | 43 +++++++++++++------ tests/app/s3_client/test_s3_csv_client.py | 2 +- 9 files changed, 121 insertions(+), 47 deletions(-) diff --git a/app/config.py b/app/config.py index 7b8984287..85a72ccb9 100644 --- a/app/config.py +++ b/app/config.py @@ -68,6 +68,7 @@ class Config(object): WTF_CSRF_ENABLED = True WTF_CSRF_TIME_LIMIT = None CSV_UPLOAD_BUCKET_NAME = 'local-notifications-csv-upload' + CONTACT_LIST_UPLOAD_BUCKET_NAME = 'local-contact-list' ACTIVITY_STATS_LIMIT_DAYS = 7 TEST_MESSAGE_FILENAME = 'Report' @@ -98,6 +99,7 @@ class Development(Config): SESSION_PROTECTION = None STATSD_ENABLED = False CSV_UPLOAD_BUCKET_NAME = 'development-notifications-csv-upload' + CONTACT_LIST_UPLOAD_BUCKET_NAME = 'development-contact-list' LOGO_UPLOAD_BUCKET_NAME = 'public-logos-tools' MOU_BUCKET_NAME = 'notify.tools-mou' TRANSIENT_UPLOADED_LETTERS = 'development-transient-uploaded-letters' @@ -121,6 +123,7 @@ class Test(Development): STATSD_ENABLED = False WTF_CSRF_ENABLED = False CSV_UPLOAD_BUCKET_NAME = 'test-notifications-csv-upload' + CONTACT_LIST_UPLOAD_BUCKET_NAME = 'test-contact-list' LOGO_UPLOAD_BUCKET_NAME = 'public-logos-test' MOU_BUCKET_NAME = 'test-mou' TRANSIENT_UPLOADED_LETTERS = 'test-transient-uploaded-letters' @@ -140,6 +143,7 @@ class Preview(Config): HEADER_COLOUR = '#F499BE' # $baby-pink STATSD_ENABLED = True CSV_UPLOAD_BUCKET_NAME = 'preview-notifications-csv-upload' + CONTACT_LIST_UPLOAD_BUCKET_NAME = 'preview-contact-list' LOGO_UPLOAD_BUCKET_NAME = 'public-logos-preview' MOU_BUCKET_NAME = 'notify.works-mou' TRANSIENT_UPLOADED_LETTERS = 'preview-transient-uploaded-letters' @@ -158,6 +162,7 @@ class Staging(Config): HEADER_COLOUR = '#6F72AF' # $mauve STATSD_ENABLED = True CSV_UPLOAD_BUCKET_NAME = 'staging-notifications-csv-upload' + CONTACT_LIST_UPLOAD_BUCKET_NAME = 'staging-contact-list' LOGO_UPLOAD_BUCKET_NAME = 'public-logos-staging' MOU_BUCKET_NAME = 'staging-notify.works-mou' TRANSIENT_UPLOADED_LETTERS = 'staging-transient-uploaded-letters' @@ -173,6 +178,7 @@ class Live(Config): HTTP_PROTOCOL = 'https' STATSD_ENABLED = True CSV_UPLOAD_BUCKET_NAME = 'live-notifications-csv-upload' + CONTACT_LIST_UPLOAD_BUCKET_NAME = 'live-contact-list' LOGO_UPLOAD_BUCKET_NAME = 'public-logos-production' MOU_BUCKET_NAME = 'notifications.service.gov.uk-mou' TRANSIENT_UPLOADED_LETTERS = 'production-transient-uploaded-letters' diff --git a/app/main/views/send.py b/app/main/views/send.py index dbfb3421b..f8833723b 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -151,7 +151,7 @@ def send_messages(service_id, template_id): try: upload_id = s3upload( service_id, - Spreadsheet.from_file(form.file.data, filename=form.file.data.filename).as_dict, + Spreadsheet.from_file_form(form).as_dict, current_app.config['AWS_REGION'] ) return redirect(url_for( diff --git a/app/main/views/uploads.py b/app/main/views/uploads.py index ea9fc5770..feedb58f5 100644 --- a/app/main/views/uploads.py +++ b/app/main/views/uploads.py @@ -28,11 +28,7 @@ from app import current_service, notification_api_client, service_api_client from app.extensions import antivirus_client from app.main import main from app.main.forms import CsvUploadForm, LetterUploadPostageForm, PDFUploadForm -from app.s3_client.s3_csv_client import ( - s3download, - s3upload, - set_metadata_on_csv_upload, -) +from app.models.contact_list import ContactList from app.s3_client.s3_letter_upload_client import ( get_letter_metadata, get_letter_pdf_and_metadata, @@ -307,10 +303,9 @@ def upload_contact_list(service_id): if form.validate_on_submit(): try: - upload_id = s3upload( - service_id, - Spreadsheet.from_file(form.file.data, filename=form.file.data.filename).as_dict, - current_app.config['AWS_REGION'], + upload_id = ContactList.upload( + current_service.id, + Spreadsheet.from_file_form(form).as_dict, ) return redirect(url_for( '.check_contact_list', @@ -345,7 +340,7 @@ def check_contact_list(service_id, upload_id): form = CsvUploadForm() - contents = s3download(service_id, upload_id).strip() + contents = ContactList.download(service_id, upload_id) first_row = contents.splitlines()[0].strip().rstrip(',') if contents else '' template_type = { @@ -414,7 +409,7 @@ def check_contact_list(service_id, upload_id): 'template_type': template_type } - set_metadata_on_csv_upload(service_id, upload_id, **metadata_kwargs) + ContactList.set_metadata(service_id, upload_id, **metadata_kwargs) return render_template( 'views/uploads/contact-list/ok.html', @@ -427,7 +422,7 @@ def check_contact_list(service_id, upload_id): @main.route("/services//save-contact-list/", methods=['POST']) @user_has_permissions('send_messages') def save_contact_list(service_id, upload_id): - current_service.save_contact_list(upload_id) + ContactList.create(current_service.id, upload_id) return redirect(url_for( '.contact_list', service_id=current_service.id, diff --git a/app/models/contact_list.py b/app/models/contact_list.py index 67cabc6d9..fb32a4327 100644 --- a/app/models/contact_list.py +++ b/app/models/contact_list.py @@ -1,16 +1,60 @@ -from flask import abort +from flask import abort, current_app +from notifications_utils.formatters import strip_whitespace from app.models import JSONModel from app.notify_client.contact_list_api_client import contact_list_api_client -from app.s3_client.s3_csv_client import get_csv_metadata +from app.s3_client.s3_csv_client import ( + get_csv_metadata, + s3download, + s3upload, + set_metadata_on_csv_upload, +) class ContactList(JSONModel): + @staticmethod + def get_bucket_name(): + return current_app.config['CONTACT_LIST_UPLOAD_BUCKET_NAME'] + + @staticmethod + def upload(service_id, file_dict): + return s3upload( + service_id, + file_dict, + current_app.config['AWS_REGION'], + bucket=ContactList.get_bucket_name(), + ) + + @staticmethod + def download(service_id, upload_id): + return strip_whitespace(s3download( + service_id, + upload_id, + bucket=ContactList.get_bucket_name(), + )) + + @staticmethod + def set_metadata(service_id, upload_id, **kwargs): + return set_metadata_on_csv_upload( + service_id, + upload_id, + bucket=ContactList.get_bucket_name(), + **kwargs, + ) + + @staticmethod + def get_metadata(service_id, upload_id): + return get_csv_metadata( + service_id, + upload_id, + bucket=ContactList.get_bucket_name(), + ) + @classmethod def create(cls, service_id, upload_id): - metadata = get_csv_metadata(service_id, upload_id) + metadata = cls.get_metadata(service_id, upload_id) if not metadata.get('valid'): abort(403) diff --git a/app/models/service.py b/app/models/service.py index 6b883724b..132ac157c 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -6,7 +6,6 @@ from notifications_utils.timezones import local_timezone from werkzeug.utils import cached_property from app.models import JSONModel -from app.models.contact_list import ContactList from app.models.job import ( ImmediateJobs, PaginatedJobs, @@ -133,9 +132,6 @@ class Service(JSONModel): return [] return ScheduledJobs(self.id) - def save_contact_list(self, upload_id): - return ContactList.create(self.id, upload_id) - @cached_property def invited_users(self): return InvitedUsers(self.id) diff --git a/app/s3_client/s3_csv_client.py b/app/s3_client/s3_csv_client.py index 7999426e1..8ddfb1a57 100644 --- a/app/s3_client/s3_csv_client.py +++ b/app/s3_client/s3_csv_client.py @@ -9,20 +9,20 @@ from app.s3_client.s3_logo_client import get_s3_object FILE_LOCATION_STRUCTURE = 'service-{}-notify/{}.csv' -def get_csv_location(service_id, upload_id): +def get_csv_location(service_id, upload_id, bucket=None): return ( - current_app.config['CSV_UPLOAD_BUCKET_NAME'], + bucket or 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 get_csv_upload(service_id, upload_id, bucket=None): + return get_s3_object(*get_csv_location(service_id, upload_id, bucket)) -def s3upload(service_id, filedata, region): +def s3upload(service_id, filedata, region, bucket=None): upload_id = str(uuid.uuid4()) - bucket_name, file_location = get_csv_location(service_id, upload_id) + bucket_name, file_location = get_csv_location(service_id, upload_id, bucket) utils_s3upload( filedata=filedata['data'], region=region, @@ -32,10 +32,10 @@ def s3upload(service_id, filedata, region): return upload_id -def s3download(service_id, upload_id): +def s3download(service_id, upload_id, bucket=None): contents = '' try: - key = get_csv_upload(service_id, upload_id) + key = get_csv_upload(service_id, upload_id, bucket) contents = key.get()['Body'].read().decode('utf-8') except botocore.exceptions.ClientError as e: current_app.logger.error("Unable to download s3 file {}".format( @@ -44,11 +44,11 @@ def s3download(service_id, upload_id): return contents -def set_metadata_on_csv_upload(service_id, upload_id, **kwargs): +def set_metadata_on_csv_upload(service_id, upload_id, bucket=None, **kwargs): get_csv_upload( - service_id, upload_id + service_id, upload_id, bucket=bucket ).copy_from( - CopySource='{}/{}'.format(*get_csv_location(service_id, upload_id)), + CopySource='{}/{}'.format(*get_csv_location(service_id, upload_id, bucket=bucket)), ServerSideEncryption='AES256', Metadata={ key: str(value) for key, value in kwargs.items() @@ -57,9 +57,18 @@ def set_metadata_on_csv_upload(service_id, upload_id, **kwargs): ) -def get_csv_metadata(service_id, upload_id): +def set_metadata_on_contact_list(service_id, upload_id, **kwargs): + return set_metadata_on_csv_upload( + service_id, + upload_id, + bucket=current_app.config['CONTACT_LIST_UPLOAD_BUCKET_NAME'], + **kwargs, + ) + + +def get_csv_metadata(service_id, upload_id, bucket=None): try: - key = get_csv_upload(service_id, upload_id) + key = get_csv_upload(service_id, upload_id, bucket) return key.get()['Metadata'] except botocore.exceptions.ClientError as e: current_app.logger.error("Unable to download s3 file {}".format( diff --git a/app/utils.py b/app/utils.py index 07d7b78c2..0fae647f0 100644 --- a/app/utils.py +++ b/app/utils.py @@ -323,6 +323,13 @@ class Spreadsheet(): pyexcel.free_resources() return instance + @classmethod + def from_file_form(cls, form): + return cls.from_file( + form.file.data, + filename=form.file.data.filename, + ) + @property def as_rows(self): if not self._rows: diff --git a/tests/app/main/views/test_uploads.py b/tests/app/main/views/test_uploads.py index fa9b12d9d..de794df60 100644 --- a/tests/app/main/views/test_uploads.py +++ b/tests/app/main/views/test_uploads.py @@ -885,9 +885,14 @@ def test_upload_csv_file_shows_error_banner( expected_thead, expected_tbody, ): - - mock_upload = mocker.patch('app.main.views.uploads.s3upload', return_value=fake_uuid) - mock_download = mocker.patch('app.main.views.uploads.s3download', return_value=file_contents) + mock_upload = mocker.patch( + 'app.models.contact_list.s3upload', + return_value=fake_uuid, + ) + mock_download = mocker.patch( + 'app.models.contact_list.s3download', + return_value=file_contents, + ) page = client_request.post( 'main.upload_contact_list', @@ -896,9 +901,16 @@ def test_upload_csv_file_shows_error_banner( _follow_redirects=True, ) mock_upload.assert_called_once_with( - SERVICE_ONE_ID, {'data': '', 'file_name': 'invalid.csv'}, ANY, + SERVICE_ONE_ID, + {'data': '', 'file_name': 'invalid.csv'}, + ANY, + bucket='test-contact-list', + ) + mock_download.assert_called_once_with( + SERVICE_ONE_ID, + fake_uuid, + bucket='test-contact-list', ) - mock_download.assert_called_once_with(SERVICE_ONE_ID, fake_uuid) assert normalize_spaces(page.select_one('.banner-dangerous').text) == expected_error @@ -920,9 +932,8 @@ def test_upload_csv_file_shows_error_banner_for_too_many_rows( mock_get_users_by_service, fake_uuid, ): - - mocker.patch('app.main.views.uploads.s3upload', return_value=fake_uuid) - mocker.patch('app.main.views.uploads.s3download', return_value='\n'.join( + mocker.patch('app.models.contact_list.s3upload', return_value=fake_uuid) + mocker.patch('app.models.contact_list.s3download', return_value='\n'.join( ['phone number'] + (['07700900986'] * 50001) )) @@ -952,8 +963,8 @@ def test_upload_csv_shows_trial_mode_error( fake_uuid, mocker ): - mocker.patch('app.main.views.uploads.s3upload', return_value=fake_uuid) - mocker.patch('app.main.views.uploads.s3download', return_value=( + mocker.patch('app.models.contact_list.s3upload', return_value=fake_uuid) + mocker.patch('app.models.contact_list.s3download', return_value=( 'phone number\n' '07900900321' # Not in team )) @@ -983,10 +994,10 @@ def test_upload_csv_shows_ok_page( fake_uuid, mocker ): - mocker.patch('app.main.views.uploads.s3download', return_value='\n'.join( + mocker.patch('app.models.contact_list.s3download', return_value='\n'.join( ['email address'] + ['test@example.com'] * 51 )) - mock_metadata_set = mocker.patch('app.main.views.uploads.set_metadata_on_csv_upload') + mock_metadata_set = mocker.patch('app.models.contact_list.set_metadata_on_csv_upload') page = client_request.get( 'main.check_contact_list', @@ -999,6 +1010,7 @@ def test_upload_csv_shows_ok_page( mock_metadata_set.assert_called_once_with( SERVICE_ONE_ID, fake_uuid, + bucket='test-contact-list', row_count=51, original_file_name='good times.xlsx', template_type='email', @@ -1038,7 +1050,7 @@ def test_save_contact_list( fake_uuid, mock_create_contact_list, ): - mocker.patch('app.models.contact_list.get_csv_metadata', return_value={ + mock_get_metadata = mocker.patch('app.models.contact_list.get_csv_metadata', return_value={ 'row_count': 999, 'valid': True, 'original_file_name': 'example.csv', @@ -1056,6 +1068,11 @@ def test_save_contact_list( _external=True, ) ) + mock_get_metadata.assert_called_once_with( + SERVICE_ONE_ID, + fake_uuid, + bucket='test-contact-list', + ) mock_create_contact_list.assert_called_once_with( service_id=SERVICE_ONE_ID, upload_id=fake_uuid, diff --git a/tests/app/s3_client/test_s3_csv_client.py b/tests/app/s3_client/test_s3_csv_client.py index f4014065f..c8e07092e 100644 --- a/tests/app/s3_client/test_s3_csv_client.py +++ b/tests/app/s3_client/test_s3_csv_client.py @@ -12,7 +12,7 @@ def test_sets_metadata(client, mocker): set_metadata_on_csv_upload('1234', '5678', foo='bar', baz=True) - mocked_get_s3_object.assert_called_once_with('1234', '5678') + mocked_get_s3_object.assert_called_once_with('1234', '5678', bucket=None) mocked_s3_object.copy_from.assert_called_once_with( CopySource='test-notifications-csv-upload/service-1234-notify/5678.csv', Metadata={'baz': 'True', 'foo': 'bar'},