From 75ca86ad0d5a3dc784ab3cf527e070477623fc69 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 19 Dec 2016 15:19:05 +0000 Subject: [PATCH 1/3] Update the send_user_confirm_new_email to persist the notification then put on the "notify" queue for delivery. The reason for doing this is to ensure the tasks performed for the Notify users are not queued behind a large job, a way to ensure priority for messages. --- app/user/rest.py | 31 ++++++++--------- tests/app/user/test_rest.py | 68 +++++++++++++------------------------ 2 files changed, 39 insertions(+), 60 deletions(-) diff --git a/app/user/rest.py b/app/user/rest.py index 1844c7dc6..9326dba7a 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -18,7 +18,7 @@ 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, KEY_TYPE_NORMAL +from app.models import SMS_TYPE, KEY_TYPE_NORMAL, EMAIL_TYPE from app.notifications.process_notifications import ( persist_notification, send_notification_to_queue @@ -33,7 +33,6 @@ from app.schemas import ( ) from app.celery.tasks import ( - send_sms, send_email ) @@ -175,24 +174,24 @@ def send_user_confirm_new_email(user_id): raise InvalidRequest(message=errors, status_code=400) template = dao_get_template_by_id(current_app.config['CHANGE_EMAIL_CONFIRMATION_TEMPLATE_ID']) - message = { - 'template': str(template.id), - 'template_version': template.version, - 'to': email['email'], - 'personalisation': { + notify_service_id = current_app.config['NOTIFY_SERVICE_ID'] + + saved_notification = persist_notification( + template_id=template.id, + template_version=template.version, + recipient=email['email'], + service_id=notify_service_id, + personalisation={ 'name': user_to_send_to.name, 'url': _create_confirmation_url(user=user_to_send_to, email_address=email['email']), 'feedback_url': current_app.config['ADMIN_BASE_URL'] + '/feedback' - } - } - - send_email.apply_async(( - current_app.config['NOTIFY_SERVICE_ID'], - str(uuid.uuid4()), - encryption.encrypt(message), - datetime.utcnow().strftime(DATETIME_FORMAT) - ), queue='notify') + }, + notification_type=EMAIL_TYPE, + api_key_id=None, + key_type=KEY_TYPE_NORMAL + ) + send_notification_to_queue(saved_notification, False, queue='notify') return jsonify({}), 204 diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py index a6513e0f9..e11748a62 100644 --- a/tests/app/user/test_rest.py +++ b/tests/app/user/test_rest.py @@ -5,9 +5,8 @@ from flask import url_for, current_app from freezegun import freeze_time import app -from app.models import (User, Permission, MANAGE_SETTINGS, MANAGE_TEMPLATES) +from app.models import (User, Permission, MANAGE_SETTINGS, MANAGE_TEMPLATES, Notification) from app.dao.permissions_dao import default_service_permissions -from app.utils import url_with_token from tests import create_authorization_header @@ -543,48 +542,29 @@ def test_send_already_registered_email_returns_400_when_data_is_missing(notify_a assert json.loads(resp.get_data(as_text=True))['message'] == {'email': ['Missing data for required field.']} -@freeze_time("2016-01-01T11:09:00.061258") -def test_send_user_confirm_new_email_returns_204(notify_api, sample_user, change_email_confirmation_template, mocker): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mocked = mocker.patch('app.celery.tasks.send_email.apply_async') - mocker.patch('uuid.uuid4', return_value='some_uuid') # for the notification id - new_email = 'new_address@dig.gov.uk' - data = json.dumps({'email': new_email}) - auth_header = create_authorization_header() +def test_send_user_confirm_new_email_returns_204(client, sample_user, change_email_confirmation_template, mocker): + mocked = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') + new_email = 'new_address@dig.gov.uk' + data = json.dumps({'email': new_email}) + auth_header = create_authorization_header() - resp = client.post(url_for('user.send_user_confirm_new_email', user_id=str(sample_user.id)), - data=data, - headers=[('Content-Type', 'application/json'), auth_header]) - assert resp.status_code == 204 - token_data = json.dumps({'user_id': str(sample_user.id), 'email': new_email}) - url = url_with_token(data=token_data, url='/user-profile/email/confirm/', config=current_app.config) - message = { - 'template': current_app.config['CHANGE_EMAIL_CONFIRMATION_TEMPLATE_ID'], - 'template_version': 1, - 'to': 'new_address@dig.gov.uk', - 'personalisation': { - 'name': sample_user.name, - 'url': url, - 'feedback_url': current_app.config['ADMIN_BASE_URL'] + '/feedback' - } - } - mocked.assert_called_once_with(( - str(current_app.config['NOTIFY_SERVICE_ID']), - "some_uuid", - app.encryption.encrypt(message), - "2016-01-01T11:09:00.061258Z"), queue="notify") + resp = client.post(url_for('user.send_user_confirm_new_email', user_id=str(sample_user.id)), + data=data, + headers=[('Content-Type', 'application/json'), auth_header]) + assert resp.status_code == 204 + notification = Notification.query.first() + mocked.assert_called_once_with( + ([str(notification.id)]), + queue="notify") -def test_send_user_confirm_new_email_returns_400_when_email_missing(notify_api, sample_user, mocker): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mocked = mocker.patch('app.celery.tasks.send_email.apply_async') - data = json.dumps({}) - auth_header = create_authorization_header() - resp = client.post(url_for('user.send_user_confirm_new_email', user_id=str(sample_user.id)), - data=data, - headers=[('Content-Type', 'application/json'), auth_header]) - assert resp.status_code == 400 - assert json.loads(resp.get_data(as_text=True))['message'] == {'email': ['Missing data for required field.']} - mocked.assert_not_called() +def test_send_user_confirm_new_email_returns_400_when_email_missing(client, sample_user, mocker): + mocked = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') + data = json.dumps({}) + auth_header = create_authorization_header() + resp = client.post(url_for('user.send_user_confirm_new_email', user_id=str(sample_user.id)), + data=data, + headers=[('Content-Type', 'application/json'), auth_header]) + assert resp.status_code == 400 + assert json.loads(resp.get_data(as_text=True))['message'] == {'email': ['Missing data for required field.']} + mocked.assert_not_called() From 741cbd1741458c47a8dd2e19c81d3fd8e094ee00 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 19 Dec 2016 15:31:54 +0000 Subject: [PATCH 2/3] Refactor send_user_email_verification to persist the notification then put on the "notify" queue for delivery. The reason for doing this is to ensure the tasks performed for the Notify users are not queued behind a large job, a way to ensure priority for messages. --- app/user/rest.py | 29 +++++++------ tests/app/user/test_rest_verify.py | 68 +++++++++++------------------- 2 files changed, 39 insertions(+), 58 deletions(-) diff --git a/app/user/rest.py b/app/user/rest.py index 1844c7dc6..8b245774f 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -18,7 +18,7 @@ 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, KEY_TYPE_NORMAL +from app.models import SMS_TYPE, KEY_TYPE_NORMAL, EMAIL_TYPE from app.notifications.process_notifications import ( persist_notification, send_notification_to_queue @@ -203,21 +203,22 @@ def send_user_email_verification(user_id): create_user_code(user_to_send_to, secret_code, 'email') template = dao_get_template_by_id(current_app.config['EMAIL_VERIFY_CODE_TEMPLATE_ID']) - message = { - 'template': str(template.id), - 'template_version': template.version, - 'to': user_to_send_to.email_address, - 'personalisation': { + + saved_notification = persist_notification( + template_id=template.id, + template_version=template.version, + recipient=user_to_send_to.email_address, + service_id=current_app.config['NOTIFY_SERVICE_ID'], + personalisation={ 'name': user_to_send_to.name, 'url': _create_verification_url(user_to_send_to, secret_code) - } - } - send_email.apply_async(( - current_app.config['NOTIFY_SERVICE_ID'], - str(uuid.uuid4()), - encryption.encrypt(message), - datetime.utcnow().strftime(DATETIME_FORMAT) - ), queue='notify') + }, + notification_type=EMAIL_TYPE, + api_key_id=None, + key_type=KEY_TYPE_NORMAL + ) + + 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 b948bf76e..f389312f8 100644 --- a/tests/app/user/test_rest_verify.py +++ b/tests/app/user/test_rest_verify.py @@ -311,55 +311,35 @@ def test_send_sms_code_returns_404_for_bad_input_data(notify_api, notify_db, not assert json.loads(resp.get_data(as_text=True))['message'] == 'No result found' -@freeze_time("2016-01-01 11:09:00.061258") -def test_send_user_email_verification(notify_api, +def test_send_user_email_verification(client, sample_user, mocker, email_verification_template): - - with notify_api.test_request_context(): - with notify_api.test_client() as client: - data = json.dumps({}) - mocker.patch('uuid.uuid4', return_value='some_uuid') # for the notification id - mocked = mocker.patch('app.celery.tasks.send_email.apply_async') - mocker.patch('notifications_utils.url_safe_token.generate_token', return_value='the-token') - auth_header = create_authorization_header() - resp = client.post( - url_for('user.send_user_email_verification', user_id=str(sample_user.id)), - data=data, - headers=[('Content-Type', 'application/json'), auth_header]) - assert resp.status_code == 204 - assert mocked.call_count == 1 - message = { - 'template': str(email_verification_template.id), - 'template_version': email_verification_template.version, - 'to': sample_user.email_address, - 'personalisation': { - 'name': sample_user.name, - 'url': current_app.config['ADMIN_BASE_URL'] + '/verify-email/' + 'the-token' - } - } - app.celery.tasks.send_email.apply_async.assert_called_once_with( - (str(current_app.config['NOTIFY_SERVICE_ID']), - 'some_uuid', - encryption.encrypt(message), - "2016-01-01T11:09:00.061258Z"), - queue="notify") + data = json.dumps({}) + mocked = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') + auth_header = create_authorization_header() + resp = client.post( + url_for('user.send_user_email_verification', user_id=str(sample_user.id)), + data=data, + headers=[('Content-Type', 'application/json'), auth_header]) + assert resp.status_code == 204 + notification = Notification.query.first() + mocked.assert_called_once_with( + ([str(notification.id)]), + queue="notify") -def test_send_email_verification_returns_404_for_bad_input_data(notify_api, notify_db, notify_db_session): +def test_send_email_verification_returns_404_for_bad_input_data(client, notify_db, notify_db_session): """ Tests POST endpoint /user//sms-code return 404 for bad input data """ - with notify_api.test_request_context(): - with notify_api.test_client() as client: - data = json.dumps({}) - import uuid - uuid_ = uuid.uuid4() - auth_header = create_authorization_header() - resp = client.post( - url_for('user.send_user_email_verification', user_id=uuid_), - data=data, - headers=[('Content-Type', 'application/json'), auth_header]) - assert resp.status_code == 404 - assert json.loads(resp.get_data(as_text=True))['message'] == 'No result found' + data = json.dumps({}) + import uuid + uuid_ = uuid.uuid4() + auth_header = create_authorization_header() + resp = client.post( + url_for('user.send_user_email_verification', user_id=uuid_), + data=data, + headers=[('Content-Type', 'application/json'), auth_header]) + assert resp.status_code == 404 + assert json.loads(resp.get_data(as_text=True))['message'] == 'No result found' From bcbfb0851b607e383baada7cc7a7f837bce292bb Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 19 Dec 2016 15:33:30 +0000 Subject: [PATCH 3/3] Fix extra space in test --- tests/app/user/test_rest.py | 42 ++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py index e11748a62..e561146de 100644 --- a/tests/app/user/test_rest.py +++ b/tests/app/user/test_rest.py @@ -543,28 +543,28 @@ def test_send_already_registered_email_returns_400_when_data_is_missing(notify_a def test_send_user_confirm_new_email_returns_204(client, sample_user, change_email_confirmation_template, mocker): - mocked = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') - new_email = 'new_address@dig.gov.uk' - data = json.dumps({'email': new_email}) - auth_header = create_authorization_header() + mocked = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') + new_email = 'new_address@dig.gov.uk' + data = json.dumps({'email': new_email}) + auth_header = create_authorization_header() - resp = client.post(url_for('user.send_user_confirm_new_email', user_id=str(sample_user.id)), - data=data, - headers=[('Content-Type', 'application/json'), auth_header]) - assert resp.status_code == 204 - notification = Notification.query.first() - mocked.assert_called_once_with( - ([str(notification.id)]), - queue="notify") + resp = client.post(url_for('user.send_user_confirm_new_email', user_id=str(sample_user.id)), + data=data, + headers=[('Content-Type', 'application/json'), auth_header]) + assert resp.status_code == 204 + notification = Notification.query.first() + mocked.assert_called_once_with( + ([str(notification.id)]), + queue="notify") def test_send_user_confirm_new_email_returns_400_when_email_missing(client, sample_user, mocker): - mocked = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') - data = json.dumps({}) - auth_header = create_authorization_header() - resp = client.post(url_for('user.send_user_confirm_new_email', user_id=str(sample_user.id)), - data=data, - headers=[('Content-Type', 'application/json'), auth_header]) - assert resp.status_code == 400 - assert json.loads(resp.get_data(as_text=True))['message'] == {'email': ['Missing data for required field.']} - mocked.assert_not_called() + mocked = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') + data = json.dumps({}) + auth_header = create_authorization_header() + resp = client.post(url_for('user.send_user_confirm_new_email', user_id=str(sample_user.id)), + data=data, + headers=[('Content-Type', 'application/json'), auth_header]) + assert resp.status_code == 400 + assert json.loads(resp.get_data(as_text=True))['message'] == {'email': ['Missing data for required field.']} + mocked.assert_not_called()