diff --git a/app/main/views/letter_branding.py b/app/main/views/letter_branding.py index 20810929d..c2a126878 100644 --- a/app/main/views/letter_branding.py +++ b/app/main/views/letter_branding.py @@ -1,3 +1,4 @@ +from botocore.exceptions import ClientError as BotoClientError from flask import ( current_app, redirect, @@ -45,6 +46,92 @@ def letter_branding(): ) +@main.route("/letter-branding//edit", methods=['GET', 'POST']) +@main.route("/letter-branding//edit/", methods=['GET', 'POST']) +@login_required +@user_is_platform_admin +def update_letter_branding(branding_id, logo=None): + letter_branding = letter_branding_client.get_letter_branding(branding_id) + + file_upload_form = SVGFileUpload() + letter_branding_details_form = ServiceLetterBrandingDetails( + name=letter_branding['name'], + domain=letter_branding['domain'] + ) + + file_upload_form_submitted = file_upload_form.file.data + details_form_submitted = request.form.get('operation') == 'branding-details' + + logo = logo if logo else permanent_letter_logo_name(letter_branding['filename'], 'svg') + + if file_upload_form_submitted and file_upload_form.validate_on_submit(): + upload_filename = upload_letter_temp_logo( + file_upload_form.file.data.filename, + file_upload_form.file.data, + current_app.config['AWS_REGION'], + user_id=session["user_id"] + ) + + if logo.startswith(LETTER_TEMP_TAG.format(user_id=session['user_id'])): + delete_letter_temp_file(logo) + + return redirect(url_for('.update_letter_branding', branding_id=branding_id, logo=upload_filename)) + + if details_form_submitted and letter_branding_details_form.validate_on_submit(): + db_filename = letter_filename_for_db(logo, session['user_id']) + + try: + if db_filename == letter_branding['filename']: + + letter_branding_client.update_letter_branding( + branding_id=branding_id, + filename=db_filename, + name=letter_branding_details_form.name.data, + domain=letter_branding_details_form.domain.data + ) + + return redirect(url_for('main.letter_branding')) + else: + png_file = get_png_file_from_svg(logo) + + letter_branding_client.update_letter_branding( + branding_id=branding_id, + filename=db_filename, + name=letter_branding_details_form.name.data, + domain=letter_branding_details_form.domain.data + ) + + upload_letter_logos(logo, db_filename, png_file, session['user_id']) + + return redirect(url_for('main.letter_branding')) + + except HTTPError as e: + if 'domain' in e.message: + letter_branding_details_form.domain.errors.append(e.message['domain'][0]) + elif 'name' in e.message: + letter_branding_details_form.name.errors.append(e.message['name'][0]) + else: + raise e + except BotoClientError: + # we had a problem saving the file - rollback the db changes + letter_branding_client.update_letter_branding( + branding_id=branding_id, + filename=letter_branding['filename'], + name=letter_branding['name'], + domain=letter_branding['domain'] + ) + file_upload_form.file.errors = ['Error saving uploaded file - try uploading again'] + + return render_template( + 'views/letter-branding/manage-letter-branding.html', + file_upload_form=file_upload_form, + letter_branding_details_form=letter_branding_details_form, + cdn_url=get_logo_cdn_domain(), + logo=logo, + is_update=True + ) + + @main.route("/letter-branding/create", methods=['GET', 'POST']) @main.route("/letter-branding/create/", methods=['GET', 'POST']) @login_required diff --git a/app/navigation.py b/app/navigation.py index 6ba3117e1..7d00606ce 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -88,6 +88,7 @@ class HeaderNavigation(Navigation): 'suspend_service', 'trial_services', 'update_email_branding', + 'update_letter_branding', 'user_information', 'view_provider', 'view_providers', @@ -499,6 +500,7 @@ class MainNavigation(Navigation): 'two_factor_email', 'two_factor_email_sent', 'update_email_branding', + 'update_letter_branding', 'user_information', 'user_profile', 'user_profile_email', @@ -731,6 +733,7 @@ class CaseworkNavigation(Navigation): 'two_factor_email', 'two_factor_email_sent', 'update_email_branding', + 'update_letter_branding', 'usage', 'user_information', 'user_profile', @@ -964,6 +967,7 @@ class OrgNavigation(Navigation): 'two_factor_email', 'two_factor_email_sent', 'update_email_branding', + 'update_letter_branding', 'usage', 'user_information', 'user_profile', diff --git a/app/templates/views/letter-branding/manage-letter-branding.html b/app/templates/views/letter-branding/manage-letter-branding.html index 1e76d9e3f..0bde542b6 100644 --- a/app/templates/views/letter-branding/manage-letter-branding.html +++ b/app/templates/views/letter-branding/manage-letter-branding.html @@ -5,12 +5,12 @@ {% from "components/form.html" import form_wrapper %} {% block per_page_title %} - Create letter branding + {{ '{} letter branding'.format('Update' if is_update else 'Create')}} {% endblock %} {% block platform_admin_content %} -

Add letter branding

+

{{ '{} letter branding'.format('Update' if is_update else 'Add')}}

{% if logo %} @@ -18,7 +18,7 @@
{% endif %} - {{ file_upload(file_upload_form.file) }} + {{ file_upload(file_upload_form.file, button_text='{} logo'.format('Update' if is_update else 'Upload')) }} {% call form_wrapper() %}
{{textbox(letter_branding_details_form.name)}}
diff --git a/app/templates/views/letter-branding/select-letter-branding.html b/app/templates/views/letter-branding/select-letter-branding.html index fe845c260..b915c3b73 100644 --- a/app/templates/views/letter-branding/select-letter-branding.html +++ b/app/templates/views/letter-branding/select-letter-branding.html @@ -20,7 +20,9 @@ {% for brand in letter_brandings %}
- {{ brand.name }} + + {{ brand.name }} +

{% if brand.domain %} diff --git a/tests/app/main/views/test_letter_branding.py b/tests/app/main/views/test_letter_branding.py index e73070f3a..2520d16b0 100644 --- a/tests/app/main/views/test_letter_branding.py +++ b/tests/app/main/views/test_letter_branding.py @@ -1,35 +1,35 @@ from io import BytesIO -from unittest.mock import Mock +from unittest.mock import Mock, call +from uuid import UUID import pytest +from botocore.exceptions import ClientError as BotoClientError from bs4 import BeautifulSoup from flask import current_app, url_for from notifications_python_client.errors import HTTPError from app.main.views.letter_branding import get_png_file_from_svg -from app.s3_client.s3_logo_client import LETTER_TEMP_LOGO_LOCATION +from app.s3_client.s3_logo_client import ( + LETTER_TEMP_LOGO_LOCATION, + permanent_letter_logo_name, +) from tests.conftest import normalize_spaces def test_letter_branding_page_shows_full_branding_list( - mocker, logged_in_platform_admin_client, + mock_get_all_letter_branding ): - letter_brandings = [ - {'id': '1', 'filename': '1-test', 'domain': None, 'name': 'test brand'}, - {'id': '2', 'filename': '2-example', 'domain': 'cabinet-office.gov.uk', 'name': 'example brand'} - ] - mocker.patch('app.main.views.letter_branding.letter_branding_client.get_all_letter_branding', - return_value=letter_brandings) - response = logged_in_platform_admin_client.get( url_for('.letter_branding') ) assert response.status_code == 200 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - brand_names = [normalize_spaces(name.text) for name in page.select('.letter-brand .message-name')] - brand_hints = [normalize_spaces(hint.text) for hint in page.select('.letter-brand .message-type')] + links = page.select('.message-name a') + brand_names = [normalize_spaces(link.text) for link in links] + hrefs = [link['href'] for link in links] + brand_hints = [normalize_spaces(hint.text) for hint in page.select('.message-type')] assert normalize_spaces( page.select_one('h1').text @@ -40,8 +40,323 @@ def test_letter_branding_page_shows_full_branding_list( assert list(zip( brand_names, brand_hints )) == [ - ('test brand', '–'), - ('example brand', 'Default for cabinet-office.gov.uk'), + ('HM Government', '–'), + ('Land Registry', 'Default for landregistry.gov.uk'), + ('Animal and Plant Health Agency', '–'), + ] + + assert hrefs == [ + url_for('.update_letter_branding', branding_id=str(UUID(int=0))), + url_for('.update_letter_branding', branding_id=str(UUID(int=1))), + url_for('.update_letter_branding', branding_id=str(UUID(int=2))), + ] + + +def test_update_letter_branding_shows_the_current_letter_brand( + logged_in_platform_admin_client, + mock_get_letter_branding_by_id, +): + response = logged_in_platform_admin_client.get( + url_for('.update_letter_branding', branding_id='abc') + ) + + assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + + assert page.find('h1').text == 'Update letter branding' + assert page.select_one('#logo-img > img')['src'].endswith('/hm-government.svg') + assert page.select_one('#name').attrs.get('value') == 'HM Government' + assert page.select_one('#domain').attrs.get('value') == 'cabinet-office.gov.uk' + + +def test_update_letter_branding_with_new_valid_file( + mocker, + logged_in_platform_admin_client, + mock_get_letter_branding_by_id, + fake_uuid +): + with logged_in_platform_admin_client.session_transaction() as session: + user_id = session["user_id"] + + filename = 'new_file.svg' + expected_temp_filename = LETTER_TEMP_LOGO_LOCATION.format(user_id=user_id, unique_id=fake_uuid, filename=filename) + + mock_s3_upload = mocker.patch('app.s3_client.s3_logo_client.utils_s3upload') + mocker.patch('app.s3_client.s3_logo_client.uuid.uuid4', return_value=fake_uuid) + mock_delete_temp_files = mocker.patch('app.main.views.letter_branding.delete_letter_temp_file') + + response = logged_in_platform_admin_client.post( + url_for('.update_letter_branding', branding_id='abc'), + data={'file': (BytesIO(''.encode('utf-8')), filename)}, + follow_redirects=True + ) + assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + + assert page.select_one('#logo-img > img')['src'].endswith(expected_temp_filename) + assert page.select_one('#name').attrs.get('value') == 'HM Government' + assert page.select_one('#domain').attrs.get('value') == 'cabinet-office.gov.uk' + + assert mock_s3_upload.called + mock_delete_temp_files.assert_not_called() + + +def test_update_letter_branding_when_uploading_invalid_file( + logged_in_platform_admin_client, + mock_get_letter_branding_by_id +): + response = logged_in_platform_admin_client.post( + url_for('.update_letter_branding', branding_id='abc'), + data={'file': (BytesIO(''.encode('utf-8')), 'test.png')}, + follow_redirects=True + ) + + assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + + assert page.find('h1').text == 'Update letter branding' + assert page.select_one('.error-message').text.strip() == 'SVG Images only!' + + +def test_update_letter_branding_deletes_any_temp_files_when_uploading_a_file( + mocker, + logged_in_platform_admin_client, + mock_get_letter_branding_by_id, + fake_uuid, +): + with logged_in_platform_admin_client.session_transaction() as session: + user_id = session["user_id"] + + temp_logo = LETTER_TEMP_LOGO_LOCATION.format(user_id=user_id, unique_id=fake_uuid, filename='temp.svg') + + mock_s3_upload = mocker.patch('app.s3_client.s3_logo_client.utils_s3upload') + mock_delete_temp_files = mocker.patch('app.main.views.letter_branding.delete_letter_temp_file') + + response = logged_in_platform_admin_client.post( + url_for('.update_letter_branding', branding_id='abc', logo=temp_logo), + data={'file': (BytesIO(''.encode('utf-8')), 'new_uploaded_file.svg')}, + follow_redirects=True + ) + + assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + + assert mock_s3_upload.called + assert mock_delete_temp_files.called + assert page.find('h1').text == 'Update letter branding' + + +def test_update_letter_branding_with_original_file_and_new_details( + mocker, + logged_in_platform_admin_client, + mock_get_all_letter_branding, + mock_get_letter_branding_by_id, + fake_uuid +): + mock_client_update = mocker.patch('app.main.views.letter_branding.letter_branding_client.update_letter_branding') + mock_template_preview = mocker.patch('app.main.views.letter_branding.get_png_file_from_svg') + mock_upload_logos = mocker.patch('app.main.views.letter_branding.upload_letter_logos') + + response = logged_in_platform_admin_client.post( + url_for('.update_letter_branding', branding_id=fake_uuid), + data={ + 'name': 'Updated name', + 'domain': 'bl.uk', + 'operation': 'branding-details' + }, + follow_redirects=True + ) + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert response.status_code == 200 + assert page.find('h1').text == 'Letter branding' + + mock_upload_logos.assert_not_called() + mock_template_preview.assert_not_called() + + mock_client_update.assert_called_once_with( + branding_id=fake_uuid, + domain='bl.uk', + filename='hm-government', + name='Updated name' + ) + + +def test_update_letter_branding_does_not_require_a_domain( + mocker, + logged_in_platform_admin_client, + mock_get_all_letter_branding, + mock_get_letter_branding_by_id, + fake_uuid +): + mock_client_update = mocker.patch('app.main.views.letter_branding.letter_branding_client.update_letter_branding') + logo = permanent_letter_logo_name('hm-government', 'svg') + + response = logged_in_platform_admin_client.post( + url_for('.update_letter_branding', branding_id=fake_uuid, logo=logo), + data={ + 'name': 'Updated name', + 'domain': '', + 'operation': 'branding-details' + }, + follow_redirects=True + ) + assert response.status_code == 200 + + mock_client_update.assert_called_once_with( + branding_id=fake_uuid, + domain=None, + filename='hm-government', + name='Updated name' + ) + + +def test_update_letter_branding_shows_form_errors_on_name_and_domain_fields( + mocker, + logged_in_platform_admin_client, + mock_get_letter_branding_by_id, + fake_uuid +): + mocker.patch('app.main.views.letter_branding.letter_branding_client.update_letter_branding') + + logo = permanent_letter_logo_name('hm-government', 'svg') + + response = logged_in_platform_admin_client.post( + url_for('.update_letter_branding', branding_id=fake_uuid, logo=logo), + data={ + 'name': '', + 'domain': 'example.com', + 'operation': 'branding-details' + }, + follow_redirects=True + ) + + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + error_messages = page.find_all('span', class_='error-message') + + assert page.find('h1').text == 'Update letter branding' + assert len(error_messages) == 2 + assert error_messages[0].text.strip() == 'This field is required.' + assert error_messages[1].text.strip() == 'Not a known government domain (you might need to update domains.yml)' + + +@pytest.mark.parametrize('error_field', ['name', 'domain']) +def test_update_letter_branding_shows_database_errors_on_name_and_domain_fields( + mocker, + logged_in_platform_admin_client, + mock_get_letter_branding_by_id, + fake_uuid, + error_field +): + mocker.patch('app.main.views.letter_branding.get_png_file_from_svg') + mocker.patch('app.main.views.letter_branding.letter_branding_client.update_letter_branding', side_effect=HTTPError( + response=Mock( + status_code=400, + json={ + 'result': 'error', + 'message': { + error_field: { + '{} already in use'.format(error_field) + } + } + } + ), + message={error_field: ['{} already in use'.format(error_field)]} + )) + + response = logged_in_platform_admin_client.post( + url_for('.update_letter_branding', branding_id='abc'), + data={ + 'name': 'my brand', + 'domain': None, + 'operation': 'branding-details' + } + ) + + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + error_message = page.find('span', class_='error-message').text.strip() + + assert page.find('h1').text == 'Update letter branding' + assert error_message == '{} already in use'.format(error_field) + + +def test_update_letter_branding_with_new_file_and_new_details( + mocker, + logged_in_platform_admin_client, + mock_get_all_letter_branding, + mock_get_letter_branding_by_id, + fake_uuid +): + temp_logo = LETTER_TEMP_LOGO_LOCATION.format(user_id=fake_uuid, unique_id=fake_uuid, filename='new_file.svg') + + mock_client_update = mocker.patch('app.main.views.letter_branding.letter_branding_client.update_letter_branding') + mock_template_preview = mocker.patch( + 'app.main.views.letter_branding.get_png_file_from_svg', + return_value='fake_png') + mock_persist_logo = mocker.patch('app.main.views.letter_branding.persist_logo') + mock_upload_png = mocker.patch('app.main.views.letter_branding.upload_letter_png_logo') + mock_delete_temp_files = mocker.patch('app.main.views.letter_branding.delete_letter_temp_files_created_by') + + response = logged_in_platform_admin_client.post( + url_for('.update_letter_branding', branding_id=fake_uuid, logo=temp_logo), + data={ + 'name': 'Updated name', + 'domain': 'bl.uk', + 'operation': 'branding-details' + }, + follow_redirects=True + ) + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert response.status_code == 200 + assert page.find('h1').text == 'Letter branding' + + assert mock_template_preview.called + mock_client_update.assert_called_once_with( + branding_id=fake_uuid, + domain='bl.uk', + filename='{}-new_file'.format(fake_uuid), + name='Updated name' + ) + mock_persist_logo.assert_called_once_with( + temp_logo, + 'letters/static/images/letter-template/{}-new_file.svg'.format(fake_uuid) + ) + mock_upload_png.assert_called_once_with( + 'letters/static/images/letter-template/{}-new_file.png'.format(fake_uuid), + 'fake_png', + current_app.config['AWS_REGION'] + ) + mock_delete_temp_files.assert_called_once_with(fake_uuid) + + +def test_update_letter_branding_rolls_back_db_changes_and_shows_error_if_saving_to_s3_fails( + mocker, + logged_in_platform_admin_client, + mock_get_letter_branding_by_id, + fake_uuid +): + mocker.patch('app.main.views.letter_branding.get_png_file_from_svg',) + mock_client_update = mocker.patch('app.main.views.letter_branding.letter_branding_client.update_letter_branding') + mocker.patch('app.main.views.letter_branding.upload_letter_logos', side_effect=BotoClientError({}, 'error')) + + temp_logo = LETTER_TEMP_LOGO_LOCATION.format(user_id=fake_uuid, unique_id=fake_uuid, filename='new_file.svg') + response = logged_in_platform_admin_client.post( + url_for('.update_letter_branding', branding_id=fake_uuid, logo=temp_logo), + data={ + 'name': 'Updated name', + 'domain': 'bl.uk', + 'operation': 'branding-details' + }, + follow_redirects=True + ) + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert response.status_code == 200 + assert page.find('h1').text == 'Update letter branding' + assert page.select_one('.error-message').text.strip() == 'Error saving uploaded file - try uploading again' + + assert mock_client_update.call_count == 2 + assert mock_client_update.call_args_list == [ + call(branding_id=fake_uuid, domain='bl.uk', filename='{}-new_file'.format(fake_uuid), name='Updated name'), + call(branding_id=fake_uuid, domain='cabinet-office.gov.uk', filename='hm-government', name='HM Government') ] @@ -169,12 +484,10 @@ def test_create_letter_branding_shows_an_error_when_submitting_details_with_no_l assert page.select_one('.error-message').text.strip() == 'You need to upload a file to submit' -@pytest.mark.parametrize('domain', ['bl.uk', '']) def test_create_letter_branding_persists_logo_when_all_data_is_valid( mocker, logged_in_platform_admin_client, fake_uuid, - domain ): with logged_in_platform_admin_client.session_transaction() as session: user_id = session["user_id"] diff --git a/tests/conftest.py b/tests/conftest.py index a653c3eb8..61decdcd7 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2540,7 +2540,7 @@ def mock_get_all_letter_branding(mocker): 'id': str(UUID(int=1)), 'name': 'Land Registry', 'filename': 'land-registry', - 'domain': None, + 'domain': 'landregistry.gov.uk', }, { 'id': str(UUID(int=2)), @@ -2562,7 +2562,7 @@ def mock_get_letter_branding_by_id(mocker): 'id': _id, 'name': 'HM Government', 'filename': 'hm-government', - 'domain': None, + 'domain': 'cabinet-office.gov.uk', } return mocker.patch( 'app.letter_branding_client.get_letter_branding', side_effect=_get_branding_by_id