From db91a87fb238f7e35a88f356067f4b7d1b29a750 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 2 Nov 2016 14:48:15 +0000 Subject: [PATCH 01/50] Remove link from v2 error response --- app/errors.py | 2 -- app/schema_validation/__init__.py | 1 - app/v2/errors.py | 3 --- tests/app/notifications/test_validators.py | 5 ----- tests/app/v2/notifications/test_notification_schemas.py | 3 --- tests/app/v2/notifications/test_post_notifications.py | 3 --- 6 files changed, 17 deletions(-) diff --git a/app/errors.py b/app/errors.py index 8b027ba60..2588bd54a 100644 --- a/app/errors.py +++ b/app/errors.py @@ -10,7 +10,6 @@ from app.authentication.auth import AuthError class InvalidRequest(Exception): code = None - link = None fields = [] def __init__(self, message, status_code): @@ -28,7 +27,6 @@ class InvalidRequest(Exception): return { "code": self.code, "message": self.message, - "link": self.link, "fields": self.fields } diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py index 38837b467..e6a2c9748 100644 --- a/app/schema_validation/__init__.py +++ b/app/schema_validation/__init__.py @@ -21,7 +21,6 @@ def build_error_message(errors, schema): message = { "code": "1001", "message": "Validation error occurred - {}".format(schema['title']), - "link": "link to error documentation (not yet implemented)", "fields": fields } diff --git a/app/v2/errors.py b/app/v2/errors.py index b7bd5f586..b7f7d229c 100644 --- a/app/v2/errors.py +++ b/app/v2/errors.py @@ -11,9 +11,7 @@ from app.errors import InvalidRequest class TooManyRequestsError(InvalidRequest): status_code = 429 - # code and link will be in a static file code = "10429" - link = "link to docs" message_template = 'Exceeded send limits ({}) for today' def __init__(self, sending_limit): @@ -23,7 +21,6 @@ class TooManyRequestsError(InvalidRequest): class BadRequestError(InvalidRequest): status_code = 400 code = 10400 - link = "link to documentation" message = "An error occurred" def __init__(self, fields=[], message=None): diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index e41d5f81c..7855e95eb 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -52,7 +52,6 @@ def test_check_template_is_for_notification_type_fails_when_template_type_does_n assert e.value.code == 10400 error_message = '{0} template is not suitable for {1} notification'.format(template_type, notification_type) assert e.value.message == error_message - assert e.value.link == 'link to documentation' assert e.value.fields == [{'template': error_message}] @@ -69,7 +68,6 @@ def test_check_template_is_active_fails(sample_template): assert e.value.status_code == 400 assert e.value.code == 10400 assert e.value.message == 'Template has been deleted' - assert e.value.link == "link to documentation" assert e.value.fields == [{'template': 'Template has been deleted'}] @@ -124,7 +122,6 @@ def test_service_can_send_to_recipient_fails_when_recipient_is_not_on_team(recip assert exec_info.value.status_code == 400 assert exec_info.value.code == 10400 assert exec_info.value.message == error_message - assert exec_info.value.link == 'link to documentation' assert exec_info.value.fields == [] @@ -137,7 +134,6 @@ def test_service_can_send_to_recipient_fails_when_mobile_number_is_not_on_team(n assert e.value.status_code == 400 assert e.value.code == 10400 assert e.value.message == 'Can’t send to this recipient using a team-only API key' - assert e.value.link == 'link to documentation' assert e.value.fields == [] @@ -154,5 +150,4 @@ def test_check_sms_content_char_count_fails(char_count, notify_api): assert e.value.code == 10400 assert e.value.message == 'Content for template has a character count greater than the limit of {}'.format( notify_api.config['SMS_CHAR_COUNT_LIMIT']) - assert e.value.link == 'link to documentation' assert e.value.fields == [] diff --git a/tests/app/v2/notifications/test_notification_schemas.py b/tests/app/v2/notifications/test_notification_schemas.py index 96f9e1284..13ccc1124 100644 --- a/tests/app/v2/notifications/test_notification_schemas.py +++ b/tests/app/v2/notifications/test_notification_schemas.py @@ -33,7 +33,6 @@ def test_post_sms_json_schema_bad_uuid_and_missing_phone_number(): assert {"phone_number": "is a required property"} in error['fields'] assert {"template_id": "not a valid UUID"} in error['fields'] assert error.get('code') == '1001' - assert error.get('link', None) is not None def test_post_sms_schema_with_personalisation_that_is_not_a_dict(): @@ -50,7 +49,6 @@ def test_post_sms_schema_with_personalisation_that_is_not_a_dict(): assert len(error.get('fields')) == 1 assert error['fields'][0] == {"personalisation": "should contain key value pairs"} assert error.get('code') == '1001' - assert error.get('link', None) == 'link to error documentation (not yet implemented)' valid_response = { @@ -87,6 +85,5 @@ def test_post_sms_response_schema_missing_uri(): validate(j, post_sms_response) error = json.loads(e.value.message) assert '1001' == error['code'] - assert 'link to error documentation (not yet implemented)' == error['link'] assert 'Validation error occurred - response v2/notifications/sms' == error['message'] assert [{"uri": "is a required property"}] == error['fields'] diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index bbc07e309..e8e500410 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -58,7 +58,6 @@ def test_post_sms_notification_returns_404_and_missing_template(notify_api, samp assert error_json['code'] == 10400 assert error_json['message'] == 'Template not found' assert error_json['fields'] == [{'template': 'Template not found'}] - assert error_json['link'] == 'link to documentation' def test_post_sms_notification_returns_403_and_well_formed_auth_error(notify_api, sample_template): @@ -80,7 +79,6 @@ def test_post_sms_notification_returns_403_and_well_formed_auth_error(notify_api assert error_resp['code'] == 401 assert error_resp['message'] == 'Unauthorized, authentication token must be provided' assert error_resp['fields'] == {'token': ['Unauthorized, authentication token must be provided']} - assert error_resp['link'] == 'link to docs' def test_post_sms_notification_returns_400_and_for_schema_problems(notify_api, sample_template): @@ -102,5 +100,4 @@ def test_post_sms_notification_returns_400_and_for_schema_problems(notify_api, s error_resp = json.loads(response.get_data(as_text=True)) assert error_resp['code'] == '1001' assert error_resp['message'] == 'Validation error occurred - POST v2/notifications/sms' - assert error_resp['link'] == "link to error documentation (not yet implemented)" assert error_resp['fields'] == [{"template_id": "is a required property"}] From 4cb38e2d12fd66d59af11976939158f851134054 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 2 Nov 2016 14:58:39 +0000 Subject: [PATCH 02/50] Use status_code in error response. Remove code. --- app/authentication/auth.py | 9 +++++---- app/errors.py | 2 +- app/schema_validation/__init__.py | 2 +- app/v2/errors.py | 2 -- tests/app/notifications/test_validators.py | 7 +------ tests/app/v2/notifications/test_notification_schemas.py | 6 +++--- tests/app/v2/notifications/test_post_notifications.py | 5 ++--- 7 files changed, 13 insertions(+), 20 deletions(-) diff --git a/app/authentication/auth.py b/app/authentication/auth.py index 0ca700a62..0f18f2322 100644 --- a/app/authentication/auth.py +++ b/app/authentication/auth.py @@ -16,10 +16,11 @@ class AuthError(Exception): self.code = code def to_dict_v2(self): - return {'code': self.code, - 'message': self.short_message, - 'fields': self.message, - 'link': 'link to docs'} + return { + 'status_code': self.code, + 'message': self.short_message, + 'fields': self.message + } def get_auth_token(req): diff --git a/app/errors.py b/app/errors.py index 2588bd54a..db5826cbd 100644 --- a/app/errors.py +++ b/app/errors.py @@ -25,7 +25,7 @@ class InvalidRequest(Exception): Version 2 of the public api error response. ''' return { - "code": self.code, + "status_code": self.code, "message": self.message, "fields": self.fields } diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py index e6a2c9748..51d43df7b 100644 --- a/app/schema_validation/__init__.py +++ b/app/schema_validation/__init__.py @@ -19,7 +19,7 @@ def build_error_message(errors, schema): field = {s[1]: s[2].strip()} fields.append(field) message = { - "code": "1001", + "status_code": 400, "message": "Validation error occurred - {}".format(schema['title']), "fields": fields } diff --git a/app/v2/errors.py b/app/v2/errors.py index b7f7d229c..8c98e48f8 100644 --- a/app/v2/errors.py +++ b/app/v2/errors.py @@ -11,7 +11,6 @@ from app.errors import InvalidRequest class TooManyRequestsError(InvalidRequest): status_code = 429 - code = "10429" message_template = 'Exceeded send limits ({}) for today' def __init__(self, sending_limit): @@ -20,7 +19,6 @@ class TooManyRequestsError(InvalidRequest): class BadRequestError(InvalidRequest): status_code = 400 - code = 10400 message = "An error occurred" def __init__(self, fields=[], message=None): diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index 7855e95eb..2810e75e5 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -28,7 +28,6 @@ def test_check_service_message_limit_over_message_limit_fails(key_type, notify_d with pytest.raises(TooManyRequestsError) as e: check_service_message_limit(key_type, service) assert e.value.status_code == 429 - assert e.value.code == '10429' assert e.value.message == 'Exceeded send limits (4) for today' assert e.value.fields == [] @@ -49,7 +48,7 @@ def test_check_template_is_for_notification_type_fails_when_template_type_does_n with pytest.raises(BadRequestError) as e: check_template_is_for_notification_type(notification_type=notification_type, template_type=template_type) - assert e.value.code == 10400 + e.value.status_code == 400 error_message = '{0} template is not suitable for {1} notification'.format(template_type, notification_type) assert e.value.message == error_message assert e.value.fields == [{'template': error_message}] @@ -66,7 +65,6 @@ def test_check_template_is_active_fails(sample_template): with pytest.raises(BadRequestError) as e: check_template_is_active(sample_template) assert e.value.status_code == 400 - assert e.value.code == 10400 assert e.value.message == 'Template has been deleted' assert e.value.fields == [{'template': 'Template has been deleted'}] @@ -120,7 +118,6 @@ def test_service_can_send_to_recipient_fails_when_recipient_is_not_on_team(recip key_type, trial_mode_service) assert exec_info.value.status_code == 400 - assert exec_info.value.code == 10400 assert exec_info.value.message == error_message assert exec_info.value.fields == [] @@ -132,7 +129,6 @@ def test_service_can_send_to_recipient_fails_when_mobile_number_is_not_on_team(n 'team', live_service) assert e.value.status_code == 400 - assert e.value.code == 10400 assert e.value.message == 'Can’t send to this recipient using a team-only API key' assert e.value.fields == [] @@ -147,7 +143,6 @@ def test_check_sms_content_char_count_fails(char_count, notify_api): with pytest.raises(BadRequestError) as e: check_sms_content_char_count(char_count) assert e.value.status_code == 400 - assert e.value.code == 10400 assert e.value.message == 'Content for template has a character count greater than the limit of {}'.format( notify_api.config['SMS_CHAR_COUNT_LIMIT']) assert e.value.fields == [] diff --git a/tests/app/v2/notifications/test_notification_schemas.py b/tests/app/v2/notifications/test_notification_schemas.py index 13ccc1124..ac63ec6d7 100644 --- a/tests/app/v2/notifications/test_notification_schemas.py +++ b/tests/app/v2/notifications/test_notification_schemas.py @@ -32,7 +32,7 @@ def test_post_sms_json_schema_bad_uuid_and_missing_phone_number(): assert len(error.get('fields')) == 2 assert {"phone_number": "is a required property"} in error['fields'] assert {"template_id": "not a valid UUID"} in error['fields'] - assert error.get('code') == '1001' + assert error.get('status_code') == 400 def test_post_sms_schema_with_personalisation_that_is_not_a_dict(): @@ -48,7 +48,7 @@ def test_post_sms_schema_with_personalisation_that_is_not_a_dict(): assert "POST v2/notifications/sms" in error['message'] assert len(error.get('fields')) == 1 assert error['fields'][0] == {"personalisation": "should contain key value pairs"} - assert error.get('code') == '1001' + assert error.get('status_code') == 400 valid_response = { @@ -84,6 +84,6 @@ def test_post_sms_response_schema_missing_uri(): with pytest.raises(ValidationError) as e: validate(j, post_sms_response) error = json.loads(e.value.message) - assert '1001' == error['code'] + assert error['status_code'] == 400 assert 'Validation error occurred - response v2/notifications/sms' == error['message'] assert [{"uri": "is a required property"}] == error['fields'] diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index e8e500410..4b93a7aa5 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -55,7 +55,6 @@ def test_post_sms_notification_returns_404_and_missing_template(notify_api, samp assert response.headers['Content-type'] == 'application/json' error_json = json.loads(response.get_data(as_text=True)) - assert error_json['code'] == 10400 assert error_json['message'] == 'Template not found' assert error_json['fields'] == [{'template': 'Template not found'}] @@ -76,7 +75,7 @@ def test_post_sms_notification_returns_403_and_well_formed_auth_error(notify_api assert response.status_code == 401 assert response.headers['Content-type'] == 'application/json' error_resp = json.loads(response.get_data(as_text=True)) - assert error_resp['code'] == 401 + assert error_resp['status_code'] == 401 assert error_resp['message'] == 'Unauthorized, authentication token must be provided' assert error_resp['fields'] == {'token': ['Unauthorized, authentication token must be provided']} @@ -98,6 +97,6 @@ def test_post_sms_notification_returns_400_and_for_schema_problems(notify_api, s assert response.status_code == 400 assert response.headers['Content-type'] == 'application/json' error_resp = json.loads(response.get_data(as_text=True)) - assert error_resp['code'] == '1001' + assert error_resp['status_code'] == 400 assert error_resp['message'] == 'Validation error occurred - POST v2/notifications/sms' assert error_resp['fields'] == [{"template_id": "is a required property"}] From 37c95b08b69e54fcd0cec91083a82814f23c652b Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 3 Nov 2016 11:11:51 +0000 Subject: [PATCH 03/50] Refactor saving user profile --- app/dao/users_dao.py | 9 ++++++--- app/user/rest.py | 10 +++------- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/app/dao/users_dao.py b/app/dao/users_dao.py index 1a240fecc..264481bfe 100644 --- a/app/dao/users_dao.py +++ b/app/dao/users_dao.py @@ -7,6 +7,11 @@ from app import db from app.models import (User, VerifyCode) +def _remove_values_for_keys_if_present(dict, keys): + for key in keys: + dict.pop(key, None) + + def create_secret_code(): return ''.join(map(str, random.sample(range(9), 5))) @@ -16,9 +21,7 @@ def save_model_user(usr, update_dict={}, pwd=None): usr.password = pwd usr.password_changed_at = datetime.utcnow() if update_dict: - if update_dict.get('id'): - del update_dict['id'] - update_dict.pop('password_changed_at') + _remove_values_for_keys_if_present(update_dict, ['id', 'password', 'password_changed_at']) db.session.query(User).filter_by(id=usr.id).update(update_dict) else: db.session.add(usr) diff --git a/app/user/rest.py b/app/user/rest.py index 2e6e6b021..7aee5ac3f 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -55,14 +55,10 @@ def create_user(): def update_user(user_id): user_to_update = get_model_users(user_id=user_id) req_json = request.get_json() - update_dct, errors = user_schema_load_json.load(req_json) pwd = req_json.get('password', None) - # TODO password validation, it is already done on the admin app - # but would be good to have the same validation here. - if pwd is not None and not pwd: - errors.update({'password': ['Invalid data for field']}) - raise InvalidRequest(errors, status_code=400) - save_model_user(user_to_update, update_dict=update_dct, pwd=pwd) + if not pwd: + raise InvalidRequest('Invalid entry for password', status_code=400) + save_model_user(user_to_update, update_dict=req_json, pwd=pwd) return jsonify(data=user_schema.dump(user_to_update).data), 200 From a41e7d5a2f68cdd70438635bd44d34cb648f1caa Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 3 Nov 2016 11:46:58 +0000 Subject: [PATCH 04/50] Add the right password check --- app/user/rest.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/user/rest.py b/app/user/rest.py index 7aee5ac3f..4fd21e202 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -56,7 +56,7 @@ def update_user(user_id): user_to_update = get_model_users(user_id=user_id) req_json = request.get_json() pwd = req_json.get('password', None) - if not pwd: + if pwd is not None and not pwd: raise InvalidRequest('Invalid entry for password', status_code=400) save_model_user(user_to_update, update_dict=req_json, pwd=pwd) return jsonify(data=user_schema.dump(user_to_update).data), 200 From f089b7512915d4b8f5724f33acd1f248d204c27d Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 3 Nov 2016 17:05:25 +0000 Subject: [PATCH 05/50] update python client to 2.0.0 this is to prevent 500 errors because <2.0.0 raised AssertionError if supplied JWT tokens were incorrectly formatted tests added --- app/authentication/auth.py | 12 +++--- requirements.txt | 2 +- .../app/authentication/test_authentication.py | 43 +++++++++++++++++++ 3 files changed, 51 insertions(+), 6 deletions(-) diff --git a/app/authentication/auth.py b/app/authentication/auth.py index 0ca700a62..ef830f6db 100644 --- a/app/authentication/auth.py +++ b/app/authentication/auth.py @@ -3,7 +3,7 @@ from sqlalchemy.exc import DataError from sqlalchemy.orm.exc import NoResultFound from notifications_python_client.authentication import decode_jwt_token, get_token_issuer -from notifications_python_client.errors import TokenDecodeError, TokenExpiredError +from notifications_python_client.errors import TokenDecodeError, TokenExpiredError, TokenIssuerError from app.dao.api_key_dao import get_model_api_keys from app.dao.services_dao import dao_fetch_service_by_id @@ -39,8 +39,10 @@ def requires_auth(): auth_token = get_auth_token(request) try: client = get_token_issuer(auth_token) - except TokenDecodeError: - raise AuthError("Invalid token: signature", 403) + except TokenDecodeError as e: + raise AuthError(e.message, 403) + except TokenIssuerError: + raise AuthError("Invalid token: iss not provided", 403) if client == current_app.config.get('ADMIN_CLIENT_USER_NAME'): return handle_admin_key(auth_token, current_app.config.get('ADMIN_CLIENT_SECRET')) @@ -76,8 +78,8 @@ def handle_admin_key(auth_token, secret): try: get_decode_errors(auth_token, secret) return - except TokenDecodeError: - raise AuthError("Invalid token: signature", 403) + except TokenDecodeError as e: + raise AuthError(e.message, 403) def get_decode_errors(auth_token, unsigned_secret): diff --git a/requirements.txt b/requirements.txt index a71037824..594f12317 100644 --- a/requirements.txt +++ b/requirements.txt @@ -19,7 +19,7 @@ monotonic==1.2 statsd==3.2.1 jsonschema==2.5.1 -git+https://github.com/alphagov/notifications-python-client.git@1.3.0#egg=notifications-python-client==1.3.0 +git+https://github.com/alphagov/notifications-python-client.git@2.0.0#egg=notifications-python-client==2.0.0 git+https://github.com/alphagov/notifications-utils.git@9.1.1#egg=notifications-utils==9.1.1 diff --git a/tests/app/authentication/test_authentication.py b/tests/app/authentication/test_authentication.py index 3b9ff1009..42c6fb9c3 100644 --- a/tests/app/authentication/test_authentication.py +++ b/tests/app/authentication/test_authentication.py @@ -1,3 +1,6 @@ +import jwt +import uuid +import time from datetime import datetime import pytest @@ -41,6 +44,46 @@ def test_should_not_allow_request_with_incorrect_token(notify_api, sample_user): assert data['message'] == {"token": ['Invalid token: signature']} +def test_should_not_allow_request_with_no_iss(client): + # code copied from notifications_python_client.authentication.py::create_jwt_token + headers = { + "typ": 'JWT', + "alg": 'HS256' + } + + claims = { + # 'iss': not provided + 'iat': int(time.time()) + } + + token = jwt.encode(payload=claims, key=str(uuid.uuid4()), headers=headers).decode() + + response = client.get('/service', headers={'Authorization': 'Bearer {}'.format(token)}) + assert response.status_code == 403 + data = json.loads(response.get_data()) + assert data['message'] == {"token": ['Invalid token: iss field not provided']} + + +def test_should_not_allow_request_with_no_iat(client, sample_api_key): + # code copied from notifications_python_client.authentication.py::create_jwt_token + headers = { + "typ": 'JWT', + "alg": 'HS256' + } + + claims = { + 'iss': str(sample_api_key.service_id) + # 'iat': not provided + } + + token = jwt.encode(payload=claims, key=str(uuid.uuid4()), headers=headers).decode() + + response = client.get('/service', headers={'Authorization': 'Bearer {}'.format(token)}) + assert response.status_code == 403 + data = json.loads(response.get_data()) + assert data['message'] == {"token": ['Invalid token: signature, api token is not valid']} + + def test_should_not_allow_invalid_secret(notify_api, sample_api_key): with notify_api.test_request_context(): with notify_api.test_client() as client: From ed187f7fada04284dd5d43477154cd8663fb144e Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 4 Nov 2016 10:11:09 +0000 Subject: [PATCH 06/50] remove 500/unplanned exception handlers in test this means if your code throws an exception, in the test you'll see the full stack trace to help debugging --- tests/conftest.py | 60 +++++++++++++++++++++++++++-------------------- 1 file changed, 34 insertions(+), 26 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 11f570de5..36ad8c58b 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2,7 +2,6 @@ from contextlib import contextmanager import os import boto3 -from unittest import mock import pytest from alembic.command import upgrade from alembic.config import Config @@ -13,16 +12,31 @@ from app import create_app, db @pytest.fixture(scope='session') -def notify_api(request): +def notify_api(): app = create_app() + + # deattach server-error error handlers - error_handler_spec looks like: + # {'blueprint_name': { + # status_code: [error_handlers], + # None: [ tuples of (exception, )] + # }} + for error_handlers in app.error_handler_spec.values(): + error_handlers.pop(500, None) + if None in error_handlers: + error_handlers[None] = [ + exception_handler + for exception_handler in error_handlers[None] + if exception_handler[0] != Exception + ] + if error_handlers[None] == []: + error_handlers.pop(None) + ctx = app.app_context() ctx.push() - def teardown(): - ctx.pop() + yield app - request.addfinalizer(teardown) - return app + ctx.pop() @pytest.fixture(scope='function') @@ -32,7 +46,7 @@ def client(notify_api): @pytest.fixture(scope='session') -def notify_db(notify_api, request): +def notify_db(notify_api): Migrate(notify_api, db) Manager(db, MigrateCommand) BASE_DIR = os.path.dirname(os.path.dirname(__file__)) @@ -43,36 +57,30 @@ def notify_db(notify_api, request): with notify_api.app_context(): upgrade(config, 'head') - def teardown(): - db.session.remove() - db.get_engine(notify_api).dispose() + yield db - request.addfinalizer(teardown) - return db + db.session.remove() + db.get_engine(notify_api).dispose() @pytest.fixture(scope='function') -def notify_db_session(request, notify_db): - def teardown(): - notify_db.session.remove() - for tbl in reversed(notify_db.metadata.sorted_tables): - if tbl.name not in ["provider_details", "key_types", "branding_type", "job_status"]: - notify_db.engine.execute(tbl.delete()) - notify_db.session.commit() +def notify_db_session(notify_db): + yield notify_db - request.addfinalizer(teardown) + notify_db.session.remove() + for tbl in reversed(notify_db.metadata.sorted_tables): + if tbl.name not in ["provider_details", "key_types", "branding_type", "job_status"]: + notify_db.engine.execute(tbl.delete()) + notify_db.session.commit() @pytest.fixture(scope='function') -def os_environ(request): - env_patch = mock.patch('os.environ', {}) - request.addfinalizer(env_patch.stop) - - return env_patch.start() +def os_environ(mocker): + mocker.patch('os.environ', {}) @pytest.fixture(scope='function') -def sqs_client_conn(request): +def sqs_client_conn(): boto3.setup_default_session(region_name='eu-west-1') return boto3.resource('sqs') From f842a8c8935aa8dab9843e0729e9b7b5fbe66e9f Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 4 Nov 2016 17:04:51 +0000 Subject: [PATCH 07/50] update test to pytest.raises instaed of assert response == 500 --- .../app/notifications/rest/test_send_notification.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index a28590c40..78c45b748 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -698,14 +698,15 @@ def test_should_delete_notification_and_return_error_if_sqs_fails( save_model_api_key(api_key) auth_header = create_jwt_token(secret=api_key.unsigned_secret, client_id=str(api_key.service_id)) - response = client.post( - path='/notifications/{}'.format(template_type), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))]) + with pytest.raises(Exception) as exc: + response = client.post( + path='/notifications/{}'.format(template_type), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), ('Authorization', 'Bearer {}'.format(auth_header))]) mocked.assert_called_once_with([fake_uuid], queue='send-{}'.format(template_type)) + assert str(exc.value) == 'failed to talk to SQS' - assert response.status_code == 500 assert not notifications_dao.get_notification_by_id(fake_uuid) assert not NotificationHistory.query.get(fake_uuid) From 8a126c738770ce09c961e76db64ac903d223888e Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 7 Nov 2016 17:41:49 +0000 Subject: [PATCH 08/50] Add a schema to validate a single user attr 'strictly' --- app/schemas.py | 36 ++++++++++++++++++++++++++++++++++++ tests/app/test_schemas.py | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+) diff --git a/app/schemas.py b/app/schemas.py index 6f5934114..b9f75cb2c 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -105,6 +105,41 @@ class UserSchema(BaseSchema): strict = True +class UserUpdateAttributeSchema(BaseSchema): + + class Meta: + model = models.User + exclude = ( + "updated_at", "created_at", "user_to_service", + "_password", "verify_codes") + strict = True + + @validates('name') + def validate_name(self, value): + if not value: + raise ValidationError('Invalid name') + + @validates('email_address') + def validate_email_address(self, value): + try: + validate_email_address(value) + except InvalidEmailError as e: + raise ValidationError(e.message) + + @validates('mobile_number') + def validate_mobile_number(self, value): + try: + validate_phone_number(value) + except InvalidPhoneError as error: + raise ValidationError('Invalid phone number: {}'.format(error)) + + @validates_schema(pass_original=True) + def check_unknown_fields(self, data, original_data): + for key in original_data: + if key not in self.fields: + raise ValidationError('Unknown field name {}'.format(key)) + + class ProviderDetailsSchema(BaseSchema): class Meta: model = models.ProviderDetails @@ -529,6 +564,7 @@ class UnarchivedTemplateSchema(BaseSchema): user_schema = UserSchema() user_schema_load_json = UserSchema(load_json=True) +user_update_schema_load_json = UserUpdateAttributeSchema(load_json=True, partial=True) service_schema = ServiceSchema() service_schema_load_json = ServiceSchema(load_json=True) detailed_service_schema = DetailedServiceSchema() diff --git a/tests/app/test_schemas.py b/tests/app/test_schemas.py index 4ba1c98b5..5f2bc4371 100644 --- a/tests/app/test_schemas.py +++ b/tests/app/test_schemas.py @@ -1,3 +1,6 @@ +import pytest + + def test_job_schema_doesnt_return_notifications(sample_notification_with_job): from app.schemas import job_schema @@ -22,3 +25,34 @@ def test_notification_schema_adds_api_key_name(sample_notification_with_api_key) data = notification_with_template_schema.dump(sample_notification_with_api_key).data assert data['key_name'] == 'Test key' + + +@pytest.mark.parametrize('user_attribute, user_value', [ + ('name', 'New User'), + ('email_address', 'newuser@mail.com'), + ('mobile_number', '+4407700900460') +]) +def test_user_schema_accepts_valid_attributes(user_attribute, user_value): + update_dict = { + user_attribute: user_value + } + from app.schemas import user_update_schema_load_json + + data, errors = user_update_schema_load_json.load(update_dict) + assert not errors + + +@pytest.mark.parametrize('user_attribute, user_value', [ + ('name', None), + ('name', ''), + ('email_address', 'bademail@...com'), + ('mobile_number', '+44077009') +]) +def test_user_schema_rejects_invalid_attributes(user_attribute, user_value): + from app.schemas import user_update_schema_load_json + update_dict = { + user_attribute: user_value + } + + with pytest.raises(Exception): + data, errors = user_update_schema_load_json.load(update_dict) From 461d8a9b2c4850c6caa74936dfcf945f3df34a91 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 7 Nov 2016 17:42:23 +0000 Subject: [PATCH 09/50] Add separate endpoint to update a single user attr --- app/service/rest.py | 8 ++++---- app/user/rest.py | 36 +++++++++++++++++++++++++----------- tests/app/user/test_rest.py | 24 ++++++++++++++++++++++++ 3 files changed, 53 insertions(+), 15 deletions(-) diff --git a/app/service/rest.py b/app/service/rest.py index 2cc7dab90..fb1ea29e1 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -34,7 +34,7 @@ from app.dao.service_whitelist_dao import ( ) from app.dao import notifications_dao from app.dao.provider_statistics_dao import get_fragment_count -from app.dao.users_dao import get_model_users +from app.dao.users_dao import get_user_by_id from app.errors import ( register_errors, InvalidRequest @@ -88,7 +88,7 @@ def create_service(): errors = {'user_id': ['Missing data for required field.']} raise InvalidRequest(errors, status_code=400) - user = get_model_users(data['user_id']) + user = get_user_by_id(data['user_id']) data.pop('user_id', None) valid_service = service_schema.load(request.get_json()).data dao_create_service(valid_service, user) @@ -148,7 +148,7 @@ def get_users_for_service(service_id): @service_blueprint.route('//users/', methods=['POST']) def add_user_to_service(service_id, user_id): service = dao_fetch_service_by_id(service_id) - user = get_model_users(user_id=user_id) + user = get_user_by_id(user_id=user_id) if user in service.users: error = 'User id: {} already part of service id: {}'.format(user_id, service_id) @@ -163,7 +163,7 @@ def add_user_to_service(service_id, user_id): @service_blueprint.route('//users/', methods=['DELETE']) def remove_user_from_service(service_id, user_id): service = dao_fetch_service_by_id(service_id) - user = get_model_users(user_id=user_id) + user = get_user_by_id(user_id=user_id) if user not in service.users: error = 'User not found' raise InvalidRequest(error, status_code=404) diff --git a/app/user/rest.py b/app/user/rest.py index 2e6e6b021..e41e10877 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -4,7 +4,7 @@ from datetime import datetime from flask import (jsonify, request, Blueprint, current_app) from app import encryption, DATETIME_FORMAT from app.dao.users_dao import ( - get_model_users, + get_user_by_id, save_model_user, create_user_code, get_user_code, @@ -12,7 +12,8 @@ from app.dao.users_dao import ( increment_failed_login_count, reset_failed_login_count, get_user_by_email, - create_secret_code + create_secret_code, + save_user_attribute ) from app.dao.permissions_dao import permission_dao from app.dao.services_dao import dao_fetch_service_by_id @@ -22,8 +23,10 @@ from app.schemas import ( email_data_request_schema, user_schema, request_verify_code_schema, + permission_schema, user_schema_load_json, - permission_schema) + user_update_schema_load_json +) from app.celery.tasks import ( send_sms, @@ -53,7 +56,7 @@ def create_user(): @user.route('/', methods=['PUT']) def update_user(user_id): - user_to_update = get_model_users(user_id=user_id) + user_to_update = get_user_by_id(user_id=user_id) req_json = request.get_json() update_dct, errors = user_schema_load_json.load(req_json) pwd = req_json.get('password', None) @@ -66,9 +69,20 @@ def update_user(user_id): return jsonify(data=user_schema.dump(user_to_update).data), 200 +@user.route('//update-attribute', methods=['PUT']) +def update_user_attribute(user_id): + user_to_update = get_user_by_id(user_id=user_id) + req_json = request.get_json() + update_dct, errors = user_update_schema_load_json.load(req_json) + if errors: + raise InvalidRequest(errors, status_code=400) + save_user_attribute(user_to_update, update_dict=update_dct) + return jsonify(data=user_schema.dump(user_to_update).data), 200 + + @user.route('//verify/password', methods=['POST']) def verify_user_password(user_id): - user_to_verify = get_model_users(user_id=user_id) + user_to_verify = get_user_by_id(user_id=user_id) txt_pwd = None try: @@ -92,7 +106,7 @@ def verify_user_password(user_id): @user.route('//verify/code', methods=['POST']) def verify_user_code(user_id): - user_to_verify = get_model_users(user_id=user_id) + user_to_verify = get_user_by_id(user_id=user_id) txt_code = None resp_json = request.get_json() @@ -120,7 +134,7 @@ def verify_user_code(user_id): @user.route('//sms-code', methods=['POST']) def send_user_sms_code(user_id): - user_to_send_to = get_model_users(user_id=user_id) + user_to_send_to = get_user_by_id(user_id=user_id) verify_code, errors = request_verify_code_schema.load(request.get_json()) secret_code = create_secret_code() @@ -149,7 +163,7 @@ def send_user_sms_code(user_id): @user.route('//change-email-verification', methods=['POST']) def send_user_confirm_new_email(user_id): - user_to_send_to = get_model_users(user_id=user_id) + user_to_send_to = get_user_by_id(user_id=user_id) email, errors = email_data_request_schema.load(request.get_json()) if errors: raise InvalidRequest(message=errors, status_code=400) @@ -178,7 +192,7 @@ def send_user_confirm_new_email(user_id): @user.route('//email-verification', methods=['POST']) def send_user_email_verification(user_id): - user_to_send_to = get_model_users(user_id=user_id) + user_to_send_to = get_user_by_id(user_id=user_id) secret_code = create_secret_code() create_user_code(user_to_send_to, secret_code, 'email') @@ -230,7 +244,7 @@ def send_already_registered_email(user_id): @user.route('/', methods=['GET']) @user.route('', methods=['GET']) def get_user(user_id=None): - users = get_model_users(user_id=user_id) + users = get_user_by_id(user_id=user_id) result = user_schema.dump(users, many=True) if isinstance(users, list) else user_schema.dump(users) return jsonify(data=result.data) @@ -239,7 +253,7 @@ def get_user(user_id=None): def set_permissions(user_id, service_id): # TODO fix security hole, how do we verify that the user # who is making this request has permission to make the request. - user = get_model_users(user_id=user_id) + user = get_user_by_id(user_id=user_id) service = dao_fetch_service_by_id(service_id=service_id) permissions, errors = permission_schema.load(request.get_json(), many=True) diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py index 14b26d387..d9261fbb8 100644 --- a/tests/app/user/test_rest.py +++ b/tests/app/user/test_rest.py @@ -1,4 +1,5 @@ import json +import pytest from flask import url_for, current_app from freezegun import freeze_time @@ -180,6 +181,29 @@ def test_put_user(notify_api, notify_db, notify_db_session, sample_service): assert sorted(expected_permissions) == sorted(fetched['permissions'][str(sample_service.id)]) +@pytest.mark.parametrize('user_attribute, user_value', [ + ('name', 'New User'), + ('email_address', 'newuser@mail.com'), + ('mobile_number', '+4407700900460') +]) +def test_put_user_attribute(client, sample_user, user_attribute, user_value): + assert getattr(sample_user, user_attribute) != user_value + update_dict = { + user_attribute: user_value + } + auth_header = create_authorization_header() + headers = [('Content-Type', 'application/json'), auth_header] + + resp = client.put( + url_for('user.update_user_attribute', user_id=sample_user.id), + data=json.dumps(update_dict), + headers=headers) + + assert resp.status_code == 200 + json_resp = json.loads(resp.get_data(as_text=True)) + assert json_resp['data'][user_attribute] == user_value + + def test_put_user_update_password(notify_api, notify_db, notify_db_session, From 3f10e59db3fd072ff28a774f22e13ff0196fef61 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Mon, 7 Nov 2016 17:42:39 +0000 Subject: [PATCH 10/50] Add user dao method to update a single user attr --- app/dao/users_dao.py | 16 ++++++++++++---- tests/app/dao/test_users_dao.py | 29 ++++++++++++++++++++++------- 2 files changed, 34 insertions(+), 11 deletions(-) diff --git a/app/dao/users_dao.py b/app/dao/users_dao.py index 1a240fecc..6654234a5 100644 --- a/app/dao/users_dao.py +++ b/app/dao/users_dao.py @@ -7,18 +7,26 @@ from app import db from app.models import (User, VerifyCode) +def _remove_values_for_keys_if_present(dict, keys): + for key in keys: + dict.pop(key, None) + + def create_secret_code(): return ''.join(map(str, random.sample(range(9), 5))) +def save_user_attribute(usr, update_dict={}): + db.session.query(User).filter_by(id=usr.id).update(update_dict) + db.session.commit() + + def save_model_user(usr, update_dict={}, pwd=None): if pwd: usr.password = pwd usr.password_changed_at = datetime.utcnow() if update_dict: - if update_dict.get('id'): - del update_dict['id'] - update_dict.pop('password_changed_at') + _remove_values_for_keys_if_present(update_dict, ['id', 'password_changed_at']) db.session.query(User).filter_by(id=usr.id).update(update_dict) else: db.session.add(usr) @@ -74,7 +82,7 @@ def delete_user_verify_codes(user): db.session.commit() -def get_model_users(user_id=None): +def get_user_by_id(user_id=None): if user_id: return User.query.filter_by(id=user_id).one() return User.query.filter_by().all() diff --git a/tests/app/dao/test_users_dao.py b/tests/app/dao/test_users_dao.py index 7afcf72ce..7d1901f0d 100644 --- a/tests/app/dao/test_users_dao.py +++ b/tests/app/dao/test_users_dao.py @@ -7,12 +7,13 @@ import pytest from app.dao.users_dao import ( save_model_user, - get_model_users, + save_user_attribute, + get_user_by_id, delete_model_user, increment_failed_login_count, reset_failed_login_count, get_user_by_email, - delete_codes_older_created_more_than_a_day_ago + delete_codes_older_created_more_than_a_day_ago, ) from tests.app.conftest import sample_user as create_sample_user @@ -37,13 +38,13 @@ def test_create_user(notify_api, notify_db, notify_db_session): def test_get_all_users(notify_api, notify_db, notify_db_session, sample_user): assert User.query.count() == 1 - assert len(get_model_users()) == 1 + assert len(get_user_by_id()) == 1 email = "another.notify@digital.cabinet-office.gov.uk" another_user = create_sample_user(notify_db, notify_db_session, email=email) assert User.query.count() == 2 - assert len(get_model_users()) == 2 + assert len(get_user_by_id()) == 2 def test_get_user(notify_api, notify_db, notify_db_session): @@ -51,12 +52,12 @@ def test_get_user(notify_api, notify_db, notify_db_session): another_user = create_sample_user(notify_db, notify_db_session, email=email) - assert get_model_users(user_id=another_user.id).email_address == email + assert get_user_by_id(user_id=another_user.id).email_address == email def test_get_user_not_exists(notify_api, notify_db, notify_db_session, fake_uuid): try: - get_model_users(user_id=fake_uuid) + get_user_by_id(user_id=fake_uuid) pytest.fail("NoResultFound exception not thrown.") except NoResultFound as e: pass @@ -64,7 +65,7 @@ def test_get_user_not_exists(notify_api, notify_db, notify_db_session, fake_uuid def test_get_user_invalid_id(notify_api, notify_db, notify_db_session): try: - get_model_users(user_id="blah") + get_user_by_id(user_id="blah") pytest.fail("DataError exception not thrown.") except DataError: pass @@ -131,3 +132,17 @@ def make_verify_code(user, age=timedelta(hours=0), code="12335"): ) db.session.add(verify_code) db.session.commit() + + +@pytest.mark.parametrize('user_attribute, user_value', [ + ('name', 'New User'), + ('email_address', 'newuser@mail.com'), + ('mobile_number', '+4407700900460') +]) +def test_update_user_attribute(client, sample_user, user_attribute, user_value): + assert getattr(sample_user, user_attribute) != user_value + update_dict = { + user_attribute: user_value + } + save_user_attribute(sample_user, update_dict) + assert getattr(sample_user, user_attribute) == user_value From da2fa5b4bcf9787a9457711eea3c1b709fc2cf48 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 31 Oct 2016 18:14:29 +0000 Subject: [PATCH 11/50] move sqlalchemy defaults from booleans to SQL constructs booleans aren't actually allowed, and quietly do nothing also default Services.active to true --- app/models.py | 18 +++++++-------- .../versions/0059_set_services_to_active.py | 23 +++++++++++++++++++ tests/app/conftest.py | 2 +- tests/app/dao/test_services_dao.py | 14 ----------- tests/app/service/test_rest.py | 1 - 5 files changed, 33 insertions(+), 25 deletions(-) create mode 100644 migrations/versions/0059_set_services_to_active.py diff --git a/app/models.py b/app/models.py index 526288990..f5dc10ff9 100644 --- a/app/models.py +++ b/app/models.py @@ -5,7 +5,7 @@ from sqlalchemy.dialects.postgresql import ( UUID, JSON ) -from sqlalchemy import UniqueConstraint, and_ +from sqlalchemy import UniqueConstraint, and_, false, true from sqlalchemy.orm import foreign, remote from notifications_utils.recipients import ( validate_email_address, @@ -58,7 +58,7 @@ class User(db.Model): logged_in_at = db.Column(db.DateTime, nullable=True) failed_login_count = db.Column(db.Integer, nullable=False, default=0) state = db.Column(db.String, nullable=False, default='pending') - platform_admin = db.Column(db.Boolean, nullable=False, default=False) + platform_admin = db.Column(db.Boolean, nullable=False, default=false()) @property def password(self): @@ -115,15 +115,15 @@ class Service(db.Model, Versioned): unique=False, nullable=True, onupdate=datetime.datetime.utcnow) - active = db.Column(db.Boolean, index=False, unique=False, nullable=False) + active = db.Column(db.Boolean, index=False, unique=False, nullable=False, default=true()) message_limit = db.Column(db.BigInteger, index=False, unique=False, nullable=False) users = db.relationship( 'User', secondary=user_to_service, backref=db.backref('user_to_service', lazy='dynamic')) restricted = db.Column(db.Boolean, index=False, unique=False, nullable=False) - research_mode = db.Column(db.Boolean, index=False, unique=False, nullable=False, default=False) - can_send_letters = db.Column(db.Boolean, nullable=False, default=False) + research_mode = db.Column(db.Boolean, index=False, unique=False, nullable=False, default=false()) + can_send_letters = db.Column(db.Boolean, nullable=False, default=false()) email_from = db.Column(db.Text, index=False, unique=True, nullable=False) created_by = db.relationship('User') created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), index=True, nullable=False) @@ -273,7 +273,7 @@ class Template(db.Model): nullable=True, onupdate=datetime.datetime.utcnow) content = db.Column(db.Text, index=False, unique=False, nullable=False) - archived = db.Column(db.Boolean, index=False, nullable=False, default=False) + archived = db.Column(db.Boolean, index=False, nullable=False, default=false()) service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, unique=False, nullable=False) service = db.relationship('Service', backref=db.backref('templates', lazy='dynamic')) subject = db.Column(db.Text, index=False, unique=False, nullable=True) @@ -291,7 +291,7 @@ class TemplateHistory(db.Model): created_at = db.Column(db.DateTime, nullable=False) updated_at = db.Column(db.DateTime) content = db.Column(db.Text, nullable=False) - archived = db.Column(db.Boolean, nullable=False, default=False) + archived = db.Column(db.Boolean, nullable=False, default=false()) service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, nullable=False) service = db.relationship('Service') subject = db.Column(db.Text) @@ -344,7 +344,7 @@ class ProviderDetails(db.Model): identifier = db.Column(db.String, nullable=False) priority = db.Column(db.Integer, nullable=False) notification_type = db.Column(notification_types, nullable=False) - active = db.Column(db.Boolean, default=False) + active = db.Column(db.Boolean, default=false()) JOB_STATUS_PENDING = 'pending' @@ -431,7 +431,7 @@ class VerifyCode(db.Model): code_type = db.Column(db.Enum(*VERIFY_CODE_TYPES, name='verify_code_types'), index=False, unique=False, nullable=False) expiry_datetime = db.Column(db.DateTime, nullable=False) - code_used = db.Column(db.Boolean, default=False) + code_used = db.Column(db.Boolean, default=false()) created_at = db.Column( db.DateTime, index=False, diff --git a/migrations/versions/0059_set_services_to_active.py b/migrations/versions/0059_set_services_to_active.py new file mode 100644 index 000000000..f402d1a70 --- /dev/null +++ b/migrations/versions/0059_set_services_to_active.py @@ -0,0 +1,23 @@ +""" +we weren't previously using the services.active column , and by default it was set to false. lets set all services to +active, so that in the future we can turn it off to signify deactivating a service + +Revision ID: 0059_set_services_to_active +Revises: 0058_add_letters_flag +Create Date: 2016-10-31 15:17:16.716450 + +""" + +# revision identifiers, used by Alembic. +revision = '0059_set_services_to_active' +down_revision = '0058_add_letters_flag' + +from alembic import op + + +def upgrade(): + op.execute('UPDATE services SET active = TRUE') + + +def downgrade(): + op.execute('UPDATE services SET active = FALSE') diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 002c7c13c..3d1af5359 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -139,7 +139,7 @@ def sample_service(notify_db, data = { 'name': service_name, 'message_limit': limit, - 'active': False, + 'active': True, 'restricted': restricted, 'email_from': email_from, 'created_by': user diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index a24f26c0b..ae7eeda06 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -58,7 +58,6 @@ def test_create_service(sample_user): service = Service(name="service_name", email_from="email_from", message_limit=1000, - active=True, restricted=False, created_by=sample_user) dao_create_service(service, sample_user) @@ -77,14 +76,12 @@ def test_cannot_create_two_services_with_same_name(sample_user): service1 = Service(name="service_name", email_from="email_from1", message_limit=1000, - active=True, restricted=False, created_by=sample_user) service2 = Service(name="service_name", email_from="email_from2", message_limit=1000, - active=True, restricted=False, created_by=sample_user) with pytest.raises(IntegrityError) as excinfo: @@ -98,13 +95,11 @@ def test_cannot_create_two_services_with_same_email_from(sample_user): service1 = Service(name="service_name1", email_from="email_from", message_limit=1000, - active=True, restricted=False, created_by=sample_user) service2 = Service(name="service_name2", email_from="email_from", message_limit=1000, - active=True, restricted=False, created_by=sample_user) with pytest.raises(IntegrityError) as excinfo: @@ -118,7 +113,6 @@ def test_cannot_create_service_with_no_user(notify_db_session, sample_user): service = Service(name="service_name", email_from="email_from", message_limit=1000, - active=True, restricted=False, created_by=sample_user) with pytest.raises(FlushError) as excinfo: @@ -130,7 +124,6 @@ def test_should_add_user_to_service(sample_user): service = Service(name="service_name", email_from="email_from", message_limit=1000, - active=True, restricted=False, created_by=sample_user) dao_create_service(service, sample_user) @@ -150,7 +143,6 @@ def test_should_remove_user_from_service(sample_user): service = Service(name="service_name", email_from="email_from", message_limit=1000, - active=True, restricted=False, created_by=sample_user) dao_create_service(service, sample_user) @@ -244,7 +236,6 @@ def test_create_service_creates_a_history_record_with_current_data(sample_user): service = Service(name="service_name", email_from="email_from", message_limit=1000, - active=True, restricted=False, created_by=sample_user) dao_create_service(service, sample_user) @@ -270,7 +261,6 @@ def test_update_service_creates_a_history_record_with_current_data(sample_user): service = Service(name="service_name", email_from="email_from", message_limit=1000, - active=True, restricted=False, created_by=sample_user) dao_create_service(service, sample_user) @@ -299,7 +289,6 @@ def test_create_service_and_history_is_transactional(sample_user): service = Service(name=None, email_from="email_from", message_limit=1000, - active=True, restricted=False, created_by=sample_user) @@ -348,7 +337,6 @@ def test_add_existing_user_to_another_service_doesnot_change_old_permissions(sam service_one = Service(name="service_one", email_from="service_one", message_limit=1000, - active=True, restricted=False, created_by=sample_user) @@ -367,7 +355,6 @@ def test_add_existing_user_to_another_service_doesnot_change_old_permissions(sam service_two = Service(name="service_two", email_from="service_two", message_limit=1000, - active=True, restricted=False, created_by=other_user) dao_create_service(service_two, other_user) @@ -397,7 +384,6 @@ def test_fetch_stats_filters_on_service(sample_notification): service_two = Service(name="service_two", created_by=sample_notification.service.created_by, email_from="hello", - active=False, restricted=False, message_limit=1000) dao_create_service(service_two, sample_notification.service.created_by) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 3959e401d..15c807266 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -285,7 +285,6 @@ def test_should_not_create_service_if_missing_data(notify_api, sample_user): assert resp.status_code == 400 assert json_resp['result'] == 'error' assert 'Missing data for required field.' in json_resp['message']['name'] - assert 'Missing data for required field.' in json_resp['message']['active'] assert 'Missing data for required field.' in json_resp['message']['message_limit'] assert 'Missing data for required field.' in json_resp['message']['restricted'] From 089ac099f3beae9a467bcb71de7cc04d77f175d0 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 1 Nov 2016 16:09:55 +0000 Subject: [PATCH 12/50] POST /service/{id}/deactivate deactivates a service: * set active=False on the service * renames service to "_archived_{old_name}" * archives all templates for the service * revokes all api keys for the service --- app/service/rest.py | 26 ++++++++++ tests/app/service/test_deactivate.py | 75 ++++++++++++++++++++++++++++ 2 files changed, 101 insertions(+) create mode 100644 tests/app/service/test_deactivate.py diff --git a/app/service/rest.py b/app/service/rest.py index 2cc7dab90..4e6d94ba0 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -32,6 +32,9 @@ from app.dao.service_whitelist_dao import ( dao_add_and_commit_whitelisted_contacts, dao_remove_service_whitelist ) +from app.dao.templates_dao import ( + dao_update_template +) from app.dao import notifications_dao from app.dao.provider_statistics_dao import get_fragment_count from app.dao.users_dao import get_model_users @@ -312,6 +315,29 @@ def update_whitelist(service_id): return '', 204 +@service_blueprint.route('//deactivate', methods=['POST']) +def deactivate_service(service_id): + service = dao_fetch_service_by_id(service_id) + + if not service.active: + # assume already inactive, don't change service name + return '', 204 + + service.active = False + service.name = '_archived_' + service.name + service.email_from = '_archived_' + service.email_from + dao_update_service(service) + + for template in service.templates: + template.archived = True + dao_update_template(template) + + for api_key in service.api_keys: + expire_api_key(service.id, api_key.id) + + return '', 204 + + @service_blueprint.route('//billable-units') def get_billable_unit_count(service_id): try: diff --git a/tests/app/service/test_deactivate.py b/tests/app/service/test_deactivate.py new file mode 100644 index 000000000..50e552816 --- /dev/null +++ b/tests/app/service/test_deactivate.py @@ -0,0 +1,75 @@ +import uuid + +import pytest + +from app.models import Service +from tests import create_authorization_header +from tests.app.conftest import ( + sample_template as create_template, + sample_api_key as create_api_key +) + + +def test_deactivate_only_allows_post(client, sample_service): + auth_header = create_authorization_header(service_id=str(sample_service.id)) + response = client.get('/service/{}/deactivate'.format(uuid.uuid4()), headers=[auth_header]) + assert response.status_code == 405 + + +def test_deactivate_service_errors_with_bad_service_id(client, sample_service): + auth_header = create_authorization_header(service_id=str(sample_service.id)) + response = client.post('/service/{}/deactivate'.format(uuid.uuid4()), headers=[auth_header]) + assert response.status_code == 404 + + +def test_deactivating_inactive_service_does_nothing(client, sample_service): + auth_header = create_authorization_header(service_id=str(sample_service.id)) + sample_service.active = False + response = client.post('/service/{}/deactivate'.format(sample_service.id), headers=[auth_header]) + assert response.status_code == 204 + assert sample_service.name == 'Sample service' + + +@pytest.fixture +def deactivated_service(client, notify_db, notify_db_session, sample_service): + create_template(notify_db, notify_db_session, template_name='a') + create_template(notify_db, notify_db_session, template_name='b') + create_api_key(notify_db, notify_db_session) + create_api_key(notify_db, notify_db_session) + + auth_header = create_authorization_header(service_id=str(sample_service.id)) + response = client.post('/service/{}/deactivate'.format(sample_service.id), headers=[auth_header]) + assert response.status_code == 204 + assert response.data == b'' + return sample_service + + +def test_deactivating_service_changes_name_and_email(deactivated_service): + assert deactivated_service.name == '_archived_Sample service' + assert deactivated_service.email_from == '_archived_sample.service' + + +def test_deactivating_service_revokes_api_keys(deactivated_service): + assert deactivated_service.api_keys.count() == 2 + for key in deactivated_service.api_keys: + assert key.expiry_date is not None + assert key.version == 2 + + +def test_deactivating_service_archives_templates(deactivated_service): + assert deactivated_service.templates.count() == 2 + for template in deactivated_service.templates: + assert template.archived is True + assert template.version == 2 + + +def test_deactivating_service_creates_history(deactivated_service): + ServiceHistory = Service.get_history_model() + history = ServiceHistory.query.filter_by( + id=deactivated_service.id + ).order_by( + ServiceHistory.version.desc() + ).first() + + assert history.version == 2 + assert history.active is False From f6de17d6b73284dcf111673d919b14bdab4be67a Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 8 Nov 2016 13:48:14 +0000 Subject: [PATCH 13/50] dont let people accidentally delete their local database by forcing it to run against a different DB (ie test_notifications_api) --- tests/conftest.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/conftest.py b/tests/conftest.py index 11f570de5..b8094852c 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -33,6 +33,8 @@ def client(notify_api): @pytest.fixture(scope='session') def notify_db(notify_api, request): + assert db.engine.url.database != 'notification_api', 'dont run tests against main db' + Migrate(notify_api, db) Manager(db, MigrateCommand) BASE_DIR = os.path.dirname(os.path.dirname(__file__)) From efd2be9acb6c30a9599b7fe8b46e28450cc85643 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 8 Nov 2016 13:49:47 +0000 Subject: [PATCH 14/50] set service to active on creation --- app/dao/services_dao.py | 1 + tests/app/dao/test_services_dao.py | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index c89886f1a..ceb1d1635 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -70,6 +70,7 @@ def dao_create_service(service, user): service.users.append(user) permission_dao.add_default_service_permissions_for_user(user, service) service.id = uuid.uuid4() # must be set now so version history model can use same id + service.active = True service.research_mode = False db.session.add(service) diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index ae7eeda06..6140231e4 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -67,7 +67,8 @@ def test_create_service(sample_user): assert service_db.name == "service_name" assert service_db.id == service.id assert service_db.branding == BRANDING_GOVUK - assert not service_db.research_mode + assert service_db.research_mode is False + assert service.active is True assert sample_user in service_db.users From 3cbacecf1966d9c2da427d7db80729694ed3b402 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 9 Nov 2016 11:32:07 +0000 Subject: [PATCH 15/50] fix non-functional/overengineered existing tests --- tests/app/service/test_rest.py | 93 +++++++++++++++------------------- 1 file changed, 42 insertions(+), 51 deletions(-) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 15c807266..c1983321e 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -11,8 +11,8 @@ from app.dao.services_dao import dao_remove_user_from_service from app.models import User, Organisation from tests import create_authorization_header from tests.app.conftest import ( - sample_service as create_sample_service, - sample_service_permission as create_sample_service_permission, + sample_service as create_service, + sample_service_permission as create_service_permission, sample_user as create_sample_user, sample_notification as create_sample_notification, sample_notification_with_job) @@ -38,50 +38,41 @@ def test_get_service_list(notify_api, service_factory): assert json_resp['data'][2]['name'] == 'three' -def test_get_service_list_by_user(notify_api, sample_user, service_factory): +def test_get_service_list_by_user(notify_db, notify_db_session, client, sample_user, service_factory): + other_user = create_sample_user(notify_db, notify_db_session, email='foo@bar.gov.uk') + service_factory.get('one', sample_user, email_from='one') + service_factory.get('two', sample_user, email_from='two') + service_factory.get('three', other_user, email_from='three') - with notify_api.test_request_context(): - with notify_api.test_client() as client: - service_factory.get('one', sample_user, email_from='one') - service_factory.get('two', sample_user, email_from='two') - service_factory.get('three', sample_user, email_from='three') - - auth_header = create_authorization_header() - response = client.get( - '/service?user_id='.format(sample_user.id), - headers=[auth_header] - ) - json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 200 - assert len(json_resp['data']) == 3 - assert json_resp['data'][0]['name'] == 'one' - assert json_resp['data'][1]['name'] == 'two' - assert json_resp['data'][2]['name'] == 'three' + auth_header = create_authorization_header() + response = client.get( + '/service?user_id={}'.format(sample_user.id), + headers=[auth_header] + ) + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 200 + assert len(json_resp['data']) == 2 + assert json_resp['data'][0]['name'] == 'one' + assert json_resp['data'][1]['name'] == 'two' -def test_get_service_list_by_user_should_return_empty_list_if_no_services(notify_api, service_factory, sample_user): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - new_user = User( - name='Test User', - email_address='new_user@digital.cabinet-office.gov.uk', - password='password', - mobile_number='+447700900986' - ) - save_model_user(new_user) +def test_get_service_list_by_user_should_return_empty_list_if_no_services( + notify_db, + notify_db_session, + client, + sample_service +): + # service is already created by sample user + new_user = create_sample_user(notify_db, notify_db_session, email='foo@bar.gov.uk') - service_factory.get('one', sample_user, email_from='one') - service_factory.get('two', sample_user, email_from='two') - service_factory.get('three', sample_user, email_from='three') - - auth_header = create_authorization_header() - response = client.get( - '/service?user_id={}'.format(new_user.id), - headers=[auth_header] - ) - json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 200 - assert len(json_resp['data']) == 0 + auth_header = create_authorization_header() + response = client.get( + '/service?user_id={}'.format(new_user.id), + headers=[auth_header] + ) + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 200 + assert len(json_resp['data']) == 0 def test_get_service_list_should_return_empty_list_if_no_services(notify_api, notify_db, notify_db_session): @@ -445,7 +436,7 @@ def test_should_not_update_service_with_duplicate_name(notify_api, with notify_api.test_request_context(): with notify_api.test_client() as client: service_name = "another name" - service = create_sample_service( + service = create_service( notify_db, notify_db_session, service_name=service_name, @@ -478,7 +469,7 @@ def test_should_not_update_service_with_duplicate_email_from(notify_api, with notify_api.test_client() as client: email_from = "duplicate.name" service_name = "duplicate name" - service = create_sample_service( + service = create_service( notify_db, notify_db_session, service_name=service_name, @@ -914,7 +905,7 @@ def test_remove_user_from_service(notify_api, notify_db, notify_db_session, samp notify_db_session, email="new@digital.cabinet-office.gov.uk") # Simulates successfully adding a user to the service - second_permission = create_sample_service_permission( + second_permission = create_service_permission( notify_db, notify_db_session, user=second_user) @@ -1018,8 +1009,8 @@ def test_set_reply_to_email_for_service(notify_api, sample_service): def test_get_all_notifications_for_service_in_order(notify_api, notify_db, notify_db_session): with notify_api.test_request_context(), notify_api.test_client() as client: - service_1 = create_sample_service(notify_db, notify_db_session, service_name="1", email_from='1') - service_2 = create_sample_service(notify_db, notify_db_session, service_name="2", email_from='2') + service_1 = create_service(notify_db, notify_db_session, service_name="1", email_from='1') + service_2 = create_service(notify_db, notify_db_session, service_name="2", email_from='2') create_sample_notification(notify_db, notify_db_session, service=service_2) @@ -1239,8 +1230,8 @@ def test_get_services_with_detailed_flag(notify_api, notify_db, notify_db_sessio def test_get_detailed_services_groups_by_service(notify_db, notify_db_session): from app.service.rest import get_detailed_services - service_1 = create_sample_service(notify_db, notify_db_session, service_name="1", email_from='1') - service_2 = create_sample_service(notify_db, notify_db_session, service_name="2", email_from='2') + service_1 = create_service(notify_db, notify_db_session, service_name="1", email_from='1') + service_2 = create_service(notify_db, notify_db_session, service_name="2", email_from='2') create_sample_notification(notify_db, notify_db_session, service=service_1, status='created') create_sample_notification(notify_db, notify_db_session, service=service_2, status='created') @@ -1266,8 +1257,8 @@ def test_get_detailed_services_groups_by_service(notify_db, notify_db_session): def test_get_detailed_services_includes_services_with_no_notifications(notify_db, notify_db_session): from app.service.rest import get_detailed_services - service_1 = create_sample_service(notify_db, notify_db_session, service_name="1", email_from='1') - service_2 = create_sample_service(notify_db, notify_db_session, service_name="2", email_from='2') + service_1 = create_service(notify_db, notify_db_session, service_name="1", email_from='1') + service_2 = create_service(notify_db, notify_db_session, service_name="2", email_from='2') create_sample_notification(notify_db, notify_db_session, service=service_1) From d4a300ec7a7fd3af08a67c7e035d4aca253d72cd Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 9 Nov 2016 11:45:39 +0000 Subject: [PATCH 16/50] add only_active flag to GET /services/ does what it says on the tin --- app/dao/services_dao.py | 11 ++++++++--- app/service/rest.py | 2 +- tests/app/conftest.py | 3 ++- tests/app/service/test_rest.py | 17 +++++++++++++++++ 4 files changed, 28 insertions(+), 5 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index ceb1d1635..6a7be124e 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -28,12 +28,17 @@ from app.models import ( from app.statsd_decorators import statsd -def dao_fetch_all_services(): - return Service.query.order_by( +def dao_fetch_all_services(only_active=False): + query = Service.query.order_by( asc(Service.created_at) ).options( joinedload('users') - ).all() + ) + + if only_active: + query = query.filter(Service.active) + + return query.all() def dao_fetch_service_by_id(service_id): diff --git a/app/service/rest.py b/app/service/rest.py index 4e6d94ba0..fbdbb04bf 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -67,7 +67,7 @@ def get_services(): elif request.args.get('detailed') == 'True': return jsonify(data=get_detailed_services()) else: - services = dao_fetch_all_services() + services = dao_fetch_all_services(only_active=request.args.get('only_active') == 'True') data = service_schema.dump(services, many=True).data return jsonify(data=data) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 3d1af5359..aa4b7b5fd 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -129,6 +129,7 @@ def sample_service(notify_db, notify_db_session, service_name="Sample service", user=None, + active=True, restricted=False, limit=1000, email_from=None): @@ -139,7 +140,7 @@ def sample_service(notify_db, data = { 'name': service_name, 'message_limit': limit, - 'active': True, + 'active': active, 'restricted': restricted, 'email_from': email_from, 'created_by': user diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index c1983321e..42184d6de 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -38,6 +38,23 @@ def test_get_service_list(notify_api, service_factory): assert json_resp['data'][2]['name'] == 'three' +def test_get_service_list_with_only_active_flag(client, service_factory): + inactive = service_factory.get('one', email_from='one') + active = service_factory.get('two', email_from='two') + + inactive.active = False + + auth_header = create_authorization_header() + response = client.get( + '/service?only_active=True', + headers=[auth_header] + ) + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True)) + assert len(json_resp['data']) == 1 + assert json_resp['data'][0]['id'] == str(active.id) + + def test_get_service_list_by_user(notify_db, notify_db_session, client, sample_user, service_factory): other_user = create_sample_user(notify_db, notify_db_session, email='foo@bar.gov.uk') service_factory.get('one', sample_user, email_from='one') From 346d90e3199f0def6a0d75dae23ad8babe13a0b5 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 9 Nov 2016 14:56:54 +0000 Subject: [PATCH 17/50] update V2 error response to {status_code: 403, errors: [error: AuthError, message: token has expired}] } --- app/authentication/auth.py | 8 +- app/errors.py | 10 ++- app/schema_validation/__init__.py | 7 +- .../test_notification_schemas.py | 25 +++--- .../notifications/test_post_notifications.py | 14 +-- tests/app/v2/test_errors.py | 89 +++++++++++++++++++ 6 files changed, 128 insertions(+), 25 deletions(-) create mode 100644 tests/app/v2/test_errors.py diff --git a/app/authentication/auth.py b/app/authentication/auth.py index f8ea32cd4..5b69c71c5 100644 --- a/app/authentication/auth.py +++ b/app/authentication/auth.py @@ -18,8 +18,12 @@ class AuthError(Exception): def to_dict_v2(self): return { 'status_code': self.code, - 'message': self.short_message, - 'fields': self.message + "errors": [ + { + "error": "AuthError", + "message": self.short_message + } + ] } diff --git a/app/errors.py b/app/errors.py index db5826cbd..546f75f0c 100644 --- a/app/errors.py +++ b/app/errors.py @@ -25,9 +25,13 @@ class InvalidRequest(Exception): Version 2 of the public api error response. ''' return { - "status_code": self.code, - "message": self.message, - "fields": self.fields + "status_code": self.status_code, + "errors": [ + { + "error": self.__class__.__name__, + "message": self.message + } + ] } def __str__(self): diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py index 51d43df7b..0f7099939 100644 --- a/app/schema_validation/__init__.py +++ b/app/schema_validation/__init__.py @@ -1,4 +1,6 @@ import json +from collections import OrderedDict + from jsonschema import Draft4Validator, ValidationError @@ -16,12 +18,11 @@ def build_error_message(errors, schema): field = "'{}' {}".format(e.path[0], e.schema.get('validationMessage')) if e.schema.get( 'validationMessage') else e.message s = field.split("'") - field = {s[1]: s[2].strip()} + field = OrderedDict({"error": "ValidationError", "message": {s[1]: s[2].strip()}}) fields.append(field) message = { "status_code": 400, - "message": "Validation error occurred - {}".format(schema['title']), - "fields": fields + "errors": fields } return json.dumps(message) diff --git a/tests/app/v2/notifications/test_notification_schemas.py b/tests/app/v2/notifications/test_notification_schemas.py index ac63ec6d7..d917a8cf7 100644 --- a/tests/app/v2/notifications/test_notification_schemas.py +++ b/tests/app/v2/notifications/test_notification_schemas.py @@ -20,7 +20,7 @@ valid_json_with_optionals = { @pytest.mark.parametrize("input", [valid_json, valid_json_with_optionals]) def test_post_sms_schema_is_valid(input): - validate(input, post_sms_request) + assert validate(input, post_sms_request) == input def test_post_sms_json_schema_bad_uuid_and_missing_phone_number(): @@ -28,11 +28,13 @@ def test_post_sms_json_schema_bad_uuid_and_missing_phone_number(): with pytest.raises(ValidationError) as e: validate(j, post_sms_request) error = json.loads(e.value.message) - assert "POST v2/notifications/sms" in error['message'] - assert len(error.get('fields')) == 2 - assert {"phone_number": "is a required property"} in error['fields'] - assert {"template_id": "not a valid UUID"} in error['fields'] + assert len(error.keys()) == 2 assert error.get('status_code') == 400 + assert len(error.get('errors')) == 2 + assert {'error': 'ValidationError', + 'message': {"phone_number": "is a required property"}} in error['errors'] + assert {'error': 'ValidationError', + 'message': {"template_id": "not a valid UUID"}} in error['errors'] def test_post_sms_schema_with_personalisation_that_is_not_a_dict(): @@ -45,10 +47,11 @@ def test_post_sms_schema_with_personalisation_that_is_not_a_dict(): with pytest.raises(ValidationError) as e: validate(j, post_sms_request) error = json.loads(e.value.message) - assert "POST v2/notifications/sms" in error['message'] - assert len(error.get('fields')) == 1 - assert error['fields'][0] == {"personalisation": "should contain key value pairs"} + assert len(error.get('errors')) == 1 + assert error['errors'] == [{'error': 'ValidationError', + 'message': {"personalisation": "should contain key value pairs"}}] assert error.get('status_code') == 400 + assert len(error.keys()) == 2 valid_response = { @@ -75,7 +78,7 @@ valid_response_with_optionals = { @pytest.mark.parametrize('input', [valid_response]) def test_post_sms_response_schema_is_valid(input): - validate(input, post_sms_response) + assert validate(input, post_sms_response) == input def test_post_sms_response_schema_missing_uri(): @@ -85,5 +88,5 @@ def test_post_sms_response_schema_missing_uri(): validate(j, post_sms_response) error = json.loads(e.value.message) assert error['status_code'] == 400 - assert 'Validation error occurred - response v2/notifications/sms' == error['message'] - assert [{"uri": "is a required property"}] == error['fields'] + assert error['errors'] == [{'error': 'ValidationError', + 'message': {"uri": "is a required property"}}] diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 4b93a7aa5..6512eeff9 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -55,8 +55,8 @@ def test_post_sms_notification_returns_404_and_missing_template(notify_api, samp assert response.headers['Content-type'] == 'application/json' error_json = json.loads(response.get_data(as_text=True)) - assert error_json['message'] == 'Template not found' - assert error_json['fields'] == [{'template': 'Template not found'}] + assert error_json['errors'] == [{"error": "BadRequestError", + "message": 'Template not found'}] def test_post_sms_notification_returns_403_and_well_formed_auth_error(notify_api, sample_template): @@ -76,8 +76,8 @@ def test_post_sms_notification_returns_403_and_well_formed_auth_error(notify_api assert response.headers['Content-type'] == 'application/json' error_resp = json.loads(response.get_data(as_text=True)) assert error_resp['status_code'] == 401 - assert error_resp['message'] == 'Unauthorized, authentication token must be provided' - assert error_resp['fields'] == {'token': ['Unauthorized, authentication token must be provided']} + assert error_resp['errors'] == [{'error': "AuthError", + 'message': 'Unauthorized, authentication token must be provided'}] def test_post_sms_notification_returns_400_and_for_schema_problems(notify_api, sample_template): @@ -98,5 +98,7 @@ def test_post_sms_notification_returns_400_and_for_schema_problems(notify_api, s assert response.headers['Content-type'] == 'application/json' error_resp = json.loads(response.get_data(as_text=True)) assert error_resp['status_code'] == 400 - assert error_resp['message'] == 'Validation error occurred - POST v2/notifications/sms' - assert error_resp['fields'] == [{"template_id": "is a required property"}] + print(error_resp['errors']) + assert error_resp['errors'] == [{'error': 'ValidationError', + 'message': {"template_id": "is a required property"} + }] diff --git a/tests/app/v2/test_errors.py b/tests/app/v2/test_errors.py new file mode 100644 index 000000000..15bd5af87 --- /dev/null +++ b/tests/app/v2/test_errors.py @@ -0,0 +1,89 @@ +import json + +import pytest +from flask import url_for + + +@pytest.fixture(scope='function') +def app_for_test(mocker): + import flask + from flask import Blueprint + from app.authentication.auth import AuthError + from app.v2.errors import BadRequestError, TooManyRequestsError + + app = flask.Flask(__name__) + app.config['TESTING'] = True + + from app.v2.errors import register_errors + blue = Blueprint("v2_under_test", __name__, url_prefix='/v2/under_test') + + @blue.route("/raise_auth_error", methods=["GET"]) + def raising_auth_error(): + raise AuthError("some message", 403) + + @blue.route("/raise_bad_request", methods=["GET"]) + def raising_bad_request(): + raise BadRequestError(message="you forgot the thing") + + @blue.route("/raise_too_many_requests", methods=["GET"]) + def raising_too_many_requests(): + raise TooManyRequestsError(sending_limit="452") + + @blue.route("/raise_validation_error", methods=["GET"]) + def raising_validation_error(): + from app.schema_validation import validate + from app.v2.notifications.notification_schemas import post_sms_request + validate({"template_id": "bad_uuid"}, post_sms_request) + + register_errors(blue) + app.register_blueprint(blue) + + return app + + +def test_auth_error(app_for_test): + with app_for_test.test_request_context(): + with app_for_test.test_client() as client: + response = client.get(url_for('v2_under_test.raising_auth_error')) + assert response.status_code == 403 + error = json.loads(response.get_data(as_text=True)) + assert error == {"status_code": 403, + "errors": [{"error": "AuthError", + "message": "some message"}]} + + +def test_bad_request_error(app_for_test): + with app_for_test.test_request_context(): + with app_for_test.test_client() as client: + response = client.get(url_for('v2_under_test.raising_bad_request')) + assert response.status_code == 400 + error = json.loads(response.get_data(as_text=True)) + assert error == {"status_code": 400, + "errors": [{"error": "BadRequestError", + "message": "you forgot the thing"}]} + + +def test_too_many_requests_error(app_for_test): + with app_for_test.test_request_context(): + with app_for_test.test_client() as client: + response = client.get(url_for('v2_under_test.raising_too_many_requests')) + assert response.status_code == 429 + error = json.loads(response.get_data(as_text=True)) + assert error == {"status_code": 429, + "errors": [{"error": "TooManyRequestsError", + "message": "Exceeded send limits (452) for today"}]} + + +def test_validation_error(app_for_test): + with app_for_test.test_request_context(): + with app_for_test.test_client() as client: + response = client.get(url_for('v2_under_test.raising_validation_error')) + assert response.status_code == 400 + error = json.loads(response.get_data(as_text=True)) + assert len(error.keys()) == 2 + assert error['status_code'] == 400 + assert len(error['errors']) == 2 + assert {'error': 'ValidationError', + 'message': {'phone_number': 'is a required property'}} in error['errors'] + assert {'error': 'ValidationError', + 'message': {'template_id': 'not a valid UUID'}} in error['errors'] From b0e240267a880c1543a3b2a6ac65b57ef3f802dd Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 9 Nov 2016 15:07:23 +0000 Subject: [PATCH 18/50] add only_active flag to get services functionality --- app/dao/services_dao.py | 22 ++++++++++++----- app/service/rest.py | 15 +++++++----- tests/app/service/test_rest.py | 44 +++++++++++++++++++++++++++------- 3 files changed, 60 insertions(+), 21 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 6a7be124e..dde38af32 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -41,22 +41,32 @@ def dao_fetch_all_services(only_active=False): return query.all() -def dao_fetch_service_by_id(service_id): - return Service.query.filter_by( +def dao_fetch_service_by_id(service_id, only_active=False): + query = Service.query.filter_by( id=service_id ).options( joinedload('users') - ).one() + ) + + if only_active: + query = query.filter(Service.active) + + return query.one() -def dao_fetch_all_services_by_user(user_id): - return Service.query.filter( +def dao_fetch_all_services_by_user(user_id, only_active=False): + query = Service.query.filter( Service.users.any(id=user_id) ).order_by( asc(Service.created_at) ).options( joinedload('users') - ).all() + ) + + if only_active: + query = query.filter(Service.active) + + return query.all() def dao_fetch_service_by_id_and_user(service_id, user_id): diff --git a/app/service/rest.py b/app/service/rest.py index fbdbb04bf..78138b434 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -61,13 +61,16 @@ register_errors(service_blueprint) @service_blueprint.route('', methods=['GET']) def get_services(): + only_active = request.args.get('only_active') == 'True' + detailed = request.args.get('detailed') == 'True' user_id = request.args.get('user_id', None) + if user_id: - services = dao_fetch_all_services_by_user(user_id) - elif request.args.get('detailed') == 'True': - return jsonify(data=get_detailed_services()) + services = dao_fetch_all_services_by_user(user_id, only_active) + elif detailed: + return jsonify(data=get_detailed_services(only_active)) else: - services = dao_fetch_all_services(only_active=request.args.get('only_active') == 'True') + services = dao_fetch_all_services(only_active) data = service_schema.dump(services, many=True).data return jsonify(data=data) @@ -267,8 +270,8 @@ def get_detailed_service(service_id, today_only=False): return detailed_service_schema.dump(service).data -def get_detailed_services(): - services = {service.id: service for service in dao_fetch_all_services()} +def get_detailed_services(only_active=False): + services = {service.id: service for service in dao_fetch_all_services(only_active)} stats = dao_fetch_todays_stats_for_all_services() for service_id, rows in itertools.groupby(stats, lambda x: x.service_id): diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 42184d6de..3b3bdc744 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -22,9 +22,9 @@ from app.models import KEY_TYPE_TEST def test_get_service_list(notify_api, service_factory): with notify_api.test_request_context(): with notify_api.test_client() as client: - service_factory.get('one', email_from='one') - service_factory.get('two', email_from='two') - service_factory.get('three', email_from='three') + service_factory.get('one') + service_factory.get('two') + service_factory.get('three') auth_header = create_authorization_header() response = client.get( '/service', @@ -39,8 +39,8 @@ def test_get_service_list(notify_api, service_factory): def test_get_service_list_with_only_active_flag(client, service_factory): - inactive = service_factory.get('one', email_from='one') - active = service_factory.get('two', email_from='two') + inactive = service_factory.get('one') + active = service_factory.get('two') inactive.active = False @@ -55,11 +55,37 @@ def test_get_service_list_with_only_active_flag(client, service_factory): assert json_resp['data'][0]['id'] == str(active.id) +def test_get_service_list_with_user_id_and_only_active_flag( + notify_db, + notify_db_session, + client, + sample_user, + service_factory +): + other_user = create_sample_user(notify_db, notify_db_session, email='foo@bar.gov.uk') + + inactive = service_factory.get('one', user=sample_user) + active = service_factory.get('two', user=sample_user) + from_other_user = service_factory.get('three', user=other_user) + + inactive.active = False + + auth_header = create_authorization_header() + response = client.get( + '/service?user_id={}&only_active=True'.format(sample_user.id), + headers=[auth_header] + ) + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True)) + assert len(json_resp['data']) == 1 + assert json_resp['data'][0]['id'] == str(active.id) + + def test_get_service_list_by_user(notify_db, notify_db_session, client, sample_user, service_factory): other_user = create_sample_user(notify_db, notify_db_session, email='foo@bar.gov.uk') - service_factory.get('one', sample_user, email_from='one') - service_factory.get('two', sample_user, email_from='two') - service_factory.get('three', other_user, email_from='three') + service_factory.get('one', sample_user) + service_factory.get('two', sample_user) + service_factory.get('three', other_user) auth_header = create_authorization_header() response = client.get( @@ -140,7 +166,7 @@ def test_get_service_by_id_should_404_if_no_service(notify_api, notify_db): def test_get_service_by_id_and_user(notify_api, service_factory, sample_user): with notify_api.test_request_context(): with notify_api.test_client() as client: - service = service_factory.get('new service', sample_user, email_from='new.service') + service = service_factory.get('new.service', sample_user) auth_header = create_authorization_header() resp = client.get( '/service/{}?user_id={}'.format(service.id, sample_user.id), From b2149bf02aaf81d38b215698e7d5d8eb7e6c837e Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 10 Nov 2016 10:38:36 +0000 Subject: [PATCH 19/50] undo unnecessary change to models --- app/models.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/app/models.py b/app/models.py index f5dc10ff9..cf46530ca 100644 --- a/app/models.py +++ b/app/models.py @@ -58,7 +58,7 @@ class User(db.Model): logged_in_at = db.Column(db.DateTime, nullable=True) failed_login_count = db.Column(db.Integer, nullable=False, default=0) state = db.Column(db.String, nullable=False, default='pending') - platform_admin = db.Column(db.Boolean, nullable=False, default=false()) + platform_admin = db.Column(db.Boolean, nullable=False, default=False) @property def password(self): @@ -115,15 +115,15 @@ class Service(db.Model, Versioned): unique=False, nullable=True, onupdate=datetime.datetime.utcnow) - active = db.Column(db.Boolean, index=False, unique=False, nullable=False, default=true()) + active = db.Column(db.Boolean, index=False, unique=False, nullable=False, default=True) message_limit = db.Column(db.BigInteger, index=False, unique=False, nullable=False) users = db.relationship( 'User', secondary=user_to_service, backref=db.backref('user_to_service', lazy='dynamic')) restricted = db.Column(db.Boolean, index=False, unique=False, nullable=False) - research_mode = db.Column(db.Boolean, index=False, unique=False, nullable=False, default=false()) - can_send_letters = db.Column(db.Boolean, nullable=False, default=false()) + research_mode = db.Column(db.Boolean, index=False, unique=False, nullable=False, default=False) + can_send_letters = db.Column(db.Boolean, nullable=False, default=False) email_from = db.Column(db.Text, index=False, unique=True, nullable=False) created_by = db.relationship('User') created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), index=True, nullable=False) @@ -273,7 +273,7 @@ class Template(db.Model): nullable=True, onupdate=datetime.datetime.utcnow) content = db.Column(db.Text, index=False, unique=False, nullable=False) - archived = db.Column(db.Boolean, index=False, nullable=False, default=false()) + archived = db.Column(db.Boolean, index=False, nullable=False, default=False) service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, unique=False, nullable=False) service = db.relationship('Service', backref=db.backref('templates', lazy='dynamic')) subject = db.Column(db.Text, index=False, unique=False, nullable=True) @@ -291,7 +291,7 @@ class TemplateHistory(db.Model): created_at = db.Column(db.DateTime, nullable=False) updated_at = db.Column(db.DateTime) content = db.Column(db.Text, nullable=False) - archived = db.Column(db.Boolean, nullable=False, default=false()) + archived = db.Column(db.Boolean, nullable=False, default=False) service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, nullable=False) service = db.relationship('Service') subject = db.Column(db.Text) @@ -344,7 +344,7 @@ class ProviderDetails(db.Model): identifier = db.Column(db.String, nullable=False) priority = db.Column(db.Integer, nullable=False) notification_type = db.Column(notification_types, nullable=False) - active = db.Column(db.Boolean, default=false()) + active = db.Column(db.Boolean, default=False) JOB_STATUS_PENDING = 'pending' @@ -431,7 +431,7 @@ class VerifyCode(db.Model): code_type = db.Column(db.Enum(*VERIFY_CODE_TYPES, name='verify_code_types'), index=False, unique=False, nullable=False) expiry_datetime = db.Column(db.DateTime, nullable=False) - code_used = db.Column(db.Boolean, default=false()) + code_used = db.Column(db.Boolean, default=False) created_at = db.Column( db.DateTime, index=False, From e8c3a5cddef6b9887b5aa771dbb121a6a1a0c3de Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 10 Nov 2016 11:07:12 +0000 Subject: [PATCH 20/50] add check for inactive services to auth handler cleaned up some auth code to marginally improve efficiency of error checking and hopefully make it easier to read fixed some incorrect auth headers in the deactivate tests --- app/authentication/auth.py | 22 ++++++++++--------- app/models.py | 2 +- .../app/authentication/test_authentication.py | 11 ++++++++++ tests/app/service/test_deactivate.py | 10 ++++----- 4 files changed, 29 insertions(+), 16 deletions(-) diff --git a/app/authentication/auth.py b/app/authentication/auth.py index 841222279..68e7acf8b 100644 --- a/app/authentication/auth.py +++ b/app/authentication/auth.py @@ -46,10 +46,19 @@ def requires_auth(): return handle_admin_key(auth_token, current_app.config.get('ADMIN_CLIENT_SECRET')) try: - api_keys = get_model_api_keys(client) + service = dao_fetch_service_by_id(client) except DataError: raise AuthError("Invalid token: service id is not the right data type", 403) - for api_key in api_keys: + except NoResultFound: + raise AuthError("Invalid token: service not found", 403) + + if not service.api_keys: + raise AuthError("Invalid token: service has no API keys", 403) + + if not service.active: + raise AuthError("Invalid token: service is archived", 403) + + for api_key in service.api_keys: try: get_decode_errors(auth_token, api_key.unsigned_secret) except TokenDecodeError: @@ -60,15 +69,8 @@ def requires_auth(): _request_ctx_stack.top.api_user = api_key return - - try: - dao_fetch_service_by_id(client) - except NoResultFound: - raise AuthError("Invalid token: service not found", 403) - - if not api_keys: - raise AuthError("Invalid token: service has no API keys", 403) else: + # service has API keys, but none matching the one the user provided raise AuthError("Invalid token: signature, api token is not valid", 403) diff --git a/app/models.py b/app/models.py index cf46530ca..75b9615fc 100644 --- a/app/models.py +++ b/app/models.py @@ -188,7 +188,7 @@ class ApiKey(db.Model, Versioned): name = db.Column(db.String(255), nullable=False) secret = db.Column(db.String(255), unique=True, nullable=False) service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, nullable=False) - service = db.relationship('Service', backref=db.backref('api_keys', lazy='dynamic')) + service = db.relationship('Service', backref='api_keys') key_type = db.Column(db.String(255), db.ForeignKey('key_types.name'), index=True, nullable=False) expiry_date = db.Column(db.DateTime) created_at = db.Column( diff --git a/tests/app/authentication/test_authentication.py b/tests/app/authentication/test_authentication.py index 3a6d5b4d2..7c445687a 100644 --- a/tests/app/authentication/test_authentication.py +++ b/tests/app/authentication/test_authentication.py @@ -223,6 +223,17 @@ def test_authentication_returns_error_when_service_doesnt_exit( assert error_message['message'] == {'token': ['Invalid token: service not found']} +def test_authentication_returns_error_when_service_inactive(client, sample_api_key): + sample_api_key.service.active = False + token = create_jwt_token(secret=str(sample_api_key.id), client_id=str(sample_api_key.service_id)) + + response = client.get('/service', headers={'Authorization': 'Bearer {}'.format(token)}) + + assert response.status_code == 403 + error_message = json.loads(response.get_data()) + assert error_message['message'] == {'token': ['Invalid token: service is archived']} + + def test_authentication_returns_error_when_service_has_no_secrets(notify_api, sample_service, fake_uuid): diff --git a/tests/app/service/test_deactivate.py b/tests/app/service/test_deactivate.py index 50e552816..d81de0e10 100644 --- a/tests/app/service/test_deactivate.py +++ b/tests/app/service/test_deactivate.py @@ -11,19 +11,19 @@ from tests.app.conftest import ( def test_deactivate_only_allows_post(client, sample_service): - auth_header = create_authorization_header(service_id=str(sample_service.id)) + auth_header = create_authorization_header() response = client.get('/service/{}/deactivate'.format(uuid.uuid4()), headers=[auth_header]) assert response.status_code == 405 def test_deactivate_service_errors_with_bad_service_id(client, sample_service): - auth_header = create_authorization_header(service_id=str(sample_service.id)) + auth_header = create_authorization_header() response = client.post('/service/{}/deactivate'.format(uuid.uuid4()), headers=[auth_header]) assert response.status_code == 404 def test_deactivating_inactive_service_does_nothing(client, sample_service): - auth_header = create_authorization_header(service_id=str(sample_service.id)) + auth_header = create_authorization_header() sample_service.active = False response = client.post('/service/{}/deactivate'.format(sample_service.id), headers=[auth_header]) assert response.status_code == 204 @@ -37,7 +37,7 @@ def deactivated_service(client, notify_db, notify_db_session, sample_service): create_api_key(notify_db, notify_db_session) create_api_key(notify_db, notify_db_session) - auth_header = create_authorization_header(service_id=str(sample_service.id)) + auth_header = create_authorization_header() response = client.post('/service/{}/deactivate'.format(sample_service.id), headers=[auth_header]) assert response.status_code == 204 assert response.data == b'' @@ -50,7 +50,7 @@ def test_deactivating_service_changes_name_and_email(deactivated_service): def test_deactivating_service_revokes_api_keys(deactivated_service): - assert deactivated_service.api_keys.count() == 2 + assert len(deactivated_service.api_keys) == 2 for key in deactivated_service.api_keys: assert key.expiry_date is not None assert key.version == 2 From 8474344c9a57a2b72bc37f63d3b27e8d45eaefee Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 1 Nov 2016 15:19:56 +0000 Subject: [PATCH 21/50] Ignore case and whitespace in personalisation keys From a support ticket: > it's possible to add a personalisation token with trailing whitespace > (eg. "key " rather than "key"). Can this be trimmed in the UI to guard > against this? (one of our devs copied and pasted it from a document > and inadvertently included the space) > Nothing major but caused a few hours of investigations! Rather than trim the placeholder in the template, we should treat placeholders in API calls the same way we do with CSV files, ie we ignore case and spacing in the name of the placeholder. So `(( First Name))` is equivalent to `((first_name))`, and both would be populated with a dictionary like `{'firstName': 'Chris'}`. Depends on: - [x] https://github.com/alphagov/notifications-utils/pull/77 --- app/celery/tasks.py | 5 +---- requirements.txt | 2 +- tests/app/conftest.py | 3 ++- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 9bf47c9d7..3ab7043a2 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -72,10 +72,7 @@ def process_job(job_id): 'job': str(job.id), 'to': recipient, 'row_number': row_number, - 'personalisation': { - key: personalisation.get(key) - for key in template.placeholders - } + 'personalisation': dict(personalisation) }) if template.template_type == SMS_TYPE: diff --git a/requirements.txt b/requirements.txt index a71037824..08834bd7b 100644 --- a/requirements.txt +++ b/requirements.txt @@ -21,6 +21,6 @@ jsonschema==2.5.1 git+https://github.com/alphagov/notifications-python-client.git@1.3.0#egg=notifications-python-client==1.3.0 -git+https://github.com/alphagov/notifications-utils.git@9.1.1#egg=notifications-utils==9.1.1 +git+https://github.com/alphagov/notifications-utils.git@9.2.1#egg=notifications-utils==9.2.1 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 002c7c13c..2817e5dcc 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -190,7 +190,8 @@ def sample_template(notify_db, @pytest.fixture(scope='function') def sample_template_with_placeholders(notify_db, notify_db_session): - return sample_template(notify_db, notify_db_session, content="Hello ((name))\nYour thing is due soon") + # deliberate space and title case in placeholder + return sample_template(notify_db, notify_db_session, content="Hello (( Name))\nYour thing is due soon") @pytest.fixture(scope='function') From 988bf061320988ebb2f6fa6977d83ac8ece5701a Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 1 Nov 2016 15:20:28 +0000 Subject: [PATCH 22/50] Update error message for invalid email addresses It has a trailing full stop since: https://github.com/alphagov/notifications-utils/pull/76 --- tests/app/invite/test_invite_rest.py | 2 +- tests/app/notifications/rest/test_send_notification.py | 4 ++-- tests/app/user/test_rest.py | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/app/invite/test_invite_rest.py b/tests/app/invite/test_invite_rest.py index 301bf6e69..9cf9c2f13 100644 --- a/tests/app/invite/test_invite_rest.py +++ b/tests/app/invite/test_invite_rest.py @@ -85,7 +85,7 @@ def test_create_invited_user_invalid_email(notify_api, sample_service, mocker): assert response.status_code == 400 json_resp = json.loads(response.get_data(as_text=True)) assert json_resp['result'] == 'error' - assert json_resp['message'] == {'email_address': ['Not a valid email address']} + assert json_resp['message'] == {'email_address': ['Not a valid email address.']} app.celery.tasks.send_email.apply_async.assert_not_called() diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index a28590c40..e87b6410e 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -306,7 +306,7 @@ def test_should_reject_email_notification_with_bad_email(notify_api, sample_emai mocked.apply_async.assert_not_called() assert response.status_code == 400 assert data['result'] == 'error' - assert data['message']['to'][0] == 'Not a valid email address' + assert data['message']['to'][0] == 'Not a valid email address.' @freeze_time("2016-01-01 11:09:00.061258") @@ -913,7 +913,7 @@ def test_create_template_raises_invalid_request_exception_with_missing_personali from app.notifications.rest import create_template_object_for_notification with pytest.raises(InvalidRequest) as e: create_template_object_for_notification(template, {}) - assert {'template': ['Missing personalisation: name']} == e.value.message + assert {'template': ['Missing personalisation: Name']} == e.value.message def test_create_template_raises_invalid_request_exception_with_too_much_personalisation_data( diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py index 14b26d387..2de1f7c38 100644 --- a/tests/app/user/test_rest.py +++ b/tests/app/user/test_rest.py @@ -470,7 +470,7 @@ def test_send_user_reset_password_should_return_400_when_data_is_not_email_addre headers=[('Content-Type', 'application/json'), auth_header]) assert resp.status_code == 400 - assert json.loads(resp.get_data(as_text=True))['message'] == {'email': ['Not a valid email address']} + assert json.loads(resp.get_data(as_text=True))['message'] == {'email': ['Not a valid email address.']} @freeze_time("2016-01-01 11:09:00.061258") From 8b64aa7e79f63a33aa4e9436f6ba1631f6b66575 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 10 Nov 2016 12:07:29 +0000 Subject: [PATCH 23/50] Use POST endpoint for updating a user attr --- app/user/rest.py | 2 +- tests/app/user/test_rest.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/user/rest.py b/app/user/rest.py index e41e10877..a188f162e 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -69,7 +69,7 @@ def update_user(user_id): return jsonify(data=user_schema.dump(user_to_update).data), 200 -@user.route('//update-attribute', methods=['PUT']) +@user.route('/', methods=['POST']) def update_user_attribute(user_id): user_to_update = get_user_by_id(user_id=user_id) req_json = request.get_json() diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py index d9261fbb8..951931e01 100644 --- a/tests/app/user/test_rest.py +++ b/tests/app/user/test_rest.py @@ -186,7 +186,7 @@ def test_put_user(notify_api, notify_db, notify_db_session, sample_service): ('email_address', 'newuser@mail.com'), ('mobile_number', '+4407700900460') ]) -def test_put_user_attribute(client, sample_user, user_attribute, user_value): +def test_post_user_attribute(client, sample_user, user_attribute, user_value): assert getattr(sample_user, user_attribute) != user_value update_dict = { user_attribute: user_value @@ -194,7 +194,7 @@ def test_put_user_attribute(client, sample_user, user_attribute, user_value): auth_header = create_authorization_header() headers = [('Content-Type', 'application/json'), auth_header] - resp = client.put( + resp = client.post( url_for('user.update_user_attribute', user_id=sample_user.id), data=json.dumps(update_dict), headers=headers) From f85ee547075baa83f20431b6fa9a022827f5abff Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Thu, 10 Nov 2016 13:09:25 +0000 Subject: [PATCH 24/50] Refactor stuff + stricter validation for updating only ALLOWED user attrs --- app/schemas.py | 5 +++-- tests/app/dao/test_users_dao.py | 5 +---- tests/app/test_schemas.py | 25 ++++++++++++++++++++++--- 3 files changed, 26 insertions(+), 9 deletions(-) diff --git a/app/schemas.py b/app/schemas.py index b9f75cb2c..7f52b9541 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -110,8 +110,9 @@ class UserUpdateAttributeSchema(BaseSchema): class Meta: model = models.User exclude = ( - "updated_at", "created_at", "user_to_service", - "_password", "verify_codes") + 'id', 'updated_at', 'created_at', 'user_to_service', + '_password', 'verify_codes', 'logged_in_at', 'password_changed_at', + 'failed_login_count', 'state', 'platform_admin') strict = True @validates('name') diff --git a/tests/app/dao/test_users_dao.py b/tests/app/dao/test_users_dao.py index 7d1901f0d..cc814b160 100644 --- a/tests/app/dao/test_users_dao.py +++ b/tests/app/dao/test_users_dao.py @@ -64,11 +64,8 @@ def test_get_user_not_exists(notify_api, notify_db, notify_db_session, fake_uuid def test_get_user_invalid_id(notify_api, notify_db, notify_db_session): - try: + with pytest.raises(DataError): get_user_by_id(user_id="blah") - pytest.fail("DataError exception not thrown.") - except DataError: - pass def test_delete_users(notify_api, notify_db, notify_db_session, sample_user): diff --git a/tests/app/test_schemas.py b/tests/app/test_schemas.py index 5f2bc4371..1d66f3290 100644 --- a/tests/app/test_schemas.py +++ b/tests/app/test_schemas.py @@ -1,5 +1,7 @@ import pytest +from marshmallow import ValidationError + def test_job_schema_doesnt_return_notifications(sample_notification_with_job): from app.schemas import job_schema @@ -32,7 +34,7 @@ def test_notification_schema_adds_api_key_name(sample_notification_with_api_key) ('email_address', 'newuser@mail.com'), ('mobile_number', '+4407700900460') ]) -def test_user_schema_accepts_valid_attributes(user_attribute, user_value): +def test_user_update_schema_accepts_valid_attribute_pairs(user_attribute, user_value): update_dict = { user_attribute: user_value } @@ -48,11 +50,28 @@ def test_user_schema_accepts_valid_attributes(user_attribute, user_value): ('email_address', 'bademail@...com'), ('mobile_number', '+44077009') ]) -def test_user_schema_rejects_invalid_attributes(user_attribute, user_value): +def test_user_update_schema_rejects_invalid_attribute_pairs(user_attribute, user_value): from app.schemas import user_update_schema_load_json update_dict = { user_attribute: user_value } - with pytest.raises(Exception): + with pytest.raises(ValidationError): data, errors = user_update_schema_load_json.load(update_dict) + + +@pytest.mark.parametrize('user_attribute', [ + 'id', 'updated_at', 'created_at', 'user_to_service', + '_password', 'verify_codes', 'logged_in_at', 'password_changed_at', + 'failed_login_count', 'state', 'platform_admin' +]) +def test_user_update_schema_rejects_disallowed_attribute_keys(user_attribute): + update_dict = { + user_attribute: 'not important' + } + from app.schemas import user_update_schema_load_json + + with pytest.raises(ValidationError) as excinfo: + data, errors = user_update_schema_load_json.load(update_dict) + + assert excinfo.value.messages['_schema'][0] == 'Unknown field name {}'.format(user_attribute) From 7d763260ce93b960a26a2fe1d9a34b62ab143b4d Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 10 Nov 2016 14:53:39 +0000 Subject: [PATCH 25/50] Update format of the errors for DataError and exception --- app/v2/errors.py | 8 ++++---- tests/app/v2/test_errors.py | 30 +++++++++++++++++++++++++++++- 2 files changed, 33 insertions(+), 5 deletions(-) diff --git a/app/v2/errors.py b/app/v2/errors.py index 8c98e48f8..26cc3b36e 100644 --- a/app/v2/errors.py +++ b/app/v2/errors.py @@ -1,10 +1,8 @@ import json - from flask import jsonify, current_app from jsonschema import ValidationError from sqlalchemy.exc import DataError from sqlalchemy.orm.exc import NoResultFound - from app.authentication.auth import AuthError from app.errors import InvalidRequest @@ -42,7 +40,8 @@ def register_errors(blueprint): @blueprint.errorhandler(DataError) def no_result_found(e): current_app.logger.exception(e) - return jsonify(message="No result found"), 404 + return jsonify(status_code=404, + errors=[{"error": e.__class__.__name__, "message": "No result found"}]), 404 @blueprint.errorhandler(AuthError) def auth_error(error): @@ -51,4 +50,5 @@ def register_errors(blueprint): @blueprint.errorhandler(Exception) def internal_server_error(error): current_app.logger.exception(error) - return jsonify(message='Internal server error'), 500 + return jsonify(status_code=500, + errors=[{"error": error.__class__.__name__, "message": 'Internal server error'}]), 500 diff --git a/tests/app/v2/test_errors.py b/tests/app/v2/test_errors.py index 15bd5af87..a95ddb680 100644 --- a/tests/app/v2/test_errors.py +++ b/tests/app/v2/test_errors.py @@ -1,7 +1,7 @@ import json - import pytest from flask import url_for +from sqlalchemy.exc import DataError @pytest.fixture(scope='function') @@ -35,6 +35,14 @@ def app_for_test(mocker): from app.v2.notifications.notification_schemas import post_sms_request validate({"template_id": "bad_uuid"}, post_sms_request) + @blue.route("raise_data_error", methods=["GET"]) + def raising_data_error(): + raise DataError("There was a db problem", "params", "orig") + + @blue.route("raise_exception", methods=["GET"]) + def raising_exception(): + raise AssertionError("Raising any old exception") + register_errors(blue) app.register_blueprint(blue) @@ -87,3 +95,23 @@ def test_validation_error(app_for_test): 'message': {'phone_number': 'is a required property'}} in error['errors'] assert {'error': 'ValidationError', 'message': {'template_id': 'not a valid UUID'}} in error['errors'] + + +def test_data_errors(app_for_test): + with app_for_test.test_request_context(): + with app_for_test.test_client() as client: + response = client.get(url_for('v2_under_test.raising_data_error')) + assert response.status_code == 404 + error = json.loads(response.get_data(as_text=True)) + assert error == {"status_code": 404, + "errors": [{"error": "DataError", "message": "No result found"}]} + + +def test_internal_server_error_handler(app_for_test): + with app_for_test.test_request_context(): + with app_for_test.test_client() as client: + response = client.get(url_for("v2_under_test.raising_exception")) + assert response.status_code == 500 + error = json.loads(response.get_data(as_text=True)) + assert error == {"status_code": 500, + "errors": [{"error": "AssertionError", "message": "Internal server error"}]} From c758694b988a348ee093e7c7446489b04c68b757 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 10 Nov 2016 16:30:51 +0000 Subject: [PATCH 26/50] Change validation error message to a string from a dict. --- app/schema_validation/__init__.py | 2 +- app/schema_validation/definitions.py | 2 +- tests/app/v2/notifications/test_notification_schemas.py | 8 ++++---- tests/app/v2/notifications/test_post_notifications.py | 4 ++-- tests/app/v2/test_errors.py | 5 +++-- 5 files changed, 11 insertions(+), 10 deletions(-) diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py index 0f7099939..4cb27b12b 100644 --- a/app/schema_validation/__init__.py +++ b/app/schema_validation/__init__.py @@ -18,7 +18,7 @@ def build_error_message(errors, schema): field = "'{}' {}".format(e.path[0], e.schema.get('validationMessage')) if e.schema.get( 'validationMessage') else e.message s = field.split("'") - field = OrderedDict({"error": "ValidationError", "message": {s[1]: s[2].strip()}}) + field = OrderedDict({"error": "ValidationError", "message": "{}{}".format(s[1], s[2])}) fields.append(field) message = { "status_code": 400, diff --git a/app/schema_validation/definitions.py b/app/schema_validation/definitions.py index e65566d7b..3841b64a8 100644 --- a/app/schema_validation/definitions.py +++ b/app/schema_validation/definitions.py @@ -6,7 +6,7 @@ If the definition is specific to a version put it in a definition file in the ve uuid = { "type": "string", "pattern": "^[a-fA-F0-9]{8}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{12}$", - "validationMessage": "not a valid UUID", + "validationMessage": "is not a valid UUID", "code": "1001", # yet to be implemented "link": "link to our error documentation not yet implemented" } diff --git a/tests/app/v2/notifications/test_notification_schemas.py b/tests/app/v2/notifications/test_notification_schemas.py index d917a8cf7..74c734c2f 100644 --- a/tests/app/v2/notifications/test_notification_schemas.py +++ b/tests/app/v2/notifications/test_notification_schemas.py @@ -32,9 +32,9 @@ def test_post_sms_json_schema_bad_uuid_and_missing_phone_number(): assert error.get('status_code') == 400 assert len(error.get('errors')) == 2 assert {'error': 'ValidationError', - 'message': {"phone_number": "is a required property"}} in error['errors'] + 'message': "phone_number is a required property"} in error['errors'] assert {'error': 'ValidationError', - 'message': {"template_id": "not a valid UUID"}} in error['errors'] + 'message': "template_id is not a valid UUID"} in error['errors'] def test_post_sms_schema_with_personalisation_that_is_not_a_dict(): @@ -49,7 +49,7 @@ def test_post_sms_schema_with_personalisation_that_is_not_a_dict(): error = json.loads(e.value.message) assert len(error.get('errors')) == 1 assert error['errors'] == [{'error': 'ValidationError', - 'message': {"personalisation": "should contain key value pairs"}}] + 'message': "personalisation should contain key value pairs"}] assert error.get('status_code') == 400 assert len(error.keys()) == 2 @@ -89,4 +89,4 @@ def test_post_sms_response_schema_missing_uri(): error = json.loads(e.value.message) assert error['status_code'] == 400 assert error['errors'] == [{'error': 'ValidationError', - 'message': {"uri": "is a required property"}}] + 'message': "uri is a required property"}] diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 6512eeff9..c02e2d8e9 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -55,6 +55,7 @@ def test_post_sms_notification_returns_404_and_missing_template(notify_api, samp assert response.headers['Content-type'] == 'application/json' error_json = json.loads(response.get_data(as_text=True)) + assert error_json['status_code'] == 400 assert error_json['errors'] == [{"error": "BadRequestError", "message": 'Template not found'}] @@ -98,7 +99,6 @@ def test_post_sms_notification_returns_400_and_for_schema_problems(notify_api, s assert response.headers['Content-type'] == 'application/json' error_resp = json.loads(response.get_data(as_text=True)) assert error_resp['status_code'] == 400 - print(error_resp['errors']) assert error_resp['errors'] == [{'error': 'ValidationError', - 'message': {"template_id": "is a required property"} + 'message': "template_id is a required property" }] diff --git a/tests/app/v2/test_errors.py b/tests/app/v2/test_errors.py index a95ddb680..5f0ed0f81 100644 --- a/tests/app/v2/test_errors.py +++ b/tests/app/v2/test_errors.py @@ -88,13 +88,14 @@ def test_validation_error(app_for_test): response = client.get(url_for('v2_under_test.raising_validation_error')) assert response.status_code == 400 error = json.loads(response.get_data(as_text=True)) + print(error) assert len(error.keys()) == 2 assert error['status_code'] == 400 assert len(error['errors']) == 2 assert {'error': 'ValidationError', - 'message': {'phone_number': 'is a required property'}} in error['errors'] + 'message': "phone_number is a required property"} in error['errors'] assert {'error': 'ValidationError', - 'message': {'template_id': 'not a valid UUID'}} in error['errors'] + 'message': "template_id is not a valid UUID"} in error['errors'] def test_data_errors(app_for_test): From 6501a871bc26981e274b3780206175be9db5c027 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 7 Nov 2016 16:20:41 +0000 Subject: [PATCH 27/50] Add letter to possible template types in DB A letter type was added to the `enum` in the `Template` model at the same it was added to the `Notification` model. But the migration was only done for the `notifications` table, not the `templates` table. See: https://github.com/alphagov/notifications-api/commit/25db1bce#diff-516aab258e161fc65e7564dabd2c625aR19 This commit does the migration to add `letter` as a possible value for the `template_type` column, which is a bit fiddly because `enum`s. Before: ``` notification_api=# select enum_range(null::template_type); enum_range ------------- {sms,email} (1 row) ``` After upgrade: ``` notification_api=# select enum_range(null::template_type); enum_range -------------------- {sms,email,letter} (1 row) ``` After downgrade ``` notification_api=# select enum_range(null::template_type); enum_range ------------- {sms,email} (1 row) ``` --- .../versions/0059_add_letter_template_type.py | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 migrations/versions/0059_add_letter_template_type.py diff --git a/migrations/versions/0059_add_letter_template_type.py b/migrations/versions/0059_add_letter_template_type.py new file mode 100644 index 000000000..2b4cb6eea --- /dev/null +++ b/migrations/versions/0059_add_letter_template_type.py @@ -0,0 +1,56 @@ +"""empty message + +Revision ID: f266fb67597a +Revises: 0058_add_letters_flag +Create Date: 2016-11-07 16:13:18.961527 + +""" + +from alembic import op +import sqlalchemy as sa + +# revision identifiers, used by Alembic. +revision = '0059_add_letter_template_type' +down_revision = '0058_add_letters_flag' + + +name = 'template_type' +tmp_name = 'tmp_' + name + +old_options = ('sms', 'email') +new_options = old_options + ('letter',) + +new_type = sa.Enum(*new_options, name=name) +old_type = sa.Enum(*old_options, name=name) + +tcr = sa.sql.table( + 'templates', + sa.Column('template_type', new_type, nullable=False) +) + + +def upgrade(): + op.execute('ALTER TYPE ' + name + ' RENAME TO ' + tmp_name) + + new_type.create(op.get_bind()) + op.execute( + 'ALTER TABLE templates ALTER COLUMN template_type ' + + 'TYPE ' + name + ' USING template_type::text::' + name + ) + op.execute('DROP TYPE ' + tmp_name) + + +def downgrade(): + # Convert 'letter' template into 'email' + op.execute( + tcr.update().where(tcr.c.template_type=='letter').values(template_type='email') + ) + + op.execute('ALTER TYPE ' + name + ' RENAME TO ' + tmp_name) + + old_type.create(op.get_bind()) + op.execute( + 'ALTER TABLE templates ALTER COLUMN template_type ' + + 'TYPE ' + name + ' USING template_type::text::' + name + ) + op.execute('DROP TYPE ' + tmp_name) From eafbbd9809d356572967ba6d28fcae1e477c3b17 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 11 Nov 2016 10:41:39 +0000 Subject: [PATCH 28/50] Refactor send_sms and send_email to use common code to persist the notificaiton. --- app/celery/tasks.py | 61 ++++++++---- app/models.py | 8 +- app/notifications/process_notifications.py | 9 +- tests/app/celery/test_tasks.py | 102 ++++++++++----------- 4 files changed, 101 insertions(+), 79 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 9bf47c9d7..778ff8b29 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -17,7 +17,7 @@ from app.dao.jobs_dao import ( dao_update_job, dao_get_job_by_id ) -from app.dao.notifications_dao import (dao_create_notification) +from app.dao.notifications_dao import dao_create_notification from app.dao.services_dao import dao_fetch_service_by_id, fetch_todays_total_message_count from app.dao.templates_dao import dao_get_template_by_id from app.models import ( @@ -26,6 +26,7 @@ from app.models import ( SMS_TYPE, KEY_TYPE_NORMAL ) +from app.notifications.process_notifications import persist_notification from app.service.utils import service_allowed_to_send_to from app.statsd_decorators import statsd @@ -41,16 +42,7 @@ def process_job(job_id): service = job.service - total_sent = fetch_todays_total_message_count(service.id) - - if total_sent + job.notification_count > service.message_limit: - job.job_status = 'sending limits exceeded' - job.processing_finished = datetime.utcnow() - dao_update_job(job) - current_app.logger.info( - "Job {} size {} error. Sending limits {} exceeded".format( - job_id, job.notification_count, service.message_limit) - ) + if __sending_limits_for_job_exceeded(service, job, job_id): return job.job_status = 'in progress' @@ -106,6 +98,21 @@ def process_job(job_id): ) +def __sending_limits_for_job_exceeded(service, job, job_id): + total_sent = fetch_todays_total_message_count(service.id) + + if total_sent + job.notification_count > service.message_limit: + job.job_status = 'sending limits exceeded' + job.processing_finished = datetime.utcnow() + dao_update_job(job) + current_app.logger.info( + "Job {} size {} error. Sending limits {} exceeded".format( + job_id, job.notification_count, service.message_limit) + ) + return True + return False + + @notify_celery.task(bind=True, name="send-sms", max_retries=5, default_retry_delay=300) @statsd(namespace="tasks") def send_sms(self, @@ -125,11 +132,18 @@ def send_sms(self, return try: - dao_create_notification( - Notification.from_api_request( - created_at, notification, notification_id, service.id, SMS_TYPE, api_key_id, key_type - ) - ) + persist_notification(template_id=notification['template'], + template_version=notification['template_version'], + recipient=notification['to'], + service_id=service.id, + personalisation=notification.get('personalisation'), + notification_type=SMS_TYPE, + api_key_id=api_key_id, + key_type=key_type, + job_id=notification.get('job', None), + job_row_number=notification.get('row_number', None), + ) + provider_tasks.deliver_sms.apply_async( [notification_id], queue='send-sms' if not service.research_mode else 'research-mode' @@ -166,10 +180,17 @@ def send_email(self, service_id, return try: - dao_create_notification( - Notification.from_api_request( - created_at, notification, notification_id, service.id, EMAIL_TYPE, api_key_id, key_type - ) + persist_notification( + template_id=notification['template'], + template_version=notification['template_version'], + recipient=notification['to'], + service_id=service.id, + personalisation=notification.get('personalisation'), + notification_type=EMAIL_TYPE, + api_key_id=api_key_id, + key_type=key_type, + job_id=notification.get('job', None), + job_row_number=notification.get('row_number', None), ) provider_tasks.deliver_email.apply_async( diff --git a/app/models.py b/app/models.py index 526288990..fb6673ab2 100644 --- a/app/models.py +++ b/app/models.py @@ -572,7 +572,9 @@ class Notification(db.Model): personalisation, notification_type, api_key_id, - key_type): + key_type, + job_id, + job_row_number): return cls( template_id=template_id, template_version=template_version, @@ -583,7 +585,9 @@ class Notification(db.Model): personalisation=personalisation, notification_type=notification_type, api_key_id=api_key_id, - key_type=key_type + key_type=key_type, + job_id=job_id, + job_row_number=job_row_number, ) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 83a3fcf5f..91943d9fb 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -40,7 +40,9 @@ def persist_notification(template_id, personalisation, notification_type, api_key_id, - key_type): + key_type, + job_id=None, + job_row_number=None): notification = Notification.from_v2_api_request(template_id=template_id, template_version=template_version, recipient=recipient, @@ -48,7 +50,10 @@ def persist_notification(template_id, personalisation=personalisation, notification_type=notification_type, api_key_id=api_key_id, - key_type=key_type) + key_type=key_type, + job_id=job_id, + job_row_number=job_row_number + ) dao_create_notification(notification) return notification diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 4191857c5..307d33132 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1,11 +1,9 @@ import uuid import pytest from datetime import datetime - from freezegun import freeze_time from sqlalchemy.exc import SQLAlchemyError from sqlalchemy.orm.exc import NoResultFound - from app import (encryption, DATETIME_FORMAT) from app.celery import provider_tasks from app.celery import tasks @@ -337,7 +335,7 @@ def test_should_send_template_to_correct_sms_task_and_persist(sample_template_wi notification = _notification_json(sample_template_with_placeholders, to="+447234123123", personalisation={"name": "Jo"}) - mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + mocked_deliver_sms = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') notification_id = uuid.uuid4() @@ -352,9 +350,9 @@ def test_should_send_template_to_correct_sms_task_and_persist(sample_template_wi [notification_id], queue="send-sms" ) - - persisted_notification = Notification.query.filter_by(id=notification_id).one() - assert persisted_notification.id == notification_id + assert mocked_deliver_sms.called + assert Notification.query.count() == 1 + persisted_notification = Notification.query.all()[0] assert persisted_notification.to == '+447234123123' assert persisted_notification.template_id == sample_template_with_placeholders.id assert persisted_notification.template_version == sample_template_with_placeholders.version @@ -377,7 +375,7 @@ def test_should_put_send_sms_task_in_research_mode_queue_if_research_mode_servic notification = _notification_json(template, to="+447234123123") - mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + mocked_deliver_sms = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') notification_id = uuid.uuid4() @@ -392,6 +390,7 @@ def test_should_put_send_sms_task_in_research_mode_queue_if_research_mode_servic [notification_id], queue="research-mode" ) + assert mocked_deliver_sms.called def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notify_db_session, mocker): @@ -400,7 +399,7 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif template = sample_template(notify_db, notify_db_session, service=service) notification = _notification_json(template, "+447700900890") # The user’s own number, but in a different format - mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + mocked_deliver_sms = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') notification_id = uuid.uuid4() encrypt_notification = encryption.encrypt(notification) @@ -416,8 +415,9 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif queue="send-sms" ) - persisted_notification = Notification.query.filter_by(id=notification_id).one() - assert persisted_notification.id == notification_id + assert mocked_deliver_sms.called + assert Notification.query.count() == 1 + persisted_notification = Notification.query.all()[0] assert persisted_notification.to == '+447700900890' assert persisted_notification.template_id == template.id assert persisted_notification.template_version == template.version @@ -430,15 +430,15 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif assert persisted_notification.notification_type == 'sms' -def test_should_not_send_sms_if_restricted_service_and_invalid_number_with_test_key(notify_db, - notify_db_session, - mocker): +def test_should_send_sms_if_restricted_service_and_non_team_number_with_test_key(notify_db, + notify_db_session, + mocker): user = sample_user(notify_db, notify_db_session, mobile_numnber="07700 900205") service = sample_service(notify_db, notify_db_session, user=user, restricted=True) template = sample_template(notify_db, notify_db_session, service=service) notification = _notification_json(template, "07700 900849") - mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + mocked_deliver_sms = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') notification_id = uuid.uuid4() send_sms( @@ -454,13 +454,13 @@ def test_should_not_send_sms_if_restricted_service_and_invalid_number_with_test_ queue="send-sms" ) - persisted_notification = Notification.query.filter_by(id=notification_id).one() - assert persisted_notification.id == notification_id + assert mocked_deliver_sms.called + assert Notification.query.count() == 1 -def test_should_not_send_email_if_restricted_service_and_invalid_email_address_with_test_key(notify_db, - notify_db_session, - mocker): +def test_should_send_email_if_restricted_service_and_non_team_email_address_with_test_key(notify_db, + notify_db_session, + mocker): user = sample_user(notify_db, notify_db_session) service = sample_service(notify_db, notify_db_session, user=user, restricted=True) template = sample_template( @@ -468,7 +468,7 @@ def test_should_not_send_email_if_restricted_service_and_invalid_email_address_w ) notification = _notification_json(template, to="test@example.com") - mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') + mocked_deliver_email = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') notification_id = uuid.uuid4() send_email( @@ -483,9 +483,8 @@ def test_should_not_send_email_if_restricted_service_and_invalid_email_address_w [notification_id], queue="send-email" ) - - persisted_notification = Notification.query.filter_by(id=notification_id).one() - assert persisted_notification.id == notification_id + assert Notification.query.count() == 1 + assert mocked_deliver_email.called def test_should_not_send_sms_if_restricted_service_and_invalid_number(notify_db, notify_db_session, mocker): @@ -504,8 +503,7 @@ def test_should_not_send_sms_if_restricted_service_and_invalid_number(notify_db, datetime.utcnow().strftime(DATETIME_FORMAT) ) assert provider_tasks.deliver_sms.apply_async.called is False - with pytest.raises(NoResultFound): - Notification.query.filter_by(id=notification_id).one() + assert Notification.query.count() == 0 def test_should_not_send_email_if_restricted_service_and_invalid_email_address(notify_db, notify_db_session, mocker): @@ -524,8 +522,7 @@ def test_should_not_send_email_if_restricted_service_and_invalid_email_address(n datetime.utcnow().strftime(DATETIME_FORMAT) ) - with pytest.raises(NoResultFound): - Notification.query.filter_by(id=notification_id).one() + assert Notification.query.count() == 0 def test_should_put_send_email_task_in_research_mode_queue_if_research_mode_service( @@ -577,8 +574,8 @@ def test_should_send_sms_template_to_and_persist_with_job_id(sample_job, sample_ [notification_id], queue="send-sms" ) - persisted_notification = Notification.query.filter_by(id=notification_id).one() - assert persisted_notification.id == notification_id + assert Notification.query.count() == 1 + persisted_notification = Notification.query.all()[0] assert persisted_notification.to == '+447234123123' assert persisted_notification.job_id == sample_job.id assert persisted_notification.template_id == sample_job.template.id @@ -618,14 +615,13 @@ def test_should_not_send_email_if_team_key_and_recipient_not_in_team(sample_emai key_type=KEY_TYPE_TEAM ) - with pytest.raises(NoResultFound): - persisted_notification = Notification.query.filter_by(id=notification_id).one() - print(persisted_notification) + assert Notification.query.count() == 0 apply_async.not_called() def test_should_not_send_sms_if_team_key_and_recipient_not_in_team(notify_db, notify_db_session, mocker): + assert Notification.query.count() == 0 user = sample_user(notify_db, notify_db_session, mobile_numnber="07700 900205") service = sample_service(notify_db, notify_db_session, user=user, restricted=True) template = sample_template(notify_db, notify_db_session, service=service) @@ -644,8 +640,7 @@ def test_should_not_send_sms_if_team_key_and_recipient_not_in_team(notify_db, no datetime.utcnow().strftime(DATETIME_FORMAT) ) assert provider_tasks.deliver_sms.apply_async.called is False - with pytest.raises(NoResultFound): - Notification.query.filter_by(id=notification_id).one() + assert Notification.query.count() == 0 def test_should_use_email_template_and_persist(sample_email_template_with_placeholders, sample_api_key, mocker): @@ -671,15 +666,14 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh key_type=sample_api_key.key_type ) - persisted_notification = Notification.query.filter_by(id=notification_id).one() provider_tasks.deliver_email.apply_async.assert_called_once_with( [notification_id], queue='send-email') - - assert persisted_notification.id == notification_id + assert Notification.query.count() == 1 + persisted_notification = Notification.query.all()[0] assert persisted_notification.to == 'my_email@my_email.com' assert persisted_notification.template_id == sample_email_template_with_placeholders.id assert persisted_notification.template_version == sample_email_template_with_placeholders.version - assert persisted_notification.created_at == now + assert persisted_notification.created_at >= now assert not persisted_notification.sent_at assert persisted_notification.status == 'created' assert not persisted_notification.sent_by @@ -712,12 +706,12 @@ def test_send_email_should_use_template_version_from_job_not_latest(sample_email provider_tasks.deliver_email.apply_async.assert_called_once_with([notification_id], queue='send-email') - persisted_notification = Notification.query.filter_by(id=notification_id).one() - assert persisted_notification.id == notification_id + assert Notification.query.count() == 1 + persisted_notification = Notification.query.all()[0] assert persisted_notification.to == 'my_email@my_email.com' assert persisted_notification.template_id == sample_email_template.id assert persisted_notification.template_version == version_on_notification - assert persisted_notification.created_at == now + assert persisted_notification.created_at >= now assert not persisted_notification.sent_at assert persisted_notification.status == 'created' assert not persisted_notification.sent_by @@ -740,11 +734,12 @@ def test_should_use_email_template_subject_placeholders(sample_email_template_wi provider_tasks.deliver_email.apply_async.assert_called_once_with( [notification_id], queue='send-email' ) - persisted_notification = Notification.query.filter_by(id=notification_id).one() - assert persisted_notification.id == notification_id + assert Notification.query.count() == 1 + persisted_notification = Notification.query.all()[0] assert persisted_notification.to == 'my_email@my_email.com' assert persisted_notification.template_id == sample_email_template_with_placeholders.id assert persisted_notification.status == 'created' + assert persisted_notification.created_at >= now assert not persisted_notification.sent_by assert persisted_notification.personalisation == {"name": "Jo"} assert not persisted_notification.reference @@ -755,6 +750,8 @@ def test_should_use_email_template_and_persist_without_personalisation(sample_em notification = _notification_json(sample_email_template, "my_email@my_email.com") mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') + assert Notification.query.count() == 0 + notification_id = uuid.uuid4() now = datetime.utcnow() @@ -765,12 +762,11 @@ def test_should_use_email_template_and_persist_without_personalisation(sample_em now.strftime(DATETIME_FORMAT) ) provider_tasks.deliver_email.apply_async.assert_called_once_with([notification_id], queue='send-email') - - persisted_notification = Notification.query.filter_by(id=notification_id).one() - assert persisted_notification.id == notification_id + assert Notification.query.count() == 1 + persisted_notification = Notification.query.all()[0] assert persisted_notification.to == 'my_email@my_email.com' assert persisted_notification.template_id == sample_email_template.id - assert persisted_notification.created_at == now + assert persisted_notification.created_at >= now assert not persisted_notification.sent_at assert persisted_notification.status == 'created' assert not persisted_notification.sent_by @@ -786,7 +782,7 @@ def test_send_sms_should_go_to_retry_queue_if_database_errors(sample_template, m mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') mocker.patch('app.celery.tasks.send_sms.retry', side_effect=Exception()) - mocker.patch('app.celery.tasks.dao_create_notification', side_effect=expected_exception) + mocker.patch('app.notifications.process_notifications.dao_create_notification', side_effect=expected_exception) now = datetime.utcnow() notification_id = uuid.uuid4() @@ -801,9 +797,7 @@ def test_send_sms_should_go_to_retry_queue_if_database_errors(sample_template, m assert provider_tasks.deliver_sms.apply_async.called is False tasks.send_sms.retry.assert_called_with(exc=expected_exception, queue='retry') - with pytest.raises(NoResultFound) as e: - Notification.query.filter_by(id=notification_id).one() - assert 'No row was found for one' in str(e.value) + assert Notification.query.count() == 0 def test_send_email_should_go_to_retry_queue_if_database_errors(sample_email_template, mocker): @@ -813,7 +807,7 @@ def test_send_email_should_go_to_retry_queue_if_database_errors(sample_email_tem mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') mocker.patch('app.celery.tasks.send_email.retry', side_effect=Exception()) - mocker.patch('app.celery.tasks.dao_create_notification', side_effect=expected_exception) + mocker.patch('app.notifications.process_notifications.dao_create_notification', side_effect=expected_exception) now = datetime.utcnow() notification_id = uuid.uuid4() @@ -828,6 +822,4 @@ def test_send_email_should_go_to_retry_queue_if_database_errors(sample_email_tem assert provider_tasks.deliver_email.apply_async.called is False tasks.send_email.retry.assert_called_with(exc=expected_exception, queue='retry') - with pytest.raises(NoResultFound) as e: - Notification.query.filter_by(id=notification_id).one() - assert 'No row was found for one' in str(e.value) + assert Notification.query.count() == 0 From b0d88e0888cb5da0f6f928d8cb70fa6c41a02cef Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 11 Nov 2016 10:50:38 +0000 Subject: [PATCH 29/50] Removed OrderedDict Added missing assert in test --- app/schema_validation/__init__.py | 2 +- tests/app/notifications/test_validators.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py index 4cb27b12b..88f0e2421 100644 --- a/app/schema_validation/__init__.py +++ b/app/schema_validation/__init__.py @@ -18,7 +18,7 @@ def build_error_message(errors, schema): field = "'{}' {}".format(e.path[0], e.schema.get('validationMessage')) if e.schema.get( 'validationMessage') else e.message s = field.split("'") - field = OrderedDict({"error": "ValidationError", "message": "{}{}".format(s[1], s[2])}) + field = {"error": "ValidationError", "message": "{}{}".format(s[1], s[2])} fields.append(field) message = { "status_code": 400, diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index 2810e75e5..03781bcb8 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -48,7 +48,7 @@ def test_check_template_is_for_notification_type_fails_when_template_type_does_n with pytest.raises(BadRequestError) as e: check_template_is_for_notification_type(notification_type=notification_type, template_type=template_type) - e.value.status_code == 400 + assert e.value.status_code == 400 error_message = '{0} template is not suitable for {1} notification'.format(template_type, notification_type) assert e.value.message == error_message assert e.value.fields == [{'template': error_message}] From 488ba7a1ef35f5f6a3e28aa559bf16ee4ee90560 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 11 Nov 2016 10:56:21 +0000 Subject: [PATCH 30/50] Remove import --- app/schema_validation/__init__.py | 1 - 1 file changed, 1 deletion(-) diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py index 88f0e2421..71dd37c18 100644 --- a/app/schema_validation/__init__.py +++ b/app/schema_validation/__init__.py @@ -1,5 +1,4 @@ import json -from collections import OrderedDict from jsonschema import Draft4Validator, ValidationError From 9ae6e14140aa90c1b15b1b47525fef402f2b85d8 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 10 Nov 2016 17:07:02 +0000 Subject: [PATCH 31/50] move deactivate functionality into one database transactions this means that any errors will cause the entire thing to roll back unfortunately, to do this we have to circumvent our regular code, which calls commit a lot, and lazily loads a lot of things, which will flush, and cause the version decorators to fail. so we have to write a lot of stuff by hand and re-select the service (even though it's already been queried) just to populate the api_keys and templates relationship on it --- app/dao/dao_utils.py | 2 -- app/dao/services_dao.py | 29 +++++++++++++++++++++++++++- app/models.py | 2 +- app/service/rest.py | 18 +++-------------- tests/app/service/test_deactivate.py | 2 +- 5 files changed, 33 insertions(+), 20 deletions(-) diff --git a/app/dao/dao_utils.py b/app/dao/dao_utils.py index bdaafc074..96628be06 100644 --- a/app/dao/dao_utils.py +++ b/app/dao/dao_utils.py @@ -9,7 +9,6 @@ def transactional(func): @wraps(func) def commit_or_rollback(*args, **kwargs): from flask import current_app - from app import db try: res = func(*args, **kwargs) db.session.commit() @@ -27,7 +26,6 @@ def version_class(model_class, history_cls=None): def versioned(func): @wraps(func) def record_version(*args, **kwargs): - from app import db func(*args, **kwargs) history_objects = [create_hist(obj) for obj in itertools.chain(db.session.new, db.session.dirty) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index dde38af32..604f0dd25 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -1,5 +1,5 @@ import uuid -from datetime import date +from datetime import date, datetime from sqlalchemy import asc, func from sqlalchemy.orm import joinedload @@ -69,6 +69,33 @@ def dao_fetch_all_services_by_user(user_id, only_active=False): return query.all() +@transactional +@version_class(Service) +@version_class(Template, TemplateHistory) +@version_class(ApiKey) +def dao_deactive_service(service_id): + # have to eager load templates and api keys so that we don't flush when we loop through them + service = Service.query.options( + joinedload('templates'), + joinedload('api_keys'), + ).filter(Service.id == service_id).one() + + service.active = False + service.name = '_archived_' + service.name + service.email_from = '_archived_' + service.email_from + + + for template in service.templates: + template.archived = True + + for api_key in service.api_keys: + api_key.expiry_date = datetime.utcnow() + + db.session.add(service) + db.session.add_all(service.templates) + db.session.add_all(service.api_keys) + + def dao_fetch_service_by_id_and_user(service_id, user_id): return Service.query.filter( Service.users.any(id=user_id), diff --git a/app/models.py b/app/models.py index 75b9615fc..2f2c6163d 100644 --- a/app/models.py +++ b/app/models.py @@ -275,7 +275,7 @@ class Template(db.Model): content = db.Column(db.Text, index=False, unique=False, nullable=False) archived = db.Column(db.Boolean, index=False, nullable=False, default=False) service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, unique=False, nullable=False) - service = db.relationship('Service', backref=db.backref('templates', lazy='dynamic')) + service = db.relationship('Service', backref='templates') subject = db.Column(db.Text, index=False, unique=False, nullable=True) created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), index=True, nullable=False) created_by = db.relationship('User') diff --git a/app/service/rest.py b/app/service/rest.py index 78138b434..312839435 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -25,16 +25,14 @@ from app.dao.services_dao import ( dao_fetch_stats_for_service, dao_fetch_todays_stats_for_service, dao_fetch_weekly_historical_stats_for_service, - dao_fetch_todays_stats_for_all_services + dao_fetch_todays_stats_for_all_services, + dao_deactive_service ) from app.dao.service_whitelist_dao import ( dao_fetch_service_whitelist, dao_add_and_commit_whitelisted_contacts, dao_remove_service_whitelist ) -from app.dao.templates_dao import ( - dao_update_template -) from app.dao import notifications_dao from app.dao.provider_statistics_dao import get_fragment_count from app.dao.users_dao import get_model_users @@ -326,17 +324,7 @@ def deactivate_service(service_id): # assume already inactive, don't change service name return '', 204 - service.active = False - service.name = '_archived_' + service.name - service.email_from = '_archived_' + service.email_from - dao_update_service(service) - - for template in service.templates: - template.archived = True - dao_update_template(template) - - for api_key in service.api_keys: - expire_api_key(service.id, api_key.id) + dao_deactive_service(service.id) return '', 204 diff --git a/tests/app/service/test_deactivate.py b/tests/app/service/test_deactivate.py index d81de0e10..df39fecac 100644 --- a/tests/app/service/test_deactivate.py +++ b/tests/app/service/test_deactivate.py @@ -57,7 +57,7 @@ def test_deactivating_service_revokes_api_keys(deactivated_service): def test_deactivating_service_archives_templates(deactivated_service): - assert deactivated_service.templates.count() == 2 + assert len(deactivated_service.templates) == 2 for template in deactivated_service.templates: assert template.archived is True assert template.version == 2 From 195f3615e648fa30bd5b0d2253227f266bdb9133 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 11 Nov 2016 12:43:51 +0000 Subject: [PATCH 32/50] add test that if we have an exception, nothing is committed --- app/dao/services_dao.py | 6 +----- app/models.py | 2 +- tests/__init__.py | 9 +++++++++ tests/app/service/test_deactivate.py | 22 ++++++++++++++++++---- 4 files changed, 29 insertions(+), 10 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 604f0dd25..bead23a6e 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -75,6 +75,7 @@ def dao_fetch_all_services_by_user(user_id, only_active=False): @version_class(ApiKey) def dao_deactive_service(service_id): # have to eager load templates and api keys so that we don't flush when we loop through them + # to ensure that db.session still contains the models when it comes to creating history objects service = Service.query.options( joinedload('templates'), joinedload('api_keys'), @@ -84,17 +85,12 @@ def dao_deactive_service(service_id): service.name = '_archived_' + service.name service.email_from = '_archived_' + service.email_from - for template in service.templates: template.archived = True for api_key in service.api_keys: api_key.expiry_date = datetime.utcnow() - db.session.add(service) - db.session.add_all(service.templates) - db.session.add_all(service.api_keys) - def dao_fetch_service_by_id_and_user(service_id, user_id): return Service.query.filter( diff --git a/app/models.py b/app/models.py index 2f2c6163d..699ae2275 100644 --- a/app/models.py +++ b/app/models.py @@ -5,7 +5,7 @@ from sqlalchemy.dialects.postgresql import ( UUID, JSON ) -from sqlalchemy import UniqueConstraint, and_, false, true +from sqlalchemy import UniqueConstraint, and_ from sqlalchemy.orm import foreign, remote from notifications_utils.recipients import ( validate_email_address, diff --git a/tests/__init__.py b/tests/__init__.py index 526e0efc1..a40d209cf 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -30,3 +30,12 @@ def create_authorization_header(service_id=None, key_type=KEY_TYPE_NORMAL): token = create_jwt_token(secret=secret, client_id=client_id) return 'Authorization', 'Bearer {}'.format(token) + + +def unwrap_function(fn): + """ + Given a function, returns its undecorated original. + """ + while hasattr(fn, '__wrapped__'): + fn = fn.__wrapped__ + return fn diff --git a/tests/app/service/test_deactivate.py b/tests/app/service/test_deactivate.py index df39fecac..0f9a3dcce 100644 --- a/tests/app/service/test_deactivate.py +++ b/tests/app/service/test_deactivate.py @@ -1,22 +1,26 @@ import uuid +from unittest import mock import pytest -from app.models import Service -from tests import create_authorization_header +from app import db +from app.models import Service, TemplateHistory, ApiKey +from app.dao.services_dao import dao_deactive_service + +from tests import create_authorization_header, unwrap_function from tests.app.conftest import ( sample_template as create_template, sample_api_key as create_api_key ) -def test_deactivate_only_allows_post(client, sample_service): +def test_deactivate_only_allows_post(client): auth_header = create_authorization_header() response = client.get('/service/{}/deactivate'.format(uuid.uuid4()), headers=[auth_header]) assert response.status_code == 405 -def test_deactivate_service_errors_with_bad_service_id(client, sample_service): +def test_deactivate_service_errors_with_bad_service_id(client): auth_header = create_authorization_header() response = client.post('/service/{}/deactivate'.format(uuid.uuid4()), headers=[auth_header]) assert response.status_code == 404 @@ -73,3 +77,13 @@ def test_deactivating_service_creates_history(deactivated_service): assert history.version == 2 assert history.active is False + + +def test_deactivating_service_rolls_back_everything_on_error(sample_service, sample_api_key, sample_template): + unwrapped_deactive_service = unwrap_function(dao_deactive_service) + + unwrapped_deactive_service(sample_service.id) + + assert sample_service in db.session.dirty + assert sample_api_key in db.session.dirty + assert sample_template in db.session.dirty From c45d1f63a70ba461503e5848d16e0c47bfca4d2b Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 11 Nov 2016 14:56:33 +0000 Subject: [PATCH 33/50] Use the same method to persist a notification. Refactored the send_sms task to use this method. --- app/celery/tasks.py | 5 +- app/models.py | 53 +++++------------ app/notifications/process_notifications.py | 28 +++++---- tests/app/celery/test_tasks.py | 18 +++--- .../test_process_notification.py | 31 +++++++++- tests/app/test_model.py | 58 ++++++++----------- 6 files changed, 94 insertions(+), 99 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 778ff8b29..dde08958e 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -17,11 +17,9 @@ from app.dao.jobs_dao import ( dao_update_job, dao_get_job_by_id ) -from app.dao.notifications_dao import dao_create_notification from app.dao.services_dao import dao_fetch_service_by_id, fetch_todays_total_message_count from app.dao.templates_dao import dao_get_template_by_id from app.models import ( - Notification, EMAIL_TYPE, SMS_TYPE, KEY_TYPE_NORMAL @@ -140,6 +138,7 @@ def send_sms(self, notification_type=SMS_TYPE, api_key_id=api_key_id, key_type=key_type, + created_at=created_at, job_id=notification.get('job', None), job_row_number=notification.get('row_number', None), ) @@ -189,8 +188,10 @@ def send_email(self, service_id, notification_type=EMAIL_TYPE, api_key_id=api_key_id, key_type=key_type, + created_at=created_at, job_id=notification.get('job', None), job_row_number=notification.get('row_number', None), + ) provider_tasks.deliver_email.apply_async( diff --git a/app/models.py b/app/models.py index fb6673ab2..fadd057d3 100644 --- a/app/models.py +++ b/app/models.py @@ -538,56 +538,31 @@ class Notification(db.Model): self._personalisation = encryption.encrypt(personalisation) @classmethod - def from_api_request( - cls, - created_at, - notification, - notification_id, - service_id, - notification_type, - api_key_id, - key_type): - return cls( - id=notification_id, - template_id=notification['template'], - template_version=notification['template_version'], - to=notification['to'], - service_id=service_id, - job_id=notification.get('job', None), - job_row_number=notification.get('row_number', None), - status='created', - created_at=datetime.datetime.strptime(created_at, DATETIME_FORMAT), - personalisation=notification.get('personalisation'), - notification_type=notification_type, - api_key_id=api_key_id, - key_type=key_type - ) - - @classmethod - def from_v2_api_request(cls, - template_id, - template_version, - recipient, - service_id, - personalisation, - notification_type, - api_key_id, - key_type, - job_id, - job_row_number): + def from_request(cls, + template_id, + template_version, + recipient, + service_id, + personalisation, + notification_type, + api_key_id, + key_type, + job_id, + job_row_number, + created_at): return cls( template_id=template_id, template_version=template_version, to=recipient, service_id=service_id, status='created', - created_at=datetime.datetime.utcnow(), + created_at=created_at, personalisation=personalisation, notification_type=notification_type, api_key_id=api_key_id, key_type=key_type, job_id=job_id, - job_row_number=job_row_number, + job_row_number=job_row_number ) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 91943d9fb..ea08144c7 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -1,7 +1,10 @@ +from datetime import datetime + from flask import current_app from notifications_utils.renderers import PassThrough from notifications_utils.template import Template +from app import DATETIME_FORMAT from app.celery import provider_tasks from app.dao.notifications_dao import dao_create_notification, dao_delete_notifications_and_history_by_id from app.models import SMS_TYPE, Notification, KEY_TYPE_TEST, EMAIL_TYPE @@ -41,19 +44,22 @@ def persist_notification(template_id, notification_type, api_key_id, key_type, + created_at=None, job_id=None, job_row_number=None): - notification = Notification.from_v2_api_request(template_id=template_id, - template_version=template_version, - recipient=recipient, - service_id=service_id, - personalisation=personalisation, - notification_type=notification_type, - api_key_id=api_key_id, - key_type=key_type, - job_id=job_id, - job_row_number=job_row_number - ) + notification = Notification.from_request( + template_id=template_id, + template_version=template_version, + recipient=recipient, + service_id=service_id, + personalisation=personalisation, + notification_type=notification_type, + api_key_id=api_key_id, + key_type=key_type, + created_at=created_at if created_at else datetime.utcnow().strftime(DATETIME_FORMAT), + job_id=job_id, + job_row_number=job_row_number + ) dao_create_notification(notification) return notification diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 307d33132..2b9086a64 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -3,7 +3,6 @@ import pytest from datetime import datetime from freezegun import freeze_time from sqlalchemy.exc import SQLAlchemyError -from sqlalchemy.orm.exc import NoResultFound from app import (encryption, DATETIME_FORMAT) from app.celery import provider_tasks from app.celery import tasks @@ -189,8 +188,6 @@ def test_should_not_process_email_job_if_would_exceed_send_limits_inc_today(noti mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('email')) mocker.patch('app.celery.tasks.send_email.apply_async') - mocker.patch('app.encryption.encrypt', return_value="something_encrypted") - mocker.patch('app.celery.tasks.create_uuid', return_value="uuid") process_job(job.id) @@ -208,8 +205,6 @@ def test_should_not_process_email_job_if_would_exceed_send_limits(notify_db, not mocker.patch('app.celery.tasks.s3.get_job_from_s3') mocker.patch('app.celery.tasks.send_email.apply_async') - mocker.patch('app.encryption.encrypt', return_value="something_encrypted") - mocker.patch('app.celery.tasks.create_uuid', return_value="uuid") process_job(job.id) @@ -562,11 +557,12 @@ def test_should_send_sms_template_to_and_persist_with_job_id(sample_job, sample_ mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') notification_id = uuid.uuid4() + now = datetime.utcnow() send_sms( sample_job.service.id, notification_id, encryption.encrypt(notification), - datetime.utcnow().strftime(DATETIME_FORMAT), + now.strftime(DATETIME_FORMAT), api_key_id=str(sample_api_key.id), key_type=KEY_TYPE_NORMAL ) @@ -581,7 +577,7 @@ def test_should_send_sms_template_to_and_persist_with_job_id(sample_job, sample_ assert persisted_notification.template_id == sample_job.template.id assert persisted_notification.status == 'created' assert not persisted_notification.sent_at - assert persisted_notification.created_at <= datetime.utcnow() + assert persisted_notification.created_at <= now assert not persisted_notification.sent_by assert persisted_notification.job_row_number == 2 assert persisted_notification.api_key_id == sample_api_key.id @@ -673,7 +669,7 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh assert persisted_notification.to == 'my_email@my_email.com' assert persisted_notification.template_id == sample_email_template_with_placeholders.id assert persisted_notification.template_version == sample_email_template_with_placeholders.version - assert persisted_notification.created_at >= now + assert persisted_notification.created_at == now assert not persisted_notification.sent_at assert persisted_notification.status == 'created' assert not persisted_notification.sent_by @@ -711,7 +707,7 @@ def test_send_email_should_use_template_version_from_job_not_latest(sample_email assert persisted_notification.to == 'my_email@my_email.com' assert persisted_notification.template_id == sample_email_template.id assert persisted_notification.template_version == version_on_notification - assert persisted_notification.created_at >= now + assert persisted_notification.created_at == now assert not persisted_notification.sent_at assert persisted_notification.status == 'created' assert not persisted_notification.sent_by @@ -739,7 +735,7 @@ def test_should_use_email_template_subject_placeholders(sample_email_template_wi assert persisted_notification.to == 'my_email@my_email.com' assert persisted_notification.template_id == sample_email_template_with_placeholders.id assert persisted_notification.status == 'created' - assert persisted_notification.created_at >= now + assert persisted_notification.created_at == now assert not persisted_notification.sent_by assert persisted_notification.personalisation == {"name": "Jo"} assert not persisted_notification.reference @@ -766,7 +762,7 @@ def test_should_use_email_template_and_persist_without_personalisation(sample_em persisted_notification = Notification.query.all()[0] assert persisted_notification.to == 'my_email@my_email.com' assert persisted_notification.template_id == sample_email_template.id - assert persisted_notification.created_at >= now + assert persisted_notification.created_at == now assert not persisted_notification.sent_at assert persisted_notification.status == 'created' assert not persisted_notification.sent_by diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index ddc559201..851865a6b 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -1,3 +1,5 @@ +import datetime + import pytest from boto3.exceptions import Boto3Error from sqlalchemy.exc import SQLAlchemyError @@ -46,21 +48,44 @@ def test_persist_notification_creates_and_save_to_db(sample_template, sample_api assert NotificationHistory.query.count() == 1 -def test_persist_notification_throws_exception_when_missing_template(sample_template, sample_api_key): +def test_persist_notification_throws_exception_when_missing_template(sample_api_key): assert Notification.query.count() == 0 assert NotificationHistory.query.count() == 0 with pytest.raises(SQLAlchemyError): persist_notification(template_id=None, template_version=None, recipient='+447111111111', - service_id=sample_template.service.id, - personalisation=None, notification_type='sms', + service_id=sample_api_key.service_id, + personalisation=None, + notification_type='sms', api_key_id=sample_api_key.id, key_type=sample_api_key.key_type) assert Notification.query.count() == 0 assert NotificationHistory.query.count() == 0 +def test_persist_notification_with_job_and_created(sample_job, sample_api_key): + assert Notification.query.count() == 0 + assert NotificationHistory.query.count() == 0 + created_at = datetime.datetime(2016, 11, 11, 16, 8, 18) + persist_notification(template_id=sample_job.template.id, + template_version=sample_job.template.version, + recipient='+447111111111', + service_id=sample_job.service.id, + personalisation=None, notification_type='sms', + api_key_id=sample_api_key.id, + key_type=sample_api_key.key_type, + created_at=created_at, + job_id=sample_job.id, + job_row_number=10) + assert Notification.query.count() == 1 + assert NotificationHistory.query.count() == 1 + persisted_notification = Notification.query.all()[0] + assert persisted_notification.job_id == sample_job.id + assert persisted_notification.job_row_number == 10 + assert persisted_notification.created_at == created_at + + @pytest.mark.parametrize('research_mode, queue, notification_type, key_type', [(True, 'research-mode', 'sms', 'normal'), (True, 'research-mode', 'email', 'normal'), diff --git a/tests/app/test_model.py b/tests/app/test_model.py index 78827235a..e6e5877dd 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -1,3 +1,4 @@ +import uuid from datetime import datetime import pytest @@ -12,23 +13,20 @@ from app.models import ( def test_should_build_notification_from_minimal_set_of_api_derived_params(notify_api): now = datetime.utcnow() - notification = { - 'template': 'template', - 'template_version': '1', - 'to': 'someone', - 'personalisation': {} - } - notification = Notification.from_api_request( - created_at=now.strftime(DATETIME_FORMAT), - notification=notification, - notification_id="notification_id", - service_id="service_id", + notification = Notification.from_request( + template_id='template', + template_version='1', + recipient='someone', + service_id='service_id', notification_type='SMS', + created_at=now, api_key_id='api_key_id', - key_type='key_type' + key_type='key_type', + personalisation={}, + job_id=None, + job_row_number=None ) assert notification.created_at == now - assert notification.id == "notification_id" assert notification.template_id == 'template' assert notification.template_version == '1' assert not notification.job_row_number @@ -44,28 +42,22 @@ def test_should_build_notification_from_minimal_set_of_api_derived_params(notify def test_should_build_notification_from_full_set_of_api_derived_params(notify_api): now = datetime.utcnow() - - notification = { - 'template': 'template', - 'template_version': '1', - 'to': 'someone', - 'personalisation': {'key': 'value'}, - 'job': 'job_id', - 'row_number': 100 - } - notification = Notification.from_api_request( - created_at=now.strftime(DATETIME_FORMAT), - notification=notification, - notification_id="notification_id", - service_id="service_id", - notification_type='SMS', - api_key_id='api_key_id', - key_type='key_type' - ) + notification = Notification.from_request(template_id='template', + template_version=1, + recipient='someone', + service_id='service_id', + personalisation={'key': 'value'}, + notification_type='SMS', + api_key_id='api_key_id', + key_type='key_type', + job_id='job_id', + job_row_number=100, + created_at=now + ) assert notification.created_at == now - assert notification.id == "notification_id" + assert notification.id is None assert notification.template_id == 'template' - assert notification.template_version == '1' + assert notification.template_version == 1 assert notification.job_row_number == 100 assert notification.job_id == 'job_id' assert notification.to == 'someone' From 4fe8d700ae52f8693f79cb3f67142f9d7e122082 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 11 Nov 2016 16:00:31 +0000 Subject: [PATCH 34/50] Fix the send_sms and send_email code to send the notification id that has been persisted. --- app/celery/tasks.py | 32 ++++----- app/notifications/process_notifications.py | 2 +- tests/app/celery/test_tasks.py | 80 +++++++++++----------- 3 files changed, 56 insertions(+), 58 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index dde08958e..52890b6e9 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -130,26 +130,26 @@ def send_sms(self, return try: - persist_notification(template_id=notification['template'], - template_version=notification['template_version'], - recipient=notification['to'], - service_id=service.id, - personalisation=notification.get('personalisation'), - notification_type=SMS_TYPE, - api_key_id=api_key_id, - key_type=key_type, - created_at=created_at, - job_id=notification.get('job', None), - job_row_number=notification.get('row_number', None), - ) + saved_notification = persist_notification(template_id=notification['template'], + template_version=notification['template_version'], + recipient=notification['to'], + service_id=service.id, + personalisation=notification.get('personalisation'), + notification_type=SMS_TYPE, + api_key_id=api_key_id, + key_type=key_type, + created_at=created_at, + job_id=notification.get('job', None), + job_row_number=notification.get('row_number', None), + ) provider_tasks.deliver_sms.apply_async( - [notification_id], + [saved_notification.id], queue='send-sms' if not service.research_mode else 'research-mode' ) current_app.logger.info( - "SMS {} created at {} for job {}".format(notification_id, created_at, notification.get('job', None)) + "SMS {} created at {} for job {}".format(saved_notification.id, created_at, notification.get('job', None)) ) except SQLAlchemyError as e: @@ -179,7 +179,7 @@ def send_email(self, service_id, return try: - persist_notification( + saved_notification = persist_notification( template_id=notification['template'], template_version=notification['template_version'], recipient=notification['to'], @@ -195,7 +195,7 @@ def send_email(self, service_id, ) provider_tasks.deliver_email.apply_async( - [notification_id], + [saved_notification.id], queue='send-email' if not service.research_mode else 'research-mode' ) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index ea08144c7..e797350d2 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -77,7 +77,7 @@ def send_notification_to_queue(notification, research_mode): [str(notification.id)], queue='send-email' if not research_mode else 'research-mode' ) - except Exception as e: + except Exception: current_app.logger.exception("Failed to send to SQS exception") dao_delete_notifications_and_history_by_id(notification.id) raise diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 2b9086a64..7dca93dde 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -332,20 +332,13 @@ def test_should_send_template_to_correct_sms_task_and_persist(sample_template_wi mocked_deliver_sms = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') - notification_id = uuid.uuid4() - send_sms( sample_template_with_placeholders.service_id, - notification_id, + uuid.uuid4(), encryption.encrypt(notification), datetime.utcnow().strftime(DATETIME_FORMAT) ) - provider_tasks.deliver_sms.apply_async.assert_called_once_with( - [notification_id], - queue="send-sms" - ) - assert mocked_deliver_sms.called assert Notification.query.count() == 1 persisted_notification = Notification.query.all()[0] assert persisted_notification.to == '+447234123123' @@ -359,6 +352,10 @@ def test_should_send_template_to_correct_sms_task_and_persist(sample_template_wi assert persisted_notification.personalisation == {'name': 'Jo'} assert persisted_notification._personalisation == encryption.encrypt({"name": "Jo"}) assert persisted_notification.notification_type == 'sms' + mocked_deliver_sms.assert_called_once_with( + [persisted_notification.id], + queue="send-sms" + ) def test_should_put_send_sms_task_in_research_mode_queue_if_research_mode_service(notify_db, notify_db_session, mocker): @@ -380,9 +377,10 @@ def test_should_put_send_sms_task_in_research_mode_queue_if_research_mode_servic encryption.encrypt(notification), datetime.utcnow().strftime(DATETIME_FORMAT) ) - + assert Notification.query.count() == 1 + persisted_notification = Notification.query.all()[0] provider_tasks.deliver_sms.apply_async.assert_called_once_with( - [notification_id], + [persisted_notification.id], queue="research-mode" ) assert mocked_deliver_sms.called @@ -405,12 +403,6 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif datetime.utcnow().strftime(DATETIME_FORMAT) ) - provider_tasks.deliver_sms.apply_async.assert_called_once_with( - [notification_id], - queue="send-sms" - ) - - assert mocked_deliver_sms.called assert Notification.query.count() == 1 persisted_notification = Notification.query.all()[0] assert persisted_notification.to == '+447700900890' @@ -423,6 +415,10 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif assert not persisted_notification.job_id assert not persisted_notification.personalisation assert persisted_notification.notification_type == 'sms' + provider_tasks.deliver_sms.apply_async.assert_called_once_with( + [persisted_notification.id], + queue="send-sms" + ) def test_should_send_sms_if_restricted_service_and_non_team_number_with_test_key(notify_db, @@ -444,14 +440,13 @@ def test_should_send_sms_if_restricted_service_and_non_team_number_with_test_key key_type=KEY_TYPE_TEST ) - provider_tasks.deliver_sms.apply_async.assert_called_once_with( - [notification_id], + assert Notification.query.count() == 1 + persisted_notification = Notification.query.all()[0] + mocked_deliver_sms.assert_called_once_with( + [persisted_notification.id], queue="send-sms" ) - assert mocked_deliver_sms.called - assert Notification.query.count() == 1 - def test_should_send_email_if_restricted_service_and_non_team_email_address_with_test_key(notify_db, notify_db_session, @@ -474,12 +469,12 @@ def test_should_send_email_if_restricted_service_and_non_team_email_address_with key_type=KEY_TYPE_TEST ) - provider_tasks.deliver_email.apply_async.assert_called_once_with( - [notification_id], + assert Notification.query.count() == 1 + persisted_notification = Notification.query.all()[0] + mocked_deliver_email.assert_called_once_with( + [persisted_notification.id], queue="send-email" ) - assert Notification.query.count() == 1 - assert mocked_deliver_email.called def test_should_not_send_sms_if_restricted_service_and_invalid_number(notify_db, notify_db_session, mocker): @@ -542,8 +537,10 @@ def test_should_put_send_email_task_in_research_mode_queue_if_research_mode_serv datetime.utcnow().strftime(DATETIME_FORMAT) ) + assert Notification.query.count() == 1 + persisted_notification = Notification.query.all()[0] provider_tasks.deliver_email.apply_async.assert_called_once_with( - [notification_id], + [persisted_notification.id], queue="research-mode" ) @@ -566,10 +563,6 @@ def test_should_send_sms_template_to_and_persist_with_job_id(sample_job, sample_ api_key_id=str(sample_api_key.id), key_type=KEY_TYPE_NORMAL ) - provider_tasks.deliver_sms.apply_async.assert_called_once_with( - [notification_id], - queue="send-sms" - ) assert Notification.query.count() == 1 persisted_notification = Notification.query.all()[0] assert persisted_notification.to == '+447234123123' @@ -584,6 +577,11 @@ def test_should_send_sms_template_to_and_persist_with_job_id(sample_job, sample_ assert persisted_notification.key_type == KEY_TYPE_NORMAL assert persisted_notification.notification_type == 'sms' + provider_tasks.deliver_sms.apply_async.assert_called_once_with( + [persisted_notification.id], + queue="send-sms" + ) + def test_should_not_send_email_if_team_key_and_recipient_not_in_team(sample_email_template_with_placeholders, sample_team_api_key, @@ -662,8 +660,6 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh key_type=sample_api_key.key_type ) - provider_tasks.deliver_email.apply_async.assert_called_once_with( - [notification_id], queue='send-email') assert Notification.query.count() == 1 persisted_notification = Notification.query.all()[0] assert persisted_notification.to == 'my_email@my_email.com' @@ -680,6 +676,9 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh assert persisted_notification.key_type == KEY_TYPE_NORMAL assert persisted_notification.notification_type == 'email' + provider_tasks.deliver_email.apply_async.assert_called_once_with( + [persisted_notification.id], queue='send-email') + def test_send_email_should_use_template_version_from_job_not_latest(sample_email_template, mocker): notification = _notification_json(sample_email_template, 'my_email@my_email.com') @@ -691,17 +690,14 @@ def test_send_email_should_use_template_version_from_job_not_latest(sample_email dao_update_template(sample_email_template) t = dao_get_template_by_id(sample_email_template.id) assert t.version > version_on_notification - notification_id = uuid.uuid4() now = datetime.utcnow() send_email( sample_email_template.service_id, - notification_id, + uuid.uuid4(), encryption.encrypt(notification), now.strftime(DATETIME_FORMAT) ) - provider_tasks.deliver_email.apply_async.assert_called_once_with([notification_id], queue='send-email') - assert Notification.query.count() == 1 persisted_notification = Notification.query.all()[0] assert persisted_notification.to == 'my_email@my_email.com' @@ -712,6 +708,7 @@ def test_send_email_should_use_template_version_from_job_not_latest(sample_email assert persisted_notification.status == 'created' assert not persisted_notification.sent_by assert persisted_notification.notification_type == 'email' + provider_tasks.deliver_email.apply_async.assert_called_once_with([persisted_notification.id], queue='send-email') def test_should_use_email_template_subject_placeholders(sample_email_template_with_placeholders, mocker): @@ -727,9 +724,6 @@ def test_should_use_email_template_subject_placeholders(sample_email_template_wi encryption.encrypt(notification), now.strftime(DATETIME_FORMAT) ) - provider_tasks.deliver_email.apply_async.assert_called_once_with( - [notification_id], queue='send-email' - ) assert Notification.query.count() == 1 persisted_notification = Notification.query.all()[0] assert persisted_notification.to == 'my_email@my_email.com' @@ -740,6 +734,9 @@ def test_should_use_email_template_subject_placeholders(sample_email_template_wi assert persisted_notification.personalisation == {"name": "Jo"} assert not persisted_notification.reference assert persisted_notification.notification_type == 'email' + provider_tasks.deliver_email.apply_async.assert_called_once_with( + [persisted_notification.id], queue='send-email' + ) def test_should_use_email_template_and_persist_without_personalisation(sample_email_template, mocker): @@ -757,7 +754,6 @@ def test_should_use_email_template_and_persist_without_personalisation(sample_em encryption.encrypt(notification), now.strftime(DATETIME_FORMAT) ) - provider_tasks.deliver_email.apply_async.assert_called_once_with([notification_id], queue='send-email') assert Notification.query.count() == 1 persisted_notification = Notification.query.all()[0] assert persisted_notification.to == 'my_email@my_email.com' @@ -769,6 +765,8 @@ def test_should_use_email_template_and_persist_without_personalisation(sample_em assert not persisted_notification.personalisation assert not persisted_notification.reference assert persisted_notification.notification_type == 'email' + provider_tasks.deliver_email.apply_async.assert_called_once_with([persisted_notification.id], + queue='send-email') def test_send_sms_should_go_to_retry_queue_if_database_errors(sample_template, mocker): @@ -815,7 +813,7 @@ def test_send_email_should_go_to_retry_queue_if_database_errors(sample_email_tem encryption.encrypt(notification), now.strftime(DATETIME_FORMAT) ) - assert provider_tasks.deliver_email.apply_async.called is False + assert not provider_tasks.deliver_email.apply_async.called tasks.send_email.retry.assert_called_with(exc=expected_exception, queue='retry') assert Notification.query.count() == 0 From 4f5254134b56b339864c225a27bac81162a04f4b Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 11 Nov 2016 16:09:09 +0000 Subject: [PATCH 35/50] Fix logging in send_sms and send_email --- app/celery/tasks.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 52890b6e9..7e29acc51 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -153,12 +153,12 @@ def send_sms(self, ) except SQLAlchemyError as e: - current_app.logger.exception("RETRY: send_sms notification {}".format(notification_id), e) + current_app.logger.exception("RETRY: send_sms notification {}".format(saved_notification.id), e) try: raise self.retry(queue="retry", exc=e) except self.MaxRetriesExceededError: current_app.logger.exception( - "RETRY FAILED: task send_sms failed for notification {}".format(notification.id), + "RETRY FAILED: task send_sms failed for notification {}".format(saved_notification.id), e ) @@ -199,13 +199,13 @@ def send_email(self, service_id, queue='send-email' if not service.research_mode else 'research-mode' ) - current_app.logger.info("Email {} created at {}".format(notification_id, created_at)) + current_app.logger.info("Email {} created at {}".format(saved_notification.id, created_at)) except SQLAlchemyError as e: - current_app.logger.exception("RETRY: send_email notification {}".format(notification_id), e) + current_app.logger.exception("RETRY: send_email notification {}".format(saved_notification.id), e) try: raise self.retry(queue="retry", exc=e) except self.MaxRetriesExceededError: current_app.logger.error( - "RETRY FAILED: task send_email failed for notification {}".format(notification.id), + "RETRY FAILED: task send_email failed for notification {}".format(saved_notification.id), e ) From 35e1e06db4fb987eb566ff7d0fdbc11e71fe5221 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 11 Nov 2016 16:30:05 +0000 Subject: [PATCH 36/50] Fix conflicting migration versions Two different versions of 0059 were merged. `0059_set_services_to_active` was merged first, so this renames my migration to `0060`, so it goes in afterwards. --- ...er_template_type.py => 0060_add_letter_template_type.py} | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) rename migrations/versions/{0059_add_letter_template_type.py => 0060_add_letter_template_type.py} (90%) diff --git a/migrations/versions/0059_add_letter_template_type.py b/migrations/versions/0060_add_letter_template_type.py similarity index 90% rename from migrations/versions/0059_add_letter_template_type.py rename to migrations/versions/0060_add_letter_template_type.py index 2b4cb6eea..cf5057de4 100644 --- a/migrations/versions/0059_add_letter_template_type.py +++ b/migrations/versions/0060_add_letter_template_type.py @@ -1,7 +1,7 @@ """empty message Revision ID: f266fb67597a -Revises: 0058_add_letters_flag +Revises: 0059_set_services_to_active Create Date: 2016-11-07 16:13:18.961527 """ @@ -10,8 +10,8 @@ from alembic import op import sqlalchemy as sa # revision identifiers, used by Alembic. -revision = '0059_add_letter_template_type' -down_revision = '0058_add_letters_flag' +revision = '0060_add_letter_template_type' +down_revision = '0059_set_services_to_active' name = 'template_type' From 3e6d581f737a8f027661b543cb4df6157bf37e2e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 7 Nov 2016 15:20:53 +0000 Subject: [PATCH 37/50] Use constants for template type MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Handy if we ever want to rename these I guess… --- app/schemas.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/schemas.py b/app/schemas.py index 7f52b9541..a9811dfc3 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -213,7 +213,7 @@ class TemplateSchema(BaseTemplateSchema): @validates_schema def validate_type(self, data): template_type = data.get('template_type') - if template_type and template_type == 'email': + if template_type and template_type == models.EMAIL_TYPE: subject = data.get('subject') if not subject or subject.strip() == '': raise ValidationError('Invalid template subject', 'subject') From 705a0e7ab887060f0777b57b8b6149c292c7d98a Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 7 Nov 2016 15:21:48 +0000 Subject: [PATCH 38/50] Remove redundant assignment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This variable is used exactly once, on the next line 🤔 --- app/schemas.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/schemas.py b/app/schemas.py index a9811dfc3..610683a2d 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -212,8 +212,7 @@ class TemplateSchema(BaseTemplateSchema): @validates_schema def validate_type(self, data): - template_type = data.get('template_type') - if template_type and template_type == models.EMAIL_TYPE: + if data.get('template_type') in [models.EMAIL_TYPE, models.LETTER_TYPE]: subject = data.get('subject') if not subject or subject.strip() == '': raise ValidationError('Invalid template subject', 'subject') From aef62dcffb83184be74a4d7665b037dc9ac0c123 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 7 Nov 2016 15:34:54 +0000 Subject: [PATCH 39/50] Parametrize template create Cleaner than having two almost identical tests. --- tests/app/dao/test_templates_dao.py | 10 ++++++++-- tests/app/template/test_rest.py | 20 ++++++++++++++++---- 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/tests/app/dao/test_templates_dao.py b/tests/app/dao/test_templates_dao.py index a01126445..287db454c 100644 --- a/tests/app/dao/test_templates_dao.py +++ b/tests/app/dao/test_templates_dao.py @@ -10,14 +10,20 @@ from app.models import Template, TemplateHistory import pytest -def test_create_template(sample_service, sample_user): +@pytest.mark.parametrize('template_type, subject', [ + ('sms', None), + ('email', 'subject'), +]) +def test_create_template(sample_service, sample_user, template_type, subject): data = { 'name': 'Sample Template', - 'template_type': "sms", + 'template_type': template_type, 'content': "Template content", 'service': sample_service, 'created_by': sample_user } + if subject: + data.update({'subject': subject}) template = Template(**data) dao_create_template(template) diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index 2d36e4360..8f18e5f0e 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -1,3 +1,4 @@ +import pytest import json import random import string @@ -8,16 +9,24 @@ from tests.app.conftest import sample_template as create_sample_template from app.dao.templates_dao import dao_get_template_by_id -def test_should_create_a_new_sms_template_for_a_service(notify_api, sample_user, sample_service): +@pytest.mark.parametrize('template_type, subject', [ + ('sms', None), + ('email', 'subject'), +]) +def test_should_create_a_new_template_for_a_service( + notify_api, sample_user, sample_service, template_type, subject +): with notify_api.test_request_context(): with notify_api.test_client() as client: data = { 'name': 'my template', - 'template_type': 'sms', + 'template_type': template_type, 'content': 'template content', 'service': str(sample_service.id), 'created_by': str(sample_user.id) } + if subject: + data.update({'subject': subject}) data = json.dumps(data) auth_header = create_authorization_header() @@ -29,12 +38,15 @@ def test_should_create_a_new_sms_template_for_a_service(notify_api, sample_user, assert response.status_code == 201 json_resp = json.loads(response.get_data(as_text=True)) assert json_resp['data']['name'] == 'my template' - assert json_resp['data']['template_type'] == 'sms' + assert json_resp['data']['template_type'] == template_type assert json_resp['data']['content'] == 'template content' assert json_resp['data']['service'] == str(sample_service.id) assert json_resp['data']['id'] assert json_resp['data']['version'] == 1 - assert not json_resp['data']['subject'] + if subject: + assert json_resp['data']['subject'] == 'subject' + else: + assert not json_resp['data']['subject'] def test_should_create_a_new_email_template_for_a_service(notify_api, sample_user, sample_service): From f34b58e7cd3c68f6fab7868cf68371266edf5a8f Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 7 Nov 2016 15:35:15 +0000 Subject: [PATCH 40/50] Delete redundant test Not needed now that the test above this one is parametrized. --- tests/app/dao/test_templates_dao.py | 17 ---------------- tests/app/template/test_rest.py | 30 ----------------------------- 2 files changed, 47 deletions(-) diff --git a/tests/app/dao/test_templates_dao.py b/tests/app/dao/test_templates_dao.py index 287db454c..95eb9b19c 100644 --- a/tests/app/dao/test_templates_dao.py +++ b/tests/app/dao/test_templates_dao.py @@ -32,23 +32,6 @@ def test_create_template(sample_service, sample_user, template_type, subject): assert dao_get_all_templates_for_service(sample_service.id)[0].name == 'Sample Template' -def test_create_email_template(sample_service, sample_user): - data = { - 'name': 'Sample Template', - 'template_type': "email", - 'subject': "subject", - 'content': "Template content", - 'service': sample_service, - 'created_by': sample_user - } - template = Template(**data) - dao_create_template(template) - - assert Template.query.count() == 1 - assert len(dao_get_all_templates_for_service(sample_service.id)) == 1 - assert dao_get_all_templates_for_service(sample_service.id)[0].name == 'Sample Template' - - def test_update_template(sample_service, sample_user): data = { 'name': 'Sample Template', diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index 8f18e5f0e..890410bd3 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -49,36 +49,6 @@ def test_should_create_a_new_template_for_a_service( assert not json_resp['data']['subject'] -def test_should_create_a_new_email_template_for_a_service(notify_api, sample_user, sample_service): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - data = { - 'name': 'my template', - 'template_type': 'email', - 'subject': 'subject', - 'content': 'template content', - 'service': str(sample_service.id), - 'created_by': str(sample_user.id) - } - data = json.dumps(data) - auth_header = create_authorization_header() - - response = client.post( - '/service/{}/template'.format(sample_service.id), - headers=[('Content-Type', 'application/json'), auth_header], - data=data - ) - assert response.status_code == 201 - json_resp = json.loads(response.get_data(as_text=True)) - assert json_resp['data']['name'] == 'my template' - assert json_resp['data']['template_type'] == 'email' - assert json_resp['data']['content'] == 'template content' - assert json_resp['data']['service'] == str(sample_service.id) - assert json_resp['data']['subject'] == 'subject' - assert json_resp['data']['version'] == 1 - assert json_resp['data']['id'] - - def test_should_be_error_if_service_does_not_exist_on_create(notify_api, sample_user, fake_uuid): with notify_api.test_request_context(): with notify_api.test_client() as client: From 73fe331dc8dce4560d056b4df35e6c1af7f93659 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 7 Nov 2016 15:38:04 +0000 Subject: [PATCH 41/50] Test can create letters template It works already, just making sure it stays working. --- tests/app/dao/test_templates_dao.py | 1 + tests/app/template/test_rest.py | 6 ++++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/app/dao/test_templates_dao.py b/tests/app/dao/test_templates_dao.py index 95eb9b19c..94be5c965 100644 --- a/tests/app/dao/test_templates_dao.py +++ b/tests/app/dao/test_templates_dao.py @@ -13,6 +13,7 @@ import pytest @pytest.mark.parametrize('template_type, subject', [ ('sms', None), ('email', 'subject'), + ('letter', 'subject'), ]) def test_create_template(sample_service, sample_user, template_type, subject): data = { diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index 890410bd3..d448f3d62 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -12,6 +12,7 @@ from app.dao.templates_dao import dao_get_template_by_id @pytest.mark.parametrize('template_type, subject', [ ('sms', None), ('email', 'subject'), + ('letter', 'subject'), ]) def test_should_create_a_new_template_for_a_service( notify_api, sample_user, sample_service, template_type, subject @@ -116,12 +117,13 @@ def test_should_be_error_if_service_does_not_exist_on_update(notify_api, fake_uu assert json_resp['message'] == 'No result found' -def test_must_have_a_subject_on_an_email_template(notify_api, sample_user, sample_service): +@pytest.mark.parametrize('template_type', ['email', 'letter']) +def test_must_have_a_subject_on_an_email_or_letter_template(notify_api, sample_user, sample_service, template_type): with notify_api.test_request_context(): with notify_api.test_client() as client: data = { 'name': 'my template', - 'template_type': 'email', + 'template_type': template_type, 'content': 'template content', 'service': str(sample_service.id), 'created_by': str(sample_user.id) From 6127ee422aa513692d0510751882cbce7e991867 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 7 Nov 2016 15:44:13 +0000 Subject: [PATCH 42/50] Test can get letter template It works already, just making sure it stays working. --- tests/app/conftest.py | 2 +- tests/app/template/test_rest.py | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 435ea3bf5..566c888e1 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -180,7 +180,7 @@ def sample_template(notify_db, 'created_by': created_by, 'archived': archived } - if template_type == 'email': + if template_type in ['email', 'letter']: data.update({ 'subject': subject_line }) diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index d448f3d62..7c6525244 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -321,6 +321,11 @@ def test_should_get_only_templates_for_that_service(notify_api, sample_user, ser None, 'hello ((name)) we’ve received your ((thing))', 'sms' + ), + ( + 'about your ((thing))', + 'hello ((name)) we’ve received your ((thing))', + 'letter' ) ] ) From 28d5da638b2cd6711962ad1736617f6b94183a6c Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 7 Nov 2016 15:37:39 +0000 Subject: [PATCH 43/50] Remove old style fixture Use new `client` one instead --- tests/app/template/test_rest.py | 713 +++++++++++++++----------------- 1 file changed, 343 insertions(+), 370 deletions(-) diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index 7c6525244..fbd8c83ad 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -15,299 +15,281 @@ from app.dao.templates_dao import dao_get_template_by_id ('letter', 'subject'), ]) def test_should_create_a_new_template_for_a_service( - notify_api, sample_user, sample_service, template_type, subject + client, sample_user, sample_service, template_type, subject ): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - data = { - 'name': 'my template', - 'template_type': template_type, - 'content': 'template content', - 'service': str(sample_service.id), - 'created_by': str(sample_user.id) - } - if subject: - data.update({'subject': subject}) - data = json.dumps(data) - auth_header = create_authorization_header() + data = { + 'name': 'my template', + 'template_type': template_type, + 'content': 'template content', + 'service': str(sample_service.id), + 'created_by': str(sample_user.id) + } + if subject: + data.update({'subject': subject}) + data = json.dumps(data) + auth_header = create_authorization_header() - response = client.post( - '/service/{}/template'.format(sample_service.id), - headers=[('Content-Type', 'application/json'), auth_header], - data=data - ) - assert response.status_code == 201 - json_resp = json.loads(response.get_data(as_text=True)) - assert json_resp['data']['name'] == 'my template' - assert json_resp['data']['template_type'] == template_type - assert json_resp['data']['content'] == 'template content' - assert json_resp['data']['service'] == str(sample_service.id) - assert json_resp['data']['id'] - assert json_resp['data']['version'] == 1 - if subject: - assert json_resp['data']['subject'] == 'subject' - else: - assert not json_resp['data']['subject'] + response = client.post( + '/service/{}/template'.format(sample_service.id), + headers=[('Content-Type', 'application/json'), auth_header], + data=data + ) + assert response.status_code == 201 + json_resp = json.loads(response.get_data(as_text=True)) + assert json_resp['data']['name'] == 'my template' + assert json_resp['data']['template_type'] == template_type + assert json_resp['data']['content'] == 'template content' + assert json_resp['data']['service'] == str(sample_service.id) + assert json_resp['data']['id'] + assert json_resp['data']['version'] == 1 + if subject: + assert json_resp['data']['subject'] == 'subject' + else: + assert not json_resp['data']['subject'] -def test_should_be_error_if_service_does_not_exist_on_create(notify_api, sample_user, fake_uuid): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - data = { - 'name': 'my template', - 'template_type': 'sms', - 'content': 'template content', - 'service': fake_uuid, - 'created_by': str(sample_user.id) - } - data = json.dumps(data) - auth_header = create_authorization_header() +def test_should_be_error_if_service_does_not_exist_on_create(client, sample_user, fake_uuid): + data = { + 'name': 'my template', + 'template_type': 'sms', + 'content': 'template content', + 'service': fake_uuid, + 'created_by': str(sample_user.id) + } + data = json.dumps(data) + auth_header = create_authorization_header() - response = client.post( - '/service/{}/template'.format(fake_uuid), - headers=[('Content-Type', 'application/json'), auth_header], - data=data - ) - json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 404 - assert json_resp['result'] == 'error' - assert json_resp['message'] == 'No result found' + response = client.post( + '/service/{}/template'.format(fake_uuid), + headers=[('Content-Type', 'application/json'), auth_header], + data=data + ) + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 404 + assert json_resp['result'] == 'error' + assert json_resp['message'] == 'No result found' -def test_should_error_if_created_by_missing(notify_api, sample_user, sample_service): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - service_id = str(sample_service.id) - data = { - 'name': 'my template', - 'template_type': 'sms', - 'content': 'template content', - 'service': service_id - } - data = json.dumps(data) - auth_header = create_authorization_header() +def test_should_error_if_created_by_missing(client, sample_user, sample_service): + service_id = str(sample_service.id) + data = { + 'name': 'my template', + 'template_type': 'sms', + 'content': 'template content', + 'service': service_id + } + data = json.dumps(data) + auth_header = create_authorization_header() - response = client.post( - '/service/{}/template'.format(service_id), - headers=[('Content-Type', 'application/json'), auth_header], - data=data - ) - json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 400 - assert json_resp['result'] == 'error' + response = client.post( + '/service/{}/template'.format(service_id), + headers=[('Content-Type', 'application/json'), auth_header], + data=data + ) + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 400 + assert json_resp['result'] == 'error' -def test_should_be_error_if_service_does_not_exist_on_update(notify_api, fake_uuid): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - data = { - 'name': 'my template' - } - data = json.dumps(data) - auth_header = create_authorization_header() +def test_should_be_error_if_service_does_not_exist_on_update(client, fake_uuid): + data = { + 'name': 'my template' + } + data = json.dumps(data) + auth_header = create_authorization_header() - response = client.post( - '/service/{}/template/{}'.format(fake_uuid, fake_uuid), - headers=[('Content-Type', 'application/json'), auth_header], - data=data - ) - json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 404 - assert json_resp['result'] == 'error' - assert json_resp['message'] == 'No result found' + response = client.post( + '/service/{}/template/{}'.format(fake_uuid, fake_uuid), + headers=[('Content-Type', 'application/json'), auth_header], + data=data + ) + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 404 + assert json_resp['result'] == 'error' + assert json_resp['message'] == 'No result found' @pytest.mark.parametrize('template_type', ['email', 'letter']) -def test_must_have_a_subject_on_an_email_or_letter_template(notify_api, sample_user, sample_service, template_type): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - data = { - 'name': 'my template', - 'template_type': template_type, - 'content': 'template content', - 'service': str(sample_service.id), - 'created_by': str(sample_user.id) - } - data = json.dumps(data) - auth_header = create_authorization_header() +def test_must_have_a_subject_on_an_email_or_letter_template(client, sample_user, sample_service, template_type): + data = { + 'name': 'my template', + 'template_type': template_type, + 'content': 'template content', + 'service': str(sample_service.id), + 'created_by': str(sample_user.id) + } + data = json.dumps(data) + auth_header = create_authorization_header() - response = client.post( - '/service/{}/template'.format(sample_service.id), - headers=[('Content-Type', 'application/json'), auth_header], - data=data - ) - json_resp = json.loads(response.get_data(as_text=True)) - assert response.status_code == 400 - assert json_resp['result'] == 'error' - assert json_resp['message'] == {'subject': ['Invalid template subject']} + response = client.post( + '/service/{}/template'.format(sample_service.id), + headers=[('Content-Type', 'application/json'), auth_header], + data=data + ) + json_resp = json.loads(response.get_data(as_text=True)) + assert response.status_code == 400 + assert json_resp['result'] == 'error' + assert json_resp['message'] == {'subject': ['Invalid template subject']} -def test_update_should_update_a_template(notify_api, sample_user, sample_template): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - data = { - 'content': 'my template has new content ', - 'created_by': str(sample_user.id) - } - data = json.dumps(data) - auth_header = create_authorization_header() +def test_update_should_update_a_template(client, sample_user, sample_template): + data = { + 'content': 'my template has new content ', + 'created_by': str(sample_user.id) + } + data = json.dumps(data) + auth_header = create_authorization_header() - update_response = client.post( - '/service/{}/template/{}'.format(sample_template.service_id, sample_template.id), - headers=[('Content-Type', 'application/json'), auth_header], - data=data - ) + update_response = client.post( + '/service/{}/template/{}'.format(sample_template.service_id, sample_template.id), + headers=[('Content-Type', 'application/json'), auth_header], + data=data + ) - assert update_response.status_code == 200 - update_json_resp = json.loads(update_response.get_data(as_text=True)) - assert update_json_resp['data']['content'] == 'my template has new content alert("foo")' - assert update_json_resp['data']['name'] == sample_template.name - assert update_json_resp['data']['template_type'] == sample_template.template_type - assert update_json_resp['data']['version'] == 2 + assert update_response.status_code == 200 + update_json_resp = json.loads(update_response.get_data(as_text=True)) + assert update_json_resp['data']['content'] == 'my template has new content alert("foo")' + assert update_json_resp['data']['name'] == sample_template.name + assert update_json_resp['data']['template_type'] == sample_template.template_type + assert update_json_resp['data']['version'] == 2 -def test_should_be_able_to_archive_template(notify_api, sample_template): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - data = { - 'name': sample_template.name, - 'template_type': sample_template.template_type, - 'content': sample_template.content, - 'archived': True, - 'service': str(sample_template.service.id), - 'created_by': str(sample_template.created_by.id) - } +def test_should_be_able_to_archive_template(client, sample_template): + data = { + 'name': sample_template.name, + 'template_type': sample_template.template_type, + 'content': sample_template.content, + 'archived': True, + 'service': str(sample_template.service.id), + 'created_by': str(sample_template.created_by.id) + } - json_data = json.dumps(data) + json_data = json.dumps(data) - auth_header = create_authorization_header() + auth_header = create_authorization_header() - resp = client.post( - '/service/{}/template/{}'.format(sample_template.service.id, sample_template.id), - headers=[('Content-Type', 'application/json'), auth_header], - data=json_data - ) + resp = client.post( + '/service/{}/template/{}'.format(sample_template.service.id, sample_template.id), + headers=[('Content-Type', 'application/json'), auth_header], + data=json_data + ) - assert resp.status_code == 200 - assert Template.query.first().archived + assert resp.status_code == 200 + assert Template.query.first().archived -def test_should_be_able_to_get_all_templates_for_a_service(notify_api, sample_user, sample_service): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - data = { - 'name': 'my template 1', - 'template_type': 'email', - 'subject': 'subject 1', - 'content': 'template content', - 'service': str(sample_service.id), - 'created_by': str(sample_user.id) - } - data_1 = json.dumps(data) - data = { - 'name': 'my template 2', - 'template_type': 'email', - 'subject': 'subject 2', - 'content': 'template content', - 'service': str(sample_service.id), - 'created_by': str(sample_user.id) - } - data_2 = json.dumps(data) - auth_header = create_authorization_header() - client.post( - '/service/{}/template'.format(sample_service.id), - headers=[('Content-Type', 'application/json'), auth_header], - data=data_1 - ) - auth_header = create_authorization_header() +def test_should_be_able_to_get_all_templates_for_a_service(client, sample_user, sample_service): + data = { + 'name': 'my template 1', + 'template_type': 'email', + 'subject': 'subject 1', + 'content': 'template content', + 'service': str(sample_service.id), + 'created_by': str(sample_user.id) + } + data_1 = json.dumps(data) + data = { + 'name': 'my template 2', + 'template_type': 'email', + 'subject': 'subject 2', + 'content': 'template content', + 'service': str(sample_service.id), + 'created_by': str(sample_user.id) + } + data_2 = json.dumps(data) + auth_header = create_authorization_header() + client.post( + '/service/{}/template'.format(sample_service.id), + headers=[('Content-Type', 'application/json'), auth_header], + data=data_1 + ) + auth_header = create_authorization_header() - client.post( - '/service/{}/template'.format(sample_service.id), - headers=[('Content-Type', 'application/json'), auth_header], - data=data_2 - ) + client.post( + '/service/{}/template'.format(sample_service.id), + headers=[('Content-Type', 'application/json'), auth_header], + data=data_2 + ) - auth_header = create_authorization_header() + auth_header = create_authorization_header() - response = client.get( - '/service/{}/template'.format(sample_service.id), - headers=[auth_header] - ) + response = client.get( + '/service/{}/template'.format(sample_service.id), + headers=[auth_header] + ) - assert response.status_code == 200 - update_json_resp = json.loads(response.get_data(as_text=True)) - assert update_json_resp['data'][0]['name'] == 'my template 2' - assert update_json_resp['data'][0]['version'] == 1 - assert update_json_resp['data'][0]['created_at'] - assert update_json_resp['data'][1]['name'] == 'my template 1' - assert update_json_resp['data'][1]['version'] == 1 - assert update_json_resp['data'][1]['created_at'] + assert response.status_code == 200 + update_json_resp = json.loads(response.get_data(as_text=True)) + assert update_json_resp['data'][0]['name'] == 'my template 2' + assert update_json_resp['data'][0]['version'] == 1 + assert update_json_resp['data'][0]['created_at'] + assert update_json_resp['data'][1]['name'] == 'my template 1' + assert update_json_resp['data'][1]['version'] == 1 + assert update_json_resp['data'][1]['created_at'] -def test_should_get_only_templates_for_that_service(notify_api, sample_user, service_factory): - with notify_api.test_request_context(): - with notify_api.test_client() as client: +def test_should_get_only_templates_for_that_service(client, sample_user, service_factory): - service_1 = service_factory.get('service 1', email_from='service.1') - service_2 = service_factory.get('service 2', email_from='service.2') + service_1 = service_factory.get('service 1', email_from='service.1') + service_2 = service_factory.get('service 2', email_from='service.2') - auth_header_1 = create_authorization_header() + auth_header_1 = create_authorization_header() - response_1 = client.get( - '/service/{}/template'.format(service_1.id), - headers=[auth_header_1] - ) + response_1 = client.get( + '/service/{}/template'.format(service_1.id), + headers=[auth_header_1] + ) - auth_header_2 = create_authorization_header() + auth_header_2 = create_authorization_header() - response_2 = client.get( - '/service/{}/template'.format(service_2.id), - headers=[auth_header_2] - ) + response_2 = client.get( + '/service/{}/template'.format(service_2.id), + headers=[auth_header_2] + ) - assert response_1.status_code == 200 - assert response_2.status_code == 200 + assert response_1.status_code == 200 + assert response_2.status_code == 200 - json_resp_1 = json.loads(response_1.get_data(as_text=True)) - json_resp_2 = json.loads(response_2.get_data(as_text=True)) + json_resp_1 = json.loads(response_1.get_data(as_text=True)) + json_resp_2 = json.loads(response_2.get_data(as_text=True)) - assert len(json_resp_1['data']) == 1 - assert len(json_resp_2['data']) == 1 + assert len(json_resp_1['data']) == 1 + assert len(json_resp_2['data']) == 1 - data = { - 'name': 'my template 2', - 'template_type': 'email', - 'subject': 'subject 2', - 'content': 'template content', - 'service': str(service_1.id), - 'created_by': str(sample_user.id) - } - data = json.dumps(data) - create_auth_header = create_authorization_header() - resp = client.post( - '/service/{}/template'.format(service_1.id), - headers=[('Content-Type', 'application/json'), create_auth_header], - data=data - ) + data = { + 'name': 'my template 2', + 'template_type': 'email', + 'subject': 'subject 2', + 'content': 'template content', + 'service': str(service_1.id), + 'created_by': str(sample_user.id) + } + data = json.dumps(data) + create_auth_header = create_authorization_header() + resp = client.post( + '/service/{}/template'.format(service_1.id), + headers=[('Content-Type', 'application/json'), create_auth_header], + data=data + ) - response_3 = client.get( - '/service/{}/template'.format(service_1.id), - headers=[auth_header_1] - ) + response_3 = client.get( + '/service/{}/template'.format(service_1.id), + headers=[auth_header_1] + ) - response_4 = client.get( - '/service/{}/template'.format(service_2.id), - headers=[auth_header_2] - ) + response_4 = client.get( + '/service/{}/template'.format(service_2.id), + headers=[auth_header_2] + ) - assert response_3.status_code == 200 - assert response_4.status_code == 200 + assert response_3.status_code == 200 + assert response_4.status_code == 200 - json_resp_3 = json.loads(response_3.get_data(as_text=True)) - json_resp_4 = json.loads(response_4.get_data(as_text=True)) + json_resp_3 = json.loads(response_3.get_data(as_text=True)) + json_resp_4 = json.loads(response_4.get_data(as_text=True)) - assert len(json_resp_3['data']) == 2 - assert len(json_resp_4['data']) == 1 + assert len(json_resp_3['data']) == 2 + assert len(json_resp_4['data']) == 1 @pytest.mark.parametrize( @@ -331,29 +313,28 @@ def test_should_get_only_templates_for_that_service(notify_api, sample_user, ser ) def test_should_get_a_single_template( notify_db, - notify_api, + client, sample_user, service_factory, subject, content, template_type ): - with notify_api.test_request_context(), notify_api.test_client() as client: - template = create_sample_template( - notify_db, notify_db.session, subject_line=subject, content=content, template_type=template_type - ) + template = create_sample_template( + notify_db, notify_db.session, subject_line=subject, content=content, template_type=template_type + ) - response = client.get( - '/service/{}/template/{}'.format(template.service.id, template.id), - headers=[create_authorization_header()] - ) + response = client.get( + '/service/{}/template/{}'.format(template.service.id, template.id), + headers=[create_authorization_header()] + ) - data = json.loads(response.get_data(as_text=True))['data'] + data = json.loads(response.get_data(as_text=True))['data'] - assert response.status_code == 200 - assert data['content'] == content - assert data['subject'] == subject + assert response.status_code == 200 + assert data['content'] == content + assert data['subject'] == subject @pytest.mark.parametrize( @@ -392,7 +373,7 @@ def test_should_get_a_single_template( ) def test_should_preview_a_single_template( notify_db, - notify_api, + client, sample_user, service_factory, subject, @@ -402,144 +383,136 @@ def test_should_preview_a_single_template( expected_content, expected_error ): - with notify_api.test_request_context(), notify_api.test_client() as client: - template = create_sample_template( - notify_db, notify_db.session, subject_line=subject, content=content, template_type='email' - ) + template = create_sample_template( + notify_db, notify_db.session, subject_line=subject, content=content, template_type='email' + ) - response = client.get( - path.format(template.service.id, template.id), - headers=[create_authorization_header()] - ) + response = client.get( + path.format(template.service.id, template.id), + headers=[create_authorization_header()] + ) - content = json.loads(response.get_data(as_text=True)) + content = json.loads(response.get_data(as_text=True)) - if expected_error: - assert response.status_code == 400 - assert content['message']['template'] == [expected_error] - else: - assert response.status_code == 200 - assert content['content'] == expected_content - assert content['subject'] == expected_subject + if expected_error: + assert response.status_code == 400 + assert content['message']['template'] == [expected_error] + else: + assert response.status_code == 200 + assert content['content'] == expected_content + assert content['subject'] == expected_subject -def test_should_return_empty_array_if_no_templates_for_service(notify_api, sample_service): - with notify_api.test_request_context(): - with notify_api.test_client() as client: +def test_should_return_empty_array_if_no_templates_for_service(client, sample_service): - auth_header = create_authorization_header() + auth_header = create_authorization_header() - response = client.get( - '/service/{}/template'.format(sample_service.id), - headers=[auth_header] - ) + response = client.get( + '/service/{}/template'.format(sample_service.id), + headers=[auth_header] + ) - assert response.status_code == 200 - json_resp = json.loads(response.get_data(as_text=True)) - assert len(json_resp['data']) == 0 + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True)) + assert len(json_resp['data']) == 0 -def test_should_return_404_if_no_templates_for_service_with_id(notify_api, sample_service, fake_uuid): - with notify_api.test_request_context(): - with notify_api.test_client() as client: +def test_should_return_404_if_no_templates_for_service_with_id(client, sample_service, fake_uuid): - auth_header = create_authorization_header() + auth_header = create_authorization_header() - response = client.get( - '/service/{}/template/{}'.format(sample_service.id, fake_uuid), - headers=[auth_header] - ) + response = client.get( + '/service/{}/template/{}'.format(sample_service.id, fake_uuid), + headers=[auth_header] + ) - assert response.status_code == 404 - json_resp = json.loads(response.get_data(as_text=True)) - assert json_resp['result'] == 'error' - assert json_resp['message'] == 'No result found' + assert response.status_code == 404 + json_resp = json.loads(response.get_data(as_text=True)) + assert json_resp['result'] == 'error' + assert json_resp['message'] == 'No result found' -def test_create_400_for_over_limit_content(notify_api, sample_user, sample_service, fake_uuid): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - limit = notify_api.config.get('SMS_CHAR_COUNT_LIMIT') - content = ''.join(random.choice(string.ascii_uppercase + string.digits) for _ in range(limit + 1)) - data = { - 'name': 'too big template', - 'template_type': 'sms', - 'content': content, - 'service': str(sample_service.id), - 'created_by': str(sample_user.id) - } - data = json.dumps(data) - auth_header = create_authorization_header() +def test_create_400_for_over_limit_content(client, notify_api, sample_user, sample_service, fake_uuid): - response = client.post( - '/service/{}/template'.format(sample_service.id), - headers=[('Content-Type', 'application/json'), auth_header], - data=data - ) - assert response.status_code == 400 - json_resp = json.loads(response.get_data(as_text=True)) - assert ( - 'Content has a character count greater than the limit of {}' - ).format(limit) in json_resp['message']['content'] + limit = notify_api.config.get('SMS_CHAR_COUNT_LIMIT') + content = ''.join(random.choice(string.ascii_uppercase + string.digits) for _ in range(limit + 1)) + data = { + 'name': 'too big template', + 'template_type': 'sms', + 'content': content, + 'service': str(sample_service.id), + 'created_by': str(sample_user.id) + } + data = json.dumps(data) + auth_header = create_authorization_header() + + response = client.post( + '/service/{}/template'.format(sample_service.id), + headers=[('Content-Type', 'application/json'), auth_header], + data=data + ) + assert response.status_code == 400 + json_resp = json.loads(response.get_data(as_text=True)) + assert ( + 'Content has a character count greater than the limit of {}' + ).format(limit) in json_resp['message']['content'] -def test_update_400_for_over_limit_content(notify_api, sample_user, sample_template): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - limit = notify_api.config.get('SMS_CHAR_COUNT_LIMIT') - json_data = json.dumps({ - 'content': ''.join(random.choice(string.ascii_uppercase + string.digits) for _ in range(limit + 1)), - 'created_by': str(sample_user.id) - }) - auth_header = create_authorization_header() - resp = client.post( - '/service/{}/template/{}'.format(sample_template.service.id, sample_template.id), - headers=[('Content-Type', 'application/json'), auth_header], - data=json_data - ) - assert resp.status_code == 400 - json_resp = json.loads(resp.get_data(as_text=True)) - assert ( - 'Content has a character count greater than the limit of {}' - ).format(limit) in json_resp['message']['content'] +def test_update_400_for_over_limit_content(client, notify_api, sample_user, sample_template): + + limit = notify_api.config.get('SMS_CHAR_COUNT_LIMIT') + json_data = json.dumps({ + 'content': ''.join(random.choice(string.ascii_uppercase + string.digits) for _ in range(limit + 1)), + 'created_by': str(sample_user.id) + }) + auth_header = create_authorization_header() + resp = client.post( + '/service/{}/template/{}'.format(sample_template.service.id, sample_template.id), + headers=[('Content-Type', 'application/json'), auth_header], + data=json_data + ) + assert resp.status_code == 400 + json_resp = json.loads(resp.get_data(as_text=True)) + assert ( + 'Content has a character count greater than the limit of {}' + ).format(limit) in json_resp['message']['content'] -def test_should_return_all_template_versions_for_service_and_template_id(notify_api, sample_template): +def test_should_return_all_template_versions_for_service_and_template_id(client, sample_template): original_content = sample_template.content from app.dao.templates_dao import dao_update_template sample_template.content = original_content + '1' dao_update_template(sample_template) sample_template.content = original_content + '2' dao_update_template(sample_template) - with notify_api.test_request_context(): - with notify_api.test_client() as client: - auth_header = create_authorization_header() - resp = client.get('/service/{}/template/{}/versions'.format(sample_template.service_id, sample_template.id), - headers=[('Content-Type', 'application/json'), auth_header]) - assert resp.status_code == 200 - resp_json = json.loads(resp.get_data(as_text=True))['data'] - assert len(resp_json) == 3 - for x in resp_json: - if x['version'] == 1: - assert x['content'] == original_content - elif x['version'] == 2: - assert x['content'] == original_content + '1' - else: - assert x['content'] == original_content + '2' + + auth_header = create_authorization_header() + resp = client.get('/service/{}/template/{}/versions'.format(sample_template.service_id, sample_template.id), + headers=[('Content-Type', 'application/json'), auth_header]) + assert resp.status_code == 200 + resp_json = json.loads(resp.get_data(as_text=True))['data'] + assert len(resp_json) == 3 + for x in resp_json: + if x['version'] == 1: + assert x['content'] == original_content + elif x['version'] == 2: + assert x['content'] == original_content + '1' + else: + assert x['content'] == original_content + '2' -def test_update_does_not_create_new_version_when_there_is_no_change(notify_api, sample_template): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - auth_header = create_authorization_header() - data = { - 'template_type': sample_template.template_type, - 'content': sample_template.content, - } - resp = client.post('/service/{}/template/{}'.format(sample_template.service_id, sample_template.id), - data=json.dumps(data), - headers=[('Content-Type', 'application/json'), auth_header]) - assert resp.status_code == 200 +def test_update_does_not_create_new_version_when_there_is_no_change(client, sample_template): + + auth_header = create_authorization_header() + data = { + 'template_type': sample_template.template_type, + 'content': sample_template.content, + } + resp = client.post('/service/{}/template/{}'.format(sample_template.service_id, sample_template.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + assert resp.status_code == 200 + template = dao_get_template_by_id(sample_template.id) assert template.version == 1 From 4379308189668e6fcab2a8b09c3fb346aeeb13dd Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 11 Nov 2016 17:28:15 +0000 Subject: [PATCH 44/50] Fix test failures. --- app/celery/tasks.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 75109fd85..1de9b3be9 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -150,12 +150,12 @@ def send_sms(self, ) except SQLAlchemyError as e: - current_app.logger.exception("RETRY: send_sms notification {}".format(saved_notification.id), e) + current_app.logger.exception("RETRY: send_sms notification", e) try: raise self.retry(queue="retry", exc=e) except self.MaxRetriesExceededError: current_app.logger.exception( - "RETRY FAILED: task send_sms failed for notification {}".format(saved_notification.id), + "RETRY FAILED: task send_sms failed for notification", e ) @@ -198,11 +198,11 @@ def send_email(self, service_id, current_app.logger.info("Email {} created at {}".format(saved_notification.id, created_at)) except SQLAlchemyError as e: - current_app.logger.exception("RETRY: send_email notification {}".format(saved_notification.id), e) + current_app.logger.exception("RETRY: send_email notification", e) try: raise self.retry(queue="retry", exc=e) except self.MaxRetriesExceededError: current_app.logger.error( - "RETRY FAILED: task send_email failed for notification {}".format(saved_notification.id), + "RETRY FAILED: task send_email failed for notification", e ) From d706d86c9c7e73a65be57c8171a841c01cf6d36f Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 14 Nov 2016 14:10:40 +0000 Subject: [PATCH 45/50] don't re-delete already archived/revoked templates/api-keys --- app/dao/services_dao.py | 6 +++-- tests/app/service/test_deactivate.py | 38 ++++++++++++++++++++++++++-- 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index bead23a6e..08b30b381 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -86,10 +86,12 @@ def dao_deactive_service(service_id): service.email_from = '_archived_' + service.email_from for template in service.templates: - template.archived = True + if not template.archived: + template.archived = True for api_key in service.api_keys: - api_key.expiry_date = datetime.utcnow() + if not api_key.expiry_date: + api_key.expiry_date = datetime.utcnow() def dao_fetch_service_by_id_and_user(service_id, user_id): diff --git a/tests/app/service/test_deactivate.py b/tests/app/service/test_deactivate.py index 0f9a3dcce..2ed2c01a6 100644 --- a/tests/app/service/test_deactivate.py +++ b/tests/app/service/test_deactivate.py @@ -1,11 +1,14 @@ import uuid -from unittest import mock +from datetime import datetime import pytest +from freezegun import freeze_time from app import db -from app.models import Service, TemplateHistory, ApiKey +from app.models import Service from app.dao.services_dao import dao_deactive_service +from app.dao.api_key_dao import expire_api_key +from app.dao.templates_dao import dao_update_template from tests import create_authorization_header, unwrap_function from tests.app.conftest import ( @@ -79,6 +82,37 @@ def test_deactivating_service_creates_history(deactivated_service): assert history.active is False +@pytest.fixture +def deactivated_service_with_deleted_stuff(client, notify_db, notify_db_session, sample_service): + with freeze_time('2001-01-01'): + template = create_template(notify_db, notify_db_session, template_name='a') + api_key = create_api_key(notify_db, notify_db_session) + + expire_api_key(sample_service.id, api_key.id) + + template.archived = True + dao_update_template(template) + + with freeze_time('2002-02-02'): + auth_header = create_authorization_header() + response = client.post('/service/{}/deactivate'.format(sample_service.id), headers=[auth_header]) + + assert response.status_code == 204 + assert response.data == b'' + return sample_service + + +def test_deactivating_service_doesnt_affect_existing_archived_templates(deactivated_service_with_deleted_stuff): + assert deactivated_service_with_deleted_stuff.templates[0].archived is True + assert deactivated_service_with_deleted_stuff.templates[0].updated_at == datetime(2001, 1, 1, 0, 0, 0) + assert deactivated_service_with_deleted_stuff.templates[0].version == 2 + + +def test_deactivating_service_doesnt_affect_existing_revoked_api_keys(deactivated_service_with_deleted_stuff): + assert deactivated_service_with_deleted_stuff.api_keys[0].expiry_date == datetime(2001, 1, 1, 0, 0, 0) + assert deactivated_service_with_deleted_stuff.api_keys[0].version == 2 + + def test_deactivating_service_rolls_back_everything_on_error(sample_service, sample_api_key, sample_template): unwrapped_deactive_service = unwrap_function(dao_deactive_service) From c00eb0f5a4a33d44a0716cd09a6a4752f184c424 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 14 Nov 2016 14:41:32 +0000 Subject: [PATCH 46/50] Remove from_request method on the Notification db model, was not needed anymore. Use Notifications.query.one(), rather than assert count is 1 and query for all and get first item in list. --- app/models.py | 28 ---------- app/notifications/process_notifications.py | 4 +- tests/app/celery/test_tasks.py | 34 ++++--------- tests/app/test_model.py | 59 ---------------------- 4 files changed, 13 insertions(+), 112 deletions(-) diff --git a/app/models.py b/app/models.py index 5fa8f6a98..5e2d16f6f 100644 --- a/app/models.py +++ b/app/models.py @@ -537,34 +537,6 @@ class Notification(db.Model): if personalisation: self._personalisation = encryption.encrypt(personalisation) - @classmethod - def from_request(cls, - template_id, - template_version, - recipient, - service_id, - personalisation, - notification_type, - api_key_id, - key_type, - job_id, - job_row_number, - created_at): - return cls( - template_id=template_id, - template_version=template_version, - to=recipient, - service_id=service_id, - status='created', - created_at=created_at, - personalisation=personalisation, - notification_type=notification_type, - api_key_id=api_key_id, - key_type=key_type, - job_id=job_id, - job_row_number=job_row_number - ) - class NotificationHistory(db.Model): __tablename__ = 'notification_history' diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index e797350d2..5df9e7b36 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -47,10 +47,10 @@ def persist_notification(template_id, created_at=None, job_id=None, job_row_number=None): - notification = Notification.from_request( + notification = Notification( template_id=template_id, template_version=template_version, - recipient=recipient, + to=recipient, service_id=service_id, personalisation=personalisation, notification_type=notification_type, diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 7dca93dde..44dcb7ce5 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -339,8 +339,7 @@ def test_should_send_template_to_correct_sms_task_and_persist(sample_template_wi datetime.utcnow().strftime(DATETIME_FORMAT) ) - assert Notification.query.count() == 1 - persisted_notification = Notification.query.all()[0] + persisted_notification = Notification.query.one() assert persisted_notification.to == '+447234123123' assert persisted_notification.template_id == sample_template_with_placeholders.id assert persisted_notification.template_version == sample_template_with_placeholders.version @@ -377,8 +376,7 @@ def test_should_put_send_sms_task_in_research_mode_queue_if_research_mode_servic encryption.encrypt(notification), datetime.utcnow().strftime(DATETIME_FORMAT) ) - assert Notification.query.count() == 1 - persisted_notification = Notification.query.all()[0] + persisted_notification = Notification.query.one() provider_tasks.deliver_sms.apply_async.assert_called_once_with( [persisted_notification.id], queue="research-mode" @@ -392,7 +390,7 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif template = sample_template(notify_db, notify_db_session, service=service) notification = _notification_json(template, "+447700900890") # The user’s own number, but in a different format - mocked_deliver_sms = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') notification_id = uuid.uuid4() encrypt_notification = encryption.encrypt(notification) @@ -403,8 +401,7 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif datetime.utcnow().strftime(DATETIME_FORMAT) ) - assert Notification.query.count() == 1 - persisted_notification = Notification.query.all()[0] + persisted_notification = Notification.query.one() assert persisted_notification.to == '+447700900890' assert persisted_notification.template_id == template.id assert persisted_notification.template_version == template.version @@ -440,8 +437,7 @@ def test_should_send_sms_if_restricted_service_and_non_team_number_with_test_key key_type=KEY_TYPE_TEST ) - assert Notification.query.count() == 1 - persisted_notification = Notification.query.all()[0] + persisted_notification = Notification.query.one() mocked_deliver_sms.assert_called_once_with( [persisted_notification.id], queue="send-sms" @@ -469,8 +465,7 @@ def test_should_send_email_if_restricted_service_and_non_team_email_address_with key_type=KEY_TYPE_TEST ) - assert Notification.query.count() == 1 - persisted_notification = Notification.query.all()[0] + persisted_notification = Notification.query.one() mocked_deliver_email.assert_called_once_with( [persisted_notification.id], queue="send-email" @@ -537,8 +532,7 @@ def test_should_put_send_email_task_in_research_mode_queue_if_research_mode_serv datetime.utcnow().strftime(DATETIME_FORMAT) ) - assert Notification.query.count() == 1 - persisted_notification = Notification.query.all()[0] + persisted_notification = Notification.query.one() provider_tasks.deliver_email.apply_async.assert_called_once_with( [persisted_notification.id], queue="research-mode" @@ -563,8 +557,7 @@ def test_should_send_sms_template_to_and_persist_with_job_id(sample_job, sample_ api_key_id=str(sample_api_key.id), key_type=KEY_TYPE_NORMAL ) - assert Notification.query.count() == 1 - persisted_notification = Notification.query.all()[0] + persisted_notification = Notification.query.one() assert persisted_notification.to == '+447234123123' assert persisted_notification.job_id == sample_job.id assert persisted_notification.template_id == sample_job.template.id @@ -660,8 +653,7 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh key_type=sample_api_key.key_type ) - assert Notification.query.count() == 1 - persisted_notification = Notification.query.all()[0] + persisted_notification = Notification.query.one() assert persisted_notification.to == 'my_email@my_email.com' assert persisted_notification.template_id == sample_email_template_with_placeholders.id assert persisted_notification.template_version == sample_email_template_with_placeholders.version @@ -698,8 +690,7 @@ def test_send_email_should_use_template_version_from_job_not_latest(sample_email now.strftime(DATETIME_FORMAT) ) - assert Notification.query.count() == 1 - persisted_notification = Notification.query.all()[0] + persisted_notification = Notification.query.one() assert persisted_notification.to == 'my_email@my_email.com' assert persisted_notification.template_id == sample_email_template.id assert persisted_notification.template_version == version_on_notification @@ -724,8 +715,7 @@ def test_should_use_email_template_subject_placeholders(sample_email_template_wi encryption.encrypt(notification), now.strftime(DATETIME_FORMAT) ) - assert Notification.query.count() == 1 - persisted_notification = Notification.query.all()[0] + persisted_notification = Notification.query.one() assert persisted_notification.to == 'my_email@my_email.com' assert persisted_notification.template_id == sample_email_template_with_placeholders.id assert persisted_notification.status == 'created' @@ -743,8 +733,6 @@ def test_should_use_email_template_and_persist_without_personalisation(sample_em notification = _notification_json(sample_email_template, "my_email@my_email.com") mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') - assert Notification.query.count() == 0 - notification_id = uuid.uuid4() now = datetime.utcnow() diff --git a/tests/app/test_model.py b/tests/app/test_model.py index e6e5877dd..abbd167a5 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -10,65 +10,6 @@ from app.models import ( MOBILE_TYPE, EMAIL_TYPE) -def test_should_build_notification_from_minimal_set_of_api_derived_params(notify_api): - now = datetime.utcnow() - - notification = Notification.from_request( - template_id='template', - template_version='1', - recipient='someone', - service_id='service_id', - notification_type='SMS', - created_at=now, - api_key_id='api_key_id', - key_type='key_type', - personalisation={}, - job_id=None, - job_row_number=None - ) - assert notification.created_at == now - assert notification.template_id == 'template' - assert notification.template_version == '1' - assert not notification.job_row_number - assert not notification.job_id - assert notification.to == 'someone' - assert notification.service_id == 'service_id' - assert notification.status == 'created' - assert not notification.personalisation - assert notification.notification_type == 'SMS' - assert notification.api_key_id == 'api_key_id' - assert notification.key_type == 'key_type' - - -def test_should_build_notification_from_full_set_of_api_derived_params(notify_api): - now = datetime.utcnow() - notification = Notification.from_request(template_id='template', - template_version=1, - recipient='someone', - service_id='service_id', - personalisation={'key': 'value'}, - notification_type='SMS', - api_key_id='api_key_id', - key_type='key_type', - job_id='job_id', - job_row_number=100, - created_at=now - ) - assert notification.created_at == now - assert notification.id is None - assert notification.template_id == 'template' - assert notification.template_version == 1 - assert notification.job_row_number == 100 - assert notification.job_id == 'job_id' - assert notification.to == 'someone' - assert notification.service_id == 'service_id' - assert notification.status == 'created' - assert notification.personalisation == {'key': 'value'} - assert notification.notification_type == 'SMS' - assert notification.api_key_id == 'api_key_id' - assert notification.key_type == 'key_type' - - @pytest.mark.parametrize('mobile_number', [ '07700 900678', '+44 7700 900678' From 2d9d4013e8132e890c65ddc73bd2020cab03023c Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 14 Nov 2016 15:29:23 +0000 Subject: [PATCH 47/50] Missed one --- tests/app/celery/test_tasks.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 44dcb7ce5..04b449cff 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -742,8 +742,7 @@ def test_should_use_email_template_and_persist_without_personalisation(sample_em encryption.encrypt(notification), now.strftime(DATETIME_FORMAT) ) - assert Notification.query.count() == 1 - persisted_notification = Notification.query.all()[0] + persisted_notification = Notification.query.one() assert persisted_notification.to == 'my_email@my_email.com' assert persisted_notification.template_id == sample_email_template.id assert persisted_notification.created_at == now From d6c537cf440aba48b34c3514dd49386d948e9008 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 15 Nov 2016 11:44:48 +0000 Subject: [PATCH 48/50] Added job_id and row_number to the exception error message if the task needs to be retried. That way we have a way to tie the exception messages together --- app/celery/tasks.py | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 1de9b3be9..a4b4fd94b 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -117,6 +117,7 @@ def send_sms(self, created_at, api_key_id=None, key_type=KEY_TYPE_NORMAL): + # notification_id is not used, it is created by the db model object notification = encryption.decrypt(encrypted_notification) service = dao_fetch_service_by_id(service_id) @@ -150,14 +151,15 @@ def send_sms(self, ) except SQLAlchemyError as e: - current_app.logger.exception("RETRY: send_sms notification", e) + current_app.logger.exception( + "RETRY: send_sms notification for job {} row number {}".format(notification.get('job', None), + notification.get('row_number', None)), e) try: raise self.retry(queue="retry", exc=e) except self.MaxRetriesExceededError: current_app.logger.exception( - "RETRY FAILED: task send_sms failed for notification", - e - ) + "RETRY FAILED: task send_sms failed for notification".format(notification.get('job', None), + notification.get('row_number', None)), e) @notify_celery.task(bind=True, name="send-email", max_retries=5, default_retry_delay=300) @@ -168,6 +170,7 @@ def send_email(self, service_id, created_at, api_key_id=None, key_type=KEY_TYPE_NORMAL): + # notification_id is not used, it is created by the db model object notification = encryption.decrypt(encrypted_notification) service = dao_fetch_service_by_id(service_id) @@ -198,11 +201,11 @@ def send_email(self, service_id, current_app.logger.info("Email {} created at {}".format(saved_notification.id, created_at)) except SQLAlchemyError as e: - current_app.logger.exception("RETRY: send_email notification", e) + current_app.logger.exception("RETRY: send_email notification".format(notification.get('job', None), + notification.get('row_number', None)), e) try: raise self.retry(queue="retry", exc=e) except self.MaxRetriesExceededError: current_app.logger.error( - "RETRY FAILED: task send_email failed for notification", - e - ) + "RETRY FAILED: task send_email failed for notification".format(notification.get('job', None), + notification.get('row_number', None)), e) From a51dfb41d092b4664892e4996d800af23ca226d4 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 16 Nov 2016 16:11:23 +0000 Subject: [PATCH 49/50] typo paul tried to copy + paste command --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 28dc6e38f..94f88a902 100644 --- a/README.md +++ b/README.md @@ -76,7 +76,7 @@ scripts/run_celery_beat.sh ## To test the application -First, ensure that `scripts/boostrap.sh` has been run, as it creates the test database. +First, ensure that `scripts/bootstrap.sh` has been run, as it creates the test database. Then simply run From 247668202bef0cb9ca5cf61673de7e0737bf295f Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 16 Nov 2016 16:15:30 +0000 Subject: [PATCH 50/50] Fix functional tests --- app/celery/tasks.py | 4 ++-- tests/app/celery/test_tasks.py | 23 ++++++++++++----------- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index a4b4fd94b..45cf6398e 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -142,7 +142,7 @@ def send_sms(self, ) provider_tasks.deliver_sms.apply_async( - [saved_notification.id], + [str(saved_notification.id)], queue='send-sms' if not service.research_mode else 'research-mode' ) @@ -195,7 +195,7 @@ def send_email(self, service_id, ) provider_tasks.deliver_email.apply_async( - [saved_notification.id], + [str(saved_notification.id)], queue='send-email' if not service.research_mode else 'research-mode' ) diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 04b449cff..da4d5201c 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -352,7 +352,7 @@ def test_should_send_template_to_correct_sms_task_and_persist(sample_template_wi assert persisted_notification._personalisation == encryption.encrypt({"name": "Jo"}) assert persisted_notification.notification_type == 'sms' mocked_deliver_sms.assert_called_once_with( - [persisted_notification.id], + [str(persisted_notification.id)], queue="send-sms" ) @@ -378,7 +378,7 @@ def test_should_put_send_sms_task_in_research_mode_queue_if_research_mode_servic ) persisted_notification = Notification.query.one() provider_tasks.deliver_sms.apply_async.assert_called_once_with( - [persisted_notification.id], + [str(persisted_notification.id)], queue="research-mode" ) assert mocked_deliver_sms.called @@ -413,7 +413,7 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif assert not persisted_notification.personalisation assert persisted_notification.notification_type == 'sms' provider_tasks.deliver_sms.apply_async.assert_called_once_with( - [persisted_notification.id], + [str(persisted_notification.id)], queue="send-sms" ) @@ -439,7 +439,7 @@ def test_should_send_sms_if_restricted_service_and_non_team_number_with_test_key persisted_notification = Notification.query.one() mocked_deliver_sms.assert_called_once_with( - [persisted_notification.id], + [str(persisted_notification.id)], queue="send-sms" ) @@ -467,7 +467,7 @@ def test_should_send_email_if_restricted_service_and_non_team_email_address_with persisted_notification = Notification.query.one() mocked_deliver_email.assert_called_once_with( - [persisted_notification.id], + [str(persisted_notification.id)], queue="send-email" ) @@ -534,7 +534,7 @@ def test_should_put_send_email_task_in_research_mode_queue_if_research_mode_serv persisted_notification = Notification.query.one() provider_tasks.deliver_email.apply_async.assert_called_once_with( - [persisted_notification.id], + [str(persisted_notification.id)], queue="research-mode" ) @@ -571,7 +571,7 @@ def test_should_send_sms_template_to_and_persist_with_job_id(sample_job, sample_ assert persisted_notification.notification_type == 'sms' provider_tasks.deliver_sms.apply_async.assert_called_once_with( - [persisted_notification.id], + [str(persisted_notification.id)], queue="send-sms" ) @@ -669,7 +669,7 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh assert persisted_notification.notification_type == 'email' provider_tasks.deliver_email.apply_async.assert_called_once_with( - [persisted_notification.id], queue='send-email') + [str(persisted_notification.id)], queue='send-email') def test_send_email_should_use_template_version_from_job_not_latest(sample_email_template, mocker): @@ -699,7 +699,8 @@ def test_send_email_should_use_template_version_from_job_not_latest(sample_email assert persisted_notification.status == 'created' assert not persisted_notification.sent_by assert persisted_notification.notification_type == 'email' - provider_tasks.deliver_email.apply_async.assert_called_once_with([persisted_notification.id], queue='send-email') + provider_tasks.deliver_email.apply_async.assert_called_once_with([str(persisted_notification.id)], + queue='send-email') def test_should_use_email_template_subject_placeholders(sample_email_template_with_placeholders, mocker): @@ -725,7 +726,7 @@ def test_should_use_email_template_subject_placeholders(sample_email_template_wi assert not persisted_notification.reference assert persisted_notification.notification_type == 'email' provider_tasks.deliver_email.apply_async.assert_called_once_with( - [persisted_notification.id], queue='send-email' + [str(persisted_notification.id)], queue='send-email' ) @@ -752,7 +753,7 @@ def test_should_use_email_template_and_persist_without_personalisation(sample_em assert not persisted_notification.personalisation assert not persisted_notification.reference assert persisted_notification.notification_type == 'email' - provider_tasks.deliver_email.apply_async.assert_called_once_with([persisted_notification.id], + provider_tasks.deliver_email.apply_async.assert_called_once_with([str(persisted_notification.id)], queue='send-email')