From 7a0dca61da8bb02a129d9a42328885bee910e6c4 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Mon, 17 Feb 2020 11:41:52 +0000 Subject: [PATCH 01/10] Remove unused argument --- app/aws/s3.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index 01fa04ac3..edb464ff1 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -56,7 +56,7 @@ def remove_job_from_s3(service_id, job_id): return remove_s3_object(*get_job_location(service_id, job_id)) -def get_s3_bucket_objects(bucket_name, subfolder='', older_than=7, limit_days=2): +def get_s3_bucket_objects(bucket_name, subfolder=''): boto_client = client('s3', current_app.config['AWS_REGION']) paginator = boto_client.get_paginator('list_objects_v2') page_iterator = paginator.paginate( From dc9bf757a86a4baafe20211c422e1318b8fa515d Mon Sep 17 00:00:00 2001 From: David McDonald Date: Mon, 17 Feb 2020 15:59:53 +0000 Subject: [PATCH 02/10] Change which letters we want to be sent to look at all days Previously, when running the `collate_letter_pdfs_for_day` task, we would only send letters that were created between 5:30pm yesterday and 5:30 today. Now we send letters that were created before 5:30pm today and that are still waiting to be sent. This will help us automatically attempt to send letters that may have fallen through the gaps and not been sent the previous day when they should have been. Previously we solved the problem of letters that had fallen the gap by having to run the task with a date parameter for example `collate_letter_pdfs_for_day('2020-02-18'). We no longer need this date parameter as we will always look back across previous days too for letters that still need sending. Note, we have to change from using the pagination `list_objects_v2` to instead getting each individual notification from s3. We reduce load by using `HEAD` rather than `GET` but this will still greatly increase the number of API calls. We acknowledge there will be a small cost to this, say 50p for 5000 letters and think this is tolerable. Boto3 also handles retries itself so if when making one of the many HEAD requests, there is a networking blip then it should be retried automatically for us. --- app/aws/s3.py | 6 + app/celery/letters_pdf_tasks.py | 61 +++++-- app/dao/notifications_dao.py | 18 +++ tests/app/celery/test_letters_pdf_tasks.py | 179 ++++++++++++++++++--- 4 files changed, 228 insertions(+), 36 deletions(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index edb464ff1..5c91bf8fc 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -19,6 +19,12 @@ def get_s3_object(bucket_name, file_location): return s3.Object(bucket_name, file_location) +def head_s3_object(bucket_name, file_location): + # https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/s3.html#S3.Client.head_object + boto_client = client('s3', current_app.config['AWS_REGION']) + return boto_client.head_object(Bucket=bucket_name, Key=file_location) + + def file_exists(bucket_name, file_location): try: # try and access metadata of object diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 6f31d2f91..a4649ce6c 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -1,6 +1,6 @@ import math import base64 -from datetime import datetime +from datetime import datetime, timedelta from uuid import UUID from hashlib import sha512 from base64 import urlsafe_b64encode @@ -14,6 +14,8 @@ from requests import ( from celery.exceptions import MaxRetriesExceededError from notifications_utils.statsd_decorators import statsd from notifications_utils.s3 import s3upload +from notifications_utils.letter_timings import LETTER_PROCESSING_DEADLINE +from notifications_utils.timezones import convert_utc_to_bst from app import encryption, notify_celery from app.aws import s3 @@ -25,7 +27,9 @@ from app.dao.notifications_dao import ( dao_get_notification_by_reference, dao_get_notifications_by_references, dao_update_notifications_by_reference, + dao_get_letters_to_be_printed, ) +from app.letters.utils import get_letter_pdf_filename from app.errors import VirusScanError from app.exceptions import NotificationTechnicalFailureException from app.letters.utils import ( @@ -117,26 +121,31 @@ def get_letters_pdf(template, contact_block, filename, values): @notify_celery.task(name='collate-letter-pdfs-for-day') @cronitor("collate-letter-pdfs-for-day") -def collate_letter_pdfs_for_day(date=None): - if not date: - # Using the truncated date is ok because UTC to BST does not make a difference to the date, - # since it is triggered mid afternoon. - date = datetime.utcnow().strftime("%Y-%m-%d") +def collate_letter_pdfs_for_day(): + """ + Finds all letters which are still waiting to be sent to DVLA for printing - letter_pdfs = sorted( - s3.get_s3_bucket_objects( - current_app.config['LETTERS_PDF_BUCKET_NAME'], - subfolder=date - ), - key=lambda letter: letter['Key'] - ) - for i, letters in enumerate(group_letters(letter_pdfs)): + This would usually be run at 5.50pm and collect up letters created between before 5:30pm today + that have not yet been sent. + If run after midnight, it will collect up letters created before 5:30pm the day before. + """ + date = convert_utc_to_bst(datetime.utcnow()) + if date.time() < LETTER_PROCESSING_DEADLINE: + date = date - timedelta(days=1) + + # Using the truncated date is ok because UTC to BST does not make a difference to the date, + # since it is triggered mid afternoon. + print_run_date = date.strftime("%Y-%m-%d") + + letters_to_print = get_key_and_size_of_letters_to_be_sent_to_print(print_run_date) + + for i, letters in enumerate(group_letters(letters_to_print)): filenames = [letter['Key'] for letter in letters] hash = urlsafe_b64encode(sha512(''.join(filenames).encode()).digest())[:20].decode() # eg NOTIFY.2018-12-31.001.Wjrui5nAvObjPd-3GEL-.ZIP dvla_filename = 'NOTIFY.{date}.{num:03}.{hash}.ZIP'.format( - date=date, + date=print_run_date, num=i + 1, hash=hash ) @@ -159,6 +168,28 @@ def collate_letter_pdfs_for_day(date=None): ) +def get_key_and_size_of_letters_to_be_sent_to_print(print_run_date): + letters_awaiting_sending = dao_get_letters_to_be_printed(print_run_date) + + letter_pdfs = [] + for letter in letters_awaiting_sending: + letter_file_name = get_letter_pdf_filename( + reference=letter.reference, + crown=letter.service.crown, + sending_date=letter.created_at, + postage=letter.postage + ) + + current_app.logger.info( + f'Found notification {letter.id} to send to DVLA to print: {letter_file_name}' + ) + + letter_head = s3.head_s3_object(current_app.config['LETTERS_PDF_BUCKET_NAME'], letter_file_name) + letter_pdfs.append({"Key": letter_file_name, "Size": letter_head['ContentLength']}) + + return letter_pdfs + + def group_letters(letter_pdfs): """ Group letters in chunks of MAX_LETTER_PDF_ZIP_FILESIZE. Will add files to lists, never going over that size. diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 5aa8d72a1..89b2bd1c1 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -731,6 +731,24 @@ def notifications_not_yet_sent(should_be_sending_after_seconds, notification_typ return notifications +def dao_get_letters_to_be_printed(print_run_date): + """ + Given a date for a print run, Return all letters created before 5:30pm that day that have not yet been sent + """ + last_processing_deadline = datetime.strptime(print_run_date, "%Y-%m-%d").replace( + hour=17, minute=30, second=0, microsecond=0 + ) + + notifications = Notification.query.filter( + Notification.created_at < convert_bst_to_utc(last_processing_deadline), + Notification.notification_type == LETTER_TYPE, + Notification.status == NOTIFICATION_CREATED + ).order_by( + Notification.created_at + ).all() + return notifications + + def dao_old_letters_with_created_status(): yesterday_bst = convert_utc_to_bst(datetime.utcnow()) - timedelta(days=1) last_processing_deadline = yesterday_bst.replace(hour=17, minute=30, second=0, microsecond=0) diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 58568d52e..efdab98db 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -2,7 +2,7 @@ from unittest.mock import Mock, call, ANY import base64 import boto3 -from datetime import datetime +from datetime import datetime, timedelta from moto import mock_s3 from flask import current_app from freezegun import freeze_time @@ -20,6 +20,7 @@ from app.celery.letters_pdf_tasks import ( create_letters_pdf, get_letters_pdf, collate_letter_pdfs_for_day, + get_key_and_size_of_letters_to_be_sent_to_print, group_letters, letter_in_created_state, process_sanitised_letter, @@ -242,27 +243,113 @@ def test_create_letters_gets_the_right_logo_when_service_has_letter_branding_log ) -def test_collate_letter_pdfs_for_day(notify_api, mocker): - mock_s3 = mocker.patch('app.celery.tasks.s3.get_s3_bucket_objects', return_value=[ - {'Key': 'B.pDf', 'Size': 2}, - {'Key': 'A.PDF', 'Size': 1}, - {'Key': 'C.pdf', 'Size': 3} +@freeze_time('2020-02-17 18:00:00') +def test_get_key_and_size_of_letters_to_be_sent_to_print(notify_api, mocker, sample_letter_template): + create_notification( + template=sample_letter_template, + status='created', + reference='ref0', + created_at=(datetime.now() - timedelta(hours=2)).isoformat() + ) + + create_notification( + template=sample_letter_template, + status='created', + reference='ref1', + created_at=(datetime.now() - timedelta(hours=3)).isoformat() + ) + + create_notification( + template=sample_letter_template, + status='created', + reference='ref2', + created_at=(datetime.now() - timedelta(days=2)).isoformat() + ) + + # notifications we don't expect to get sent to print as they are in the wrong status + for status in ['delivered', 'validation-failed', 'cancelled', 'sending']: + create_notification( + template=sample_letter_template, + status=status, + reference='ref3', + created_at=(datetime.now() - timedelta(days=2)).isoformat() + ) + + # notification we don't expect to get sent as instead will make into this evenings print run + create_notification( + template=sample_letter_template, + status='created', + reference='ref4', + created_at=(datetime.now() - timedelta(minutes=1)).isoformat() + ) + + mock_s3 = mocker.patch('app.celery.tasks.s3.head_s3_object', side_effect=[ + {'ContentLength': 2}, + {'ContentLength': 1}, + {'ContentLength': 3}, ]) - mock_group_letters = mocker.patch('app.celery.letters_pdf_tasks.group_letters', return_value=[ - [{'Key': 'A.PDF', 'Size': 1}, {'Key': 'B.pDf', 'Size': 2}], - [{'Key': 'C.pdf', 'Size': 3}] + + results = get_key_and_size_of_letters_to_be_sent_to_print('2020-02-17') + + assert mock_s3.call_count == 3 + mock_s3.assert_has_calls( + [ + call(current_app.config['LETTERS_PDF_BUCKET_NAME'], '2020-02-16/NOTIFY.REF2.D.2.C.C.20200215180000.PDF'), + call(current_app.config['LETTERS_PDF_BUCKET_NAME'], '2020-02-17/NOTIFY.REF1.D.2.C.C.20200217150000.PDF'), + call(current_app.config['LETTERS_PDF_BUCKET_NAME'], '2020-02-17/NOTIFY.REF0.D.2.C.C.20200217160000.PDF'), + ] + ) + + assert len(results) == 3 + assert results == [ + {'Key': '2020-02-16/NOTIFY.REF2.D.2.C.C.20200215180000.PDF', 'Size': 2}, + {'Key': '2020-02-17/NOTIFY.REF1.D.2.C.C.20200217150000.PDF', 'Size': 1}, + {'Key': '2020-02-17/NOTIFY.REF0.D.2.C.C.20200217160000.PDF', 'Size': 3}, + ] + + +@freeze_time('2020-02-17 18:00:00') +def test_collate_letter_pdfs_for_day(notify_api, sample_letter_template, mocker): + create_notification( + template=sample_letter_template, + status='created', + reference='ref0', + created_at=(datetime.now() - timedelta(hours=2)).isoformat() + ) + + create_notification( + template=sample_letter_template, + status='created', + reference='ref1', + created_at=(datetime.now() - timedelta(hours=3)).isoformat() + ) + + create_notification( + template=sample_letter_template, + status='created', + reference='ref2', + created_at=(datetime.now() - timedelta(days=2)).isoformat() + ) + + mocker.patch('app.celery.tasks.s3.head_s3_object', side_effect=[ + {'ContentLength': 2}, + {'ContentLength': 1}, + {'ContentLength': 3}, + ]) + mocker.patch('app.celery.letters_pdf_tasks.group_letters', return_value=[ + [{'Key': '2020-02-16/A.PDF', 'Size': 1}, {'Key': '2020-02-17/B.pDf', 'Size': 2}], + [{'Key': '2020-02-17/C.pdf', 'Size': 3}] ]) mock_celery = mocker.patch('app.celery.letters_pdf_tasks.notify_celery.send_task') - collate_letter_pdfs_for_day('2017-01-02') + collate_letter_pdfs_for_day() - mock_s3.assert_called_once_with('test-letters-pdf', subfolder='2017-01-02') - mock_group_letters.assert_called_once_with(sorted(mock_s3.return_value, key=lambda x: x['Key'])) + assert len(mock_celery.call_args_list) == 2 assert mock_celery.call_args_list[0] == call( name='zip-and-send-letter-pdfs', kwargs={ - 'filenames_to_zip': ['A.PDF', 'B.pDf'], - 'upload_filename': 'NOTIFY.2017-01-02.001.oqdjIM2-NAUU9Sm5Slmi.ZIP' + 'filenames_to_zip': ['2020-02-16/A.PDF', '2020-02-17/B.pDf'], + 'upload_filename': 'NOTIFY.2020-02-17.001.fGRjtoP1V9XUrng24Zzq.ZIP' }, queue='process-ftp-tasks', compression='zlib' @@ -270,20 +357,70 @@ def test_collate_letter_pdfs_for_day(notify_api, mocker): assert mock_celery.call_args_list[1] == call( name='zip-and-send-letter-pdfs', kwargs={ - 'filenames_to_zip': ['C.pdf'], - 'upload_filename': 'NOTIFY.2017-01-02.002.tdr7hcdPieiqjkVoS4kU.ZIP' + 'filenames_to_zip': ['2020-02-17/C.pdf'], + 'upload_filename': 'NOTIFY.2020-02-17.002.Ay8mP80pe12NH46clM08.ZIP' }, queue='process-ftp-tasks', compression='zlib' ) -@freeze_time('2018-09-12 17:50:00') -def test_collate_letter_pdfs_for_day_works_without_date_param(notify_api, mocker): - mock_s3 = mocker.patch('app.celery.tasks.s3.get_s3_bucket_objects') +@freeze_time('2020-02-18 02:00:00') +def test_collate_letter_pdfs_for_day_when_run_after_midnight(notify_api, sample_letter_template, mocker): + # created_at times for notifications choosen to match times in above test test_collate_letter_pdfs_for_day + create_notification( + template=sample_letter_template, + status='created', + reference='ref0', + created_at=(datetime.now() - timedelta(hours=10)).isoformat() + ) + + create_notification( + template=sample_letter_template, + status='created', + reference='ref1', + created_at=(datetime.now() - timedelta(hours=11)).isoformat() + ) + + create_notification( + template=sample_letter_template, + status='created', + reference='ref2', + created_at=(datetime.now() - timedelta(days=2, hours=8)).isoformat() + ) + + mocker.patch('app.celery.tasks.s3.head_s3_object', side_effect=[ + {'ContentLength': 2}, + {'ContentLength': 1}, + {'ContentLength': 3}, + ]) + mocker.patch('app.celery.letters_pdf_tasks.group_letters', return_value=[ + [{'Key': '2020-02-16/A.PDF', 'Size': 1}, {'Key': '2020-02-17/B.pDf', 'Size': 2}], + [{'Key': '2020-02-17/C.pdf', 'Size': 3}] + ]) + mock_celery = mocker.patch('app.celery.letters_pdf_tasks.notify_celery.send_task') + collate_letter_pdfs_for_day() - expected_date = '2018-09-12' - mock_s3.assert_called_once_with('test-letters-pdf', subfolder=expected_date) + + assert len(mock_celery.call_args_list) == 2 + assert mock_celery.call_args_list[0] == call( + name='zip-and-send-letter-pdfs', + kwargs={ + 'filenames_to_zip': ['2020-02-16/A.PDF', '2020-02-17/B.pDf'], + 'upload_filename': 'NOTIFY.2020-02-17.001.fGRjtoP1V9XUrng24Zzq.ZIP' + }, + queue='process-ftp-tasks', + compression='zlib' + ) + assert mock_celery.call_args_list[1] == call( + name='zip-and-send-letter-pdfs', + kwargs={ + 'filenames_to_zip': ['2020-02-17/C.pdf'], + 'upload_filename': 'NOTIFY.2020-02-17.002.Ay8mP80pe12NH46clM08.ZIP' + }, + queue='process-ftp-tasks', + compression='zlib' + ) def test_group_letters_splits_on_file_size(notify_api, mocker): From 5c5eb8a96aff9f9b65f43be8df69de40392df90a Mon Sep 17 00:00:00 2001 From: David McDonald Date: Wed, 19 Feb 2020 12:54:06 +0000 Subject: [PATCH 03/10] Remove unneeded check that notification is in created state We instead rely on the fact that only files being passed into this function we already know are in the created state --- app/celery/letters_pdf_tasks.py | 23 +------------ tests/app/celery/test_letters_pdf_tasks.py | 40 ++-------------------- 2 files changed, 4 insertions(+), 59 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index a4649ce6c..f76492849 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -25,7 +25,6 @@ from app.dao.notifications_dao import ( update_notification_status_by_id, dao_update_notification, dao_get_notification_by_reference, - dao_get_notifications_by_references, dao_update_notifications_by_reference, dao_get_letters_to_be_printed, ) @@ -180,10 +179,6 @@ def get_key_and_size_of_letters_to_be_sent_to_print(print_run_date): postage=letter.postage ) - current_app.logger.info( - f'Found notification {letter.id} to send to DVLA to print: {letter_file_name}' - ) - letter_head = s3.head_s3_object(current_app.config['LETTERS_PDF_BUCKET_NAME'], letter_file_name) letter_pdfs.append({"Key": letter_file_name, "Size": letter_head['ContentLength']}) @@ -199,7 +194,7 @@ def group_letters(letter_pdfs): running_filesize = 0 list_of_files = [] for letter in letter_pdfs: - if letter['Key'].lower().endswith('.pdf') and letter_in_created_state(letter['Key']): + if letter['Key'].lower().endswith('.pdf'): 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'] @@ -215,22 +210,6 @@ def group_letters(letter_pdfs): 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 = get_reference_from_filename(filename) - 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 - - @notify_celery.task(bind=True, name='process-virus-scan-passed', max_retries=15, default_retry_delay=300) def process_virus_scan_passed(self, filename): reference = get_reference_from_filename(filename) diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index efdab98db..f8c566e37 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -22,7 +22,6 @@ from app.celery.letters_pdf_tasks import ( collate_letter_pdfs_for_day, get_key_and_size_of_letters_to_be_sent_to_print, group_letters, - letter_in_created_state, process_sanitised_letter, process_virus_scan_passed, process_virus_scan_failed, @@ -41,7 +40,6 @@ from app.models import ( NOTIFICATION_CREATED, NOTIFICATION_DELIVERED, NOTIFICATION_PENDING_VIRUS_CHECK, - NOTIFICATION_SENDING, NOTIFICATION_TECHNICAL_FAILURE, NOTIFICATION_VALIDATION_FAILED, NOTIFICATION_VIRUS_SCAN_FAILED, @@ -423,8 +421,7 @@ def test_collate_letter_pdfs_for_day_when_run_after_midnight(notify_api, sample_ ) -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) +def test_group_letters_splits_on_file_size(notify_api): letters = [ # ends under max but next one is too big {'Key': 'A.pdf', 'Size': 1}, {'Key': 'B.pdf', 'Size': 2}, @@ -450,8 +447,7 @@ def test_group_letters_splits_on_file_size(notify_api, mocker): assert next(x, None) is None -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) +def test_group_letters_splits_on_file_count(notify_api): letters = [ {'Key': 'A.pdf', 'Size': 1}, {'Key': 'B.pdf', 'Size': 2}, @@ -474,8 +470,7 @@ def test_group_letters_splits_on_file_count(notify_api, mocker): assert next(x, None) is None -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) +def test_group_letters_splits_on_file_size_and_file_count(notify_api): letters = [ # ends under max file size but next file is too big {'Key': 'A.pdf', 'Size': 1}, @@ -510,43 +505,14 @@ def test_group_letters_splits_on_file_size_and_file_count(notify_api, mocker): 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_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 - - @freeze_time('2018-01-01 18:00') @mock_s3 @pytest.mark.parametrize('key_type,noti_status,bucket_config_name,destination_folder', [ From 7578e01b3b5ac2ac39b82055af2e8475c1e14dba Mon Sep 17 00:00:00 2001 From: David McDonald Date: Wed, 19 Feb 2020 13:21:30 +0000 Subject: [PATCH 04/10] Test cases explicitely In previous tests we check that we can deal with files that end in `pdf` in various forms of upper and lowercase. I've moved the testing of this behaviour into it's own test so that's explicit and not just implied that we care about behaviour on the casing of filenames. Note however that s3 is case sensitive and we upload all our files in upper case so technically we'd never expect to see a file ending in `pdf`. I've updated some of our test data to reflect this but didn't bother doing it everywhere. I've left the test in anyway but it could be argued that is is redundant as we don't ever expect to see that case anyway. --- tests/app/celery/test_letters_pdf_tasks.py | 37 +++++++++++++--------- 1 file changed, 22 insertions(+), 15 deletions(-) diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index f8c566e37..efd3f1004 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -335,8 +335,8 @@ def test_collate_letter_pdfs_for_day(notify_api, sample_letter_template, mocker) {'ContentLength': 3}, ]) mocker.patch('app.celery.letters_pdf_tasks.group_letters', return_value=[ - [{'Key': '2020-02-16/A.PDF', 'Size': 1}, {'Key': '2020-02-17/B.pDf', 'Size': 2}], - [{'Key': '2020-02-17/C.pdf', 'Size': 3}] + [{'Key': '2020-02-16/A.PDF', 'Size': 1}, {'Key': '2020-02-17/B.PDF', 'Size': 2}], + [{'Key': '2020-02-17/C.PDF', 'Size': 3}] ]) mock_celery = mocker.patch('app.celery.letters_pdf_tasks.notify_celery.send_task') @@ -346,8 +346,8 @@ def test_collate_letter_pdfs_for_day(notify_api, sample_letter_template, mocker) assert mock_celery.call_args_list[0] == call( name='zip-and-send-letter-pdfs', kwargs={ - 'filenames_to_zip': ['2020-02-16/A.PDF', '2020-02-17/B.pDf'], - 'upload_filename': 'NOTIFY.2020-02-17.001.fGRjtoP1V9XUrng24Zzq.ZIP' + 'filenames_to_zip': ['2020-02-16/A.PDF', '2020-02-17/B.PDF'], + 'upload_filename': 'NOTIFY.2020-02-17.001.pneY1OU2TUq7KfFSrJ2Q.ZIP' }, queue='process-ftp-tasks', compression='zlib' @@ -355,8 +355,8 @@ def test_collate_letter_pdfs_for_day(notify_api, sample_letter_template, mocker) assert mock_celery.call_args_list[1] == call( name='zip-and-send-letter-pdfs', kwargs={ - 'filenames_to_zip': ['2020-02-17/C.pdf'], - 'upload_filename': 'NOTIFY.2020-02-17.002.Ay8mP80pe12NH46clM08.ZIP' + 'filenames_to_zip': ['2020-02-17/C.PDF'], + 'upload_filename': 'NOTIFY.2020-02-17.002.Wy0jBtrnVzWeGqLXhE_f.ZIP' }, queue='process-ftp-tasks', compression='zlib' @@ -393,8 +393,8 @@ def test_collate_letter_pdfs_for_day_when_run_after_midnight(notify_api, sample_ {'ContentLength': 3}, ]) mocker.patch('app.celery.letters_pdf_tasks.group_letters', return_value=[ - [{'Key': '2020-02-16/A.PDF', 'Size': 1}, {'Key': '2020-02-17/B.pDf', 'Size': 2}], - [{'Key': '2020-02-17/C.pdf', 'Size': 3}] + [{'Key': '2020-02-16/A.PDF', 'Size': 1}, {'Key': '2020-02-17/B.PDF', 'Size': 2}], + [{'Key': '2020-02-17/C.PDF', 'Size': 3}] ]) mock_celery = mocker.patch('app.celery.letters_pdf_tasks.notify_celery.send_task') @@ -404,8 +404,8 @@ def test_collate_letter_pdfs_for_day_when_run_after_midnight(notify_api, sample_ assert mock_celery.call_args_list[0] == call( name='zip-and-send-letter-pdfs', kwargs={ - 'filenames_to_zip': ['2020-02-16/A.PDF', '2020-02-17/B.pDf'], - 'upload_filename': 'NOTIFY.2020-02-17.001.fGRjtoP1V9XUrng24Zzq.ZIP' + 'filenames_to_zip': ['2020-02-16/A.PDF', '2020-02-17/B.PDF'], + 'upload_filename': 'NOTIFY.2020-02-17.001.pneY1OU2TUq7KfFSrJ2Q.ZIP' }, queue='process-ftp-tasks', compression='zlib' @@ -413,8 +413,8 @@ def test_collate_letter_pdfs_for_day_when_run_after_midnight(notify_api, sample_ assert mock_celery.call_args_list[1] == call( name='zip-and-send-letter-pdfs', kwargs={ - 'filenames_to_zip': ['2020-02-17/C.pdf'], - 'upload_filename': 'NOTIFY.2020-02-17.002.Ay8mP80pe12NH46clM08.ZIP' + 'filenames_to_zip': ['2020-02-17/C.PDF'], + 'upload_filename': 'NOTIFY.2020-02-17.002.Wy0jBtrnVzWeGqLXhE_f.ZIP' }, queue='process-ftp-tasks', compression='zlib' @@ -504,12 +504,19 @@ 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, mocker): - letters = [{'Key': 'A.zip'}] +@pytest.mark.parametrize('key', ["A.ZIP", "B.zip"]) +def test_group_letters_ignores_non_pdfs(key): + letters = [{'Key': key, 'Size': 1}] assert list(group_letters(letters)) == [] -def test_group_letters_with_no_letters(notify_api, mocker): +@pytest.mark.parametrize('key', ["A.PDF", "B.pdf", "C.PdF"]) +def test_group_letters_includes_pdf_files(key): + letters = [{'Key': key, 'Size': 1}] + assert list(group_letters(letters)) == [[{'Key': key, 'Size': 1}]] + + +def test_group_letters_with_no_letters(): assert list(group_letters([])) == [] From 6226d9e122e031668fd368219a3e3e3d89d7ccde Mon Sep 17 00:00:00 2001 From: David McDonald Date: Wed, 19 Feb 2020 13:36:05 +0000 Subject: [PATCH 05/10] Don't send test letters to dvla to print --- app/dao/notifications_dao.py | 3 ++- tests/app/celery/test_letters_pdf_tasks.py | 9 +++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 89b2bd1c1..7bec34351 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -742,7 +742,8 @@ def dao_get_letters_to_be_printed(print_run_date): notifications = Notification.query.filter( Notification.created_at < convert_bst_to_utc(last_processing_deadline), Notification.notification_type == LETTER_TYPE, - Notification.status == NOTIFICATION_CREATED + Notification.status == NOTIFICATION_CREATED, + Notification.key_type == KEY_TYPE_NORMAL ).order_by( Notification.created_at ).all() diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index efd3f1004..a66045e2b 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -281,6 +281,15 @@ def test_get_key_and_size_of_letters_to_be_sent_to_print(notify_api, mocker, sam created_at=(datetime.now() - timedelta(minutes=1)).isoformat() ) + # test notification we don't expect to get sent + create_notification( + template=sample_letter_template, + status='created', + reference='ref4', + created_at=(datetime.now() - timedelta(days=1)).isoformat(), + key_type=KEY_TYPE_TEST + ) + mock_s3 = mocker.patch('app.celery.tasks.s3.head_s3_object', side_effect=[ {'ContentLength': 2}, {'ContentLength': 1}, From 148a5ab4565da5f751f40be9e30eacbdbd6bf46f Mon Sep 17 00:00:00 2001 From: David McDonald Date: Wed, 19 Feb 2020 14:23:33 +0000 Subject: [PATCH 06/10] Refactor dates being passed around I believe this way is nicer to read, we don't have to change between datetimes and strings and back. --- app/celery/letters_pdf_tasks.py | 22 ++++++++++++---------- app/dao/notifications_dao.py | 10 +++------- tests/app/celery/test_letters_pdf_tasks.py | 2 +- 3 files changed, 16 insertions(+), 18 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index f76492849..43ca345b2 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -128,15 +128,17 @@ def collate_letter_pdfs_for_day(): that have not yet been sent. If run after midnight, it will collect up letters created before 5:30pm the day before. """ - date = convert_utc_to_bst(datetime.utcnow()) - if date.time() < LETTER_PROCESSING_DEADLINE: - date = date - timedelta(days=1) + print_run_date = convert_utc_to_bst(datetime.utcnow()) + if print_run_date.time() < LETTER_PROCESSING_DEADLINE: + print_run_date = print_run_date - timedelta(days=1) - # Using the truncated date is ok because UTC to BST does not make a difference to the date, - # since it is triggered mid afternoon. - print_run_date = date.strftime("%Y-%m-%d") + # We can truncate the datetime to a date and add our own time (5:30pm) because UTC to BST does not + # make a difference to the date since it is triggered mid afternoon. + print_run_deadline = print_run_date.replace( + hour=17, minute=30, second=0, microsecond=0 + ) - letters_to_print = get_key_and_size_of_letters_to_be_sent_to_print(print_run_date) + letters_to_print = get_key_and_size_of_letters_to_be_sent_to_print(print_run_deadline) for i, letters in enumerate(group_letters(letters_to_print)): filenames = [letter['Key'] for letter in letters] @@ -144,7 +146,7 @@ def collate_letter_pdfs_for_day(): hash = urlsafe_b64encode(sha512(''.join(filenames).encode()).digest())[:20].decode() # eg NOTIFY.2018-12-31.001.Wjrui5nAvObjPd-3GEL-.ZIP dvla_filename = 'NOTIFY.{date}.{num:03}.{hash}.ZIP'.format( - date=print_run_date, + date=print_run_deadline.strftime("%Y-%m-%d"), num=i + 1, hash=hash ) @@ -167,8 +169,8 @@ def collate_letter_pdfs_for_day(): ) -def get_key_and_size_of_letters_to_be_sent_to_print(print_run_date): - letters_awaiting_sending = dao_get_letters_to_be_printed(print_run_date) +def get_key_and_size_of_letters_to_be_sent_to_print(print_run_deadline): + letters_awaiting_sending = dao_get_letters_to_be_printed(print_run_deadline) letter_pdfs = [] for letter in letters_awaiting_sending: diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 7bec34351..645bf9090 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -731,16 +731,12 @@ def notifications_not_yet_sent(should_be_sending_after_seconds, notification_typ return notifications -def dao_get_letters_to_be_printed(print_run_date): +def dao_get_letters_to_be_printed(print_run_deadline): """ - Given a date for a print run, Return all letters created before 5:30pm that day that have not yet been sent + Return all letters created before the print run deadline that have not yet been sent """ - last_processing_deadline = datetime.strptime(print_run_date, "%Y-%m-%d").replace( - hour=17, minute=30, second=0, microsecond=0 - ) - notifications = Notification.query.filter( - Notification.created_at < convert_bst_to_utc(last_processing_deadline), + Notification.created_at < convert_bst_to_utc(print_run_deadline), Notification.notification_type == LETTER_TYPE, Notification.status == NOTIFICATION_CREATED, Notification.key_type == KEY_TYPE_NORMAL diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index a66045e2b..911907009 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -296,7 +296,7 @@ def test_get_key_and_size_of_letters_to_be_sent_to_print(notify_api, mocker, sam {'ContentLength': 3}, ]) - results = get_key_and_size_of_letters_to_be_sent_to_print('2020-02-17') + results = get_key_and_size_of_letters_to_be_sent_to_print(datetime.now() - timedelta(minutes=30)) assert mock_s3.call_count == 3 mock_s3.assert_has_calls( From e6767590d4c747231960679bd79d2c7c367bc978 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Wed, 19 Feb 2020 14:31:57 +0000 Subject: [PATCH 07/10] Change function and task name to be more accurate Will require us to change a cronitor set up --- app/celery/letters_pdf_tasks.py | 6 +++--- app/config.py | 4 ++-- tests/app/celery/test_letters_pdf_tasks.py | 14 +++++++------- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 43ca345b2..e0685b2c9 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -118,9 +118,9 @@ def get_letters_pdf(template, contact_block, filename, values): return resp.content, billable_units -@notify_celery.task(name='collate-letter-pdfs-for-day') -@cronitor("collate-letter-pdfs-for-day") -def collate_letter_pdfs_for_day(): +@notify_celery.task(name='collate-letter-pdfs-to-be-sent') +@cronitor("collate-letter-pdfs-to-be-sent") +def collate_letter_pdfs_to_be_sent(): """ Finds all letters which are still waiting to be sent to DVLA for printing diff --git a/app/config.py b/app/config.py index 602f57f2c..a4949a175 100644 --- a/app/config.py +++ b/app/config.py @@ -296,8 +296,8 @@ class Config(object): }, # The collate-letter-pdf does assume it is called in an hour that BST does not make a # difference to the truncate date which translates to the filename to process - 'collate-letter-pdfs-for-day': { - 'task': 'collate-letter-pdfs-for-day', + 'collate-letter-pdfs-to-be-sent': { + 'task': 'collate-letter-pdfs-to-be-sent', 'schedule': crontab(hour=17, minute=50), 'options': {'queue': QueueNames.PERIODIC} }, diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 911907009..8f43ee797 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -19,7 +19,7 @@ from app.exceptions import NotificationTechnicalFailureException from app.celery.letters_pdf_tasks import ( create_letters_pdf, get_letters_pdf, - collate_letter_pdfs_for_day, + collate_letter_pdfs_to_be_sent, get_key_and_size_of_letters_to_be_sent_to_print, group_letters, process_sanitised_letter, @@ -52,7 +52,7 @@ from tests.conftest import set_config_values def test_should_have_decorated_tasks_functions(): assert create_letters_pdf.__wrapped__.__name__ == 'create_letters_pdf' - assert collate_letter_pdfs_for_day.__wrapped__.__name__ == 'collate_letter_pdfs_for_day' + assert collate_letter_pdfs_to_be_sent.__wrapped__.__name__ == 'collate_letter_pdfs_to_be_sent' assert process_virus_scan_passed.__wrapped__.__name__ == 'process_virus_scan_passed' assert process_virus_scan_failed.__wrapped__.__name__ == 'process_virus_scan_failed' assert process_virus_scan_error.__wrapped__.__name__ == 'process_virus_scan_error' @@ -316,7 +316,7 @@ def test_get_key_and_size_of_letters_to_be_sent_to_print(notify_api, mocker, sam @freeze_time('2020-02-17 18:00:00') -def test_collate_letter_pdfs_for_day(notify_api, sample_letter_template, mocker): +def test_collate_letter_pdfs_to_be_sent(notify_api, sample_letter_template, mocker): create_notification( template=sample_letter_template, status='created', @@ -349,7 +349,7 @@ def test_collate_letter_pdfs_for_day(notify_api, sample_letter_template, mocker) ]) mock_celery = mocker.patch('app.celery.letters_pdf_tasks.notify_celery.send_task') - collate_letter_pdfs_for_day() + collate_letter_pdfs_to_be_sent() assert len(mock_celery.call_args_list) == 2 assert mock_celery.call_args_list[0] == call( @@ -373,8 +373,8 @@ def test_collate_letter_pdfs_for_day(notify_api, sample_letter_template, mocker) @freeze_time('2020-02-18 02:00:00') -def test_collate_letter_pdfs_for_day_when_run_after_midnight(notify_api, sample_letter_template, mocker): - # created_at times for notifications choosen to match times in above test test_collate_letter_pdfs_for_day +def test_collate_letter_pdfs_to_be_sent_when_run_after_midnight(notify_api, sample_letter_template, mocker): + # created_at times for notifications choosen to match times in above test test_collate_letter_pdfs_to_be_sent create_notification( template=sample_letter_template, status='created', @@ -407,7 +407,7 @@ def test_collate_letter_pdfs_for_day_when_run_after_midnight(notify_api, sample_ ]) mock_celery = mocker.patch('app.celery.letters_pdf_tasks.notify_celery.send_task') - collate_letter_pdfs_for_day() + collate_letter_pdfs_to_be_sent() assert len(mock_celery.call_args_list) == 2 assert mock_celery.call_args_list[0] == call( From 6f663268e140e712d9ce6916d42c66b423739939 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Fri, 21 Feb 2020 16:19:47 +0000 Subject: [PATCH 08/10] remove unneeded mock --- tests/app/celery/test_letters_pdf_tasks.py | 41 ++++++++++++---------- 1 file changed, 23 insertions(+), 18 deletions(-) diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 8f43ee797..96d358dff 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -343,20 +343,21 @@ def test_collate_letter_pdfs_to_be_sent(notify_api, sample_letter_template, mock {'ContentLength': 1}, {'ContentLength': 3}, ]) - mocker.patch('app.celery.letters_pdf_tasks.group_letters', return_value=[ - [{'Key': '2020-02-16/A.PDF', 'Size': 1}, {'Key': '2020-02-17/B.PDF', 'Size': 2}], - [{'Key': '2020-02-17/C.PDF', 'Size': 3}] - ]) + mock_celery = mocker.patch('app.celery.letters_pdf_tasks.notify_celery.send_task') - collate_letter_pdfs_to_be_sent() + with set_config_values(notify_api, {'MAX_LETTER_PDF_COUNT_PER_ZIP': 2}): + collate_letter_pdfs_to_be_sent() assert len(mock_celery.call_args_list) == 2 assert mock_celery.call_args_list[0] == call( name='zip-and-send-letter-pdfs', kwargs={ - 'filenames_to_zip': ['2020-02-16/A.PDF', '2020-02-17/B.PDF'], - 'upload_filename': 'NOTIFY.2020-02-17.001.pneY1OU2TUq7KfFSrJ2Q.ZIP' + 'filenames_to_zip': [ + '2020-02-16/NOTIFY.REF2.D.2.C.C.20200215180000.PDF', + '2020-02-17/NOTIFY.REF1.D.2.C.C.20200217150000.PDF' + ], + 'upload_filename': 'NOTIFY.2020-02-17.001.k3x_WqC5KhB6e2DWv9Ma.ZIP' }, queue='process-ftp-tasks', compression='zlib' @@ -364,8 +365,10 @@ def test_collate_letter_pdfs_to_be_sent(notify_api, sample_letter_template, mock assert mock_celery.call_args_list[1] == call( name='zip-and-send-letter-pdfs', kwargs={ - 'filenames_to_zip': ['2020-02-17/C.PDF'], - 'upload_filename': 'NOTIFY.2020-02-17.002.Wy0jBtrnVzWeGqLXhE_f.ZIP' + 'filenames_to_zip': [ + '2020-02-17/NOTIFY.REF0.D.2.C.C.20200217160000.PDF' + ], + 'upload_filename': 'NOTIFY.2020-02-17.002.J85cUw-FWlKuAIOcwdLS.ZIP' }, queue='process-ftp-tasks', compression='zlib' @@ -401,20 +404,20 @@ def test_collate_letter_pdfs_to_be_sent_when_run_after_midnight(notify_api, samp {'ContentLength': 1}, {'ContentLength': 3}, ]) - mocker.patch('app.celery.letters_pdf_tasks.group_letters', return_value=[ - [{'Key': '2020-02-16/A.PDF', 'Size': 1}, {'Key': '2020-02-17/B.PDF', 'Size': 2}], - [{'Key': '2020-02-17/C.PDF', 'Size': 3}] - ]) mock_celery = mocker.patch('app.celery.letters_pdf_tasks.notify_celery.send_task') - collate_letter_pdfs_to_be_sent() + with set_config_values(notify_api, {'MAX_LETTER_PDF_COUNT_PER_ZIP': 2}): + collate_letter_pdfs_to_be_sent() assert len(mock_celery.call_args_list) == 2 assert mock_celery.call_args_list[0] == call( name='zip-and-send-letter-pdfs', kwargs={ - 'filenames_to_zip': ['2020-02-16/A.PDF', '2020-02-17/B.PDF'], - 'upload_filename': 'NOTIFY.2020-02-17.001.pneY1OU2TUq7KfFSrJ2Q.ZIP' + 'filenames_to_zip': [ + '2020-02-16/NOTIFY.REF2.D.2.C.C.20200215180000.PDF', + '2020-02-17/NOTIFY.REF1.D.2.C.C.20200217150000.PDF' + ], + 'upload_filename': 'NOTIFY.2020-02-17.001.k3x_WqC5KhB6e2DWv9Ma.ZIP' }, queue='process-ftp-tasks', compression='zlib' @@ -422,8 +425,10 @@ def test_collate_letter_pdfs_to_be_sent_when_run_after_midnight(notify_api, samp assert mock_celery.call_args_list[1] == call( name='zip-and-send-letter-pdfs', kwargs={ - 'filenames_to_zip': ['2020-02-17/C.PDF'], - 'upload_filename': 'NOTIFY.2020-02-17.002.Wy0jBtrnVzWeGqLXhE_f.ZIP' + 'filenames_to_zip': [ + '2020-02-17/NOTIFY.REF0.D.2.C.C.20200217160000.PDF' + ], + 'upload_filename': 'NOTIFY.2020-02-17.002.J85cUw-FWlKuAIOcwdLS.ZIP' }, queue='process-ftp-tasks', compression='zlib' From 2cd05dea8953e80e7ed7e73bd3e36cee849a3015 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Fri, 21 Feb 2020 16:27:15 +0000 Subject: [PATCH 09/10] Make test DRY --- tests/app/celery/test_letters_pdf_tasks.py | 107 +++++---------------- 1 file changed, 26 insertions(+), 81 deletions(-) diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 96d358dff..297201117 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -315,28 +315,32 @@ def test_get_key_and_size_of_letters_to_be_sent_to_print(notify_api, mocker, sam ] -@freeze_time('2020-02-17 18:00:00') -def test_collate_letter_pdfs_to_be_sent(notify_api, sample_letter_template, mocker): - create_notification( - template=sample_letter_template, - status='created', - reference='ref0', - created_at=(datetime.now() - timedelta(hours=2)).isoformat() - ) +@pytest.mark.parametrize('time_to_run_task', [ + "2020-02-17 18:00:00", # after 5:30pm + "2020-02-18 02:00:00", # the next day after midnight, before 5:30pm we expect the same results +]) +def test_collate_letter_pdfs_to_be_sent(notify_api, sample_letter_template, mocker, time_to_run_task): + with freeze_time("2020-02-17 18:00:00"): + create_notification( + template=sample_letter_template, + status='created', + reference='ref0', + created_at=(datetime.now() - timedelta(hours=2)).isoformat() + ) - create_notification( - template=sample_letter_template, - status='created', - reference='ref1', - created_at=(datetime.now() - timedelta(hours=3)).isoformat() - ) + create_notification( + template=sample_letter_template, + status='created', + reference='ref1', + created_at=(datetime.now() - timedelta(hours=3)).isoformat() + ) - create_notification( - template=sample_letter_template, - status='created', - reference='ref2', - created_at=(datetime.now() - timedelta(days=2)).isoformat() - ) + create_notification( + template=sample_letter_template, + status='created', + reference='ref2', + created_at=(datetime.now() - timedelta(days=2)).isoformat() + ) mocker.patch('app.celery.tasks.s3.head_s3_object', side_effect=[ {'ContentLength': 2}, @@ -347,67 +351,8 @@ def test_collate_letter_pdfs_to_be_sent(notify_api, sample_letter_template, mock mock_celery = mocker.patch('app.celery.letters_pdf_tasks.notify_celery.send_task') with set_config_values(notify_api, {'MAX_LETTER_PDF_COUNT_PER_ZIP': 2}): - collate_letter_pdfs_to_be_sent() - - assert len(mock_celery.call_args_list) == 2 - assert mock_celery.call_args_list[0] == call( - name='zip-and-send-letter-pdfs', - kwargs={ - 'filenames_to_zip': [ - '2020-02-16/NOTIFY.REF2.D.2.C.C.20200215180000.PDF', - '2020-02-17/NOTIFY.REF1.D.2.C.C.20200217150000.PDF' - ], - 'upload_filename': 'NOTIFY.2020-02-17.001.k3x_WqC5KhB6e2DWv9Ma.ZIP' - }, - queue='process-ftp-tasks', - compression='zlib' - ) - assert mock_celery.call_args_list[1] == call( - name='zip-and-send-letter-pdfs', - kwargs={ - 'filenames_to_zip': [ - '2020-02-17/NOTIFY.REF0.D.2.C.C.20200217160000.PDF' - ], - 'upload_filename': 'NOTIFY.2020-02-17.002.J85cUw-FWlKuAIOcwdLS.ZIP' - }, - queue='process-ftp-tasks', - compression='zlib' - ) - - -@freeze_time('2020-02-18 02:00:00') -def test_collate_letter_pdfs_to_be_sent_when_run_after_midnight(notify_api, sample_letter_template, mocker): - # created_at times for notifications choosen to match times in above test test_collate_letter_pdfs_to_be_sent - create_notification( - template=sample_letter_template, - status='created', - reference='ref0', - created_at=(datetime.now() - timedelta(hours=10)).isoformat() - ) - - create_notification( - template=sample_letter_template, - status='created', - reference='ref1', - created_at=(datetime.now() - timedelta(hours=11)).isoformat() - ) - - create_notification( - template=sample_letter_template, - status='created', - reference='ref2', - created_at=(datetime.now() - timedelta(days=2, hours=8)).isoformat() - ) - - mocker.patch('app.celery.tasks.s3.head_s3_object', side_effect=[ - {'ContentLength': 2}, - {'ContentLength': 1}, - {'ContentLength': 3}, - ]) - mock_celery = mocker.patch('app.celery.letters_pdf_tasks.notify_celery.send_task') - - with set_config_values(notify_api, {'MAX_LETTER_PDF_COUNT_PER_ZIP': 2}): - collate_letter_pdfs_to_be_sent() + with freeze_time(time_to_run_task): + collate_letter_pdfs_to_be_sent() assert len(mock_celery.call_args_list) == 2 assert mock_celery.call_args_list[0] == call( From 2c41b21ddf0560b2ca367b2e99bf1f85815229a1 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Fri, 21 Feb 2020 16:42:37 +0000 Subject: [PATCH 10/10] Remove unnecessary code and unrelevant comment --- app/celery/letters_pdf_tasks.py | 2 -- tests/app/celery/test_letters_pdf_tasks.py | 18 +++++++++--------- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index e0685b2c9..b78028596 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -132,8 +132,6 @@ def collate_letter_pdfs_to_be_sent(): if print_run_date.time() < LETTER_PROCESSING_DEADLINE: print_run_date = print_run_date - timedelta(days=1) - # We can truncate the datetime to a date and add our own time (5:30pm) because UTC to BST does not - # make a difference to the date since it is triggered mid afternoon. print_run_deadline = print_run_date.replace( hour=17, minute=30, second=0, microsecond=0 ) diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 297201117..ef95a17b0 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -247,21 +247,21 @@ def test_get_key_and_size_of_letters_to_be_sent_to_print(notify_api, mocker, sam template=sample_letter_template, status='created', reference='ref0', - created_at=(datetime.now() - timedelta(hours=2)).isoformat() + created_at=(datetime.now() - timedelta(hours=2)) ) create_notification( template=sample_letter_template, status='created', reference='ref1', - created_at=(datetime.now() - timedelta(hours=3)).isoformat() + created_at=(datetime.now() - timedelta(hours=3)) ) create_notification( template=sample_letter_template, status='created', reference='ref2', - created_at=(datetime.now() - timedelta(days=2)).isoformat() + created_at=(datetime.now() - timedelta(days=2)) ) # notifications we don't expect to get sent to print as they are in the wrong status @@ -270,7 +270,7 @@ def test_get_key_and_size_of_letters_to_be_sent_to_print(notify_api, mocker, sam template=sample_letter_template, status=status, reference='ref3', - created_at=(datetime.now() - timedelta(days=2)).isoformat() + created_at=(datetime.now() - timedelta(days=2)) ) # notification we don't expect to get sent as instead will make into this evenings print run @@ -278,7 +278,7 @@ def test_get_key_and_size_of_letters_to_be_sent_to_print(notify_api, mocker, sam template=sample_letter_template, status='created', reference='ref4', - created_at=(datetime.now() - timedelta(minutes=1)).isoformat() + created_at=(datetime.now() - timedelta(minutes=1)) ) # test notification we don't expect to get sent @@ -286,7 +286,7 @@ def test_get_key_and_size_of_letters_to_be_sent_to_print(notify_api, mocker, sam template=sample_letter_template, status='created', reference='ref4', - created_at=(datetime.now() - timedelta(days=1)).isoformat(), + created_at=(datetime.now() - timedelta(days=1)), key_type=KEY_TYPE_TEST ) @@ -325,21 +325,21 @@ def test_collate_letter_pdfs_to_be_sent(notify_api, sample_letter_template, mock template=sample_letter_template, status='created', reference='ref0', - created_at=(datetime.now() - timedelta(hours=2)).isoformat() + created_at=(datetime.now() - timedelta(hours=2)) ) create_notification( template=sample_letter_template, status='created', reference='ref1', - created_at=(datetime.now() - timedelta(hours=3)).isoformat() + created_at=(datetime.now() - timedelta(hours=3)) ) create_notification( template=sample_letter_template, status='created', reference='ref2', - created_at=(datetime.now() - timedelta(days=2)).isoformat() + created_at=(datetime.now() - timedelta(days=2)) ) mocker.patch('app.celery.tasks.s3.head_s3_object', side_effect=[