From 8df4919029887ed198dc29636ad64b42ae37e110 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 31 Mar 2016 17:46:18 +0100 Subject: [PATCH 1/3] The admin app now sends the email from when creating a service and when updating the service name. This PR removes the need for the email_safe function. The api does not create the email_from field for the service. Tests were updated to reflect this change. --- app/__init__.py | 6 ---- app/service/rest.py | 3 -- tests/app/conftest.py | 13 ++++---- tests/app/dao/test_jobs_dao.py | 3 +- tests/app/dao/test_services_dao.py | 32 ++++++++++---------- tests/app/dao/test_templates_dao.py | 4 +-- tests/app/notifications/test_rest.py | 14 +++++---- tests/app/service/test_api_key_endpoints.py | 2 +- tests/app/service/test_rest.py | 33 ++++++++++++--------- tests/app/template/test_rest.py | 4 +-- 10 files changed, 57 insertions(+), 57 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index f34177786..6802f5353 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -93,11 +93,5 @@ def init_app(app): return response -def email_safe(string): - return "".join([ - character.lower() if character.isalnum() or character == "." else "" for character in re.sub("\s+", ".", string.strip()) # noqa - ]) - - def create_uuid(): return str(uuid.uuid4()) diff --git a/app/service/rest.py b/app/service/rest.py index 9cd34bbb2..b396317d9 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -7,7 +7,6 @@ from flask import ( ) from sqlalchemy.orm.exc import NoResultFound -from app import email_safe from app.dao.api_key_dao import ( save_model_api_key, get_model_api_keys, @@ -72,8 +71,6 @@ def create_service(): user = get_model_users(data['user_id']) data.pop('user_id', None) - if 'name' in data: - data['email_from'] = email_safe(data.get('name', None)) valid_service, errors = service_schema.load(request.get_json()) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index e79b5fe76..0b329724c 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -1,6 +1,6 @@ import pytest from datetime import datetime -from app import (email_safe, db) +from app import db from app.models import ( User, Service, Template, ApiKey, Job, Notification, InvitedUser, Permission) from app.dao.users_dao import (save_model_user, create_user_code, create_secret_code) @@ -16,16 +16,16 @@ import uuid @pytest.fixture(scope='function') def service_factory(notify_db, notify_db_session): class ServiceFactory(object): - def get(self, service_name, user=None, template_type=None): + def get(self, service_name, user=None, template_type=None, email_from=None): if not user: user = sample_user(notify_db, notify_db_session) - service = sample_service(notify_db, notify_db_session, service_name, user) + service = sample_service(notify_db, notify_db_session, service_name, user, email_from=email_from) if template_type == 'email': sample_template( notify_db, notify_db_session, template_type=template_type, - subject_line=email_safe(service_name), + subject_line=service.email_from, service=service ) else: @@ -102,7 +102,8 @@ def sample_service(notify_db, service_name="Sample service", user=None, restricted=False, - limit=1000): + limit=1000, + email_from="sample.service"): if user is None: user = sample_user(notify_db, notify_db_session) data = { @@ -110,7 +111,7 @@ def sample_service(notify_db, 'limit': limit, 'active': False, 'restricted': restricted, - 'email_from': email_safe(service_name) + 'email_from': email_from } service = Service.query.filter_by(name=service_name).first() if not service: diff --git a/tests/app/dao/test_jobs_dao.py b/tests/app/dao/test_jobs_dao.py index d17c3a720..de5894387 100644 --- a/tests/app/dao/test_jobs_dao.py +++ b/tests/app/dao/test_jobs_dao.py @@ -49,7 +49,8 @@ def test_get_jobs_for_service(notify_db, notify_db_session, sample_template): one_job = create_job(notify_db, notify_db_session, sample_template.service, sample_template) other_user = create_user(notify_db, notify_db_session, email="test@digital.cabinet-office.gov.uk") - other_service = create_service(notify_db, notify_db_session, user=other_user, service_name="other service") + other_service = create_service(notify_db, notify_db_session, user=other_user, service_name="other service", + email_from='other.service') other_template = create_template(notify_db, notify_db_session, service=other_service) other_job = create_job(notify_db, notify_db_session, service=other_service, template=other_template) diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index abc946bc4..140256686 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -85,20 +85,20 @@ def test_should_remove_user_from_service(sample_user): def test_get_all_services(service_factory): - service_factory.get('service 1') + service_factory.get('service 1', email_from='service.1') assert len(dao_fetch_all_services()) == 1 assert dao_fetch_all_services()[0].name == 'service 1' - service_factory.get('service 2') + service_factory.get('service 2', email_from='service.2') assert len(dao_fetch_all_services()) == 2 assert dao_fetch_all_services()[1].name == 'service 2' def test_get_all_services_should_return_in_created_order(service_factory): - service_factory.get('service 1') - service_factory.get('service 2') - service_factory.get('service 3') - service_factory.get('service 4') + service_factory.get('service 1', email_from='service.1') + service_factory.get('service 2', email_from='service.2') + service_factory.get('service 3', email_from='service.3') + service_factory.get('service 4', email_from='service.4') assert len(dao_fetch_all_services()) == 4 assert dao_fetch_all_services()[0].name == 'service 1' assert dao_fetch_all_services()[1].name == 'service 2' @@ -111,9 +111,9 @@ def test_get_all_services_should_return_empty_list_if_no_services(): def test_get_all_services_for_user(service_factory, sample_user): - service_factory.get('service 1', sample_user) - service_factory.get('service 2', sample_user) - service_factory.get('service 3', sample_user) + service_factory.get('service 1', sample_user, email_from='service.1') + service_factory.get('service 2', sample_user, email_from='service.2') + service_factory.get('service 3', sample_user, email_from='service.3') assert len(dao_fetch_all_services_by_user(sample_user.id)) == 3 assert dao_fetch_all_services_by_user(sample_user.id)[0].name == 'service 1' assert dao_fetch_all_services_by_user(sample_user.id)[1].name == 'service 2' @@ -121,9 +121,9 @@ def test_get_all_services_for_user(service_factory, sample_user): def test_get_all_only_services_user_has_access_to(service_factory, sample_user): - service_factory.get('service 1', sample_user) - service_factory.get('service 2', sample_user) - service_3 = service_factory.get('service 3', sample_user) + service_factory.get('service 1', sample_user, email_from='service.1') + service_factory.get('service 2', sample_user, email_from='service.2') + service_3 = service_factory.get('service 3', sample_user, email_from='service.3') new_user = User( name='Test User', email_address='new_user@digital.cabinet-office.gov.uk', @@ -151,17 +151,17 @@ def test_get_service_by_id_returns_none_if_no_service(notify_db): def test_get_service_by_id_returns_service(service_factory): - service = service_factory.get('testing') + service = service_factory.get('testing', email_from='testing') assert dao_fetch_service_by_id(service.id).name == 'testing' def test_can_get_service_by_id_and_user(service_factory, sample_user): - service = service_factory.get('service 1', sample_user) + service = service_factory.get('service 1', sample_user, email_from='service.1') assert dao_fetch_service_by_id_and_user(service.id, sample_user.id).name == 'service 1' def test_cannot_get_service_by_id_and_owned_by_different_user(service_factory, sample_user): - service1 = service_factory.get('service 1', sample_user) + service1 = service_factory.get('service 1', sample_user, email_from='service.1') new_user = User( name='Test User', email_address='new_user@digital.cabinet-office.gov.uk', @@ -169,7 +169,7 @@ def test_cannot_get_service_by_id_and_owned_by_different_user(service_factory, s mobile_number='+447700900986' ) save_model_user(new_user) - service2 = service_factory.get('service 2', new_user) + service2 = service_factory.get('service 2', new_user, email_from='service.2') assert dao_fetch_service_by_id_and_user(service1.id, sample_user.id).name == 'service 1' with pytest.raises(NoResultFound) as e: dao_fetch_service_by_id_and_user(service2.id, sample_user.id) diff --git a/tests/app/dao/test_templates_dao.py b/tests/app/dao/test_templates_dao.py index 6b712a642..a3d74afb2 100644 --- a/tests/app/dao/test_templates_dao.py +++ b/tests/app/dao/test_templates_dao.py @@ -74,8 +74,8 @@ def test_update_template(sample_service): def test_get_all_templates_for_service(service_factory): - service_1 = service_factory.get('service 1') - service_2 = service_factory.get('service 2') + service_1 = service_factory.get('service 1', email_from='service.1') + service_2 = service_factory.get('service 2', email_from='service.2') assert Template.query.count() == 2 assert len(dao_get_all_templates_for_service(service_1.id)) == 1 diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index ceec614d2..1b3b8f6a4 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -115,8 +115,8 @@ def test_get_all_notifications_newest_first(notify_api, notify_db, notify_db_ses def test_get_all_notifications_for_service_in_order(notify_api, notify_db, notify_db_session): with notify_api.test_request_context(): with notify_api.test_client() as client: - service_1 = create_sample_service(notify_db, notify_db_session, service_name="1") - service_2 = create_sample_service(notify_db, notify_db_session, service_name="2") + 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') create_sample_notification(notify_db, notify_db_session, service=service_2) @@ -586,8 +586,8 @@ def test_should_not_allow_template_from_another_service(notify_api, service_fact with notify_api.test_client() as client: mocker.patch('app.celery.tasks.send_sms.apply_async') - service_1 = service_factory.get('service 1', user=sample_user) - service_2 = service_factory.get('service 2', user=sample_user) + service_1 = service_factory.get('service 1', user=sample_user, email_from='service.1') + service_2 = service_factory.get('service 2', user=sample_user, email_from='service.2') service_2_templates = dao_get_all_templates_for_service(service_id=service_2.id) data = { @@ -736,8 +736,10 @@ def test_should_not_allow_email_template_from_another_service(notify_api, servic with notify_api.test_client() as client: mocker.patch('app.celery.tasks.send_email.apply_async') - service_1 = service_factory.get('service 1', template_type='email', user=sample_user) - service_2 = service_factory.get('service 2', template_type='email', user=sample_user) + service_1 = service_factory.get('service 1', template_type='email', user=sample_user, + email_from='service.1') + service_2 = service_factory.get('service 2', template_type='email', user=sample_user, + email_from='service.2') service_2_templates = dao_get_all_templates_for_service(service_id=service_2.id) diff --git a/tests/app/service/test_api_key_endpoints.py b/tests/app/service/test_api_key_endpoints.py index aee852396..10a7cebf3 100644 --- a/tests/app/service/test_api_key_endpoints.py +++ b/tests/app/service/test_api_key_endpoints.py @@ -98,7 +98,7 @@ def test_get_api_keys_should_return_all_keys_for_service(notify_api, notify_db, with notify_api.test_client() as client: another_user = create_user(notify_db, notify_db_session, email='another@it.gov.uk') another_service = create_sample_service(notify_db, notify_db_session, service_name='another', - user=another_user) + user=another_user, email_from='another') create_sample_api_key(notify_db, notify_db_session, service=another_service) api_key2 = ApiKey(**{'service_id': sample_api_key.service_id, 'name': 'second_api_key'}) api_key3 = ApiKey(**{'service_id': sample_api_key.service_id, 'name': 'third_api_key', diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 7b2c4e8dd..8dc241fd5 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -13,9 +13,9 @@ from tests.app.conftest import sample_user as create_sample_user 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') - service_factory.get('two') - service_factory.get('three') + service_factory.get('one', email_from='one') + service_factory.get('two', email_from='two') + service_factory.get('three', email_from='three') auth_header = create_authorization_header( path='/service', method='GET' @@ -36,9 +36,9 @@ def test_get_service_list_by_user(notify_api, sample_user, service_factory): with notify_api.test_request_context(): with notify_api.test_client() as client: - service_factory.get('one', sample_user) - service_factory.get('two', sample_user) - service_factory.get('three', sample_user) + 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( path='/service', @@ -67,9 +67,9 @@ def test_get_service_list_by_user_should_return_empty_list_if_no_services(notify ) save_model_user(new_user) - service_factory.get('one', sample_user) - service_factory.get('two', sample_user) - service_factory.get('three', sample_user) + 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( path='/service', @@ -138,7 +138,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) + service = service_factory.get('new service', sample_user, email_from='new.service') auth_header = create_authorization_header( path='/service/{}'.format(service.id), method='GET' @@ -179,7 +179,8 @@ def test_create_service(notify_api, sample_user): 'user_id': sample_user.id, 'limit': 1000, 'restricted': False, - 'active': False} + 'active': False, + 'email_from': 'created.service'} auth_header = create_authorization_header( path='/service', method='POST', @@ -333,7 +334,8 @@ def test_update_service(notify_api, sample_service): assert json_resp['data']['name'] == sample_service.name data = { - 'name': 'updated service name' + 'name': 'updated service name', + 'email_from': 'updated.service.name' } auth_header = create_authorization_header( @@ -350,6 +352,7 @@ def test_update_service(notify_api, sample_service): result = json.loads(resp.get_data(as_text=True)) assert resp.status_code == 200 assert result['data']['name'] == 'updated service name' + assert result['data']['email_from'] == 'updated.service.name' def test_should_not_update_service_with_duplicate_name(notify_api, @@ -364,7 +367,8 @@ def test_should_not_update_service_with_duplicate_name(notify_api, notify_db, notify_db_session, service_name=service_name, - user=sample_user) + user=sample_user, + email_from='another.name') data = { 'name': service_name } @@ -483,7 +487,8 @@ def test_default_permissions_are_added_for_user_service(notify_api, 'user_id': sample_user.id, 'limit': 1000, 'restricted': False, - 'active': False} + 'active': False, + 'email_from': 'created.service'} auth_header = create_authorization_header( path='/service', method='POST', diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index 32ba0e567..fbb3cd6d2 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -303,8 +303,8 @@ def test_should_get_only_templates_for_that_servcie(notify_api, service_factory) with notify_api.test_request_context(): with notify_api.test_client() as client: - service_1 = service_factory.get('service 1') - service_2 = service_factory.get('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( path='/service/{}/template'.format(service_1.id), From 8493e29acc0949ebbc440750a697ab6d2faee080 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 1 Apr 2016 13:42:11 +0100 Subject: [PATCH 2/3] Added some more tests. Removed the validation in the schema - it was adding complexity, let the unique constraint on the db throw the exception. This should only ever happen on a race condition which seems unlikely (two people changing a service to the same name at the same time) Do no set debug=true to the test config. If debug=true it changes the behaviour of the error handlers, throwing the exception rather than returning a 500. --- app/schemas.py | 15 ------- config.py | 4 +- tests/app/service/test_rest.py | 79 +++++++++++++++++++++++++++++----- 3 files changed, 71 insertions(+), 27 deletions(-) diff --git a/app/schemas.py b/app/schemas.py index e943e8419..dc1ce3c48 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -73,21 +73,6 @@ class ServiceSchema(BaseSchema): model = models.Service exclude = ("updated_at", "created_at", "api_keys", "templates", "jobs", 'old_id') - @validates_schema - def validate_all(self, data): - # There are 2 instances where we want to check - # for duplicate service name. One when they updating - # an existing service and when they are creating a service - name = data.get('name', None) - service = models.Service.query.filter_by(name=name).first() - error_msg = "Duplicate service name '{}'".format(name) - if 'id' in data: - if service and str(service.id) != data['id']: - raise ValidationError(error_msg, 'name') - else: - if service: - raise ValidationError(error_msg, 'name') - class TemplateSchema(BaseSchema): class Meta: diff --git a/config.py b/config.py index dde70a44e..e188940b0 100644 --- a/config.py +++ b/config.py @@ -83,8 +83,8 @@ class Development(Config): DEBUG = True -class Test(Development): - DEBUG = True +class Test(Config): + pass configs = { 'live': 'config_live.Live', diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 8dc241fd5..60f1ed3cd 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1,6 +1,7 @@ import json import uuid from flask import url_for + from app.dao.users_dao import save_model_user from app.dao.services_dao import dao_remove_user_from_service from app.models import User @@ -301,7 +302,8 @@ def test_should_not_create_service_with_duplicate_name(notify_api, 'user_id': sample_service.users[0].id, 'limit': 1000, 'restricted': False, - 'active': False} + 'active': False, + 'email_from': 'sample.service2'} auth_header = create_authorization_header( path='/service', method='POST', @@ -312,10 +314,36 @@ def test_should_not_create_service_with_duplicate_name(notify_api, '/service', data=json.dumps(data), headers=headers) - json_resp = json.loads(resp.get_data(as_text=True)) - assert resp.status_code == 400 - assert "Duplicate service name '{}'".format( - sample_service.name) in json_resp['message']['name'] + assert resp.status_code == 500 + assert 'Key (name)=(Sample service) already exists.' in resp.get_data(as_text=True) + + +def test_create_service_should_throw_duplicate_key_constraint_for_existing_email_from(notify_api, + notify_db, + notify_db_session, + service_factory): + first_service = service_factory.get('First service', email_from='first.service') + with notify_api.test_request_context(): + with notify_api.test_client() as client: + data = { + 'name': 'First SERVICE', + 'user_id': first_service.users[0].id, + 'limit': 1000, + 'restricted': False, + 'active': False, + 'email_from': 'first.service'} + auth_header = create_authorization_header( + path='/service', + method='POST', + request_body=json.dumps(data) + ) + headers = [('Content-Type', 'application/json'), auth_header] + resp = client.post( + '/service', + data=json.dumps(data), + headers=headers) + assert resp.status_code == 500 + assert 'Key (email_from)=(first.service) already exists.' in resp.get_data(as_text=True) def test_update_service(notify_api, sample_service): @@ -363,7 +391,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" - another_service = create_sample_service( + create_sample_service( notify_db, notify_db_session, service_name=service_name, @@ -384,10 +412,41 @@ def test_should_not_update_service_with_duplicate_name(notify_api, data=json.dumps(data), headers=[('Content-Type', 'application/json'), auth_header] ) - json_resp = json.loads(resp.get_data(as_text=True)) - assert resp.status_code == 400 - assert "Duplicate service name '{}'".format( - service_name) in json_resp['message']['name'] + assert resp.status_code == 500 + assert 'Key (name)=(another name) already exists.' in resp.get_data(as_text=True) + + +def test_should_not_update_service_with_duplicate_email_from(notify_api, + notify_db, + notify_db_session, + sample_user, + sample_service): + with notify_api.test_request_context(): + with notify_api.test_client() as client: + email_from = "duplicate.name" + create_sample_service( + notify_db, + notify_db_session, + service_name="duplicate name", + user=sample_user, + email_from=email_from) + data = { + 'email_from': email_from + } + + auth_header = create_authorization_header( + path='/service/{}'.format(sample_service.id), + method='POST', + request_body=json.dumps(data) + ) + + resp = client.post( + '/service/{}'.format(sample_service.id), + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header] + ) + assert resp.status_code == 500 + assert 'Key (email_from)=(duplicate.name) already exists.' in resp.get_data(as_text=True) def test_update_service_should_404_if_id_is_invalid(notify_api, notify_db, notify_db_session): From df6f784f9ee37e0aac036dfc2f3a2f0c1b99c161 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 1 Apr 2016 14:19:50 +0100 Subject: [PATCH 3/3] - Remove different behaviour for debug mode in errors.py --- app/errors.py | 4 ---- config.py | 2 +- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/app/errors.py b/app/errors.py index ac0f7bf60..7617e0add 100644 --- a/app/errors.py +++ b/app/errors.py @@ -40,8 +40,6 @@ def register_errors(blueprint): @blueprint.app_errorhandler(500) def internal_server_error(e): - if current_app.config.get('DEBUG'): - raise e if isinstance(e, str): current_app.logger.exception(e) elif isinstance(e, Exception): @@ -60,7 +58,5 @@ def register_errors(blueprint): @blueprint.app_errorhandler(SQLAlchemyError) def db_error(e): - if current_app.config.get('DEBUG'): - raise e current_app.logger.exception(e) return jsonify(result='error', message=str(e)), 500 diff --git a/config.py b/config.py index e188940b0..0bf45618c 100644 --- a/config.py +++ b/config.py @@ -83,7 +83,7 @@ class Development(Config): DEBUG = True -class Test(Config): +class Test(Development): pass configs = {