diff --git a/app/celery/nightly_tasks.py b/app/celery/nightly_tasks.py index e452befd1..56593ab1d 100644 --- a/app/celery/nightly_tasks.py +++ b/app/celery/nightly_tasks.py @@ -16,14 +16,14 @@ from app.celery.service_callback_tasks import ( create_delivery_status_callback_data, ) from app.config import QueueNames -from app.dao.inbound_sms_dao import delete_inbound_sms_created_more_than_a_week_ago +from app.dao.inbound_sms_dao import delete_inbound_sms_older_than_retention from app.dao.jobs_dao import ( dao_get_jobs_older_than_data_retention, dao_archive_job ) from app.dao.notifications_dao import ( dao_timeout_notifications, - delete_notifications_created_more_than_a_week_ago_by_type, + delete_notifications_older_than_retention_by_type, ) from app.dao.service_callback_api_dao import get_service_delivery_status_callback_api_for_service from app.exceptions import NotificationTechnicalFailureException @@ -64,10 +64,10 @@ def _remove_csv_files(job_types): @notify_celery.task(name="delete-sms-notifications") @cronitor("delete-sms-notifications") @statsd(namespace="tasks") -def delete_sms_notifications_older_than_seven_days(): +def delete_sms_notifications_older_than_retention(): try: start = datetime.utcnow() - deleted = delete_notifications_created_more_than_a_week_ago_by_type('sms') + deleted = delete_notifications_older_than_retention_by_type('sms') current_app.logger.info( "Delete {} job started {} finished {} deleted {} sms notifications".format( 'sms', @@ -84,10 +84,10 @@ def delete_sms_notifications_older_than_seven_days(): @notify_celery.task(name="delete-email-notifications") @cronitor("delete-email-notifications") @statsd(namespace="tasks") -def delete_email_notifications_older_than_seven_days(): +def delete_email_notifications_older_than_retention(): try: start = datetime.utcnow() - deleted = delete_notifications_created_more_than_a_week_ago_by_type('email') + deleted = delete_notifications_older_than_retention_by_type('email') current_app.logger.info( "Delete {} job started {} finished {} deleted {} email notifications".format( 'email', @@ -104,10 +104,10 @@ def delete_email_notifications_older_than_seven_days(): @notify_celery.task(name="delete-letter-notifications") @cronitor("delete-letter-notifications") @statsd(namespace="tasks") -def delete_letter_notifications_older_than_seven_days(): +def delete_letter_notifications_older_than_retention(): try: start = datetime.utcnow() - deleted = delete_notifications_created_more_than_a_week_ago_by_type('letter') + deleted = delete_notifications_older_than_retention_by_type('letter') current_app.logger.info( "Delete {} job started {} finished {} deleted {} letter notifications".format( 'letter', @@ -190,10 +190,10 @@ def send_total_sent_notifications_to_performance_platform(day): @notify_celery.task(name="delete-inbound-sms") @cronitor("delete-inbound-sms") @statsd(namespace="tasks") -def delete_inbound_sms_older_than_seven_days(): +def delete_inbound_sms(): try: start = datetime.utcnow() - deleted = delete_inbound_sms_created_more_than_a_week_ago() + deleted = delete_inbound_sms_older_than_retention() current_app.logger.info( "Delete inbound sms job started {} finished {} deleted {} inbound sms notifications".format( start, diff --git a/app/dao/inbound_sms_dao.py b/app/dao/inbound_sms_dao.py index 22b0d468b..0b6334479 100644 --- a/app/dao/inbound_sms_dao.py +++ b/app/dao/inbound_sms_dao.py @@ -1,8 +1,3 @@ -from datetime import ( - timedelta, - datetime, - date -) from flask import current_app from notifications_utils.statsd_decorators import statsd from sqlalchemy import desc, and_ @@ -10,8 +5,8 @@ from sqlalchemy.orm import aliased from app import db from app.dao.dao_utils import transactional -from app.models import InboundSms -from app.utils import get_london_midnight_in_utc +from app.models import InboundSms, Service, ServiceDataRetention, SMS_TYPE +from app.utils import midnight_n_days_ago @transactional @@ -19,14 +14,15 @@ def dao_create_inbound_sms(inbound_sms): db.session.add(inbound_sms) -def dao_get_inbound_sms_for_service(service_id, limit=None, user_number=None): - start_date = get_london_midnight_in_utc(date.today() - timedelta(days=6)) +def dao_get_inbound_sms_for_service(service_id, limit=None, user_number=None, limit_days=7): q = InboundSms.query.filter( - InboundSms.service_id == service_id, - InboundSms.created_at >= start_date + InboundSms.service_id == service_id ).order_by( InboundSms.created_at.desc() ) + if limit_days is not None: + start_date = midnight_n_days_ago(limit_days) + q = q.filter(InboundSms.created_at >= start_date) if user_number: q = q.filter(InboundSms.user_number == user_number) @@ -60,21 +56,63 @@ def dao_get_paginated_inbound_sms_for_service_for_public_api( def dao_count_inbound_sms_for_service(service_id): - start_date = get_london_midnight_in_utc(date.today() - timedelta(days=6)) + start_date = midnight_n_days_ago(6) return InboundSms.query.filter( InboundSms.service_id == service_id, InboundSms.created_at >= start_date ).count() +def _delete_inbound_sms(datetime_to_delete_from, query_filter): + query_limit = 10000 + + subquery = db.session.query( + InboundSms.id + ).filter( + InboundSms.created_at < datetime_to_delete_from, + *query_filter + ).limit( + query_limit + ).subquery() + + deleted = 0 + # set to nonzero just to enter the loop + number_deleted = 1 + while number_deleted > 0: + number_deleted = InboundSms.query.filter(InboundSms.id.in_(subquery)).delete(synchronize_session='fetch') + deleted += number_deleted + + return deleted + + @statsd(namespace="dao") @transactional -def delete_inbound_sms_created_more_than_a_week_ago(): - seven_days_ago = datetime.utcnow() - timedelta(days=7) +def delete_inbound_sms_older_than_retention(): + current_app.logger.info('Deleting inbound sms for services with flexible data retention') - deleted = db.session.query(InboundSms).filter( - InboundSms.created_at < seven_days_ago - ).delete(synchronize_session='fetch') + flexible_data_retention = ServiceDataRetention.query.join( + ServiceDataRetention.service, + Service.inbound_number + ).filter( + ServiceDataRetention.notification_type == SMS_TYPE + ).all() + + deleted = 0 + for f in flexible_data_retention: + n_days_ago = midnight_n_days_ago(f.days_of_retention) + + current_app.logger.info("Deleting inbound sms for service id: {}".format(f.service_id)) + deleted += _delete_inbound_sms(n_days_ago, query_filter=[InboundSms.service_id == f.service_id]) + + current_app.logger.info('Deleting inbound sms for services without flexible data retention') + + seven_days_ago = midnight_n_days_ago(7) + + deleted += _delete_inbound_sms(seven_days_ago, query_filter=[ + InboundSms.service_id.notin_(x.service_id for x in flexible_data_retention), + ]) + + current_app.logger.info('Deleted {} inbound sms'.format(deleted)) return deleted @@ -108,7 +146,7 @@ def dao_get_paginated_most_recent_inbound_sms_by_user_number_for_service( ORDER BY t1.created_at DESC; LIMIT 50 OFFSET :page """ - start_date = get_london_midnight_in_utc(date.today() - timedelta(days=6)) + start_date = midnight_n_days_ago(6) t2 = aliased(InboundSms) q = db.session.query( InboundSms diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 722f03165..4b9b6c7e9 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -293,7 +293,7 @@ def _filter_query(query, filter_dict=None): @statsd(namespace="dao") -def delete_notifications_created_more_than_a_week_ago_by_type(notification_type, qry_limit=10000): +def delete_notifications_older_than_retention_by_type(notification_type, qry_limit=10000): current_app.logger.info( 'Deleting {} notifications for services with flexible data retention'.format(notification_type)) diff --git a/tests/app/celery/test_nightly_tasks.py b/tests/app/celery/test_nightly_tasks.py index 93e047448..179aa95ff 100644 --- a/tests/app/celery/test_nightly_tasks.py +++ b/tests/app/celery/test_nightly_tasks.py @@ -11,10 +11,10 @@ from notifications_utils.clients.zendesk.zendesk_client import ZendeskClient from app.celery import nightly_tasks from app.celery.nightly_tasks import ( delete_dvla_response_files_older_than_seven_days, - delete_email_notifications_older_than_seven_days, - delete_inbound_sms_older_than_seven_days, - delete_letter_notifications_older_than_seven_days, - delete_sms_notifications_older_than_seven_days, + delete_email_notifications_older_than_retention, + delete_inbound_sms, + delete_letter_notifications_older_than_retention, + delete_sms_notifications_older_than_retention, raise_alert_if_letter_notifications_still_sending, remove_letter_csv_files, remove_sms_email_csv_files, @@ -157,21 +157,21 @@ def test_remove_csv_files_filters_by_type(mocker, sample_service): def test_should_call_delete_sms_notifications_more_than_week_in_task(notify_api, mocker): - mocked = mocker.patch('app.celery.nightly_tasks.delete_notifications_created_more_than_a_week_ago_by_type') - delete_sms_notifications_older_than_seven_days() + mocked = mocker.patch('app.celery.nightly_tasks.delete_notifications_older_than_retention_by_type') + delete_sms_notifications_older_than_retention() mocked.assert_called_once_with('sms') def test_should_call_delete_email_notifications_more_than_week_in_task(notify_api, mocker): mocked_notifications = mocker.patch( - 'app.celery.nightly_tasks.delete_notifications_created_more_than_a_week_ago_by_type') - delete_email_notifications_older_than_seven_days() + 'app.celery.nightly_tasks.delete_notifications_older_than_retention_by_type') + delete_email_notifications_older_than_retention() mocked_notifications.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.nightly_tasks.delete_notifications_created_more_than_a_week_ago_by_type') - delete_letter_notifications_older_than_seven_days() + mocked = mocker.patch('app.celery.nightly_tasks.delete_notifications_older_than_retention_by_type') + delete_letter_notifications_older_than_retention() mocked.assert_called_once_with('letter') @@ -291,10 +291,10 @@ def test_send_total_sent_notifications_to_performance_platform_calls_with_correc ]) -def test_should_call_delete_inbound_sms_older_than_seven_days(notify_api, mocker): - mocker.patch('app.celery.nightly_tasks.delete_inbound_sms_created_more_than_a_week_ago') - delete_inbound_sms_older_than_seven_days() - assert nightly_tasks.delete_inbound_sms_created_more_than_a_week_ago.call_count == 1 +def test_should_call_delete_inbound_sms(notify_api, mocker): + mocker.patch('app.celery.nightly_tasks.delete_inbound_sms_older_than_retention') + delete_inbound_sms() + assert nightly_tasks.delete_inbound_sms_older_than_retention.call_count == 1 @freeze_time('2017-01-01 10:00:00') diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 0776a45fe..91df8ec72 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -19,7 +19,7 @@ from app.dao.notifications_dao import ( dao_timeout_notifications, dao_update_notification, dao_update_notifications_by_reference, - delete_notifications_created_more_than_a_week_ago_by_type, + delete_notifications_older_than_retention_by_type, get_notification_by_id, get_notification_for_job, get_notification_with_personalisation, @@ -79,7 +79,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_by_type.__wrapped__.__name__ == 'delete_notifications_created_more_than_a_week_ago_by_type' # noqa + assert delete_notifications_older_than_retention_by_type.__wrapped__.__name__ == 'delete_notifications_older_than_retention_by_type' # noqa assert dao_delete_notifications_and_history_by_id.__wrapped__.__name__ == 'dao_delete_notifications_and_history_by_id' # noqa diff --git a/tests/app/dao/notification_dao/test_notification_dao_delete_notifications.py b/tests/app/dao/notification_dao/test_notification_dao_delete_notifications.py index 4b3d8c9e8..3e913a673 100644 --- a/tests/app/dao/notification_dao/test_notification_dao_delete_notifications.py +++ b/tests/app/dao/notification_dao/test_notification_dao_delete_notifications.py @@ -6,7 +6,7 @@ from datetime import ( import pytest from flask import current_app from freezegun import freeze_time -from app.dao.notifications_dao import delete_notifications_created_more_than_a_week_ago_by_type +from app.dao.notifications_dao import delete_notifications_older_than_retention_by_type from app.models import Notification, NotificationHistory from tests.app.db import ( create_template, @@ -48,7 +48,7 @@ def test_should_delete_notifications_by_type_after_seven_days( assert len(all_notifications) == 30 # Records from before 3rd should be deleted with freeze_time(delete_run_time): - delete_notifications_created_more_than_a_week_ago_by_type(notification_type) + delete_notifications_older_than_retention_by_type(notification_type) remaining_sms_notifications = Notification.query.filter_by(notification_type='sms').all() remaining_letter_notifications = Notification.query.filter_by(notification_type='letter').all() remaining_email_notifications = Notification.query.filter_by(notification_type='email').all() @@ -77,7 +77,7 @@ def test_should_not_delete_notification_history(sample_service, notification_typ create_notification(template=letter_template, status='permanent-failure') assert Notification.query.count() == 3 assert NotificationHistory.query.count() == 3 - delete_notifications_created_more_than_a_week_ago_by_type(notification_type) + delete_notifications_older_than_retention_by_type(notification_type) assert Notification.query.count() == 2 assert NotificationHistory.query.count() == 3 @@ -109,7 +109,7 @@ def test_delete_notifications_for_days_of_retention(sample_service, notification created_at=datetime.utcnow() - timedelta(days=8)) create_service_data_retention(service_id=sample_service.id, notification_type=notification_type) assert len(Notification.query.all()) == 9 - delete_notifications_created_more_than_a_week_ago_by_type(notification_type) + delete_notifications_older_than_retention_by_type(notification_type) assert len(Notification.query.all()) == 7 assert len(Notification.query.filter_by(notification_type=notification_type).all()) == 1 if notification_type == 'letter': @@ -146,7 +146,7 @@ def test_delete_notifications_keep_data_for_days_of_retention_is_longer(sample_s create_notification(template=default_letter_template, status='temporary-failure', created_at=datetime.utcnow() - timedelta(days=8)) assert len(Notification.query.all()) == 9 - delete_notifications_created_more_than_a_week_ago_by_type(notification_type) + delete_notifications_older_than_retention_by_type(notification_type) assert len(Notification.query.filter_by().all()) == 8 assert len(Notification.query.filter_by(notification_type=notification_type).all()) == 2 if notification_type == 'letter': @@ -171,7 +171,7 @@ def test_delete_notifications_delete_notification_type_for_default_time_if_no_da create_notification(template=letter_template, status='temporary-failure', created_at=datetime.utcnow() - timedelta(days=14)) assert len(Notification.query.all()) == 6 - delete_notifications_created_more_than_a_week_ago_by_type('email') + delete_notifications_older_than_retention_by_type('email') assert len(Notification.query.filter_by().all()) == 5 assert len(Notification.query.filter_by(notification_type='email').all()) == 1 @@ -182,7 +182,7 @@ def test_delete_notifications_does_try_to_delete_from_s3_when_letter_has_not_bee create_notification(template=letter_template, status='sending', reference='LETTER_REF') - delete_notifications_created_more_than_a_week_ago_by_type('email', qry_limit=1) + delete_notifications_older_than_retention_by_type('email', qry_limit=1) mock_get_s3.assert_not_called() @@ -196,7 +196,7 @@ def test_delete_notifications_calls_subquery( create_notification(template=sms_template, created_at=datetime.now() - timedelta(days=8)) assert Notification.query.count() == 3 - delete_notifications_created_more_than_a_week_ago_by_type('sms', qry_limit=1) + delete_notifications_older_than_retention_by_type('sms', qry_limit=1) assert Notification.query.count() == 0 diff --git a/tests/app/dao/test_inbound_sms_dao.py b/tests/app/dao/test_inbound_sms_dao.py index 6fd43a47b..24eb1761c 100644 --- a/tests/app/dao/test_inbound_sms_dao.py +++ b/tests/app/dao/test_inbound_sms_dao.py @@ -1,19 +1,18 @@ -from datetime import datetime, timedelta +from datetime import datetime +from itertools import product from freezegun import freeze_time from app.dao.inbound_sms_dao import ( dao_get_inbound_sms_for_service, dao_count_inbound_sms_for_service, - delete_inbound_sms_created_more_than_a_week_ago, + delete_inbound_sms_older_than_retention, dao_get_inbound_sms_by_id, dao_get_paginated_inbound_sms_for_service_for_public_api, dao_get_paginated_most_recent_inbound_sms_by_user_number_for_service ) from tests.conftest import set_config -from tests.app.db import create_inbound_sms, create_service - -from app.models import InboundSms +from tests.app.db import create_inbound_sms, create_service, create_service_data_retention def test_get_all_inbound_sms(sample_service): @@ -59,15 +58,10 @@ def test_get_all_inbound_sms_filters_on_service(notify_db_session): def test_get_all_inbound_sms_filters_on_time(sample_service, notify_db_session): - create_inbound_sms(sample_service, user_number='447700900111', content='111 1', created_at=datetime(2017, 1, 2)) - sms_two = create_inbound_sms( - sample_service, - user_number='447700900111', - content='111 2', - created_at=datetime(2017, 1, 3) - ) + create_inbound_sms(sample_service, created_at=datetime(2017, 8, 6, 22, 59)) # sunday evening + sms_two = create_inbound_sms(sample_service, created_at=datetime(2017, 8, 6, 23, 0)) # monday (7th) morning - with freeze_time('2017-01-09'): + with freeze_time('2017-08-14 12:00'): res = dao_get_inbound_sms_for_service(sample_service.id) assert len(res) == 1 @@ -93,28 +87,43 @@ def test_count_inbound_sms_for_service_filters_messages_older_than_seven_days(sa assert dao_count_inbound_sms_for_service(sample_service.id) == 1 -@freeze_time("2017-01-01 12:00:00") -def test_should_delete_inbound_sms_older_than_seven_days(sample_service): - older_than_seven_days = datetime.utcnow() - timedelta(days=7, seconds=1) - create_inbound_sms(sample_service, created_at=older_than_seven_days) - delete_inbound_sms_created_more_than_a_week_ago() +@freeze_time("2017-06-08 12:00:00") +def test_should_delete_inbound_sms_according_to_data_retention(notify_db_session): + no_retention_service = create_service(service_name='no retention') + short_retention_service = create_service(service_name='three days') + long_retention_service = create_service(service_name='thirty days') - assert len(InboundSms.query.all()) == 0 + services = [short_retention_service, no_retention_service, long_retention_service] + create_service_data_retention(long_retention_service.id, notification_type='sms', days_of_retention=30) + create_service_data_retention(short_retention_service.id, notification_type='sms', days_of_retention=3) + # email retention doesn't affect anything + create_service_data_retention(short_retention_service.id, notification_type='email', days_of_retention=4) -@freeze_time("2017-01-01 12:00:00") -def test_should_not_delete_inbound_sms_before_seven_days(sample_service): - yesterday = datetime.utcnow() - timedelta(days=1) - just_before_seven_days = datetime.utcnow() - timedelta(days=6, hours=23, minutes=59, seconds=59) - older_than_seven_days = datetime.utcnow() - timedelta(days=7, seconds=1) + dates = [ + datetime(2017, 6, 4, 23, 00), # just before three days + datetime(2017, 6, 4, 22, 59), # older than three days + datetime(2017, 5, 31, 23, 00), # just before seven days + datetime(2017, 5, 31, 22, 59), # older than seven days + datetime(2017, 5, 1, 0, 0), # older than thirty days + ] - create_inbound_sms(sample_service, created_at=yesterday) - create_inbound_sms(sample_service, created_at=just_before_seven_days) - create_inbound_sms(sample_service, created_at=older_than_seven_days) + for date, service in product(dates, services): + create_inbound_sms(service, created_at=date) - delete_inbound_sms_created_more_than_a_week_ago() + deleted_count = delete_inbound_sms_older_than_retention() - assert len(InboundSms.query.all()) == 2 + # four deleted for the 3-day service, two for the default seven days one, one for the 30 day + assert deleted_count == 7 + assert { + x.created_at for x in dao_get_inbound_sms_for_service(short_retention_service.id, limit_days=None) + } == set(dates[:1]) + assert { + x.created_at for x in dao_get_inbound_sms_for_service(no_retention_service.id, limit_days=None) + } == set(dates[:3]) + assert { + x.created_at for x in dao_get_inbound_sms_for_service(long_retention_service.id, limit_days=None) + } == set(dates[:4]) def test_get_inbound_sms_by_id_returns(sample_service): diff --git a/tests/app/db.py b/tests/app/db.py index 1b301a2f2..3661a157f 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -1,3 +1,4 @@ +import random import uuid from datetime import datetime, date @@ -324,10 +325,18 @@ def create_inbound_sms( provider="mmg", created_at=None ): + if not service.inbound_number: + create_inbound_number( + # create random inbound number + notify_number or '07{:09}'.format(random.randint(0, 1e9 - 1)), + provider=provider, + service_id=service.id + ) + inbound = InboundSms( service=service, created_at=created_at or datetime.utcnow(), - notify_number=notify_number or service.get_default_sms_sender(), + notify_number=service.get_inbound_number(), user_number=user_number, provider_date=provider_date or datetime.utcnow(), provider_reference=provider_reference or 'foo',