From b28fffc330580e4e471a9324ef767f1fa02c4162 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Thu, 2 Apr 2020 09:36:56 +0100 Subject: [PATCH 1/2] Refactor `raise_alert_if_letter_notifications_still_sending` Move finding of letter logic into a separate method so it can be unit tested rather than having to test it by checking the contents of a zendesk ticket api call. This will enable us to change the zendesk api ticket call message without needing to edit lots of tests. --- app/celery/nightly_tasks.py | 60 ++++++++------ tests/app/celery/test_nightly_tasks.py | 109 ++++++++++++------------- 2 files changed, 87 insertions(+), 82 deletions(-) diff --git a/app/celery/nightly_tasks.py b/app/celery/nightly_tasks.py index 154fec694..0c077a79a 100644 --- a/app/celery/nightly_tasks.py +++ b/app/celery/nightly_tasks.py @@ -245,35 +245,12 @@ def delete_dvla_response_files_older_than_seven_days(): @cronitor("raise-alert-if-letter-notifications-still-sending") @statsd(namespace="tasks") def raise_alert_if_letter_notifications_still_sending(): - today = datetime.utcnow().date() + still_sending_count, sent_date = get_letter_notifications_still_sending_when_they_shouldnt_be() - # Do nothing on the weekend - if today.isoweekday() in {6, 7}: # sat, sun - return - - if today.isoweekday() in {1, 2}: # mon, tues. look for files from before the weekend - offset_days = 4 - else: - offset_days = 2 - - q = Notification.query.filter( - Notification.notification_type == LETTER_TYPE, - Notification.status == NOTIFICATION_SENDING, - Notification.key_type == KEY_TYPE_NORMAL, - func.date(Notification.sent_at) <= today - timedelta(days=offset_days) - ) - - if today.isoweekday() in {2, 4}: # on tue, thu, we only care about first class letters - q = q.filter( - Notification.postage == 'first' - ) - - still_sending = q.count() - - if still_sending: + if still_sending_count: message = "There are {} letters in the 'sending' state from {}".format( - still_sending, - (today - timedelta(days=offset_days)).strftime('%A %d %B') + still_sending_count, + sent_date.strftime('%A %d %B') ) # Only send alerts in production if current_app.config['NOTIFY_ENVIRONMENT'] in ['live', 'production', 'test']: @@ -286,6 +263,35 @@ def raise_alert_if_letter_notifications_still_sending(): current_app.logger.info(message) +def get_letter_notifications_still_sending_when_they_shouldnt_be(): + today = datetime.utcnow().date() + + # Do nothing on the weekend + if today.isoweekday() in {6, 7}: # sat, sun + return 0, None + + if today.isoweekday() in {1, 2}: # mon, tues. look for files from before the weekend + offset_days = 4 + else: + offset_days = 2 + + expected_sent_date = today - timedelta(days=offset_days) + + q = Notification.query.filter( + Notification.notification_type == LETTER_TYPE, + Notification.status == NOTIFICATION_SENDING, + Notification.key_type == KEY_TYPE_NORMAL, + func.date(Notification.sent_at) <= expected_sent_date + ) + + if today.isoweekday() in {2, 4}: # on tue, thu, we only care about first class letters + q = q.filter( + Notification.postage == 'first' + ) + + return q.count(), expected_sent_date + + @notify_celery.task(name='raise-alert-if-no-letter-ack-file') @cronitor('raise-alert-if-no-letter-ack-file') @statsd(namespace="tasks") diff --git a/tests/app/celery/test_nightly_tasks.py b/tests/app/celery/test_nightly_tasks.py index 1713f8f49..e48f3fd16 100644 --- a/tests/app/celery/test_nightly_tasks.py +++ b/tests/app/celery/test_nightly_tasks.py @@ -14,6 +14,7 @@ from app.celery.nightly_tasks import ( delete_inbound_sms, delete_letter_notifications_older_than_retention, delete_sms_notifications_older_than_retention, + get_letter_notifications_still_sending_when_they_shouldnt_be, raise_alert_if_letter_notifications_still_sending, remove_letter_csv_files, remove_sms_email_csv_files, @@ -335,11 +336,12 @@ def test_delete_dvla_response_files_older_than_seven_days_does_not_remove_files( remove_s3_mock.assert_not_called() -@freeze_time("Thursday 17th January 2018 17:00") -def test_alert_if_letter_notifications_still_sending(sample_letter_template, mocker): - two_days_ago = datetime(2018, 1, 15, 13, 30) - create_notification(template=sample_letter_template, status='sending', sent_at=two_days_ago) +def test_create_ticket_if_letter_notifications_still_sending(mocker): + mock_get_letters = mocker.patch( + "app.celery.nightly_tasks.get_letter_notifications_still_sending_when_they_shouldnt_be" + ) + mock_get_letters.return_value = 1, date(2018, 1, 15) mock_create_ticket = mocker.patch("app.celery.nightly_tasks.zendesk_client.create_ticket") raise_alert_if_letter_notifications_still_sending() @@ -351,83 +353,86 @@ def test_alert_if_letter_notifications_still_sending(sample_letter_template, moc ) -def test_alert_if_letter_notifications_still_sending_a_day_ago_no_alert(sample_letter_template, mocker): +def test_dont_create_ticket_if_letter_notifications_not_still_sending(mocker): + mock_get_letters = mocker.patch( + "app.celery.nightly_tasks.get_letter_notifications_still_sending_when_they_shouldnt_be" + ) + + mock_get_letters.return_value = 0, None + mock_create_ticket = mocker.patch("app.celery.nightly_tasks.zendesk_client.create_ticket") + + raise_alert_if_letter_notifications_still_sending() + + mock_create_ticket.assert_not_called() + + +@freeze_time("Thursday 17th January 2018 17:00") +def test_get_letter_notifications_still_sending_when_they_shouldnt_finds_no_letters_if_sent_a_day_ago( + sample_letter_template +): today = datetime.utcnow() one_day_ago = today - timedelta(days=1) create_notification(template=sample_letter_template, status='sending', sent_at=one_day_ago) - mock_create_ticket = mocker.patch("app.celery.nightly_tasks.zendesk_client.create_ticket") - - raise_alert_if_letter_notifications_still_sending() - assert not mock_create_ticket.called + count, expected_sent_date = get_letter_notifications_still_sending_when_they_shouldnt_be() + assert count == 0 @freeze_time("Thursday 17th January 2018 17:00") -def test_alert_if_letter_notifications_still_sending_only_alerts_sending(sample_letter_template, mocker): +def test_get_letter_notifications_still_sending_when_they_shouldnt_only_finds_letters_still_in_sending_status( + sample_letter_template +): two_days_ago = datetime(2018, 1, 15, 13, 30) create_notification(template=sample_letter_template, status='sending', sent_at=two_days_ago) create_notification(template=sample_letter_template, status='delivered', sent_at=two_days_ago) create_notification(template=sample_letter_template, status='failed', sent_at=two_days_ago) - mock_create_ticket = mocker.patch("app.celery.nightly_tasks.zendesk_client.create_ticket") - - raise_alert_if_letter_notifications_still_sending() - - mock_create_ticket.assert_called_once_with( - subject="[test] Letters still sending", - message="There are 1 letters in the 'sending' state from Monday 15 January", - ticket_type='incident' - ) + count, expected_sent_date = get_letter_notifications_still_sending_when_they_shouldnt_be() + assert count == 1 + assert expected_sent_date == date(2018, 1, 15) @freeze_time("Thursday 17th January 2018 17:00") -def test_alert_if_letter_notifications_still_sending_alerts_for_older_than_offset(sample_letter_template, mocker): +def test_get_letter_notifications_still_sending_when_they_shouldnt_finds_letters_older_than_offset( + sample_letter_template +): three_days_ago = datetime(2018, 1, 14, 13, 30) create_notification(template=sample_letter_template, status='sending', sent_at=three_days_ago) - mock_create_ticket = mocker.patch("app.celery.nightly_tasks.zendesk_client.create_ticket") - - raise_alert_if_letter_notifications_still_sending() - - mock_create_ticket.assert_called_once_with( - subject="[test] Letters still sending", - message="There are 1 letters in the 'sending' state from Monday 15 January", - ticket_type='incident' - ) + count, expected_sent_date = get_letter_notifications_still_sending_when_they_shouldnt_be() + assert count == 1 + assert expected_sent_date == date(2018, 1, 15) @freeze_time("Sunday 14th January 2018 17:00") -def test_alert_if_letter_notifications_still_sending_does_nothing_on_the_weekend(sample_letter_template, mocker): +def test_get_letter_notifications_still_sending_when_they_shouldnt_be_finds_no_letters_on_weekend( + sample_letter_template +): yesterday = datetime(2018, 1, 13, 13, 30) create_notification(template=sample_letter_template, status='sending', sent_at=yesterday) - mock_create_ticket = mocker.patch("app.celery.nightly_tasks.zendesk_client.create_ticket") - - raise_alert_if_letter_notifications_still_sending() - - assert not mock_create_ticket.called + count, expected_sent_date = get_letter_notifications_still_sending_when_they_shouldnt_be() + assert count == 0 @freeze_time("Monday 15th January 2018 17:00") -def test_monday_alert_if_letter_notifications_still_sending_reports_thursday_letters(sample_letter_template, mocker): +def test_get_letter_notifications_still_sending_when_they_shouldnt_finds_thursday_letters_when_run_on_monday( + sample_letter_template +): thursday = datetime(2018, 1, 11, 13, 30) yesterday = datetime(2018, 1, 14, 13, 30) create_notification(template=sample_letter_template, status='sending', sent_at=thursday, postage='second') create_notification(template=sample_letter_template, status='sending', sent_at=yesterday, postage='second') - mock_create_ticket = mocker.patch("app.celery.nightly_tasks.zendesk_client.create_ticket") - - raise_alert_if_letter_notifications_still_sending() - - mock_create_ticket.assert_called_once_with( - subject="[test] Letters still sending", - message="There are 1 letters in the 'sending' state from Thursday 11 January", - ticket_type='incident' - ) + count, expected_sent_date = get_letter_notifications_still_sending_when_they_shouldnt_be() + assert count == 1 + assert expected_sent_date == date(2018, 1, 11) @freeze_time("Tuesday 16th January 2018 17:00") -def test_tuesday_alert_if_letter_notifications_still_sending_reports_friday_letters(sample_letter_template, mocker): +def test_get_letter_notifications_still_sending_when_they_shouldnt_finds_friday_letters_when_run_on_tuesday( + sample_letter_template +): friday = datetime(2018, 1, 12, 13, 30) yesterday = datetime(2018, 1, 14, 13, 30) create_notification(template=sample_letter_template, status='sending', sent_at=friday, postage='first') @@ -435,15 +440,9 @@ def test_tuesday_alert_if_letter_notifications_still_sending_reports_friday_lett # doesn't get reported because it's second class, and it's tuesday today create_notification(template=sample_letter_template, status='sending', sent_at=friday, postage='second') - mock_create_ticket = mocker.patch("app.celery.nightly_tasks.zendesk_client.create_ticket") - - raise_alert_if_letter_notifications_still_sending() - - mock_create_ticket.assert_called_once_with( - subject="[test] Letters still sending", - message="There are 1 letters in the 'sending' state from Friday 12 January", - ticket_type='incident' - ) + count, expected_sent_date = get_letter_notifications_still_sending_when_they_shouldnt_be() + assert count == 1 + assert expected_sent_date == date(2018, 1, 12) @freeze_time('2018-01-11T23:00:00') From 98d83888056252f1cc162a0787dda1346bde411b Mon Sep 17 00:00:00 2001 From: David McDonald Date: Thu, 2 Apr 2020 09:42:46 +0100 Subject: [PATCH 2/2] Add runbook link to ticket --- app/celery/nightly_tasks.py | 1 + tests/app/celery/test_nightly_tasks.py | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/app/celery/nightly_tasks.py b/app/celery/nightly_tasks.py index 0c077a79a..700303dc9 100644 --- a/app/celery/nightly_tasks.py +++ b/app/celery/nightly_tasks.py @@ -254,6 +254,7 @@ def raise_alert_if_letter_notifications_still_sending(): ) # Only send alerts in production if current_app.config['NOTIFY_ENVIRONMENT'] in ['live', 'production', 'test']: + message += ". Resolve using https://github.com/alphagov/notifications-manuals/wiki/Support-Runbook#deal-with-letters-still-in-sending" # noqa zendesk_client.create_ticket( subject="[{}] Letters still sending".format(current_app.config['NOTIFY_ENVIRONMENT']), message=message, diff --git a/tests/app/celery/test_nightly_tasks.py b/tests/app/celery/test_nightly_tasks.py index e48f3fd16..a09a40281 100644 --- a/tests/app/celery/test_nightly_tasks.py +++ b/tests/app/celery/test_nightly_tasks.py @@ -348,7 +348,7 @@ def test_create_ticket_if_letter_notifications_still_sending(mocker): mock_create_ticket.assert_called_once_with( subject="[test] Letters still sending", - message="There are 1 letters in the 'sending' state from Monday 15 January", + message="There are 1 letters in the 'sending' state from Monday 15 January. Resolve using https://github.com/alphagov/notifications-manuals/wiki/Support-Runbook#deal-with-letters-still-in-sending", # noqa ticket_type=ZendeskClient.TYPE_INCIDENT )