From d654a87f5c2f34952190b5c7b2263e1a0126dd4f Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Wed, 30 Jan 2019 10:57:35 +0000 Subject: [PATCH] Rename the current s3 logo client functions We will be adding methods to deal with letter logos, so this adds 'email' to the method names for email logos to avoid confusion later. --- app/main/views/email_branding.py | 24 ++++---- app/s3_client/s3_logo_client.py | 16 +++--- tests/app/main/views/test_email_branding.py | 62 ++++++++++----------- tests/app/s3_client/test_s3_logo_client.py | 34 +++++------ 4 files changed, 68 insertions(+), 68 deletions(-) diff --git a/app/main/views/email_branding.py b/app/main/views/email_branding.py index 6b754df11..338de3c77 100644 --- a/app/main/views/email_branding.py +++ b/app/main/views/email_branding.py @@ -6,11 +6,11 @@ from app.main import main from app.main.forms import SearchTemplatesForm, ServiceUpdateEmailBranding from app.s3_client.s3_logo_client import ( TEMP_TAG, - delete_temp_file, - delete_temp_files_created_by, - permanent_logo_name, + delete_email_temp_file, + delete_email_temp_files_created_by, + permanent_email_logo_name, persist_logo, - upload_logo, + upload_email_logo, ) from app.utils import AgreementInfo, get_logo_cdn_domain, user_is_platform_admin @@ -49,7 +49,7 @@ def update_email_branding(branding_id, logo=None): if form.validate_on_submit(): if form.file.data: - upload_filename = upload_logo( + upload_filename = upload_email_logo( form.file.data.filename, form.file.data, current_app.config['AWS_REGION'], @@ -57,11 +57,11 @@ def update_email_branding(branding_id, logo=None): ) if logo and logo.startswith(TEMP_TAG.format(user_id=session['user_id'])): - delete_temp_file(logo) + delete_email_temp_file(logo) return redirect(url_for('.update_email_branding', branding_id=branding_id, logo=upload_filename)) - updated_logo_name = permanent_logo_name(logo, session["user_id"]) if logo else None + updated_logo_name = permanent_email_logo_name(logo, session["user_id"]) if logo else None email_branding_client.update_email_branding( branding_id=branding_id, @@ -76,7 +76,7 @@ def update_email_branding(branding_id, logo=None): if logo: persist_logo(logo, updated_logo_name) - delete_temp_files_created_by(session["user_id"]) + delete_email_temp_files_created_by(session["user_id"]) return redirect(url_for('.email_branding', branding_id=branding_id)) @@ -98,7 +98,7 @@ def create_email_branding(logo=None): if form.validate_on_submit(): if form.file.data: - upload_filename = upload_logo( + upload_filename = upload_email_logo( form.file.data.filename, form.file.data, current_app.config['AWS_REGION'], @@ -106,11 +106,11 @@ def create_email_branding(logo=None): ) if logo and logo.startswith(TEMP_TAG.format(user_id=session['user_id'])): - delete_temp_file(logo) + delete_email_temp_file(logo) return redirect(url_for('.create_email_branding', logo=upload_filename)) - updated_logo_name = permanent_logo_name(logo, session["user_id"]) if logo else None + updated_logo_name = permanent_email_logo_name(logo, session["user_id"]) if logo else None email_branding_client.create_email_branding( logo=updated_logo_name, @@ -124,7 +124,7 @@ def create_email_branding(logo=None): if logo: persist_logo(logo, updated_logo_name) - delete_temp_files_created_by(session["user_id"]) + delete_email_temp_files_created_by(session["user_id"]) return redirect(url_for('.email_branding')) diff --git a/app/s3_client/s3_logo_client.py b/app/s3_client/s3_logo_client.py index a9ca81b97..63dadb10d 100644 --- a/app/s3_client/s3_logo_client.py +++ b/app/s3_client/s3_logo_client.py @@ -5,7 +5,7 @@ from flask import current_app from notifications_utils.s3 import s3upload as utils_s3upload TEMP_TAG = 'temp-{user_id}_' -LOGO_LOCATION_STRUCTURE = '{temp}{unique_id}-{filename}' +EMAIL_LOGO_LOCATION_STRUCTURE = '{temp}{unique_id}-{filename}' def get_s3_object(bucket_name, filename): @@ -33,12 +33,12 @@ def get_s3_objects_filter_by_prefix(prefix): return s3.Bucket(bucket_name).objects.filter(Prefix=prefix) -def get_temp_truncated_filename(filename, user_id): +def get_temp_truncated_email_filename(filename, user_id): return filename[len(TEMP_TAG.format(user_id=user_id)):] -def upload_logo(filename, filedata, region, user_id): - upload_file_name = LOGO_LOCATION_STRUCTURE.format( +def upload_email_logo(filename, filedata, region, user_id): + upload_file_name = EMAIL_LOGO_LOCATION_STRUCTURE.format( temp=TEMP_TAG.format(user_id=user_id), unique_id=str(uuid.uuid4()), filename=filename @@ -55,19 +55,19 @@ def upload_logo(filename, filedata, region, user_id): return upload_file_name -def permanent_logo_name(filename, user_id): +def permanent_email_logo_name(filename, user_id): if filename.startswith(TEMP_TAG.format(user_id=user_id)): - return get_temp_truncated_filename(filename=filename, user_id=user_id) + return get_temp_truncated_email_filename(filename=filename, user_id=user_id) else: return filename -def delete_temp_files_created_by(user_id): +def delete_email_temp_files_created_by(user_id): for obj in get_s3_objects_filter_by_prefix(TEMP_TAG.format(user_id=user_id)): delete_s3_object(obj.key) -def delete_temp_file(filename): +def delete_email_temp_file(filename): if not filename.startswith(TEMP_TAG[:5]): raise ValueError('Not a temp file: {}'.format(filename)) diff --git a/tests/app/main/views/test_email_branding.py b/tests/app/main/views/test_email_branding.py index 3dd168537..be702a3bd 100644 --- a/tests/app/main/views/test_email_branding.py +++ b/tests/app/main/views/test_email_branding.py @@ -6,7 +6,7 @@ from bs4 import BeautifulSoup from flask import url_for from notifications_python_client.errors import HTTPError -from app.s3_client.s3_logo_client import LOGO_LOCATION_STRUCTURE, TEMP_TAG +from app.s3_client.s3_logo_client import EMAIL_LOGO_LOCATION_STRUCTURE, TEMP_TAG from tests.conftest import ( mock_get_email_branding, normalize_spaces, @@ -114,7 +114,7 @@ def test_create_new_email_branding_without_logo( } mock_persist = mocker.patch('app.main.views.email_branding.persist_logo') - mocker.patch('app.main.views.email_branding.delete_temp_files_created_by') + mocker.patch('app.main.views.email_branding.delete_email_temp_files_created_by') logged_in_platform_admin_client.post( url_for('.create_email_branding'), @@ -141,7 +141,7 @@ def test_cant_create_new_email_branding_with_unknown_domain( mock_create_email_branding ): mock_persist = mocker.patch('app.main.views.email_branding.persist_logo') - mocker.patch('app.main.views.email_branding.delete_temp_files_created_by') + mocker.patch('app.main.views.email_branding.delete_email_temp_files_created_by') client_request.login(platform_admin_user(fake_uuid)) page = client_request.post( @@ -197,7 +197,7 @@ def test_rejects_non_canonical_domain_when_adding_email_branding( expected_error, ): mocker.patch('app.main.views.email_branding.persist_logo') - mocker.patch('app.main.views.email_branding.delete_temp_files_created_by') + mocker.patch('app.main.views.email_branding.delete_email_temp_files_created_by') data = { 'logo': None, 'colour': '#ff0000', @@ -225,7 +225,7 @@ def test_create_email_branding_requires_a_name_when_submitting_logo_details( mock_create_email_branding, ): mocker.patch('app.main.views.email_branding.persist_logo') - mocker.patch('app.main.views.email_branding.delete_temp_files_created_by') + mocker.patch('app.main.views.email_branding.delete_email_temp_files_created_by') data = { 'operation': 'email-branding-details', 'logo': '', @@ -252,7 +252,7 @@ def test_create_email_branding_does_not_require_a_name_when_uploading_a_file( mocker, fake_uuid, ): - mocker.patch('app.main.views.email_branding.upload_logo', return_value='temp_filename') + mocker.patch('app.main.views.email_branding.upload_email_logo', return_value='temp_filename') data = { 'file': (BytesIO(''.encode('utf-8')), 'test.png'), 'colour': '', @@ -290,14 +290,14 @@ def test_create_new_email_branding_when_branding_saved( 'brand_type': 'org_banner' } - temp_filename = LOGO_LOCATION_STRUCTURE.format( + temp_filename = EMAIL_LOGO_LOCATION_STRUCTURE.format( temp=TEMP_TAG.format(user_id=user_id), unique_id=fake_uuid, filename=data['logo'] ) mocker.patch('app.main.views.email_branding.persist_logo') - mocker.patch('app.main.views.email_branding.delete_temp_files_created_by') + mocker.patch('app.main.views.email_branding.delete_email_temp_files_created_by') logged_in_platform_admin_client.post( url_for('.create_email_branding', logo=temp_filename), @@ -342,24 +342,24 @@ def test_deletes_previous_temp_logo_after_uploading_logo( with logged_in_platform_admin_client.session_transaction() as session: user_id = session["user_id"] - temp_old_filename = LOGO_LOCATION_STRUCTURE.format( + temp_old_filename = EMAIL_LOGO_LOCATION_STRUCTURE.format( temp=TEMP_TAG.format(user_id=user_id), unique_id=fake_uuid, filename='old_test.png' ) - temp_filename = LOGO_LOCATION_STRUCTURE.format( + temp_filename = EMAIL_LOGO_LOCATION_STRUCTURE.format( temp=TEMP_TAG.format(user_id=user_id), unique_id=fake_uuid, filename='test.png' ) - mocked_upload_logo = mocker.patch( - 'app.main.views.email_branding.upload_logo', + mocked_upload_email_logo = mocker.patch( + 'app.main.views.email_branding.upload_email_logo', return_value=temp_filename ) - mocked_delete_temp_file = mocker.patch('app.main.views.email_branding.delete_temp_file') + mocked_delete_email_temp_file = mocker.patch('app.main.views.email_branding.delete_email_temp_file') logged_in_platform_admin_client.post( url_for('main.create_email_branding', logo=temp_old_filename, branding_id=fake_uuid), @@ -367,9 +367,9 @@ def test_deletes_previous_temp_logo_after_uploading_logo( content_type='multipart/form-data' ) - assert mocked_upload_logo.called - assert mocked_delete_temp_file.called - assert mocked_delete_temp_file.call_args == call(temp_old_filename) + assert mocked_upload_email_logo.called + assert mocked_delete_email_temp_file.called + assert mocked_delete_email_temp_file.call_args == call(temp_old_filename) def test_update_existing_branding( @@ -391,14 +391,14 @@ def test_update_existing_branding( 'brand_type': 'both' } - temp_filename = LOGO_LOCATION_STRUCTURE.format( + temp_filename = EMAIL_LOGO_LOCATION_STRUCTURE.format( temp=TEMP_TAG.format(user_id=user_id), unique_id=fake_uuid, filename=data['logo'] ) mocker.patch('app.main.views.email_branding.persist_logo') - mocker.patch('app.main.views.email_branding.delete_temp_files_created_by') + mocker.patch('app.main.views.email_branding.delete_email_temp_files_created_by') logged_in_platform_admin_client.post( url_for('.update_email_branding', logo=temp_filename, branding_id=fake_uuid), @@ -431,14 +431,14 @@ def test_temp_logo_is_shown_after_uploading_logo( with logged_in_platform_admin_client.session_transaction() as session: user_id = session["user_id"] - temp_filename = LOGO_LOCATION_STRUCTURE.format( + temp_filename = EMAIL_LOGO_LOCATION_STRUCTURE.format( temp=TEMP_TAG.format(user_id=user_id), unique_id=fake_uuid, filename='test.png' ) - mocker.patch('app.main.views.email_branding.upload_logo', return_value=temp_filename) - mocker.patch('app.main.views.email_branding.delete_temp_file') + mocker.patch('app.main.views.email_branding.upload_email_logo', return_value=temp_filename) + mocker.patch('app.main.views.email_branding.delete_email_temp_file') response = logged_in_platform_admin_client.post( url_for('main.create_email_branding'), @@ -463,12 +463,12 @@ def test_logo_persisted_when_organisation_saved( with logged_in_platform_admin_client.session_transaction() as session: user_id = session["user_id"] - temp_filename = LOGO_LOCATION_STRUCTURE.format( + temp_filename = EMAIL_LOGO_LOCATION_STRUCTURE.format( temp=TEMP_TAG.format(user_id=user_id), unique_id=fake_uuid, filename='test.png') - mocked_upload_logo = mocker.patch('app.main.views.email_branding.upload_logo') + mocked_upload_email_logo = mocker.patch('app.main.views.email_branding.upload_email_logo') mocked_persist_logo = mocker.patch('app.main.views.email_branding.persist_logo') - mocked_delete_temp_files_by = mocker.patch('app.main.views.email_branding.delete_temp_files_created_by') + mocked_delete_email_temp_files_by = mocker.patch('app.main.views.email_branding.delete_email_temp_files_created_by') resp = logged_in_platform_admin_client.post( url_for('.create_email_branding', logo=temp_filename), @@ -476,10 +476,10 @@ def test_logo_persisted_when_organisation_saved( ) assert resp.status_code == 302 - assert not mocked_upload_logo.called + assert not mocked_upload_email_logo.called assert mocked_persist_logo.called - assert mocked_delete_temp_files_by.called - assert mocked_delete_temp_files_by.call_args == call(user_id) + assert mocked_delete_email_temp_files_by.called + assert mocked_delete_email_temp_files_by.call_args == call(user_id) assert mock_create_email_branding.called @@ -492,11 +492,11 @@ def test_logo_does_not_get_persisted_if_updating_email_branding_client_throws_an with logged_in_platform_admin_client.session_transaction() as session: user_id = session["user_id"] - temp_filename = LOGO_LOCATION_STRUCTURE.format( + temp_filename = EMAIL_LOGO_LOCATION_STRUCTURE.format( temp=TEMP_TAG.format(user_id=user_id), unique_id=fake_uuid, filename='test.png') mocked_persist_logo = mocker.patch('app.main.views.email_branding.persist_logo') - mocked_delete_temp_files_by = mocker.patch('app.main.views.email_branding.delete_temp_files_created_by') + mocked_delete_email_temp_files_by = mocker.patch('app.main.views.email_branding.delete_email_temp_files_created_by') mocker.patch('app.main.views.email_branding.email_branding_client.create_email_branding', side_effect=HTTPError()) logged_in_platform_admin_client.post( @@ -505,7 +505,7 @@ def test_logo_does_not_get_persisted_if_updating_email_branding_client_throws_an ) assert not mocked_persist_logo.called - assert not mocked_delete_temp_files_by.called + assert not mocked_delete_email_temp_files_by.called @pytest.mark.parametrize('colour_hex, expected_status_code', [ @@ -530,7 +530,7 @@ def test_colour_regex_validation( 'brand_type': 'org' } - mocker.patch('app.main.views.email_branding.delete_temp_files_created_by') + mocker.patch('app.main.views.email_branding.delete_email_temp_files_created_by') response = logged_in_platform_admin_client.post( url_for('.create_email_branding'), diff --git a/tests/app/s3_client/test_s3_logo_client.py b/tests/app/s3_client/test_s3_logo_client.py index 332598f78..82c427cd0 100644 --- a/tests/app/s3_client/test_s3_logo_client.py +++ b/tests/app/s3_client/test_s3_logo_client.py @@ -4,13 +4,13 @@ from unittest.mock import call import pytest from app.s3_client.s3_logo_client import ( - LOGO_LOCATION_STRUCTURE, + EMAIL_LOGO_LOCATION_STRUCTURE, TEMP_TAG, - delete_temp_file, - delete_temp_files_created_by, - permanent_logo_name, + delete_email_temp_file, + delete_email_temp_files_created_by, + permanent_email_logo_name, persist_logo, - upload_logo, + upload_email_logo, ) bucket = 'test_bucket' @@ -22,16 +22,16 @@ region = 'eu-west1' @pytest.fixture def upload_filename(fake_uuid): - return LOGO_LOCATION_STRUCTURE.format( + return EMAIL_LOGO_LOCATION_STRUCTURE.format( temp=TEMP_TAG.format(user_id=fake_uuid), unique_id=upload_id, filename=filename) -def test_upload_logo_calls_correct_args(client, mocker, fake_uuid, upload_filename): +def test_upload_email_logo_calls_correct_args(client, mocker, fake_uuid, upload_filename): mocker.patch('uuid.uuid4', return_value=upload_id) mocker.patch.dict('flask.current_app.config', {'LOGO_UPLOAD_BUCKET_NAME': bucket}) mocked_s3_upload = mocker.patch('app.s3_client.s3_logo_client.utils_s3upload') - upload_logo(filename=filename, user_id=fake_uuid, filedata=data, region=region) + upload_email_logo(filename=filename, user_id=fake_uuid, filedata=data, region=region) assert mocked_s3_upload.called_once_with( filedata=data, @@ -46,7 +46,7 @@ def test_persist_logo(client, mocker, fake_uuid, upload_filename): mocked_get_s3_object = mocker.patch('app.s3_client.s3_logo_client.get_s3_object') mocked_delete_s3_object = mocker.patch('app.s3_client.s3_logo_client.delete_s3_object') - new_filename = permanent_logo_name(upload_filename, fake_uuid) + new_filename = permanent_email_logo_name(upload_filename, fake_uuid) persist_logo(upload_filename, new_filename) @@ -65,27 +65,27 @@ def test_persist_logo_returns_if_not_temp(client, mocker, fake_uuid): mocked_delete_s3_object.assert_not_called() -def test_permanent_logo_name_removes_temp_tag_from_filename(upload_filename, fake_uuid): - new_name = permanent_logo_name(upload_filename, fake_uuid) +def test_permanent_email_logo_name_removes_TEMP_TAG_from_filename(upload_filename, fake_uuid): + new_name = permanent_email_logo_name(upload_filename, fake_uuid) assert new_name == 'test_uuid-test.png' -def test_permanent_logo_name_does_not_change_filenames_with_no_temp_tag(): +def test_permanent_email_logo_name_does_not_change_filenames_with_no_TEMP_TAG(): filename = 'logo.png' - new_name = permanent_logo_name(filename, filename) + new_name = permanent_email_logo_name(filename, filename) assert new_name == filename -def test_delete_temp_files_created_by_user(client, mocker, fake_uuid): +def test_delete_email_temp_files_created_by_user(client, mocker, fake_uuid): obj = namedtuple("obj", ["key"]) objs = [obj(key='test1'), obj(key='test2')] mocker.patch('app.s3_client.s3_logo_client.get_s3_objects_filter_by_prefix', return_value=objs) mocked_delete_s3_object = mocker.patch('app.s3_client.s3_logo_client.delete_s3_object') - delete_temp_files_created_by(fake_uuid) + delete_email_temp_files_created_by(fake_uuid) assert mocked_delete_s3_object.called_with_args(objs[0].key) for index, arg in enumerate(mocked_delete_s3_object.call_args_list): @@ -95,7 +95,7 @@ def test_delete_temp_files_created_by_user(client, mocker, fake_uuid): def test_delete_single_temp_file(client, mocker, fake_uuid, upload_filename): mocked_delete_s3_object = mocker.patch('app.s3_client.s3_logo_client.delete_s3_object') - delete_temp_file(upload_filename) + delete_email_temp_file(upload_filename) assert mocked_delete_s3_object.called_with_args(upload_filename) @@ -105,7 +105,7 @@ def test_does_not_delete_non_temp_file(client, mocker, fake_uuid): mocked_delete_s3_object = mocker.patch('app.s3_client.s3_logo_client.delete_s3_object') with pytest.raises(ValueError) as error: - delete_temp_file(filename) + delete_email_temp_file(filename) assert mocked_delete_s3_object.called_with_args(filename) assert str(error.value) == 'Not a temp file: {}'.format(filename)