From 0e20ca44a3fdac4060a5b217d70ed9ca8d29a2a2 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 1 Feb 2019 16:31:34 +0000 Subject: [PATCH 1/8] rename branding to email_branding to help transition to having letter branding as well --- app/main/forms.py | 10 +++++----- app/main/views/service_settings.py | 8 ++++---- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 4cc423401..4c0c784f5 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -782,7 +782,7 @@ class RadioFieldWithRequiredMessage(RadioField): raise ValueError(self.required_message) -class ServiceSetBranding(StripWhitespaceForm): +class ServiceSetEmailBranding(StripWhitespaceForm): branding_style = RadioFieldWithNoneOption( 'Branding style', @@ -793,14 +793,14 @@ class ServiceSetBranding(StripWhitespaceForm): DEFAULT = (FieldWithNoneOption.NONE_OPTION_VALUE, 'GOV.UK') - def __init__(self, all_email_brandings, current_email_branding): + def __init__(self, all_branding_options, current_branding): - super().__init__(branding_style=current_email_branding) + super().__init__(branding_style=current_branding) self.branding_style.choices = sorted( - all_email_brandings + [self.DEFAULT], + all_branding_options + [self.DEFAULT], key=lambda branding: ( - branding[0] != current_email_branding, + branding[0] != current_branding, branding[0] is not self.DEFAULT[0], branding[1].lower(), ), diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index ed8c5a5a4..4f5371417 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -45,7 +45,7 @@ from app.main.forms import ( ServiceLetterContactBlockForm, ServicePreviewBranding, ServiceReplyToEmailForm, - ServiceSetBranding, + ServiceSetEmailBranding, ServiceSmsSenderForm, ServiceSwitchChannelForm, SMSPrefixForm, @@ -759,9 +759,9 @@ def set_free_sms_allowance(service_id): def service_set_email_branding(service_id): email_branding = email_branding_client.get_all_email_branding() - form = ServiceSetBranding( - all_email_brandings=get_branding_as_value_and_label(email_branding), - current_email_branding=current_service.email_branding_id, + form = ServiceSetEmailBranding( + all_branding_options=get_branding_as_value_and_label(email_branding), + current_branding=current_service.email_branding_id, ) if form.validate_on_submit(): From a1caf77b0ed7b08e41c560dce2a77456ebfb3766 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 1 Feb 2019 16:34:54 +0000 Subject: [PATCH 2/8] use new letter branding instead of dvla organisation id new code is copied stylistically from the email branding patterns. Instead of `service.dvla_organisation`, there's now `service.letter_branding` and `service.letter_branding_id`. However, unlike email branding we're not currently showing a preview of the logo. That can come later when we work out how we want to do it. --- app/main/forms.py | 5 +++++ app/main/views/service_settings.py | 13 +++++++----- app/models/service.py | 11 +++++++--- app/notify_client/letter_branding_client.py | 20 ++++++++++++++++--- app/notify_client/service_api_client.py | 1 + app/templates/views/service-settings.html | 4 ++-- .../service-settings/set-letter-branding.html | 14 +++++++------ 7 files changed, 49 insertions(+), 19 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 4c0c784f5..759f522a3 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -807,6 +807,11 @@ class ServiceSetEmailBranding(StripWhitespaceForm): ) +class ServiceSetLetterBranding(ServiceSetEmailBranding): + # form is the same, but instead of GOV.UK we have None as a valid option + DEFAULT = (FieldWithNoneOption.NONE_OPTION_VALUE, 'None') + + class ServicePreviewBranding(StripWhitespaceForm): branding_style = HiddenFieldWithNoneOption('branding_style') diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 4f5371417..cb6e759f7 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -46,6 +46,7 @@ from app.main.forms import ( ServicePreviewBranding, ServiceReplyToEmailForm, ServiceSetEmailBranding, + ServiceSetLetterBranding, ServiceSmsSenderForm, ServiceSwitchChannelForm, SMSPrefixForm, @@ -805,16 +806,17 @@ def service_preview_email_branding(service_id): @main.route("/services//service-settings/set-letter-branding", methods=['GET', 'POST']) @login_required @user_is_platform_admin -def set_letter_branding(service_id): +def service_set_letter_branding(service_id): + letter_branding = letter_branding_client.get_all_letter_branding() - form = LetterBranding( - choices=letter_branding_client.get_letter_branding().items(), - dvla_org_id=current_service.dvla_organisation, + form = ServiceSetLetterBranding( + all_branding_options=get_branding_as_value_and_label(letter_branding), + current_branding=current_service.letter_branding_id, ) if form.validate_on_submit(): current_service.update( - dvla_organisation=form.dvla_org_id.data + letter_branding=form.branding_style.data ) return redirect(url_for('.service_settings', service_id=service_id)) @@ -822,6 +824,7 @@ def set_letter_branding(service_id): 'views/service-settings/set-letter-branding.html', form=form, search_form=SearchTemplatesForm(), + show_search_box=(len(letter_branding) > 6) ) diff --git a/app/models/service.py b/app/models/service.py index 8bfe6d52a..036af98e1 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -28,6 +28,7 @@ class Service(): 'email_from', 'id', 'inbound_api', + 'letter_branding', 'letter_contact_block', 'letter_logo_filename', 'message_limit', @@ -303,11 +304,15 @@ class Service(): return email_branding_client.get_email_branding(self.email_branding_id)['email_branding'] return None + @property + def letter_branding_id(self): + return self._dict['letter_branding'] + @cached_property def letter_branding(self): - return letter_branding_client.get_letter_branding().get( - self.dvla_organisation, '001' - ) + if self.letter_branding_id: + return letter_branding_client.get_letter_branding(self.letter_branding_id) + return None @cached_property def organisation_name(self): diff --git a/app/notify_client/letter_branding_client.py b/app/notify_client/letter_branding_client.py index 84ab0b80f..9e6e90f21 100644 --- a/app/notify_client/letter_branding_client.py +++ b/app/notify_client/letter_branding_client.py @@ -1,10 +1,24 @@ -from app.notify_client import NotifyAdminAPIClient +from app.notify_client import NotifyAdminAPIClient, cache class LetterBrandingClient(NotifyAdminAPIClient): - def get_letter_branding(self): - return self.get(url='/dvla_organisations') + @cache.set('letter_branding-{branding_id}') + def get_letter_branding(self, branding_id): + return self.get(url='/letter-branding/{}'.format(branding_id)) + + @cache.set('letter_branding') + def get_all_letter_branding(self, sort_key=None): + brandings = self.get(url='/letter-branding') + if sort_key and sort_key in brandings[0]: + brandings.sort(key=lambda branding: branding[sort_key].lower()) + return brandings + + def get_letter_branding_id_for_domain(self, domain): + for branding in self.get_all_letter_branding(): + if domain and branding.get('domain') == domain: + return branding['id'] + return None def create_letter_branding(self, filename, name, domain): data = { diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index 75e3bdb04..c8bf7cf31 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -75,6 +75,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): 'sms_sender', 'created_by', 'branding', + 'letter_branding', 'email_branding', 'letter_contact_block', 'dvla_organisation', diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index eec63aba5..93c3e7c24 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -309,8 +309,8 @@ {% endcall %} {% call row() %} {{ text_field('Letter branding')}} - {{ text_field(current_service.letter_branding) }} - {{ edit_field('Change', url_for('.set_letter_branding', service_id=current_service.id)) }} + {{ text_field(current_service.letter_branding.name) }} + {{ edit_field('Change', url_for('.service_set_letter_branding', service_id=current_service.id)) }} {% endcall %} {% call row() %} {{ text_field('Data retention')}} diff --git a/app/templates/views/service-settings/set-letter-branding.html b/app/templates/views/service-settings/set-letter-branding.html index 93f1fca71..d9aad44c1 100644 --- a/app/templates/views/service-settings/set-letter-branding.html +++ b/app/templates/views/service-settings/set-letter-branding.html @@ -13,12 +13,14 @@

Set letter branding

{% call form_wrapper() %} {{ live_search(target_selector='.multiple-choice', show=True, form=search_form, label='Search by name') }} - {{ radios(form.dvla_org_id, hide_legend=True) }} - {{ page_footer( - 'Save', - back_link=url_for('.service_settings', service_id=current_service.id), - back_link_text='Back to settings' - ) }} + {{ radios(form.branding_style, hide_legend=True) }} +
+ {{ page_footer( + 'Save', + back_link=url_for('.service_settings', service_id=current_service.id), + back_link_text='Back to settings' + ) }} +
{% endcall %} {% endblock %} From 8266635a7a2b486f2616ef86d32502904b26ba85 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 1 Feb 2019 17:24:04 +0000 Subject: [PATCH 3/8] remove dvla_organisation and other unused things dvla_organisation, branding_dict, a weird `branding` property on update_service. all gone :boom: --- app/main/forms.py | 21 --------------------- app/main/views/service_settings.py | 12 ------------ app/models/service.py | 1 - app/notify_client/letter_branding_client.py | 7 +------ app/notify_client/service_api_client.py | 2 -- 5 files changed, 1 insertion(+), 42 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 759f522a3..6e8ab42f4 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -886,27 +886,6 @@ class CreateOrUpdateOrganisation(StripWhitespaceForm): name = StringField('Name', validators=[DataRequired()]) -class LetterBranding(StripWhitespaceForm): - - def __init__(self, choices=[], *args, **kwargs): - super().__init__(*args, **kwargs) - self.dvla_org_id.choices = list(sorted( - choices, - key=lambda choice: ( - choice[0] != kwargs.get('dvla_org_id'), - choice[0] != '001', - choice[1], - ), - )) - - dvla_org_id = RadioField( - 'Which logo should this service’s letter have?', - validators=[ - DataRequired() - ] - ) - - class EmailFieldInWhitelist(EmailField, StripWhitespaceStringField): pass diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index cb6e759f7..22b950755 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -31,7 +31,6 @@ from app.main.forms import ( ConfirmPasswordForm, FreeSMSAllowance, InternationalSMSForm, - LetterBranding, LinkOrganisationsForm, OrganisationTypeForm, RenameServiceForm, @@ -55,7 +54,6 @@ from app.main.forms import ( from app.utils import ( AgreementInfo, email_safe, - get_logo_cdn_domain, user_has_permissions, user_is_gov_user, user_is_platform_admin, @@ -775,7 +773,6 @@ def service_set_email_branding(service_id): return render_template( 'views/service-settings/set-email-branding.html', form=form, - branding_dict=get_branding_as_dict(email_branding), search_form=SearchTemplatesForm(), show_search_box=(len(email_branding) > 6) ) @@ -961,15 +958,6 @@ def get_branding_as_value_and_label(email_branding): ] -def get_branding_as_dict(email_branding): - return { - branding['id']: { - 'logo': 'https://{}/{}'.format(get_logo_cdn_domain(), branding['logo']), - 'colour': branding['colour'] - } for branding in email_branding - } - - def convert_dictionary_to_wtforms_choices_format(dictionary, value, label): return [ (item[value], item[label]) for item in dictionary diff --git a/app/models/service.py b/app/models/service.py index 036af98e1..8ecd722a2 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -23,7 +23,6 @@ class Service(): ALLOWED_PROPERTIES = { 'active', 'contact_link', - 'dvla_organisation', 'email_branding', 'email_from', 'id', diff --git a/app/notify_client/letter_branding_client.py b/app/notify_client/letter_branding_client.py index 9e6e90f21..2643e2f1e 100644 --- a/app/notify_client/letter_branding_client.py +++ b/app/notify_client/letter_branding_client.py @@ -14,12 +14,7 @@ class LetterBrandingClient(NotifyAdminAPIClient): brandings.sort(key=lambda branding: branding[sort_key].lower()) return brandings - def get_letter_branding_id_for_domain(self, domain): - for branding in self.get_all_letter_branding(): - if domain and branding.get('domain') == domain: - return branding['id'] - return None - + @cache.delete('letter_branding') def create_letter_branding(self, filename, name, domain): data = { "filename": filename, diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index c8bf7cf31..7bce1b2d3 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -74,11 +74,9 @@ class ServiceAPIClient(NotifyAdminAPIClient): 'research_mode', 'sms_sender', 'created_by', - 'branding', 'letter_branding', 'email_branding', 'letter_contact_block', - 'dvla_organisation', 'permissions', 'organisation_type', 'free_sms_fragment_limit', From 3be82bafd57c65b2db008797a82da475e0e61101 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 5 Feb 2019 16:33:36 +0000 Subject: [PATCH 4/8] fix request letter branding page (it can now have a null type). also fix nav.py --- app/navigation.py | 8 ++++---- .../views/service-settings/request-letter-branding.html | 6 +++++- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/app/navigation.py b/app/navigation.py index f784360fe..13355a3a3 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -248,7 +248,7 @@ class HeaderNavigation(Navigation): 'service_switch_research_mode', 'services_or_dashboard', 'set_free_sms_allowance', - 'set_letter_branding', + 'service_set_letter_branding', 'set_organisation_type', 'set_sender', 'set_template_sender', @@ -364,7 +364,7 @@ class MainNavigation(Navigation): 'service_settings', 'service_sms_senders', 'set_free_sms_allowance', - 'set_letter_branding', + 'service_set_letter_branding', 'set_organisation_type', 'submit_request_to_go_live', }, @@ -703,7 +703,7 @@ class CaseworkNavigation(Navigation): 'service_switch_research_mode', 'services_or_dashboard', 'set_free_sms_allowance', - 'set_letter_branding', + 'service_set_letter_branding', 'set_organisation_type', 'set_sender', 'set_template_sender', @@ -935,7 +935,7 @@ class OrgNavigation(Navigation): 'service_switch_research_mode', 'services_or_dashboard', 'set_free_sms_allowance', - 'set_letter_branding', + 'service_set_letter_branding', 'set_organisation_type', 'set_sender', 'set_template_sender', diff --git a/app/templates/views/service-settings/request-letter-branding.html b/app/templates/views/service-settings/request-letter-branding.html index 6a390bd3f..85c645fe9 100644 --- a/app/templates/views/service-settings/request-letter-branding.html +++ b/app/templates/views/service-settings/request-letter-branding.html @@ -12,7 +12,11 @@

- Your letters have the {{ current_service.letter_branding }} logo. + {% if current_service.letter_branding_id %} + Your letters have the {{ current_service.letter_branding.name }} logo. + {% else %} + Your letters have no logo. + {% endif %}

Contact support From 5405c2e1be9576f36c98a9609d2a4e28b1fac9e6 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 5 Feb 2019 16:34:05 +0000 Subject: [PATCH 5/8] fix service settings letter branding tests some tests are now expanded to handle the fact that letter branding can be null --- tests/__init__.py | 2 +- .../test_service_setting_permissions.py | 4 +- tests/app/main/views/test_service_settings.py | 119 +++++++++++------- .../test_letter_branding_client.py | 37 +++++- tests/conftest.py | 44 +++++-- 5 files changed, 144 insertions(+), 62 deletions(-) diff --git a/tests/__init__.py b/tests/__init__.py index 458873f55..da3bf8c04 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -167,8 +167,8 @@ def service_json( 'email_branding': email_branding, 'branding': branding, 'created_at': created_at or str(datetime.utcnow()), + 'letter_branding': None, 'letter_contact_block': letter_contact_block, - 'dvla_organisation': '001', 'permissions': permissions, 'inbound_api': inbound_api, 'service_callback_api': service_callback_api, diff --git a/tests/app/main/views/service_settings/test_service_setting_permissions.py b/tests/app/main/views/service_settings/test_service_setting_permissions.py index c6a8e959d..76a540ca1 100644 --- a/tests/app/main/views/service_settings/test_service_setting_permissions.py +++ b/tests/app/main/views/service_settings/test_service_setting_permissions.py @@ -10,7 +10,7 @@ def get_service_settings_page( platform_admin_user, service_one, mock_get_inbound_number_for_service, - mock_get_letter_branding, + mock_get_all_letter_branding, mock_get_service_organisation, mock_get_free_sms_fragment_limit, no_reply_to_email_addresses, @@ -94,7 +94,7 @@ def test_normal_user_doesnt_see_any_toggle_buttons( no_letter_contact_blocks, mock_get_service_organisation, single_sms_sender, - mock_get_letter_branding, + mock_get_all_letter_branding, mock_get_inbound_number_for_service, mock_get_free_sms_fragment_limit, mock_get_service_data_retention diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 7d2da0a14..683db61d9 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -1,8 +1,7 @@ -import uuid from functools import partial from unittest.mock import ANY, PropertyMock, call from urllib.parse import parse_qs, urlparse -from uuid import uuid4 +from uuid import UUID, uuid4 import pytest from bs4 import BeautifulSoup @@ -38,7 +37,7 @@ from tests.conftest import ( @pytest.fixture def mock_get_service_settings_page_common( - mock_get_letter_branding, + mock_get_all_letter_branding, mock_get_inbound_number_for_service, mock_get_free_sms_fragment_limit, mock_get_service_data_retention, @@ -95,7 +94,7 @@ def mock_get_service_settings_page_common( 'Organisation type Central Change', 'Free text message allowance 250,000 Change', 'Email branding GOV.UK Change', - 'Letter branding HM Government Change', + 'Letter branding Change', 'Data retention email Change' ]), @@ -187,7 +186,7 @@ def test_should_show_overview( 'Label Value Action', 'Send letters On Change', 'Sender addresses 1 Example Street Manage', - 'Letter branding HM Government Change', + 'Letter branding None Change', ]), ]) @@ -1173,7 +1172,7 @@ def test_route_for_platform_admin_update_service( client, platform_admin_user, service_one, - mock_get_letter_branding, + mock_get_all_letter_branding, route, ): mocker.patch('app.service_api_client.archive_service') @@ -2033,7 +2032,7 @@ def test_set_letter_contact_block_redirects_to_template( mock_update_service, ): service_one['permissions'] = ['letter'] - fake_template_id = uuid.uuid4() + fake_template_id = uuid4() response = logged_in_client.post( url_for( 'main.service_set_letter_contact_block', @@ -2069,15 +2068,14 @@ def test_set_letter_contact_block_has_max_10_lines( def test_request_letter_branding( client_request, - mock_get_letter_branding, + mock_get_letter_branding_by_id, + service_one ): request_page = client_request.get( 'main.request_letter_branding', service_id=SERVICE_ONE_ID, ) - assert request_page.select_one('main p').text.strip() == ( - 'Your letters have the HM Government logo.' - ) + assert request_page.select_one('main p').text.strip() == 'Your letters have no logo.' link_href = request_page.select_one('main a')['href'] feedback_page = client_request.get_url(link_href) assert feedback_page.select_one('textarea').text.strip() == ( @@ -2085,70 +2083,97 @@ def test_request_letter_branding( ) -def test_set_letter_branding_platform_admin_only( +def test_request_letter_branding_if_already_have_branding( + client_request, + mock_get_letter_branding_by_id, + service_one, +): + service_one['letter_branding'] = uuid4() + + request_page = client_request.get( + 'main.request_letter_branding', + service_id=SERVICE_ONE_ID, + ) + + mock_get_letter_branding_by_id.assert_called_once_with(service_one['letter_branding']) + assert request_page.select_one('main p').text.strip() == 'Your letters have the HM Government logo.' + + +def test_service_set_letter_branding_platform_admin_only( logged_in_client, service_one, ): - response = logged_in_client.get(url_for('main.set_letter_branding', service_id=service_one['id'])) + response = logged_in_client.get(url_for('main.service_set_letter_branding', service_id=service_one['id'])) assert response.status_code == 403 -@pytest.mark.parametrize('current_dvla_org_id, expected_selected, expected_items', [ - (None, '001', ( - ('001', 'HM Government'), - ('999', 'Animal and Plant Health Agency'), - ('500', 'Land Registry'), +@pytest.mark.parametrize('letter_branding, expected_selected, expected_items', [ + # expected order: currently selected, then default, then rest alphabetically + (None, '__NONE__', ( + ('__NONE__', 'None'), + (str(UUID(int=2)), 'Animal and Plant Health Agency'), + (str(UUID(int=0)), 'HM Government'), + (str(UUID(int=1)), 'Land Registry'), )), - ('500', '500', ( - ('500', 'Land Registry'), - ('001', 'HM Government'), - ('999', 'Animal and Plant Health Agency'), + (str(UUID(int=1)), str(UUID(int=1)), ( + (str(UUID(int=1)), 'Land Registry'), + ('__NONE__', 'None'), + (str(UUID(int=2)), 'Animal and Plant Health Agency'), + (str(UUID(int=0)), 'HM Government'), )), - ('999', '999', ( - ('999', 'Animal and Plant Health Agency'), - ('001', 'HM Government'), - ('500', 'Land Registry'), + (str(UUID(int=2)), str(UUID(int=2)), ( + (str(UUID(int=2)), 'Animal and Plant Health Agency'), + ('__NONE__', 'None'), + (str(UUID(int=0)), 'HM Government'), + (str(UUID(int=1)), 'Land Registry'), )), ]) -def test_set_letter_branding_prepopulates( +def test_service_set_letter_branding_prepopulates( logged_in_platform_admin_client, service_one, - mock_get_letter_branding, - current_dvla_org_id, + mock_get_all_letter_branding, + letter_branding, expected_selected, expected_items, ): - if current_dvla_org_id: - service_one['dvla_organisation'] = current_dvla_org_id - response = logged_in_platform_admin_client.get(url_for('main.set_letter_branding', service_id=service_one['id'])) + service_one['letter_branding'] = letter_branding + response = logged_in_platform_admin_client.get( + url_for('main.service_set_letter_branding', service_id=service_one['id']) + ) assert response.status_code == 200 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - for element in {'label[for^=dvla_org_id]', 'input[type=radio]'}: - assert len(page.select(element)) == len(expected_items) - - for index, expected_item in enumerate(expected_items): - expected_value, expected_label = expected_item - assert normalize_spaces(page.select('label[for^=dvla_org_id]')[index].text) == expected_label - assert page.select('input[type=radio]')[index]['value'] == expected_value - assert len(page.select('input[checked]')) == 1 assert page.select('input[checked]')[0]['value'] == expected_selected + for element in {'label[for^=branding_style]', 'input[type=radio]'}: + assert len(page.select(element)) == len(expected_items) -def test_set_letter_branding_saves( + for index, expected_item in enumerate(expected_items): + expected_value, expected_label = expected_item + assert normalize_spaces(page.select('label[for^=branding_style]')[index].text) == expected_label + assert page.select('input[type=radio]')[index]['value'] == expected_value + + +@pytest.mark.parametrize('selected_letter_branding, expected_post_data', [ + (str(UUID(int=1)), str(UUID(int=1))), + ('__NONE__', None), +]) +def test_service_set_letter_branding_saves( logged_in_platform_admin_client, service_one, mock_update_service, - mock_get_letter_branding, + mock_get_all_letter_branding, + selected_letter_branding, + expected_post_data ): response = logged_in_platform_admin_client.post( - url_for('main.set_letter_branding', service_id=service_one['id']), - data={'dvla_org_id': '500'} + url_for('main.service_set_letter_branding', service_id=service_one['id']), + data={'branding_style': selected_letter_branding} ) assert response.status_code == 302 assert response.location == url_for('main.service_settings', service_id=service_one['id'], _external=True) - mock_update_service.assert_called_once_with(service_one['id'], dvla_organisation='500') + mock_update_service.assert_called_once_with(service_one['id'], letter_branding=expected_post_data) @pytest.mark.parametrize('current_branding, expected_values, expected_labels', [ @@ -3063,7 +3088,7 @@ def test_service_settings_when_inbound_number_is_not_set( mock_get_service_organisation, single_sms_sender, mocker, - mock_get_letter_branding, + mock_get_all_letter_branding, mock_get_free_sms_fragment_limit, mock_get_service_data_retention, ): @@ -3081,7 +3106,7 @@ def test_set_inbound_sms_when_inbound_number_is_not_set( single_reply_to_email_address, single_letter_contact_block, mocker, - mock_get_letter_branding, + mock_get_all_letter_branding, ): mocker.patch('app.inbound_number_client.get_inbound_sms_number_for_service', return_value={'data': {}}) diff --git a/tests/app/notify_client/test_letter_branding_client.py b/tests/app/notify_client/test_letter_branding_client.py index 23f962971..8e29510b6 100644 --- a/tests/app/notify_client/test_letter_branding_client.py +++ b/tests/app/notify_client/test_letter_branding_client.py @@ -1,11 +1,38 @@ from app.notify_client.letter_branding_client import LetterBrandingClient -def test_get_letter_branding(mocker): - mock_get = mocker.patch('app.notify_client.letter_branding_client.LetterBrandingClient.get') - LetterBrandingClient().get_letter_branding() - mock_get.assert_called_once_with( - url='/dvla_organisations' +def test_get_letter_branding(mocker, fake_uuid): + mock_get = mocker.patch( + 'app.notify_client.letter_branding_client.LetterBrandingClient.get', + return_value={'foo': 'bar'} + ) + mock_redis_get = mocker.patch('app.notify_client.RedisClient.get', return_value=None) + mock_redis_set = mocker.patch('app.notify_client.RedisClient.set') + + LetterBrandingClient().get_letter_branding(fake_uuid) + + mock_get.assert_called_once_with(url='/letter-branding/{}'.format(fake_uuid)) + mock_redis_get.assert_called_once_with('letter_branding-{}'.format(fake_uuid)) + mock_redis_set.assert_called_once_with( + 'letter_branding-{}'.format(fake_uuid), + '{"foo": "bar"}', + ex=604800, + ) + + +def test_get_all_letter_branding(mocker): + mock_get = mocker.patch('app.notify_client.letter_branding_client.LetterBrandingClient.get', return_value=[1, 2, 3]) + mock_redis_get = mocker.patch('app.notify_client.RedisClient.get', return_value=None) + mock_redis_set = mocker.patch('app.notify_client.RedisClient.set') + + LetterBrandingClient().get_all_letter_branding() + + mock_get.assert_called_once_with(url='/letter-branding') + mock_redis_get.assert_called_once_with('letter_branding') + mock_redis_set.assert_called_once_with( + 'letter_branding', + '[1, 2, 3]', + ex=604800, ) diff --git a/tests/conftest.py b/tests/conftest.py index 023b4a9c7..b37d4c478 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3,6 +3,7 @@ import os from contextlib import contextmanager from datetime import date, datetime, timedelta from unittest.mock import Mock +from uuid import UUID import pytest from bs4 import BeautifulSoup @@ -2548,16 +2549,45 @@ def mock_get_email_branding_that_can_fit_onscreen(mocker): @pytest.fixture(scope='function') -def mock_get_letter_branding(mocker): +def mock_get_all_letter_branding(mocker): def _get_letter_branding(): - return { - '001': 'HM Government', - '500': 'Land Registry', - '999': 'Animal and Plant Health Agency', - } + return [ + { + 'id': str(UUID(int=0)), + 'name': 'HM Government', + 'filename': 'hm-government', + 'domain': None, + }, + { + 'id': str(UUID(int=1)), + 'name': 'Land Registry', + 'filename': 'land-registry', + 'domain': None, + }, + { + 'id': str(UUID(int=2)), + 'name': 'Animal and Plant Health Agency', + 'filename': 'animal', + 'domain': None, + } + ] return mocker.patch( - 'app.letter_branding_client.get_letter_branding', side_effect=_get_letter_branding + 'app.letter_branding_client.get_all_letter_branding', side_effect=_get_letter_branding + ) + + +@pytest.fixture +def mock_get_letter_branding_by_id(mocker): + def _get_branding_by_id(_id): + return { + 'id': _id, + 'name': 'HM Government', + 'filename': 'hm-government', + 'domain': None, + } + return mocker.patch( + 'app.letter_branding_client.get_letter_branding', side_effect=_get_branding_by_id ) From d5446774fa6e6ff44239738a91ae3865a405da13 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 5 Feb 2019 17:21:09 +0000 Subject: [PATCH 6/8] letter branding text fields should be optional --- app/templates/views/service-settings.html | 4 ++-- tests/app/main/views/test_service_settings.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index 93c3e7c24..92ecbc8a3 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -232,7 +232,7 @@ {% call settings_row(if_has_permission='letter') %} {{ text_field('Letter branding') }} - {{ text_field(current_service.letter_branding) }} + {{ optional_text_field(current_service.letter_branding.name) }} {{ edit_field( 'Change', url_for('.request_letter_branding', service_id=current_service.id), @@ -309,7 +309,7 @@ {% endcall %} {% call row() %} {{ text_field('Letter branding')}} - {{ text_field(current_service.letter_branding.name) }} + {{ optional_text_field(current_service.letter_branding.name) }} {{ edit_field('Change', url_for('.service_set_letter_branding', service_id=current_service.id)) }} {% endcall %} {% call row() %} diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 683db61d9..c35adaa37 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -94,7 +94,7 @@ def mock_get_service_settings_page_common( 'Organisation type Central Change', 'Free text message allowance 250,000 Change', 'Email branding GOV.UK Change', - 'Letter branding Change', + 'Letter branding Not set Change', 'Data retention email Change' ]), @@ -186,7 +186,7 @@ def test_should_show_overview( 'Label Value Action', 'Send letters On Change', 'Sender addresses 1 Example Street Manage', - 'Letter branding None Change', + 'Letter branding Not set Change', ]), ]) From 7590b3dba9fb9ae04f2b2dd9928b530f51ac8406 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 6 Feb 2019 15:53:36 +0000 Subject: [PATCH 7/8] remove unused sort_key param --- app/notify_client/letter_branding_client.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/app/notify_client/letter_branding_client.py b/app/notify_client/letter_branding_client.py index 2643e2f1e..88afe99c6 100644 --- a/app/notify_client/letter_branding_client.py +++ b/app/notify_client/letter_branding_client.py @@ -8,11 +8,8 @@ class LetterBrandingClient(NotifyAdminAPIClient): return self.get(url='/letter-branding/{}'.format(branding_id)) @cache.set('letter_branding') - def get_all_letter_branding(self, sort_key=None): - brandings = self.get(url='/letter-branding') - if sort_key and sort_key in brandings[0]: - brandings.sort(key=lambda branding: branding[sort_key].lower()) - return brandings + def get_all_letter_branding(self): + return self.get(url='/letter-branding') @cache.delete('letter_branding') def create_letter_branding(self, filename, name, domain): From f83910599c0b568acc5577651c69f5af76138f70 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 6 Feb 2019 17:32:13 +0000 Subject: [PATCH 8/8] remove option for branding to sometimes not show search it wouldn't show search if there were under a certain amount of letter or email branding options - however we know there will always be more than that amount so lets remove some complexity. Also, rename the SearchTemplatesForm because it can search anything - it just prompts you to search by name is all. --- app/main/forms.py | 2 +- app/main/views/conversation.py | 4 +-- app/main/views/email_branding.py | 5 ++- app/main/views/index.py | 4 +-- app/main/views/service_settings.py | 8 ++--- app/main/views/templates.py | 8 ++--- .../views/email-branding/select-branding.html | 2 +- .../service-settings/set-email-branding.html | 2 +- tests/app/main/views/test_service_settings.py | 31 ------------------- tests/conftest.py | 22 ------------- 10 files changed, 16 insertions(+), 72 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 6e8ab42f4..3be4b71d4 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -960,7 +960,7 @@ class ChooseTemplateType(StripWhitespaceForm): ]) -class SearchTemplatesForm(StripWhitespaceForm): +class SearchByNameForm(StripWhitespaceForm): search = SearchField('Search by name') diff --git a/app/main/views/conversation.py b/app/main/views/conversation.py index abe71fc3a..65016bca3 100644 --- a/app/main/views/conversation.py +++ b/app/main/views/conversation.py @@ -6,7 +6,7 @@ from notifications_utils.template import SMSPreviewTemplate from app import current_service, notification_api_client, service_api_client from app.main import main -from app.main.forms import SearchTemplatesForm +from app.main.forms import SearchByNameForm from app.utils import user_has_permissions @@ -50,7 +50,7 @@ def conversation_reply( templates=templates, show_search_box=(len(templates) > 7), template_type='sms', - search_form=SearchTemplatesForm(), + search_form=SearchByNameForm(), notification_id=notification_id, ) diff --git a/app/main/views/email_branding.py b/app/main/views/email_branding.py index 338de3c77..22b29325d 100644 --- a/app/main/views/email_branding.py +++ b/app/main/views/email_branding.py @@ -3,7 +3,7 @@ from flask_login import login_required from app import email_branding_client from app.main import main -from app.main.forms import SearchTemplatesForm, ServiceUpdateEmailBranding +from app.main.forms import SearchByNameForm, ServiceUpdateEmailBranding from app.s3_client.s3_logo_client import ( TEMP_TAG, delete_email_temp_file, @@ -24,8 +24,7 @@ def email_branding(): return render_template( 'views/email-branding/select-branding.html', email_brandings=brandings, - search_form=SearchTemplatesForm(), - show_search_box=len(brandings) > 9, + search_form=SearchByNameForm(), agreement_info=AgreementInfo, ) diff --git a/app/main/views/index.py b/app/main/views/index.py index 75e8baab9..c68296f9a 100644 --- a/app/main/views/index.py +++ b/app/main/views/index.py @@ -14,7 +14,7 @@ from notifications_utils.template import HTMLEmailTemplate from app import email_branding_client from app.main import main -from app.main.forms import FieldWithNoneOption, SearchTemplatesForm +from app.main.forms import FieldWithNoneOption, SearchByNameForm from app.main.views.sub_navigation_dictionaries import features_nav from app.utils import AgreementInfo, get_logo_cdn_domain @@ -74,7 +74,7 @@ def pricing(): (cc, country['names'], country['billable_units']) for cc, country in INTERNATIONAL_BILLING_RATES.items() ], key=lambda x: x[0]), - search_form=SearchTemplatesForm(), + search_form=SearchByNameForm(), agreement_info=AgreementInfo.from_current_user(), ) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 22b950755..6d3b51de3 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -35,7 +35,7 @@ from app.main.forms import ( OrganisationTypeForm, RenameServiceForm, RequestToGoLiveForm, - SearchTemplatesForm, + SearchByNameForm, ServiceContactDetailsForm, ServiceDataRetentionEditForm, ServiceDataRetentionForm, @@ -773,8 +773,7 @@ def service_set_email_branding(service_id): return render_template( 'views/service-settings/set-email-branding.html', form=form, - search_form=SearchTemplatesForm(), - show_search_box=(len(email_branding) > 6) + search_form=SearchByNameForm() ) @@ -820,8 +819,7 @@ def service_set_letter_branding(service_id): return render_template( 'views/service-settings/set-letter-branding.html', form=form, - search_form=SearchTemplatesForm(), - show_search_box=(len(letter_branding) > 6) + search_form=SearchByNameForm() ) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 2e0831cf1..c277e2892 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -22,7 +22,7 @@ from app.main.forms import ( EmailTemplateForm, LetterTemplateForm, LetterTemplatePostageForm, - SearchTemplatesForm, + SearchByNameForm, SetTemplateSenderForm, SMSTemplateForm, TemplateAndFoldersSelectionForm, @@ -149,7 +149,7 @@ def choose_template(service_id, template_type='all', template_folder_id=None): ), template_nav_items=get_template_nav_items(template_folder_id), template_type=template_type, - search_form=SearchTemplatesForm(), + search_form=SearchByNameForm(), templates_and_folders_form=templates_and_folders_form, move_to_children=templates_and_folders_form.move_to.children(), option_hints=option_hints @@ -348,7 +348,7 @@ def choose_template_to_copy( ), template_folder_path=service.get_template_folder_path(from_folder), from_service=service, - search_form=SearchTemplatesForm(), + search_form=SearchByNameForm(), ) else: @@ -358,7 +358,7 @@ def choose_template_to_copy( Service(service) for service in user_api_client.get_services_for_user(current_user) ]), - search_form=SearchTemplatesForm(), + search_form=SearchByNameForm(), ) diff --git a/app/templates/views/email-branding/select-branding.html b/app/templates/views/email-branding/select-branding.html index 19d172dda..6007d7f98 100644 --- a/app/templates/views/email-branding/select-branding.html +++ b/app/templates/views/email-branding/select-branding.html @@ -17,7 +17,7 @@ Add new brand

- {{ live_search(target_selector='.email-brand', show=show_search_box, form=search_form) }} + {{ live_search(target_selector='.email-brand', show=True, form=search_form) }}