From ab990679b3e1c6e6961c37e4c0120bc33a185c20 Mon Sep 17 00:00:00 2001 From: Paul Craig Date: Mon, 28 Nov 2016 14:22:51 +0000 Subject: [PATCH 1/3] Replace marshmallow with jsonschema Created a new schema that accepts request parameters for the get_notifications v2 route. Using that to validate now instead of the marshmallow validation. Also changed the way formatted error messages are returned because the previous way was cutting off our failing `enum` messages. --- app/schema_validation/__init__.py | 4 +-- app/v2/notifications/get_notifications.py | 14 +++++--- app/v2/notifications/notification_schemas.py | 23 +++++++++++++ .../notifications/test_get_notifications.py | 34 +++++++++++++++++++ 4 files changed, 68 insertions(+), 7 deletions(-) diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py index ee8f27943..fe643ad61 100644 --- a/app/schema_validation/__init__.py +++ b/app/schema_validation/__init__.py @@ -42,6 +42,4 @@ def build_error_message(errors): def __format_message(e): - s = e.message.split("'") - msg = "{}{}".format(s[1], s[2]) - return msg if not e.cause else "{} {}".format(e.path[0], e.cause.message) + return e.message.replace("'", "") if not e.cause else "{} {}".format(e.path[0], e.cause.message) diff --git a/app/v2/notifications/get_notifications.py b/app/v2/notifications/get_notifications.py index 274df52d7..5158e563f 100644 --- a/app/v2/notifications/get_notifications.py +++ b/app/v2/notifications/get_notifications.py @@ -2,8 +2,9 @@ from flask import jsonify, request, url_for from app import api_user from app.dao import notifications_dao -from app.schemas import notifications_filter_schema +from app.schema_validation import validate from app.v2.notifications import notification_blueprint +from app.v2.notifications.notification_schemas import get_notifications_request @notification_blueprint.route("/", methods=['GET']) @@ -17,7 +18,12 @@ def get_notification_by_id(id): @notification_blueprint.route("", methods=['GET']) def get_notifications(): - data = notifications_filter_schema.load(request.args).data + _data = request.args.to_dict(flat=False) + if 'older_than' in _data: + # flat=False makes everything a list, but we only ever allow one value for "older_than" + _data['older_than'] = _data['older_than'][0] + + data = validate(_data, get_notifications_request) paginated_notifications = notifications_dao.get_notifications_for_service( str(api_user.service_id), @@ -29,11 +35,11 @@ def get_notifications(): def _build_links(notifications): _links = { - 'current': url_for(".get_notifications", _external=True, **request.args.to_dict(flat=False)), + 'current': url_for(".get_notifications", _external=True, **data), } if len(notifications): - next_query_params = dict(request.args.to_dict(flat=False), older_than=notifications[-1].id) + next_query_params = dict(data, older_than=notifications[-1].id) _links['next'] = url_for(".get_notifications", _external=True, **next_query_params) return _links diff --git a/app/v2/notifications/notification_schemas.py b/app/v2/notifications/notification_schemas.py index 934d6d400..016cb7503 100644 --- a/app/v2/notifications/notification_schemas.py +++ b/app/v2/notifications/notification_schemas.py @@ -1,3 +1,4 @@ +from app.models import NOTIFICATION_STATUS_TYPES, TEMPLATE_TYPES from app.schema_validation.definitions import (uuid, personalisation) # this may belong in a templates module @@ -72,6 +73,28 @@ get_notification_response = { ] } +get_notifications_request = { + "$schema": "http://json-schema.org/draft-04/schema#", + "description": "schema for query parameters allowed when getting list of notifications", + "type": "object", + "properties": { + "status": { + "type": "array", + "items": { + "enum": NOTIFICATION_STATUS_TYPES + } + }, + "template_type": { + "type": "array", + "items": { + "enum": TEMPLATE_TYPES + } + }, + "older_than": uuid + }, + "additionalProperties": False, +} + get_notifications_response = { "$schema": "http://json-schema.org/draft-04/schema#", "description": "GET list of notifications response schema", diff --git a/tests/app/v2/notifications/test_get_notifications.py b/tests/app/v2/notifications/test_get_notifications.py index 3c1b93134..adc8e48f3 100644 --- a/tests/app/v2/notifications/test_get_notifications.py +++ b/tests/app/v2/notifications/test_get_notifications.py @@ -156,6 +156,40 @@ def test_get_all_notifications_filter_by_single_status(client, notify_db, notify assert json_response['notifications'][0]['status'] == "pending" +@pytest.mark.parametrize('invalid_statuses, valid_statuses', [ + # one invalid status + (["elephant"], []), + # multiple invalid statuses + (["elephant", "giraffe", "cheetah"], []), + # one bad status and one good status + (["elephant"], ["created"]), +]) +def test_get_all_notifications_filter_by_status_invalid_status( + client, notify_db, notify_db_session, invalid_statuses, valid_statuses +): + notification = create_sample_notification(notify_db, notify_db_session, status="pending") + create_sample_notification(notify_db, notify_db_session) + + auth_header = create_authorization_header(service_id=notification.service_id) + response = client.get( + path='/v2/notifications?{}'.format( + "&".join(["status={}".format(status) for status in invalid_statuses + valid_statuses]) + ), + headers=[('Content-Type', 'application/json'), auth_header]) + + json_response = json.loads(response.get_data(as_text=True)) + partial_error_message = "is not one of " \ + "[created, sending, delivered, pending, failed, technical-failure, temporary-failure, permanent-failure]" + + assert response.status_code == 400 + assert response.headers['Content-type'] == "application/json" + + assert json_response['status_code'] == 400 + assert len(json_response['errors']) == len(invalid_statuses) + for index, invalid_status in enumerate(invalid_statuses): + assert json_response['errors'][index]['message'] == "{} {}".format(invalid_status, partial_error_message) + + def test_get_all_notifications_filter_by_multiple_statuses(client, notify_db, notify_db_session): notifications = [ create_sample_notification(notify_db, notify_db_session, status=_status) From 2ca675eb731685b243157480e3e335f367d667ea Mon Sep 17 00:00:00 2001 From: Paul Craig Date: Mon, 28 Nov 2016 15:10:13 +0000 Subject: [PATCH 2/3] Test invalid statuses and template_types Run through a few scenarios in the `test_schema_notifications.py` test file, calling `.validate()` on some data instead of actually calling the route. --- .../notifications/test_get_notifications.py | 24 ++----- .../test_notification_schemas.py | 70 +++++++++++++++++++ 2 files changed, 75 insertions(+), 19 deletions(-) diff --git a/tests/app/v2/notifications/test_get_notifications.py b/tests/app/v2/notifications/test_get_notifications.py index adc8e48f3..b93a0274c 100644 --- a/tests/app/v2/notifications/test_get_notifications.py +++ b/tests/app/v2/notifications/test_get_notifications.py @@ -156,38 +156,24 @@ def test_get_all_notifications_filter_by_single_status(client, notify_db, notify assert json_response['notifications'][0]['status'] == "pending" -@pytest.mark.parametrize('invalid_statuses, valid_statuses', [ - # one invalid status - (["elephant"], []), - # multiple invalid statuses - (["elephant", "giraffe", "cheetah"], []), - # one bad status and one good status - (["elephant"], ["created"]), -]) -def test_get_all_notifications_filter_by_status_invalid_status( - client, notify_db, notify_db_session, invalid_statuses, valid_statuses -): +def test_get_all_notifications_filter_by_status_invalid_status(client, notify_db, notify_db_session): notification = create_sample_notification(notify_db, notify_db_session, status="pending") create_sample_notification(notify_db, notify_db_session) auth_header = create_authorization_header(service_id=notification.service_id) response = client.get( - path='/v2/notifications?{}'.format( - "&".join(["status={}".format(status) for status in invalid_statuses + valid_statuses]) - ), + path='/v2/notifications?status=elephant', headers=[('Content-Type', 'application/json'), auth_header]) json_response = json.loads(response.get_data(as_text=True)) - partial_error_message = "is not one of " \ - "[created, sending, delivered, pending, failed, technical-failure, temporary-failure, permanent-failure]" assert response.status_code == 400 assert response.headers['Content-type'] == "application/json" assert json_response['status_code'] == 400 - assert len(json_response['errors']) == len(invalid_statuses) - for index, invalid_status in enumerate(invalid_statuses): - assert json_response['errors'][index]['message'] == "{} {}".format(invalid_status, partial_error_message) + assert len(json_response['errors']) == 1 + assert json_response['errors'][0]['message'] == "elephant is not one of [created, sending, delivered, " \ + "pending, failed, technical-failure, temporary-failure, permanent-failure]" def test_get_all_notifications_filter_by_multiple_statuses(client, notify_db, notify_db_session): diff --git a/tests/app/v2/notifications/test_notification_schemas.py b/tests/app/v2/notifications/test_notification_schemas.py index 73afbb29e..bd54d3e8f 100644 --- a/tests/app/v2/notifications/test_notification_schemas.py +++ b/tests/app/v2/notifications/test_notification_schemas.py @@ -5,10 +5,80 @@ from flask import json from jsonschema import ValidationError from app.v2.notifications.notification_schemas import ( + get_notifications_request, post_sms_request, post_sms_response, post_email_request, post_email_response ) from app.schema_validation import validate + +@pytest.mark.parametrize('invalid_statuses, valid_statuses', [ + # one invalid status + (["elephant"], []), + # multiple invalid statuses + (["elephant", "giraffe", "cheetah"], []), + # one bad status and one good status + (["elephant"], ["created"]), +]) +def test_get_notifications_request_invalid_statuses( + invalid_statuses, valid_statuses +): + partial_error_status = "is not one of " \ + "[created, sending, delivered, pending, failed, technical-failure, temporary-failure, permanent-failure]" + + with pytest.raises(ValidationError) as e: + validate({'status': invalid_statuses + valid_statuses}, get_notifications_request) + + errors = json.loads(e.value.message).get('errors') + assert len(errors) == len(invalid_statuses) + for index, value in enumerate(invalid_statuses): + assert errors[index]['message'] == "{} {}".format(value, partial_error_status) + + +@pytest.mark.parametrize('invalid_template_types, valid_template_types', [ + # one invalid template_type + (["orange"], []), + # multiple invalid template_types + (["orange", "avocado", "banana"], []), + # one bad template_type and one good template_type + (["orange"], ["sms"]), +]) +def test_get_notifications_request_invalid_template_types( + invalid_template_types, valid_template_types +): + partial_error_template_type = "is not one of [sms, email, letter]" + + with pytest.raises(ValidationError) as e: + validate({'template_type': invalid_template_types + valid_template_types}, get_notifications_request) + + errors = json.loads(e.value.message).get('errors') + assert len(errors) == len(invalid_template_types) + for index, value in enumerate(invalid_template_types): + assert errors[index]['message'] == "{} {}".format(value, partial_error_template_type) + + +def test_get_notifications_request_invalid_statuses_and_template_types(): + with pytest.raises(ValidationError) as e: + validate({ + 'status': ["created", "elephant", "giraffe"], + 'template_type': ["sms", "orange", "avocado"] + }, get_notifications_request) + + errors = json.loads(e.value.message).get('errors') + + assert len(errors) == 4 + + error_messages = [error['message'] for error in errors] + + for invalid_status in ["elephant", "giraffe"]: + assert "{} is not one of [created, sending, delivered, " \ + "pending, failed, technical-failure, temporary-failure, permanent-failure]".format( + invalid_status + ) in error_messages + + for invalid_template_type in ["orange", "avocado"]: + assert "{} is not one of [sms, email, letter]".format(invalid_template_type) in error_messages + + valid_json = {"phone_number": "07515111111", "template_id": str(uuid.uuid4()) } From 4082d38c0c5f95af718b552d0492a3439770d197 Mon Sep 17 00:00:00 2001 From: Paul Craig Date: Mon, 28 Nov 2016 15:12:03 +0000 Subject: [PATCH 3/3] Test invalid older_than, template_types, and bad ids Come up with some simple tests in the routes, just to see we get back what we expect as errors. --- app/dao/notifications_dao.py | 2 +- app/v2/notifications/get_notifications.py | 1 - .../notifications/test_get_notifications.py | 76 +++++++++++++++++-- 3 files changed, 71 insertions(+), 8 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 86cdda671..802800068 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -8,7 +8,7 @@ from itertools import groupby from flask import current_app from werkzeug.datastructures import MultiDict -from sqlalchemy import (desc, func, or_, and_, asc, cast, Text) +from sqlalchemy import (desc, func, or_, and_, asc) from sqlalchemy.orm import joinedload from app import db, create_uuid diff --git a/app/v2/notifications/get_notifications.py b/app/v2/notifications/get_notifications.py index 5158e563f..1e664fb75 100644 --- a/app/v2/notifications/get_notifications.py +++ b/app/v2/notifications/get_notifications.py @@ -1,5 +1,4 @@ from flask import jsonify, request, url_for - from app import api_user from app.dao import notifications_dao from app.schema_validation import validate diff --git a/tests/app/v2/notifications/test_get_notifications.py b/tests/app/v2/notifications/test_get_notifications.py index b93a0274c..eecf58b0a 100644 --- a/tests/app/v2/notifications/test_get_notifications.py +++ b/tests/app/v2/notifications/test_get_notifications.py @@ -60,6 +60,44 @@ def test_get_notification_by_id_returns_200( assert json_response == expected_response +def test_get_notification_by_id_nonexistent_id(client, sample_notification): + auth_header = create_authorization_header(service_id=sample_notification.service_id) + response = client.get( + path='/v2/notifications/dd4b8b9d-d414-4a83-9256-580046bf18f9', + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 404 + assert response.headers['Content-type'] == 'application/json' + + json_response = json.loads(response.get_data(as_text=True)) + assert json_response == { + "errors": [ + { + "error": "NoResultFound", + "message": "No result found" + } + ], + "status_code": 404 + } + + +def test_get_notification_by_id_invalid_id(client, sample_notification): + auth_header = create_authorization_header(service_id=sample_notification.service_id) + response = client.get( + path='/v2/notifications/1234-badly-formatted-id-7890', + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 404 + assert response.headers['Content-type'] == 'application/json' + + json_response = json.loads(response.get_data(as_text=True)) + assert json_response == { + "message": "The requested URL was not found on the server. " + "If you entered the URL manually please check your spelling and try again.", + "result": "error" + } + + def test_get_all_notifications_returns_200(client, notify_db, notify_db_session): notifications = [create_sample_notification(notify_db, notify_db_session) for _ in range(2)] notification = notifications[-1] @@ -88,7 +126,7 @@ def test_get_all_notifications_returns_200(client, notify_db, notify_db_session) assert json_response['notifications'][0]['type'] == "sms" -def test_get_all_notifications_no_notifications_if_no_notificatons(client, sample_service): +def test_get_all_notifications_no_notifications_if_no_notifications(client, sample_service): auth_header = create_authorization_header(service_id=sample_service.id) response = client.get( path='/v2/notifications', @@ -135,6 +173,22 @@ def test_get_all_notifications_filter_by_template_type(client, notify_db, notify assert json_response['notifications'][0]['type'] == "email" +def test_get_all_notifications_filter_by_template_type_invalid_template_type(client, sample_notification): + auth_header = create_authorization_header(service_id=sample_notification.service_id) + response = client.get( + path='/v2/notifications?template_type=orange', + headers=[('Content-Type', 'application/json'), auth_header]) + + json_response = json.loads(response.get_data(as_text=True)) + + assert response.status_code == 400 + assert response.headers['Content-type'] == "application/json" + + assert json_response['status_code'] == 400 + assert len(json_response['errors']) == 1 + assert json_response['errors'][0]['message'] == "orange is not one of [sms, email, letter]" + + def test_get_all_notifications_filter_by_single_status(client, notify_db, notify_db_session): notification = create_sample_notification(notify_db, notify_db_session, status="pending") create_sample_notification(notify_db, notify_db_session) @@ -156,11 +210,8 @@ def test_get_all_notifications_filter_by_single_status(client, notify_db, notify assert json_response['notifications'][0]['status'] == "pending" -def test_get_all_notifications_filter_by_status_invalid_status(client, notify_db, notify_db_session): - notification = create_sample_notification(notify_db, notify_db_session, status="pending") - create_sample_notification(notify_db, notify_db_session) - - auth_header = create_authorization_header(service_id=notification.service_id) +def test_get_all_notifications_filter_by_status_invalid_status(client, sample_notification): + auth_header = create_authorization_header(service_id=sample_notification.service_id) response = client.get( path='/v2/notifications?status=elephant', headers=[('Content-Type', 'application/json'), auth_header]) @@ -250,6 +301,19 @@ def test_get_all_notifications_filter_by_id(client, notify_db, notify_db_session assert json_response['notifications'][0]['id'] == str(older_notification.id) +def test_get_all_notifications_filter_by_id_invalid_id(client, sample_notification): + auth_header = create_authorization_header(service_id=sample_notification.service_id) + response = client.get( + path='/v2/notifications?older_than=1234-badly-formatted-id-7890', + headers=[('Content-Type', 'application/json'), auth_header]) + + json_response = json.loads(response.get_data(as_text=True)) + + assert json_response['status_code'] == 400 + assert len(json_response['errors']) == 1 + assert json_response['errors'][0]['message'] == "older_than is not a valid UUID" + + def test_get_all_notifications_filter_by_id_no_notifications_if_nonexistent_id(client, notify_db, notify_db_session): notification = create_sample_notification(notify_db, notify_db_session)