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)