Merge pull request #2235 from alphagov/sort-branding-radios-alphabetically

Sort branding radios alphabetically

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':

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

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):
+    def get_all_email_branding(self, sort_key=None):
-        return self.get(url='/email-branding')['email_branding']
+        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')

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(

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'
+    radio_labels = [
-    assert page.find('input', attrs={"id": "branding_type-1"})['value'] == 'both'
+        page.find('label', attrs={"for": branding_style_choices[idx]['id']}).get_text().strip()
-    assert page.find('input', attrs={"id": "branding_type-2"})['value'] == 'org'
+        for idx, element in enumerate(branding_style_choices)]
-    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 len(branding_style_choices) == 6
-    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 branding_style_choices[0]['value'] == 'None'
-    assert 'checked' not in page.find('input', attrs={"id": "branding_type-3"}).attrs
+    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'])

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():
+    def _get_all_email_branding(sort_key=None):
-        return [
+        if sort_key:
-            {'id': '1', 'name': 'org 1', 'text': 'org 1', 'colour': 'red', 'logo': 'logo1.png'},
+            return [
-            {'id': '2', 'name': 'org 2', 'text': 'org 2', 'colour': 'orange', 'logo': 'logo2.png'},
+                {'id': '1', 'name': 'org 1', 'text': 'org 1', 'colour': 'red', 'logo': 'logo1.png'},
-            {'id': '3', 'name': 'org 3', 'text': None, 'colour': None, 'logo': 'logo3.png'},
+                {'id': '2', 'name': 'org 2', 'text': 'org 2', 'colour': 'orange', 'logo': 'logo2.png'},
-            {'id': '4', 'name': 'org 4', 'text': 'org 4', 'colour': None, 'logo': 'logo4.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'},
-        ]
+                {'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