From 9c4e43bfacf9f5437194df2fc79a5f8de60865e7 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 11 Jan 2018 16:37:39 +0000 Subject: [PATCH 1/7] Some pseudo code and notes of how to implement a check for the letter acknowledgement file. --- app/celery/scheduled_tasks.py | 19 +++++++++++++++++++ app/config.py | 5 +++++ .../notifications_letter_callback.py | 1 + 3 files changed, 25 insertions(+) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index 5bac860a6..c6a36aca9 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -447,3 +447,22 @@ def daily_stats_template_usage_by_month(): result.year, result.count ) + + +@notify_celery.task(name='raise-alert-if-no-letter-ack-file') +@statsd(namespace="tasks") +def raise_alert_if_no_letter_ack_file(): + """ + Get all files sent "today" + list_of_zip_files => get file names s3.get_s3_bucket_objects() look in the folder with todays date + + list_of_ack_files => Get all files sent today with name containing ack.txt from a diff bucket + + For filename in list_of_zip_files: + for ack_file in list_of_ack_files if name= file.strip("NOTIFY.", ".ZIP") + IF no ack_file + raise NoAckFileReceived(status=500, message="No ack file received for {filename}") + + """ + + pass \ No newline at end of file diff --git a/app/config.py b/app/config.py index 42ba4177e..25a2cd80a 100644 --- a/app/config.py +++ b/app/config.py @@ -249,6 +249,11 @@ class Config(object): 'schedule': crontab(hour=17, minute=50), 'options': {'queue': QueueNames.PERIODIC} }, + 'raise-alert-if-no-letter-ack-file': { + 'task': 'raise-alert-if-no-letter-ack-file', + 'schedule': crontab(hour=19, minute=00), + 'options': {'queue': QueueNames.PERIODIC} + }, 'run-letter-api-notifications': { 'task': 'run-letter-api-notifications', 'schedule': crontab(hour=17, minute=40), diff --git a/app/notifications/notifications_letter_callback.py b/app/notifications/notifications_letter_callback.py index ac2de9e5c..06d24a40f 100644 --- a/app/notifications/notifications_letter_callback.py +++ b/app/notifications/notifications_letter_callback.py @@ -53,6 +53,7 @@ def process_letter_response(): message = json.loads(req_json['Message']) filename = message['Records'][0]['s3']['object']['key'] current_app.logger.info('Received file from DVLA: {}'.format(filename)) + # IF file name contains .rs.txt THEN continue ELSE log message with file name. and return current_app.logger.info('DVLA callback: Calling task to update letter notifications') update_letter_notifications_statuses.apply_async([filename], queue=QueueNames.NOTIFY) From e1d150a88237388f70e7c8b5db00d168504d4393 Mon Sep 17 00:00:00 2001 From: venusbb Date: Fri, 12 Jan 2018 15:10:42 +0000 Subject: [PATCH 2/7] Added process for dvla acknowledgement file Daily schedule task to check ack file against zip file lists if we haven't receive ack for a zip file, raise a 500 exception --- app/aws/s3.py | 16 +++++ app/celery/scheduled_tasks.py | 36 ++++++---- app/config.py | 2 +- .../notifications_letter_callback.py | 5 +- app/v2/errors.py | 17 +++++ requirements.txt | 2 +- tests/app/celery/test_scheduled_tasks.py | 50 +++++++++++++- tests/app/s3/test_s3.py | 66 +++++++++++++++++++ 8 files changed, 175 insertions(+), 19 deletions(-) create mode 100644 tests/app/s3/test_s3.py diff --git a/app/aws/s3.py b/app/aws/s3.py index 5ffb24068..6771921e7 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -107,3 +107,19 @@ def upload_letters_pdf(reference, crown, filedata): current_app.logger.info("Uploading letters PDF {} to {}".format( upload_file_name, current_app.config['LETTERS_PDF_BUCKET_NAME'])) + + +def get_list_of_files_by_suffix(bucket_name, subfolder='', suffix=''): + s3_client = client('s3', current_app.config['AWS_REGION']) + paginator = s3_client.get_paginator('list_objects_v2') + + page_iterator = paginator.paginate( + Bucket=bucket_name, + Prefix=subfolder + ) + + for page in page_iterator: + for obj in page['Contents']: + key = obj['Key'] + if key.endswith(suffix): + yield key diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index c6a36aca9..ca082657f 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -59,7 +59,7 @@ from app.celery.tasks import ( ) from app.config import QueueNames, TaskNames from app.utils import convert_utc_to_bst -from app.v2.errors import JobIncompleteError +from app.v2.errors import JobIncompleteError, NoAckFileReceived from app.dao.service_callback_api_dao import get_service_callback_api_for_service from app.celery.service_callback_tasks import send_delivery_status_to_service @@ -451,18 +451,30 @@ def daily_stats_template_usage_by_month(): @notify_celery.task(name='raise-alert-if-no-letter-ack-file') @statsd(namespace="tasks") -def raise_alert_if_no_letter_ack_file(): - """ - Get all files sent "today" - list_of_zip_files => get file names s3.get_s3_bucket_objects() look in the folder with todays date +def letter_raise_alert_if_no_ack_file_for_zip(): + # get a list of today's zip files + zip_file_list = [] + for key in s3.get_list_of_files_by_suffix(bucket_name=current_app.config['LETTERS_PDF_BUCKET_NAME'], + subfolder=datetime.utcnow().strftime('%Y-%m-%d'), suffix='.ZIP'): + zip_file_list.append(key) - list_of_ack_files => Get all files sent today with name containing ack.txt from a diff bucket + # get acknowledgement file + ack_file_list = [] + for key in s3.get_list_of_files_by_suffix(bucket_name=current_app.config['DVLA_RESPONSE_BUCKET_NAME'], + subfolder='root/dispatch', suffix='.ACK.txt'): + ack_file_list.append(key) - For filename in list_of_zip_files: - for ack_file in list_of_ack_files if name= file.strip("NOTIFY.", ".ZIP") - IF no ack_file - raise NoAckFileReceived(status=500, message="No ack file received for {filename}") + todaystr = datetime.utcnow().strftime('%Y%m%d') - """ + for key in ack_file_list: + if todaystr in key: + content = s3.get_s3_file(current_app.config['DVLA_RESPONSE_BUCKET_NAME'], key) - pass \ No newline at end of file + for zip_file in content.split('\n'): # each line + s = zip_file.split('|') + for zf in zip_file_list: + if s[0] in zf: + zip_file_list.remove(zf) + + if zip_file_list: + raise NoAckFileReceived(message=zip_file_list) diff --git a/app/config.py b/app/config.py index 25a2cd80a..63beae32b 100644 --- a/app/config.py +++ b/app/config.py @@ -251,7 +251,7 @@ class Config(object): }, 'raise-alert-if-no-letter-ack-file': { 'task': 'raise-alert-if-no-letter-ack-file', - 'schedule': crontab(hour=19, minute=00), + 'schedule': crontab(hour=23, minute=00), 'options': {'queue': QueueNames.PERIODIC} }, 'run-letter-api-notifications': { diff --git a/app/notifications/notifications_letter_callback.py b/app/notifications/notifications_letter_callback.py index 06d24a40f..eac703186 100644 --- a/app/notifications/notifications_letter_callback.py +++ b/app/notifications/notifications_letter_callback.py @@ -53,9 +53,8 @@ def process_letter_response(): message = json.loads(req_json['Message']) filename = message['Records'][0]['s3']['object']['key'] current_app.logger.info('Received file from DVLA: {}'.format(filename)) - # IF file name contains .rs.txt THEN continue ELSE log message with file name. and return - current_app.logger.info('DVLA callback: Calling task to update letter notifications') - update_letter_notifications_statuses.apply_async([filename], queue=QueueNames.NOTIFY) + if 'rs.txt' in filename.lower(): + update_letter_notifications_statuses.apply_async([filename], queue=QueueNames.NOTIFY) return jsonify( result="success", message="DVLA callback succeeded" diff --git a/app/v2/errors.py b/app/v2/errors.py index a70d61d53..bba50b162 100644 --- a/app/v2/errors.py +++ b/app/v2/errors.py @@ -26,6 +26,23 @@ class JobIncompleteError(Exception): } +class NoAckFileReceived(Exception): + def __init__(self, message): + self.message = message + self.status_code = 500 + + def to_dict_v2(self): + return { + 'status_code': self.status_code, + "errors": [ + { + "error": 'NoAckFileReceived', + "message": str(self.message) + } + ] + } + + class TooManyRequestsError(InvalidRequest): status_code = 429 message_template = 'Exceeded send limits ({}) for today' diff --git a/requirements.txt b/requirements.txt index 83f2b5a9e..21a7b9ea1 100644 --- a/requirements.txt +++ b/requirements.txt @@ -25,6 +25,6 @@ notifications-python-client==4.7.1 awscli==1.14.16 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@23.4.0#egg=notifications-utils==23.4.0 +git+https://github.com/alphagov/notifications-utils.git@23.4.1#egg=notifications-utils==23.4.1 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 83f6a6696..5054afd82 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -34,7 +34,8 @@ from app.celery.scheduled_tasks import ( switch_current_sms_provider_on_slow_delivery, timeout_job_statistics, timeout_notifications, - daily_stats_template_usage_by_month + daily_stats_template_usage_by_month, + letter_raise_alert_if_no_ack_file_for_zip ) from app.clients.performance_platform.performance_platform_client import PerformancePlatformClient from app.config import QueueNames, TaskNames @@ -60,7 +61,7 @@ from app.models import ( SMS_TYPE ) from app.utils import get_london_midnight_in_utc -from app.v2.errors import JobIncompleteError +from app.v2.errors import JobIncompleteError, NoAckFileReceived from tests.app.db import create_notification, create_service, create_template, create_job, create_rate from tests.app.conftest import ( @@ -1026,3 +1027,48 @@ def test_dao_fetch_monthly_historical_stats_by_template_null_template_id_not_cou ).all() assert len(result) == 1 + + +def mock_s3_get_list_match(bucket_name, subfolder='', suffix=''): + + if subfolder == '2018-01-11': + return ['NOTIFY.20180111175007.ZIP', 'NOTIFY.20180111175008.ZIP'] + print(suffix) + if subfolder == 'root/dispatch': + return ['root/dispatch/NOTIFY.20180111175733.ACK.txt'] + + +def mock_s3_get_list_diff(bucket_name, subfolder='', suffix=''): + + if subfolder == '2018-01-11': + return ['NOTIFY.20180111175007.ZIP', 'NOTIFY.20180111175008.ZIP', 'NOTIFY.20180111175009.ZIP'] + print(suffix) + if subfolder == 'root/dispatch': + return ['root/dispatch/NOTIFY.20180111175733.ACK.txt'] + + +@freeze_time('2018-01-11T23:00:00') +def test_letter_not_raise_alert_if_ack_files_match_zip_list(mocker, notify_db): + mock_file_list = mocker.patch("app.aws.s3.get_list_of_files_by_suffix", side_effect=mock_s3_get_list_match) + mock_get_file = mocker.patch("app.aws.s3.get_s3_file", + return_value='NOTIFY.20180111175007.ZIP|20180111175733\n' + 'NOTIFY.20180111175008.ZIP|20180111175734') + + letter_raise_alert_if_no_ack_file_for_zip() + + assert mock_file_list.call_count == 2 + assert mock_get_file.call_count == 1 + + +@freeze_time('2018-01-11T23:00:00') +def test_letter_not_raise_alert_if_ack_files_not_match_zip_list(mocker, notify_db): + mock_file_list = mocker.patch("app.aws.s3.get_list_of_files_by_suffix", side_effect=mock_s3_get_list_diff) + mock_get_file = mocker.patch("app.aws.s3.get_s3_file", + return_value='NOTIFY.20180111175007.ZIP|20180111175733\n' + 'NOTIFY.20180111175008.ZIP|20180111175734') + with pytest.raises(expected_exception=NoAckFileReceived) as e: + letter_raise_alert_if_no_ack_file_for_zip() + + assert e.value.message == ['NOTIFY.20180111175009.ZIP'] + assert mock_file_list.call_count == 2 + assert mock_get_file.call_count == 1 diff --git a/tests/app/s3/test_s3.py b/tests/app/s3/test_s3.py new file mode 100644 index 000000000..9f36a2cdc --- /dev/null +++ b/tests/app/s3/test_s3.py @@ -0,0 +1,66 @@ +from app.aws.s3 import get_list_of_files_by_suffix, get_s3_file + + +zip_bucket_name = 'development-letters-pdf' +zip_sub_folder = '2018-01-11' +zip_file_name = '2018-01-11/NOTIFY.20180111175007.ZIP' +ack_bucket_name = 'development-letters-pdf' +ack_subfolder = 'root/dispatch' +ack_file_name = 'root/dispatch/NOTIFY.20180111175733.ACK.txt' + +# Tests for boto3 and s3, can only perform locally against the Tools aws account and have permissions to access S3. +# The tests are based on the above folders and files already uploaded to S3 Tools aws account (If these are removed or +# renamed, the tests won't pass. + + +def test_get_zip_files(): + zip_file_list = [] + for key in get_list_of_files_by_suffix(bucket_name=zip_bucket_name, subfolder=zip_sub_folder, suffix='.ZIP'): + print('File: ' + key) + zip_file_list.append(key) + assert zip_file_name in zip_file_list + + +def test_get_ack_files(): + ack_file_list = [] + for key in get_list_of_files_by_suffix(bucket_name=ack_bucket_name, subfolder=ack_subfolder, suffix='.ACK.txt'): + print('File: ' + key) + ack_file_list.append(key) + assert ack_file_name in ack_file_list + + +def test_get_file_content(): + ack_file_list = [] + for key in get_list_of_files_by_suffix(bucket_name=ack_bucket_name, subfolder=ack_subfolder, suffix='.ACK.txt'): + ack_file_list.append(key) + assert ack_file_name in key + + todaystr = '20180111' + for key in ack_file_list: + if todaystr in key: + content = get_s3_file(ack_bucket_name, key) + print(content) + + +def test_letter_ack_file_strip_correctly(): + # Test ack files are stripped correctly. In the acknowledgement file, there should be 2 zip files, + # 'NOTIFY.20180111175007.ZIP','NOTIFY.20180111175008.ZIP'. + zip_file_list = ['NOTIFY.20180111175007.ZIP', 'NOTIFY.20180111175008.ZIP', 'NOTIFY.20180111175009.ZIP'] + # get acknowledgement file + ack_file_list = [] + for key in get_list_of_files_by_suffix(bucket_name=ack_bucket_name, subfolder=ack_subfolder, suffix='.ACK.txt'): + ack_file_list.append(key) + + for key in ack_file_list: + if '20180111' in key: + content = get_s3_file(ack_bucket_name, key) + print(content) + for zip_file in content.split(): # iterate each line + s = zip_file.split('|') + print(s[0]) + for zf in zip_file_list: + if s[0] in zf: + zip_file_list.remove(zf) + + print('zip_file_list: ' + str(zip_file_list)) + assert zip_file_list == ['NOTIFY.20180111175009.ZIP'] From 24b785e7e0b009768c9b2ae2deeb92f473c4ab90 Mon Sep 17 00:00:00 2001 From: venusbb Date: Fri, 12 Jan 2018 15:10:42 +0000 Subject: [PATCH 3/7] Added process for dvla acknowledgement file Daily schedule task to check ack file against zip file lists if we haven't receive ack for a zip file, raise a 500 exception --- app/aws/s3.py | 16 +++++ app/celery/scheduled_tasks.py | 36 ++++++---- app/config.py | 2 +- .../notifications_letter_callback.py | 1 - app/v2/errors.py | 17 +++++ requirements.txt | 2 +- scripts/test_s3.py | 66 +++++++++++++++++++ tests/app/celery/test_scheduled_tasks.py | 50 +++++++++++++- 8 files changed, 173 insertions(+), 17 deletions(-) create mode 100644 scripts/test_s3.py diff --git a/app/aws/s3.py b/app/aws/s3.py index 5ffb24068..6771921e7 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -107,3 +107,19 @@ def upload_letters_pdf(reference, crown, filedata): current_app.logger.info("Uploading letters PDF {} to {}".format( upload_file_name, current_app.config['LETTERS_PDF_BUCKET_NAME'])) + + +def get_list_of_files_by_suffix(bucket_name, subfolder='', suffix=''): + s3_client = client('s3', current_app.config['AWS_REGION']) + paginator = s3_client.get_paginator('list_objects_v2') + + page_iterator = paginator.paginate( + Bucket=bucket_name, + Prefix=subfolder + ) + + for page in page_iterator: + for obj in page['Contents']: + key = obj['Key'] + if key.endswith(suffix): + yield key diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index c6a36aca9..ca082657f 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -59,7 +59,7 @@ from app.celery.tasks import ( ) from app.config import QueueNames, TaskNames from app.utils import convert_utc_to_bst -from app.v2.errors import JobIncompleteError +from app.v2.errors import JobIncompleteError, NoAckFileReceived from app.dao.service_callback_api_dao import get_service_callback_api_for_service from app.celery.service_callback_tasks import send_delivery_status_to_service @@ -451,18 +451,30 @@ def daily_stats_template_usage_by_month(): @notify_celery.task(name='raise-alert-if-no-letter-ack-file') @statsd(namespace="tasks") -def raise_alert_if_no_letter_ack_file(): - """ - Get all files sent "today" - list_of_zip_files => get file names s3.get_s3_bucket_objects() look in the folder with todays date +def letter_raise_alert_if_no_ack_file_for_zip(): + # get a list of today's zip files + zip_file_list = [] + for key in s3.get_list_of_files_by_suffix(bucket_name=current_app.config['LETTERS_PDF_BUCKET_NAME'], + subfolder=datetime.utcnow().strftime('%Y-%m-%d'), suffix='.ZIP'): + zip_file_list.append(key) - list_of_ack_files => Get all files sent today with name containing ack.txt from a diff bucket + # get acknowledgement file + ack_file_list = [] + for key in s3.get_list_of_files_by_suffix(bucket_name=current_app.config['DVLA_RESPONSE_BUCKET_NAME'], + subfolder='root/dispatch', suffix='.ACK.txt'): + ack_file_list.append(key) - For filename in list_of_zip_files: - for ack_file in list_of_ack_files if name= file.strip("NOTIFY.", ".ZIP") - IF no ack_file - raise NoAckFileReceived(status=500, message="No ack file received for {filename}") + todaystr = datetime.utcnow().strftime('%Y%m%d') - """ + for key in ack_file_list: + if todaystr in key: + content = s3.get_s3_file(current_app.config['DVLA_RESPONSE_BUCKET_NAME'], key) - pass \ No newline at end of file + for zip_file in content.split('\n'): # each line + s = zip_file.split('|') + for zf in zip_file_list: + if s[0] in zf: + zip_file_list.remove(zf) + + if zip_file_list: + raise NoAckFileReceived(message=zip_file_list) diff --git a/app/config.py b/app/config.py index 25a2cd80a..63beae32b 100644 --- a/app/config.py +++ b/app/config.py @@ -251,7 +251,7 @@ class Config(object): }, 'raise-alert-if-no-letter-ack-file': { 'task': 'raise-alert-if-no-letter-ack-file', - 'schedule': crontab(hour=19, minute=00), + 'schedule': crontab(hour=23, minute=00), 'options': {'queue': QueueNames.PERIODIC} }, 'run-letter-api-notifications': { diff --git a/app/notifications/notifications_letter_callback.py b/app/notifications/notifications_letter_callback.py index 06d24a40f..ac2de9e5c 100644 --- a/app/notifications/notifications_letter_callback.py +++ b/app/notifications/notifications_letter_callback.py @@ -53,7 +53,6 @@ def process_letter_response(): message = json.loads(req_json['Message']) filename = message['Records'][0]['s3']['object']['key'] current_app.logger.info('Received file from DVLA: {}'.format(filename)) - # IF file name contains .rs.txt THEN continue ELSE log message with file name. and return current_app.logger.info('DVLA callback: Calling task to update letter notifications') update_letter_notifications_statuses.apply_async([filename], queue=QueueNames.NOTIFY) diff --git a/app/v2/errors.py b/app/v2/errors.py index a70d61d53..bba50b162 100644 --- a/app/v2/errors.py +++ b/app/v2/errors.py @@ -26,6 +26,23 @@ class JobIncompleteError(Exception): } +class NoAckFileReceived(Exception): + def __init__(self, message): + self.message = message + self.status_code = 500 + + def to_dict_v2(self): + return { + 'status_code': self.status_code, + "errors": [ + { + "error": 'NoAckFileReceived', + "message": str(self.message) + } + ] + } + + class TooManyRequestsError(InvalidRequest): status_code = 429 message_template = 'Exceeded send limits ({}) for today' diff --git a/requirements.txt b/requirements.txt index 83f2b5a9e..21a7b9ea1 100644 --- a/requirements.txt +++ b/requirements.txt @@ -25,6 +25,6 @@ notifications-python-client==4.7.1 awscli==1.14.16 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@23.4.0#egg=notifications-utils==23.4.0 +git+https://github.com/alphagov/notifications-utils.git@23.4.1#egg=notifications-utils==23.4.1 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 diff --git a/scripts/test_s3.py b/scripts/test_s3.py new file mode 100644 index 000000000..3809cba24 --- /dev/null +++ b/scripts/test_s3.py @@ -0,0 +1,66 @@ +from app.aws.s3 import get_list_of_files_by_suffix, get_s3_file + + +zip_bucket_name = 'development-letters-pdf' +zip_sub_folder = '2018-01-11' +zip_file_name = '2018-01-11/NOTIFY.20180111175007.ZIP' +ack_bucket_name = 'development-letters-pdf' +ack_subfolder = 'root/dispatch' +ack_file_name = 'root/dispatch/NOTIFY.20180111175733.ACK.txt' + +# Tests for boto3 and s3, can only perform locally against the Tools aws account and have permissions to access S3. +# The tests are based on the above folders and files already uploaded to S3 Tools aws account (If these are removed or +# renamed, the tests won't pass. + + +def test_get_zip_files(): + zip_file_list = [] + for key in get_list_of_files_by_suffix(bucket_name=zip_bucket_name, subfolder=zip_sub_folder, suffix='.ZIP'): + print('File: ' + key) + zip_file_list.append(key) + assert zip_file_name in zip_file_list + + +def test_get_ack_files(): + ack_file_list = [] + for key in get_list_of_files_by_suffix(bucket_name=ack_bucket_name, subfolder=ack_subfolder, suffix='.ACK.txt'): + print('File: ' + key) + ack_file_list.append(key) + assert ack_file_name in ack_file_list + + +def test_get_file_content(): + ack_file_list = [] + for key in get_list_of_files_by_suffix(bucket_name=ack_bucket_name, subfolder=ack_subfolder, suffix='.ACK.txt'): + ack_file_list.append(key) + assert ack_file_name in key + + todaystr = '20180111' + for key in ack_file_list: + if todaystr in key: + content = get_s3_file(ack_bucket_name, key) + print(content) + + +def test_letter_ack_file_strip_correctly(): + # Test ack files are stripped correctly. In the acknowledgement file, there should be 2 zip files, + # 'NOTIFY.20180111175007.ZIP','NOTIFY.20180111175008.ZIP'. + zip_file_list = ['NOTIFY.20180111175007.ZIP', 'NOTIFY.20180111175008.ZIP', 'NOTIFY.20180111175009.ZIP'] + # get acknowledgement file + ack_file_list = [] + for key in get_list_of_files_by_suffix(bucket_name=ack_bucket_name, subfolder=ack_subfolder, suffix='.ACK.txt'): + ack_file_list.append(key) + + for key in ack_file_list: + if '20180111' in key: + content = get_s3_file(ack_bucket_name, key) + print(content) + for zip_file in content.split(): # iterate each line + s = zip_file.split('|') + print(s[0]) + for zf in zip_file_list: + if s[0] in zf: + zip_file_list.remove(zf) + + print('zip_file_list: ' + str(zip_file_list)) + assert zip_file_list == ['NOTIFY.20180111175009.ZIP'] diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 83f6a6696..5054afd82 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -34,7 +34,8 @@ from app.celery.scheduled_tasks import ( switch_current_sms_provider_on_slow_delivery, timeout_job_statistics, timeout_notifications, - daily_stats_template_usage_by_month + daily_stats_template_usage_by_month, + letter_raise_alert_if_no_ack_file_for_zip ) from app.clients.performance_platform.performance_platform_client import PerformancePlatformClient from app.config import QueueNames, TaskNames @@ -60,7 +61,7 @@ from app.models import ( SMS_TYPE ) from app.utils import get_london_midnight_in_utc -from app.v2.errors import JobIncompleteError +from app.v2.errors import JobIncompleteError, NoAckFileReceived from tests.app.db import create_notification, create_service, create_template, create_job, create_rate from tests.app.conftest import ( @@ -1026,3 +1027,48 @@ def test_dao_fetch_monthly_historical_stats_by_template_null_template_id_not_cou ).all() assert len(result) == 1 + + +def mock_s3_get_list_match(bucket_name, subfolder='', suffix=''): + + if subfolder == '2018-01-11': + return ['NOTIFY.20180111175007.ZIP', 'NOTIFY.20180111175008.ZIP'] + print(suffix) + if subfolder == 'root/dispatch': + return ['root/dispatch/NOTIFY.20180111175733.ACK.txt'] + + +def mock_s3_get_list_diff(bucket_name, subfolder='', suffix=''): + + if subfolder == '2018-01-11': + return ['NOTIFY.20180111175007.ZIP', 'NOTIFY.20180111175008.ZIP', 'NOTIFY.20180111175009.ZIP'] + print(suffix) + if subfolder == 'root/dispatch': + return ['root/dispatch/NOTIFY.20180111175733.ACK.txt'] + + +@freeze_time('2018-01-11T23:00:00') +def test_letter_not_raise_alert_if_ack_files_match_zip_list(mocker, notify_db): + mock_file_list = mocker.patch("app.aws.s3.get_list_of_files_by_suffix", side_effect=mock_s3_get_list_match) + mock_get_file = mocker.patch("app.aws.s3.get_s3_file", + return_value='NOTIFY.20180111175007.ZIP|20180111175733\n' + 'NOTIFY.20180111175008.ZIP|20180111175734') + + letter_raise_alert_if_no_ack_file_for_zip() + + assert mock_file_list.call_count == 2 + assert mock_get_file.call_count == 1 + + +@freeze_time('2018-01-11T23:00:00') +def test_letter_not_raise_alert_if_ack_files_not_match_zip_list(mocker, notify_db): + mock_file_list = mocker.patch("app.aws.s3.get_list_of_files_by_suffix", side_effect=mock_s3_get_list_diff) + mock_get_file = mocker.patch("app.aws.s3.get_s3_file", + return_value='NOTIFY.20180111175007.ZIP|20180111175733\n' + 'NOTIFY.20180111175008.ZIP|20180111175734') + with pytest.raises(expected_exception=NoAckFileReceived) as e: + letter_raise_alert_if_no_ack_file_for_zip() + + assert e.value.message == ['NOTIFY.20180111175009.ZIP'] + assert mock_file_list.call_count == 2 + assert mock_get_file.call_count == 1 From 96b3c6c2edf4452a60c79783ef75548789a07b61 Mon Sep 17 00:00:00 2001 From: venusbb Date: Fri, 12 Jan 2018 19:24:36 +0000 Subject: [PATCH 4/7] Add tests for checking not updating database when ack file is received. Moved S3 test scripts Modified scheduled_task test --- .../test_scripts_for_s3.py | 2 +- tests/app/celery/test_scheduled_tasks.py | 6 +++--- tests/app/notifications/rest/test_callbacks.py | 12 +++++++----- 3 files changed, 11 insertions(+), 9 deletions(-) rename scripts/test_s3.py => test_scripts/test_scripts_for_s3.py (97%) diff --git a/scripts/test_s3.py b/test_scripts/test_scripts_for_s3.py similarity index 97% rename from scripts/test_s3.py rename to test_scripts/test_scripts_for_s3.py index 3809cba24..4510c4e6d 100644 --- a/scripts/test_s3.py +++ b/test_scripts/test_scripts_for_s3.py @@ -42,7 +42,7 @@ def test_get_file_content(): print(content) -def test_letter_ack_file_strip_correctly(): +def test_letter_ack_file_parse_content_correctly(): # Test ack files are stripped correctly. In the acknowledgement file, there should be 2 zip files, # 'NOTIFY.20180111175007.ZIP','NOTIFY.20180111175008.ZIP'. zip_file_list = ['NOTIFY.20180111175007.ZIP', 'NOTIFY.20180111175008.ZIP', 'NOTIFY.20180111175009.ZIP'] diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 5054afd82..2dfef2623 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -1039,9 +1039,9 @@ def mock_s3_get_list_match(bucket_name, subfolder='', suffix=''): def mock_s3_get_list_diff(bucket_name, subfolder='', suffix=''): - if subfolder == '2018-01-11': - return ['NOTIFY.20180111175007.ZIP', 'NOTIFY.20180111175008.ZIP', 'NOTIFY.20180111175009.ZIP'] + return ['NOTIFY.20180111175007.ZIP', 'NOTIFY.20180111175008.ZIP', 'NOTIFY.20180111175009.ZIP', + 'NOTIFY.20180111175010.ZIP'] print(suffix) if subfolder == 'root/dispatch': return ['root/dispatch/NOTIFY.20180111175733.ACK.txt'] @@ -1069,6 +1069,6 @@ def test_letter_not_raise_alert_if_ack_files_not_match_zip_list(mocker, notify_d with pytest.raises(expected_exception=NoAckFileReceived) as e: letter_raise_alert_if_no_ack_file_for_zip() - assert e.value.message == ['NOTIFY.20180111175009.ZIP'] + assert e.value.message == ['NOTIFY.20180111175009.ZIP', 'NOTIFY.20180111175010.ZIP'] assert mock_file_list.call_count == 2 assert mock_get_file.call_count == 1 diff --git a/tests/app/notifications/rest/test_callbacks.py b/tests/app/notifications/rest/test_callbacks.py index af51f7719..e81a91042 100644 --- a/tests/app/notifications/rest/test_callbacks.py +++ b/tests/app/notifications/rest/test_callbacks.py @@ -80,12 +80,14 @@ def test_dvla_callback_calls_update_letter_notifications_task(client, mocker): update_task.assert_called_with(['bar.rs.txt'], queue='notify-internal-tasks') -def test_dvla_callback_does_not_raise_error_parsing_json_for_plaintext_header(client, mocker): - mocker.patch('app.notifications.notifications_letter_callback.update_letter_notifications_statuses.apply_async') - data = _sample_sns_s3_callback() +def test_dvla_ack_calls_does_not_call_letter_notifications_task(client, mocker): + update_task = \ + mocker.patch('app.notifications.notifications_letter_callback.update_letter_notifications_statuses.apply_async') + data = _sample_sns_s3_dvla_ack() response = dvla_post(client, data) assert response.status_code == 200 + update_task.assert_not_called() def test_firetext_callback_should_not_need_auth(client, mocker): @@ -460,7 +462,7 @@ def test_firetext_callback_should_record_statsd(client, notify_db, notify_db_ses app.statsd_client.incr.assert_any_call("callback.firetext.delivered") -def _sample_sns_s3_callback(): +def _sample_sns_s3_dvla_ack(): return json.dumps({ "SigningCertURL": "foo.pem", "UnsubscribeURL": "bar", @@ -471,7 +473,7 @@ def _sample_sns_s3_callback(): "MessageId": "6adbfe0a-d610-509a-9c47-af894e90d32d", "Subject": "Amazon S3 Notification", "TopicArn": "sample-topic-arn", - "Message": '{"Records":[{"eventVersion":"2.0","eventSource":"aws:s3","awsRegion":"eu-west-1","eventTime":"2017-05-16T11:38:41.073Z","eventName":"ObjectCreated:Put","userIdentity":{"principalId":"some-p-id"},"requestParameters":{"sourceIPAddress":"8.8.8.8"},"responseElements":{"x-amz-request-id":"some-r-id","x-amz-id-2":"some-x-am-id"},"s3":{"s3SchemaVersion":"1.0","configurationId":"some-c-id","bucket":{"name":"some-bucket","ownerIdentity":{"principalId":"some-p-id"},"arn":"some-bucket-arn"},"object":{"key":"bar.txt","size":200,"eTag":"some-e-tag","versionId":"some-v-id","sequencer":"some-seq"}}}]}' # noqa + "Message": '{"Records":[{"eventVersion":"2.0","eventSource":"aws:s3","awsRegion":"eu-west-1","eventTime":"2017-05-16T11:38:41.073Z","eventName":"ObjectCreated:Put","userIdentity":{"principalId":"some-p-id"},"requestParameters":{"sourceIPAddress":"8.8.8.8"},"responseElements":{"x-amz-request-id":"some-r-id","x-amz-id-2":"some-x-am-id"},"s3":{"s3SchemaVersion":"1.0","configurationId":"some-c-id","bucket":{"name":"some-bucket","ownerIdentity":{"principalId":"some-p-id"},"arn":"some-bucket-arn"},"object":{"key":"bar.ack.txt","size":200,"eTag":"some-e-tag","versionId":"some-v-id","sequencer":"some-seq"}}}]}' # noqa }) From 6a13450bfc51821c1eede425778d2930249f1f67 Mon Sep 17 00:00:00 2001 From: venusbb Date: Mon, 15 Jan 2018 12:30:46 +0000 Subject: [PATCH 5/7] remove s3 testscript from this repo This file does not belong to this repo, although it is useful in initially testing the s3 boto3 scipts. Will consider to move it somewhere else. --- test_scripts/test_scripts_for_s3.py | 66 ----------------------------- 1 file changed, 66 deletions(-) delete mode 100644 test_scripts/test_scripts_for_s3.py diff --git a/test_scripts/test_scripts_for_s3.py b/test_scripts/test_scripts_for_s3.py deleted file mode 100644 index 4510c4e6d..000000000 --- a/test_scripts/test_scripts_for_s3.py +++ /dev/null @@ -1,66 +0,0 @@ -from app.aws.s3 import get_list_of_files_by_suffix, get_s3_file - - -zip_bucket_name = 'development-letters-pdf' -zip_sub_folder = '2018-01-11' -zip_file_name = '2018-01-11/NOTIFY.20180111175007.ZIP' -ack_bucket_name = 'development-letters-pdf' -ack_subfolder = 'root/dispatch' -ack_file_name = 'root/dispatch/NOTIFY.20180111175733.ACK.txt' - -# Tests for boto3 and s3, can only perform locally against the Tools aws account and have permissions to access S3. -# The tests are based on the above folders and files already uploaded to S3 Tools aws account (If these are removed or -# renamed, the tests won't pass. - - -def test_get_zip_files(): - zip_file_list = [] - for key in get_list_of_files_by_suffix(bucket_name=zip_bucket_name, subfolder=zip_sub_folder, suffix='.ZIP'): - print('File: ' + key) - zip_file_list.append(key) - assert zip_file_name in zip_file_list - - -def test_get_ack_files(): - ack_file_list = [] - for key in get_list_of_files_by_suffix(bucket_name=ack_bucket_name, subfolder=ack_subfolder, suffix='.ACK.txt'): - print('File: ' + key) - ack_file_list.append(key) - assert ack_file_name in ack_file_list - - -def test_get_file_content(): - ack_file_list = [] - for key in get_list_of_files_by_suffix(bucket_name=ack_bucket_name, subfolder=ack_subfolder, suffix='.ACK.txt'): - ack_file_list.append(key) - assert ack_file_name in key - - todaystr = '20180111' - for key in ack_file_list: - if todaystr in key: - content = get_s3_file(ack_bucket_name, key) - print(content) - - -def test_letter_ack_file_parse_content_correctly(): - # Test ack files are stripped correctly. In the acknowledgement file, there should be 2 zip files, - # 'NOTIFY.20180111175007.ZIP','NOTIFY.20180111175008.ZIP'. - zip_file_list = ['NOTIFY.20180111175007.ZIP', 'NOTIFY.20180111175008.ZIP', 'NOTIFY.20180111175009.ZIP'] - # get acknowledgement file - ack_file_list = [] - for key in get_list_of_files_by_suffix(bucket_name=ack_bucket_name, subfolder=ack_subfolder, suffix='.ACK.txt'): - ack_file_list.append(key) - - for key in ack_file_list: - if '20180111' in key: - content = get_s3_file(ack_bucket_name, key) - print(content) - for zip_file in content.split(): # iterate each line - s = zip_file.split('|') - print(s[0]) - for zf in zip_file_list: - if s[0] in zf: - zip_file_list.remove(zf) - - print('zip_file_list: ' + str(zip_file_list)) - assert zip_file_list == ['NOTIFY.20180111175009.ZIP'] From f273b23c25ab28c22eacc41997c283c9bbaa24e6 Mon Sep 17 00:00:00 2001 From: venusbb Date: Tue, 16 Jan 2018 09:29:31 +0000 Subject: [PATCH 6/7] Get ack files only from day before the ack file is received. Take care of upper and lower case of file names and contents Add a test for s3 get_list_of_files_by_suffix --- app/aws/s3.py | 9 +++--- app/celery/scheduled_tasks.py | 25 ++++++++++----- tests/app/aws/test_s3.py | 39 ++++++++++++++++++++++-- tests/app/celery/test_scheduled_tasks.py | 6 ++-- 4 files changed, 62 insertions(+), 17 deletions(-) diff --git a/app/aws/s3.py b/app/aws/s3.py index 6771921e7..1b8d436d3 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -109,7 +109,7 @@ def upload_letters_pdf(reference, crown, filedata): upload_file_name, current_app.config['LETTERS_PDF_BUCKET_NAME'])) -def get_list_of_files_by_suffix(bucket_name, subfolder='', suffix=''): +def get_list_of_files_by_suffix(bucket_name, subfolder='', suffix='', last_modified=None): s3_client = client('s3', current_app.config['AWS_REGION']) paginator = s3_client.get_paginator('list_objects_v2') @@ -120,6 +120,7 @@ def get_list_of_files_by_suffix(bucket_name, subfolder='', suffix=''): for page in page_iterator: for obj in page['Contents']: - key = obj['Key'] - if key.endswith(suffix): - yield key + key = obj['Key'].lower() + if key.endswith(suffix.lower()): + if not last_modified or obj['LastModified'] >= last_modified: + yield key diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index ca082657f..6f840ad6b 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -1,3 +1,4 @@ +import pytz from datetime import ( date, datetime, @@ -452,29 +453,39 @@ def daily_stats_template_usage_by_month(): @notify_celery.task(name='raise-alert-if-no-letter-ack-file') @statsd(namespace="tasks") def letter_raise_alert_if_no_ack_file_for_zip(): - # get a list of today's zip files + # get a list of zip files since yesterday zip_file_list = [] + for key in s3.get_list_of_files_by_suffix(bucket_name=current_app.config['LETTERS_PDF_BUCKET_NAME'], - subfolder=datetime.utcnow().strftime('%Y-%m-%d'), suffix='.ZIP'): + subfolder=datetime.utcnow().strftime('%Y-%m-%d'), + suffix='.zip'): zip_file_list.append(key) # get acknowledgement file ack_file_list = [] + yesterday = datetime.now(tz=pytz.utc) - timedelta(days=1) for key in s3.get_list_of_files_by_suffix(bucket_name=current_app.config['DVLA_RESPONSE_BUCKET_NAME'], - subfolder='root/dispatch', suffix='.ACK.txt'): + subfolder='root/dispatch', suffix='.ACK.txt', lastModified=yesterday): ack_file_list.append(key) - todaystr = datetime.utcnow().strftime('%Y%m%d') + today_str = datetime.utcnow().strftime('%Y%m%d') + zip_not_today = [] for key in ack_file_list: - if todaystr in key: + if today_str in key: content = s3.get_s3_file(current_app.config['DVLA_RESPONSE_BUCKET_NAME'], key) - for zip_file in content.split('\n'): # each line s = zip_file.split('|') for zf in zip_file_list: - if s[0] in zf: + if s[0].lower() in zf.lower(): zip_file_list.remove(zf) + else: + zip_not_today.append(s[0]) if zip_file_list: raise NoAckFileReceived(message=zip_file_list) + + if zip_not_today: + current_app.logger.info( + "letter ack contains zip that is not for today {} ".format(zip_not_today) + ) diff --git a/tests/app/aws/test_s3.py b/tests/app/aws/test_s3.py index 23f890b9f..d4d3f196c 100644 --- a/tests/app/aws/test_s3.py +++ b/tests/app/aws/test_s3.py @@ -1,7 +1,7 @@ from unittest.mock import call from datetime import datetime, timedelta import pytest - +import pytz from flask import current_app from freezegun import freeze_time @@ -11,7 +11,8 @@ from app.aws.s3 import ( get_s3_file, filter_s3_bucket_objects_within_date_range, remove_transformed_dvla_file, - upload_letters_pdf + upload_letters_pdf, + get_list_of_files_by_suffix, ) from tests.app.conftest import datetime_in_past @@ -173,3 +174,37 @@ def test_upload_letters_pdf_puts_in_tomorrows_bucket_after_half_five(notify_api, # in tomorrow's folder, but still has this evening's timestamp file_location='2017-12-05/NOTIFY.FOO.D.2.C.C.20171204173100.PDF' ) + + +@freeze_time("2018-01-11 00:00:00") +@pytest.mark.parametrize('suffix_str, days_before, returned_no', [ + ('.ACK.txt', None, 1), + ('.ack.txt', None, 1), + ('.ACK.TXT', None, 1), + ('', None, 2), + ('', 1, 1), +]) +def test_get_list_of_files_by_suffix(notify_api, mocker, suffix_str, days_before, returned_no): + paginator_mock = mocker.patch('app.aws.s3.client') + multiple_pages_s3_object = [ + { + "Contents": [ + single_s3_object_stub('bar/foo.ACK.txt', datetime_in_past(1, 0)), + ] + }, + { + "Contents": [ + single_s3_object_stub('bar/foo1.rs.txt', datetime_in_past(2, 0)), + ] + } + ] + paginator_mock.return_value.get_paginator.return_value.paginate.return_value = multiple_pages_s3_object + if (days_before): + key = get_list_of_files_by_suffix('foo-bucket', subfolder='bar', suffix=suffix_str, + last_modified=datetime.now(tz=pytz.utc) - timedelta(days=days_before)) + else: + key = get_list_of_files_by_suffix('foo-bucket', subfolder='bar', suffix=suffix_str) + + assert sum(1 for x in key) == returned_no + for k in key: + assert k == 'bar/foo.ACK.txt' diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 2dfef2623..e600f1748 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -1029,20 +1029,18 @@ def test_dao_fetch_monthly_historical_stats_by_template_null_template_id_not_cou assert len(result) == 1 -def mock_s3_get_list_match(bucket_name, subfolder='', suffix=''): +def mock_s3_get_list_match(bucket_name, subfolder='', suffix='', lastModified=None): if subfolder == '2018-01-11': return ['NOTIFY.20180111175007.ZIP', 'NOTIFY.20180111175008.ZIP'] - print(suffix) if subfolder == 'root/dispatch': return ['root/dispatch/NOTIFY.20180111175733.ACK.txt'] -def mock_s3_get_list_diff(bucket_name, subfolder='', suffix=''): +def mock_s3_get_list_diff(bucket_name, subfolder='', suffix='', lastModified=None): if subfolder == '2018-01-11': return ['NOTIFY.20180111175007.ZIP', 'NOTIFY.20180111175008.ZIP', 'NOTIFY.20180111175009.ZIP', 'NOTIFY.20180111175010.ZIP'] - print(suffix) if subfolder == 'root/dispatch': return ['root/dispatch/NOTIFY.20180111175733.ACK.txt'] From cd2e98c388a2afb538c4b5359ede3aaf47b500d8 Mon Sep 17 00:00:00 2001 From: venusbb Date: Tue, 16 Jan 2018 16:06:08 +0000 Subject: [PATCH 7/7] Change datetime to use utc --- app/celery/scheduled_tasks.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index 6f840ad6b..0562e4baa 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -1,4 +1,3 @@ -import pytz from datetime import ( date, datetime, @@ -463,7 +462,8 @@ def letter_raise_alert_if_no_ack_file_for_zip(): # get acknowledgement file ack_file_list = [] - yesterday = datetime.now(tz=pytz.utc) - timedelta(days=1) + # yesterday = datetime.now(tz=pytz.utc) - timedelta(days=1) + yesterday = datetime.utcnow() - timedelta(days=1) for key in s3.get_list_of_files_by_suffix(bucket_name=current_app.config['DVLA_RESPONSE_BUCKET_NAME'], subfolder='root/dispatch', suffix='.ACK.txt', lastModified=yesterday): ack_file_list.append(key) @@ -483,6 +483,7 @@ def letter_raise_alert_if_no_ack_file_for_zip(): zip_not_today.append(s[0]) if zip_file_list: + raise NoAckFileReceived(message=zip_file_list) if zip_not_today: