From 8accf7127fbf8e02a802d8c79eb83ea0aaf2f4f5 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 17 May 2017 15:35:21 +0100 Subject: [PATCH 01/40] Enable placeholders in letter contact block Depends on: - [x] https://github.com/alphagov/notifications-api/pull/950 - [x] https://github.com/alphagov/notifications-template-preview/pull/18 - [x] https://github.com/alphagov/notifications-utils/pull/161 - [ ] https://github.com/alphagov/notifications-utils/pull/164 --- app/main/forms.py | 6 +++++- app/main/views/service_settings.py | 5 ++++- app/templates/views/service-settings.html | 2 +- .../views/service-settings/set-letter-contact-block.html | 3 ++- requirements.txt | 2 +- 5 files changed, 13 insertions(+), 5 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index a688c8bdb..ba3c49350 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -502,7 +502,11 @@ class ServiceSmsSender(Form): class ServiceLetterContactBlock(Form): - letter_contact_block = TextAreaField() + letter_contact_block = TextAreaField( + validators=[ + NoCommasInPlaceHolders() + ] + ) def validate_letter_contact_block(form, field): line_count = field.data.strip().count('\n') diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index d891164ea..e8a32fc1b 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -14,6 +14,8 @@ from flask_login import ( login_required, current_user ) + +from notifications_utils.field import Field from notifications_python_client.errors import HTTPError from app import service_api_client @@ -46,7 +48,8 @@ def service_settings(service_id): organisation=organisation, letter_branding=letter_branding_organisations.get( current_service.get('dvla_organisation', '001') - ) + ), + letter_contact_block=Field(current_service['letter_contact_block'], html='escape') ) diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index b133d1aa6..f22347ff8 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -56,7 +56,7 @@ {% call row() %} {{ text_field('Letter contact details') }} {% call field(status='' if current_service.letter_contact_block else 'default') %} - {{ current_service.letter_contact_block | escape | nl2br | safe }} + {{ letter_contact_block | string | nl2br | safe if current_service.letter_contact_block else 'None'}} {% endcall %} {{ edit_field('Change', url_for('.service_set_letter_contact_block', service_id=current_service.id)) }} {% endcall %} diff --git a/app/templates/views/service-settings/set-letter-contact-block.html b/app/templates/views/service-settings/set-letter-contact-block.html index c33121942..ddd46ffe6 100644 --- a/app/templates/views/service-settings/set-letter-contact-block.html +++ b/app/templates/views/service-settings/set-letter-contact-block.html @@ -18,7 +18,8 @@ label='How should users contact your service?
This applies to all the letters you send.'|safe, hint='10 lines maximum', width='1-1', - rows=10 + rows=10, + highlight_tags=True ) }} {{ page_footer( 'Save', diff --git a/requirements.txt b/requirements.txt index 738e5f12f..98ed8f5bf 100644 --- a/requirements.txt +++ b/requirements.txt @@ -28,4 +28,4 @@ notifications-python-client>=3.1,<3.2 awscli>=1.11,<1.12 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@17.0.1#egg=notifications-utils==17.0.1 +git+https://github.com/alphagov/notifications-utils.git@17.1.2#egg=notifications-utils==17.1.2 From cc2e0fd1cb9dbf1dd62e88ddc9f419144a6fccfb Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 19 May 2017 08:16:41 +0100 Subject: [PATCH 02/40] =?UTF-8?q?Show=20spreadsheet=20before=20template=20?= =?UTF-8?q?if=20there=E2=80=99s=20errors?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s easier to work out what the errors are in your file if you can see the big red error message right next to the contents of your file. When users get errors they tend to go back and forth between the error message and the view of the file, not the message and the template preview. --- app/templates/views/check.html | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/app/templates/views/check.html b/app/templates/views/check.html index 933bda753..bb9c4012c 100644 --- a/app/templates/views/check.html +++ b/app/templates/views/check.html @@ -150,7 +150,9 @@ {% endif %} - {{ template|string }} + {% if not errors %} + {{ template|string }} + {% endif %}
{% if errors %} @@ -234,4 +236,9 @@

{% endif %} + {% if errors %} +

Preview of {{ template.name }}

+ {{ template|string }} + {% endif %} + {% endblock %} From 6c2bba5e203af1693de9a3e5507ed780901d2691 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 23 May 2017 13:40:18 +0100 Subject: [PATCH 03/40] Remove fallback for empty SMS sender The API will always return a value for this now (defaults to GOV.UK) --- app/templates/views/service-settings.html | 2 +- tests/__init__.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index 153069c93..c0ec97358 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -36,7 +36,7 @@ {% call row() %} {{ text_field('Text message sender') }} - {{ text_field(current_service.sms_sender or 'GOVUK') }} + {{ text_field(current_service.sms_sender) }} {{ edit_field('Change', url_for('.service_set_sms_sender', service_id=current_service.id)) }} {% endcall %} diff --git a/tests/__init__.py b/tests/__init__.py index 10a48f9f6..bb0311efe 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -47,7 +47,7 @@ def service_json( restricted=True, email_from=None, reply_to_email_address=None, - sms_sender=None, + sms_sender='GOVUK', research_mode=False, can_send_letters=False, can_send_international_sms=False, From ad5fc9a5831c68bd384186fc6c0f98a48fb20963 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 23 May 2017 14:21:59 +0100 Subject: [PATCH 04/40] Put error flag on whole row MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We’ve seen people miss errors if they’re in the right-hand columns of the spreadsheet because people’s eyes scan down the left edge of the table. --- app/templates/components/table.html | 4 ++-- app/templates/views/check.html | 8 ++++++-- tests/app/main/views/test_send.py | 2 +- 3 files changed, 9 insertions(+), 5 deletions(-) diff --git a/app/templates/components/table.html b/app/templates/components/table.html index 2925ee2f2..2697cc6f6 100644 --- a/app/templates/components/table.html +++ b/app/templates/components/table.html @@ -73,9 +73,9 @@ {%- endmacro %} -{% macro index_field(text) -%} +{% macro index_field(text=None) -%} - {{ text }} + {{ text if text != None else caller() }} {%- endmacro %} diff --git a/app/templates/views/check.html b/app/templates/views/check.html index 933bda753..5fd5590c0 100644 --- a/app/templates/views/check.html +++ b/app/templates/views/check.html @@ -191,11 +191,15 @@ ''|safe ] + recipients.column_headers ) %} - {{ index_field(item.index + 2) }} + {% call index_field() %} + + {{ item.index + 2 }} + + {% endcall %} {% for column in recipients.column_headers %} {% if item['columns'][column].error and not recipients.missing_column_headers %} {% call field() %} - + {{ item['columns'][column].error }} {{ item['columns'][column].data if item['columns'][column].data != None }} diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index a90d690f1..83a25adc7 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -255,7 +255,7 @@ def test_upload_valid_csv_shows_file_contents( assert response.status_code == 200 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') for index, cell in enumerate([ - ' 2 ', + ' 2 ', '
07700900986
', '
Jo
', ( From a0cc7922224038eb0d9f7a7b571a8f580bfb4621 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 23 May 2017 14:24:42 +0100 Subject: [PATCH 05/40] Make row numbers bold MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit They’re headings, not part of the data. --- app/assets/stylesheets/components/table.scss | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/assets/stylesheets/components/table.scss b/app/assets/stylesheets/components/table.scss index 085b16e6f..ca079ea0b 100644 --- a/app/assets/stylesheets/components/table.scss +++ b/app/assets/stylesheets/components/table.scss @@ -123,6 +123,7 @@ } &-index { + @include bold-16; width: 15px; } @@ -132,6 +133,10 @@ } +.table-font-xsmall td.table-field-index { // overrides GOV.UK Elements + @include bold-16; +} + .table-field-headings, .table-field-headings-visible { From f425c5756d887cbc996ddc2435a4dfe8b66268dc Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 23 May 2017 14:28:28 +0100 Subject: [PATCH 06/40] Make sure row numbers stay vertically aligned Just looks tidier. --- app/assets/stylesheets/components/table.scss | 6 ++++++ app/templates/views/check.html | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/app/assets/stylesheets/components/table.scss b/app/assets/stylesheets/components/table.scss index ca079ea0b..7d1931395 100644 --- a/app/assets/stylesheets/components/table.scss +++ b/app/assets/stylesheets/components/table.scss @@ -74,6 +74,12 @@ } + &-invisible-error { + border-left: 5px solid transparent; + padding-left: 7px; + display: block; + } + &-status { &-default { diff --git a/app/templates/views/check.html b/app/templates/views/check.html index 5fd5590c0..b349bbcb2 100644 --- a/app/templates/views/check.html +++ b/app/templates/views/check.html @@ -188,7 +188,7 @@ caption=original_file_name, caption_visible=False, field_headings=[ - ''|safe + ''.format("table-field-invisible-error" if errors else "")|safe ] + recipients.column_headers ) %} {% call index_field() %} From 217e975ae92ef5783ae48b635c00a4da07aa2894 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 24 May 2017 12:43:40 +0100 Subject: [PATCH 07/40] Clean up unused SCSS in SMS message component MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Things we don’t do with SMS messages any more: - put paragraphs in them (we use `
` tags instead, to allow for multiple linebreaks) - pick them using radio buttons - render the template’s name as part of the template - render the phone number that the message will be sent from as part of the template --- .../stylesheets/components/sms-message.scss | 35 ------------------- 1 file changed, 35 deletions(-) diff --git a/app/assets/stylesheets/components/sms-message.scss b/app/assets/stylesheets/components/sms-message.scss index a5a79e6d6..2c33a64fc 100644 --- a/app/assets/stylesheets/components/sms-message.scss +++ b/app/assets/stylesheets/components/sms-message.scss @@ -1,4 +1,3 @@ -%sms-message-wrapper, .sms-message-wrapper { width: 100%; @@ -13,21 +12,6 @@ clear: both; word-wrap: break-word; - p { - margin: 0; - line-height: 1.6; - } - - p + p { - margin-top: 20px; - } - -} - -.sms-message-wrapper-with-radio { - @extend %sms-message-wrapper; - padding-left: 45px; - cursor: pointer; } .sms-message-recipient { @@ -35,22 +19,3 @@ color: $secondary-text-colour; margin: 10px 0 0 0; } - -.sms-message-name { - @include bold-24; - margin: 20px 0 5px 0; -} - -.sms-message-picker { - display: block; - margin: 7px 0 0 0; - position: absolute; - left: 15px; - top: 50%; - z-index: 50; -} - -.sms-message-from { - @include bold-19; - display: block; -} From 47c55ccb2b4227616fe779db54211e6c647cfc47 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 24 May 2017 12:55:07 +0100 Subject: [PATCH 08/40] Add tail to SMS message Because: - drawing things in CSS is fun - when we have inbound messages, having a tail pointing the other way will help differentiate which messages are inbound --- .../stylesheets/components/sms-message.scss | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/app/assets/stylesheets/components/sms-message.scss b/app/assets/stylesheets/components/sms-message.scss index 2c33a64fc..96efbf308 100644 --- a/app/assets/stylesheets/components/sms-message.scss +++ b/app/assets/stylesheets/components/sms-message.scss @@ -1,5 +1,8 @@ +$tail-angle: 20deg; + .sms-message-wrapper { + position: relative; width: 100%; max-width: 464px; box-sizing: border-box; @@ -12,6 +15,20 @@ clear: both; word-wrap: break-word; + &:after { + content: ""; + display: block; + position: absolute; + bottom: -4px; + right: -20px; + border: 10px solid transparent; + border-left-width: 13px; + border-right-width: 13px; + border-bottom-color: $panel-colour; + border-left-color: $panel-colour; + transform: rotate($tail-angle); + } + } .sms-message-recipient { From d04bdaa6fbcef7b3497dd8204f78f8886d147b4b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 25 May 2017 08:31:58 +0100 Subject: [PATCH 09/40] Combine related tests into one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These tests were both requesting the same page with the same input, saves a lot of boilerplate if they’re not separated. --- tests/app/main/views/test_send.py | 31 +++---------------------------- 1 file changed, 3 insertions(+), 28 deletions(-) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 83a25adc7..e22550798 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -23,6 +23,7 @@ from tests.conftest import ( mock_get_international_service, mock_get_service_template, mock_get_service_email_template, + SERVICE_ONE_ID, ) template_types = ['email', 'sms'] @@ -204,32 +205,6 @@ def test_upload_csv_invalid_extension( assert "invalid.txt isn’t a spreadsheet that Notify can read" in resp.get_data(as_text=True) -def test_upload_valid_csv_shows_page_title( - logged_in_client, - mocker, - mock_get_service_template_with_placeholders, - mock_s3_upload, - mock_get_users_by_service, - mock_get_detailed_service_for_today, - service_one, - fake_uuid, -): - - mocker.patch('app.main.views.send.s3download', return_value=""" - phone number,name\n07700900986,Jo - """) - - response = logged_in_client.post( - url_for('main.send_messages', service_id=service_one['id'], template_id=fake_uuid), - data={'file': (BytesIO(''.encode('utf-8')), 'valid.csv')}, - follow_redirects=True, - ) - - assert response.status_code == 200 - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - assert page.h1.text.strip() == 'Preview of Two week reminder' - - def test_upload_valid_csv_shows_file_contents( logged_in_client, mocker, @@ -237,7 +212,6 @@ def test_upload_valid_csv_shows_file_contents( mock_s3_upload, mock_get_users_by_service, mock_get_detailed_service_for_today, - service_one, fake_uuid, ): @@ -247,13 +221,14 @@ def test_upload_valid_csv_shows_file_contents( """) response = logged_in_client.post( - url_for('main.send_messages', service_id=service_one['id'], template_id=fake_uuid), + url_for('main.send_messages', service_id=SERVICE_ONE_ID, template_id=fake_uuid), data={'file': (BytesIO(''.encode('utf-8')), 'valid.csv')}, follow_redirects=True, ) assert response.status_code == 200 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert page.h1.text.strip() == 'Preview of Two week reminder' for index, cell in enumerate([ ' 2 ', '
07700900986
', From ff00058847d5541af040f51947f85cc78a793920 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 25 May 2017 15:03:49 +0100 Subject: [PATCH 10/40] =?UTF-8?q?Don=E2=80=99t=20hit=20template=20preview?= =?UTF-8?q?=20app=20when=20running=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s annoying for tests to pass locally because the template preview app is running locally, but fail on Jenkins because the template preview app doesn’t exist. This commit changes it’s hostname to use a dummy port in tests. --- app/config.py | 1 + tests/app/test_template_previews.py | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/app/config.py b/app/config.py index 53eb1e8ac..1a462df1c 100644 --- a/app/config.py +++ b/app/config.py @@ -98,6 +98,7 @@ class Test(Development): WTF_CSRF_ENABLED = False CSV_UPLOAD_BUCKET_NAME = 'test-notifications-csv-upload' NOTIFY_ENVIRONMENT = 'test' + TEMPLATE_PREVIEW_API_HOST = 'http://localhost:9999' class Preview(Config): diff --git a/tests/app/test_template_previews.py b/tests/app/test_template_previews.py index 8c8200576..e1088bbdf 100644 --- a/tests/app/test_template_previews.py +++ b/tests/app/test_template_previews.py @@ -29,15 +29,15 @@ def test_from_utils_template_calls_through( @pytest.mark.parametrize('partial_call, expected_url', [ ( partial(TemplatePreview.from_database_object, filetype='bar'), - 'http://localhost:6013/preview.bar', + 'http://localhost:9999/preview.bar', ), ( partial(TemplatePreview.from_database_object, filetype='baz'), - 'http://localhost:6013/preview.baz', + 'http://localhost:9999/preview.baz', ), ( partial(TemplatePreview.from_database_object, filetype='bar', page=99), - 'http://localhost:6013/preview.bar?page=99', + 'http://localhost:9999/preview.bar?page=99', ), ]) def test_from_database_object_makes_request( From 8cb16b28cbb400e8cdcf3e9edfc4751af8546446 Mon Sep 17 00:00:00 2001 From: Pete Herlihy Date: Thu, 25 May 2017 15:35:14 +0100 Subject: [PATCH 11/40] 51-52 for HMCTS Registration --- app/templates/views/signedout.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/templates/views/signedout.html b/app/templates/views/signedout.html index 702924301..f27df26f5 100644 --- a/app/templates/views/signedout.html +++ b/app/templates/views/signedout.html @@ -117,7 +117,7 @@
-
51
+
52
services
From a1d004fa45e1bf96a7eb81fed094a97e148e16b5 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 26 May 2017 10:40:48 +0100 Subject: [PATCH 12/40] Add cjsm.net to whitelist --- app/config.py | 1 + tests/app/main/test_validators.py | 1 + 2 files changed, 2 insertions(+) diff --git a/app/config.py b/app/config.py index 1a462df1c..9d3a34671 100644 --- a/app/config.py +++ b/app/config.py @@ -81,6 +81,7 @@ class Config(object): r"hmcts\.net", r"scotent\.co\.uk", r"assembly\.wales", + r"cjsm\.net", ] diff --git a/tests/app/main/test_validators.py b/tests/app/main/test_validators.py index 4818a1ee8..1a77885a9 100644 --- a/tests/app/main/test_validators.py +++ b/tests/app/main/test_validators.py @@ -90,6 +90,7 @@ def _gen_mock_field(x): 'test@hmcts.net', 'test@scotent.co.uk', 'test@assembly.wales', + 'test@cjsm.net' ]) def test_valid_list_of_white_list_email_domains( client, From eabd9fcbf1324dd62b80aa536b31699907e4390d Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 25 May 2017 08:55:05 +0100 Subject: [PATCH 13/40] Add a third route through populating a template MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit adds a route which is identical to send yourself a test, but with its own endpoint. This will let us add a slightly different ‘send a one-off message’ flow. This commit just adds the route though, and makes sure that the tests pass for both routes. --- app/main/views/send.py | 37 +++++++++++++++++++++----- tests/app/main/views/test_send.py | 44 +++++++++++++++++++++++++------ 2 files changed, 66 insertions(+), 15 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 638f67cc0..d650c90dd 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -157,14 +157,18 @@ def get_example_csv(service_id, template_id): } -@main.route("/services//send//test") +@main.route("/services//send//test", endpoint='send_test') +@main.route("/services//send//one-off", endpoint='send_one_off') @login_required @user_has_permissions('send_texts', 'send_emails', 'send_letters') def send_test(service_id, template_id): session['send_test_values'] = dict() session['send_test_letter_page_count'] = None return redirect(url_for( - '.send_test_step', + { + 'main.send_test': '.send_test_step', + 'main.send_one_off': '.send_one_off_step', + }[request.endpoint], service_id=service_id, template_id=template_id, step_index=0, @@ -172,14 +176,28 @@ def send_test(service_id, template_id): )) -@main.route("/services//send//test/step-", methods=['GET', 'POST']) +@main.route( + "/services//send//test/step-", + methods=['GET', 'POST'], + endpoint='send_test_step', +) +@main.route( + "/services//send//one-off/step-", + methods=['GET', 'POST'], + endpoint='send_one_off_step', +) @login_required @user_has_permissions('send_texts', 'send_emails', 'send_letters') def send_test_step(service_id, template_id, step_index): if 'send_test_values' not in session: return redirect(url_for( - '.send_test', service_id=service_id, template_id=template_id + { + 'main.send_test_step': '.send_test', + 'main.send_one_off_step': '.send_one_off', + }[request.endpoint], + service_id=service_id, + template_id=template_id, )) template = service_api_client.get_service_template(service_id, template_id)['data'] @@ -210,7 +228,12 @@ def send_test_step(service_id, template_id, step_index): current_placeholder = placeholders[step_index] except IndexError: return redirect(url_for( - '.send_test', service_id=service_id, template_id=template_id + { + 'main.send_test_step': '.send_test', + 'main.send_one_off_step': '.send_one_off', + }[request.endpoint], + service_id=service_id, + template_id=template_id, )) optional_placeholder = (current_placeholder in optional_address_columns) form = get_placeholder_form_instance( @@ -230,7 +253,7 @@ def send_test_step(service_id, template_id, step_index): return make_and_upload_csv_file(service_id, template) return redirect(url_for( - '.send_test_step', + request.endpoint, service_id=service_id, template_id=template_id, step_index=step_index + 1, @@ -247,7 +270,7 @@ def send_test_step(service_id, template_id, step_index): ) else: back_link = url_for( - '.send_test_step', + request.endpoint, service_id=service_id, template_id=template_id, step_index=step_index - 1, diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index e22550798..e5152763a 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -297,18 +297,23 @@ def test_send_test_sms_message( mock_s3_upload.assert_called_with(service_one['id'], expected_data, 'eu-west-1') +@pytest.mark.parametrize('endpoint', [ + 'main.send_test_step', + 'main.send_one_off_step', +]) def test_send_test_step_redirects_if_session_not_setup( logged_in_client, service_one, fake_uuid, mock_get_service_email_template, + endpoint, ): with logged_in_client.session_transaction() as session: assert 'send_test_values' not in session response = logged_in_client.get( - url_for('main.send_test_step', service_id=service_one['id'], template_id=fake_uuid, step_index=0), + url_for(endpoint, service_id=service_one['id'], template_id=fake_uuid, step_index=0), follow_redirects=True ) assert response.status_code == 200 @@ -317,6 +322,10 @@ def test_send_test_step_redirects_if_session_not_setup( assert session['send_test_values'] == {'email address': 'test@user.gov.uk'} +@pytest.mark.parametrize('endpoint', [ + 'main.send_test_step', + 'main.send_one_off_step', +]) def test_send_test_redirects_to_end_if_step_out_of_bounds( logged_in_client, service_one, @@ -325,13 +334,14 @@ def test_send_test_redirects_to_end_if_step_out_of_bounds( mock_s3_upload, mock_get_users_by_service, mock_get_detailed_service_for_today, + endpoint, ): with logged_in_client.session_transaction() as session: session['send_test_values'] = {'name': 'foo'} response = logged_in_client.get(url_for( - 'main.send_test_step', + endpoint, service_id=service_one['id'], template_id=fake_uuid, step_index=999, @@ -351,6 +361,10 @@ def test_send_test_redirects_to_end_if_step_out_of_bounds( ) +@pytest.mark.parametrize('endpoint, expected_redirect', [ + ('main.send_test_step', 'main.send_test'), + ('main.send_one_off_step', 'main.send_one_off'), +]) def test_send_test_redirects_to_start_if_you_skip_steps( logged_in_platform_admin_client, service_one, @@ -360,6 +374,8 @@ def test_send_test_redirects_to_start_if_you_skip_steps( mock_get_users_by_service, mock_get_detailed_service_for_today, mocker, + endpoint, + expected_redirect, ): with logged_in_platform_admin_client.session_transaction() as session: @@ -367,20 +383,24 @@ def test_send_test_redirects_to_start_if_you_skip_steps( session['send_test_values'] = {'address_line_1': 'foo'} response = logged_in_platform_admin_client.get(url_for( - 'main.send_test_step', + endpoint, service_id=service_one['id'], template_id=fake_uuid, step_index=7, # letter template has 7 placeholders – we’re at the end )) assert response.status_code == 302 assert response.location == url_for( - 'main.send_test', + expected_redirect, service_id=service_one['id'], template_id=fake_uuid, _external=True, ) +@pytest.mark.parametrize('endpoint, expected_redirect', [ + ('main.send_test_step', 'main.send_test'), + ('main.send_one_off_step', 'main.send_one_off'), +]) def test_send_test_redirects_to_start_if_index_out_of_bounds_and_some_placeholders_empty( logged_in_client, service_one, @@ -389,13 +409,15 @@ def test_send_test_redirects_to_start_if_index_out_of_bounds_and_some_placeholde mock_s3_download, mock_get_users_by_service, mock_get_detailed_service_for_today, + endpoint, + expected_redirect, ): with logged_in_client.session_transaction() as session: session['send_test_values'] = {'name': 'foo'} response = logged_in_client.get(url_for( - 'main.send_test_step', + endpoint, service_id=service_one['id'], template_id=fake_uuid, step_index=999, @@ -403,24 +425,30 @@ def test_send_test_redirects_to_start_if_index_out_of_bounds_and_some_placeholde assert response.status_code == 302 assert response.location == url_for( - 'main.send_test', + expected_redirect, service_id=service_one['id'], template_id=fake_uuid, _external=True, ) +@pytest.mark.parametrize('endpoint, expected_redirect', [ + ('main.send_test', 'main.send_test_step'), + ('main.send_one_off', 'main.send_one_off_step'), +]) def test_send_test_sms_message_redirects_with_help_argument( logged_in_client, service_one, fake_uuid, + endpoint, + expected_redirect, ): response = logged_in_client.get( - url_for('main.send_test', service_id=service_one['id'], template_id=fake_uuid, help=1) + url_for(endpoint, service_id=service_one['id'], template_id=fake_uuid, help=1) ) assert response.status_code == 302 assert response.location == url_for( - 'main.send_test_step', + expected_redirect, service_id=service_one['id'], template_id=fake_uuid, step_index=0, From f41830e5d3669c781101fe200dcf15723259a61c Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 30 May 2017 12:55:13 +0100 Subject: [PATCH 14/40] Add query string parameter to search by recipient MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit > Service teams that use the admin interface often need to know the > outcome of a message... at the moment they have to page through all > the results in the activity stream. They should be able to find > notifications by email address or phone number. – https://www.pivotaltracker.com/n/projects/1443052 This commit adds an additional query string parameter (`to`) to the URL, which users can use to filter down the list of notifications. It: - takes the status into account - doesn’t update the counts based on the search term (in reality each service will only send a handful of notifications to one person in any 7 day period) In other words the funnel that filters down the notifications looks like: > all notifications for service → only failed → only to this phone > number --- app/main/views/jobs.py | 7 +++++-- app/notify_client/notification_api_client.py | 5 ++++- app/templates/views/notifications.html | 2 +- tests/app/main/views/test_jobs.py | 18 ++++++++++++++++-- tests/conftest.py | 5 ++++- 5 files changed, 30 insertions(+), 7 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index a44c41581..8e85062c7 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -198,7 +198,8 @@ def view_notifications(service_id, message_type): partials=get_notifications(service_id, message_type), message_type=message_type, status=request.args.get('status'), - page=request.args.get('page', 1) + page=request.args.get('page', 1), + to=request.args.get('to'), ) @@ -241,7 +242,9 @@ def get_notifications(service_id, message_type, status_override=None): page=page, template_type=[message_type], status=filter_args.get('status'), - limit_days=current_app.config['ACTIVITY_STATS_LIMIT_DAYS']) + limit_days=current_app.config['ACTIVITY_STATS_LIMIT_DAYS'], + to=request.args.get('to'), + ) url_args = { 'message_type': message_type, diff --git a/app/notify_client/notification_api_client.py b/app/notify_client/notification_api_client.py index 7ea6831e9..4a4399723 100644 --- a/app/notify_client/notification_api_client.py +++ b/app/notify_client/notification_api_client.py @@ -21,7 +21,8 @@ class NotificationApiClient(NotifyAdminAPIClient): limit_days=None, include_jobs=None, include_from_test_key=None, - format_for_csv=None + format_for_csv=None, + to=None, ): params = {} if page is not None: @@ -38,6 +39,8 @@ class NotificationApiClient(NotifyAdminAPIClient): params['include_from_test_key'] = include_from_test_key if format_for_csv is not None: params['format_for_csv'] = format_for_csv + if to is not None: + params['to'] = to if job_id: return self.get( url='/service/{}/job/{}/notifications'.format(service_id, job_id), diff --git a/app/templates/views/notifications.html b/app/templates/views/notifications.html index 61f16aff7..b45587cb2 100644 --- a/app/templates/views/notifications.html +++ b/app/templates/views/notifications.html @@ -20,7 +20,7 @@ {{ ajax_block( partials, - url_for('.get_notifications_as_json', service_id=current_service.id, message_type=message_type, status=status, page=page), + url_for('.get_notifications_as_json', service_id=current_service.id, message_type=message_type, status=status, page=page, to=to), 'notifications' ) }} diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 0980bfcaf..b19f127e6 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -311,6 +311,13 @@ def test_should_show_updates_for_one_job_as_json( (None, 1) ] ) +@pytest.mark.parametrize( + "to_argument, expected_to_argument", [ + ('', ''), + ('+447900900123', '+447900900123'), + ('test@example.com', 'test@example.com'), + ] +) def test_can_show_notifications( logged_in_client, service_one, @@ -322,13 +329,17 @@ def test_can_show_notifications( expected_api_call, page_argument, expected_page_argument, + to_argument, + expected_to_argument, ): response = logged_in_client.get(url_for( 'main.view_notifications', service_id=service_one['id'], message_type=message_type, status=status_argument, - page=page_argument)) + page=page_argument, + to=to_argument, + )) assert response.status_code == 200 content = response.get_data(as_text=True) notifications = notification_json(service_one['id']) @@ -348,13 +359,16 @@ def test_can_show_notifications( assert query_dict['status'] == [status_argument] if expected_page_argument: assert query_dict['page'] == [str(expected_page_argument)] + if to_argument: + assert query_dict['to'] == [to_argument] mock_get_notifications.assert_called_with( limit_days=7, page=expected_page_argument, service_id=service_one['id'], status=expected_api_call, - template_type=[message_type] + template_type=[message_type], + to=expected_to_argument, ) json_response = logged_in_client.get(url_for( diff --git a/tests/conftest.py b/tests/conftest.py index 37dda3ed3..e9171cb8e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1069,6 +1069,7 @@ def mock_get_notifications(mocker, api_user_active): rows=5, include_jobs=None, include_from_test_key=None, + to=None, ): job = None if job_id is not None: @@ -1101,7 +1102,9 @@ def mock_get_notifications_with_previous_next(mocker): status=None, limit_days=None, include_jobs=None, - include_from_test_key=None): + include_from_test_key=None, + to=None, + ): return notification_json(service_id, with_links=True) return mocker.patch( From b9bf18b9362ea38e5d338da0943e506c268cfe9b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 30 May 2017 13:51:25 +0100 Subject: [PATCH 15/40] Add a form to filter notifications by recipient MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Because manually editing the URL isn’t a great user interface, this commit adds a search field to do this on the user’s behalf. For this pass at the story it doesn’t do any validation – the user will just get no results if they search by something which isn’t a phone number or email address. If the user navigates to a different ‘bucket’ of notifications (eg delivered, failed) then the search term is reset, because they’ve changed the filter which is at a level above the search term. --- .../stylesheets/components/page-footer.scss | 18 +++++++ app/main/forms.py | 5 ++ app/main/views/jobs.py | 8 +-- app/templates/views/notifications.html | 20 ++++++++ tests/app/main/views/test_jobs.py | 49 +++++++++++++++++++ 5 files changed, 97 insertions(+), 3 deletions(-) diff --git a/app/assets/stylesheets/components/page-footer.scss b/app/assets/stylesheets/components/page-footer.scss index 04b6a31b1..e40c42a35 100644 --- a/app/assets/stylesheets/components/page-footer.scss +++ b/app/assets/stylesheets/components/page-footer.scss @@ -59,3 +59,21 @@ } } + +.align-button-with-textbox { + + .button { + + @include media(desktop) { + position: relative; + top: 32px; + left: -30px; + width: 100%; + margin-right: -30px; + padding-top: 8px; + box-sizing: content-box; + } + + } + +} diff --git a/app/main/forms.py b/app/main/forms.py index eb60cc8f4..af4dd52ac 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -625,6 +625,11 @@ class SearchTemplatesForm(Form): search = SearchField('Search by name') +class SearchNotificationsForm(Form): + + to = SearchField('Search by phone number or email address') + + class PlaceholderForm(Form): pass diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 8e85062c7..1e85faf0b 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -26,6 +26,7 @@ from app import ( current_service, format_datetime_short) from app.main import main +from app.main.forms import SearchNotificationsForm from app.utils import ( get_page_from_request, generate_next_dict, @@ -197,9 +198,10 @@ def view_notifications(service_id, message_type): 'views/notifications.html', partials=get_notifications(service_id, message_type), message_type=message_type, - status=request.args.get('status'), + status=request.args.get('status') or 'sending,delivered,failed', page=request.args.get('page', 1), to=request.args.get('to'), + search_form=SearchNotificationsForm(to=request.args.get('to')), ) @@ -252,11 +254,11 @@ def get_notifications(service_id, message_type, status_override=None): } prev_page = None - if notifications['links'].get('prev', None): + if 'links' in notifications and notifications['links'].get('prev', None): prev_page = generate_previous_dict('main.view_notifications', service_id, page, url_args=url_args) next_page = None - if notifications['links'].get('next', None): + if 'links' in notifications and notifications['links'].get('next', None): next_page = generate_next_dict('main.view_notifications', service_id, page, url_args) return { diff --git a/app/templates/views/notifications.html b/app/templates/views/notifications.html index b45587cb2..3190b4fa0 100644 --- a/app/templates/views/notifications.html +++ b/app/templates/views/notifications.html @@ -1,6 +1,8 @@ {% extends "withnav_template.html" %} {% from "components/ajax-block.html" import ajax_block %} {% from "components/message-count-label.html" import message_count_label, recipient_count_label %} +{% from "components/page-footer.html" import page_footer %} +{% from "components/textbox.html" import textbox %} {% block service_page_title %} {{ message_count_label(99, message_type, suffix='') | capitalize }} @@ -18,6 +20,24 @@ 'counts' ) }} +
+
+ + {{ textbox( + search_form.to, + width='1-1', + label='Search by {}'.format('email address' if message_type == 'email' else 'phone number') + ) }} +
+
+ +
+
+ {{ ajax_block( partials, url_for('.get_notifications_as_json', service_id=current_service.id, message_type=message_type, status=status, page=page, to=to), diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index b19f127e6..89b4f8ada 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -8,6 +8,7 @@ from bs4 import BeautifulSoup from app.main.views.jobs import get_time_left, get_status_filters from tests import notification_json +from tests.conftest import SERVICE_ONE_ID from freezegun import freeze_time @@ -381,6 +382,54 @@ def test_can_show_notifications( assert json_content.keys() == {'counts', 'notifications'} +@pytest.mark.parametrize("initial_query_arguments, expected_status_field_value, expected_search_box_contents", [ + ( + { + 'message_type': 'sms', + }, + 'sending,delivered,failed', + '', + ), + ( + { + 'status': 'failed', + 'message_type': 'email', + 'page': '99', + 'to': 'test@example.com', + }, + 'failed', + 'test@example.com', + ), +]) +def test_search_recipient_form( + logged_in_client, + mock_get_notifications, + mock_get_detailed_service, + initial_query_arguments, + expected_status_field_value, + expected_search_box_contents, +): + response = logged_in_client.get(url_for( + 'main.view_notifications', + service_id=SERVICE_ONE_ID, + **initial_query_arguments + )) + assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + + action_url = page.find("form")['action'] + url = urlparse(action_url) + assert url.path == '/services/{}/notifications/{}'.format( + SERVICE_ONE_ID, + initial_query_arguments['message_type'] + ) + query_dict = parse_qs(url.query) + assert query_dict == {} + + assert page.find("input", {'name': 'status'})['value'] == expected_status_field_value + assert page.find("input", {'name': 'to'})['value'] == expected_search_box_contents + + def test_should_show_notifications_for_a_service_with_next_previous( logged_in_client, service_one, From 311732ff9da15c146dff276f4a8058405b17c6f1 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 31 May 2017 11:53:10 +0100 Subject: [PATCH 16/40] Add test for international phone no. --- tests/app/main/views/test_jobs.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 89b4f8ada..0625f37e7 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -390,6 +390,14 @@ def test_can_show_notifications( 'sending,delivered,failed', '', ), + ( + { + 'message_type': 'sms', + 'to': '+33(0)5-12-34-56-78', + }, + 'sending,delivered,failed', + '+33(0)5-12-34-56-78', + ), ( { 'status': 'failed', From 7310e8b435bc725dfc4dc0aa5a7a7d6357a468c0 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 25 May 2017 09:18:26 +0100 Subject: [PATCH 17/40] Make send one-off route ask for phone number/email MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit adds an extra, initial, step to the ‘send one-off message’ flow to ask for a phone number or email address. This is the first pass at making a feature which caseworkers or similar could use Notify to send individual messages while they’re working a case. --- app/main/views/send.py | 23 +++++++++++------ tests/app/main/views/test_send.py | 42 +++++++++++++++++++++++-------- 2 files changed, 47 insertions(+), 18 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index d650c90dd..b874bc07c 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -219,7 +219,10 @@ def send_test_step(service_id, template_id, step_index): page_count=session['send_test_letter_page_count'] ) - placeholders = fields_to_fill_in(template) + placeholders = fields_to_fill_in( + template, + prefill_current_user=(request.endpoint == 'main.send_test_step'), + ) if len(placeholders) == 0: return make_and_upload_csv_file(service_id, template) @@ -227,6 +230,8 @@ def send_test_step(service_id, template_id, step_index): try: current_placeholder = placeholders[step_index] except IndexError: + if all_placeholders_in_session(placeholders): + return make_and_upload_csv_file(service_id, template) return redirect(url_for( { 'main.send_test_step': '.send_test', @@ -246,10 +251,7 @@ def send_test_step(service_id, template_id, step_index): session['send_test_values'][current_placeholder] = form.placeholder_value.data - if all( - get_normalised_send_test_values_from_session().get(placeholder, False) not in (False, None) - for placeholder in placeholders - ): + if all_placeholders_in_session(placeholders): return make_and_upload_csv_file(service_id, template) return redirect(url_for( @@ -493,11 +495,11 @@ def get_check_messages_back_url(service_id, template_type): return url_for('main.choose_template', service_id=service_id) -def fields_to_fill_in(template): +def fields_to_fill_in(template, prefill_current_user=False): recipient_columns = first_column_headings[template.template_type] - if 'letter' == template.template_type: + if 'letter' == template.template_type or not prefill_current_user: return recipient_columns + list(template.placeholders) session['send_test_values'][recipient_columns[0]] = { @@ -536,3 +538,10 @@ def make_and_upload_csv_file(service_id, template): from_test=True, help=2 if get_help_argument() else 0 )) + + +def all_placeholders_in_session(placeholders): + return all( + get_normalised_send_test_values_from_session().get(placeholder, False) not in (False, None) + for placeholder in placeholders + ) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index e5152763a..3f07e4108 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -297,34 +297,52 @@ def test_send_test_sms_message( mock_s3_upload.assert_called_with(service_one['id'], expected_data, 'eu-west-1') -@pytest.mark.parametrize('endpoint', [ - 'main.send_test_step', - 'main.send_one_off_step', +@pytest.mark.parametrize('endpoint, template_mock, expected_session_contents', [ + ('main.send_test_step', mock_get_service_template_with_placeholders, {'phone number': '07700 900762'}), + ('main.send_test_step', mock_get_service_email_template, {'email address': 'test@user.gov.uk'}), + ('main.send_test_step', mock_get_service_letter_template, {}), + ('main.send_one_off_step', mock_get_service_template, {}), + ('main.send_one_off_step', mock_get_service_email_template, {}), + ('main.send_one_off_step', mock_get_service_letter_template, {}), ]) def test_send_test_step_redirects_if_session_not_setup( + mocker, logged_in_client, - service_one, + mock_get_detailed_service_for_today, + mock_get_users_by_service, fake_uuid, - mock_get_service_email_template, endpoint, + template_mock, + expected_session_contents, ): + template_mock(mocker) + mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=99) + with logged_in_client.session_transaction() as session: assert 'send_test_values' not in session response = logged_in_client.get( - url_for(endpoint, service_id=service_one['id'], template_id=fake_uuid, step_index=0), + url_for(endpoint, service_id=SERVICE_ONE_ID, template_id=fake_uuid, step_index=0), follow_redirects=True ) assert response.status_code == 200 with logged_in_client.session_transaction() as session: - assert session['send_test_values'] == {'email address': 'test@user.gov.uk'} + assert session['send_test_values'] == expected_session_contents -@pytest.mark.parametrize('endpoint', [ - 'main.send_test_step', - 'main.send_one_off_step', +@pytest.mark.parametrize('endpoint, expected_redirect, send_test_values', [ + ( + 'main.send_test_step', + 'main.send_test', + {'name': 'foo'}, + ), + ( + 'main.send_one_off_step', + 'main.send_one_off', + {'name': 'foo', 'phone number': '07900900123'}, + ), ]) def test_send_test_redirects_to_end_if_step_out_of_bounds( logged_in_client, @@ -335,10 +353,12 @@ def test_send_test_redirects_to_end_if_step_out_of_bounds( mock_get_users_by_service, mock_get_detailed_service_for_today, endpoint, + send_test_values, + expected_redirect, ): with logged_in_client.session_transaction() as session: - session['send_test_values'] = {'name': 'foo'} + session['send_test_values'] = send_test_values response = logged_in_client.get(url_for( endpoint, From 27c14632130d27b199488ffb91199c2565fca3dd Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 25 May 2017 09:40:37 +0100 Subject: [PATCH 18/40] Validate recipients in send a one-off message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It would be annoying to get all the way to the end of the flow and get told that the phone number or email address you entered isn’t valid. So this commit reuses the existing WTForms objects that we have to do some extra validation on the first step in the send one-off message flow. It also accounts for international phone numbers, if the service is allowed to send them. It doesn’t reject other people’s phone numbers if your service is restricted, because I think it’s better to let users play with the feature – it’s good for learning. --- app/main/forms.py | 42 ++++++++++++++++++----- app/main/views/send.py | 1 + tests/app/main/test_placeholder_form.py | 44 +++++++++++++++++++++++++ 3 files changed, 79 insertions(+), 8 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index eb60cc8f4..bf95901b3 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -7,6 +7,7 @@ from notifications_utils.recipients import ( validate_phone_number, InvalidPhoneError ) +from notifications_utils.columns import Columns from wtforms import ( validators, StringField, @@ -102,11 +103,26 @@ class UKMobileNumber(TelField): raise ValidationError(str(e)) -def mobile_number(): - return UKMobileNumber('Mobile number', +class InternationalPhoneNumber(TelField): + def pre_validate(self, form): + try: + validate_phone_number(self.data, international=True) + except InvalidPhoneError as e: + raise ValidationError(str(e)) + + +def mobile_number(label='Mobile number'): + return UKMobileNumber(label, validators=[DataRequired(message='Can’t be empty')]) +def international_phone_number(label='Mobile number'): + return InternationalPhoneNumber( + label, + validators=[DataRequired(message='Can’t be empty')] + ) + + def password(label='Password'): return PasswordField(label, validators=[DataRequired(message='Can’t be empty'), @@ -633,15 +649,25 @@ class PlaceholderForm(Form): def get_placeholder_form_instance( placeholder_name, dict_to_populate_from, - optional_placeholder=False + optional_placeholder=False, + allow_international_phone_numbers=False, ): - PlaceholderForm.placeholder_value = StringField( - placeholder_name, - validators=[ + if Columns.make_key(placeholder_name) == 'emailaddress': + field = email_address(label=placeholder_name, gov_user=False) + elif Columns.make_key(placeholder_name) == 'phonenumber': + if allow_international_phone_numbers: + field = international_phone_number(label=placeholder_name) + else: + field = mobile_number(label=placeholder_name) + elif optional_placeholder: + field = StringField(placeholder_name) + else: + field = StringField(placeholder_name, validators=[ DataRequired(message='Can’t be empty') - ] if not optional_placeholder else [] - ) + ]) + + PlaceholderForm.placeholder_value = field return PlaceholderForm( placeholder_value=dict_to_populate_from.get(placeholder_name, '') diff --git a/app/main/views/send.py b/app/main/views/send.py index b874bc07c..7051094b2 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -245,6 +245,7 @@ def send_test_step(service_id, template_id, step_index): current_placeholder, dict_to_populate_from=get_normalised_send_test_values_from_session(), optional_placeholder=optional_placeholder, + allow_international_phone_numbers=current_service['can_send_international_sms'], ) if form.validate_on_submit(): diff --git a/tests/app/main/test_placeholder_form.py b/tests/app/main/test_placeholder_form.py index 07fac6675..4c6093944 100644 --- a/tests/app/main/test_placeholder_form.py +++ b/tests/app/main/test_placeholder_form.py @@ -1,3 +1,4 @@ +import pytest from app.main.forms import get_placeholder_form_instance from wtforms import Label @@ -16,3 +17,46 @@ def test_form_class_not_mutated(app_): assert str(form1.placeholder_value.label) == '' assert str(form2.placeholder_value.label) == '' + + +@pytest.mark.parametrize('service_can_send_international_sms, placeholder_name, value, expected_error', [ + + (False, 'email address', '', 'Can’t be empty'), + (False, 'email address', '12345', 'Enter a valid email address'), + (False, 'email address', 'test@example.com', None), + (False, 'email address', 'test@example.gov.uk', None), + + (False, 'phone number', '', 'Can’t be empty'), + (False, 'phone number', '+1-2345-678890', 'Not a UK mobile number'), + (False, 'phone number', '07900900123', None), + (False, 'phone number', '+44(0)7900 900-123', None), + + (True, 'phone number', '+123', 'Not enough digits'), + (True, 'phone number', '+44(0)7900 900-123', None), + (True, 'phone number', '+1-2345-678890', None), + + (False, 'anything else', '', 'Can’t be empty'), + +]) +def test_validates_recipients( + app_, + placeholder_name, + value, + service_can_send_international_sms, + expected_error, +): + with app_.test_request_context( + method='POST', + data={'placeholder_value': value} + ): + form = get_placeholder_form_instance( + placeholder_name, + {}, + allow_international_phone_numbers=service_can_send_international_sms, + ) + + if expected_error: + assert not form.validate_on_submit() + assert form.placeholder_value.errors[0] == expected_error + else: + assert form.validate_on_submit() From 82549c817a1656aa3c045225f28271ab5b442863 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 25 May 2017 13:31:23 +0100 Subject: [PATCH 19/40] Ensure correct page titles on send test flows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We have some fairly complicated nested if statements in our Jinja that decide what the page titles should be. It’s only going to get more complicated with the send individual message routes. So this commit: - moves the logic from Jinja to Python - adds tests to check things are working as expected - sets the page titles to the right thing for each flow --- app/main/views/send.py | 12 ++++++ app/templates/views/send-test.html | 16 +------- tests/app/main/views/test_send.py | 65 ++++++++++++++++++++++++++++++ 3 files changed, 79 insertions(+), 14 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 7051094b2..cbf7acf0e 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -284,6 +284,7 @@ def send_test_step(service_id, template_id, step_index): return render_template( 'views/send-test.html', + page_title=get_send_test_page_title(template.template_type, request.endpoint), template=template, form=form, optional_placeholder=optional_placeholder, @@ -546,3 +547,14 @@ def all_placeholders_in_session(placeholders): get_normalised_send_test_values_from_session().get(placeholder, False) not in (False, None) for placeholder in placeholders ) + + +def get_send_test_page_title(template_type, endpoint): + if get_help_argument(): + return 'Example text message' + if template_type == 'letter': + return 'Print a test letter' + return { + 'main.send_test_step': 'Send yourself a test', + 'main.send_one_off_step': 'Send one-off message', + }[endpoint] diff --git a/app/templates/views/send-test.html b/app/templates/views/send-test.html index c7b0b0e2e..05eae2aeb 100644 --- a/app/templates/views/send-test.html +++ b/app/templates/views/send-test.html @@ -5,25 +5,13 @@ {% from "components/table.html" import list_table, field, text_field, index_field, index_field_heading %} {% block service_page_title %} - {% if request.args['help'] %} - Example text message - {% else %} - Send yourself a test - {% endif %} + {{ page_title }} {% endblock %} {% block maincolumn_content %}

- {% if request.args['help'] %} - Example text message - {% else %} - {% if template.template_type == 'letter' %} - Print a test letter - {% else %} - Send yourself a test - {% endif %} - {% endif %} + {{ page_title }}

diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 3f07e4108..ded6677a5 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -332,6 +332,71 @@ def test_send_test_step_redirects_if_session_not_setup( assert session['send_test_values'] == expected_session_contents +@pytest.mark.parametrize('template_mock, partial_url, expected_h1', [ + ( + mock_get_service_template_with_placeholders, + partial(url_for, 'main.send_test'), + 'Send yourself a test', + ), + ( + mock_get_service_template_with_placeholders, + partial(url_for, 'main.send_one_off'), + 'Send one-off message', + ), + ( + mock_get_service_template_with_placeholders, + partial(url_for, 'main.send_test', help=1), + 'Example text message', + ), + ( + mock_get_service_email_template, + partial(url_for, 'main.send_test', help=1), + 'Example text message', + ), + ( + mock_get_service_email_template, + partial(url_for, 'main.send_test'), + 'Send yourself a test', + ), + ( + mock_get_service_email_template, + partial(url_for, 'main.send_one_off'), + 'Send one-off message', + ), + ( + mock_get_service_letter_template, + partial(url_for, 'main.send_test'), + 'Print a test letter', + ), + ( + mock_get_service_letter_template, + partial(url_for, 'main.send_one_off'), + 'Print a test letter', + ), +]) +def test_send_one_off_or_test_has_correct_page_titles( + logged_in_client, + service_one, + fake_uuid, + mocker, + template_mock, + partial_url, + expected_h1, +): + + template_mock(mocker) + mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=99) + + response = logged_in_client.get( + partial_url(service_id=service_one['id'], template_id=fake_uuid, step_index=0), + follow_redirects=True, + ) + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + + assert response.status_code == 200 + assert page.h1.text.strip() == expected_h1 + + @pytest.mark.parametrize('endpoint, expected_redirect, send_test_values', [ ( 'main.send_test_step', From 43f6d21e1dd22e2778431a86f3886c45bf9f528b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 25 May 2017 13:31:51 +0100 Subject: [PATCH 20/40] Replace test with one-off MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s a confusing proposition to have two features which are almost identical. Even differentiating between them in the template menu would be tricky. I think the better thing to do is rename the whole feature to ‘send one-off message’. Then if someone wants to use there own phone number or email address, give them a quick shortcut to doing that, once they’re in the flow. In the background this reuses the ‘send yourself a test’ code, but the user is never aware that they’re going through a different route to send an individual message. So the proposition stays nice and clean. --- app/assets/stylesheets/_grids.scss | 13 +++++ app/config.py | 2 +- app/main/views/send.py | 25 ++++++--- app/templates/views/send-test.html | 18 ++++-- app/templates/views/templates/_template.html | 4 +- tests/app/main/views/test_send.py | 58 +++++++++++++++++--- tests/app/main/views/test_templates.py | 4 +- 7 files changed, 100 insertions(+), 24 deletions(-) diff --git a/app/assets/stylesheets/_grids.scss b/app/assets/stylesheets/_grids.scss index c7becdee2..4fbd20f8f 100644 --- a/app/assets/stylesheets/_grids.scss +++ b/app/assets/stylesheets/_grids.scss @@ -26,6 +26,19 @@ @include grid-column(7/8); } +%top-gutter, +.top-gutter { + @extend %contain-floats; + display: block; + margin-top: $gutter; + clear: both; +} + +.top-gutter-4-3 { + @extend %top-gutter; + margin-top: $gutter * 4 / 3; +} + %bottom-gutter, .bottom-gutter { @extend %contain-floats; diff --git a/app/config.py b/app/config.py index 9d3a34671..d48821d8b 100644 --- a/app/config.py +++ b/app/config.py @@ -57,7 +57,7 @@ class Config(object): CSV_UPLOAD_BUCKET_NAME = 'local-notifications-csv-upload' DESKPRO_PERSON_EMAIL = 'donotreply@notifications.service.gov.uk' ACTIVITY_STATS_LIMIT_DAYS = 7 - TEST_MESSAGE_FILENAME = 'Test message' + TEST_MESSAGE_FILENAME = 'One-off message' STATSD_ENABLED = False STATSD_HOST = "statsd.hostedgraphite.com" diff --git a/app/main/views/send.py b/app/main/views/send.py index cbf7acf0e..2ff3cef75 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -282,13 +282,25 @@ def send_test_step(service_id, template_id, step_index): template.values = get_normalised_send_test_values_from_session() template.values[current_placeholder] = None + if ( + request.endpoint == 'main.send_one_off_step' and + step_index == 0 and + template.template_type != 'letter' + ): + skip_link = ( + 'Use my {}'.format(first_column_headings[template.template_type][0]), + url_for('.send_test', service_id=service_id, template_id=template.id), + ) + else: + skip_link = None + return render_template( 'views/send-test.html', - page_title=get_send_test_page_title(template.template_type, request.endpoint), + page_title=get_send_test_page_title(template.template_type, get_help_argument()), template=template, form=form, + skip_link=skip_link, optional_placeholder=optional_placeholder, - help=get_help_argument(), back_link=back_link, ) @@ -549,12 +561,9 @@ def all_placeholders_in_session(placeholders): ) -def get_send_test_page_title(template_type, endpoint): - if get_help_argument(): +def get_send_test_page_title(template_type, help_argument): + if help_argument: return 'Example text message' if template_type == 'letter': return 'Print a test letter' - return { - 'main.send_test_step': 'Send yourself a test', - 'main.send_one_off_step': 'Send one-off message', - }[endpoint] + return 'Send one-off message' diff --git a/app/templates/views/send-test.html b/app/templates/views/send-test.html index 05eae2aeb..bae3939cc 100644 --- a/app/templates/views/send-test.html +++ b/app/templates/views/send-test.html @@ -15,10 +15,20 @@ - {{ textbox( - form.placeholder_value, - hint='Optional' if optional_placeholder else None - ) }} +
+
+ {{ textbox( + form.placeholder_value, + hint='Optional' if optional_placeholder else None, + width='1-1', + ) }} +
+ {% if skip_link %} +
+ {{ skip_link[0] }} +
+ {% endif %} +
{{ page_footer('Next', back_link=back_link) }} diff --git a/app/templates/views/templates/_template.html b/app/templates/views/templates/_template.html index e74e8e0a5..31ce14918 100644 --- a/app/templates/views/templates/_template.html +++ b/app/templates/views/templates/_template.html @@ -13,8 +13,8 @@
- - {{ 'Print a test letter' if template.template_type == 'letter' else 'Send yourself a test' }} + + {{ 'Print a test letter' if template.template_type == 'letter' else 'Send one-off message' }}
{% endif %} diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index ded6677a5..eeb7d6ecf 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -286,7 +286,6 @@ def test_send_test_sms_message( mock_get_detailed_service_for_today, ): - expected_data = {'data': 'phone number\r\n07700 900762\r\n', 'file_name': 'Test message'} mocker.patch('app.main.views.send.s3download', return_value='phone number\r\n+4412341234') response = logged_in_client.get( @@ -294,7 +293,11 @@ def test_send_test_sms_message( follow_redirects=True ) assert response.status_code == 200 - mock_s3_upload.assert_called_with(service_one['id'], expected_data, 'eu-west-1') + mock_s3_upload.assert_called_with( + service_one['id'], + {'data': 'phone number\r\n07700 900762\r\n', 'file_name': 'One-off message'}, + 'eu-west-1' + ) @pytest.mark.parametrize('endpoint, template_mock, expected_session_contents', [ @@ -336,7 +339,7 @@ def test_send_test_step_redirects_if_session_not_setup( ( mock_get_service_template_with_placeholders, partial(url_for, 'main.send_test'), - 'Send yourself a test', + 'Send one-off message', ), ( mock_get_service_template_with_placeholders, @@ -356,7 +359,7 @@ def test_send_test_step_redirects_if_session_not_setup( ( mock_get_service_email_template, partial(url_for, 'main.send_test'), - 'Send yourself a test', + 'Send one-off message', ), ( mock_get_service_email_template, @@ -397,6 +400,44 @@ def test_send_one_off_or_test_has_correct_page_titles( assert page.h1.text.strip() == expected_h1 +@pytest.mark.parametrize('template_mock, expected_link_text, expected_link_url', [ + (mock_get_service_template, 'Use my phone number', partial(url_for, 'main.send_test')), + (mock_get_service_email_template, 'Use my email address', partial(url_for, 'main.send_test')), + (mock_get_service_letter_template, None, None), +]) +def test_send_one_off_has_skip_link( + logged_in_client, + service_one, + fake_uuid, + mock_get_service_email_template, + mocker, + template_mock, + expected_link_text, + expected_link_url, +): + + template_mock(mocker) + mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=99) + + response = logged_in_client.get( + url_for('main.send_one_off_step', service_id=service_one['id'], template_id=fake_uuid, step_index=0), + follow_redirects=True + ) + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + skip_links = page.select('a.top-gutter-4-3') + + assert response.status_code == 200 + + if expected_link_text and expected_link_url: + assert skip_links[0].text.strip() == expected_link_text + assert skip_links[0]['href'] == expected_link_url( + service_id=service_one['id'], + template_id=fake_uuid, + ) + else: + assert not skip_links + + @pytest.mark.parametrize('endpoint, expected_redirect, send_test_values', [ ( 'main.send_test_step', @@ -553,7 +594,6 @@ def test_send_test_email_message_without_placeholders( fake_uuid, ): - expected_data = {'data': 'email address\r\ntest@user.gov.uk\r\n', 'file_name': 'Test message'} mocker.patch('app.main.views.send.s3download', return_value='email address\r\ntest@user.gov.uk') response = logged_in_client.get( @@ -561,7 +601,11 @@ def test_send_test_email_message_without_placeholders( follow_redirects=True ) assert response.status_code == 200 - mock_s3_upload.assert_called_with(service_one['id'], expected_data, 'eu-west-1') + mock_s3_upload.assert_called_with( + service_one['id'], + {'data': 'email address\r\ntest@user.gov.uk\r\n', 'file_name': 'One-off message'}, + 'eu-west-1' + ) def test_send_test_sms_message_with_placeholders_shows_first_field( @@ -780,7 +824,7 @@ def test_send_test_sms_message_puts_submitted_data_in_session_and_file( service_one['id'], { 'data': 'name,phone number\r\nJo,07700 900762\r\n', - 'file_name': 'Test message' + 'file_name': 'One-off message' }, 'eu-west-1' ) diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index cf0bc3fdd..acafae751 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -44,11 +44,11 @@ def test_should_show_page_for_one_template( ), ( ['send_texts', 'send_emails', 'send_letters'], - ['.send_messages', '.send_test'] + ['.send_messages', '.send_one_off'] ), ( ['send_texts', 'send_emails', 'send_letters', 'manage_templates'], - ['.send_messages', '.send_test', '.edit_service_template'] + ['.send_messages', '.send_one_off', '.edit_service_template'] ), ]) def test_should_be_able_to_view_a_template_with_links( From 6425dcbc9461ad2f69870acb65ba03d1a6fabe32 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 1 Jun 2017 13:03:22 +0100 Subject: [PATCH 21/40] =?UTF-8?q?Rename=20the=20feature=20to=20=E2=80=98se?= =?UTF-8?q?nd=20to=20one=20recipient=E2=80=99?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ‘One-off’ is a bit wooly. Feels like our name for the thing. ‘Send to one recipient’ matches ‘Upload recipients’. This also means making the `

` on job page ‘Report’ for one-off messages. It doesn’t make sense to call the feature ‘send to one recipient’ when we’re not using the language of one-off any more. --- app/config.py | 2 +- app/main/views/send.py | 2 +- app/templates/views/templates/_template.html | 2 +- tests/app/main/views/test_send.py | 14 +++++++------- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/app/config.py b/app/config.py index d48821d8b..f9b737929 100644 --- a/app/config.py +++ b/app/config.py @@ -57,7 +57,7 @@ class Config(object): CSV_UPLOAD_BUCKET_NAME = 'local-notifications-csv-upload' DESKPRO_PERSON_EMAIL = 'donotreply@notifications.service.gov.uk' ACTIVITY_STATS_LIMIT_DAYS = 7 - TEST_MESSAGE_FILENAME = 'One-off message' + TEST_MESSAGE_FILENAME = 'Report' STATSD_ENABLED = False STATSD_HOST = "statsd.hostedgraphite.com" diff --git a/app/main/views/send.py b/app/main/views/send.py index 2ff3cef75..bfa47c33a 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -566,4 +566,4 @@ def get_send_test_page_title(template_type, help_argument): return 'Example text message' if template_type == 'letter': return 'Print a test letter' - return 'Send one-off message' + return 'Send to one recipient' diff --git a/app/templates/views/templates/_template.html b/app/templates/views/templates/_template.html index 31ce14918..6a2add4df 100644 --- a/app/templates/views/templates/_template.html +++ b/app/templates/views/templates/_template.html @@ -14,7 +14,7 @@

- {{ 'Print a test letter' if template.template_type == 'letter' else 'Send one-off message' }} + {{ 'Print a test letter' if template.template_type == 'letter' else 'Send to one recipient' }}
{% endif %} diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index eeb7d6ecf..f5559bba8 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -295,7 +295,7 @@ def test_send_test_sms_message( assert response.status_code == 200 mock_s3_upload.assert_called_with( service_one['id'], - {'data': 'phone number\r\n07700 900762\r\n', 'file_name': 'One-off message'}, + {'data': 'phone number\r\n07700 900762\r\n', 'file_name': 'Report'}, 'eu-west-1' ) @@ -339,12 +339,12 @@ def test_send_test_step_redirects_if_session_not_setup( ( mock_get_service_template_with_placeholders, partial(url_for, 'main.send_test'), - 'Send one-off message', + 'Send to one recipient', ), ( mock_get_service_template_with_placeholders, partial(url_for, 'main.send_one_off'), - 'Send one-off message', + 'Send to one recipient', ), ( mock_get_service_template_with_placeholders, @@ -359,12 +359,12 @@ def test_send_test_step_redirects_if_session_not_setup( ( mock_get_service_email_template, partial(url_for, 'main.send_test'), - 'Send one-off message', + 'Send to one recipient', ), ( mock_get_service_email_template, partial(url_for, 'main.send_one_off'), - 'Send one-off message', + 'Send to one recipient', ), ( mock_get_service_letter_template, @@ -603,7 +603,7 @@ def test_send_test_email_message_without_placeholders( assert response.status_code == 200 mock_s3_upload.assert_called_with( service_one['id'], - {'data': 'email address\r\ntest@user.gov.uk\r\n', 'file_name': 'One-off message'}, + {'data': 'email address\r\ntest@user.gov.uk\r\n', 'file_name': 'Report'}, 'eu-west-1' ) @@ -824,7 +824,7 @@ def test_send_test_sms_message_puts_submitted_data_in_session_and_file( service_one['id'], { 'data': 'name,phone number\r\nJo,07700 900762\r\n', - 'file_name': 'One-off message' + 'file_name': 'Report' }, 'eu-west-1' ) From f12e0fde394b3d67b5617bea681d13311cc878b6 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 1 Jun 2017 13:29:30 +0100 Subject: [PATCH 22/40] Ensure the tour sidebar gets shown MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I accidentally broke it by removing a parameter. This commit reinstates that parameter and adds some tests to make sure it doesn’t happen again. --- app/main/views/send.py | 1 + tests/app/main/views/test_send.py | 13 ++++++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index bfa47c33a..de6be4ff1 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -302,6 +302,7 @@ def send_test_step(service_id, template_id, step_index): skip_link=skip_link, optional_placeholder=optional_placeholder, back_link=back_link, + help=get_help_argument(), ) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index f5559bba8..69ebddf74 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -335,46 +335,54 @@ def test_send_test_step_redirects_if_session_not_setup( assert session['send_test_values'] == expected_session_contents -@pytest.mark.parametrize('template_mock, partial_url, expected_h1', [ +@pytest.mark.parametrize('template_mock, partial_url, expected_h1, tour_shown', [ ( mock_get_service_template_with_placeholders, partial(url_for, 'main.send_test'), 'Send to one recipient', + False, ), ( mock_get_service_template_with_placeholders, partial(url_for, 'main.send_one_off'), 'Send to one recipient', + False, ), ( mock_get_service_template_with_placeholders, partial(url_for, 'main.send_test', help=1), 'Example text message', + True, ), ( mock_get_service_email_template, partial(url_for, 'main.send_test', help=1), 'Example text message', + True, ), ( mock_get_service_email_template, partial(url_for, 'main.send_test'), 'Send to one recipient', + False, ), ( mock_get_service_email_template, partial(url_for, 'main.send_one_off'), 'Send to one recipient', + False, ), ( mock_get_service_letter_template, partial(url_for, 'main.send_test'), 'Print a test letter', + False, ), ( mock_get_service_letter_template, partial(url_for, 'main.send_one_off'), 'Print a test letter', + False, ), ]) def test_send_one_off_or_test_has_correct_page_titles( @@ -385,6 +393,7 @@ def test_send_one_off_or_test_has_correct_page_titles( template_mock, partial_url, expected_h1, + tour_shown, ): template_mock(mocker) @@ -399,6 +408,8 @@ def test_send_one_off_or_test_has_correct_page_titles( assert response.status_code == 200 assert page.h1.text.strip() == expected_h1 + assert (len(page.select('.banner-tour')) == 1) == tour_shown + @pytest.mark.parametrize('template_mock, expected_link_text, expected_link_url', [ (mock_get_service_template, 'Use my phone number', partial(url_for, 'main.send_test')), From 74e79e175b70f6438c5a445f3fdd0abdace0e880 Mon Sep 17 00:00:00 2001 From: minglis Date: Thu, 1 Jun 2017 16:53:36 +0100 Subject: [PATCH 23/40] Fix typo --- tests/app/main/views/test_dashboard.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index 731e5df03..a6ff4e6a8 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -698,7 +698,7 @@ def test_should_show_all_jobs_with_valid_statuses( }) -def test_should_show_remaining_fee_tier_count( +def test_should_show_remaining_free_tier_count( logged_in_client, mock_get_service_templates, mock_get_template_statistics, From 8f70dcf549cc8ee49e0191cd85a70efabd97b456 Mon Sep 17 00:00:00 2001 From: minglis Date: Thu, 1 Jun 2017 16:55:49 +0100 Subject: [PATCH 24/40] Fix typo --- tests/app/main/views/test_dashboard.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index a6ff4e6a8..cda5b9f21 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -719,7 +719,7 @@ def test_should_show_remaining_free_tier_count( assert 'free text messages left' in response.get_data(as_text=True) -def test_should_show_cost_if_exceeded_fee_tier_count( +def test_should_show_cost_if_exceeded_free_tier_count( logged_in_client, mock_get_service_templates, mock_get_template_statistics, From 0dbd3fa10d6b0a5923a4529d97dacb859eb5a8b8 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 2 Jun 2017 10:41:42 +0100 Subject: [PATCH 25/40] Revert "Merge pull request #1288 from alphagov/add-new-rate-logic" This reverts commit adbff63d1f3c6ded12ccd4ddb7878bf905c4d98d, reversing changes made to 80f0b4b2a2fa95e62d1798371647e8f21c9a1d5c. --- app/config.py | 2 - app/main/views/dashboard.py | 17 ++----- app/notify_client/service_api_client.py | 6 --- app/templates/views/dashboard/_usage.html | 2 +- tests/app/main/views/test_dashboard.py | 54 ----------------------- tests/app/main/views/test_sign_out.py | 1 - tests/conftest.py | 9 ---- 7 files changed, 4 insertions(+), 87 deletions(-) diff --git a/app/config.py b/app/config.py index 48bb5fa8d..f9b737929 100644 --- a/app/config.py +++ b/app/config.py @@ -59,8 +59,6 @@ class Config(object): ACTIVITY_STATS_LIMIT_DAYS = 7 TEST_MESSAGE_FILENAME = 'Report' - SMS_FREE_TIER_AMOUNT = 250000 - STATSD_ENABLED = False STATSD_HOST = "statsd.hostedgraphite.com" STATSD_PORT = 8125 diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index 9091a2036..2d59d822f 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -2,7 +2,6 @@ from datetime import datetime from functools import partial from flask import ( render_template, - current_app, url_for, session, jsonify, @@ -187,7 +186,7 @@ def get_dashboard_partials(service_id): 'has_jobs': bool(immediate_jobs), 'usage': render_template( 'views/dashboard/_usage.html', - **calculate_free_tier_usage(service_api_client.get_yearly_sms_unit_count_and_cost( + **calculate_usage(service_api_client.get_service_usage( service_id, get_current_financial_year(), )) @@ -202,19 +201,9 @@ def get_dashboard_totals(statistics): return statistics -def calculate_free_tier_usage(usage): - sms_free_allowance = current_app.config['SMS_FREE_TIER_AMOUNT'] - - return({ - 'sms_chargeable': max(0, usage['billable_sms_units'] - sms_free_allowance), - 'total_sms_bill': usage['billable_sms_units'], - 'total_sms_cost': usage['total_cost'], - 'sms_allowance_remaining': sms_free_allowance - int(usage['billable_sms_units']) - }) - - def calculate_usage(usage): - sms_free_allowance = current_app.config['SMS_FREE_TIER_AMOUNT'] + # TODO: Don't hardcode these - get em from the API + sms_free_allowance = 250000 sms_rate = 0 if len(usage) == 0 else usage[0].get("rate", 0) sms_sent = get_sum_billing_units(breakdown for breakdown in usage if breakdown['notification_type'] == 'sms') diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index fd672a464..dc48d0043 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -221,12 +221,6 @@ class ServiceAPIClient(NotifyAdminAPIClient): params=dict(year=year) ) - def get_yearly_sms_unit_count_and_cost(self, service_id, year=None): - return self.get( - '/service/{0}/yearly-sms-billable-units'.format(service_id), - params=dict(year=year) - ) - def get_monthly_notification_stats(self, service_id, year): return self.get(url='/service/{}/notifications/monthly?year={}'.format(service_id, year)) diff --git a/app/templates/views/dashboard/_usage.html b/app/templates/views/dashboard/_usage.html index 7a908d53c..ff0530549 100644 --- a/app/templates/views/dashboard/_usage.html +++ b/app/templates/views/dashboard/_usage.html @@ -10,7 +10,7 @@
{% if sms_chargeable %} {{ big_number( - total_sms_cost, + (sms_chargeable * sms_rate), 'spent on text messages', currency="£", smaller=True diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index cda5b9f21..b6c8b9404 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -44,7 +44,6 @@ def test_get_started( mock_get_jobs, mock_get_detailed_service, mock_get_usage, - mock_get_yearly_sms_unit_count_and_cost ): mock_template_stats = mocker.patch('app.template_statistics_client.get_template_statistics_for_service', return_value=copy.deepcopy(stub_template_stats)) @@ -63,7 +62,6 @@ def test_get_started_is_hidden_once_templates_exist( mock_get_jobs, mock_get_detailed_service, mock_get_usage, - mock_get_yearly_sms_unit_count_and_cost ): mock_template_stats = mocker.patch('app.template_statistics_client.get_template_statistics_for_service', return_value=copy.deepcopy(stub_template_stats)) @@ -81,7 +79,6 @@ def test_should_show_recent_templates_on_dashboard( mock_get_jobs, mock_get_detailed_service, mock_get_usage, - mock_get_yearly_sms_unit_count_and_cost ): mock_template_stats = mocker.patch('app.template_statistics_client.get_template_statistics_for_service', return_value=copy.deepcopy(stub_template_stats)) @@ -147,7 +144,6 @@ def test_should_show_upcoming_jobs_on_dashboard( mock_get_detailed_service, mock_get_jobs, mock_get_usage, - mock_get_yearly_sms_unit_count_and_cost ): response = logged_in_client.get(url_for('main.service_dashboard', service_id=SERVICE_ONE_ID)) @@ -178,7 +174,6 @@ def test_should_show_recent_jobs_on_dashboard( mock_get_detailed_service, mock_get_jobs, mock_get_usage, - mock_get_yearly_sms_unit_count_and_cost ): response = logged_in_client.get(url_for('main.service_dashboard', service_id=SERVICE_ONE_ID)) @@ -297,7 +292,6 @@ def test_menu_send_messages( mock_get_template_statistics, mock_get_detailed_service, mock_get_usage, - mock_get_yearly_sms_unit_count_and_cost ): with app_.test_request_context(): resp = _test_dashboard_menu( @@ -328,7 +322,6 @@ def test_menu_manage_service( mock_get_template_statistics, mock_get_detailed_service, mock_get_usage, - mock_get_yearly_sms_unit_count_and_cost ): with app_.test_request_context(): resp = _test_dashboard_menu( @@ -358,7 +351,6 @@ def test_menu_manage_api_keys( mock_get_template_statistics, mock_get_detailed_service, mock_get_usage, - mock_get_yearly_sms_unit_count_and_cost ): with app_.test_request_context(): resp = _test_dashboard_menu( @@ -388,7 +380,6 @@ def test_menu_all_services_for_platform_admin_user( mock_get_template_statistics, mock_get_detailed_service, mock_get_usage, - mock_get_yearly_sms_unit_count_and_cost ): with app_.test_request_context(): resp = _test_dashboard_menu( @@ -418,7 +409,6 @@ def test_route_for_service_permissions( mock_get_template_statistics, mock_get_detailed_service, mock_get_usage, - mock_get_yearly_sms_unit_count_and_cost ): with app_.test_request_context(): validate_route_permission( @@ -455,7 +445,6 @@ def test_service_dashboard_updates_gets_dashboard_totals( mock_get_detailed_service, mock_get_jobs, mock_get_usage, - mock_get_yearly_sms_unit_count_and_cost ): mocker.patch('app.main.views.dashboard.get_dashboard_totals', return_value={ 'email': {'requested': 123, 'delivered': 0, 'failed': 0}, @@ -679,7 +668,6 @@ def test_should_show_all_jobs_with_valid_statuses( mock_get_detailed_service, mock_get_jobs, mock_get_usage, - mock_get_yearly_sms_unit_count_and_cost ): get_dashboard_partials(service_id=SERVICE_ONE_ID) @@ -696,45 +684,3 @@ def test_should_show_all_jobs_with_valid_statuses( 'ready to send', 'sent to dvla' }) - - -def test_should_show_remaining_free_tier_count( - logged_in_client, - mock_get_service_templates, - mock_get_template_statistics, - mock_get_detailed_service, - mock_get_jobs, - mock_get_usage, - mocker -): - mocker.patch( - 'app.service_api_client.get_yearly_sms_unit_count_and_cost', - return_value={"billable_sms_units": 100, "total_cost": 200.0} - ) - - response = logged_in_client.get(url_for('main.service_dashboard', service_id=SERVICE_ONE_ID)) - - assert response.status_code == 200 - assert '249,900' in response.get_data(as_text=True) - assert 'free text messages left' in response.get_data(as_text=True) - - -def test_should_show_cost_if_exceeded_free_tier_count( - logged_in_client, - mock_get_service_templates, - mock_get_template_statistics, - mock_get_detailed_service, - mock_get_jobs, - mock_get_usage, - mocker -): - mocker.patch( - 'app.service_api_client.get_yearly_sms_unit_count_and_cost', - return_value={"billable_sms_units": 300000, "total_cost": 1500.50} - ) - - response = logged_in_client.get(url_for('main.service_dashboard', service_id=SERVICE_ONE_ID)) - - assert response.status_code == 200 - assert '£1,500.50' in response.get_data(as_text=True) - assert 'spent on text messages' in response.get_data(as_text=True) diff --git a/tests/app/main/views/test_sign_out.py b/tests/app/main/views/test_sign_out.py index 9c7a21f04..d4dc88b46 100644 --- a/tests/app/main/views/test_sign_out.py +++ b/tests/app/main/views/test_sign_out.py @@ -23,7 +23,6 @@ def test_sign_out_user( mock_has_permissions, mock_get_template_statistics, mock_get_detailed_service, - mock_get_yearly_sms_unit_count_and_cost, mock_get_usage, ): with logged_in_client.session_transaction() as session: diff --git a/tests/conftest.py b/tests/conftest.py index 776411230..e9171cb8e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1335,15 +1335,6 @@ def mock_get_usage(mocker, service_one, fake_uuid): 'app.service_api_client.get_service_usage', side_effect=_get_usage) -@pytest.fixture(scope='function') -def mock_get_yearly_sms_unit_count_and_cost(mocker, service_one, fake_uuid): - def _get_usage(service_id, year=None): - return {"billable_sms_units": 100, "total_cost": 200.0} - - return mocker.patch( - 'app.service_api_client.get_yearly_sms_unit_count_and_cost', side_effect=_get_usage) - - @pytest.fixture(scope='function') def mock_get_billable_units(mocker): def _get_usage(service_id, year): From b73a3067185f7dbd616a01714de11af29af0bdb3 Mon Sep 17 00:00:00 2001 From: Pete Herlihy Date: Fri, 2 Jun 2017 11:26:22 +0100 Subject: [PATCH 26/40] 52-53 for Immigration Asylum Chamber Tribunal --- app/templates/views/signedout.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/templates/views/signedout.html b/app/templates/views/signedout.html index f27df26f5..9dee799ef 100644 --- a/app/templates/views/signedout.html +++ b/app/templates/views/signedout.html @@ -117,7 +117,7 @@
-
52
+
53
services
From fe24501075e9449f1621a8e23e6967d3966c9b2d Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 2 Jun 2017 16:25:24 +0100 Subject: [PATCH 27/40] Add a platform admin button to service-settings page to turn the inbound_sms messaging on and off. If clicked you will be prompted to enter a sms sender number, when setting the permission on or off. Team members will always be able to see the number, but will only be able to change it if the inbound_sms permission is off. --- app/main/views/service_settings.py | 26 +++++++++--- app/notify_client/service_api_client.py | 1 + app/templates/views/service-settings.html | 14 ++++++- tests/__init__.py | 4 +- tests/app/main/views/test_service_settings.py | 40 +++++++++++++++++++ tests/conftest.py | 3 +- 6 files changed, 78 insertions(+), 10 deletions(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index e8a32fc1b..97cf75fad 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -49,6 +49,7 @@ def service_settings(service_id): letter_branding=letter_branding_organisations.get( current_service.get('dvla_organisation', '001') ), + can_receive_inbound=('inbound_sms' in current_service['permissions']), letter_contact_block=Field(current_service['letter_contact_block'], html='escape') ) @@ -267,14 +268,27 @@ def service_set_reply_to_email(service_id): @user_has_permissions('manage_settings', admin_override=True) def service_set_sms_sender(service_id): form = ServiceSmsSender() + if form.validate_on_submit(): + set_inbound_sms = request.args.get('set_inbound_sms', False) + if set_inbound_sms == 'True': + permissions = current_service['permissions'] + if 'inbound_sms' in permissions: + permissions.remove('inbound_sms') + else: + permissions.append('inbound_sms') + service_api_client.update_service_with_properties( + current_service['id'], + {'permissions': permissions, + 'sms_sender': form.sms_sender.data or None} + ) + else: + service_api_client.update_service( + current_service['id'], + sms_sender=form.sms_sender.data or None + ) + return redirect(url_for('.service_settings', service_id=service_id)) if request.method == 'GET': form.sms_sender.data = current_service.get('sms_sender') - if form.validate_on_submit(): - service_api_client.update_service( - current_service['id'], - sms_sender=form.sms_sender.data or None - ) - return redirect(url_for('.service_settings', service_id=service_id)) return render_template( 'views/service-settings/set-sms-sender.html', form=form) diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index dc48d0043..e97919257 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -94,6 +94,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): 'organisation', 'letter_contact_block', 'dvla_organisation', + 'permissions' } if disallowed_attributes: raise TypeError('Not allowed to update service attributes: {}'.format( diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index 64874597b..34580721c 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -36,8 +36,13 @@ {% call row() %} {{ text_field('Text message sender') }} - {{ text_field(current_service.sms_sender) }} - {{ edit_field('Change', url_for('.service_set_sms_sender', service_id=current_service.id)) }} + {{ text_field(current_service.sms_sender}} + {% if current_user.has_permissions([], admin_override=True) or not can_receive_inbound %} + {{ edit_field('Change', url_for('.service_set_sms_sender', service_id=current_service.id, set_inbound_sms=False)) }} + {% else %} + {{ text_field('') }} + {% endif %} + {% endcall %} {% call row() %} @@ -162,6 +167,11 @@ {% endif %} +
  • + + {{ 'Stop inbound sms' if can_receive_inbound else 'Allow inbound sms' }} + +
    • {% endif %} diff --git a/tests/__init__.py b/tests/__init__.py index bb0311efe..9454fe727 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -54,7 +54,8 @@ def service_json( organisation=None, branding='govuk', created_at=None, - letter_contact_block=None + letter_contact_block=None, + permissions=[] ): if users is None: users = [] @@ -76,6 +77,7 @@ def service_json( 'created_at': created_at or str(datetime.utcnow()), 'letter_contact_block': letter_contact_block, 'dvla_organisation': '001', + 'permissions': permissions, } diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 6fc6ea56e..245460259 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -92,6 +92,22 @@ def test_if_cant_send_letters_then_cant_see_letter_contact_block( assert 'Letter contact block' not in response.get_data(as_text=True) +def test_if_can_receive_inbound_then_cant_change_sms_sender( + logged_in_client, + service_one, + mock_get_letter_organisations, +): + service_one['permissions'] = ['inbound_sms'] + service_one['sms_sender'] = 'SomeNumber' + response = logged_in_client.get(url_for( + 'main.service_settings', service_id=service_one['id'] + )) + assert 'Text message sender SomeNumber Change' not in response.get_data(as_text=True) + assert url_for('.service_set_sms_sender', service_id=service_one['id'], + set_inbound_sms=False) not in response.get_data(as_text=True) + assert 'SomeNumber' in response.get_data(as_text=True) + + def test_letter_contact_block_shows_none_if_not_set( logged_in_client, service_one, @@ -615,6 +631,30 @@ def test_set_text_message_sender( ) +def test_set_text_message_sender_and_inbound_sms( + logged_in_client, + service_one, + mock_get_letter_organisations, + mocker, +): + + update_service_mock = mocker.patch('app.service_api_client.update_service_with_properties', + return_value=service_one) + + data = {"sms_sender": "elevenchars"} + response = logged_in_client.post(url_for('main.service_set_sms_sender', service_id=service_one['id'], + set_inbound_sms=True), + data=data, + follow_redirects=True) + assert response.status_code == 200 + + update_service_mock.assert_called_with( + service_one['id'], + {'permissions': ['inbound_sms'], + 'sms_sender': "elevenchars"} + ) + + @pytest.mark.parametrize('content, expected_error', [ ("", "Can’t be empty"), ("twelvecharss", "Enter 11 characters or fewer"), diff --git a/tests/conftest.py b/tests/conftest.py index e9171cb8e..a59ce5382 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -167,7 +167,8 @@ def mock_update_service(mocker): 'restricted', 'email_from', 'reply_to_email_address', - 'sms_sender' + 'sms_sender', + 'permissions' ]} ) return {'data': service} From 1960a252ea2c3c2dbaabd35c2c43a61ca6010316 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 2 Jun 2017 16:37:45 +0100 Subject: [PATCH 28/40] Fix merge conflict and typo in html --- app/templates/views/service-settings.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/templates/views/service-settings.html b/app/templates/views/service-settings.html index 34580721c..0a2490b9b 100644 --- a/app/templates/views/service-settings.html +++ b/app/templates/views/service-settings.html @@ -36,7 +36,7 @@ {% call row() %} {{ text_field('Text message sender') }} - {{ text_field(current_service.sms_sender}} + {{ text_field(current_service.sms_sender) }} {% if current_user.has_permissions([], admin_override=True) or not can_receive_inbound %} {{ edit_field('Change', url_for('.service_set_sms_sender', service_id=current_service.id, set_inbound_sms=False)) }} {% else %} From e4bf0c3e7d5ae262eb1c8d21a99818a81944d31e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 2 Jun 2017 17:01:26 +0100 Subject: [PATCH 29/40] =?UTF-8?q?Don=E2=80=99t=20link=20to=20one-off=20rep?= =?UTF-8?q?orts=20from=20activity?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There’s no need to see the ‘Report’ page for a one-off message – pretty much all the info about the notification is there on the page. And ‘Sent from _Report_’ looks weird. We should display it more like an API call instead. --- app/templates/views/activity/notifications.html | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/templates/views/activity/notifications.html b/app/templates/views/activity/notifications.html index 42c6cf169..06158fb75 100644 --- a/app/templates/views/activity/notifications.html +++ b/app/templates/views/activity/notifications.html @@ -21,7 +21,10 @@ {{ item.to }}

    - {% if item.job %} + {% if item.job and item.job.original_file_name == 'Report' %} + {{ item.template.name }} + sent to one recipient + {% elif item.job %} From {{ item.job.original_file_name }} {% else %} {{ item.template.name }} From f40e81bc3aa09d07c3fe7f8abe0ceb7fe2a640d6 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 5 Jun 2017 12:01:39 +0100 Subject: [PATCH 30/40] Added a test to turn off the inbound permission. --- tests/app/main/views/test_service_settings.py | 28 ++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 245460259..73de6916c 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -637,7 +637,7 @@ def test_set_text_message_sender_and_inbound_sms( mock_get_letter_organisations, mocker, ): - + service_one['permissions'] = [] update_service_mock = mocker.patch('app.service_api_client.update_service_with_properties', return_value=service_one) @@ -653,6 +653,32 @@ def test_set_text_message_sender_and_inbound_sms( {'permissions': ['inbound_sms'], 'sms_sender': "elevenchars"} ) + assert app.current_service['permissions'] == ['inbound_sms'] + + +def test_turn_inbound_sms_off( + logged_in_client, + service_one, + mock_get_letter_organisations, + mocker, +): + service_one['permissions'] = ['inbound_sms'] + update_service_mock = mocker.patch('app.service_api_client.update_service_with_properties', + return_value=service_one) + + data = {"sms_sender": "elevenchars"} + response = logged_in_client.post(url_for('main.service_set_sms_sender', service_id=service_one['id'], + set_inbound_sms=True), + data=data, + follow_redirects=True) + assert response.status_code == 200 + + update_service_mock.assert_called_with( + service_one['id'], + {'permissions': [], + 'sms_sender': "elevenchars"} + ) + assert app.current_service['permissions'] == [] @pytest.mark.parametrize('content, expected_error', [ From da77737c1623d2feffe9be9540815653c3a82a3b Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 5 Jun 2017 12:13:40 +0100 Subject: [PATCH 31/40] Refactor if statement --- app/main/views/service_settings.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 97cf75fad..56fe13772 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -270,7 +270,7 @@ def service_set_sms_sender(service_id): form = ServiceSmsSender() if form.validate_on_submit(): set_inbound_sms = request.args.get('set_inbound_sms', False) - if set_inbound_sms == 'True': + if set_inbound_sms: permissions = current_service['permissions'] if 'inbound_sms' in permissions: permissions.remove('inbound_sms') From 9d7510a52f4f7c543feecce8f8a53ca4aaed97d8 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 5 Jun 2017 13:46:02 +0100 Subject: [PATCH 32/40] Revert if statement refactor, if set_inbound_sms is False it still resolves to True, because it is not a boolean but a string. --- app/main/views/service_settings.py | 2 +- tests/app/main/views/test_service_settings.py | 24 +++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 56fe13772..97cf75fad 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -270,7 +270,7 @@ def service_set_sms_sender(service_id): form = ServiceSmsSender() if form.validate_on_submit(): set_inbound_sms = request.args.get('set_inbound_sms', False) - if set_inbound_sms: + if set_inbound_sms == 'True': permissions = current_service['permissions'] if 'inbound_sms' in permissions: permissions.remove('inbound_sms') diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 73de6916c..4d9ba1814 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -681,6 +681,30 @@ def test_turn_inbound_sms_off( assert app.current_service['permissions'] == [] +def test_set_text_message_sender_and_not_inbound_sms( + logged_in_client, + service_one, + mock_get_letter_organisations, + mocker, +): + service_one['permissions'] = [] + update_service_mock = mocker.patch('app.service_api_client.update_service', + return_value=service_one) + + data = {"sms_sender": "elevenchars"} + response = logged_in_client.post(url_for('main.service_set_sms_sender', service_id=service_one['id'], + set_inbound_sms=False), + data=data, + follow_redirects=True) + assert response.status_code == 200 + + update_service_mock.assert_called_with( + service_one['id'], + sms_sender="elevenchars" + ) + assert app.current_service['permissions'] == [] + + @pytest.mark.parametrize('content, expected_error', [ ("", "Can’t be empty"), ("twelvecharss", "Enter 11 characters or fewer"), From 3d45454330e812fb46062fd22747708a77dead63 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 5 Jun 2017 13:51:53 +0100 Subject: [PATCH 33/40] Add Care Quality Commission to email whitelist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CQC is an executive non-departmental public body, sponsored by the Department of Health. They have asked to be allowed to register for Notify using the `cqc.org.uk` and `digital.cqc.org.uk` domains. We know that this really is there domain because it’s linked to from here: https://www.gov.uk/government/organisations/care-quality-commission --- app/config.py | 1 + tests/app/main/test_validators.py | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/app/config.py b/app/config.py index f9b737929..c19849a7e 100644 --- a/app/config.py +++ b/app/config.py @@ -82,6 +82,7 @@ class Config(object): r"scotent\.co\.uk", r"assembly\.wales", r"cjsm\.net", + r"cqc\.org\.uk", ] diff --git a/tests/app/main/test_validators.py b/tests/app/main/test_validators.py index 1a77885a9..e9bf96db2 100644 --- a/tests/app/main/test_validators.py +++ b/tests/app/main/test_validators.py @@ -90,7 +90,9 @@ def _gen_mock_field(x): 'test@hmcts.net', 'test@scotent.co.uk', 'test@assembly.wales', - 'test@cjsm.net' + 'test@cjsm.net', + 'test@cqc.org.uk', + 'test@digital.cqc.org.uk', ]) def test_valid_list_of_white_list_email_domains( client, From e373296bd9726f821ffe8f4db4486112f82daf06 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 22 May 2017 17:02:03 +0100 Subject: [PATCH 34/40] Show inbound messages on the dashboard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit adds two things: a section on the dashboard to show how many inbound messages the service has received in the last 7 days, and how recently an inbound message has been received --- Doesn’t show the contents of any messages, just like how the rest of the dashboard is an aggregation, never individual messages. a page to show all the inbound messages the service has received in the last 7 days --- This shows the first line of the message. Eventually this will link through to a ‘conversation’ page, where a service can see all the messages it’s received from a given phone number. --- .../stylesheets/components/big-number.scss | 23 +++++ app/assets/stylesheets/components/table.scss | 8 ++ app/assets/stylesheets/views/dashboard.scss | 4 + app/main/views/dashboard.py | 14 +++ app/notify_client/service_api_client.py | 11 +++ app/templates/components/big-number.html | 25 ++--- app/templates/views/dashboard/_inbox.html | 19 ++++ app/templates/views/dashboard/dashboard.html | 2 + app/templates/views/dashboard/inbox.html | 43 +++++++++ tests/app/main/views/test_dashboard.py | 96 ++++++++++++++++++- tests/app/main/views/test_sign_out.py | 1 + tests/app/test_utils.py | 6 +- tests/conftest.py | 62 ++++++++++++ 13 files changed, 300 insertions(+), 14 deletions(-) create mode 100644 app/templates/views/dashboard/_inbox.html create mode 100644 app/templates/views/dashboard/inbox.html diff --git a/app/assets/stylesheets/components/big-number.scss b/app/assets/stylesheets/components/big-number.scss index 2375757dd..d2cfba1b0 100644 --- a/app/assets/stylesheets/components/big-number.scss +++ b/app/assets/stylesheets/components/big-number.scss @@ -129,3 +129,26 @@ } } + +.big-number-meta-wrapper { + + position: relative; + margin: $gutter-half 0 $gutter 0; + background: $govuk-blue; + + .big-number-meta { + + padding: ($gutter / 3) $gutter-half; + color: $white; + pointer-events: none; + + @include media(desktop) { + position: absolute; + bottom: 7px; + right: 5px; + text-align: right; + } + + } + +} diff --git a/app/assets/stylesheets/components/table.scss b/app/assets/stylesheets/components/table.scss index 7d1931395..152d209be 100644 --- a/app/assets/stylesheets/components/table.scss +++ b/app/assets/stylesheets/components/table.scss @@ -210,3 +210,11 @@ a.table-show-more-link { border-bottom: 1px solid $border-colour; padding: 0.75em 0 0.5625em 0; } + +.wide-left-hand-column { + display: block; + max-width: 560px; + white-space: nowrap; + overflow: hidden; + text-overflow: ellipsis; +} diff --git a/app/assets/stylesheets/views/dashboard.scss b/app/assets/stylesheets/views/dashboard.scss index a99623384..a1fed973a 100644 --- a/app/assets/stylesheets/views/dashboard.scss +++ b/app/assets/stylesheets/views/dashboard.scss @@ -94,3 +94,7 @@ } } + +.align-with-message-body { + margin-top: $gutter * 5 / 6; +} diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index 2d59d822f..aef002398 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -135,6 +135,16 @@ def monthly(service_id): ) +@main.route("/services//inbox") +@login_required +@user_has_permissions('manage_settings', admin_override=True) +def inbox(service_id): + return render_template( + 'views/dashboard/inbox.html', + messages=service_api_client.get_inbound_sms(service_id), + ) + + def aggregate_usage(template_statistics, sort_key='count'): return sorted( template_statistics, @@ -166,6 +176,10 @@ def get_dashboard_partials(service_id): 'views/dashboard/_upcoming.html', scheduled_jobs=scheduled_jobs ), + 'inbox': render_template( + 'views/dashboard/_inbox.html', + inbound_sms_summary=service_api_client.get_inbound_sms_summary(service_id), + ), 'totals': render_template( 'views/dashboard/_totals.html', service_id=service_id, diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index e97919257..b5c82656c 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -1,4 +1,5 @@ from __future__ import unicode_literals + from flask import url_for from app.utils import BrowsableItem from app.notify_client import _attach_current_user, NotifyAdminAPIClient @@ -237,6 +238,16 @@ class ServiceAPIClient(NotifyAdminAPIClient): params=dict(year=year) ) + def get_inbound_sms(self, service_id): + return self.get( + '/service/{}/inbound-sms'.format(service_id) + )['data'] + + def get_inbound_sms_summary(self, service_id): + return self.get( + '/service/{}/inbound-sms/summary'.format(service_id) + ) + class ServicesBrowsableItem(BrowsableItem): @property diff --git a/app/templates/components/big-number.html b/app/templates/components/big-number.html index af0e56383..048a0f3df 100644 --- a/app/templates/components/big-number.html +++ b/app/templates/components/big-number.html @@ -31,22 +31,25 @@ failure_percentage, danger_zone=False, failure_link=None, - link=None + link=None, + show_failures=True ) %}

    {{ big_number(number, label, link=link) }} -
    - {% if failures %} - {% if failure_link %} - + {% if show_failures %} +
    + {% if failures %} + {% if failure_link %} + + {{ "{:,}".format(failures) }} failed – {{ failure_percentage }}% + + {% else %} {{ "{:,}".format(failures) }} failed – {{ failure_percentage }}% - + {% endif %} {% else %} - {{ "{:,}".format(failures) }} failed – {{ failure_percentage }}% + No failures {% endif %} - {% else %} - No failures - {% endif %} -
    +
    + {% endif %}
    {% endmacro %} diff --git a/app/templates/views/dashboard/_inbox.html b/app/templates/views/dashboard/_inbox.html new file mode 100644 index 000000000..7cba62016 --- /dev/null +++ b/app/templates/views/dashboard/_inbox.html @@ -0,0 +1,19 @@ +{% from "components/big-number.html" import big_number, big_number_with_status %} + +
    +
    + {{ + big_number_with_status( + inbound_sms_summary.count, + 'text messages received', + link=url_for('.inbox', service_id=current_service.id), + show_failures=False + ) + }} +
    + {% if inbound_sms_summary.latest_message %} + latest message {{ inbound_sms_summary.latest_message | format_delta }} + {% endif %} +
    +
    +
    diff --git a/app/templates/views/dashboard/dashboard.html b/app/templates/views/dashboard/dashboard.html index c18fbb4e4..8e7e58f40 100644 --- a/app/templates/views/dashboard/dashboard.html +++ b/app/templates/views/dashboard/dashboard.html @@ -29,6 +29,8 @@ In the last 7 days + {{ ajax_block(partials, updates_url, 'inbox') }} + {{ ajax_block(partials, updates_url, 'totals') }} {{ show_more( url_for('.monthly', service_id=current_service.id), diff --git a/app/templates/views/dashboard/inbox.html b/app/templates/views/dashboard/inbox.html new file mode 100644 index 000000000..ace1cafbd --- /dev/null +++ b/app/templates/views/dashboard/inbox.html @@ -0,0 +1,43 @@ +{% from "components/table.html" import list_table, field, hidden_field_heading, right_aligned_field_heading, row_heading %} +{% from "components/message-count-label.html" import message_count_label %} + +{% extends "withnav_template.html" %} + +{% block service_page_title %} + Inbox +{% endblock %} + +{% block maincolumn_content %} + +

    + Received text messages +

    +
    + {% call(item, row_number) list_table( + messages, + caption="Inbox", + caption_visible=False, + empty_message='When users text your service’s phone number ({}) you’ll see the messages here'.format(current_service.sms_sender), + field_headings=[ + 'From', + 'First two lines of message' + ], + field_headings_visible=False + ) %} + {% call field() %} + {{ item.user_number }} + {{ item.content }} + {% endcall %} + {% call field(align='right') %} + + {{ item.created_at | format_delta }} + + {% endcall %} + {% endcall %} + {% if messages %} +

    + Showing all {{ messages | length }} messages +

    + {% endif %} +
    +{% endblock %} diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index b6c8b9404..32a2e5319 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -5,6 +5,7 @@ from unittest.mock import call, ANY from flask import url_for import pytest from bs4 import BeautifulSoup +from datetime import datetime, timedelta from freezegun import freeze_time from app.main.views.dashboard import ( @@ -18,7 +19,11 @@ from app.main.views.dashboard import ( ) from tests import validate_route_permission -from tests.conftest import SERVICE_ONE_ID +from tests.conftest import ( + SERVICE_ONE_ID, + mock_get_inbound_sms_summary, + mock_get_inbound_sms_summary_with_no_messages, +) from tests.app.test_utils import normalize_spaces stub_template_stats = [ @@ -44,6 +49,7 @@ def test_get_started( mock_get_jobs, mock_get_detailed_service, mock_get_usage, + mock_get_inbound_sms_summary, ): mock_template_stats = mocker.patch('app.template_statistics_client.get_template_statistics_for_service', return_value=copy.deepcopy(stub_template_stats)) @@ -62,6 +68,7 @@ def test_get_started_is_hidden_once_templates_exist( mock_get_jobs, mock_get_detailed_service, mock_get_usage, + mock_get_inbound_sms_summary, ): mock_template_stats = mocker.patch('app.template_statistics_client.get_template_statistics_for_service', return_value=copy.deepcopy(stub_template_stats)) @@ -72,6 +79,83 @@ def test_get_started_is_hidden_once_templates_exist( assert 'Get started' not in response.get_data(as_text=True) +@pytest.mark.parametrize('inbound_summary_mock, expected_text', [ + (mock_get_inbound_sms_summary_with_no_messages, '0 text messages received'), + (mock_get_inbound_sms_summary, '99 text messages received latest message just now'), +]) +def test_inbound_messages_shows_count_of_messages( + logged_in_client, + mocker, + mock_get_service_templates_when_no_templates_exist, + mock_get_jobs, + mock_get_detailed_service, + mock_get_template_statistics, + mock_get_usage, + inbound_summary_mock, + expected_text, +): + + inbound_summary_mock(mocker) + + response = logged_in_client.get(url_for('main.service_dashboard', service_id=SERVICE_ONE_ID)) + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + + assert response.status_code == 200 + assert normalize_spaces(page.select('.big-number-meta-wrapper')[0].text) == expected_text + assert page.select('.big-number-meta-wrapper a')[0]['href'] == url_for( + 'main.inbox', service_id=SERVICE_ONE_ID + ) + + +@pytest.mark.parametrize('index, expected_row', enumerate([ + '07900900000 foo 1 hour ago', + '07900900001 foo 2 hours ago', + '07900900002 foo 3 hours ago', + '07900900003 foo 4 hours ago', + '07900900004 foo 5 hours ago', +])) +def test_inbox_showing_inbound_messages( + logged_in_client, + mocker, + mock_get_service_templates_when_no_templates_exist, + mock_get_jobs, + mock_get_detailed_service, + mock_get_template_statistics, + mock_get_usage, + mock_get_inbound_sms, + index, + expected_row, +): + + response = logged_in_client.get(url_for('main.inbox', service_id=SERVICE_ONE_ID)) + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + + assert response.status_code == 200 + rows = page.select('tbody tr') + assert len(rows) == 5 + assert normalize_spaces(rows[index].text) == expected_row + + +def test_empty_inbox( + logged_in_client, + mocker, + mock_get_service_templates_when_no_templates_exist, + mock_get_jobs, + mock_get_detailed_service, + mock_get_template_statistics, + mock_get_usage, + mock_get_inbound_sms_with_no_messages, +): + + response = logged_in_client.get(url_for('main.inbox', service_id=SERVICE_ONE_ID)) + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + + assert response.status_code == 200 + assert normalize_spaces(page.select('tbody tr')) == ( + 'When users text your service’s phone number (GOVUK) you’ll see the messages here' + ) + + def test_should_show_recent_templates_on_dashboard( logged_in_client, mocker, @@ -79,6 +163,7 @@ def test_should_show_recent_templates_on_dashboard( mock_get_jobs, mock_get_detailed_service, mock_get_usage, + mock_get_inbound_sms_summary, ): mock_template_stats = mocker.patch('app.template_statistics_client.get_template_statistics_for_service', return_value=copy.deepcopy(stub_template_stats)) @@ -144,6 +229,7 @@ def test_should_show_upcoming_jobs_on_dashboard( mock_get_detailed_service, mock_get_jobs, mock_get_usage, + mock_get_inbound_sms_summary, ): response = logged_in_client.get(url_for('main.service_dashboard', service_id=SERVICE_ONE_ID)) @@ -174,6 +260,7 @@ def test_should_show_recent_jobs_on_dashboard( mock_get_detailed_service, mock_get_jobs, mock_get_usage, + mock_get_inbound_sms_summary, ): response = logged_in_client.get(url_for('main.service_dashboard', service_id=SERVICE_ONE_ID)) @@ -292,6 +379,7 @@ def test_menu_send_messages( mock_get_template_statistics, mock_get_detailed_service, mock_get_usage, + mock_get_inbound_sms_summary, ): with app_.test_request_context(): resp = _test_dashboard_menu( @@ -322,6 +410,7 @@ def test_menu_manage_service( mock_get_template_statistics, mock_get_detailed_service, mock_get_usage, + mock_get_inbound_sms_summary, ): with app_.test_request_context(): resp = _test_dashboard_menu( @@ -351,6 +440,7 @@ def test_menu_manage_api_keys( mock_get_template_statistics, mock_get_detailed_service, mock_get_usage, + mock_get_inbound_sms_summary, ): with app_.test_request_context(): resp = _test_dashboard_menu( @@ -380,6 +470,7 @@ def test_menu_all_services_for_platform_admin_user( mock_get_template_statistics, mock_get_detailed_service, mock_get_usage, + mock_get_inbound_sms_summary, ): with app_.test_request_context(): resp = _test_dashboard_menu( @@ -409,6 +500,7 @@ def test_route_for_service_permissions( mock_get_template_statistics, mock_get_detailed_service, mock_get_usage, + mock_get_inbound_sms_summary, ): with app_.test_request_context(): validate_route_permission( @@ -445,6 +537,7 @@ def test_service_dashboard_updates_gets_dashboard_totals( mock_get_detailed_service, mock_get_jobs, mock_get_usage, + mock_get_inbound_sms_summary, ): mocker.patch('app.main.views.dashboard.get_dashboard_totals', return_value={ 'email': {'requested': 123, 'delivered': 0, 'failed': 0}, @@ -668,6 +761,7 @@ def test_should_show_all_jobs_with_valid_statuses( mock_get_detailed_service, mock_get_jobs, mock_get_usage, + mock_get_inbound_sms_summary, ): get_dashboard_partials(service_id=SERVICE_ONE_ID) diff --git a/tests/app/main/views/test_sign_out.py b/tests/app/main/views/test_sign_out.py index d4dc88b46..3b550f377 100644 --- a/tests/app/main/views/test_sign_out.py +++ b/tests/app/main/views/test_sign_out.py @@ -24,6 +24,7 @@ def test_sign_out_user( mock_get_template_statistics, mock_get_detailed_service, mock_get_usage, + mock_get_inbound_sms_summary, ): with logged_in_client.session_transaction() as session: assert session.get('user_id') is not None diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index 0c0da896e..9bfe8828b 100644 --- a/tests/app/test_utils.py +++ b/tests/app/test_utils.py @@ -156,5 +156,7 @@ def test_generate_notifications_csv_calls_twice_if_next_link(mocker): assert mock_get_notifications.mock_calls[1][2]['page'] == 2 -def normalize_spaces(string): - return ' '.join(string.split()) +def normalize_spaces(input): + if isinstance(input, str): + return ' '.join(input.split()) + return normalize_spaces(' '.join(item.text for item in input)) diff --git a/tests/conftest.py b/tests/conftest.py index a59ce5382..0201f99bc 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1132,6 +1132,68 @@ def mock_get_notifications_with_no_notifications(mocker): ) +@pytest.fixture(scope='function') +def mock_get_inbound_sms(mocker): + def _get_inbound_sms( + service_id, + ): + return [{ + 'user_number': '0790090000' + str(i), + 'content': 'foo', + 'created_at': (datetime.utcnow() - timedelta(minutes=60 * (i + 1))).isoformat() + } for i in range(5)] + + return mocker.patch( + 'app.service_api_client.get_inbound_sms', + side_effect=_get_inbound_sms, + ) + + +@pytest.fixture(scope='function') +def mock_get_inbound_sms_with_no_messages(mocker): + def _get_inbound_sms( + service_id, + ): + return [] + + return mocker.patch( + 'app.service_api_client.get_inbound_sms', + side_effect=_get_inbound_sms, + ) + + +@pytest.fixture(scope='function') +def mock_get_inbound_sms_summary(mocker): + def _get_inbound_sms_summary( + service_id, + ): + return { + 'count': 99, + 'latest_message': datetime.utcnow().isoformat() + } + + return mocker.patch( + 'app.service_api_client.get_inbound_sms_summary', + side_effect=_get_inbound_sms_summary, + ) + + +@pytest.fixture(scope='function') +def mock_get_inbound_sms_summary_with_no_messages(mocker): + def _get_inbound_sms_summary( + service_id, + ): + return { + 'count': 0, + 'latest_message': None + } + + return mocker.patch( + 'app.service_api_client.get_inbound_sms_summary', + side_effect=_get_inbound_sms_summary, + ) + + @pytest.fixture(scope='function') def mock_has_permissions(mocker): def _has_permission(permissions=None, any_=False, admin_override=False): From 95e613c3f928cd5fe6385aee9ad710aa88696706 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 23 May 2017 10:54:44 +0100 Subject: [PATCH 35/40] =?UTF-8?q?Clarify=20that=20big=20numbers=20mean=20?= =?UTF-8?q?=E2=80=98sent=E2=80=99?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since we now have inbound and outbound messages we should be clear about which the counts refer to. --- app/templates/views/dashboard/_totals.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/templates/views/dashboard/_totals.html b/app/templates/views/dashboard/_totals.html index d1cb19821..281a66f7b 100644 --- a/app/templates/views/dashboard/_totals.html +++ b/app/templates/views/dashboard/_totals.html @@ -6,7 +6,7 @@
    {{ big_number_with_status( statistics['email']['requested'], - message_count_label(statistics['email']['requested'], 'email', suffix=''), + message_count_label(statistics['email']['requested'], 'email', suffix='sent'), statistics['email']['failed'], statistics['email']['failed_percentage'], statistics['email']['show_warning'], @@ -17,7 +17,7 @@
    {{ big_number_with_status( statistics['sms']['requested'], - message_count_label(statistics['sms']['requested'], 'sms', suffix=''), + message_count_label(statistics['sms']['requested'], 'sms', suffix='sent'), statistics['sms']['failed'], statistics['sms']['failed_percentage'], statistics['sms']['show_warning'], From 85105fd6cd9048cb3e16d70e655eb769afeee499 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 5 Jun 2017 14:48:24 +0100 Subject: [PATCH 36/40] Only show inbound stuff if service has permission MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only services that have inbound SMS turned on should be able to see the dashboard and ‘Received messages’ page. There’s probably a cleaner way (decorator) of doing this permissions stuff, but I think it can wait until we ship this. --- app/main/views/dashboard.py | 10 +++++- app/templates/views/dashboard/_inbox.html | 30 ++++++++-------- tests/__init__.py | 4 ++- tests/app/main/views/test_dashboard.py | 44 +++++++++++++++++++++-- 4 files changed, 69 insertions(+), 19 deletions(-) diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index aef002398..b78fadc13 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -12,6 +12,7 @@ from flask_login import login_required from app.main import main from app import ( + current_service, job_api_client, service_api_client, template_statistics_client @@ -139,6 +140,10 @@ def monthly(service_id): @login_required @user_has_permissions('manage_settings', admin_override=True) def inbox(service_id): + + if 'inbound_sms' not in current_service['permissions']: + abort(403) + return render_template( 'views/dashboard/inbox.html', messages=service_api_client.get_inbound_sms(service_id), @@ -178,7 +183,10 @@ def get_dashboard_partials(service_id): ), 'inbox': render_template( 'views/dashboard/_inbox.html', - inbound_sms_summary=service_api_client.get_inbound_sms_summary(service_id), + inbound_sms_summary=( + service_api_client.get_inbound_sms_summary(service_id) + if 'inbound_sms' in current_service['permissions'] else None + ), ), 'totals': render_template( 'views/dashboard/_totals.html', diff --git a/app/templates/views/dashboard/_inbox.html b/app/templates/views/dashboard/_inbox.html index 7cba62016..c79b0888f 100644 --- a/app/templates/views/dashboard/_inbox.html +++ b/app/templates/views/dashboard/_inbox.html @@ -1,19 +1,21 @@ {% from "components/big-number.html" import big_number, big_number_with_status %}
    -
    - {{ - big_number_with_status( - inbound_sms_summary.count, - 'text messages received', - link=url_for('.inbox', service_id=current_service.id), - show_failures=False - ) - }} -
    - {% if inbound_sms_summary.latest_message %} - latest message {{ inbound_sms_summary.latest_message | format_delta }} - {% endif %} + {% if inbound_sms_summary != None %} +
    + {{ + big_number_with_status( + inbound_sms_summary.count, + 'text messages received', + link=url_for('.inbox', service_id=current_service.id), + show_failures=False + ) + }} +
    + {% if inbound_sms_summary.latest_message %} + latest message {{ inbound_sms_summary.latest_message | format_delta }} + {% endif %} +
    -
    + {% endif %}
    diff --git a/tests/__init__.py b/tests/__init__.py index 9454fe727..19e724a8a 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -55,10 +55,12 @@ def service_json( branding='govuk', created_at=None, letter_contact_block=None, - permissions=[] + permissions=None, ): if users is None: users = [] + if permissions is None: + permissions = [] return { 'id': id_, 'name': name, diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index 32a2e5319..152d0a9d0 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -79,6 +79,27 @@ def test_get_started_is_hidden_once_templates_exist( assert 'Get started' not in response.get_data(as_text=True) +def test_inbound_messages_not_visible_to_service_without_permissions( + logged_in_client, + service_one, + mock_get_service_templates_when_no_templates_exist, + mock_get_jobs, + mock_get_detailed_service, + mock_get_template_statistics, + mock_get_usage, + mock_get_inbound_sms_summary, +): + + service_one['permissions'] = [] + + response = logged_in_client.get(url_for('main.service_dashboard', service_id=SERVICE_ONE_ID)) + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + + assert response.status_code == 200 + assert not page.select('.big-number-meta-wrapper') + assert mock_get_inbound_sms_summary.called is False + + @pytest.mark.parametrize('inbound_summary_mock, expected_text', [ (mock_get_inbound_sms_summary_with_no_messages, '0 text messages received'), (mock_get_inbound_sms_summary, '99 text messages received latest message just now'), @@ -86,6 +107,7 @@ def test_get_started_is_hidden_once_templates_exist( def test_inbound_messages_shows_count_of_messages( logged_in_client, mocker, + service_one, mock_get_service_templates_when_no_templates_exist, mock_get_jobs, mock_get_detailed_service, @@ -95,6 +117,7 @@ def test_inbound_messages_shows_count_of_messages( expected_text, ): + service_one['permissions'] = ['inbound_sms'] inbound_summary_mock(mocker) response = logged_in_client.get(url_for('main.service_dashboard', service_id=SERVICE_ONE_ID)) @@ -116,7 +139,7 @@ def test_inbound_messages_shows_count_of_messages( ])) def test_inbox_showing_inbound_messages( logged_in_client, - mocker, + service_one, mock_get_service_templates_when_no_templates_exist, mock_get_jobs, mock_get_detailed_service, @@ -127,6 +150,8 @@ def test_inbox_showing_inbound_messages( expected_row, ): + service_one['permissions'] = ['inbound_sms'] + response = logged_in_client.get(url_for('main.inbox', service_id=SERVICE_ONE_ID)) page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') @@ -138,7 +163,7 @@ def test_inbox_showing_inbound_messages( def test_empty_inbox( logged_in_client, - mocker, + service_one, mock_get_service_templates_when_no_templates_exist, mock_get_jobs, mock_get_detailed_service, @@ -147,6 +172,8 @@ def test_empty_inbox( mock_get_inbound_sms_with_no_messages, ): + service_one['permissions'] = ['inbound_sms'] + response = logged_in_client.get(url_for('main.inbox', service_id=SERVICE_ONE_ID)) page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') @@ -156,6 +183,16 @@ def test_empty_inbox( ) +def test_inbox_not_accessible_to_service_without_permissions( + logged_in_client, + service_one, +): + service_one['permissions'] = [] + response = logged_in_client.get(url_for('main.inbox', service_id=SERVICE_ONE_ID)) + + assert response.status_code == 403 + + def test_should_show_recent_templates_on_dashboard( logged_in_client, mocker, @@ -759,11 +796,12 @@ def test_should_show_all_jobs_with_valid_statuses( logged_in_client, mock_get_template_statistics, mock_get_detailed_service, + mock_get_service_templates_when_no_templates_exist, mock_get_jobs, mock_get_usage, mock_get_inbound_sms_summary, ): - get_dashboard_partials(service_id=SERVICE_ONE_ID) + logged_in_client.get(url_for('main.service_dashboard', service_id=SERVICE_ONE_ID)) first_call = mock_get_jobs.call_args_list[0] # first call - scheduled jobs only From 6a716ef162aa002e91de293a5591a604151ad8ff Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 6 Jun 2017 11:02:11 +0100 Subject: [PATCH 37/40] Add DATA.GOV.UK logo as email branding option --- .../email-template/crests/data_gov_uk_x2.png | Bin 0 -> 3371 bytes 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 app/assets/images/email-template/crests/data_gov_uk_x2.png diff --git a/app/assets/images/email-template/crests/data_gov_uk_x2.png b/app/assets/images/email-template/crests/data_gov_uk_x2.png new file mode 100644 index 0000000000000000000000000000000000000000..e006ef534333dceac29df736472572cbf3e4b3e0 GIT binary patch literal 3371 zcmai1XEYlQ_YNgiP^}UKQEC?zrM1)AdjvtLmBii}p|<*|U86*)RjQ(9mDb*B1+{1T zQ&l@gZT0$}^Z)ps^M1MG+;hJ@_dd__(#SxQk)D$t001y*YpIz402F~&wJa_0Dh@S7 zGF>GOe|4n4sTbNGYwwE!z@5AtP*80Tdko41W$%RZ`G!&i06;_9YH&p0?52&bGpjDU zvLoX8KP^yU4fYV5V^wO+JF&Mretz{?cA)$Ui>GTazCJ|iC^i@=enX|cAjpN4kY-SC zy6vP2a%DpcaKaHK6V6?$^J{+_FND&z<^Qa|-5xy2&q~2>mzlm_AOEm3&QA@HbaTF}r)$H}S{wE_xGKgsQW;c)lp~IxUqnnTj$(6?)C^ zzc(X$$Wj@3aU?*@q9Ph0Nh_>!2XVjmQgEu((5X3Tk9=Qh^+XIPbyl9z3g?DZyAAehA`|DE;<^5CV=&wyZ-` z6sMkHv$Up6C{FaNU!Oi)oM4A%Ff|6;%+NFL@D$>xKYu2@Fg`7-XEc4^{_7)WHLc(BItlG48Qw5B}O*Y z^U>d1Q?can6~MAm*)Rnmp(F;|mv3mY$kKgCoMZM)8u9aB<2a^eeD0_@>>?|1GY7YN z!-*8o&w9A2jX5i&WwI*iITwRy$PWaDA_{l{nz70p$B^5^JPyyIA*<5H3|@JE+MeEC z?sW}TQ&Us#&FOm$zAHLnBaS5-)H94em)D8Y3xUX_f8sw(}a9jXq)S~SQ*G1BN`eQe9oqXRb9x%8d|OVXI|$+z0y1 z;OERn31{D<$Q`p*e+8E>#QeN`O@0(5AF~o zS(9e-sD2-Cj;-9wz$$gxUyT#{X$Q*QCuhJXDCmr_5sMkD5E|xCBlm` zkX69pU5!>RO5ba_zq8jyP&`>Ny0NfVEdhg1b8vId#L+cZnV&gPqor7%tT2>4i*HH+ zJ@=DJkgXDdf281ao;C8?lGO zob!O+=R0frgLZzk%;1R3Uif|2tz@q_jREW{ij&Yt06_9LIUir9x?WT@)&!m8k$t^ zK0}Gu10HV8(b(A7jEs)L%N(yM46F9Ur|#DLYe!1EZJM3!kuP!H6d?a3fjK1KyT?QT za{ke}9{tfvCHPR>hrCIch-?_viw(H*HO7ja9U_EBO0hakcDFmCMtm|W`u`@#U}h3h z0$vL+wImlYdr#2$kar&e@!J|(uj1zOSu>7!gz!=ZSu5>W>UXKXq)SaqfO|~hVYb;Z zzwwt7X(7jUtzt0Qo42-woJeon<{8yahVmiV@b3cJf-^%d-Qi9y5|1?=H+F#<#P!oE z-1dj&rCulf<^UyPzB;}Si{nvI3g$k4c~giqh=&a?lqw?YJ-MGW2i=l*(h&al@Z#)f zc$HvLX?Jn@M@mU)D5Lk!#*{trt&-6PL_=sRdWiG)+FC^L(e|APiZn9bft>^L@sWxX8S+l8s}$FH9smO5t(^he{esABQ9^|8~b~=cB3&5Kbeo@(&A7IR{eX zV@qBZ>7jG$pE`lssJq-YBE5+v9BTbi0>~H&rjS_h5;QxeuD~ND>g|tFX0*S4y}RO8 zyXyKE&?#|XmkBK9EZnN1UK@7#w@svD@}qT#NVeNl#ZaM+#M73+I=#K&T!q?}jkPuW z<54|*eY3gQS;_(bE5Y-?M5biU#a-PVYa3*}_dS7$;HQ(M--~N9GK&*5kS-26u%^qh z2SZNF8*;BNd!k%aY*2lGOA}>d^1StcZb0nsf9gZ&Apx)gq@?I+_nz#%61B4zymu2= z7^W3nk0kD^{LlwrV?M^Qr}HRtAnx>a+%F1U0wW z@(zhcVc$s`x)%yOEaGAVgR3|qV?zT2##aSrz8W__d7;_L-&ao1jVt6+OS08){O0Q~ zFfvb3oA1q32egIh;K#Hu!>8bs(4&Vu%IwRzaQGyg6z|ygx*aA{BV&>mE-t=fgHSQD zkV9()@CSYPD%@1JYpL-Ck4T&hAXh$ZeRfQ)uD*ACu$I>OCO9i6$3L=Q5xcY4e73(3 z&n}sRN}2tzrdH#c$KXra7-JoU_}>pNxKro#v`x|8PTBm_gB6 z4Sies)L7Y&8}Gra`R*wL{sp*$=X|tC)Bbyjer;s=2SmRmyw+z~V+k{yr?MlUlB0+l z(++)bg>%uBxLabRgSGGE${amujkc)83k^d z@Ec_7u{i)3t}nReL-~z>yDMQxrFC#y48{8b@$P46K3{)|(1N;BpJdlcTH@?61s1xm zF0esDnMO{w@0Y|8#20&FaycqzooP~PYM@+BiA8$l;gICr&luoM&bhzBvHSZjj2@pT zgNAEA@D5fh;wab4PVcFP5W8ce4g9xq+Yi(o5Lqbgg zUb4yFXNvkih}%%SB|(L#9fI@28f_~<-3Z_PwSc8s-HzbHrU@HpD3?*S#7^i?NdCKg zNt@GZ&Y#O4xg=9WgP)}s=)_Vl-GYluMPUQMXIe{FEUmCHjkBw?@LBTuEv%Y50?tqEhc Date: Tue, 6 Jun 2017 12:01:15 +0100 Subject: [PATCH 38/40] 53-54 --- app/templates/views/signedout.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/templates/views/signedout.html b/app/templates/views/signedout.html index 9dee799ef..bd37ad7a5 100644 --- a/app/templates/views/signedout.html +++ b/app/templates/views/signedout.html @@ -117,7 +117,7 @@
    -
    53
    +
    54
    services
    From 67137f557088d181a3eafc28f015f67461f010ac Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 6 Jun 2017 12:44:59 +0100 Subject: [PATCH 39/40] Fix timestamp of latest inbound text on dashboard Key was misnamed. --- app/templates/views/dashboard/_inbox.html | 4 ++-- tests/conftest.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/templates/views/dashboard/_inbox.html b/app/templates/views/dashboard/_inbox.html index c79b0888f..37ad932e4 100644 --- a/app/templates/views/dashboard/_inbox.html +++ b/app/templates/views/dashboard/_inbox.html @@ -12,8 +12,8 @@ ) }}
    - {% if inbound_sms_summary.latest_message %} - latest message {{ inbound_sms_summary.latest_message | format_delta }} + {% if inbound_sms_summary.most_recent %} + latest message {{ inbound_sms_summary.most_recent | format_delta }} {% endif %}
    diff --git a/tests/conftest.py b/tests/conftest.py index 0201f99bc..958aac94d 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1169,7 +1169,7 @@ def mock_get_inbound_sms_summary(mocker): ): return { 'count': 99, - 'latest_message': datetime.utcnow().isoformat() + 'most_recent': datetime.utcnow().isoformat() } return mocker.patch( From 4259fb02bae9a4c35dff5a9d49b7ad2527ffefcc Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 7 Jun 2017 12:03:47 +0100 Subject: [PATCH 40/40] Add handler for old feedback form MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cool URIs don’t change 😎 https://www.w3.org/Provider/Style/URI We still have links to `/feedback` in our emails. These will live in people’s inboxes forever. --- app/main/views/feedback.py | 5 +++++ tests/app/main/views/test_feedback.py | 15 ++++++++++++--- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/app/main/views/feedback.py b/app/main/views/feedback.py index 775d79878..6f1631a2f 100644 --- a/app/main/views/feedback.py +++ b/app/main/views/feedback.py @@ -8,6 +8,11 @@ from app.main.forms import SupportType, Feedback, Problem, Triage from datetime import datetime +@main.route('/feedback', methods=['GET']) +def old_feedback(): + return redirect(url_for('.support')) + + @main.route('/support', methods=['GET', 'POST']) def support(): form = SupportType() diff --git a/tests/app/main/views/test_feedback.py b/tests/app/main/views/test_feedback.py index 6b4bfa0f2..97e1ae0d7 100644 --- a/tests/app/main/views/test_feedback.py +++ b/tests/app/main/views/test_feedback.py @@ -17,9 +17,18 @@ def no_redirect(): return lambda _external=True: None -def test_get_support_index_page(client): - resp = client.get(url_for('main.support')) - assert resp.status_code == 200 +@pytest.mark.parametrize('endpoint', [ + 'main.old_feedback', + 'main.support', +]) +def test_get_support_index_page( + client, + endpoint, +): + response = client.get(url_for('main.support'), follow_redirects=True) + assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert page.h1.string.strip() == 'Support' @freeze_time('2016-12-12 12:00:00.000000')