Merge pull request #905 from alphagov/letter-scheduled-task

exclude letters from scheduled cleanup tasks

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 current_status == NOTIFICATION_PENDING and status == NOTIFICATION_PERMANENT_FAILURE:
-        if status == 'permanent-failure':
+        status = NOTIFICATION_TEMPORARY_FAILURE
-            status = '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',
+        or_(Notification.status == NOTIFICATION_CREATED,
-            Notification.status == 'sending',
+            Notification.status == NOTIFICATION_SENDING,
-            Notification.status == 'pending')).first()
+            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',
+        or_(Notification.status == NOTIFICATION_SENDING,
-            Notification.status == 'pending')).first()
+            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):
+def _timeout_notifications(current_statuses, new_status, timeout_start, updated_at):
-    # update all notifications that are older that the timeout_period_in_seconds
+    for table in [NotificationHistory, Notification]:
-    # with a status of created|sending|pending
+        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
+    updated = timeout([NOTIFICATION_CREATED], NOTIFICATION_TECHNICAL_FAILURE)
-    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)
 
     # 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 += timeout([NOTIFICATION_SENDING, NOTIFICATION_PENDING], NOTIFICATION_TEMPORARY_FAILURE)
-    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)
     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

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,
+def test_update_status_of_notifications_after_timeout(notify_api, sample_template):
-                                                      notify_db,
-                                                      notify_db_session,
-                                                      sample_service,
-                                                      sample_template,
-                                                      mmg_provider):
     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,
+def test_not_update_status_of_notification_before_timeout(notify_api, sample_template):
-                                                          notify_db,
-                                                          notify_db_session,
-                                                          sample_service,
-                                                          sample_template,
-                                                          mmg_provider):
     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')
 

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')
+        created = create_notification(sample_template, status='created')
-        sending = sample_notification(notify_db, notify_db_session, status='sending')
+        sending = create_notification(sample_template, status='sending')
-        pending = sample_notification(notify_db, notify_db_session, status='pending')
+        pending = create_notification(sample_template, status='pending')
-        delivered = sample_notification(notify_db, notify_db_session, status='delivered')
+        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')
+        created = create_notification(sample_template, status='created')
-        sending = sample_notification(notify_db, notify_db_session, status='sending')
+        sending = create_notification(sample_template, status='sending')
-        pending = sample_notification(notify_db, notify_db_session, status='pending')
+        pending = create_notification(sample_template, status='pending')
-        delivered = sample_notification(notify_db, notify_db_session, status='delivered')
+        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
 

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,