diff --git a/app/aws/s3.py b/app/aws/s3.py index 8020c18a7..aa1bbed97 100644 --- a/app/aws/s3.py +++ b/app/aws/s3.py @@ -5,10 +5,7 @@ from flask import current_app import pytz from boto3 import client, resource -from notifications_utils.s3 import s3upload as utils_s3upload - FILE_LOCATION_STRUCTURE = 'service-{}-notify/{}.csv' -LETTERS_PDF_FILE_LOCATION_STRUCTURE = '{folder}/NOTIFY.{reference}.{duplex}.{letter_class}.{colour}.{crown}.{date}.pdf' def get_s3_file(bucket_name, file_location): @@ -81,34 +78,6 @@ def remove_transformed_dvla_file(job_id): return obj.delete() -def upload_letters_pdf(reference, crown, filedata): - now = datetime.utcnow() - - print_datetime = now - if now.time() > current_app.config.get('LETTER_PROCESSING_DEADLINE'): - print_datetime = now + timedelta(days=1) - - upload_file_name = LETTERS_PDF_FILE_LOCATION_STRUCTURE.format( - folder=print_datetime.date(), - reference=reference, - duplex="D", - letter_class="2", - colour="C", - crown="C" if crown else "N", - date=now.strftime('%Y%m%d%H%M%S') - ).upper() - - utils_s3upload( - filedata=filedata, - region=current_app.config['AWS_REGION'], - bucket_name=current_app.config['LETTERS_PDF_BUCKET_NAME'], - file_location=upload_file_name - ) - - 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='', last_modified=None): s3_client = client('s3', current_app.config['AWS_REGION']) paginator = s3_client.get_paginator('list_objects_v2') diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 8de14f28b..0404a3a4c 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -1,3 +1,4 @@ +from datetime import datetime, timedelta import math from flask import current_app @@ -7,6 +8,8 @@ from requests import ( ) from botocore.exceptions import ClientError as BotoClientError +from notifications_utils.s3 import s3upload + from app import notify_celery from app.aws import s3 from app.config import QueueNames, TaskNames @@ -19,6 +22,29 @@ from app.dao.notifications_dao import ( from app.models import NOTIFICATION_CREATED from app.statsd_decorators import statsd +LETTERS_PDF_FILE_LOCATION_STRUCTURE = \ + '{folder}/NOTIFY.{reference}.{duplex}.{letter_class}.{colour}.{crown}.{date}.pdf' + + +def get_letter_pdf_filename(reference, crown): + now = datetime.utcnow() + + print_datetime = now + if now.time() > current_app.config.get('LETTER_PROCESSING_DEADLINE'): + print_datetime = now + timedelta(days=1) + + upload_file_name = LETTERS_PDF_FILE_LOCATION_STRUCTURE.format( + folder=print_datetime.date(), + reference=reference, + duplex="D", + letter_class="2", + colour="C", + crown="C" if crown else "N", + date=now.strftime('%Y%m%d%H%M%S') + ).upper() + + return upload_file_name + @notify_celery.task(bind=True, name="create-letters-pdf", max_retries=15, default_retry_delay=300) @statsd(namespace="tasks") @@ -34,7 +60,19 @@ def create_letters_pdf(self, notification_id): ) current_app.logger.info("PDF Letter {} reference {} created at {}, {} bytes".format( notification.id, notification.reference, notification.created_at, len(pdf_data))) - s3.upload_letters_pdf(reference=notification.reference, crown=notification.service.crown, filedata=pdf_data) + + upload_file_name = get_letter_pdf_filename( + notification.reference, notification.service.crown) + + s3upload( + filedata=pdf_data, + region=current_app.config['AWS_REGION'], + bucket_name=current_app.config['LETTERS_PDF_BUCKET_NAME'], + file_location=upload_file_name + ) + + current_app.logger.info("Uploaded letters PDF {} to {}".format( + upload_file_name, current_app.config['LETTERS_PDF_BUCKET_NAME'])) notification.billable_units = billable_units dao_update_notification(notification) diff --git a/tests/app/aws/test_s3.py b/tests/app/aws/test_s3.py index 81ee10bfd..f122ebdc5 100644 --- a/tests/app/aws/test_s3.py +++ b/tests/app/aws/test_s3.py @@ -11,7 +11,6 @@ from app.aws.s3 import ( get_s3_file, filter_s3_bucket_objects_within_date_range, remove_transformed_dvla_file, - upload_letters_pdf, get_list_of_files_by_suffix, ) from tests.app.conftest import datetime_in_past @@ -144,38 +143,6 @@ def test_get_s3_bucket_objects_does_not_return_outside_of_date_range(notify_api, assert len(filtered_items) == 0 -@pytest.mark.parametrize('crown_flag,expected_crown_text', [ - (True, 'C'), - (False, 'N'), -]) -@freeze_time("2017-12-04 17:29:00") -def test_upload_letters_pdf_calls_utils_s3upload_with_correct_args( - notify_api, mocker, crown_flag, expected_crown_text): - s3_upload_mock = mocker.patch('app.aws.s3.utils_s3upload') - upload_letters_pdf(reference='foo', crown=crown_flag, filedata='some_data') - - s3_upload_mock.assert_called_with( - filedata='some_data', - region='eu-west-1', - bucket_name='test-letters-pdf', - file_location='2017-12-04/NOTIFY.FOO.D.2.C.{}.20171204172900.PDF'.format(expected_crown_text) - ) - - -@freeze_time("2017-12-04 17:31:00") -def test_upload_letters_pdf_puts_in_tomorrows_bucket_after_half_five(notify_api, mocker): - s3_upload_mock = mocker.patch('app.aws.s3.utils_s3upload') - upload_letters_pdf(reference='foo', crown=True, filedata='some_data') - - s3_upload_mock.assert_called_with( - filedata='some_data', - region='eu-west-1', - bucket_name='test-letters-pdf', - # 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), diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 95942712c..7d58d278c 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -1,5 +1,8 @@ +from flask import current_app + from unittest.mock import call +from freezegun import freeze_time import pytest import requests_mock from botocore.exceptions import ClientError @@ -13,6 +16,7 @@ from app.celery.letters_pdf_tasks import ( collate_letter_pdfs_for_day, group_letters, letter_in_created_state, + get_letter_pdf_filename, ) from app.models import Notification, NOTIFICATION_SENDING @@ -23,6 +27,25 @@ def test_should_have_decorated_tasks_functions(): assert create_letters_pdf.__wrapped__.__name__ == 'create_letters_pdf' +@pytest.mark.parametrize('crown_flag,expected_crown_text', [ + (True, 'C'), + (False, 'N'), +]) +@freeze_time("2017-12-04 17:29:00") +def test_get_letter_pdf_filename_returns_correct_filename( + notify_api, mocker, crown_flag, expected_crown_text): + filename = get_letter_pdf_filename(reference='foo', crown=crown_flag) + + assert filename == '2017-12-04/NOTIFY.FOO.D.2.C.{}.20171204172900.PDF'.format(expected_crown_text) + + +@freeze_time("2017-12-04 17:31:00") +def test_get_letter_pdf_filename_returns_tomorrows_filename(notify_api, mocker): + filename = get_letter_pdf_filename(reference='foo', crown=True) + + assert filename == '2017-12-05/NOTIFY.FOO.D.2.C.C.20171204173100.PDF' + + @pytest.mark.parametrize('personalisation', [{'name': 'test'}, None]) def test_get_letters_pdf_calls_notifications_template_preview_service_correctly( notify_api, mocker, client, sample_letter_template, personalisation): @@ -79,22 +102,28 @@ def test_get_letters_pdf_calculates_billing_units( assert billable_units == expected_billable_units -def test_create_letters_pdf_calls_upload_letters_pdf(mocker, sample_letter_notification): +def test_create_letters_pdf_calls_s3upload(mocker, sample_letter_notification): mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', return_value=(b'\x00\x01', '1')) - mock_s3 = mocker.patch('app.celery.tasks.s3.upload_letters_pdf') + mock_s3 = mocker.patch('app.celery.letters_pdf_tasks.s3upload') create_letters_pdf(sample_letter_notification.id) - mock_s3.assert_called_with( + filename = get_letter_pdf_filename( reference=sample_letter_notification.reference, - crown=sample_letter_notification.service.crown, - filedata=b'\x00\x01' + crown=sample_letter_notification.service.crown + ) + + mock_s3.assert_called_with( + bucket_name=current_app.config['LETTERS_PDF_BUCKET_NAME'], + file_location=filename, + filedata=b'\x00\x01', + region=current_app.config['AWS_REGION'] ) def test_create_letters_pdf_sets_billable_units(mocker, sample_letter_notification): mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', return_value=(b'\x00\x01', 1)) - mocker.patch('app.celery.tasks.s3.upload_letters_pdf') + mocker.patch('app.celery.letters_pdf_tasks.s3upload') create_letters_pdf(sample_letter_notification.id) noti = Notification.query.filter(Notification.reference == sample_letter_notification.reference).one() @@ -118,7 +147,7 @@ def test_create_letters_pdf_handles_request_errors(mocker, sample_letter_notific def test_create_letters_pdf_handles_s3_errors(mocker, sample_letter_notification): mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', return_value=(b'\x00\x01', 1)) - mock_s3 = mocker.patch('app.celery.tasks.s3.upload_letters_pdf', side_effect=ClientError({}, 'operation_name')) + mock_s3 = mocker.patch('app.celery.letters_pdf_tasks.s3upload', side_effect=ClientError({}, 'operation_name')) mock_retry = mocker.patch('app.celery.letters_pdf_tasks.create_letters_pdf.retry') create_letters_pdf(sample_letter_notification.id) @@ -137,7 +166,6 @@ def test_create_letters_pdf_sets_technical_failure_max_retries(mocker, sample_le assert mock_get_letters_pdf.called assert mock_retry.called - assert mock_update_noti.called mock_update_noti.assert_called_once_with(sample_letter_notification.id, 'technical-failure')