From 101392c35062c1115b3e79a0e19c516deec1abcb Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 1 Nov 2016 17:12:25 +0000 Subject: [PATCH 1/6] Fix page title to match H1 on create API key page The page `` was using previous wording. --- app/templates/views/api/keys/create.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/templates/views/api/keys/create.html b/app/templates/views/api/keys/create.html index 474475618..8862a3e97 100644 --- a/app/templates/views/api/keys/create.html +++ b/app/templates/views/api/keys/create.html @@ -4,7 +4,7 @@ {% from "components/radios.html" import radios %} {% block page_title %} - Add a new API key – GOV.UK Notify + Create an API key – GOV.UK Notify {% endblock %} {% block maincolumn_content %} From 9ec1f21f722cb27eaa7025934a68d7b1e640120e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott <me@quis.cc> Date: Tue, 1 Nov 2016 15:29:52 +0000 Subject: [PATCH 2/6] Add a message to the whitelist The confusion about the whitelist was how it related to your team. This commit adds some text to make the relationship clearer. --- app/templates/views/api/whitelist.html | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/app/templates/views/api/whitelist.html b/app/templates/views/api/whitelist.html index 47534da1b..3e7c25b6e 100644 --- a/app/templates/views/api/whitelist.html +++ b/app/templates/views/api/whitelist.html @@ -14,6 +14,12 @@ Whitelist </h1> + <p> + You and members of + <a href="{{ url_for('main.manage_users', service_id=current_service.id) }}">your team</a> + are included in the whitelist automatically. + </p> + <form method="post"> <div class="grid-row"> From 4ae28386a63f2be5436180e68f57ea8b4ae0dd09 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott <me@quis.cc> Date: Tue, 1 Nov 2016 15:34:04 +0000 Subject: [PATCH 3/6] Always show live key, reword key labels MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is trying to resolve these confusions: - that you’re in trial mode, which means you can’t have a live key yet ( or you can but it wont work, which is what we used to have) - what does simulate mean The create key page is the right place to resolve these confusions because it’s where users are actively reading. This commit also removes the trial mode banner from API integration page because this where users _aren’t_ actively reading. A whole bunch of users weren’t seeing this banner at all. The implementation of the disabled API key options is kinda clunky because WTForms doesn’t have a native way of doing this. ¯\_(ツ)_/¯ --- app/assets/stylesheets/app.scss | 9 ++++++++ app/main/views/api_keys.py | 27 +++++++++++++++++------- app/templates/components/radios.html | 19 +++++++++++++++-- app/templates/views/api/index.html | 9 -------- app/templates/views/api/keys.html | 4 ++-- app/templates/views/api/keys/create.html | 3 ++- tests/app/main/views/test_api_keys.py | 6 +----- 7 files changed, 50 insertions(+), 27 deletions(-) diff --git a/app/assets/stylesheets/app.scss b/app/assets/stylesheets/app.scss index 8144e50ba..878d94497 100644 --- a/app/assets/stylesheets/app.scss +++ b/app/assets/stylesheets/app.scss @@ -214,3 +214,12 @@ summary::-webkit-details-marker { details .arrow { font-size: 16px; } + +.block-label-hint { + @include core-16; + margin-top: 5px; +} + +.block-label input[disabled] { + opacity: 0.5; +} diff --git a/app/main/views/api_keys.py b/app/main/views/api_keys.py index de7c65181..fd3352a54 100644 --- a/app/main/views/api_keys.py +++ b/app/main/views/api_keys.py @@ -1,4 +1,4 @@ -from flask import request, render_template, redirect, url_for, flash +from flask import request, render_template, redirect, url_for, flash, Markup, abort from flask_login import login_required from app.main import main from app.main.forms import CreateKeyForm, Whitelist @@ -68,13 +68,22 @@ def create_api_key(service_id): key['name'] for key in api_key_api_client.get_api_keys(service_id=service_id)['apiKeys'] ] form = CreateKeyForm(key_names) - form.key_type.choices = filter(None, [ - (KEY_TYPE_NORMAL, 'Send messages to anyone') - if not current_service['restricted'] else None, - (KEY_TYPE_TEST, 'Simulate sending messages to anyone'), - (KEY_TYPE_TEAM, 'Only send messages to your team or whitelist') - ]) + form.key_type.choices = [ + (KEY_TYPE_NORMAL, 'Send messages to anyone'), + (KEY_TYPE_TEAM, 'Send messages to anyone on my whitelist'), + (KEY_TYPE_TEST, 'Pretend to send messages to anyone'), + ] + if current_service['restricted']: + disabled_options = [KEY_TYPE_NORMAL] + option_hints = {KEY_TYPE_NORMAL: Markup( + 'This option is not available because your service is in ' + '<a href="{}">trial mode</a>'.format(url_for(".trial_mode")) + )} + else: + disabled_options, option_hints = [], {} if form.validate_on_submit(): + if form.key_type.data in disabled_options: + abort(400) secret = api_key_api_client.create_api_key( service_id=service_id, key_name=form.key_name.data, @@ -88,7 +97,9 @@ def create_api_key(service_id): ) return render_template( 'views/api/keys/create.html', - form=form + form=form, + disabled_options=disabled_options, + option_hints=option_hints ) diff --git a/app/templates/components/radios.html b/app/templates/components/radios.html index c361c443e..a24c49d1c 100644 --- a/app/templates/components/radios.html +++ b/app/templates/components/radios.html @@ -1,6 +1,8 @@ {% macro radios( field, - hint=None + hint=None, + disable=[], + option_hints={} ) %} <div class="form-group {% if field.errors %} error{% endif %}"> <fieldset> @@ -14,8 +16,21 @@ </legend> {% for option in field %} <label class="block-label" for="{{ option.id }}"> - {{ option }} + <input + id="{{ option.id }}" name="{{ option.name }}" type="radio" value="{{ option.data }}" + {% if option.data in disable %} + disabled + {% endif %} + {% if option.checked %} + checked + {% endif %} + > {{ option.label.text }} + {% if option_hints[option.data] %} + <div class="block-label-hint"> + {{ option_hints[option.data] }} + </div> + {% endif %} </label> {% endfor %} </fieldset> diff --git a/app/templates/views/api/index.html b/app/templates/views/api/index.html index cd650bc85..7b6144e04 100644 --- a/app/templates/views/api/index.html +++ b/app/templates/views/api/index.html @@ -13,15 +13,6 @@ API integration </h1> - {% if current_service.restricted %} - {% call banner_wrapper(type='warning') %} - <h2 class="heading-medium">Your service is in trial mode</h2> - <p> - You can only send messages to people in your team or whitelist. - </p> - {% endcall %} - {% endif %} - <nav class="grid-row bottom-gutter-1-2"> <div class="column-one-third"> <a class="pill-separate-item" href="{{ url_for('.api_keys', service_id=current_service.id) }}">API keys</a> diff --git a/app/templates/views/api/keys.html b/app/templates/views/api/keys.html index 5c67da32f..5168c5ceb 100644 --- a/app/templates/views/api/keys.html +++ b/app/templates/views/api/keys.html @@ -37,9 +37,9 @@ {% if item.key_type == 'normal' %} <span class="visually-hidden">Normal</span> {% elif item.key_type == 'team' %} - Only sends to team members or whitelist + Sends to anyone on your whitelist {% elif item.key_type == 'test' %} - Simulates sending messages + Pretends to send messages {% endif %} </span> </div> diff --git a/app/templates/views/api/keys/create.html b/app/templates/views/api/keys/create.html index 8862a3e97..936bb8a05 100644 --- a/app/templates/views/api/keys/create.html +++ b/app/templates/views/api/keys/create.html @@ -2,6 +2,7 @@ {% from "components/page-footer.html" import page_footer %} {% from "components/textbox.html" import textbox %} {% from "components/radios.html" import radios %} +{% from "components/banner.html" import banner_wrapper %} {% block page_title %} Create an API key – GOV.UK Notify @@ -14,8 +15,8 @@ </h1> <form method="post"> - {{ radios(form.key_type) }} {{ textbox(form.key_name, label='Name for this key') }} + {{ radios(form.key_type, disable=disabled_options, option_hints=option_hints) }} {{ page_footer('Continue') }} </form> diff --git a/tests/app/main/views/test_api_keys.py b/tests/app/main/views/test_api_keys.py index 0de62599f..fe45ad2d7 100644 --- a/tests/app/main/views/test_api_keys.py +++ b/tests/app/main/views/test_api_keys.py @@ -21,7 +21,6 @@ def test_should_show_api_page( assert response.status_code == 200 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') assert page.h1.string.strip() == 'API integration' - assert 'Your service is in trial mode' in page.find('div', {'class': 'banner-warning'}).text rows = page.find_all('details') assert len(rows) == 5 for index, row in enumerate(rows): @@ -201,10 +200,7 @@ def test_cant_create_normal_api_key_in_trial_mode( 'key_type': 'normal' } ) - assert response.status_code == 200 - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - assert page.find('span', {'class': 'error-message'}).text.strip() == 'Not a valid choice' - + assert response.status_code == 400 mock_post.assert_not_called() From dae10429e6aac16e51345a9ef0bbde3e6bd61687 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott <me@quis.cc> Date: Wed, 2 Nov 2016 15:46:22 +0000 Subject: [PATCH 4/6] Use radio button desc. pattern on go live page Nicer than using en dashes. --- app/main/forms.py | 4 ++-- app/templates/views/service-settings/request-to-go-live.html | 5 ++++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index a1c6db9bb..95afc47c6 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -391,8 +391,8 @@ class RequestToGoLiveForm(Form): ), choices=[ ('yes', 'Yes'), - ('no', 'No – we’ll send you a copy'), - ('don’t know', 'I don’t know – we’ll check for you') + ('no', 'No'), + ('don’t know', 'I don’t know') ], validators=[DataRequired()] ) diff --git a/app/templates/views/service-settings/request-to-go-live.html b/app/templates/views/service-settings/request-to-go-live.html index e86e8fa4d..5ae0d5baf 100644 --- a/app/templates/views/service-settings/request-to-go-live.html +++ b/app/templates/views/service-settings/request-to-go-live.html @@ -33,7 +33,10 @@ <form method="post"> <div class="form-group"> <p>We need permission to process your data before we can make your service live.</p> - {{ radios(form.mou) }} + {{ radios(form.mou, option_hints={ + 'no': 'We’ll send you a copy', + 'don’t know': 'We’ll check for you', + }) }} </div> <div class="form-group"> {{ radios(form.channel) }} From a36f38edb8aba258d51e2b3941dc068354959148 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott <me@quis.cc> Date: Fri, 4 Nov 2016 09:50:11 +0000 Subject: [PATCH 5/6] Remove banner for trial mode message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The yellow banner didn’t make this information much more noticeable, and it made some people miss the request to go live link because it wasn’t blue. This commit brings the design back to where it was as of this PR: https://github.com/alphagov/notifications-admin/pull/904 --- app/assets/stylesheets/_grids.scss | 4 +++ app/templates/views/service-settings.html | 34 +++++++++++------------ 2 files changed, 20 insertions(+), 18 deletions(-) diff --git a/app/assets/stylesheets/_grids.scss b/app/assets/stylesheets/_grids.scss index 00f8809e6..cdd68defe 100644 --- a/app/assets/stylesheets/_grids.scss +++ b/app/assets/stylesheets/_grids.scss @@ -35,6 +35,10 @@ margin-bottom: $gutter-half; } +.bottom-gutter-3-2 { + margin-bottom: $gutter * 3/2; +} + .bottom-gutter-2 { margin-bottom: $gutter * 2; } diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index 3fda72be4..03746a982 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -11,24 +11,7 @@ <h1 class="heading-large">Settings</h1> - {% if current_service.restricted %} - {% call banner_wrapper(type='warning') %} - <h2 class="heading-medium">Your service is in trial mode</h2> - - <ul class='list list-bullet'> - <li>you can only send messages to yourself</li> - <li>you can add people to your team, then you can send messages to them too</li> - <li>you can only send 50 messages per day</li> - </ul> - - <p> - To remove these restrictions - <a href="{{ url_for('.service_request_to_go_live', service_id=current_service.id) }}">request to go live</a>. - </p> - {% endcall %} - {% endif %} - - <div class="bottom-gutter-2"> + <div class="bottom-gutter-3-2"> {% call mapping_table( caption='Settings', @@ -57,6 +40,21 @@ {% endcall %} </div> + {% if current_service.restricted %} + <h2 class="heading-medium">Your service is in trial mode</h2> + + <ul class='list list-bullet'> + <li>you can only send messages to yourself</li> + <li>you can add people to your team, then you can send messages to them too</li> + <li>you can only send 50 messages per day</li> + </ul> + + <p> + To remove these restrictions + <a href="{{ url_for('.service_request_to_go_live', service_id=current_service.id) }}">request to go live</a>. + </p> + {% endif %} + {% if current_user.has_permissions([], admin_override=True) %} <h2 class="heading-medium">Platform admin settings</h2> From 12248d32f739ee87e71aa02d514313b16257d3ca Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott <me@quis.cc> Date: Fri, 4 Nov 2016 10:38:37 +0000 Subject: [PATCH 6/6] Tell users when their service is live MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seen it a few times in research where people are like “so it’s live now … I think. Is it?” Let’s tell them exactly what’s happened. Also a chance to get @minglis’ idea about showing the daily limit. --- app/templates/views/service-settings.html | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index 03746a982..cfe3b9009 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -53,6 +53,19 @@ To remove these restrictions <a href="{{ url_for('.service_request_to_go_live', service_id=current_service.id) }}">request to go live</a>. </p> + {% else %} + <h2 class="heading-medium">Your service is live</h2> + + <p> + You can send up to + {{ "{:,}".format(current_service.message_limit) }} messages + per day. + </p> + <p> + Problems or comments? + <a href="{{ url_for('main.feedback') }}">Give feedback</a>. + </p> + {% endif %} {% if current_user.has_permissions([], admin_override=True) %}