Merge pull request #980 from alphagov/change-delete-jobs-to-delete-by-type

Change delete jobs to delete by type

app/celery/scheduled_tasks.py

@@ -12,10 +12,9 @@ from app import performance_platform_client
 from app.dao.invited_user_dao import delete_invitations_created_more_than_two_days_ago
 from app.dao.jobs_dao import dao_set_scheduled_jobs_to_pending, dao_get_jobs_older_than_limited_by
 from app.dao.notifications_dao import (
-    delete_notifications_created_more_than_a_week_ago,
     dao_timeout_notifications,
-    is_delivery_slow_for_provider
+    is_delivery_slow_for_provider,
-)
+    delete_notifications_created_more_than_a_week_ago_by_type)
 from app.dao.statistics_dao import dao_timeout_job_statistics
 from app.dao.provider_details_dao import (
     get_current_provider,
@@ -61,42 +60,60 @@ def delete_verify_codes():
         raise
 
 
-@notify_celery.task(name="delete-successful-notifications")
+@notify_celery.task(name="delete-sms-notifications")
 @statsd(namespace="tasks")
-def delete_successful_notifications():
+def delete_sms_notifications_older_than_seven_days():
     try:
         start = datetime.utcnow()
-        deleted = delete_notifications_created_more_than_a_week_ago('delivered')
+        deleted = delete_notifications_created_more_than_a_week_ago_by_type('sms')
         current_app.logger.info(
-            "Delete job started {} finished {} deleted {} successful notifications".format(
+            "Delete {} job started {} finished {} deleted {} sms notifications".format(
+                'sms',
                 start,
                 datetime.utcnow(),
                 deleted
             )
         )
     except SQLAlchemyError as e:
-        current_app.logger.exception("Failed to delete successful notifications")
+        current_app.logger.exception("Failed to delete sms notifications")
         raise
 
 
-@notify_celery.task(name="delete-failed-notifications")
+@notify_celery.task(name="delete-email-notifications")
 @statsd(namespace="tasks")
-def delete_failed_notifications():
+def delete_email_notifications_older_than_seven_days():
     try:
         start = datetime.utcnow()
-        deleted = delete_notifications_created_more_than_a_week_ago('failed')
+        deleted = delete_notifications_created_more_than_a_week_ago_by_type('email')
-        deleted += delete_notifications_created_more_than_a_week_ago('technical-failure')
-        deleted += delete_notifications_created_more_than_a_week_ago('temporary-failure')
-        deleted += delete_notifications_created_more_than_a_week_ago('permanent-failure')
         current_app.logger.info(
-            "Delete job started {} finished {} deleted {} failed notifications".format(
+            "Delete {} job started {} finished {} deleted {} email notifications".format(
+                'email',
                 start,
                 datetime.utcnow(),
                 deleted
             )
         )
     except SQLAlchemyError as e:
-        current_app.logger.exception("Failed to delete failed notifications")
+        current_app.logger.exception("Failed to delete sms notifications")
+        raise
+
+
+@notify_celery.task(name="delete-letter-notifications")
+@statsd(namespace="tasks")
+def delete_letter_notifications_older_than_seven_days():
+    try:
+        start = datetime.utcnow()
+        deleted = delete_notifications_created_more_than_a_week_ago_by_type('letter')
+        current_app.logger.info(
+            "Delete {} job started {} finished {} deleted {} letter notifications".format(
+                'letter',
+                start,
+                datetime.utcnow(),
+                deleted
+            )
+        )
+    except SQLAlchemyError as e:
+        current_app.logger.exception("Failed to delete sms notifications")
         raise
 
 

app/config.py

@@ -119,14 +119,19 @@ class Config(object):
             'schedule': timedelta(minutes=66),
             'options': {'queue': 'periodic'}
         },
-        'delete-failed-notifications': {
+        'delete-sms-notifications': {
-            'task': 'delete-failed-notifications',
+            'task': 'delete-sms-notifications',
             'schedule': crontab(minute=0, hour=0),
             'options': {'queue': 'periodic'}
         },
-        'delete-successful-notifications': {
+        'delete-email-notifications': {
-            'task': 'delete-successful-notifications',
+            'task': 'delete-email-notifications',
-            'schedule': crontab(minute=0, hour=1),
+            'schedule': crontab(minute=20, hour=0),
+            'options': {'queue': 'periodic'}
+        },
+        'delete-letter-notifications': {
+            'task': 'delete-letter-notifications',
+            'schedule': crontab(minute=40, hour=0),
             'options': {'queue': 'periodic'}
         },
         'send-daily-performance-platform-stats': {

app/dao/notifications_dao.py

@@ -351,11 +351,11 @@ def _filter_query(query, filter_dict=None):
 
 
 @statsd(namespace="dao")
-def delete_notifications_created_more_than_a_week_ago(status):
+def delete_notifications_created_more_than_a_week_ago_by_type(notification_type):
     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.notification_type == notification_type,
     ).delete(synchronize_session='fetch')
     db.session.commit()
     return deleted

tests/app/celery/test_scheduled_tasks.py

@@ -5,13 +5,13 @@ from functools import partial
 
 from flask import current_app
 from freezegun import freeze_time
-from app.celery.scheduled_tasks import s3, timeout_job_statistics
+from app.celery.scheduled_tasks import s3, timeout_job_statistics, delete_sms_notifications_older_than_seven_days, \
+    delete_letter_notifications_older_than_seven_days, delete_email_notifications_older_than_seven_days
 from app.celery import scheduled_tasks
 from app.celery.scheduled_tasks import (
     delete_verify_codes,
     remove_csv_files,
-    delete_successful_notifications,
+    delete_notifications_created_more_than_a_week_ago_by_type,
-    delete_failed_notifications,
     delete_invitations,
     timeout_notifications,
     run_scheduled_jobs,
@@ -70,8 +70,7 @@ def prepare_current_provider(restore_provider_details):
 
 def test_should_have_decorated_tasks_functions():
     assert delete_verify_codes.__wrapped__.__name__ == 'delete_verify_codes'
-    assert delete_successful_notifications.__wrapped__.__name__ == 'delete_successful_notifications'
+    assert delete_notifications_created_more_than_a_week_ago_by_type.__wrapped__.__name__ == 'delete_notifications_created_more_than_a_week_ago_by_type'  # noqa
-    assert delete_failed_notifications.__wrapped__.__name__ == 'delete_failed_notifications'
     assert timeout_notifications.__wrapped__.__name__ == 'timeout_notifications'
     assert delete_invitations.__wrapped__.__name__ == 'delete_invitations'
     assert run_scheduled_jobs.__wrapped__.__name__ == 'run_scheduled_jobs'
@@ -81,16 +80,22 @@ def test_should_have_decorated_tasks_functions():
         'switch_current_sms_provider_on_slow_delivery'
 
 
-def test_should_call_delete_successful_notifications_more_than_week_in_task(notify_api, mocker):
+def test_should_call_delete_sms_notifications_more_than_week_in_task(notify_api, mocker):
-    mocked = mocker.patch('app.celery.scheduled_tasks.delete_notifications_created_more_than_a_week_ago')
+    mocked = mocker.patch('app.celery.scheduled_tasks.delete_notifications_created_more_than_a_week_ago_by_type')
-    delete_successful_notifications()
+    delete_sms_notifications_older_than_seven_days()
-    mocked.assert_called_once_with('delivered')
+    mocked.assert_called_once_with('sms')
 
 
-def test_should_call_delete_failed_notifications_more_than_week_in_task(notify_api, mocker):
+def test_should_call_delete_email_notifications_more_than_week_in_task(notify_api, mocker):
-    mocker.patch('app.celery.scheduled_tasks.delete_notifications_created_more_than_a_week_ago')
+    mocked = mocker.patch('app.celery.scheduled_tasks.delete_notifications_created_more_than_a_week_ago_by_type')
-    delete_failed_notifications()
+    delete_email_notifications_older_than_seven_days()
-    assert scheduled_tasks.delete_notifications_created_more_than_a_week_ago.call_count == 4
+    mocked.assert_called_once_with('email')
+
+
+def test_should_call_delete_letter_notifications_more_than_week_in_task(notify_api, mocker):
+    mocked = mocker.patch('app.celery.scheduled_tasks.delete_notifications_created_more_than_a_week_ago_by_type')
+    delete_letter_notifications_older_than_seven_days()
+    mocked.assert_called_once_with('letter')
 
 
 def test_should_call_delete_codes_on_delete_verify_codes_task(notify_api, mocker):

tests/app/dao/test_notification_dao.py

@@ -17,7 +17,8 @@ from app.models import (
     NOTIFICATION_SENT,
     KEY_TYPE_NORMAL,
     KEY_TYPE_TEAM,
-    KEY_TYPE_TEST)
+    KEY_TYPE_TEST
+)
 
 from app.dao.notifications_dao import (
     dao_create_notification,
@@ -26,7 +27,7 @@ from app.dao.notifications_dao import (
     dao_get_potential_notification_statistics_for_day,
     dao_get_template_usage,
     dao_update_notification,
-    delete_notifications_created_more_than_a_week_ago,
+    delete_notifications_created_more_than_a_week_ago_by_type,
     get_notification_by_id,
     get_notification_for_job,
     get_notification_billable_unit_count_per_month,
@@ -49,8 +50,8 @@ from tests.app.conftest import (
     sample_email_template,
     sample_service,
     sample_job,
-    sample_notification_history as create_notification_history
+    sample_notification_history as create_notification_history,
-)
+    sample_letter_template)
 
 
 def test_should_have_decorated_notifications_dao_functions():
@@ -66,7 +67,7 @@ def test_should_have_decorated_notifications_dao_functions():
     assert get_notification_with_personalisation.__wrapped__.__name__ == 'get_notification_with_personalisation'  # noqa
     assert get_notifications_for_service.__wrapped__.__name__ == 'get_notifications_for_service'  # noqa
     assert get_notification_by_id.__wrapped__.__name__ == 'get_notification_by_id'  # noqa
-    assert delete_notifications_created_more_than_a_week_ago.__wrapped__.__name__ == 'delete_notifications_created_more_than_a_week_ago'  # noqa
+    assert delete_notifications_created_more_than_a_week_ago_by_type.__wrapped__.__name__ == 'delete_notifications_created_more_than_a_week_ago_by_type'  # noqa
     assert dao_delete_notifications_and_history_by_id.__wrapped__.__name__ == 'dao_delete_notifications_and_history_by_id'  # noqa
 
 
@@ -868,63 +869,118 @@ def test_updating_notification_with_no_notification_status_updates_notification_
     assert hist_from_db._status_fkey == 'failed'
 
 
+@pytest.mark.parametrize('notification_type, expected_sms_count, expected_email_count, expected_letter_count', [
+    ('sms', 8, 10, 10), ('email', 10, 8, 10), ('letter', 10, 10, 8)
+])
 @freeze_time("2016-01-10 12:00:00.000000")
-def test_should_delete_notifications_after_seven_days(notify_db, notify_db_session):
+def test_should_delete_notifications_by_type_after_seven_days(
+        notify_db,
+        notify_db_session,
+        sample_service,
+        notification_type,
+        expected_sms_count,
+        expected_email_count,
+        expected_letter_count
+):
     assert len(Notification.query.all()) == 0
 
-    # create one notification a day between 1st and 10th from 11:00 to 19:00
+    email_template = sample_email_template(notify_db, notify_db_session, service=sample_service)
+    sms_template = sample_template(notify_db, notify_db_session, service=sample_service)
+    letter_template = sample_letter_template(sample_service)
+
+    # create one notification a day between 1st and 10th from 11:00 to 19:00 of each type
     for i in range(1, 11):
         past_date = '2016-01-{0:02d}  {0:02d}:00:00.000000'.format(i)
         with freeze_time(past_date):
-            sample_notification(notify_db, notify_db_session, created_at=datetime.utcnow(), status="failed")
+            sample_notification(
+                notify_db,
+                notify_db_session,
+                created_at=datetime.utcnow(),
+                status="failed",
+                service=sample_service,
+                template=email_template
+            )
+            sample_notification(
+                notify_db,
+                notify_db_session,
+                created_at=datetime.utcnow(),
+                status="failed",
+                service=sample_service,
+                template=sms_template
+            )
+            sample_notification(
+                notify_db,
+                notify_db_session,
+                created_at=datetime.utcnow(),
+                status="failed",
+                service=sample_service,
+                template=letter_template
+            )
 
     all_notifications = Notification.query.all()
-    assert len(all_notifications) == 10
+    assert len(all_notifications) == 30
 
     # Records from before 3rd should be deleted
-    delete_notifications_created_more_than_a_week_ago('failed')
+    delete_notifications_created_more_than_a_week_ago_by_type(notification_type)
-    remaining_notifications = Notification.query.all()
+    remaining_sms_notifications = Notification.query.filter_by(notification_type='sms').all()
-    assert len(remaining_notifications) == 8
+    remaining_letter_notifications = Notification.query.filter_by(notification_type='letter').all()
-    for notification in remaining_notifications:
+    remaining_email_notifications = Notification.query.filter_by(notification_type='email').all()
+
+    assert len(remaining_sms_notifications) == expected_sms_count
+    assert len(remaining_email_notifications) == expected_email_count
+    assert len(remaining_letter_notifications) == expected_letter_count
+
+    if notification_type == 'sms':
+        notifications_to_check = remaining_sms_notifications
+    if notification_type == 'email':
+        notifications_to_check = remaining_email_notifications
+    if notification_type == 'letter':
+        notifications_to_check = remaining_letter_notifications
+
+    for notification in notifications_to_check:
         assert notification.created_at.date() >= date(2016, 1, 3)
 
 
+@pytest.mark.parametrize('notification_type', ['sms', 'email', 'letter'])
 @freeze_time("2016-01-10 12:00:00.000000")
-def test_should_not_delete_notification_history(notify_db, notify_db_session):
+def test_should_not_delete_notification_history(notify_db, notify_db_session, sample_service, notification_type):
     with freeze_time('2016-01-01 12:00'):
-        notification = sample_notification(notify_db, notify_db_session, created_at=datetime.utcnow(), status="failed")
+        email_template = sample_email_template(notify_db, notify_db_session, service=sample_service)
-        notification_id = notification.id
+        sms_template = sample_template(notify_db, notify_db_session, service=sample_service)
+        letter_template = sample_letter_template(sample_service)
 
-    assert Notification.query.count() == 1
+        sample_notification(
-    assert NotificationHistory.query.count() == 1
+            notify_db,
+            notify_db_session,
+            created_at=datetime.utcnow(),
+            status="failed",
+            service=sample_service,
+            template=email_template
+        )
+        sample_notification(
+            notify_db,
+            notify_db_session,
+            created_at=datetime.utcnow(),
+            status="failed",
+            service=sample_service,
+            template=sms_template
+        )
+        sample_notification(
+            notify_db,
+            notify_db_session,
+            created_at=datetime.utcnow(),
+            status="failed",
+            service=sample_service,
+            template=letter_template
+        )
 
-    delete_notifications_created_more_than_a_week_ago('failed')
+    assert Notification.query.count() == 3
+    assert NotificationHistory.query.count() == 3
 
-    assert Notification.query.count() == 0
+    delete_notifications_created_more_than_a_week_ago_by_type(notification_type)
-    assert NotificationHistory.query.count() == 1
-    assert NotificationHistory.query.one().id == notification_id
 
-
+    assert Notification.query.count() == 2
-def test_should_not_delete_failed_notifications_before_seven_days(notify_db, notify_db_session):
+    assert NotificationHistory.query.count() == 3
-    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 == '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")