Merge pull request #2931 from alphagov/rationalise-settings

Allow more space for values in settings tables

app/assets/stylesheets/components/table.scss

@@ -51,6 +51,48 @@
 
 }
 
+.settings-table {
+
+  table {
+    table-layout: fixed;
+  }
+
+  th {
+
+    &:first-child {
+      width: 35%;  // 33.33% + fudge
+    }
+
+    &:last-child {
+      width: 17.5%  // 16.67% + fudge
+    }
+
+  }
+
+  td.table-field-left-aligned {
+
+    &:first-child {
+
+      div {
+        white-space: normal;
+      }
+
+    }
+
+    div {
+      white-space: nowrap;
+      overflow: hidden;
+      text-overflow: ellipsis;
+    }
+
+    ul li {
+      margin-bottom: 5px;
+    }
+
+  }
+
+}
+
 %table-field,
 .table-field {
 

app/templates/components/table.html

@@ -67,7 +67,7 @@
 
 {% macro field(align='left', status='', border=True) -%}
 
-    {% set field_alignment = 'table-field-right-aligned' if align == 'right' else 'table-field-center-aligned' %}
+    {% set field_alignment = 'table-field-right-aligned' if align == 'right' else 'table-field-left-aligned' %}
     {% set border = '' if border else 'table-field-noborder' %}
 
     <td class="{{ [field_alignment, border]|join(' ') }}">
@@ -90,7 +90,7 @@
 {% macro text_field(text, status='', truncate=false) -%}
   {% call field(status=status) %}
     {% if text is iterable and text is not string %}
-      <ul class="list list-bullet">
+      <ul>
         {% for item in text %}
           {% if item %}
             <li>{{ item }}</li>

app/templates/views/organisations/organisation/settings/index.html

@@ -29,7 +29,7 @@
 
   {% if current_user.platform_admin %}
     <h2 class="heading-medium">Platform admin settings</h2>
-    <div class="bottom-gutter-3-2 dashboard-table body-copy-table">
+    <div class="settings-table body-copy-table">
       {% call mapping_table(
         caption='Platform admin settings',
         field_headings=['Label', 'Value', 'Action'],

app/templates/views/service-settings.html

@@ -10,7 +10,7 @@
 
     <h1 class="heading-large">Settings</h1>
 
-    <div class="bottom-gutter-3-2 dashboard-table body-copy-table">
+    <div class="bottom-gutter-3-2 settings-table body-copy-table">
 
       {% call mapping_table(
         caption='General',
@@ -278,98 +278,105 @@
 
     {% if current_user.platform_admin %}
 
-      <h2 class="heading-medium">Platform admin settings</h2>
+      <div class="settings-table body-copy-table top-gutter-4-3">
-      {% call mapping_table(
-        caption='Settings',
-        field_headings=['Label', 'Value', 'Action'],
-        field_headings_visible=False,
-        caption_visible=False
-      ) %}
 
-        {% call row() %}
+        <h2 class="heading-medium">Platform admin settings</h2>
-          {{ text_field('Live')}}
-          {% if current_service.trial_mode and not current_service.organisation %}
-            {{ text_field('No (you need to assign this service to an organisation before you can make it live)') }}
-            {{ text_field('') }}
-          {% else %}
-            {{ boolean_field(not current_service.trial_mode) }}
-            {{ edit_field('Change', url_for('.service_switch_live', service_id=current_service.id)) }}
-          {% endif %}
-        {% endcall %}
 
-        {% call row() %}
+        {% call mapping_table(
-          {{ text_field('Count in list of live services')}}
+          caption='Settings',
-          {{ text_field('Yes' if current_service.count_as_live else 'No') }}
+          field_headings=['Label', 'Value', 'Action'],
-          {{ edit_field('Change', url_for('.service_switch_count_as_live', service_id=current_service.id)) }}
+          field_headings_visible=False,
-        {% endcall %}
+          caption_visible=False
+        ) %}
 
-        {% call row() %}
+          {% call row() %}
-          {{ text_field('Organisation')}}
+            {{ text_field('Live')}}
-          {{ optional_text_field(current_service.organisation.name) }}
+            {% if current_service.trial_mode and not current_service.organisation %}
-          {{ edit_field('Change', url_for('.link_service_to_organisation', service_id=current_service.id)) }}
+              {{ text_field('No (you need to assign this service to an organisation before you can make it live)') }}
-        {% endcall %}
+              {{ text_field('') }}
-        {% call row() %}
+            {% else %}
-          {{ text_field('Organisation type')}}
+              {{ boolean_field(not current_service.trial_mode) }}
-          {{ optional_text_field(
+              {{ edit_field('Change', url_for('.service_switch_live', service_id=current_service.id)) }}
-            (current_service.organisation_type or '')|title
+            {% endif %}
-          ) }}
-          {{ edit_field('Change', url_for('.set_organisation_type', service_id=current_service.id)) }}
-        {% endcall %}
-        {% call row() %}
-          {{ text_field('Free text message allowance')}}
-          {{ text_field('{:,}'.format(current_service.free_sms_fragment_limit)) }}
-          {{ edit_field('Change', url_for('.set_free_sms_allowance', service_id=current_service.id)) }}
-        {% endcall %}
-        {% call row() %}
-          {{ text_field('Email branding' )}}
-          {{ text_field(current_service.email_branding_name) }}
-          {{ edit_field('Change', url_for('.service_set_email_branding', service_id=current_service.id)) }}
-        {% endcall %}
-        {% call row() %}
-          {{ text_field('Letter branding')}}
-          {{ optional_text_field(current_service.letter_branding.name) }}
-          {{ edit_field('Change', url_for('.service_set_letter_branding', service_id=current_service.id)) }}
-        {% endcall %}
-        {% call row() %}
-          {{ text_field('Data retention')}}
-          {% call field() %}
-              {{ current_service.data_retention | join(', ', attribute='notification_type') }}
           {% endcall %}
-          {{ edit_field('Change', url_for('.data_retention', service_id=current_service.id)) }}
+
+          {% call row() %}
+            {{ text_field('Count in list of live services')}}
+            {{ text_field('Yes' if current_service.count_as_live else 'No') }}
+            {{ edit_field('Change', url_for('.service_switch_count_as_live', service_id=current_service.id)) }}
+          {% endcall %}
+
+          {% call row() %}
+            {{ text_field('Organisation')}}
+            {{ optional_text_field(current_service.organisation.name) }}
+            {{ edit_field('Change', url_for('.link_service_to_organisation', service_id=current_service.id)) }}
+          {% endcall %}
+          {% call row() %}
+            {{ text_field('Organisation type')}}
+            {{ optional_text_field(
+              (current_service.organisation_type or '')|title
+            ) }}
+            {{ edit_field('Change', url_for('.set_organisation_type', service_id=current_service.id)) }}
+          {% endcall %}
+          {% call row() %}
+            {{ text_field('Free text message allowance')}}
+            {{ text_field('{:,}'.format(current_service.free_sms_fragment_limit)) }}
+            {{ edit_field('Change', url_for('.set_free_sms_allowance', service_id=current_service.id)) }}
+          {% endcall %}
+          {% call row() %}
+            {{ text_field('Email branding' )}}
+            {{ text_field(current_service.email_branding_name) }}
+            {{ edit_field('Change', url_for('.service_set_email_branding', service_id=current_service.id)) }}
+          {% endcall %}
+          {% call row() %}
+            {{ text_field('Letter branding')}}
+            {{ optional_text_field(current_service.letter_branding.name) }}
+            {{ edit_field('Change', url_for('.service_set_letter_branding', service_id=current_service.id)) }}
+          {% endcall %}
+          {% call row() %}
+            {{ text_field('Data retention')}}
+            {% call field() %}
+                {{ current_service.data_retention | join(', ', attribute='notification_type') }}
+            {% endcall %}
+            {{ edit_field('Change', url_for('.data_retention', service_id=current_service.id)) }}
+          {% endcall %}
+
+          {% for permission in service_permissions %}
+            {% if not service_permissions[permission].requires or current_service.has_permission(service_permissions[permission].requires) %}
+              {% call row() %}
+                {{ text_field(service_permissions[permission].title)}}
+                {{ boolean_field(current_service.has_permission(permission)) }}
+                {{ edit_field('Change', url_for(service_permissions[permission].endpoint or '.service_set_permission', service_id=current_service.id, permission=permission if not service_permissions[permission].endpoint else None)) }}
+              {% endcall %}
+            {% endif %}
+          {% endfor %}
+
         {% endcall %}
 
-        {% for permission in service_permissions %}
+        <p>
-          {% if not service_permissions[permission].requires or current_service.has_permission(service_permissions[permission].requires) %}
+          {% if current_service.active %}
-            {% call row() %}
+            <span class="page-footer-delete-link page-footer-delete-link-without-button">
-              {{ text_field(service_permissions[permission].title)}}
+              <a href="{{ url_for('.archive_service', service_id=current_service.id) }}">
-              {{ boolean_field(current_service.has_permission(permission)) }}
+                Archive service
-              {{ edit_field('Change', url_for(service_permissions[permission].endpoint or '.service_set_permission', service_id=current_service.id, permission=permission if not service_permissions[permission].endpoint else None)) }}
+              </a>
-            {% endcall %}
+            </span>
+            <span class="page-footer-delete-link">
+              <a href="{{ url_for('.suspend_service', service_id=current_service.id) }}" class="page-footer-delete-link">
+                  Suspend service
+                </a>
+            </span>
+          {% else %}
+            <div class="hint bottom-gutter-1-2">
+              Service suspended
+            </div>
+            <span class="page-footer-delete-link page-footer-delete-link-without-button">
+              <a href="{{ url_for('.resume_service', service_id=current_service.id) }}">
+                Resume service
+              </a>
+            </span>
           {% endif %}
-        {% endfor %}
+        </p>
-
+      </div>
-      {% endcall %}
-
-      <ul>
-        {% if current_service.active %}
-          <li class="bottom-gutter">
-            <a href="{{ url_for('.archive_service', service_id=current_service.id) }}" class="button">
-              Archive service
-            </a>
-          </li>
-          <li class="bottom-gutter">
-            <a href="{{ url_for('.suspend_service', service_id=current_service.id) }}" class="button">
-              Suspend service
-            </a>
-          </li>
-        {% else %}
-          <li class="bottom-gutter">
-            <a href="{{ url_for('.resume_service', service_id=current_service.id) }}" class="button">
-              Resume service
-            </a>
-          </li>
-        {% endif %}
-      </ul>
 
     {% endif %}
 

tests/app/main/views/service_settings/test_service_setting_permissions.py

@@ -92,16 +92,29 @@ def test_service_setting_toggles_show(get_service_settings_page, service_one, se
     assert normalize_spaces(page.find('a', {'href': button_url}).find_parent('tr').text.strip()) == text
 
 
-@pytest.mark.parametrize('service_fields, endpoint, kwargs, text', [
+@pytest.mark.parametrize('service_fields, endpoint, index, text', [
-    ({'active': True}, '.archive_service', {}, 'Archive service'),
+    ({'active': True}, '.archive_service', 0, 'Archive service'),
-    ({'active': True}, '.suspend_service', {}, 'Suspend service'),
+    ({'active': True}, '.suspend_service', 1, 'Suspend service'),
-    ({'active': False}, '.resume_service', {}, 'Resume service'),
+    ({'active': False}, '.resume_service', 0, 'Resume service'),
+    pytest.param(
+        {'active': False}, '.archive_service', 1, 'Resume service',
+        marks=pytest.mark.xfail(raises=IndexError)
+    )
 ])
-def test_service_setting_button_toggles(get_service_settings_page, service_one, service_fields, endpoint, kwargs, text):
+def test_service_setting_button_toggles(
-    button_url = url_for(endpoint, **kwargs, service_id=service_one['id'])
+    get_service_settings_page,
+    service_one,
+    service_fields,
+    endpoint,
+    index,
+    text,
+):
+    button_url = url_for(endpoint, service_id=service_one['id'])
     service_one.update(service_fields)
     page = get_service_settings_page()
-    assert normalize_spaces(page.find('a', {'class': 'button', 'href': button_url}).text.strip()) == text
+    link = page.select('.page-footer-delete-link a')[index]
+    assert normalize_spaces(link.text) == text
+    assert link['href'] == button_url
 
 
 @pytest.mark.parametrize('permissions,permissions_text,visible', [

tests/app/main/views/test_dashboard.py

@@ -569,7 +569,7 @@ def test_monthly_shows_letters_in_breakdown(
         service_id=service_one['id']
     )
 
-    columns = page.select('.table-field-center-aligned .big-number-label')
+    columns = page.select('.table-field-left-aligned .big-number-label')
 
     assert normalize_spaces(columns[0].text) == 'emails'
     assert normalize_spaces(columns[1].text) == 'text messages'

tests/app/main/views/test_send.py

@@ -656,12 +656,12 @@ def test_upload_valid_csv_shows_preview_and_table(
 
     for row_index, row in enumerate([
         (
-            '<td class="table-field-center-aligned"> <div class=""> 07700900001 </div> </td>',
+            '<td class="table-field-left-aligned"> <div class=""> 07700900001 </div> </td>',
-            '<td class="table-field-center-aligned"> <div class=""> A </div> </td>',
+            '<td class="table-field-left-aligned"> <div class=""> A </div> </td>',
             (
-                '<td class="table-field-center-aligned"> '
+                '<td class="table-field-left-aligned"> '
                 '<div class="table-field-status-default"> '
-                '<ul class="list list-bullet"> '
+                '<ul> '
                 '<li>foo</li> <li>foo</li> <li>foo</li> '
                 '</ul> '
                 '</div> '
@@ -669,12 +669,12 @@ def test_upload_valid_csv_shows_preview_and_table(
             )
         ),
         (
-            '<td class="table-field-center-aligned"> <div class=""> 07700900002 </div> </td>',
+            '<td class="table-field-left-aligned"> <div class=""> 07700900002 </div> </td>',
-            '<td class="table-field-center-aligned"> <div class=""> B </div> </td>',
+            '<td class="table-field-left-aligned"> <div class=""> B </div> </td>',
             (
-                '<td class="table-field-center-aligned"> '
+                '<td class="table-field-left-aligned"> '
                 '<div class="table-field-status-default"> '
-                '<ul class="list list-bullet"> '
+                '<ul> '
                 '<li>foo</li> <li>foo</li> <li>foo</li> '
                 '</ul> '
                 '</div> '
@@ -682,12 +682,12 @@ def test_upload_valid_csv_shows_preview_and_table(
             )
         ),
         (
-            '<td class="table-field-center-aligned"> <div class=""> 07700900003 </div> </td>',
+            '<td class="table-field-left-aligned"> <div class=""> 07700900003 </div> </td>',
-            '<td class="table-field-center-aligned"> <div class=""> C </div> </td>',
+            '<td class="table-field-left-aligned"> <div class=""> C </div> </td>',
             (
-                '<td class="table-field-center-aligned"> '
+                '<td class="table-field-left-aligned"> '
                 '<div class="table-field-status-default"> '
-                '<ul class="list list-bullet"> '
+                '<ul> '
                 '<li>foo</li> <li>foo</li> '
                 '</ul> '
                 '</div> '