From 53627fd1ba2e06192f7ee728a3caf2e4761062c4 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Fri, 8 Feb 2019 14:11:10 +0000 Subject: [PATCH 1/3] Add update letter branding method to letter branding client --- app/notify_client/letter_branding_client.py | 11 ++++++++++ .../test_letter_branding_client.py | 20 +++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/app/notify_client/letter_branding_client.py b/app/notify_client/letter_branding_client.py index 88afe99c6..0a577d55e 100644 --- a/app/notify_client/letter_branding_client.py +++ b/app/notify_client/letter_branding_client.py @@ -20,5 +20,16 @@ class LetterBrandingClient(NotifyAdminAPIClient): } return self.post(url="/letter-branding", data=data) + @cache.delete('letter_branding') + @cache.delete('letter_branding-{branding_id}') + def update_letter_branding(self, branding_id, filename, name, domain): + data = { + "filename": filename, + "name": name, + "domain": domain, + + } + return self.post(url="/letter-branding/{}".format(branding_id), data=data) + letter_branding_client = LetterBrandingClient() diff --git a/tests/app/notify_client/test_letter_branding_client.py b/tests/app/notify_client/test_letter_branding_client.py index 454c1f3dc..232d55fa8 100644 --- a/tests/app/notify_client/test_letter_branding_client.py +++ b/tests/app/notify_client/test_letter_branding_client.py @@ -1,3 +1,5 @@ +from unittest.mock import call + from app.notify_client.letter_branding_client import LetterBrandingClient @@ -51,3 +53,21 @@ def test_create_letter_branding(mocker): ) mock_redis_delete.assert_called_once_with('letter_branding') + + +def test_update_letter_branding(mocker, fake_uuid): + branding = {'filename': 'uuid-test', 'name': 'my letters', 'domain': 'example.com'} + + mock_post = mocker.patch('app.notify_client.letter_branding_client.LetterBrandingClient.post') + mock_redis_delete = mocker.patch('app.notify_client.RedisClient.delete') + LetterBrandingClient().update_letter_branding( + branding_id=fake_uuid, filename=branding['filename'], name=branding['name'], domain=branding['domain']) + + mock_post.assert_called_once_with( + url='/letter-branding/{}'.format(fake_uuid), + data=branding + ) + assert mock_redis_delete.call_args_list == [ + call('letter_branding'), + call('letter_branding-{}'.format(fake_uuid)), + ] From 1cb1ce310ab82a549fbd7dfecc86eb362aed74ee Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Mon, 11 Feb 2019 15:31:12 +0000 Subject: [PATCH 2/3] Add update_letter_branding view function This works in a similar way to the create_letter_branding view function, but with a few minor differences: * Since we already have a file, uploading a file is no longer required (since we can just use the current file if a new one is not uploaded) * We save the changes in the database, then upload the new files to S3. If saving to S3 raises an error, we now rollback the database changes to prevent any errors when trying to view letters with the original logo. --- app/main/views/letter_branding.py | 87 +++++ app/navigation.py | 4 + .../manage-letter-branding.html | 6 +- .../select-letter-branding.html | 4 +- tests/app/main/views/test_letter_branding.py | 345 +++++++++++++++++- tests/conftest.py | 4 +- 6 files changed, 428 insertions(+), 22 deletions(-) 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 From 31a1c1ca518327363950b61d9b99d76a44539577 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Tue, 12 Feb 2019 14:02:21 +0000 Subject: [PATCH 3/3] Pass service domain to api when adding a new service We need to pass the domain to api when adding a service so that api can link the domain of the service with a letter brand. --- app/main/views/add_service.py | 8 ++++---- app/notify_client/service_api_client.py | 4 +++- tests/app/main/views/test_add_service.py | 15 +++++++++------ .../app/notify_client/test_service_api_client.py | 2 ++ tests/app/notify_client/test_user_client.py | 2 +- tests/conftest.py | 2 ++ 6 files changed, 21 insertions(+), 12 deletions(-) diff --git a/app/main/views/add_service.py b/app/main/views/add_service.py index 1fed9c5ad..8e6d942f1 100644 --- a/app/main/views/add_service.py +++ b/app/main/views/add_service.py @@ -10,10 +10,9 @@ from app.utils import AgreementInfo, email_safe, user_is_gov_user def _create_service(service_name, organisation_type, email_from, form): free_sms_fragment_limit = current_app.config['DEFAULT_FREE_SMS_FRAGMENT_LIMITS'].get(organisation_type) - email_branding = email_branding_client.get_email_branding_id_for_domain( - 'nhs.uk' if organisation_type == 'nhs' else - AgreementInfo.from_current_user().canonical_domain - ) + + domain = 'nhs.uk' if organisation_type == 'nhs' else AgreementInfo.from_current_user().canonical_domain + email_branding = email_branding_client.get_email_branding_id_for_domain(domain) try: service_id = service_api_client.create_service( service_name=service_name, @@ -22,6 +21,7 @@ def _create_service(service_name, organisation_type, email_from, form): restricted=True, user_id=session['user_id'], email_from=email_from, + service_domain=domain ) session['service_id'] = service_id diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index 7bce1b2d3..8dfd24b97 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -13,6 +13,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): restricted, user_id, email_from, + service_domain, ): """ Create a service and return the json. @@ -24,7 +25,8 @@ class ServiceAPIClient(NotifyAdminAPIClient): "message_limit": message_limit, "user_id": user_id, "restricted": restricted, - "email_from": email_from + "email_from": email_from, + "service_domain": service_domain } data = _attach_current_user(data) return self.post("/service", data)['data']['id'] diff --git a/tests/app/main/views/test_add_service.py b/tests/app/main/views/test_add_service.py index 56002884c..708aeb388 100644 --- a/tests/app/main/views/test_add_service.py +++ b/tests/app/main/views/test_add_service.py @@ -49,7 +49,8 @@ def test_should_add_service_and_redirect_to_tour_when_no_services( message_limit=app_.config['DEFAULT_SERVICE_LIMIT'], restricted=True, user_id=api_user_active.id, - email_from='testing.the.post' + email_from='testing.the.post', + service_domain=None ) mock_create_service_template.assert_called_once_with( 'Example text message template', @@ -71,10 +72,10 @@ def test_should_add_service_and_redirect_to_tour_when_no_services( mock_create_or_update_free_sms_fragment_limit.assert_called_once_with(101, 25000) -@pytest.mark.parametrize('organisation_type, free_allowance', [ - ('central', 250 * 1000), - ('local', 25 * 1000), - ('nhs', 25 * 1000), +@pytest.mark.parametrize('organisation_type, free_allowance, service_domain', [ + ('central', 250 * 1000, None), + ('local', 25 * 1000, None), + ('nhs', 25 * 1000, 'nhs.uk'), ]) def test_should_add_service_and_redirect_to_dashboard_when_existing_service( app_, @@ -86,6 +87,7 @@ def test_should_add_service_and_redirect_to_dashboard_when_existing_service( api_user_active, organisation_type, free_allowance, + service_domain, mock_create_or_update_free_sms_fragment_limit, mock_get_all_email_branding, ): @@ -103,7 +105,8 @@ def test_should_add_service_and_redirect_to_dashboard_when_existing_service( message_limit=app_.config['DEFAULT_SERVICE_LIMIT'], restricted=True, user_id=api_user_active.id, - email_from='testing.the.post' + email_from='testing.the.post', + service_domain=service_domain ) mock_create_or_update_free_sms_fragment_limit.assert_called_once_with(101, free_allowance) assert len(mock_create_service_template.call_args_list) == 0 diff --git a/tests/app/notify_client/test_service_api_client.py b/tests/app/notify_client/test_service_api_client.py index b5dd793a2..f809259db 100644 --- a/tests/app/notify_client/test_service_api_client.py +++ b/tests/app/notify_client/test_service_api_client.py @@ -74,6 +74,7 @@ def test_client_creates_service_with_correct_data( True, fake_uuid, 'test@example.com', + 'nhs.uk' ) mock_post.assert_called_once_with( '/service', @@ -89,6 +90,7 @@ def test_client_creates_service_with_correct_data( restricted=True, user_id=fake_uuid, email_from='test@example.com', + service_domain='nhs.uk' ), ) diff --git a/tests/app/notify_client/test_user_client.py b/tests/app/notify_client/test_user_client.py index 6eebb9cd2..c3b93cd2b 100644 --- a/tests/app/notify_client/test_user_client.py +++ b/tests/app/notify_client/test_user_client.py @@ -249,7 +249,7 @@ def test_returns_value_from_cache( (user_api_client, 'set_user_permissions', [user_id, SERVICE_ONE_ID, []], {}), (user_api_client, 'activate_user', [api_user_pending(sample_uuid())], {}), (service_api_client, 'remove_user_from_service', [SERVICE_ONE_ID, user_id], {}), - (service_api_client, 'create_service', ['', '', 0, False, user_id, sample_uuid()], {}), + (service_api_client, 'create_service', ['', '', 0, False, user_id, sample_uuid(), ''], {}), (invite_api_client, 'accept_invite', [SERVICE_ONE_ID, user_id], {}), ]) def test_deletes_user_cache( diff --git a/tests/conftest.py b/tests/conftest.py index 61decdcd7..f96cba090 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -625,6 +625,7 @@ def mock_create_service(mocker): restricted, user_id, email_from, + service_domain, ): service = service_json( 101, service_name, [user_id], message_limit=message_limit, restricted=restricted, email_from=email_from) @@ -643,6 +644,7 @@ def mock_create_duplicate_service(mocker): restricted, user_id, email_from, + service_domain, ): json_mock = Mock(return_value={'message': {'name': ["Duplicate service name '{}'".format(service_name)]}}) resp_mock = Mock(status_code=400, json=json_mock)