From 1ee972bd0411942f284fcd417285c3a4b083b8b4 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Thu, 15 Nov 2018 16:40:55 +0000 Subject: [PATCH 1/3] Add endpoint for cancelling letters to NotificationApiClient --- app/notify_client/notification_api_client.py | 5 +++++ tests/app/notify_client/test_notification_client.py | 9 +++++++++ 2 files changed, 14 insertions(+) diff --git a/app/notify_client/notification_api_client.py b/app/notify_client/notification_api_client.py index 10f0d3e6f..dae4f5825 100644 --- a/app/notify_client/notification_api_client.py +++ b/app/notify_client/notification_api_client.py @@ -93,5 +93,10 @@ class NotificationApiClient(NotifyAdminAPIClient): return self.get(url=get_url) + def update_notification_to_cancelled(self, service_id, notification_id): + return self.post( + url='/service/{}/notifications/{}/cancel'.format(service_id, notification_id), + data={}) + notification_api_client = NotificationApiClient() diff --git a/tests/app/notify_client/test_notification_client.py b/tests/app/notify_client/test_notification_client.py index aeb051994..6486741e3 100644 --- a/tests/app/notify_client/test_notification_client.py +++ b/tests/app/notify_client/test_notification_client.py @@ -85,3 +85,12 @@ def test_get_api_notifications_changes_letter_statuses(mocker): assert ret['notifications'][0]['status'] == 'created' assert ret['notifications'][1]['status'] == 'created' assert ret['notifications'][2]['status'] == 'accepted' + + +def test_update_notification_to_cancelled(mocker): + mock_post = mocker.patch('app.notify_client.notification_api_client.NotificationApiClient.post') + NotificationApiClient().update_notification_to_cancelled('foo', 'bar') + mock_post.assert_called_once_with( + url='/service/foo/notifications/bar/cancel', + data={}, + ) From 429a23934d2846ce2453d2f4c8976647acef32a9 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Tue, 4 Dec 2018 15:07:20 +0000 Subject: [PATCH 2/3] Display cancelled letters show as failed In the long term, we don't want to show cancelled letters. But for now, this changes cancelled letters to display in the same way that letters with a status of permanent-failure, since we are currently giving letters that we want to cancel the status of permanent failure. --- app/__init__.py | 2 ++ app/notify_client/job_api_client.py | 3 ++- app/templates/components/table.html | 2 +- app/templates/views/notifications/notification.html | 2 +- app/utils.py | 3 ++- tests/app/main/views/test_activity.py | 5 +++-- tests/app/main/views/test_jobs.py | 6 ++++-- tests/app/main/views/test_notifications.py | 4 ++++ 8 files changed, 19 insertions(+), 8 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 92397a688..0221981a8 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -373,6 +373,7 @@ def format_notification_status(status, template_type): 'pending-virus-check': 'Pending virus check', 'virus-scan-failed': 'Virus detected', 'returned-letter': 'Delivered', + 'cancelled': 'Cancelled,' } }[template_type].get(status, status) @@ -398,6 +399,7 @@ def format_notification_status_as_field_status(status, notification_type): 'pending-virus-check': None, 'virus-scan-failed': 'error', 'returned-letter': None, + 'cancelled': 'error', } }.get( notification_type, diff --git a/app/notify_client/job_api_client.py b/app/notify_client/job_api_client.py index 34dfdbe88..c40feebce 100644 --- a/app/notify_client/job_api_client.py +++ b/app/notify_client/job_api_client.py @@ -25,7 +25,8 @@ class JobApiClient(NotifyAdminAPIClient): def __convert_statistics(job): results = defaultdict(int) for outcome in job['statistics']: - if outcome['status'] in ['failed', 'technical-failure', 'temporary-failure', 'permanent-failure']: + if outcome['status'] in ['failed', 'technical-failure', 'temporary-failure', + 'permanent-failure', 'cancelled']: results['failed'] += outcome['count'] if outcome['status'] in ['sending', 'pending', 'created']: results['sending'] += outcome['count'] diff --git a/app/templates/components/table.html b/app/templates/components/table.html index bb0c76196..cb5622c60 100644 --- a/app/templates/components/table.html +++ b/app/templates/components/table.html @@ -162,7 +162,7 @@ notification.template.template_type ) }} {% endif %} - {% if notification.notification_type == "letter" and notification.status in ['permanent-failure', 'validation-failed'] %} + {% if notification.notification_type == "letter" and notification.status in ['permanent-failure', 'validation-failed', 'cancelled'] %} Cancelled {% endif %} {% if notification.status|format_notification_status_as_url(notification.notification_type) %} diff --git a/app/templates/views/notifications/notification.html b/app/templates/views/notifications/notification.html index 78a618908..29bf446d0 100644 --- a/app/templates/views/notifications/notification.html +++ b/app/templates/views/notifications/notification.html @@ -37,7 +37,7 @@

{% if template.template_type == 'letter' %} - {% if notification_status == 'permanent-failure' %} + {% if notification_status in ('permanent-failure', 'cancelled') %}

Cancelled {{ updated_at|format_datetime_short }}

diff --git a/app/utils.py b/app/utils.py index d15ed9cc6..0d771275c 100644 --- a/app/utils.py +++ b/app/utils.py @@ -39,7 +39,8 @@ from werkzeug.datastructures import MultiDict SENDING_STATUSES = ['created', 'pending', 'sending', 'pending-virus-check'] DELIVERED_STATUSES = ['delivered', 'sent', 'returned-letter'] -FAILURE_STATUSES = ['failed', 'temporary-failure', 'permanent-failure', 'technical-failure', 'virus-scan-failed'] +FAILURE_STATUSES = ['failed', 'temporary-failure', 'permanent-failure', + 'technical-failure', 'virus-scan-failed', 'cancelled'] REQUESTED_STATUSES = SENDING_STATUSES + DELIVERED_STATUSES + FAILURE_STATUSES diff --git a/tests/app/main/views/test_activity.py b/tests/app/main/views/test_activity.py index 1a028867b..b6c2f1a52 100644 --- a/tests/app/main/views/test_activity.py +++ b/tests/app/main/views/test_activity.py @@ -52,7 +52,8 @@ from tests.conftest import ( [ 'created', 'pending', 'sending', 'pending-virus-check', 'delivered', 'sent', 'returned-letter', - 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure', 'virus-scan-failed', + 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure', + 'virus-scan-failed', 'cancelled', ] ), ( @@ -65,7 +66,7 @@ from tests.conftest import ( ), ( 'failed', - ['failed', 'temporary-failure', 'permanent-failure', 'technical-failure', 'virus-scan-failed'] + ['failed', 'temporary-failure', 'permanent-failure', 'technical-failure', 'virus-scan-failed', 'cancelled'] ) ] ) diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 98729d3f4..22d5ac11a 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -156,7 +156,8 @@ def test_jobs_page_doesnt_show_scheduled_on_page_2( [ 'created', 'pending', 'sending', 'pending-virus-check', 'delivered', 'sent', 'returned-letter', - 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure', 'virus-scan-failed', + 'failed', 'temporary-failure', 'permanent-failure', 'technical-failure', + 'virus-scan-failed', 'cancelled', ] ), ( @@ -169,7 +170,7 @@ def test_jobs_page_doesnt_show_scheduled_on_page_2( ), ( 'failed', - ['failed', 'temporary-failure', 'permanent-failure', 'technical-failure', 'virus-scan-failed'] + ['failed', 'temporary-failure', 'permanent-failure', 'technical-failure', 'virus-scan-failed', 'cancelled'] ) ] ) @@ -339,6 +340,7 @@ def test_should_show_letter_job( 'permanent-failure', 'technical-failure', 'virus-scan-failed', + 'cancelled', ], ) diff --git a/tests/app/main/views/test_notifications.py b/tests/app/main/views/test_notifications.py index 48c234b27..0c7df6afb 100644 --- a/tests/app/main/views/test_notifications.py +++ b/tests/app/main/views/test_notifications.py @@ -195,6 +195,10 @@ def test_notification_page_shows_page_for_letter_notification( 'permanent-failure', 'Cancelled 1 January at 1:02am', ), + ( + 'cancelled', + 'Cancelled 1 January at 1:02am', + ), ( 'validation-failed', 'Cancelled 1 January at 1:02am (letter has content outside the printable area)', From 5406efa0cc7becd69c57766ea0818be209d97f28 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Wed, 5 Dec 2018 10:58:27 +0000 Subject: [PATCH 3/3] Add link to cancel letters Added a link to cancel letters from the letter notification pages if the letter is still able to be cancelled. Clicking on this link will show a confirmation box, and will then cancel the letter if the user confirms. --- .../stick-at-top-when-scrolling.scss | 4 + app/main/views/notifications.py | 23 ++- app/navigation.py | 4 + app/templates/flash_messages.html | 2 +- .../views/notifications/notification.html | 8 +- tests/app/main/views/test_notifications.py | 135 ++++++++++++++++++ 6 files changed, 173 insertions(+), 3 deletions(-) diff --git a/app/assets/stylesheets/components/stick-at-top-when-scrolling.scss b/app/assets/stylesheets/components/stick-at-top-when-scrolling.scss index 34bb42526..03584dcdf 100644 --- a/app/assets/stylesheets/components/stick-at-top-when-scrolling.scss +++ b/app/assets/stylesheets/components/stick-at-top-when-scrolling.scss @@ -51,6 +51,10 @@ min-height: 50px; } + .page-footer-delete-link-without-button { + margin-top: 10px; + } + .notification-status { margin: 0; } diff --git a/app/main/views/notifications.py b/app/main/views/notifications.py index 6b08fc86c..5b2a2d99e 100644 --- a/app/main/views/notifications.py +++ b/app/main/views/notifications.py @@ -8,7 +8,9 @@ from dateutil import parser from flask import ( Response, abort, + flash, jsonify, + redirect, render_template, request, stream_with_context, @@ -83,6 +85,11 @@ def view_notification(service_id, notification_id): letter_print_day = get_letter_printing_statement(notification['status'], notification['created_at']) + notification_created = parser.parse(notification['created_at']).replace(tzinfo=None) + + show_cancel_button = notification['notification_type'] == 'letter' and \ + letter_can_be_cancelled(notification['status'], notification_created) + return render_template( 'views/notifications/notification.html', finished=(notification['status'] in (DELIVERED_STATUSES + FAILURE_STATUSES)), @@ -110,10 +117,24 @@ def view_notification(service_id, notification_id): postage=notification['postage'], can_receive_inbound=(current_service.has_permission('inbound_sms')), is_precompiled_letter=notification['template']['is_precompiled_letter'], - letter_print_day=letter_print_day + letter_print_day=letter_print_day, + show_cancel_button=show_cancel_button ) +@main.route("/services//notification//cancel", methods=['GET', 'POST']) +@login_required +@user_has_permissions('view_activity', 'send_messages') +def cancel_letter(service_id, notification_id): + + if request.method == 'POST': + notification_api_client.update_notification_to_cancelled(current_service.id, notification_id) + return redirect(url_for('main.view_notification', service_id=service_id, notification_id=notification_id)) + + flash("Are you sure you want to cancel sending this letter?", 'cancel') + return view_notification(service_id, notification_id) + + def get_letter_printing_statement(status, created_at): created_at_dt = parser.parse(created_at).replace(tzinfo=None) diff --git a/app/navigation.py b/app/navigation.py index 5236a261a..2fb37551d 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -121,6 +121,7 @@ class HeaderNavigation(Navigation): 'cancel_invited_org_user', 'cancel_invited_user', 'cancel_job', + 'cancel_letter', 'check_and_resend_text_code', 'check_and_resend_verification_code', 'check_messages', @@ -397,6 +398,7 @@ class MainNavigation(Navigation): 'cancel_invited_org_user', 'cancel_invited_user', 'cancel_job', + 'cancel_letter', 'check_and_resend_text_code', 'check_and_resend_verification_code', 'check_messages_preview', @@ -569,6 +571,7 @@ class CaseworkNavigation(Navigation): 'cancel_invited_org_user', 'cancel_invited_user', 'cancel_job', + 'cancel_letter', 'check_and_resend_text_code', 'check_and_resend_verification_code', 'check_messages', @@ -806,6 +809,7 @@ class OrgNavigation(Navigation): 'cancel_invited_org_user', 'cancel_invited_user', 'cancel_job', + 'cancel_letter', 'check_and_resend_text_code', 'check_and_resend_verification_code', 'check_messages', diff --git a/app/templates/flash_messages.html b/app/templates/flash_messages.html index 55617e7fd..a95f2fb19 100644 --- a/app/templates/flash_messages.html +++ b/app/templates/flash_messages.html @@ -6,7 +6,7 @@ {{ banner( message if message is string else message[0], 'default' if ((category == 'default') or (category == 'default_with_tick')) else 'dangerous', - delete_button="Yes, {}".format(category) if category in ['delete', 'suspend', 'resume', 'remove', 'revoke this API key'] else None, + delete_button="Yes, {}".format(category) if category in ['cancel', 'delete', 'suspend', 'resume', 'remove', 'revoke this API key'] else None, with_tick=True if category == 'default_with_tick' else False, context=message[1] if message is not string )}} diff --git a/app/templates/views/notifications/notification.html b/app/templates/views/notifications/notification.html index 29bf446d0..1149ef9db 100644 --- a/app/templates/views/notifications/notification.html +++ b/app/templates/views/notifications/notification.html @@ -64,7 +64,13 @@ {% if template.template_type == 'letter' %}
-
 
+ {% if show_cancel_button %} + + Cancel sending this letter + + {% else %} +
 
+ {% endif %} Download as a PDF
diff --git a/tests/app/main/views/test_notifications.py b/tests/app/main/views/test_notifications.py index 0c7df6afb..353c6bc94 100644 --- a/tests/app/main/views/test_notifications.py +++ b/tests/app/main/views/test_notifications.py @@ -241,6 +241,82 @@ def test_notification_page_shows_cancelled_letter( assert page.select_one('main img')['src'].endswith('.png?page=1') +@pytest.mark.parametrize('notification_type', ['email', 'sms']) +@freeze_time('2016-01-01 15:00') +def test_notification_page_does_not_show_cancel_link_for_sms_or_email_notifications( + client_request, + mocker, + fake_uuid, + notification_type, +): + mock_get_notification( + mocker, + fake_uuid, + template_type=notification_type, + notification_status='created', + ) + + page = client_request.get( + 'main.view_notification', + service_id=SERVICE_ONE_ID, + notification_id=fake_uuid, + ) + + assert 'Cancel sending this letter' not in normalize_spaces(page.text) + + +@freeze_time('2016-01-01 15:00') +def test_notification_page_shows_cancel_link_for_letter_which_can_be_cancelled( + client_request, + mocker, + fake_uuid, +): + mock_get_notification( + mocker, + fake_uuid, + template_type='letter', + notification_status='created', + ) + mocker.patch( + 'app.main.views.notifications.get_page_count_for_letter', + return_value=1 + ) + + page = client_request.get( + 'main.view_notification', + service_id=SERVICE_ONE_ID, + notification_id=fake_uuid, + ) + + assert 'Cancel sending this letter' in normalize_spaces(page.text) + + +@freeze_time('2016-01-01 15:00') +def test_notification_page_does_not_show_cancel_link_for_letter_which_cannot_be_cancelled( + client_request, + mocker, + fake_uuid, +): + mock_get_notification( + mocker, + fake_uuid, + template_type='letter', + notification_status='delivered', + ) + mocker.patch( + 'app.main.views.notifications.get_page_count_for_letter', + return_value=1 + ) + + page = client_request.get( + 'main.view_notification', + service_id=SERVICE_ONE_ID, + notification_id=fake_uuid, + ) + + assert 'Cancel sending this letter' not in normalize_spaces(page.text) + + @freeze_time("2016-01-01 18:00") def test_notification_page_shows_page_for_first_class_letter_notification( client_request, @@ -531,3 +607,62 @@ def test_get_letter_printing_statement_for_letter_that_has_been_sent(created_at, statement = get_letter_printing_statement('delivered', created_at) assert statement == 'Printed on {}'.format(print_day) + + +@freeze_time('2016-01-01 15:00') +def test_show_cancel_letter_confirmation( + client_request, + mocker, + fake_uuid, +): + mock_get_notification( + mocker, + fake_uuid, + template_type='letter', + notification_status='created', + ) + mocker.patch( + 'app.main.views.notifications.get_page_count_for_letter', + return_value=1 + ) + + page = client_request.get( + 'main.cancel_letter', + service_id=SERVICE_ONE_ID, + notification_id=fake_uuid, + ) + + flash_message = normalize_spaces(page.find('div', class_='banner-dangerous').text) + + assert 'Are you sure you want to cancel sending this letter?' in flash_message + + +@freeze_time('2016-01-01 15:00') +def test_cancelling_a_letter_calls_the_api( + client_request, + mocker, + fake_uuid, +): + mock_get_notification( + mocker, + fake_uuid, + template_type='letter', + notification_status='created', + ) + mocker.patch( + 'app.main.views.notifications.get_page_count_for_letter', + return_value=1 + ) + cancel_endpoint = mocker.patch( + 'app.main.views.notifications.notification_api_client.update_notification_to_cancelled' + ) + + client_request.post( + 'main.cancel_letter', + service_id=SERVICE_ONE_ID, + notification_id=fake_uuid, + _follow_redirects=True, + _expected_redirect=None, + ) + + assert cancel_endpoint.called