Merge pull request #1379 from alphagov/consistent-banners

Make banners more consistent

app/assets/stylesheets/components/banner.scss

@@ -3,14 +3,14 @@
 .banner-default {
 
   @include core-19;
-  background: $govuk-blue;
+  color: $text-colour;
-  color: $white;
   display: block;
   padding: $gutter-half;
   margin: $gutter-half 0 $gutter 0;
   text-align: left;
   position: relative;
   clear: both;
+  border: 5px solid $button-colour;
 
   &-title {
     @include bold-24;
@@ -29,9 +29,9 @@
 %banner-with-tick,
 .banner-with-tick {
   padding: $gutter-half ($gutter + $gutter-half);
-  background-image: file-url('tick-white.png');
+  background-image: file-url('tick.png');
   @include ie-lte(8) {
-    background-image: file-url('tick-white-16px.png');
+    background-image: file-url('tick-16px.png');
   }
   background-size: 19px;
   background-repeat: no-repeat;
@@ -49,7 +49,7 @@
   @extend %banner;
   @include bold-19;
   background: $white;
-  color: $error-colour;
+  color: $text-colour;
   border: 5px solid $error-colour;
   margin: 15px 0;
   text-align: left;
@@ -160,7 +160,7 @@
   background: $yellow;
   color: $text-colour;
   border: 5px solid $text-colour;
-  margin: $gutter 0 $gutter 0;
+  margin: $gutter-half 0 $gutter 0;
   text-align: left;
   padding: 20px;
 

app/main/views/api_keys.py

@@ -110,8 +110,9 @@ def revoke_api_key(service_id, key_id):
     key_name = api_key_api_client.get_api_keys(service_id=service_id, key_id=key_id)['apiKeys'][0]['name']
     if request.method == 'GET':
         return render_template(
-            'views/api/keys/revoke.html',
+            'views/api/keys.html',
-            key_name=key_name
+            revoke_key=key_name,
+            keys=api_key_api_client.get_api_keys(service_id=service_id)['apiKeys'],
         )
     elif request.method == 'POST':
         api_key_api_client.revoke_api_key(service_id=service_id, key_id=key_id)

app/main/views/templates.py

@@ -428,22 +428,24 @@ def delete_service_template(service_id, template_id):
         last_used_notification = template_statistics_client.get_template_statistics_for_template(
             service_id, template['id']
         )
-        message = '{} was last used {} ago'.format(
+        message = 'It was last used {} ago.'.format(
-            last_used_notification['template']['name'],
             get_human_readable_delta(
                 parse(last_used_notification['created_at']).replace(tzinfo=None),
-                datetime.utcnow())
+                datetime.utcnow()
+            )
         )
     except HTTPError as e:
         if e.status_code == 404:
-            message = '{} has never been used'.format(template['name'])
+            message = None
         else:
             raise e
 
-    flash('{}. Are you sure you want to delete it?'.format(message), 'delete')
-
     return render_template(
         'views/templates/template.html',
+        template_delete_confirmation_message=(
+            'Are you sure you want to delete {}?'.format(template['name']),
+            message,
+        ),
         template=get_template(
             template,
             current_service,

app/templates/components/banner.html

@@ -7,7 +7,7 @@
     {% endif %}
   >
     {% if subhead -%}
-        {{ subhead }} 
+      <h1 class="banner-title">{{ subhead }}</h1>
     {%- endif -%}
     {{ body }}
     {% if delete_button %}

app/templates/views/api/keys.html

@@ -1,4 +1,5 @@
 {% extends "withnav_template.html" %}
+{% from "components/banner.html" import banner_wrapper %}
 {% from "components/table.html" import list_table, field, hidden_field_heading %}
 {% from "components/api-key.html" import api_key %}
 {% from "components/page-footer.html" import page_footer %}
@@ -9,16 +10,31 @@
 
 {% block maincolumn_content %}
 
-  <div class="grid-row">
+  {% if revoke_key %}
-    <div class="column-two-thirds">
+    <div class="bottom-gutter">
-      <h1 class="heading-large">
+      {% call banner_wrapper(type='dangerous', subhead='Are you sure you want to revoke this API key?') %}
-        API keys
+        <p>
-      </h1>
+          ‘{{ revoke_key }}’ will no longer let you connect to GOV.UK Notify.
+        </p>
+        <form method='post'>
+          <input type="hidden" name="csrf_token" value="{{ csrf_token() }}" />
+          <input type="submit" class="button" name="delete" value="Confirm" />
+        </form>
+      {% endcall %}
     </div>
-    <div class="column-one-third">
+  {% else %}
-      <a href="{{ url_for('.create_api_key', service_id=current_service.id) }}" class="button align-with-heading">Create an API key</a>
+    <div class="grid-row">
+      <div class="column-two-thirds">
+        <h1 class="heading-large">
+          API keys
+        </h1>
+      </div>
+      <div class="column-one-third">
+        <a href="{{ url_for('.create_api_key', service_id=current_service.id) }}" class="button align-with-heading">Create an API key</a>
+      </div>
     </div>
-  </div>
+  {% endif %}
+
   <div class="body-copy-table">
     {% call(item, row_number) list_table(
       keys,

app/templates/views/api/keys/revoke.html

@@ -1,31 +0,0 @@
-{% extends "withnav_template.html" %}
-{% from "components/page-footer.html" import page_footer %}
-{% from "components/api-key.html" import api_key %}
-
-{% block service_page_title %}
-  Revoke API key
-{% endblock %}
-
-{% block maincolumn_content %}
-
-    <h1 class="heading-large">
-      Revoke API key
-    </h1>
-
-    <p>
-      ‘{{ key_name }}’ will no longer let you connect to GOV.UK Notify.
-    </p>
-    <p>
-      You can’t undo this.
-    </p>
-
-    <form method="post">
-      {{ page_footer(
-        'Revoke this API key',
-        back_link=url_for('.api_keys', service_id=current_service.id),
-        back_link_text='Back to API keys',
-        destructive=True
-      ) }}
-    </form>
-
-{% endblock %}

app/templates/views/dashboard/no-permissions-banner.html

@@ -1,5 +1,6 @@
 {% from "components/banner.html" import banner_wrapper %}
 
-{% call banner_wrapper(type="default") %}
+{% call banner_wrapper(type="warning") %}
-  You only have permission to view this service
+  You only have permission to view this service. To send messages, edit
+  templates or manage team members, contact the person who invited you.
 {% endcall %}

app/templates/views/templates/template.html

@@ -27,10 +27,24 @@
         </form>
       {% endcall %}
     </div>
+  {% elif template_delete_confirmation_message %}
+    <div class="bottom-gutter">
+      {% call banner_wrapper(type='dangerous', subhead=template_delete_confirmation_message[0]) %}
+        {% if template_delete_confirmation_message[1] %}
+          <p>
+            {{ template_delete_confirmation_message[1] }}
+          </p>
+        {% endif %}
+        <form method='post'>
+          <input type="hidden" name="csrf_token" value="{{ csrf_token() }}" />
+          <input type="submit" class="button" name="delete" value="Confirm" />
+        </form>
+      {% endcall %}
+    </div>
+  {% else %}
+    <h1 class="heading-large">{{ template.name }}</h1>
   {% endif %}
 
-  <h1 class="heading-large">{{ template.name }}</h1>
-
   <div class="grid-row">
     {% with show_title=False, expanded=True %}
       {% include 'views/templates/_template.html' %}

tests/app/main/views/test_api_keys.py

@@ -4,8 +4,10 @@ from collections import OrderedDict
 import pytest
 from flask import url_for
 from bs4 import BeautifulSoup
+from unittest.mock import call
 
 from tests import validate_route_permission
+from tests.conftest import normalize_spaces, SERVICE_ONE_ID
 
 
 def test_should_show_api_page(
@@ -196,18 +198,27 @@ def test_cant_create_normal_api_key_in_trial_mode(
 
 
 def test_should_show_confirm_revoke_api_key(
-    logged_in_client,
+    client_request,
-    api_user_active,
-    mock_login,
     mock_get_api_keys,
-    mock_get_service,
-    mock_has_permissions,
     fake_uuid,
 ):
-    response = logged_in_client.get(url_for('main.revoke_api_key', service_id=fake_uuid, key_id=fake_uuid))
+    page = client_request.get(
-    assert response.status_code == 200
+        'main.revoke_api_key', service_id=SERVICE_ONE_ID, key_id=fake_uuid,
-    assert 'some key name' in response.get_data(as_text=True)
+        _test_page_title=False,
-    mock_get_api_keys.assert_called_once_with(service_id=fake_uuid, key_id=fake_uuid)
+    )
+    assert normalize_spaces(page.select('.banner-dangerous')[0].text) == (
+        'Are you sure you want to revoke this API key? '
+        '‘some key name’ will no longer let you connect to GOV.UK Notify.'
+    )
+    assert mock_get_api_keys.call_args_list == [
+        call(
+            key_id=fake_uuid,
+            service_id='596364a0-858e-42c8-9062-a8fe822260eb',
+        ),
+        call(
+            service_id='596364a0-858e-42c8-9062-a8fe822260eb'
+        ),
+    ]
 
 
 def test_should_redirect_after_revoking_api_key(

tests/app/main/views/test_templates.py

@@ -679,16 +679,10 @@ def test_should_redirect_when_saving_a_template_email(
 
 
 def test_should_show_delete_template_page_with_time_block(
-    logged_in_client,
+    client_request,
-    api_user_active,
-    mock_login,
-    mock_get_service,
     mock_get_service_template,
-    mock_get_user,
-    mock_get_user_by_email,
-    mock_has_permissions,
-    fake_uuid,
     mocker,
+    fake_uuid
 ):
     with freeze_time('2012-01-01 12:00:00'):
         template = template_json('1234', '1234', "Test template", "sms", "Something very interesting")
@@ -698,30 +692,25 @@ def test_should_show_delete_template_page_with_time_block(
                      return_value=notification)
 
     with freeze_time('2012-01-01 12:10:00'):
-        service_id = fake_uuid
+        page = client_request.get(
-        template_id = fake_uuid
-        response = logged_in_client.get(url_for(
             '.delete_service_template',
-            service_id=service_id,
+            service_id=SERVICE_ONE_ID,
-            template_id=template_id))
+            template_id=fake_uuid,
-    content = response.get_data(as_text=True)
+            _test_page_title=False,
-    assert response.status_code == 200
+        )
-    assert 'Test template was last used 10 minutes ago. Are you sure you want to delete it?' in content
+    assert page.h1.text == 'Are you sure you want to delete Two week reminder?'
-    assert 'Are you sure' in content
+    assert normalize_spaces(page.select('.banner-dangerous p')[0].text) == (
-    assert 'Two week reminder' in content
+        'It was last used 10 minutes ago.'
-    assert 'Template <em>content</em> with & entity' in content
+    )
-    mock_get_service_template.assert_called_with(service_id, template_id)
+    assert normalize_spaces(page.select('.sms-message-wrapper')[0].text) == (
+        'service one: Template <em>content</em> with & entity'
+    )
+    mock_get_service_template.assert_called_with(SERVICE_ONE_ID, fake_uuid)
 
 
 def test_should_show_delete_template_page_with_never_used_block(
-    logged_in_client,
+    client_request,
-    api_user_active,
-    mock_login,
-    mock_get_service,
     mock_get_service_template,
-    mock_get_user,
-    mock_get_user_by_email,
-    mock_has_permissions,
     fake_uuid,
     mocker,
 ):
@@ -729,20 +718,18 @@ def test_should_show_delete_template_page_with_never_used_block(
         'app.template_statistics_client.get_template_statistics_for_template',
         side_effect=HTTPError(response=Mock(status_code=404), message="Default message")
     )
-    service_id = fake_uuid
+    page = client_request.get(
-    template_id = fake_uuid
-    response = logged_in_client.get(url_for(
         '.delete_service_template',
-        service_id=service_id,
+        service_id=SERVICE_ONE_ID,
-        template_id=template_id))
+        template_id=fake_uuid,
-
+        _test_page_title=False,
-    content = response.get_data(as_text=True)
+    )
-    assert response.status_code == 200
+    assert page.h1.text == 'Are you sure you want to delete Two week reminder?'
-    assert 'Two week reminder has never been used. Are you sure you want to delete it?' in content
+    assert not page.select('.banner-dangerous p')
-    assert 'Are you sure' in content
+    assert normalize_spaces(page.select('.sms-message-wrapper')[0].text) == (
-    assert 'Two week reminder' in content
+        'service one: Template <em>content</em> with & entity'
-    assert 'Template &lt;em&gt;content&lt;/em&gt; with &amp; entity' in content
+    )
-    mock_get_service_template.assert_called_with(service_id, template_id)
+    mock_get_service_template.assert_called_with(SERVICE_ONE_ID, fake_uuid)
 
 
 def test_should_redirect_when_deleting_a_template(
@@ -1094,6 +1081,7 @@ def test_should_show_message_before_redacting_template(
         'main.redact_template',
         service_id=SERVICE_ONE_ID,
         template_id=fake_uuid,
+        _test_page_title=False,
     )
 
     assert (