From a8cdabcecd12c970119149a028b8cbb893d3479e Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 17 Jan 2017 12:08:24 +0000 Subject: [PATCH] - Refactor v2 post_notification to use a single method for sms and email. - Added the `simulate` notification logic to version 2. We have 3 email addresses and phone numbers that are used to simulate a successful post to /notifications. This was missed out of the version 2 endpoint. - Added a test to template_dao to check for the default value of normal for new templates - in v2 get_notifications, casted the path param to a uuid, if not uuid abort(404) --- app/notifications/process_notifications.py | 14 ++- app/notifications/rest.py | 46 +++++----- app/notifications/validators.py | 11 ++- app/v2/notifications/get_notifications.py | 12 ++- app/v2/notifications/post_notifications.py | 89 +++++++++---------- config.py | 4 +- tests/app/dao/test_templates_dao.py | 1 + .../rest/test_send_notification.py | 4 +- .../test_process_notification.py | 25 +++++- tests/app/notifications/test_rest.py | 51 ++++++----- .../notifications/test_post_notifications.py | 40 +++++++++ 11 files changed, 193 insertions(+), 104 deletions(-) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 81d37df2b..297fa4d39 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -41,7 +41,8 @@ def persist_notification(template_id, job_id=None, job_row_number=None, reference=None, - notification_id=None): + notification_id=None, + simulated=False): notification = Notification( id=notification_id, template_id=template_id, @@ -58,8 +59,9 @@ def persist_notification(template_id, job_row_number=job_row_number, client_reference=reference ) - dao_create_notification(notification) - redis_store.incr(redis.daily_limit_cache_key(service.id)) + if not simulated: + dao_create_notification(notification) + redis_store.incr(redis.daily_limit_cache_key(service.id)) return notification @@ -87,3 +89,9 @@ def send_notification_to_queue(notification, research_mode, queue=None): current_app.logger.info( "{} {} created at {}".format(notification.notification_type, notification.id, notification.created_at) ) + + +def simulated_recipient(to_address, notification_type): + return (to_address in current_app.config['SIMULATED_SMS_NUMBERS'] + if notification_type == SMS_TYPE + else to_address in current_app.config['SIMULATED_EMAIL_ADDRESSES']) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 2af3cd981..9695471c6 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -8,7 +8,7 @@ from flask import ( json ) -from app import api_user, create_uuid, statsd_client +from app import api_user, statsd_client from app.clients.email.aws_ses import get_aws_responses from app.dao import ( templates_dao, @@ -21,9 +21,12 @@ 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.notifications.process_notifications import (persist_notification, + send_notification_to_queue, + simulated_recipient) +from app.notifications.validators import (check_service_message_limit, + check_template_is_for_notification_type, + check_template_is_active) from app.schemas import ( email_notification_schema, sms_template_notification_schema, @@ -233,23 +236,26 @@ def send_notification(notification_type): _service_allowed_to_send_to(notification, service) - saved_notification = None - if not _simulated_recipient(notification['to'], notification_type): - saved_notification = persist_notification(template_id=template.id, - template_version=template.version, - recipient=notification['to'], - service=service, - personalisation=notification.get('personalisation', None), - notification_type=notification_type, - api_key_id=api_user.id, - key_type=api_user.key_type) + # Do not persist or send notification to the queue if it is a simulated recipient + simulated = simulated_recipient(notification['to'], notification_type) + saved_notification = persist_notification(template_id=template.id, + template_version=template.version, + recipient=notification['to'], + service=service, + personalisation=notification.get('personalisation', None), + notification_type=notification_type, + api_key_id=api_user.id, + key_type=api_user.key_type, + simulated=simulated) + if not simulated: send_notification_to_queue(saved_notification, service.research_mode) - - notification_id = create_uuid() if saved_notification is None else saved_notification.id + else: + current_app.logger.info("POST simulated notification for id: {}".format(saved_notification.id)) notification.update({"template_version": template.version}) + return jsonify( data=get_notification_return_data( - notification_id, + saved_notification.id, notification, template_object) ), 201 @@ -268,12 +274,6 @@ def get_notification_return_data(notification_id, notification, template): return output -def _simulated_recipient(to_address, notification_type): - return (to_address in current_app.config['SIMULATED_SMS_NUMBERS'] - if notification_type == SMS_TYPE - else to_address in current_app.config['SIMULATED_EMAIL_ADDRESSES']) - - 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 63a46d6ff..aefee34e2 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -1,7 +1,8 @@ from flask import current_app +from notifications_utils.recipients import validate_and_format_phone_number, validate_and_format_email_address from app.dao import services_dao -from app.models import KEY_TYPE_TEST, KEY_TYPE_TEAM +from app.models import KEY_TYPE_TEST, KEY_TYPE_TEAM, SMS_TYPE from app.service.utils import service_allowed_to_send_to from app.v2.errors import TooManyRequestsError, BadRequestError from app import redis_store @@ -44,6 +45,14 @@ def service_can_send_to_recipient(send_to, key_type, service): raise BadRequestError(message=message) +def validate_and_format_recipient(send_to, key_type, service, notification_type): + service_can_send_to_recipient(send_to, key_type, service) + if notification_type == SMS_TYPE: + return validate_and_format_phone_number(number=send_to) + else: + return validate_and_format_email_address(email_address=send_to) + + def check_sms_content_char_count(content_count): char_count_limit = current_app.config.get('SMS_CHAR_COUNT_LIMIT') if ( diff --git a/app/v2/notifications/get_notifications.py b/app/v2/notifications/get_notifications.py index a1ddb386d..dd86fcc99 100644 --- a/app/v2/notifications/get_notifications.py +++ b/app/v2/notifications/get_notifications.py @@ -1,4 +1,8 @@ +import uuid + from flask import jsonify, request, url_for +from werkzeug.exceptions import abort + from app import api_user from app.dao import notifications_dao from app.schema_validation import validate @@ -6,10 +10,14 @@ from app.v2.notifications import notification_blueprint from app.v2.notifications.notification_schemas import get_notifications_request -@notification_blueprint.route("/", methods=['GET']) +@notification_blueprint.route("/", methods=['GET']) def get_notification_by_id(id): + try: + casted_id = uuid.UUID(id) + except ValueError: + abort(404) notification = notifications_dao.get_notification_with_personalisation( - str(api_user.service_id), id, key_type=None + api_user.service_id, casted_id, key_type=None ) return jsonify(notification.serialize()), 200 diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index ed68f8e8b..136aca0b2 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -6,12 +6,13 @@ from app.dao import services_dao, templates_dao from app.models import SMS_TYPE, EMAIL_TYPE from app.notifications.process_notifications import (create_content_for_notification, persist_notification, - send_notification_to_queue) + send_notification_to_queue, + simulated_recipient) 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) + check_sms_content_char_count, + validate_and_format_recipient) from app.schema_validation import validate from app.v2.errors import BadRequestError from app.v2.notifications import notification_blueprint @@ -20,62 +21,52 @@ from app.v2.notifications.notification_schemas import (post_sms_request, create_post_email_response_from_notification) -@notification_blueprint.route('/sms', methods=['POST']) -def post_sms_notification(): - form = validate(request.get_json(), post_sms_request) +@notification_blueprint.route('/', methods=['POST']) +def post_notification(notification_type): + if notification_type == EMAIL_TYPE: + form = validate(request.get_json(), post_email_request) + else: + form = validate(request.get_json(), post_sms_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['phone_number'], api_user.key_type, service) + form_send_to = form['phone_number'] if notification_type == SMS_TYPE else form['email_address'] + send_to = validate_and_format_recipient(send_to=form_send_to, + key_type=api_user.key_type, + service=service, + notification_type=notification_type) - template, template_with_content = __validate_template(form, service, SMS_TYPE) + template, template_with_content = __validate_template(form, service, notification_type) + # Do not persist or send notification to the queue if it is a simulated recipient + simulated = simulated_recipient(send_to, notification_type) notification = persist_notification(template_id=template.id, template_version=template.version, - recipient=form['phone_number'], + recipient=send_to, service=service, personalisation=form.get('personalisation', None), - notification_type=SMS_TYPE, + notification_type=notification_type, api_key_id=api_user.id, key_type=api_user.key_type, - reference=form.get('reference')) - send_notification_to_queue(notification, service.research_mode) - sms_sender = service.sms_sender if service.sms_sender else current_app.config.get('FROM_NUMBER') - resp = create_post_sms_response_from_notification(notification=notification, - body=str(template_with_content), - from_number=sms_sender, - url_root=request.url_root, - service_id=service.id) - return jsonify(resp), 201 - - -@notification_blueprint.route('/email', methods=['POST']) -def post_email_notification(): - 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=service, - personalisation=form.get('personalisation', None), - notification_type=EMAIL_TYPE, - api_key_id=api_user.id, - key_type=api_user.key_type, - reference=form.get('reference')) - - send_notification_to_queue(notification, service.research_mode) - - resp = create_post_email_response_from_notification(notification=notification, - content=str(template_with_content), - subject=template_with_content.subject, - email_from=service.email_from, - url_root=request.url_root, - service_id=service.id) + reference=form.get('reference', None), + simulated=simulated) + if not simulated: + send_notification_to_queue(notification, service.research_mode) + else: + current_app.logger.info("POST simulated notification for id: {}".format(notification.id)) + if notification_type == SMS_TYPE: + sms_sender = service.sms_sender if service.sms_sender else current_app.config.get('FROM_NUMBER') + resp = create_post_sms_response_from_notification(notification=notification, + body=str(template_with_content), + from_number=sms_sender, + url_root=request.url_root, + service_id=service.id) + else: + resp = create_post_email_response_from_notification(notification=notification, + content=str(template_with_content), + subject=template_with_content.subject, + email_from=service.email_from, + url_root=request.url_root, + service_id=service.id) return jsonify(resp), 201 diff --git a/config.py b/config.py index 884fed7eb..6ce3c493f 100644 --- a/config.py +++ b/config.py @@ -140,8 +140,8 @@ class Config(object): SENDING_NOTIFICATIONS_TIMEOUT_PERIOD = 259200 # 3 days SIMULATED_EMAIL_ADDRESSES = ('simulate-delivered@notifications.service.gov.uk', - 'simulate-permanent-failure@notifications.service.gov.uk', - 'simulate-temporary-failure@notifications.service.gov.uk', + 'simulate-delivered-2@notifications.service.gov.uk', + 'simulate-delivered-3@notifications.service.gov.uk', ) SIMULATED_SMS_NUMBERS = ('+447700900000', '+447700900111', '+447700900222') diff --git a/tests/app/dao/test_templates_dao.py b/tests/app/dao/test_templates_dao.py index 94be5c965..2670254c2 100644 --- a/tests/app/dao/test_templates_dao.py +++ b/tests/app/dao/test_templates_dao.py @@ -31,6 +31,7 @@ def test_create_template(sample_service, sample_user, template_type, subject): assert Template.query.count() == 1 assert len(dao_get_all_templates_for_service(sample_service.id)) == 1 assert dao_get_all_templates_for_service(sample_service.id)[0].name == 'Sample Template' + assert dao_get_all_templates_for_service(sample_service.id)[0].process_type == 'normal' def test_update_template(sample_service, sample_user): diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index 637d7771e..89478db2f 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -710,8 +710,8 @@ def test_should_delete_notification_and_return_error_if_sqs_fails( @pytest.mark.parametrize('to_email', [ 'simulate-delivered@notifications.service.gov.uk', - 'simulate-permanent-failure@notifications.service.gov.uk', - 'simulate-temporary-failure@notifications.service.gov.uk' + 'simulate-delivered-2@notifications.service.gov.uk', + 'simulate-delivered-3@notifications.service.gov.uk' ]) def test_should_not_persist_notification_or_send_email_if_simulated_email( client, diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index f14c0fbc0..6679a7f2f 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -10,7 +10,9 @@ from collections import namedtuple from app.models import Template, Notification, NotificationHistory from app.notifications import SendNotificationToQueueError from app.notifications.process_notifications import (create_content_for_notification, - persist_notification, send_notification_to_queue) + persist_notification, + send_notification_to_queue, + simulated_recipient) from app.v2.errors import BadRequestError @@ -193,3 +195,24 @@ def test_send_notification_to_queue_throws_exception_deletes_notification(sample assert Notification.query.count() == 0 assert NotificationHistory.query.count() == 0 + + +@pytest.mark.parametrize("to_address, notification_type, expected", + [("+447700900000", "sms", True), + ("+447700900111", "sms", True), + ("+447700900222", "sms", True), + ("simulate-delivered@notifications.service.gov.uk", "email", True), + ("simulate-delivered-2@notifications.service.gov.uk", "email", True), + ("simulate-delivered-3@notifications.service.gov.uk", "email", True), + ("07515896969", "sms", False), + ("valid_email@test.com", "email", False)]) +def test_simulated_recipient(notify_api, to_address, notification_type, expected): + # The values where the expected = 'research-mode' are listed in the config['SIMULATED_EMAIL_ADDRESSES'] + # and config['SIMULATED_SMS_NUMBERS']. These values should result in using the research mode queue. + # SIMULATED_EMAIL_ADDRESSES = ('simulate-delivered@notifications.service.gov.uk', + # 'simulate-delivered-2@notifications.service.gov.uk', + # 'simulate-delivered-2@notifications.service.gov.uk') + # SIMULATED_SMS_NUMBERS = ('+447700900000', '+447700900111', '+447700900222') + + actual = simulated_recipient(to_address, notification_type) + assert actual == expected diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index eff7aee43..db60e2f47 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -13,34 +13,43 @@ from tests import create_authorization_header from tests.app.conftest import sample_notification as create_sample_notification -def test_get_sms_notification_by_id(notify_api, sample_notification): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - auth_header = create_authorization_header(service_id=sample_notification.service_id) +def test_get_sms_notification_by_id(client, sample_notification): + auth_header = create_authorization_header(service_id=sample_notification.service_id) - response = client.get( - '/notifications/{}'.format(sample_notification.id), - headers=[auth_header]) + response = client.get( + '/notifications/{}'.format(sample_notification.id), + headers=[auth_header]) - assert response.status_code == 200 - notification = json.loads(response.get_data(as_text=True))['data']['notification'] - assert notification['status'] == 'created' - assert notification['template'] == { - 'id': str(sample_notification.template.id), - 'name': sample_notification.template.name, - 'template_type': sample_notification.template.template_type, - 'version': 1 - } - assert notification['to'] == '+447700900855' - assert notification['service'] == str(sample_notification.service_id) - assert notification['body'] == "This is a template:\nwith a newline" - assert not notification.get('subject') + assert response.status_code == 200 + notification = json.loads(response.get_data(as_text=True))['data']['notification'] + assert notification['status'] == 'created' + assert notification['template'] == { + 'id': str(sample_notification.template.id), + 'name': sample_notification.template.name, + 'template_type': sample_notification.template.template_type, + 'version': 1 + } + assert notification['to'] == '+447700900855' + assert notification['service'] == str(sample_notification.service_id) + assert notification['body'] == "This is a template:\nwith a newline" + assert not notification.get('subject') + + +def test_get_sms_notification_by_invalid_id(client, sample_notification): + auth_header = create_authorization_header(service_id=sample_notification.service_id) + + response = client.get( + '/notifications/{}'.format("not_a_valid_id"), + headers=[auth_header]) + + assert response.status_code == 405 def test_get_email_notification_by_id(notify_api, notify_db, notify_db_session, sample_email_template): email_notification = create_sample_notification(notify_db, notify_db_session, + to_field="sample_email@example.com", service=sample_email_template.service, template=sample_email_template, status='sending') @@ -62,7 +71,7 @@ def test_get_email_notification_by_id(notify_api, notify_db, notify_db_session, 'template_type': email_notification.template.template_type, 'version': 1 } - assert notification['to'] == '+447700900855' + assert notification['to'] == 'sample_email@example.com' assert notification['service'] == str(email_notification.service_id) assert response.status_code == 200 assert notification['body'] == sample_email_template.content diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 34aa043bb..9c73400ab 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -166,3 +166,43 @@ def test_post_email_notification_returns_404_and_missing_template(notify_api, sa assert error_json['status_code'] == 400 assert error_json['errors'] == [{"error": "BadRequestError", "message": 'Template not found'}] + + +@pytest.mark.parametrize('recipient, notification_type', [ + ('simulate-delivered@notifications.service.gov.uk', 'email'), + ('simulate-delivered-2@notifications.service.gov.uk', 'email'), + ('simulate-delivered-3@notifications.service.gov.uk', 'email'), + ('07700 900000', 'sms'), + ('07700 900111', 'sms'), + ('07700 900222', 'sms') +]) +def test_should_not_persist_notification_or_send_notification_if_simulated_recipient( + client, + recipient, + notification_type, + sample_email_template, + sample_template, + mocker): + apply_async = mocker.patch('app.celery.provider_tasks.deliver_{}.apply_async'.format(notification_type)) + + if notification_type == 'sms': + data = { + 'phone_number': recipient, + 'template_id': str(sample_template.id) + } + else: + data = { + 'email_address': recipient, + '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/{}'.format(notification_type), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 201 + apply_async.assert_not_called() + assert Notification.query.count() == 0