From ab4cb029dfc7e97d477c960f38baf59277d92990 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 6 Dec 2021 14:07:52 +0000 Subject: [PATCH] Remove alert for email / sms in created In response to [1]. [1]: https://github.com/alphagov/notifications-api/pull/3383#discussion_r759379988 It turns out the code that inspired this new alert - in the old "timeout-sending-notifications" task - was actually redundant as we already have a task to "replay" notifications still in "created", which is much better than just alerting about them. It's possible the replayed notifications will also fail, but in both cases we should see some kind of error due to this, so I don't think we're losing anything by not having an alert. --- app/celery/scheduled_tasks.py | 14 ---------- app/config.py | 6 ---- app/dao/notifications_dao.py | 10 ------- tests/app/celery/test_scheduled_tasks.py | 19 ------------- .../notification_dao/test_notification_dao.py | 28 ------------------- 5 files changed, 77 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index 804194cb5..968a4231d 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -30,7 +30,6 @@ from app.dao.jobs_dao import ( find_missing_row_for_job, ) from app.dao.notifications_dao import ( - dao_check_notifications_still_in_created, dao_old_letters_with_created_status, dao_precompiled_letters_still_pending_virus_check, is_delivery_slow_for_providers, @@ -119,19 +118,6 @@ def switch_current_sms_provider_on_slow_delivery(): dao_reduce_sms_provider_priority(provider_name, time_threshold=timedelta(minutes=10)) -@notify_celery.task(name='raise-alert-if-email-sms-still-in-created') -def raise_alert_if_email_sms_still_in_created(): - alert_above_age = current_app.config.get('CREATED_NOTIFICATIONS_ALERT_AGE') - still_in_created = dao_check_notifications_still_in_created(alert_above_age) - message = f"{still_in_created} notifications are still in 'created'." - - if still_in_created == 0: - current_app.logger.info(message) - return - - current_app.logger.error(message) - - @notify_celery.task(name='tend-providers-back-to-middle') def tend_providers_back_to_middle(): dao_adjust_provider_priority_back_to_resting_points() diff --git a/app/config.py b/app/config.py index d953e1433..ab5a08b27 100644 --- a/app/config.py +++ b/app/config.py @@ -310,11 +310,6 @@ class Config(object): 'schedule': crontab(hour=15, minute=30), 'options': {'queue': QueueNames.PERIODIC} }, - 'raise-alert-if-email-sms-still-in-created': { - 'task': 'raise-alert-if-email-sms-still-in-created', - 'schedule': crontab(minute=30), - 'options': {'queue': QueueNames.PERIODIC}, - }, # The collate-letter-pdf does assume it is called in an hour that BST does not make a # difference to the truncate date which translates to the filename to process 'collate-letter-pdfs-to-be-sent': { @@ -360,7 +355,6 @@ class Config(object): STATSD_ENABLED = bool(STATSD_HOST) SENDING_NOTIFICATIONS_TIMEOUT_PERIOD = 259200 # 3 days - CREATED_NOTIFICATIONS_ALERT_AGE = 3600 # 1 hour SIMULATED_EMAIL_ADDRESSES = ( 'simulate-delivered@notifications.service.gov.uk', diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 48a45b597..b8e461705 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -487,16 +487,6 @@ def dao_delete_notifications_by_id(notification_id): ).delete(synchronize_session='fetch') -def dao_check_notifications_still_in_created(minimum_age_in_seconds): - min_created_at = datetime.utcnow() - timedelta(seconds=minimum_age_in_seconds) - - return Notification.query.filter( - Notification.created_at < min_created_at, - Notification.status == NOTIFICATION_CREATED, - Notification.notification_type.in_([SMS_TYPE, EMAIL_TYPE]) - ).count() - - def dao_timeout_notifications(timeout_period_in_seconds): """ Timeout SMS and email notifications by the following rules: diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 68ae6d073..edd8cbeee 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -19,7 +19,6 @@ from app.celery.scheduled_tasks import ( check_job_status, delete_invitations, delete_verify_codes, - raise_alert_if_email_sms_still_in_created, remove_yesterdays_planned_tests_on_govuk_alerts, replay_created_notifications, run_scheduled_jobs, @@ -109,24 +108,6 @@ def test_should_update_all_scheduled_jobs_and_put_on_queue(sample_template, mock ]) -def test_raise_alert_if_email_sms_still_in_created(notify_api, mocker): - mock_check = mocker.patch('app.celery.scheduled_tasks.dao_check_notifications_still_in_created') - mock_logger = mocker.patch('app.celery.scheduled_tasks.current_app.logger') - - mock_check.return_value = 0 - raise_alert_if_email_sms_still_in_created() - mock_logger.info.assert_called_once_with("0 notifications are still in 'created'.") - - assert mock_check.called_once_with( - notify_api.config['CREATED_NOTIFICATIONS_ALERT_AGE'] - ) - - # try again with something to alert about - mock_check.return_value = 1 - raise_alert_if_email_sms_still_in_created() - mock_logger.error.assert_called_once_with("1 notifications are still in 'created'.") - - @freeze_time('2017-05-01 14:00:00') def test_switch_current_sms_provider_on_slow_delivery_switches_when_one_provider_is_slow( mocker, diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 180d0ec92..a95bf47d2 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -8,7 +8,6 @@ from sqlalchemy.exc import IntegrityError, SQLAlchemyError from sqlalchemy.orm.exc import NoResultFound from app.dao.notifications_dao import ( - dao_check_notifications_still_in_created, dao_create_notification, dao_delete_notifications_by_id, dao_get_last_notification_added_for_job_id, @@ -665,33 +664,6 @@ def _notification_json(sample_template, job_id=None, id=None, status=None): return data -def test_dao_check_notifications_still_in_created( - sample_template, - sample_email_template, - sample_letter_template -): - with freeze_time(datetime.utcnow() - timedelta(minutes=2)): - # old sending notification (ignored) - create_notification(sample_template, status='sending') - # old letter notification (ignored) - create_notification(sample_letter_template, status='created') - - with freeze_time(datetime.utcnow() + timedelta(minutes=10)): - # new / future notification (ignored) - create_notification(sample_template, status='created') - - # first prove all the above notifications are ignored - assert dao_check_notifications_still_in_created(1) == 0 - - with freeze_time(datetime.utcnow() - timedelta(minutes=2)): - # now add old, eligible notifications - create_notification(sample_template, status='created') - create_notification(sample_email_template, status='created') - - # now prove the check only picks up on these ones - assert dao_check_notifications_still_in_created(1) == 2 - - def test_dao_timeout_notifications(sample_template): with freeze_time(datetime.utcnow() - timedelta(minutes=2)): created = create_notification(sample_template, status='created')