From 186202cc9d9fd694ea9f425a803c10aa6928ddb4 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 7 Aug 2017 11:30:25 +0100 Subject: [PATCH] [WIP] Let the API return a 400 error message if the service name is a duplicate. --- app/main/forms.py | 32 ++++++++++++++++-------- app/main/views/add_service.py | 14 +++++------ package-lock.json | 12 --------- tests/app/main/views/test_add_service.py | 14 +++++------ tests/conftest.py | 12 +++++++++ 5 files changed, 47 insertions(+), 37 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 0b92466dd..fafe580da 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -3,6 +3,8 @@ import pytz from flask_wtf import FlaskForm as Form from datetime import datetime, timedelta + +from notifications_python_client.errors import HTTPError from notifications_utils.recipients import ( validate_phone_number, InvalidPhoneError @@ -211,13 +213,7 @@ class TextNotReceivedForm(Form): class AddServiceForm(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(AddServiceForm, self).__init__(*args, **kwargs) name = StringField( @@ -227,11 +223,27 @@ class AddServiceForm(Form): ] ) + service_id = HiddenField('service_id') + + def _create_service(self, service_name, email_from): + from app import service_api_client + from flask import current_app, session + service_id = service_api_client.create_service(service_name=service_name, + message_limit=current_app.config['DEFAULT_SERVICE_LIMIT'], + restricted=True, + user_id=session['user_id'], + email_from=email_from) + session['service_id'] = service_id + return service_id + 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') + email_from = email_safe(a.data) + try: + self.service_id = self._create_service(a.data, email_from) + except HTTPError as e: + if e.status_code == 400 and e.message['name']: + raise ValidationError(message=e.message['name'][0]) class ServiceNameForm(Form): diff --git a/app/main/views/add_service.py b/app/main/views/add_service.py index 115ca77c5..fde726af0 100644 --- a/app/main/views/add_service.py +++ b/app/main/views/add_service.py @@ -1,3 +1,4 @@ + from flask import ( render_template, redirect, @@ -10,7 +11,6 @@ from flask_login import ( current_user, login_required ) - from werkzeug.exceptions import abort from app.main import main @@ -60,15 +60,15 @@ def add_service(): if not is_gov_user(current_user.email_address): abort(403) - form = AddServiceForm(service_api_client.find_all_service_email_from) + form = AddServiceForm() heading = 'Which service do you want to set up notifications for?' if form.validate_on_submit(): - email_from = email_safe(form.name.data) - service_name = form.name.data - service_id = _create_service(service_name, email_from) - - if (len(service_api_client.get_active_services({'user_id': session['user_id']}).get('data', [])) > 1): + # email_from = email_safe(form.name.data) + # service_name = form.name.data + # service_id = _create_service(service_name, email_from) + service_id = form.service_id + if len(service_api_client.get_active_services({'user_id': session['user_id']}).get('data', [])) > 1: return redirect(url_for('main.service_dashboard', service_id=service_id)) example_sms_template = service_api_client.create_service_template( diff --git a/package-lock.json b/package-lock.json index 8abfbd0d6..6544fc159 100644 --- a/package-lock.json +++ b/package-lock.json @@ -97,12 +97,6 @@ } } }, - "argparse": { - "version": "1.0.9", - "resolved": "https://registry.npmjs.org/argparse/-/argparse-1.0.9.tgz", - "integrity": "sha1-c9g7wmP4bpf4zE9rrhsOkKfSLIY=", - "dev": true - }, "arr-diff": { "version": "2.0.0", "resolved": "https://registry.npmjs.org/arr-diff/-/arr-diff-2.0.0.tgz", @@ -3766,12 +3760,6 @@ "resolved": "https://registry.npmjs.org/split/-/split-0.2.10.tgz", "integrity": "sha1-Zwl8YB1pfOE2j0GPBs0gHPBSGlc=" }, - "sprintf-js": { - "version": "1.0.3", - "resolved": "https://registry.npmjs.org/sprintf-js/-/sprintf-js-1.0.3.tgz", - "integrity": "sha1-BOaSb2YolTVPPdAVIDYzuFcpfiw=", - "dev": true - }, "sshpk": { "version": "1.13.1", "resolved": "https://registry.npmjs.org/sshpk/-/sshpk-1.13.1.tgz", diff --git a/tests/app/main/views/test_add_service.py b/tests/app/main/views/test_add_service.py index 6a178cc2b..ca5fcaa10 100644 --- a/tests/app/main/views/test_add_service.py +++ b/tests/app/main/views/test_add_service.py @@ -1,5 +1,5 @@ from flask import url_for, session -from unittest.mock import ANY + import app from app.utils import is_gov_user @@ -107,16 +107,14 @@ def test_should_return_form_errors_with_duplicate_service_name_regardless_of_cas mocker, service_one, api_user_active, - mock_create_service, + mock_create_duplicate_service, ): - mocker.patch('app.service_api_client.find_all_service_email_from', - return_value=['service_one', 'service.two']) response = logged_in_client.post(url_for('main.add_service'), data={'name': 'SERVICE TWO'}) - - assert response.status_code == 200 + print(response.status_code) + assert response.status_code == 400 + assert response.message == "Duplicate service name 'SERVICE_TWO'" assert 'This service name is already in use' in response.get_data(as_text=True) - app.service_api_client.find_all_service_email_from.assert_called_once_with() - assert not mock_create_service.called + assert mock_create_duplicate_service.called def test_non_whitelist_user_cannot_access_create_service_page( diff --git a/tests/conftest.py b/tests/conftest.py index 955440e97..45364f807 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -173,6 +173,18 @@ def mock_create_service(mocker): 'app.service_api_client.create_service', side_effect=_create) +@pytest.fixture(scope='function') +def mock_create_duplicate_service(mocker): + def _create(service_name, message_limit, restricted, user_id, 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") + raise http_error + + return mocker.patch( + 'app.service_api_client.create_service', side_effect=_create) + + @pytest.fixture(scope='function') def mock_update_service(mocker): def _update(service_id, **kwargs):