diff --git a/app/assets/javascripts/emailPreviewPane.js b/app/assets/javascripts/emailPreviewPane.js index 7e42e99ba..0509efa27 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 6cec88500..00bc5c7bd 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/index.py b/app/main/views/index.py index 183dfee32..56c74b4bf 100644 --- a/app/main/views/index.py +++ b/app/main/views/index.py @@ -91,17 +91,20 @@ def design_content(): @main.route('/_email') def email_template(): - branding_type = request.args.get('branding_type', 'govuk') + branding_type = 'govuk' branding_style = request.args.get('branding_style', 'None') - if branding_type == 'govuk' or branding_style == 'None': + if branding_style != 'None': + 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 diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 2a82c39c8..e7cb3559b 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -874,19 +874,18 @@ 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), 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 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' @@ -903,16 +902,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)) @@ -992,8 +989,15 @@ def link_service_to_organisation(service_id): @user_has_permissions('manage_service') def branding_request(service_id): + 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=current_service.branding + options=branding_type ) if form.validate_on_submit(): 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) }}
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 b3c884ac1..91dc11dbf 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -1846,31 +1846,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, @@ -1897,7 +1872,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 @@ -1955,8 +1930,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() @@ -1975,10 +1950,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']) @@ -1994,7 +1967,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' } ) @@ -2004,7 +1976,6 @@ def test_should_set_branding_and_organisations( mock_update_service.assert_called_once_with( service_one['id'], - branding='org', email_branding='1' ) @@ -2886,13 +2857,16 @@ 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 ): page = client_request.get( '.branding_request', service_id=SERVICE_ONE_ID ) + mock_get_email_branding.assert_not_called() + radios = page.select('input[type=radio]') for index, option in enumerate(( @@ -2905,6 +2879,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'), diff --git a/tests/conftest.py b/tests/conftest.py index d27c7934f..55012896c 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',