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

@@ -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)):

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(

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):

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),