Merge pull request #2421 from alphagov/inbound-retention

make inbound sms page respect data retention

app/dao/inbound_sms_dao.py

@@ -55,11 +55,10 @@ def dao_get_paginated_inbound_sms_for_service_for_public_api(
     ).items
 
 
-def dao_count_inbound_sms_for_service(service_id):
-    start_date = midnight_n_days_ago(6)
+def dao_count_inbound_sms_for_service(service_id, limit_days):
     return InboundSms.query.filter(
         InboundSms.service_id == service_id,
-        InboundSms.created_at >= start_date
+        InboundSms.created_at >= midnight_n_days_ago(limit_days)
     ).count()
 
 
@@ -126,10 +125,11 @@ def dao_get_inbound_sms_by_id(service_id, inbound_id):
 
 def dao_get_paginated_most_recent_inbound_sms_by_user_number_for_service(
     service_id,
-    page
+    page,
+    limit_days
 ):
     """
-    This query starts from inbound_sms and joins on to itself to find the most recent row for each user_number
+    This query starts from inbound_sms and joins on to itself to find the most recent row for each user_number.
 
     Equivalent sql:
 
@@ -146,7 +146,6 @@ 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 = midnight_n_days_ago(6)
     t2 = aliased(InboundSms)
     q = db.session.query(
         InboundSms
@@ -160,7 +159,7 @@ def dao_get_paginated_most_recent_inbound_sms_by_user_number_for_service(
     ).filter(
         t2.id == None,  # noqa
         InboundSms.service_id == service_id,
-        InboundSms.created_at >= start_date
+        InboundSms.created_at >= midnight_n_days_ago(limit_days)
     ).order_by(
         InboundSms.created_at.desc()
     )

app/inbound_sms/rest.py

@@ -12,6 +12,7 @@ from app.dao.inbound_sms_dao import (
     dao_get_inbound_sms_by_id,
     dao_get_paginated_most_recent_inbound_sms_by_user_number_for_service
 )
+from app.dao.service_data_retention_dao import fetch_service_data_retention_by_notification_type
 from app.errors import register_errors
 from app.schema_validation import validate
 
@@ -55,9 +56,12 @@ def get_inbound_sms_for_service(service_id):
 def get_most_recent_inbound_sms_for_service(service_id):
     # used on the service inbox page
     page = request.args.get('page', 1)
-    # get most recent message for each user for service
 
-    results = dao_get_paginated_most_recent_inbound_sms_by_user_number_for_service(service_id, int(page))
+    inbound_data_retention = fetch_service_data_retention_by_notification_type(service_id, 'sms')
+    limit_days = inbound_data_retention.days_of_retention if inbound_data_retention else 7
+
+    # get most recent message for each user for service
+    results = dao_get_paginated_most_recent_inbound_sms_by_user_number_for_service(service_id, int(page), limit_days)
     return jsonify(
         data=[row.serialize() for row in results.items],
         has_next=results.has_next
@@ -66,7 +70,8 @@ def get_most_recent_inbound_sms_for_service(service_id):
 
 @inbound_sms.route('/summary')
 def get_inbound_sms_summary_for_service(service_id):
-    count = dao_count_inbound_sms_for_service(service_id)
+    # this is for the dashboard, so always limit to 7 days, even if they have a longer data retention
+    count = dao_count_inbound_sms_for_service(service_id, limit_days=7)
     most_recent = dao_get_inbound_sms_for_service(service_id, limit=1)
 
     return jsonify(

tests/app/dao/test_inbound_sms_dao.py

@@ -76,15 +76,17 @@ def test_count_inbound_sms_for_service(notify_db_session):
     create_inbound_sms(service_one)
     create_inbound_sms(service_two)
 
-    assert dao_count_inbound_sms_for_service(service_one.id) == 2
+    assert dao_count_inbound_sms_for_service(service_one.id, limit_days=1) == 2
 
 
-def test_count_inbound_sms_for_service_filters_messages_older_than_seven_days(sample_service, notify_db_session):
-    create_inbound_sms(sample_service, user_number='447700900111', content='111 2', created_at=datetime(2017, 1, 2))
-    create_inbound_sms(sample_service, user_number='447700900111', content='111 2', created_at=datetime(2017, 1, 3))
+def test_count_inbound_sms_for_service_filters_messages_older_than_n_days(sample_service):
+    # test between evening sunday 2nd of june and morning of monday 3rd
+    create_inbound_sms(sample_service, created_at=datetime(2019, 6, 2, 22, 59))
+    create_inbound_sms(sample_service, created_at=datetime(2019, 6, 2, 22, 59))
+    create_inbound_sms(sample_service, created_at=datetime(2019, 6, 2, 23, 1))
 
-    with freeze_time('2017-01-09'):
-        assert dao_count_inbound_sms_for_service(sample_service.id) == 1
+    with freeze_time('Monday 10th June 2019 12:00'):
+        assert dao_count_inbound_sms_for_service(sample_service.id, limit_days=7) == 1
 
 
 @freeze_time("2017-06-08 12:00:00")
@@ -222,7 +224,7 @@ def test_most_recent_inbound_sms_only_returns_most_recent_for_each_number(notify
 
     with set_config(notify_api, 'PAGE_SIZE', 3):
         with freeze_time('2017-01-02'):
-            res = dao_get_paginated_most_recent_inbound_sms_by_user_number_for_service(sample_service.id, page=1)
+            res = dao_get_paginated_most_recent_inbound_sms_by_user_number_for_service(sample_service.id, limit_days=7, page=1)  # noqa
 
     assert len(res.items) == 2
     assert res.has_next is False
@@ -244,7 +246,7 @@ def test_most_recent_inbound_sms_paginates_properly(notify_api, sample_service):
     with set_config(notify_api, 'PAGE_SIZE', 2):
         with freeze_time('2017-01-02'):
             # first page has most recent 444 and 333
-            res = dao_get_paginated_most_recent_inbound_sms_by_user_number_for_service(sample_service.id, page=1)
+            res = dao_get_paginated_most_recent_inbound_sms_by_user_number_for_service(sample_service.id, limit_days=7, page=1)  # noqa
             assert len(res.items) == 2
             assert res.has_next is True
             assert res.per_page == 2
@@ -252,30 +254,21 @@ def test_most_recent_inbound_sms_paginates_properly(notify_api, sample_service):
             assert res.items[1].content == '333 2'
 
             # second page has no 444 or 333 - just most recent 222 and 111
-            res = dao_get_paginated_most_recent_inbound_sms_by_user_number_for_service(sample_service.id, page=2)
+            res = dao_get_paginated_most_recent_inbound_sms_by_user_number_for_service(sample_service.id, limit_days=7, page=2)  # noqa
             assert len(res.items) == 2
             assert res.has_next is False
             assert res.items[0].content == '222 2'
             assert res.items[1].content == '111 2'
 
 
-def test_most_recent_inbound_sms_only_returns_values_within_7_days(notify_api, sample_service):
-    create_inbound_sms(sample_service, user_number='447700900111', content='111 1', created_at=datetime(2017, 4, 1))
-    create_inbound_sms(sample_service, user_number='447700900111', content='111 2', created_at=datetime(2017, 4, 1))
-    create_inbound_sms(sample_service, user_number='447700900222', content='222 1', created_at=datetime(2017, 4, 1))
-    create_inbound_sms(sample_service, user_number='447700900222', content='222 2', created_at=datetime(2017, 4, 1))
-    create_inbound_sms(sample_service, user_number='447700900333', content='333 1', created_at=datetime(2017, 4, 2))
-    create_inbound_sms(sample_service, user_number='447700900333', content='333 2', created_at=datetime(2017, 4, 3))
-    create_inbound_sms(sample_service, user_number='447700900444', content='444 1', created_at=datetime(2017, 4, 4))
-    create_inbound_sms(sample_service, user_number='447700900444', content='444 2', created_at=datetime(2017, 4, 5))
+def test_most_recent_inbound_sms_only_returns_values_within_7_days(sample_service):
+    # just out of bounds
+    create_inbound_sms(sample_service, user_number='1', content='old', created_at=datetime(2017, 4, 2, 22, 59, 59))
+    # just in bounds
+    create_inbound_sms(sample_service, user_number='2', content='new', created_at=datetime(2017, 4, 2, 23, 0, 0))
 
-    # 7 days ago BST midnight
-    create_inbound_sms(sample_service, user_number='447700900666', content='666 1', created_at='2017-04-02T23:00:00')
+    with freeze_time('Monday 10th April 2017 12:00:00'):
+        res = dao_get_paginated_most_recent_inbound_sms_by_user_number_for_service(sample_service.id, limit_days=7, page=1)  # noqa
 
-    with freeze_time('2017-04-09T12:00:00'):
-        res = dao_get_paginated_most_recent_inbound_sms_by_user_number_for_service(sample_service.id, page=1)
-
-    assert len(res.items) == 3
-    assert res.items[0].content == '444 2'
-    assert res.items[1].content == '333 2'
-    assert res.items[2].content == '666 1'
+    assert len(res.items) == 1
+    assert res.items[0].content == 'new'

tests/app/inbound_sms/test_rest.py

@@ -1,10 +1,10 @@
-from datetime import datetime
+from datetime import datetime, timedelta
 
 import pytest
 from freezegun import freeze_time
 
 from tests.app.db import (
-    create_inbound_sms, create_service, create_service_with_inbound_number,
+    create_inbound_sms, create_service, create_service_with_inbound_number, create_service_data_retention
 )
 
 
@@ -297,3 +297,40 @@ def test_get_most_recent_inbound_sms_for_service(
 
     assert len(response['data']) == expected_rows
     assert response['has_next'] == has_next_link
+
+
+@freeze_time('Monday 10th April 2017 12:00')
+def test_get_most_recent_inbound_sms_for_service_respects_data_retention(
+    admin_request,
+    sample_service
+):
+    create_service_data_retention(sample_service.id, 'sms', 5)
+    for i in range(10):
+        created = datetime.utcnow() - timedelta(days=i)
+        create_inbound_sms(sample_service, user_number='44770090000{}'.format(i), created_at=created)
+
+    response = admin_request.get('inbound_sms.get_most_recent_inbound_sms_for_service', service_id=sample_service.id)
+
+    assert len(response['data']) == 6
+    assert [x['created_at'] for x in response['data']] == [
+        '2017-04-10T12:00:00.000000Z',
+        '2017-04-09T12:00:00.000000Z',
+        '2017-04-08T12:00:00.000000Z',
+        '2017-04-07T12:00:00.000000Z',
+        '2017-04-06T12:00:00.000000Z',
+        '2017-04-05T12:00:00.000000Z',
+    ]
+
+
+@freeze_time('Monday 10th April 2017 12:00')
+def test_get_most_recent_inbound_sms_for_service_respects_data_retention_if_older_than_a_week(
+    admin_request,
+    sample_service
+):
+    create_service_data_retention(sample_service.id, 'sms', 14)
+    create_inbound_sms(sample_service, created_at=datetime(2017, 4, 1, 12, 0))
+
+    response = admin_request.get('inbound_sms.get_most_recent_inbound_sms_for_service', service_id=sample_service.id)
+
+    assert len(response['data']) == 1
+    assert response['data'][0]['created_at'] == '2017-04-01T12:00:00.000000Z'