From 701851118ec731ea6ecfd5d479955dc17703f100 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 16 Jan 2017 11:15:48 +0000 Subject: [PATCH 1/6] Update templates.process_type to be `normal` and set the column as not null. --- app/models.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models.py b/app/models.py index 43d7f9930..2db1d2906 100644 --- a/app/models.py +++ b/app/models.py @@ -305,7 +305,7 @@ class Template(db.Model): process_type = db.Column(db.String(255), db.ForeignKey('template_process_type.name'), index=True, - nullable=True, + nullable=False, default=NORMAL) def get_link(self): @@ -337,7 +337,7 @@ class TemplateHistory(db.Model): process_type = db.Column(db.String(255), db.ForeignKey('template_process_type.name'), index=True, - nullable=True, + nullable=False, default=NORMAL) MMG_PROVIDER = "mmg" From 31de10ae6f7d784073e7e8eb894bd0db9760ef90 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 16 Jan 2017 11:19:46 +0000 Subject: [PATCH 2/6] dB script to default templates/templates_history.process_type = 'normal' and set the column to not null --- .../0064_update_template_process_type.py | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 migrations/versions/0064_update_template_process_type.py diff --git a/migrations/versions/0064_update_template_process_type.py b/migrations/versions/0064_update_template_process_type.py new file mode 100644 index 000000000..25df98fdc --- /dev/null +++ b/migrations/versions/0064_update_template_process_type.py @@ -0,0 +1,34 @@ +"""empty message + +Revision ID: 0064_update_template_process_type +Revises: 0063_templates_process_type +Create Date: 2017-01-16 11:08:00.520678 + +""" + +# revision identifiers, used by Alembic. +revision = '0064_update_template_process' +down_revision = '0063_templates_process_type' + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + op.execute("Update templates set process_type = 'normal'") + op.execute("Update templates_history set process_type = 'normal'") + op.alter_column('templates', 'process_type', + existing_type=sa.VARCHAR(length=255), + nullable=False) + op.alter_column('templates_history', 'process_type', + existing_type=sa.VARCHAR(length=255), + nullable=False) + + +def downgrade(): + op.alter_column('templates_history', 'process_type', + existing_type=sa.VARCHAR(length=255), + nullable=True) + op.alter_column('templates', 'process_type', + existing_type=sa.VARCHAR(length=255), + nullable=True) From a8cdabcecd12c970119149a028b8cbb893d3479e Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 17 Jan 2017 12:08:24 +0000 Subject: [PATCH 3/6] - 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 From b34c5f0bcd9a0c30745fcd12af312db7ed6e8a70 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 17 Jan 2017 13:16:26 +0000 Subject: [PATCH 4/6] If the template.process_type = PRIROITY send the message to the notify queue. We are using the notify queue in this iteration because that queue is a low volume queue with it's own dedicated workers. This just saves us from building a new queue at this point, and a new queue may not be necessary. --- app/notifications/rest.py | 12 +- app/v2/notifications/post_notifications.py | 5 +- tests/app/conftest.py | 6 +- .../rest/test_send_notification.py | 33 ++++ .../notifications/test_post_notifications.py | 178 ++++++++++-------- 5 files changed, 145 insertions(+), 89 deletions(-) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 9695471c6..ecc93c7e4 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -15,7 +15,7 @@ from app.dao import ( services_dao, notifications_dao ) -from app.models import KEY_TYPE_TEAM +from app.models import KEY_TYPE_TEAM, PRIORITY from app.models import SMS_TYPE from app.notifications.process_client_response import ( validate_callback_data, @@ -215,8 +215,7 @@ def send_notification(notification_type): if notification_type not in ['sms', 'email']: assert False - service_id = str(api_user.service_id) - service = services_dao.dao_fetch_service_by_id(service_id) + service = services_dao.dao_fetch_service_by_id(api_user.service_id) notification, errors = ( sms_template_notification_schema if notification_type == SMS_TYPE else email_notification_schema @@ -227,7 +226,7 @@ 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) + service_id=service.id) check_template_is_for_notification_type(notification_type, template.template_type) check_template_is_active(template) @@ -248,7 +247,10 @@ def send_notification(notification_type): key_type=api_user.key_type, simulated=simulated) if not simulated: - send_notification_to_queue(saved_notification, service.research_mode) + queue_name = 'notify' if template.process_type == PRIORITY else None + send_notification_to_queue(notification=saved_notification, + research_mode=service.research_mode, + queue=queue_name) else: current_app.logger.info("POST simulated notification for id: {}".format(saved_notification.id)) notification.update({"template_version": template.version}) diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 136aca0b2..3674d3a90 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -3,7 +3,7 @@ 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, EMAIL_TYPE +from app.models import SMS_TYPE, EMAIL_TYPE, PRIORITY from app.notifications.process_notifications import (create_content_for_notification, persist_notification, send_notification_to_queue, @@ -50,7 +50,8 @@ def post_notification(notification_type): reference=form.get('reference', None), simulated=simulated) if not simulated: - send_notification_to_queue(notification, service.research_mode) + queue_name = 'notify' if template.process_type == PRIORITY else None + send_notification_to_queue(notification=notification, research_mode=service.research_mode, queue=queue_name) else: current_app.logger.info("POST simulated notification for id: {}".format(notification.id)) if notification_type == SMS_TYPE: diff --git a/tests/app/conftest.py b/tests/app/conftest.py index ea702ecd4..14db86218 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -156,7 +156,8 @@ def sample_template(notify_db, subject_line='Subject', user=None, service=None, - created_by=None): + created_by=None, + process_type='normal'): if user is None: user = create_user() if service is None: @@ -169,7 +170,8 @@ def sample_template(notify_db, 'content': content, 'service': service, 'created_by': created_by, - 'archived': archived + 'archived': archived, + 'process_type': process_type } if template_type in ['email', 'letter']: data.update({ diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index 89478db2f..baf655b9a 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -957,3 +957,36 @@ def test_create_template_raises_invalid_request_when_content_too_large( if not should_error: pytest.fail("do not expect an InvalidRequest") assert e.message == {'content': ['Content has a character count greater than the limit of {}'.format(limit)]} + + +@pytest.mark.parametrize("notification_type, send_to", + [("sms", "07700 900 855"), + ("email", "sample@email.com")]) +def test_send_notification_uses_priority_queue_when_template_is_marked_as_priority(client, notify_db, + notify_db_session, mocker, + notification_type, send_to): + sample = create_sample_template( + notify_db, + notify_db_session, + template_type=notification_type, + process_type='priority' + ) + mocked = mocker.patch('app.celery.provider_tasks.deliver_{}.apply_async'.format(notification_type)) + + data = { + 'to': send_to, + 'template': str(sample.id) + } + + auth_header = create_authorization_header(service_id=sample.service_id) + + response = client.post( + path='/notifications/{}'.format(notification_type), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + response_data = json.loads(response.data)['data'] + notification_id = response_data['notification']['id'] + + assert response.status_code == 201 + mocked.assert_called_once_with([notification_id], queue='notify') diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 9c73400ab..3e1089b6a 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -1,9 +1,9 @@ import uuid - import pytest from flask import json from app.models import Notification from tests import create_authorization_header +from tests.app.conftest import sample_template as create_sample_template @pytest.mark.parametrize("reference", [None, "reference_from_client"]) @@ -43,71 +43,77 @@ def test_post_sms_notification_returns_201(notify_api, sample_template_with_plac assert mocked.called -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(uuid.uuid4()) - } - auth_header = create_authorization_header(service_id=sample_service.id) +@pytest.mark.parametrize("notification_type, key_send_to, send_to", + [("sms", "phone_number", "+447700900855"), + ("email", "email_address", "sample@email.com")]) +def test_post_sms_notification_returns_404_and_missing_template(client, sample_service, + notification_type, key_send_to, send_to): + data = { + key_send_to: send_to, + 'template_id': str(uuid.uuid4()) + } + auth_header = create_authorization_header(service_id=sample_service.id) - response = client.post( - path='/v2/notifications/sms', - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) + response = client.post( + path='/v2/notifications/{}'.format(notification_type), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) - assert response.status_code == 400 - assert response.headers['Content-type'] == 'application/json' + 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'}] + 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'}] -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: - data = { - 'phone_number': '+447700900855', - 'template_id': str(sample_template.id) - } +@pytest.mark.parametrize("notification_type, key_send_to, send_to", + [("sms", "phone_number", "+447700900855"), + ("email", "email_address", "sample@email.com")]) +def test_post_notification_returns_403_and_well_formed_auth_error(client, sample_template, + notification_type, key_send_to, send_to): + data = { + key_send_to: send_to, + 'template_id': str(sample_template.id) + } - response = client.post( - path='/v2/notifications/sms', - data=json.dumps(data), - headers=[('Content-Type', 'application/json')]) + response = client.post( + path='/v2/notifications/{}'.format(notification_type), + 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['status_code'] == 401 - assert error_resp['errors'] == [{'error': "AuthError", - 'message': 'Unauthorized, authentication token must be provided'}] + 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['status_code'] == 401 + assert error_resp['errors'] == [{'error': "AuthError", + 'message': '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) +@pytest.mark.parametrize("notification_type, key_send_to, send_to", + [("sms", "phone_number", "+447700900855"), + ("email", "email_address", "sample@email.com")]) +def test_notification_returns_400_and_for_schema_problems(client, sample_template, notification_type, key_send_to, + send_to): + data = { + key_send_to: send_to, + '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]) + response = client.post( + path='/v2/notifications/{}'.format(notification_type), + 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['status_code'] == 400 - assert error_resp['errors'] == [{'error': 'ValidationError', - 'message': "template_id is a required property" - }] + 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['status_code'] == 400 + assert error_resp['errors'] == [{'error': 'ValidationError', + 'message': "template_id is a required property" + }] @pytest.mark.parametrize("reference", [None, "reference_from_client"]) @@ -131,9 +137,9 @@ def test_post_email_notification_returns_201(client, sample_email_template_with_ assert resp_json['id'] == str(notification.id) assert resp_json['reference'] == reference assert notification.reference is None - assert resp_json['content']['body'] == sample_email_template_with_placeholders.content\ + assert resp_json['content']['body'] == sample_email_template_with_placeholders.content \ .replace('((name))', 'Bob').replace('GOV.UK', u'GOV.\u200bUK') - assert resp_json['content']['subject'] == sample_email_template_with_placeholders.subject\ + assert resp_json['content']['subject'] == sample_email_template_with_placeholders.subject \ .replace('((name))', 'Bob') assert resp_json['content']['from_email'] == sample_email_template_with_placeholders.service.email_from assert 'v2/notifications/{}'.format(notification.id) in resp_json['uri'] @@ -145,29 +151,6 @@ def test_post_email_notification_returns_201(client, sample_email_template_with_ 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'}] - - @pytest.mark.parametrize('recipient, notification_type', [ ('simulate-delivered@notifications.service.gov.uk', 'email'), ('simulate-delivered-2@notifications.service.gov.uk', 'email'), @@ -206,3 +189,38 @@ def test_should_not_persist_notification_or_send_notification_if_simulated_recip assert response.status_code == 201 apply_async.assert_not_called() assert Notification.query.count() == 0 + + +@pytest.mark.parametrize("notification_type, key_send_to, send_to", + [("sms", "phone_number", "07700 900 855"), + ("email", "email_address", "sample@email.com")]) +def test_send_notification_uses_priority_queue_when_template_is_marked_as_priority(client, notify_db, + notify_db_session, + mocker, + notification_type, + key_send_to, + send_to): + sample = create_sample_template( + notify_db, + notify_db_session, + template_type=notification_type, + process_type='priority' + ) + mocked = mocker.patch('app.celery.provider_tasks.deliver_{}.apply_async'.format(notification_type)) + + data = { + key_send_to: send_to, + 'template_id': str(sample.id) + } + + auth_header = create_authorization_header(service_id=sample.service_id) + + response = client.post( + path='/v2/notifications/{}'.format(notification_type), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + notification_id = json.loads(response.data)['id'] + + assert response.status_code == 201 + mocked.assert_called_once_with([notification_id], queue='notify') From 41b49eb8e0d232bfba345dc1c55674dc26786241 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 17 Jan 2017 15:48:51 +0000 Subject: [PATCH 5/6] Make the update template endpoint work when process_type is present. --- app/schemas.py | 1 + app/template/rest.py | 2 +- tests/app/template/test_rest.py | 22 +++++++++++++++++++++- 3 files changed, 23 insertions(+), 2 deletions(-) diff --git a/app/schemas.py b/app/schemas.py index a7fe2ff11..a33a6d47c 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -208,6 +208,7 @@ class BaseTemplateSchema(BaseSchema): class TemplateSchema(BaseTemplateSchema): created_by = field_for(models.Template, 'created_by', required=True) + process_type = field_for(models.Template, 'process_type') @validates_schema def validate_type(self, data): diff --git a/app/template/rest.py b/app/template/rest.py index 0f3fabbed..cfa1643e2 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -144,5 +144,5 @@ def _strip_html(content): def _template_has_not_changed(current_data, updated_template): return all( current_data[key] == updated_template[key] - for key in ('name', 'content', 'subject', 'archived') + for key in ('name', 'content', 'subject', 'archived', 'process_type') ) diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index fbd8c83ad..8f6e01ba3 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -1,4 +1,3 @@ -import pytest import json import random import string @@ -42,11 +41,17 @@ def test_should_create_a_new_template_for_a_service( assert json_resp['data']['service'] == str(sample_service.id) assert json_resp['data']['id'] assert json_resp['data']['version'] == 1 + assert json_resp['data']['process_type'] == 'normal' + assert json_resp['data']['created_by'] == str(sample_user.id) if subject: assert json_resp['data']['subject'] == 'subject' else: assert not json_resp['data']['subject'] + template = Template.query.get(json_resp['data']['id']) + from app.schemas import template_schema + assert sorted(json_resp['data']) == sorted(template_schema.dump(template).data) + def test_should_be_error_if_service_does_not_exist_on_create(client, sample_user, fake_uuid): data = { @@ -335,6 +340,7 @@ def test_should_get_a_single_template( assert response.status_code == 200 assert data['content'] == content assert data['subject'] == subject + assert data['process_type'] == 'normal' @pytest.mark.parametrize( @@ -516,3 +522,17 @@ def test_update_does_not_create_new_version_when_there_is_no_change(client, samp template = dao_get_template_by_id(sample_template.id) assert template.version == 1 + + +def test_update_set_process_type_on_template(client, sample_template): + auth_header = create_authorization_header() + data = { + 'process_type': 'priority' + } + resp = client.post('/service/{}/template/{}'.format(sample_template.service_id, sample_template.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + assert resp.status_code == 200 + + template = dao_get_template_by_id(sample_template.id) + assert template.process_type == 'priority' From f66670c55889ff3da693074416e2bfb32f60e562 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 18 Jan 2017 09:56:26 +0000 Subject: [PATCH 6/6] - Added logging to indicate that we created the notification and that we sent the notification to a delivery queue. - Small updates as recommended by review comments. --- app/notifications/process_notifications.py | 9 +++++-- app/notifications/rest.py | 26 +++++++++---------- app/v2/notifications/get_notifications.py | 2 +- tests/app/notifications/test_rest.py | 5 ++-- .../notifications/test_get_notifications.py | 5 ++-- .../notifications/test_post_notifications.py | 2 +- 6 files changed, 28 insertions(+), 21 deletions(-) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 297fa4d39..1b66bd36d 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -43,6 +43,7 @@ def persist_notification(template_id, reference=None, notification_id=None, simulated=False): + # if simulated create a Notification model to return but do not persist the Notification to the dB notification = Notification( id=notification_id, template_id=template_id, @@ -62,6 +63,9 @@ def persist_notification(template_id, if not simulated: dao_create_notification(notification) redis_store.incr(redis.daily_limit_cache_key(service.id)) + current_app.logger.info( + "{} {} created at {}".format(notification.notification_type, notification.id, notification.created_at) + ) return notification @@ -87,8 +91,9 @@ def send_notification_to_queue(notification, research_mode, queue=None): raise SendNotificationToQueueError() current_app.logger.info( - "{} {} created at {}".format(notification.notification_type, notification.id, notification.created_at) - ) + "{} {} sent to the {} queue for delivery".format(notification.notification_type, + notification.id, + queue)) def simulated_recipient(to_address, notification_type): diff --git a/app/notifications/rest.py b/app/notifications/rest.py index ecc93c7e4..da49eed42 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -217,7 +217,7 @@ def send_notification(notification_type): service = services_dao.dao_fetch_service_by_id(api_user.service_id) - notification, errors = ( + notification_form, errors = ( sms_template_notification_schema if notification_type == SMS_TYPE else email_notification_schema ).load(request.get_json()) if errors: @@ -225,40 +225,40 @@ 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'], + template = templates_dao.dao_get_template_by_id_and_service_id(template_id=notification_form['template'], service_id=service.id) 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', {})) + template_object = create_template_object_for_notification(template, notification_form.get('personalisation', {})) - _service_allowed_to_send_to(notification, service) + _service_allowed_to_send_to(notification_form, service) # 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, + simulated = simulated_recipient(notification_form['to'], notification_type) + notification_model = persist_notification(template_id=template.id, template_version=template.version, - recipient=notification['to'], + recipient=notification_form['to'], service=service, - personalisation=notification.get('personalisation', None), + personalisation=notification_form.get('personalisation', None), notification_type=notification_type, api_key_id=api_user.id, key_type=api_user.key_type, simulated=simulated) if not simulated: queue_name = 'notify' if template.process_type == PRIORITY else None - send_notification_to_queue(notification=saved_notification, + send_notification_to_queue(notification=notification_model, research_mode=service.research_mode, queue=queue_name) else: - current_app.logger.info("POST simulated notification for id: {}".format(saved_notification.id)) - notification.update({"template_version": template.version}) + current_app.logger.info("POST simulated notification for id: {}".format(notification_model.id)) + notification_form.update({"template_version": template.version}) return jsonify( data=get_notification_return_data( - saved_notification.id, - notification, + notification_model.id, + notification_form, template_object) ), 201 diff --git a/app/v2/notifications/get_notifications.py b/app/v2/notifications/get_notifications.py index dd86fcc99..7473cb8c0 100644 --- a/app/v2/notifications/get_notifications.py +++ b/app/v2/notifications/get_notifications.py @@ -14,7 +14,7 @@ from app.v2.notifications.notification_schemas import get_notifications_request def get_notification_by_id(id): try: casted_id = uuid.UUID(id) - except ValueError: + except ValueError or AttributeError: abort(404) notification = notifications_dao.get_notification_with_personalisation( api_user.service_id, casted_id, key_type=None diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index db60e2f47..96f36722f 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -35,11 +35,12 @@ def test_get_sms_notification_by_id(client, sample_notification): assert not notification.get('subject') -def test_get_sms_notification_by_invalid_id(client, sample_notification): +@pytest.mark.parametrize("id", ["1234-badly-formatted-id-7890", "0"]) +def test_get_sms_notification_by_invalid_id(client, sample_notification, id): auth_header = create_authorization_header(service_id=sample_notification.service_id) response = client.get( - '/notifications/{}'.format("not_a_valid_id"), + '/notifications/{}'.format(id), headers=[auth_header]) assert response.status_code == 405 diff --git a/tests/app/v2/notifications/test_get_notifications.py b/tests/app/v2/notifications/test_get_notifications.py index e7a21ff8f..5c2c31af3 100644 --- a/tests/app/v2/notifications/test_get_notifications.py +++ b/tests/app/v2/notifications/test_get_notifications.py @@ -164,10 +164,11 @@ def test_get_notification_by_id_nonexistent_id(client, sample_notification): } -def test_get_notification_by_id_invalid_id(client, sample_notification): +@pytest.mark.parametrize("id", ["1234-badly-formatted-id-7890", "0"]) +def test_get_notification_by_id_invalid_id(client, sample_notification, id): auth_header = create_authorization_header(service_id=sample_notification.service_id) response = client.get( - path='/v2/notifications/1234-badly-formatted-id-7890', + path='/v2/notifications/{}'.format(id), headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 404 diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 3e1089b6a..8ad0204e8 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -159,7 +159,7 @@ def test_post_email_notification_returns_201(client, sample_email_template_with_ ('07700 900111', 'sms'), ('07700 900222', 'sms') ]) -def test_should_not_persist_notification_or_send_notification_if_simulated_recipient( +def test_should_not_persist_or_send_notification_if_simulated_recipient( client, recipient, notification_type,