From 05b1292e9d8c13b3ceaec8651458447d5c997a36 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 24 Jul 2017 14:26:26 +0100 Subject: [PATCH 1/7] Make text black in error/danger banners For consistency with what GOV.UK Elements does, the text in our banners should be black, not red. See examples here: http://govuk-elements.herokuapp.com/errors/#summarise-errors This also makes us consistent with what Pay are doing. --- app/assets/stylesheets/components/banner.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/stylesheets/components/banner.scss b/app/assets/stylesheets/components/banner.scss index 3422f7fd3..f8cd258b6 100644 --- a/app/assets/stylesheets/components/banner.scss +++ b/app/assets/stylesheets/components/banner.scss @@ -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; From c15491974d31c2494981d21c0cbc042b215a87ea Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 24 Jul 2017 16:19:09 +0100 Subject: [PATCH 2/7] Use black text with a border for default banner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We use panels with a blue banner to indicate something that’s clickable. So we should move away from this style for things that are just notifications. We can’t use teal like other bits of GOV.UK because it doesn’t pass colour contrast. Pay are using a box with a green border, similar to the error validation box (which has a red border). So let’s do the same for now. --- app/assets/stylesheets/components/banner.scss | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/assets/stylesheets/components/banner.scss b/app/assets/stylesheets/components/banner.scss index f8cd258b6..ac610bf76 100644 --- a/app/assets/stylesheets/components/banner.scss +++ b/app/assets/stylesheets/components/banner.scss @@ -3,14 +3,14 @@ .banner-default { @include core-19; - background: $govuk-blue; - color: $white; + color: $text-colour; 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; From d51ffe6b39f4b239142baf8b09514c75fb6a7ff0 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 24 Jul 2017 16:19:28 +0100 Subject: [PATCH 3/7] Use yellow warning banner for permissions message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The green bordered banner feels too much like ‘success’ or ‘confirmation’. Doesn’t feel like it’s something which just gives you the status of a thing, or here’s a thing you should be aware of. --- app/assets/stylesheets/components/banner.scss | 2 +- app/templates/views/dashboard/no-permissions-banner.html | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/app/assets/stylesheets/components/banner.scss b/app/assets/stylesheets/components/banner.scss index ac610bf76..92837983a 100644 --- a/app/assets/stylesheets/components/banner.scss +++ b/app/assets/stylesheets/components/banner.scss @@ -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; diff --git a/app/templates/views/dashboard/no-permissions-banner.html b/app/templates/views/dashboard/no-permissions-banner.html index c51a0c61e..fdc8ad68c 100644 --- a/app/templates/views/dashboard/no-permissions-banner.html +++ b/app/templates/views/dashboard/no-permissions-banner.html @@ -1,5 +1,6 @@ {% from "components/banner.html" import banner_wrapper %} -{% call banner_wrapper(type="default") %} - You only have permission to view this service +{% call banner_wrapper(type="warning") %} + You only have permission to view this service. To send messages, edit + templates or manage team members, contact the person who invited you. {% endcall %} From 40e79c6827ecc541e7de84e571d605e18b332b03 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 24 Jul 2017 15:36:38 +0100 Subject: [PATCH 4/7] Use confirmation banner for revoking API keys Currently revoking an API key takes you to a separate page. It should work the same way as other destructive actions, ie staying on the same page but with a banner asking you to confirm the action. --- app/main/views/api_keys.py | 5 ++-- app/templates/views/api/keys.html | 17 +++++++++++++ app/templates/views/api/keys/revoke.html | 31 ------------------------ tests/app/main/views/test_api_keys.py | 29 +++++++++++++++------- 4 files changed, 40 insertions(+), 42 deletions(-) delete mode 100644 app/templates/views/api/keys/revoke.html diff --git a/app/main/views/api_keys.py b/app/main/views/api_keys.py index ee832bfb3..d12cf3629 100644 --- a/app/main/views/api_keys.py +++ b/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', - key_name=key_name + 'views/api/keys.html', + 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) diff --git a/app/templates/views/api/keys.html b/app/templates/views/api/keys.html index 44bbdf177..8293d31cc 100644 --- a/app/templates/views/api/keys.html +++ b/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,6 +10,22 @@ {% block maincolumn_content %} + {% if revoke_key %} +
+ {% call banner_wrapper(type='dangerous', subhead='Are you sure you want to revoke this API key?') %} +

+ ‘{{ revoke_key }}’ will no longer let you connect to GOV.UK Notify. +

+
+ + +
+ {% endcall %} +
+ {% else %} + + {% endif %} +

diff --git a/app/templates/views/api/keys/revoke.html b/app/templates/views/api/keys/revoke.html deleted file mode 100644 index 005c91d4b..000000000 --- a/app/templates/views/api/keys/revoke.html +++ /dev/null @@ -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 %} - -

- Revoke API key -

- -

- ‘{{ key_name }}’ will no longer let you connect to GOV.UK Notify. -

-

- You can’t undo this. -

- -
- {{ 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 - ) }} -
- -{% endblock %} diff --git a/tests/app/main/views/test_api_keys.py b/tests/app/main/views/test_api_keys.py index a3c809e67..caceb457c 100644 --- a/tests/app/main/views/test_api_keys.py +++ b/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, - api_user_active, - mock_login, + client_request, 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)) - assert response.status_code == 200 - assert 'some key name' in response.get_data(as_text=True) - mock_get_api_keys.assert_called_once_with(service_id=fake_uuid, key_id=fake_uuid) + page = client_request.get( + 'main.revoke_api_key', service_id=SERVICE_ONE_ID, key_id=fake_uuid, + _test_page_title=False, + ) + 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( From befe93ec0b8062d97d76fa97c70dfc46366ebd2d Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 24 Jul 2017 14:50:56 +0100 Subject: [PATCH 5/7] Make sure confirmation/danger banners have a H1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This makes errors on all pages have a `

` element, which is important for accessibility. It means a bit of rewriting the messages, but I think they’re better for it. --- app/main/views/templates.py | 14 +++-- app/templates/components/banner.html | 2 +- app/templates/views/templates/template.html | 14 +++++ tests/app/main/views/test_templates.py | 70 +++++++++------------ 4 files changed, 53 insertions(+), 47 deletions(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index f2b0f5727..180c4fc5e 100644 --- a/app/main/views/templates.py +++ b/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( - last_used_notification['template']['name'], + message = 'It was last used {} ago.'.format( 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 = 'It’s never been used.'.format(template['name']) 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, diff --git a/app/templates/components/banner.html b/app/templates/components/banner.html index a21984b25..d797525d4 100644 --- a/app/templates/components/banner.html +++ b/app/templates/components/banner.html @@ -7,7 +7,7 @@ {% endif %} > {% if subhead -%} - {{ subhead }}  +

{{ subhead }}

{%- endif -%} {{ body }} {% if delete_button %} diff --git a/app/templates/views/templates/template.html b/app/templates/views/templates/template.html index fb98e8a90..a2638582d 100644 --- a/app/templates/views/templates/template.html +++ b/app/templates/views/templates/template.html @@ -29,6 +29,20 @@
{% endif %} + {% if template_delete_confirmation_message %} +
+ {% call banner_wrapper(type='dangerous', subhead=template_delete_confirmation_message[0]) %} +

+ {{ template_delete_confirmation_message[1] }} +

+
+ + +
+ {% endcall %} +
+ {% endif %} +

{{ template.name }}

diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index dfc9f8df8..5c8fe8b44 100644 --- a/tests/app/main/views/test_templates.py +++ b/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, - api_user_active, - mock_login, - mock_get_service, + client_request, 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 - template_id = fake_uuid - response = logged_in_client.get(url_for( + page = client_request.get( '.delete_service_template', - service_id=service_id, - template_id=template_id)) - content = response.get_data(as_text=True) - 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 'Are you sure' in content - assert 'Two week reminder' in content - assert 'Template <em>content</em> with & entity' in content - mock_get_service_template.assert_called_with(service_id, template_id) + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + _test_page_title=False, + ) + assert page.h1.text == 'Are you sure you want to delete Two week reminder?' + assert normalize_spaces(page.select('.banner-dangerous p')[0].text) == ( + 'It was last used 10 minutes ago.' + ) + assert normalize_spaces(page.select('.sms-message-wrapper')[0].text) == ( + 'service one: Template content 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, - api_user_active, - mock_login, - mock_get_service, + client_request, mock_get_service_template, - mock_get_user, - mock_get_user_by_email, - mock_has_permissions, fake_uuid, mocker, ): @@ -729,20 +718,20 @@ 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 - template_id = fake_uuid - response = logged_in_client.get(url_for( + page = client_request.get( '.delete_service_template', - service_id=service_id, - template_id=template_id)) - - content = response.get_data(as_text=True) - assert response.status_code == 200 - assert 'Two week reminder has never been used. Are you sure you want to delete it?' in content - assert 'Are you sure' in content - assert 'Two week reminder' in content - assert 'Template <em>content</em> with & entity' in content - mock_get_service_template.assert_called_with(service_id, template_id) + service_id=SERVICE_ONE_ID, + template_id=fake_uuid, + _test_page_title=False, + ) + assert page.h1.text == 'Are you sure you want to delete Two week reminder?' + assert normalize_spaces(page.select('.banner-dangerous p')[0].text) == ( + 'It’s never been used.' + ) + assert normalize_spaces(page.select('.sms-message-wrapper')[0].text) == ( + 'service one: Template content with & entity' + ) + mock_get_service_template.assert_called_with(SERVICE_ONE_ID, fake_uuid) def test_should_redirect_when_deleting_a_template( @@ -1094,6 +1083,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 ( From e2bd2e0439cbacb957004839fa1591fbf8946869 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 24 Jul 2017 14:52:52 +0100 Subject: [PATCH 6/7] Ensure only one `

` per page, even with errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A page should have only one `

` element. So if there’s an error message, which contains a `

`, it should replace the page’s normal `

` element, rather than sit above it. --- app/templates/views/api/keys.html | 21 ++++++++++----------- app/templates/views/templates/template.html | 8 +++----- 2 files changed, 13 insertions(+), 16 deletions(-) diff --git a/app/templates/views/api/keys.html b/app/templates/views/api/keys.html index 8293d31cc..95d797331 100644 --- a/app/templates/views/api/keys.html +++ b/app/templates/views/api/keys.html @@ -23,19 +23,18 @@ {% endcall %}

{% else %} - +
+
+

+ API keys +

+
+
+ Create an API key +
+
{% endif %} -
-
-

- API keys -

-
-
- Create an API key -
-
{% call(item, row_number) list_table( keys, diff --git a/app/templates/views/templates/template.html b/app/templates/views/templates/template.html index a2638582d..95ea73c83 100644 --- a/app/templates/views/templates/template.html +++ b/app/templates/views/templates/template.html @@ -27,9 +27,7 @@ {% endcall %}
- {% endif %} - - {% if template_delete_confirmation_message %} + {% elif template_delete_confirmation_message %}
{% call banner_wrapper(type='dangerous', subhead=template_delete_confirmation_message[0]) %}

@@ -41,10 +39,10 @@ {% endcall %}

+ {% else %} +

{{ template.name }}

{% endif %} -

{{ template.name }}

-
{% with show_title=False, expanded=True %} {% include 'views/templates/_template.html' %} From 17a7f5f2c6c1878db18d7ab1074e5acd7bc16f66 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 24 Jul 2017 14:55:39 +0100 Subject: [PATCH 7/7] =?UTF-8?q?Remove=20last=20used=20message=20if=20key?= =?UTF-8?q?=E2=80=99s=20never=20been=20used?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I don’t think it adds anything to tell you that a key’s never been used. The value of the ‘key was used 3 minutes ago’ message is in stopping you accidentally revoking something you shouldn’t have. --- app/main/views/templates.py | 2 +- app/templates/views/templates/template.html | 8 +++++--- tests/app/main/views/test_templates.py | 4 +--- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 180c4fc5e..892f570d6 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -436,7 +436,7 @@ def delete_service_template(service_id, template_id): ) except HTTPError as e: if e.status_code == 404: - message = 'It’s never been used.'.format(template['name']) + message = None else: raise e diff --git a/app/templates/views/templates/template.html b/app/templates/views/templates/template.html index 95ea73c83..66142db28 100644 --- a/app/templates/views/templates/template.html +++ b/app/templates/views/templates/template.html @@ -30,9 +30,11 @@ {% elif template_delete_confirmation_message %}
{% call banner_wrapper(type='dangerous', subhead=template_delete_confirmation_message[0]) %} -

- {{ template_delete_confirmation_message[1] }} -

+ {% if template_delete_confirmation_message[1] %} +

+ {{ template_delete_confirmation_message[1] }} +

+ {% endif %}
diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index 5c8fe8b44..e2cb7aaa3 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -725,9 +725,7 @@ def test_should_show_delete_template_page_with_never_used_block( _test_page_title=False, ) assert page.h1.text == 'Are you sure you want to delete Two week reminder?' - assert normalize_spaces(page.select('.banner-dangerous p')[0].text) == ( - 'It’s never been used.' - ) + assert not page.select('.banner-dangerous p') assert normalize_spaces(page.select('.sms-message-wrapper')[0].text) == ( 'service one: Template content with & entity' )