Merge pull request #2381 from alphagov/inbound-sms-retention

Inbound sms now deletes according to data retention

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,

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.models import InboundSms, Service, ServiceDataRetention, SMS_TYPE
-from app.utils import get_london_midnight_in_utc
+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):
+def dao_get_inbound_sms_for_service(service_id, limit=None, user_number=None, limit_days=7):
-    start_date = get_london_midnight_in_utc(date.today() - timedelta(days=6))
     q = InboundSms.query.filter(
-        InboundSms.service_id == service_id,
+        InboundSms.service_id == service_id
-        InboundSms.created_at >= start_date
     ).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():
+def delete_inbound_sms_older_than_retention():
-    seven_days_ago = datetime.utcnow() - timedelta(days=7)
+    current_app.logger.info('Deleting inbound sms for services with flexible data retention')
 
-    deleted = db.session.query(InboundSms).filter(
+    flexible_data_retention = ServiceDataRetention.query.join(
-        InboundSms.created_at < seven_days_ago
+        ServiceDataRetention.service,
-    ).delete(synchronize_session='fetch')
+        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

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))
 

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_email_notifications_older_than_retention,
-    delete_inbound_sms_older_than_seven_days,
+    delete_inbound_sms,
-    delete_letter_notifications_older_than_seven_days,
+    delete_letter_notifications_older_than_retention,
-    delete_sms_notifications_older_than_seven_days,
+    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')
+    mocked = mocker.patch('app.celery.nightly_tasks.delete_notifications_older_than_retention_by_type')
-    delete_sms_notifications_older_than_seven_days()
+    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')
+        'app.celery.nightly_tasks.delete_notifications_older_than_retention_by_type')
-    delete_email_notifications_older_than_seven_days()
+    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')
+    mocked = mocker.patch('app.celery.nightly_tasks.delete_notifications_older_than_retention_by_type')
-    delete_letter_notifications_older_than_seven_days()
+    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):
+def test_should_call_delete_inbound_sms(notify_api, mocker):
-    mocker.patch('app.celery.nightly_tasks.delete_inbound_sms_created_more_than_a_week_ago')
+    mocker.patch('app.celery.nightly_tasks.delete_inbound_sms_older_than_retention')
-    delete_inbound_sms_older_than_seven_days()
+    delete_inbound_sms()
-    assert nightly_tasks.delete_inbound_sms_created_more_than_a_week_ago.call_count == 1
+    assert nightly_tasks.delete_inbound_sms_older_than_retention.call_count == 1
 
 
 @freeze_time('2017-01-01 10:00:00')

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
 
 

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
 
 

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 tests.app.db import create_inbound_sms, create_service, create_service_data_retention
-
-from app.models import InboundSms
 
 
 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))
+    create_inbound_sms(sample_service, created_at=datetime(2017, 8, 6, 22, 59))  # sunday evening
-    sms_two = create_inbound_sms(
+    sms_two = create_inbound_sms(sample_service, created_at=datetime(2017, 8, 6, 23, 0))  # monday (7th) morning
-        sample_service,
-        user_number='447700900111',
-        content='111 2',
-        created_at=datetime(2017, 1, 3)
-    )
 
-    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")
+@freeze_time("2017-06-08 12:00:00")
-def test_should_delete_inbound_sms_older_than_seven_days(sample_service):
+def test_should_delete_inbound_sms_according_to_data_retention(notify_db_session):
-    older_than_seven_days = datetime.utcnow() - timedelta(days=7, seconds=1)
+    no_retention_service = create_service(service_name='no retention')
-    create_inbound_sms(sample_service, created_at=older_than_seven_days)
+    short_retention_service = create_service(service_name='three days')
-    delete_inbound_sms_created_more_than_a_week_ago()
+    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")
+    dates = [
-def test_should_not_delete_inbound_sms_before_seven_days(sample_service):
+        datetime(2017, 6, 4, 23, 00),  # just before three days
-    yesterday = datetime.utcnow() - timedelta(days=1)
+        datetime(2017, 6, 4, 22, 59),  # older than three days
-    just_before_seven_days = datetime.utcnow() - timedelta(days=6, hours=23, minutes=59, seconds=59)
+        datetime(2017, 5, 31, 23, 00),  # just before seven days
-    older_than_seven_days = datetime.utcnow() - timedelta(days=7, seconds=1)
+        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)
+    for date, service in product(dates, services):
-    create_inbound_sms(sample_service, created_at=just_before_seven_days)
+        create_inbound_sms(service, created_at=date)
-    create_inbound_sms(sample_service, created_at=older_than_seven_days)
 
-    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):

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',