From 2b2c240bee3d4049061c721a3e89d4d6a9246f39 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 27 Jul 2021 15:17:47 +0100 Subject: [PATCH 1/2] Update utils version to bring in too_late_to_cancel_letter We need that method to show right errors to the user when cancelling letter fails Update dependencies --- requirements-app.txt | 2 +- requirements.txt | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/requirements-app.txt b/requirements-app.txt index 43c7fbb3d..3215a4992 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -38,7 +38,7 @@ notifications-python-client==6.0.2 # PaaS awscli-cwlogs==1.4.6 -git+https://github.com/alphagov/notifications-utils.git@44.4.3#egg=notifications-utils==44.4.3 +git+https://github.com/alphagov/notifications-utils.git@44.5.1#egg=notifications-utils==44.5.1 # gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains prometheus-client==0.10.1 diff --git a/requirements.txt b/requirements.txt index 4215ba16e..ca60a7ce8 100644 --- a/requirements.txt +++ b/requirements.txt @@ -40,7 +40,7 @@ notifications-python-client==6.0.2 # PaaS awscli-cwlogs==1.4.6 -git+https://github.com/alphagov/notifications-utils.git@44.4.3#egg=notifications-utils==44.4.3 +git+https://github.com/alphagov/notifications-utils.git@44.5.1#egg=notifications-utils==44.5.1 # gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains prometheus-client==0.10.1 @@ -51,14 +51,14 @@ alembic==1.6.5 amqp==1.4.9 anyjson==0.3.3 attrs==21.2.0 -awscli==1.20.1 +awscli==1.20.8 bcrypt==3.2.0 billiard==3.3.0.23 bleach==3.3.0 blinker==1.4 boto==2.49.0 -boto3==1.18.1 -botocore==1.21.1 +boto3==1.18.8 +botocore==1.21.8 certifi==2021.5.30 charset-normalizer==2.0.3 click==8.0.1 @@ -78,7 +78,7 @@ MarkupSafe==2.0.1 mistune==0.8.4 orderedset==2.0.3 packaging==21.0 -phonenumbers==8.12.27 +phonenumbers==8.12.28 pyasn1==0.4.8 pycparser==2.20 pyparsing==2.4.7 @@ -86,7 +86,7 @@ PyPDF2==1.26.0 pyrsistent==0.18.0 python-dateutil==2.8.2 python-editor==1.0.4 -python-json-logger==2.0.1 +python-json-logger==2.0.2 pytz==2021.1 PyYAML==5.4.1 redis==3.5.3 From 0c8dd247f9cd4c704c871cf00e5bb244bda436cb Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Fri, 23 Jul 2021 17:26:01 +0100 Subject: [PATCH 2/2] Show separate error for when user tries to cancel letter that is already cancelled vs when it is too late to cancel letter vs when we don't know what's the cause of failure. This is so we could show useful error messages to the users and also for better debugging. --- app/service/rest.py | 17 ++++++++++++++--- tests/app/service/test_rest.py | 9 ++++++++- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/app/service/rest.py b/app/service/rest.py index 86908a92b..a22373b02 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -2,7 +2,10 @@ import itertools from datetime import datetime from flask import Blueprint, current_app, jsonify, request -from notifications_utils.letter_timings import letter_can_be_cancelled +from notifications_utils.letter_timings import ( + letter_can_be_cancelled, + too_late_to_cancel_letter, +) from notifications_utils.timezones import convert_utc_to_bst from sqlalchemy.exc import IntegrityError from sqlalchemy.orm.exc import NoResultFound @@ -473,9 +476,17 @@ def cancel_notification_for_service(service_id, notification_id): raise InvalidRequest('Notification cannot be cancelled - only letters can be cancelled', status_code=400) elif not letter_can_be_cancelled(notification.status, notification.created_at): print_day = letter_print_day(notification.created_at) - + if too_late_to_cancel_letter(notification.created_at): + message = "It’s too late to cancel this letter. Printing started {} at 5.30pm".format(print_day) + elif notification.status == 'cancelled': + message = "This letter has already been cancelled." + else: + message = ( + f"We could not cancel this letter. " + f"Letter status: {notification.status}, created_at: {notification.created_at}" + ) raise InvalidRequest( - "It’s too late to cancel this letter. Printing started {} at 5.30pm".format(print_day), + message, status_code=400) updated_notification = notifications_dao.update_notification_status_by_id( diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index f80063dd6..b16ab5fb8 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -3445,6 +3445,7 @@ def test_cancel_notification_for_service_raises_invalid_request_when_letter_is_i notification_status, ): sample_letter_notification.status = notification_status + sample_letter_notification.created_at = datetime.now() response = admin_request.post( 'service.cancel_notification_for_service', @@ -3452,7 +3453,13 @@ def test_cancel_notification_for_service_raises_invalid_request_when_letter_is_i notification_id=sample_letter_notification.id, _expected_status=400 ) - assert response['message'] == 'It’s too late to cancel this letter. Printing started today at 5.30pm' + if notification_status == 'cancelled': + assert response['message'] == 'This letter has already been cancelled.' + else: + assert response['message'] == ( + f"We could not cancel this letter. " + f"Letter status: {notification_status}, created_at: 2018-07-07 12:00:00" + ) assert response['result'] == 'error'