From 52acf890ddea4926e8af669317a3b080c2174a46 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Fri, 25 Jan 2019 12:07:04 +0000 Subject: [PATCH] Stop persisting email branding logos if saving to db fails We were previously persisting the logo for the email branding and deleting the temp files that get created before trying to update the database with the new email branding. This meant that if there was an error when saving (e.g. the domain used was a duplicate) the final logo was already in S3 and trying to go 'back' in the browser would give an error since the temp files needed to display the create branding page had already been deleted. This changes the order we do things in, so that we try persisting the email branding to the database first. --- app/main/s3_client.py | 13 +++---- app/main/views/email_branding.py | 25 +++++++------ tests/app/main/test_s3_client.py | 37 +++++++++++++------ tests/app/main/views/test_email_branding.py | 40 ++++++++++++++++++--- 4 files changed, 81 insertions(+), 34 deletions(-) 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),