From a5e07d8affd4d815d7c5effe4020e0acf3c569b9 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 25 Oct 2016 14:53:31 +0100 Subject: [PATCH 1/9] V2 schemas for post sms notifications, post_sms_request and post_sms_response --- app/schema_validation/__init__.py | 26 ++++++ app/schema_validation/definitions.py | 20 ++++ app/v2/notifications/notification_schemas.py | 56 ++++++++++++ tests/app/v2/__init__.py | 0 tests/app/v2/notifications/__init__.py | 0 .../test_notification_schemas.py | 91 +++++++++++++++++++ 6 files changed, 193 insertions(+) create mode 100644 app/schema_validation/__init__.py create mode 100644 app/schema_validation/definitions.py create mode 100644 app/v2/notifications/notification_schemas.py create mode 100644 tests/app/v2/__init__.py create mode 100644 tests/app/v2/notifications/__init__.py create mode 100644 tests/app/v2/notifications/test_notification_schemas.py diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py new file mode 100644 index 000000000..98f308637 --- /dev/null +++ b/app/schema_validation/__init__.py @@ -0,0 +1,26 @@ +import json +from jsonschema import Draft4Validator, ValidationError + + +def validate(json_to_validate, schema): + validator = Draft4Validator(schema) + errors = list(validator.iter_errors(json_to_validate)) + if errors.__len__() > 0: + raise ValidationError(build_error_message(errors, schema)) + return json_to_validate + + +def build_error_message(errors, schema): + fields = [] + for e in errors: + field = "'{}' {}".format(e.path[0], e.schema.get('validationMessage')) if e.schema.get( + 'validationMessage') else e.message + fields.append(field) + message = { + "code": "1001", + "message": "Validation error occurred - {}".format(schema['title']), + "link": "link to error documentation (not yet implemented)", + "fields": fields + } + + return json.dumps(message) \ No newline at end of file diff --git a/app/schema_validation/definitions.py b/app/schema_validation/definitions.py new file mode 100644 index 000000000..e65566d7b --- /dev/null +++ b/app/schema_validation/definitions.py @@ -0,0 +1,20 @@ +""" +Definitions are intended for schema definitions that are not likely to change from version to version. +If the definition is specific to a version put it in a definition file in the version package +""" + +uuid = { + "type": "string", + "pattern": "^[a-fA-F0-9]{8}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{12}$", + "validationMessage": "not a valid UUID", + "code": "1001", # yet to be implemented + "link": "link to our error documentation not yet implemented" +} + + +personalisation = { + "type": "object", + "validationMessage": "should contain key value pairs", + "code": "1001", # yet to be implemented + "link": "link to our error documentation not yet implemented" +} diff --git a/app/v2/notifications/notification_schemas.py b/app/v2/notifications/notification_schemas.py new file mode 100644 index 000000000..24bb7c1de --- /dev/null +++ b/app/v2/notifications/notification_schemas.py @@ -0,0 +1,56 @@ +from app.schema_validation.definitions import (uuid, personalisation) + +post_sms_request = { + "$schema": "http://json-schema.org/draft-04/schema#", + "description": "POST sms notification schema", + "type": "object", + "title": "POST v2/notifications/sms", + "properties": { + "reference": {"type": "string"}, + "phone_number": {"type": "string", "format": "sms"}, + "template_id": uuid, + "personalisation": personalisation + }, + "required": ["phone_number", "template_id"] +} + +content = { + "$schema": "http://json-schema.org/draft-04/schema#", + "description": "POST sms notification response schema", + "type": "object", + "title": "notification content", + "properties": { + "body": {"type": "string"}, + "from_number": {"type": "string"} + }, + "required": ["body"] +} + +# this may belong in a templates module +template = { + "$schema": "http://json-schema.org/draft-04/schema#", + "description": "POST sms notification response schema", + "type": "object", + "title": "notification content", + "properties": { + "id": uuid, + "version": {"type": "integer"}, + "uri": {"type": "string"} + }, + "required": ["id", "version", "uri"] +} + +post_sms_response = { + "$schema": "http://json-schema.org/draft-04/schema#", + "description": "POST sms notification response schema", + "type": "object", + "title": "response v2/notifications/sms", + "properties": { + "id": uuid, + "reference": {"type": "string"}, + "content": content, + "uri": {"type": "string"}, + "template": template + }, + "required": ["id", "content", "uri", "template"] +} diff --git a/tests/app/v2/__init__.py b/tests/app/v2/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/app/v2/notifications/__init__.py b/tests/app/v2/notifications/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/app/v2/notifications/test_notification_schemas.py b/tests/app/v2/notifications/test_notification_schemas.py new file mode 100644 index 000000000..cbccf3653 --- /dev/null +++ b/tests/app/v2/notifications/test_notification_schemas.py @@ -0,0 +1,91 @@ +import uuid + +import pytest +from flask import json +from jsonschema import ValidationError + +from app.v2.notifications.notification_schemas import post_sms_request, post_sms_response +from app.schema_validation import validate + +valid_json = {"phone_number": "07515111111", + "template_id": str(uuid.uuid4()) + } +valid_json_with_optionals = { + "phone_number": "07515111111", + "template_id": str(uuid.uuid4()), + "reference": "reference from caller", + "personalisation": {"key": "value"} +} + + +@pytest.mark.parametrize("input", [valid_json, valid_json_with_optionals]) +def test_post_sms_schema_is_valid(input): + validate(input, post_sms_request) + + +def test_post_sms_json_schema_bad_uuid_and_missing_phone_number(): + j = {"template_id": "notUUID"} + try: + validate(j, post_sms_request) + except ValidationError as e: + error = json.loads(e.message) + assert "POST v2/notifications/sms" in error['message'] + assert len(error.get('fields')) == 2 + assert "phone_number" in e.message + assert "template_id" in e.message + assert error.get('code') == '1001' + assert error.get('link', None) is not None + + +def test_post_sms_schema_with_personalisation_that_is_not_a_dict(): + j = { + "phone_number": "07515111111", + "template_id": str(uuid.uuid4()), + "reference": "reference from caller", + "personalisation": "not_a_dict" + } + try: + validate(j, post_sms_request) + except ValidationError as e: + error = json.loads(e.message) + assert "POST v2/notifications/sms" in error['message'] + assert len(error.get('fields')) == 1 + assert "personalisation" in e.message + assert error.get('code') == '1001' + assert error.get('link', None) is not None + + +valid_response = { + "id": str(uuid.uuid4()), + "content": {"body": "contents of message", + "from_number": "46045"}, + "uri": "/v2/notifications/id", + "template": {"id": str(uuid.uuid4()), + "version": 1, + "uri": "/v2/template/id"} +} + +valid_response_with_optionals = { + "id": str(uuid.uuid4()), + "reference": "reference_from_service", + "content": {"body": "contents of message", + "from_number": "46045"}, + "uri": "/v2/notifications/id", + "template": {"id": str(uuid.uuid4()), + "version": 1, + "uri": "/v2/template/id"} +} + + +@pytest.mark.parametrize('input', [valid_response]) +def test_post_sms_response_schema_is_valid(input): + validate(input, post_sms_response) + + +def test_post_sms_response_schema_missing_uri(): + j = valid_response + del j["uri"] + try: + validate(j, post_sms_response) + except ValidationError as e: + assert 'uri' in e.message From 23a4f00e567e6249d84cfda8dd7bfcc8ef11f234 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 25 Oct 2016 18:04:03 +0100 Subject: [PATCH 2/9] New package structure for the version 2 of the public api. Start building up the validators required for post notificaiton. The app/v2/errors.py is a rough sketch, will be passed a code, the error can look up the message and link for the error message. --- app/notifications/process_notifications.py | 14 +++++ app/notifications/validators.py | 25 +++++++++ app/schema_validation/__init__.py | 2 +- app/v2/__init__.py | 0 app/v2/errors.py | 29 +++++++++++ app/v2/notifications/__init__.py | 7 +++ app/v2/notifications/get_notifications.py | 14 +++++ app/v2/notifications/post_notifications.py | 49 ++++++++++++++++++ tests/app/notifications/test_validators.py | 60 ++++++++++++++++++++++ 9 files changed, 199 insertions(+), 1 deletion(-) create mode 100644 app/notifications/process_notifications.py create mode 100644 app/notifications/validators.py create mode 100644 app/v2/__init__.py create mode 100644 app/v2/errors.py create mode 100644 app/v2/notifications/__init__.py create mode 100644 app/v2/notifications/get_notifications.py create mode 100644 app/v2/notifications/post_notifications.py create mode 100644 tests/app/notifications/test_validators.py diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py new file mode 100644 index 000000000..c82dc519e --- /dev/null +++ b/app/notifications/process_notifications.py @@ -0,0 +1,14 @@ +def persist_notification(): + ''' + persist the notification + :return: + ''' + pass + + +def send_notificaiton_to_queue(): + ''' + send the notification to the queue + :return: + ''' + pass diff --git a/app/notifications/validators.py b/app/notifications/validators.py new file mode 100644 index 000000000..47bc847ba --- /dev/null +++ b/app/notifications/validators.py @@ -0,0 +1,25 @@ +from app.dao import services_dao +from app.errors import InvalidRequest +from app.models import KEY_TYPE_TEST + + +def check_service_message_limit(key_type, service): + if all((key_type != KEY_TYPE_TEST, + service.restricted)): + service_stats = services_dao.fetch_todays_total_message_count(service.id) + if service_stats >= service.message_limit: + error = 'Exceeded send limits ({}) for today'.format(service.message_limit) + + raise InvalidRequest(error, status_code=429) + + +def check_template_is_for_notification_type(notification_type, template_type): + if notification_type != template_type: + raise InvalidRequest("{0} template is not suitable for {1} notification".format(template_type, + notification_type), + status_code=400) + + +def check_template_is_active(template): + if template.archived: + raise InvalidRequest('Template has been deleted', status_code=400) diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py index 98f308637..84144601b 100644 --- a/app/schema_validation/__init__.py +++ b/app/schema_validation/__init__.py @@ -23,4 +23,4 @@ def build_error_message(errors, schema): "fields": fields } - return json.dumps(message) \ No newline at end of file + return json.dumps(message) diff --git a/app/v2/__init__.py b/app/v2/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/app/v2/errors.py b/app/v2/errors.py new file mode 100644 index 000000000..01577b456 --- /dev/null +++ b/app/v2/errors.py @@ -0,0 +1,29 @@ +from flask import jsonify + +from app.errors import InvalidRequest + + +class BadRequestError(Exception): + status_code = 400 + + def __init__(self, message, fields, code): + self.code = code + self.message = message + self.fields = fields + + +def register_errors(blueprint): + @blueprint.app_errorhandler(Exception) + def authentication_error(error): + # v2 error format - NOT this + return jsonify(result='error', message=error.message), error.code + + @blueprint.app_errorhandler(InvalidRequest) + def handle_invalid_request(error): + # { + # "code", + # "link", + # "message" , + # "fields": + # } + return "build_error_message" diff --git a/app/v2/notifications/__init__.py b/app/v2/notifications/__init__.py new file mode 100644 index 000000000..f805256c0 --- /dev/null +++ b/app/v2/notifications/__init__.py @@ -0,0 +1,7 @@ +from flask import Blueprint + +from app.v2.errors import register_errors + +notification_blueprint = Blueprint(__name__, __name__, url_prefix='/v2/notifications') + +register_errors(notification_blueprint) diff --git a/app/v2/notifications/get_notifications.py b/app/v2/notifications/get_notifications.py new file mode 100644 index 000000000..b2fee08e0 --- /dev/null +++ b/app/v2/notifications/get_notifications.py @@ -0,0 +1,14 @@ +from app.v2.notifications import notification_blueprint + + +@notification_blueprint.route("/", methods=['GET']) +def get_notification_by_id(id): + pass + + +@notification_blueprint.route("/", methods=['GET']) +def get_notifications(): + # validate notifications request arguments + # fetch all notifications + # return notifications_response schema + pass diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py new file mode 100644 index 000000000..130716fbd --- /dev/null +++ b/app/v2/notifications/post_notifications.py @@ -0,0 +1,49 @@ +from app.v2.notifications import notification_blueprint + + +@notification_blueprint.route('/sms', methods=['POST']) +def post_sms_notification(): + # # get service + # service = services_dao.dao_fetch_service_by_id(api_user.service_id) + # # validate input against json schema (not marshmallow) + # form = validate(request.get_json(), post_sms_request) + # + # # following checks will be in a common function for all versions of the endpoint. + # # check service has not exceeded the sending limit + # check_service_message_limit(api_user.key_type, service) + # template = templates_dao.dao_get_template_by_id_and_service_id( + # template_id=form['template_id'], + # service_id=service.id + # ) + # # check template is for sms + # check_template_is_for_notification_type(SMS_TYPE, template.template_type) + # # check template is not archived + # check_template_is_active(template) + # # check service is allowed to send + # service_can_send_to_recipient(form['phone_number'], api_user.key_type, service) + # # create body of message (create_template_object_for_notification) + # create_template_object_for_notification(template, form.get('personalisation', {})) + # # persist notification + # # send sms to provider queue for research mode queue + # + + # validate post form against post_sms_request schema + # validate service + # validate template + # create content + # persist notification + # send notification to queue + # return post_sms_response schema + return "post_sms_response schema", 201 + + +@notification_blueprint.route('/email', methods=['POST']) +def post_email_notification(): + # validate post form against post_email_request schema + # validate service + # validate template + # persist notification + # send notification to queue + # create content + # return post_email_response schema + pass diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py new file mode 100644 index 000000000..374b31765 --- /dev/null +++ b/tests/app/notifications/test_validators.py @@ -0,0 +1,60 @@ +import pytest + +from app.errors import InvalidRequest +from app.notifications.validators import check_service_message_limit, check_template_is_for_notification_type, \ + check_template_is_active +from tests.app.conftest import (sample_notification as create_notification, + sample_service as create_service) + + +@pytest.mark.parametrize('key_type', ['test', 'team', 'live']) +def test_check_service_message_limit_with_unrestricted_service_passes(key_type, + sample_service, + sample_notification): + assert check_service_message_limit(key_type, sample_service) is None + + +@pytest.mark.parametrize('key_type', ['test', 'team', 'live']) +def test_check_service_message_limit_under_message_limit_passes(key_type, + sample_service, + sample_notification): + assert check_service_message_limit(key_type, sample_service) is None + + +@pytest.mark.parametrize('key_type', ['team', 'live']) +def test_check_service_message_limit_over_message_limit_fails(key_type, notify_db, notify_db_session): + service = create_service(notify_db, notify_db_session, restricted=True, limit=4) + for x in range(5): + create_notification(notify_db, notify_db_session, service=service) + with pytest.raises(InvalidRequest): + check_service_message_limit(key_type, service) + + +@pytest.mark.parametrize('template_type, notification_type', + [('email', 'email'), + ('sms', 'sms')]) +def test_check_template_is_for_notification_type_pass(template_type, notification_type): + assert check_template_is_for_notification_type(notification_type=notification_type, + template_type=template_type) is None + + +@pytest.mark.parametrize('template_type, notification_type', + [('sms', 'email'), + ('email', 'sms')]) +def test_check_template_is_for_notification_type_fails_when_template_type_does_not_match_notification_type( + template_type, notification_type): + with pytest.raises(InvalidRequest): + check_template_is_for_notification_type(notification_type=notification_type, + template_type=template_type) + + +def test_check_template_is_active_passes(sample_template): + assert check_template_is_active(sample_template) is None + + +def test_check_template_is_active_passes(sample_template): + sample_template.archived = True + from app.dao.templates_dao import dao_update_template + dao_update_template(sample_template) + with pytest.raises(InvalidRequest): + check_template_is_active(sample_template) From c2eecdae36e50d35f3d0cfc00d41fa0da67efdbe Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 27 Oct 2016 11:46:37 +0100 Subject: [PATCH 3/9] - Add validation methods for post notification. - Use these validation methods in post_sms_notification and the version 1 of post_notification. - Create a v2 error handlers. - InvalidRequest has a to_dict method for private and v1 error responses and a to_dict_v2 method to create the v2 of the error responses. - Each validation method has extensive unit tests, so the unit test for the endpoint do not need to check every error case, but check that the error handle formats the message correctly. - The format of the error messages is still a work on progress. - This version of the api could be deployed without causing a problem to the application. - The new endpoing is still a work in progress and is not being used yet. --- app/__init__.py | 21 +++- app/errors.py | 42 ++++--- app/notifications/process_notifications.py | 29 +++++ app/notifications/rest.py | 22 +--- app/notifications/validators.py | 46 ++++++-- app/v2/errors.py | 44 +++++--- app/v2/notifications/__init__.py | 2 +- app/v2/notifications/post_notifications.py | 58 +++++----- .../rest/test_send_notification.py | 2 +- .../test_process_notification.py | 17 +++ tests/app/notifications/test_validators.py | 103 ++++++++++++++++-- .../notifications/test_post_notifications.py | 43 ++++++++ 12 files changed, 328 insertions(+), 101 deletions(-) create mode 100644 tests/app/notifications/test_process_notification.py create mode 100644 tests/app/v2/notifications/test_post_notifications.py diff --git a/app/__init__.py b/app/__init__.py index c602003fb..165a8b884 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -1,7 +1,7 @@ import uuid import os -from flask import request, url_for, g +from flask import request, url_for, g, jsonify from flask import Flask, _request_ctx_stack from flask.ext.sqlalchemy import SQLAlchemy from flask_marshmallow import Marshmallow @@ -57,6 +57,13 @@ def create_app(app_name=None): encryption.init_app(application) clients.init_app(sms_clients=[firetext_client, mmg_client, loadtest_client], email_clients=[aws_ses_client]) + register_blueprint(application) + register_v2_blueprints(application) + + return application + + +def register_blueprint(application): from app.service.rest import service_blueprint from app.user.rest import user as user_blueprint from app.template.rest import template as template_blueprint @@ -81,14 +88,16 @@ def create_app(app_name=None): application.register_blueprint(invite_blueprint) application.register_blueprint(delivery_blueprint) application.register_blueprint(accept_invite, url_prefix='/invite') - application.register_blueprint(template_statistics_blueprint) application.register_blueprint(events_blueprint) application.register_blueprint(provider_details_blueprint, url_prefix='/provider-details') application.register_blueprint(spec_blueprint, url_prefix='/spec') application.register_blueprint(organisation_blueprint, url_prefix='/organisation') - return application + +def register_v2_blueprints(application): + from app.v2.notifications.post_notifications import notification_blueprint + application.register_blueprint(notification_blueprint) def init_app(app): @@ -120,6 +129,12 @@ def init_app(app): response.headers.add('Access-Control-Allow-Methods', 'GET,PUT,POST,DELETE') return response + @app.errorhandler(404) + def page_not_found(e): + msg = e.description or "Not found" + app.logger.exception(msg) + return jsonify(result='error', message=msg), 404 + def create_uuid(): return str(uuid.uuid4()) diff --git a/app/errors.py b/app/errors.py index 3aa0b2831..8b027ba60 100644 --- a/app/errors.py +++ b/app/errors.py @@ -9,6 +9,9 @@ from app.authentication.auth import AuthError class InvalidRequest(Exception): + code = None + link = None + fields = [] def __init__(self, message, status_code): super().__init__() @@ -18,62 +21,67 @@ class InvalidRequest(Exception): def to_dict(self): return {'result': 'error', 'message': self.message} + def to_dict_v2(self): + ''' + Version 2 of the public api error response. + ''' + return { + "code": self.code, + "message": self.message, + "link": self.link, + "fields": self.fields + } + def __str__(self): return str(self.to_dict()) def register_errors(blueprint): - @blueprint.app_errorhandler(AuthError) + @blueprint.errorhandler(AuthError) def authentication_error(error): return jsonify(result='error', message=error.message), error.code - @blueprint.app_errorhandler(ValidationError) + @blueprint.errorhandler(ValidationError) def validation_error(error): current_app.logger.error(error) return jsonify(result='error', message=error.messages), 400 - @blueprint.app_errorhandler(InvalidRequest) + @blueprint.errorhandler(InvalidRequest) def invalid_data(error): response = jsonify(error.to_dict()) response.status_code = error.status_code current_app.logger.error(error) return response - @blueprint.app_errorhandler(400) + @blueprint.errorhandler(400) def bad_request(e): msg = e.description or "Invalid request parameters" current_app.logger.exception(msg) return jsonify(result='error', message=str(msg)), 400 - @blueprint.app_errorhandler(401) + @blueprint.errorhandler(401) def unauthorized(e): error_message = "Unauthorized, authentication token must be provided" return jsonify(result='error', message=error_message), 401, [('WWW-Authenticate', 'Bearer')] - @blueprint.app_errorhandler(403) + @blueprint.errorhandler(403) def forbidden(e): error_message = "Forbidden, invalid authentication token provided" return jsonify(result='error', message=error_message), 403 - @blueprint.app_errorhandler(404) - def page_not_found(e): - msg = e.description or "Not found" - current_app.logger.exception(msg) - return jsonify(result='error', message=msg), 404 - - @blueprint.app_errorhandler(429) + @blueprint.errorhandler(429) def limit_exceeded(e): current_app.logger.exception(e) return jsonify(result='error', message=str(e.description)), 429 - @blueprint.app_errorhandler(NoResultFound) - @blueprint.app_errorhandler(DataError) + @blueprint.errorhandler(NoResultFound) + @blueprint.errorhandler(DataError) def no_result_found(e): current_app.logger.exception(e) return jsonify(result='error', message="No result found"), 404 - @blueprint.app_errorhandler(SQLAlchemyError) + @blueprint.errorhandler(SQLAlchemyError) def db_error(e): current_app.logger.exception(e) if e.orig.pgerror and \ @@ -89,7 +97,7 @@ def register_errors(blueprint): # this must be defined after all other error handlers since it catches the generic Exception object @blueprint.app_errorhandler(500) - @blueprint.app_errorhandler(Exception) + @blueprint.errorhandler(Exception) def internal_server_error(e): current_app.logger.exception(e) return jsonify(result='error', message="Internal server error"), 500 diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index c82dc519e..830c51c47 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -1,3 +1,32 @@ +from notifications_utils.renderers import PassThrough +from notifications_utils.template import Template + +from app.models import SMS_TYPE +from app.notifications.validators import check_sms_content_char_count +from app.v2.errors import BadRequestError + + +def create_content_for_notification(template, personalisation): + template_object = Template( + template.__dict__, + personalisation, + renderer=PassThrough() + ) + if template_object.missing_data: + message = 'Missing personalisation: {}'.format(", ".join(template_object.missing_data)) + errors = {'template': [message]} + raise BadRequestError(errors) + + if template_object.additional_data: + message = 'Personalisation not needed for template: {}'.format(", ".join(template_object.additional_data)) + errors = {'template': [message]} + raise BadRequestError(fields=errors) + + if template_object.template_type == SMS_TYPE: + check_sms_content_char_count(template_object.replaced_content_count) + return template_object + + def persist_notification(): ''' persist the notification diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 2a0e9319e..71c3340c0 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -24,6 +24,8 @@ from app.notifications.process_client_response import ( validate_callback_data, process_sms_client_response ) +from app.notifications.validators import check_service_message_limit, check_template_is_for_notification_type, \ + check_template_is_active from app.service.utils import service_allowed_to_send_to from app.schemas import ( email_notification_schema, @@ -31,8 +33,7 @@ from app.schemas import ( notification_with_personalisation_schema, notifications_filter_schema, notifications_statistics_schema, - day_schema, - unarchived_template_schema + day_schema ) from app.utils import pagination_links @@ -216,26 +217,15 @@ def send_notification(notification_type): if errors: raise InvalidRequest(errors, status_code=400) - if all((api_user.key_type != KEY_TYPE_TEST, - service.restricted)): - service_stats = services_dao.fetch_todays_total_message_count(service.id) - if service_stats >= service.message_limit: - error = 'Exceeded send limits ({}) for today'.format(service.message_limit) - raise InvalidRequest(error, status_code=429) + check_service_message_limit(api_user.key_type, service) template = templates_dao.dao_get_template_by_id_and_service_id( template_id=notification['template'], service_id=service_id ) - if notification_type != template.template_type: - raise InvalidRequest("{0} template is not suitable for {1} notification".format(template.template_type, - notification_type), - status_code=400) - - errors = unarchived_template_schema.validate({'archived': template.archived}) - if errors: - raise InvalidRequest(errors, status_code=400) + check_template_is_for_notification_type(notification_type, template.template_type) + check_template_is_active(template) template_object = create_template_object_for_notification(template, notification.get('personalisation', {})) diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 47bc847ba..5fb02c01e 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -1,6 +1,9 @@ +from flask import current_app + from app.dao import services_dao -from app.errors import InvalidRequest -from app.models import KEY_TYPE_TEST +from app.models import KEY_TYPE_TEST, KEY_TYPE_TEAM +from app.service.utils import service_allowed_to_send_to +from app.v2.errors import TooManyRequestsError, BadRequestError def check_service_message_limit(key_type, service): @@ -8,18 +11,43 @@ def check_service_message_limit(key_type, service): service.restricted)): service_stats = services_dao.fetch_todays_total_message_count(service.id) if service_stats >= service.message_limit: - error = 'Exceeded send limits ({}) for today'.format(service.message_limit) - - raise InvalidRequest(error, status_code=429) + raise TooManyRequestsError(service.message_limit) def check_template_is_for_notification_type(notification_type, template_type): if notification_type != template_type: - raise InvalidRequest("{0} template is not suitable for {1} notification".format(template_type, - notification_type), - status_code=400) + raise BadRequestError( + message="{0} template is not suitable for {1} notification".format(template_type, + notification_type), + fields=[{"template": "{0} template is not suitable for {1} notification".format(template_type, + notification_type)}]) def check_template_is_active(template): if template.archived: - raise InvalidRequest('Template has been deleted', status_code=400) + raise BadRequestError(fields=[{"template": "has been deleted"}], + message="Template has been deleted") + + +def service_can_send_to_recipient(send_to, key_type, service, recipient_type): + if not service_allowed_to_send_to(send_to, service, key_type): + if key_type == KEY_TYPE_TEAM: + message = 'Can’t send to this recipient using a team-only API key' + else: + message = ( + 'Can’t send to this recipient when service is in trial mode ' + '– see https://www.notifications.service.gov.uk/trial-mode' + ) + raise BadRequestError( + fields={recipient_type: [message]} + ) + + +def check_sms_content_char_count(content_count): + char_count_limit = current_app.config.get('SMS_CHAR_COUNT_LIMIT') + if ( + content_count > char_count_limit + ): + message = 'Content has a character count greater than the limit of {}'.format(char_count_limit) + errors = {'content': [message]} + raise BadRequestError(fields=errors) diff --git a/app/v2/errors.py b/app/v2/errors.py index 01577b456..ffe7db3db 100644 --- a/app/v2/errors.py +++ b/app/v2/errors.py @@ -1,29 +1,37 @@ -from flask import jsonify - +from flask import jsonify, current_app from app.errors import InvalidRequest -class BadRequestError(Exception): - status_code = 400 +class TooManyRequestsError(InvalidRequest): + status_code = 429 + # code and link will be in a static file + code = "10429" + link = "link to docs" + message_template = 'Exceeded send limits ({}) for today' - def __init__(self, message, fields, code): - self.code = code - self.message = message + def __init__(self, sending_limit): + self.message = self.message_template.format(sending_limit) + + +class BadRequestError(InvalidRequest): + status_code = 400 + code = "10400" + link = "link to documentation" + message = "An error occurred" + + def __init__(self, fields, message=None): self.fields = fields + self.message = message if message else self.message def register_errors(blueprint): - @blueprint.app_errorhandler(Exception) + @blueprint.errorhandler(InvalidRequest) + def invalid_data(error): + current_app.logger.error(error) + response = jsonify(error.to_dict_v2()), error.status_code + return response + + @blueprint.errorhandler(Exception) def authentication_error(error): # v2 error format - NOT this return jsonify(result='error', message=error.message), error.code - - @blueprint.app_errorhandler(InvalidRequest) - def handle_invalid_request(error): - # { - # "code", - # "link", - # "message" , - # "fields": - # } - return "build_error_message" diff --git a/app/v2/notifications/__init__.py b/app/v2/notifications/__init__.py index f805256c0..8d3808fbf 100644 --- a/app/v2/notifications/__init__.py +++ b/app/v2/notifications/__init__.py @@ -2,6 +2,6 @@ from flask import Blueprint from app.v2.errors import register_errors -notification_blueprint = Blueprint(__name__, __name__, url_prefix='/v2/notifications') +notification_blueprint = Blueprint("v2_notifications", __name__, url_prefix='/v2/notifications') register_errors(notification_blueprint) diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 130716fbd..2b36fbeca 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -1,38 +1,40 @@ +from flask import request + +from app import api_user +from app.dao import services_dao, templates_dao +from app.models import SMS_TYPE +from app.notifications.process_notifications import create_content_for_notification +from app.notifications.validators import (check_service_message_limit, + check_template_is_for_notification_type, + check_template_is_active, + service_can_send_to_recipient, + check_sms_content_char_count) +from app.schema_validation import validate from app.v2.notifications import notification_blueprint +from app.v2.notifications.notification_schemas import post_sms_request @notification_blueprint.route('/sms', methods=['POST']) def post_sms_notification(): - # # get service - # service = services_dao.dao_fetch_service_by_id(api_user.service_id) - # # validate input against json schema (not marshmallow) - # form = validate(request.get_json(), post_sms_request) - # - # # following checks will be in a common function for all versions of the endpoint. - # # check service has not exceeded the sending limit - # check_service_message_limit(api_user.key_type, service) - # template = templates_dao.dao_get_template_by_id_and_service_id( - # template_id=form['template_id'], - # service_id=service.id - # ) - # # check template is for sms - # check_template_is_for_notification_type(SMS_TYPE, template.template_type) - # # check template is not archived - # check_template_is_active(template) - # # check service is allowed to send - # service_can_send_to_recipient(form['phone_number'], api_user.key_type, service) - # # create body of message (create_template_object_for_notification) - # create_template_object_for_notification(template, form.get('personalisation', {})) - # # persist notification - # # send sms to provider queue for research mode queue - # + form = validate(request.get_json(), post_sms_request) + service = services_dao.dao_fetch_service_by_id(api_user.service_id) + + # following checks will be in a common function for all versions of the endpoint. + # check service has not exceeded the sending limit + check_service_message_limit(api_user.key_type, service) + service_can_send_to_recipient(form['phone_number'], api_user.key_type, service, SMS_TYPE) + + template = templates_dao.dao_get_template_by_id_and_service_id( + template_id=form['template_id'], + service_id=service.id) + + check_template_is_for_notification_type(SMS_TYPE, template.template_type) + check_template_is_active(template) + template_with_content = create_content_for_notification(template, form.get('personalisation', {})) + check_sms_content_char_count(template_with_content.replaced_content_count) - # validate post form against post_sms_request schema - # validate service - # validate template - # create content # persist notification - # send notification to queue + # send sms to provider queue for research mode queue # return post_sms_response schema return "post_sms_response schema", 201 diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index a45ff4daf..11b4d2f0b 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -150,7 +150,7 @@ def test_should_not_send_notification_for_archived_template(notify_api, sample_t headers=[('Content-Type', 'application/json'), auth_header]) assert resp.status_code == 400 json_resp = json.loads(resp.get_data(as_text=True)) - assert 'Template has been deleted' in json_resp['message']['template'] + assert 'Template has been deleted' in json_resp['message'] @pytest.mark.parametrize('template_type, to', diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py new file mode 100644 index 000000000..a7edc571e --- /dev/null +++ b/tests/app/notifications/test_process_notification.py @@ -0,0 +1,17 @@ +import pytest + +from app.models import Template +from app.notifications.process_notifications import create_content_for_notification +from app.v2.errors import BadRequestError + + +def test_create_content_for_notification_passes(sample_email_template): + template = Template.query.get(sample_email_template.id) + content = create_content_for_notification(template, None) + assert content.content == template.content + + +def test_create_content_for_notification_fails_with_missing_personalisation(sample_template_with_placeholders): + template = Template.query.get(sample_template_with_placeholders.id) + with pytest.raises(BadRequestError): + create_content_for_notification(template, None) diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index 374b31765..65cece1e9 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -2,31 +2,32 @@ import pytest from app.errors import InvalidRequest from app.notifications.validators import check_service_message_limit, check_template_is_for_notification_type, \ - check_template_is_active + check_template_is_active, service_can_send_to_recipient, check_sms_content_char_count +from app.v2.errors import BadRequestError, TooManyRequestsError from tests.app.conftest import (sample_notification as create_notification, - sample_service as create_service) + sample_service as create_service, sample_service_whitelist) -@pytest.mark.parametrize('key_type', ['test', 'team', 'live']) +@pytest.mark.parametrize('key_type', ['test', 'team', 'normal']) def test_check_service_message_limit_with_unrestricted_service_passes(key_type, sample_service, sample_notification): assert check_service_message_limit(key_type, sample_service) is None -@pytest.mark.parametrize('key_type', ['test', 'team', 'live']) +@pytest.mark.parametrize('key_type', ['test', 'team', 'normal']) def test_check_service_message_limit_under_message_limit_passes(key_type, sample_service, sample_notification): assert check_service_message_limit(key_type, sample_service) is None -@pytest.mark.parametrize('key_type', ['team', 'live']) +@pytest.mark.parametrize('key_type', ['team', 'normal']) def test_check_service_message_limit_over_message_limit_fails(key_type, notify_db, notify_db_session): service = create_service(notify_db, notify_db_session, restricted=True, limit=4) for x in range(5): create_notification(notify_db, notify_db_session, service=service) - with pytest.raises(InvalidRequest): + with pytest.raises(TooManyRequestsError): check_service_message_limit(key_type, service) @@ -43,7 +44,7 @@ def test_check_template_is_for_notification_type_pass(template_type, notificatio ('email', 'sms')]) def test_check_template_is_for_notification_type_fails_when_template_type_does_not_match_notification_type( template_type, notification_type): - with pytest.raises(InvalidRequest): + with pytest.raises(BadRequestError): check_template_is_for_notification_type(notification_type=notification_type, template_type=template_type) @@ -56,5 +57,91 @@ def test_check_template_is_active_passes(sample_template): sample_template.archived = True from app.dao.templates_dao import dao_update_template dao_update_template(sample_template) - with pytest.raises(InvalidRequest): + try: check_template_is_active(sample_template) + except BadRequestError as e: + assert e.status_code == 400 + assert e.code == '10400' + assert e.message == 'Template has been deleted' + assert e.link == "link to documentation" + assert e.fields[0]["template"] == "has been deleted" + + +@pytest.mark.parametrize('key_type', + ['test', 'normal']) +def test_service_can_send_to_recipient_passes(key_type, notify_db, notify_db_session): + trial_mode_service = create_service(notify_db, notify_db_session, service_name='trial mode', restricted=True) + assert service_can_send_to_recipient(trial_mode_service.users[0].email_address, + key_type, + trial_mode_service, + "email") is None + assert service_can_send_to_recipient(trial_mode_service.users[0].mobile_number, + key_type, + trial_mode_service, + "sms") is None + + +@pytest.mark.parametrize('key_type', + ['test', 'normal']) +def test_service_can_send_to_recipient_passes_for_live_service_non_team_member(key_type, notify_db, notify_db_session): + live_service = create_service(notify_db, notify_db_session, service_name='live', restricted=False) + assert service_can_send_to_recipient("some_other_email@test.com", + key_type, + live_service, + "email") is None + assert service_can_send_to_recipient('07513332413', + key_type, + live_service, + "sms") is None + + +@pytest.mark.parametrize('key_type', + ['team']) +def test_service_can_send_to_recipient_passes_for_whitelisted_recipient_passes(key_type, notify_db, notify_db_session, + sample_service): + sample_service_whitelist(notify_db, notify_db_session, email_address="some_other_email@test.com") + assert service_can_send_to_recipient("some_other_email@test.com", + key_type, + sample_service, + "email") is None + sample_service_whitelist(notify_db, notify_db_session, mobile_number='07513332413') + assert service_can_send_to_recipient('07513332413', + key_type, + sample_service, + "sms") is None + + +@pytest.mark.parametrize('key_type', + ['team', 'normal']) +def test_service_can_send_to_recipient_fails_when_recipient_is_not_on_team(key_type, notify_db, notify_db_session): + trial_mode_service = create_service(notify_db, notify_db_session, service_name='trial mode', restricted=True) + with pytest.raises(BadRequestError): + assert service_can_send_to_recipient("some_other_email@test.com", + key_type, + trial_mode_service, + "email") is None + with pytest.raises(BadRequestError): + assert service_can_send_to_recipient('07513332413', + key_type, + trial_mode_service, + "sms") is None + + +def test_service_can_send_to_recipient_fails_when_mobile_number_is_not_on_team(notify_db, notify_db_session): + live_service = create_service(notify_db, notify_db_session, service_name='live mode', restricted=False) + with pytest.raises(BadRequestError): + assert service_can_send_to_recipient("0758964221", + 'team', + live_service, + "sms") is None + + +@pytest.mark.parametrize('char_count', [495, 0, 494, 200]) +def test_check_sms_content_char_count_passes(char_count, notify_api): + assert check_sms_content_char_count(char_count) is None + + +@pytest.mark.parametrize('char_count', [496, 500, 6000]) +def test_check_sms_content_char_count_fails(char_count, notify_api): + with pytest.raises(BadRequestError): + check_sms_content_char_count(char_count) diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py new file mode 100644 index 000000000..e76ec5ef7 --- /dev/null +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -0,0 +1,43 @@ +from flask import json + +from tests import create_authorization_header + + +def test_post_sms_notification_returns_201(notify_api, sample_template): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + data = { + 'phone_number': '+447700900855', + 'template_id': str(sample_template.id) + } + auth_header = create_authorization_header(service_id=sample_template.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 + + +def test_post_sms_notification_returns_404_when_template_is_wrong_type(notify_api, sample_email_template): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + data = { + 'phone_number': '+447700900855', + 'template_id': str(sample_email_template.id) + } + auth_header = create_authorization_header(service_id=sample_email_template.service_id) + + response = client.post( + path='/v2/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 400 + resp_text = json.loads(response.get_data(as_text=True)) + assert resp_text['code'] == '10400' + assert resp_text['message'] == '{0} template is not suitable for {1} notification'.format('email', 'sms') + assert resp_text['link'] == 'link to documentation' + field = "{0} template is not suitable for {1} notification".format("email", "sms") + assert resp_text['fields'][0]['template'] == field From 6e4bad135a74767c0cb2901fb28e084f49df12ab Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 27 Oct 2016 17:34:54 +0100 Subject: [PATCH 4/9] - Implemented persist_notification and send_notification_to_queue in the process_notifications module - Not sure I want to create a new classmethod on Notifications to create from v2 request. Will take another look at that. --- app/models.py | 26 ++++++++- app/notifications/process_notifications.py | 57 ++++++++++++++----- app/v2/notifications/notification_schemas.py | 11 ++++ app/v2/notifications/post_notifications.py | 23 +++++--- .../test_process_notification.py | 56 +++++++++++++++++- .../notifications/test_post_notifications.py | 9 ++- 6 files changed, 158 insertions(+), 24 deletions(-) diff --git a/app/models.py b/app/models.py index 445977251..a9a4b8748 100644 --- a/app/models.py +++ b/app/models.py @@ -22,7 +22,7 @@ from app.authentication.utils import get_secret from app import ( db, encryption, - DATETIME_FORMAT) + DATETIME_FORMAT, create_uuid) from app.history_meta import Versioned @@ -562,6 +562,30 @@ class Notification(db.Model): key_type=key_type ) + @classmethod + def from_v2_api_request(cls, + template_id, + template_version, + recipient, + service_id, + personalisation, + notification_type, + api_key_id, + key_type): + return cls( + id=create_uuid(), + template_id=template_id, + template_version=template_version, + to=recipient, + service_id=service_id, + status='created', + created_at=datetime.datetime.strftime(datetime.datetime.utcnow(), DATETIME_FORMAT), + personalisation=personalisation, + notification_type=notification_type, + api_key_id=api_key_id, + key_type=key_type + ) + class NotificationHistory(db.Model): __tablename__ = 'notification_history' diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 830c51c47..2b0482e97 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -1,7 +1,11 @@ +from flask import current_app from notifications_utils.renderers import PassThrough from notifications_utils.template import Template -from app.models import SMS_TYPE +from app.celery import provider_tasks +from app.dao.notifications_dao import dao_create_notification, dao_delete_notifications_and_history_by_id +from app.errors import InvalidRequest +from app.models import SMS_TYPE, Notification, KEY_TYPE_TEST, EMAIL_TYPE from app.notifications.validators import check_sms_content_char_count from app.v2.errors import BadRequestError @@ -27,17 +31,44 @@ def create_content_for_notification(template, personalisation): return template_object -def persist_notification(): - ''' - persist the notification - :return: - ''' - pass +def persist_notification(template_id, + template_version, + recipient, + service_id, + personalisation, + notification_type, + api_key_id, + key_type): + notification = Notification.from_v2_api_request(template_id, + template_version, + recipient, + service_id, + personalisation, + notification_type, + api_key_id, + key_type) + dao_create_notification(notification) + return notification -def send_notificaiton_to_queue(): - ''' - send the notification to the queue - :return: - ''' - pass +def send_notification_to_queue(notification, research_mode): + try: + research_mode = research_mode or notification.key_type == KEY_TYPE_TEST + if notification.notification_type == SMS_TYPE: + provider_tasks.deliver_sms.apply_async( + [str(notification.id)], + queue='send-sms' if not research_mode else 'research-mode' + ) + if notification.notification_type == EMAIL_TYPE: + provider_tasks.deliver_email.apply_async( + [str(notification.id)], + queue='send-email' if not research_mode else 'research-mode' + ) + except Exception as e: + current_app.logger.exception("Failed to send to SQS exception") + dao_delete_notifications_and_history_by_id(notification.id) + raise InvalidRequest(message="Internal server error", status_code=500) + + current_app.logger.info( + "{} {} created at {}".format(notification.notification_type, notification.id, notification.created_at) + ) diff --git a/app/v2/notifications/notification_schemas.py b/app/v2/notifications/notification_schemas.py index 24bb7c1de..1fac4c7c7 100644 --- a/app/v2/notifications/notification_schemas.py +++ b/app/v2/notifications/notification_schemas.py @@ -54,3 +54,14 @@ post_sms_response = { }, "required": ["id", "content", "uri", "template"] } + + +def create_post_sms_response_from_notification(notification, content): + return {"id": notification.id, + "reference": None, # not yet implemented + "content": content, + "uri": "v2/notifications/{}".format(notification.id), + "template": {"id": notification.template_id, + "version": notification.template_version, + "uri": "v2/templates/{}".format(notification.template_id)} + } diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 2b36fbeca..4b4a3162a 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -1,9 +1,9 @@ -from flask import request - +from flask import request, jsonify from app import api_user from app.dao import services_dao, templates_dao from app.models import SMS_TYPE -from app.notifications.process_notifications import create_content_for_notification +from app.notifications.process_notifications import create_content_for_notification, persist_notification, \ + send_notification_to_queue from app.notifications.validators import (check_service_message_limit, check_template_is_for_notification_type, check_template_is_active, @@ -11,7 +11,8 @@ from app.notifications.validators import (check_service_message_limit, check_sms_content_char_count) from app.schema_validation import validate from app.v2.notifications import notification_blueprint -from app.v2.notifications.notification_schemas import post_sms_request +from app.v2.notifications.notification_schemas import (post_sms_request, + create_post_sms_response_from_notification) @notification_blueprint.route('/sms', methods=['POST']) @@ -34,9 +35,17 @@ def post_sms_notification(): check_sms_content_char_count(template_with_content.replaced_content_count) # persist notification - # send sms to provider queue for research mode queue - # return post_sms_response schema - return "post_sms_response schema", 201 + notification = persist_notification(template_id=template.id, + template_version=template.version, + recipient=form['phone_number'], + service_id=service.id, + personalisation=form.get('personalisation', None), + notification_type=SMS_TYPE, + api_key_id=api_user.id, + key_type=api_user.key_type) + send_notification_to_queue(notification, service.research_mode) + resp = create_post_sms_response_from_notification(notification, template_with_content.content) + return jsonify(resp), 201 @notification_blueprint.route('/email', methods=['POST']) diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index a7edc571e..7407ffe56 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -1,8 +1,12 @@ import pytest +from sqlalchemy.exc import SQLAlchemyError -from app.models import Template -from app.notifications.process_notifications import create_content_for_notification +from app.errors import InvalidRequest +from app.models import Template, Notification, NotificationHistory +from app.notifications.process_notifications import (create_content_for_notification, + persist_notification, send_notification_to_queue) from app.v2.errors import BadRequestError +from tests.app.conftest import sample_notification, sample_template, sample_email_template def test_create_content_for_notification_passes(sample_email_template): @@ -15,3 +19,51 @@ def test_create_content_for_notification_fails_with_missing_personalisation(samp template = Template.query.get(sample_template_with_placeholders.id) with pytest.raises(BadRequestError): create_content_for_notification(template, None) + + +def test_persist_notification_creates_and_save_to_db(sample_template, sample_api_key): + assert Notification.query.count() == 0 + assert NotificationHistory.query.count() == 0 + notification = persist_notification(sample_template.id, sample_template.version, '+447111111111', + sample_template.service.id, {}, 'sms', sample_api_key.id, + sample_api_key.key_type) + assert Notification.query.count() == 1 + assert Notification.query.get(notification.id).__eq__(notification) + assert NotificationHistory.query.count() == 1 + + +def test_persist_notification_throws_exception_when_missing_template(sample_template, sample_api_key): + assert Notification.query.count() == 0 + with pytest.raises(SQLAlchemyError): + persist_notification(template_id=None, + template_version=None, + recipient='+447111111111', + service_id=sample_template.service.id, + personalisation=None, notification_type='sms', + api_key_id=sample_api_key.id, + key_type=sample_api_key.key_type) + + +@pytest.mark.parametrize('research_mode, queue, notification_type, key_type', + [(True, 'research-mode', 'sms', 'normal'), + (False, 'send-sms', 'sms', 'normal'), + (True, 'research-mode', 'email', 'normal'), + (False, 'send-email', 'email', 'normal'), + (False, 'research-mode', 'sms', 'test')]) +def test_send_notification_to_queue(notify_db, notify_db_session, + research_mode, notification_type, + queue, key_type, mocker): + mocked = mocker.patch('app.celery.provider_tasks.deliver_{}.apply_async'.format(notification_type)) + template = sample_template(notify_db, notify_db_session) if notification_type == 'sms' \ + else sample_email_template(notify_db, notify_db_session) + notification = sample_notification(notify_db, notify_db_session, template=template, key_type=key_type) + send_notification_to_queue(notification=notification, research_mode=research_mode) + + mocked.assert_called_once_with([str(notification.id)], queue=queue) + + +def test_send_notification_to_queue(sample_notification, mocker): + mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async', side_effect=Exception("EXPECTED")) + with pytest.raises(InvalidRequest): + send_notification_to_queue(sample_notification, False) + mocked.assert_called_once_with([(str(sample_notification.id))], queue='send-sms') diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index e76ec5ef7..44e5e78ac 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -3,9 +3,10 @@ from flask import json from tests import create_authorization_header -def test_post_sms_notification_returns_201(notify_api, sample_template): +def test_post_sms_notification_returns_201(notify_api, sample_template, mocker): 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) @@ -18,6 +19,12 @@ def test_post_sms_notification_returns_201(notify_api, sample_template): headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 201 + resp_json = json.loads(response.get_data(as_text=True)) + assert resp_json['id'] is not None + assert resp_json['reference'] is None + assert resp_json['template']['id'] == str(sample_template.id) + assert resp_json['template']['version'] == sample_template.version + assert mocked.called def test_post_sms_notification_returns_404_when_template_is_wrong_type(notify_api, sample_email_template): From 8cf2fc72a8c80a4acb9a138b630180f48abd3b5f Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 28 Oct 2016 17:10:00 +0100 Subject: [PATCH 5/9] - Refactor version 1 of post notificaitons to use the common persist_notificaiton and send_notification_to_queue methods. - It would be nice to refactor the send_sms and send_email tasks to use these common functions as well, that way I can get rid of the new Notifications.from_v2_api_request method. - Still not happy with the format of the errors. Would like to find a happy place, where the message is descript enough that we do not need external documentation to explain the error. Perhaps we still only need documentation to explain the trial mode concept. --- app/dao/notifications_dao.py | 5 +- app/models.py | 5 +- app/notifications/process_notifications.py | 40 +++++----- app/notifications/rest.py | 80 +++++-------------- app/notifications/validators.py | 18 ++--- app/v2/errors.py | 2 +- app/v2/notifications/post_notifications.py | 31 +++---- .../rest/test_send_notification.py | 12 +-- .../test_process_notification.py | 27 ++++++- tests/app/notifications/test_validators.py | 31 +++---- .../notifications/test_post_notifications.py | 3 +- 11 files changed, 109 insertions(+), 145 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index dd23d32b5..a1d15d87a 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -1,4 +1,3 @@ -import uuid import pytz from datetime import ( datetime, @@ -12,7 +11,7 @@ from werkzeug.datastructures import MultiDict from sqlalchemy import (desc, func, or_, and_, asc, cast, Text) from sqlalchemy.orm import joinedload -from app import db +from app import db, create_uuid from app.dao import days_ago from app.models import ( Service, @@ -125,7 +124,7 @@ def dao_get_last_template_usage(template_id): def dao_create_notification(notification): if not notification.id: # need to populate defaulted fields before we create the notification history object - notification.id = uuid.uuid4() + notification.id = create_uuid() if not notification.status: notification.status = 'created' diff --git a/app/models.py b/app/models.py index a9a4b8748..05b86d4a2 100644 --- a/app/models.py +++ b/app/models.py @@ -22,7 +22,7 @@ from app.authentication.utils import get_secret from app import ( db, encryption, - DATETIME_FORMAT, create_uuid) + DATETIME_FORMAT) from app.history_meta import Versioned @@ -573,13 +573,12 @@ class Notification(db.Model): api_key_id, key_type): return cls( - id=create_uuid(), template_id=template_id, template_version=template_version, to=recipient, service_id=service_id, status='created', - created_at=datetime.datetime.strftime(datetime.datetime.utcnow(), DATETIME_FORMAT), + created_at=datetime.datetime.utcnow(), personalisation=personalisation, notification_type=notification_type, api_key_id=api_key_id, diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 2b0482e97..1f8e84920 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -4,7 +4,6 @@ from notifications_utils.template import Template from app.celery import provider_tasks from app.dao.notifications_dao import dao_create_notification, dao_delete_notifications_and_history_by_id -from app.errors import InvalidRequest from app.models import SMS_TYPE, Notification, KEY_TYPE_TEST, EMAIL_TYPE from app.notifications.validators import check_sms_content_char_count from app.v2.errors import BadRequestError @@ -16,21 +15,24 @@ def create_content_for_notification(template, personalisation): personalisation, renderer=PassThrough() ) - if template_object.missing_data: - message = 'Missing personalisation: {}'.format(", ".join(template_object.missing_data)) - errors = {'template': [message]} - raise BadRequestError(errors) - - if template_object.additional_data: - message = 'Personalisation not needed for template: {}'.format(", ".join(template_object.additional_data)) - errors = {'template': [message]} - raise BadRequestError(fields=errors) + check_placeholders(template_object) if template_object.template_type == SMS_TYPE: check_sms_content_char_count(template_object.replaced_content_count) return template_object +def check_placeholders(template_object): + if template_object.missing_data: + message = 'Template missing personalisation: {}'.format(", ".join(template_object.missing_data)) + raise BadRequestError(message=message) + + if template_object.additional_data: + message = 'Template personalisation not needed for template: {}'.format( + ", ".join(template_object.additional_data)) + raise BadRequestError(message=message) + + def persist_notification(template_id, template_version, recipient, @@ -39,14 +41,14 @@ def persist_notification(template_id, notification_type, api_key_id, key_type): - notification = Notification.from_v2_api_request(template_id, - template_version, - recipient, - service_id, - personalisation, - notification_type, - api_key_id, - key_type) + notification = Notification.from_v2_api_request(template_id=template_id, + template_version=template_version, + recipient=recipient, + service_id=service_id, + personalisation=personalisation, + notification_type=notification_type, + api_key_id=api_key_id, + key_type=key_type) dao_create_notification(notification) return notification @@ -67,7 +69,7 @@ def send_notification_to_queue(notification, research_mode): except Exception as e: current_app.logger.exception("Failed to send to SQS exception") dao_delete_notifications_and_history_by_id(notification.id) - raise InvalidRequest(message="Internal server error", status_code=500) + raise current_app.logger.info( "{} {} created at {}".format(notification.notification_type, notification.id, notification.created_at) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 71c3340c0..b2e49bd51 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -7,26 +7,25 @@ from flask import ( current_app, json ) - -from notifications_utils.template import Template from notifications_utils.renderers import PassThrough +from notifications_utils.template import Template + +from app import api_user, create_uuid, statsd_client from app.clients.email.aws_ses import get_aws_responses -from app import api_user, create_uuid, DATETIME_FORMAT, statsd_client -from app.dao.notifications_dao import dao_create_notification, dao_delete_notifications_and_history_by_id -from app.models import KEY_TYPE_TEAM, KEY_TYPE_TEST, Notification, KEY_TYPE_NORMAL, EMAIL_TYPE from app.dao import ( templates_dao, services_dao, notifications_dao ) +from app.models import KEY_TYPE_TEAM from app.models import SMS_TYPE from app.notifications.process_client_response import ( validate_callback_data, process_sms_client_response ) +from app.notifications.process_notifications import persist_notification, send_notification_to_queue from app.notifications.validators import check_service_message_limit, check_template_is_for_notification_type, \ check_template_is_active -from app.service.utils import service_allowed_to_send_to from app.schemas import ( email_notification_schema, sms_template_notification_schema, @@ -35,6 +34,7 @@ from app.schemas import ( notifications_statistics_schema, day_schema ) +from app.service.utils import service_allowed_to_send_to from app.utils import pagination_links notifications = Blueprint('notifications', __name__) @@ -45,7 +45,6 @@ from app.errors import ( ) register_errors(notifications) -from app.celery import provider_tasks @notifications.route('/notifications/email/ses', methods=['POST']) @@ -219,10 +218,8 @@ def send_notification(notification_type): check_service_message_limit(api_user.key_type, service) - template = templates_dao.dao_get_template_by_id_and_service_id( - template_id=notification['template'], - service_id=service_id - ) + template = templates_dao.dao_get_template_by_id_and_service_id(template_id=notification['template'], + service_id=service_id) check_template_is_for_notification_type(notification_type, template.template_type) check_template_is_active(template) @@ -231,18 +228,20 @@ def send_notification(notification_type): _service_allowed_to_send_to(notification, service) - notification_id = create_uuid() - notification.update({"template_version": template.version}) + saved_notification = None if not _simulated_recipient(notification['to'], notification_type): - persist_notification( - service, - notification_id, - notification, - datetime.utcnow().strftime(DATETIME_FORMAT), - notification_type, - str(api_user.id), - api_user.key_type - ) + saved_notification = persist_notification(template_id=template.id, + template_version=template.version, + recipient=notification['to'], + service_id=service.id, + personalisation=notification.get('personalisation', None), + notification_type=notification_type, + api_key_id=api_user.id, + key_type=api_user.key_type) + send_notification_to_queue(saved_notification, service.research_mode) + + notification_id = create_uuid() if saved_notification is None else saved_notification.id + notification.update({"template_version": template.version}) return jsonify( data=get_notification_return_data( @@ -271,43 +270,6 @@ def _simulated_recipient(to_address, notification_type): else to_address in current_app.config['SIMULATED_EMAIL_ADDRESSES']) -def persist_notification( - service, - notification_id, - notification, - created_at, - notification_type, - api_key_id=None, - key_type=KEY_TYPE_NORMAL, -): - dao_create_notification( - Notification.from_api_request( - created_at, notification, notification_id, service.id, notification_type, api_key_id, key_type - ) - ) - - try: - research_mode = service.research_mode or key_type == KEY_TYPE_TEST - if notification_type == SMS_TYPE: - provider_tasks.deliver_sms.apply_async( - [str(notification_id)], - queue='send-sms' if not research_mode else 'research-mode' - ) - if notification_type == EMAIL_TYPE: - provider_tasks.deliver_email.apply_async( - [str(notification_id)], - queue='send-email' if not research_mode else 'research-mode' - ) - except Exception as e: - current_app.logger.exception("Failed to send to SQS exception") - dao_delete_notifications_and_history_by_id(notification_id) - raise InvalidRequest(message="Internal server error", status_code=500) - - current_app.logger.info( - "{} {} created at {}".format(notification_type, notification_id, created_at) - ) - - def _service_allowed_to_send_to(notification, service): if not service_allowed_to_send_to(notification['to'], service, api_user.key_type): if api_user.key_type == KEY_TYPE_TEAM: diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 5fb02c01e..bdd6aa09f 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -18,18 +18,15 @@ def check_template_is_for_notification_type(notification_type, template_type): if notification_type != template_type: raise BadRequestError( message="{0} template is not suitable for {1} notification".format(template_type, - notification_type), - fields=[{"template": "{0} template is not suitable for {1} notification".format(template_type, - notification_type)}]) + notification_type)) def check_template_is_active(template): if template.archived: - raise BadRequestError(fields=[{"template": "has been deleted"}], - message="Template has been deleted") + raise BadRequestError(message="Template has been deleted") -def service_can_send_to_recipient(send_to, key_type, service, recipient_type): +def service_can_send_to_recipient(send_to, key_type, service): if not service_allowed_to_send_to(send_to, service, key_type): if key_type == KEY_TYPE_TEAM: message = 'Can’t send to this recipient using a team-only API key' @@ -38,9 +35,7 @@ def service_can_send_to_recipient(send_to, key_type, service, recipient_type): 'Can’t send to this recipient when service is in trial mode ' '– see https://www.notifications.service.gov.uk/trial-mode' ) - raise BadRequestError( - fields={recipient_type: [message]} - ) + raise BadRequestError(message=message) def check_sms_content_char_count(content_count): @@ -48,6 +43,5 @@ def check_sms_content_char_count(content_count): if ( content_count > char_count_limit ): - message = 'Content has a character count greater than the limit of {}'.format(char_count_limit) - errors = {'content': [message]} - raise BadRequestError(fields=errors) + message = 'Content for template has a character count greater than the limit of {}'.format(char_count_limit) + raise BadRequestError(message=message) diff --git a/app/v2/errors.py b/app/v2/errors.py index ffe7db3db..62b59ffda 100644 --- a/app/v2/errors.py +++ b/app/v2/errors.py @@ -19,7 +19,7 @@ class BadRequestError(InvalidRequest): link = "link to documentation" message = "An error occurred" - def __init__(self, fields, message=None): + def __init__(self, fields=None, message=None): self.fields = fields self.message = message if message else self.message diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 4b4a3162a..3852f415b 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -2,8 +2,9 @@ from flask import request, jsonify from app import api_user from app.dao import services_dao, templates_dao from app.models import SMS_TYPE -from app.notifications.process_notifications import create_content_for_notification, persist_notification, \ - send_notification_to_queue +from app.notifications.process_notifications import (create_content_for_notification, + persist_notification, + send_notification_to_queue) from app.notifications.validators import (check_service_message_limit, check_template_is_for_notification_type, check_template_is_active, @@ -20,21 +21,11 @@ def post_sms_notification(): form = validate(request.get_json(), post_sms_request) service = services_dao.dao_fetch_service_by_id(api_user.service_id) - # following checks will be in a common function for all versions of the endpoint. - # check service has not exceeded the sending limit check_service_message_limit(api_user.key_type, service) - service_can_send_to_recipient(form['phone_number'], api_user.key_type, service, SMS_TYPE) + service_can_send_to_recipient(form['phone_number'], api_user.key_type, service) - template = templates_dao.dao_get_template_by_id_and_service_id( - template_id=form['template_id'], - service_id=service.id) + template, content = __validate_template(form, service) - check_template_is_for_notification_type(SMS_TYPE, template.template_type) - check_template_is_active(template) - template_with_content = create_content_for_notification(template, form.get('personalisation', {})) - check_sms_content_char_count(template_with_content.replaced_content_count) - - # persist notification notification = persist_notification(template_id=template.id, template_version=template.version, recipient=form['phone_number'], @@ -44,7 +35,7 @@ def post_sms_notification(): api_key_id=api_user.id, key_type=api_user.key_type) send_notification_to_queue(notification, service.research_mode) - resp = create_post_sms_response_from_notification(notification, template_with_content.content) + resp = create_post_sms_response_from_notification(notification, content) return jsonify(resp), 201 @@ -58,3 +49,13 @@ def post_email_notification(): # create content # return post_email_response schema pass + + +def __validate_template(form, service): + template = templates_dao.dao_get_template_by_id_and_service_id(template_id=form['template_id'], + service_id=service.id) + check_template_is_for_notification_type(SMS_TYPE, template.template_type) + check_template_is_active(template) + template_with_content = create_content_for_notification(template, form.get('personalisation', {})) + check_sms_content_char_count(template_with_content.replaced_content_count) + return template, template_with_content.replaced_content_count diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index 11b4d2f0b..a28590c40 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -516,7 +516,7 @@ def test_should_not_send_sms_if_team_api_key_and_not_a_service_user(notify_api, def test_should_send_email_if_team_api_key_and_a_service_user(notify_api, sample_email_template, fake_uuid, mocker): with notify_api.test_request_context(), notify_api.test_client() as client: mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') - mocker.patch('app.notifications.rest.create_uuid', return_value=fake_uuid) + mocker.patch('app.dao.notifications_dao.create_uuid', return_value=fake_uuid) data = { 'to': sample_email_template.service.created_by.email_address, @@ -545,7 +545,7 @@ def test_should_send_sms_to_anyone_with_test_key( ): with notify_api.test_request_context(), notify_api.test_client() as client: mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') - mocker.patch('app.notifications.rest.create_uuid', return_value=fake_uuid) + mocker.patch('app.dao.notifications_dao.create_uuid', return_value=fake_uuid) data = { 'to': '07811111111', @@ -578,7 +578,7 @@ def test_should_send_email_to_anyone_with_test_key( ): with notify_api.test_request_context(), notify_api.test_client() as client: mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') - mocker.patch('app.notifications.rest.create_uuid', return_value=fake_uuid) + mocker.patch('app.dao.notifications_dao.create_uuid', return_value=fake_uuid) data = { 'to': 'anyone123@example.com', @@ -608,7 +608,7 @@ def test_should_send_email_to_anyone_with_test_key( def test_should_send_sms_if_team_api_key_and_a_service_user(notify_api, sample_template, fake_uuid, mocker): with notify_api.test_request_context(), notify_api.test_client() as client: mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') - mocker.patch('app.notifications.rest.create_uuid', return_value=fake_uuid) + mocker.patch('app.dao.notifications_dao.create_uuid', return_value=fake_uuid) data = { 'to': sample_template.service.created_by.mobile_number, @@ -638,7 +638,7 @@ def test_should_persist_notification(notify_api, sample_template, fake_uuid, mocker): with notify_api.test_request_context(), notify_api.test_client() as client: mocked = mocker.patch('app.celery.provider_tasks.deliver_{}.apply_async'.format(template_type)) - mocker.patch('app.notifications.rest.create_uuid', return_value=fake_uuid) + mocker.patch('app.dao.notifications_dao.create_uuid', return_value=fake_uuid) template = sample_template if template_type == 'sms' else sample_email_template to = sample_template.service.created_by.mobile_number if template_type == 'sms' \ else sample_email_template.service.created_by.email_address @@ -682,7 +682,7 @@ def test_should_delete_notification_and_return_error_if_sqs_fails( 'app.celery.provider_tasks.deliver_{}.apply_async'.format(template_type), side_effect=Exception("failed to talk to SQS") ) - mocker.patch('app.notifications.rest.create_uuid', return_value=fake_uuid) + m1 = mocker.patch('app.dao.notifications_dao.create_uuid', return_value=fake_uuid) template = sample_template if template_type == 'sms' else sample_email_template to = sample_template.service.created_by.mobile_number if template_type == 'sms' \ else sample_email_template.service.created_by.email_address diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 7407ffe56..1e0683457 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -1,7 +1,7 @@ import pytest +from boto3.exceptions import Boto3Error from sqlalchemy.exc import SQLAlchemyError -from app.errors import InvalidRequest from app.models import Template, Notification, NotificationHistory from app.notifications.process_notifications import (create_content_for_notification, persist_notification, send_notification_to_queue) @@ -12,7 +12,14 @@ from tests.app.conftest import sample_notification, sample_template, sample_emai def test_create_content_for_notification_passes(sample_email_template): template = Template.query.get(sample_email_template.id) content = create_content_for_notification(template, None) + assert content.replaced == template.content + + +def test_create_content_for_notification_with_placeholders_passes(sample_template_with_placeholders): + template = Template.query.get(sample_template_with_placeholders.id) + content = create_content_for_notification(template, {'name': 'Bobby'}) assert content.content == template.content + assert 'Bobby' in content.replaced def test_create_content_for_notification_fails_with_missing_personalisation(sample_template_with_placeholders): @@ -21,6 +28,12 @@ def test_create_content_for_notification_fails_with_missing_personalisation(samp create_content_for_notification(template, None) +def test_create_content_for_notification_fails_with_additional_personalisation(sample_template_with_placeholders): + template = Template.query.get(sample_template_with_placeholders.id) + with pytest.raises(BadRequestError): + create_content_for_notification(template, {'name': 'Bobbhy', 'Additional': 'Data'}) + + def test_persist_notification_creates_and_save_to_db(sample_template, sample_api_key): assert Notification.query.count() == 0 assert NotificationHistory.query.count() == 0 @@ -34,6 +47,7 @@ def test_persist_notification_creates_and_save_to_db(sample_template, sample_api def test_persist_notification_throws_exception_when_missing_template(sample_template, sample_api_key): assert Notification.query.count() == 0 + assert NotificationHistory.query.count() == 0 with pytest.raises(SQLAlchemyError): persist_notification(template_id=None, template_version=None, @@ -42,6 +56,8 @@ def test_persist_notification_throws_exception_when_missing_template(sample_temp personalisation=None, notification_type='sms', api_key_id=sample_api_key.id, key_type=sample_api_key.key_type) + assert Notification.query.count() == 0 + assert NotificationHistory.query.count() == 0 @pytest.mark.parametrize('research_mode, queue, notification_type, key_type', @@ -62,8 +78,11 @@ def test_send_notification_to_queue(notify_db, notify_db_session, mocked.assert_called_once_with([str(notification.id)], queue=queue) -def test_send_notification_to_queue(sample_notification, mocker): - mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async', side_effect=Exception("EXPECTED")) - with pytest.raises(InvalidRequest): +def test_send_notification_to_queue_throws_exception_deletes_notification(sample_notification, mocker): + mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async', side_effect=Boto3Error("EXPECTED")) + with pytest.raises(Boto3Error): send_notification_to_queue(sample_notification, False) mocked.assert_called_once_with([(str(sample_notification.id))], queue='send-sms') + + assert Notification.query.count() == 0 + assert NotificationHistory.query.count() == 0 diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index 65cece1e9..cbeb4654d 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -1,6 +1,5 @@ import pytest -from app.errors import InvalidRequest from app.notifications.validators import check_service_message_limit, check_template_is_for_notification_type, \ check_template_is_active, service_can_send_to_recipient, check_sms_content_char_count from app.v2.errors import BadRequestError, TooManyRequestsError @@ -53,7 +52,7 @@ def test_check_template_is_active_passes(sample_template): assert check_template_is_active(sample_template) is None -def test_check_template_is_active_passes(sample_template): +def test_check_template_is_active_fails(sample_template): sample_template.archived = True from app.dao.templates_dao import dao_update_template dao_update_template(sample_template) @@ -64,7 +63,6 @@ def test_check_template_is_active_passes(sample_template): assert e.code == '10400' assert e.message == 'Template has been deleted' assert e.link == "link to documentation" - assert e.fields[0]["template"] == "has been deleted" @pytest.mark.parametrize('key_type', @@ -73,12 +71,10 @@ def test_service_can_send_to_recipient_passes(key_type, notify_db, notify_db_ses trial_mode_service = create_service(notify_db, notify_db_session, service_name='trial mode', restricted=True) assert service_can_send_to_recipient(trial_mode_service.users[0].email_address, key_type, - trial_mode_service, - "email") is None + trial_mode_service) is None assert service_can_send_to_recipient(trial_mode_service.users[0].mobile_number, key_type, - trial_mode_service, - "sms") is None + trial_mode_service) is None @pytest.mark.parametrize('key_type', @@ -87,12 +83,10 @@ def test_service_can_send_to_recipient_passes_for_live_service_non_team_member(k live_service = create_service(notify_db, notify_db_session, service_name='live', restricted=False) assert service_can_send_to_recipient("some_other_email@test.com", key_type, - live_service, - "email") is None + live_service) is None assert service_can_send_to_recipient('07513332413', key_type, - live_service, - "sms") is None + live_service) is None @pytest.mark.parametrize('key_type', @@ -102,13 +96,11 @@ def test_service_can_send_to_recipient_passes_for_whitelisted_recipient_passes(k sample_service_whitelist(notify_db, notify_db_session, email_address="some_other_email@test.com") assert service_can_send_to_recipient("some_other_email@test.com", key_type, - sample_service, - "email") is None + sample_service) is None sample_service_whitelist(notify_db, notify_db_session, mobile_number='07513332413') assert service_can_send_to_recipient('07513332413', key_type, - sample_service, - "sms") is None + sample_service) is None @pytest.mark.parametrize('key_type', @@ -118,13 +110,11 @@ def test_service_can_send_to_recipient_fails_when_recipient_is_not_on_team(key_t with pytest.raises(BadRequestError): assert service_can_send_to_recipient("some_other_email@test.com", key_type, - trial_mode_service, - "email") is None + trial_mode_service) is None with pytest.raises(BadRequestError): assert service_can_send_to_recipient('07513332413', key_type, - trial_mode_service, - "sms") is None + trial_mode_service) is None def test_service_can_send_to_recipient_fails_when_mobile_number_is_not_on_team(notify_db, notify_db_session): @@ -132,8 +122,7 @@ def test_service_can_send_to_recipient_fails_when_mobile_number_is_not_on_team(n with pytest.raises(BadRequestError): assert service_can_send_to_recipient("0758964221", 'team', - live_service, - "sms") is None + live_service) is None @pytest.mark.parametrize('char_count', [495, 0, 494, 200]) diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 44e5e78ac..edf16710f 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -46,5 +46,4 @@ def test_post_sms_notification_returns_404_when_template_is_wrong_type(notify_ap assert resp_text['code'] == '10400' assert resp_text['message'] == '{0} template is not suitable for {1} notification'.format('email', 'sms') assert resp_text['link'] == 'link to documentation' - field = "{0} template is not suitable for {1} notification".format("email", "sms") - assert resp_text['fields'][0]['template'] == field + assert resp_text.get('fields', None) is None From fc298367c5edf465114a68411f263c3a5440d958 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 31 Oct 2016 12:22:26 +0000 Subject: [PATCH 6/9] Updated test_validators to test the contents of the error messages. Added some tests to the test_post_notifications. Added a errorhandler for AuthErrors. This endpoint is not being used anywhere, however there is some common code being used in the v1 post endpoint. The only thing that may be affected is the error response, hopefully they are the same. --- app/notifications/process_notifications.py | 4 +- app/notifications/validators.py | 9 +-- app/v2/errors.py | 27 ++++++-- app/v2/notifications/post_notifications.py | 13 +++- .../test_process_notification.py | 4 +- tests/app/notifications/test_validators.py | 64 +++++++++++++------ .../notifications/test_post_notifications.py | 41 +++++++++--- 7 files changed, 120 insertions(+), 42 deletions(-) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 1f8e84920..83a3fcf5f 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -25,12 +25,12 @@ def create_content_for_notification(template, personalisation): def check_placeholders(template_object): if template_object.missing_data: message = 'Template missing personalisation: {}'.format(", ".join(template_object.missing_data)) - raise BadRequestError(message=message) + raise BadRequestError(fields=[{'template': message}], message=message) if template_object.additional_data: message = 'Template personalisation not needed for template: {}'.format( ", ".join(template_object.additional_data)) - raise BadRequestError(message=message) + raise BadRequestError(fields=[{'template': message}], message=message) def persist_notification(template_id, diff --git a/app/notifications/validators.py b/app/notifications/validators.py index bdd6aa09f..5684dfedb 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -16,14 +16,15 @@ def check_service_message_limit(key_type, service): def check_template_is_for_notification_type(notification_type, template_type): if notification_type != template_type: - raise BadRequestError( - message="{0} template is not suitable for {1} notification".format(template_type, - notification_type)) + message = "{0} template is not suitable for {1} notification".format(template_type, + notification_type) + raise BadRequestError(fields=[{'template': message}], message=message) def check_template_is_active(template): if template.archived: - raise BadRequestError(message="Template has been deleted") + raise BadRequestError(fields=[{'template': 'Template has been deleted'}], + message="Template has been deleted") def service_can_send_to_recipient(send_to, key_type, service): diff --git a/app/v2/errors.py b/app/v2/errors.py index 62b59ffda..7fe378419 100644 --- a/app/v2/errors.py +++ b/app/v2/errors.py @@ -1,4 +1,8 @@ from flask import jsonify, current_app +from sqlalchemy.exc import SQLAlchemyError, DataError +from sqlalchemy.orm.exc import NoResultFound + +from app.authentication.auth import AuthError from app.errors import InvalidRequest @@ -15,11 +19,11 @@ class TooManyRequestsError(InvalidRequest): class BadRequestError(InvalidRequest): status_code = 400 - code = "10400" + code = 10400 link = "link to documentation" message = "An error occurred" - def __init__(self, fields=None, message=None): + def __init__(self, fields=[], message=None): self.fields = fields self.message = message if message else self.message @@ -31,7 +35,20 @@ def register_errors(blueprint): response = jsonify(error.to_dict_v2()), error.status_code return response + @blueprint.errorhandler(NoResultFound) + @blueprint.errorhandler(DataError) + def no_result_found(e): + current_app.logger.exception(e) + return jsonify(message="No result found"), 404 + + @blueprint.errorhandler(AuthError) + def auth_error(error): + return jsonify(status_code=error.code, + message=error.message, + code=error.code, + link='link to docs'), error.code + @blueprint.errorhandler(Exception) - def authentication_error(error): - # v2 error format - NOT this - return jsonify(result='error', message=error.message), error.code + def internal_server_error(error): + current_app.logger.exception(error) + return jsonify(message='Internal server error'), 500 diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 3852f415b..7bd0d4170 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -1,4 +1,6 @@ from flask import request, jsonify +from sqlalchemy.orm.exc import NoResultFound + from app import api_user from app.dao import services_dao, templates_dao from app.models import SMS_TYPE @@ -11,6 +13,7 @@ from app.notifications.validators import (check_service_message_limit, service_can_send_to_recipient, check_sms_content_char_count) from app.schema_validation import validate +from app.v2.errors import BadRequestError from app.v2.notifications import notification_blueprint from app.v2.notifications.notification_schemas import (post_sms_request, create_post_sms_response_from_notification) @@ -52,8 +55,14 @@ def post_email_notification(): def __validate_template(form, service): - template = templates_dao.dao_get_template_by_id_and_service_id(template_id=form['template_id'], - service_id=service.id) + try: + template = templates_dao.dao_get_template_by_id_and_service_id(template_id=form['template_id'], + service_id=service.id) + except NoResultFound: + message = 'Template not found' + raise BadRequestError(message=message, + fields=[{'template': message}]) + check_template_is_for_notification_type(SMS_TYPE, template.template_type) check_template_is_active(template) template_with_content = create_content_for_notification(template, form.get('personalisation', {})) diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 1e0683457..fa1698dbc 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -62,9 +62,11 @@ def test_persist_notification_throws_exception_when_missing_template(sample_temp @pytest.mark.parametrize('research_mode, queue, notification_type, key_type', [(True, 'research-mode', 'sms', 'normal'), - (False, 'send-sms', 'sms', 'normal'), (True, 'research-mode', 'email', 'normal'), + (True, 'research-mode', 'email', 'team'), + (False, 'send-sms', 'sms', 'normal'), (False, 'send-email', 'email', 'normal'), + (False, 'send-sms', 'sms', 'team'), (False, 'research-mode', 'sms', 'test')]) def test_send_notification_to_queue(notify_db, notify_db_session, research_mode, notification_type, diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index cbeb4654d..3490105f3 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -1,5 +1,4 @@ import pytest - from app.notifications.validators import check_service_message_limit, check_template_is_for_notification_type, \ check_template_is_active, service_can_send_to_recipient, check_sms_content_char_count from app.v2.errors import BadRequestError, TooManyRequestsError @@ -26,8 +25,12 @@ def test_check_service_message_limit_over_message_limit_fails(key_type, notify_d service = create_service(notify_db, notify_db_session, restricted=True, limit=4) for x in range(5): create_notification(notify_db, notify_db_session, service=service) - with pytest.raises(TooManyRequestsError): + with pytest.raises(TooManyRequestsError) as e: check_service_message_limit(key_type, service) + assert e.value.status_code == 429 + assert e.value.code == '10429' + assert e.value.message == 'Exceeded send limits (4) for today' + assert e.value.fields == [] @pytest.mark.parametrize('template_type, notification_type', @@ -43,9 +46,14 @@ def test_check_template_is_for_notification_type_pass(template_type, notificatio ('email', 'sms')]) def test_check_template_is_for_notification_type_fails_when_template_type_does_not_match_notification_type( template_type, notification_type): - with pytest.raises(BadRequestError): + with pytest.raises(BadRequestError) as e: check_template_is_for_notification_type(notification_type=notification_type, template_type=template_type) + assert e.value.code == 10400 + error_message = '{0} template is not suitable for {1} notification'.format(template_type, notification_type) + assert e.value.message == error_message + assert e.value.link == 'link to documentation' + assert e.value.fields == [{'template': error_message}] def test_check_template_is_active_passes(sample_template): @@ -56,13 +64,13 @@ def test_check_template_is_active_fails(sample_template): sample_template.archived = True from app.dao.templates_dao import dao_update_template dao_update_template(sample_template) - try: + with pytest.raises(BadRequestError) as e: check_template_is_active(sample_template) - except BadRequestError as e: - assert e.status_code == 400 - assert e.code == '10400' - assert e.message == 'Template has been deleted' - assert e.link == "link to documentation" + assert e.value.status_code == 400 + assert e.value.code == 10400 + assert e.value.message == 'Template has been deleted' + assert e.value.link == "link to documentation" + assert e.value.fields == [{'template': 'Template has been deleted'}] @pytest.mark.parametrize('key_type', @@ -103,26 +111,36 @@ def test_service_can_send_to_recipient_passes_for_whitelisted_recipient_passes(k sample_service) is None -@pytest.mark.parametrize('key_type', - ['team', 'normal']) -def test_service_can_send_to_recipient_fails_when_recipient_is_not_on_team(key_type, notify_db, notify_db_session): +@pytest.mark.parametrize('recipient', ['07513332413', 'some_other_email@test.com']) +@pytest.mark.parametrize('key_type, error_message', + [('team', 'Can’t send to this recipient using a team-only API key'), + ('normal', + "Can’t send to this recipient when service is in trial mode – see https://www.notifications.service.gov.uk/trial-mode")]) # noqa +def test_service_can_send_to_recipient_fails_when_recipient_is_not_on_team(recipient, key_type, error_message, + notify_db, notify_db_session): trial_mode_service = create_service(notify_db, notify_db_session, service_name='trial mode', restricted=True) - with pytest.raises(BadRequestError): - assert service_can_send_to_recipient("some_other_email@test.com", - key_type, - trial_mode_service) is None - with pytest.raises(BadRequestError): - assert service_can_send_to_recipient('07513332413', + with pytest.raises(BadRequestError) as exec_info: + assert service_can_send_to_recipient(recipient, key_type, trial_mode_service) is None + assert exec_info.value.status_code == 400 + assert exec_info.value.code == 10400 + assert exec_info.value.message == error_message + assert exec_info.value.link == 'link to documentation' + assert exec_info.value.fields == [] def test_service_can_send_to_recipient_fails_when_mobile_number_is_not_on_team(notify_db, notify_db_session): live_service = create_service(notify_db, notify_db_session, service_name='live mode', restricted=False) - with pytest.raises(BadRequestError): + with pytest.raises(BadRequestError) as e: assert service_can_send_to_recipient("0758964221", 'team', live_service) is None + assert e.value.status_code == 400 + assert e.value.code == 10400 + assert e.value.message == 'Can’t send to this recipient using a team-only API key' + assert e.value.link == 'link to documentation' + assert e.value.fields == [] @pytest.mark.parametrize('char_count', [495, 0, 494, 200]) @@ -132,5 +150,11 @@ def test_check_sms_content_char_count_passes(char_count, notify_api): @pytest.mark.parametrize('char_count', [496, 500, 6000]) def test_check_sms_content_char_count_fails(char_count, notify_api): - with pytest.raises(BadRequestError): + with pytest.raises(BadRequestError) as e: check_sms_content_char_count(char_count) + assert e.value.status_code == 400 + assert e.value.code == 10400 + assert e.value.message == 'Content for template has a character count greater than the limit of {}'.format( + notify_api.config['SMS_CHAR_COUNT_LIMIT']) + assert e.value.link == 'link to documentation' + assert e.value.fields == [] diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index edf16710f..8bb633629 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -1,3 +1,5 @@ +import uuid + from flask import json from tests import create_authorization_header @@ -27,14 +29,14 @@ def test_post_sms_notification_returns_201(notify_api, sample_template, mocker): assert mocked.called -def test_post_sms_notification_returns_404_when_template_is_wrong_type(notify_api, sample_email_template): +def test_post_sms_notification_returns_404_and_missing_template(notify_api, sample_service): with notify_api.test_request_context(): with notify_api.test_client() as client: data = { 'phone_number': '+447700900855', - 'template_id': str(sample_email_template.id) + 'template_id': str(uuid.uuid4()) } - auth_header = create_authorization_header(service_id=sample_email_template.service_id) + auth_header = create_authorization_header(service_id=sample_service.id) response = client.post( path='/v2/notifications/sms', @@ -42,8 +44,31 @@ def test_post_sms_notification_returns_404_when_template_is_wrong_type(notify_ap headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 400 - resp_text = json.loads(response.get_data(as_text=True)) - assert resp_text['code'] == '10400' - assert resp_text['message'] == '{0} template is not suitable for {1} notification'.format('email', 'sms') - assert resp_text['link'] == 'link to documentation' - assert resp_text.get('fields', None) is None + assert response.headers['Content-type'] == 'application/json' + + error_json = json.loads(response.get_data(as_text=True)) + assert error_json['code'] == 10400 + assert error_json['message'] == 'Template not found' + assert error_json['fields'] == [{'template': 'Template not found'}] + assert error_json['link'] == 'link to documentation' + + +def test_post_sms_notification_returns_403_and_well_formed_auth_error(notify_api, sample_template, mocker): + 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) + } + + response = client.post( + path='/v2/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json')]) + + assert response.status_code == 401 + assert response.headers['Content-type'] == 'application/json' + error_resp = json.loads(response.get_data(as_text=True)) + assert error_resp['code'] == 401 + assert error_resp['message'] == {'token': ['Unauthorized, authentication token must be provided']} From a358f3cb3a60e90c241bcc71a63f602acf7ee5d6 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 31 Oct 2016 15:43:11 +0000 Subject: [PATCH 7/9] Error handler for schema validation errors --- app/v2/errors.py | 10 ++++++- .../notifications/test_post_notifications.py | 26 +++++++++++++++++-- 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/app/v2/errors.py b/app/v2/errors.py index 7fe378419..f6fa7e911 100644 --- a/app/v2/errors.py +++ b/app/v2/errors.py @@ -1,5 +1,8 @@ +import json + from flask import jsonify, current_app -from sqlalchemy.exc import SQLAlchemyError, DataError +from jsonschema import ValidationError +from sqlalchemy.exc import DataError from sqlalchemy.orm.exc import NoResultFound from app.authentication.auth import AuthError @@ -35,6 +38,11 @@ def register_errors(blueprint): response = jsonify(error.to_dict_v2()), error.status_code return response + @blueprint.errorhandler(ValidationError) + def validation_error(error): + current_app.logger.exception(error) + return jsonify(json.loads(error.message)), 400 + @blueprint.errorhandler(NoResultFound) @blueprint.errorhandler(DataError) def no_result_found(e): diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 8bb633629..7eb20121c 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -53,10 +53,9 @@ def test_post_sms_notification_returns_404_and_missing_template(notify_api, samp assert error_json['link'] == 'link to documentation' -def test_post_sms_notification_returns_403_and_well_formed_auth_error(notify_api, sample_template, mocker): +def test_post_sms_notification_returns_403_and_well_formed_auth_error(notify_api, sample_template): 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) @@ -72,3 +71,26 @@ def test_post_sms_notification_returns_403_and_well_formed_auth_error(notify_api error_resp = json.loads(response.get_data(as_text=True)) assert error_resp['code'] == 401 assert error_resp['message'] == {'token': ['Unauthorized, authentication token must be provided']} + + +def test_post_sms_notification_returns_400_and_for_schema_problems(notify_api, sample_template): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + data = { + 'phone_number': '+447700900855', + 'template': str(sample_template.id) + } + auth_header = create_authorization_header(service_id=sample_template.service_id) + + response = client.post( + path='/v2/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 400 + assert response.headers['Content-type'] == 'application/json' + error_resp = json.loads(response.get_data(as_text=True)) + assert error_resp['code'] == '1001' + assert error_resp['message'] == 'Validation error occurred - POST v2/notifications/sms' + assert error_resp['link'] == "link to error documentation (not yet implemented)" + assert error_resp['fields'] == ["'template_id' is a required property"] From 482d10545b5ea4521d4fc26f1fe2fffa161fa991 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 1 Nov 2016 10:33:34 +0000 Subject: [PATCH 8/9] Improvements to the tests. Update AuthError with a to_dict_v2 method. --- app/authentication/auth.py | 7 ++++ app/v2/errors.py | 5 +-- app/v2/notifications/notification_schemas.py | 9 +++-- app/v2/notifications/post_notifications.py | 7 ++-- .../test_process_notification.py | 7 ++-- tests/app/notifications/test_validators.py | 20 +++++----- .../test_notification_schemas.py | 37 +++++++++---------- .../notifications/test_post_notifications.py | 12 +++++- 8 files changed, 58 insertions(+), 46 deletions(-) diff --git a/app/authentication/auth.py b/app/authentication/auth.py index 09ccadb66..0ca700a62 100644 --- a/app/authentication/auth.py +++ b/app/authentication/auth.py @@ -12,8 +12,15 @@ from app.dao.services_dao import dao_fetch_service_by_id class AuthError(Exception): def __init__(self, message, code): self.message = {"token": [message]} + self.short_message = message self.code = code + def to_dict_v2(self): + return {'code': self.code, + 'message': self.short_message, + 'fields': self.message, + 'link': 'link to docs'} + def get_auth_token(req): auth_header = req.headers.get('Authorization', None) diff --git a/app/v2/errors.py b/app/v2/errors.py index f6fa7e911..b7bd5f586 100644 --- a/app/v2/errors.py +++ b/app/v2/errors.py @@ -51,10 +51,7 @@ def register_errors(blueprint): @blueprint.errorhandler(AuthError) def auth_error(error): - return jsonify(status_code=error.code, - message=error.message, - code=error.code, - link='link to docs'), error.code + return jsonify(error.to_dict_v2()), error.code @blueprint.errorhandler(Exception) def internal_server_error(error): diff --git a/app/v2/notifications/notification_schemas.py b/app/v2/notifications/notification_schemas.py index 1fac4c7c7..3207d0b48 100644 --- a/app/v2/notifications/notification_schemas.py +++ b/app/v2/notifications/notification_schemas.py @@ -56,12 +56,13 @@ post_sms_response = { } -def create_post_sms_response_from_notification(notification, content): +def create_post_sms_response_from_notification(notification, body, from_number, url_root): return {"id": notification.id, "reference": None, # not yet implemented - "content": content, - "uri": "v2/notifications/{}".format(notification.id), + "content": {'body': body, + 'from_number': from_number}, + "uri": "{}/v2/notifications/{}".format(url_root, str(notification.id)), "template": {"id": notification.template_id, "version": notification.template_version, - "uri": "v2/templates/{}".format(notification.template_id)} + "uri": "{}/v2/templates/{}".format(url_root, str(notification.template_id))} } diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 7bd0d4170..03982f081 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -1,4 +1,4 @@ -from flask import request, jsonify +from flask import request, jsonify, current_app from sqlalchemy.orm.exc import NoResultFound from app import api_user @@ -38,7 +38,8 @@ def post_sms_notification(): api_key_id=api_user.id, key_type=api_user.key_type) send_notification_to_queue(notification, service.research_mode) - resp = create_post_sms_response_from_notification(notification, content) + + resp = create_post_sms_response_from_notification(notification, content, service.sms_sender, request.url_root) return jsonify(resp), 201 @@ -67,4 +68,4 @@ def __validate_template(form, service): check_template_is_active(template) template_with_content = create_content_for_notification(template, form.get('personalisation', {})) check_sms_content_char_count(template_with_content.replaced_content_count) - return template, template_with_content.replaced_content_count + return template, template_with_content.content diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index fa1698dbc..ddc559201 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -30,8 +30,9 @@ def test_create_content_for_notification_fails_with_missing_personalisation(samp def test_create_content_for_notification_fails_with_additional_personalisation(sample_template_with_placeholders): template = Template.query.get(sample_template_with_placeholders.id) - with pytest.raises(BadRequestError): - create_content_for_notification(template, {'name': 'Bobbhy', 'Additional': 'Data'}) + with pytest.raises(BadRequestError) as e: + create_content_for_notification(template, {'name': 'Bobby', 'Additional placeholder': 'Data'}) + assert e.value.message == 'Template personalisation not needed for template: Additional placeholder' def test_persist_notification_creates_and_save_to_db(sample_template, sample_api_key): @@ -41,7 +42,7 @@ def test_persist_notification_creates_and_save_to_db(sample_template, sample_api sample_template.service.id, {}, 'sms', sample_api_key.id, sample_api_key.key_type) assert Notification.query.count() == 1 - assert Notification.query.get(notification.id).__eq__(notification) + assert Notification.query.get(notification.id) is not None assert NotificationHistory.query.count() == 1 diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index 3490105f3..e41d5f81c 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -97,17 +97,15 @@ def test_service_can_send_to_recipient_passes_for_live_service_non_team_member(k live_service) is None -@pytest.mark.parametrize('key_type', - ['team']) -def test_service_can_send_to_recipient_passes_for_whitelisted_recipient_passes(key_type, notify_db, notify_db_session, +def test_service_can_send_to_recipient_passes_for_whitelisted_recipient_passes(notify_db, notify_db_session, sample_service): sample_service_whitelist(notify_db, notify_db_session, email_address="some_other_email@test.com") assert service_can_send_to_recipient("some_other_email@test.com", - key_type, + 'team', sample_service) is None sample_service_whitelist(notify_db, notify_db_session, mobile_number='07513332413') assert service_can_send_to_recipient('07513332413', - key_type, + 'team', sample_service) is None @@ -120,9 +118,9 @@ def test_service_can_send_to_recipient_fails_when_recipient_is_not_on_team(recip notify_db, notify_db_session): trial_mode_service = create_service(notify_db, notify_db_session, service_name='trial mode', restricted=True) with pytest.raises(BadRequestError) as exec_info: - assert service_can_send_to_recipient(recipient, - key_type, - trial_mode_service) is None + service_can_send_to_recipient(recipient, + key_type, + trial_mode_service) assert exec_info.value.status_code == 400 assert exec_info.value.code == 10400 assert exec_info.value.message == error_message @@ -133,9 +131,9 @@ def test_service_can_send_to_recipient_fails_when_recipient_is_not_on_team(recip def test_service_can_send_to_recipient_fails_when_mobile_number_is_not_on_team(notify_db, notify_db_session): live_service = create_service(notify_db, notify_db_session, service_name='live mode', restricted=False) with pytest.raises(BadRequestError) as e: - assert service_can_send_to_recipient("0758964221", - 'team', - live_service) is None + service_can_send_to_recipient("0758964221", + 'team', + live_service) assert e.value.status_code == 400 assert e.value.code == 10400 assert e.value.message == 'Can’t send to this recipient using a team-only API key' diff --git a/tests/app/v2/notifications/test_notification_schemas.py b/tests/app/v2/notifications/test_notification_schemas.py index cbccf3653..0873884bf 100644 --- a/tests/app/v2/notifications/test_notification_schemas.py +++ b/tests/app/v2/notifications/test_notification_schemas.py @@ -25,16 +25,15 @@ def test_post_sms_schema_is_valid(input): def test_post_sms_json_schema_bad_uuid_and_missing_phone_number(): j = {"template_id": "notUUID"} - try: + with pytest.raises(ValidationError) as e: validate(j, post_sms_request) - except ValidationError as e: - error = json.loads(e.message) - assert "POST v2/notifications/sms" in error['message'] - assert len(error.get('fields')) == 2 - assert "phone_number" in e.message - assert "template_id" in e.message - assert error.get('code') == '1001' - assert error.get('link', None) is not None + error = json.loads(e.value.message) + assert "POST v2/notifications/sms" in error['message'] + assert len(error.get('fields')) == 2 + assert "'phone_number' is a required property" in error['fields'] + assert "'template_id' not a valid UUID" in error['fields'] + assert error.get('code') == '1001' + assert error.get('link', None) is not None def test_post_sms_schema_with_personalisation_that_is_not_a_dict(): @@ -44,15 +43,14 @@ def test_post_sms_schema_with_personalisation_that_is_not_a_dict(): "reference": "reference from caller", "personalisation": "not_a_dict" } - try: + with pytest.raises(ValidationError) as e: validate(j, post_sms_request) - except ValidationError as e: - error = json.loads(e.message) - assert "POST v2/notifications/sms" in error['message'] - assert len(error.get('fields')) == 1 - assert "personalisation" in e.message - assert error.get('code') == '1001' - assert error.get('link', None) is not None + error = json.loads(e.value.message) + assert "POST v2/notifications/sms" in error['message'] + assert len(error.get('fields')) == 1 + assert error['fields'][0] == "'personalisation' should contain key value pairs" + assert error.get('code') == '1001' + assert error.get('link', None) is not None valid_response = { @@ -85,7 +83,6 @@ def test_post_sms_response_schema_is_valid(input): def test_post_sms_response_schema_missing_uri(): j = valid_response del j["uri"] - try: + with pytest.raises(ValidationError) as e: validate(j, post_sms_response) - except ValidationError as e: - assert 'uri' in e.message + assert 'uri' in e.value.message diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 7eb20121c..51d69bc49 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -2,6 +2,7 @@ import uuid from flask import json +from app.models import Notification from tests import create_authorization_header @@ -22,10 +23,17 @@ def test_post_sms_notification_returns_201(notify_api, sample_template, mocker): assert response.status_code == 201 resp_json = json.loads(response.get_data(as_text=True)) + notifications = Notification.query.all() + assert len(notifications) == 1 + notification_id = notifications[0].id assert resp_json['id'] is not None assert resp_json['reference'] is None + assert resp_json['content']['body'] == sample_template.content + assert resp_json['content']['from_number'] == sample_template.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 mocked.called @@ -70,7 +78,9 @@ def test_post_sms_notification_returns_403_and_well_formed_auth_error(notify_api assert response.headers['Content-type'] == 'application/json' error_resp = json.loads(response.get_data(as_text=True)) assert error_resp['code'] == 401 - assert error_resp['message'] == {'token': ['Unauthorized, authentication token must be provided']} + assert error_resp['message'] == 'Unauthorized, authentication token must be provided' + assert error_resp['fields'] == {'token': ['Unauthorized, authentication token must be provided']} + assert error_resp['link'] == 'link to docs' def test_post_sms_notification_returns_400_and_for_schema_problems(notify_api, sample_template): From 36ac00811dd98351e0e9c08e9c3a6150abe7eebd Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 2 Nov 2016 09:13:48 +0000 Subject: [PATCH 9/9] Create dict for the fields in the error response. --- app/schema_validation/__init__.py | 2 ++ .../v2/notifications/test_notification_schemas.py | 14 +++++++++----- .../v2/notifications/test_post_notifications.py | 2 +- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py index 84144601b..38837b467 100644 --- a/app/schema_validation/__init__.py +++ b/app/schema_validation/__init__.py @@ -15,6 +15,8 @@ def build_error_message(errors, schema): for e in errors: field = "'{}' {}".format(e.path[0], e.schema.get('validationMessage')) if e.schema.get( 'validationMessage') else e.message + s = field.split("'") + field = {s[1]: s[2].strip()} fields.append(field) message = { "code": "1001", diff --git a/tests/app/v2/notifications/test_notification_schemas.py b/tests/app/v2/notifications/test_notification_schemas.py index 0873884bf..96f9e1284 100644 --- a/tests/app/v2/notifications/test_notification_schemas.py +++ b/tests/app/v2/notifications/test_notification_schemas.py @@ -30,8 +30,8 @@ def test_post_sms_json_schema_bad_uuid_and_missing_phone_number(): error = json.loads(e.value.message) assert "POST v2/notifications/sms" in error['message'] assert len(error.get('fields')) == 2 - assert "'phone_number' is a required property" in error['fields'] - assert "'template_id' not a valid UUID" in error['fields'] + assert {"phone_number": "is a required property"} in error['fields'] + assert {"template_id": "not a valid UUID"} in error['fields'] assert error.get('code') == '1001' assert error.get('link', None) is not None @@ -48,9 +48,9 @@ def test_post_sms_schema_with_personalisation_that_is_not_a_dict(): error = json.loads(e.value.message) assert "POST v2/notifications/sms" in error['message'] assert len(error.get('fields')) == 1 - assert error['fields'][0] == "'personalisation' should contain key value pairs" + assert error['fields'][0] == {"personalisation": "should contain key value pairs"} assert error.get('code') == '1001' - assert error.get('link', None) is not None + assert error.get('link', None) == 'link to error documentation (not yet implemented)' valid_response = { @@ -85,4 +85,8 @@ def test_post_sms_response_schema_missing_uri(): del j["uri"] with pytest.raises(ValidationError) as e: validate(j, post_sms_response) - assert 'uri' in e.value.message + error = json.loads(e.value.message) + assert '1001' == error['code'] + assert 'link to error documentation (not yet implemented)' == error['link'] + assert 'Validation error occurred - response v2/notifications/sms' == error['message'] + assert [{"uri": "is a required property"}] == error['fields'] diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 51d69bc49..bbc07e309 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -103,4 +103,4 @@ def test_post_sms_notification_returns_400_and_for_schema_problems(notify_api, s assert error_resp['code'] == '1001' assert error_resp['message'] == 'Validation error occurred - POST v2/notifications/sms' assert error_resp['link'] == "link to error documentation (not yet implemented)" - assert error_resp['fields'] == ["'template_id' is a required property"] + assert error_resp['fields'] == [{"template_id": "is a required property"}]