Skip to content

Commit

Permalink
set letter to technical-failure if we cant find it in s3
Browse files Browse the repository at this point in the history
otherwise we'll just do nothing until the next day, when we'll try and
send it again. i think the chance of it appearing in s3 overnight feels
slim to unlikely - this was behaviour we added three years ago[^1] but
the log message never fired due to a bug in how the function was called.
it feels safer and more correct to set the letter to technical-failure.
we'll get a proper exception log and it'll be easier to distinguish
failed letters in technical-failure that we need to action from new
letters in created that will go out as normal

[^1]: #2760
  • Loading branch information
leohemsted committed Mar 16, 2023
1 parent 0a40773 commit c968f91
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 9 deletions.
6 changes: 4 additions & 2 deletions app/celery/provider_tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,10 @@ def deliver_letter(self, notification_id):
try:
file_bytes = find_letter_pdf_in_s3(notification).get()["Body"].read()
except (BotoClientError, LetterPDFNotFound) as e:
current_app.logger.exception(f"Error getting letter from bucket for notification: {notification_id}", str(e))
return
update_notification_status_by_id(notification_id, NOTIFICATION_TECHNICAL_FAILURE)
raise NotificationTechnicalFailureException(
f"Error getting letter from bucket for notification {notification_id}"
) from e

try:
dvla_client.send_letter(
Expand Down
13 changes: 6 additions & 7 deletions tests/app/celery/test_provider_tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,7 @@ def test_deliver_letter(mocker, sample_letter_template, sample_organisation, is_
@freeze_time("2020-02-17 16:00:00")
def test_deliver_letter_when_file_is_not_in_S3_logs_an_error(mocker, sample_letter_template, sample_organisation):
mock_send_letter = mocker.patch("app.celery.provider_tasks.dvla_client.send_letter")
mock_logger_exception = mocker.patch("app.celery.tasks.current_app.logger.exception")
mock_retry = mocker.patch("app.celery.provider_tasks.deliver_letter.retry", side_effect=MaxRetriesExceededError())

letter = create_notification(
template=sample_letter_template,
Expand All @@ -311,14 +311,13 @@ def test_deliver_letter_when_file_is_not_in_S3_logs_an_error(mocker, sample_lett
s3 = boto3.client("s3", region_name="eu-west-1")
s3.create_bucket(Bucket=pdf_bucket, CreateBucketConfiguration={"LocationConstraint": "eu-west-1"})

deliver_letter(letter.id)
with pytest.raises(NotificationTechnicalFailureException) as e:
deliver_letter(letter.id)

mock_logger_exception.assert_called_once_with(
f"Error getting letter from bucket for notification: {letter.id}",
"File not found in bucket test-letters-pdf with prefix 2020-02-17/NOTIFY.REF1",
)
assert str(letter.id) in str(e.value)
assert not mock_retry.called
assert not mock_send_letter.called
assert letter.status == NOTIFICATION_CREATED
assert letter.status == NOTIFICATION_TECHNICAL_FAILURE


@mock_s3
Expand Down

0 comments on commit c968f91

Please sign in to comment.