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/requirements.txt b/requirements.txt index 79f7715a8..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 @@ -22,7 +22,7 @@ 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