From 45a0a767f4cf83ce0842de78820462d211b69130 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 12 Jul 2017 15:50:11 +0100 Subject: [PATCH 1/2] Split settings page into multiple sections MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There are quite a few more options that there used to be in the settings page. This means it’s hard to find the thing you want to change. Grouping options is a common way of making things easier to find. Grouping by channel (text, email, letter) is something we do elsewhere that seems to work pretty well. --- app/assets/stylesheets/components/table.scss | 19 +++++++++ app/templates/views/service-settings.html | 32 ++++++++++++++- tests/app/main/views/test_service_settings.py | 40 +++++++++++++++++-- 3 files changed, 86 insertions(+), 5 deletions(-) diff --git a/app/assets/stylesheets/components/table.scss b/app/assets/stylesheets/components/table.scss index 152d209be..c86d5cc12 100644 --- a/app/assets/stylesheets/components/table.scss +++ b/app/assets/stylesheets/components/table.scss @@ -143,6 +143,25 @@ @include bold-16; } + +.table-field-headings { + + th { + padding: 1px; /* needs some height for the grey border to show */ + } + +} + +.table-field-headings-visible { + + height: auto; + + th { + padding: .75em 1.25em .5625em 0; + } + +} + .table-field-headings, .table-field-headings-visible { diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index 371f37c82..c1c6c73fc 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -11,20 +11,30 @@

Settings

-
+
{% call mapping_table( - caption='Settings', + caption='General', field_headings=['Label', 'Value', 'Action'], field_headings_visible=False, caption_visible=False ) %} + {% call row() %} {{ text_field('Service name') }} {{ text_field(current_service.name) }} {{ edit_field('Change', url_for('.service_name_change', service_id=current_service.id)) }} {% endcall %} + {% endcall %} + + {% call mapping_table( + caption='Email', + field_headings=['Label', 'Value', 'Action'], + field_headings_visible=False, + caption_visible=True + ) %} + {% call row() %} {{ text_field('Send emails') }} {{ boolean_field('email' in current_service.permissions) }} @@ -44,6 +54,15 @@ {% endif %} + {% endcall %} + + {% call mapping_table( + caption='Text messages', + field_headings=['Label', 'Value', 'Action'], + field_headings_visible=False, + caption_visible=True + ) %} + {% call row() %} {{ text_field('Send text messages') }} {{ boolean_field('sms' in current_service.permissions) }} @@ -88,6 +107,15 @@ {% endif %} + {% endcall %} + + {% call mapping_table( + caption='Letters', + field_headings=['Label', 'Value', 'Action'], + field_headings_visible=False, + caption_visible=True + ) %} + {% call row() %} {{ text_field('Letters') }} {{ boolean_field('letter' in current_service.permissions) }} diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 8e3281ddf..10b268cb7 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -17,29 +17,46 @@ from tests.conftest import active_user_with_permissions, platform_admin_user @pytest.mark.parametrize('user, expected_rows', [ (active_user_with_permissions, [ + 'Label Value Action', 'Service name service one Change', + + 'Label Value Action', 'Send emails On Change', 'Email reply to address None Change', + + 'Label Value Action', 'Send text messages On Change', 'Text message sender GOVUK Change', 'International text messages Off Change', 'Receive text messages Off Change', + + 'Label Value Action', 'Letters Off Change', + ]), (platform_admin_user, [ + 'Label Value Action', 'Service name service one Change', + + 'Label Value Action', 'Send emails On Change', 'Email reply to address None Change', + + 'Label Value Action', 'Send text messages On Change', 'Text message sender GOVUK Change', 'International text messages Off Change', 'Receive text messages Off Change', + + 'Label Value Action', 'Letters Off Change', + 'Label Value Action', 'Email branding GOV.UK Change', 'Letter branding HM Government Change', + ]), ]) def test_should_show_overview( @@ -51,6 +68,7 @@ def test_should_show_overview( user, expected_rows, ): + service_one['permissions'] = ['sms', 'email'] client.login(user(fake_uuid), mocker, service_one) @@ -60,7 +78,7 @@ def test_should_show_overview( assert response.status_code == 200 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') assert page.find('h1').text == 'Settings' - rows = page.find_all('tr') + rows = page.select('tr') assert len(rows) == len(expected_rows) for index, row in enumerate(expected_rows): assert row == " ".join(rows[index].text.split()) @@ -69,25 +87,41 @@ def test_should_show_overview( @pytest.mark.parametrize('permissions, expected_rows', [ (['email', 'sms', 'inbound_sms', 'international_sms'], [ + 'Service name service one Change', + + 'Label Value Action', 'Send emails On Change', 'Email reply to address test@example.com Change', + + 'Label Value Action', 'Send text messages On Change', 'Text message sender elevenchars', 'International text messages On Change', 'Receive text messages On Change', 'API endpoint for received text messages None Change', + + 'Label Value Action', 'Letters Off Change', + ]), (['email', 'sms'], [ + 'Service name service one Change', + + 'Label Value Action', 'Send emails On Change', 'Email reply to address test@example.com Change', + + 'Label Value Action', 'Send text messages On Change', 'Text message sender elevenchars Change', 'International text messages Off Change', 'Receive text messages Off Change', + + 'Label Value Action', 'Letters Off Change', + ]), ]) def test_should_show_overview_for_service_with_more_things_set( @@ -187,7 +221,7 @@ def test_letter_contact_block_shows_none_if_not_set( )) page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - div = page.find_all('tr')[5].find_all('td')[1].div + div = page.find_all('tr')[8].find_all('td')[1].div assert div.text.strip() == 'None' assert 'default' in div.attrs['class'][0] @@ -205,7 +239,7 @@ def test_escapes_letter_contact_block( )) page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - div = str(page.find_all('tr')[5].find_all('td')[1].div) + div = str(page.find_all('tr')[8].find_all('td')[1].div) assert 'foo
bar' in div assert '