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/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/requirements.txt b/requirements.txt index 21a7b9ea1..58823e03b 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 @@ -17,12 +17,12 @@ 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 # 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 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 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 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