rename letter get_folder_name args

`_now`? why would we ever use a different _now? instead say created_at,
because that's what it'll always be set to, even if we're replaying old
letters. We always set the folder name to when the letter was
created_at, or we might not know where to look to find it.

`dont_use_sending_date` doesn't really tell us what might happen if we
don't use it - the answer is we return an empty string. we ignore the
folder entirely. so lets call it that.

Also, remove use of freeze_gun in the tests, to prove that we don't use
the current time in any calculations. Also add an assert to a mock in
the get_pdf_for_templated_letter test, because we were mocking but not
asserting before, so the tests didn't fail when the function signature
changed.
This commit is contained in:
Leo Hemsted
2020-09-21 13:46:31 +01:00
parent 120b760cae
commit bb33927b3d
6 changed files with 33 additions and 23 deletions

View File

@@ -60,12 +60,11 @@ def create_letters_pdf(self, notification_id):
def get_pdf_for_templated_letter(self, notification_id): def get_pdf_for_templated_letter(self, notification_id):
try: try:
notification = get_notification_by_id(notification_id, _raise=True) notification = get_notification_by_id(notification_id, _raise=True)
letter_filename = get_letter_pdf_filename( letter_filename = get_letter_pdf_filename(
reference=notification.reference, reference=notification.reference,
crown=notification.service.crown, crown=notification.service.crown,
sending_date=notification.created_at, sending_date=notification.created_at,
dont_use_sending_date=notification.key_type == KEY_TYPE_TEST, ignore_folder=notification.key_type == KEY_TYPE_TEST,
postage=notification.postage postage=notification.postage
) )
letter_data = { letter_data = {
@@ -313,7 +312,7 @@ def process_sanitised_letter(self, sanitise_data):
reference=notification.reference, reference=notification.reference,
crown=notification.service.crown, crown=notification.service.crown,
sending_date=notification.created_at, sending_date=notification.created_at,
dont_use_sending_date=True, ignore_folder=True,
postage=notification.postage postage=notification.postage
) )

View File

@@ -433,7 +433,7 @@ def _delete_letters_from_s3(
prefix = get_letter_pdf_filename(reference=letter.reference, prefix = get_letter_pdf_filename(reference=letter.reference,
crown=letter.service.crown, crown=letter.service.crown,
sending_date=letter.created_at, sending_date=letter.created_at,
dont_use_sending_date=letter.key_type == KEY_TYPE_TEST, ignore_folder=letter.key_type == KEY_TYPE_TEST,
postage=letter.postage) postage=letter.postage)
s3_objects = get_s3_bucket_objects(bucket_name=bucket_name, subfolder=prefix) s3_objects = get_s3_bucket_objects(bucket_name=bucket_name, subfolder=prefix)
for s3_object in s3_objects: for s3_object in s3_objects:

View File

@@ -27,20 +27,20 @@ LETTERS_PDF_FILE_LOCATION_STRUCTURE = \
PRECOMPILED_BUCKET_PREFIX = '{folder}NOTIFY.{reference}' PRECOMPILED_BUCKET_PREFIX = '{folder}NOTIFY.{reference}'
def get_folder_name(_now, *, dont_use_sending_date=False): def get_folder_name(created_at, *, ignore_folder=False):
if dont_use_sending_date: if ignore_folder:
folder_name = '' folder_name = ''
else: else:
print_datetime = convert_utc_to_bst(_now) print_datetime = convert_utc_to_bst(created_at)
if print_datetime.time() > LETTER_PROCESSING_DEADLINE: if print_datetime.time() > LETTER_PROCESSING_DEADLINE:
print_datetime += timedelta(days=1) print_datetime += timedelta(days=1)
folder_name = '{}/'.format(print_datetime.date()) folder_name = '{}/'.format(print_datetime.date())
return folder_name return folder_name
def get_letter_pdf_filename(reference, crown, sending_date, dont_use_sending_date=False, postage=SECOND_CLASS): def get_letter_pdf_filename(reference, crown, sending_date, ignore_folder=False, postage=SECOND_CLASS):
upload_file_name = LETTERS_PDF_FILE_LOCATION_STRUCTURE.format( upload_file_name = LETTERS_PDF_FILE_LOCATION_STRUCTURE.format(
folder=get_folder_name(sending_date, dont_use_sending_date=dont_use_sending_date), folder=get_folder_name(sending_date, ignore_folder=ignore_folder),
reference=reference, reference=reference,
duplex="D", duplex="D",
letter_class=RESOLVE_POSTAGE_FOR_FILE_NAME[postage], letter_class=RESOLVE_POSTAGE_FOR_FILE_NAME[postage],
@@ -59,7 +59,7 @@ def get_bucket_name_and_prefix_for_notification(notification):
bucket_name = current_app.config['TEST_LETTERS_BUCKET_NAME'] bucket_name = current_app.config['TEST_LETTERS_BUCKET_NAME']
else: else:
bucket_name = current_app.config['LETTERS_PDF_BUCKET_NAME'] bucket_name = current_app.config['LETTERS_PDF_BUCKET_NAME']
folder = get_folder_name(notification.created_at, dont_use_sending_date=False) folder = get_folder_name(notification.created_at, ignore_folder=False)
upload_file_name = PRECOMPILED_BUCKET_PREFIX.format( upload_file_name = PRECOMPILED_BUCKET_PREFIX.format(
folder=folder, folder=folder,
@@ -83,7 +83,7 @@ def upload_letter_pdf(notification, pdf_data, precompiled=False):
reference=notification.reference, reference=notification.reference,
crown=notification.service.crown, crown=notification.service.crown,
sending_date=notification.created_at, sending_date=notification.created_at,
dont_use_sending_date=precompiled or notification.key_type == KEY_TYPE_TEST, ignore_folder=precompiled or notification.key_type == KEY_TYPE_TEST,
postage=notification.postage postage=notification.postage
) )
@@ -151,7 +151,7 @@ def move_uploaded_pdf_to_letters_bucket(source_filename, upload_filename):
def move_sanitised_letter_to_test_or_live_pdf_bucket(filename, is_test_letter, created_at, new_filename): def move_sanitised_letter_to_test_or_live_pdf_bucket(filename, is_test_letter, created_at, new_filename):
target_bucket_config = 'TEST_LETTERS_BUCKET_NAME' if is_test_letter else 'LETTERS_PDF_BUCKET_NAME' target_bucket_config = 'TEST_LETTERS_BUCKET_NAME' if is_test_letter else 'LETTERS_PDF_BUCKET_NAME'
target_bucket_name = current_app.config[target_bucket_config] target_bucket_name = current_app.config[target_bucket_config]
target_filename = get_folder_name(created_at, dont_use_sending_date=is_test_letter) + new_filename target_filename = get_folder_name(created_at, ignore_folder=is_test_letter) + new_filename
_move_s3_object( _move_s3_object(
source_bucket=current_app.config['LETTER_SANITISE_BUCKET_NAME'], source_bucket=current_app.config['LETTER_SANITISE_BUCKET_NAME'],

View File

@@ -196,7 +196,7 @@ def send_pdf_letter_notification(service_id, post_data):
reference=notification.reference, reference=notification.reference,
crown=notification.service.crown, crown=notification.service.crown,
sending_date=notification.created_at, sending_date=notification.created_at,
dont_use_sending_date=False, ignore_folder=False,
postage=notification.postage postage=notification.postage
) )

View File

@@ -62,7 +62,10 @@ def test_get_pdf_for_templated_letter_happy_path(mocker, sample_letter_notificat
letter_branding = create_letter_branding(name=branding_name, filename=logo_filename) letter_branding = create_letter_branding(name=branding_name, filename=logo_filename)
sample_letter_notification.service.letter_branding = letter_branding sample_letter_notification.service.letter_branding = letter_branding
mock_celery = mocker.patch('app.celery.letters_pdf_tasks.notify_celery.send_task') mock_celery = mocker.patch('app.celery.letters_pdf_tasks.notify_celery.send_task')
mocker.patch('app.celery.letters_pdf_tasks.get_letter_pdf_filename', return_value='LETTER.PDF') mock_get_letter_pdf_filename = mocker.patch(
'app.celery.letters_pdf_tasks.get_letter_pdf_filename',
return_value='LETTER.PDF'
)
get_pdf_for_templated_letter(sample_letter_notification.id) get_pdf_for_templated_letter(sample_letter_notification.id)
letter_data = { letter_data = {
@@ -87,6 +90,14 @@ def test_get_pdf_for_templated_letter_happy_path(mocker, sample_letter_notificat
queue=QueueNames.SANITISE_LETTERS queue=QueueNames.SANITISE_LETTERS
) )
mock_get_letter_pdf_filename.assert_called_once_with(
reference=sample_letter_notification.reference,
crown=True,
sending_date=sample_letter_notification.created_at,
ignore_folder=False,
postage='second'
)
def test_get_pdf_for_templated_letter_non_existent_notification(notify_api, mocker, fake_uuid): def test_get_pdf_for_templated_letter_non_existent_notification(notify_api, mocker, fake_uuid):
with pytest.raises(expected_exception=NoResultFound): with pytest.raises(expected_exception=NoResultFound):

View File

@@ -1,6 +1,7 @@
import pytest import pytest
from datetime import datetime from datetime import datetime
import dateutil
import boto3 import boto3
from flask import current_app from flask import current_app
from freezegun import freeze_time from freezegun import freeze_time
@@ -161,7 +162,7 @@ def test_get_letter_pdf_filename_returns_correct_filename_for_test_letters(
notify_api, mocker): notify_api, mocker):
sending_date = datetime(2017, 12, 4, 17, 29) sending_date = datetime(2017, 12, 4, 17, 29)
filename = get_letter_pdf_filename(reference='foo', crown='C', filename = get_letter_pdf_filename(reference='foo', crown='C',
sending_date=sending_date, dont_use_sending_date=True) sending_date=sending_date, ignore_folder=True)
assert filename == 'NOTIFY.FOO.D.2.C.C.20171204172900.PDF' assert filename == 'NOTIFY.FOO.D.2.C.C.20171204172900.PDF'
@@ -216,7 +217,7 @@ def test_upload_letter_pdf_to_correct_bucket(
reference=sample_letter_notification.reference, reference=sample_letter_notification.reference,
crown=sample_letter_notification.service.crown, crown=sample_letter_notification.service.crown,
sending_date=sample_letter_notification.created_at, sending_date=sample_letter_notification.created_at,
dont_use_sending_date=is_precompiled_letter ignore_folder=is_precompiled_letter
) )
upload_letter_pdf(sample_letter_notification, b'\x00\x01', precompiled=is_precompiled_letter) upload_letter_pdf(sample_letter_notification, b'\x00\x01', precompiled=is_precompiled_letter)
@@ -243,7 +244,7 @@ def test_upload_letter_pdf_uses_postage_from_notification(
reference=letter_notification.reference, reference=letter_notification.reference,
crown=letter_notification.service.crown, crown=letter_notification.service.crown,
sending_date=letter_notification.created_at, sending_date=letter_notification.created_at,
dont_use_sending_date=False, ignore_folder=False,
postage=letter_notification.postage postage=letter_notification.postage
) )
@@ -293,7 +294,7 @@ def test_move_failed_pdf_scan_failed(notify_api):
assert filename not in [o.key for o in bucket.objects.all()] assert filename not in [o.key for o in bucket.objects.all()]
@pytest.mark.parametrize("freeze_date, expected_folder_name", @pytest.mark.parametrize("timestamp, expected_folder_name",
[("2018-04-01 17:50:00", "2018-04-02/"), [("2018-04-01 17:50:00", "2018-04-02/"),
("2018-07-02 16:29:00", "2018-07-02/"), ("2018-07-02 16:29:00", "2018-07-02/"),
("2018-07-02 16:30:00", "2018-07-02/"), ("2018-07-02 16:30:00", "2018-07-02/"),
@@ -309,15 +310,14 @@ def test_move_failed_pdf_scan_failed(notify_api):
("2018-01-02 23:30:00", "2018-01-03/"), ("2018-01-02 23:30:00", "2018-01-03/"),
("2018-01-03 00:30:00", "2018-01-03/"), ("2018-01-03 00:30:00", "2018-01-03/"),
]) ])
def test_get_folder_name_in_british_summer_time(notify_api, freeze_date, expected_folder_name): def test_get_folder_name_in_british_summer_time(notify_api, timestamp, expected_folder_name):
with freeze_time(freeze_date): timestamp = dateutil.parser.parse(timestamp)
now = datetime.utcnow() folder_name = get_folder_name(created_at=timestamp, ignore_folder=False)
folder_name = get_folder_name(_now=now, dont_use_sending_date=False)
assert folder_name == expected_folder_name assert folder_name == expected_folder_name
def test_get_folder_name_returns_empty_string_for_test_letter(): def test_get_folder_name_returns_empty_string_for_test_letter():
assert '' == get_folder_name(datetime.utcnow(), dont_use_sending_date=True) assert '' == get_folder_name(datetime.utcnow(), ignore_folder=True)
@mock_s3 @mock_s3