From 6ea8491b39cb7f8564d4611ada2f1c7de2293e13 Mon Sep 17 00:00:00 2001 From: Nicholas Staples Date: Thu, 10 Mar 2016 14:29:31 +0000 Subject: [PATCH] Service name uniqueness handled in all cases and tests passing. --- app/main/forms.py | 19 ++++++- app/main/views/service_settings.py | 23 +++++--- tests/app/main/views/test_service_settings.py | 52 ++++++++++++++++++- tests/conftest.py | 21 ++++++++ 4 files changed, 106 insertions(+), 9 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 474f90209..1dbf8594f 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -205,7 +205,24 @@ class AddServiceForm(Form): class ServiceNameForm(Form): - name = StringField(u'New name') + def __init__(self, names_func, *args, **kwargs): + """ + Keyword arguments: + names_func -- Returns a list of unique service_names already registered + on the system. + """ + self._names_func = names_func + super(ServiceNameForm, self).__init__(*args, **kwargs) + + name = StringField( + u'New name', + validators=[ + DataRequired(message='Service name can’t be empty') + ]) + + def validate_name(self, a): + if a.data in self._names_func(): + raise ValidationError('This service name is already in use') class ConfirmPasswordForm(Form): diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index a57696375..89718a9f9 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -18,7 +18,8 @@ from notifications_python_client.errors import HTTPError from app.main.dao.services_dao import ( get_service_by_id, delete_service, - update_service + update_service, + find_all_service_names ) from app.main import main @@ -57,7 +58,7 @@ def service_name_change(service_id): else: raise e - form = ServiceNameForm() + form = ServiceNameForm(find_all_service_names) if form.validate_on_submit(): session['service_name_change'] = form.name.data @@ -89,10 +90,20 @@ def service_name_change_confirm(service_id): if form.validate_on_submit(): service['name'] = session['service_name_change'] - update_service(service) - session['service_name'] = service['name'] - session.pop('service_name_change') - return redirect(url_for('.service_settings', service_id=service_id)) + try: + update_service(service) + 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']: + # Redirect the user back to the change service name screen + flash('This service name is already in use', 'error') + return redirect(url_for('main.service_name_change', service_id=service_id)) + else: + raise e + else: + session['service_name'] = service['name'] + session.pop('service_name_change') + return redirect(url_for('.service_settings', service_id=service_id)) return render_template( 'views/service-settings/confirm.html', heading='Change your service name', diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index f0210dd7e..65f7030e7 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -54,8 +54,9 @@ def test_should_redirect_after_change_service_name(app_, with app_.test_client() as client: client.login(api_user_active) service_id = 123 - response = client.post(url_for( - 'main.service_name_change', service_id=service_id)) + response = client.post( + url_for('main.service_name_change', service_id=service_id), + data={'name': "new name"}) assert response.status_code == 302 settings_url = url_for( @@ -64,6 +65,27 @@ def test_should_redirect_after_change_service_name(app_, assert mock_get_service.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, + mock_get_services): + with app_.test_request_context(): + with app_.test_client() as client: + client.login(api_user_active) + service_id = 123 + response = client.post( + url_for('main.service_name_change', service_id=service_id), + data={'name': "service_one"}) + + assert response.status_code == 200 + resp_data = response.get_data(as_text=True) + assert 'This service name is already in use' in resp_data + + def test_should_show_service_name_confirmation(app_, api_user_active, mock_get_service, @@ -112,6 +134,32 @@ def test_should_redirect_after_service_name_confirmation(app_, assert mock_update_service.called +def test_should_raise_duplicate_name_handled(app_, + api_user_active, + mock_get_service, + mock_update_service_raise_httperror_duplicate_name, + mock_get_user, + mock_get_user_by_email, + mock_login, + mock_verify_password, + mock_has_permissions): + with app_.test_request_context(): + with app_.test_client() as client: + client.login(api_user_active) + service_id = 123 + 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)) + + 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) + assert name_change_url == response.location + + def test_should_show_request_to_go_live(app_, api_user_active, mock_get_service, diff --git a/tests/conftest.py b/tests/conftest.py index 93dce2265..be599df6b 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,5 +1,6 @@ import uuid from datetime import date, datetime, timedelta +from unittest.mock import Mock import pytest from app import create_app @@ -18,6 +19,8 @@ from app.notify_client.models import ( InvitedUser ) +from notifications_python_client.errors import HTTPError + @pytest.fixture(scope='session') def app_(request): @@ -90,6 +93,24 @@ def mock_update_service(mocker): 'app.notifications_api_client.update_service', side_effect=_update) +@pytest.fixture(scope='function') +def mock_update_service_raise_httperror_duplicate_name(mocker): + + def _update(service_id, + service_name, + active, + limit, + restricted, + users): + 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") + raise http_error + + return mocker.patch( + 'app.notifications_api_client.update_service', side_effect=_update) + + SERVICE_ONE_ID = "596364a0-858e-42c8-9062-a8fe822260eb" SERVICE_TWO_ID = "147ad62a-2951-4fa1-9ca0-093cd1a52c52"