diff --git a/app/main/s3_client.py b/app/main/s3_client.py index 474848c0a..5fc79c6f2 100644 --- a/app/main/s3_client.py +++ b/app/main/s3_client.py @@ -31,7 +31,9 @@ def delete_s3_object(filename): get_s3_object(bucket_name, filename).delete() -def rename_s3_object(old_name, new_name): +def persist_logo(old_name, new_name): + if old_name == new_name: + return bucket_name = current_app.config['LOGO_UPLOAD_BUCKET_NAME'] get_s3_object(bucket_name, new_name).copy_from( CopySource='{}/{}'.format(bucket_name, old_name)) @@ -110,17 +112,12 @@ def upload_logo(filename, filedata, region, user_id): return upload_file_name -def persist_logo(filename, user_id): +def permanent_logo_name(filename, user_id): if filename.startswith(TEMP_TAG.format(user_id=user_id)): - persisted_filename = get_temp_truncated_filename( - filename=filename, user_id=user_id) + return get_temp_truncated_filename(filename=filename, user_id=user_id) else: return filename - rename_s3_object(filename, persisted_filename) - - return persisted_filename - def delete_temp_files_created_by(user_id): for obj in get_s3_objects_filter_by_prefix(TEMP_TAG.format(user_id=user_id)): diff --git a/app/main/views/email_branding.py b/app/main/views/email_branding.py index 855075c78..5a5b3f9a4 100644 --- a/app/main/views/email_branding.py +++ b/app/main/views/email_branding.py @@ -8,6 +8,7 @@ from app.main.s3_client import ( TEMP_TAG, delete_temp_file, delete_temp_files_created_by, + permanent_logo_name, persist_logo, upload_logo, ) @@ -60,14 +61,11 @@ def update_email_branding(branding_id, logo=None): return redirect(url_for('.update_email_branding', branding_id=branding_id, logo=upload_filename)) - if logo: - logo = persist_logo(logo, session["user_id"]) - - delete_temp_files_created_by(session["user_id"]) + updated_logo_name = permanent_logo_name(logo, session["user_id"]) if logo else None email_branding_client.update_email_branding( branding_id=branding_id, - logo=logo, + logo=updated_logo_name, name=form.name.data, text=form.text.data, colour=form.colour.data, @@ -75,6 +73,11 @@ def update_email_branding(branding_id, logo=None): brand_type=form.brand_type.data, ) + if logo: + persist_logo(logo, updated_logo_name) + + delete_temp_files_created_by(session["user_id"]) + return redirect(url_for('.email_branding', branding_id=branding_id)) return render_template( @@ -107,13 +110,10 @@ def create_email_branding(logo=None): return redirect(url_for('.create_email_branding', logo=upload_filename)) - if logo: - logo = persist_logo(logo, session["user_id"]) - - delete_temp_files_created_by(session["user_id"]) + updated_logo_name = permanent_logo_name(logo, session["user_id"]) if logo else None email_branding_client.create_email_branding( - logo=logo, + logo=updated_logo_name, name=form.name.data, text=form.text.data, colour=form.colour.data, @@ -121,6 +121,11 @@ def create_email_branding(logo=None): brand_type=form.brand_type.data, ) + if logo: + persist_logo(logo, updated_logo_name) + + delete_temp_files_created_by(session["user_id"]) + return redirect(url_for('.email_branding')) return render_template( diff --git a/tests/app/main/test_s3_client.py b/tests/app/main/test_s3_client.py index db05ee128..20c6d80a3 100644 --- a/tests/app/main/test_s3_client.py +++ b/tests/app/main/test_s3_client.py @@ -8,7 +8,7 @@ from app.main.s3_client import ( TEMP_TAG, delete_temp_file, delete_temp_files_created_by, - get_temp_truncated_filename, + permanent_logo_name, persist_logo, set_metadata_on_csv_upload, upload_logo, @@ -44,24 +44,39 @@ def test_upload_logo_calls_correct_args(client, mocker, fake_uuid, upload_filena def test_persist_logo(client, mocker, fake_uuid, upload_filename): mocker.patch.dict('flask.current_app.config', {'LOGO_UPLOAD_BUCKET_NAME': bucket}) - mocked_rename_s3_object = mocker.patch('app.main.s3_client.rename_s3_object') + mocked_get_s3_object = mocker.patch('app.main.s3_client.get_s3_object') + mocked_delete_s3_object = mocker.patch('app.main.s3_client.delete_s3_object') - persisted_filename = persist_logo(filename=upload_filename, user_id=fake_uuid) + new_filename = permanent_logo_name(upload_filename, fake_uuid) - assert mocked_rename_s3_object.called_once_with( - upload_filename, get_temp_truncated_filename(upload_filename, fake_uuid)) - assert persisted_filename == get_temp_truncated_filename(upload_filename, fake_uuid) + persist_logo(upload_filename, new_filename) + + assert mocked_get_s3_object.called_once_with(bucket, new_filename) + assert mocked_delete_s3_object.called_once_with(bucket, upload_filename) def test_persist_logo_returns_if_not_temp(client, mocker, fake_uuid): filename = 'logo.png' - mocker.patch.dict('flask.current_app.config', {'LOGO_UPLOAD_BUCKET_NAME': bucket}) - mocked_rename_s3_object = mocker.patch('app.main.s3_client.rename_s3_object') + persist_logo(filename, filename) - persisted_filename = persist_logo(filename=filename, user_id=fake_uuid) + mocked_get_s3_object = mocker.patch('app.main.s3_client.get_s3_object') + mocked_delete_s3_object = mocker.patch('app.main.s3_client.delete_s3_object') - assert not mocked_rename_s3_object.called - assert persisted_filename == filename + mocked_get_s3_object.assert_not_called() + 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) + + assert new_name == 'test_uuid-test.png' + + +def test_permanent_logo_name_does_not_change_filenames_with_no_temp_tag(): + filename = 'logo.png' + new_name = permanent_logo_name(filename, filename) + + assert new_name == filename def test_delete_temp_files_created_by_user(client, mocker, fake_uuid): diff --git a/tests/app/main/views/test_email_branding.py b/tests/app/main/views/test_email_branding.py index 2e4a6bead..fdf872096 100644 --- a/tests/app/main/views/test_email_branding.py +++ b/tests/app/main/views/test_email_branding.py @@ -4,6 +4,7 @@ from unittest.mock import call import pytest from bs4 import BeautifulSoup from flask import url_for +from notifications_python_client.errors import HTTPError from app.main.s3_client import LOGO_LOCATION_STRUCTURE, TEMP_TAG from tests.conftest import ( @@ -241,7 +242,7 @@ def test_create_new_email_branding_when_branding_saved( filename=data['logo'] ) - mocker.patch('app.main.views.email_branding.persist_logo', return_value=data['logo']) + mocker.patch('app.main.views.email_branding.persist_logo') mocker.patch('app.main.views.email_branding.delete_temp_files_created_by') logged_in_platform_admin_client.post( @@ -257,9 +258,11 @@ def test_create_new_email_branding_when_branding_saved( } ) + updated_logo_name = '{}-{}'.format(fake_uuid, data['logo']) + assert mock_create_email_branding.called assert mock_create_email_branding.call_args == call( - logo=data['logo'], + logo=updated_logo_name, name=data['name'], text=data['text'], colour=data['colour'], @@ -340,7 +343,7 @@ def test_update_existing_branding( filename=data['logo'] ) - mocker.patch('app.main.views.email_branding.persist_logo', return_value=data['logo']) + mocker.patch('app.main.views.email_branding.persist_logo') mocker.patch('app.main.views.email_branding.delete_temp_files_created_by') logged_in_platform_admin_client.post( @@ -352,10 +355,12 @@ def test_update_existing_branding( } ) + updated_logo_name = '{}-{}'.format(fake_uuid, data['logo']) + assert mock_update_email_branding.called assert mock_update_email_branding.call_args == call( branding_id=fake_uuid, - logo=data['logo'], + logo=updated_logo_name, name=data['name'], text=data['text'], colour=data['colour'], @@ -408,7 +413,7 @@ def test_logo_persisted_when_organisation_saved( 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_persist_logo = mocker.patch('app.main.views.email_branding.persist_logo', return_value='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') resp = logged_in_platform_admin_client.post( @@ -424,6 +429,31 @@ def test_logo_persisted_when_organisation_saved( assert mock_create_email_branding.called +def test_logo_does_not_get_persisted_if_updating_email_branding_client_throws_an_error( + logged_in_platform_admin_client, + mock_create_email_branding, + mocker, + fake_uuid +): + with logged_in_platform_admin_client.session_transaction() as session: + user_id = session["user_id"] + + temp_filename = 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') + mocker.patch('app.main.views.email_branding.email_branding_client.create_email_branding', side_effect=HTTPError()) + + logged_in_platform_admin_client.post( + url_for('.create_email_branding', logo=temp_filename), + content_type='multipart/form-data' + ) + + assert not mocked_persist_logo.called + assert not mocked_delete_temp_files_by.called + + @pytest.mark.parametrize('colour_hex, expected_status_code', [ ('#FF00FF', 302), ('hello', 200),