From 2c46f201b5342d023db751856d81d9ef07a80650 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 9 Oct 2017 14:51:27 +0100 Subject: [PATCH 1/4] Purge noti to email reply to older than 7 days --- app/celery/scheduled_tasks.py | 12 ++++ app/dao/notifications_dao.py | 21 +++++- tests/app/celery/test_scheduled_tasks.py | 8 ++- .../notification_dao/test_notification_dao.py | 72 ++++++++++++++----- 4 files changed, 92 insertions(+), 21 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index f014c65ed..27bfe5c62 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -27,6 +27,7 @@ from app.dao.notifications_dao import ( dao_timeout_notifications, is_delivery_slow_for_provider, delete_notifications_created_more_than_a_week_ago_by_type, + delete_notification_to_email_reply_to_more_than_a_week_ago, dao_get_scheduled_notifications, set_scheduled_notification_to_processed, dao_set_created_live_letter_api_notifications_to_pending, @@ -118,6 +119,17 @@ def delete_sms_notifications_older_than_seven_days(): @statsd(namespace="tasks") def delete_email_notifications_older_than_seven_days(): try: + start = datetime.utcnow() + deleted_noti_to_reply_to = delete_notification_to_email_reply_to_more_than_a_week_ago() + current_app.logger.info( + "Delete {} job started {} finished {} deleted {} notification to email reply to mappings".format( + 'email', + start, + datetime.utcnow(), + deleted_noti_to_reply_to + ) + ) + start = datetime.utcnow() deleted = delete_notifications_created_more_than_a_week_ago_by_type('email') current_app.logger.info( diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 4183c6f4c..01c843f9e 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -26,14 +26,15 @@ from app.dao.date_util import get_financial_year from app.models import ( Service, Notification, + NotificationEmailReplyTo, NotificationHistory, NotificationStatistics, - NotificationEmailReplyTo, - ServiceEmailReplyTo, ScheduledNotification, + ServiceEmailReplyTo, Template, KEY_TYPE_NORMAL, KEY_TYPE_TEST, + EMAIL_TYPE, LETTER_TYPE, NOTIFICATION_CREATED, NOTIFICATION_DELIVERED, @@ -376,6 +377,22 @@ def delete_notifications_created_more_than_a_week_ago_by_type(notification_type) return deleted +@statsd(namespace="dao") +def delete_notification_to_email_reply_to_more_than_a_week_ago(): + seven_days_ago = date.today() - timedelta(days=7) + + subq = db.session.query(Notification.id).filter( + func.date(Notification.created_at) < seven_days_ago + ).subquery() + deleted = db.session.query( + NotificationEmailReplyTo + ).filter( + NotificationEmailReplyTo.notification_id.in_(subq) + ).delete(synchronize_session='fetch') + db.session.commit() + return deleted + + @statsd(namespace="dao") @transactional def dao_delete_notifications_and_history_by_id(notification_id): diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 9d770888b..f816948d7 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -126,9 +126,13 @@ def test_should_call_delete_sms_notifications_more_than_week_in_task(notify_api, def test_should_call_delete_email_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') + mocked_delete_noti_to_reply_to = mocker.patch( + 'app.celery.scheduled_tasks.delete_notification_to_email_reply_to_more_than_a_week_ago') + mocked_notifications = mocker.patch( + 'app.celery.scheduled_tasks.delete_notifications_created_more_than_a_week_ago_by_type') delete_email_notifications_older_than_seven_days() - mocked.assert_called_once_with('email') + mocked_delete_noti_to_reply_to.assert_called_once_with() + mocked_notifications.assert_called_once_with('email') def test_should_call_delete_letter_notifications_more_than_week_in_task(notify_api, mocker): diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 3124b3a13..86f99480b 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -6,49 +6,51 @@ import pytest from freezegun import freeze_time from sqlalchemy.exc import SQLAlchemyError, IntegrityError -from app.dao.service_email_reply_to_dao import dao_get_reply_to_by_service_id +from app.dao.service_email_reply_to_dao import dao_create_reply_to_email_address, dao_get_reply_to_by_service_id from app.models import ( - Notification, - NotificationHistory, Job, + Notification, NotificationEmailReplyTo, + NotificationHistory, NotificationStatistics, ScheduledNotification, + ServiceEmailReplyTo, + EMAIL_TYPE, NOTIFICATION_STATUS_TYPES, NOTIFICATION_STATUS_TYPES_FAILED, NOTIFICATION_SENT, NOTIFICATION_DELIVERED, KEY_TYPE_NORMAL, KEY_TYPE_TEAM, - KEY_TYPE_TEST, -) + KEY_TYPE_TEST) from app.dao.notifications_dao import ( dao_create_notification, - dao_create_notification_email_reply_to_mapping, - dao_created_scheduled_notification, - dao_delete_notifications_and_history_by_id, - dao_get_notifications_by_to_field, dao_get_last_template_usage, - dao_get_notification_email_reply_for_notification, dao_get_notification_statistics_for_service_and_day, dao_get_potential_notification_statistics_for_day, - dao_get_scheduled_notifications, dao_get_template_usage, - dao_timeout_notifications, dao_update_notification, - dao_update_notifications_for_job_to_sent_to_dvla, delete_notifications_created_more_than_a_week_ago_by_type, + delete_notification_to_email_reply_to_more_than_a_week_ago, get_notification_by_id, get_notification_for_job, get_notification_with_personalisation, get_notifications_for_job, get_notifications_for_service, get_total_sent_notifications_in_date_range, - is_delivery_slow_for_provider, - set_scheduled_notification_to_processed, update_notification_status_by_id, - update_notification_status_by_reference + update_notification_status_by_reference, + dao_delete_notifications_and_history_by_id, + dao_timeout_notifications, + is_delivery_slow_for_provider, + dao_update_notifications_for_job_to_sent_to_dvla, + dao_get_notifications_by_to_field, + dao_created_scheduled_notification, + dao_get_scheduled_notifications, + set_scheduled_notification_to_processed, + dao_get_notification_email_reply_for_notification, + dao_create_notification_email_reply_to_mapping ) from app.dao.services_dao import dao_update_service @@ -1000,6 +1002,42 @@ def test_should_delete_notifications_by_type_after_seven_days( assert notification.created_at.date() >= date(2016, 1, 3) +@freeze_time("2016-01-10 12:00:00.000000") +def test_should_delete_notification_to_email_reply_to_after_seven_days( + notify_db, notify_db_session, sample_service, +): + assert len(Notification.query.all()) == 0 + + reply_to = create_reply_to_email(sample_service, 'test@example.com') + + email_template = sample_email_template(notify_db, notify_db_session, service=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): + notification = create_notification(email_template) + dao_create_notification_email_reply_to_mapping(notification.id, reply_to.id) + + all_notifications = Notification.query.all() + assert len(all_notifications) == 10 + + all_notification_email_reply_to = NotificationEmailReplyTo.query.all() + assert len(all_notification_email_reply_to) == 10 + + # Records before 3rd should be deleted + delete_notification_to_email_reply_to_more_than_a_week_ago() + delete_notifications_created_more_than_a_week_ago_by_type(EMAIL_TYPE) + remaining_email_notifications = Notification.query.filter_by(notification_type=EMAIL_TYPE).all() + remaining_notification_to_email_reply_to = NotificationEmailReplyTo.query.filter_by().all() + + assert len(remaining_email_notifications) == 8 + assert len(remaining_notification_to_email_reply_to) == 8 + + for notification in remaining_email_notifications: + 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, sample_service, notification_type): @@ -1987,7 +2025,7 @@ def test_dao_create_multiple_notification_email_reply_to_mapping(sample_service, assert email_reply_to[0].service_email_reply_to_id == reply_to_address.id -def test_dao_get_notification_ememail_reply_toail_reply_for_notification(sample_service, sample_notification): +def test_dao_get_notification_email_reply_for_notification(sample_service, sample_notification): reply_to_address = create_reply_to_email(sample_service, "test@test.com") dao_create_notification_email_reply_to_mapping(sample_notification.id, reply_to_address.id) assert dao_get_notification_email_reply_for_notification(sample_notification.id) == "test@test.com" From b7bc04f47b63f409e95004400d64ed90b4ce5d14 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 9 Oct 2017 15:02:15 +0100 Subject: [PATCH 2/4] Removed redundant import --- tests/app/dao/notification_dao/test_notification_dao.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 86f99480b..881b8105b 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -6,7 +6,7 @@ import pytest from freezegun import freeze_time from sqlalchemy.exc import SQLAlchemyError, IntegrityError -from app.dao.service_email_reply_to_dao import dao_create_reply_to_email_address, dao_get_reply_to_by_service_id +from app.dao.service_email_reply_to_dao import dao_get_reply_to_by_service_id from app.models import ( Job, Notification, From b77a6066993f7098bc3897472eb5a88607030d78 Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Mon, 9 Oct 2017 15:10:59 +0100 Subject: [PATCH 3/4] Reorder imports --- .../notification_dao/test_notification_dao.py | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 881b8105b..4738e8577 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -22,15 +22,24 @@ from app.models import ( NOTIFICATION_DELIVERED, KEY_TYPE_NORMAL, KEY_TYPE_TEAM, - KEY_TYPE_TEST) + KEY_TYPE_TEST +) from app.dao.notifications_dao import ( dao_create_notification, + dao_create_notification_email_reply_to_mapping, + dao_created_scheduled_notification, + dao_delete_notifications_and_history_by_id, dao_get_last_template_usage, + dao_get_notification_email_reply_for_notification, + dao_get_notifications_by_to_field, dao_get_notification_statistics_for_service_and_day, dao_get_potential_notification_statistics_for_day, + dao_get_scheduled_notifications, dao_get_template_usage, + dao_timeout_notifications, dao_update_notification, + dao_update_notifications_for_job_to_sent_to_dvla, delete_notifications_created_more_than_a_week_ago_by_type, delete_notification_to_email_reply_to_more_than_a_week_ago, get_notification_by_id, @@ -39,18 +48,10 @@ from app.dao.notifications_dao import ( get_notifications_for_job, get_notifications_for_service, get_total_sent_notifications_in_date_range, - update_notification_status_by_id, - update_notification_status_by_reference, - dao_delete_notifications_and_history_by_id, - dao_timeout_notifications, is_delivery_slow_for_provider, - dao_update_notifications_for_job_to_sent_to_dvla, - dao_get_notifications_by_to_field, - dao_created_scheduled_notification, - dao_get_scheduled_notifications, set_scheduled_notification_to_processed, - dao_get_notification_email_reply_for_notification, - dao_create_notification_email_reply_to_mapping + update_notification_status_by_id, + update_notification_status_by_reference ) from app.dao.services_dao import dao_update_service From 1eaf6e089b0be577f8b15502fc85c9e9dbd8434b Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Wed, 11 Oct 2017 12:26:58 +0100 Subject: [PATCH 4/4] Refactor code to make it clearer - Added code in `delete_notifications_created_more_than_a_week_ago_by_type` to remove notifications to email_reply_to older than 7 days - Added transactional to `delete_notifications_created_more_than_a_week_ago_by_type` --- app/celery/scheduled_tasks.py | 12 ------- app/dao/notifications_dao.py | 33 +++++++++---------- tests/app/celery/test_scheduled_tasks.py | 3 -- .../notification_dao/test_notification_dao.py | 2 -- 4 files changed, 15 insertions(+), 35 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index 27bfe5c62..f014c65ed 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -27,7 +27,6 @@ from app.dao.notifications_dao import ( dao_timeout_notifications, is_delivery_slow_for_provider, delete_notifications_created_more_than_a_week_ago_by_type, - delete_notification_to_email_reply_to_more_than_a_week_ago, dao_get_scheduled_notifications, set_scheduled_notification_to_processed, dao_set_created_live_letter_api_notifications_to_pending, @@ -119,17 +118,6 @@ def delete_sms_notifications_older_than_seven_days(): @statsd(namespace="tasks") def delete_email_notifications_older_than_seven_days(): try: - start = datetime.utcnow() - deleted_noti_to_reply_to = delete_notification_to_email_reply_to_more_than_a_week_ago() - current_app.logger.info( - "Delete {} job started {} finished {} deleted {} notification to email reply to mappings".format( - 'email', - start, - datetime.utcnow(), - deleted_noti_to_reply_to - ) - ) - start = datetime.utcnow() deleted = delete_notifications_created_more_than_a_week_ago_by_type('email') current_app.logger.info( diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 01c843f9e..1487cf94e 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -32,9 +32,9 @@ from app.models import ( ScheduledNotification, ServiceEmailReplyTo, Template, + EMAIL_TYPE, KEY_TYPE_NORMAL, KEY_TYPE_TEST, - EMAIL_TYPE, LETTER_TYPE, NOTIFICATION_CREATED, NOTIFICATION_DELIVERED, @@ -367,29 +367,26 @@ def _filter_query(query, filter_dict=None): @statsd(namespace="dao") +@transactional def delete_notifications_created_more_than_a_week_ago_by_type(notification_type): seven_days_ago = date.today() - timedelta(days=7) + + # Following could be refactored when NotificationSmsReplyTo and NotificationLetterContact in models.py + if notification_type == EMAIL_TYPE: + subq = db.session.query(Notification.id).filter( + func.date(Notification.created_at) < seven_days_ago, + Notification.notification_type == notification_type + ).subquery() + deleted = db.session.query( + NotificationEmailReplyTo + ).filter( + NotificationEmailReplyTo.notification_id.in_(subq) + ).delete(synchronize_session='fetch') + deleted = db.session.query(Notification).filter( func.date(Notification.created_at) < seven_days_ago, Notification.notification_type == notification_type, ).delete(synchronize_session='fetch') - db.session.commit() - return deleted - - -@statsd(namespace="dao") -def delete_notification_to_email_reply_to_more_than_a_week_ago(): - seven_days_ago = date.today() - timedelta(days=7) - - subq = db.session.query(Notification.id).filter( - func.date(Notification.created_at) < seven_days_ago - ).subquery() - deleted = db.session.query( - NotificationEmailReplyTo - ).filter( - NotificationEmailReplyTo.notification_id.in_(subq) - ).delete(synchronize_session='fetch') - db.session.commit() return deleted diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index f816948d7..60be9c2cd 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -126,12 +126,9 @@ def test_should_call_delete_sms_notifications_more_than_week_in_task(notify_api, def test_should_call_delete_email_notifications_more_than_week_in_task(notify_api, mocker): - mocked_delete_noti_to_reply_to = mocker.patch( - 'app.celery.scheduled_tasks.delete_notification_to_email_reply_to_more_than_a_week_ago') mocked_notifications = mocker.patch( 'app.celery.scheduled_tasks.delete_notifications_created_more_than_a_week_ago_by_type') delete_email_notifications_older_than_seven_days() - mocked_delete_noti_to_reply_to.assert_called_once_with() mocked_notifications.assert_called_once_with('email') diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 4738e8577..b6901ec86 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -41,7 +41,6 @@ from app.dao.notifications_dao import ( dao_update_notification, dao_update_notifications_for_job_to_sent_to_dvla, delete_notifications_created_more_than_a_week_ago_by_type, - delete_notification_to_email_reply_to_more_than_a_week_ago, get_notification_by_id, get_notification_for_job, get_notification_with_personalisation, @@ -1027,7 +1026,6 @@ def test_should_delete_notification_to_email_reply_to_after_seven_days( assert len(all_notification_email_reply_to) == 10 # Records before 3rd should be deleted - delete_notification_to_email_reply_to_more_than_a_week_ago() delete_notifications_created_more_than_a_week_ago_by_type(EMAIL_TYPE) remaining_email_notifications = Notification.query.filter_by(notification_type=EMAIL_TYPE).all() remaining_notification_to_email_reply_to = NotificationEmailReplyTo.query.filter_by().all()