From 2ee5880e0cd3620c1fd54dccff7de03330cc6616 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 19 Jun 2017 16:54:27 +0100 Subject: [PATCH 01/17] Change URL of notification page This page is going to be used to show all notifications now, not just ones sent as one-offs. --- app/main/views/notifications.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/main/views/notifications.py b/app/main/views/notifications.py index ec51789bd..881c14ba4 100644 --- a/app/main/views/notifications.py +++ b/app/main/views/notifications.py @@ -39,7 +39,7 @@ def get_status_arg(filter_args): return REQUESTED_STATUSES -@main.route("/services//one-off-notification/") +@main.route("/services//notification/") @login_required @user_has_permissions('view_activity', admin_override=True) def view_notification(service_id, notification_id): @@ -72,7 +72,7 @@ def view_notification(service_id, notification_id): ) -@main.route("/services//one-off-notification/.json") +@main.route("/services//notification/.json") @user_has_permissions('view_activity', admin_override=True) def view_notification_updates(service_id, notification_id): return jsonify(**get_single_notification_partials( From 7d0aed0ae8405a4272135783796cd6c792f4b0ab Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 19 Jun 2017 14:29:58 +0100 Subject: [PATCH 02/17] Link to each recipient on activity and job pages We have a page for individual notifications now. On the job and activity pages each row of the table represents an individual notification. So it makes sense for these things to be linked together, so that a user can navigate from one to the other. This will make more sense once we make some more changes to the individual notification page. --- app/templates/partials/jobs/notifications.html | 4 +++- app/templates/views/activity/notifications.html | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/app/templates/partials/jobs/notifications.html b/app/templates/partials/jobs/notifications.html index 8c2964bdd..d1c319d59 100644 --- a/app/templates/partials/jobs/notifications.html +++ b/app/templates/partials/jobs/notifications.html @@ -48,7 +48,9 @@ field_headings_visible=False ) %} {% call row_heading() %} -

{{ item.to }}

+

+ {{ item.to }} +

{% endcall %} {{ notification_status_field(item) }} {% endcall %} diff --git a/app/templates/views/activity/notifications.html b/app/templates/views/activity/notifications.html index 06158fb75..d6f96a37b 100644 --- a/app/templates/views/activity/notifications.html +++ b/app/templates/views/activity/notifications.html @@ -18,7 +18,7 @@ {% call row_heading() %}

- {{ item.to }} + {{ item.to }}

{% if item.job and item.job.original_file_name == 'Report' %} From 742173dd338122f399a17c24ad72ef44934b3525 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 19 Jun 2017 14:31:08 +0100 Subject: [PATCH 03/17] Remove counts from notification page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The counts on the notification page will only ever show 1 thing. Which feels like overkill, especially if you’re only sending one-off messages. It’s also confusing when you come from the job/activity pages which have one set of numbers to then be confronted with a different set of numbers. The important stuff on this page is: - what the message was - some meta information about it Sorry Leo 😢 --- app/main/views/notifications.py | 40 ----------------- .../views/notifications/notification.html | 1 - tests/app/main/views/test_notifications.py | 43 ------------------- 3 files changed, 84 deletions(-) diff --git a/app/main/views/notifications.py b/app/main/views/notifications.py index 881c14ba4..29f5bda5f 100644 --- a/app/main/views/notifications.py +++ b/app/main/views/notifications.py @@ -59,7 +59,6 @@ def view_notification(service_id, notification_id): filetype='png', ), ), - status=request.args.get('status'), updates_url=url_for( ".view_notification_updates", service_id=service_id, @@ -80,49 +79,10 @@ def view_notification_updates(service_id, notification_id): )) -def _get_single_notification_counts(notification, help_argument): - return [ - ( - label, - query_param, - url_for( - ".view_notification", - service_id=notification['service'], - notification_id=notification['id'], - status=query_param, - help=help_argument - ), - count - ) for label, query_param, count in [ - [ - 'total', '', - 1 - ], - [ - 'sending', 'sending', - int(notification['status'] in SENDING_STATUSES) - ], - [ - 'delivered', 'delivered', - int(notification['status'] in DELIVERED_STATUSES) - ], - [ - 'failed', 'failed', - int(notification['status'] in FAILURE_STATUSES) - ] - ] - ] - - def get_single_notification_partials(notification): status_args = get_status_arg(request.args) return { - 'counts': render_template( - 'partials/count.html', - counts=_get_single_notification_counts(notification, request.args.get('help', 0)), - status=status_args - ), 'notifications': render_template( 'partials/notifications/notifications.html', notification=notification, diff --git a/app/templates/views/notifications/notification.html b/app/templates/views/notifications/notification.html index f4936fb53..4a60fd1bd 100644 --- a/app/templates/views/notifications/notification.html +++ b/app/templates/views/notifications/notification.html @@ -16,7 +16,6 @@ {{ template|string }} {{ ajax_block(partials, updates_url, 'status', finished=finished) }} - {{ ajax_block(partials, updates_url, 'counts', finished=finished) }} {{ ajax_block(partials, updates_url, 'notifications', finished=finished) }} {{ page_footer( diff --git a/tests/app/main/views/test_notifications.py b/tests/app/main/views/test_notifications.py index c03b4969a..e72396cba 100644 --- a/tests/app/main/views/test_notifications.py +++ b/tests/app/main/views/test_notifications.py @@ -13,23 +13,6 @@ from app.utils import ( from tests.conftest import mock_get_notification -@pytest.mark.parametrize('multidict_args, expected_statuses', [ - ([], REQUESTED_STATUSES), - ([('status', '')], REQUESTED_STATUSES), - ([('status', 'garbage')], REQUESTED_STATUSES), - ([('status', 'sending')], SENDING_STATUSES), - ([('status', 'delivered')], DELIVERED_STATUSES), - ([('status', 'failed')], FAILURE_STATUSES), -]) -def test_status_filters(mocker, multidict_args, expected_statuses): - mocker.patch('app.main.views.notifications.current_app') - - args = MultiDict(multidict_args) - args['status'] = get_status_arg(args) - - assert sorted(args['status']) == sorted(expected_statuses) - - @freeze_time("2016-01-01 11:09:00.061258") def test_notification_status_page_shows_details( client_request, @@ -50,29 +33,3 @@ def test_notification_status_page_shows_details( service_one['id'], fake_uuid ) - - -@pytest.mark.parametrize('notification_status, expected_big_number_vals', [ - ('created', [1, 1, 0, 0]), - ('sending', [1, 1, 0, 0]), - ('delivered', [1, 0, 1, 0]), - ('temporary-failure', [1, 0, 0, 1]), -]) -def test_notification_status_page_shows_correct_numbers( - client_request, - mocker, - service_one, - fake_uuid, - notification_status, - expected_big_number_vals -): - mock_get_notification(mocker, fake_uuid, notification_status=notification_status) - - page = client_request.get( - 'main.view_notification', - service_id=service_one['id'], - notification_id=fake_uuid - ) - - big_numbers = page.find_all('div', {'class': 'big-number-number'}) - assert expected_big_number_vals == [int(num.text.strip()) for num in big_numbers] From 16d92b9a62f9036195ef000a752352727a29271b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 19 Jun 2017 14:31:22 +0100 Subject: [PATCH 04/17] Remove notifications table from notification page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This table will only over show one notification. There’s a better way to display who the recipient was, and what the status of the notification is. --- app/templates/views/notifications/notification.html | 1 - tests/app/main/views/test_notifications.py | 5 ++++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/app/templates/views/notifications/notification.html b/app/templates/views/notifications/notification.html index 4a60fd1bd..c219f2321 100644 --- a/app/templates/views/notifications/notification.html +++ b/app/templates/views/notifications/notification.html @@ -16,7 +16,6 @@ {{ template|string }} {{ ajax_block(partials, updates_url, 'status', finished=finished) }} - {{ ajax_block(partials, updates_url, 'notifications', finished=finished) }} {{ page_footer( secondary_link=url_for('.view_template', service_id=current_service.id, template_id=template.id), diff --git a/tests/app/main/views/test_notifications.py b/tests/app/main/views/test_notifications.py index e72396cba..683ef63c4 100644 --- a/tests/app/main/views/test_notifications.py +++ b/tests/app/main/views/test_notifications.py @@ -10,6 +10,7 @@ from app.utils import ( DELIVERED_STATUSES, ) +from tests.app.test_utils import normalize_spaces from tests.conftest import mock_get_notification @@ -27,7 +28,9 @@ def test_notification_status_page_shows_details( ) assert page.find('div', {'class': 'sms-message-wrapper'}).text.strip() == 'service one: template content' - assert ' '.join(page.find('tbody').find('tr').text.split()) == '07123456789 Delivered 1 January at 11:10am' + assert normalize_spaces(page.select('.ajax-block-container p')[0].text) == ( + 'Sent by Test User on 1 January at 11:09am' + ) mock_get_notification.assert_called_with( service_one['id'], From a964417555261b4d6e6ed8eaa8b8d387024a5aaa Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 19 Jun 2017 14:32:50 +0100 Subject: [PATCH 05/17] Show recipient on notification page Since we removed the notifications table from this page we need a different way of showing who the message was sent to. Our `Template` classes already have a way of doing this, and we have a flag to switch this on (which is what this PR changes). --- app/main/views/notifications.py | 1 + 1 file changed, 1 insertion(+) diff --git a/app/main/views/notifications.py b/app/main/views/notifications.py index 29f5bda5f..f7af4bdf4 100644 --- a/app/main/views/notifications.py +++ b/app/main/views/notifications.py @@ -58,6 +58,7 @@ def view_notification(service_id, notification_id): version=notification['template_version'], filetype='png', ), + show_recipient=True, ), updates_url=url_for( ".view_notification_updates", From b578c3589b85d6ddd00c77ecd4b0ef4a3267ffa2 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 19 Jun 2017 14:37:16 +0100 Subject: [PATCH 06/17] Put status under message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since we’ve removed the table of notifications from the single notification page there’s no way of knowing the status of a notification. This re-adds it in a way that’s similar to how it looks on inbound messages. --- app/assets/stylesheets/main.scss | 1 + .../stylesheets/views/notification.scss | 18 +++++++++++++ .../partials/notifications/status.html | 13 +++++++--- tests/app/main/views/test_notifications.py | 26 +++++++++++++++---- 4 files changed, 50 insertions(+), 8 deletions(-) create mode 100644 app/assets/stylesheets/views/notification.scss diff --git a/app/assets/stylesheets/main.scss b/app/assets/stylesheets/main.scss index 29c376245..89b283ddd 100644 --- a/app/assets/stylesheets/main.scss +++ b/app/assets/stylesheets/main.scss @@ -66,6 +66,7 @@ $path: '/static/images/'; @import 'views/api'; @import 'views/product-page'; @import 'views/template'; +@import 'views/notification'; // TODO: break this up @import 'app'; diff --git a/app/assets/stylesheets/views/notification.scss b/app/assets/stylesheets/views/notification.scss new file mode 100644 index 000000000..08def6684 --- /dev/null +++ b/app/assets/stylesheets/views/notification.scss @@ -0,0 +1,18 @@ +.notification-status { + + @include core-16; + color: $secondary-text-colour; + margin-top: -$gutter-half; + + &.error { + + color: $error-colour; + font-weight: bold; + + a { + color: $error-colour; + } + + } + +} diff --git a/app/templates/partials/notifications/status.html b/app/templates/partials/notifications/status.html index 481b32446..730eb6954 100644 --- a/app/templates/partials/notifications/status.html +++ b/app/templates/partials/notifications/status.html @@ -1,6 +1,13 @@

-

- Sent {% if notification.created_by %}by {{ notification.created_by.name }} {% endif %} - on {{ notification.created_at|format_datetime_short }} +

+ {% if notification.status|format_notification_status_as_url %} + + {% endif %} + {{ notification.status|format_notification_status( + notification.template.template_type + ) }} + {% if notification.status|format_notification_status_as_url %} + + {% endif %}

diff --git a/tests/app/main/views/test_notifications.py b/tests/app/main/views/test_notifications.py index 683ef63c4..d5aceaecd 100644 --- a/tests/app/main/views/test_notifications.py +++ b/tests/app/main/views/test_notifications.py @@ -1,8 +1,6 @@ from freezegun import freeze_time import pytest -from werkzeug.datastructures import MultiDict -from app.main.views.notifications import get_status_arg from app.utils import ( REQUESTED_STATUSES, FAILURE_STATUSES, @@ -14,13 +12,31 @@ from tests.app.test_utils import normalize_spaces from tests.conftest import mock_get_notification +@pytest.mark.parametrize('notification_status, expected_status', [ + ('created', 'Sending'), + ('sending', 'Sending'), + ('delivered', 'Delivered'), + ('failed', 'Failed'), + ('temporary-failure', 'Phone not accepting messages right now'), + ('permanent-failure', 'Phone number doesn’t exist'), + ('technical-failure', 'Technical failure'), +]) @freeze_time("2016-01-01 11:09:00.061258") def test_notification_status_page_shows_details( client_request, - mock_get_notification, + mocker, service_one, fake_uuid, + notification_status, + expected_status, ): + + _mock_get_notification = mock_get_notification( + mocker, + fake_uuid, + notification_status=notification_status + ) + page = client_request.get( 'main.view_notification', service_id=service_one['id'], @@ -29,10 +45,10 @@ def test_notification_status_page_shows_details( assert page.find('div', {'class': 'sms-message-wrapper'}).text.strip() == 'service one: template content' assert normalize_spaces(page.select('.ajax-block-container p')[0].text) == ( - 'Sent by Test User on 1 January at 11:09am' + expected_status ) - mock_get_notification.assert_called_with( + _mock_get_notification.assert_called_with( service_one['id'], fake_uuid ) From 5e4aff2a7b6e29305b8a6ad3d6ec472896e11d54 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 19 Jun 2017 15:00:12 +0100 Subject: [PATCH 07/17] Refactor test to match subsequent line MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Easier to read these asserts if they’re formatted the same. --- tests/app/main/views/test_notifications.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/app/main/views/test_notifications.py b/tests/app/main/views/test_notifications.py index d5aceaecd..7decfcd80 100644 --- a/tests/app/main/views/test_notifications.py +++ b/tests/app/main/views/test_notifications.py @@ -43,7 +43,9 @@ def test_notification_status_page_shows_details( notification_id=fake_uuid ) - assert page.find('div', {'class': 'sms-message-wrapper'}).text.strip() == 'service one: template content' + assert normalize_spaces(page.select('.sms-message-wrapper')[0].text) == ( + 'service one: template content' + ) assert normalize_spaces(page.select('.ajax-block-container p')[0].text) == ( expected_status ) From 67b2937123df6568767af138ee39f95d571d666d Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 19 Jun 2017 14:32:21 +0100 Subject: [PATCH 08/17] Show full message content on notification page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We’ve had a few teams talk about wanting to go back and check what their users are sending out, including the content of any placeholders. We already provide this functionality through the API, this commit makes it the default in the admin app too. We couldn’t do this before because we didn’t have the individual notification page. It’s better to do this by re-hydrating the template than pulling the content from the API, because things like letters have multiple areas of content – this is more complex than what we can get from API at the moment. --- app/main/views/notifications.py | 26 ++++++++++++---------- tests/app/main/views/test_notifications.py | 2 +- tests/conftest.py | 3 ++- 3 files changed, 17 insertions(+), 14 deletions(-) diff --git a/app/main/views/notifications.py b/app/main/views/notifications.py index f7af4bdf4..f7351249f 100644 --- a/app/main/views/notifications.py +++ b/app/main/views/notifications.py @@ -44,22 +44,24 @@ def get_status_arg(filter_args): @user_has_permissions('view_activity', admin_override=True) def view_notification(service_id, notification_id): notification = notification_api_client.get_notification(service_id, notification_id) + template = get_template( + notification['template'], + current_service, + letter_preview_url=url_for( + '.view_template_version_preview', + service_id=service_id, + template_id=notification['template']['id'], + version=notification['template_version'], + filetype='png', + ), + show_recipient=True, + ) + template.values = notification['personalisation'] return render_template( 'views/notifications/notification.html', finished=(notification['status'] in (DELIVERED_STATUSES + FAILURE_STATUSES)), uploaded_file_name='Report', - template=get_template( - notification['template'], - current_service, - letter_preview_url=url_for( - '.view_template_version_preview', - service_id=service_id, - template_id=notification['template']['id'], - version=notification['template_version'], - filetype='png', - ), - show_recipient=True, - ), + template=template, updates_url=url_for( ".view_notification_updates", service_id=service_id, diff --git a/tests/app/main/views/test_notifications.py b/tests/app/main/views/test_notifications.py index 7decfcd80..10cfa2485 100644 --- a/tests/app/main/views/test_notifications.py +++ b/tests/app/main/views/test_notifications.py @@ -44,7 +44,7 @@ def test_notification_status_page_shows_details( ) assert normalize_spaces(page.select('.sms-message-wrapper')[0].text) == ( - 'service one: template content' + 'service one: hello Jo' ) assert normalize_spaces(page.select('.ajax-block-container p')[0].text) == ( expected_status diff --git a/tests/conftest.py b/tests/conftest.py index 7f556c22d..d4c1f6754 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1635,7 +1635,8 @@ def mock_get_notification(mocker, fake_uuid, notification_status='delivered'): 'name': 'Test User', 'email_address': 'test@user.gov.uk' } - noti['template'] = template_json(service_id, str(generate_uuid())) + noti['personalisation'] = {'name': 'Jo'} + noti['template'] = template_json(service_id, str(generate_uuid()), content='hello ((name))') return noti return mocker.patch( From 974953a64f4a063b1c833bd0d0be1c9d9fa13a6a Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 19 Jun 2017 16:57:38 +0100 Subject: [PATCH 09/17] Remove file name from single notification page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Having a file name for single messages made sense when they were called ‘Test message’. They made a bit of sense when single messages still had a file, which had to be called _something_. Now that individual messages don’t come from a file, and don’t land you on a page that looks like a ‘report’. --- app/templates/views/notifications/notification.html | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/templates/views/notifications/notification.html b/app/templates/views/notifications/notification.html index c219f2321..5d17f2f2a 100644 --- a/app/templates/views/notifications/notification.html +++ b/app/templates/views/notifications/notification.html @@ -1,6 +1,7 @@ {% extends "withnav_template.html" %} {% from "components/banner.html" import banner %} {% from "components/ajax-block.html" import ajax_block %} +{% from "components/message-count-label.html" import message_count_label %} {% from "components/page-footer.html" import page_footer %} {% block service_page_title %} @@ -10,7 +11,7 @@ {% block maincolumn_content %}

- Report + {{ message_count_label(1, template.template_type, suffix='') | capitalize }}

{{ template|string }} From 929e2b841fac93a43b5cf0cfc33aebfb9f8f140e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 19 Jun 2017 14:36:25 +0100 Subject: [PATCH 10/17] Put notification meta above message, remove AJAX MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The meta info about a notification (who sent it, when they sent it) won’t ever change, so there’s no need for it to reload it using AJAX. Putting it above the message and under the `

` makes it match how this information is displayed on the job page. On the job page this information is bold, but visually the job page is using too much bold now (nothing is emphasised/differentiated if everything is bold). So this commit also makes this line of info regular on both the notification and job pages. --- app/main/views/notifications.py | 2 ++ app/templates/partials/jobs/status.html | 2 +- app/templates/views/notifications/notification.html | 5 +++++ 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/app/main/views/notifications.py b/app/main/views/notifications.py index f7351249f..c501eb0b1 100644 --- a/app/main/views/notifications.py +++ b/app/main/views/notifications.py @@ -70,6 +70,8 @@ def view_notification(service_id, notification_id): help=get_help_argument() ), partials=get_single_notification_partials(notification), + created_by=notification.get('created_by'), + created_at=notification['created_at'], help=get_help_argument() ) diff --git a/app/templates/partials/jobs/status.html b/app/templates/partials/jobs/status.html index c66bc6427..c243ce572 100644 --- a/app/templates/partials/jobs/status.html +++ b/app/templates/partials/jobs/status.html @@ -1,5 +1,5 @@
-

+

{% if job.scheduled_for %} {% if job.processing_started %} Sent by {{ job.created_by.name }} on {{ job.processing_started|format_datetime_short }} diff --git a/app/templates/views/notifications/notification.html b/app/templates/views/notifications/notification.html index 5d17f2f2a..e077877fa 100644 --- a/app/templates/views/notifications/notification.html +++ b/app/templates/views/notifications/notification.html @@ -14,6 +14,11 @@ {{ message_count_label(1, template.template_type, suffix='') | capitalize }}

+

+ Sent {% if created_by %}by {{ created_by.name }} {% endif %} + on {{ created_at|format_datetime_short }} +

+ {{ template|string }} {{ ajax_block(partials, updates_url, 'status', finished=finished) }} From 1eb3a5aca0769c4079407b4f96f9e6591e84187e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 19 Jun 2017 14:43:01 +0100 Subject: [PATCH 11/17] Link to the relevant job from notification page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If a notification has been sent from a job then that’s important context to know about it. So we should surface that information on the page. It also gives users an easy way of going back, if that’s the page they’ve come from. --- app/main/views/notifications.py | 6 ++++++ .../views/notifications/notification.html | 14 ++++++++------ 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/app/main/views/notifications.py b/app/main/views/notifications.py index c501eb0b1..20d309a21 100644 --- a/app/main/views/notifications.py +++ b/app/main/views/notifications.py @@ -10,6 +10,7 @@ from flask_login import login_required from app import ( notification_api_client, + job_api_client, current_service ) from app.main import main @@ -57,11 +58,16 @@ def view_notification(service_id, notification_id): show_recipient=True, ) template.values = notification['personalisation'] + if notification['job']: + job = job_api_client.get_job(service_id, notification['job']['id'])['data'] + else: + job = None return render_template( 'views/notifications/notification.html', finished=(notification['status'] in (DELIVERED_STATUSES + FAILURE_STATUSES)), uploaded_file_name='Report', template=template, + job=job, updates_url=url_for( ".view_notification_updates", service_id=service_id, diff --git a/app/templates/views/notifications/notification.html b/app/templates/views/notifications/notification.html index e077877fa..274776b22 100644 --- a/app/templates/views/notifications/notification.html +++ b/app/templates/views/notifications/notification.html @@ -15,7 +15,14 @@

- Sent {% if created_by %}by {{ created_by.name }} {% endif %} + {{ template.name }} + sent + {% if job %} + from + {{ job.original_file_name }} + {% elif created_by %} + by {{ created_by.name }} + {% endif %} on {{ created_at|format_datetime_short }}

@@ -23,9 +30,4 @@ {{ ajax_block(partials, updates_url, 'status', finished=finished) }} - {{ page_footer( - secondary_link=url_for('.view_template', service_id=current_service.id, template_id=template.id), - secondary_link_text='Back to {}'.format(template.name) - ) }} - {% endblock %} From f5b49b16e9adf95ba4c3318e3fa8a89f7d334262 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 19 Jun 2017 17:01:05 +0100 Subject: [PATCH 12/17] =?UTF-8?q?Don=E2=80=99t=20link=20to=20=E2=80=98Repo?= =?UTF-8?q?rt=E2=80=99=20jobs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ‘Report’ jobs are what we used to have for one-off messages. The page for a report job doesn’t contain any extra info from what’s on the notification page. We will still have ‘Report’ jobs while we transition to sending one-off messages through the API. So while we still have these jobs, let’s hide any links to them because they’re not useful pages. --- app/templates/views/notifications/notification.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/templates/views/notifications/notification.html b/app/templates/views/notifications/notification.html index 274776b22..aba8beee4 100644 --- a/app/templates/views/notifications/notification.html +++ b/app/templates/views/notifications/notification.html @@ -17,7 +17,7 @@

{{ template.name }} sent - {% if job %} + {% if job and job.original_file_name != 'Report' %} from {{ job.original_file_name }} {% elif created_by %} From c554e9e32dac20c93f3fbad9477809dab994b2a7 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 19 Jun 2017 16:37:28 +0100 Subject: [PATCH 13/17] Put template content or subject in tables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In tables where we show rows and rows of information we used to give some meta information about the notification, or at least as much as we could give in the very limited space available. This information is now on the notifications page, so the information we show in these tables should just be whatever helps users identify the right message. I reckon that this is: - the content of the message for text messages - the subject for emails and letters This also makes these pages consistent with: - the inbound SMS page - the way the people’s inboxes work for their text messsages/Whatsapps/emails For consistency’s sake this makes the job page work the same way. It may be slightly less useful here because on the job page every message is sent from the same template, so will have broadly the same content. --- app/main/views/jobs.py | 22 +++++++++++++++++-- .../partials/jobs/notifications.html | 3 ++- .../views/activity/notifications.html | 12 ++-------- tests/__init__.py | 8 +++++-- tests/app/main/views/test_activity.py | 12 ++++++---- tests/app/main/views/test_jobs.py | 3 ++- 6 files changed, 40 insertions(+), 20 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 266811651..dba4adeb4 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -14,6 +14,10 @@ from flask import ( stream_with_context ) from flask_login import login_required +from notifications_utils.template import ( + Template, + WithSubjectTemplate, +) from werkzeug.datastructures import MultiDict from app import ( @@ -271,7 +275,7 @@ def get_notifications(service_id, message_type, status_override=None): ), 'notifications': render_template( 'views/activity/notifications.html', - notifications=notifications['notifications'], + notifications=add_preview_of_content_to_notifications(notifications['notifications']), page=page, prev_page=prev_page, next_page=next_page, @@ -368,7 +372,7 @@ def get_job_partials(job): ), 'notifications': render_template( 'partials/jobs/notifications.html', - notifications=notifications['notifications'], + notifications=add_preview_of_content_to_notifications(notifications['notifications']), more_than_one_page=bool(notifications.get('links', {}).get('next')), percentage_complete=(job['notifications_requested'] / job['notification_count'] * 100), download_link=url_for( @@ -386,3 +390,17 @@ def get_job_partials(job): job=job ), } + + +def add_preview_of_content_to_notifications(notifications): + return ( + dict( + preview_of_content=( + str(Template(notification['template'], notification['personalisation'])) + if notification['template']['template_type'] == 'sms' else + WithSubjectTemplate(notification['template'], notification['personalisation']).subject + ), + **notification + ) + for notification in notifications + ) diff --git a/app/templates/partials/jobs/notifications.html b/app/templates/partials/jobs/notifications.html index d1c319d59..a6f781e0d 100644 --- a/app/templates/partials/jobs/notifications.html +++ b/app/templates/partials/jobs/notifications.html @@ -48,8 +48,9 @@ field_headings_visible=False ) %} {% call row_heading() %} + {{ item.to }}

- {{ item.to }} + {{ item.preview_of_content }}

{% endcall %} {{ notification_status_field(item) }} diff --git a/app/templates/views/activity/notifications.html b/app/templates/views/activity/notifications.html index d6f96a37b..2b6234d50 100644 --- a/app/templates/views/activity/notifications.html +++ b/app/templates/views/activity/notifications.html @@ -20,16 +20,8 @@

{{ item.to }}

-

- {% 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 an API call - {% endif %} +

+ {{ item.preview_of_content }}

{% endcall %} diff --git a/tests/__init__.py b/tests/__init__.py index 70fbaf095..8e2ebbe9e 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -103,6 +103,8 @@ def template_json(service_id, 'archived': archived, 'process_type': process_type } + if subject is None and type_ != 'sms': + template['subject'] = "template subject" if subject is not None: template['subject'] = subject return template @@ -216,7 +218,8 @@ def notification_json( created_at=None, updated_at=None, with_links=False, - rows=5 + rows=5, + personalisation=None, ): if template is None: template = template_json(service_id, str(generate_uuid())) @@ -255,7 +258,8 @@ def notification_json( 'updated_at': updated_at, 'job_row_number': job_row_number, 'service': service_id, - 'template_version': template['version'] + 'template_version': template['version'], + 'personalisation': personalisation or {}, } for i in range(rows)], 'total': rows, 'page_size': 50, diff --git a/tests/app/main/views/test_activity.py b/tests/app/main/views/test_activity.py index b0f230ced..e53afd442 100644 --- a/tests/app/main/views/test_activity.py +++ b/tests/app/main/views/test_activity.py @@ -92,13 +92,17 @@ def test_can_show_notifications( page=page_argument, )) assert response.status_code == 200 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') content = response.get_data(as_text=True) notifications = notification_json(service_one['id']) notification = notifications['notifications'][0] - assert notification['to'] in content - assert notification['status'] in content - assert notification['template']['name'] in content - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + text_of_first_row = page.select('tbody tr')[0].text + assert '07123456789' in text_of_first_row + assert ( + 'template content' in text_of_first_row or + 'template subject' in text_of_first_row + ) + assert 'Delivered' in text_of_first_row assert page_title in page.h1.text.strip() path_to_json = page.find("div", {'data-key': 'notifications'})['data-resource'] diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 1034fca7d..decd53bdb 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -80,6 +80,7 @@ def test_should_show_page_for_one_job( status_argument, expected_api_call, ): + response = logged_in_client.get(url_for( 'main.view_job', service_id=service_one['id'], @@ -94,7 +95,7 @@ def test_should_show_page_for_one_job( '{}: Template content with & entity'.format(service_one['name']) ) assert ' '.join(page.find('tbody').find('tr').text.split()) == ( - '07123456789 Delivered 1 January at 11:10am' + '07123456789 template content Delivered 1 January at 11:10am' ) assert page.find('div', {'data-key': 'notifications'})['data-resource'] == url_for( 'main.view_job_updates', From ba93be5ea5d8f699415fc380f66d77e86c84599f Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 19 Jun 2017 14:44:47 +0100 Subject: [PATCH 14/17] Remove template from job page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Showing the template on the job page was semi-useful when you couldn’t see the contents of each individual message. It was still a bit weird because it just showed the template, never the actual messages that went out, with the placeholders. Now that users can click through to see individual messages, and can see a short preview of the content on each row I think we can safely lose the template preview on this page. --- app/main/views/jobs.py | 16 +--------------- app/templates/main_nav.html | 2 +- app/templates/views/jobs/job.html | 9 --------- tests/app/main/views/test_jobs.py | 3 --- 4 files changed, 2 insertions(+), 28 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index dba4adeb4..0b9f06106 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -114,21 +114,7 @@ def view_job(service_id, job_id): 'views/jobs/job.html', finished=(total_notifications == processed_notifications), uploaded_file_name=job['original_file_name'], - template=get_template( - service_api_client.get_service_template( - service_id=service_id, - template_id=job['template'], - version=job['template_version'] - )['data'], - current_service, - letter_preview_url=url_for( - '.view_template_version_preview', - service_id=service_id, - template_id=job['template'], - version=job['template_version'], - filetype='png', - ), - ), + template_id=job['template'], status=request.args.get('status', ''), updates_url=url_for( ".view_job_updates", diff --git a/app/templates/main_nav.html b/app/templates/main_nav.html index 2dc588d0e..3c0ca9ef2 100644 --- a/app/templates/main_nav.html +++ b/app/templates/main_nav.html @@ -32,7 +32,7 @@ Notify delivers the message

{% if help == '3' %} - + Now go to your dashboard {% endif %} diff --git a/app/templates/views/jobs/job.html b/app/templates/views/jobs/job.html index 14aa297bf..45d0b8892 100644 --- a/app/templates/views/jobs/job.html +++ b/app/templates/views/jobs/job.html @@ -13,17 +13,8 @@ {{ uploaded_file_name }} - {{ template|string }} - {{ ajax_block(partials, updates_url, 'status', finished=finished) }} {{ ajax_block(partials, updates_url, 'counts', finished=finished) }} {{ ajax_block(partials, updates_url, 'notifications', finished=finished) }} - {% if not help %} - {{ page_footer( - secondary_link=url_for('.view_template', service_id=current_service.id, template_id=template.id), - secondary_link_text='Back to {}'.format(template.name) - ) }} - {% endif %} - {% endblock %} diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index decd53bdb..3cf2a086d 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -91,9 +91,6 @@ def test_should_show_page_for_one_job( assert response.status_code == 200 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') assert page.h1.text.strip() == 'thisisatest.csv' - assert page.find('div', {'class': 'sms-message-wrapper'}).text.strip() == ( - '{}: Template content with & entity'.format(service_one['name']) - ) assert ' '.join(page.find('tbody').find('tr').text.split()) == ( '07123456789 template content Delivered 1 January at 11:10am' ) From ff4a580ab44ee49a41ea710464fddb29b5409f7e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 21 Jun 2017 14:52:47 +0100 Subject: [PATCH 15/17] Indicate template on scheduled jobs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We’ve removed the template on the jobs page, so you can no longer see which template a job is about to be sent with. This is removing information which might enable you to undo a costly mistake. I don’t think we need to bring back the whole template – giving its name, and a link to it meets the need just as well. --- app/main/views/jobs.py | 9 ++++++++- app/templates/partials/jobs/notifications.html | 4 +++- tests/__init__.py | 2 +- tests/app/main/views/test_jobs.py | 15 ++++++++++++--- 4 files changed, 24 insertions(+), 6 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 0b9f06106..17d9996ed 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -350,6 +350,11 @@ def get_job_partials(job): notifications = notification_api_client.get_notifications_for_service( job['service'], job['id'], status=filter_args['status'] ) + template = service_api_client.get_service_template( + service_id=current_service['id'], + template_id=job['template'], + version=job['template_version'] + )['data'] return { 'counts': render_template( 'partials/count.html', @@ -369,7 +374,9 @@ def get_job_partials(job): ), help=get_help_argument(), time_left=get_time_left(job['created_at']), - job=job + job=job, + template=template, + template_version=job['template_version'], ), 'status': render_template( 'partials/jobs/status.html', diff --git a/app/templates/partials/jobs/notifications.html b/app/templates/partials/jobs/notifications.html index a6f781e0d..c033e9427 100644 --- a/app/templates/partials/jobs/notifications.html +++ b/app/templates/partials/jobs/notifications.html @@ -5,7 +5,9 @@ {% if job.job_status == 'scheduled' %}

- Sending will start {{ job.scheduled_for|format_datetime_relative }} + Sending + {{ template.name }} + {{ job.scheduled_for|format_datetime_relative }}

diff --git a/tests/__init__.py b/tests/__init__.py index 8e2ebbe9e..ecf4f86ea 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -182,7 +182,7 @@ def job_json( if job_id is None: job_id = str(generate_uuid()) if template_id is None: - template_id = str(generate_uuid()) + template_id = "5d729fbd-239c-44ab-b498-75a985f3198f" if created_at is None: created_at = str(datetime.now(timezone.utc).strftime('%Y-%m-%dT%H:%M:%S.%f%z')) data = { diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 3cf2a086d..242ad6ca9 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.app.test_utils import normalize_spaces from tests.conftest import SERVICE_ONE_ID from freezegun import freeze_time @@ -161,7 +162,6 @@ def test_should_show_job_in_progress( @freeze_time("2016-01-01T00:00:00.061258") def test_should_show_scheduled_job( logged_in_client, - service_one, active_user_with_permissions, mock_get_service_template, mock_get_scheduled_job, @@ -171,13 +171,21 @@ def test_should_show_scheduled_job( ): response = logged_in_client.get(url_for( 'main.view_job', - service_id=service_one['id'], + service_id=SERVICE_ONE_ID, job_id=fake_uuid )) assert response.status_code == 200 page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') - assert page.find('main').find_all('p')[1].text.strip() == 'Sending will start today at midnight' + assert normalize_spaces(page.select('main p')[1].text) == ( + 'Sending Two week reminder today at midnight' + ) + assert page.select('main p a')[0]['href'] == url_for( + 'main.view_template_version', + service_id=SERVICE_ONE_ID, + template_id='5d729fbd-239c-44ab-b498-75a985f3198f', + version=1, + ) assert page.find('input', {'type': 'submit', 'value': 'Cancel sending'}) @@ -254,6 +262,7 @@ def test_should_show_updates_for_one_job_as_json( service_one, active_user_with_permissions, mock_get_notifications, + mock_get_service_template, mock_get_job, mocker, fake_uuid, From b8cc0de44ed0d0d51195b953405f19300ca90358 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 19 Jun 2017 17:05:14 +0100 Subject: [PATCH 16/17] Use file list pattern for notifications Most of our tables are now lists which have: - a thing you click on - some secondary info about that thing underneath - some extra numbers/status on the right hand side Since these are so similar, they should share the same pattern, which this commit does (using the `file-list` pattern). Visually this means: - the secondary text on the inbox becomes grey not black, for consistency - the status bit of the notifications tables bottom align like the status of messages in the inbox These both seem like OK changes to make for the sake of consistency. --- app/assets/stylesheets/views/dashboard.scss | 1 + app/templates/components/table.html | 2 ++ app/templates/partials/jobs/notifications.html | 4 ++-- app/templates/views/activity/notifications.html | 6 ++---- app/templates/views/dashboard/inbox.html | 4 ++-- 5 files changed, 9 insertions(+), 8 deletions(-) diff --git a/app/assets/stylesheets/views/dashboard.scss b/app/assets/stylesheets/views/dashboard.scss index a1fed973a..ddd35428a 100644 --- a/app/assets/stylesheets/views/dashboard.scss +++ b/app/assets/stylesheets/views/dashboard.scss @@ -96,5 +96,6 @@ } .align-with-message-body { + display: block; margin-top: $gutter * 5 / 6; } diff --git a/app/templates/components/table.html b/app/templates/components/table.html index 8708465b2..5ad942fe6 100644 --- a/app/templates/components/table.html +++ b/app/templates/components/table.html @@ -120,6 +120,7 @@ {% macro notification_status_field(notification) %} {% call field(status=notification.status|format_notification_status_as_field_status, align='right') %} + {% if notification.status in ['created', 'sending', 'delivered'] %}{% endif %} {% if notification.status|format_notification_status_as_url %} {% endif %} @@ -135,6 +136,7 @@ (notification.updated_at or notification.created_at)|format_datetime_short ) }} + {% if notification.status in ['created', 'sending', 'delivered'] %}{% endif %} {% endcall %} {% endmacro %} diff --git a/app/templates/partials/jobs/notifications.html b/app/templates/partials/jobs/notifications.html index c033e9427..4bec657b3 100644 --- a/app/templates/partials/jobs/notifications.html +++ b/app/templates/partials/jobs/notifications.html @@ -50,8 +50,8 @@ field_headings_visible=False ) %} {% call row_heading() %} - {{ item.to }} -

+ {{ item.to }} +

{{ item.preview_of_content }}

{% endcall %} diff --git a/app/templates/views/activity/notifications.html b/app/templates/views/activity/notifications.html index 2b6234d50..cabcd436b 100644 --- a/app/templates/views/activity/notifications.html +++ b/app/templates/views/activity/notifications.html @@ -17,10 +17,8 @@ ) %} {% call row_heading() %} -

- {{ item.to }} -

-

+ {{ item.to }} +

{{ item.preview_of_content }}

{% endcall %} diff --git a/app/templates/views/dashboard/inbox.html b/app/templates/views/dashboard/inbox.html index fe343b1bc..6dbc2fb5b 100644 --- a/app/templates/views/dashboard/inbox.html +++ b/app/templates/views/dashboard/inbox.html @@ -31,10 +31,10 @@ > {{ item.user_number | format_phone_number_human_readable }} - {{ item.content }} + {{ item.content }} {% endcall %} {% call field(align='right') %} - + {{ item.created_at | format_delta }} {% endcall %} From 010886e5bebc6f904dc9c5de8bf5124740778d0e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 20 Jun 2017 09:50:55 +0100 Subject: [PATCH 17/17] Make clickable areas bigger on tables with links MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Generally, bigger click areas are better[1], as long as they don’t cause ambiguity or accidental clicks. This commit expands the clickable area of tables where the left-hand column is a link to include the meta information under the link. We can’t make the whole row clickable, because sometimes we have links in the right hand column which go to a different place This commit also removes the CSS for `.spark-bar-label`, because these elements are visually identical to `file-list`s now. 1. https://en.wikipedia.org/wiki/Fitts%27s_law --- app/assets/stylesheets/views/dashboard.scss | 16 +++++----------- .../views/dashboard/all-template-statistics.html | 8 +++----- .../views/dashboard/template-statistics.html | 8 +++----- 3 files changed, 11 insertions(+), 21 deletions(-) diff --git a/app/assets/stylesheets/views/dashboard.scss b/app/assets/stylesheets/views/dashboard.scss index ddd35428a..c3bc7b9b7 100644 --- a/app/assets/stylesheets/views/dashboard.scss +++ b/app/assets/stylesheets/views/dashboard.scss @@ -46,17 +46,6 @@ transition: width 0.6s ease-in-out; } - &-label { - @include bold-19; - display: block; - white-space: nowrap; - overflow: hidden; - text-overflow: ellipsis; - color: $govuk-blue; - max-width: 100%; - background: $white; - } - } .file-list { @@ -67,12 +56,17 @@ white-space: nowrap; overflow: hidden; text-overflow: ellipsis; + padding-bottom: 30px; + padding-top: 10px; + margin-bottom: -30px; + margin-top: -10px; } &-hint { @include core-16; display: block; color: $secondary-text-colour; + pointer-events: none; } } diff --git a/app/templates/views/dashboard/all-template-statistics.html b/app/templates/views/dashboard/all-template-statistics.html index a0c803fce..202e3bc9c 100644 --- a/app/templates/views/dashboard/all-template-statistics.html +++ b/app/templates/views/dashboard/all-template-statistics.html @@ -41,11 +41,9 @@ field_headings_visible=False ) %} {% call row_heading() %} - - {{ item.name }} - - {{ message_count_label(1, item.type, suffix='template')|capitalize }} - + {{ item.name }} + + {{ message_count_label(1, item.type, suffix='template')|capitalize }} {% endcall %} {{ spark_bar_field(item.requested_count, most_used_template_count) }} diff --git a/app/templates/views/dashboard/template-statistics.html b/app/templates/views/dashboard/template-statistics.html index 2c3833655..94564d0e3 100644 --- a/app/templates/views/dashboard/template-statistics.html +++ b/app/templates/views/dashboard/template-statistics.html @@ -17,11 +17,9 @@ ) %} {% call row_heading() %} - - {{ item.template_name }} - - {{ message_count_label(1, item.template_type, suffix='template')|capitalize }} - + {{ item.template_name }} + + {{ message_count_label(1, item.template_type, suffix='template')|capitalize }} {% endcall %} {% if template_statistics|length > 1 %}