diff --git a/app/main/forms.py b/app/main/forms.py index bc334ab41..45ba28782 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -212,26 +212,8 @@ class TextNotReceivedForm(Form): mobile_number = mobile_number() -class AddServiceForm(Form): - def __init__(self, *args, **kwargs): - super(AddServiceForm, self).__init__(*args, **kwargs) - - name = StringField( - 'Service name', - validators=[ - DataRequired(message='Can’t be empty') - ] - ) - - class ServiceNameForm(Form): - 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 + def __init__(self, *args, **kwargs): super(ServiceNameForm, self).__init__(*args, **kwargs) name = StringField( @@ -240,12 +222,6 @@ class ServiceNameForm(Form): DataRequired(message='Can’t be empty') ]) - def validate_name(self, a): - 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') - class ConfirmPasswordForm(Form): def __init__(self, validate_password_func, *args, **kwargs): diff --git a/app/main/views/add_service.py b/app/main/views/add_service.py index fc7f43a22..9b7d5b9fd 100644 --- a/app/main/views/add_service.py +++ b/app/main/views/add_service.py @@ -14,7 +14,7 @@ from notifications_python_client.errors import HTTPError from werkzeug.exceptions import abort from app.main import main -from app.main.forms import AddServiceForm +from app.main.forms import ServiceNameForm from app.notify_client.models import InvitedUser from app import ( @@ -78,7 +78,7 @@ def add_service(): if not is_gov_user(current_user.email_address): abort(403) - form = AddServiceForm() + form = ServiceNameForm() heading = 'Which service do you want to set up notifications for?' if form.validate_on_submit(): diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 7e694cb06..cc5017c99 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -81,12 +81,16 @@ def service_settings(service_id): @login_required @user_has_permissions('manage_settings', admin_override=True) def service_name_change(service_id): - form = ServiceNameForm(service_api_client.find_all_service_email_from) + form = ServiceNameForm() if request.method == 'GET': form.name.data = current_service.get('name') if form.validate_on_submit(): + unique_name = service_api_client.is_service_name_unique(form.name.data, email_safe(form.name.data)) + if not unique_name: + form.name.errors.append("This service name is already in use") + return render_template('views/service-settings/name.html', form=form) session['service_name_change'] = form.name.data return redirect(url_for('.service_name_change_confirm', service_id=service_id)) diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index b72fe77c9..2173e3402 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -216,9 +216,13 @@ class ServiceAPIClient(NotifyAdminAPIClient): data = _attach_current_user(data) return self.post(endpoint, data=data) - def find_all_service_email_from(self, user_id=None): - resp = self.get_services(user_id) - return [x['email_from'] for x in resp['data']] + def is_service_name_unique(self, name, email_from): + """ + Check that the service name or email from are unique across all services. + """ + endpoint = "/service/unique" + params = {"name": name, "email_from": email_from} + return self.get(url=endpoint, params=params)["result"] # Temp access of service history data. Includes service and api key history def get_service_history(self, service_id): diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 606d4c2c8..8e3281ddf 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -227,7 +227,7 @@ def test_should_redirect_after_change_service_name( logged_in_client, service_one, mock_update_service, - mock_get_services, + mock_service_name_is_unique ): response = logged_in_client.post( url_for('main.service_name_change', service_id=service_one['id']), @@ -237,7 +237,7 @@ def test_should_redirect_after_change_service_name( settings_url = url_for( 'main.service_name_change_confirm', service_id=service_one['id'], _external=True) assert settings_url == response.location - assert mock_get_services.called + assert mock_service_name_is_unique.called def test_show_restricted_service( @@ -302,11 +302,9 @@ def test_switch_service_to_restricted( def test_should_not_allow_duplicate_names( logged_in_client, - mocker, + mock_service_name_is_not_unique, 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 = logged_in_client.post( url_for('main.service_name_change', service_id=service_id), @@ -315,7 +313,7 @@ def test_should_not_allow_duplicate_names( 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() + app.service_api_client.is_service_name_unique.assert_called_once_with('SErvICE TWO', 'service.two') def test_should_show_service_name_confirmation( diff --git a/tests/conftest.py b/tests/conftest.py index 45364f807..255f3a62c 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -150,6 +150,16 @@ def mock_get_detailed_services(mocker, fake_uuid): return mocker.patch('app.service_api_client.get_services', return_value=services) +@pytest.fixture(scope='function') +def mock_service_name_is_not_unique(mocker): + return mocker.patch('app.service_api_client.is_service_name_unique', return_value=False) + + +@pytest.fixture(scope='function') +def mock_service_name_is_unique(mocker): + return mocker.patch('app.service_api_client.is_service_name_unique', return_value=True) + + @pytest.fixture(scope='function') def mock_get_live_service(mocker, api_user_active): def _get(service_id):