diff --git a/app/main/views/email_branding.py b/app/main/views/email_branding.py index 60a9745d7..4f55b1e7f 100644 --- a/app/main/views/email_branding.py +++ b/app/main/views/email_branding.py @@ -26,10 +26,11 @@ from app.utils import get_cdn_domain, user_is_platform_admin @login_required @user_is_platform_admin def email_branding(): - brandings = email_branding_client.get_all_email_branding() + brandings = email_branding_client.get_all_email_branding(sort_key='name') form = ServiceSelectEmailBranding() - form.email_branding.choices = get_branding_as_value_and_label(brandings) + [('None', 'Create a new email branding')] + email_brandings = get_branding_as_value_and_label(brandings) + form.email_branding.choices = email_brandings + [('None', 'Create a new email branding')] if form.validate_on_submit(): if form.email_branding.data != 'None': diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index a180fdcba..5eb1b21f6 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -872,7 +872,9 @@ def service_set_email_branding(service_id): form = ServiceSetBranding(branding_type=branding_type) # dynamically create org choices, including the null option - form.branding_style.choices = [('None', 'None')] + get_branding_as_value_and_label(email_branding) + 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 if form.validate_on_submit(): branding_style = None if form.branding_style.data == 'None' else form.branding_style.data diff --git a/app/notify_client/email_branding_client.py b/app/notify_client/email_branding_client.py index 59ebbde3f..ae2375144 100644 --- a/app/notify_client/email_branding_client.py +++ b/app/notify_client/email_branding_client.py @@ -9,8 +9,11 @@ class EmailBrandingClient(NotifyAdminAPIClient): def get_email_branding(self, branding_id): return self.get(url='/email-branding/{}'.format(branding_id)) - def get_all_email_branding(self): - return self.get(url='/email-branding')['email_branding'] + def get_all_email_branding(self, sort_key=None): + brandings = self.get(url='/email-branding')['email_branding'] + if sort_key and sort_key in brandings[0]: + brandings.sort(key=lambda branding: branding[sort_key].lower()) + return brandings def get_letter_email_branding(self): return self.get(url='/dvla_organisations') diff --git a/tests/app/main/views/test_email_branding.py b/tests/app/main/views/test_email_branding.py index b084164b8..a5b88bc80 100644 --- a/tests/app/main/views/test_email_branding.py +++ b/tests/app/main/views/test_email_branding.py @@ -20,15 +20,19 @@ def test_email_branding_page_shows_full_branding_list( assert response.status_code == 200 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + radio_labels = page.select('div.multiple-choice > label') + brand_names = [element.get_text().strip() for idx, element in enumerate(radio_labels)] assert normalize_spaces( page.select_one('h1').text ) == "Select an email branding to update or create a new email branding" - first_label = page.select('div.multiple-choice > label')[0] + first_label = radio_labels[0] assert normalize_spaces(first_label.text) == 'org 1' + assert brand_names == [ + 'org 1', 'org 2', 'org 3', 'org 4', 'org 5', 'Create a new email branding'] - assert normalize_spaces((page.select('div.multiple-choice > label')[-1]).text) == 'Create a new email branding' + assert normalize_spaces((radio_labels[-1]).text) == 'Create a new email branding' def test_edit_email_branding_shows_the_correct_branding_info( diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index e20604f4f..12974d332 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -1718,11 +1718,10 @@ def test_set_letter_branding_saves( mock_update_service.assert_called_once_with(service_one['id'], dvla_organisation='500') -def test_should_show_branding( +def test_should_show_branding_types( logged_in_platform_admin_client, service_one, mock_get_all_email_branding, - mock_get_letter_email_branding, ): response = logged_in_platform_admin_client.get(url_for( 'main.service_set_email_branding', service_id=service_one['id'] @@ -1744,7 +1743,7 @@ def test_should_show_branding( app.service_api_client.get_service.assert_called_once_with(service_one['id']) -def test_should_show_organisations( +def test_should_show_branding_styles( logged_in_platform_admin_client, service_one, mock_get_all_email_branding, @@ -1754,16 +1753,30 @@ def test_should_show_organisations( )) assert response.status_code == 200 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + branding_style_choices = page.find_all('input', attrs={"name": "branding_style"}) - 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' + radio_labels = [ + page.find('label', attrs={"for": branding_style_choices[idx]['id']}).get_text().strip() + for idx, element in enumerate(branding_style_choices)] - 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 + 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' + + # 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 'checked' in branding_style_choices[0].attrs + assert 'checked' not in branding_style_choices[1].attrs + assert 'checked' not in branding_style_choices[2].attrs + assert 'checked' not in branding_style_choices[3].attrs + assert 'checked' not in branding_style_choices[4].attrs + assert 'checked' not in branding_style_choices[5].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']) diff --git a/tests/conftest.py b/tests/conftest.py index 8008b5d64..a056d43b2 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2438,14 +2438,23 @@ def mock_send_already_registered_email(mocker): @pytest.fixture(scope='function') def mock_get_all_email_branding(mocker): - def _get_all_email_branding(): - return [ - {'id': '1', 'name': 'org 1', 'text': 'org 1', 'colour': 'red', 'logo': 'logo1.png'}, - {'id': '2', 'name': 'org 2', 'text': 'org 2', 'colour': 'orange', 'logo': 'logo2.png'}, - {'id': '3', 'name': 'org 3', 'text': None, 'colour': None, 'logo': 'logo3.png'}, - {'id': '4', 'name': 'org 4', 'text': 'org 4', 'colour': None, 'logo': 'logo4.png'}, - {'id': '5', 'name': 'org 5', 'text': None, 'colour': 'blue', 'logo': 'logo5.png'}, - ] + def _get_all_email_branding(sort_key=None): + if sort_key: + return [ + {'id': '1', 'name': 'org 1', 'text': 'org 1', 'colour': 'red', 'logo': 'logo1.png'}, + {'id': '2', 'name': 'org 2', 'text': 'org 2', 'colour': 'orange', 'logo': 'logo2.png'}, + {'id': '3', 'name': 'org 3', 'text': None, 'colour': None, 'logo': 'logo3.png'}, + {'id': '4', 'name': 'org 4', 'text': 'org 4', 'colour': None, 'logo': 'logo4.png'}, + {'id': '5', 'name': 'org 5', 'text': None, 'colour': 'blue', 'logo': 'logo5.png'}, + ] + else: + return [ + {'id': '1', 'name': 'org 1', 'text': 'org 1', 'colour': 'red', 'logo': 'logo1.png'}, + {'id': '2', 'name': 'org 2', 'text': 'org 2', 'colour': 'orange', 'logo': 'logo2.png'}, + {'id': '3', 'name': 'org 3', 'text': None, 'colour': None, 'logo': 'logo3.png'}, + {'id': '5', 'name': 'org 5', 'text': None, 'colour': 'blue', 'logo': 'logo5.png'}, + {'id': '4', 'name': 'org 4', 'text': 'org 4', 'colour': None, 'logo': 'logo4.png'}, + ] return mocker.patch( 'app.email_branding_client.get_all_email_branding', side_effect=_get_all_email_branding