From afcdedf598c359ab5bab1d6c96e030419126b80b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 12 Sep 2019 14:33:11 +0100 Subject: [PATCH] =?UTF-8?q?Allow=20elaboration=20when=20=E2=80=98something?= =?UTF-8?q?=20else=E2=80=99=20is=20chosen?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Letting people input a bit of free text should reduce the amount of back and forth we have to do over support tickets when setting up someone’s branding. If something else is the only option then we don’t show the radio button at all and have just the free text input on the page (not behind a progressive disclosure). --- app/main/forms.py | 29 +++++-- app/main/views/service_settings.py | 5 ++ .../branding/email-options.html | 24 +++++- tests/app/main/views/test_service_settings.py | 79 +++++++++++-------- 4 files changed, 95 insertions(+), 42 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 363e9d3be..270da37a0 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1359,16 +1359,17 @@ class LinkOrganisationsForm(StripWhitespaceForm): class BrandingOptionsEmail(StripWhitespaceForm): - options = RadioField( - 'Choose your new email branding', - validators=[ - DataRequired() - ], - ) + FALLBACK_OPTION_VALUE = 'something_else' + FALLBACK_OPTION = (FALLBACK_OPTION_VALUE, 'Something else') + + options = RadioField('Choose your new email branding') + something_else = TextAreaField('Describe the branding you want') def __init__(self, service, *args, **kwargs): super().__init__(*args, **kwargs) self.options.choices = tuple(self.get_available_choices(service)) + if not self.something_else_is_only_option: + self.options.validators.append(DataRequired()) @staticmethod def get_available_choices(service): @@ -1404,7 +1405,21 @@ class BrandingOptionsEmail(StripWhitespaceForm): ): yield ('organisation', service.organisation.name) - yield ('something_else', 'Something else') + yield BrandingOptionsEmail.FALLBACK_OPTION + + @property + def something_else_is_only_option(self): + return self.options.choices == (self.FALLBACK_OPTION,) + + def validate_something_else(self, field): + if ( + self.something_else_is_only_option or + self.options.data == self.FALLBACK_OPTION_VALUE + ) and not field.data: + raise ValidationError('Can’t be empty') + + if self.options.data != self.FALLBACK_OPTION_VALUE: + field.data = '' class ServiceDataRetentionForm(StripWhitespaceForm): diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 4036f6d74..a1b165869 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -1047,12 +1047,17 @@ def branding_request(service_id): '\n---' '\nCurrent branding: {current_branding}' '\nBranding requested: {branding_requested}' + '{new_paragraph}' + '{detail}' + '\n' ).format( organisation=current_service.organisation.as_info_for_branding_request(current_user.email_domain), service_name=current_service.name, dashboard_url=url_for('main.service_dashboard', service_id=current_service.id, _external=True), current_branding=current_service.email_branding_name, branding_requested=dict(form.options.choices)[form.options.data], + new_paragraph='\n\n' if form.something_else.data else '', + detail=form.something_else.data or '' ), ticket_type=zendesk_client.TYPE_QUESTION, user_email=current_user.email_address, diff --git a/app/templates/views/service-settings/branding/email-options.html b/app/templates/views/service-settings/branding/email-options.html index 81b65c764..1e7957720 100644 --- a/app/templates/views/service-settings/branding/email-options.html +++ b/app/templates/views/service-settings/branding/email-options.html @@ -1,5 +1,6 @@ {% extends "withnav_template.html" %} -{% from "components/radios.html" import radios %} +{% from "components/radios.html" import radio, conditional_radio_panel %} +{% from "components/select-input.html" import select_wrapper %} {% from "components/textbox.html" import textbox %} {% from "components/page-header.html" import page_header %} {% from "components/page-footer.html" import page_footer %} @@ -17,7 +18,26 @@ ) }} {% call form_wrapper() %} - {{ radios(form.options) }} + {% if form.something_else_is_only_option %} + {{ textbox( + form.something_else, + hint='Include links to your brand guidelines or examples of how to use your branding', + width='1-1', + ) }} + {% else %} + {% call select_wrapper(form.options) %} + {% for option in form.options %} + {{ radio(option, data_target='panel-something-else' if option.data == form.FALLBACK_OPTION_VALUE else '') }} + {% endfor %} + {% endcall %} + {% call conditional_radio_panel('panel-something-else') %} + {{ textbox( + form.something_else, + hint='Include links to your brand guidelines or examples of how to use your branding', + width='1-1', + ) }} + {% endcall %} + {% endif %}

We’ll email you once your branding’s ready to use, or if we need any more information. diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 110826fe7..c31041e23 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -4378,12 +4378,8 @@ def test_update_service_organisation_does_not_update_if_same_value( @pytest.mark.parametrize('organisation_type, expected_options', ( - ('central', [ - ('something_else', 'Something else'), - ]), - ('local', [ - ('something_else', 'Something else'), - ]), + ('central', None), + ('local', None), ('nhs_central', [ ('nhs', 'NHS'), ('something_else', 'Something else'), @@ -4396,12 +4392,8 @@ def test_update_service_organisation_does_not_update_if_same_value( ('nhs', 'NHS'), ('something_else', 'Something else'), ]), - ('emergency_service', [ - ('something_else', 'Something else'), - ]), - ('other', [ - ('something_else', 'Something else'), - ]), + ('emergency_service', None), + ('other', None), )) def test_show_email_branding_request_page_when_no_email_branding_is_set( mocker, @@ -4424,13 +4416,26 @@ def test_show_email_branding_request_page_when_no_email_branding_is_set( mock_get_email_branding.assert_not_called() - assert [ - ( - radio['value'], - page.select_one('label[for={}]'.format(radio['id'])).text.strip() + if expected_options: + assert [ + ( + radio['value'], + page.select_one('label[for={}]'.format(radio['id'])).text.strip() + ) + for radio in page.select('input[type=radio]') + ] == expected_options + assert page.select_one( + '.conditional-radios-panel#panel-something-else textarea' + )['name'] == ( + 'something_else' ) - for radio in page.select('input[type=radio]') - ] == expected_options + else: + assert page.select_one( + 'textarea' + )['name'] == ( + 'something_else' + ) + assert not page.select('.conditional-radios-panel') @pytest.mark.parametrize('organisation_type, expected_options', ( @@ -4541,19 +4546,12 @@ def test_show_email_branding_request_page_when_email_branding_is_same_as_org( '.branding_request', service_id=SERVICE_ONE_ID ) - assert [ - ( - radio['value'], - page.select_one('label[for={}]'.format(radio['id'])).text.strip() - ) - for radio in page.select('input[type=radio]') - ] == [ - # Central government organisations who have their own default - # branding will do so because they’re exempt from GOV.UK. - # We also don’t show their organisation’s branding because they - # have it already. - ('something_else', 'Something else'), - ] + # Central government organisations who have their own default + # branding will do so because they’re exempt from GOV.UK. + # We also don’t show their organisation’s branding because they + # have it already. So ‘Something else’ is the only option. + assert not page.select('input[type=radio]') + assert page.select_one('textarea')['name'] == 'something_else' @pytest.mark.parametrize('data, requested_branding', ( @@ -4564,10 +4562,25 @@ def test_show_email_branding_request_page_when_email_branding_is_same_as_org( 'GOV.UK', ), ( + { + 'options': 'govuk', + 'something_else': 'ignored', + }, + 'GOV.UK', + ), + ( + { + 'options': 'something_else', + 'something_else': 'Homer Simpson' + }, + 'Something else\n\nHomer Simpson' + ), + pytest.param( { 'options': 'something_else', }, - 'Something else' + '[Missing details]', + marks=pytest.mark.xfail(raises=AssertionError), ), pytest.param( {'options': 'foo'}, @@ -4618,7 +4631,7 @@ def test_submit_email_branding_request( '', '---', 'Current branding: Organisation name', - 'Branding requested: {}', + 'Branding requested: {}\n', ]).format(expected_organisation, requested_branding), subject='Email branding request - service one', ticket_type='question',