From b0ee09a9f69caa5bff839086d534da62a8794e75 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 14 Nov 2016 13:56:09 +0000 Subject: [PATCH 1/6] Implemented the post email notifications endpoint for v2 --- app/schema_validation/__init__.py | 15 +++- app/v2/errors.py | 7 ++ app/v2/notifications/notification_schemas.py | 79 +++++++++++++++++-- app/v2/notifications/post_notifications.py | 52 ++++++++---- .../test_notification_schemas.py | 68 +++++++++++++++- .../notifications/test_post_notifications.py | 54 ++++++++++++- tests/app/v2/test_errors.py | 15 ++++ 7 files changed, 262 insertions(+), 28 deletions(-) diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py index 71dd37c18..ce26e2b04 100644 --- a/app/schema_validation/__init__.py +++ b/app/schema_validation/__init__.py @@ -1,10 +1,21 @@ import json -from jsonschema import Draft4Validator, ValidationError +from jsonschema import (Draft4Validator, ValidationError, FormatChecker) +from notifications_utils.recipients import (validate_phone_number, validate_email_address) def validate(json_to_validate, schema): - validator = Draft4Validator(schema) + format_checker = FormatChecker() + + @format_checker.checks('phone_number') + def validate_schema_phone_number(instance): + return validate_phone_number(instance) + + @format_checker.checks('email_address') + def validate_schema_email_address(instance): + return validate_email_address(instance) + + validator = Draft4Validator(schema, format_checker=format_checker) errors = list(validator.iter_errors(json_to_validate)) if errors.__len__() > 0: raise ValidationError(build_error_message(errors, schema)) diff --git a/app/v2/errors.py b/app/v2/errors.py index 26cc3b36e..872ce41ab 100644 --- a/app/v2/errors.py +++ b/app/v2/errors.py @@ -1,6 +1,7 @@ import json from flask import jsonify, current_app from jsonschema import ValidationError +from notifications_utils.recipients import InvalidPhoneError from sqlalchemy.exc import DataError from sqlalchemy.orm.exc import NoResultFound from app.authentication.auth import AuthError @@ -47,6 +48,12 @@ def register_errors(blueprint): def auth_error(error): return jsonify(error.to_dict_v2()), error.code + @blueprint.errorhandler(InvalidPhoneError) + def invalid_phone_error(error): + current_app.logger.exception(error) + return jsonify(status_code=400, + errors=[{"error": error.__class__.__name__, "message": error.message}]), 400 + @blueprint.errorhandler(Exception) def internal_server_error(error): current_app.logger.exception(error) diff --git a/app/v2/notifications/notification_schemas.py b/app/v2/notifications/notification_schemas.py index 3207d0b48..97402a288 100644 --- a/app/v2/notifications/notification_schemas.py +++ b/app/v2/notifications/notification_schemas.py @@ -7,16 +7,16 @@ post_sms_request = { "title": "POST v2/notifications/sms", "properties": { "reference": {"type": "string"}, - "phone_number": {"type": "string", "format": "sms"}, + "phone_number": {"type": "string", "format": "phone_number"}, "template_id": uuid, "personalisation": personalisation }, "required": ["phone_number", "template_id"] } -content = { +sms_content = { "$schema": "http://json-schema.org/draft-04/schema#", - "description": "POST sms notification response schema", + "description": "content schema for SMS notification response schema", "type": "object", "title": "notification content", "properties": { @@ -29,7 +29,7 @@ content = { # this may belong in a templates module template = { "$schema": "http://json-schema.org/draft-04/schema#", - "description": "POST sms notification response schema", + "description": "template schema", "type": "object", "title": "notification content", "properties": { @@ -48,7 +48,50 @@ post_sms_response = { "properties": { "id": uuid, "reference": {"type": "string"}, - "content": content, + "content": sms_content, + "uri": {"type": "string"}, + "template": template + }, + "required": ["id", "content", "uri", "template"] +} + + +post_email_request = { + "$schema": "http://json-schema.org/draft-04/schema#", + "description": "POST email notification schema", + "type": "object", + "title": "POST v2/notifications/email", + "properties": { + "reference": {"type": "string"}, + "email_address": {"type": "string", "format": "email_address"}, + "template_id": uuid, + "personalisation": personalisation + }, + "required": ["email_address", "template_id"] +} + +email_content = { + "$schema": "http://json-schema.org/draft-04/schema#", + "description": "Email content for POST email notification", + "type": "object", + "title": "notification email content", + "properties": { + "from_email": {"type": "string", "format": "email_address"}, + "body": {"type": "string"}, + "subject": {"type": "string"} + }, + "required": ["body"] +} + +post_email_response = { + "$schema": "http://json-schema.org/draft-04/schema#", + "description": "POST sms notification response schema", + "type": "object", + "title": "response v2/notifications/email", + "properties": { + "id": uuid, + "reference": {"type": "string"}, + "content": email_content, "uri": {"type": "string"}, "template": template }, @@ -62,7 +105,27 @@ def create_post_sms_response_from_notification(notification, body, from_number, "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(url_root, str(notification.template_id))} + "template": __create_template_from_notification(notification=notification, url_root=url_root) } + + +def create_post_email_response_from_notification(notification, content, subject, email_from, url_root): + return { + "id": notification.id, + "reference": notification.reference, + "content": { + "from_email": email_from, + "body": content, + "subject": subject + }, + "uri": "{}/v2/notifications/{}".format(url_root, str(notification.id)), + "template": __create_template_from_notification(notification=notification, url_root=url_root) + } + + +def __create_template_from_notification(notification, url_root): + return { + "id": notification.template_id, + "version": notification.template_version, + "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 03982f081..b8c15081a 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -1,9 +1,9 @@ -from flask import request, jsonify, current_app +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 +from app.models import SMS_TYPE, EMAIL_TYPE from app.notifications.process_notifications import (create_content_for_notification, persist_notification, send_notification_to_queue) @@ -16,7 +16,8 @@ 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) + create_post_sms_response_from_notification, post_email_request, + create_post_email_response_from_notification) @notification_blueprint.route('/sms', methods=['POST']) @@ -27,7 +28,7 @@ def post_sms_notification(): check_service_message_limit(api_user.key_type, service) service_can_send_to_recipient(form['phone_number'], api_user.key_type, service) - template, content = __validate_template(form, service) + template, template_with_content = __validate_template(form, service, SMS_TYPE) notification = persist_notification(template_id=template.id, template_version=template.version, @@ -39,23 +40,42 @@ def post_sms_notification(): key_type=api_user.key_type) send_notification_to_queue(notification, service.research_mode) - resp = create_post_sms_response_from_notification(notification, content, service.sms_sender, request.url_root) + resp = create_post_sms_response_from_notification(notification, + template_with_content.content, + service.sms_sender, + request.url_root) return jsonify(resp), 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 + form = validate(request.get_json(), post_email_request) + service = services_dao.dao_fetch_service_by_id(api_user.service_id) + + check_service_message_limit(api_user.key_type, service) + service_can_send_to_recipient(form['email_address'], api_user.key_type, service) + + template, template_with_content = __validate_template(form, service, EMAIL_TYPE) + notification = persist_notification(template_id=template.id, + template_version=template.version, + recipient=form['email_address'], + service_id=service.id, + personalisation=form.get('personalisation', None), + notification_type=EMAIL_TYPE, + api_key_id=api_user.id, + key_type=api_user.key_type) + + send_notification_to_queue(notification, service.research_mode) + + resp = create_post_email_response_from_notification(notification=notification, + content=template_with_content.content, + subject=template_with_content.subject, + email_from=service.email_from, + url_root=request.url_root) + return jsonify(resp), 201 -def __validate_template(form, service): +def __validate_template(form, service, notification_type): try: template = templates_dao.dao_get_template_by_id_and_service_id(template_id=form['template_id'], service_id=service.id) @@ -64,8 +84,8 @@ def __validate_template(form, service): raise BadRequestError(message=message, fields=[{'template': message}]) - check_template_is_for_notification_type(SMS_TYPE, template.template_type) + check_template_is_for_notification_type(notification_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.content + return template, template_with_content diff --git a/tests/app/v2/notifications/test_notification_schemas.py b/tests/app/v2/notifications/test_notification_schemas.py index 74c734c2f..a9aceef46 100644 --- a/tests/app/v2/notifications/test_notification_schemas.py +++ b/tests/app/v2/notifications/test_notification_schemas.py @@ -3,8 +3,10 @@ import uuid import pytest from flask import json from jsonschema import ValidationError +from notifications_utils.recipients import InvalidPhoneError, InvalidEmailError -from app.v2.notifications.notification_schemas import post_sms_request, post_sms_response +from app.v2.notifications.notification_schemas import post_sms_request, post_sms_response, post_email_request, \ + post_email_response from app.schema_validation import validate valid_json = {"phone_number": "07515111111", @@ -54,6 +56,15 @@ def test_post_sms_schema_with_personalisation_that_is_not_a_dict(): assert len(error.keys()) == 2 +@pytest.mark.parametrize('invalid_phone_number', + ['notaphoneumber', '08515111111', '07515111*11']) +def test_post_sms_request_invalid_phone_number(invalid_phone_number): + j = {"phone_number": invalid_phone_number, + "template_id": str(uuid.uuid4()) + } + with pytest.raises(InvalidPhoneError): + validate(j, post_sms_request) + valid_response = { "id": str(uuid.uuid4()), "content": {"body": "contents of message", @@ -90,3 +101,58 @@ def test_post_sms_response_schema_missing_uri(): assert error['status_code'] == 400 assert error['errors'] == [{'error': 'ValidationError', 'message': "uri is a required property"}] + + +valid_post_email_json = {"email_address": "test@example.gov.uk", + "template_id": str(uuid.uuid4()) + } +valid_post_emaiL_json_with_optionals = { + "email_address": "test@example.gov.uk", + "template_id": str(uuid.uuid4()), + "reference": "reference from caller", + "personalisation": {"key": "value"} +} + + +@pytest.mark.parametrize("input", [valid_post_email_json, valid_post_emaiL_json_with_optionals]) +def test_post_email_schema_is_valid(input): + assert validate(input, post_email_request) == input + + +def test_post_email_schema_bad_uuid_and_missing_email_address(): + j = {"template_id": "bad_template"} + with pytest.raises(ValidationError): + validate(j, post_email_request) + + +def test_post_email_schema_invalid_email_address(): + j = {"template_id": str(uuid.uuid4()), + "email_address": "notavalidemail@address"} + with pytest.raises(InvalidEmailError): + validate(j, post_email_request) + + +valid_email_response = {"id": str(uuid.uuid4()), + "content": {"body": "the body of the message", + "subject": "subject of the message", + "from_email": "service@dig.gov.uk"}, + "uri": "/v2/notifications/id", + "template": {"id": str(uuid.uuid4()), + "version": 1, + "uri": "/v2/template/id"} + } +valid_email_response_with_optionals = {"id": str(uuid.uuid4()), + "reference": "some reference", + "content": {"body": "the body of the message", + "subject": "subject of the message", + "from_email": "service@dig.gov.uk"}, + "uri": "/v2/notifications/id", + "template": {"id": str(uuid.uuid4()), + "version": 1, + "uri": "/v2/template/id"} + } + + +@pytest.mark.parametrize("input", [valid_email_response, valid_email_response_with_optionals]) +def test_post_email_response(input): + assert validate(input, post_email_response) == input diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index c02e2d8e9..c67066387 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -1,7 +1,7 @@ import uuid +import pytest from flask import json - from app.models import Notification from tests import create_authorization_header @@ -102,3 +102,55 @@ def test_post_sms_notification_returns_400_and_for_schema_problems(notify_api, s assert error_resp['errors'] == [{'error': 'ValidationError', 'message': "template_id is a required property" }] + + +def test_post_email_notification_returns_201(client, sample_email_template, mocker): + mocked = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') + data = { + "reference": "reference from caller", + "email_address": sample_email_template.service.users[0].email_address, + "template_id": sample_email_template.id, + } + auth_header = create_authorization_header(service_id=sample_email_template.service_id) + response = client.post( + path="v2/notifications/email", + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + assert response.status_code == 201 + resp_json = json.loads(response.get_data(as_text=True)) + notifications = Notification.query.all() + 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_email_template.content + assert resp_json['content']['subject'] == sample_email_template.subject + assert resp_json['content']['from_email'] == sample_email_template.service.email_from + assert 'v2/notifications/{}'.format(notification_id) in resp_json['uri'] + assert resp_json['template']['id'] == str(sample_email_template.id) + assert resp_json['template']['version'] == sample_email_template.version + assert 'v2/templates/{}'.format(sample_email_template.id) in resp_json['template']['uri'] + assert mocked.called + + +def test_post_email_notification_returns_404_and_missing_template(notify_api, sample_service): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + data = { + "email_address": sample_service.users[0].email_address, + 'template_id': str(uuid.uuid4()) + } + auth_header = create_authorization_header(service_id=sample_service.id) + + response = client.post( + path='/v2/notifications/email', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 400 + assert response.headers['Content-type'] == 'application/json' + + error_json = json.loads(response.get_data(as_text=True)) + assert error_json['status_code'] == 400 + assert error_json['errors'] == [{"error": "BadRequestError", + "message": 'Template not found'}] diff --git a/tests/app/v2/test_errors.py b/tests/app/v2/test_errors.py index 5f0ed0f81..4ce3f2868 100644 --- a/tests/app/v2/test_errors.py +++ b/tests/app/v2/test_errors.py @@ -1,6 +1,7 @@ import json import pytest from flask import url_for +from notifications_utils.recipients import InvalidPhoneError from sqlalchemy.exc import DataError @@ -39,6 +40,10 @@ def app_for_test(mocker): def raising_data_error(): raise DataError("There was a db problem", "params", "orig") + @blue.route("raise_phone_error", methods=["GET"]) + def raising_invalid_phone_error(): + raise InvalidPhoneError("The phone number is wrong") + @blue.route("raise_exception", methods=["GET"]) def raising_exception(): raise AssertionError("Raising any old exception") @@ -116,3 +121,13 @@ def test_internal_server_error_handler(app_for_test): error = json.loads(response.get_data(as_text=True)) assert error == {"status_code": 500, "errors": [{"error": "AssertionError", "message": "Internal server error"}]} + + +def test_invalid_phone_error_handler(app_for_test): + with app_for_test.test_request_context(): + with app_for_test.test_client() as client: + response = client.get(url_for("v2_under_test.raising_invalid_phone_error")) + assert response.status_code == 400 + error = json.loads(response.get_data(as_text=True)) + assert error == {"status_code": 400, + "errors": [{"error": "InvalidPhoneError", "message": "The phone number is wrong"}]} From 924cec05b4ab1de16a978d1f00f1076f47463101 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 16 Nov 2016 15:44:16 +0000 Subject: [PATCH 2/6] Included reference when persisting the notification. This was already part of the data model. Not included in the send_sms|email task because there is no reference from the job. --- app/notifications/process_notifications.py | 8 +++++--- app/v2/notifications/notification_schemas.py | 2 +- app/v2/notifications/post_notifications.py | 6 ++++-- tests/app/v2/notifications/test_post_notifications.py | 7 ++++--- 4 files changed, 14 insertions(+), 9 deletions(-) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 5df9e7b36..d76d92521 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -46,7 +46,8 @@ def persist_notification(template_id, key_type, created_at=None, job_id=None, - job_row_number=None): + job_row_number=None, + reference=None): notification = Notification( template_id=template_id, template_version=template_version, @@ -56,9 +57,10 @@ def persist_notification(template_id, notification_type=notification_type, api_key_id=api_key_id, key_type=key_type, - created_at=created_at if created_at else datetime.utcnow().strftime(DATETIME_FORMAT), + created_at=created_at or datetime.utcnow().strftime(DATETIME_FORMAT), job_id=job_id, - job_row_number=job_row_number + job_row_number=job_row_number, + reference=reference ) dao_create_notification(notification) return notification diff --git a/app/v2/notifications/notification_schemas.py b/app/v2/notifications/notification_schemas.py index 97402a288..42f418567 100644 --- a/app/v2/notifications/notification_schemas.py +++ b/app/v2/notifications/notification_schemas.py @@ -101,7 +101,7 @@ post_email_response = { def create_post_sms_response_from_notification(notification, body, from_number, url_root): return {"id": notification.id, - "reference": None, # not yet implemented + "reference": notification.reference, "content": {'body': body, 'from_number': from_number}, "uri": "{}/v2/notifications/{}".format(url_root, str(notification.id)), diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index b8c15081a..07127b730 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -37,7 +37,8 @@ def post_sms_notification(): personalisation=form.get('personalisation', None), notification_type=SMS_TYPE, api_key_id=api_user.id, - key_type=api_user.key_type) + key_type=api_user.key_type, + reference=form['reference']) send_notification_to_queue(notification, service.research_mode) resp = create_post_sms_response_from_notification(notification, @@ -63,7 +64,8 @@ def post_email_notification(): personalisation=form.get('personalisation', None), notification_type=EMAIL_TYPE, api_key_id=api_user.id, - key_type=api_user.key_type) + key_type=api_user.key_type, + reference=form['reference']) send_notification_to_queue(notification, service.research_mode) diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index c67066387..eb612ea89 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -12,7 +12,8 @@ def test_post_sms_notification_returns_201(notify_api, sample_template, mocker): mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') data = { 'phone_number': '+447700900855', - 'template_id': str(sample_template.id) + 'template_id': str(sample_template.id), + 'reference': 'reference_from_client' } auth_header = create_authorization_header(service_id=sample_template.service_id) @@ -27,7 +28,7 @@ def test_post_sms_notification_returns_201(notify_api, sample_template, mocker): 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['reference'] == 'reference_from_client' 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'] @@ -122,7 +123,7 @@ def test_post_email_notification_returns_201(client, sample_email_template, mock 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['reference'] == "reference from caller" assert resp_json['content']['body'] == sample_email_template.content assert resp_json['content']['subject'] == sample_email_template.subject assert resp_json['content']['from_email'] == sample_email_template.service.email_from From df62be421f643e5096b0ad587277033ab9afc90f Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 16 Nov 2016 17:25:00 +0000 Subject: [PATCH 3/6] Make email_from and subject required attributes of the email_content schema. Update the format_checkers to raise the specific exception that why the validator can handle multiple messages. Which led to a refactor of build_error_message. --- app/schema_validation/__init__.py | 31 ++++++++++++------- app/v2/notifications/notification_schemas.py | 2 +- .../test_notification_schemas.py | 21 ++++++++++--- 3 files changed, 36 insertions(+), 18 deletions(-) diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py index ce26e2b04..63f3a198d 100644 --- a/app/schema_validation/__init__.py +++ b/app/schema_validation/__init__.py @@ -1,38 +1,45 @@ import json from jsonschema import (Draft4Validator, ValidationError, FormatChecker) -from notifications_utils.recipients import (validate_phone_number, validate_email_address) +from notifications_utils.recipients import (validate_phone_number, validate_email_address, InvalidPhoneError, + InvalidEmailError) def validate(json_to_validate, schema): format_checker = FormatChecker() - @format_checker.checks('phone_number') + @format_checker.checks('phone_number', raises=InvalidPhoneError) def validate_schema_phone_number(instance): - return validate_phone_number(instance) + validate_phone_number(instance) + return True - @format_checker.checks('email_address') + @format_checker.checks('email_address', raises=InvalidEmailError) def validate_schema_email_address(instance): - return validate_email_address(instance) + validate_email_address(instance) + return True validator = Draft4Validator(schema, format_checker=format_checker) errors = list(validator.iter_errors(json_to_validate)) if errors.__len__() > 0: - raise ValidationError(build_error_message(errors, schema)) + raise ValidationError(build_error_message(errors)) return json_to_validate -def build_error_message(errors, schema): +def build_error_message(errors): fields = [] 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 = {"error": "ValidationError", "message": "{}{}".format(s[1], s[2])} - fields.append(field) + field = "{} {}".format(e.path[0], e.schema.get('validationMessage')) if e.schema.get( + 'validationMessage') else __format_message(e) + fields.append({"error": "ValidationError", "message": field}) message = { "status_code": 400, "errors": fields } return json.dumps(message) + + +def __format_message(e): + s = e.message.split("'") + msg = "{}{}".format(s[1], s[2]) + return msg if not e.cause else "'{}' {}".format(e.path[0], e.cause.message) diff --git a/app/v2/notifications/notification_schemas.py b/app/v2/notifications/notification_schemas.py index 42f418567..2832487a9 100644 --- a/app/v2/notifications/notification_schemas.py +++ b/app/v2/notifications/notification_schemas.py @@ -80,7 +80,7 @@ email_content = { "body": {"type": "string"}, "subject": {"type": "string"} }, - "required": ["body"] + "required": ["body", "from_email", "subject"] } post_email_response = { diff --git a/tests/app/v2/notifications/test_notification_schemas.py b/tests/app/v2/notifications/test_notification_schemas.py index a9aceef46..13b797f37 100644 --- a/tests/app/v2/notifications/test_notification_schemas.py +++ b/tests/app/v2/notifications/test_notification_schemas.py @@ -57,14 +57,25 @@ def test_post_sms_schema_with_personalisation_that_is_not_a_dict(): @pytest.mark.parametrize('invalid_phone_number', - ['notaphoneumber', '08515111111', '07515111*11']) + ['08515111111', '07515111*11', 'notaphoneumber']) def test_post_sms_request_invalid_phone_number(invalid_phone_number): j = {"phone_number": invalid_phone_number, "template_id": str(uuid.uuid4()) } - with pytest.raises(InvalidPhoneError): + with pytest.raises(ValidationError): validate(j, post_sms_request) + +def test_post_sms_request_invalid_phone_number_and_missing_template(): + j = {"phone_number": '08515111111', + } + with pytest.raises(ValidationError) as e: + validate(j, post_sms_request) + error = json.loads(e.value.message) + print(error) + assert len(error.get('errors')) == 2 + + valid_response = { "id": str(uuid.uuid4()), "content": {"body": "contents of message", @@ -106,7 +117,7 @@ def test_post_sms_response_schema_missing_uri(): valid_post_email_json = {"email_address": "test@example.gov.uk", "template_id": str(uuid.uuid4()) } -valid_post_emaiL_json_with_optionals = { +valid_post_email_json_with_optionals = { "email_address": "test@example.gov.uk", "template_id": str(uuid.uuid4()), "reference": "reference from caller", @@ -114,7 +125,7 @@ valid_post_emaiL_json_with_optionals = { } -@pytest.mark.parametrize("input", [valid_post_email_json, valid_post_emaiL_json_with_optionals]) +@pytest.mark.parametrize("input", [valid_post_email_json, valid_post_email_json_with_optionals]) def test_post_email_schema_is_valid(input): assert validate(input, post_email_request) == input @@ -128,7 +139,7 @@ def test_post_email_schema_bad_uuid_and_missing_email_address(): def test_post_email_schema_invalid_email_address(): j = {"template_id": str(uuid.uuid4()), "email_address": "notavalidemail@address"} - with pytest.raises(InvalidEmailError): + with pytest.raises(ValidationError): validate(j, post_email_request) From dc5e21a78e5f596c7bce32cdc95e021774780067 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 17 Nov 2016 09:09:24 +0000 Subject: [PATCH 4/6] Remove v2 error handler for InvalidPhoneError, no longer expect to throw this exception. --- app/v2/errors.py | 7 ------- tests/app/v2/test_errors.py | 15 --------------- 2 files changed, 22 deletions(-) diff --git a/app/v2/errors.py b/app/v2/errors.py index 872ce41ab..26cc3b36e 100644 --- a/app/v2/errors.py +++ b/app/v2/errors.py @@ -1,7 +1,6 @@ import json from flask import jsonify, current_app from jsonschema import ValidationError -from notifications_utils.recipients import InvalidPhoneError from sqlalchemy.exc import DataError from sqlalchemy.orm.exc import NoResultFound from app.authentication.auth import AuthError @@ -48,12 +47,6 @@ def register_errors(blueprint): def auth_error(error): return jsonify(error.to_dict_v2()), error.code - @blueprint.errorhandler(InvalidPhoneError) - def invalid_phone_error(error): - current_app.logger.exception(error) - return jsonify(status_code=400, - errors=[{"error": error.__class__.__name__, "message": error.message}]), 400 - @blueprint.errorhandler(Exception) def internal_server_error(error): current_app.logger.exception(error) diff --git a/tests/app/v2/test_errors.py b/tests/app/v2/test_errors.py index 4ce3f2868..5f0ed0f81 100644 --- a/tests/app/v2/test_errors.py +++ b/tests/app/v2/test_errors.py @@ -1,7 +1,6 @@ import json import pytest from flask import url_for -from notifications_utils.recipients import InvalidPhoneError from sqlalchemy.exc import DataError @@ -40,10 +39,6 @@ def app_for_test(mocker): def raising_data_error(): raise DataError("There was a db problem", "params", "orig") - @blue.route("raise_phone_error", methods=["GET"]) - def raising_invalid_phone_error(): - raise InvalidPhoneError("The phone number is wrong") - @blue.route("raise_exception", methods=["GET"]) def raising_exception(): raise AssertionError("Raising any old exception") @@ -121,13 +116,3 @@ def test_internal_server_error_handler(app_for_test): error = json.loads(response.get_data(as_text=True)) assert error == {"status_code": 500, "errors": [{"error": "AssertionError", "message": "Internal server error"}]} - - -def test_invalid_phone_error_handler(app_for_test): - with app_for_test.test_request_context(): - with app_for_test.test_client() as client: - response = client.get(url_for("v2_under_test.raising_invalid_phone_error")) - assert response.status_code == 400 - error = json.loads(response.get_data(as_text=True)) - assert error == {"status_code": 400, - "errors": [{"error": "InvalidPhoneError", "message": "The phone number is wrong"}]} From bc434f173612ceed508e617372cf5eb22627e69f Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 17 Nov 2016 13:42:34 +0000 Subject: [PATCH 5/6] Create new column in notifications and notification_history to store the client_reference because I remembered that reference is used for email providers. --- app/models.py | 2 ++ app/notifications/process_notifications.py | 2 +- app/v2/notifications/notification_schemas.py | 4 ++-- .../versions/0061_add_client_reference.py | 24 +++++++++++++++++++ .../test_process_notification.py | 7 ++++-- .../notifications/test_post_notifications.py | 9 ++++--- 6 files changed, 38 insertions(+), 10 deletions(-) create mode 100644 migrations/versions/0061_add_client_reference.py diff --git a/app/models.py b/app/models.py index 5e2d16f6f..bd805c400 100644 --- a/app/models.py +++ b/app/models.py @@ -519,6 +519,7 @@ class Notification(db.Model): onupdate=datetime.datetime.utcnow) status = db.Column(NOTIFICATION_STATUS_TYPES_ENUM, index=True, nullable=False, default='created') reference = db.Column(db.String, nullable=True, index=True) + client_reference = db.Column(db.String, index=True, nullable=True) _personalisation = db.Column(db.String, nullable=True) template_history = db.relationship('TemplateHistory', primaryjoin=and_( @@ -561,6 +562,7 @@ class NotificationHistory(db.Model): updated_at = db.Column(db.DateTime, index=False, unique=False, nullable=True) status = db.Column(NOTIFICATION_STATUS_TYPES_ENUM, index=True, nullable=False, default='created') reference = db.Column(db.String, nullable=True, index=True) + client_reference = db.Column(db.String, nullable=True) @classmethod def from_notification(cls, notification): diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index d76d92521..b5ab8eb97 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -60,7 +60,7 @@ def persist_notification(template_id, created_at=created_at or datetime.utcnow().strftime(DATETIME_FORMAT), job_id=job_id, job_row_number=job_row_number, - reference=reference + client_reference=reference ) dao_create_notification(notification) return notification diff --git a/app/v2/notifications/notification_schemas.py b/app/v2/notifications/notification_schemas.py index 2832487a9..d77a4d21a 100644 --- a/app/v2/notifications/notification_schemas.py +++ b/app/v2/notifications/notification_schemas.py @@ -101,7 +101,7 @@ post_email_response = { def create_post_sms_response_from_notification(notification, body, from_number, url_root): return {"id": notification.id, - "reference": notification.reference, + "reference": notification.client_reference, "content": {'body': body, 'from_number': from_number}, "uri": "{}/v2/notifications/{}".format(url_root, str(notification.id)), @@ -112,7 +112,7 @@ def create_post_sms_response_from_notification(notification, body, from_number, def create_post_email_response_from_notification(notification, content, subject, email_from, url_root): return { "id": notification.id, - "reference": notification.reference, + "reference": notification.client_reference, "content": { "from_email": email_from, "body": content, diff --git a/migrations/versions/0061_add_client_reference.py b/migrations/versions/0061_add_client_reference.py new file mode 100644 index 000000000..05bf0d7e6 --- /dev/null +++ b/migrations/versions/0061_add_client_reference.py @@ -0,0 +1,24 @@ +"""empty message + +Revision ID: 0061_add_client_reference +Revises: 0060_add_letter_template_type +Create Date: 2016-11-17 13:19:25.820617 + +""" + +# revision identifiers, used by Alembic. +revision = '0061_add_client_reference' +down_revision = '0060_add_letter_template_type' + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + op.add_column('notifications', sa.Column('client_reference', sa.String(), index=True, nullable=True)) + op.add_column('notification_history', sa.Column('client_reference', sa.String(), nullable=True)) + + +def downgrade(): + op.drop_column('notifications', 'client_reference') + op.drop_column('notification_history', 'client_reference') diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 851865a6b..8789909bc 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -64,7 +64,7 @@ def test_persist_notification_throws_exception_when_missing_template(sample_api_ assert NotificationHistory.query.count() == 0 -def test_persist_notification_with_job_and_created(sample_job, sample_api_key): +def test_persist_notification_with_optionals(sample_job, sample_api_key): assert Notification.query.count() == 0 assert NotificationHistory.query.count() == 0 created_at = datetime.datetime(2016, 11, 11, 16, 8, 18) @@ -77,13 +77,16 @@ def test_persist_notification_with_job_and_created(sample_job, sample_api_key): key_type=sample_api_key.key_type, created_at=created_at, job_id=sample_job.id, - job_row_number=10) + job_row_number=10, + reference="ref from client") assert Notification.query.count() == 1 assert NotificationHistory.query.count() == 1 persisted_notification = Notification.query.all()[0] assert persisted_notification.job_id == sample_job.id assert persisted_notification.job_row_number == 10 assert persisted_notification.created_at == created_at + assert persisted_notification.client_reference == "ref from client" + assert persisted_notification.reference is None @pytest.mark.parametrize('research_mode, queue, notification_type, key_type', diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index eb612ea89..a509d2a2f 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -119,15 +119,14 @@ def test_post_email_notification_returns_201(client, sample_email_template, mock headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 201 resp_json = json.loads(response.get_data(as_text=True)) - notifications = Notification.query.all() - assert len(notifications) == 1 - notification_id = notifications[0].id - assert resp_json['id'] is not None + notification = Notification.query.first() + assert resp_json['id'] == str(notification.id) assert resp_json['reference'] == "reference from caller" + assert notification.reference is None assert resp_json['content']['body'] == sample_email_template.content assert resp_json['content']['subject'] == sample_email_template.subject assert resp_json['content']['from_email'] == sample_email_template.service.email_from - assert 'v2/notifications/{}'.format(notification_id) in resp_json['uri'] + assert 'v2/notifications/{}'.format(notification.id) in resp_json['uri'] assert resp_json['template']['id'] == str(sample_email_template.id) assert resp_json['template']['version'] == sample_email_template.version assert 'v2/templates/{}'.format(sample_email_template.id) in resp_json['template']['uri'] From f5e3c6f63ba282f6aa35580c1f6d661ffb27a2b7 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 17 Nov 2016 14:02:44 +0000 Subject: [PATCH 6/6] Remove print stmt and added assert error content --- app/schema_validation/__init__.py | 2 +- .../test_notification_schemas.py | 20 ++++++++++++------- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py index 63f3a198d..e3f267999 100644 --- a/app/schema_validation/__init__.py +++ b/app/schema_validation/__init__.py @@ -42,4 +42,4 @@ def build_error_message(errors): def __format_message(e): s = e.message.split("'") msg = "{}{}".format(s[1], s[2]) - return msg if not e.cause else "'{}' {}".format(e.path[0], e.cause.message) + return msg if not e.cause else "{} {}".format(e.path[0], e.cause.message) diff --git a/tests/app/v2/notifications/test_notification_schemas.py b/tests/app/v2/notifications/test_notification_schemas.py index 13b797f37..8b6620444 100644 --- a/tests/app/v2/notifications/test_notification_schemas.py +++ b/tests/app/v2/notifications/test_notification_schemas.py @@ -56,14 +56,19 @@ def test_post_sms_schema_with_personalisation_that_is_not_a_dict(): assert len(error.keys()) == 2 -@pytest.mark.parametrize('invalid_phone_number', - ['08515111111', '07515111*11', 'notaphoneumber']) -def test_post_sms_request_invalid_phone_number(invalid_phone_number): +@pytest.mark.parametrize('invalid_phone_number, err_msg', + [('08515111111', 'phone_number Not a UK mobile number'), + ('07515111*11', 'phone_number Must not contain letters or symbols'), + ('notaphoneumber', 'phone_number Must not contain letters or symbols')]) +def test_post_sms_request_invalid_phone_number(invalid_phone_number, err_msg): j = {"phone_number": invalid_phone_number, "template_id": str(uuid.uuid4()) } - with pytest.raises(ValidationError): + with pytest.raises(ValidationError) as e: validate(j, post_sms_request) + errors = json.loads(e.value.message).get('errors') + assert len(errors) == 1 + assert {"error": "ValidationError", "message": err_msg} == errors[0] def test_post_sms_request_invalid_phone_number_and_missing_template(): @@ -71,9 +76,10 @@ def test_post_sms_request_invalid_phone_number_and_missing_template(): } with pytest.raises(ValidationError) as e: validate(j, post_sms_request) - error = json.loads(e.value.message) - print(error) - assert len(error.get('errors')) == 2 + errors = json.loads(e.value.message).get('errors') + assert len(errors) == 2 + assert {"error": "ValidationError", "message": "phone_number Not a UK mobile number"} in errors + assert {"error": "ValidationError", "message": "template_id is a required property"} in errors valid_response = {