From d5d079a150f245894da9103821c3e44730d4aa3c Mon Sep 17 00:00:00 2001 From: Jenny Duckett Date: Fri, 9 Dec 2016 12:10:42 +0000 Subject: [PATCH 1/3] Add optional queue param to send_notification_to_queue We want to use this function for sending internal notifications to a different queue. --- app/notifications/process_notifications.py | 25 +++++++++-------- .../test_process_notification.py | 28 +++++++++++-------- 2 files changed, 30 insertions(+), 23 deletions(-) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 549b9d595..06c4207eb 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -70,19 +70,22 @@ def persist_notification(template_id, return notification -def send_notification_to_queue(notification, research_mode): - try: - research_mode = research_mode or notification.key_type == KEY_TYPE_TEST +def send_notification_to_queue(notification, research_mode, queue=None): + if research_mode or notification.key_type == KEY_TYPE_TEST: + queue = 'research-mode' + elif not queue: if notification.notification_type == SMS_TYPE: - provider_tasks.deliver_sms.apply_async( - [str(notification.id)], - queue='send-sms' if not research_mode else 'research-mode' - ) + queue = 'send-sms' if notification.notification_type == EMAIL_TYPE: - provider_tasks.deliver_email.apply_async( - [str(notification.id)], - queue='send-email' if not research_mode else 'research-mode' - ) + queue = 'send-email' + + if notification.notification_type == SMS_TYPE: + deliver_task = provider_tasks.deliver_sms + if notification.notification_type == EMAIL_TYPE: + deliver_task = provider_tasks.deliver_email + + try: + deliver_task.apply_async([str(notification.id)], queue=queue) except Exception as e: current_app.logger.exception(e) dao_delete_notifications_and_history_by_id(notification.id) diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 9a12fdf57..16b7ee0f2 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -135,24 +135,28 @@ def test_persist_notification_with_optionals(sample_job, sample_api_key, mocker) assert persisted_notification.reference is None -@pytest.mark.parametrize('research_mode, queue, notification_type, key_type', - [(True, 'research-mode', 'sms', 'normal'), - (True, 'research-mode', 'email', 'normal'), - (True, 'research-mode', 'email', 'team'), - (False, 'send-sms', 'sms', 'normal'), - (False, 'send-email', 'email', 'normal'), - (False, 'send-sms', 'sms', 'team'), - (False, 'research-mode', 'sms', 'test')]) +@pytest.mark.parametrize('research_mode, requested_queue, expected_queue, notification_type, key_type', + [(True, None, 'research-mode', 'sms', 'normal'), + (True, None, 'research-mode', 'email', 'normal'), + (True, None, 'research-mode', 'email', 'team'), + (False, None, 'send-sms', 'sms', 'normal'), + (False, None, 'send-email', 'email', 'normal'), + (False, None, 'send-sms', 'sms', 'team'), + (False, None, 'research-mode', 'sms', 'test'), + (True, 'notify', 'research-mode', 'email', 'normal'), + (False, 'notify', 'notify', 'sms', 'normal'), + (False, 'notify', 'notify', 'email', 'normal'), + (False, 'notify', 'research-mode', 'sms', 'test')]) def test_send_notification_to_queue(notify_db, notify_db_session, - research_mode, notification_type, - queue, key_type, mocker): + research_mode, requested_queue, expected_queue, + notification_type, key_type, mocker): mocked = mocker.patch('app.celery.provider_tasks.deliver_{}.apply_async'.format(notification_type)) template = sample_template(notify_db, notify_db_session) if notification_type == 'sms' \ else sample_email_template(notify_db, notify_db_session) notification = sample_notification(notify_db, notify_db_session, template=template, key_type=key_type) - send_notification_to_queue(notification=notification, research_mode=research_mode) + send_notification_to_queue(notification=notification, research_mode=research_mode, queue=requested_queue) - mocked.assert_called_once_with([str(notification.id)], queue=queue) + mocked.assert_called_once_with([str(notification.id)], queue=expected_queue) def test_send_notification_to_queue_throws_exception_deletes_notification(sample_notification, mocker): From e569c54f45a6f0a13a78a9f976811a25119eeaaa Mon Sep 17 00:00:00 2001 From: Jenny Duckett Date: Fri, 9 Dec 2016 14:55:24 +0000 Subject: [PATCH 2/3] Send Notify's 2FA codes via only the `notify` queue This means that these codes won't be delayed by large jobs going through the send-sms/email queues. send_user_sms_code now works much more like the endpoints for sending notifications, by persisting the notification and only using the deliver_sms task (instead of using send_sms as well). The workers consuming the `notify` queue should be able to handle the deliver task as well, so no change should be needed to the celery workers to support this. I think there's also a change in behaviour here: previously, if the Notify service was in research mode, 2FA codes would not have been sent out, making it impossible to log into the admin. Now, a call to this endpoint will always send out the notification even if we've put the Notify service into research mode, since we set the notification's key type to normal and ignore the service's research mode setting when sending the notification to the queue. --- app/user/rest.py | 34 +++++++++------ tests/app/user/test_rest_verify.py | 70 +++++++++++++++--------------- 2 files changed, 55 insertions(+), 49 deletions(-) diff --git a/app/user/rest.py b/app/user/rest.py index a188f162e..1844c7dc6 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -18,7 +18,11 @@ from app.dao.users_dao import ( from app.dao.permissions_dao import permission_dao from app.dao.services_dao import dao_fetch_service_by_id from app.dao.templates_dao import dao_get_template_by_id -from app.models import SMS_TYPE +from app.models import SMS_TYPE, KEY_TYPE_NORMAL +from app.notifications.process_notifications import ( + persist_notification, + send_notification_to_queue +) from app.schemas import ( email_data_request_schema, user_schema, @@ -143,20 +147,22 @@ def send_user_sms_code(user_id): mobile = user_to_send_to.mobile_number if verify_code.get('to', None) is None else verify_code.get('to') sms_code_template_id = current_app.config['SMS_CODE_TEMPLATE_ID'] sms_code_template = dao_get_template_by_id(sms_code_template_id) - verification_message = encryption.encrypt({ - 'template': sms_code_template_id, - 'template_version': sms_code_template.version, - 'to': mobile, - 'personalisation': { - 'verify_code': secret_code - } + notify_service_id = current_app.config['NOTIFY_SERVICE_ID'] - }) - send_sms.apply_async([current_app.config['NOTIFY_SERVICE_ID'], - str(uuid.uuid4()), - verification_message, - datetime.utcnow().strftime(DATETIME_FORMAT) - ], queue='notify') + saved_notification = persist_notification( + template_id=sms_code_template_id, + template_version=sms_code_template.version, + recipient=mobile, + service_id=notify_service_id, + personalisation={'verify_code': secret_code}, + notification_type=SMS_TYPE, + api_key_id=None, + key_type=KEY_TYPE_NORMAL + ) + # Assume that we never want to observe the Notify service's research mode + # setting for this notification - we still need to be able to log into the + # admin even if we're doing user research using this service: + send_notification_to_queue(saved_notification, False, queue='notify') return jsonify({}), 204 diff --git a/tests/app/user/test_rest_verify.py b/tests/app/user/test_rest_verify.py index 006247292..b948bf76e 100644 --- a/tests/app/user/test_rest_verify.py +++ b/tests/app/user/test_rest_verify.py @@ -1,5 +1,6 @@ import json import moto +import pytest from datetime import ( datetime, @@ -7,9 +8,11 @@ from datetime import ( ) from flask import url_for, current_app +from app.dao.services_dao import dao_update_service, dao_fetch_service_by_id from app.models import ( VerifyCode, - User + User, + Notification ) from app import db, encryption @@ -214,41 +217,47 @@ def test_user_verify_password_missing_password(notify_api, assert 'Required field missing data' in json_resp['message']['password'] +@pytest.mark.parametrize('research_mode', [True, False]) @freeze_time("2016-01-01 11:09:00.061258") def test_send_user_sms_code(notify_api, sample_user, sms_code_template, - mocker): + mocker, + research_mode): """ Tests POST endpoint /user//sms-code """ with notify_api.test_request_context(): with notify_api.test_client() as client: + if research_mode: + notify_service = dao_fetch_service_by_id(current_app.config['NOTIFY_SERVICE_ID']) + notify_service.research_mode = True + dao_update_service(notify_service) + data = json.dumps({}) auth_header = create_authorization_header() mocked = mocker.patch('app.user.rest.create_secret_code', return_value='11111') - mocker.patch('app.celery.tasks.send_sms.apply_async') - mocker.patch('uuid.uuid4', return_value='some_uuid') # for the notification id + mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + resp = client.post( url_for('user.send_user_sms_code', user_id=sample_user.id), data=data, headers=[('Content-Type', 'application/json'), auth_header]) assert resp.status_code == 204 + assert mocked.call_count == 1 - encrypted = encryption.encrypt({ - 'template': current_app.config['SMS_CODE_TEMPLATE_ID'], - 'template_version': 1, - 'to': sample_user.mobile_number, - 'personalisation': { - 'verify_code': '11111' - } - }) - app.celery.tasks.send_sms.apply_async.assert_called_once_with( - ([current_app.config['NOTIFY_SERVICE_ID'], - "some_uuid", - encrypted, - "2016-01-01T11:09:00.061258Z"]), + assert VerifyCode.query.count() == 1 + assert VerifyCode.query.first().check_code('11111') + + assert Notification.query.count() == 1 + notification = Notification.query.first() + assert notification.personalisation == {'verify_code': '11111'} + assert notification.to == sample_user.mobile_number + assert str(notification.service_id) == current_app.config['NOTIFY_SERVICE_ID'] + + app.celery.provider_tasks.deliver_sms.apply_async.assert_called_once_with( + ([str(notification.id)]), queue="notify" ) @@ -257,38 +266,29 @@ def test_send_user_sms_code(notify_api, def test_send_user_code_for_sms_with_optional_to_field(notify_api, sample_user, sms_code_template, - mock_encryption, mocker): """ - Tests POST endpoint '//code' successful sms with optional to field + Tests POST endpoint /user//sms-code with optional to field """ with notify_api.test_request_context(): with notify_api.test_client() as client: + to_number = '+441119876757' mocked = mocker.patch('app.user.rest.create_secret_code', return_value='11111') - mocker.patch('uuid.uuid4', return_value='some_uuid') # for the notification id - mocker.patch('app.celery.tasks.send_sms.apply_async') - data = json.dumps({'to': '+441119876757'}) + mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + data = json.dumps({'to': to_number}) auth_header = create_authorization_header() + resp = client.post( url_for('user.send_user_sms_code', user_id=sample_user.id), data=data, headers=[('Content-Type', 'application/json'), auth_header]) assert resp.status_code == 204 - encrypted = encryption.encrypt({ - 'template': current_app.config['SMS_CODE_TEMPLATE_ID'], - 'template_version': 1, - 'to': '+441119876757', - 'personalisation': { - 'verify_code': '11111' - } - }) assert mocked.call_count == 1 - app.celery.tasks.send_sms.apply_async.assert_called_once_with( - ([current_app.config['NOTIFY_SERVICE_ID'], - "some_uuid", - encrypted, - "2016-01-01T11:09:00.061258Z"]), + notification = Notification.query.first() + assert notification.to == to_number + app.celery.provider_tasks.deliver_sms.apply_async.assert_called_once_with( + ([str(notification.id)]), queue="notify" ) From 7332874415245be45828325059089cefa1c5513d Mon Sep 17 00:00:00 2001 From: Jenny Duckett Date: Fri, 9 Dec 2016 17:37:18 +0000 Subject: [PATCH 3/3] Use namedtuple in test_send_notification_to_queue This makes this test a couple of seconds faster - 0.7s instead of 2.5s for me locally. sample_notification also creates a service, template, user and permissions, but we don't need any of these objects to exist in the database for this test. It's particularly helpful for this test because there are so many parameterized cases. Thanks @leohemsted for suggesting doing this here. --- tests/app/notifications/test_process_notification.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 16b7ee0f2..7190e0fac 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -5,6 +5,7 @@ import pytest from boto3.exceptions import Boto3Error from sqlalchemy.exc import SQLAlchemyError from freezegun import freeze_time +from collections import namedtuple from app.models import Template, Notification, NotificationHistory from app.notifications import SendNotificationToQueueError @@ -151,9 +152,14 @@ def test_send_notification_to_queue(notify_db, notify_db_session, research_mode, requested_queue, expected_queue, notification_type, key_type, mocker): mocked = mocker.patch('app.celery.provider_tasks.deliver_{}.apply_async'.format(notification_type)) - template = sample_template(notify_db, notify_db_session) if notification_type == 'sms' \ - else sample_email_template(notify_db, notify_db_session) - notification = sample_notification(notify_db, notify_db_session, template=template, key_type=key_type) + Notification = namedtuple('Notification', ['id', 'key_type', 'notification_type', 'created_at']) + notification = Notification( + id=uuid.uuid4(), + key_type=key_type, + notification_type=notification_type, + created_at=datetime.datetime(2016, 11, 11, 16, 8, 18), + ) + send_notification_to_queue(notification=notification, research_mode=research_mode, queue=requested_queue) mocked.assert_called_once_with([str(notification.id)], queue=expected_queue)