Refactor code to move business logic

- DVLA specific logic is now moved to letters_pdf_tasks

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')

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)

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),

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')