From aba1cd2ed557c10c934ec1952ed0f11bf3af9081 Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Wed, 3 Feb 2016 13:16:19 +0000 Subject: [PATCH 01/28] Sqs queues now populated from all create_notification api calls. Marshmallow schemas added for notification. --- app/aws_sqs.py | 18 ++++ app/dao/templates_dao.py | 1 - app/notifications/rest.py | 134 +++++---------------------- app/schemas.py | 75 ++++++++++++++- tests/app/notifications/test_rest.py | 108 +++++++++++++-------- tests/app/user/test_rest_verify.py | 2 - tests/conftest.py | 7 ++ 7 files changed, 184 insertions(+), 161 deletions(-) create mode 100644 app/aws_sqs.py diff --git a/app/aws_sqs.py b/app/aws_sqs.py new file mode 100644 index 000000000..3d6317b2a --- /dev/null +++ b/app/aws_sqs.py @@ -0,0 +1,18 @@ +import uuid +import boto3 +from itsdangerous import URLSafeSerializer +from flask import current_app + + +def add_notification_to_queue(service_id, template_id, type_, notification): + q = boto3.resource( + 'sqs', region_name=current_app.config['AWS_REGION'] + ).create_queue(QueueName=str(service_id)) + message_id = str(uuid.uuid4()) + serializer = URLSafeSerializer(current_app.config.get('SECRET_KEY')) + encrypted = serializer.dumps(notification, current_app.config.get('DANGEROUS_SALT')) + q.send_message(MessageBody=encrypted, + MessageAttributes={'type': {'StringValue': type_, 'DataType': 'String'}, + 'message_id': {'StringValue': message_id, 'DataType': 'String'}, + 'service_id': {'StringValue': str(service_id), 'DataType': 'String'}, + 'template_id': {'StringValue': str(template_id), 'DataType': 'String'}}) diff --git a/app/dao/templates_dao.py b/app/dao/templates_dao.py index 42ef387fb..d6557f29d 100644 --- a/app/dao/templates_dao.py +++ b/app/dao/templates_dao.py @@ -23,7 +23,6 @@ def delete_model_template(template): def get_model_templates(template_id=None, service_id=None): - temp = Template.query.first() # TODO need better mapping from function params to sql query. if template_id and service_id: return Template.query.filter_by( diff --git a/app/notifications/rest.py b/app/notifications/rest.py index a130ade45..59dc1eaa1 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -7,10 +7,11 @@ from flask import ( request, current_app ) -from itsdangerous import URLSafeSerializer -from app import notify_alpha_client -from app import api_user +from app import (notify_alpha_client, api_user) +from app.aws_sqs import add_notification_to_queue from app.dao import (templates_dao, services_dao) +from app.schemas import ( + email_notification_schema, sms_admin_notification_schema, sms_template_notification_schema) import re mobile_regex = re.compile("^\\+44[\\d]{10}$") @@ -25,129 +26,38 @@ def get_notifications(notification_id): @notifications.route('/sms', methods=['POST']) def create_sms_notification(): - notification = request.get_json()['notification'] - errors = {} - to, to_errors = validate_to(notification) - if to_errors['to']: - errors.update(to_errors) + resp_json = request.get_json() # TODO: should create a different endpoint for the admin client to send verify codes. if api_user['client'] == current_app.config.get('ADMIN_CLIENT_USER_NAME'): - content, content_errors = validate_content_for_admin_client(notification) - if content_errors['content']: - errors.update(content_errors) + notification, errors = sms_admin_notification_schema.load(resp_json) if errors: return jsonify(result="error", message=errors), 400 - - return jsonify(notify_alpha_client.send_sms(mobile_number=to, message=content)), 200 - + template_id = 'admin' + message = notification['content'] else: - to, restricted_errors = validate_to_for_service(to, api_user['client']) - if restricted_errors['restricted']: - errors.update(restricted_errors) - - template, template_errors = validate_template(notification, api_user['client']) - if template_errors['template']: - errors.update(template_errors) + notification, errors = sms_template_notification_schema.load(resp_json) if errors: return jsonify(result="error", message=errors), 400 + template_id = notification['template'] + message = notification['template'] - # add notification to the queue - service = services_dao.get_model_services(api_user['client'], _raise=False) - _add_notification_to_queue(template.id, service, 'sms', to) - return jsonify(notify_alpha_client.send_sms(mobile_number=to, message=template.content)), 200 + add_notification_to_queue(api_user['client'], template_id, 'sms', notification) + return jsonify(notify_alpha_client.send_sms( + mobile_number=notification['to'], message=message)), 200 @notifications.route('/email', methods=['POST']) def create_email_notification(): - notification = request.get_json()['notification'] - errors = {} - for k in ['to', 'from', 'subject', 'message']: - k_error = validate_required_and_something(notification, k) - if k_error: - errors.update(k_error) - + resp_json = request.get_json() + notification, errors = email_notification_schema.load(resp_json) if errors: return jsonify(result="error", message=errors), 400 - + # At the moment we haven't hooked up + # template handling for sending email notifications. + add_notification_to_queue(api_user['client'], "admin", 'email', notification) return jsonify(notify_alpha_client.send_email( - notification['to'], - notification['message'], - notification['from'], + notification['to_address'], + notification['body'], + notification['from_address'], notification['subject'])) - - -def validate_to_for_service(mob, service_id): - errors = {"restricted": []} - service = services_dao.get_model_services(service_id=service_id) - if service.restricted: - valid = False - for usr in service.users: - if mob == usr.mobile_number: - valid = True - break - if not valid: - errors['restricted'].append('Invalid phone number for restricted service') - return mob, errors - - -def validate_to(json_body): - errors = {"to": []} - mob = json_body.get('to', None) - if not mob: - errors['to'].append('Required data missing') - else: - if not mobile_regex.match(mob): - errors['to'].append('invalid phone number, must be of format +441234123123') - return mob, errors - - -def validate_template(json_body, service_id): - errors = {"template": []} - template_id = json_body.get('template', None) - template = '' - if not template_id: - errors['template'].append('Required data missing') - else: - try: - template = templates_dao.get_model_templates( - template_id=json_body['template'], - service_id=service_id) - except: - errors['template'].append("Unable to load template.") - return template, errors - - -def validate_content_for_admin_client(json_body): - errors = {"content": []} - content = json_body.get('template', None) - if not content: - errors['content'].append('Required content') - - return content, errors - - -def validate_required_and_something(json_body, field): - errors = [] - if field not in json_body and json_body[field]: - errors.append('Required data for field.') - return {field: errors} if errors else None - - -def _add_notification_to_queue(template_id, service, msg_type, to): - q = boto3.resource('sqs', region_name=current_app.config['AWS_REGION']).create_queue( - QueueName=str(service.id)) - import uuid - message_id = str(uuid.uuid4()) - notification = json.dumps({'message_id': message_id, - 'service_id': str(service.id), - 'to': to, - 'message_type': msg_type, - 'template_id': template_id}) - serializer = URLSafeSerializer(current_app.config.get('SECRET_KEY')) - encrypted = serializer.dumps(notification, current_app.config.get('DANGEROUS_SALT')) - q.send_message(MessageBody=encrypted, - MessageAttributes={'type': {'StringValue': msg_type, 'DataType': 'String'}, - 'message_id': {'StringValue': message_id, 'DataType': 'String'}, - 'service_id': {'StringValue': str(service.id), 'DataType': 'String'}, - 'template_id': {'StringValue': str(template_id), 'DataType': 'String'}}) diff --git a/app/schemas.py b/app/schemas.py index 4f2c3e3b8..f494c38f9 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -1,7 +1,11 @@ +import re from flask_marshmallow.fields import fields from . import ma from . import models -from marshmallow import post_load, ValidationError +from marshmallow_sqlalchemy.fields import Related +from marshmallow import (post_load, ValidationError, validates, validates_schema) + +mobile_regex = re.compile("^\\+44[\\d]{10}$") # TODO I think marshmallow provides a better integration and error handling. @@ -59,12 +63,70 @@ class JobSchema(BaseSchema): class RequestVerifyCodeSchema(ma.Schema): - def verify_code_type(self): - if self not in models.VERIFY_CODE_TYPES: + + code_type = fields.Str(required=True) + to = fields.Str(required=False) + + @validates('code_type') + def validate_code_type(self, code): + if code not in models.VERIFY_CODE_TYPES: raise ValidationError('Invalid code type') - code_type = fields.Str(required=True, validate=verify_code_type) - to = fields.Str(required=False) + +# TODO main purpose to be added later +# when processing templates, template will be +# common for all notifications. +class NotificationSchema(ma.Schema): + pass + + +class SmsNotificationSchema(NotificationSchema): + to = fields.Str(required=True) + + @validates('to') + def validate_to(self, value): + if not mobile_regex.match(value): + raise ValidationError('Invalid phone number, must be of format +441234123123') + + +class SmsTemplateNotificationSchema(SmsNotificationSchema): + template = fields.Int(required=True) + + @validates('template') + def validate_template(self, value): + if not models.Template.query.filter_by(id=value).first(): + # TODO is this message consistent with what marshmallow + # would normally produce. + raise ValidationError('Template not found') + + @validates_schema + def validate_schema(self, data): + """ + Validate the to field is valid for this template + """ + template_id = data.get('template', None) + template = models.Template.query.filter_by(id=template_id).first() + if template: + service = template.service + if service.restricted: + valid = False + for usr in service.users: + if data['to'] == usr.mobile_number: + valid = True + break + if not valid: + raise ValidationError('Invalid phone number for restricted service', 'restricted') + + +class SmsAdminNotificationSchema(SmsNotificationSchema): + content = fields.Str(required=True) + + +class EmailNotificationSchema(NotificationSchema): + to_address = fields.Str(load_from="to", dump_to='to', required=True) + from_address = fields.Str(load_from="from", dump_to='from', required=True) + subject = fields.Str(required=True) + body = fields.Str(load_from="message", dump_to='message', required=True) user_schema = UserSchema() @@ -83,3 +145,6 @@ job_schema = JobSchema() job_schema_load_json = JobSchema(load_json=True) jobs_schema = JobSchema(many=True) request_verify_code_schema = RequestVerifyCodeSchema() +sms_admin_notification_schema = SmsAdminNotificationSchema() +sms_template_notification_schema = SmsTemplateNotificationSchema() +email_notification_schema = EmailNotificationSchema() diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index e940a8e6e..27d598f37 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -1,4 +1,3 @@ -import boto3 import moto from tests import create_authorization_header @@ -85,9 +84,7 @@ def test_create_sms_should_reject_if_no_phone_numbers( return_value='success' ) data = { - 'notification': { - 'template': "my message" - } + 'template': "my message" } auth_header = create_authorization_header( service_id=sample_api_key.service_id, @@ -103,7 +100,7 @@ def test_create_sms_should_reject_if_no_phone_numbers( json_resp = json.loads(response.get_data(as_text=True)) assert response.status_code == 400 assert json_resp['result'] == 'error' - assert 'Required data missing' in json_resp['message']['to'][0] + assert 'Missing data for required field.' in json_resp['message']['to'][0] assert not notify_alpha_client.send_sms.called @@ -119,10 +116,8 @@ def test_should_reject_bad_phone_numbers( return_value='success' ) data = { - 'notification': { - 'to': 'invalid', - 'template': "my message" - } + 'to': 'invalid', + 'template': "my message" } auth_header = create_authorization_header( request_body=json.dumps(data), @@ -137,7 +132,7 @@ def test_should_reject_bad_phone_numbers( json_resp = json.loads(response.get_data(as_text=True)) assert response.status_code == 400 assert json_resp['result'] == 'error' - assert 'invalid phone number, must be of format +441234123123' in json_resp['message']['to'] + assert 'Invalid phone number, must be of format +441234123123' in json_resp['message']['to'] assert not notify_alpha_client.send_sms.called @@ -153,9 +148,7 @@ def test_should_reject_missing_content( return_value='success' ) data = { - 'notification': { - 'to': '+441234123123' - } + 'to': '+441234123123' } auth_header = create_authorization_header( request_body=json.dumps(data), @@ -170,7 +163,7 @@ def test_should_reject_missing_content( json_resp = json.loads(response.get_data(as_text=True)) assert response.status_code == 400 assert json_resp['result'] == 'error' - assert 'Required content' in json_resp['message']['content'] + assert 'Missing data for required field.' in json_resp['message']['content'] assert not notify_alpha_client.send_sms.called @@ -178,13 +171,13 @@ def test_should_reject_missing_content( def test_send_template_content(notify_api, notify_db, notify_db_session, + sqs_client_conn, mocker): """ Test POST endpoint '/sms' with service notification. """ with notify_api.test_request_context(): with notify_api.test_client() as client: - set_up_mock_queue() mobile = '+447719087678' msg = 'Message content' mocker.patch( @@ -202,10 +195,8 @@ def test_send_template_content(notify_api, } ) data = { - 'notification': { - 'to': mobile, - 'template': msg - } + 'to': mobile, + 'content': msg } auth_header = create_authorization_header( request_body=json.dumps(data), @@ -247,10 +238,8 @@ def test_send_notification_restrict_mobile(notify_api, return_value={} ) data = { - 'notification': { - 'to': invalid_mob, - 'template': sample_template.id - } + 'to': invalid_mob, + 'template': sample_template.id } assert invalid_mob != sample_user.mobile_number auth_header = create_authorization_header( @@ -270,14 +259,59 @@ def test_send_notification_restrict_mobile(notify_api, assert not notify_alpha_client.send_sms.called +def test_send_notification_invalid_template_id(notify_api, + notify_db, + notify_db_session, + sample_api_key, + sample_template, + sample_user, + mocker): + """ + Tests POST endpoint '/sms' with notifications-admin notification with invalid template id + """ + with notify_api.test_request_context(): + with notify_api.test_client() as client: + + Service.query.filter_by( + id=sample_template.service.id).update({'restricted': True}) + invalid_mob = '+449999999999' + mocker.patch( + 'app.notify_alpha_client.send_sms', + return_value={} + ) + data = { + 'to': invalid_mob, + 'template': 9999 + } + assert invalid_mob != sample_user.mobile_number + auth_header = create_authorization_header( + service_id=sample_template.service.id, + request_body=json.dumps(data), + path=url_for('notifications.create_sms_notification'), + method='POST') + + response = client.post( + url_for('notifications.create_sms_notification'), + 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 + assert 'Template not found' in json_resp['message']['template'] + assert not notify_alpha_client.send_sms.called + + @moto.mock_sqs -def test_should_allow_valid_message(notify_api, notify_db, notify_db_session, mocker): +def test_should_allow_valid_message(notify_api, + notify_db, + notify_db_session, + sqs_client_conn, + mocker): """ Tests POST endpoint '/sms' with notifications-admin notification. """ with notify_api.test_request_context(): with notify_api.test_client() as client: - set_up_mock_queue() mocker.patch( 'app.notify_alpha_client.send_sms', return_value={ @@ -293,10 +327,8 @@ def test_should_allow_valid_message(notify_api, notify_db, notify_db_session, mo } ) data = { - 'notification': { - 'to': '+441234123123', - 'template': 'valid' - } + 'to': '+441234123123', + 'content': 'valid' } auth_header = create_authorization_header( request_body=json.dumps(data), @@ -314,11 +346,13 @@ def test_should_allow_valid_message(notify_api, notify_db, notify_db_session, mo notify_alpha_client.send_sms.assert_called_with(mobile_number='+441234123123', message="valid") +@moto.mock_sqs def test_send_email_valid_data(notify_api, notify_db, notify_db_session, sample_service, sample_admin_service_id, + sqs_client_conn, mocker): with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -343,12 +377,10 @@ def test_send_email_valid_data(notify_api, } ) data = { - 'notification': { - 'to': to_address, - 'from': from_address, - 'subject': subject, - 'message': message - } + 'to': to_address, + 'from': from_address, + 'subject': subject, + 'message': message } auth_header = create_authorization_header( request_body=json.dumps(data), @@ -365,9 +397,3 @@ def test_send_email_valid_data(notify_api, assert json_resp['notification']['id'] == 100 notify_alpha_client.send_email.assert_called_with( to_address, message, from_address, subject) - - -def set_up_mock_queue(): - # set up mock queue - boto3.setup_default_session(region_name='eu-west-1') - conn = boto3.resource('sqs') diff --git a/tests/app/user/test_rest_verify.py b/tests/app/user/test_rest_verify.py index a2228d44b..8d325d191 100644 --- a/tests/app/user/test_rest_verify.py +++ b/tests/app/user/test_rest_verify.py @@ -351,11 +351,9 @@ def test_send_user_code_for_email_uses_optional_to_field(notify_api, def test_request_verify_code_schema_invalid_code_type(notify_api, notify_db, notify_db_session, sample_user): - import json from app.schemas import request_verify_code_schema data = json.dumps({'code_type': 'not_sms'}) code, error = request_verify_code_schema.loads(data) - assert code == {} assert error == {'code_type': ['Invalid code type']} diff --git a/tests/conftest.py b/tests/conftest.py index 9ca193f57..37ee9bbd3 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,6 +1,7 @@ import pytest import mock import os +import boto3 from config import configs from alembic.command import upgrade from alembic.config import Config @@ -70,3 +71,9 @@ def os_environ(request): request.addfinalizer(env_patch.stop) return env_patch.start() + + +@pytest.fixture(scope='function') +def sqs_client_conn(request): + boto3.setup_default_session(region_name='eu-west-1') + return boto3.resource('sqs') From 8fd15b44eb5128bf732f20a4c0d93364b7e06ad7 Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Wed, 3 Feb 2016 13:52:09 +0000 Subject: [PATCH 02/28] Missed a couple of places where we should push to the queue. --- app/user/rest.py | 18 +++++++++++++----- tests/app/user/test_rest_verify.py | 12 +++++++++++- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/app/user/rest.py b/app/user/rest.py index b4ee71d1e..96eba04d0 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -1,8 +1,9 @@ from datetime import datetime -from flask import (jsonify, request, abort) +from flask import (jsonify, request, abort, Blueprint) from sqlalchemy.exc import DataError from sqlalchemy.orm.exc import NoResultFound from app.dao.services_dao import get_model_services +from app.aws_sqs import add_notification_to_queue from app.dao.users_dao import ( get_model_users, save_model_user, @@ -16,8 +17,7 @@ from app.dao.users_dao import ( from app.schemas import ( user_schema, users_schema, service_schema, services_schema, request_verify_code_schema, user_schema_load_json) -from app import notify_alpha_client -from flask import Blueprint +from app import (notify_alpha_client, api_user) user = Blueprint('user', __name__) @@ -137,16 +137,24 @@ def send_user_code(user_id): # notify_alpha_client if verify_code.get('code_type') == 'sms': mobile = user.mobile_number if verify_code.get('to', None) is None else verify_code.get('to') + notification = {'to': mobile, 'content': secret_code} + add_notification_to_queue(api_user['client'], 'admin', 'sms', notification) notify_alpha_client.send_sms( mobile_number=mobile, message=secret_code) elif verify_code.get('code_type') == 'email': email = user.email_address if verify_code.get('to', None) is None else verify_code.get('to') + notification = { + 'to_address': email, + 'from_address': 'notify@digital.cabinet-office.gov.uk', + 'subject': 'Verification code', + 'body': secret_code} + add_notification_to_queue(api_user['client'], 'admin', 'sms', notification) notify_alpha_client.send_email( email, secret_code, - 'notify@digital.cabinet-office.gov.uk', - 'Verification code') + notification['from_address'], + notification['subject']) else: abort(500) return jsonify({}), 204 diff --git a/tests/app/user/test_rest_verify.py b/tests/app/user/test_rest_verify.py index 8d325d191..b52a4dc8e 100644 --- a/tests/app/user/test_rest_verify.py +++ b/tests/app/user/test_rest_verify.py @@ -1,4 +1,5 @@ import json +import moto from datetime import (datetime, timedelta) from flask import url_for @@ -55,9 +56,11 @@ def test_user_verify_code_sms_missing_code(notify_api, assert not VerifyCode.query.first().code_used +@moto.mock_sqs def test_user_verify_code_email(notify_api, notify_db, notify_db_session, + sqs_client_conn, sample_email_code): """ Tests POST endpoint '//verify/code' @@ -244,10 +247,12 @@ def test_user_verify_password_missing_password(notify_api, assert 'Required field missing data' in json_resp['message']['password'] +@moto.mock_sqs def test_send_user_code_for_sms(notify_api, notify_db, notify_db_session, sample_sms_code, + sqs_client_conn, mock_notify_client_send_sms, mock_secret_code): """ @@ -270,10 +275,12 @@ def test_send_user_code_for_sms(notify_api, message='11111') +@moto.mock_sqs def test_send_user_code_for_sms_with_optional_to_field(notify_api, notify_db, notify_db_session, sample_sms_code, + sqs_client_conn, mock_notify_client_send_sms, mock_secret_code): """ @@ -296,10 +303,12 @@ def test_send_user_code_for_sms_with_optional_to_field(notify_api, message='11111') +@moto.mock_sqs def test_send_user_code_for_email(notify_api, notify_db, notify_db_session, sample_email_code, + sqs_client_conn, mock_notify_client_send_email, mock_secret_code): """ @@ -323,10 +332,12 @@ def test_send_user_code_for_email(notify_api, 'Verification code') +@moto.mock_sqs def test_send_user_code_for_email_uses_optional_to_field(notify_api, notify_db, notify_db_session, sample_email_code, + sqs_client_conn, mock_notify_client_send_email, mock_secret_code): """ @@ -358,7 +369,6 @@ def test_request_verify_code_schema_invalid_code_type(notify_api, notify_db, not def test_request_verify_code_schema_with_to(notify_api, notify_db, notify_db_session, sample_user): - import json from app.schemas import request_verify_code_schema data = json.dumps({'code_type': 'sms', 'to': 'some@one.gov.uk'}) code, error = request_verify_code_schema.loads(data) From 6286646d7f68a0a6114f7ee7489c59e93db3e002 Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Wed, 3 Feb 2016 15:53:16 +0000 Subject: [PATCH 03/28] Fix for review comments. --- app/notifications/rest.py | 3 --- app/schemas.py | 1 - 2 files changed, 4 deletions(-) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 59dc1eaa1..db7a6d22e 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -12,9 +12,6 @@ from app.aws_sqs import add_notification_to_queue from app.dao import (templates_dao, services_dao) from app.schemas import ( email_notification_schema, sms_admin_notification_schema, sms_template_notification_schema) -import re - -mobile_regex = re.compile("^\\+44[\\d]{10}$") notifications = Blueprint('notifications', __name__) diff --git a/app/schemas.py b/app/schemas.py index f494c38f9..981924765 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -2,7 +2,6 @@ import re from flask_marshmallow.fields import fields from . import ma from . import models -from marshmallow_sqlalchemy.fields import Related from marshmallow import (post_load, ValidationError, validates, validates_schema) mobile_regex = re.compile("^\\+44[\\d]{10}$") From 1d4d03dbe817a73c7d0baca9d96b0c0c99e761a9 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 4 Feb 2016 12:07:26 +0000 Subject: [PATCH 04/28] Update to create_sms_notification Removed the logic to check the api_user is the admin client user name. There is another controller method to handle sending the verification codes. --- app/notifications/rest.py | 31 ++++------ tests/app/notifications/test_rest.py | 89 ++-------------------------- 2 files changed, 17 insertions(+), 103 deletions(-) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index db7a6d22e..7e4889e9f 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -1,17 +1,14 @@ -import json - -import boto3 from flask import ( Blueprint, jsonify, - request, - current_app + request ) + from app import (notify_alpha_client, api_user) from app.aws_sqs import add_notification_to_queue -from app.dao import (templates_dao, services_dao) +from app.dao import (templates_dao) from app.schemas import ( - email_notification_schema, sms_admin_notification_schema, sms_template_notification_schema) + email_notification_schema, sms_template_notification_schema) notifications = Blueprint('notifications', __name__) @@ -25,21 +22,15 @@ def get_notifications(notification_id): def create_sms_notification(): resp_json = request.get_json() - # TODO: should create a different endpoint for the admin client to send verify codes. - if api_user['client'] == current_app.config.get('ADMIN_CLIENT_USER_NAME'): - notification, errors = sms_admin_notification_schema.load(resp_json) - if errors: - return jsonify(result="error", message=errors), 400 - template_id = 'admin' - message = notification['content'] - else: - notification, errors = sms_template_notification_schema.load(resp_json) - if errors: - return jsonify(result="error", message=errors), 400 - template_id = notification['template'] - message = notification['template'] + notification, errors = sms_template_notification_schema.load(resp_json) + if errors: + return jsonify(result="error", message=errors), 400 + template_id = notification['template'] + # TODO: remove once beta is reading notifications from the queue + message = templates_dao.get_model_templates(template_id).content add_notification_to_queue(api_user['client'], template_id, 'sms', notification) + # TODO: remove once beta is reading notifications from the queue return jsonify(notify_alpha_client.send_sms( mobile_number=notification['to'], message=message)), 200 diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 27d598f37..4595e755f 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -136,86 +136,6 @@ def test_should_reject_bad_phone_numbers( assert not notify_alpha_client.send_sms.called -def test_should_reject_missing_content( - notify_api, notify_db, notify_db_session, mocker): - """ - Tests GET endpoint '/' to retrieve entire service list. - """ - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mocker.patch( - 'app.notify_alpha_client.send_sms', - return_value='success' - ) - data = { - 'to': '+441234123123' - } - auth_header = create_authorization_header( - request_body=json.dumps(data), - path=url_for('notifications.create_sms_notification'), - method='POST') - - response = client.post( - url_for('notifications.create_sms_notification'), - 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 - assert json_resp['result'] == 'error' - assert 'Missing data for required field.' in json_resp['message']['content'] - assert not notify_alpha_client.send_sms.called - - -@moto.mock_sqs -def test_send_template_content(notify_api, - notify_db, - notify_db_session, - sqs_client_conn, - mocker): - """ - Test POST endpoint '/sms' with service notification. - """ - with notify_api.test_request_context(): - with notify_api.test_client() as client: - mobile = '+447719087678' - msg = 'Message content' - mocker.patch( - 'app.notify_alpha_client.send_sms', - return_value={ - "notification": { - "createdAt": "2015-11-03T09:37:27.414363Z", - "id": 100, - "jobId": 65, - "message": msg, - "method": "sms", - "status": "created", - "to": mobile - } - } - ) - data = { - 'to': mobile, - 'content': msg - } - auth_header = create_authorization_header( - request_body=json.dumps(data), - path=url_for('notifications.create_sms_notification'), - method='POST') - - response = client.post( - url_for('notifications.create_sms_notification'), - 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 == 200 - assert json_resp['notification']['id'] == 100 - notify_alpha_client.send_sms.assert_called_with( - mobile_number=mobile, - message=msg) - - def test_send_notification_restrict_mobile(notify_api, notify_db, notify_db_session, @@ -306,6 +226,8 @@ def test_should_allow_valid_message(notify_api, notify_db, notify_db_session, sqs_client_conn, + sample_user, + sample_template, mocker): """ Tests POST endpoint '/sms' with notifications-admin notification. @@ -322,13 +244,13 @@ def test_should_allow_valid_message(notify_api, "message": "valid", "method": "sms", "status": "created", - "to": "+449999999999" + "to": sample_user.mobile_number } } ) data = { 'to': '+441234123123', - 'content': 'valid' + 'template': sample_template.id } auth_header = create_authorization_header( request_body=json.dumps(data), @@ -343,7 +265,8 @@ def test_should_allow_valid_message(notify_api, json_resp = json.loads(response.get_data(as_text=True)) assert response.status_code == 200 assert json_resp['notification']['id'] == 100 - notify_alpha_client.send_sms.assert_called_with(mobile_number='+441234123123', message="valid") + notify_alpha_client.send_sms.assert_called_with(mobile_number='+441234123123', + message=sample_template.content) @moto.mock_sqs From aa57730fc9cfce4948fb6e2f622f2e7e6927b232 Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Thu, 4 Feb 2016 12:34:53 +0000 Subject: [PATCH 05/28] Add more properties of job to job message. --- app/job/rest.py | 12 ++++++++++-- tests/app/job/test_job_rest.py | 2 ++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/app/job/rest.py b/app/job/rest.py index 975b2eb9e..853408b10 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -61,7 +61,15 @@ def _enqueue_job(job): queue_name = current_app.config['NOTIFY_JOB_QUEUE'] queue = boto3.resource('sqs', region_name=aws_region).create_queue(QueueName=queue_name) - job_json = json.dumps({'job_id': str(job.id), 'service_id': str(job.service.id)}) + data = { + 'job_id': str(job.id), + 'service_id': str(job.service.id), + 'template_id': job.template_id, + 'bucket_name': job.bucket_name + } + job_json = json.dumps(data) queue.send_message(MessageBody=job_json, MessageAttributes={'job_id': {'StringValue': str(job.id), 'DataType': 'String'}, - 'service_id': {'StringValue': str(job.service.id), 'DataType': 'String'}}) + 'service_id': {'StringValue': str(job.service.id), 'DataType': 'String'}, + 'template_id': {'StringValue': str(job.template_id), 'DataType': 'Number'}, + 'bucket_name': {'StringValue': job.bucket_name, 'DataType': 'String'}}) diff --git a/tests/app/job/test_job_rest.py b/tests/app/job/test_job_rest.py index b8af8f7e9..e70b64120 100644 --- a/tests/app/job/test_job_rest.py +++ b/tests/app/job/test_job_rest.py @@ -121,6 +121,8 @@ def test_create_job(notify_api, notify_db, notify_db_session, sample_template): expected_message = json.loads(messages[0].body) assert expected_message['job_id'] == str(job_id) assert expected_message['service_id'] == str(service_id) + assert expected_message['template_id'] == template_id + assert expected_message['bucket_name'] == bucket_name def _setup_jobs(notify_db, notify_db_session, template, number_of_jobs=5): From bec4bbe04e2fe7d156899f06d8bbb60b6025491c Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Thu, 4 Feb 2016 20:55:09 +0000 Subject: [PATCH 06/28] Endpoint and dao method for updating job status. --- app/dao/jobs_dao.py | 12 ++++++++--- app/job/rest.py | 17 ++++++++++++++- tests/app/dao/test_jobs_dao.py | 21 +++++++++++++++++++ tests/app/job/test_job_rest.py | 38 ++++++++++++++++++++++++++++++++++ 4 files changed, 84 insertions(+), 4 deletions(-) diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index d826c822c..e3df928a5 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -2,9 +2,15 @@ from app import db from app.models import Job -def save_job(job): - db.session.add(job) - db.session.commit() +def save_job(job, update_dict={}): + if update_dict: + update_dict.pop('id') + update_dict.pop('service') + update_dict.pop('template') + 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): diff --git a/app/job/rest.py b/app/job/rest.py index 853408b10..5597cc181 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -8,6 +8,7 @@ from flask import ( current_app ) +from sqlalchemy.exc import SQLAlchemyError from sqlalchemy.exc import DataError from sqlalchemy.orm.exc import NoResultFound @@ -19,7 +20,8 @@ from app.dao.jobs_dao import ( from app.schemas import ( job_schema, - jobs_schema + jobs_schema, + job_schema_load_json ) job = Blueprint('job', __name__, url_prefix='/service//job') @@ -56,6 +58,19 @@ def create_job(service_id): return jsonify(data=job_schema.dump(job).data), 201 +@job.route('/', methods=['PUT']) +def update_job(service_id, job_id): + job = get_job(service_id, job_id) + update_dict, errors = job_schema_load_json.load(request.get_json()) + if errors: + return jsonify(result="error", message=errors), 400 + try: + save_job(job, update_dict=update_dict) + except Exception as e: + return jsonify(result="error", message=str(e)), 400 + return jsonify(data=job_schema.dump(job).data), 200 + + def _enqueue_job(job): aws_region = current_app.config['AWS_REGION'] queue_name = current_app.config['NOTIFY_JOB_QUEUE'] diff --git a/tests/app/dao/test_jobs_dao.py b/tests/app/dao/test_jobs_dao.py index d4134f30e..06f515fd6 100644 --- a/tests/app/dao/test_jobs_dao.py +++ b/tests/app/dao/test_jobs_dao.py @@ -1,4 +1,5 @@ import uuid +import json from app.dao.jobs_dao import ( save_job, @@ -79,3 +80,23 @@ def test_get_all_jobs(notify_db, notify_db_session, sample_template): sample_template) jobs_from_db = _get_jobs() assert len(jobs_from_db) == 5 + + +def test_update_job(notify_db, notify_db_session, 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' + } + + save_job(sample_job, update_dict=update_dict) + + job_from_db = Job.query.get(sample_job.id) + + assert job_from_db.status == 'in progress' diff --git a/tests/app/job/test_job_rest.py b/tests/app/job/test_job_rest.py index e70b64120..6a7b385d8 100644 --- a/tests/app/job/test_job_rest.py +++ b/tests/app/job/test_job_rest.py @@ -125,6 +125,44 @@ def test_create_job(notify_api, notify_db, notify_db_session, sample_template): 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' + } + + 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 _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, From cc5dad3744f0b9f0278a44894399c3de358c4fd8 Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Fri, 5 Feb 2016 11:12:59 +0000 Subject: [PATCH 07/28] Update queues with a prefix. --- app/aws_sqs.py | 4 +++- config.py | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/app/aws_sqs.py b/app/aws_sqs.py index 3d6317b2a..86b28eab8 100644 --- a/app/aws_sqs.py +++ b/app/aws_sqs.py @@ -7,7 +7,9 @@ from flask import current_app def add_notification_to_queue(service_id, template_id, type_, notification): q = boto3.resource( 'sqs', region_name=current_app.config['AWS_REGION'] - ).create_queue(QueueName=str(service_id)) + ).create_queue(QueueName="{}_{}".format( + current_app.config['NOTIFICATION_QUEUE_PREFIX'], + str(service_id))) message_id = str(uuid.uuid4()) serializer = URLSafeSerializer(current_app.config.get('SECRET_KEY')) encrypted = serializer.dumps(notification, current_app.config.get('DANGEROUS_SALT')) diff --git a/config.py b/config.py index 14d595348..8329e0edf 100644 --- a/config.py +++ b/config.py @@ -16,6 +16,8 @@ class Config(object): AWS_REGION = 'eu-west-1' NOTIFY_JOB_QUEUE = os.getenv('NOTIFY_JOB_QUEUE', 'notify-jobs-queue') + # Notification Queue names are a combination of a prefx plus a name + NOTIFICATION_QUEUE_PREFIX = 'notification' class Development(Config): From e024db68588ddf8d04164f630df807cfebde425b Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Fri, 5 Feb 2016 13:07:02 +0000 Subject: [PATCH 08/28] As job update is a PUT then all non nullable fields need to be sent with update. Also bug in not committing update fixed. --- app/dao/jobs_dao.py | 8 ++++---- app/job/rest.py | 20 +++++++++++++------- tests/app/job/test_job_rest.py | 2 +- 3 files changed, 18 insertions(+), 12 deletions(-) diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index e3df928a5..9cb192af0 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -4,13 +4,13 @@ from app.models import Job def save_job(job, update_dict={}): if update_dict: - update_dict.pop('id') - update_dict.pop('service') - update_dict.pop('template') + 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() + db.session.commit() def get_job(service_id, job_id): diff --git a/app/job/rest.py b/app/job/rest.py index 5597cc181..fab9b2a8c 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -60,6 +60,7 @@ def create_job(service_id): @job.route('/', methods=['PUT']) def update_job(service_id, job_id): + job = get_job(service_id, job_id) update_dict, errors = job_schema_load_json.load(request.get_json()) if errors: @@ -77,14 +78,19 @@ def _enqueue_job(job): queue = boto3.resource('sqs', region_name=aws_region).create_queue(QueueName=queue_name) data = { - 'job_id': str(job.id), + 'id': str(job.id), 'service_id': str(job.service.id), - 'template_id': job.template_id, - 'bucket_name': job.bucket_name + 'template_id': 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={'job_id': {'StringValue': str(job.id), 'DataType': 'String'}, - 'service_id': {'StringValue': str(job.service.id), 'DataType': 'String'}, - 'template_id': {'StringValue': str(job.template_id), 'DataType': 'Number'}, - 'bucket_name': {'StringValue': job.bucket_name, 'DataType': 'String'}}) + 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'}}) diff --git a/tests/app/job/test_job_rest.py b/tests/app/job/test_job_rest.py index 6a7b385d8..59f8049a7 100644 --- a/tests/app/job/test_job_rest.py +++ b/tests/app/job/test_job_rest.py @@ -119,7 +119,7 @@ def test_create_job(notify_api, notify_db, notify_db_session, sample_template): assert len(messages) == 1 expected_message = json.loads(messages[0].body) - assert expected_message['job_id'] == str(job_id) + assert expected_message['id'] == str(job_id) assert expected_message['service_id'] == str(service_id) assert expected_message['template_id'] == template_id assert expected_message['bucket_name'] == bucket_name From 23f4ce7255b9884460e8c73c46c77408d22d041b Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Fri, 5 Feb 2016 16:33:07 +0000 Subject: [PATCH 09/28] Fix for incorrect property name for service and template. --- app/job/rest.py | 4 ++-- tests/app/job/test_job_rest.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/job/rest.py b/app/job/rest.py index fab9b2a8c..2caaa00d9 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -79,8 +79,8 @@ def _enqueue_job(job): queue = boto3.resource('sqs', region_name=aws_region).create_queue(QueueName=queue_name) data = { 'id': str(job.id), - 'service_id': str(job.service.id), - 'template_id': job.template.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 diff --git a/tests/app/job/test_job_rest.py b/tests/app/job/test_job_rest.py index 59f8049a7..20b520e1b 100644 --- a/tests/app/job/test_job_rest.py +++ b/tests/app/job/test_job_rest.py @@ -120,8 +120,8 @@ def test_create_job(notify_api, notify_db, notify_db_session, sample_template): expected_message = json.loads(messages[0].body) assert expected_message['id'] == str(job_id) - assert expected_message['service_id'] == str(service_id) - assert expected_message['template_id'] == template_id + assert expected_message['service'] == str(service_id) + assert expected_message['template'] == template_id assert expected_message['bucket_name'] == bucket_name From 4ecdc9e4207f1bf47a83c2ee8b91066b342d0dd5 Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Mon, 8 Feb 2016 09:41:10 +0000 Subject: [PATCH 10/28] Bug fixed for posting the correct type of notification to the aws queue. --- app/aws_sqs.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/aws_sqs.py b/app/aws_sqs.py index 86b28eab8..1399bed80 100644 --- a/app/aws_sqs.py +++ b/app/aws_sqs.py @@ -13,6 +13,8 @@ def add_notification_to_queue(service_id, template_id, type_, notification): message_id = str(uuid.uuid4()) serializer = URLSafeSerializer(current_app.config.get('SECRET_KEY')) encrypted = serializer.dumps(notification, current_app.config.get('DANGEROUS_SALT')) + print("add_notification_to_queue") + print(type_) q.send_message(MessageBody=encrypted, MessageAttributes={'type': {'StringValue': type_, 'DataType': 'String'}, 'message_id': {'StringValue': message_id, 'DataType': 'String'}, From 409857fba645c3ca22d41721430f8ad15d140169 Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Mon, 8 Feb 2016 09:43:19 +0000 Subject: [PATCH 11/28] Now done. --- app/aws_sqs.py | 2 -- app/user/rest.py | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/app/aws_sqs.py b/app/aws_sqs.py index 1399bed80..86b28eab8 100644 --- a/app/aws_sqs.py +++ b/app/aws_sqs.py @@ -13,8 +13,6 @@ def add_notification_to_queue(service_id, template_id, type_, notification): message_id = str(uuid.uuid4()) serializer = URLSafeSerializer(current_app.config.get('SECRET_KEY')) encrypted = serializer.dumps(notification, current_app.config.get('DANGEROUS_SALT')) - print("add_notification_to_queue") - print(type_) q.send_message(MessageBody=encrypted, MessageAttributes={'type': {'StringValue': type_, 'DataType': 'String'}, 'message_id': {'StringValue': message_id, 'DataType': 'String'}, diff --git a/app/user/rest.py b/app/user/rest.py index 96eba04d0..05aef526d 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -149,7 +149,7 @@ def send_user_code(user_id): 'from_address': 'notify@digital.cabinet-office.gov.uk', 'subject': 'Verification code', 'body': secret_code} - add_notification_to_queue(api_user['client'], 'admin', 'sms', notification) + add_notification_to_queue(api_user['client'], 'admin', 'email', notification) notify_alpha_client.send_email( email, secret_code, From 877a8a0411f29555c3280ec724c514ccbf93d570 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 8 Feb 2016 11:10:54 +0000 Subject: [PATCH 12/28] Added logging for the authentication errors. Moved the "no api secret" error message to the end and only create it if there are no api client secrets --- app/authentication/auth.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/app/authentication/auth.py b/app/authentication/auth.py index 0f4100e6d..8172a91b1 100644 --- a/app/authentication/auth.py +++ b/app/authentication/auth.py @@ -1,10 +1,11 @@ -from flask import request, jsonify, _request_ctx_stack +from flask import request, jsonify, _request_ctx_stack, current_app from client.authentication import decode_jwt_token, get_token_issuer from client.errors import TokenDecodeError, TokenRequestError, TokenExpiredError, TokenPayloadError from app.dao.api_key_dao import get_unsigned_secrets def authentication_response(message, code): + current_app.logger.info(message) return jsonify( error=message ), code @@ -27,8 +28,8 @@ def requires_auth(): return authentication_response("Invalid token: signature", 403) if api_client is None: authentication_response("Invalid credentials", 403) - # If the api_client does not have any secrets return response saying that - errors_resp = authentication_response("Invalid token: api client has no secrets", 403) + + errors_resp = None for secret in api_client['secret']: try: decode_jwt_token( @@ -49,11 +50,14 @@ def requires_auth(): except TokenDecodeError: errors_resp = authentication_response("Invalid token: signature", 403) + if errors_resp is None: + # If we got this far with out any errors then the api client has no secrets + errors_resp = authentication_response("Invalid token: api client has no secrets", 403) + return errors_resp def fetch_client(client): - from flask import current_app if client == current_app.config.get('ADMIN_CLIENT_USER_NAME'): return { "client": client, From 416dd00ac82aba84ac6630db1647bf75cfa7bf6d Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 8 Feb 2016 11:29:53 +0000 Subject: [PATCH 13/28] Added a test for the case when there is no secret for the api client. Fix codestyle --- app/authentication/auth.py | 5 ----- .../app/authentication/test_authentication.py | 20 +++++++++++++++++++ 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/app/authentication/auth.py b/app/authentication/auth.py index 8172a91b1..8cf9e187c 100644 --- a/app/authentication/auth.py +++ b/app/authentication/auth.py @@ -29,7 +29,6 @@ def requires_auth(): if api_client is None: authentication_response("Invalid credentials", 403) - errors_resp = None for secret in api_client['secret']: try: decode_jwt_token( @@ -50,10 +49,6 @@ def requires_auth(): except TokenDecodeError: errors_resp = authentication_response("Invalid token: signature", 403) - if errors_resp is None: - # If we got this far with out any errors then the api client has no secrets - errors_resp = authentication_response("Invalid token: api client has no secrets", 403) - return errors_resp diff --git a/tests/app/authentication/test_authentication.py b/tests/app/authentication/test_authentication.py index ea6567a8a..74c67c4ef 100644 --- a/tests/app/authentication/test_authentication.py +++ b/tests/app/authentication/test_authentication.py @@ -214,6 +214,26 @@ def test_authentication_returns_token_expired_when_service_uses_expired_key_and_ assert data['error'] == 'Invalid token: signature' +def test_authentication_returns_error_when_api_client_has_no_secrets(notify_api, + notify_db, + notify_db_session): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + api_secret = notify_api.config.get('ADMIN_CLIENT_SECRET') + token = create_jwt_token(request_method="GET", + request_path=url_for('service.get_service'), + secret=api_secret, + client_id=notify_api.config.get('ADMIN_CLIENT_USER_NAME') + ) + notify_api.config['ADMIN_CLIENT_SECRET'] = '' + response = client.get(url_for('service.get_service'), + headers={'Authorization': 'Bearer {}'.format(token)}) + assert response.status_code == 403 + error_message = json.loads(response.get_data()) + assert error_message['error'] == 'Invalid token: signature' + notify_api.config['ADMIN_CLIENT_SECRET'] = api_secret + + def __create_get_token(service_id): if service_id: return create_jwt_token(request_method="GET", From 0580f5ab065d4768fb4764850b65d2b0f329afbb Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Mon, 8 Feb 2016 14:54:15 +0000 Subject: [PATCH 14/28] New endpoint for delivery app to use. Once removal of code that uses existing alpha is done, then duplicated code from /notifications/sms and the new endpoint can be merged. Job id is now avaiable in notificaiton but is not used yet. --- app/authentication/auth.py | 5 ++ app/notifications/rest.py | 32 ++++++++++ app/schemas.py | 1 + config.py | 11 ++-- tests/app/notifications/test_rest.py | 88 ++++++++++++++++++++++++++++ 5 files changed, 131 insertions(+), 6 deletions(-) diff --git a/app/authentication/auth.py b/app/authentication/auth.py index 8cf9e187c..e80e6f8a7 100644 --- a/app/authentication/auth.py +++ b/app/authentication/auth.py @@ -58,6 +58,11 @@ def fetch_client(client): "client": client, "secret": [current_app.config.get('ADMIN_CLIENT_SECRET')] } + elif client == current_app.config.get('DELIVERY_CLIENT_USER_NAME'): + return { + "client": client, + "secret": [current_app.config.get('DELIVERY_CLIENT_SECRET')] + } else: return { "client": client, diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 7e4889e9f..4781cd10b 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -1,3 +1,5 @@ +import uuid + from flask import ( Blueprint, jsonify, @@ -49,3 +51,33 @@ def create_email_notification(): notification['body'], notification['from_address'], notification['subject'])) + + +@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 + + # Actual client is delivery app, but this is sent on behalf of service + add_notification_to_queue(service_id, template_id, 'sms', notification) + + # TODO: remove once beta is reading notifications from the queue + content = template.content + return jsonify(notify_alpha_client.send_sms( + mobile_number=notification['to'], message=content)), 200 diff --git a/app/schemas.py b/app/schemas.py index 981924765..7d4e329df 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -90,6 +90,7 @@ class SmsNotificationSchema(NotificationSchema): class SmsTemplateNotificationSchema(SmsNotificationSchema): template = fields.Int(required=True) + job = fields.String() @validates('template') def validate_template(self, value): diff --git a/config.py b/config.py index 8329e0edf..8f728f1e1 100644 --- a/config.py +++ b/config.py @@ -13,6 +13,8 @@ class Config(object): NOTIFY_DATA_API_AUTH_TOKEN = os.getenv('NOTIFY_API_TOKEN', "dev-token") ADMIN_CLIENT_USER_NAME = None ADMIN_CLIENT_SECRET = None + DELIVERY_CLIENT_USER_NAME = None + DELIVERY_CLIENT_SECRET = None AWS_REGION = 'eu-west-1' NOTIFY_JOB_QUEUE = os.getenv('NOTIFY_JOB_QUEUE', 'notify-jobs-queue') @@ -26,15 +28,12 @@ class Development(Config): DANGEROUS_SALT = 'dangerous-salt' ADMIN_CLIENT_USER_NAME = 'dev-notify-admin' ADMIN_CLIENT_SECRET = 'dev-notify-secret-key' + DELIVERY_CLIENT_USER_NAME = 'dev-notify-delivery' + DELIVERY_CLIENT_SECRET = 'dev-notify-secret-key' -class Test(Config): - DEBUG = True +class Test(Development): SQLALCHEMY_DATABASE_URI = 'postgresql://localhost/test_notification_api' - SECRET_KEY = 'secret-key' - DANGEROUS_SALT = 'dangerous-salt' - ADMIN_CLIENT_USER_NAME = 'dev-notify-admin' - ADMIN_CLIENT_SECRET = 'dev-notify-secret-key' class Live(Config): diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 4595e755f..fb0f2aa8f 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -1,4 +1,5 @@ import moto +import uuid from tests import create_authorization_header from flask import url_for, json @@ -320,3 +321,90 @@ def test_send_email_valid_data(notify_api, assert json_resp['notification']['id'] == 100 notify_alpha_client.send_email.assert_called_with( to_address, message, from_address, subject) + + +@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: + mocker.patch( + 'app.notify_alpha_client.send_sms', + return_value={ + "notification": { + "createdAt": "2015-11-03T09:37:27.414363Z", + "id": 100, + "jobId": 65, + "message": "valid", + "method": "sms", + "status": "created", + "to": sample_user.mobile_number + } + } + ) + job_id = uuid.uuid4() + service_id = sample_template.service.id + url = url_for('notifications.create_sms_for_service', service_id=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 == 200 + assert json_resp['notification']['id'] == 100 + notify_alpha_client.send_sms.assert_called_with(mobile_number='+441234123123', + message=sample_template.content) + + +@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 = url_for('notifications.create_sms_for_service', service_id=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 2fda7ee59bf3e1fdac50f775ce9bd7e555e6e7fc Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Tue, 9 Feb 2016 11:38:57 +0000 Subject: [PATCH 15/28] Alpha client removed from code. Tests fixed but will wait till other notifications jobs are done before creating a pull request. --- app/notifications/rest.py | 30 +++----- app/user/rest.py | 10 --- requirements.txt | 2 - tests/app/conftest.py | 18 ----- tests/app/notifications/test_rest.py | 105 --------------------------- tests/app/user/test_rest_verify.py | 16 ---- 6 files changed, 9 insertions(+), 172 deletions(-) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 4781cd10b..925910bdb 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -17,7 +17,8 @@ notifications = Blueprint('notifications', __name__) @notifications.route('/', methods=['GET']) def get_notifications(notification_id): - return jsonify(notify_alpha_client.fetch_notification_by_id(notification_id)), 200 + # TODO return notification id details + return jsonify({'id': notification_id}), 200 @notifications.route('/sms', methods=['POST']) @@ -27,14 +28,10 @@ def create_sms_notification(): notification, errors = sms_template_notification_schema.load(resp_json) if errors: return jsonify(result="error", message=errors), 400 - template_id = notification['template'] - # TODO: remove once beta is reading notifications from the queue - message = templates_dao.get_model_templates(template_id).content - add_notification_to_queue(api_user['client'], template_id, 'sms', notification) - # TODO: remove once beta is reading notifications from the queue - return jsonify(notify_alpha_client.send_sms( - mobile_number=notification['to'], message=message)), 200 + add_notification_to_queue(api_user['client'], notification['template'], 'sms', notification) + # TODO data to be returned + return jsonify({}), 200 @notifications.route('/email', methods=['POST']) @@ -43,14 +40,9 @@ def create_email_notification(): notification, errors = email_notification_schema.load(resp_json) if errors: return jsonify(result="error", message=errors), 400 - # At the moment we haven't hooked up - # template handling for sending email notifications. add_notification_to_queue(api_user['client'], "admin", 'email', notification) - return jsonify(notify_alpha_client.send_email( - notification['to_address'], - notification['body'], - notification['from_address'], - notification['subject'])) + # TODO data to be returned + return jsonify({}), 200 @notifications.route('/sms/service/', methods=['POST']) @@ -74,10 +66,6 @@ def create_sms_for_service(service_id): message = "Invalid template: id {} for service id: {}".format(template.id, service_id) return jsonify(result="error", message=message), 400 - # Actual client is delivery app, but this is sent on behalf of service add_notification_to_queue(service_id, template_id, 'sms', notification) - - # TODO: remove once beta is reading notifications from the queue - content = template.content - return jsonify(notify_alpha_client.send_sms( - mobile_number=notification['to'], message=content)), 200 + # TODO data to be returned + return jsonify({}), 200 diff --git a/app/user/rest.py b/app/user/rest.py index 05aef526d..02d46732d 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -133,15 +133,10 @@ def send_user_code(user_id): from app.dao.users_dao import create_secret_code secret_code = create_secret_code() create_user_code(user, secret_code, verify_code.get('code_type')) - # TODO this will need to fixed up when we stop using - # notify_alpha_client if verify_code.get('code_type') == 'sms': mobile = user.mobile_number if verify_code.get('to', None) is None else verify_code.get('to') notification = {'to': mobile, 'content': secret_code} add_notification_to_queue(api_user['client'], 'admin', 'sms', notification) - notify_alpha_client.send_sms( - mobile_number=mobile, - message=secret_code) elif verify_code.get('code_type') == 'email': email = user.email_address if verify_code.get('to', None) is None else verify_code.get('to') notification = { @@ -150,11 +145,6 @@ def send_user_code(user_id): 'subject': 'Verification code', 'body': secret_code} add_notification_to_queue(api_user['client'], 'admin', 'email', notification) - notify_alpha_client.send_email( - email, - secret_code, - notification['from_address'], - notification['subject']) else: abort(500) return jsonify({}), 204 diff --git a/requirements.txt b/requirements.txt index 96d3663b6..8c297ceea 100644 --- a/requirements.txt +++ b/requirements.txt @@ -17,5 +17,3 @@ boto3==1.2.3 git+https://github.com/alphagov/notifications-python-client.git@0.2.1#egg=notifications-python-client==0.2.1 git+https://github.com/alphagov/notifications-utils.git@0.0.3#egg=notifications-utils==0.0.3 - -git+https://github.com/alphagov/notify-api-client.git@0.1.6#egg=notify-api-client==0.1.6 diff --git a/tests/app/conftest.py b/tests/app/conftest.py index e1abea888..338e18022 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -155,24 +155,6 @@ def sample_admin_service_id(notify_db, notify_db_session): return admin_service.id -@pytest.fixture(scope='function') -def mock_notify_client_send_sms(mocker): - def _send(mobile_number, message): - pass - - mock_class = mocker.patch('app.notify_alpha_client.send_sms', side_effect=_send) - return mock_class - - -@pytest.fixture(scope='function') -def mock_notify_client_send_email(mocker): - def _send(email_address, message, from_address, subject): - pass - - mock_class = mocker.patch('app.notify_alpha_client.send_email', side_effect=_send) - return mock_class - - @pytest.fixture(scope='function') def mock_secret_code(mocker): def _create(): diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index fb0f2aa8f..5ce297fe2 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -14,18 +14,6 @@ def test_get_notifications( """ with notify_api.test_request_context(): with notify_api.test_client() as client: - mocker.patch( - 'app.notify_alpha_client.fetch_notification_by_id', - return_value={ - 'notifications': [ - { - 'id': 'my_id', - 'notification': 'some notify' - } - ] - } - ) - auth_header = create_authorization_header( service_id=sample_api_key.service_id, path=url_for('notifications.get_notifications', notification_id=123), @@ -35,12 +23,7 @@ def test_get_notifications( url_for('notifications.get_notifications', notification_id=123), headers=[auth_header]) - json_resp = json.loads(response.get_data(as_text=True)) assert response.status_code == 200 - assert len(json_resp['notifications']) == 1 - assert json_resp['notifications'][0]['id'] == 'my_id' - assert json_resp['notifications'][0]['notification'] == 'some notify' - notify_alpha_client.fetch_notification_by_id.assert_called_with("123") def test_get_notifications_empty_result( @@ -50,14 +33,6 @@ def test_get_notifications_empty_result( """ with notify_api.test_request_context(): with notify_api.test_client() as client: - mocker.patch( - 'app.notify_alpha_client.fetch_notification_by_id', - return_value={ - 'notifications': [ - ] - } - ) - auth_header = create_authorization_header( service_id=sample_api_key.service_id, path=url_for('notifications.get_notifications', notification_id=123), @@ -67,10 +42,7 @@ def test_get_notifications_empty_result( url_for('notifications.get_notifications', notification_id=123), headers=[auth_header]) - json_resp = json.loads(response.get_data(as_text=True)) assert response.status_code == 200 - assert len(json_resp['notifications']) == 0 - notify_alpha_client.fetch_notification_by_id.assert_called_with("123") def test_create_sms_should_reject_if_no_phone_numbers( @@ -80,10 +52,6 @@ def test_create_sms_should_reject_if_no_phone_numbers( """ with notify_api.test_request_context(): with notify_api.test_client() as client: - mocker.patch( - 'app.notify_alpha_client.send_sms', - return_value='success' - ) data = { 'template': "my message" } @@ -102,7 +70,6 @@ def test_create_sms_should_reject_if_no_phone_numbers( assert response.status_code == 400 assert json_resp['result'] == 'error' assert 'Missing data for required field.' in json_resp['message']['to'][0] - assert not notify_alpha_client.send_sms.called def test_should_reject_bad_phone_numbers( @@ -112,10 +79,6 @@ def test_should_reject_bad_phone_numbers( """ with notify_api.test_request_context(): with notify_api.test_client() as client: - mocker.patch( - 'app.notify_alpha_client.send_sms', - return_value='success' - ) data = { 'to': 'invalid', 'template': "my message" @@ -134,7 +97,6 @@ def test_should_reject_bad_phone_numbers( assert response.status_code == 400 assert json_resp['result'] == 'error' assert 'Invalid phone number, must be of format +441234123123' in json_resp['message']['to'] - assert not notify_alpha_client.send_sms.called def test_send_notification_restrict_mobile(notify_api, @@ -150,14 +112,9 @@ def test_send_notification_restrict_mobile(notify_api, """ with notify_api.test_request_context(): with notify_api.test_client() as client: - Service.query.filter_by( id=sample_template.service.id).update({'restricted': True}) invalid_mob = '+449999999999' - mocker.patch( - 'app.notify_alpha_client.send_sms', - return_value={} - ) data = { 'to': invalid_mob, 'template': sample_template.id @@ -177,7 +134,6 @@ def test_send_notification_restrict_mobile(notify_api, json_resp = json.loads(response.get_data(as_text=True)) assert response.status_code == 400 assert 'Invalid phone number for restricted service' in json_resp['message']['restricted'] - assert not notify_alpha_client.send_sms.called def test_send_notification_invalid_template_id(notify_api, @@ -196,10 +152,6 @@ def test_send_notification_invalid_template_id(notify_api, Service.query.filter_by( id=sample_template.service.id).update({'restricted': True}) invalid_mob = '+449999999999' - mocker.patch( - 'app.notify_alpha_client.send_sms', - return_value={} - ) data = { 'to': invalid_mob, 'template': 9999 @@ -219,7 +171,6 @@ def test_send_notification_invalid_template_id(notify_api, json_resp = json.loads(response.get_data(as_text=True)) assert response.status_code == 400 assert 'Template not found' in json_resp['message']['template'] - assert not notify_alpha_client.send_sms.called @moto.mock_sqs @@ -235,20 +186,6 @@ def test_should_allow_valid_message(notify_api, """ with notify_api.test_request_context(): with notify_api.test_client() as client: - mocker.patch( - 'app.notify_alpha_client.send_sms', - return_value={ - "notification": { - "createdAt": "2015-11-03T09:37:27.414363Z", - "id": 100, - "jobId": 65, - "message": "valid", - "method": "sms", - "status": "created", - "to": sample_user.mobile_number - } - } - ) data = { 'to': '+441234123123', 'template': sample_template.id @@ -263,11 +200,7 @@ def test_should_allow_valid_message(notify_api, 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 == 200 - assert json_resp['notification']['id'] == 100 - notify_alpha_client.send_sms.assert_called_with(mobile_number='+441234123123', - message=sample_template.content) @moto.mock_sqs @@ -284,22 +217,6 @@ def test_send_email_valid_data(notify_api, from_address = "from@notify.com" subject = "This is the subject" message = "This is the message" - mocker.patch( - 'app.notify_alpha_client.send_email', - return_value={ - "notification": { - "createdAt": "2015-11-03T09:37:27.414363Z", - "id": 100, - "jobId": 65, - "subject": subject, - "message": message, - "method": "email", - "status": "created", - "to": to_address, - "from": from_address - } - } - ) data = { 'to': to_address, 'from': from_address, @@ -316,11 +233,7 @@ def test_send_email_valid_data(notify_api, 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 == 200 - assert json_resp['notification']['id'] == 100 - notify_alpha_client.send_email.assert_called_with( - to_address, message, from_address, subject) @moto.mock_sqs @@ -333,20 +246,6 @@ def test_valid_message_with_service_id(notify_api, mocker): with notify_api.test_request_context(): with notify_api.test_client() as client: - mocker.patch( - 'app.notify_alpha_client.send_sms', - return_value={ - "notification": { - "createdAt": "2015-11-03T09:37:27.414363Z", - "id": 100, - "jobId": 65, - "message": "valid", - "method": "sms", - "status": "created", - "to": sample_user.mobile_number - } - } - ) job_id = uuid.uuid4() service_id = sample_template.service.id url = url_for('notifications.create_sms_for_service', service_id=service_id) @@ -365,11 +264,7 @@ def test_valid_message_with_service_id(notify_api, 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 == 200 - assert json_resp['notification']['id'] == 100 - notify_alpha_client.send_sms.assert_called_with(mobile_number='+441234123123', - message=sample_template.content) @moto.mock_sqs diff --git a/tests/app/user/test_rest_verify.py b/tests/app/user/test_rest_verify.py index b52a4dc8e..a58fa6a1a 100644 --- a/tests/app/user/test_rest_verify.py +++ b/tests/app/user/test_rest_verify.py @@ -253,7 +253,6 @@ def test_send_user_code_for_sms(notify_api, notify_db_session, sample_sms_code, sqs_client_conn, - mock_notify_client_send_sms, mock_secret_code): """ Tests POST endpoint '//code' successful sms @@ -271,8 +270,6 @@ def test_send_user_code_for_sms(notify_api, headers=[('Content-Type', 'application/json'), auth_header]) assert resp.status_code == 204 - mock_notify_client_send_sms.assert_called_once_with(mobile_number=sample_sms_code.user.mobile_number, - message='11111') @moto.mock_sqs @@ -281,7 +278,6 @@ def test_send_user_code_for_sms_with_optional_to_field(notify_api, notify_db_session, sample_sms_code, sqs_client_conn, - mock_notify_client_send_sms, mock_secret_code): """ Tests POST endpoint '//code' successful sms with optional to field @@ -299,8 +295,6 @@ def test_send_user_code_for_sms_with_optional_to_field(notify_api, headers=[('Content-Type', 'application/json'), auth_header]) assert resp.status_code == 204 - mock_notify_client_send_sms.assert_called_once_with(mobile_number='+441119876757', - message='11111') @moto.mock_sqs @@ -309,7 +303,6 @@ def test_send_user_code_for_email(notify_api, notify_db_session, sample_email_code, sqs_client_conn, - mock_notify_client_send_email, mock_secret_code): """ Tests POST endpoint '//code' successful email @@ -326,10 +319,6 @@ def test_send_user_code_for_email(notify_api, data=data, headers=[('Content-Type', 'application/json'), auth_header]) assert resp.status_code == 204 - mock_notify_client_send_email.assert_called_once_with(sample_email_code.user.email_address, - '11111', - 'notify@digital.cabinet-office.gov.uk', - 'Verification code') @moto.mock_sqs @@ -338,7 +327,6 @@ def test_send_user_code_for_email_uses_optional_to_field(notify_api, notify_db_session, sample_email_code, sqs_client_conn, - mock_notify_client_send_email, mock_secret_code): """ Tests POST endpoint '//code' successful email with included in body @@ -355,10 +343,6 @@ def test_send_user_code_for_email_uses_optional_to_field(notify_api, data=data, headers=[('Content-Type', 'application/json'), auth_header]) assert resp.status_code == 204 - mock_notify_client_send_email.assert_called_once_with('different@email.gov.uk', - '11111', - 'notify@digital.cabinet-office.gov.uk', - 'Verification code') def test_request_verify_code_schema_invalid_code_type(notify_api, notify_db, notify_db_session, sample_user): From c7121be5a2c59f62b1baad62db1b93ed22897584 Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Tue, 9 Feb 2016 12:01:17 +0000 Subject: [PATCH 16/28] [WIP] New model class and dao for notification. This will be used for recording status and outcome of sending notifications. --- app/dao/notifications_dao.py | 22 ++++++ app/models.py | 29 ++++++++ migrations/versions/0013_add_notifications.py | 47 ++++++++++++ tests/app/conftest.py | 31 +++++++- tests/app/dao/test_notification_dao.py | 74 +++++++++++++++++++ 5 files changed, 202 insertions(+), 1 deletion(-) create mode 100644 app/dao/notifications_dao.py create mode 100644 migrations/versions/0013_add_notifications.py create mode 100644 tests/app/dao/test_notification_dao.py diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py new file mode 100644 index 000000000..be4e56ef6 --- /dev/null +++ b/app/dao/notifications_dao.py @@ -0,0 +1,22 @@ +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_id', None) + update_dict.pop('template_id', None) + Notification.query.filter_by(id=notification.id).update(update_dict) + else: + db.session.add(notification) + db.session.commit() + + +def get_notification(job_id, notification_id): + return Notification.query.filter_by(job_id=job_id, id=notification_id).one() + + +def get_notifications_by_job(job_id): + return Notification.query.filter_by(job_id=job_id).all() diff --git a/app/models.py b/app/models.py index dc260836b..22337a6a5 100644 --- a/app/models.py +++ b/app/models.py @@ -189,3 +189,32 @@ class VerifyCode(db.Model): def check_code(self, cde): return check_hash(cde, self._code) + + +NOTIFICATION_STATUS_TYPES = ['sent', 'failed'] + + +class Notification(db.Model): + + __tablename__ = 'notifications' + + id = db.Column(UUID(as_uuid=True), primary_key=True) + to = db.Column(db.String, nullable=False) + job_id = db.Column(UUID(as_uuid=True), db.ForeignKey('jobs.id'), index=True, unique=False, nullable=False) + job = db.relationship('Job', backref=db.backref('notifications', lazy='dynamic')) + service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, unique=False) + template_id = db.Column(db.BigInteger, db.ForeignKey('templates.id'), index=True, unique=False) + created_at = db.Column( + db.DateTime, + index=False, + unique=False, + nullable=False, + default=datetime.datetime.now) + updated_at = db.Column( + db.DateTime, + index=False, + unique=False, + nullable=True, + onupdate=datetime.datetime.now) + status = db.Column( + db.Enum(*NOTIFICATION_STATUS_TYPES, name='notification_status_types'), nullable=False, default='sent') diff --git a/migrations/versions/0013_add_notifications.py b/migrations/versions/0013_add_notifications.py new file mode 100644 index 000000000..61fd55a66 --- /dev/null +++ b/migrations/versions/0013_add_notifications.py @@ -0,0 +1,47 @@ +"""empty message + +Revision ID: 0013_add_notifications +Revises: 0012_add_status_to_job +Create Date: 2016-02-09 11:14:46.708551 + +""" + +# revision identifiers, used by Alembic. +revision = '0013_add_notifications' +down_revision = '0012_add_status_to_job' + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +def upgrade(): + ### commands auto generated by Alembic - please adjust! ### + op.create_table('notifications', + sa.Column('id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('to', sa.String(), nullable=False), + sa.Column('job_id', postgresql.UUID(as_uuid=True), nullable=False), + sa.Column('service_id', postgresql.UUID(as_uuid=True), nullable=True), + sa.Column('template_id', sa.BigInteger(), nullable=True), + sa.Column('created_at', sa.DateTime(), nullable=False), + sa.Column('updated_at', sa.DateTime(), nullable=True), + sa.Column('status', sa.Enum('sent', 'failed', name='notification_status_types'), nullable=False), + sa.ForeignKeyConstraint(['job_id'], ['jobs.id'], ), + sa.ForeignKeyConstraint(['service_id'], ['services.id'], ), + sa.ForeignKeyConstraint(['template_id'], ['templates.id'], ), + sa.PrimaryKeyConstraint('id') + ) + op.create_index(op.f('ix_notifications_job_id'), 'notifications', ['job_id'], unique=False) + op.create_index(op.f('ix_notifications_service_id'), 'notifications', ['service_id'], unique=False) + op.create_index(op.f('ix_notifications_template_id'), 'notifications', ['template_id'], unique=False) + ### end Alembic commands ### + + +def downgrade(): + ### commands auto generated by Alembic - please adjust! ### + op.drop_index(op.f('ix_notifications_template_id'), table_name='notifications') + op.drop_index(op.f('ix_notifications_service_id'), table_name='notifications') + op.drop_index(op.f('ix_notifications_job_id'), table_name='notifications') + op.drop_table('notifications') + op.get_bind() + op.execute("drop type notification_status_types") + ### end Alembic commands ### diff --git a/tests/app/conftest.py b/tests/app/conftest.py index e1abea888..741d896eb 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -1,12 +1,13 @@ import pytest from flask import jsonify -from app.models import (User, Service, Template, ApiKey, Job, VerifyCode) +from app.models import (User, Service, Template, ApiKey, Job, VerifyCode, Notification) from app.dao.users_dao import (save_model_user, create_user_code, create_secret_code) from app.dao.services_dao import save_model_service from app.dao.templates_dao import save_model_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 import uuid @@ -180,3 +181,31 @@ def mock_secret_code(mocker): mock_class = mocker.patch('app.dao.users_dao.create_secret_code', side_effect=_create) return mock_class + + +@pytest.fixture(scope='function') +def sample_notification(notify_db, + notify_db_session, + service=None, + template=None, + job=None): + if service is None: + service = sample_service(notify_db, notify_db_session) + if template is None: + template = sample_template(notify_db, notify_db_session, service=service) + if job is None: + job = sample_job(notify_db, notify_db_session, service=service, template=template) + + notificaton_id = uuid.uuid4() + to = '+44709123456' + + data = { + 'id': notificaton_id, + 'to': to, + 'job_id': job.id, + 'service_id': service.id, + 'template_id': template.id + } + notification = Notification(**data) + save_notification(notification) + return notification diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py new file mode 100644 index 000000000..23ff6fe23 --- /dev/null +++ b/tests/app/dao/test_notification_dao.py @@ -0,0 +1,74 @@ +import uuid + +from app.models import Notification + +from app.dao.notifications_dao import ( + save_notification, + get_notification, + get_notifications_by_job +) + + +def test_save_notification(notify_db, notify_db_session, sample_template, sample_job): + + assert Notification.query.count() == 0 + + notification_id = uuid.uuid4() + to = '+44709123456' + job_id = sample_job.id + data = { + 'id': notification_id, + 'to': to, + 'job_id': job_id, + 'service_id': sample_template.service.id, + 'template_id': sample_template.id + } + + notification = Notification(**data) + save_notification(notification) + + assert Notification.query.count() == 1 + + notification_from_db = Notification.query.get(notification_id) + + assert data['id'] == notification_from_db.id + assert data['to'] == notification_from_db.to + assert data['job_id'] == notification_from_db.job_id + assert data['service_id'] == notification_from_db.service_id + assert data['template_id'] == notification_from_db.template_id + assert 'sent' == notification_from_db.status + + +def test_get_notification_for_job(notify_db, notify_db_session, sample_notification): + notifcation_from_db = get_notification(sample_notification.job_id, sample_notification.id) + assert sample_notification == notifcation_from_db + + +def test_get_all_notifications_for_job(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) + + notifcations_from_db = get_notifications_by_job(sample_job.id) + assert len(notifcations_from_db) == 5 + + +def test_update_notification(notify_db, notify_db_session, sample_notification): + assert sample_notification.status == 'sent' + + update_dict = { + 'id': sample_notification.id, + 'service_id': sample_notification.service_id, + 'template_id': sample_notification.template_id, + 'job': sample_notification.job, + 'status': 'failed' + } + + save_notification(sample_notification, update_dict=update_dict) + notification_from_db = Notification.query.get(sample_notification.id) + assert notification_from_db.status == 'failed' From e5e049d73564a9035265ec6eb142da92a9e37ea9 Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Tue, 9 Feb 2016 12:48:27 +0000 Subject: [PATCH 17/28] Added service and template relationship to notification model. This makes it more consistent with other model classes with respect to marhmallow serialisation/deserialisation. --- app/dao/notifications_dao.py | 4 ++-- app/models.py | 2 ++ tests/app/conftest.py | 6 +++--- tests/app/dao/test_notification_dao.py | 17 ++++++++--------- 4 files changed, 15 insertions(+), 14 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index be4e56ef6..8c42ac6ca 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -6,8 +6,8 @@ def save_notification(notification, update_dict={}): if update_dict: update_dict.pop('id', None) update_dict.pop('job', None) - update_dict.pop('service_id', None) - update_dict.pop('template_id', 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) diff --git a/app/models.py b/app/models.py index 22337a6a5..1ce4a7dbf 100644 --- a/app/models.py +++ b/app/models.py @@ -203,7 +203,9 @@ class Notification(db.Model): job_id = db.Column(UUID(as_uuid=True), db.ForeignKey('jobs.id'), index=True, unique=False, nullable=False) job = db.relationship('Job', backref=db.backref('notifications', lazy='dynamic')) service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, unique=False) + service = db.relationship('Service') template_id = db.Column(db.BigInteger, db.ForeignKey('templates.id'), index=True, unique=False) + template = db.relationship('Template') created_at = db.Column( db.DateTime, index=False, diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 741d896eb..0f6a33461 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -202,9 +202,9 @@ def sample_notification(notify_db, data = { 'id': notificaton_id, 'to': to, - 'job_id': job.id, - 'service_id': service.id, - 'template_id': template.id + 'job': job, + 'service': service, + 'template': template } notification = Notification(**data) save_notification(notification) diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 23ff6fe23..7d1218e74 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -15,13 +15,12 @@ def test_save_notification(notify_db, notify_db_session, sample_template, sample notification_id = uuid.uuid4() to = '+44709123456' - job_id = sample_job.id data = { 'id': notification_id, 'to': to, - 'job_id': job_id, - 'service_id': sample_template.service.id, - 'template_id': sample_template.id + 'job': sample_job, + 'service': sample_template.service, + 'template': sample_template } notification = Notification(**data) @@ -33,9 +32,9 @@ def test_save_notification(notify_db, notify_db_session, sample_template, sample assert data['id'] == notification_from_db.id assert data['to'] == notification_from_db.to - assert data['job_id'] == notification_from_db.job_id - assert data['service_id'] == notification_from_db.service_id - assert data['template_id'] == notification_from_db.template_id + assert data['job'] == notification_from_db.job + assert data['service'] == notification_from_db.service + assert data['template'] == notification_from_db.template assert 'sent' == notification_from_db.status @@ -63,8 +62,8 @@ def test_update_notification(notify_db, notify_db_session, sample_notification): update_dict = { 'id': sample_notification.id, - 'service_id': sample_notification.service_id, - 'template_id': sample_notification.template_id, + 'service': sample_notification.service, + 'template': sample_notification.template, 'job': sample_notification.job, 'status': 'failed' } From c2424d650996f25ae8f3c05488cdb46f1c63f945 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 9 Feb 2016 13:41:20 +0000 Subject: [PATCH 18/28] Added code to read environemnt from a file If file does not exist default to live config. --- wsgi.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/wsgi.py b/wsgi.py index 138a48c6f..fd773b3e0 100644 --- a/wsgi.py +++ b/wsgi.py @@ -1,9 +1,17 @@ from app import create_app from credstash import getAllSecrets +import os + +config = 'live' +default_env_file = '/home/ubuntu/environment' + +if os.path.isfile(default_env_file): + environment = open(default_env_file, 'r') + config = environment.readline().strip() secrets = getAllSecrets(region="eu-west-1") -application = create_app('live', secrets) +application = create_app(config, secrets) if __name__ == "__main__": application.run() From 66763a061c18f4598cd04a5166b8137c92d4fcd8 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 9 Feb 2016 13:56:11 +0000 Subject: [PATCH 19/28] Added preview and staging config blocks --- config.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/config.py b/config.py index 8f728f1e1..6ec03eac7 100644 --- a/config.py +++ b/config.py @@ -36,12 +36,22 @@ class Test(Development): SQLALCHEMY_DATABASE_URI = 'postgresql://localhost/test_notification_api' +class Preview(Config): + pass + + +class Staging(Config): + pass + + class Live(Config): pass configs = { 'development': Development, + 'preview': Preview, + 'staging': Staging, 'test': Test, 'live': Live, } From 38e30034d1171203446438788683d8344d23755b Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Tue, 9 Feb 2016 14:07:43 +0000 Subject: [PATCH 20/28] updated environment variables. --- config.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/config.py b/config.py index 6ec03eac7..17639d51f 100644 --- a/config.py +++ b/config.py @@ -30,22 +30,24 @@ class Development(Config): ADMIN_CLIENT_SECRET = 'dev-notify-secret-key' DELIVERY_CLIENT_USER_NAME = 'dev-notify-delivery' DELIVERY_CLIENT_SECRET = 'dev-notify-secret-key' + NOTIFICATION_QUEUE_PREFIX = 'notification_development' class Test(Development): SQLALCHEMY_DATABASE_URI = 'postgresql://localhost/test_notification_api' + NOTIFICATION_QUEUE_PREFIX = 'notification_test' class Preview(Config): - pass + NOTIFICATION_QUEUE_PREFIX = 'notification_preview' class Staging(Config): - pass + NOTIFICATION_QUEUE_PREFIX = 'notification_staging' class Live(Config): - pass + NOTIFICATION_QUEUE_PREFIX = 'notification_live' configs = { From 17e5e70f6c746a4e81a2287b6137b9a748c9dce3 Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Tue, 9 Feb 2016 14:17:42 +0000 Subject: [PATCH 21/28] [WIP] Added endpoints under /job for creating, updating and reading notification status. --- app/dao/notifications_dao.py | 8 +- app/job/rest.py | 61 +++++++++++- app/schemas.py | 9 ++ tests/app/dao/test_notification_dao.py | 16 ++-- tests/app/job/test_job_rest.py | 124 +++++++++++++++++++++++++ 5 files changed, 205 insertions(+), 13 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 8c42ac6ca..ed3cd0f6d 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -14,9 +14,9 @@ def save_notification(notification, update_dict={}): db.session.commit() -def get_notification(job_id, notification_id): - return Notification.query.filter_by(job_id=job_id, id=notification_id).one() +def get_notification(service_id, job_id, notification_id): + return Notification.query.filter_by(service_id=service_id, job_id=job_id, id=notification_id).one() -def get_notifications_by_job(job_id): - return Notification.query.filter_by(job_id=job_id).all() +def get_notifications(service_id, job_id): + return Notification.query.filter_by(service_id=service_id, job_id=job_id).all() diff --git a/app/job/rest.py b/app/job/rest.py index 2caaa00d9..17f1e341c 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -8,7 +8,6 @@ from flask import ( current_app ) -from sqlalchemy.exc import SQLAlchemyError from sqlalchemy.exc import DataError from sqlalchemy.orm.exc import NoResultFound @@ -18,10 +17,19 @@ from app.dao.jobs_dao import ( get_jobs_by_service ) +from app.dao.notifications_dao import ( + save_notification, + get_notification, + get_notifications +) + from app.schemas import ( job_schema, jobs_schema, - job_schema_load_json + job_schema_load_json, + notification_status_schema, + notifications_status_schema, + notification_status_schema_load_json ) job = Blueprint('job', __name__, url_prefix='/service//job') @@ -72,6 +80,55 @@ def update_job(service_id, job_id): 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 + try: + save_notification(notification) + except Exception as e: + return jsonify(result="error", message=str(e)), 500 + 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 = get_notification(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 = get_notifications(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 = get_notification(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 + try: + save_notification(notification, update_dict=update_dict) + except Exception as e: + return jsonify(result="error", message=str(e)), 400 + + 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'] diff --git a/app/schemas.py b/app/schemas.py index 7d4e329df..3a0ab41df 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -129,6 +129,12 @@ class EmailNotificationSchema(NotificationSchema): body = fields.Str(load_from="message", dump_to='message', required=True) +class NotificationStatusSchema(BaseSchema): + + class Meta: + model = models.Notification + + user_schema = UserSchema() user_schema_load_json = UserSchema(load_json=True) users_schema = UserSchema(many=True) @@ -148,3 +154,6 @@ request_verify_code_schema = RequestVerifyCodeSchema() sms_admin_notification_schema = SmsAdminNotificationSchema() sms_template_notification_schema = SmsTemplateNotificationSchema() email_notification_schema = EmailNotificationSchema() +notification_status_schema = NotificationStatusSchema() +notifications_status_schema = NotificationStatusSchema(many=True) +notification_status_schema_load_json = NotificationStatusSchema(load_json=True) diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 7d1218e74..cf1e70a6f 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -5,7 +5,7 @@ from app.models import Notification from app.dao.notifications_dao import ( save_notification, get_notification, - get_notifications_by_job + get_notifications ) @@ -39,7 +39,9 @@ def test_save_notification(notify_db, notify_db_session, sample_template, sample def test_get_notification_for_job(notify_db, notify_db_session, sample_notification): - notifcation_from_db = get_notification(sample_notification.job_id, sample_notification.id) + notifcation_from_db = get_notification(sample_notification.service.id, + sample_notification.job_id, + sample_notification.id) assert sample_notification == notifcation_from_db @@ -53,7 +55,7 @@ def test_get_all_notifications_for_job(notify_db, notify_db_session, sample_job) template=sample_job.template, job=sample_job) - notifcations_from_db = get_notifications_by_job(sample_job.id) + notifcations_from_db = get_notifications(sample_job.service.id, sample_job.id) assert len(notifcations_from_db) == 5 @@ -61,10 +63,10 @@ def test_update_notification(notify_db, notify_db_session, sample_notification): assert sample_notification.status == 'sent' update_dict = { - 'id': sample_notification.id, - 'service': sample_notification.service, - 'template': sample_notification.template, - 'job': sample_notification.job, + 'id': str(sample_notification.id), + 'service': str(sample_notification.service.id), + 'template': sample_notification.template.id, + 'job': str(sample_notification.job.id), 'status': 'failed' } diff --git a/tests/app/job/test_job_rest.py b/tests/app/job/test_job_rest.py index 20b520e1b..d16e2ac8f 100644 --- a/tests/app/job/test_job_rest.py +++ b/tests/app/job/test_job_rest.py @@ -163,6 +163,130 @@ def test_get_update_job_status(notify_api, 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): + + notificaton_id = uuid.uuid4() + to = '+44709123456' + data = { + 'id': str(notificaton_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 data['id'] == 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_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, From 68b6444eed6f3a748aa3a0126c20a52a5d56e6f9 Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Tue, 9 Feb 2016 16:02:38 +0000 Subject: [PATCH 22/28] Comment added for missing code. --- app/notifications/rest.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 925910bdb..84b119d46 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -31,7 +31,7 @@ def create_sms_notification(): add_notification_to_queue(api_user['client'], notification['template'], 'sms', notification) # TODO data to be returned - return jsonify({}), 200 + return jsonify({}), 204 @notifications.route('/email', methods=['POST']) @@ -42,7 +42,7 @@ def create_email_notification(): return jsonify(result="error", message=errors), 400 add_notification_to_queue(api_user['client'], "admin", 'email', notification) # TODO data to be returned - return jsonify({}), 200 + return jsonify({}), 204 @notifications.route('/sms/service/', methods=['POST']) @@ -68,4 +68,4 @@ def create_sms_for_service(service_id): add_notification_to_queue(service_id, template_id, 'sms', notification) # TODO data to be returned - return jsonify({}), 200 + return jsonify({}), 204 From 09d2f0d79d6bfede616ca4d4fb9bd7bfa1c82b0c Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Tue, 9 Feb 2016 16:04:49 +0000 Subject: [PATCH 23/28] Fix tests. --- tests/app/notifications/test_rest.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 5ce297fe2..1715c7a12 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -200,7 +200,7 @@ def test_should_allow_valid_message(notify_api, data=json.dumps(data), headers=[('Content-Type', 'application/json'), auth_header]) - assert response.status_code == 200 + assert response.status_code == 204 @moto.mock_sqs @@ -233,7 +233,7 @@ def test_send_email_valid_data(notify_api, data=json.dumps(data), headers=[('Content-Type', 'application/json'), auth_header]) - assert response.status_code == 200 + assert response.status_code == 204 @moto.mock_sqs @@ -264,7 +264,7 @@ def test_valid_message_with_service_id(notify_api, data=json.dumps(data), headers=[('Content-Type', 'application/json'), auth_header]) - assert response.status_code == 200 + assert response.status_code == 204 @moto.mock_sqs From 1b25a3c7628adce9c8ae8be6b8550175edca4021 Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Tue, 9 Feb 2016 16:13:48 +0000 Subject: [PATCH 24/28] Removed alpha client imports. --- app/__init__.py | 3 --- app/notifications/rest.py | 2 +- app/user/rest.py | 2 +- tests/app/notifications/test_rest.py | 1 - 4 files changed, 2 insertions(+), 6 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 5fefc46d4..86bf0f115 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -10,12 +10,10 @@ from flask_marshmallow import Marshmallow from werkzeug.local import LocalProxy from config import configs from utils import logging -from notify_client import NotifyAPIClient db = SQLAlchemy() ma = Marshmallow() -notify_alpha_client = NotifyAPIClient() api_user = LocalProxy(lambda: _request_ctx_stack.top.api_user) @@ -30,7 +28,6 @@ def create_app(config_name, config_overrides=None): ma.init_app(application) init_app(application, config_overrides) logging.init_app(application) - notify_alpha_client.init_app(application) from app.service.rest import service as service_blueprint from app.user.rest import user as user_blueprint diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 84b119d46..0c3cdf569 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -6,7 +6,7 @@ from flask import ( request ) -from app import (notify_alpha_client, api_user) +from app import api_user from app.aws_sqs import add_notification_to_queue from app.dao import (templates_dao) from app.schemas import ( diff --git a/app/user/rest.py b/app/user/rest.py index 02d46732d..0fd28f67a 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -17,7 +17,7 @@ from app.dao.users_dao import ( from app.schemas import ( user_schema, users_schema, service_schema, services_schema, request_verify_code_schema, user_schema_load_json) -from app import (notify_alpha_client, api_user) +from app import api_user user = Blueprint('user', __name__) diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 1715c7a12..b506bb10a 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -3,7 +3,6 @@ import uuid from tests import create_authorization_header from flask import url_for, json -from app import notify_alpha_client from app.models import Service From e6a7e075055a4132c7fd70815480afc0a22be4ca Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Tue, 9 Feb 2016 18:28:10 +0000 Subject: [PATCH 25/28] Fix for create job id on api side --- app/models.py | 2 +- tests/app/dao/test_notification_dao.py | 11 ++--------- tests/app/job/test_job_rest.py | 4 +--- 3 files changed, 4 insertions(+), 13 deletions(-) diff --git a/app/models.py b/app/models.py index 1ce4a7dbf..1282a2ac0 100644 --- a/app/models.py +++ b/app/models.py @@ -198,7 +198,7 @@ class Notification(db.Model): __tablename__ = 'notifications' - id = db.Column(UUID(as_uuid=True), primary_key=True) + id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) to = db.Column(db.String, nullable=False) job_id = db.Column(UUID(as_uuid=True), db.ForeignKey('jobs.id'), index=True, unique=False, nullable=False) job = db.relationship('Job', backref=db.backref('notifications', lazy='dynamic')) diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index cf1e70a6f..9cbf6b6d9 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -1,5 +1,3 @@ -import uuid - from app.models import Notification from app.dao.notifications_dao import ( @@ -12,11 +10,8 @@ from app.dao.notifications_dao import ( def test_save_notification(notify_db, notify_db_session, sample_template, sample_job): assert Notification.query.count() == 0 - - notification_id = uuid.uuid4() to = '+44709123456' data = { - 'id': notification_id, 'to': to, 'job': sample_job, 'service': sample_template.service, @@ -27,10 +22,8 @@ def test_save_notification(notify_db, notify_db_session, sample_template, sample save_notification(notification) assert Notification.query.count() == 1 - - notification_from_db = Notification.query.get(notification_id) - - assert data['id'] == notification_from_db.id + notification_from_db = Notification.query.all()[0] + assert notification_from_db.id assert data['to'] == notification_from_db.to assert data['job'] == notification_from_db.job assert data['service'] == notification_from_db.service diff --git a/tests/app/job/test_job_rest.py b/tests/app/job/test_job_rest.py index d16e2ac8f..a10c004da 100644 --- a/tests/app/job/test_job_rest.py +++ b/tests/app/job/test_job_rest.py @@ -219,10 +219,8 @@ def test_get_notifications(notify_api, notify_db, notify_db_session, sample_job) def test_add_notification(notify_api, notify_db, notify_db_session, sample_job): - notificaton_id = uuid.uuid4() to = '+44709123456' data = { - 'id': str(notificaton_id), 'to': to, 'job': str(sample_job.id), 'service': str(sample_job.service.id), @@ -245,7 +243,7 @@ def test_add_notification(notify_api, notify_db, notify_db_session, sample_job): resp_json = json.loads(response.get_data(as_text=True)) - assert data['id'] == resp_json['data']['id'] + 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'] From d38ba0d36a6281cb708f27ee7d0cafab655513ef Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 9 Feb 2016 18:48:02 +0000 Subject: [PATCH 26/28] bumped client version --- app/authentication/auth.py | 4 ++-- requirements.txt | 2 +- tests/__init__.py | 2 +- tests/app/authentication/test_authentication.py | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/authentication/auth.py b/app/authentication/auth.py index e80e6f8a7..8a919fdf6 100644 --- a/app/authentication/auth.py +++ b/app/authentication/auth.py @@ -1,6 +1,6 @@ from flask import request, jsonify, _request_ctx_stack, current_app -from client.authentication import decode_jwt_token, get_token_issuer -from client.errors import TokenDecodeError, TokenRequestError, TokenExpiredError, TokenPayloadError +from notifications_python_client.authentication import decode_jwt_token, get_token_issuer +from notifications_python_client.errors import TokenDecodeError, TokenRequestError, TokenExpiredError, TokenPayloadError from app.dao.api_key_dao import get_unsigned_secrets diff --git a/requirements.txt b/requirements.txt index 8c297ceea..a6c759bc3 100644 --- a/requirements.txt +++ b/requirements.txt @@ -14,6 +14,6 @@ Flask-Bcrypt==0.6.2 credstash==1.8.0 boto3==1.2.3 -git+https://github.com/alphagov/notifications-python-client.git@0.2.1#egg=notifications-python-client==0.2.1 +git+https://github.com/alphagov/notifications-python-client.git@0.2.5#egg=notifications-python-client==0.2.5 git+https://github.com/alphagov/notifications-utils.git@0.0.3#egg=notifications-utils==0.0.3 diff --git a/tests/__init__.py b/tests/__init__.py index e76b036be..7491049d4 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -1,5 +1,5 @@ from flask import current_app -from client.authentication import create_jwt_token +from notifications_python_client.authentication import create_jwt_token from app.dao.api_key_dao import get_unsigned_secrets diff --git a/tests/app/authentication/test_authentication.py b/tests/app/authentication/test_authentication.py index 74c67c4ef..18156f9e2 100644 --- a/tests/app/authentication/test_authentication.py +++ b/tests/app/authentication/test_authentication.py @@ -1,6 +1,6 @@ from datetime import datetime, timedelta -from client.authentication import create_jwt_token +from notifications_python_client.authentication import create_jwt_token from flask import json, url_for, current_app from app.dao.api_key_dao import get_unsigned_secrets, save_model_api_key, get_unsigned_secret from app.models import ApiKey, Service From 3a2cfc96e6be261ec8133f1f63589da00bb6705a Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Wed, 10 Feb 2016 11:08:24 +0000 Subject: [PATCH 27/28] job on notification now nullable. --- app/models.py | 2 +- migrations/versions/0014_job_id_nullable.py | 30 +++++++++++++++++++++ tests/app/dao/test_notification_dao.py | 22 +++++++++++++++ 3 files changed, 53 insertions(+), 1 deletion(-) create mode 100644 migrations/versions/0014_job_id_nullable.py diff --git a/app/models.py b/app/models.py index 1282a2ac0..4b53483dc 100644 --- a/app/models.py +++ b/app/models.py @@ -200,7 +200,7 @@ class Notification(db.Model): id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) to = db.Column(db.String, nullable=False) - job_id = db.Column(UUID(as_uuid=True), db.ForeignKey('jobs.id'), index=True, unique=False, nullable=False) + job_id = db.Column(UUID(as_uuid=True), db.ForeignKey('jobs.id'), index=True, unique=False) job = db.relationship('Job', backref=db.backref('notifications', lazy='dynamic')) service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, unique=False) service = db.relationship('Service') diff --git a/migrations/versions/0014_job_id_nullable.py b/migrations/versions/0014_job_id_nullable.py new file mode 100644 index 000000000..8c27b3ce1 --- /dev/null +++ b/migrations/versions/0014_job_id_nullable.py @@ -0,0 +1,30 @@ +"""empty message + +Revision ID: 0014_job_id_nullable +Revises: 0013_add_notifications +Create Date: 2016-02-10 10:57:39.414061 + +""" + +# revision identifiers, used by Alembic. +revision = '0014_job_id_nullable' +down_revision = '0013_add_notifications' + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +def upgrade(): + ### commands auto generated by Alembic - please adjust! ### + op.alter_column('notifications', 'job_id', + existing_type=postgresql.UUID(), + nullable=True) + ### end Alembic commands ### + + +def downgrade(): + ### commands auto generated by Alembic - please adjust! ### + op.alter_column('notifications', 'job_id', + existing_type=postgresql.UUID(), + nullable=False) + ### end Alembic commands ### diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 9cbf6b6d9..a28851520 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -31,6 +31,28 @@ def test_save_notification(notify_db, notify_db_session, sample_template, sample assert 'sent' == notification_from_db.status +def test_save_notification_no_job_id(notify_db, notify_db_session, sample_template): + + assert Notification.query.count() == 0 + to = '+44709123456' + data = { + 'to': to, + 'service': sample_template.service, + 'template': sample_template + } + + notification = Notification(**data) + save_notification(notification) + + assert Notification.query.count() == 1 + notification_from_db = Notification.query.all()[0] + assert notification_from_db.id + assert data['to'] == notification_from_db.to + assert data['service'] == notification_from_db.service + assert data['template'] == notification_from_db.template + assert 'sent' == notification_from_db.status + + def test_get_notification_for_job(notify_db, notify_db_session, sample_notification): notifcation_from_db = get_notification(sample_notification.service.id, sample_notification.job_id, From a1be4e3ca548cccafc32fd8854877842b3b79e90 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 10 Feb 2016 12:33:54 +0000 Subject: [PATCH 28/28] Updated python client verion --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index a6c759bc3..2b4789aeb 100644 --- a/requirements.txt +++ b/requirements.txt @@ -14,6 +14,6 @@ Flask-Bcrypt==0.6.2 credstash==1.8.0 boto3==1.2.3 -git+https://github.com/alphagov/notifications-python-client.git@0.2.5#egg=notifications-python-client==0.2.5 +git+https://github.com/alphagov/notifications-python-client.git@0.2.6#egg=notifications-python-client==0.2.6 git+https://github.com/alphagov/notifications-utils.git@0.0.3#egg=notifications-utils==0.0.3