From a997fe94a8bb4dae699dbde60528b4cb4fa77c84 Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Thu, 11 Jan 2018 20:27:25 +0000 Subject: [PATCH 1/6] Update pytest-xdist from 1.21.0 to 1.22.0 --- requirements_for_test.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements_for_test.txt b/requirements_for_test.txt index eda5d1c28..27ddd354a 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -3,7 +3,7 @@ flake8==3.5.0 pytest==3.3.2 pytest-mock==1.6.3 pytest-cov==2.5.1 -pytest-xdist==1.21.0 +pytest-xdist==1.22.0 coveralls==1.2.0 freezegun==0.3.9 requests-mock==1.4.0 From 7b4abd076b142286776eb3e974b5240ba042f0ab Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Thu, 11 Jan 2018 14:25:40 +0000 Subject: [PATCH 2/6] Add validation to check that sms recipient is not None Previously, if the SMS recipient was None there would be a 500 error with no message displayed to the user. We now check if the recipient is None and raise a BadRequestError if this is the case. --- app/notifications/validators.py | 3 +++ tests/app/notifications/test_validators.py | 7 +++++++ 2 files changed, 10 insertions(+) diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 921ee350b..85909c5a2 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -98,6 +98,9 @@ def check_service_can_schedule_notification(permissions, scheduled_for): def validate_and_format_recipient(send_to, key_type, service, notification_type): + if send_to is None: + raise BadRequestError(message="Recipient can't be empty") + service_can_send_to_recipient(send_to, key_type, service) if notification_type == SMS_TYPE: diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index 4e426c152..1eb712f2e 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -347,6 +347,13 @@ def test_allows_api_calls_with_international_numbers_if_service_does_allow_int_s assert result == '201212341234' +def test_rejects_api_calls_with_no_recipient(): + with pytest.raises(BadRequestError) as e: + validate_and_format_recipient(None, 'key_type', 'service', 'SMS_TYPE') + assert e.value.status_code == 400 + assert e.value.message == "Recipient can't be empty" + + @pytest.mark.parametrize('notification_type', ['sms', 'email', 'letter']) def test_check_service_email_reply_to_id_where_reply_to_id_is_none(notification_type): assert check_service_email_reply_to_id(None, None, notification_type) is None From 0e6b18854be8de3f2ecf7a55ac9bda05531b7b25 Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Mon, 15 Jan 2018 15:00:27 +0000 Subject: [PATCH 3/6] Update sqlalchemy from 1.2.0 to 1.2.1 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 21a7b9ea1..79f7715a8 100644 --- a/requirements.txt +++ b/requirements.txt @@ -17,7 +17,7 @@ monotonic==1.4 psycopg2==2.7.3.2 PyJWT==1.5.3 SQLAlchemy-Utils==0.32.21 -SQLAlchemy==1.2.0 +SQLAlchemy==1.2.1 notifications-python-client==4.7.1 From 218fc5e14da3fb2e489fc2f7342a43ccea4a4e28 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 15 Jan 2018 17:00:00 +0000 Subject: [PATCH 4/6] only send letters in created state to ftp app for zipping this means if we end up with some notifications sending and others not, due to problems with the ftp connectivity for example, we don't re-send those that worked. As a reminder, letter pdf notifications start as created and stay that way until we have sent the zip file to DVLA, at which point they are updated to sending # --- app/celery/letters_pdf_tasks.py | 22 +++++++++- tests/app/celery/test_letters_pdf_tasks.py | 47 ++++++++++++++++++---- 2 files changed, 60 insertions(+), 9 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 7096f281a..8de14f28b 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -13,8 +13,10 @@ from app.config import QueueNames, TaskNames from app.dao.notifications_dao import ( get_notification_by_id, update_notification_status_by_id, - dao_update_notification + dao_update_notification, + dao_get_notifications_by_references, ) +from app.models import NOTIFICATION_CREATED from app.statsd_decorators import statsd @@ -112,7 +114,7 @@ def group_letters(letter_pdfs): running_filesize = 0 list_of_files = [] for letter in letter_pdfs: - if letter['Key'].lower().endswith('.pdf'): + if letter['Key'].lower().endswith('.pdf') and letter_in_created_state(letter['Key']): if ( running_filesize + letter['Size'] > current_app.config['MAX_LETTER_PDF_ZIP_FILESIZE'] or len(list_of_files) >= current_app.config['MAX_LETTER_PDF_COUNT_PER_ZIP'] @@ -126,3 +128,19 @@ def group_letters(letter_pdfs): if list_of_files: yield list_of_files + + +def letter_in_created_state(filename): + # filename looks like '2018-01-13/NOTIFY.ABCDEF1234567890.D.2.C.C.20180113120000.PDF' + subfolder = filename.split('/')[0] + ref = filename.split('.')[1] + notifications = dao_get_notifications_by_references([ref]) + if notifications: + if notifications[0].status == NOTIFICATION_CREATED: + return True + current_app.logger.info('Collating letters for {} but notification with reference {} already in {}'.format( + subfolder, + ref, + notifications[0].status + )) + return False diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 5c2532934..95942712c 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -11,9 +11,10 @@ from app.celery.letters_pdf_tasks import ( create_letters_pdf, get_letters_pdf, collate_letter_pdfs_for_day, - group_letters + group_letters, + letter_in_created_state, ) -from app.models import Notification +from app.models import Notification, NOTIFICATION_SENDING from tests.conftest import set_config_values @@ -166,7 +167,8 @@ def test_collate_letter_pdfs_for_day(notify_api, mocker): ) -def test_group_letters_splits_on_file_size(notify_api): +def test_group_letters_splits_on_file_size(notify_api, mocker): + mocker.patch('app.celery.letters_pdf_tasks.letter_in_created_state', return_value=True) letters = [ # ends under max but next one is too big {'Key': 'A.pdf', 'Size': 1}, {'Key': 'B.pdf', 'Size': 2}, @@ -192,7 +194,8 @@ def test_group_letters_splits_on_file_size(notify_api): assert next(x, None) is None -def test_group_letters_splits_on_file_count(notify_api): +def test_group_letters_splits_on_file_count(notify_api, mocker): + mocker.patch('app.celery.letters_pdf_tasks.letter_in_created_state', return_value=True) letters = [ {'Key': 'A.pdf', 'Size': 1}, {'Key': 'B.pdf', 'Size': 2}, @@ -215,7 +218,8 @@ def test_group_letters_splits_on_file_count(notify_api): assert next(x, None) is None -def test_group_letters_splits_on_file_size_and_file_count(notify_api): +def test_group_letters_splits_on_file_size_and_file_count(notify_api, mocker): + mocker.patch('app.celery.letters_pdf_tasks.letter_in_created_state', return_value=True) letters = [ # ends under max file size but next file is too big {'Key': 'A.pdf', 'Size': 1}, @@ -249,10 +253,39 @@ def test_group_letters_splits_on_file_size_and_file_count(notify_api): assert next(x, None) is None -def test_group_letters_ignores_non_pdfs(notify_api): +def test_group_letters_ignores_non_pdfs(notify_api, mocker): + mocker.patch('app.celery.letters_pdf_tasks.letter_in_created_state', return_value=True) letters = [{'Key': 'A.zip'}] assert list(group_letters(letters)) == [] -def test_group_letters_with_no_letters(notify_api): +def test_group_letters_ignores_notifications_already_sent(notify_api, mocker): + mock = mocker.patch('app.celery.letters_pdf_tasks.letter_in_created_state', return_value=False) + letters = [{'Key': 'A.pdf'}] + assert list(group_letters(letters)) == [] + mock.assert_called_once_with('A.pdf') + + +def test_group_letters_with_no_letters(notify_api, mocker): + mocker.patch('app.celery.letters_pdf_tasks.letter_in_created_state', return_value=True) assert list(group_letters([])) == [] + + +def test_letter_in_created_state(sample_notification): + sample_notification.reference = 'ABCDEF1234567890' + filename = '2018-01-13/NOTIFY.ABCDEF1234567890.D.2.C.C.20180113120000.PDF' + + assert letter_in_created_state(filename) is True + + +def test_letter_in_created_state_fails_if_notification_not_in_created(sample_notification): + sample_notification.reference = 'ABCDEF1234567890' + sample_notification.status = NOTIFICATION_SENDING + filename = '2018-01-13/NOTIFY.ABCDEF1234567890.D.2.C.C.20180113120000.PDF' + assert letter_in_created_state(filename) is False + + +def test_letter_in_created_state_fails_if_notification_doesnt_exist(sample_notification): + sample_notification.reference = 'QWERTY1234567890' + filename = '2018-01-13/NOTIFY.ABCDEF1234567890.D.2.C.C.20180113120000.PDF' + assert letter_in_created_state(filename) is False From afcbfda566cd53a127a9ad09da85d5fc37bfbdc0 Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Tue, 16 Jan 2018 02:54:27 +0000 Subject: [PATCH 5/6] Update boto3 from 1.5.6 to 1.5.15 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 21a7b9ea1..c5f19084a 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,4 +1,4 @@ -boto3==1.5.6 +boto3==1.5.15 cffi==1.11.0 # pyup: != 1.11.1, != 1.11.2 # These versions are missing .whl celery==3.1.25 # pyup: <4 docopt==0.6.2 From a2cf254a17d0dae2133ef6f9c561438866eb00f2 Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Tue, 16 Jan 2018 02:54:36 +0000 Subject: [PATCH 6/6] Update awscli from 1.14.16 to 1.14.25 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 21a7b9ea1..9cc784c82 100644 --- a/requirements.txt +++ b/requirements.txt @@ -22,7 +22,7 @@ SQLAlchemy==1.2.0 notifications-python-client==4.7.1 # PaaS -awscli==1.14.16 +awscli==1.14.25 awscli-cwlogs>=1.4,<1.5 git+https://github.com/alphagov/notifications-utils.git@23.4.1#egg=notifications-utils==23.4.1