From 18e41902fbb2f5ddd487ba3b42bea3fea7ce754f Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Thu, 23 Aug 2018 20:30:06 +0100 Subject: [PATCH 01/19] Update pyexcel-io from 0.5.8 to 0.5.9 --- requirements-app.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements-app.txt b/requirements-app.txt index 69be07785..c29bba6cc 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -8,7 +8,7 @@ Flask-Login==0.4.1 blinker==1.4 pyexcel==0.5.8 -pyexcel-io==0.5.8 +pyexcel-io==0.5.9 pyexcel-xls==0.5.7 pyexcel-xlsx==0.5.6 pyexcel-ods3==0.5.2 From 0df88ea182ef380ae8a747fedbd4b6dd75ecc20d Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Wed, 29 Aug 2018 11:36:50 +0100 Subject: [PATCH 02/19] Remove branding_type from set_email_branding page Includes updates to the controller so branding_type is got from the email_branding model instead of from user input. Follows on from: https://github.com/alphagov/notifications-admin/pull/2249 --- app/assets/javascripts/emailPreviewPane.js | 15 +++++---------- app/main/forms.py | 18 ------------------ app/main/views/service_settings.py | 9 +++------ .../preview-email-branding.html | 2 +- .../service-settings/set-email-branding.html | 7 ++----- 5 files changed, 11 insertions(+), 40 deletions(-) diff --git a/app/assets/javascripts/emailPreviewPane.js b/app/assets/javascripts/emailPreviewPane.js index 7e42e99ba..5871118a3 100644 --- a/app/assets/javascripts/emailPreviewPane.js +++ b/app/assets/javascripts/emailPreviewPane.js @@ -5,18 +5,16 @@ const root = this, $ = this.jQuery; - let branding_type = $('.multiple-choice input[name="branding_type"]:checked'); let branding_style = $('.multiple-choice input[name="branding_style"]:checked'); - if (!branding_type.length || !branding_style.length) { return; } + if (!branding_style.length) { return; } - branding_type = branding_type.val(); branding_style = branding_style.val(); const $paneWrapper = $('
'); const $form = $('form'); - const $previewPane = $(''); function buildQueryString () { @@ -25,13 +23,10 @@ function setPreviewPane (e) { const $target = $(e.target); - if ($target.attr('name') == 'branding_type') { - branding_type = $target.val(); - } if ($target.attr('name') == 'branding_style') { branding_style = $target.val(); } - $previewPane.attr('src', '/_email?' + buildQueryString(['branding_type', branding_type], ['branding_style', branding_style])); + $previewPane.attr('src', '/_email?' + buildQueryString(['branding_style', branding_style])); } $paneWrapper.append($previewPane); @@ -39,5 +34,5 @@ $form.attr('action', location.pathname.replace(/set-email-branding$/, 'preview-email-branding')); $form.find('button[type="submit"]').text('Save'); - $('fieldset').on('change', 'input[name="branding_type"], input[name="branding_style"]', setPreviewPane); + $('fieldset').on('change', 'input[name="branding_style"]', setPreviewPane); })(); diff --git a/app/main/forms.py b/app/main/forms.py index f9897cf34..eea0a3110 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -682,23 +682,6 @@ class ServiceSwitchLettersForm(StripWhitespaceForm): class ServiceSetBranding(StripWhitespaceForm): - def __init__(self, email_branding=[], *args, **kwargs): - self.branding_style.choices = email_branding - super(ServiceSetBranding, self).__init__(*args, **kwargs) - - branding_type = RadioField( - 'Branding type', - choices=[ - ('govuk', 'GOV.UK only'), - ('both', 'GOV.UK and branding'), - ('org', 'Branding only'), - ('org_banner', 'Branding banner') - ], - validators=[ - DataRequired() - ] - ) - branding_style = RadioField( 'Branding style', validators=[ @@ -709,7 +692,6 @@ class ServiceSetBranding(StripWhitespaceForm): class ServicePreviewBranding(StripWhitespaceForm): - branding_type = HiddenField('branding_type') branding_style = HiddenField('branding_style') diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index e7c87f6a7..85a7b094c 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -868,9 +868,8 @@ def set_free_sms_allowance(service_id): @user_is_platform_admin def service_set_email_branding(service_id): email_branding = email_branding_client.get_all_email_branding() - branding_type = current_service.get('branding') - form = ServiceSetBranding(branding_type=branding_type) + form = ServiceSetBranding() # dynamically create org choices, including the null option email_brandings = sorted(get_branding_as_value_and_label(email_branding), @@ -880,7 +879,7 @@ def service_set_email_branding(service_id): if form.validate_on_submit(): branding_style = None if form.branding_style.data == 'None' else form.branding_style.data return redirect(url_for('.service_preview_email_branding', service_id=service_id, - branding_type=form.branding_type.data, branding_style=branding_style)) + branding_style=branding_style)) form.branding_style.data = current_service['email_branding'] or 'None' @@ -897,16 +896,14 @@ def service_set_email_branding(service_id): @login_required @user_is_platform_admin def service_preview_email_branding(service_id): - branding_type = request.args.get('branding_type', None) branding_style = request.args.get('branding_style', None) - form = ServicePreviewBranding(branding_type=branding_type, branding_style=branding_style) + form = ServicePreviewBranding(branding_style=branding_style) if form.validate_on_submit(): branding_style = None if form.branding_style.data == 'None' else form.branding_style.data service_api_client.update_service( service_id, - branding=form.branding_type.data, email_branding=branding_style ) return redirect(url_for('.service_settings', service_id=service_id)) diff --git a/app/templates/views/service-settings/preview-email-branding.html b/app/templates/views/service-settings/preview-email-branding.html index 51ecdb7e2..917a0bc44 100644 --- a/app/templates/views/service-settings/preview-email-branding.html +++ b/app/templates/views/service-settings/preview-email-branding.html @@ -9,7 +9,7 @@

Preview email branding

- +
{{ form.hidden_tag() }} diff --git a/app/templates/views/service-settings/set-email-branding.html b/app/templates/views/service-settings/set-email-branding.html index bbdd7d86c..ee8d8d73e 100644 --- a/app/templates/views/service-settings/set-email-branding.html +++ b/app/templates/views/service-settings/set-email-branding.html @@ -12,14 +12,11 @@

Set email branding

-
+
-
- {{ radios(form.branding_type) }} -
-
+
{{ live_search(target_selector='.brand_styles .multiple-choice', show=show_search_box, form=search_form, label='Search branding styles by name') }} {{ radios(form.branding_style) }}
From 151488fa784242f5de342aaff39800a96617ed37 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Wed, 29 Aug 2018 11:38:40 +0100 Subject: [PATCH 03/19] Update email preview page No longer requires the branding_type GET param. Retains the 'govuk' default brand. --- app/assets/javascripts/emailPreviewPane.js | 2 +- app/main/views/index.py | 11 +++++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/app/assets/javascripts/emailPreviewPane.js b/app/assets/javascripts/emailPreviewPane.js index 5871118a3..0509efa27 100644 --- a/app/assets/javascripts/emailPreviewPane.js +++ b/app/assets/javascripts/emailPreviewPane.js @@ -13,7 +13,7 @@ const $paneWrapper = $('
'); const $form = $('form'); - const $previewPane = $(''); diff --git a/app/main/views/index.py b/app/main/views/index.py index ed07c2a15..685cb77f9 100644 --- a/app/main/views/index.py +++ b/app/main/views/index.py @@ -80,17 +80,20 @@ def design_content(): @main.route('/_email') def email_template(): - branding_type = request.args.get('branding_type', 'govuk') - branding_style = request.args.get('branding_style', 'None') + branding_type = 'govuk' + branding_style = request.args.get('branding_style', None) - if branding_type == 'govuk' or branding_style == 'None': + if branding_style: + email_branding = email_branding_client.get_email_branding(branding_style)['email_branding'] + branding_type = email_branding['brand_type'] + + if branding_type == 'govuk': brand_name = None brand_colour = None brand_logo = None govuk_banner = True brand_banner = False else: - email_branding = email_branding_client.get_email_branding(branding_style)['email_branding'] colour = email_branding['colour'] brand_name = email_branding['text'] brand_colour = colour From 55f5cc8e1ea59a5b839aac11b021a8a3e90ed10f Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Wed, 29 Aug 2018 11:40:55 +0100 Subject: [PATCH 04/19] Update tests for changes Removes any checks for branding_type from tests for set_email_branding page. Updates tests for email preview page (usually iframed) so non-default brand_type is got from the email_branding model instead of a GET param. --- tests/app/main/views/test_email_preview.py | 25 +++---- tests/app/main/views/test_service_settings.py | 33 +-------- tests/conftest.py | 70 +++++++++++++------ 3 files changed, 64 insertions(+), 64 deletions(-) diff --git a/tests/app/main/views/test_email_preview.py b/tests/app/main/views/test_email_preview.py index e1f819f52..55421cb7f 100644 --- a/tests/app/main/views/test_email_preview.py +++ b/tests/app/main/views/test_email_preview.py @@ -32,9 +32,9 @@ def test_displays_govuk_branding_by_default(client): assert page.find("a", attrs={"href": "https://www.gov.uk"}) -def test_displays_govuk_branding(client): +def test_displays_govuk_branding(client, mock_get_email_branding_with_govuk_brand_type): - response = client.get(url_for('main.email_template', branding_type="govuk", branding_style="1")) + response = client.get(url_for('main.email_template', branding_style="1")) page = BeautifulSoup(response.data.decode("utf-8"), "html.parser") @@ -43,14 +43,14 @@ def test_displays_govuk_branding(client): assert page.find("a", attrs={"href": "https://www.gov.uk"}) -def test_displays_both_branding(client, mock_get_email_branding): +def test_displays_both_branding(client, mock_get_email_branding_with_both_brand_type): - response = client.get(url_for('main.email_template', branding_type="both", branding_style="1")) + response = client.get(url_for('main.email_template', branding_style="1")) page = BeautifulSoup(response.data.decode("utf-8"), "html.parser") assert response.status_code == 200 - mock_get_email_branding.assert_called_once_with('1') + mock_get_email_branding_with_both_brand_type.assert_called_once_with('1') assert page.find("a", attrs={"href": "https://www.gov.uk"}) assert page.find("img", attrs={"src": re.compile("example.png$")}) @@ -60,7 +60,8 @@ def test_displays_both_branding(client, mock_get_email_branding): def test_displays_org_branding(client, mock_get_email_branding): - response = client.get(url_for('main.email_template', branding_type="org", branding_style="1")) + # mock_get_email_branding has 'brand_type' of 'org' + response = client.get(url_for('main.email_template', branding_style="1")) page = BeautifulSoup(response.data.decode("utf-8"), "html.parser") @@ -74,15 +75,15 @@ def test_displays_org_branding(client, mock_get_email_branding): .get_text().strip() == 'Organisation text' # brand text is set -def test_displays_org_branding_with_banner(client, mock_get_email_branding): +def test_displays_org_branding_with_banner( + client, mock_get_email_branding_with_org_banner_brand_type): - response = client.get(url_for('main.email_template', branding_type="org_banner", - branding_style="1")) + response = client.get(url_for('main.email_template', branding_style="1")) page = BeautifulSoup(response.data.decode("utf-8"), "html.parser") assert response.status_code == 200 - mock_get_email_branding.assert_called_once_with('1') + mock_get_email_branding_with_org_banner_brand_type.assert_called_once_with('1') assert not page.find("a", attrs={"href": "https://www.gov.uk"}) assert page.find("img", attrs={"src": re.compile("example.png")}) @@ -94,8 +95,8 @@ def test_displays_org_branding_with_banner(client, mock_get_email_branding): def test_displays_org_branding_with_banner_without_brand_text( client, mock_get_email_branding_without_brand_text): - response = client.get(url_for('main.email_template', branding_type="org_banner", - branding_style="1")) + # mock_get_email_branding_without_brand_text has 'brand_type' of 'org_banner' + response = client.get(url_for('main.email_template', branding_style="1")) page = BeautifulSoup(response.data.decode("utf-8"), "html.parser") diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index b87f834bd..d3b85ca05 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -1718,31 +1718,6 @@ def test_set_letter_branding_saves( mock_update_service.assert_called_once_with(service_one['id'], dvla_organisation='500') -def test_should_show_branding_types( - logged_in_platform_admin_client, - service_one, - mock_get_all_email_branding, -): - response = logged_in_platform_admin_client.get(url_for( - 'main.service_set_email_branding', service_id=service_one['id'] - )) - assert response.status_code == 200 - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - - assert page.find('input', attrs={"id": "branding_type-0"})['value'] == 'govuk' - assert page.find('input', attrs={"id": "branding_type-1"})['value'] == 'both' - assert page.find('input', attrs={"id": "branding_type-2"})['value'] == 'org' - assert page.find('input', attrs={"id": "branding_type-3"})['value'] == 'org_banner' - - assert 'checked' in page.find('input', attrs={"id": "branding_type-0"}).attrs - assert 'checked' not in page.find('input', attrs={"id": "branding_type-1"}).attrs - assert 'checked' not in page.find('input', attrs={"id": "branding_type-2"}).attrs - assert 'checked' not in page.find('input', attrs={"id": "branding_type-3"}).attrs - - app.email_branding_client.get_all_email_branding.assert_called_once_with() - app.service_api_client.get_service.assert_called_once_with(service_one['id']) - - def test_should_show_branding_styles( logged_in_platform_admin_client, service_one, @@ -1827,8 +1802,8 @@ def test_should_send_branding_and_organisations_to_preview( ) assert response.status_code == 302 assert response.location == url_for('main.service_preview_email_branding', - service_id=service_one['id'], branding_type='org', - branding_style='1', _external=True) + service_id=service_one['id'], branding_style='1', + _external=True) mock_get_all_email_branding.assert_called_once_with() @@ -1847,10 +1822,8 @@ def test_should_preview_email_branding( iframeURLComponents = urlparse(iframe['src']) iframeQString = parse_qs(iframeURLComponents.query) - assert page.find('input', attrs={"id": "branding_type"})['value'] == 'org' assert page.find('input', attrs={"id": "branding_style"})['value'] == '1' assert iframeURLComponents.path == '/_email' - assert iframeQString['branding_type'] == ['org'] assert iframeQString['branding_style'] == ['1'] app.service_api_client.get_service.assert_called_once_with(service_one['id']) @@ -1866,7 +1839,6 @@ def test_should_set_branding_and_organisations( 'main.service_preview_email_branding', service_id=service_one['id'] ), data={ - 'branding_type': 'org', 'branding_style': '1' } ) @@ -1876,7 +1848,6 @@ def test_should_set_branding_and_organisations( mock_update_service.assert_called_once_with( service_one['id'], - branding='org', email_branding='1' ) diff --git a/tests/conftest.py b/tests/conftest.py index 5ed2681da..e00d303e2 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2519,20 +2519,57 @@ def mock_no_email_branding(mocker): ) +def create_email_branding(id, non_standard_values={}): + branding = { + 'logo': 'example.png', + 'name': 'Organisation name', + 'text': 'Organisation text', + 'id': id, + 'colour': '#f00', + 'domain': 'sample.com', + 'brand_type': 'org', + } + + if bool(non_standard_values): + branding.update(non_standard_values) + + return {'email_branding': branding} + + @pytest.fixture(scope='function') def mock_get_email_branding(mocker, fake_uuid): def _get_email_branding(id): - return { - 'email_branding': { - 'logo': 'example.png', - 'name': 'Organisation name', - 'text': 'Organisation text', - 'id': fake_uuid, - 'colour': '#f00', - 'domain': 'sample.com', - 'brand_type': 'org', - } - } + return create_email_branding(fake_uuid) + + return mocker.patch( + 'app.email_branding_client.get_email_branding', side_effect=_get_email_branding + ) + + +@pytest.fixture(scope='function') +def mock_get_email_branding_with_govuk_brand_type(mocker, fake_uuid): + def _get_email_branding(id): + return create_email_branding(fake_uuid, {'brand_type': 'govuk'}) + + return mocker.patch( + 'app.email_branding_client.get_email_branding', side_effect=_get_email_branding + ) + + +@pytest.fixture(scope='function') +def mock_get_email_branding_with_both_brand_type(mocker, fake_uuid): + def _get_email_branding(id): + return create_email_branding(fake_uuid, {'brand_type': 'both'}) + + return mocker.patch( + 'app.email_branding_client.get_email_branding', side_effect=_get_email_branding + ) + + +@pytest.fixture(scope='function') +def mock_get_email_branding_with_org_banner_brand_type(mocker, fake_uuid): + def _get_email_branding(id): + return create_email_branding(fake_uuid, {'brand_type': 'org_banner'}) return mocker.patch( 'app.email_branding_client.get_email_branding', side_effect=_get_email_branding @@ -2542,16 +2579,7 @@ def mock_get_email_branding(mocker, fake_uuid): @pytest.fixture(scope='function') def mock_get_email_branding_without_brand_text(mocker, fake_uuid): def _get_email_branding_without_brand_text(id): - return { - 'email_branding': { - 'logo': 'example.png', - 'name': 'Organisation name', - 'text': '', - 'id': fake_uuid, - 'colour': '#f00', - 'brand_type': 'org_banner' - } - } + return create_email_branding(fake_uuid, {'text': '', 'brand_type': 'org_banner'}) return mocker.patch( 'app.email_branding_client.get_email_branding', From 272049bb172ca28c2b8c23d57da37167ffec2985 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Wed, 29 Aug 2018 15:20:46 +0100 Subject: [PATCH 05/19] Update branding request page It was getting the brand_type from the service model. This changes the controller to get it from the new email_branding model instead. --- app/main/views/service_settings.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 85a7b094c..b774f03fc 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -971,8 +971,11 @@ def link_service_to_organisation(service_id): @user_has_permissions('manage_service') def branding_request(service_id): + email_branding = email_branding_client.get_email_branding( + current_service.email_branding)['email_branding'] + form = BrandingOptionsEmail( - options=current_service.branding + options=email_branding['brand_type'] ) if form.validate_on_submit(): From 1d3b5fda629c581aa72fad6ad1cc0a3d83c0f539 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Wed, 29 Aug 2018 15:22:20 +0100 Subject: [PATCH 06/19] Update brand request tests --- tests/app/main/views/test_service_settings.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index d3b85ca05..284fe148f 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -2731,11 +2731,14 @@ def test_update_service_organisation_does_not_update_if_same_value( def test_show_email_branding_request_page( client_request, + mock_get_email_branding ): page = client_request.get( '.branding_request', service_id=SERVICE_ONE_ID ) + mock_get_email_branding.called_once_with(None) + radios = page.select('input[type=radio]') for index, option in enumerate(( @@ -2767,6 +2770,11 @@ def test_submit_email_branding_request( single_sms_sender, ): + email_branding_client = mocker.patch( + 'app.email_branding_client.get_email_branding', + return_value={'email_branding': {'brand_type': choice}} + ) + zendesk = mocker.patch( 'app.main.views.service_settings.zendesk_client.create_ticket', autospec=True, @@ -2780,6 +2788,8 @@ def test_submit_email_branding_request( _follow_redirects=True, ) + email_branding_client.assert_called_once_with(None) + zendesk.assert_called_once_with( message='\n'.join([ 'Organisation: Can’t tell (domain is user.gov.uk)', From 3cfe5a07e57e1b51dd2064d8e30a7336ce39f333 Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Wed, 29 Aug 2018 16:32:24 +0100 Subject: [PATCH 07/19] Update coveralls from 1.3.0 to 1.4.0 --- requirements_for_test.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements_for_test.txt b/requirements_for_test.txt index f1f05cf5f..f40c891d9 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -5,7 +5,7 @@ pytest-env==0.6.2 pytest-mock==1.10.0 pytest-cov==2.5.1 pytest-xdist==1.23.0 -coveralls==1.3.0 +coveralls==1.4.0 httpretty==0.9.5 beautifulsoup4==4.6.3 freezegun==0.3.10 From 2328075924cd2cc9ed80595d24c7ece11fa55a8b Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Wed, 29 Aug 2018 22:32:18 +0100 Subject: [PATCH 08/19] Update pytest from 3.7.3 to 3.7.4 --- requirements_for_test.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements_for_test.txt b/requirements_for_test.txt index f1f05cf5f..35fde9e9a 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -1,6 +1,6 @@ -r requirements.txt isort==4.3.4 -pytest==3.7.3 +pytest==3.7.4 pytest-env==0.6.2 pytest-mock==1.10.0 pytest-cov==2.5.1 From 896072f16991738e883dda7f43cb7d93f00df585 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 30 Aug 2018 09:11:06 +0100 Subject: [PATCH 09/19] Remove reference to service branding on settings This attribute is deprecated in favour of an attribute on the `email_branding` itself. --- app/templates/views/service-settings.html | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index f84809dcb..ad6b64909 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -101,9 +101,7 @@ {% call settings_row(if_has_permission='email') %} {{ text_field('Email branding') }} - {{ text_field( - 'GOV.UK' if current_service.branding == 'govuk' else 'Your branding' - ) }} + {{ text_field('Your branding' if email_branding else 'GOV.UK') }} {{ edit_field( 'Change', url_for('.branding_request', service_id=current_service.id), @@ -301,17 +299,7 @@ {% endcall %} {% call row() %} {{ text_field('Email branding' )}} - {% call field() %} - {% if current_service.branding == 'govuk' %} - GOV.UK - {% elif current_service.branding == 'both' %} - GOV.UK and {{ email_branding.name if email_branding else None }} - {% elif current_service.branding == 'org' %} - Only {{ email_branding.name if email_branding else None }} - {% elif current_service.branding == 'org_banner' %} - Only {{ email_branding.name if email_branding else None }} banner - {% endif %} - {% endcall %} + {{ text_field(email_branding.name or 'GOV.UK') }} {{ edit_field('Change', url_for('.service_set_email_branding', service_id=current_service.id)) }} {% endcall %} {% call row() %} From ad0ffcc3f36356850b0f540f41723c3919fb5836 Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Thu, 30 Aug 2018 11:38:47 +0100 Subject: [PATCH 10/19] Fix issue that happens with branding_style of None Setting the branding_style to 'None' causes the API to remove the email_branding field from a service model. The branding request page controller was depending on that value being to present get the brand type. The email preview page (used in an iframe on various pages) wasn't able to recognise a branding_style of 'None', causing a blank page to render. --- app/main/views/index.py | 4 ++-- app/main/views/service_settings.py | 10 +++++++--- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/app/main/views/index.py b/app/main/views/index.py index 685cb77f9..5e91a7982 100644 --- a/app/main/views/index.py +++ b/app/main/views/index.py @@ -81,9 +81,9 @@ def design_content(): @main.route('/_email') def email_template(): branding_type = 'govuk' - branding_style = request.args.get('branding_style', None) + branding_style = request.args.get('branding_style', 'None') - if branding_style: + if branding_style != 'None': email_branding = email_branding_client.get_email_branding(branding_style)['email_branding'] branding_type = email_branding['brand_type'] diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index b774f03fc..c3046abbe 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -971,11 +971,15 @@ def link_service_to_organisation(service_id): @user_has_permissions('manage_service') def branding_request(service_id): - email_branding = email_branding_client.get_email_branding( - current_service.email_branding)['email_branding'] + branding_type = 'govuk' + + if current_service.email_branding: + email_branding = email_branding_client.get_email_branding( + current_service.email_branding)['email_branding'] + branding_type = email_branding['brand_type'] form = BrandingOptionsEmail( - options=email_branding['brand_type'] + options=branding_type ) if form.validate_on_submit(): From c2075b6d716cb83798cb186e98bce122b77ee13a Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Thu, 30 Aug 2018 11:44:36 +0100 Subject: [PATCH 11/19] Make label for None branding_style read as 'GOV.UK' --- app/main/views/service_settings.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index c3046abbe..a6b3ad532 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -874,7 +874,7 @@ def service_set_email_branding(service_id): # dynamically create org choices, including the null option email_brandings = sorted(get_branding_as_value_and_label(email_branding), key=lambda tup: tup[1].lower()) - form.branding_style.choices = [('None', 'None')] + email_brandings + form.branding_style.choices = [('None', 'GOV.UK')] + email_brandings if form.validate_on_submit(): branding_style = None if form.branding_style.data == 'None' else form.branding_style.data From 18985821a91dde6783d368621bd6474c051b9c50 Mon Sep 17 00:00:00 2001 From: Pete Herlihy Date: Thu, 30 Aug 2018 11:53:52 +0100 Subject: [PATCH 12/19] Advising about future letter pricing from 1 October This is just the pricing page, we'll sort the pricing table etc separately... W're removing non-crown pricing as we won't be charged differently any more. But we will retain the concept of non-crown as we may change differently ourselves in the future. --- app/templates/views/pricing.html | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/app/templates/views/pricing.html b/app/templates/views/pricing.html index 7171b15fb..acb432ef3 100644 --- a/app/templates/views/pricing.html +++ b/app/templates/views/pricing.html @@ -93,21 +93,21 @@
{% call mapping_table( caption='Letter pricing', - field_headings=['', 'Crown bodies', 'Other public sector organisations'], + field_headings=['', 'Current price', 'Price from 1 October 2018'], field_headings_visible=True, caption_visible=False ) %} - {% for sheets, central, local in [ - ('1 sheet', '30', '33'), - ('2 sheets', '33', '39'), - ('3 sheets', '36', '45'), - ('4 sheets', '39', '51'), - ('5 sheets', '42', '57'), + {% for sheets, current, future in [ + ('1 sheet', '30', '30'), + ('2 sheets', '33', '35'), + ('3 sheets', '36', '40'), + ('4 sheets', '39', '45'), + ('5 sheets', '42', '50'), ] %} {% call row() %} {% call row_heading() %} {{ sheets }} (double-sided) {% endcall %} - {{ text_field(central + 'p + VAT') }} - {{ text_field(local + 'p + VAT') }} + {{ text_field(current + 'p + VAT') }} + {{ text_field(future + 'p + VAT') }} {% endcall %} {% endfor %} {% endcall %} From 2ff81ea59d59119c03b8e83fc00c4f6a6c816f1e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 30 Aug 2018 14:36:40 +0100 Subject: [PATCH 13/19] Reword templates line on go live task list This version might read a little clearer and look a little neater. --- .../views/service-settings/request-to-go-live.html | 4 ++-- tests/app/main/views/test_service_settings.py | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/templates/views/service-settings/request-to-go-live.html b/app/templates/views/service-settings/request-to-go-live.html index f80287c1a..28cf3c503 100644 --- a/app/templates/views/service-settings/request-to-go-live.html +++ b/app/templates/views/service-settings/request-to-go-live.html @@ -24,8 +24,8 @@ ) }} {{ task_list_item( has_templates, - 'Add content to -templates to show the kind of messages you’ll send'.format( + 'Add templates with examples of the content you plan to send +'.format( url_for('main.choose_template', service_id=current_service.id) )|safe, ) }} diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index dd539d949..47989d873 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -509,9 +509,9 @@ def test_should_raise_duplicate_name_handled( (2, 'Add a team member who can manage settings, team and usage Completed'), ]) @pytest.mark.parametrize('count_of_templates, expected_templates_checklist_item', [ - (0, 'Add content to templates to show the kind of messages you’ll send Not completed'), - (1, 'Add content to templates to show the kind of messages you’ll send Completed'), - (2, 'Add content to templates to show the kind of messages you’ll send Completed'), + (0, 'Add templates with examples of the content you plan to send Not completed'), + (1, 'Add templates with examples of the content you plan to send Completed'), + (2, 'Add templates with examples of the content you plan to send Completed'), ]) @pytest.mark.parametrize('count_of_email_templates, reply_to_email_addresses, expected_reply_to_checklist_item', [ pytest.mark.xfail((0, [], ''), raises=IndexError), From 4f67502806ce411c197884ec9b1fa4aa72350e0e Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Thu, 30 Aug 2018 15:48:51 +0100 Subject: [PATCH 14/19] Add tests for changes to brand request page Tests the new code that gets the brand type from the email_branding model. Includes checks for a service without the email_branding field set. It also amends the test for a POST from that page, removing mocking of the email_branding client. This test runs against the default service which has its email_branding field set to None so no call is made to the client. It's testing the brand_type values selected so doesn't need the service to have an email_branding already set. --- tests/app/main/views/test_service_settings.py | 40 ++++++++++++++----- 1 file changed, 31 insertions(+), 9 deletions(-) diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 284fe148f..5360abd79 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -2729,7 +2729,7 @@ def test_update_service_organisation_does_not_update_if_same_value( mock_update_service_organisation.called is False -def test_show_email_branding_request_page( +def test_show_email_branding_request_page_when_no_email_branding_is_set( client_request, mock_get_email_branding ): @@ -2737,7 +2737,7 @@ def test_show_email_branding_request_page( '.branding_request', service_id=SERVICE_ONE_ID ) - mock_get_email_branding.called_once_with(None) + mock_get_email_branding.assert_not_called() radios = page.select('input[type=radio]') @@ -2751,6 +2751,35 @@ def test_show_email_branding_request_page( assert radios[index]['value'] == option +def test_show_email_branding_request_page_when_email_branding_is_set( + client_request, + mock_get_email_branding, + active_user_with_permissions, +): + + service_one = service_json(email_branding='1234') + client_request.login(active_user_with_permissions, service=service_one) + + page = client_request.get( + '.branding_request', service_id=SERVICE_ONE_ID + ) + + mock_get_email_branding.called_once_with('1234') + + radios = page.select('input[type=radio]') + + for index, option in enumerate(( + 'govuk', + 'both', + 'org', + 'org_banner', + )): + assert radios[index]['name'] == 'options' + assert radios[index]['value'] == option + if option == 'org': + assert 'checked' in radios[index].attrs + + @pytest.mark.parametrize('choice, requested_branding', ( ('govuk', 'GOV.UK only'), ('both', 'GOV.UK and logo'), @@ -2770,11 +2799,6 @@ def test_submit_email_branding_request( single_sms_sender, ): - email_branding_client = mocker.patch( - 'app.email_branding_client.get_email_branding', - return_value={'email_branding': {'brand_type': choice}} - ) - zendesk = mocker.patch( 'app.main.views.service_settings.zendesk_client.create_ticket', autospec=True, @@ -2788,8 +2812,6 @@ def test_submit_email_branding_request( _follow_redirects=True, ) - email_branding_client.assert_called_once_with(None) - zendesk.assert_called_once_with( message='\n'.join([ 'Organisation: Can’t tell (domain is user.gov.uk)', From d12fa00066c7c5ad0ed89bea4c628113238965fb Mon Sep 17 00:00:00 2001 From: Tom Byers Date: Thu, 30 Aug 2018 16:03:20 +0100 Subject: [PATCH 15/19] Update tests to allow new 'GOV.UK' label --- tests/app/main/views/test_service_settings.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 5360abd79..c41b18b46 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -1744,7 +1744,7 @@ def test_should_show_branding_styles( assert branding_style_choices[5]['value'] == '5' # radios should be in alphabetical order, based on their labels - assert radio_labels == ['None', 'org 1', 'org 2', 'org 3', 'org 4', 'org 5'] + assert radio_labels == ['GOV.UK', 'org 1', 'org 2', 'org 3', 'org 4', 'org 5'] assert 'checked' in branding_style_choices[0].attrs assert 'checked' not in branding_style_choices[1].attrs From 2686f4f0e2cb3d39c8eb464274ac2f14bee568b6 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 30 Aug 2018 17:08:35 +0100 Subject: [PATCH 16/19] Freeze requirements --- requirements.txt | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/requirements.txt b/requirements.txt index 74da1b40e..3c59571b0 100644 --- a/requirements.txt +++ b/requirements.txt @@ -10,7 +10,7 @@ Flask-Login==0.4.1 blinker==1.4 pyexcel==0.5.8 -pyexcel-io==0.5.8 +pyexcel-io==0.5.9 pyexcel-xls==0.5.7 pyexcel-xlsx==0.5.6 pyexcel-ods3==0.5.2 @@ -26,11 +26,11 @@ awscli-cwlogs>=1.4,<1.5 git+https://github.com/alphagov/notifications-utils.git@30.1.2#egg=notifications-utils==30.1.2 ## The following requirements were added by pip freeze: -awscli==1.15.83 +awscli==1.16.4 bleach==2.1.3 boto3==1.6.16 -botocore==1.10.82 -certifi==2018.8.13 +botocore==1.11.4 +certifi==2018.8.24 chardet==3.0.4 click==6.7 colorama==0.3.9 @@ -52,7 +52,7 @@ lxml==4.2.4 MarkupSafe==1.0 mistune==0.8.3 monotonic==1.5 -openpyxl==2.5.5 +openpyxl==2.5.6 orderedset==2.0.1 phonenumbers==8.9.4 pyasn1==0.4.4 From 7f458ad06649d49a051f03f6669c5cf9e9c79527 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 31 Aug 2018 10:30:45 +0100 Subject: [PATCH 17/19] Explain text message sender MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We sometimes have to do this over support tickets as part of the go-live process; now we’re directing people to add a sender (as part of the task list) we can explain what it is in context. --- app/templates/views/service-settings/sms-senders.html | 5 ++++- tests/app/main/views/test_service_settings.py | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/app/templates/views/service-settings/sms-senders.html b/app/templates/views/service-settings/sms-senders.html index 2d76635b3..297148220 100644 --- a/app/templates/views/service-settings/sms-senders.html +++ b/app/templates/views/service-settings/sms-senders.html @@ -23,7 +23,7 @@
{% if not sms_senders %}
- You haven’t added any sms senders yet + You haven’t added any text message senders yet
{% endif %} {% for item in sms_senders %} @@ -46,4 +46,7 @@
{% endfor %}
+

+ The text message sender tells your users who the message is from. +

{% endblock %} diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 47989d873..b3c884ac1 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -1021,7 +1021,7 @@ def test_default_option_shows_for_default_sender( ( 'main.service_sms_senders', no_sms_senders, - 'You haven’t added any sms senders yet' + 'You haven’t added any text message senders yet' ), ]) def test_no_senders_message_shows( From 631c3478ae6727d4f13847755deb558c3d56e76d Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 31 Aug 2018 10:32:26 +0100 Subject: [PATCH 18/19] Remove `branding` attribute from service model MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit To correspond with us dropping this column from the database. Remove the attribute from the model gives us more confidence that it’s not being used (because it will raise exceptions in any tests that refer to it). --- app/notify_client/models.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/notify_client/models.py b/app/notify_client/models.py index 87a977a8a..10a5eefad 100644 --- a/app/notify_client/models.py +++ b/app/notify_client/models.py @@ -270,7 +270,6 @@ class Service(dict): ALLOWED_PROPERTIES = { 'active', - 'branding', 'dvla_organisation', 'email_branding', 'email_from', @@ -293,7 +292,7 @@ class Service(dict): def __getattr__(self, attr): if attr in self.ALLOWED_PROPERTIES: return self[attr] - raise AttributeError + raise AttributeError('`{}` is not a service attribute'.format(attr)) @property def trial_mode(self): From 7d951ad1f3af2c1e130add74fd0bbec7684a4b73 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 3 Sep 2018 09:33:04 +0100 Subject: [PATCH 19/19] =?UTF-8?q?Don=E2=80=99t=20version=20control=20Pytes?= =?UTF-8?q?t=20cache?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pytest moved its cache from `./.cache` (which is in our `.gitignore`) to `./.pytest_cache` (which isn’t). It’s annoying having to be careful not to commit it all the time, so this commit makes it ignored. See https://github.com/pytest-dev/pytest/issues/3286 for more context. --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 0eae6ec00..5ec4a9689 100644 --- a/.gitignore +++ b/.gitignore @@ -40,6 +40,7 @@ htmlcov/ .coverage .coverage.* .cache +.pytest_cache coverage.xml test_results.xml *,cover