From 677237ba470995a66a1ca03d421e3fe8ea065929 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 30 Mar 2016 17:12:00 +0100 Subject: [PATCH] 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")