Use the correct bucket for storing contact lists

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.
2026-07-31 11:49:58 -04:00 · 2020-03-13 13:53:18 +00:00
parent 1c02476ee7
commit 03f2368deb
9 changed files with 121 additions and 47 deletions
--- 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'
--- 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(
--- 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/<uuid:service_id>/save-contact-list/<uuid:upload_id>", 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,
--- 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)
--- 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)
--- 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(
--- 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:
--- 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,
--- 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'},