diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 614585a51..29632e69d 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -1,3 +1,4 @@ +import functools import pytz from datetime import ( datetime, @@ -23,7 +24,9 @@ from app.models import ( NOTIFICATION_PENDING, NOTIFICATION_TECHNICAL_FAILURE, NOTIFICATION_TEMPORARY_FAILURE, - KEY_TYPE_NORMAL, KEY_TYPE_TEST + NOTIFICATION_PERMANENT_FAILURE, + KEY_TYPE_NORMAL, KEY_TYPE_TEST, + LETTER_TYPE ) from app.dao.dao_utils import transactional @@ -137,7 +140,7 @@ def dao_create_notification(notification): # need to populate defaulted fields before we create the notification history object notification.id = create_uuid() if not notification.status: - notification.status = 'created' + notification.status = NOTIFICATION_CREATED db.session.add(notification) if _should_record_notification_in_history_table(notification): @@ -155,9 +158,8 @@ def _should_record_notification_in_history_table(notification): def _decide_permanent_temporary_failure(current_status, status): # Firetext will send pending, then send either succes or fail. # If we go from pending to delivered we need to set failure type as temporary-failure - if current_status == 'pending': - if status == 'permanent-failure': - status = 'temporary-failure' + if current_status == NOTIFICATION_PENDING and status == NOTIFICATION_PERMANENT_FAILURE: + status = NOTIFICATION_TEMPORARY_FAILURE return status @@ -173,9 +175,9 @@ def _update_notification_status(notification, status): def update_notification_status_by_id(notification_id, status): notification = Notification.query.with_lockmode("update").filter( Notification.id == notification_id, - or_(Notification.status == 'created', - Notification.status == 'sending', - Notification.status == 'pending')).first() + or_(Notification.status == NOTIFICATION_CREATED, + Notification.status == NOTIFICATION_SENDING, + Notification.status == NOTIFICATION_PENDING)).first() if not notification: return None @@ -191,8 +193,8 @@ def update_notification_status_by_id(notification_id, status): def update_notification_status_by_reference(reference, status): notification = Notification.query.filter( Notification.reference == reference, - or_(Notification.status == 'sending', - Notification.status == 'pending')).first() + or_(Notification.status == NOTIFICATION_SENDING, + Notification.status == NOTIFICATION_PENDING)).first() if not notification: return None @@ -346,7 +348,7 @@ def delete_notifications_created_more_than_a_week_ago(status): seven_days_ago = date.today() - timedelta(days=7) deleted = db.session.query(Notification).filter( func.date(Notification.created_at) < seven_days_ago, - Notification.status == status + Notification.status == status, ).delete(synchronize_session='fetch') db.session.commit() return deleted @@ -363,32 +365,40 @@ def dao_delete_notifications_and_history_by_id(notification_id): ).delete(synchronize_session='fetch') -def dao_timeout_notifications(timeout_period_in_seconds): - # update all notifications that are older that the timeout_period_in_seconds - # with a status of created|sending|pending +def _timeout_notifications(current_statuses, new_status, timeout_start, updated_at): + for table in [NotificationHistory, Notification]: + q = table.query.filter( + table.created_at < timeout_start, + table.status.in_(current_statuses), + table.notification_type != LETTER_TYPE + ) + last_update_count = q.update({'status': new_status, 'updated_at': updated_at}, synchronize_session=False) + return last_update_count + +def dao_timeout_notifications(timeout_period_in_seconds): + """ + Timeout SMS and email notifications by the following rules: + + we never sent the notification to the provider for some reason + created -> technical-failure + + the notification was sent to the provider but there was not a delivery receipt + sending -> temporary-failure + pending -> temporary-failure + + Letter notifications are not timed out + """ + timeout_start = datetime.utcnow() - timedelta(seconds=timeout_period_in_seconds) + updated_at = datetime.utcnow() + + timeout = functools.partial(_timeout_notifications, timeout_start=timeout_start, updated_at=updated_at) # Notifications still in created status are marked with a technical-failure: - # the notification has failed to go to the provider - update_at = datetime.utcnow() - updated = db.session.query(Notification). \ - filter(Notification.created_at < (datetime.utcnow() - timedelta(seconds=timeout_period_in_seconds))). \ - filter(Notification.status == NOTIFICATION_CREATED). \ - update({'status': NOTIFICATION_TECHNICAL_FAILURE, 'updated_at': update_at}, synchronize_session=False) - db.session.query(NotificationHistory). \ - filter(NotificationHistory.created_at < (datetime.utcnow() - timedelta(seconds=timeout_period_in_seconds))). \ - filter(NotificationHistory.status == NOTIFICATION_CREATED). \ - update({'status': NOTIFICATION_TECHNICAL_FAILURE, 'updated_at': update_at}, synchronize_session=False) + updated = timeout([NOTIFICATION_CREATED], NOTIFICATION_TECHNICAL_FAILURE) # Notifications still in sending or pending status are marked with a temporary-failure: - # the notification was sent to the provider but there was not a delivery receipt, try to send again. - updated += db.session.query(Notification). \ - filter(Notification.created_at < (datetime.utcnow() - timedelta(seconds=timeout_period_in_seconds))). \ - filter(Notification.status.in_([NOTIFICATION_SENDING, NOTIFICATION_PENDING])). \ - update({'status': NOTIFICATION_TEMPORARY_FAILURE, 'updated_at': update_at}, synchronize_session=False) - db.session.query(NotificationHistory). \ - filter(NotificationHistory.created_at < (datetime.utcnow() - timedelta(seconds=timeout_period_in_seconds))). \ - filter(NotificationHistory.status.in_([NOTIFICATION_SENDING, NOTIFICATION_PENDING])). \ - update({'status': NOTIFICATION_TEMPORARY_FAILURE, 'updated_at': update_at}, synchronize_session=False) + updated += timeout([NOTIFICATION_SENDING, NOTIFICATION_PENDING], NOTIFICATION_TEMPORARY_FAILURE) + db.session.commit() return updated @@ -447,10 +457,10 @@ def dao_update_notifications_sent_to_dvla(job_id, provider): now = datetime.utcnow() updated_count = db.session.query( Notification).filter(Notification.job_id == job_id).update( - {'status': 'sending', "sent_by": provider, "sent_at": now}) + {'status': NOTIFICATION_SENDING, "sent_by": provider, "sent_at": now}) db.session.query( NotificationHistory).filter(NotificationHistory.job_id == job_id).update( - {'status': 'sending', "sent_by": provider, "sent_at": now, "updated_at": now}) + {'status': NOTIFICATION_SENDING, "sent_by": provider, "sent_at": now, "updated_at": now}) return updated_count diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index cbec1e43c..038d2ca1c 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -105,12 +105,7 @@ def test_should_call_delete_invotations_on_delete_invitations_task(notify_api, m assert scheduled_tasks.delete_invitations_created_more_than_two_days_ago.call_count == 1 -def test_update_status_of_notifications_after_timeout(notify_api, - notify_db, - notify_db_session, - sample_service, - sample_template, - mmg_provider): +def test_update_status_of_notifications_after_timeout(notify_api, sample_template): with notify_api.test_request_context(): not1 = create_notification( template=sample_template, @@ -133,12 +128,7 @@ def test_update_status_of_notifications_after_timeout(notify_api, assert not3.status == 'temporary-failure' -def test_not_update_status_of_notification_before_timeout(notify_api, - notify_db, - notify_db_session, - sample_service, - sample_template, - mmg_provider): +def test_not_update_status_of_notification_before_timeout(notify_api, sample_template): with notify_api.test_request_context(): not1 = create_notification( template=sample_template, @@ -149,6 +139,17 @@ def test_not_update_status_of_notification_before_timeout(notify_api, assert not1.status == 'sending' +def test_should_not_update_status_of_letter_notifications(client, sample_letter_template): + created_at = datetime.utcnow() - timedelta(days=5) + not1 = create_notification(template=sample_letter_template, status='sending', created_at=created_at) + not2 = create_notification(template=sample_letter_template, status='created', created_at=created_at) + + timeout_notifications() + + assert not1.status == 'sending' + assert not2.status == 'created' + + def test_should_update_scheduled_jobs_and_put_on_queue(notify_db, notify_db_session, mocker): mocked = mocker.patch('app.celery.tasks.process_job.apply_async') diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index bf66a12d5..99d1200ec 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -862,6 +862,15 @@ def test_should_not_delete_failed_notifications_before_seven_days(notify_db, not assert Notification.query.first().to == 'do_not_delete' +def test_should_delete_letter_notifications(sample_letter_template): + should_delete = datetime.utcnow() - timedelta(days=8) + + create_notification(sample_letter_template, created_at=should_delete) + + delete_notifications_created_more_than_a_week_ago('created') + assert len(Notification.query.all()) == 0 + + @freeze_time("2016-01-10") def test_should_limit_notifications_return_by_day_limit_plus_one(notify_db, notify_db_session, sample_service): assert len(Notification.query.all()) == 0 @@ -1036,12 +1045,12 @@ def _notification_json(sample_template, job_id=None, id=None, status=None): return data -def test_dao_timeout_notifications(notify_db, notify_db_session, ): +def test_dao_timeout_notifications(sample_template): with freeze_time(datetime.utcnow() - timedelta(minutes=2)): - created = sample_notification(notify_db, notify_db_session, status='created') - sending = sample_notification(notify_db, notify_db_session, status='sending') - pending = sample_notification(notify_db, notify_db_session, status='pending') - delivered = sample_notification(notify_db, notify_db_session, status='delivered') + created = create_notification(sample_template, status='created') + sending = create_notification(sample_template, status='sending') + pending = create_notification(sample_template, status='pending') + delivered = create_notification(sample_template, status='delivered') assert Notification.query.get(created.id).status == 'created' assert Notification.query.get(sending.id).status == 'sending' @@ -1059,12 +1068,12 @@ def test_dao_timeout_notifications(notify_db, notify_db_session, ): assert updated == 3 -def test_dao_timeout_notifications_only_updates_for_older_notifications(notify_db, notify_db_session): +def test_dao_timeout_notifications_only_updates_for_older_notifications(sample_template): with freeze_time(datetime.utcnow() + timedelta(minutes=10)): - created = sample_notification(notify_db, notify_db_session, status='created') - sending = sample_notification(notify_db, notify_db_session, status='sending') - pending = sample_notification(notify_db, notify_db_session, status='pending') - delivered = sample_notification(notify_db, notify_db_session, status='delivered') + created = create_notification(sample_template, status='created') + sending = create_notification(sample_template, status='sending') + pending = create_notification(sample_template, status='pending') + delivered = create_notification(sample_template, status='delivered') assert Notification.query.get(created.id).status == 'created' assert Notification.query.get(sending.id).status == 'sending' @@ -1078,6 +1087,27 @@ def test_dao_timeout_notifications_only_updates_for_older_notifications(notify_d assert updated == 0 +def test_dao_timeout_notifications_doesnt_affect_letters(sample_letter_template): + with freeze_time(datetime.utcnow() - timedelta(minutes=2)): + created = create_notification(sample_letter_template, status='created') + sending = create_notification(sample_letter_template, status='sending') + pending = create_notification(sample_letter_template, status='pending') + delivered = create_notification(sample_letter_template, status='delivered') + + assert Notification.query.get(created.id).status == 'created' + assert Notification.query.get(sending.id).status == 'sending' + assert Notification.query.get(pending.id).status == 'pending' + assert Notification.query.get(delivered.id).status == 'delivered' + + updated = dao_timeout_notifications(1) + + assert NotificationHistory.query.get(created.id).status == 'created' + assert NotificationHistory.query.get(sending.id).status == 'sending' + assert NotificationHistory.query.get(pending.id).status == 'pending' + assert NotificationHistory.query.get(delivered.id).status == 'delivered' + assert updated == 0 + + def test_should_return_notifications_excluding_jobs_by_default(notify_db, notify_db_session, sample_service): assert len(Notification.query.all()) == 0 diff --git a/tests/app/db.py b/tests/app/db.py index e2a24f1c3..195a77ea7 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -86,6 +86,7 @@ def create_notification( 'to': to_field, 'job_id': job.id if job else None, 'job': job, + 'service_id': template.service.id, 'service': template.service, 'template_id': template.id if template else None, 'template': template,