From 677237ba470995a66a1ca03d421e3fe8ea065929 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 30 Mar 2016 17:12:00 +0100 Subject: [PATCH 1/3] Update email_from when the service name is changed. Update unit tests Service name is uniqueness is not based on case. --- app/main/forms.py | 2 +- app/main/views/service_settings.py | 9 +- app/notify_client/api_client.py | 8 +- app/utils.py | 6 + tests/__init__.py | 5 +- tests/app/main/views/test_service_settings.py | 137 ++++++++---------- tests/conftest.py | 10 +- 7 files changed, 87 insertions(+), 90 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 3e50ee657..a8d5ba87d 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -180,7 +180,7 @@ class ServiceNameForm(Form): ]) def validate_name(self, a): - if a.data in self._names_func(): + if a.data.lower() in self._names_func(): raise ValidationError('This service name is already in use') diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index cfa0c9897..159744fc1 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -15,7 +15,7 @@ from notifications_python_client import HTTPError from app import service_api_client from app.main import main -from app.utils import user_has_permissions +from app.utils import user_has_permissions, email_safe from app.main.forms import ConfirmPasswordForm, ServiceNameForm from app import user_api_client @@ -65,6 +65,7 @@ def service_name_change_confirm(service_id): if form.validate_on_submit(): service['name'] = session['service_name_change'] + service['email_from'] = email_safe(session['service_name_change']) try: service_api_client.update_service( service['id'], @@ -72,7 +73,8 @@ def service_name_change_confirm(service_id): service['active'], service['limit'], service['restricted'], - service['users']) + service['users'], + service['email_from']) except HTTPError as e: error_msg = "Duplicate service name '{}'".format(session['service_name_change']) if e.status_code == 400 and error_msg in e.message['name']: @@ -144,7 +146,8 @@ def service_status_change_confirm(service_id): service['active'], service['limit'], service['restricted'], - service['users']) + service['users'], + service['email_from']) return redirect(url_for('.service_settings', service_id=service_id)) return render_template( 'views/service-settings/confirm.html', diff --git a/app/notify_client/api_client.py b/app/notify_client/api_client.py index 2711254f6..93a76c20c 100644 --- a/app/notify_client/api_client.py +++ b/app/notify_client/api_client.py @@ -57,7 +57,8 @@ class ServiceAPIClient(NotificationsAPIClient): active, limit, restricted, - users): + users, + email_from): """ Update a service. """ @@ -67,7 +68,8 @@ class ServiceAPIClient(NotificationsAPIClient): "active": active, "limit": limit, "restricted": restricted, - "users": users + "users": users, + "email_from": email_from } endpoint = "/service/{0}".format(service_id) return self.post(endpoint, data) @@ -142,7 +144,7 @@ class ServiceAPIClient(NotificationsAPIClient): def find_all_service_names(self, user_id=None): resp = self.get_services(user_id) - return [x['name'] for x in resp['data']] + return [x['name'].lower() for x in resp['data']] class ServicesBrowsableItem(BrowsableItem): diff --git a/app/utils.py b/app/utils.py index 563bb07ff..ede63293a 100644 --- a/app/utils.py +++ b/app/utils.py @@ -101,3 +101,9 @@ def generate_previous_next_dict(view, view_dict, page, title, label): 'title': title, 'label': label } + + +def email_safe(string): + return "".join([ + character.lower() if character.isalnum() or character == "." else "" for character in re.sub("\s+", ".", string.strip()) # noqa + ]) diff --git a/tests/__init__.py b/tests/__init__.py index 7064eac13..bc0513c6e 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -22,14 +22,15 @@ class TestClient(FlaskClient): self.get(url_for("main.logout")) -def service_json(id_, name, users, limit=1000, active=False, restricted=True): +def service_json(id_, name, users, limit=1000, active=False, restricted=True, email_from=None): return { 'id': id_, 'name': name, 'users': users, 'limit': limit, 'active': active, - 'restricted': restricted + 'restricted': restricted, + 'email_from': email_from } diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 59af4e3d6..ef3a16ac0 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -1,82 +1,69 @@ from flask import url_for + +import app +from app.utils import email_safe from tests import validate_route_permission from bs4 import BeautifulSoup def test_should_show_overview(app_, - api_user_active, - mock_get_service, - mock_get_user, - mock_get_user_by_email, - mock_login, - mock_has_permissions): + active_user_with_permissions, + mocker, + service_one): with app_.test_request_context(): with app_.test_client() as client: - client.login(api_user_active) - service_id = 123 + client.login(active_user_with_permissions, mocker, service_one) response = client.get(url_for( - 'main.service_settings', service_id=service_id)) + 'main.service_settings', service_id=service_one['id'])) assert response.status_code == 200 resp_data = response.get_data(as_text=True) assert 'Service settings' in resp_data - service = mock_get_service.side_effect(service_id)['data'] - assert mock_get_service.called + app.service_api_client.get_service.assert_called_with(service_one['id']) def test_should_show_service_name(app_, - api_user_active, - mock_get_service, - mock_get_user, - mock_get_user_by_email, - mock_login, - mock_has_permissions): + active_user_with_permissions, + mocker, + service_one): with app_.test_request_context(): with app_.test_client() as client: - client.login(api_user_active) - service_id = 123 + client.login(active_user_with_permissions, mocker, service_one) response = client.get(url_for( - 'main.service_name_change', service_id=service_id)) + 'main.service_name_change', service_id=service_one['id'])) assert response.status_code == 200 resp_data = response.get_data(as_text=True) assert 'Change your service name' in resp_data - assert mock_get_service.called - service = mock_get_service.side_effect(service_id)['data'] + app.service_api_client.get_service.assert_called_with(service_one['id']) def test_should_redirect_after_change_service_name(app_, - api_user_active, - mock_get_service, - mock_get_user, - mock_get_user_by_email, - mock_login, - mock_has_permissions): + active_user_with_permissions, + service_one, + mocker, + mock_get_services): with app_.test_request_context(): with app_.test_client() as client: - client.login(api_user_active) - service_id = 123 + client.login(active_user_with_permissions, mocker, service_one) response = client.post( - url_for('main.service_name_change', service_id=service_id), + url_for('main.service_name_change', service_id=service_one['id']), data={'name': "new name"}) assert response.status_code == 302 settings_url = url_for( - 'main.service_name_change_confirm', service_id=service_id, _external=True) + 'main.service_name_change_confirm', service_id=service_one['id'], _external=True) assert settings_url == response.location - assert mock_get_service.called + assert mock_get_services.called def test_should_not_allow_duplicate_names(app_, - api_user_active, - mock_get_service, - mock_get_user, - mock_get_user_by_email, - mock_login, - mock_has_permissions, + active_user_with_permissions, + mocker, + service_one, mock_get_services): with app_.test_request_context(): with app_.test_client() as client: - client.login(api_user_active) - service_id = 123 + client.login(active_user_with_permissions, mocker, service_one) + service_id = service_one['id'] response = client.post( url_for('main.service_name_change', service_id=service_id), data={'name': "service_one"}) @@ -87,38 +74,32 @@ def test_should_not_allow_duplicate_names(app_, def test_should_show_service_name_confirmation(app_, - api_user_active, - mock_get_service, - mock_get_user, - mock_get_user_by_email, - mock_login, - mock_has_permissions): + active_user_with_permissions, + mocker, + service_one): with app_.test_request_context(): with app_.test_client() as client: - client.login(api_user_active) - service_id = 123 + client.login(active_user_with_permissions, mocker, service_one) + response = client.get(url_for( - 'main.service_name_change_confirm', service_id=service_id)) + 'main.service_name_change_confirm', service_id=service_one['id'])) assert response.status_code == 200 resp_data = response.get_data(as_text=True) assert 'Change your service name' in resp_data - assert mock_get_service.called + app.service_api_client.get_service.assert_called_with(service_one['id']) def test_should_redirect_after_service_name_confirmation(app_, - api_user_active, - mock_get_service, + active_user_with_permissions, + service_one, + mocker, mock_update_service, - mock_get_user, - mock_get_user_by_email, - mock_login, - mock_verify_password, - mock_has_permissions): + mock_verify_password): with app_.test_request_context(): with app_.test_client() as client: - client.login(api_user_active) - service_id = 123 + client.login(active_user_with_permissions, mocker, service_one) + service_id = service_one['id'] service_new_name = 'New Name' with client.session_transaction() as session: session['service_name_change'] = service_new_name @@ -126,38 +107,40 @@ def test_should_redirect_after_service_name_confirmation(app_, 'main.service_name_change_confirm', service_id=service_id)) assert response.status_code == 302 - settings_url = url_for( - 'main.service_settings', service_id=service_id, _external=True) - resp_data = response.get_data(as_text=True) + settings_url = url_for('main.service_settings', service_id=service_id, _external=True) assert settings_url == response.location - assert mock_get_service.called - assert mock_update_service.called + mock_update_service.assert_called_once_with(service_id, + service_new_name, + service_one['active'], + service_one['limit'], + service_one['restricted'], + service_one['users'], + email_safe(service_new_name)) + assert mock_verify_password.called def test_should_raise_duplicate_name_handled(app_, - api_user_active, - mock_get_service, + active_user_with_permissions, + service_one, + mocker, + mock_get_services, mock_update_service_raise_httperror_duplicate_name, - mock_get_user, - mock_get_user_by_email, - mock_login, - mock_verify_password, - mock_has_permissions): + mock_verify_password): with app_.test_request_context(): with app_.test_client() as client: - client.login(api_user_active) - service_id = 123 + client.login(active_user_with_permissions, mocker, service_one) service_new_name = 'New Name' with client.session_transaction() as session: session['service_name_change'] = service_new_name response = client.post(url_for( - 'main.service_name_change_confirm', service_id=service_id)) + 'main.service_name_change_confirm', service_id=service_one['id'])) assert response.status_code == 302 name_change_url = url_for( - 'main.service_name_change', service_id=service_id, _external=True) - resp_data = response.get_data(as_text=True) + 'main.service_name_change', service_id=service_one['id'], _external=True) assert name_change_url == response.location + assert mock_update_service_raise_httperror_duplicate_name.called + assert mock_verify_password.called def test_should_show_request_to_go_live(app_, diff --git a/tests/conftest.py b/tests/conftest.py index 762846a9a..7fc1acb77 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -77,14 +77,15 @@ def mock_update_service(mocker): active, limit, restricted, - users): + users, + email_from): service = service_json( service_id, service_name, users, limit=limit, - active=active, restricted=restricted) + active=active, restricted=restricted, email_from=email_from) return {'data': service} return mocker.patch( - 'app.service_api_client.update_service', side_effect=_update) + 'app.service_api_client.update_service', side_effect=_update, autospec=True) @pytest.fixture(scope='function') @@ -94,7 +95,8 @@ def mock_update_service_raise_httperror_duplicate_name(mocker): active, limit, restricted, - users): + users, + email_from): json_mock = Mock(return_value={'message': {'name': ["Duplicate service name '{}'".format(service_name)]}}) resp_mock = Mock(status_code=400, json=json_mock) http_error = HTTPError(response=resp_mock, message="Default message") From 1871243cc869944c051cbfeb181d4028df6e260c Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 31 Mar 2016 10:26:03 +0100 Subject: [PATCH 2/3] Check the uniqueness of the service name ignoring case. When the service name changes the email_from changes to. Renamed find_all_service_names to find_all_service_names_lower. --- app/main/forms.py | 2 +- app/main/views/add_service.py | 2 +- app/main/views/service_settings.py | 2 +- app/notify_client/api_client.py | 2 +- tests/__init__.py | 3 +- tests/app/main/views/test_add_service.py | 41 ++++++++----------- tests/app/main/views/test_service_settings.py | 2 +- 7 files changed, 25 insertions(+), 29 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index a8d5ba87d..b711231e4 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -159,7 +159,7 @@ class AddServiceForm(Form): ) def validate_name(self, a): - if a.data in self._names_func(): + if a.data.lower() in self._names_func(): raise ValidationError('This service name is already in use') diff --git a/app/main/views/add_service.py b/app/main/views/add_service.py index 355c9bf09..d943ddb75 100644 --- a/app/main/views/add_service.py +++ b/app/main/views/add_service.py @@ -31,7 +31,7 @@ def add_service(): invite_api_client.accept_invite(service_id, invitation.id) return redirect(url_for('main.service_dashboard', service_id=service_id)) - form = AddServiceForm(service_api_client.find_all_service_names) + form = AddServiceForm(service_api_client.find_all_service_names_lower) heading = 'Which service do you want to set up notifications for?' if form.validate_on_submit(): session['service_name'] = form.name.data diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 159744fc1..729cfee15 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -39,7 +39,7 @@ def service_settings(service_id): def service_name_change(service_id): service = service_api_client.get_service(service_id)['data'] - form = ServiceNameForm(service_api_client.find_all_service_names) + form = ServiceNameForm(service_api_client.find_all_service_names_lower) if form.validate_on_submit(): session['service_name_change'] = form.name.data diff --git a/app/notify_client/api_client.py b/app/notify_client/api_client.py index 93a76c20c..8b5200976 100644 --- a/app/notify_client/api_client.py +++ b/app/notify_client/api_client.py @@ -142,7 +142,7 @@ class ServiceAPIClient(NotificationsAPIClient): endpoint = "/service/{0}/template/{1}".format(service_id, template_id) return self.delete(endpoint) - def find_all_service_names(self, user_id=None): + def find_all_service_names_lower(self, user_id=None): resp = self.get_services(user_id) return [x['name'].lower() for x in resp['data']] diff --git a/tests/__init__.py b/tests/__init__.py index bc0513c6e..27cfd2a84 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -10,8 +10,9 @@ class TestClient(FlaskClient): with self.session_transaction() as session: session['user_id'] = user.id session['_fresh'] = True - if mocker and service: + if mocker: mocker.patch('app.user_api_client.get_user', return_value=user) + if mocker and service: mocker.patch('app.service_api_client.get_service', return_value={'data': service}) login_user(user, remember=True) diff --git a/tests/app/main/views/test_add_service.py b/tests/app/main/views/test_add_service.py index 41c444ff4..70c91a5f5 100644 --- a/tests/app/main/views/test_add_service.py +++ b/tests/app/main/views/test_add_service.py @@ -3,62 +3,57 @@ from flask import url_for def test_get_should_render_add_service_template(app_, api_user_active, - mock_login, - mock_get_service, - mock_get_services, - mock_get_user_by_email): + mocker): with app_.test_request_context(): with app_.test_client() as client: - client.login(api_user_active) + client.login(api_user_active, mocker) response = client.get(url_for('main.add_service')) assert response.status_code == 200 assert 'Which service do you want to set up notifications for?' in response.get_data(as_text=True) def test_should_add_service_and_redirect_to_next_page(app_, - mock_login, + mocker, mock_create_service, mock_get_services, api_user_active): with app_.test_request_context(): with app_.test_client() as client: - client.login(api_user_active) + client.login(api_user_active, mocker) response = client.post( url_for('main.add_service'), data={'name': 'testing the post'}) assert response.status_code == 302 assert response.location == url_for('main.service_dashboard', service_id=101, _external=True) + assert mock_get_services.called mock_create_service.asset_called_once_with('testing the post', False, app_.config['DEFAULT_SERVICE_LIMIT'], True, api_user_active.id) def test_should_return_form_errors_when_service_name_is_empty(app_, - api_user_active, - mock_get_service, - mock_get_services, - mock_get_user, - mock_get_user_by_email, - mock_login): + mocker, + api_user_active): with app_.test_request_context(): with app_.test_client() as client: - client.login(api_user_active) + client.login(api_user_active, mocker) response = client.post(url_for('main.add_service'), data={}) assert response.status_code == 200 assert 'Service name can’t be empty' in response.get_data(as_text=True) -def test_should_return_form_errors_with_duplicate_service_name(app_, - mock_login, - mock_get_services, - mock_get_user, - api_user_active, - mock_get_user_by_email): +def test_should_return_form_errors_with_duplicate_service_name_regardless_of_case(app_, + mocker, + service_one, + mock_get_services, + api_user_active, + mock_create_service): with app_.test_request_context(): with app_.test_client() as client: - client.login(api_user_active) - response = client.post( - url_for('main.add_service'), data={'name': 'service_one'}) + client.login(api_user_active, mocker, service_one) + response = client.post(url_for('main.add_service'), data={'name': 'SERVICE_TWO'}) + assert response.status_code == 200 assert 'This service name is already in use' in response.get_data(as_text=True) assert mock_get_services.called + assert not mock_create_service.called diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index ef3a16ac0..da6d255d6 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -66,7 +66,7 @@ def test_should_not_allow_duplicate_names(app_, service_id = service_one['id'] response = client.post( url_for('main.service_name_change', service_id=service_id), - data={'name': "service_one"}) + data={'name': "SErvICE_TWO"}) assert response.status_code == 200 resp_data = response.get_data(as_text=True) From 9a2cb60f5e160064d20968b7b83ab96e0682311a Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 31 Mar 2016 15:17:05 +0100 Subject: [PATCH 3/3] Update the service name validation in the ServiceNameForm and AddServiceForm to check the email_safe version of the name against a list of all service email_from fields. Update find_all_service_names to find_all_service_email_from, which returns the email_from of all services. --- app/main/forms.py | 21 +++++++++---------- app/main/views/add_service.py | 12 ++++++++--- app/main/views/service_settings.py | 2 +- app/notify_client/api_client.py | 9 ++++---- tests/app/main/test_add_service_form.py | 2 +- tests/app/main/views/test_add_service.py | 18 ++++++++++------ tests/app/main/views/test_service_settings.py | 8 ++++--- tests/app/test_utils.py | 8 +++++++ tests/conftest.py | 4 ++-- 9 files changed, 53 insertions(+), 31 deletions(-) create mode 100644 tests/app/test_utils.py diff --git a/app/main/forms.py b/app/main/forms.py index b711231e4..9153a9427 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1,13 +1,14 @@ -import re from flask_wtf import Form - +from utils.recipients import ( + validate_phone_number, + InvalidPhoneError +) from wtforms import ( StringField, PasswordField, ValidationError, TextAreaField, FileField, - RadioField, BooleanField, HiddenField ) @@ -16,12 +17,6 @@ from wtforms.validators import (DataRequired, Email, Length, Regexp) from app.main.validators import (Blacklist, CsvFileValidator, ValidEmailDomainRegex) -from utils.recipients import ( - validate_phone_number, - format_phone_number, - InvalidPhoneError -) - def email_address(label='Email address'): return EmailField(label, validators=[ @@ -159,7 +154,9 @@ class AddServiceForm(Form): ) def validate_name(self, a): - if a.data.lower() in self._names_func(): + from app.utils import email_safe + # make sure the email_from will be unique to all services + if email_safe(a.data) in self._names_func(): raise ValidationError('This service name is already in use') @@ -180,7 +177,9 @@ class ServiceNameForm(Form): ]) def validate_name(self, a): - if a.data.lower() in self._names_func(): + from app.utils import email_safe + # make sure the email_from will be unique to all services + if email_safe(a.data) in self._names_func(): raise ValidationError('This service name is already in use') diff --git a/app/main/views/add_service.py b/app/main/views/add_service.py index d943ddb75..db9628c78 100644 --- a/app/main/views/add_service.py +++ b/app/main/views/add_service.py @@ -16,6 +16,7 @@ from app import ( user_api_client, service_api_client ) +from app.utils import email_safe @main.route("/add-service", methods=['GET', 'POST']) @@ -31,12 +32,17 @@ def add_service(): invite_api_client.accept_invite(service_id, invitation.id) return redirect(url_for('main.service_dashboard', service_id=service_id)) - form = AddServiceForm(service_api_client.find_all_service_names_lower) + form = AddServiceForm(service_api_client.find_all_service_email_from) heading = 'Which service do you want to set up notifications for?' if form.validate_on_submit(): session['service_name'] = form.name.data - service_id = service_api_client.create_service( - session['service_name'], False, current_app.config['DEFAULT_SERVICE_LIMIT'], True, session['user_id']) + email_from = email_safe(session['service_name']) + service_id = service_api_client.create_service(service_name=session['service_name'], + active=False, + limit=current_app.config['DEFAULT_SERVICE_LIMIT'], + restricted=True, + user_id=session['user_id'], + email_from=email_from) return redirect(url_for('main.service_dashboard', service_id=service_id)) else: diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 729cfee15..b02dafacb 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -39,7 +39,7 @@ def service_settings(service_id): def service_name_change(service_id): service = service_api_client.get_service(service_id)['data'] - form = ServiceNameForm(service_api_client.find_all_service_names_lower) + form = ServiceNameForm(service_api_client.find_all_service_email_from) if form.validate_on_submit(): session['service_name_change'] = form.name.data diff --git a/app/notify_client/api_client.py b/app/notify_client/api_client.py index 8b5200976..aa3e1aa4d 100644 --- a/app/notify_client/api_client.py +++ b/app/notify_client/api_client.py @@ -18,7 +18,7 @@ class ServiceAPIClient(NotificationsAPIClient): self.client_id = application.config['ADMIN_CLIENT_USER_NAME'] self.secret = application.config['ADMIN_CLIENT_SECRET'] - def create_service(self, service_name, active, limit, restricted, user_id): + def create_service(self, service_name, active, limit, restricted, user_id, email_from): """ Create a service and return the json. """ @@ -27,7 +27,8 @@ class ServiceAPIClient(NotificationsAPIClient): "active": active, "limit": limit, "user_id": user_id, - "restricted": restricted + "restricted": restricted, + "email_from": email_from } return self.post("/service", data)['data']['id'] @@ -142,9 +143,9 @@ class ServiceAPIClient(NotificationsAPIClient): endpoint = "/service/{0}/template/{1}".format(service_id, template_id) return self.delete(endpoint) - def find_all_service_names_lower(self, user_id=None): + def find_all_service_email_from(self, user_id=None): resp = self.get_services(user_id) - return [x['name'].lower() for x in resp['data']] + return [x['email_from'] for x in resp['data']] class ServicesBrowsableItem(BrowsableItem): diff --git a/tests/app/main/test_add_service_form.py b/tests/app/main/test_add_service_form.py index ca021b741..8f2ce9bf8 100644 --- a/tests/app/main/test_add_service_form.py +++ b/tests/app/main/test_add_service_form.py @@ -4,7 +4,7 @@ from werkzeug.datastructures import MultiDict def test_form_should_have_errors_when_duplicate_service_is_added(app_): def _get_form_names(): - return ['some service', 'more names'] + return ['some.service', 'more.names'] with app_.test_request_context(): form = AddServiceForm(_get_form_names, formdata=MultiDict([('name', 'some service')])) diff --git a/tests/app/main/views/test_add_service.py b/tests/app/main/views/test_add_service.py index 70c91a5f5..1da5c1ab5 100644 --- a/tests/app/main/views/test_add_service.py +++ b/tests/app/main/views/test_add_service.py @@ -1,5 +1,7 @@ from flask import url_for +import app + def test_get_should_render_add_service_template(app_, api_user_active, @@ -26,9 +28,12 @@ def test_should_add_service_and_redirect_to_next_page(app_, assert response.status_code == 302 assert response.location == url_for('main.service_dashboard', service_id=101, _external=True) assert mock_get_services.called - mock_create_service.asset_called_once_with('testing the post', False, - app_.config['DEFAULT_SERVICE_LIMIT'], - True, api_user_active.id) + mock_create_service.asset_called_once_with(service_name='testing the post', + active=False, + limit=app_.config['DEFAULT_SERVICE_LIMIT'], + restricted=True, + user_id=api_user_active.id, + email_from='testing.the.post') def test_should_return_form_errors_when_service_name_is_empty(app_, @@ -45,15 +50,16 @@ def test_should_return_form_errors_when_service_name_is_empty(app_, def test_should_return_form_errors_with_duplicate_service_name_regardless_of_case(app_, mocker, service_one, - mock_get_services, api_user_active, mock_create_service): with app_.test_request_context(): with app_.test_client() as client: client.login(api_user_active, mocker, service_one) - response = client.post(url_for('main.add_service'), data={'name': 'SERVICE_TWO'}) + mocker.patch('app.service_api_client.find_all_service_email_from', + return_value=['service_one', 'service.two']) + response = client.post(url_for('main.add_service'), data={'name': 'SERVICE TWO'}) assert response.status_code == 200 assert 'This service name is already in use' in response.get_data(as_text=True) - assert mock_get_services.called + app.service_api_client.find_all_service_email_from.assert_called_once_with() assert not mock_create_service.called diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index da6d255d6..2a5bd69b2 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -58,19 +58,21 @@ def test_should_redirect_after_change_service_name(app_, def test_should_not_allow_duplicate_names(app_, active_user_with_permissions, mocker, - service_one, - mock_get_services): + service_one): with app_.test_request_context(): with app_.test_client() as client: client.login(active_user_with_permissions, mocker, service_one) + mocker.patch('app.service_api_client.find_all_service_email_from', + return_value=['service_one', 'service.two']) service_id = service_one['id'] response = client.post( url_for('main.service_name_change', service_id=service_id), - data={'name': "SErvICE_TWO"}) + data={'name': "SErvICE TWO"}) assert response.status_code == 200 resp_data = response.get_data(as_text=True) assert 'This service name is already in use' in resp_data + app.service_api_client.find_all_service_email_from.assert_called_once_with() def test_should_show_service_name_confirmation(app_, diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py new file mode 100644 index 000000000..edb400a33 --- /dev/null +++ b/tests/app/test_utils.py @@ -0,0 +1,8 @@ +from app.utils import email_safe + + +def test_email_safe_return_dot_separated_email_domain(): + test_name = 'SOME service with+stuff+ b123' + expected = 'some.service.withstuff.b123' + actual = email_safe(test_name) + assert actual == expected diff --git a/tests/conftest.py b/tests/conftest.py index 7fc1acb77..7c41b17ee 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -60,10 +60,10 @@ def mock_get_service(mocker, api_user_active): @pytest.fixture(scope='function') def mock_create_service(mocker): - def _create(service_name, active, limit, restricted, user_id): + def _create(service_name, active, limit, restricted, user_id, email_from): service = service_json( 101, service_name, [user_id], limit=limit, - active=active, restricted=restricted) + active=active, restricted=restricted, email_from=email_from) return service['id'] return mocker.patch(