Merge pull request #1416 from alphagov/check-service-is-unique

Check service name is unique - efficiently

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):

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():

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))
 

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):

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(

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):