From 635debb5a601ec8456713944244f64b1b47c0596 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 23 Feb 2016 17:30:50 +0000 Subject: [PATCH] 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