From 635debb5a601ec8456713944244f64b1b47c0596 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 23 Feb 2016 17:30:50 +0000 Subject: [PATCH 01/12] Moved the sending sms for a job into celery tasks --- app/celery/tasks.py | 3 +- app/dao/jobs_dao.py | 2 +- app/notifications/rest.py | 70 +++--- app/schemas.py | 6 + tests/app/notifications/test_rest.py | 334 +++++++++++++++++++++------ 5 files changed, 315 insertions(+), 100 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 2ab34b3b9..14f0d6314 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -9,7 +9,7 @@ from sqlalchemy.exc import SQLAlchemyError @notify_celery.task(name="send-sms") -def send_sms(service_id, notification_id, encrypted_notification): +def send_sms(service_id, notification_id, encrypted_notification, job_id=None): notification = encryption.decrypt(encrypted_notification) template = get_model_templates(notification['template']) @@ -19,6 +19,7 @@ def send_sms(service_id, notification_id, encrypted_notification): template_id=notification['template'], to=notification['to'], service_id=service_id, + job_id=job_id, status='sent' ) save_notification(notification_db_object) diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index 9cb192af0..95a79623f 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -14,7 +14,7 @@ def save_job(job, update_dict={}): def get_job(service_id, job_id): - return Job.query.filter_by(service_id=service_id, id=job_id).one() + return Job.query.filter_by(service_id=service_id, id=job_id).first() def get_jobs_by_service(service_id): diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 3035066f1..d0c8a8dae 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -8,17 +8,17 @@ from flask import ( ) from app import api_user, encryption -from app.aws_sqs import add_notification_to_queue from app.dao import ( templates_dao, - users_dao, services_dao, - notifications_dao + notifications_dao, + jobs_dao ) from app.schemas import ( email_notification_schema, sms_template_notification_schema, - notification_status_schema + notification_status_schema, + job_sms_template_notification_schema ) from app.celery.tasks import send_sms, send_email from sqlalchemy.orm.exc import NoResultFound @@ -73,6 +73,44 @@ def create_sms_notification(): return jsonify({'notification_id': notification_id}), 201 +@notifications.route('/sms/service/', methods=['POST']) +def create_sms_for_service(service_id): + notification, errors = job_sms_template_notification_schema.load(request.get_json()) + if errors: + return jsonify(result="error", message=errors), 400 + + template = templates_dao.dao_get_template_by_id_and_service_id( + template_id=notification['template'], + service_id=service_id + ) + + if not template: + return jsonify(result="error", message={'template': ['Template or service not found']}), 400 + + job_id = notification['job'] + + job = jobs_dao.get_job(service_id, job_id) + + if not job: + return jsonify(result="error", message={'job': ['Job not found']}), 400 + + service = services_dao.dao_fetch_service_by_id(service_id) + + if service.restricted: + if notification['to'] not in [user.email_address for user in service.users]: + return jsonify(result="error", message={'to': ['Invalid phone number for restricted service']}), 400 + + notification_id = create_notification_id() + + send_sms.apply_async(( + api_user['client'], + notification_id, + encryption.encrypt(notification), + job_id), + queue='sms') + return jsonify({'notification_id': notification_id}), 201 + + @notifications.route('/email', methods=['POST']) def create_email_notification(): notification, errors = email_notification_schema.load(request.get_json()) @@ -103,27 +141,3 @@ def create_email_notification(): encryption.encrypt(notification)), queue='email') return jsonify({'notification_id': notification_id}), 201 - - -@notifications.route('/sms/service/', methods=['POST']) -def create_sms_for_service(service_id): - resp_json = request.get_json() - - notification, errors = sms_template_notification_schema.load(resp_json) - if errors: - return jsonify(result="error", message=errors), 400 - - template_id = notification['template'] - job_id = notification['job'] - - # TODO: job/job_id is in notification and can used to update job status - - # TODO: remove once beta is reading notifications from the queue - template = templates_dao.get_model_templates(template_id) - - if template.service.id != uuid.UUID(service_id): - message = "Invalid template: id {} for service id: {}".format(template.id, service_id) - return jsonify(result="error", message=message), 400 - - notification_id = add_notification_to_queue(service_id, template_id, 'sms', notification) - return jsonify({'notification_id': notification_id}), 201 diff --git a/app/schemas.py b/app/schemas.py index c024c2be1..942ca643f 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -108,6 +108,11 @@ class SmsTemplateNotificationSchema(SmsNotificationSchema): job = fields.String() +class JobSmsTemplateNotificationSchema(SmsNotificationSchema): + template = fields.Int(required=True) + job = fields.String(required=True) + + class SmsAdminNotificationSchema(SmsNotificationSchema): content = fields.Str(required=True) @@ -138,6 +143,7 @@ old_request_verify_code_schema = OldRequestVerifyCodeSchema() request_verify_code_schema = RequestVerifyCodeSchema() sms_admin_notification_schema = SmsAdminNotificationSchema() sms_template_notification_schema = SmsTemplateNotificationSchema() +job_sms_template_notification_schema = JobSmsTemplateNotificationSchema() email_notification_schema = EmailNotificationSchema() notification_status_schema = NotificationStatusSchema() notifications_status_schema = NotificationStatusSchema(many=True) diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 575243e76..b4914a82c 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -6,6 +6,7 @@ from flask import json from app.models import Service from app.dao.templates_dao import get_model_templates from app.dao.services_dao import dao_update_service +from tests.app.conftest import sample_job def test_get_notification_by_id(notify_api, sample_notification): @@ -72,6 +73,32 @@ def test_create_sms_should_reject_if_missing_required_fields(notify_api, sample_ assert response.status_code == 400 +def test_create_sms_should_reject_if_missing_job_id_on_job_sms(notify_api, sample_api_key, mocker): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + mocker.patch('app.celery.tasks.send_sms.apply_async') + + data = {} + auth_header = create_authorization_header( + service_id=sample_api_key.service_id, + request_body=json.dumps(data), + path='/notifications/sms/service/{}'.format(sample_api_key.service_id), + method='POST') + + response = client.post( + path='/notifications/sms/service/{}'.format(sample_api_key.service_id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + json_resp = json.loads(response.get_data(as_text=True)) + app.celery.tasks.send_sms.apply_async.assert_not_called() + assert json_resp['result'] == 'error' + assert 'Missing data for required field.' in json_resp['message']['to'][0] + assert 'Missing data for required field.' in json_resp['message']['template'][0] + assert 'Missing data for required field.' in json_resp['message']['job'][0] + assert response.status_code == 400 + + def test_should_reject_bad_phone_numbers(notify_api, sample_template, mocker): with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -100,6 +127,35 @@ def test_should_reject_bad_phone_numbers(notify_api, sample_template, mocker): assert response.status_code == 400 +def test_should_reject_bad_phone_numbers_on_job_sms(notify_api, sample_job, mocker): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + mocker.patch('app.celery.tasks.send_sms.apply_async') + + data = { + 'to': 'invalid', + 'template': sample_job.template.id, + 'job': sample_job.id + } + auth_header = create_authorization_header( + request_body=json.dumps(data), + path='/notifications/sms/service/{}'.format(sample_job.service_id), + method='POST') + + response = client.post( + path='/notifications/sms/service/{}'.format(sample_job.service_id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + json_resp = json.loads(response.get_data(as_text=True)) + app.celery.tasks.send_sms.apply_async.assert_not_called() + + assert json_resp['result'] == 'error' + assert len(json_resp['message'].keys()) == 1 + assert 'Invalid phone number, must be of format +441234123123' in json_resp['message']['to'] + assert response.status_code == 400 + + def test_send_notification_invalid_template_id(notify_api, sample_template, mocker): with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -128,6 +184,99 @@ def test_send_notification_invalid_template_id(notify_api, sample_template, mock assert 'Template not found' in json_resp['message']['template'] +def test_send_notification_invalid_service_id_on_job_sms(notify_api, sample_job, mocker): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + mocker.patch('app.celery.tasks.send_sms.apply_async') + + service_id = uuid.uuid4() + + data = { + 'to': '+441234123123', + 'template': sample_job.template.id, + 'job': sample_job.id + } + + auth_header = create_authorization_header( + service_id=sample_job.service.id, + request_body=json.dumps(data), + path='/notifications/sms/service/{}'.format(service_id), + method='POST') + + response = client.post( + path='/notifications/sms/service/{}'.format(service_id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + json_resp = json.loads(response.get_data(as_text=True)) + app.celery.tasks.send_sms.apply_async.assert_not_called() + + assert response.status_code == 400 + assert len(json_resp['message'].keys()) == 1 + assert 'Template or service not found' in json_resp['message']['template'] + + +def test_send_notification_invalid_template_id_on_job_sms(notify_api, sample_job, mocker): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + mocker.patch('app.celery.tasks.send_sms.apply_async') + + data = { + 'to': '+441234123123', + 'template': 9999, + 'job': sample_job.id + } + + auth_header = create_authorization_header( + service_id=sample_job.service.id, + request_body=json.dumps(data), + path='/notifications/sms/service/{}'.format(sample_job.service_id), + method='POST') + + response = client.post( + path='/notifications/sms/service/{}'.format(sample_job.service_id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + json_resp = json.loads(response.get_data(as_text=True)) + app.celery.tasks.send_sms.apply_async.assert_not_called() + + assert response.status_code == 400 + assert len(json_resp['message'].keys()) == 1 + assert 'Template or service not found' in json_resp['message']['template'] + + +def test_send_notification_invalid_job_id_on_job_sms(notify_api, sample_template, mocker): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + mocker.patch('app.celery.tasks.send_sms.apply_async') + + data = { + 'to': '+441234123123', + 'template': sample_template.id, + 'job': uuid.uuid4() + + } + + auth_header = create_authorization_header( + service_id=sample_template.service.id, + request_body=json.dumps(data), + path='/notifications/sms/service/{}'.format(sample_template.service_id), + method='POST') + + response = client.post( + path='/notifications/sms/service/{}'.format(sample_template.service_id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + json_resp = json.loads(response.get_data(as_text=True)) + app.celery.tasks.send_sms.apply_async.assert_not_called() + + assert response.status_code == 400 + assert len(json_resp['message'].keys()) == 1 + assert 'Job not found' in json_resp['message']['job'] + + def test_prevents_sending_to_any_mobile_on_restricted_service(notify_api, sample_template, mocker): with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -162,6 +311,41 @@ def test_prevents_sending_to_any_mobile_on_restricted_service(notify_api, sample assert 'Invalid phone number for restricted service' in json_resp['message']['to'] +def test_prevents_sending_to_any_mobile_on_restricted_service_on_job_sms(notify_api, sample_job, mocker): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + mocker.patch('app.celery.tasks.send_sms.apply_async') + + Service.query.filter_by( + id=sample_job.service.id + ).update( + {'restricted': True} + ) + invalid_mob = '+449999999999' + data = { + 'to': invalid_mob, + 'template': sample_job.template.id, + 'job': sample_job.id + } + + auth_header = create_authorization_header( + service_id=sample_job.service.id, + request_body=json.dumps(data), + path='/notifications/sms/service/{}'.format(sample_job.service_id), + method='POST') + + response = client.post( + path='/notifications/sms/service/{}'.format(sample_job.service_id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + json_resp = json.loads(response.get_data(as_text=True)) + app.celery.tasks.send_sms.apply_async.assert_not_called() + + assert response.status_code == 400 + assert 'Invalid phone number for restricted service' in json_resp['message']['to'] + + def test_should_not_allow_template_from_another_service(notify_api, service_factory, sample_user, mocker): with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -194,6 +378,50 @@ def test_should_not_allow_template_from_another_service(notify_api, service_fact assert 'Template not found' in json_resp['message']['template'] +def test_should_not_allow_template_from_another_service_on_job_sms( + notify_db, + notify_db_session, + notify_api, + service_factory, + sample_user, + mocker): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + mocker.patch('app.celery.tasks.send_sms.apply_async') + + service_1 = service_factory.get('service 1', user=sample_user) + service_2 = service_factory.get('service 2', user=sample_user) + + service_1_templates = get_model_templates(service_id=service_2.id) + service_2_templates = get_model_templates(service_id=service_2.id) + + job_1 = sample_job(notify_db, notify_db_session, service_1, service_1_templates[0]) + sample_job(notify_db, notify_db_session, service_2, service_2_templates[0]) + + data = { + 'to': sample_user.mobile_number, + 'template': service_2_templates[0].id, + 'job': job_1.id + } + + auth_header = create_authorization_header( + service_id=service_1.id, + request_body=json.dumps(data), + path='/notifications/sms/service/{}'.format(service_1.id), + method='POST') + + response = client.post( + path='/notifications/sms/service/{}'.format(service_1.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + json_resp = json.loads(response.get_data(as_text=True)) + app.celery.tasks.send_sms.apply_async.assert_not_called() + + assert response.status_code == 400 + assert 'Template or service not found' in json_resp['message']['template'] + + def test_should_allow_valid_sms_notification(notify_api, sample_template, mocker): with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -228,6 +456,42 @@ def test_should_allow_valid_sms_notification(notify_api, sample_template, mocker assert notification_id +def test_should_allow_valid_sms_notification_for_job(notify_api, sample_job, mocker): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + mocker.patch('app.celery.tasks.send_sms.apply_async') + mocker.patch('app.encryption.encrypt', return_value="something_encrypted") + + data = { + 'to': '+441234123123', + 'template': sample_job.template.id, + 'job': sample_job.id + } + + auth_header = create_authorization_header( + request_body=json.dumps(data), + path='/notifications/sms/service/{}'.format(sample_job.service_id), + method='POST', + service_id=sample_job.service_id + ) + + response = client.post( + path='/notifications/sms/service/{}'.format(sample_job.service_id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + notification_id = json.loads(response.data)['notification_id'] + app.celery.tasks.send_sms.apply_async.assert_called_once_with( + (str(sample_job.template.service_id), + notification_id, + "something_encrypted", + str(sample_job.id)), + queue="sms" + ) + assert response.status_code == 201 + assert notification_id + + def test_create_email_should_reject_if_missing_required_fields(notify_api, sample_api_key, mocker): with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -405,73 +669,3 @@ def test_should_allow_valid_email_notification(notify_api, sample_email_template ) assert response.status_code == 201 assert notification_id - - -@moto.mock_sqs -def test_valid_message_with_service_id(notify_api, - notify_db, - notify_db_session, - sqs_client_conn, - sample_user, - sample_template, - mocker): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - job_id = uuid.uuid4() - service_id = sample_template.service.id - url = '/notifications/sms/service/{}'.format(service_id) - data = { - 'to': '+441234123123', - 'template': sample_template.id, - 'job': job_id - } - auth_header = create_authorization_header( - request_body=json.dumps(data), - path=url, - method='POST') - - response = client.post( - url, - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) - - assert response.status_code == 201 - assert json.loads(response.data)['notification_id'] is not None - - -@moto.mock_sqs -def test_message_with_incorrect_service_id_should_fail(notify_api, - notify_db, - notify_db_session, - sqs_client_conn, - sample_user, - sample_template, - mocker): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - job_id = uuid.uuid4() - invalid_service_id = uuid.uuid4() - - url = '/notifications/sms/service/{}'.format(invalid_service_id) - - data = { - 'to': '+441234123123', - 'template': sample_template.id, - 'job': job_id - } - - auth_header = create_authorization_header( - request_body=json.dumps(data), - path=url, - method='POST') - - response = client.post( - url, - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) - - json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 400 - expected_error = 'Invalid template: id {} for service id: {}'.format(sample_template.id, - invalid_service_id) - assert json_resp['message'] == expected_error From 201c2d01ba37f1f487b52e268c6abdf2f51474a7 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 23 Feb 2016 17:39:08 +0000 Subject: [PATCH 02/12] Task is the same whether job based or not - use notification to build action - notification has job - based in encrypted blob --- app/celery/tasks.py | 4 ++-- app/job/rest.py | 4 ++-- app/notifications/rest.py | 3 +-- tests/app/celery/test_tasks.py | 26 ++++++++++++++++++++++++++ tests/app/notifications/test_rest.py | 4 +--- 5 files changed, 32 insertions(+), 9 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 14f0d6314..4dab98e9f 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -9,7 +9,7 @@ from sqlalchemy.exc import SQLAlchemyError @notify_celery.task(name="send-sms") -def send_sms(service_id, notification_id, encrypted_notification, job_id=None): +def send_sms(service_id, notification_id, encrypted_notification): notification = encryption.decrypt(encrypted_notification) template = get_model_templates(notification['template']) @@ -19,7 +19,7 @@ def send_sms(service_id, notification_id, encrypted_notification, job_id=None): template_id=notification['template'], to=notification['to'], service_id=service_id, - job_id=job_id, + job_id=notification.get('job', None), status='sent' ) save_notification(notification_db_object) diff --git a/app/job/rest.py b/app/job/rest.py index c3e4cf772..9ba4a7600 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -41,12 +41,12 @@ def get_job_for_service(service_id, job_id=None): if job_id: try: job = get_job(service_id, job_id) + if not job: + return jsonify(result="error", message="Job not found"), 404 data, errors = job_schema.dump(job) return jsonify(data=data) except DataError: return jsonify(result="error", message="Invalid job id"), 400 - except NoResultFound: - return jsonify(result="error", message="Job not found"), 404 else: jobs = get_jobs_by_service(service_id) data, errors = jobs_schema.dump(jobs) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index d0c8a8dae..ae06a9703 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -105,8 +105,7 @@ def create_sms_for_service(service_id): send_sms.apply_async(( api_user['client'], notification_id, - encryption.encrypt(notification), - job_id), + encryption.encrypt(notification)), queue='sms') return jsonify({'notification_id': notification_id}), 201 diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index e252cfd09..a9207ca4e 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -31,6 +31,32 @@ def test_should_send_template_to_correct_sms_provider_and_persist(sample_templat assert persisted_notification.to == '+441234123123' assert persisted_notification.template_id == sample_template.id assert persisted_notification.status == 'sent' + assert not persisted_notification.job_id + + +def test_should_send_template_to_correct_sms_provider_and_persist_with_job_id(sample_job, mocker): + notification = { + "template": sample_job.template.id, + "job": sample_job.id, + "to": "+441234123123" + } + mocker.patch('app.encryption.decrypt', return_value=notification) + mocker.patch('app.firetext_client.send_sms') + + notification_id = uuid.uuid4() + + send_sms( + sample_job.service.id, + notification_id, + "encrypted-in-reality") + + firetext_client.send_sms.assert_called_once_with("+441234123123", sample_job.template.content) + persisted_notification = notifications_dao.get_notification(sample_job.template.service_id, notification_id) + assert persisted_notification.id == notification_id + assert persisted_notification.to == '+441234123123' + assert persisted_notification.job_id == sample_job.id + assert persisted_notification.template_id == sample_job.template.id + assert persisted_notification.status == 'sent' def test_should_send_template_to_email_provider_and_persist(sample_email_template, mocker): diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index b4914a82c..9ec412eb8 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -1,6 +1,5 @@ import uuid import app.celery.tasks -import moto from tests import create_authorization_header from flask import json from app.models import Service @@ -484,8 +483,7 @@ def test_should_allow_valid_sms_notification_for_job(notify_api, sample_job, moc app.celery.tasks.send_sms.apply_async.assert_called_once_with( (str(sample_job.template.service_id), notification_id, - "something_encrypted", - str(sample_job.id)), + "something_encrypted"), queue="sms" ) assert response.status_code == 201 From 0007c699720ed58bee3d1bbd44b454fa759b3917 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 24 Feb 2016 09:23:21 +0000 Subject: [PATCH 03/12] Restored code to share sms creation logic between: - sms for API calls - sms for Jobs --- app/notifications/rest.py | 57 ++++++++++++---------------- tests/app/notifications/test_rest.py | 22 +++++++---- 2 files changed, 38 insertions(+), 41 deletions(-) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index ae06a9703..ba6573fed 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -45,37 +45,24 @@ def get_notifications(notification_id): @notifications.route('/sms', methods=['POST']) def create_sms_notification(): - notification, errors = sms_template_notification_schema.load(request.get_json()) - if errors: - return jsonify(result="error", message=errors), 400 - - template = templates_dao.dao_get_template_by_id_and_service_id( - template_id=notification['template'], - service_id=api_user['client'] - ) - - if not template: - return jsonify(result="error", message={'template': ['Template not found']}), 400 - - service = services_dao.dao_fetch_service_by_id(api_user['client']) - - if service.restricted: - if notification['to'] not in [user.email_address for user in service.users]: - return jsonify(result="error", message={'to': ['Invalid phone number for restricted service']}), 400 - - notification_id = create_notification_id() - - send_sms.apply_async(( - api_user['client'], - notification_id, - encryption.encrypt(notification)), - queue='sms') - return jsonify({'notification_id': notification_id}), 201 + return base_create_sms_notification(expects_job=False) @notifications.route('/sms/service/', methods=['POST']) def create_sms_for_service(service_id): - notification, errors = job_sms_template_notification_schema.load(request.get_json()) + return base_create_sms_notification(service_id, expects_job=True) + + +def base_create_sms_notification(service_id=None, expects_job=False): + if not service_id: + service_id = api_user['client'] + + if expects_job: + schema = job_sms_template_notification_schema + else: + schema = sms_template_notification_schema + + notification, errors = schema.load(request.get_json()) if errors: return jsonify(result="error", message=errors), 400 @@ -85,14 +72,18 @@ def create_sms_for_service(service_id): ) if not template: - return jsonify(result="error", message={'template': ['Template or service not found']}), 400 + return jsonify( + result="error", + message={ + 'template': ['Template {} not found for service {}'.format(notification['template'], service_id)] + } + ), 400 - job_id = notification['job'] + if expects_job: + job = jobs_dao.get_job(service_id, notification['job']) - job = jobs_dao.get_job(service_id, job_id) - - if not job: - return jsonify(result="error", message={'job': ['Job not found']}), 400 + if not job: + return jsonify(result="error", message={'job': ['Job {} not found'.format(notification['job'])]}), 400 service = services_dao.dao_fetch_service_by_id(service_id) diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 9ec412eb8..6057effd6 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -180,7 +180,8 @@ def test_send_notification_invalid_template_id(notify_api, sample_template, mock assert response.status_code == 400 assert len(json_resp['message'].keys()) == 1 - assert 'Template not found' in json_resp['message']['template'] + test_string = 'Template {} not found for service {}'.format(9999, sample_template.service.id) + assert test_string in json_resp['message']['template'] def test_send_notification_invalid_service_id_on_job_sms(notify_api, sample_job, mocker): @@ -212,7 +213,8 @@ def test_send_notification_invalid_service_id_on_job_sms(notify_api, sample_job, assert response.status_code == 400 assert len(json_resp['message'].keys()) == 1 - assert 'Template or service not found' in json_resp['message']['template'] + test_string = 'Template {} not found for service {}'.format(sample_job.template.id, service_id) + assert test_string in json_resp['message']['template'] def test_send_notification_invalid_template_id_on_job_sms(notify_api, sample_job, mocker): @@ -242,18 +244,19 @@ def test_send_notification_invalid_template_id_on_job_sms(notify_api, sample_job assert response.status_code == 400 assert len(json_resp['message'].keys()) == 1 - assert 'Template or service not found' in json_resp['message']['template'] + test_string = 'Template {} not found for service {}'.format(9999, sample_job.service.id) + assert test_string in json_resp['message']['template'] def test_send_notification_invalid_job_id_on_job_sms(notify_api, sample_template, mocker): with notify_api.test_request_context(): with notify_api.test_client() as client: mocker.patch('app.celery.tasks.send_sms.apply_async') - + job_id = uuid.uuid4() data = { 'to': '+441234123123', 'template': sample_template.id, - 'job': uuid.uuid4() + 'job': job_id } @@ -273,7 +276,8 @@ def test_send_notification_invalid_job_id_on_job_sms(notify_api, sample_template assert response.status_code == 400 assert len(json_resp['message'].keys()) == 1 - assert 'Job not found' in json_resp['message']['job'] + test_string = 'Job {} not found'.format(job_id) + assert test_string in json_resp['message']['job'] def test_prevents_sending_to_any_mobile_on_restricted_service(notify_api, sample_template, mocker): @@ -374,7 +378,8 @@ def test_should_not_allow_template_from_another_service(notify_api, service_fact app.celery.tasks.send_sms.apply_async.assert_not_called() assert response.status_code == 400 - assert 'Template not found' in json_resp['message']['template'] + test_string = 'Template {} not found for service {}'.format(service_2_templates[0].id, service_1.id) + assert test_string in json_resp['message']['template'] def test_should_not_allow_template_from_another_service_on_job_sms( @@ -418,7 +423,8 @@ def test_should_not_allow_template_from_another_service_on_job_sms( app.celery.tasks.send_sms.apply_async.assert_not_called() assert response.status_code == 400 - assert 'Template or service not found' in json_resp['message']['template'] + test_string = 'Template {} not found for service {}'.format(service_2_templates[0].id, service_1.id) + assert test_string in json_resp['message']['template'] def test_should_allow_valid_sms_notification(notify_api, sample_template, mocker): From b0609b18133b65e846527d2ca40ef67a1f1e6cf9 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 24 Feb 2016 09:55:05 +0000 Subject: [PATCH 04/12] More refactors - single base method to send notifications - knows about service id (present if job based) - knows about jobs - if needed - knows about type Does the right thing Was lots of shared code around error checking now in one place. --- app/celery/tasks.py | 1 + app/notifications/rest.py | 93 ++++++++++++++-------------- app/schemas.py | 6 ++ tests/app/notifications/test_rest.py | 9 ++- 4 files changed, 61 insertions(+), 48 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 4dab98e9f..7c8be70aa 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -45,6 +45,7 @@ def send_email(service_id, notification_id, subject, from_address, encrypted_not template_id=notification['template'], to=notification['to'], service_id=service_id, + job_id=notification.get('job', None), status='sent' ) save_notification(notification_db_object) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index ba6573fed..40d0e45f1 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -18,7 +18,8 @@ from app.schemas import ( email_notification_schema, sms_template_notification_schema, notification_status_schema, - job_sms_template_notification_schema + job_sms_template_notification_schema, + job_email_template_notification_schema ) from app.celery.tasks import send_sms, send_email from sqlalchemy.orm.exc import NoResultFound @@ -29,6 +30,9 @@ from app.errors import register_errors register_errors(notifications) +SMS_NOTIFICATION = 'sms' +EMAIL_NOTIFICATION = 'email' + def create_notification_id(): return str(uuid.uuid4()) @@ -45,22 +49,36 @@ def get_notifications(notification_id): @notifications.route('/sms', methods=['POST']) def create_sms_notification(): - return base_create_sms_notification(expects_job=False) + return send_notification(notification_type=SMS_NOTIFICATION, expects_job=False) @notifications.route('/sms/service/', methods=['POST']) -def create_sms_for_service(service_id): - return base_create_sms_notification(service_id, expects_job=True) +def create_sms_for_job(service_id): + return send_notification(service_id=service_id, notification_type=SMS_NOTIFICATION, expects_job=True) -def base_create_sms_notification(service_id=None, expects_job=False): +@notifications.route('/email', methods=['POST']) +def create_email_notification(): + return send_notification(notification_type=EMAIL_NOTIFICATION, expects_job=False) + + +@notifications.route('/email/service/', methods=['POST']) +def create_email_notification_for_job(service_id): + return send_notification(service_id=service_id, notification_type=EMAIL_NOTIFICATION, expects_job=True) + + +def send_notification(notification_type, service_id=None, expects_job=False): + assert notification_type + if not service_id: service_id = api_user['client'] if expects_job: - schema = job_sms_template_notification_schema + schema = job_sms_template_notification_schema if notification_type is SMS_NOTIFICATION else \ + job_email_template_notification_schema else: - schema = sms_template_notification_schema + schema = sms_template_notification_schema if notification_type is SMS_NOTIFICATION else \ + email_notification_schema notification, errors = schema.load(request.get_json()) if errors: @@ -85,49 +103,32 @@ def base_create_sms_notification(service_id=None, expects_job=False): if not job: return jsonify(result="error", message={'job': ['Job {} not found'.format(notification['job'])]}), 400 - service = services_dao.dao_fetch_service_by_id(service_id) - - if service.restricted: - if notification['to'] not in [user.email_address for user in service.users]: - return jsonify(result="error", message={'to': ['Invalid phone number for restricted service']}), 400 - - notification_id = create_notification_id() - - send_sms.apply_async(( - api_user['client'], - notification_id, - encryption.encrypt(notification)), - queue='sms') - return jsonify({'notification_id': notification_id}), 201 - - -@notifications.route('/email', methods=['POST']) -def create_email_notification(): - notification, errors = email_notification_schema.load(request.get_json()) - if errors: - return jsonify(result="error", message=errors), 400 - - template = templates_dao.dao_get_template_by_id_and_service_id( - template_id=notification['template'], - service_id=api_user['client'] - ) - - if not template: - return jsonify(result="error", message={'template': ['Template not found']}), 400 - service = services_dao.dao_fetch_service_by_id(api_user['client']) if service.restricted: - if notification['to'] not in [user.email_address for user in service.users]: - return jsonify(result="error", message={'to': ['Email address not permitted for restricted service']}), 400 + if notification_type is SMS_NOTIFICATION: + if notification['to'] not in [user.mobile_number for user in service.users]: + return jsonify( + result="error", message={'to': ['Invalid phone number for restricted service']}), 400 + else: + if notification['to'] not in [user.email_address for user in service.users]: + return jsonify( + result="error", message={'to': ['Email address not permitted for restricted service']}), 400 notification_id = create_notification_id() - send_email.apply_async(( - api_user['client'], - notification_id, - template.subject, - "{}@{}".format(service.email_from, current_app.config['NOTIFY_EMAIL_DOMAIN']), - encryption.encrypt(notification)), - queue='email') + if notification_type is SMS_NOTIFICATION: + send_sms.apply_async(( + api_user['client'], + notification_id, + encryption.encrypt(notification)), + queue='sms') + else: + send_email.apply_async(( + api_user['client'], + notification_id, + template.subject, + "{}@{}".format(service.email_from, current_app.config['NOTIFY_EMAIL_DOMAIN']), + encryption.encrypt(notification)), + queue='email') return jsonify({'notification_id': notification_id}), 201 diff --git a/app/schemas.py b/app/schemas.py index 942ca643f..de4c52f16 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -113,6 +113,11 @@ class JobSmsTemplateNotificationSchema(SmsNotificationSchema): job = fields.String(required=True) +class JobEmailTemplateNotificationSchema(EmailNotificationSchema): + template = fields.Int(required=True) + job = fields.String(required=True) + + class SmsAdminNotificationSchema(SmsNotificationSchema): content = fields.Str(required=True) @@ -145,6 +150,7 @@ sms_admin_notification_schema = SmsAdminNotificationSchema() sms_template_notification_schema = SmsTemplateNotificationSchema() job_sms_template_notification_schema = JobSmsTemplateNotificationSchema() email_notification_schema = EmailNotificationSchema() +job_email_template_notification_schema = JobEmailTemplateNotificationSchema() notification_status_schema = NotificationStatusSchema() notifications_status_schema = NotificationStatusSchema(many=True) notification_status_schema_load_json = NotificationStatusSchema(load_json=True) diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 6057effd6..18cc4e9f1 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -572,7 +572,11 @@ def test_should_reject_email_notification_with_template_id_that_cant_be_found( app.celery.tasks.send_email.apply_async.assert_not_called() assert response.status_code == 400 assert data['result'] == 'error' - assert data['message']['template'][0] == 'Template not found' + test_string = 'Template {} not found for service {}'.format( + 1234, + sample_email_template.service.id + ) + assert test_string in data['message']['template'] def test_should_not_allow_email_template_from_another_service(notify_api, service_factory, sample_user, mocker): @@ -605,7 +609,8 @@ def test_should_not_allow_email_template_from_another_service(notify_api, servic app.celery.tasks.send_email.apply_async.assert_not_called() assert response.status_code == 400 - assert 'Template not found' in json_resp['message']['template'] + test_string = 'Template {} not found for service {}'.format(service_2_templates[0].id, service_1.id) + assert test_string in json_resp['message']['template'] def test_should_not_send_email_if_restricted_and_not_a_service_user(notify_api, sample_email_template, mocker): From 83899762502ce59933e4258349d4ee5c8938063f Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 24 Feb 2016 11:11:02 +0000 Subject: [PATCH 05/12] Use service ID, not from token to build notification --- app/notifications/rest.py | 4 ++-- tests/app/notifications/test_rest.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 40d0e45f1..2db48909a 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -119,13 +119,13 @@ def send_notification(notification_type, service_id=None, expects_job=False): if notification_type is SMS_NOTIFICATION: send_sms.apply_async(( - api_user['client'], + service_id, notification_id, encryption.encrypt(notification)), queue='sms') else: send_email.apply_async(( - api_user['client'], + service_id, notification_id, template.subject, "{}@{}".format(service.email_from, current_app.config['NOTIFY_EMAIL_DOMAIN']), diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 18cc4e9f1..0cd7f05fe 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -487,7 +487,7 @@ def test_should_allow_valid_sms_notification_for_job(notify_api, sample_job, moc notification_id = json.loads(response.data)['notification_id'] app.celery.tasks.send_sms.apply_async.assert_called_once_with( - (str(sample_job.template.service_id), + (str(sample_job.service_id), notification_id, "something_encrypted"), queue="sms" From 6ac6a64b4603b97097105f55d3ce19e063af6ea5 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 24 Feb 2016 11:44:11 +0000 Subject: [PATCH 06/12] Tests for email job notifications endpoint --- tests/app/notifications/test_rest.py | 233 +++++++++++++++++++++++++++ 1 file changed, 233 insertions(+) diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 0cd7f05fe..e3fe9c208 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -521,6 +521,32 @@ def test_create_email_should_reject_if_missing_required_fields(notify_api, sampl assert response.status_code == 400 +def test_create_email_job_should_reject_if_missing_required_fields(notify_api, sample_api_key, mocker): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + mocker.patch('app.celery.tasks.send_email.apply_async') + + data = {} + auth_header = create_authorization_header( + service_id=sample_api_key.service_id, + request_body=json.dumps(data), + path='/notifications/email/service/{}'.format(sample_api_key.service_id), + method='POST') + + response = client.post( + path='/notifications/email/service/{}'.format(sample_api_key.service_id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + json_resp = json.loads(response.get_data(as_text=True)) + app.celery.tasks.send_email.apply_async.assert_not_called() + assert json_resp['result'] == 'error' + assert 'Missing data for required field.' in json_resp['message']['to'][0] + assert 'Missing data for required field.' in json_resp['message']['template'][0] + assert 'Missing data for required field.' in json_resp['message']['job'][0] + assert response.status_code == 400 + + def test_should_reject_email_notification_with_bad_email(notify_api, sample_email_template, mocker): with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -548,6 +574,34 @@ def test_should_reject_email_notification_with_bad_email(notify_api, sample_emai assert data['message']['to'][0] == 'Invalid email' +def test_should_reject_email_job_notification_with_bad_email(notify_api, sample_job, sample_email_template, mocker): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + mocker.patch('app.celery.tasks.send_email.apply_async') + to_address = "bad-email" + data = { + 'to': to_address, + 'template': sample_email_template.service.id, + 'job': sample_job.id + } + auth_header = create_authorization_header( + service_id=sample_email_template.service.id, + request_body=json.dumps(data), + path='/notifications/email/service/{}'.format(sample_email_template.service_id), + method='POST') + + response = client.post( + path='/notifications/email/service/{}'.format(sample_email_template.service_id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + data = json.loads(response.get_data(as_text=True)) + app.celery.tasks.send_email.apply_async.assert_not_called() + assert response.status_code == 400 + assert data['result'] == 'error' + assert data['message']['to'][0] == 'Invalid email' + + def test_should_reject_email_notification_with_template_id_that_cant_be_found( notify_api, sample_email_template, mocker): with notify_api.test_request_context(): @@ -579,6 +633,38 @@ def test_should_reject_email_notification_with_template_id_that_cant_be_found( assert test_string in data['message']['template'] +def test_should_reject_email_job_notification_with_template_id_that_cant_be_found( + notify_api, sample_job, sample_email_template, mocker): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + mocker.patch('app.celery.tasks.send_email.apply_async') + data = { + 'to': 'ok@ok.com', + 'template': 1234, + 'job': sample_job.id + } + auth_header = create_authorization_header( + service_id=sample_email_template.service.id, + request_body=json.dumps(data), + path='/notifications/email/service/{}'.format(sample_job.service.id), + method='POST') + + response = client.post( + path='/notifications/email/service/{}'.format(sample_job.service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + data = json.loads(response.get_data(as_text=True)) + app.celery.tasks.send_email.apply_async.assert_not_called() + assert response.status_code == 400 + assert data['result'] == 'error' + test_string = 'Template {} not found for service {}'.format( + 1234, + sample_email_template.service.id + ) + assert test_string in data['message']['template'] + + def test_should_not_allow_email_template_from_another_service(notify_api, service_factory, sample_user, mocker): with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -613,6 +699,52 @@ def test_should_not_allow_email_template_from_another_service(notify_api, servic assert test_string in json_resp['message']['template'] +def test_should_not_allow_template_from_another_service_on_job_email( + notify_db, + notify_db_session, + notify_api, + service_factory, + sample_user, + mocker): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + mocker.patch('app.celery.tasks.send_email.apply_async') + + service_1 = service_factory.get('service 1', user=sample_user, template_type='email') + service_2 = service_factory.get('service 2', user=sample_user, template_type='email') + + service_1_templates = get_model_templates(service_id=service_2.id) + service_2_templates = get_model_templates(service_id=service_2.id) + + job_1 = sample_job(notify_db, notify_db_session, service_1, service_1_templates[0]) + sample_job(notify_db, notify_db_session, service_2, service_2_templates[0]) + + data = { + 'to': sample_user.email_address, + 'template': service_2_templates[0].id, + 'job': job_1.id + } + + auth_header = create_authorization_header( + service_id=service_1.id, + request_body=json.dumps(data), + path='/notifications/email/service/{}'.format(service_1.id), + method='POST') + + response = client.post( + path='/notifications/email/service/{}'.format(service_1.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + json_resp = json.loads(response.get_data(as_text=True)) + app.celery.tasks.send_email.apply_async.assert_not_called() + + assert response.status_code == 400 + print(json_resp) + test_string = 'Template {} not found for service {}'.format(service_2_templates[0].id, service_1.id) + assert test_string in json_resp['message']['template'] + + def test_should_not_send_email_if_restricted_and_not_a_service_user(notify_api, sample_email_template, mocker): with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -644,6 +776,38 @@ def test_should_not_send_email_if_restricted_and_not_a_service_user(notify_api, assert 'Email address not permitted for restricted service' in json_resp['message']['to'] +def test_should_not_send_email_for_job_if_restricted_and_not_a_service_user(notify_api, sample_job, sample_email_template, mocker): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + mocker.patch('app.celery.tasks.send_email.apply_async') + + sample_email_template.service.restricted = True + dao_update_service(sample_email_template) + + data = { + 'to': "not-someone-we-trust@email-address.com", + 'template': sample_job.template.id, + 'job': sample_job.id + } + + auth_header = create_authorization_header( + service_id=sample_job.service.id, + request_body=json.dumps(data), + path='/notifications/email', + method='POST') + + response = client.post( + path='/notifications/email', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + json_resp = json.loads(response.get_data(as_text=True)) + app.celery.tasks.send_email.apply_async.assert_not_called() + + assert response.status_code == 400 + assert 'Email address not permitted for restricted service' in json_resp['message']['to'] + + def test_should_allow_valid_email_notification(notify_api, sample_email_template, mocker): with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -678,3 +842,72 @@ def test_should_allow_valid_email_notification(notify_api, sample_email_template ) assert response.status_code == 201 assert notification_id + + +def test_send_notification_invalid_job_id_on_job_email(notify_api, sample_email_template, mocker): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + mocker.patch('app.celery.tasks.send_email.apply_async') + job_id = uuid.uuid4() + data = { + 'to': 'test@test.com', + 'template': sample_email_template.id, + 'job': job_id + + } + + auth_header = create_authorization_header( + service_id=sample_email_template.service.id, + request_body=json.dumps(data), + path='/notifications/email/service/{}'.format(sample_email_template.service_id), + method='POST') + + response = client.post( + path='/notifications/email/service/{}'.format(sample_email_template.service_id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + json_resp = json.loads(response.get_data(as_text=True)) + app.celery.tasks.send_email.apply_async.assert_not_called() + + assert response.status_code == 400 + assert len(json_resp['message'].keys()) == 1 + test_string = 'Job {} not found'.format(job_id) + assert test_string in json_resp['message']['job'] + + +def test_should_allow_valid_email_notification_for_job(notify_api, sample_job, sample_email_template, mocker): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + mocker.patch('app.celery.tasks.send_email.apply_async') + mocker.patch('app.encryption.encrypt', return_value="something_encrypted") + + data = { + 'to': 'ok@ok.com', + 'template': sample_email_template.id, + 'job': sample_job.id + } + + auth_header = create_authorization_header( + request_body=json.dumps(data), + path='/notifications/email/service/{}'.format(sample_job.service_id), + method='POST', + service_id=sample_job.service_id + ) + + response = client.post( + path='/notifications/email/service/{}'.format(sample_job.service_id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + notification_id = json.loads(response.data)['notification_id'] + app.celery.tasks.send_email.apply_async.assert_called_once_with( + (str(sample_job.service_id), + notification_id, + "Email Subject", + "sample.service@test.notify.com", + "something_encrypted"), + queue="email" + ) + assert response.status_code == 201 + assert notification_id From 1667f82df1aa7cc3d9a4f8c82aa1f2267ba3e342 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 24 Feb 2016 11:51:02 +0000 Subject: [PATCH 07/12] Removed some unused template dao methods --- app/celery/tasks.py | 6 +++--- app/dao/templates_dao.py | 32 ++++------------------------ tests/app/conftest.py | 6 +++--- tests/app/notifications/test_rest.py | 20 ++++++++++------- 4 files changed, 22 insertions(+), 42 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 7c8be70aa..0d636623d 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -1,7 +1,7 @@ from app import notify_celery, encryption, firetext_client, aws_ses_client from app.clients.email.aws_ses import AwsSesClientException from app.clients.sms.firetext import FiretextClientException -from app.dao.templates_dao import get_model_templates +from app.dao.templates_dao import dao_get_template_by_id from app.dao.notifications_dao import save_notification from app.models import Notification from flask import current_app @@ -11,7 +11,7 @@ from sqlalchemy.exc import SQLAlchemyError @notify_celery.task(name="send-sms") def send_sms(service_id, notification_id, encrypted_notification): notification = encryption.decrypt(encrypted_notification) - template = get_model_templates(notification['template']) + template = dao_get_template_by_id(notification['template']) try: notification_db_object = Notification( @@ -37,7 +37,7 @@ def send_sms(service_id, notification_id, encrypted_notification): @notify_celery.task(name="send-email") def send_email(service_id, notification_id, subject, from_address, encrypted_notification): notification = encryption.decrypt(encrypted_notification) - template = get_model_templates(notification['template']) + template = dao_get_template_by_id(notification['template']) try: notification_db_object = Notification( diff --git a/app/dao/templates_dao.py b/app/dao/templates_dao.py index 3085c412b..2e854f24d 100644 --- a/app/dao/templates_dao.py +++ b/app/dao/templates_dao.py @@ -3,34 +3,6 @@ from app.models import (Template, Service) from sqlalchemy import asc -def save_model_template(template, update_dict=None): - if update_dict: - update_dict.pop('id', None) - service = update_dict.pop('service') - Template.query.filter_by(id=template.id).update(update_dict) - template.service = service - else: - db.session.add(template) - db.session.commit() - - -def delete_model_template(template): - db.session.delete(template) - db.session.commit() - - -def get_model_templates(template_id=None, service_id=None): - # TODO need better mapping from function params to sql query. - if template_id and service_id: - return Template.query.filter_by( - id=template_id, service_id=service_id).one() - elif template_id: - return Template.query.filter_by(id=template_id).one() - elif service_id: - return Template.query.filter_by(service=Service.query.get(service_id)).all() - return Template.query.all() - - def dao_create_template(template): db.session.add(template) db.session.commit() @@ -45,5 +17,9 @@ def dao_get_template_by_id_and_service_id(template_id, service_id): return Template.query.filter_by(id=template_id, service_id=service_id).first() +def dao_get_template_by_id(template_id): + return Template.query.filter_by(id=template_id).first() + + def dao_get_all_templates_for_service(service_id): return Template.query.filter_by(service=Service.query.get(service_id)).order_by(asc(Template.created_at)).all() diff --git a/tests/app/conftest.py b/tests/app/conftest.py index d1be74a20..64f634951 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -4,7 +4,7 @@ from app import email_safe from app.models import (User, Service, Template, ApiKey, Job, Notification) from app.dao.users_dao import (save_model_user, create_user_code, create_secret_code) from app.dao.services_dao import dao_create_service -from app.dao.templates_dao import save_model_template +from app.dao.templates_dao import dao_create_template from app.dao.api_key_dao import save_model_api_key from app.dao.jobs_dao import save_job from app.dao.notifications_dao import save_notification @@ -138,7 +138,7 @@ def sample_template(notify_db, 'subject': subject_line }) template = Template(**data) - save_model_template(template) + dao_create_template(template) return template @@ -165,7 +165,7 @@ def sample_email_template( 'subject': subject_line }) template = Template(**data) - save_model_template(template) + dao_create_template(template) return template diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index e3fe9c208..1157800d5 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -3,7 +3,7 @@ import app.celery.tasks from tests import create_authorization_header from flask import json from app.models import Service -from app.dao.templates_dao import get_model_templates +from app.dao.templates_dao import dao_get_all_templates_for_service from app.dao.services_dao import dao_update_service from tests.app.conftest import sample_job @@ -357,7 +357,7 @@ def test_should_not_allow_template_from_another_service(notify_api, service_fact service_1 = service_factory.get('service 1', user=sample_user) service_2 = service_factory.get('service 2', user=sample_user) - service_2_templates = get_model_templates(service_id=service_2.id) + service_2_templates = dao_get_all_templates_for_service(service_id=service_2.id) data = { 'to': sample_user.mobile_number, 'template': service_2_templates[0].id @@ -396,8 +396,8 @@ def test_should_not_allow_template_from_another_service_on_job_sms( service_1 = service_factory.get('service 1', user=sample_user) service_2 = service_factory.get('service 2', user=sample_user) - service_1_templates = get_model_templates(service_id=service_2.id) - service_2_templates = get_model_templates(service_id=service_2.id) + service_1_templates = dao_get_all_templates_for_service(service_id=service_2.id) + service_2_templates = dao_get_all_templates_for_service(service_id=service_2.id) job_1 = sample_job(notify_db, notify_db_session, service_1, service_1_templates[0]) sample_job(notify_db, notify_db_session, service_2, service_2_templates[0]) @@ -673,7 +673,7 @@ def test_should_not_allow_email_template_from_another_service(notify_api, servic service_1 = service_factory.get('service 1', template_type='email', user=sample_user) service_2 = service_factory.get('service 2', template_type='email', user=sample_user) - service_2_templates = get_model_templates(service_id=service_2.id) + service_2_templates = dao_get_all_templates_for_service(service_id=service_2.id) data = { 'to': sample_user.email_address, @@ -713,8 +713,8 @@ def test_should_not_allow_template_from_another_service_on_job_email( service_1 = service_factory.get('service 1', user=sample_user, template_type='email') service_2 = service_factory.get('service 2', user=sample_user, template_type='email') - service_1_templates = get_model_templates(service_id=service_2.id) - service_2_templates = get_model_templates(service_id=service_2.id) + service_1_templates = dao_get_all_templates_for_service(service_id=service_2.id) + service_2_templates = dao_get_all_templates_for_service(service_id=service_2.id) job_1 = sample_job(notify_db, notify_db_session, service_1, service_1_templates[0]) sample_job(notify_db, notify_db_session, service_2, service_2_templates[0]) @@ -776,7 +776,11 @@ def test_should_not_send_email_if_restricted_and_not_a_service_user(notify_api, assert 'Email address not permitted for restricted service' in json_resp['message']['to'] -def test_should_not_send_email_for_job_if_restricted_and_not_a_service_user(notify_api, sample_job, sample_email_template, mocker): +def test_should_not_send_email_for_job_if_restricted_and_not_a_service_user( + notify_api, + sample_job, + sample_email_template, + mocker): with notify_api.test_request_context(): with notify_api.test_client() as client: mocker.patch('app.celery.tasks.send_email.apply_async') From b3884e2d6ce3f124faa02128654fd17c24bbf309 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 24 Feb 2016 17:12:30 +0000 Subject: [PATCH 08/12] Move job processing into celery - brings boto S3 into new AWS folder - CSV processing utils method Rejigs the jobs rest endpoint - removes some now unused endpoints, Calls to the task with the job, job processing in task, delegating SMS calls to the sms task --- app/__init__.py | 7 +- app/aws/s3.py | 7 + app/celery/tasks.py | 31 +++ app/csv.py | 12 + app/dao/jobs_dao.py | 29 ++- app/errors.py | 2 +- app/job/rest.py | 159 ++++-------- app/notifications/rest.py | 22 +- scripts/run_celery.sh | 2 +- test_csv_files/multiple_sms.csv | 11 + test_csv_files/sms.csv | 2 + tests/app/__init__.py | 7 + tests/app/celery/test_tasks.py | 50 +++- tests/app/conftest.py | 4 +- tests/app/dao/test_jobs_dao.py | 63 ++--- tests/app/dao/test_notification_dao.py | 2 +- tests/app/job/test_job_rest.py | 328 ------------------------- tests/app/job/test_rest.py | 226 +++++++++++++++++ tests/app/notifications/test_rest.py | 1 + tests/app/test_csv.py | 15 ++ 20 files changed, 453 insertions(+), 527 deletions(-) create mode 100644 app/aws/s3.py create mode 100644 app/csv.py create mode 100644 test_csv_files/multiple_sms.csv create mode 100644 test_csv_files/sms.csv delete mode 100644 tests/app/job/test_job_rest.py create mode 100644 tests/app/job/test_rest.py create mode 100644 tests/app/test_csv.py diff --git a/app/__init__.py b/app/__init__.py index 87863e235..c94608710 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -1,3 +1,4 @@ +import uuid import os import re from flask import request, url_for @@ -50,7 +51,7 @@ def create_app(): application.register_blueprint(user_blueprint, url_prefix='/user') application.register_blueprint(template_blueprint) application.register_blueprint(status_blueprint, url_prefix='/status') - application.register_blueprint(notifications_blueprint, url_prefix='/notifications') + application.register_blueprint(notifications_blueprint) application.register_blueprint(job_blueprint) return application @@ -99,3 +100,7 @@ def email_safe(string): character.lower() if character.isalnum() or character == "." else "" for character in re.sub("\s+", ".", string.strip()) ]) + + +def create_uuid(): + return str(uuid.uuid4()) diff --git a/app/aws/s3.py b/app/aws/s3.py new file mode 100644 index 000000000..ea7df1f31 --- /dev/null +++ b/app/aws/s3.py @@ -0,0 +1,7 @@ +from boto3 import resource + + +def get_job_from_s3(bucket_name, job_id): + s3 = resource('s3') + key = s3.Object(bucket_name, '{}.csv'.format(job_id)) + return key.get()['Body'].read().decode('utf-8') diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 0d636623d..ebfa2c4af 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -1,11 +1,42 @@ +from app import create_uuid from app import notify_celery, encryption, firetext_client, aws_ses_client from app.clients.email.aws_ses import AwsSesClientException from app.clients.sms.firetext import FiretextClientException from app.dao.templates_dao import dao_get_template_by_id from app.dao.notifications_dao import save_notification +from app.dao.jobs_dao import dao_update_job, dao_get_job_by_id from app.models import Notification from flask import current_app from sqlalchemy.exc import SQLAlchemyError +from app.aws import s3 +from app.csv import get_mobile_numbers_from_csv + + +@notify_celery.task(name="process-job") +def process_job(job_id): + job = dao_get_job_by_id(job_id) + job.status = 'in progress' + dao_update_job(job) + + file = s3.get_job_from_s3(job.bucket_name, job_id) + mobile_numbers = get_mobile_numbers_from_csv(file) + + for mobile_number in mobile_numbers: + notification = encryption.encrypt({ + 'template': job.template_id, + 'job': str(job.id), + 'to': mobile_number + }) + + send_sms.apply_async(( + str(job.service_id), + str(create_uuid()), + notification), + queue='sms' + ) + + job.status = 'finished' + dao_update_job(job) @notify_celery.task(name="send-sms") diff --git a/app/csv.py b/app/csv.py new file mode 100644 index 000000000..371203a54 --- /dev/null +++ b/app/csv.py @@ -0,0 +1,12 @@ +import csv + + +def get_mobile_numbers_from_csv(file_data): + numbers = [] + reader = csv.DictReader( + file_data.splitlines(), + lineterminator='\n', + quoting=csv.QUOTE_NONE) + for i, row in enumerate(reader): + numbers.append(row['phone'].replace(' ', '')) + return numbers diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index 95a79623f..38b8466e2 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -2,24 +2,23 @@ from app import db from app.models import Job -def save_job(job, update_dict={}): - if update_dict: - update_dict.pop('id', None) - update_dict.pop('service', None) - update_dict.pop('template', None) - Job.query.filter_by(id=job.id).update(update_dict) - else: - db.session.add(job) - db.session.commit() - - -def get_job(service_id, job_id): +def dao_get_job_by_service_id_and_job_id(service_id, job_id): return Job.query.filter_by(service_id=service_id, id=job_id).first() -def get_jobs_by_service(service_id): +def dao_get_jobs_by_service_id(service_id): return Job.query.filter_by(service_id=service_id).all() -def _get_jobs(): - return Job.query.all() +def dao_get_job_by_id(job_id): + return Job.query.filter_by(id=job_id).first() + + +def dao_create_job(job): + db.session.add(job) + db.session.commit() + + +def dao_update_job(job): + db.session.add(job) + db.session.commit() diff --git a/app/errors.py b/app/errors.py index da3b719ca..15f8977ad 100644 --- a/app/errors.py +++ b/app/errors.py @@ -43,7 +43,7 @@ def register_errors(blueprint): @blueprint.app_errorhandler(DataError) def no_result_found(e): current_app.logger.error(e) - return jsonify(error="No result found"), 404 + return jsonify(result="error", message="No result found"), 404 @blueprint.app_errorhandler(SQLAlchemyError) def db_error(e): diff --git a/app/job/rest.py b/app/job/rest.py index 9ba4a7600..0c9e90fe8 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -1,148 +1,81 @@ -import boto3 -import json - from flask import ( Blueprint, jsonify, - request, - current_app + request ) -from sqlalchemy.exc import DataError -from sqlalchemy.orm.exc import NoResultFound - from app.dao.jobs_dao import ( - save_job, - get_job, - get_jobs_by_service + dao_create_job, + dao_get_job_by_service_id_and_job_id, + dao_get_jobs_by_service_id, + dao_update_job ) -from app.dao import notifications_dao +from app.dao.services_dao import ( + dao_fetch_service_by_id +) from app.schemas import ( job_schema, - jobs_schema, - job_schema_load_json, - notification_status_schema, - notifications_status_schema, - notification_status_schema_load_json + jobs_schema ) +from app.celery.tasks import process_job + job = Blueprint('job', __name__, url_prefix='/service//job') - from app.errors import register_errors + register_errors(job) @job.route('/', methods=['GET']) +def get_job_by_service_and_job_id(service_id, job_id): + job = dao_get_job_by_service_id_and_job_id(service_id, job_id) + if not job: + return jsonify(result="error", message="Job {} not found for service {}".format(job_id, service_id)), 404 + data, errors = job_schema.dump(job) + return jsonify(data=data) + + @job.route('', methods=['GET']) -def get_job_for_service(service_id, job_id=None): - if job_id: - try: - job = get_job(service_id, job_id) - if not job: - return jsonify(result="error", message="Job not found"), 404 - data, errors = job_schema.dump(job) - return jsonify(data=data) - except DataError: - return jsonify(result="error", message="Invalid job id"), 400 - else: - jobs = get_jobs_by_service(service_id) - data, errors = jobs_schema.dump(jobs) - return jsonify(data=data) +def get_jobs_by_service(service_id): + jobs = dao_get_jobs_by_service_id(service_id) + data, errors = jobs_schema.dump(jobs) + return jsonify(data=data) @job.route('', methods=['POST']) def create_job(service_id): - job, errors = job_schema.load(request.get_json()) + + service = dao_fetch_service_by_id(service_id) + if not service: + return jsonify(result="error", message="Service {} not found".format(service_id)), 404 + + data = request.get_json() + data.update({ + "service": service_id + }) + job, errors = job_schema.load(data) if errors: return jsonify(result="error", message=errors), 400 - save_job(job) - _enqueue_job(job) - + dao_create_job(job) + process_job.apply_async([str(job.id)], queue="process-job") return jsonify(data=job_schema.dump(job).data), 201 -@job.route('/', methods=['PUT']) +@job.route('/', methods=['POST']) def update_job(service_id, job_id): + fetched_job = dao_get_job_by_service_id_and_job_id(service_id, job_id) + if not fetched_job: + return jsonify(result="error", message="Job {} not found for service {}".format(job_id, service_id)), 404 - job = get_job(service_id, job_id) - update_dict, errors = job_schema_load_json.load(request.get_json()) + current_data = dict(job_schema.dump(fetched_job).data.items()) + current_data.update(request.get_json()) + + update_dict, errors = job_schema.load(current_data) if errors: return jsonify(result="error", message=errors), 400 - - save_job(job, update_dict=update_dict) - - return jsonify(data=job_schema.dump(job).data), 200 - - -@job.route('//notification', methods=['POST']) -def create_notification_for_job(service_id, job_id): - - # TODO assert service_id == payload service id - # and same for job id - notification, errors = notification_status_schema.load(request.get_json()) - if errors: - return jsonify(result="error", message=errors), 400 - - notifications_dao.save_notification(notification) - - return jsonify(data=notification_status_schema.dump(notification).data), 201 - - -@job.route('//notification', methods=['GET']) -@job.route('//notification/') -def get_notification_for_job(service_id, job_id, notification_id=None): - if notification_id: - try: - notification = notifications_dao.get_notification_for_job(service_id, job_id, notification_id) - data, errors = notification_status_schema.dump(notification) - return jsonify(data=data) - except DataError: - return jsonify(result="error", message="Invalid notification id"), 400 - except NoResultFound: - return jsonify(result="error", message="Notification not found"), 404 - else: - notifications = notifications_dao.get_notifications_for_job(service_id, job_id) - data, errors = notifications_status_schema.dump(notifications) - return jsonify(data=data) - - -@job.route('//notification/', methods=['PUT']) -def update_notification_for_job(service_id, job_id, notification_id): - - notification = notifications_dao.get_notification_for_job(service_id, job_id, notification_id) - update_dict, errors = notification_status_schema_load_json.load(request.get_json()) - - if errors: - return jsonify(result="error", message=errors), 400 - - notifications_dao.save_notification(notification, update_dict=update_dict) - - return jsonify(data=job_schema.dump(notification).data), 200 - - -def _enqueue_job(job): - aws_region = current_app.config['AWS_REGION'] - queue_name = current_app.config['NOTIFY_JOB_QUEUE'] - - queue = boto3.resource('sqs', region_name=aws_region).create_queue(QueueName=queue_name) - data = { - 'id': str(job.id), - 'service': str(job.service.id), - 'template': job.template.id, - 'bucket_name': job.bucket_name, - 'file_name': job.file_name, - 'original_file_name': job.original_file_name - } - job_json = json.dumps(data) - queue.send_message(MessageBody=job_json, - MessageAttributes={'id': {'StringValue': str(job.id), 'DataType': 'String'}, - 'service': {'StringValue': str(job.service.id), 'DataType': 'String'}, - 'template': {'StringValue': str(job.template.id), 'DataType': 'String'}, - 'bucket_name': {'StringValue': job.bucket_name, 'DataType': 'String'}, - 'file_name': {'StringValue': job.file_name, 'DataType': 'String'}, - 'original_file_name': {'StringValue': job.original_file_name, - 'DataType': 'String'}}) + dao_update_job(update_dict) + return jsonify(data=job_schema.dump(update_dict).data), 200 diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 2db48909a..035a10a7e 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -1,5 +1,3 @@ -import uuid - from flask import ( Blueprint, jsonify, @@ -7,7 +5,7 @@ from flask import ( current_app ) -from app import api_user, encryption +from app import api_user, encryption, create_uuid from app.dao import ( templates_dao, services_dao, @@ -34,11 +32,7 @@ SMS_NOTIFICATION = 'sms' EMAIL_NOTIFICATION = 'email' -def create_notification_id(): - return str(uuid.uuid4()) - - -@notifications.route('/', methods=['GET']) +@notifications.route('/notifications/', methods=['GET']) def get_notifications(notification_id): try: notification = notifications_dao.get_notification(api_user['client'], notification_id) @@ -47,22 +41,22 @@ def get_notifications(notification_id): return jsonify(result="error", message="not found"), 404 -@notifications.route('/sms', methods=['POST']) +@notifications.route('/notifications/sms', methods=['POST']) def create_sms_notification(): return send_notification(notification_type=SMS_NOTIFICATION, expects_job=False) -@notifications.route('/sms/service/', methods=['POST']) +@notifications.route('/notifications/sms/service/', methods=['POST']) def create_sms_for_job(service_id): return send_notification(service_id=service_id, notification_type=SMS_NOTIFICATION, expects_job=True) -@notifications.route('/email', methods=['POST']) +@notifications.route('/notifications/email', methods=['POST']) def create_email_notification(): return send_notification(notification_type=EMAIL_NOTIFICATION, expects_job=False) -@notifications.route('/email/service/', methods=['POST']) +@notifications.route('/notifications/email/service/', methods=['POST']) def create_email_notification_for_job(service_id): return send_notification(service_id=service_id, notification_type=EMAIL_NOTIFICATION, expects_job=True) @@ -98,7 +92,7 @@ def send_notification(notification_type, service_id=None, expects_job=False): ), 400 if expects_job: - job = jobs_dao.get_job(service_id, notification['job']) + job = jobs_dao.dao_get_job_by_service_id_and_job_id(service_id, notification['job']) if not job: return jsonify(result="error", message={'job': ['Job {} not found'.format(notification['job'])]}), 400 @@ -115,7 +109,7 @@ def send_notification(notification_type, service_id=None, expects_job=False): return jsonify( result="error", message={'to': ['Email address not permitted for restricted service']}), 400 - notification_id = create_notification_id() + notification_id = create_uuid() if notification_type is SMS_NOTIFICATION: send_sms.apply_async(( diff --git a/scripts/run_celery.sh b/scripts/run_celery.sh index 023b54920..d86656a98 100755 --- a/scripts/run_celery.sh +++ b/scripts/run_celery.sh @@ -3,4 +3,4 @@ set -e source environment.sh -celery -A run_celery.notify_celery worker --loglevel=INFO --logfile=/var/log/notify/application.log --concurrency=4 -Q sms,sms-code,email-code,email +celery -A run_celery.notify_celery worker --loglevel=INFO --logfile=/var/log/notify/application.log --concurrency=4 -Q sms,sms-code,email-code,email,process-job diff --git a/test_csv_files/multiple_sms.csv b/test_csv_files/multiple_sms.csv new file mode 100644 index 000000000..3a459d07a --- /dev/null +++ b/test_csv_files/multiple_sms.csv @@ -0,0 +1,11 @@ +phone ++441234123121 ++441234123122 ++441234123123 ++441234123124 ++441234123125 ++441234123126 ++441234123127 ++441234123128 ++441234123129 ++441234123120 diff --git a/test_csv_files/sms.csv b/test_csv_files/sms.csv new file mode 100644 index 000000000..39e2eda10 --- /dev/null +++ b/test_csv_files/sms.csv @@ -0,0 +1,2 @@ +phone ++441234123123 \ No newline at end of file diff --git a/tests/app/__init__.py b/tests/app/__init__.py index e69de29bb..75631cf9a 100644 --- a/tests/app/__init__.py +++ b/tests/app/__init__.py @@ -0,0 +1,7 @@ +import os + + +def load_example_csv(file): + file_path = os.path.join("test_csv_files", "{}.csv".format(file)) + with open(file_path) as f: + return f.read() diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index a9207ca4e..f4b3b3013 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1,13 +1,49 @@ import uuid import pytest from flask import current_app -from app.celery.tasks import (send_sms, send_sms_code, send_email_code, send_email) +from app.celery.tasks import (send_sms, send_sms_code, send_email_code, send_email, process_job) from app import (firetext_client, aws_ses_client, encryption) from app.clients.email.aws_ses import AwsSesClientException from app.clients.sms.firetext import FiretextClientException -from app.dao import notifications_dao +from app.dao import notifications_dao, jobs_dao from sqlalchemy.exc import SQLAlchemyError from sqlalchemy.orm.exc import NoResultFound +from app.celery.tasks import s3 +from app.celery import tasks +from tests.app import load_example_csv + + +def test_should_process_sms_job(sample_job, mocker): + mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('sms')) + mocker.patch('app.celery.tasks.send_sms.apply_async') + mocker.patch('app.encryption.encrypt', return_value="something_encrypted") + mocker.patch('app.celery.tasks.create_uuid', return_value="uuid") + + process_job(sample_job.id) + + s3.get_job_from_s3.assert_called_once_with(sample_job.bucket_name, sample_job.id) + tasks.send_sms.apply_async.assert_called_once_with( + (str(sample_job.service_id), + "uuid", + "something_encrypted"), + queue="sms" + ) + job = jobs_dao.dao_get_job_by_id(sample_job.id) + assert job.status == 'finished' + + +def test_should_process_all_sms_job(sample_job, mocker): + mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('multiple_sms')) + mocker.patch('app.celery.tasks.send_sms.apply_async') + mocker.patch('app.encryption.encrypt', return_value="something_encrypted") + mocker.patch('app.celery.tasks.create_uuid', return_value="uuid") + + process_job(sample_job.id) + + s3.get_job_from_s3.assert_called_once_with(sample_job.bucket_name, sample_job.id) + tasks.send_sms.apply_async.call_count == 10 + job = jobs_dao.dao_get_job_by_id(sample_job.id) + assert job.status == 'finished' def test_should_send_template_to_correct_sms_provider_and_persist(sample_template, mocker): @@ -218,7 +254,9 @@ def test_should_send_email_code(mocker): send_email_code(encrypted_verification) - aws_ses_client.send_email.assert_called_once_with(current_app.config['VERIFY_CODE_FROM_EMAIL_ADDRESS'], - verification['to'], - "Verification code", - verification['secret_code']) + aws_ses_client.send_email.assert_called_once_with( + current_app.config['VERIFY_CODE_FROM_EMAIL_ADDRESS'], + verification['to'], + "Verification code", + verification['secret_code'] + ) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 64f634951..4b09250ff 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -6,7 +6,7 @@ from app.dao.users_dao import (save_model_user, create_user_code, create_secret_ from app.dao.services_dao import dao_create_service from app.dao.templates_dao import dao_create_template from app.dao.api_key_dao import save_model_api_key -from app.dao.jobs_dao import save_job +from app.dao.jobs_dao import dao_create_job from app.dao.notifications_dao import save_notification import uuid @@ -204,7 +204,7 @@ def sample_job(notify_db, 'notification_count': 1 } job = Job(**data) - save_job(job) + dao_create_job(job) return job diff --git a/tests/app/dao/test_jobs_dao.py b/tests/app/dao/test_jobs_dao.py index be9fa7b9d..0a80ad398 100644 --- a/tests/app/dao/test_jobs_dao.py +++ b/tests/app/dao/test_jobs_dao.py @@ -1,18 +1,16 @@ import uuid -import json from app.dao.jobs_dao import ( - save_job, - get_job, - get_jobs_by_service, - _get_jobs + dao_get_job_by_service_id_and_job_id, + dao_create_job, + dao_update_job, + dao_get_jobs_by_service_id ) from app.models import Job -def test_save_job(notify_db, notify_db_session, sample_template): - +def test_create_job(sample_template): assert Job.query.count() == 0 job_id = uuid.uuid4() @@ -29,39 +27,33 @@ def test_save_job(notify_db, notify_db_session, sample_template): } job = Job(**data) - save_job(job) + dao_create_job(job) assert Job.query.count() == 1 job_from_db = Job.query.get(job_id) assert job == job_from_db -def test_get_job_by_id(notify_db, notify_db_session, sample_job): - job_from_db = get_job(sample_job.service.id, sample_job.id) +def test_get_job_by_id(sample_job): + job_from_db = dao_get_job_by_service_id_and_job_id(sample_job.service.id, sample_job.id) assert sample_job == job_from_db def test_get_jobs_for_service(notify_db, notify_db_session, sample_template): - from tests.app.conftest import sample_job as create_job from tests.app.conftest import sample_service as create_service from tests.app.conftest import sample_template as create_template from tests.app.conftest import sample_user as create_user - one_job = create_job(notify_db, notify_db_session, sample_template.service, - sample_template) + one_job = create_job(notify_db, notify_db_session, sample_template.service, sample_template) - other_user = create_user(notify_db, notify_db_session, - email="test@digital.cabinet-office.gov.uk") - other_service = create_service(notify_db, notify_db_session, - user=other_user, service_name="other service") - other_template = create_template(notify_db, notify_db_session, - service=other_service) - other_job = create_job(notify_db, notify_db_session, service=other_service, - template=other_template) + other_user = create_user(notify_db, notify_db_session, email="test@digital.cabinet-office.gov.uk") + other_service = create_service(notify_db, notify_db_session, user=other_user, service_name="other service") + other_template = create_template(notify_db, notify_db_session, service=other_service) + other_job = create_job(notify_db, notify_db_session, service=other_service, template=other_template) - one_job_from_db = get_jobs_by_service(one_job.service_id) - other_job_from_db = get_jobs_by_service(other_job.service_id) + one_job_from_db = dao_get_jobs_by_service_id(one_job.service_id) + other_job_from_db = dao_get_jobs_by_service_id(other_job.service_id) assert len(one_job_from_db) == 1 assert one_job == one_job_from_db[0] @@ -72,31 +64,12 @@ def test_get_jobs_for_service(notify_db, notify_db_session, sample_template): assert one_job_from_db != other_job_from_db -def test_get_all_jobs(notify_db, notify_db_session, sample_template): - from tests.app.conftest import sample_job as create_job - for i in range(5): - create_job(notify_db, - notify_db_session, - sample_template.service, - sample_template) - jobs_from_db = _get_jobs() - assert len(jobs_from_db) == 5 - - -def test_update_job(notify_db, notify_db_session, sample_job): +def test_update_job(sample_job): assert sample_job.status == 'pending' - update_dict = { - 'id': sample_job.id, - 'service': sample_job.service.id, - 'template': sample_job.template.id, - 'bucket_name': sample_job.bucket_name, - 'file_name': sample_job.file_name, - 'original_file_name': sample_job.original_file_name, - 'status': 'in progress' - } + sample_job.status = 'in progress' - save_job(sample_job, update_dict=update_dict) + dao_update_job(sample_job) job_from_db = Job.query.get(sample_job.id) diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 384be5bff..42e74b111 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -8,7 +8,7 @@ from app.dao.notifications_dao import ( ) -def test_save_notification(notify_db, notify_db_session, sample_template, sample_job): +def test_save_notification(sample_template, sample_job): assert Notification.query.count() == 0 to = '+44709123456' diff --git a/tests/app/job/test_job_rest.py b/tests/app/job/test_job_rest.py deleted file mode 100644 index 66aa83ef1..000000000 --- a/tests/app/job/test_job_rest.py +++ /dev/null @@ -1,328 +0,0 @@ -import boto3 -import moto -import json -import uuid -from flask import url_for - -from tests import create_authorization_header -from tests.app.conftest import sample_job as create_job - - -def test_get_jobs(notify_api, notify_db, notify_db_session, sample_template): - _setup_jobs(notify_db, notify_db_session, sample_template) - - service_id = sample_template.service.id - - with notify_api.test_request_context(): - with notify_api.test_client() as client: - path = url_for('job.get_job_for_service', service_id=service_id) - auth_header = create_authorization_header(service_id=service_id, - path=path, - method='GET') - response = client.get(path, headers=[auth_header]) - assert response.status_code == 200 - resp_json = json.loads(response.get_data(as_text=True)) - assert len(resp_json['data']) == 5 - - -def test_get_job_with_invalid_id_returns400(notify_api, notify_db, - notify_db_session, - sample_template): - service_id = sample_template.service.id - with notify_api.test_request_context(): - with notify_api.test_client() as client: - path = url_for('job.get_job_for_service', job_id='invalid_id', service_id=service_id) - auth_header = create_authorization_header(service_id=sample_template.service.id, - path=path, - method='GET') - response = client.get(path, headers=[auth_header]) - assert response.status_code == 400 - resp_json = json.loads(response.get_data(as_text=True)) - assert resp_json == {'message': 'Invalid job id', - 'result': 'error'} - - -def test_get_job_with_unknown_id_returns404(notify_api, notify_db, - notify_db_session, - sample_template): - random_id = str(uuid.uuid4()) - service_id = sample_template.service.id - with notify_api.test_request_context(): - with notify_api.test_client() as client: - path = url_for('job.get_job_for_service', job_id=random_id, service_id=service_id) - auth_header = create_authorization_header(service_id=sample_template.service.id, - path=path, - method='GET') - response = client.get(path, headers=[auth_header]) - assert response.status_code == 404 - resp_json = json.loads(response.get_data(as_text=True)) - assert resp_json == {'message': 'Job not found', 'result': 'error'} - - -def test_get_job_by_id(notify_api, notify_db, notify_db_session, - sample_job): - job_id = str(sample_job.id) - service_id = sample_job.service.id - with notify_api.test_request_context(): - with notify_api.test_client() as client: - path = url_for('job.get_job_for_service', job_id=job_id, service_id=service_id) - auth_header = create_authorization_header(service_id=sample_job.service.id, - path=path, - method='GET') - response = client.get(path, headers=[auth_header]) - assert response.status_code == 200 - resp_json = json.loads(response.get_data(as_text=True)) - assert resp_json['data']['id'] == job_id - - -@moto.mock_sqs -def test_create_job(notify_api, notify_db, notify_db_session, sample_template): - job_id = uuid.uuid4() - template_id = sample_template.id - service_id = sample_template.service.id - original_file_name = 'thisisatest.csv' - bucket_name = 'service-{}-notify'.format(service_id) - file_name = '{}.csv'.format(job_id) - data = { - 'id': str(job_id), - 'service': str(service_id), - 'template': template_id, - 'original_file_name': original_file_name, - 'bucket_name': bucket_name, - 'file_name': file_name, - 'notification_count': 1 - } - - with notify_api.test_request_context(): - with notify_api.test_client() as client: - path = url_for('job.create_job', service_id=service_id) - auth_header = create_authorization_header(service_id=sample_template.service.id, - path=path, - method='POST', - request_body=json.dumps(data)) - headers = [('Content-Type', 'application/json'), auth_header] - response = client.post( - path, - data=json.dumps(data), - headers=headers) - assert response.status_code == 201 - - resp_json = json.loads(response.get_data(as_text=True)) - - assert resp_json['data']['id'] == str(job_id) - assert resp_json['data']['service'] == str(service_id) - assert resp_json['data']['template'] == template_id - assert resp_json['data']['original_file_name'] == original_file_name - - boto3.setup_default_session(region_name='eu-west-1') - q = boto3.resource('sqs').get_queue_by_name(QueueName=notify_api.config['NOTIFY_JOB_QUEUE']) - messages = q.receive_messages() - assert len(messages) == 1 - - expected_message = json.loads(messages[0].body) - assert expected_message['id'] == str(job_id) - assert expected_message['service'] == str(service_id) - assert expected_message['template'] == template_id - assert expected_message['bucket_name'] == bucket_name - - -def test_get_update_job_status(notify_api, - notify_db, - notify_db_session, - sample_job): - - assert sample_job.status == 'pending' - - job_id = str(sample_job.id) - service_id = str(sample_job.service.id) - - update_data = { - 'id': job_id, - 'service': service_id, - 'template': sample_job.template.id, - 'bucket_name': sample_job.bucket_name, - 'file_name': sample_job.file_name, - 'original_file_name': sample_job.original_file_name, - 'status': 'in progress', - 'notification_count': 1 - } - - with notify_api.test_request_context(): - with notify_api.test_client() as client: - path = url_for('job.update_job', service_id=service_id, job_id=job_id) - - auth_header = create_authorization_header(service_id=service_id, - path=path, - method='PUT', - request_body=json.dumps(update_data)) - - headers = [('Content-Type', 'application/json'), auth_header] - - response = client.put(path, headers=headers, data=json.dumps(update_data)) - - assert response.status_code == 200 - resp_json = json.loads(response.get_data(as_text=True)) - assert resp_json['data']['status'] == 'in progress' - - -def test_get_notification(notify_api, notify_db, notify_db_session, sample_notification): - - with notify_api.test_request_context(): - with notify_api.test_client() as client: - path = url_for('job.get_notification_for_job', - service_id=sample_notification.service.id, - job_id=sample_notification.job.id, - notification_id=sample_notification.id) - - auth_header = create_authorization_header(service_id=sample_notification.service.id, - path=path, - method='GET') - - headers = [('Content-Type', 'application/json'), auth_header] - response = client.get(path, headers=headers) - resp_json = json.loads(response.get_data(as_text=True)) - - assert str(sample_notification.id) == resp_json['data']['id'] - assert str(sample_notification.service.id) == resp_json['data']['service'] - assert sample_notification.template.id == resp_json['data']['template'] - assert str(sample_notification.job.id) == resp_json['data']['job'] - assert sample_notification.status == resp_json['data']['status'] - - -def test_get_notifications(notify_api, notify_db, notify_db_session, sample_job): - - from tests.app.conftest import sample_notification - for i in range(0, 5): - sample_notification(notify_db, - notify_db_session, - service=sample_job.service, - template=sample_job.template, - job=sample_job) - - service_id = str(sample_job.service.id) - job_id = str(sample_job.id) - - with notify_api.test_request_context(): - with notify_api.test_client() as client: - path = url_for('job.get_notification_for_job', - service_id=service_id, - job_id=job_id) - - auth_header = create_authorization_header(service_id=service_id, - path=path, - method='GET') - - headers = [('Content-Type', 'application/json'), auth_header] - response = client.get(path, headers=headers) - resp_json = json.loads(response.get_data(as_text=True)) - - assert len(resp_json['data']) == 5 - - -def test_add_notification(notify_api, notify_db, notify_db_session, sample_job): - - to = '+44709123456' - data = { - 'to': to, - 'job': str(sample_job.id), - 'service': str(sample_job.service.id), - 'template': sample_job.template.id - } - with notify_api.test_request_context(): - with notify_api.test_client() as client: - path = url_for('job.create_notification_for_job', - service_id=sample_job.service.id, - job_id=sample_job.id) - - auth_header = create_authorization_header(service_id=sample_job.service.id, - path=path, - method='POST', - request_body=json.dumps(data)) - - headers = [('Content-Type', 'application/json'), auth_header] - - response = client.post(path, headers=headers, data=json.dumps(data)) - - resp_json = json.loads(response.get_data(as_text=True)) - - assert resp_json['data']['id'] - assert data['to'] == resp_json['data']['to'] - assert data['service'] == resp_json['data']['service'] - assert data['template'] == resp_json['data']['template'] - assert data['job'] == resp_json['data']['job'] - assert 'sent' == resp_json['data']['status'] - - -def test_add_notification_with_id(notify_api, notify_db, notify_db_session, sample_job): - notification_id = str(uuid.uuid4()) - to = '+44709123456' - data = { - 'id': notification_id, - 'to': to, - 'job': str(sample_job.id), - 'service': str(sample_job.service.id), - 'template': sample_job.template.id - } - with notify_api.test_request_context(): - with notify_api.test_client() as client: - path = url_for('job.create_notification_for_job', - service_id=sample_job.service.id, - job_id=sample_job.id) - - auth_header = create_authorization_header(service_id=sample_job.service.id, - path=path, - method='POST', - request_body=json.dumps(data)) - - headers = [('Content-Type', 'application/json'), auth_header] - - response = client.post(path, headers=headers, data=json.dumps(data)) - - resp_json = json.loads(response.get_data(as_text=True)) - - assert resp_json['data']['id'] == notification_id - assert data['to'] == resp_json['data']['to'] - assert data['service'] == resp_json['data']['service'] - assert data['template'] == resp_json['data']['template'] - assert data['job'] == resp_json['data']['job'] - assert 'sent' == resp_json['data']['status'] - - -def test_update_notification(notify_api, notify_db, notify_db_session, sample_notification): - - assert sample_notification.status == 'sent' - - update_data = { - 'id': str(sample_notification.id), - 'to': sample_notification.to, - 'job': str(sample_notification.job.id), - 'service': str(sample_notification.service.id), - 'template': sample_notification.template.id, - 'status': 'failed' - } - with notify_api.test_request_context(): - with notify_api.test_client() as client: - path = url_for('job.update_notification_for_job', - service_id=sample_notification.service.id, - job_id=sample_notification.job.id, - notification_id=sample_notification.id) - - auth_header = create_authorization_header(service_id=sample_notification.service.id, - path=path, - method='PUT', - request_body=json.dumps(update_data)) - - headers = [('Content-Type', 'application/json'), auth_header] - - response = client.put(path, headers=headers, data=json.dumps(update_data)) - - resp_json = json.loads(response.get_data(as_text=True)) - - assert update_data['id'] == resp_json['data']['id'] - assert 'failed' == resp_json['data']['status'] - - -def _setup_jobs(notify_db, notify_db_session, template, number_of_jobs=5): - for i in range(number_of_jobs): - create_job(notify_db, notify_db_session, service=template.service, - template=template) diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py new file mode 100644 index 000000000..278366732 --- /dev/null +++ b/tests/app/job/test_rest.py @@ -0,0 +1,226 @@ +import json +import uuid +import app.celery.tasks + +from tests import create_authorization_header +from tests.app.conftest import sample_job as create_job + + +def test_get_jobs(notify_api, notify_db, notify_db_session, sample_template): + _setup_jobs(notify_db, notify_db_session, sample_template) + + service_id = sample_template.service.id + + with notify_api.test_request_context(): + with notify_api.test_client() as client: + path = '/service/{}/job'.format(service_id) + auth_header = create_authorization_header( + service_id=service_id, + path=path, + method='GET') + response = client.get(path, headers=[auth_header]) + assert response.status_code == 200 + resp_json = json.loads(response.get_data(as_text=True)) + assert len(resp_json['data']) == 5 + + +def test_get_job_with_invalid_service_id_returns404(notify_api, sample_api_key, sample_service): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + path = '/service/{}/job'.format(sample_service.id) + auth_header = create_authorization_header( + service_id=sample_service.id, + path=path, + method='GET') + response = client.get(path, headers=[auth_header]) + assert response.status_code == 200 + resp_json = json.loads(response.get_data(as_text=True)) + assert len(resp_json['data']) == 0 + + +def test_get_job_with_invalid_job_id_returns404(notify_api, sample_template): + service_id = sample_template.service.id + with notify_api.test_request_context(): + with notify_api.test_client() as client: + path = '/service/{}/job/{}'.format(service_id, "bad-id") + auth_header = create_authorization_header( + service_id=sample_template.service.id, + path=path, + method='GET') + response = client.get(path, headers=[auth_header]) + assert response.status_code == 404 + resp_json = json.loads(response.get_data(as_text=True)) + print(resp_json) + assert resp_json['result'] == 'error' + assert resp_json['message'] == 'No result found' + + +def test_get_job_with_unknown_id_returns404(notify_api, sample_template): + random_id = str(uuid.uuid4()) + service_id = sample_template.service.id + with notify_api.test_request_context(): + with notify_api.test_client() as client: + path = '/service/{}/job/{}'.format(service_id, random_id) + auth_header = create_authorization_header( + service_id=sample_template.service.id, + path=path, + method='GET') + response = client.get(path, headers=[auth_header]) + assert response.status_code == 404 + resp_json = json.loads(response.get_data(as_text=True)) + assert resp_json == { + 'message': 'Job {} not found for service {}'.format(random_id, service_id), + 'result': 'error' + } + + +def test_get_job_by_id(notify_api, sample_job): + job_id = str(sample_job.id) + service_id = sample_job.service.id + with notify_api.test_request_context(): + with notify_api.test_client() as client: + path = '/service/{}/job/{}'.format(service_id, job_id) + auth_header = create_authorization_header( + service_id=sample_job.service.id, + path=path, + method='GET') + response = client.get(path, headers=[auth_header]) + assert response.status_code == 200 + resp_json = json.loads(response.get_data(as_text=True)) + assert resp_json['data']['id'] == job_id + + +def test_create_job(notify_api, sample_template, mocker): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + mocker.patch('app.celery.tasks.process_job.apply_async') + job_id = uuid.uuid4() + data = { + 'id': str(job_id), + 'service': str(sample_template.service.id), + 'template': sample_template.id, + 'original_file_name': 'thisisatest.csv', + 'bucket_name': 'service-{}-notify'.format(sample_template.service.id), + 'file_name': '{}.csv'.format(job_id), + 'notification_count': 1 + } + path = '/service/{}/job'.format(sample_template.service.id) + auth_header = create_authorization_header( + service_id=sample_template.service.id, + path=path, + method='POST', + request_body=json.dumps(data)) + headers = [('Content-Type', 'application/json'), auth_header] + response = client.post( + path, + data=json.dumps(data), + headers=headers) + assert response.status_code == 201 + + app.celery.tasks.process_job.apply_async.assert_called_once_with( + ([str(job_id)]), + queue="process-job" + ) + + resp_json = json.loads(response.get_data(as_text=True)) + + assert resp_json['data']['id'] == str(job_id) + assert resp_json['data']['service'] == str(sample_template.service.id) + assert resp_json['data']['template'] == sample_template.id + assert resp_json['data']['original_file_name'] == 'thisisatest.csv' + + +def test_create_job_returns_400_if_missing_data(notify_api, sample_template, mocker): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + mocker.patch('app.celery.tasks.process_job.apply_async') + data = { + } + path = '/service/{}/job'.format(sample_template.service.id) + auth_header = create_authorization_header( + service_id=sample_template.service.id, + path=path, + method='POST', + request_body=json.dumps(data)) + headers = [('Content-Type', 'application/json'), auth_header] + response = client.post( + path, + data=json.dumps(data), + headers=headers) + + resp_json = json.loads(response.get_data(as_text=True)) + assert response.status_code == 400 + + app.celery.tasks.process_job.apply_async.assert_not_called() + assert resp_json['result'] == 'error' + assert 'Missing data for required field.' in resp_json['message']['original_file_name'] + assert 'Missing data for required field.' in resp_json['message']['file_name'] + assert 'Missing data for required field.' in resp_json['message']['notification_count'] + assert 'Missing data for required field.' in resp_json['message']['id'] + assert 'Missing data for required field.' in resp_json['message']['bucket_name'] + + +def test_create_job_returns_404_if_missing_service(notify_api, sample_template, mocker): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + mocker.patch('app.celery.tasks.process_job.apply_async') + random_id = str(uuid.uuid4()) + data = {} + path = '/service/{}/job'.format(random_id) + auth_header = create_authorization_header( + service_id=sample_template.service.id, + path=path, + method='POST', + request_body=json.dumps(data)) + headers = [('Content-Type', 'application/json'), auth_header] + response = client.post( + path, + data=json.dumps(data), + headers=headers) + + resp_json = json.loads(response.get_data(as_text=True)) + assert response.status_code == 404 + + app.celery.tasks.process_job.apply_async.assert_not_called() + print(resp_json) + assert resp_json['result'] == 'error' + assert resp_json['message'] == 'Service {} not found'.format(random_id) + + +def test_get_update_job(notify_api, sample_job): + assert sample_job.status == 'pending' + + job_id = str(sample_job.id) + service_id = str(sample_job.service.id) + + update_data = { + 'status': 'in progress' + } + + with notify_api.test_request_context(): + with notify_api.test_client() as client: + path = '/service/{}/job/{}'.format(service_id, job_id) + + auth_header = create_authorization_header( + service_id=service_id, + path=path, + method='POST', + request_body=json.dumps(update_data)) + + headers = [('Content-Type', 'application/json'), auth_header] + + response = client.post(path, headers=headers, data=json.dumps(update_data)) + + resp_json = json.loads(response.get_data(as_text=True)) + assert response.status_code == 200 + assert resp_json['data']['status'] == 'in progress' + + +def _setup_jobs(notify_db, notify_db_session, template, number_of_jobs=5): + for i in range(number_of_jobs): + print(i) + create_job( + notify_db, + notify_db_session, + service=template.service, + template=template) diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 1157800d5..504c7d0dc 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -904,6 +904,7 @@ def test_should_allow_valid_email_notification_for_job(notify_api, sample_job, s data=json.dumps(data), headers=[('Content-Type', 'application/json'), auth_header]) + print(json.loads(response.data)) notification_id = json.loads(response.data)['notification_id'] app.celery.tasks.send_email.apply_async.assert_called_once_with( (str(sample_job.service_id), diff --git a/tests/app/test_csv.py b/tests/app/test_csv.py new file mode 100644 index 000000000..50fa84122 --- /dev/null +++ b/tests/app/test_csv.py @@ -0,0 +1,15 @@ +from app.csv import get_mobile_numbers_from_csv +from tests.app import load_example_csv + + +def test_should_process_single_phone_number_file(): + sms_file = load_example_csv('sms') + len(get_mobile_numbers_from_csv(sms_file)) == 1 + assert get_mobile_numbers_from_csv(sms_file)[0] == '+441234123123' + + +def test_should_process_multple_phone_number_file_in_order(): + sms_file = load_example_csv('multiple_sms') + len(get_mobile_numbers_from_csv(sms_file)) == 10 + assert get_mobile_numbers_from_csv(sms_file)[0] == '+441234123121' + assert get_mobile_numbers_from_csv(sms_file)[9] == '+441234123120' From 10a764a2c140d03000b49181f63b952e7e47de22 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 25 Feb 2016 09:59:50 +0000 Subject: [PATCH 09/12] Renamed the header of the CSV to 'to' from 'number' to allow for email jobs - added new columns to Job and Notification to capture the start/end dates accurately --- app/celery/tasks.py | 57 ++++++++----- app/csv.py | 4 +- app/dao/notifications_dao.py | 16 ++-- app/models.py | 19 ++++- .../versions/0022_add_processing_dates.py | 26 ++++++ migrations/versions/0023_add_sender.py | 26 ++++++ scripts/run_celery.sh | 2 +- test_csv_files/email.csv | 2 + test_csv_files/empty.csv | 1 + test_csv_files/multiple_email.csv | 12 +++ test_csv_files/multiple_sms.csv | 2 +- test_csv_files/sms.csv | 2 +- tests/app/celery/test_tasks.py | 80 ++++++++++++++++--- tests/app/conftest.py | 38 ++++++++- tests/app/dao/test_notification_dao.py | 39 ++++----- tests/app/test_csv.py | 25 ++++-- 16 files changed, 271 insertions(+), 80 deletions(-) create mode 100644 migrations/versions/0022_add_processing_dates.py create mode 100644 migrations/versions/0023_add_sender.py create mode 100644 test_csv_files/email.csv create mode 100644 test_csv_files/empty.csv create mode 100644 test_csv_files/multiple_email.csv diff --git a/app/celery/tasks.py b/app/celery/tasks.py index ebfa2c4af..a7a47e087 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -3,13 +3,14 @@ from app import notify_celery, encryption, firetext_client, aws_ses_client from app.clients.email.aws_ses import AwsSesClientException from app.clients.sms.firetext import FiretextClientException from app.dao.templates_dao import dao_get_template_by_id -from app.dao.notifications_dao import save_notification +from app.dao.notifications_dao import dao_create_notification, dao_update_notification from app.dao.jobs_dao import dao_update_job, dao_get_job_by_id from app.models import Notification from flask import current_app from sqlalchemy.exc import SQLAlchemyError from app.aws import s3 -from app.csv import get_mobile_numbers_from_csv +from app.csv import get_recipient_from_csv +from datetime import datetime @notify_celery.task(name="process-job") @@ -19,28 +20,38 @@ def process_job(job_id): dao_update_job(job) file = s3.get_job_from_s3(job.bucket_name, job_id) - mobile_numbers = get_mobile_numbers_from_csv(file) + recipients = get_recipient_from_csv(file) - for mobile_number in mobile_numbers: - notification = encryption.encrypt({ + for recipient in recipients: + encrypted = encryption.encrypt({ 'template': job.template_id, 'job': str(job.id), - 'to': mobile_number + 'to': recipient }) - send_sms.apply_async(( - str(job.service_id), - str(create_uuid()), - notification), - queue='sms' - ) + if job.template.template_type == 'sms': + send_sms.apply_async(( + str(job.service_id), + str(create_uuid()), + encrypted), + queue='bulk-sms' + ) + + if job.template.template_type == 'email': + send_email.apply_async(( + str(job.service_id), + str(create_uuid()), + job.template.subject, + "{}@{}".format(job.service.email_from, current_app.config['NOTIFY_EMAIL_DOMAIN']), + encrypted), + queue='bulk-email') job.status = 'finished' dao_update_job(job) @notify_celery.task(name="send-sms") -def send_sms(service_id, notification_id, encrypted_notification): +def send_sms(service_id, notification_id, encrypted_notification, created_at): notification = encryption.decrypt(encrypted_notification) template = dao_get_template_by_id(notification['template']) @@ -51,22 +62,25 @@ def send_sms(service_id, notification_id, encrypted_notification): to=notification['to'], service_id=service_id, job_id=notification.get('job', None), - status='sent' + status='sent', + created_at=created_at, + sent_at=datetime.utcnow() ) - save_notification(notification_db_object) + dao_create_notification(notification_db_object) try: firetext_client.send_sms(notification['to'], template.content) except FiretextClientException as e: current_app.logger.debug(e) - save_notification(notification_db_object, {"status": "failed"}) + notification_db_object.status = 'failed' + dao_update_notification(notification_db_object) except SQLAlchemyError as e: current_app.logger.debug(e) @notify_celery.task(name="send-email") -def send_email(service_id, notification_id, subject, from_address, encrypted_notification): +def send_email(service_id, notification_id, subject, from_address, encrypted_notification, created_at): notification = encryption.decrypt(encrypted_notification) template = dao_get_template_by_id(notification['template']) @@ -77,9 +91,11 @@ def send_email(service_id, notification_id, subject, from_address, encrypted_not to=notification['to'], service_id=service_id, job_id=notification.get('job', None), - status='sent' + status='sent', + created_at=created_at, + sent_at=datetime.utcnow() ) - save_notification(notification_db_object) + dao_create_notification(notification_db_object) try: aws_ses_client.send_email( @@ -90,7 +106,8 @@ def send_email(service_id, notification_id, subject, from_address, encrypted_not ) except AwsSesClientException as e: current_app.logger.debug(e) - save_notification(notification_db_object, {"status": "failed"}) + notification_db_object.status = 'failed' + dao_update_notification(notification_db_object) except SQLAlchemyError as e: current_app.logger.debug(e) diff --git a/app/csv.py b/app/csv.py index 371203a54..db0199a48 100644 --- a/app/csv.py +++ b/app/csv.py @@ -1,12 +1,12 @@ import csv -def get_mobile_numbers_from_csv(file_data): +def get_recipient_from_csv(file_data): numbers = [] reader = csv.DictReader( file_data.splitlines(), lineterminator='\n', quoting=csv.QUOTE_NONE) for i, row in enumerate(reader): - numbers.append(row['phone'].replace(' ', '')) + numbers.append(row['to'].replace(' ', '')) return numbers diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index f917f4429..f92ac0393 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -2,15 +2,13 @@ from app import db from app.models import Notification -def save_notification(notification, update_dict={}): - if update_dict: - update_dict.pop('id', None) - update_dict.pop('job', None) - update_dict.pop('service', None) - update_dict.pop('template', None) - Notification.query.filter_by(id=notification.id).update(update_dict) - else: - db.session.add(notification) +def dao_create_notification(notification): + db.session.add(notification) + db.session.commit() + + +def dao_update_notification(notification): + db.session.add(notification) db.session.commit() diff --git a/app/models.py b/app/models.py index 84726164a..14da343d5 100644 --- a/app/models.py +++ b/app/models.py @@ -160,6 +160,16 @@ class Job(db.Model): onupdate=datetime.datetime.now) status = db.Column(db.Enum(*JOB_STATUS_TYPES, name='job_status_types'), nullable=False, default='pending') notification_count = db.Column(db.Integer, nullable=False) + processing_started = db.Column( + db.DateTime, + index=False, + unique=False, + nullable=True) + processing_finished = db.Column( + db.DateTime, + index=False, + unique=False, + nullable=True) VERIFY_CODE_TYPES = ['email', 'sms'] @@ -214,8 +224,13 @@ class Notification(db.Model): db.DateTime, index=False, unique=False, - nullable=False, - default=datetime.datetime.now) + nullable=False) + sent_at = db.Column( + db.DateTime, + index=False, + unique=False, + nullable=True) + sent_by = db.Column(db.String, nullable=True) updated_at = db.Column( db.DateTime, index=False, diff --git a/migrations/versions/0022_add_processing_dates.py b/migrations/versions/0022_add_processing_dates.py new file mode 100644 index 000000000..89f2d4d71 --- /dev/null +++ b/migrations/versions/0022_add_processing_dates.py @@ -0,0 +1,26 @@ +"""empty message + +Revision ID: 0022_add_processing_dates +Revises: 0021_add_job_metadata +Create Date: 2016-02-24 17:15:47.457200 + +""" + +# revision identifiers, used by Alembic. +revision = '0022_add_processing_dates' +down_revision = '0021_add_job_metadata' + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + op.add_column('jobs', sa.Column('processing_finished', sa.DateTime(), nullable=True)) + op.add_column('jobs', sa.Column('processing_started', sa.DateTime(), nullable=True)) + op.add_column('notifications', sa.Column('sent_at', sa.DateTime(), nullable=True)) + + +def downgrade(): + op.drop_column('notifications', 'sent_at') + op.drop_column('jobs', 'processing_started') + op.drop_column('jobs', 'processing_finished') diff --git a/migrations/versions/0023_add_sender.py b/migrations/versions/0023_add_sender.py new file mode 100644 index 000000000..fbc41aa99 --- /dev/null +++ b/migrations/versions/0023_add_sender.py @@ -0,0 +1,26 @@ +"""empty message + +Revision ID: d98c7c20f1d4 +Revises: 0022_add_processing_dates +Create Date: 2016-02-24 17:18:21.942772 + +""" + +# revision identifiers, used by Alembic. +revision = 'd98c7c20f1d4' +down_revision = '0022_add_processing_dates' + +from alembic import op +import sqlalchemy as sa + + +def upgrade(): + ### commands auto generated by Alembic - please adjust! ### + op.add_column('notifications', sa.Column('sent_by', sa.String(), nullable=True)) + ### end Alembic commands ### + + +def downgrade(): + ### commands auto generated by Alembic - please adjust! ### + op.drop_column('notifications', 'sent_by') + ### end Alembic commands ### diff --git a/scripts/run_celery.sh b/scripts/run_celery.sh index d86656a98..eb0e1805b 100755 --- a/scripts/run_celery.sh +++ b/scripts/run_celery.sh @@ -3,4 +3,4 @@ set -e source environment.sh -celery -A run_celery.notify_celery worker --loglevel=INFO --logfile=/var/log/notify/application.log --concurrency=4 -Q sms,sms-code,email-code,email,process-job +celery -A run_celery.notify_celery worker --loglevel=INFO --logfile=/var/log/notify/application.log --concurrency=4 -Q sms,sms-code,email-code,email,process-job,bulk-sms,bulk-email diff --git a/test_csv_files/email.csv b/test_csv_files/email.csv new file mode 100644 index 000000000..581e64627 --- /dev/null +++ b/test_csv_files/email.csv @@ -0,0 +1,2 @@ +to +test@test.com \ No newline at end of file diff --git a/test_csv_files/empty.csv b/test_csv_files/empty.csv new file mode 100644 index 000000000..bf3efa02f --- /dev/null +++ b/test_csv_files/empty.csv @@ -0,0 +1 @@ +to diff --git a/test_csv_files/multiple_email.csv b/test_csv_files/multiple_email.csv new file mode 100644 index 000000000..2e7bab603 --- /dev/null +++ b/test_csv_files/multiple_email.csv @@ -0,0 +1,12 @@ +to +test1@test.com +test2@test.com +test3@test.com +test4@test.com +test5@test.com +test6@test.com +test7@test.com +test8@test.com +test9@test.com +test0@test.com + diff --git a/test_csv_files/multiple_sms.csv b/test_csv_files/multiple_sms.csv index 3a459d07a..224a1a4d7 100644 --- a/test_csv_files/multiple_sms.csv +++ b/test_csv_files/multiple_sms.csv @@ -1,4 +1,4 @@ -phone +to +441234123121 +441234123122 +441234123123 diff --git a/test_csv_files/sms.csv b/test_csv_files/sms.csv index 39e2eda10..3776c8832 100644 --- a/test_csv_files/sms.csv +++ b/test_csv_files/sms.csv @@ -1,2 +1,2 @@ -phone +to +441234123123 \ No newline at end of file diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index f4b3b3013..c0abb4c06 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -11,6 +11,7 @@ from sqlalchemy.orm.exc import NoResultFound from app.celery.tasks import s3 from app.celery import tasks from tests.app import load_example_csv +from datetime import datetime def test_should_process_sms_job(sample_job, mocker): @@ -26,12 +27,45 @@ def test_should_process_sms_job(sample_job, mocker): (str(sample_job.service_id), "uuid", "something_encrypted"), - queue="sms" + queue="bulk-sms" ) job = jobs_dao.dao_get_job_by_id(sample_job.id) assert job.status == 'finished' +def test_should_not_create_send_task_for_empty_file(sample_job, mocker): + mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('empty')) + mocker.patch('app.celery.tasks.send_sms.apply_async') + + process_job(sample_job.id) + + s3.get_job_from_s3.assert_called_once_with(sample_job.bucket_name, sample_job.id) + job = jobs_dao.dao_get_job_by_id(sample_job.id) + assert job.status == 'finished' + tasks.send_sms.apply_async.assert_not_called + + +def test_should_process_email_job(sample_email_job, mocker): + mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('email')) + mocker.patch('app.celery.tasks.send_email.apply_async') + mocker.patch('app.encryption.encrypt', return_value="something_encrypted") + mocker.patch('app.celery.tasks.create_uuid', return_value="uuid") + + process_job(sample_email_job.id) + + s3.get_job_from_s3.assert_called_once_with(sample_email_job.bucket_name, sample_email_job.id) + tasks.send_email.apply_async.assert_called_once_with( + (str(sample_email_job.service_id), + "uuid", + sample_email_job.template.subject, + "{}@{}".format(sample_email_job.service.email_from, "test.notify.com"), + "something_encrypted"), + queue="bulk-email" + ) + job = jobs_dao.dao_get_job_by_id(sample_email_job.id) + assert job.status == 'finished' + + def test_should_process_all_sms_job(sample_job, mocker): mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('multiple_sms')) mocker.patch('app.celery.tasks.send_sms.apply_async') @@ -55,11 +89,13 @@ def test_should_send_template_to_correct_sms_provider_and_persist(sample_templat mocker.patch('app.firetext_client.send_sms') notification_id = uuid.uuid4() - + now = datetime.utcnow() send_sms( sample_template.service_id, notification_id, - "encrypted-in-reality") + "encrypted-in-reality", + now + ) firetext_client.send_sms.assert_called_once_with("+441234123123", sample_template.content) persisted_notification = notifications_dao.get_notification(sample_template.service_id, notification_id) @@ -67,6 +103,8 @@ def test_should_send_template_to_correct_sms_provider_and_persist(sample_templat assert persisted_notification.to == '+441234123123' assert persisted_notification.template_id == sample_template.id assert persisted_notification.status == 'sent' + assert persisted_notification.created_at == now + assert persisted_notification.sent_at > now assert not persisted_notification.job_id @@ -80,11 +118,12 @@ def test_should_send_template_to_correct_sms_provider_and_persist_with_job_id(sa mocker.patch('app.firetext_client.send_sms') notification_id = uuid.uuid4() - + now = datetime.utcnow() send_sms( sample_job.service.id, notification_id, - "encrypted-in-reality") + "encrypted-in-reality", + now) firetext_client.send_sms.assert_called_once_with("+441234123123", sample_job.template.content) persisted_notification = notifications_dao.get_notification(sample_job.template.service_id, notification_id) @@ -93,9 +132,11 @@ def test_should_send_template_to_correct_sms_provider_and_persist_with_job_id(sa assert persisted_notification.job_id == sample_job.id assert persisted_notification.template_id == sample_job.template.id assert persisted_notification.status == 'sent' + assert persisted_notification.sent_at > now + assert persisted_notification.created_at == now -def test_should_send_template_to_email_provider_and_persist(sample_email_template, mocker): +def test_should_use_email_template_and_persist(sample_email_template, mocker): notification = { "template": sample_email_template.id, "to": "my_email@my_email.com" @@ -104,13 +145,14 @@ def test_should_send_template_to_email_provider_and_persist(sample_email_templat mocker.patch('app.aws_ses_client.send_email') notification_id = uuid.uuid4() - + now = datetime.utcnow() send_email( sample_email_template.service_id, notification_id, 'subject', 'email_from', - "encrypted-in-reality") + "encrypted-in-reality", + now) aws_ses_client.send_email.assert_called_once_with( "email_from", @@ -122,6 +164,8 @@ def test_should_send_template_to_email_provider_and_persist(sample_email_templat assert persisted_notification.id == notification_id assert persisted_notification.to == 'my_email@my_email.com' assert persisted_notification.template_id == sample_email_template.id + assert persisted_notification.created_at == now + assert persisted_notification.sent_at > now assert persisted_notification.status == 'sent' @@ -132,13 +176,15 @@ def test_should_persist_notification_as_failed_if_sms_client_fails(sample_templa } mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.firetext_client.send_sms', side_effect=FiretextClientException()) + now = datetime.utcnow() notification_id = uuid.uuid4() send_sms( sample_template.service_id, notification_id, - "encrypted-in-reality") + "encrypted-in-reality", + now) firetext_client.send_sms.assert_called_once_with("+441234123123", sample_template.content) persisted_notification = notifications_dao.get_notification(sample_template.service_id, notification_id) @@ -146,6 +192,8 @@ def test_should_persist_notification_as_failed_if_sms_client_fails(sample_templa assert persisted_notification.to == '+441234123123' assert persisted_notification.template_id == sample_template.id assert persisted_notification.status == 'failed' + assert persisted_notification.created_at == now + assert persisted_notification.sent_at > now def test_should_persist_notification_as_failed_if_email_client_fails(sample_email_template, mocker): @@ -155,6 +203,7 @@ def test_should_persist_notification_as_failed_if_email_client_fails(sample_emai } mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.aws_ses_client.send_email', side_effect=AwsSesClientException()) + now = datetime.utcnow() notification_id = uuid.uuid4() @@ -163,7 +212,8 @@ def test_should_persist_notification_as_failed_if_email_client_fails(sample_emai notification_id, 'subject', 'email_from', - "encrypted-in-reality") + "encrypted-in-reality", + now) aws_ses_client.send_email.assert_called_once_with( "email_from", @@ -176,6 +226,8 @@ def test_should_persist_notification_as_failed_if_email_client_fails(sample_emai assert persisted_notification.to == 'my_email@my_email.com' assert persisted_notification.template_id == sample_email_template.id assert persisted_notification.status == 'failed' + assert persisted_notification.created_at == now + assert persisted_notification.sent_at > now def test_should_not_send_sms_if_db_peristance_failed(sample_template, mocker): @@ -186,13 +238,15 @@ def test_should_not_send_sms_if_db_peristance_failed(sample_template, mocker): mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.firetext_client.send_sms') mocker.patch('app.db.session.add', side_effect=SQLAlchemyError()) + now = datetime.utcnow() notification_id = uuid.uuid4() send_sms( sample_template.service_id, notification_id, - "encrypted-in-reality") + "encrypted-in-reality", + now) firetext_client.send_sms.assert_not_called() with pytest.raises(NoResultFound) as e: @@ -208,6 +262,7 @@ def test_should_not_send_email_if_db_peristance_failed(sample_email_template, mo mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.aws_ses_client.send_email') mocker.patch('app.db.session.add', side_effect=SQLAlchemyError()) + now = datetime.utcnow() notification_id = uuid.uuid4() @@ -216,7 +271,8 @@ def test_should_not_send_email_if_db_peristance_failed(sample_email_template, mo notification_id, 'subject', 'email_from', - "encrypted-in-reality") + "encrypted-in-reality", + now) aws_ses_client.send_email.assert_not_called() with pytest.raises(NoResultFound) as e: diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 4b09250ff..1b9e1df9e 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -1,5 +1,5 @@ import pytest - +from datetime import datetime from app import email_safe from app.models import (User, Service, Template, ApiKey, Job, Notification) from app.dao.users_dao import (save_model_user, create_user_code, create_secret_code) @@ -7,7 +7,7 @@ from app.dao.services_dao import dao_create_service from app.dao.templates_dao import dao_create_template from app.dao.api_key_dao import save_model_api_key from app.dao.jobs_dao import dao_create_job -from app.dao.notifications_dao import save_notification +from app.dao.notifications_dao import dao_create_notification import uuid @@ -208,6 +208,35 @@ def sample_job(notify_db, return job +@pytest.fixture(scope='function') +def sample_email_job(notify_db, + notify_db_session, + service=None, + template=None): + if service is None: + service = sample_service(notify_db, notify_db_session) + if template is None: + template = sample_email_template( + notify_db, + notify_db_session, + service=service) + job_id = uuid.uuid4() + bucket_name = 'service-{}-notify'.format(service.id) + file_name = '{}.csv'.format(job_id) + data = { + 'id': uuid.uuid4(), + 'service_id': service.id, + 'template_id': template.id, + 'bucket_name': bucket_name, + 'file_name': file_name, + 'original_file_name': 'some.csv', + 'notification_count': 1 + } + job = Job(**data) + dao_create_job(job) + return job + + @pytest.fixture(scope='function') def sample_admin_service_id(notify_db, notify_db_session): admin_user = sample_user(notify_db, notify_db_session, email="notify_admin@digital.cabinet-office.gov.uk") @@ -248,10 +277,11 @@ def sample_notification(notify_db, 'to': to, 'job': job, 'service': service, - 'template': template + 'template': template, + 'created_at': datetime.utcnow() } notification = Notification(**data) - save_notification(notification) + dao_create_notification(notification) return notification diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 42e74b111..9ba17c727 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -1,7 +1,8 @@ from app.models import Notification - +from datetime import datetime from app.dao.notifications_dao import ( - save_notification, + dao_create_notification, + dao_update_notification, get_notification, get_notification_for_job, get_notifications_for_job @@ -11,16 +12,16 @@ from app.dao.notifications_dao import ( def test_save_notification(sample_template, sample_job): assert Notification.query.count() == 0 - to = '+44709123456' data = { - 'to': to, + 'to': '+44709123456', 'job': sample_job, 'service': sample_template.service, - 'template': sample_template + 'template': sample_template, + 'created_at': datetime.utcnow() } notification = Notification(**data) - save_notification(notification) + dao_create_notification(notification) assert Notification.query.count() == 1 notification_from_db = Notification.query.all()[0] @@ -29,28 +30,30 @@ def test_save_notification(sample_template, sample_job): assert data['job'] == notification_from_db.job assert data['service'] == notification_from_db.service assert data['template'] == notification_from_db.template + assert data['created_at'] == notification_from_db.created_at assert 'sent' == notification_from_db.status -def test_get_notification(notify_db, notify_db_session, sample_notification): +def test_get_notification(sample_notification): notifcation_from_db = get_notification( sample_notification.service.id, sample_notification.id) assert sample_notification == notifcation_from_db -def test_save_notification_no_job_id(notify_db, notify_db_session, sample_template): +def test_save_notification_no_job_id(sample_template): assert Notification.query.count() == 0 to = '+44709123456' data = { 'to': to, 'service': sample_template.service, - 'template': sample_template + 'template': sample_template, + 'created_at': datetime.utcnow() } notification = Notification(**data) - save_notification(notification) + dao_create_notification(notification) assert Notification.query.count() == 1 notification_from_db = Notification.query.all()[0] @@ -61,7 +64,7 @@ def test_save_notification_no_job_id(notify_db, notify_db_session, sample_templa assert 'sent' == notification_from_db.status -def test_get_notification_for_job(notify_db, notify_db_session, sample_notification): +def test_get_notification_for_job(sample_notification): notifcation_from_db = get_notification_for_job( sample_notification.service.id, sample_notification.job_id, @@ -83,17 +86,9 @@ def test_get_all_notifications_for_job(notify_db, notify_db_session, sample_job) assert len(notifcations_from_db) == 5 -def test_update_notification(notify_db, notify_db_session, sample_notification): +def test_update_notification(sample_notification): assert sample_notification.status == 'sent' - - update_dict = { - 'id': str(sample_notification.id), - 'service': str(sample_notification.service.id), - 'template': sample_notification.template.id, - 'job': str(sample_notification.job.id), - 'status': 'failed' - } - - save_notification(sample_notification, update_dict=update_dict) + sample_notification.status = 'failed' + dao_update_notification(sample_notification) notification_from_db = Notification.query.get(sample_notification.id) assert notification_from_db.status == 'failed' diff --git a/tests/app/test_csv.py b/tests/app/test_csv.py index 50fa84122..df8fdcb70 100644 --- a/tests/app/test_csv.py +++ b/tests/app/test_csv.py @@ -1,15 +1,28 @@ -from app.csv import get_mobile_numbers_from_csv +from app.csv import get_recipient_from_csv from tests.app import load_example_csv def test_should_process_single_phone_number_file(): sms_file = load_example_csv('sms') - len(get_mobile_numbers_from_csv(sms_file)) == 1 - assert get_mobile_numbers_from_csv(sms_file)[0] == '+441234123123' + len(get_recipient_from_csv(sms_file)) == 1 + assert get_recipient_from_csv(sms_file)[0] == '+441234123123' def test_should_process_multple_phone_number_file_in_order(): sms_file = load_example_csv('multiple_sms') - len(get_mobile_numbers_from_csv(sms_file)) == 10 - assert get_mobile_numbers_from_csv(sms_file)[0] == '+441234123121' - assert get_mobile_numbers_from_csv(sms_file)[9] == '+441234123120' + len(get_recipient_from_csv(sms_file)) == 10 + assert get_recipient_from_csv(sms_file)[0] == '+441234123121' + assert get_recipient_from_csv(sms_file)[9] == '+441234123120' + + +def test_should_process_single_email_file(): + sms_file = load_example_csv('email') + len(get_recipient_from_csv(sms_file)) == 1 + assert get_recipient_from_csv(sms_file)[0] == 'test@test.com' + + +def test_should_process_multple_email_file_in_order(): + sms_file = load_example_csv('multiple_email') + len(get_recipient_from_csv(sms_file)) == 10 + assert get_recipient_from_csv(sms_file)[0] == 'test1@test.com' + assert get_recipient_from_csv(sms_file)[9] == 'test0@test.com' From 44632c36d343e0967243029d58676126db3670a3 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 25 Feb 2016 11:23:04 +0000 Subject: [PATCH 10/12] Add sender name to the notification - also ensure that the created time is handled properly --- app/celery/tasks.py | 36 +++++++++++++++++++++++----- app/clients/email/__init__.py | 3 +++ app/clients/email/aws_ses.py | 4 ++++ app/clients/sms/__init__.py | 3 +++ app/clients/sms/firetext.py | 4 ++++ app/clients/sms/twilio.py | 4 ++++ app/notifications/rest.py | 8 +++++-- requirements_for_test.txt | 3 ++- tests/app/celery/test_tasks.py | 20 ++++++++++++++-- tests/app/notifications/test_rest.py | 18 ++++++++++---- 10 files changed, 87 insertions(+), 16 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index a7a47e087..d4913e24d 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -15,6 +15,7 @@ from datetime import datetime @notify_celery.task(name="process-job") def process_job(job_id): + start = datetime.utcnow() job = dao_get_job_by_id(job_id) job.status = 'in progress' dao_update_job(job) @@ -33,7 +34,8 @@ def process_job(job_id): send_sms.apply_async(( str(job.service_id), str(create_uuid()), - encrypted), + encrypted, + str(datetime.utcnow())), queue='bulk-sms' ) @@ -43,11 +45,18 @@ def process_job(job_id): str(create_uuid()), job.template.subject, "{}@{}".format(job.service.email_from, current_app.config['NOTIFY_EMAIL_DOMAIN']), - encrypted), + encrypted, + str(datetime.utcnow())), queue='bulk-email') + finished = datetime.utcnow() job.status = 'finished' + job.processing_started = start + job.processing_finished = finished dao_update_job(job) + current_app.logger.info( + "Job {} created at {} started at {} finished at {}".format(job_id, job.created_at, start, finished) + ) @notify_celery.task(name="send-sms") @@ -55,7 +64,10 @@ def send_sms(service_id, notification_id, encrypted_notification, created_at): notification = encryption.decrypt(encrypted_notification) template = dao_get_template_by_id(notification['template']) + client = firetext_client + try: + sent_at = datetime.utcnow() notification_db_object = Notification( id=notification_id, template_id=notification['template'], @@ -64,17 +76,22 @@ def send_sms(service_id, notification_id, encrypted_notification, created_at): job_id=notification.get('job', None), status='sent', created_at=created_at, - sent_at=datetime.utcnow() + sent_at=sent_at, + sent_by=client.get_name() + ) dao_create_notification(notification_db_object) try: - firetext_client.send_sms(notification['to'], template.content) + client.send_sms(notification['to'], template.content) except FiretextClientException as e: current_app.logger.debug(e) notification_db_object.status = 'failed' dao_update_notification(notification_db_object) + current_app.logger.info( + "SMS {} created at {} sent at {}".format(notification_id, created_at, sent_at) + ) except SQLAlchemyError as e: current_app.logger.debug(e) @@ -84,7 +101,10 @@ def send_email(service_id, notification_id, subject, from_address, encrypted_not notification = encryption.decrypt(encrypted_notification) template = dao_get_template_by_id(notification['template']) + client = aws_ses_client + try: + sent_at = datetime.utcnow() notification_db_object = Notification( id=notification_id, template_id=notification['template'], @@ -93,12 +113,13 @@ def send_email(service_id, notification_id, subject, from_address, encrypted_not job_id=notification.get('job', None), status='sent', created_at=created_at, - sent_at=datetime.utcnow() + sent_at=sent_at, + sent_by=client.get_name() ) dao_create_notification(notification_db_object) try: - aws_ses_client.send_email( + client.send_email( from_address, notification['to'], subject, @@ -109,6 +130,9 @@ def send_email(service_id, notification_id, subject, from_address, encrypted_not notification_db_object.status = 'failed' dao_update_notification(notification_db_object) + current_app.logger.info( + "Email {} created at {} sent at {}".format(notification_id, created_at, sent_at) + ) except SQLAlchemyError as e: current_app.logger.debug(e) diff --git a/app/clients/email/__init__.py b/app/clients/email/__init__.py index 98f0cb901..15f250496 100644 --- a/app/clients/email/__init__.py +++ b/app/clients/email/__init__.py @@ -15,3 +15,6 @@ class EmailClient(Client): def send_email(self, *args, **kwargs): raise NotImplemented('TODO Need to implement.') + + def get_name(self): + raise NotImplemented('TODO Need to implement.') diff --git a/app/clients/email/aws_ses.py b/app/clients/email/aws_ses.py index 133368d84..c1f355b43 100644 --- a/app/clients/email/aws_ses.py +++ b/app/clients/email/aws_ses.py @@ -15,6 +15,10 @@ class AwsSesClient(EmailClient): def init_app(self, region, *args, **kwargs): self._client = boto3.client('ses', region_name=region) super(AwsSesClient, self).__init__(*args, **kwargs) + self.name = 'ses' + + def get_name(self): + return self.name def send_email(self, source, diff --git a/app/clients/sms/__init__.py b/app/clients/sms/__init__.py index 8b83ab8b9..90d63ed0d 100644 --- a/app/clients/sms/__init__.py +++ b/app/clients/sms/__init__.py @@ -15,3 +15,6 @@ class SmsClient(Client): def send_sms(self, *args, **kwargs): raise NotImplemented('TODO Need to implement.') + + def get_name(self): + raise NotImplemented('TODO Need to implement.') diff --git a/app/clients/sms/firetext.py b/app/clients/sms/firetext.py index 4a70947a1..235880e83 100644 --- a/app/clients/sms/firetext.py +++ b/app/clients/sms/firetext.py @@ -21,6 +21,10 @@ class FiretextClient(SmsClient): super(SmsClient, self).__init__(*args, **kwargs) self.api_key = config.config.get('FIRETEXT_API_KEY') self.from_number = config.config.get('FIRETEXT_NUMBER') + self.name = 'firetext' + + def get_name(self): + return self.name def send_sms(self, to, content): diff --git a/app/clients/sms/twilio.py b/app/clients/sms/twilio.py index eb5613878..ca33960fb 100644 --- a/app/clients/sms/twilio.py +++ b/app/clients/sms/twilio.py @@ -22,6 +22,10 @@ class TwilioClient(SmsClient): config.config.get('TWILIO_ACCOUNT_SID'), config.config.get('TWILIO_AUTH_TOKEN')) self.from_number = config.config.get('TWILIO_NUMBER') + self.name = 'twilio' + + def get_name(self): + return self.name def send_sms(self, to, content): try: diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 035a10a7e..137305eb9 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -1,3 +1,5 @@ +from datetime import datetime + from flask import ( Blueprint, jsonify, @@ -115,7 +117,8 @@ def send_notification(notification_type, service_id=None, expects_job=False): send_sms.apply_async(( service_id, notification_id, - encryption.encrypt(notification)), + encryption.encrypt(notification), + str(datetime.utcnow())), queue='sms') else: send_email.apply_async(( @@ -123,6 +126,7 @@ def send_notification(notification_type, service_id=None, expects_job=False): notification_id, template.subject, "{}@{}".format(service.email_from, current_app.config['NOTIFY_EMAIL_DOMAIN']), - encryption.encrypt(notification)), + encryption.encrypt(notification), + str(datetime.utcnow())), queue='email') return jsonify({'notification_id': notification_id}), 201 diff --git a/requirements_for_test.txt b/requirements_for_test.txt index 60ceea35b..dbbcc7ae3 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -4,4 +4,5 @@ pytest==2.8.1 pytest-mock==0.8.1 pytest-cov==2.2.0 mock==1.0.1 -moto==0.4.19 \ No newline at end of file +moto==0.4.19 +freezegun==0.3.6 diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index c0abb4c06..0c25826d5 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -12,8 +12,10 @@ from app.celery.tasks import s3 from app.celery import tasks from tests.app import load_example_csv from datetime import datetime +from freezegun import freeze_time +@freeze_time("2016-01-01 11:09:00.061258") def test_should_process_sms_job(sample_job, mocker): mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('sms')) mocker.patch('app.celery.tasks.send_sms.apply_async') @@ -26,7 +28,8 @@ def test_should_process_sms_job(sample_job, mocker): tasks.send_sms.apply_async.assert_called_once_with( (str(sample_job.service_id), "uuid", - "something_encrypted"), + "something_encrypted", + "2016-01-01 11:09:00.061258"), queue="bulk-sms" ) job = jobs_dao.dao_get_job_by_id(sample_job.id) @@ -45,6 +48,7 @@ def test_should_not_create_send_task_for_empty_file(sample_job, mocker): tasks.send_sms.apply_async.assert_not_called +@freeze_time("2016-01-01 11:09:00.061258") def test_should_process_email_job(sample_email_job, mocker): mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('email')) mocker.patch('app.celery.tasks.send_email.apply_async') @@ -59,7 +63,8 @@ def test_should_process_email_job(sample_email_job, mocker): "uuid", sample_email_job.template.subject, "{}@{}".format(sample_email_job.service.email_from, "test.notify.com"), - "something_encrypted"), + "something_encrypted", + "2016-01-01 11:09:00.061258"), queue="bulk-email" ) job = jobs_dao.dao_get_job_by_id(sample_email_job.id) @@ -87,6 +92,7 @@ def test_should_send_template_to_correct_sms_provider_and_persist(sample_templat } mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.firetext_client.send_sms') + mocker.patch('app.firetext_client.get_name', return_value="firetext") notification_id = uuid.uuid4() now = datetime.utcnow() @@ -105,6 +111,7 @@ def test_should_send_template_to_correct_sms_provider_and_persist(sample_templat assert persisted_notification.status == 'sent' assert persisted_notification.created_at == now assert persisted_notification.sent_at > now + assert persisted_notification.sent_by == 'firetext' assert not persisted_notification.job_id @@ -116,6 +123,7 @@ def test_should_send_template_to_correct_sms_provider_and_persist_with_job_id(sa } mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.firetext_client.send_sms') + mocker.patch('app.firetext_client.get_name', return_value="firetext") notification_id = uuid.uuid4() now = datetime.utcnow() @@ -134,6 +142,7 @@ def test_should_send_template_to_correct_sms_provider_and_persist_with_job_id(sa assert persisted_notification.status == 'sent' assert persisted_notification.sent_at > now assert persisted_notification.created_at == now + assert persisted_notification.sent_by == 'firetext' def test_should_use_email_template_and_persist(sample_email_template, mocker): @@ -143,6 +152,7 @@ def test_should_use_email_template_and_persist(sample_email_template, mocker): } mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.aws_ses_client.send_email') + mocker.patch('app.aws_ses_client.get_name', return_value='ses') notification_id = uuid.uuid4() now = datetime.utcnow() @@ -167,6 +177,7 @@ def test_should_use_email_template_and_persist(sample_email_template, mocker): assert persisted_notification.created_at == now assert persisted_notification.sent_at > now assert persisted_notification.status == 'sent' + assert persisted_notification.sent_by == 'ses' def test_should_persist_notification_as_failed_if_sms_client_fails(sample_template, mocker): @@ -176,6 +187,7 @@ def test_should_persist_notification_as_failed_if_sms_client_fails(sample_templa } mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.firetext_client.send_sms', side_effect=FiretextClientException()) + mocker.patch('app.firetext_client.get_name', return_value="firetext") now = datetime.utcnow() notification_id = uuid.uuid4() @@ -194,6 +206,7 @@ def test_should_persist_notification_as_failed_if_sms_client_fails(sample_templa assert persisted_notification.status == 'failed' assert persisted_notification.created_at == now assert persisted_notification.sent_at > now + assert persisted_notification.sent_by == 'firetext' def test_should_persist_notification_as_failed_if_email_client_fails(sample_email_template, mocker): @@ -203,6 +216,8 @@ def test_should_persist_notification_as_failed_if_email_client_fails(sample_emai } mocker.patch('app.encryption.decrypt', return_value=notification) mocker.patch('app.aws_ses_client.send_email', side_effect=AwsSesClientException()) + mocker.patch('app.aws_ses_client.get_name', return_value="ses") + now = datetime.utcnow() notification_id = uuid.uuid4() @@ -227,6 +242,7 @@ def test_should_persist_notification_as_failed_if_email_client_fails(sample_emai assert persisted_notification.template_id == sample_email_template.id assert persisted_notification.status == 'failed' assert persisted_notification.created_at == now + assert persisted_notification.sent_by == 'ses' assert persisted_notification.sent_at > now diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 504c7d0dc..776db5a3e 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -6,6 +6,7 @@ from app.models import Service from app.dao.templates_dao import dao_get_all_templates_for_service from app.dao.services_dao import dao_update_service from tests.app.conftest import sample_job +from freezegun import freeze_time def test_get_notification_by_id(notify_api, sample_notification): @@ -427,6 +428,7 @@ def test_should_not_allow_template_from_another_service_on_job_sms( assert test_string in json_resp['message']['template'] +@freeze_time("2016-01-01 11:09:00.061258") def test_should_allow_valid_sms_notification(notify_api, sample_template, mocker): with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -454,13 +456,15 @@ def test_should_allow_valid_sms_notification(notify_api, sample_template, mocker app.celery.tasks.send_sms.apply_async.assert_called_once_with( (str(sample_template.service_id), notification_id, - "something_encrypted"), + "something_encrypted", + "2016-01-01 11:09:00.061258"), queue="sms" ) assert response.status_code == 201 assert notification_id +@freeze_time("2016-01-01 11:09:00.061258") def test_should_allow_valid_sms_notification_for_job(notify_api, sample_job, mocker): with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -489,7 +493,8 @@ def test_should_allow_valid_sms_notification_for_job(notify_api, sample_job, moc app.celery.tasks.send_sms.apply_async.assert_called_once_with( (str(sample_job.service_id), notification_id, - "something_encrypted"), + "something_encrypted", + "2016-01-01 11:09:00.061258"), queue="sms" ) assert response.status_code == 201 @@ -812,6 +817,7 @@ def test_should_not_send_email_for_job_if_restricted_and_not_a_service_user( assert 'Email address not permitted for restricted service' in json_resp['message']['to'] +@freeze_time("2016-01-01 11:09:00.061258") def test_should_allow_valid_email_notification(notify_api, sample_email_template, mocker): with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -841,7 +847,8 @@ def test_should_allow_valid_email_notification(notify_api, sample_email_template notification_id, "Email Subject", "sample.service@test.notify.com", - "something_encrypted"), + "something_encrypted", + "2016-01-01 11:09:00.061258"), queue="email" ) assert response.status_code == 201 @@ -880,6 +887,7 @@ def test_send_notification_invalid_job_id_on_job_email(notify_api, sample_email_ assert test_string in json_resp['message']['job'] +@freeze_time("2016-01-01 11:09:00.061258") def test_should_allow_valid_email_notification_for_job(notify_api, sample_job, sample_email_template, mocker): with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -904,14 +912,14 @@ def test_should_allow_valid_email_notification_for_job(notify_api, sample_job, s data=json.dumps(data), headers=[('Content-Type', 'application/json'), auth_header]) - print(json.loads(response.data)) notification_id = json.loads(response.data)['notification_id'] app.celery.tasks.send_email.apply_async.assert_called_once_with( (str(sample_job.service_id), notification_id, "Email Subject", "sample.service@test.notify.com", - "something_encrypted"), + "something_encrypted", + "2016-01-01 11:09:00.061258"), queue="email" ) assert response.status_code == 201 From 0b63477e49781e0a82435bdc91ddd4c021aeef99 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 25 Feb 2016 11:35:32 +0000 Subject: [PATCH 11/12] Removed now unused notification for job endpoints - this is now handled in the tasks --- app/notifications/rest.py | 41 +-- app/schemas.py | 1 - tests/app/notifications/test_rest.py | 480 +-------------------------- 3 files changed, 12 insertions(+), 510 deletions(-) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 137305eb9..97db7c5b4 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -11,15 +11,12 @@ from app import api_user, encryption, create_uuid from app.dao import ( templates_dao, services_dao, - notifications_dao, - jobs_dao + notifications_dao ) from app.schemas import ( email_notification_schema, sms_template_notification_schema, - notification_status_schema, - job_sms_template_notification_schema, - job_email_template_notification_schema + notification_status_schema ) from app.celery.tasks import send_sms, send_email from sqlalchemy.orm.exc import NoResultFound @@ -45,36 +42,20 @@ def get_notifications(notification_id): @notifications.route('/notifications/sms', methods=['POST']) def create_sms_notification(): - return send_notification(notification_type=SMS_NOTIFICATION, expects_job=False) - - -@notifications.route('/notifications/sms/service/', methods=['POST']) -def create_sms_for_job(service_id): - return send_notification(service_id=service_id, notification_type=SMS_NOTIFICATION, expects_job=True) + return send_notification(notification_type=SMS_NOTIFICATION) @notifications.route('/notifications/email', methods=['POST']) def create_email_notification(): - return send_notification(notification_type=EMAIL_NOTIFICATION, expects_job=False) + return send_notification(notification_type=EMAIL_NOTIFICATION) -@notifications.route('/notifications/email/service/', methods=['POST']) -def create_email_notification_for_job(service_id): - return send_notification(service_id=service_id, notification_type=EMAIL_NOTIFICATION, expects_job=True) - - -def send_notification(notification_type, service_id=None, expects_job=False): +def send_notification(notification_type): assert notification_type - if not service_id: - service_id = api_user['client'] + service_id = api_user['client'] - if expects_job: - schema = job_sms_template_notification_schema if notification_type is SMS_NOTIFICATION else \ - job_email_template_notification_schema - else: - schema = sms_template_notification_schema if notification_type is SMS_NOTIFICATION else \ - email_notification_schema + schema = sms_template_notification_schema if notification_type is SMS_NOTIFICATION else email_notification_schema notification, errors = schema.load(request.get_json()) if errors: @@ -91,13 +72,7 @@ def send_notification(notification_type, service_id=None, expects_job=False): message={ 'template': ['Template {} not found for service {}'.format(notification['template'], service_id)] } - ), 400 - - if expects_job: - job = jobs_dao.dao_get_job_by_service_id_and_job_id(service_id, notification['job']) - - if not job: - return jsonify(result="error", message={'job': ['Job {} not found'.format(notification['job'])]}), 400 + ), 404 service = services_dao.dao_fetch_service_by_id(api_user['client']) diff --git a/app/schemas.py b/app/schemas.py index de4c52f16..68770f20f 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -143,7 +143,6 @@ api_keys_schema = ApiKeySchema(many=True) job_schema = JobSchema() job_schema_load_json = JobSchema(load_json=True) jobs_schema = JobSchema(many=True) -# TODO: Remove this schema once the admin app has stopped using the /user/code endpoint old_request_verify_code_schema = OldRequestVerifyCodeSchema() request_verify_code_schema = RequestVerifyCodeSchema() sms_admin_notification_schema = SmsAdminNotificationSchema() diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 776db5a3e..f101cac80 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -5,7 +5,6 @@ from flask import json from app.models import Service from app.dao.templates_dao import dao_get_all_templates_for_service from app.dao.services_dao import dao_update_service -from tests.app.conftest import sample_job from freezegun import freeze_time @@ -73,32 +72,6 @@ def test_create_sms_should_reject_if_missing_required_fields(notify_api, sample_ assert response.status_code == 400 -def test_create_sms_should_reject_if_missing_job_id_on_job_sms(notify_api, sample_api_key, mocker): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mocker.patch('app.celery.tasks.send_sms.apply_async') - - data = {} - auth_header = create_authorization_header( - service_id=sample_api_key.service_id, - request_body=json.dumps(data), - path='/notifications/sms/service/{}'.format(sample_api_key.service_id), - method='POST') - - response = client.post( - path='/notifications/sms/service/{}'.format(sample_api_key.service_id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) - - json_resp = json.loads(response.get_data(as_text=True)) - app.celery.tasks.send_sms.apply_async.assert_not_called() - assert json_resp['result'] == 'error' - assert 'Missing data for required field.' in json_resp['message']['to'][0] - assert 'Missing data for required field.' in json_resp['message']['template'][0] - assert 'Missing data for required field.' in json_resp['message']['job'][0] - assert response.status_code == 400 - - def test_should_reject_bad_phone_numbers(notify_api, sample_template, mocker): with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -127,35 +100,6 @@ def test_should_reject_bad_phone_numbers(notify_api, sample_template, mocker): assert response.status_code == 400 -def test_should_reject_bad_phone_numbers_on_job_sms(notify_api, sample_job, mocker): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mocker.patch('app.celery.tasks.send_sms.apply_async') - - data = { - 'to': 'invalid', - 'template': sample_job.template.id, - 'job': sample_job.id - } - auth_header = create_authorization_header( - request_body=json.dumps(data), - path='/notifications/sms/service/{}'.format(sample_job.service_id), - method='POST') - - response = client.post( - path='/notifications/sms/service/{}'.format(sample_job.service_id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) - - json_resp = json.loads(response.get_data(as_text=True)) - app.celery.tasks.send_sms.apply_async.assert_not_called() - - assert json_resp['result'] == 'error' - assert len(json_resp['message'].keys()) == 1 - assert 'Invalid phone number, must be of format +441234123123' in json_resp['message']['to'] - assert response.status_code == 400 - - def test_send_notification_invalid_template_id(notify_api, sample_template, mocker): with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -179,108 +123,12 @@ def test_send_notification_invalid_template_id(notify_api, sample_template, mock json_resp = json.loads(response.get_data(as_text=True)) app.celery.tasks.send_sms.apply_async.assert_not_called() - assert response.status_code == 400 + assert response.status_code == 404 assert len(json_resp['message'].keys()) == 1 test_string = 'Template {} not found for service {}'.format(9999, sample_template.service.id) assert test_string in json_resp['message']['template'] -def test_send_notification_invalid_service_id_on_job_sms(notify_api, sample_job, mocker): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mocker.patch('app.celery.tasks.send_sms.apply_async') - - service_id = uuid.uuid4() - - data = { - 'to': '+441234123123', - 'template': sample_job.template.id, - 'job': sample_job.id - } - - auth_header = create_authorization_header( - service_id=sample_job.service.id, - request_body=json.dumps(data), - path='/notifications/sms/service/{}'.format(service_id), - method='POST') - - response = client.post( - path='/notifications/sms/service/{}'.format(service_id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) - - json_resp = json.loads(response.get_data(as_text=True)) - app.celery.tasks.send_sms.apply_async.assert_not_called() - - assert response.status_code == 400 - assert len(json_resp['message'].keys()) == 1 - test_string = 'Template {} not found for service {}'.format(sample_job.template.id, service_id) - assert test_string in json_resp['message']['template'] - - -def test_send_notification_invalid_template_id_on_job_sms(notify_api, sample_job, mocker): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mocker.patch('app.celery.tasks.send_sms.apply_async') - - data = { - 'to': '+441234123123', - 'template': 9999, - 'job': sample_job.id - } - - auth_header = create_authorization_header( - service_id=sample_job.service.id, - request_body=json.dumps(data), - path='/notifications/sms/service/{}'.format(sample_job.service_id), - method='POST') - - response = client.post( - path='/notifications/sms/service/{}'.format(sample_job.service_id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) - - json_resp = json.loads(response.get_data(as_text=True)) - app.celery.tasks.send_sms.apply_async.assert_not_called() - - assert response.status_code == 400 - assert len(json_resp['message'].keys()) == 1 - test_string = 'Template {} not found for service {}'.format(9999, sample_job.service.id) - assert test_string in json_resp['message']['template'] - - -def test_send_notification_invalid_job_id_on_job_sms(notify_api, sample_template, mocker): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mocker.patch('app.celery.tasks.send_sms.apply_async') - job_id = uuid.uuid4() - data = { - 'to': '+441234123123', - 'template': sample_template.id, - 'job': job_id - - } - - auth_header = create_authorization_header( - service_id=sample_template.service.id, - request_body=json.dumps(data), - path='/notifications/sms/service/{}'.format(sample_template.service_id), - method='POST') - - response = client.post( - path='/notifications/sms/service/{}'.format(sample_template.service_id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) - - json_resp = json.loads(response.get_data(as_text=True)) - app.celery.tasks.send_sms.apply_async.assert_not_called() - - assert response.status_code == 400 - assert len(json_resp['message'].keys()) == 1 - test_string = 'Job {} not found'.format(job_id) - assert test_string in json_resp['message']['job'] - - def test_prevents_sending_to_any_mobile_on_restricted_service(notify_api, sample_template, mocker): with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -315,41 +163,6 @@ def test_prevents_sending_to_any_mobile_on_restricted_service(notify_api, sample assert 'Invalid phone number for restricted service' in json_resp['message']['to'] -def test_prevents_sending_to_any_mobile_on_restricted_service_on_job_sms(notify_api, sample_job, mocker): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mocker.patch('app.celery.tasks.send_sms.apply_async') - - Service.query.filter_by( - id=sample_job.service.id - ).update( - {'restricted': True} - ) - invalid_mob = '+449999999999' - data = { - 'to': invalid_mob, - 'template': sample_job.template.id, - 'job': sample_job.id - } - - auth_header = create_authorization_header( - service_id=sample_job.service.id, - request_body=json.dumps(data), - path='/notifications/sms/service/{}'.format(sample_job.service_id), - method='POST') - - response = client.post( - path='/notifications/sms/service/{}'.format(sample_job.service_id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) - - json_resp = json.loads(response.get_data(as_text=True)) - app.celery.tasks.send_sms.apply_async.assert_not_called() - - assert response.status_code == 400 - assert 'Invalid phone number for restricted service' in json_resp['message']['to'] - - def test_should_not_allow_template_from_another_service(notify_api, service_factory, sample_user, mocker): with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -378,52 +191,7 @@ def test_should_not_allow_template_from_another_service(notify_api, service_fact json_resp = json.loads(response.get_data(as_text=True)) app.celery.tasks.send_sms.apply_async.assert_not_called() - assert response.status_code == 400 - test_string = 'Template {} not found for service {}'.format(service_2_templates[0].id, service_1.id) - assert test_string in json_resp['message']['template'] - - -def test_should_not_allow_template_from_another_service_on_job_sms( - notify_db, - notify_db_session, - notify_api, - service_factory, - sample_user, - mocker): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mocker.patch('app.celery.tasks.send_sms.apply_async') - - service_1 = service_factory.get('service 1', user=sample_user) - service_2 = service_factory.get('service 2', user=sample_user) - - service_1_templates = dao_get_all_templates_for_service(service_id=service_2.id) - service_2_templates = dao_get_all_templates_for_service(service_id=service_2.id) - - job_1 = sample_job(notify_db, notify_db_session, service_1, service_1_templates[0]) - sample_job(notify_db, notify_db_session, service_2, service_2_templates[0]) - - data = { - 'to': sample_user.mobile_number, - 'template': service_2_templates[0].id, - 'job': job_1.id - } - - auth_header = create_authorization_header( - service_id=service_1.id, - request_body=json.dumps(data), - path='/notifications/sms/service/{}'.format(service_1.id), - method='POST') - - response = client.post( - path='/notifications/sms/service/{}'.format(service_1.id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) - - json_resp = json.loads(response.get_data(as_text=True)) - app.celery.tasks.send_sms.apply_async.assert_not_called() - - assert response.status_code == 400 + assert response.status_code == 404 test_string = 'Template {} not found for service {}'.format(service_2_templates[0].id, service_1.id) assert test_string in json_resp['message']['template'] @@ -464,43 +232,6 @@ def test_should_allow_valid_sms_notification(notify_api, sample_template, mocker assert notification_id -@freeze_time("2016-01-01 11:09:00.061258") -def test_should_allow_valid_sms_notification_for_job(notify_api, sample_job, mocker): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mocker.patch('app.celery.tasks.send_sms.apply_async') - mocker.patch('app.encryption.encrypt', return_value="something_encrypted") - - data = { - 'to': '+441234123123', - 'template': sample_job.template.id, - 'job': sample_job.id - } - - auth_header = create_authorization_header( - request_body=json.dumps(data), - path='/notifications/sms/service/{}'.format(sample_job.service_id), - method='POST', - service_id=sample_job.service_id - ) - - response = client.post( - path='/notifications/sms/service/{}'.format(sample_job.service_id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) - - notification_id = json.loads(response.data)['notification_id'] - app.celery.tasks.send_sms.apply_async.assert_called_once_with( - (str(sample_job.service_id), - notification_id, - "something_encrypted", - "2016-01-01 11:09:00.061258"), - queue="sms" - ) - assert response.status_code == 201 - assert notification_id - - def test_create_email_should_reject_if_missing_required_fields(notify_api, sample_api_key, mocker): with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -526,32 +257,6 @@ def test_create_email_should_reject_if_missing_required_fields(notify_api, sampl assert response.status_code == 400 -def test_create_email_job_should_reject_if_missing_required_fields(notify_api, sample_api_key, mocker): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mocker.patch('app.celery.tasks.send_email.apply_async') - - data = {} - auth_header = create_authorization_header( - service_id=sample_api_key.service_id, - request_body=json.dumps(data), - path='/notifications/email/service/{}'.format(sample_api_key.service_id), - method='POST') - - response = client.post( - path='/notifications/email/service/{}'.format(sample_api_key.service_id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) - - json_resp = json.loads(response.get_data(as_text=True)) - app.celery.tasks.send_email.apply_async.assert_not_called() - assert json_resp['result'] == 'error' - assert 'Missing data for required field.' in json_resp['message']['to'][0] - assert 'Missing data for required field.' in json_resp['message']['template'][0] - assert 'Missing data for required field.' in json_resp['message']['job'][0] - assert response.status_code == 400 - - def test_should_reject_email_notification_with_bad_email(notify_api, sample_email_template, mocker): with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -579,34 +284,6 @@ def test_should_reject_email_notification_with_bad_email(notify_api, sample_emai assert data['message']['to'][0] == 'Invalid email' -def test_should_reject_email_job_notification_with_bad_email(notify_api, sample_job, sample_email_template, mocker): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mocker.patch('app.celery.tasks.send_email.apply_async') - to_address = "bad-email" - data = { - 'to': to_address, - 'template': sample_email_template.service.id, - 'job': sample_job.id - } - auth_header = create_authorization_header( - service_id=sample_email_template.service.id, - request_body=json.dumps(data), - path='/notifications/email/service/{}'.format(sample_email_template.service_id), - method='POST') - - response = client.post( - path='/notifications/email/service/{}'.format(sample_email_template.service_id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) - - data = json.loads(response.get_data(as_text=True)) - app.celery.tasks.send_email.apply_async.assert_not_called() - assert response.status_code == 400 - assert data['result'] == 'error' - assert data['message']['to'][0] == 'Invalid email' - - def test_should_reject_email_notification_with_template_id_that_cant_be_found( notify_api, sample_email_template, mocker): with notify_api.test_request_context(): @@ -629,39 +306,7 @@ def test_should_reject_email_notification_with_template_id_that_cant_be_found( data = json.loads(response.get_data(as_text=True)) app.celery.tasks.send_email.apply_async.assert_not_called() - assert response.status_code == 400 - assert data['result'] == 'error' - test_string = 'Template {} not found for service {}'.format( - 1234, - sample_email_template.service.id - ) - assert test_string in data['message']['template'] - - -def test_should_reject_email_job_notification_with_template_id_that_cant_be_found( - notify_api, sample_job, sample_email_template, mocker): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mocker.patch('app.celery.tasks.send_email.apply_async') - data = { - 'to': 'ok@ok.com', - 'template': 1234, - 'job': sample_job.id - } - auth_header = create_authorization_header( - service_id=sample_email_template.service.id, - request_body=json.dumps(data), - path='/notifications/email/service/{}'.format(sample_job.service.id), - method='POST') - - response = client.post( - path='/notifications/email/service/{}'.format(sample_job.service.id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) - - data = json.loads(response.get_data(as_text=True)) - app.celery.tasks.send_email.apply_async.assert_not_called() - assert response.status_code == 400 + assert response.status_code == 404 assert data['result'] == 'error' test_string = 'Template {} not found for service {}'.format( 1234, @@ -699,53 +344,7 @@ def test_should_not_allow_email_template_from_another_service(notify_api, servic json_resp = json.loads(response.get_data(as_text=True)) app.celery.tasks.send_email.apply_async.assert_not_called() - assert response.status_code == 400 - test_string = 'Template {} not found for service {}'.format(service_2_templates[0].id, service_1.id) - assert test_string in json_resp['message']['template'] - - -def test_should_not_allow_template_from_another_service_on_job_email( - notify_db, - notify_db_session, - notify_api, - service_factory, - sample_user, - mocker): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mocker.patch('app.celery.tasks.send_email.apply_async') - - service_1 = service_factory.get('service 1', user=sample_user, template_type='email') - service_2 = service_factory.get('service 2', user=sample_user, template_type='email') - - service_1_templates = dao_get_all_templates_for_service(service_id=service_2.id) - service_2_templates = dao_get_all_templates_for_service(service_id=service_2.id) - - job_1 = sample_job(notify_db, notify_db_session, service_1, service_1_templates[0]) - sample_job(notify_db, notify_db_session, service_2, service_2_templates[0]) - - data = { - 'to': sample_user.email_address, - 'template': service_2_templates[0].id, - 'job': job_1.id - } - - auth_header = create_authorization_header( - service_id=service_1.id, - request_body=json.dumps(data), - path='/notifications/email/service/{}'.format(service_1.id), - method='POST') - - response = client.post( - path='/notifications/email/service/{}'.format(service_1.id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) - - json_resp = json.loads(response.get_data(as_text=True)) - app.celery.tasks.send_email.apply_async.assert_not_called() - - assert response.status_code == 400 - print(json_resp) + assert response.status_code == 404 test_string = 'Template {} not found for service {}'.format(service_2_templates[0].id, service_1.id) assert test_string in json_resp['message']['template'] @@ -853,74 +452,3 @@ def test_should_allow_valid_email_notification(notify_api, sample_email_template ) assert response.status_code == 201 assert notification_id - - -def test_send_notification_invalid_job_id_on_job_email(notify_api, sample_email_template, mocker): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mocker.patch('app.celery.tasks.send_email.apply_async') - job_id = uuid.uuid4() - data = { - 'to': 'test@test.com', - 'template': sample_email_template.id, - 'job': job_id - - } - - auth_header = create_authorization_header( - service_id=sample_email_template.service.id, - request_body=json.dumps(data), - path='/notifications/email/service/{}'.format(sample_email_template.service_id), - method='POST') - - response = client.post( - path='/notifications/email/service/{}'.format(sample_email_template.service_id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) - - json_resp = json.loads(response.get_data(as_text=True)) - app.celery.tasks.send_email.apply_async.assert_not_called() - - assert response.status_code == 400 - assert len(json_resp['message'].keys()) == 1 - test_string = 'Job {} not found'.format(job_id) - assert test_string in json_resp['message']['job'] - - -@freeze_time("2016-01-01 11:09:00.061258") -def test_should_allow_valid_email_notification_for_job(notify_api, sample_job, sample_email_template, mocker): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mocker.patch('app.celery.tasks.send_email.apply_async') - mocker.patch('app.encryption.encrypt', return_value="something_encrypted") - - data = { - 'to': 'ok@ok.com', - 'template': sample_email_template.id, - 'job': sample_job.id - } - - auth_header = create_authorization_header( - request_body=json.dumps(data), - path='/notifications/email/service/{}'.format(sample_job.service_id), - method='POST', - service_id=sample_job.service_id - ) - - response = client.post( - path='/notifications/email/service/{}'.format(sample_job.service_id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) - - notification_id = json.loads(response.data)['notification_id'] - app.celery.tasks.send_email.apply_async.assert_called_once_with( - (str(sample_job.service_id), - notification_id, - "Email Subject", - "sample.service@test.notify.com", - "something_encrypted", - "2016-01-01 11:09:00.061258"), - queue="email" - ) - assert response.status_code == 201 - assert notification_id From 34f2016b19e6d4246e652f49cc6b8196a875c71a Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Thu, 25 Feb 2016 12:19:48 +0000 Subject: [PATCH 12/12] Reorder DB scripts after merge from master --- migrations/versions/0022_add_processing_dates.py | 2 +- migrations/versions/0023_add_sender.py | 4 ++-- migrations/versions/0023_drop_token.py | 4 ++-- tests/app/conftest.py | 1 - tests/app/service/test_rest.py | 2 +- 5 files changed, 6 insertions(+), 7 deletions(-) diff --git a/migrations/versions/0022_add_processing_dates.py b/migrations/versions/0022_add_processing_dates.py index 89f2d4d71..996176b6e 100644 --- a/migrations/versions/0022_add_processing_dates.py +++ b/migrations/versions/0022_add_processing_dates.py @@ -8,7 +8,7 @@ Create Date: 2016-02-24 17:15:47.457200 # revision identifiers, used by Alembic. revision = '0022_add_processing_dates' -down_revision = '0021_add_job_metadata' +down_revision = '0022_add_invite_users' from alembic import op import sqlalchemy as sa diff --git a/migrations/versions/0023_add_sender.py b/migrations/versions/0023_add_sender.py index fbc41aa99..654c765ad 100644 --- a/migrations/versions/0023_add_sender.py +++ b/migrations/versions/0023_add_sender.py @@ -1,13 +1,13 @@ """empty message -Revision ID: d98c7c20f1d4 +Revision ID: 0023_add_sender Revises: 0022_add_processing_dates Create Date: 2016-02-24 17:18:21.942772 """ # revision identifiers, used by Alembic. -revision = 'd98c7c20f1d4' +revision = '0023_add_sender' down_revision = '0022_add_processing_dates' from alembic import op diff --git a/migrations/versions/0023_drop_token.py b/migrations/versions/0023_drop_token.py index bf6f705d7..0a003d186 100644 --- a/migrations/versions/0023_drop_token.py +++ b/migrations/versions/0023_drop_token.py @@ -1,14 +1,14 @@ """empty message Revision ID: 0023_drop_token -Revises: 0022_add_invite_users +Revises: 0023_add_sender Create Date: 2016-02-24 13:58:04.440296 """ # revision identifiers, used by Alembic. revision = '0023_drop_token' -down_revision = '0022_add_invite_users' +down_revision = '0023_add_sender' from alembic import op import sqlalchemy as sa diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 1b9e1df9e..b1590b341 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -103,7 +103,6 @@ def sample_service(notify_db, user = sample_user(notify_db, notify_db_session) data = { 'name': service_name, - 'users': [user], 'limit': 1000, 'active': False, 'restricted': False, diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 8df0b526f..bf1b74594 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -385,6 +385,6 @@ def test_get_users_for_service_returns_empty_list_if_no_users_associated_with_se '/service/{}/users'.format(sample_service.id), headers=[('Content-Type', 'application/json'), auth_header] ) - assert response.status_code == 200 result = json.loads(response.get_data(as_text=True)) + assert response.status_code == 200 assert result['data'] == []