From 38f0ea6cca5223dfcce3800bea5e22f1eead3164 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 26 Feb 2019 17:57:35 +0000 Subject: [PATCH 1/5] remove functions to not talk about 7 days remind us that data retention is flexible --- app/celery/nightly_tasks.py | 20 ++++++------- app/dao/notifications_dao.py | 2 +- tests/app/celery/test_nightly_tasks.py | 28 +++++++++---------- .../notification_dao/test_notification_dao.py | 4 +-- ...t_notification_dao_delete_notifications.py | 16 +++++------ 5 files changed, 35 insertions(+), 35 deletions(-) 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/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 From e7b8f83fc897e9e0110b48357bd8be5dd1b0333f Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 26 Feb 2019 17:57:55 +0000 Subject: [PATCH 2/5] make inbound sms honour data retention code inspired by the delete notification code, but with some clean up since we don't deal with different types etc, and only need to run the query for services with inbound numbers also, update tests.app.db.create_inbound_sms to create inbound numbers and assign them to services to ensure the test db is always accurate and reflects real world usage --- app/dao/inbound_sms_dao.py | 67 +++++++++++++++++++++------ tests/app/dao/test_inbound_sms_dao.py | 50 +++++++++++--------- tests/app/db.py | 11 ++++- 3 files changed, 91 insertions(+), 37 deletions(-) diff --git a/app/dao/inbound_sms_dao.py b/app/dao/inbound_sms_dao.py index 22b0d468b..c65eb8e81 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 @@ -20,7 +15,7 @@ def dao_create_inbound_sms(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)) + start_date = midnight_n_days_ago(6) q = InboundSms.query.filter( InboundSms.service_id == service_id, InboundSms.created_at >= start_date @@ -60,21 +55,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 +145,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/tests/app/dao/test_inbound_sms_dao.py b/tests/app/dao/test_inbound_sms_dao.py index 6fd43a47b..cb0822cdf 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): @@ -93,28 +92,37 @@ 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): + service_without_retention = create_service(service_name='without retention') + service_with_three_day_retention = create_service(service_name='three days') + service_with_month_retention = create_service(service_name='thirty days') - assert len(InboundSms.query.all()) == 0 + services = [service_with_three_day_retention, service_without_retention, service_with_month_retention] + create_service_data_retention(service_with_month_retention.id, notification_type='sms', days_of_retention=30) + create_service_data_retention(service_with_three_day_retention.id, notification_type='sms', days_of_retention=3) + # email retention doesn't affect anything + create_service_data_retention(service_with_three_day_retention.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, 7, 22, 59), # 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 dao_count_inbound_sms_for_service(service_without_retention.id) == 3 + assert dao_count_inbound_sms_for_service(service_with_three_day_retention.id) == 1 + assert dao_count_inbound_sms_for_service(service_with_month_retention.id) == 4 def test_get_inbound_sms_by_id_returns(sample_service): diff --git a/tests/app/db.py b/tests/app/db.py index 2aa6e3ae1..023e5fe30 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -1,3 +1,4 @@ +import random import uuid from datetime import datetime, date @@ -318,10 +319,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', From 88303140ec9983ff95fd1edd33eb56bf9fccdf98 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 27 Feb 2019 17:17:35 +0000 Subject: [PATCH 3/5] give dao_get_inbound_sms_for_service optional day limit defaults to 6 to preserve backwards compatibility --- app/dao/inbound_sms_dao.py | 9 +++++---- tests/app/dao/test_inbound_sms_dao.py | 28 ++++++++++++++++----------- 2 files changed, 22 insertions(+), 15 deletions(-) diff --git a/app/dao/inbound_sms_dao.py b/app/dao/inbound_sms_dao.py index c65eb8e81..cb086899a 100644 --- a/app/dao/inbound_sms_dao.py +++ b/app/dao/inbound_sms_dao.py @@ -14,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 = midnight_n_days_ago(6) +def dao_get_inbound_sms_for_service(service_id, limit=None, user_number=None, days_ago_to_start=6): 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 days_ago_to_start is not None: + start_date = midnight_n_days_ago(days_ago_to_start) + q = q.filter(InboundSms.created_at >= start_date) if user_number: q = q.filter(InboundSms.user_number == user_number) diff --git a/tests/app/dao/test_inbound_sms_dao.py b/tests/app/dao/test_inbound_sms_dao.py index cb0822cdf..f35103f1c 100644 --- a/tests/app/dao/test_inbound_sms_dao.py +++ b/tests/app/dao/test_inbound_sms_dao.py @@ -94,23 +94,23 @@ def test_count_inbound_sms_for_service_filters_messages_older_than_seven_days(sa @freeze_time("2017-06-08 12:00:00") def test_should_delete_inbound_sms_according_to_data_retention(notify_db_session): - service_without_retention = create_service(service_name='without retention') - service_with_three_day_retention = create_service(service_name='three days') - service_with_month_retention = create_service(service_name='thirty days') + 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') - services = [service_with_three_day_retention, service_without_retention, service_with_month_retention] + services = [short_retention_service, no_retention_service, long_retention_service] - create_service_data_retention(service_with_month_retention.id, notification_type='sms', days_of_retention=30) - create_service_data_retention(service_with_three_day_retention.id, notification_type='sms', days_of_retention=3) + 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(service_with_three_day_retention.id, notification_type='email', days_of_retention=4) + create_service_data_retention(short_retention_service.id, notification_type='email', days_of_retention=4) 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, 7, 22, 59), # older than thirty days + datetime(2017, 5, 1, 0, 0), # older than thirty days ] for date, service in product(dates, services): @@ -120,9 +120,15 @@ def test_should_delete_inbound_sms_according_to_data_retention(notify_db_session # four deleted for the 3-day service, two for the default seven days one, one for the 30 day assert deleted_count == 7 - assert dao_count_inbound_sms_for_service(service_without_retention.id) == 3 - assert dao_count_inbound_sms_for_service(service_with_three_day_retention.id) == 1 - assert dao_count_inbound_sms_for_service(service_with_month_retention.id) == 4 + assert { + x.created_at for x in dao_get_inbound_sms_for_service(short_retention_service.id, days_ago_to_start=None) + } == set(dates[:1]) + assert { + x.created_at for x in dao_get_inbound_sms_for_service(no_retention_service.id, days_ago_to_start=None) + } == set(dates[:3]) + assert { + x.created_at for x in dao_get_inbound_sms_for_service(long_retention_service.id, days_ago_to_start=None) + } == set(dates[:4]) def test_get_inbound_sms_by_id_returns(sample_service): From b4d1a590b71b22d89cb3ab634e377fc20c379b84 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 28 Feb 2019 13:59:28 +0000 Subject: [PATCH 4/5] rename days_ago_to_start to limit_days consistency with the rest of the app --- app/dao/inbound_sms_dao.py | 6 +++--- tests/app/dao/test_inbound_sms_dao.py | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/dao/inbound_sms_dao.py b/app/dao/inbound_sms_dao.py index cb086899a..88ab0e356 100644 --- a/app/dao/inbound_sms_dao.py +++ b/app/dao/inbound_sms_dao.py @@ -14,14 +14,14 @@ 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, days_ago_to_start=6): +def dao_get_inbound_sms_for_service(service_id, limit=None, user_number=None, limit_days=6): q = InboundSms.query.filter( InboundSms.service_id == service_id ).order_by( InboundSms.created_at.desc() ) - if days_ago_to_start is not None: - start_date = midnight_n_days_ago(days_ago_to_start) + 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: diff --git a/tests/app/dao/test_inbound_sms_dao.py b/tests/app/dao/test_inbound_sms_dao.py index f35103f1c..e8419dae6 100644 --- a/tests/app/dao/test_inbound_sms_dao.py +++ b/tests/app/dao/test_inbound_sms_dao.py @@ -121,13 +121,13 @@ def test_should_delete_inbound_sms_according_to_data_retention(notify_db_session # 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, days_ago_to_start=None) + 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, days_ago_to_start=None) + 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, days_ago_to_start=None) + x.created_at for x in dao_get_inbound_sms_for_service(long_retention_service.id, limit_days=None) } == set(dates[:4]) From 7683e340cc10f99acd76ee99faf8833884957e75 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 28 Feb 2019 15:28:13 +0000 Subject: [PATCH 5/5] get all inbound sms should default to 7 days, not 6 to be consistent with other checks. --- app/dao/inbound_sms_dao.py | 2 +- tests/app/dao/test_inbound_sms_dao.py | 11 +++-------- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/app/dao/inbound_sms_dao.py b/app/dao/inbound_sms_dao.py index 88ab0e356..0b6334479 100644 --- a/app/dao/inbound_sms_dao.py +++ b/app/dao/inbound_sms_dao.py @@ -14,7 +14,7 @@ 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, limit_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 ).order_by( diff --git a/tests/app/dao/test_inbound_sms_dao.py b/tests/app/dao/test_inbound_sms_dao.py index e8419dae6..24eb1761c 100644 --- a/tests/app/dao/test_inbound_sms_dao.py +++ b/tests/app/dao/test_inbound_sms_dao.py @@ -58,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