From 5810d46d3527fd679cbcd3704216c9529856cc5e Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Fri, 8 Apr 2022 17:20:44 +0100 Subject: [PATCH 1/4] Don't error sending a letter that's sent already Fixes this error (in Admin): File "/home/vcap/app/app/notify_client/notification_api_client.py", line 74, in send_precompiled_letter return self.post(url='/service/{}/send-pdf-letter'.format(service_id), data=data) File "/home/vcap/app/app/notify_client/__init__.py", line 59, in post return super().post(*args, **kwargs) File "/home/vcap/deps/0/python/lib/python3.9/site-packages/notifications_python_client/base.py", line 48, in post return self.request("POST", url, data=data) File "/home/vcap/deps/0/python/lib/python3.9/site-packages/notifications_python_client/base.py", line 64, in request response = self._perform_request(method, url, kwargs) File "/home/vcap/deps/0/python/lib/python3.9/site-packages/notifications_python_client/base.py", line 118, in _perform_request raise api_error notifications_python_client.errors.HTTPError: 500 - Internal server error Due to this error (in API): File "/home/vcap/app/app/service/send_notification.py", line 178, in send_pdf_letter_notification raise e File "/home/vcap/app/app/service/send_notification.py", line 173, in send_pdf_letter_notification letter = utils_s3download(current_app.config['TRANSIENT_UPLOADED_LETTERS'], file_location) File "/home/vcap/deps/0/python/lib/python3.9/site-packages/notifications_utils/s3.py", line 53, in s3download raise S3ObjectNotFound(error.response, error.operation_name) notifications_utils.s3.S3ObjectNotFound: An error occurred (NoSuchKey) when calling the GetObject operation: The specified key does not exist. I checked the DB to verify the letter does actually exist i.e. it is an instance of the problem we're fixing here. --- app/service/send_notification.py | 12 +++++-- .../test_send_pdf_letter_notification.py | 31 +++++++++++++++++-- 2 files changed, 39 insertions(+), 4 deletions(-) diff --git a/app/service/send_notification.py b/app/service/send_notification.py index 357aa8db0..119f0a38d 100644 --- a/app/service/send_notification.py +++ b/app/service/send_notification.py @@ -7,7 +7,10 @@ from sqlalchemy.orm.exc import NoResultFound from app import create_random_identifier from app.config import QueueNames -from app.dao.notifications_dao import _update_notification_status +from app.dao.notifications_dao import ( + _update_notification_status, + get_notification_by_id, +) from app.dao.service_email_reply_to_dao import dao_get_reply_to_by_id from app.dao.service_sms_sender_dao import dao_get_service_sms_senders_by_id from app.dao.services_dao import dao_fetch_service_by_id @@ -172,9 +175,14 @@ def send_pdf_letter_notification(service_id, post_data): try: letter = utils_s3download(current_app.config['TRANSIENT_UPLOADED_LETTERS'], file_location) except S3ObjectNotFound as e: - current_app.logger.exception('Letter {}.pdf not in transient {} bucket'.format( + current_app.logger.warning('Letter {}.pdf not in transient {} bucket'.format( post_data['file_id'], current_app.config['TRANSIENT_UPLOADED_LETTERS']) ) + + # notification already exists e.g. if the user clicked send in different tabs + if get_notification_by_id(post_data['file_id']): + return {'id': str(post_data['file_id'])} + raise e # Getting the page count won't raise an error since admin has already checked the PDF is valid diff --git a/tests/app/service/test_send_pdf_letter_notification.py b/tests/app/service/test_send_pdf_letter_notification.py index 850db1f96..c507b477c 100644 --- a/tests/app/service/test_send_pdf_letter_notification.py +++ b/tests/app/service/test_send_pdf_letter_notification.py @@ -75,14 +75,41 @@ def test_send_pdf_letter_notification_raises_error_when_pdf_is_not_in_transient_ notify_user, ): user = sample_service_full_permissions.users[0] - post_data = {'filename': 'valid.pdf', 'created_by': user.id, 'file_id': fake_uuid, 'postage': 'first', - 'recipient_address': 'Bugs%20Bunny%0A123%20Main%20Street%0ALooney%20Town'} + post_data = { + 'filename': + 'valid.pdf', + 'created_by': user.id, + 'file_id': fake_uuid, + 'postage': 'first', + 'recipient_address': 'Bugs%20Bunny%0A123%20Main%20Street%0ALooney%20Town' + } mocker.patch('app.service.send_notification.utils_s3download', side_effect=S3ObjectNotFound({}, '')) with pytest.raises(S3ObjectNotFound): send_pdf_letter_notification(sample_service_full_permissions.id, post_data) +def test_send_pdf_letter_notification_does_nothing_if_notification_already_exists( + mocker, + sample_service_full_permissions, + fake_uuid, + notify_user, + sample_notification, +): + user = sample_service_full_permissions.users[0] + post_data = { + 'filename': + 'valid.pdf', + 'created_by': user.id, + 'file_id': str(sample_notification.id), + 'postage': 'first', + 'recipient_address': 'Bugs%20Bunny%0A123%20Main%20Street%0ALooney%20Town' + } + mocker.patch('app.service.send_notification.utils_s3download', side_effect=S3ObjectNotFound({}, '')) + response = send_pdf_letter_notification(sample_service_full_permissions.id, post_data) + assert response['id'] == str(sample_notification.id) + + @freeze_time("2019-08-02 11:00:00") def test_send_pdf_letter_notification_creates_notification_and_moves_letter( mocker, From fa10ec77ab77f7c3412cff6a42c79e1eac98d443 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Fri, 8 Apr 2022 17:32:53 +0100 Subject: [PATCH 2/4] DRY-up test send letter test data into fixture This makes it easier to see what's different in each test. --- .../test_send_pdf_letter_notification.py | 71 +++++++------------ 1 file changed, 26 insertions(+), 45 deletions(-) diff --git a/tests/app/service/test_send_pdf_letter_notification.py b/tests/app/service/test_send_pdf_letter_notification.py index c507b477c..2ff40f884 100644 --- a/tests/app/service/test_send_pdf_letter_notification.py +++ b/tests/app/service/test_send_pdf_letter_notification.py @@ -1,5 +1,3 @@ -import uuid - import pytest from freezegun import freeze_time from notifications_utils.s3 import S3ObjectNotFound @@ -11,18 +9,27 @@ from app.v2.errors import BadRequestError, TooManyRequestsError from tests.app.db import create_service +@pytest.fixture +def post_data(sample_service_full_permissions, fake_uuid): + return { + 'filename': 'valid.pdf', + 'created_by': sample_service_full_permissions.users[0].id, + 'file_id': fake_uuid, + 'postage': 'second', + 'recipient_address': 'Bugs%20Bunny%0A123%20Main%20Street%0ALooney%20Town' + } + + @pytest.mark.parametrize('permissions', [ [EMAIL_TYPE], [UPLOAD_LETTERS], ]) def test_send_pdf_letter_notification_raises_error_if_service_does_not_have_permission( notify_db_session, - fake_uuid, permissions, + post_data, ): service = create_service(service_permissions=permissions) - post_data = {'filename': 'valid.pdf', 'created_by': fake_uuid, 'file_id': fake_uuid, 'postage': 'first', - 'recipient_address': 'Bugs%20Bunny%0A123%20Main%20Street%0ALooney%20Town'} with pytest.raises(BadRequestError): send_pdf_letter_notification(service.id, post_data) @@ -31,23 +38,22 @@ def test_send_pdf_letter_notification_raises_error_if_service_does_not_have_perm def test_send_pdf_letter_notification_raises_error_if_service_is_over_daily_message_limit( mocker, sample_service_full_permissions, - fake_uuid, + post_data, ): mocker.patch( 'app.service.send_notification.check_service_over_daily_message_limit', side_effect=TooManyRequestsError(10)) - post_data = {'filename': 'valid.pdf', 'created_by': fake_uuid, 'file_id': fake_uuid, 'postage': 'first', - 'recipient_address': 'Bugs%20Bunny%0A123%20Main%20Street%0ALooney%20Town'} with pytest.raises(TooManyRequestsError): send_pdf_letter_notification(sample_service_full_permissions.id, post_data) def test_send_pdf_letter_notification_validates_created_by( - sample_service_full_permissions, fake_uuid, sample_user + sample_service_full_permissions, + sample_user, + post_data ): - post_data = {'filename': 'valid.pdf', 'created_by': sample_user.id, 'file_id': fake_uuid, 'postage': 'first', - 'recipient_address': 'Bugs%20Bunny%0A123%20Main%20Street%0ALooney%20Town'} + post_data['created_by'] = sample_user.id with pytest.raises(BadRequestError): send_pdf_letter_notification(sample_service_full_permissions.id, post_data) @@ -56,12 +62,9 @@ def test_send_pdf_letter_notification_validates_created_by( def test_send_pdf_letter_notification_raises_error_if_service_in_trial_mode( mocker, sample_service_full_permissions, - fake_uuid, + post_data, ): sample_service_full_permissions.restricted = True - user = sample_service_full_permissions.users[0] - post_data = {'filename': 'valid.pdf', 'created_by': user.id, 'file_id': fake_uuid, - 'recipient_address': 'Bugs%20Bunny%0A123%20Main%20Street%0ALooney%20Town'} with pytest.raises(BadRequestError) as e: send_pdf_letter_notification(sample_service_full_permissions.id, post_data) @@ -71,18 +74,9 @@ def test_send_pdf_letter_notification_raises_error_if_service_in_trial_mode( def test_send_pdf_letter_notification_raises_error_when_pdf_is_not_in_transient_letter_bucket( mocker, sample_service_full_permissions, - fake_uuid, notify_user, + post_data, ): - user = sample_service_full_permissions.users[0] - post_data = { - 'filename': - 'valid.pdf', - 'created_by': user.id, - 'file_id': fake_uuid, - 'postage': 'first', - 'recipient_address': 'Bugs%20Bunny%0A123%20Main%20Street%0ALooney%20Town' - } mocker.patch('app.service.send_notification.utils_s3download', side_effect=S3ObjectNotFound({}, '')) with pytest.raises(S3ObjectNotFound): @@ -92,19 +86,11 @@ def test_send_pdf_letter_notification_raises_error_when_pdf_is_not_in_transient_ def test_send_pdf_letter_notification_does_nothing_if_notification_already_exists( mocker, sample_service_full_permissions, - fake_uuid, notify_user, sample_notification, + post_data, ): - user = sample_service_full_permissions.users[0] - post_data = { - 'filename': - 'valid.pdf', - 'created_by': user.id, - 'file_id': str(sample_notification.id), - 'postage': 'first', - 'recipient_address': 'Bugs%20Bunny%0A123%20Main%20Street%0ALooney%20Town' - } + post_data['file_id'] = sample_notification.id mocker.patch('app.service.send_notification.utils_s3download', side_effect=S3ObjectNotFound({}, '')) response = send_pdf_letter_notification(sample_service_full_permissions.id, post_data) assert response['id'] == str(sample_notification.id) @@ -115,32 +101,27 @@ def test_send_pdf_letter_notification_creates_notification_and_moves_letter( mocker, sample_service_full_permissions, notify_user, + post_data, ): - user = sample_service_full_permissions.users[0] - filename = 'valid.pdf' - file_id = uuid.uuid4() - post_data = {'filename': filename, 'created_by': user.id, 'file_id': file_id, 'postage': 'second', - 'recipient_address': 'Bugs%20Bunny%0A123%20Main%20Street%0ALooney%20Town'} - mocker.patch('app.service.send_notification.utils_s3download') mocker.patch('app.service.send_notification.get_page_count', return_value=1) s3_mock = mocker.patch('app.service.send_notification.move_uploaded_pdf_to_letters_bucket') result = send_pdf_letter_notification(sample_service_full_permissions.id, post_data) + file_id = post_data['file_id'] notification = get_notification_by_id(file_id) - assert notification.id == file_id + assert str(notification.id) == file_id assert notification.api_key_id is None - assert notification.client_reference == filename - assert notification.created_by_id == user.id + assert notification.client_reference == post_data['filename'] + assert notification.created_by_id == post_data['created_by'] assert notification.postage == 'second' assert notification.notification_type == LETTER_TYPE assert notification.billable_units == 1 assert notification.to == "Bugs Bunny\n123 Main Street\nLooney Town" assert notification.service_id == sample_service_full_permissions.id - assert result == {'id': str(notification.id)} s3_mock.assert_called_once_with( From 70430f10ea620379188af0c950ec25d168a868e2 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Fri, 8 Apr 2022 17:35:47 +0100 Subject: [PATCH 3/4] Co-locate tests for sending a notification I found the send letter tests hard to find as the name of the file didn't match the name of the one containing the code under test. --- tests/app/notifications/rest/__init__.py | 0 .../rest => service/send_notification}/test_send_notification.py | 0 .../{ => send_notification}/test_send_one_off_notification.py | 0 .../{ => send_notification}/test_send_pdf_letter_notification.py | 0 4 files changed, 0 insertions(+), 0 deletions(-) delete mode 100644 tests/app/notifications/rest/__init__.py rename tests/app/{notifications/rest => service/send_notification}/test_send_notification.py (100%) rename tests/app/service/{ => send_notification}/test_send_one_off_notification.py (100%) rename tests/app/service/{ => send_notification}/test_send_pdf_letter_notification.py (100%) diff --git a/tests/app/notifications/rest/__init__.py b/tests/app/notifications/rest/__init__.py deleted file mode 100644 index e69de29bb..000000000 diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/service/send_notification/test_send_notification.py similarity index 100% rename from tests/app/notifications/rest/test_send_notification.py rename to tests/app/service/send_notification/test_send_notification.py diff --git a/tests/app/service/test_send_one_off_notification.py b/tests/app/service/send_notification/test_send_one_off_notification.py similarity index 100% rename from tests/app/service/test_send_one_off_notification.py rename to tests/app/service/send_notification/test_send_one_off_notification.py diff --git a/tests/app/service/test_send_pdf_letter_notification.py b/tests/app/service/send_notification/test_send_pdf_letter_notification.py similarity index 100% rename from tests/app/service/test_send_pdf_letter_notification.py rename to tests/app/service/send_notification/test_send_pdf_letter_notification.py From 413c6c4c2653deba6692bed74deeb0bb8c4c7f37 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Tue, 12 Apr 2022 15:51:06 +0100 Subject: [PATCH 4/4] Move check for existing letter earlier in endpoint In response to: [^1]. [^1]: https://github.com/alphagov/notifications-api/pull/3503#discussion_r848426047 --- app/service/send_notification.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/service/send_notification.py b/app/service/send_notification.py index 119f0a38d..28208eea8 100644 --- a/app/service/send_notification.py +++ b/app/service/send_notification.py @@ -169,6 +169,10 @@ def send_pdf_letter_notification(service_id, post_data): allow_guest_list_recipients=False, ) + # notification already exists e.g. if the user clicked send in different tabs + if get_notification_by_id(post_data['file_id']): + return {'id': str(post_data['file_id'])} + template = get_precompiled_letter_template(service.id) file_location = 'service-{}/{}.pdf'.format(service.id, post_data['file_id']) @@ -179,10 +183,6 @@ def send_pdf_letter_notification(service_id, post_data): post_data['file_id'], current_app.config['TRANSIENT_UPLOADED_LETTERS']) ) - # notification already exists e.g. if the user clicked send in different tabs - if get_notification_by_id(post_data['file_id']): - return {'id': str(post_data['file_id'])} - raise e # Getting the page count won't raise an error since admin has already checked the PDF is valid