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/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..1e664fb75 100644 --- a/app/v2/notifications/get_notifications.py +++ b/app/v2/notifications/get_notifications.py @@ -1,9 +1,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 +17,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 +34,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/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index df7f8edf1..4f63c299d 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -42,7 +42,7 @@ def post_sms_notification(): send_notification_to_queue(notification, service.research_mode) resp = create_post_sms_response_from_notification(notification, - template_with_content.content, + template_with_content.replaced, service.sms_sender, request.url_root) return jsonify(resp), 201 @@ -70,7 +70,7 @@ def post_email_notification(): send_notification_to_queue(notification, service.research_mode) resp = create_post_email_response_from_notification(notification=notification, - content=template_with_content.content, + content=template_with_content.replaced, subject=template_with_content.subject, email_from=service.email_from, url_root=request.url_root) diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index 51b1e56db..477213666 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -1,4 +1,3 @@ -from datetime import datetime import pytest from freezegun import freeze_time @@ -22,9 +21,10 @@ from tests.app.conftest import ( @pytest.mark.parametrize('key_type', ['team', 'normal']) -def test_exception_thrown_by_redis_store_get_should_not_be_fatal( +def test_exception_thown_by_redis_store_get_should_not_be_fatal( notify_db, notify_db_session, + notify_api, key_type, mocker): mocker.patch('app.notifications.validators.redis_store.redis_store.get', side_effect=Exception("broken redis")) @@ -39,8 +39,7 @@ def test_exception_thrown_by_redis_store_get_should_not_be_fatal( assert e.value.status_code == 429 assert e.value.message == 'Exceeded send limits (4) for today' assert e.value.fields == [] - app.notifications.validators.redis_store.set\ - .assert_called_with("{}-{}-count".format(service.id, datetime.utcnow().strftime("%Y-%m-%d")), 5, ex=3600) + assert app.notifications.validators.redis_store.set.called @pytest.mark.parametrize('key_type', ['test', 'team', 'normal']) diff --git a/tests/app/v2/notifications/test_get_notifications.py b/tests/app/v2/notifications/test_get_notifications.py index 3c1b93134..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,6 +210,23 @@ 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, 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]) + + 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'] == "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): notifications = [ create_sample_notification(notify_db, notify_db_session, status=_status) @@ -230,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) 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()) } diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index a2d99fb64..81bf3ff44 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -7,23 +7,23 @@ from tests import create_authorization_header @pytest.mark.parametrize("reference", [None, "reference_from_client"]) -def test_post_sms_notification_returns_201(notify_api, sample_template, mocker, reference): +def test_post_sms_notification_returns_201(notify_api, sample_template_with_placeholders, mocker, reference): with notify_api.test_request_context(): with notify_api.test_client() as client: mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') data = { 'phone_number': '+447700900855', - 'template_id': str(sample_template.id) + 'template_id': str(sample_template_with_placeholders.id), + 'personalisation': {' Name': 'Jo'} } if reference: data.update({"reference": reference}) - auth_header = create_authorization_header(service_id=sample_template.service_id) + auth_header = create_authorization_header(service_id=sample_template_with_placeholders.service_id) response = client.post( path='/v2/notifications/sms', data=json.dumps(data), headers=[('Content-Type', 'application/json'), auth_header]) - assert response.status_code == 201 resp_json = json.loads(response.get_data(as_text=True)) notifications = Notification.query.all() @@ -31,12 +31,12 @@ def test_post_sms_notification_returns_201(notify_api, sample_template, mocker, notification_id = notifications[0].id assert resp_json['id'] == str(notification_id) assert resp_json['reference'] == reference - assert resp_json['content']['body'] == sample_template.content - assert resp_json['content']['from_number'] == sample_template.service.sms_sender + assert resp_json['content']['body'] == sample_template_with_placeholders.content.replace("(( Name))", "Jo") + assert resp_json['content']['from_number'] == sample_template_with_placeholders.service.sms_sender assert 'v2/notifications/{}'.format(notification_id) in resp_json['uri'] - assert resp_json['template']['id'] == str(sample_template.id) - assert resp_json['template']['version'] == sample_template.version - assert 'v2/templates/{}'.format(sample_template.id) in resp_json['template']['uri'] + assert resp_json['template']['id'] == str(sample_template_with_placeholders.id) + assert resp_json['template']['version'] == sample_template_with_placeholders.version + assert 'v2/templates/{}'.format(sample_template_with_placeholders.id) in resp_json['template']['uri'] assert mocked.called @@ -108,15 +108,16 @@ def test_post_sms_notification_returns_400_and_for_schema_problems(notify_api, s @pytest.mark.parametrize("reference", [None, "reference_from_client"]) -def test_post_email_notification_returns_201(client, sample_email_template, mocker, reference): +def test_post_email_notification_returns_201(client, sample_email_template_with_placeholders, mocker, reference): mocked = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') data = { - "email_address": sample_email_template.service.users[0].email_address, - "template_id": sample_email_template.id, + "email_address": sample_email_template_with_placeholders.service.users[0].email_address, + "template_id": sample_email_template_with_placeholders.id, + "personalisation": {"name": "Bob"} } if reference: data.update({"reference": reference}) - auth_header = create_authorization_header(service_id=sample_email_template.service_id) + auth_header = create_authorization_header(service_id=sample_email_template_with_placeholders.service_id) response = client.post( path="v2/notifications/email", data=json.dumps(data), @@ -127,13 +128,13 @@ def test_post_email_notification_returns_201(client, sample_email_template, mock assert resp_json['id'] == str(notification.id) assert resp_json['reference'] == reference assert notification.reference is None - assert resp_json['content']['body'] == sample_email_template.content - assert resp_json['content']['subject'] == sample_email_template.subject - assert resp_json['content']['from_email'] == sample_email_template.service.email_from + assert resp_json['content']['body'] == sample_email_template_with_placeholders.content.replace('((name))', 'Bob') + assert resp_json['content']['subject'] == sample_email_template_with_placeholders.subject + assert resp_json['content']['from_email'] == sample_email_template_with_placeholders.service.email_from assert 'v2/notifications/{}'.format(notification.id) in resp_json['uri'] - assert resp_json['template']['id'] == str(sample_email_template.id) - assert resp_json['template']['version'] == sample_email_template.version - assert 'v2/templates/{}'.format(sample_email_template.id) in resp_json['template']['uri'] + assert resp_json['template']['id'] == str(sample_email_template_with_placeholders.id) + assert resp_json['template']['version'] == sample_email_template_with_placeholders.version + assert 'v2/templates/{}'.format(sample_email_template_with_placeholders.id) in resp_json['template']['uri'] assert mocked.called