Merge pull request #260 from alphagov/notification-delete-schedule

Use short dates when selection notifications for deletion.

app/dao/notifications_dao.py

@@ -1,5 +1,5 @@
 import math
-from sqlalchemy import desc
+from sqlalchemy import desc, func
 
 from datetime import (
     datetime,
@@ -256,19 +256,11 @@ def filter_query(query, filter_dict=None):
     return query
 
 
-def delete_notifications_created_more_than_a_day_ago(status):
-    deleted = db.session.query(Notification).filter(
-        Notification.created_at < datetime.utcnow() - timedelta(days=1),
-        Notification.status == status
-    ).delete()
-    db.session.commit()
-    return deleted
-
-
 def delete_notifications_created_more_than_a_week_ago(status):
+    seven_days_ago = date.today() - timedelta(days=7)
     deleted = db.session.query(Notification).filter(
-        Notification.created_at < datetime.utcnow() - timedelta(days=7),
+        func.date(Notification.created_at) < seven_days_ago,
         Notification.status == status
-    ).delete()
+    ).delete(synchronize_session='fetch')
     db.session.commit()
     return deleted

app/models.py

@@ -1,7 +1,10 @@
 import uuid
 import datetime
 
-from sqlalchemy.dialects.postgresql import UUID
+from sqlalchemy.dialects.postgresql import (
+    UUID,
+    JSON
+)
 
 from sqlalchemy import UniqueConstraint
 

config.py

@@ -1,4 +1,5 @@
 from datetime import timedelta
+from celery.schedules import crontab
 from kombu import Exchange, Queue
 import os
 
@@ -51,12 +52,12 @@ class Config(object):
         },
         'delete-failed-notifications': {
             'task': 'delete-failed-notifications',
-            'schedule': timedelta(minutes=60),
+            'schedule': crontab(minute=0, hour=0),
             'options': {'queue': 'periodic'}
         },
         'delete-successful-notifications': {
             'task': 'delete-successful-notifications',
-            'schedule': timedelta(minutes=31),
+            'schedule': crontab(minute=0, hour=0),
             'options': {'queue': 'periodic'}
         }
     }

tests/app/dao/test_notification_dao.py

@@ -3,7 +3,6 @@ import uuid
 
 import pytest
 
-from app import DATE_FORMAT
 from freezegun import freeze_time
 from sqlalchemy.exc import SQLAlchemyError, IntegrityError
 
@@ -23,7 +22,6 @@ from app.dao.notifications_dao import (
     get_notification_for_job,
     get_notifications_for_job,
     dao_get_notification_statistics_for_service,
-    delete_notifications_created_more_than_a_day_ago,
     delete_notifications_created_more_than_a_week_ago,
     dao_get_notification_statistics_for_service_and_day,
     dao_get_notification_statistics_for_service_and_previous_days,
@@ -766,45 +764,38 @@ def test_update_notification(sample_notification, sample_template):
     assert notification_from_db.status == 'failed'
 
 
-def test_should_delete_notifications_after_one_day(notify_db, notify_db_session):
-    created_at = datetime.utcnow() - timedelta(hours=24)
-    sample_notification(notify_db, notify_db_session, created_at=created_at)
-    sample_notification(notify_db, notify_db_session, created_at=created_at)
-    assert len(Notification.query.all()) == 2
-    delete_notifications_created_more_than_a_day_ago('sending')
-    assert len(Notification.query.all()) == 0
-
-
+@freeze_time("2016-01-10 12:00:00.000000")
 def test_should_delete_notifications_after_seven_days(notify_db, notify_db_session):
-    created_at = datetime.utcnow() - timedelta(hours=24 * 7)
-    sample_notification(notify_db, notify_db_session, created_at=created_at, status="failed")
-    sample_notification(notify_db, notify_db_session, created_at=created_at, status="failed")
-    assert len(Notification.query.all()) == 2
-    delete_notifications_created_more_than_a_week_ago('failed')
+
     assert len(Notification.query.all()) == 0
 
+    # create one notification a day between 1st and 9th from 11:00 to 19:00
+    for i in range(1, 11):
+        past_date = '2016-01-{0:02d}  {0:02d}:00:00.000000'.format(i, i)
+        with freeze_time(past_date):
+            sample_notification(notify_db, notify_db_session, created_at=datetime.utcnow(), status="failed")
 
-def test_should_not_delete_sent_notifications_before_one_day(notify_db, notify_db_session):
-    expired = datetime.utcnow() - timedelta(hours=24)
-    valid = datetime.utcnow() - timedelta(hours=23, minutes=59, seconds=59)
-    sample_notification(notify_db, notify_db_session, created_at=expired, to_field="expired")
-    sample_notification(notify_db, notify_db_session, created_at=valid, to_field="valid")
+    all_notifications = Notification.query.all()
+    assert len(all_notifications) == 10
 
-    assert len(Notification.query.all()) == 2
-    delete_notifications_created_more_than_a_day_ago('sending')
-    assert len(Notification.query.all()) == 1
-    assert Notification.query.first().to == 'valid'
+    # Records from before 3rd should be deleted
+    delete_notifications_created_more_than_a_week_ago('failed')
+    remaining_notifications = Notification.query.all()
+    assert len(remaining_notifications) == 8
+    assert remaining_notifications[0].created_at.date() == date(2016, 1, 3)
 
 
 def test_should_not_delete_failed_notifications_before_seven_days(notify_db, notify_db_session):
-    expired = datetime.utcnow() - timedelta(hours=24 * 7)
-    valid = datetime.utcnow() - timedelta(hours=(24 * 6) + 23, minutes=59, seconds=59)
-    sample_notification(notify_db, notify_db_session, created_at=expired, status="failed", to_field="expired")
-    sample_notification(notify_db, notify_db_session, created_at=valid, status="failed", to_field="valid")
+    should_delete = datetime.utcnow() - timedelta(days=8)
+    do_not_delete = datetime.utcnow() - timedelta(days=7)
+    sample_notification(notify_db, notify_db_session, created_at=should_delete, status="failed",
+                        to_field="should_delete")
+    sample_notification(notify_db, notify_db_session, created_at=do_not_delete, status="failed",
+                        to_field="do_not_delete")
     assert len(Notification.query.all()) == 2
     delete_notifications_created_more_than_a_week_ago('failed')
     assert len(Notification.query.all()) == 1
-    assert Notification.query.first().to == 'valid'
+    assert Notification.query.first().to == 'do_not_delete'
 
 
 @freeze_time("2016-03-30")