From 80801d827a10ae5d4c3d0b2c69a8bbbd6b7f5466 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 31 Aug 2018 17:21:12 +0100 Subject: [PATCH 1/2] Put the currently selected branding top of list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There’s something that feels a bit off about not being able to see the name of the currently-selected branding when you land on the page. Putting it at the top also means that you can easily switch back to it if you change your mind. --- app/main/views/service_settings.py | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 589ced108..d8a86b54d 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -890,9 +890,14 @@ def service_set_email_branding(service_id): 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', 'GOV.UK')] + email_brandings + form.branding_style.choices = sorted( + get_branding_as_value_and_label(email_branding) + [('None', 'GOV.UK')], + key=lambda branding: ( + branding[0] != current_service.email_branding, + branding[0] is not 'None', + branding[1].lower(), + ), + ) if form.validate_on_submit(): branding_style = None if form.branding_style.data == 'None' else form.branding_style.data From d80b735b2be5c07d7a61c3338ac9e64b42e589f2 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 31 Aug 2018 17:31:26 +0100 Subject: [PATCH 2/2] Refactor some of the logic into form MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s messy having a lot of logic in the view methods. Handling the form stuff can be better encapsulated inside the `Form` subclass itself. --- app/main/forms.py | 24 ++++++++++++++++- app/main/views/service_settings.py | 23 ++++++---------- tests/app/main/views/test_service_settings.py | 26 ++++++++++++++----- 3 files changed, 50 insertions(+), 23 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 713badf07..e9bea2992 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -682,15 +682,37 @@ class ServiceSwitchLettersForm(StripWhitespaceForm): ) +class BrandingStyle(RadioField): + + def post_validate(self, form, validation_stopped): + if self.data == 'None': + self.data = None + + class ServiceSetBranding(StripWhitespaceForm): - branding_style = RadioField( + branding_style = BrandingStyle( 'Branding style', validators=[ DataRequired() ] ) + DEFAULT = ('None', 'GOV.UK') + + def __init__(self, all_email_brandings, current_email_branding): + + super().__init__(branding_style=current_email_branding) + + self.branding_style.choices = sorted( + all_email_brandings + [self.DEFAULT], + key=lambda branding: ( + branding[0] != current_email_branding, + branding[0] is not self.DEFAULT[0], + branding[1].lower(), + ), + ) + class ServicePreviewBranding(StripWhitespaceForm): diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index d8a86b54d..5cf9390db 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -887,24 +887,17 @@ 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() - - # dynamically create org choices, including the null option - form.branding_style.choices = sorted( - get_branding_as_value_and_label(email_branding) + [('None', 'GOV.UK')], - key=lambda branding: ( - branding[0] != current_service.email_branding, - branding[0] is not 'None', - branding[1].lower(), - ), + form = ServiceSetBranding( + all_email_brandings=get_branding_as_value_and_label(email_branding), + current_email_branding=current_service.email_branding, ) 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_style=branding_style)) - - form.branding_style.data = current_service['email_branding'] or 'None' + return redirect(url_for( + '.service_preview_email_branding', + service_id=service_id, + branding_style=form.branding_style.data, + )) return render_template( 'views/service-settings/set-email-branding.html', diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 0a7924353..28a2792d3 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -1849,11 +1849,27 @@ def test_set_letter_branding_saves( mock_update_service.assert_called_once_with(service_one['id'], dvla_organisation='500') +@pytest.mark.parametrize('current_branding, expected_values, expected_labels', [ + (None, [ + 'None', '1', '2', '3', '4', '5', + ], [ + 'GOV.UK', 'org 1', 'org 2', 'org 3', 'org 4', 'org 5' + ]), + ('5', [ + '5', 'None', '1', '2', '3', '4', + ], [ + 'org 5', 'GOV.UK', 'org 1', 'org 2', 'org 3', 'org 4', + ]), +]) def test_should_show_branding_styles( logged_in_platform_admin_client, service_one, mock_get_all_email_branding, + current_branding, + expected_values, + expected_labels, ): + service_one['email_branding'] = current_branding response = logged_in_platform_admin_client.get(url_for( 'main.service_set_email_branding', service_id=service_one['id'] )) @@ -1867,15 +1883,11 @@ def test_should_show_branding_styles( assert len(branding_style_choices) == 6 - assert branding_style_choices[0]['value'] == 'None' - assert branding_style_choices[1]['value'] == '1' - assert branding_style_choices[2]['value'] == '2' - assert branding_style_choices[3]['value'] == '3' - assert branding_style_choices[4]['value'] == '4' - assert branding_style_choices[5]['value'] == '5' + for index, expected_value in enumerate(expected_values): + assert branding_style_choices[index]['value'] == expected_value # radios should be in alphabetical order, based on their labels - assert radio_labels == ['GOV.UK', 'org 1', 'org 2', 'org 3', 'org 4', 'org 5'] + assert radio_labels == expected_labels assert 'checked' in branding_style_choices[0].attrs assert 'checked' not in branding_style_choices[1].attrs