From cf248a2af3c37c7a381dc381d271722e92e1fc71 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 28 Mar 2019 15:36:12 +0000 Subject: [PATCH 1/2] combine post + get inbound, and make them respect data retention also removed the limit/limit_days args as they're not used by admin --- app/dao/inbound_sms_dao.py | 2 +- app/inbound_sms/inbound_sms_schemas.py | 1 - app/inbound_sms/rest.py | 18 ++--- tests/app/dao/test_inbound_sms_dao.py | 8 +- tests/app/inbound_sms/test_rest.py | 105 ++++++++++++++++--------- 5 files changed, 82 insertions(+), 52 deletions(-) diff --git a/app/dao/inbound_sms_dao.py b/app/dao/inbound_sms_dao.py index 49d962318..d7e6f86e0 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=7): +def dao_get_inbound_sms_for_service(service_id, user_number=None, *, limit_days=None, limit=None): q = InboundSms.query.filter( InboundSms.service_id == service_id ).order_by( diff --git a/app/inbound_sms/inbound_sms_schemas.py b/app/inbound_sms/inbound_sms_schemas.py index 24e642f49..f9cd703b1 100644 --- a/app/inbound_sms/inbound_sms_schemas.py +++ b/app/inbound_sms/inbound_sms_schemas.py @@ -4,6 +4,5 @@ get_inbound_sms_for_service_schema = { "type": "object", "properties": { "phone_number": {"type": "string"}, - "limit": {"type": ["integer", "null"], "minimum": 1} } } diff --git a/app/inbound_sms/rest.py b/app/inbound_sms/rest.py index 38c6b55bd..d802a659a 100644 --- a/app/inbound_sms/rest.py +++ b/app/inbound_sms/rest.py @@ -30,25 +30,23 @@ register_errors(inbound_sms) @inbound_sms.route('', methods=['POST']) def post_query_inbound_sms_for_service(service_id): form = validate(request.get_json(), get_inbound_sms_for_service_schema) - if 'phone_number' in form: - # 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(form['phone_number'], international=True) - else: - user_number = None - results = dao_get_inbound_sms_for_service(service_id, form.get('limit'), user_number) - - return jsonify(data=[row.serialize() for row in results]) + return _get_inbound_sms(service_id, user_number=form.get('phone_number')) @inbound_sms.route('', methods=['GET']) def get_inbound_sms_for_service(service_id): - user_number = request.args.get('user_number') + return _get_inbound_sms(service_id, user_number=request.args.get('user_number')) + +def _get_inbound_sms(service_id, 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) - results = dao_get_inbound_sms_for_service(service_id, user_number=user_number) + 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 + + results = dao_get_inbound_sms_for_service(service_id, user_number=user_number, limit_days=limit_days) return jsonify(data=[row.serialize() for row in results]) diff --git a/tests/app/dao/test_inbound_sms_dao.py b/tests/app/dao/test_inbound_sms_dao.py index 9c61a8934..09a15c0af 100644 --- a/tests/app/dao/test_inbound_sms_dao.py +++ b/tests/app/dao/test_inbound_sms_dao.py @@ -62,7 +62,7 @@ def test_get_all_inbound_sms_filters_on_time(sample_service, notify_db_session): sms_two = create_inbound_sms(sample_service, created_at=datetime(2017, 8, 6, 23, 0)) # monday (7th) morning with freeze_time('2017-08-14 12:00'): - res = dao_get_inbound_sms_for_service(sample_service.id) + res = dao_get_inbound_sms_for_service(sample_service.id, limit_days=7) assert len(res) == 1 assert res[0] == sms_two @@ -118,13 +118,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, limit_days=None) + x.created_at for x in dao_get_inbound_sms_for_service(short_retention_service.id) } == set(dates[:1]) assert { - x.created_at for x in dao_get_inbound_sms_for_service(no_retention_service.id, limit_days=None) + x.created_at for x in dao_get_inbound_sms_for_service(no_retention_service.id) } == set(dates[:3]) assert { - x.created_at for x in dao_get_inbound_sms_for_service(long_retention_service.id, limit_days=None) + x.created_at for x in dao_get_inbound_sms_for_service(long_retention_service.id) } == set(dates[:4]) diff --git a/tests/app/inbound_sms/test_rest.py b/tests/app/inbound_sms/test_rest.py index cfa150d52..8575fb009 100644 --- a/tests/app/inbound_sms/test_rest.py +++ b/tests/app/inbound_sms/test_rest.py @@ -31,40 +31,6 @@ def test_post_to_get_inbound_sms_with_no_params(admin_request, sample_service): } -def test_post_to_get_inbound_sms_with_limit(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) - - data = {'limit': 1} - sms = admin_request.post( - 'inbound_sms.post_query_inbound_sms_for_service', - service_id=sample_service.id, - _data=data - )['data'] - - assert len(sms) == 1 - assert sms[0]['id'] == str(two.id) - - -def test_post_to_get_inbound_sms_should_error_with_invalid_limit(admin_request, sample_service): - data = {'limit': 'limit'} - - error_resp = admin_request.post( - 'inbound_sms.post_query_inbound_sms_for_service', - service_id=sample_service.id, - _data=data, - _expected_status=400 - ) - - assert error_resp['status_code'] == 400 - assert error_resp['errors'] == [{ - 'error': 'ValidationError', - 'message': "limit limit is not of type integer, null" - }] - - @pytest.mark.parametrize('user_number', [ '(07700) 900-001', '+4407700900001', @@ -76,7 +42,6 @@ def test_post_to_get_inbound_sms_filters_user_number(admin_request, sample_servi create_inbound_sms(sample_service, user_number='447700900002') data = { - 'limit': 1, 'phone_number': user_number } @@ -97,7 +62,6 @@ def test_post_to_get_inbound_sms_filters_international_user_number(admin_request create_inbound_sms(sample_service) data = { - 'limit': 1, 'phone_number': '+1 (202) 555-0104' } @@ -126,6 +90,29 @@ def test_post_to_get_inbound_sms_allows_badly_formatted_number(admin_request, sa assert sms[0]['user_number'] == str(one.user_number) +@freeze_time('Monday 10th April 2017 12:00') +def test_post_to_get_most_recent_inbound_sms_for_service_limits_to_a_week(admin_request, sample_service): + create_inbound_sms(sample_service, created_at=datetime(2017, 4, 2, 22, 59)) + returned_inbound = create_inbound_sms(sample_service, created_at=datetime(2017, 4, 2, 23, 30)) + + sms = admin_request.post('inbound_sms.get_inbound_sms_for_service', service_id=sample_service.id, _data={}) + + assert len(sms['data']) == 1 + assert sms['data'][0]['id'] == str(returned_inbound.id) + + +@freeze_time('Monday 10th April 2017 12:00') +def test_post_to_get_inbound_sms_for_service_respects_data_retention(admin_request, sample_service): + create_service_data_retention(sample_service.id, 'sms', 5) + create_inbound_sms(sample_service, created_at=datetime(2017, 4, 4, 22, 59)) + returned_inbound = create_inbound_sms(sample_service, created_at=datetime(2017, 4, 5, 12, 0)) + + sms = admin_request.post('inbound_sms.get_inbound_sms_for_service', service_id=sample_service.id, _data={}) + + assert len(sms['data']) == 1 + assert sms['data'][0]['id'] == str(returned_inbound.id) + + ############################################################## # REMOVE ONCE ADMIN MIGRATED AND GET ENDPOINT REMOVED ############################################################## @@ -206,6 +193,29 @@ def test_old_get_inbound_sms_allows_badly_formatted_number(admin_request, sample assert sms['data'][0]['user_number'] == str(one.user_number) +@freeze_time('Monday 10th April 2017 12:00') +def test_old_get_most_recent_inbound_sms_for_service_limits_to_a_week(admin_request, sample_service): + create_inbound_sms(sample_service, created_at=datetime(2017, 4, 2, 22, 59)) + returned_inbound = create_inbound_sms(sample_service, created_at=datetime(2017, 4, 2, 23, 30)) + + sms = admin_request.get('inbound_sms.get_inbound_sms_for_service', service_id=sample_service.id) + + assert len(sms['data']) == 1 + assert sms['data'][0]['id'] == str(returned_inbound.id) + + +@freeze_time('Monday 10th April 2017 12:00') +def test_old_get_inbound_sms_for_service_respects_data_retention(admin_request, sample_service): + create_service_data_retention(sample_service.id, 'sms', 5) + create_inbound_sms(sample_service, created_at=datetime(2017, 4, 4, 22, 59)) + returned_inbound = create_inbound_sms(sample_service, created_at=datetime(2017, 4, 5, 12, 0)) + + sms = admin_request.get('inbound_sms.get_inbound_sms_for_service', service_id=sample_service.id) + + assert len(sms['data']) == 1 + assert sms['data'][0]['id'] == str(returned_inbound.id) + + ############################## # End delete section ############################## @@ -334,3 +344,26 @@ def test_get_most_recent_inbound_sms_for_service_respects_data_retention_if_olde assert len(response['data']) == 1 assert response['data'][0]['created_at'] == '2017-04-01T12:00:00.000000Z' + + +@freeze_time('Monday 10th April 2017 12:00') +def test_get_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', + ] From ff328eb594e01400a7ffbb078563c1512d73d6b6 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 28 Mar 2019 15:55:07 +0000 Subject: [PATCH 2/2] add test for different retention lengths --- tests/app/inbound_sms/test_rest.py | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/tests/app/inbound_sms/test_rest.py b/tests/app/inbound_sms/test_rest.py index 8575fb009..dc771c5d3 100644 --- a/tests/app/inbound_sms/test_rest.py +++ b/tests/app/inbound_sms/test_rest.py @@ -101,11 +101,21 @@ def test_post_to_get_most_recent_inbound_sms_for_service_limits_to_a_week(admin_ assert sms['data'][0]['id'] == str(returned_inbound.id) +@pytest.mark.parametrize('days_of_retention, too_old_date, returned_date', [ + (5, datetime(2017, 4, 4, 22, 59), datetime(2017, 4, 5, 12, 0)), + (14, datetime(2017, 3, 26, 22, 59), datetime(2017, 3, 27, 12, 0)), +]) @freeze_time('Monday 10th April 2017 12:00') -def test_post_to_get_inbound_sms_for_service_respects_data_retention(admin_request, sample_service): - create_service_data_retention(sample_service.id, 'sms', 5) - create_inbound_sms(sample_service, created_at=datetime(2017, 4, 4, 22, 59)) - returned_inbound = create_inbound_sms(sample_service, created_at=datetime(2017, 4, 5, 12, 0)) +def test_post_to_get_inbound_sms_for_service_respects_data_retention( + admin_request, + sample_service, + days_of_retention, + too_old_date, + returned_date +): + create_service_data_retention(sample_service.id, 'sms', days_of_retention) + create_inbound_sms(sample_service, created_at=too_old_date) + returned_inbound = create_inbound_sms(sample_service, created_at=returned_date) sms = admin_request.post('inbound_sms.get_inbound_sms_for_service', service_id=sample_service.id, _data={})