Make settings page a table not a browse list

There’s no way of seeing what the current settings are for a service
without individually going to the pages where you can change them.

This commit replaces the list of settings with a table. The table plays
back the current value for each setting.

This is a pattern that has worked well on various services[1] as well
as on our own user profile page.

1. https://designpatterns.hackpad.com/Check-your-answers-page-2DSpTH9J0wU

app/templates/views/service-settings.html

@@ -1,5 +1,6 @@
 {% extends "withnav_template.html" %}
 {% from "components/browse-list.html" import browse_list %}
+{% from "components/table.html" import mapping_table, row, text_field, link_field %}
 
 {% block page_title %}
   Settings – GOV.UK Notify
@@ -9,34 +10,43 @@
 
     <h1 class="heading-large">Settings</h1>
 
-    {{ browse_list([
-      {
-        'title': 'Change your service name',
-        'link': url_for('.service_name_change', service_id=current_service.id)
-      },
-      {
-        'title': 'Set email reply to address',
-        'link': url_for('.service_set_reply_to_email', service_id=current_service.id)
-      },
-      {
-        'title': 'Set text message sender name',
-        'link': url_for('.service_set_sms_sender', service_id=current_service.id)
-      },
-      {
-        'title': 'Request to go live and turn off trial mode',
-        'link': url_for('.service_request_to_go_live', service_id=current_service.id),
-        'hint': 'A live service can send notifications to any phone number or email address',
-      } if current_service.restricted else {
-      },
-      {
-        'title': 'Temporarily suspend API keys',
-        'link': url_for('.service_status_change', service_id=current_service.id),
-        'destructive': True
-      } if not current_service.active else {
-        'title': 'Reactivate API keys',
-        'link': url_for('.service_status_change', service_id=current_service.id)
-      }
-    ]) }}
+    {% call mapping_table(
+      caption='Settings',
+      field_headings=['Label', 'Value', 'Action'],
+      field_headings_visible=False,
+      caption_visible=False
+    ) %}
+      {% call row() %}
+        {{ text_field('Service name' )}}
+        {{ text_field(current_service.name) }}
+        {{ link_field('Change', url_for('.service_name_change', service_id=current_service.id)) }}
+      {% endcall %}
+      {% call row() %}
+        {{ text_field('Email reply to address')}}
+        {{ text_field(current_service.reply_to_email_address or '{}@notifications.service.gov.uk'.format(current_service.email_from)) }}
+        {{ link_field('Change', url_for('.service_set_reply_to_email', service_id=current_service.id)) }}
+      {% endcall %}
+      {% call row() %}
+        {{ text_field('Text message sender')}}
+        {{ text_field(current_service.sms_sender or '40604') }}
+        {{ link_field('Change', url_for('.service_set_sms_sender', service_id=current_service.id)) }}
+      {% endcall %}
+      {% call row() %}
+        {{ text_field('Mode')}}
+        {% if current_service.restricted %}
+          {{ text_field('Trial') }}
+          {{ link_field('Go live', url_for('.service_request_to_go_live', service_id=current_service.id)) }}
+        {% else %}
+          {{ text_field('Live') }}
+          {{ link_field() }}
+        {% endif %}
+      {% endcall %}
+      {% call row() %}
+        {{ text_field('Active')}}
+        {{ text_field(current_service.active) }}
+        {{ link_field('Suspend', url_for('.service_status_change', service_id=current_service.id)) }}
+      {% endcall %}
+    {% endcall %}
 
     {% if current_user.has_permissions([], admin_override=True) %}
 

tests/app/main/views/test_service_settings.py

@@ -9,19 +9,47 @@ from unittest.mock import ANY, Mock
 from werkzeug.exceptions import InternalServerError
 
 
-def test_should_show_overview(app_,
-                              active_user_with_permissions,
-                              mocker,
-                              service_one):
-    with app_.test_request_context():
-        with app_.test_client() as client:
-            client.login(active_user_with_permissions, mocker, service_one)
-            response = client.get(url_for(
-                'main.service_settings', service_id=service_one['id']))
-        assert response.status_code == 200
-        resp_data = response.get_data(as_text=True)
-        assert 'Settings' in resp_data
-        app.service_api_client.get_service.assert_called_with(service_one['id'])
+def test_should_show_overview(
+    app_,
+    active_user_with_permissions,
+    mocker,
+    service_one
+):
+    with app_.test_request_context(), app_.test_client() as client:
+        client.login(active_user_with_permissions, mocker, service_one)
+        response = client.get(url_for(
+            'main.service_settings', service_id=service_one['id']
+        ))
+    assert response.status_code == 200
+    page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
+    assert page.find('h1').text == 'Settings'
+    for index, row in enumerate([
+        'Service name service one Change',
+        'Email reply to address None Change',
+        'Text message sender 40604 Change'
+    ]):
+        assert row == " ".join(page.find_all('tr')[index + 1].text.split())
+    app.service_api_client.get_service.assert_called_with(service_one['id'])
+
+
+def test_should_show_overview_for_service_with_more_things_set(
+    app_,
+    active_user_with_permissions,
+    mocker,
+    service_with_reply_to_addresses
+):
+    with app_.test_request_context(), app_.test_client() as client:
+        client.login(active_user_with_permissions, mocker, service_with_reply_to_addresses)
+        response = client.get(url_for(
+            'main.service_settings', service_id=service_with_reply_to_addresses['id']
+        ))
+    page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
+    for index, row in enumerate([
+        'Service name service one Change',
+        'Email reply to address test@example.com Change',
+        'Text message sender elevenchars Change'
+    ]):
+        assert row == " ".join(page.find_all('tr')[index + 1].text.split())
 
 
 def test_should_show_service_name(app_,

tests/conftest.py

@@ -43,6 +43,17 @@ def service_one(api_user_active):
     return service_json(SERVICE_ONE_ID, 'service one', [api_user_active.id])
 
 
+@pytest.fixture(scope='function')
+def service_with_reply_to_addresses(api_user_active):
+    return service_json(
+        SERVICE_ONE_ID,
+        'service one',
+        [api_user_active.id],
+        reply_to_email_address='test@example.com',
+        sms_sender='elevenchars',
+    )
+
+
 @pytest.fixture(scope='function')
 def mock_send_sms(request, mocker):
     return mocker.patch("app.service_api_client.send_sms")