From 0f2ddd8cfac2e7a061ac9885c5e1ac20fe9550bb Mon Sep 17 00:00:00 2001 From: chrisw Date: Wed, 4 Apr 2018 16:59:48 +0100 Subject: [PATCH] added new paginated inbound endpoint --- app/dao/inbound_sms_dao.py | 64 ++++++++++++++++++++------- app/inbound_sms/rest.py | 28 +++++++----- tests/app/dao/test_inbound_sms_dao.py | 59 +++++++++++++++++++----- tests/app/inbound_sms/test_rest.py | 41 ++++++++++------- 4 files changed, 136 insertions(+), 56 deletions(-) diff --git a/app/dao/inbound_sms_dao.py b/app/dao/inbound_sms_dao.py index 53c62f5af..cbe0ac0db 100644 --- a/app/dao/inbound_sms_dao.py +++ b/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'] + ) diff --git a/app/inbound_sms/rest.py b/app/inbound_sms/rest.py index e60625486..884ae24ce 100644 --- a/app/inbound_sms/rest.py +++ b/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') diff --git a/tests/app/dao/test_inbound_sms_dao.py b/tests/app/dao/test_inbound_sms_dao.py index 5c3ad989e..fc899af26 100644 --- a/tests/app/dao/test_inbound_sms_dao.py +++ b/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' diff --git a/tests/app/inbound_sms/test_rest.py b/tests/app/inbound_sms/test_rest.py index 323edccb5..1415e7fa2 100644 --- a/tests/app/inbound_sms/test_rest.py +++ b/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