Merge pull request #1822 from alphagov/added-new-paginated-inbound-endpoint

Added new paginated inbound endpoint

app/dao/inbound_sms_dao.py

@@ -4,7 +4,8 @@ from datetime import (
 )
 from flask import current_app
 from notifications_utils.statsd_decorators import statsd
-from sqlalchemy import desc
+from sqlalchemy import desc, and_
+from sqlalchemy.orm import aliased
 
 from app import db
 from app.dao.dao_utils import transactional
@@ -32,22 +33,6 @@ def dao_get_inbound_sms_for_service(service_id, limit=None, user_number=None):
     return q.all()
 
 
-def dao_get_paginated_inbound_sms_for_service(service_id, user_number=None, page=1):
-    q = InboundSms.query.filter(
-        InboundSms.service_id == service_id
-    ).order_by(
-        InboundSms.created_at.desc()
-    )
-
-    if user_number:
-        q = q.filter(InboundSms.user_number == user_number)
-
-    return q.paginate(
-        page=page,
-        per_page=current_app.config['PAGE_SIZE']
-    )
-
-
 def dao_get_paginated_inbound_sms_for_service_for_public_api(
     service_id,
     older_than=None,
@@ -93,3 +78,48 @@ def dao_get_inbound_sms_by_id(service_id, inbound_id):
         id=inbound_id,
         service_id=service_id
     ).one()
+
+
+def dao_get_paginated_most_recent_inbound_sms_by_user_number_for_service(
+    service_id,
+    page
+):
+    """
+    This query starts from inbound_sms and joins on to itself to find the most recent row for each user_number
+
+    Equivalent sql:
+
+    SELECT t1.*
+    FROM inbound_sms t1
+    LEFT OUTER JOIN inbound_sms AS t2 ON (
+        -- identifying
+        t1.user_number = t2.user_number AND
+        t1.service_id = t2.service_id AND
+        -- ordering
+        t1.created_at < t2.created_at
+    )
+    WHERE t2.id IS NULL AND t1.service_id = :service_id
+    ORDER BY t1.created_at DESC;
+    LIMIT 50 OFFSET :page
+    """
+    t2 = aliased(InboundSms)
+    q = db.session.query(
+        InboundSms
+    ).outerjoin(
+        t2,
+        and_(
+            InboundSms.user_number == t2.user_number,
+            InboundSms.service_id == t2.service_id,
+            InboundSms.created_at < t2.created_at
+        )
+    ).filter(
+        t2.id == None,  # noqa
+        InboundSms.service_id == service_id
+    ).order_by(
+        InboundSms.created_at.desc()
+    )
+
+    return q.paginate(
+        page=page,
+        per_page=current_app.config['PAGE_SIZE']
+    )

app/inbound_sms/rest.py

@@ -10,7 +10,7 @@ from app.dao.inbound_sms_dao import (
     dao_get_inbound_sms_for_service,
     dao_count_inbound_sms_for_service,
     dao_get_inbound_sms_by_id,
-    dao_get_paginated_inbound_sms_for_service
+    dao_get_paginated_most_recent_inbound_sms_by_user_number_for_service
 )
 from app.errors import register_errors
 from app.schema_validation import validate
@@ -41,23 +41,27 @@ def post_query_inbound_sms_for_service(service_id):
 
 @inbound_sms.route('', methods=['GET'])
 def get_inbound_sms_for_service(service_id):
-    limit = request.args.get('limit')
-    page = request.args.get('page')
     user_number = request.args.get('user_number')
 
     if user_number:
         # we use this to normalise to an international phone number - but this may fail if it's an alphanumeric
         user_number = try_validate_and_format_phone_number(user_number, international=True)
 
-    if not page:
-        results = dao_get_inbound_sms_for_service(service_id, limit, user_number)
-        return jsonify(data=[row.serialize() for row in results])
-    else:
-        results = dao_get_paginated_inbound_sms_for_service(service_id, user_number, int(page))
-        return jsonify(
-            data=[row.serialize() for row in results.items],
-            has_next=results.has_next
-        )
+    results = dao_get_inbound_sms_for_service(service_id, user_number=user_number)
+    return jsonify(data=[row.serialize() for row in results])
+
+
+@inbound_sms.route('/most-recent', methods=['GET'])
+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))
+    return jsonify(
+        data=[row.serialize() for row in results.items],
+        has_next=results.has_next
+    )
 
 
 @inbound_sms.route('/summary')

tests/app/dao/test_inbound_sms_dao.py

@@ -8,8 +8,9 @@ from app.dao.inbound_sms_dao import (
     delete_inbound_sms_created_more_than_a_week_ago,
     dao_get_inbound_sms_by_id,
     dao_get_paginated_inbound_sms_for_service_for_public_api,
-    dao_get_paginated_inbound_sms_for_service
+    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
@@ -23,16 +24,6 @@ def test_get_all_inbound_sms(sample_service):
     assert res[0] == inbound
 
 
-def test_get_all_inbound_sms_by_page(sample_service):
-    inbound = create_inbound_sms(sample_service)
-
-    res = dao_get_paginated_inbound_sms_for_service(sample_service.id)
-    assert len(res.items) == 1
-    assert res.has_next is False
-    assert res.per_page == 50
-    assert res.items[0] == inbound
-
-
 def test_get_all_inbound_sms_when_none_exist(sample_service):
     res = dao_get_inbound_sms_for_service(sample_service.id)
     assert len(res) == 0
@@ -184,3 +175,49 @@ def test_dao_get_paginated_inbound_sms_for_service_for_public_api_older_than_end
     )
 
     assert inbound_from_db == []
+
+
+def test_most_recent_inbound_sms_only_returns_most_recent_for_each_number(notify_api, sample_service):
+    create_inbound_sms(sample_service, user_number='447700900111', content='111 1', created_at=datetime(2017, 1, 1))
+    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 3', created_at=datetime(2017, 1, 3))
+    create_inbound_sms(sample_service, user_number='447700900111', content='111 4', created_at=datetime(2017, 1, 4))
+    create_inbound_sms(sample_service, user_number='447700900111', content='111 5', created_at=datetime(2017, 1, 5))
+    create_inbound_sms(sample_service, user_number='447700900222', content='222 1', created_at=datetime(2017, 1, 1))
+    create_inbound_sms(sample_service, user_number='447700900222', content='222 2', created_at=datetime(2017, 1, 2))
+
+    with set_config(notify_api, 'PAGE_SIZE', 3):
+        res = dao_get_paginated_most_recent_inbound_sms_by_user_number_for_service(sample_service.id, page=1)
+
+    assert len(res.items) == 2
+    assert res.has_next is False
+    assert res.per_page == 3
+    assert res.items[0].content == '111 5'
+    assert res.items[1].content == '222 2'
+
+
+def test_most_recent_inbound_sms_paginates_properly(notify_api, sample_service):
+    create_inbound_sms(sample_service, user_number='447700900111', content='111 1', created_at=datetime(2017, 1, 1))
+    create_inbound_sms(sample_service, user_number='447700900111', content='111 2', created_at=datetime(2017, 1, 2))
+    create_inbound_sms(sample_service, user_number='447700900222', content='222 1', created_at=datetime(2017, 1, 3))
+    create_inbound_sms(sample_service, user_number='447700900222', content='222 2', created_at=datetime(2017, 1, 4))
+    create_inbound_sms(sample_service, user_number='447700900333', content='333 1', created_at=datetime(2017, 1, 5))
+    create_inbound_sms(sample_service, user_number='447700900333', content='333 2', created_at=datetime(2017, 1, 6))
+    create_inbound_sms(sample_service, user_number='447700900444', content='444 1', created_at=datetime(2017, 1, 7))
+    create_inbound_sms(sample_service, user_number='447700900444', content='444 2', created_at=datetime(2017, 1, 8))
+
+    with set_config(notify_api, 'PAGE_SIZE', 2):
+        # 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)
+        assert len(res.items) == 2
+        assert res.has_next is True
+        assert res.per_page == 2
+        assert res.items[0].content == '444 2'
+        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)
+        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'

tests/app/inbound_sms/test_rest.py

@@ -156,22 +156,6 @@ def test_old_get_inbound_sms(admin_request, sample_service):
     }
 
 
-def test_old_get_inbound_sms_limits(admin_request, sample_service):
-    with freeze_time('2017-01-01'):
-        create_inbound_sms(sample_service)
-    with freeze_time('2017-01-02'):
-        two = create_inbound_sms(sample_service)
-
-    sms = admin_request.get(
-        'inbound_sms.get_inbound_sms_for_service',
-        service_id=sample_service.id,
-        limit=1,
-    )
-
-    assert len(sms['data']) == 1
-    assert sms['data'][0]['id'] == str(two.id)
-
-
 @pytest.mark.parametrize('user_number', [
     '(07700) 900-001',
     '+4407700900001',
@@ -289,3 +273,28 @@ def test_get_inbound_sms_by_id_with_invalid_service_id_returns_404(admin_request
         inbound_sms_id='2cfbd6a1-1575-4664-8969-f27be0ea40d9',
         _expected_status=404
     )
+
+
+@pytest.mark.parametrize('page_given, expected_rows, has_next_link', [
+    (True, 10, False),
+    (False, 50, True)
+])
+def test_get_most_recent_inbound_sms_for_service(
+    admin_request,
+    page_given,
+    sample_service,
+    expected_rows,
+    has_next_link
+):
+    for i in range(60):
+        create_inbound_sms(service=sample_service, user_number='44770090000{}'.format(i))
+
+    request_args = {'page': 2} if page_given else {}
+    response = admin_request.get(
+        'inbound_sms.get_most_recent_inbound_sms_for_service',
+        service_id=sample_service.id,
+        **request_args
+    )
+
+    assert len(response['data']) == expected_rows
+    assert response['has_next'] == has_next_link