Refactor to use query string rather than path params

app/v2/inbound_sms/get_inbound_sms.py

@@ -1,35 +1,33 @@
-import uuid
-
 from flask import jsonify, request, url_for, current_app
-from sqlalchemy.orm.exc import NoResultFound
-from werkzeug.exceptions import abort
 
 from notifications_utils.recipients import validate_and_format_phone_number
 from notifications_utils.recipients import InvalidPhoneError
 
 from app import authenticated_service
 from app.dao import inbound_sms_dao
+from app.schema_validation import validate
 from app.v2.errors import BadRequestError
 from app.v2.inbound_sms import v2_inbound_sms_blueprint
+from app.v2.inbound_sms.inbound_sms_schemas import get_inbound_sms_request
 
 
-@v2_inbound_sms_blueprint.route("/<user_number>", methods=['GET'])
-def get_inbound_sms_by_number(user_number):
-    _data = request.args.to_dict(flat=False)
+@v2_inbound_sms_blueprint.route("", methods=['GET'])
+def get_inbound_sms():
+    data = validate(request.args.to_dict(), get_inbound_sms_request)
 
-    # flat=False makes everything a list, but we only ever allow one value for "older_than"
-    if 'older_than' in _data:
-        _data['older_than'] = _data['older_than'][0]
+    if data.get('user_number'):
+        try:
+            data['user_number'] = validate_and_format_phone_number(data.get('user_number'))
+        except InvalidPhoneError as e:
+            raise BadRequestError(message=str(e))
 
-    try:
-        user_number = validate_and_format_phone_number(user_number)
-    except InvalidPhoneError as e:
-        raise BadRequestError(message=str(e))
+    user_number = data.get('user_number', None)
+    older_than = data.get('older_than', None)
 
     paginated_inbound_sms = inbound_sms_dao.dao_get_paginated_inbound_sms_for_service(
         authenticated_service.id,
         user_number=user_number,
-        older_than=_data.get('older_than'),
+        older_than=older_than,
         page_size=current_app.config.get('API_PAGE_SIZE')
     )
 
@@ -37,36 +35,14 @@ def get_inbound_sms_by_number(user_number):
         received_text_messages=[i.serialize() for i in paginated_inbound_sms],
         links=_build_links(
             paginated_inbound_sms,
-            endpoint='get_inbound_sms_by_number',
-            user_number=user_number
-        )
+            user_number=user_number)
     ), 200
 
 
-@v2_inbound_sms_blueprint.route("", methods=['GET'])
-def get_all_inbound_sms():
-    _data = request.args.to_dict(flat=False)
-
-    # flat=False makes everything a list, but we only ever allow one value for "older_than"
-    if 'older_than' in _data:
-        _data['older_than'] = _data['older_than'][0]
-
-    paginated_inbound_sms = inbound_sms_dao.dao_get_paginated_inbound_sms_for_service(
-        authenticated_service.id,
-        older_than=_data.get('older_than'),
-        page_size=current_app.config.get('API_PAGE_SIZE')
-    )
-
-    return jsonify(
-        received_text_messages=[i.serialize() for i in paginated_inbound_sms],
-        links=_build_links(paginated_inbound_sms, endpoint='get_all_inbound_sms')
-    ), 200
-
-
-def _build_links(inbound_sms_list, endpoint, user_number=None):
+def _build_links(inbound_sms_list, user_number=None):
     _links = {
         'current': url_for(
-            "v2_inbound_sms.{}".format(endpoint),
+            "v2_inbound_sms.get_inbound_sms",
             user_number=user_number,
             _external=True,
         ),
@@ -74,7 +50,7 @@ def _build_links(inbound_sms_list, endpoint, user_number=None):
 
     if inbound_sms_list:
         _links['next'] = url_for(
-            "v2_inbound_sms.{}".format(endpoint),
+            "v2_inbound_sms.get_inbound_sms",
             user_number=user_number,
             older_than=inbound_sms_list[-1].id,
             _external=True,

app/v2/inbound_sms/inbound_sms_schemas.py

@@ -1,6 +1,18 @@
 from app.schema_validation.definitions import uuid
 
 
+get_inbound_sms_request = {
+    "$schema": "http://json-schema.org/draft-04/schema#",
+    "description": "schema for query parameters allowed when getting list of received text messages",
+    "type": "object",
+    "properties": {
+        "older_than": uuid,
+        "user_number": {"type": "string"}
+    },
+    "additionalProperties": False,
+}
+
+
 get_inbound_sms_single_response = {
     "$schema": "http://json-schema.org/draft-04/schema#",
     "description": "GET inbound sms schema response",
@@ -29,6 +41,7 @@ get_inbound_sms_single_response = {
         "user_number", "created_at", "service_id",
         "notify_number", "content"
     ],
+    "additionalProperties": False,
 }
 
 get_inbound_sms_response = {
@@ -60,5 +73,6 @@ get_inbound_sms_response = {
     "required": ["received_text_messages", "links"],
     "definitions": {
         "inbound_sms": get_inbound_sms_single_response
-    }
+    },
+    "additionalProperties": False,
 }

tests/app/v2/inbound_sms/test_get_inbound_sms.py

@@ -1,8 +1,6 @@
-import datetime
 import pytest
 from flask import json, url_for
 
-from app import DATETIME_FORMAT
 from tests import create_authorization_header
 from tests.app.db import create_inbound_sms
 
@@ -34,12 +32,9 @@ def test_get_all_inbound_sms_returns_200(
     assert json_response == expected_response
 
 
-@pytest.mark.parametrize('inbound_sms_path,user_number', [
-    ('v2_inbound_sms.get_all_inbound_sms', None),
-    ('v2_inbound_sms.get_inbound_sms_by_number', '447700900111')
-])
+@pytest.mark.parametrize('user_number', [None, '447700900111'])
 def test_get_inbound_sms_generate_page_links(
-        client, sample_service, mocker, inbound_sms_path, user_number
+        client, sample_service, mocker, user_number
 ):
     mocker.patch.dict(
         "app.v2.inbound_sms.get_inbound_sms.current_app.config",
@@ -55,7 +50,7 @@ def test_get_inbound_sms_generate_page_links(
 
     auth_header = create_authorization_header(service_id=sample_service.id)
     response = client.get(
-        path=url_for(inbound_sms_path, user_number=user_number),
+        url_for('v2_inbound_sms.get_inbound_sms', user_number=user_number),
         headers=[('Content-Type', 'application/json'), auth_header])
 
     assert response.status_code == 200
@@ -65,22 +60,19 @@ def test_get_inbound_sms_generate_page_links(
 
     assert json_response['received_text_messages'] == expected_inbound_sms_list
     assert url_for(
-        inbound_sms_path,
+        'v2_inbound_sms.get_inbound_sms',
         user_number=user_number,
         _external=True) == json_response['links']['current']
     assert url_for(
-        inbound_sms_path,
+        'v2_inbound_sms.get_inbound_sms',
         user_number=user_number,
         older_than=reversed_inbound_sms[1].id,
         _external=True) == json_response['links']['next']
 
 
-@pytest.mark.parametrize('inbound_sms_path,user_number', [
-    ('v2_inbound_sms.get_all_inbound_sms', None),
-    ('v2_inbound_sms.get_inbound_sms_by_number', '447700900111')
-])
+@pytest.mark.parametrize('user_number', [None, '447700900111'])
 def test_get_next_inbound_sms_will_get_correct_inbound_sms_list(
-        client, sample_service, mocker, inbound_sms_path, user_number
+        client, sample_service, mocker, user_number
 ):
     mocker.patch.dict(
         "app.v2.inbound_sms.get_inbound_sms.current_app.config",
@@ -96,7 +88,7 @@ def test_get_next_inbound_sms_will_get_correct_inbound_sms_list(
 
     auth_header = create_authorization_header(service_id=sample_service.id)
     response = client.get(
-        path=url_for(inbound_sms_path, user_number=user_number, older_than=reversed_inbound_sms[1].id),
+        path=url_for('v2_inbound_sms.get_inbound_sms', user_number=user_number, older_than=reversed_inbound_sms[1].id),
         headers=[('Content-Type', 'application/json'), auth_header])
 
     assert response.status_code == 200
@@ -106,22 +98,19 @@ def test_get_next_inbound_sms_will_get_correct_inbound_sms_list(
 
     assert json_response['received_text_messages'] == expected_inbound_sms_list
     assert url_for(
-        inbound_sms_path,
+        'v2_inbound_sms.get_inbound_sms',
         user_number=user_number,
         _external=True) == json_response['links']['current']
     assert url_for(
-        inbound_sms_path,
+        'v2_inbound_sms.get_inbound_sms',
         user_number=user_number,
         older_than=reversed_inbound_sms[3].id,
         _external=True) == json_response['links']['next']
 
 
-@pytest.mark.parametrize('inbound_sms_path,user_number', [
-    ('v2_inbound_sms.get_all_inbound_sms', None),
-    ('v2_inbound_sms.get_inbound_sms_by_number', '447700900111')
-])
+@pytest.mark.parametrize('user_number', [None, '447700900111'])
 def test_get_next_inbound_sms_at_end_will_return_empty_inbound_sms_list(
-        client, sample_inbound_sms, mocker, inbound_sms_path, user_number
+        client, sample_inbound_sms, mocker, user_number
 ):
     mocker.patch.dict(
         "app.v2.inbound_sms.get_inbound_sms.current_app.config",
@@ -130,7 +119,7 @@ def test_get_next_inbound_sms_at_end_will_return_empty_inbound_sms_list(
 
     auth_header = create_authorization_header(service_id=sample_inbound_sms.service.id)
     response = client.get(
-        path=url_for(inbound_sms_path, user_number=user_number, older_than=sample_inbound_sms.id),
+        path=url_for('v2_inbound_sms.get_inbound_sms', user_number=user_number, older_than=sample_inbound_sms.id),
         headers=[('Content-Type', 'application/json'), auth_header])
 
     assert response.status_code == 200
@@ -139,7 +128,7 @@ def test_get_next_inbound_sms_at_end_will_return_empty_inbound_sms_list(
     expected_inbound_sms_list = []
     assert json_response['received_text_messages'] == expected_inbound_sms_list
     assert url_for(
-        inbound_sms_path,
+        'v2_inbound_sms.get_inbound_sms',
         user_number=user_number,
         _external=True) == json_response['links']['current']
     assert 'next' not in json_response['links'].keys()
@@ -178,7 +167,7 @@ def test_get_inbound_sms_by_number_returns_200(
 
     auth_header = create_authorization_header(service_id=sample_service.id)
     response = client.get(
-        path='/v2/received-text-messages/{}'.format(requested_number),
+        path='/v2/received-text-messages?user_number={}'.format(requested_number),
         headers=[('Content-Type', 'application/json'), auth_header])
 
     assert response.status_code == 200
@@ -209,10 +198,27 @@ def test_get_inbound_sms_for_no_inbound_sms_returns_200(
     assert json_response == expected_response
 
 
+def test_get_inbound_sms_with_invalid_query_string_returns_400(client, sample_service):
+    auth_header = create_authorization_header(service_id=sample_service.id)
+    response = client.get(
+        path='/v2/received-text-messages?usernumber=447700900000',
+        headers=[('Content-Type', 'application/json'), auth_header])
+
+    assert response.status_code == 400
+    assert response.headers['Content-type'] == 'application/json'
+
+    json_response = json.loads(response.get_data(as_text=True))
+
+    assert json_response['status_code'] == 400
+    assert json_response['errors'][0]['error'] == 'ValidationError'
+    assert json_response['errors'][0]['message'] == \
+        'Additional properties are not allowed (usernumber was unexpected)'
+
+
 def test_get_inbound_sms_by_nonexistent_number(client, sample_service):
     auth_header = create_authorization_header(service_id=sample_service.id)
     response = client.get(
-        path='/v2/received-text-messages/447700900000',
+        path='/v2/received-text-messages?user_number=447700900000',
         headers=[('Content-Type', 'application/json'), auth_header])
 
     assert response.status_code == 200
@@ -233,7 +239,7 @@ def test_get_inbound_sms_by_invalid_number(
         client, sample_service, invalid_number, expected_message):
     auth_header = create_authorization_header(service_id=sample_service.id)
     response = client.get(
-        path='/v2/received-text-messages/{}'.format(invalid_number),
+        path='/v2/received-text-messages?user_number={}'.format(invalid_number),
         headers=[('Content-Type', 'application/json'), auth_header])
 
     assert response.status_code == 400

tests/app/v2/inbound_sms/test_inbound_sms_schemas.py

@@ -6,6 +6,7 @@ from app.v2.inbound_sms.inbound_sms_schemas import get_inbound_sms_response, get
 from app.schema_validation import validate
 
 from tests import create_authorization_header
+from tests.app.db import create_inbound_sms
 
 
 valid_inbound_sms = {
@@ -44,7 +45,7 @@ invalid_inbound_sms_list = {
 
 def test_get_inbound_sms_contract(client, sample_inbound_sms):
     auth_header = create_authorization_header(service_id=sample_inbound_sms.service_id)
-    response = client.get('/v2/received-text-messages/{}'.format(sample_inbound_sms.user_number), headers=[auth_header])
+    response = client.get('/v2/received-text-messages', headers=[auth_header])
     response_json = json.loads(response.get_data(as_text=True))
 
     assert validate(response_json, get_inbound_sms_response)['received_text_messages'][0] \