diff --git a/app/main/forms.py b/app/main/forms.py index 0b92466dd..5f6560716 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -3,6 +3,7 @@ import pytz from flask_wtf import FlaskForm as Form from datetime import datetime, timedelta + from notifications_utils.recipients import ( validate_phone_number, InvalidPhoneError @@ -211,13 +212,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,12 +222,6 @@ class AddServiceForm(Form): ] ) - 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 ServiceNameForm(Form): def __init__(self, names_func, *args, **kwargs): diff --git a/app/main/views/add_service.py b/app/main/views/add_service.py index 115ca77c5..fc7f43a22 100644 --- a/app/main/views/add_service.py +++ b/app/main/views/add_service.py @@ -10,7 +10,7 @@ from flask_login import ( current_user, login_required ) - +from notifications_python_client.errors import HTTPError from werkzeug.exceptions import abort from app.main import main @@ -39,14 +39,32 @@ def _add_invited_user_to_service(invited_user): return service_id -def _create_service(service_name, email_from): - 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 _create_service(service_name, email_from, form): + try: + 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, None + except HTTPError as e: + if e.status_code == 400 and e.message['name']: + form.name.errors.append("This service name is already in use") + return None, e + else: + raise e + + +def _create_example_template(service_id): + example_sms_template = service_api_client.create_service_template( + 'Example text message template', + 'sms', + 'Hey ((name)), I’m trying out Notify. Today is ((day of week)) and my favourite colour is ((colour)).', + service_id, + process_type='priority', + ) + return example_sms_template @main.route("/add-service", methods=['GET', 'POST']) @@ -60,24 +78,20 @@ 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): + service_id, error = _create_service(service_name, email_from, form) + if error: + return render_template('views/add-service.html', form=form, heading=heading) + 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( - 'Example text message template', - 'sms', - 'Hey ((name)), I’m trying out Notify. Today is ((day of week)) and my favourite colour is ((colour)).', - service_id, - process_type='priority', - ) + example_sms_template = _create_example_template(service_id) return redirect(url_for( 'main.start_tour', diff --git a/tests/app/main/test_add_service_form.py b/tests/app/main/test_add_service_form.py deleted file mode 100644 index e8f5a5f74..000000000 --- a/tests/app/main/test_add_service_form.py +++ /dev/null @@ -1,11 +0,0 @@ -from app.main.forms import AddServiceForm -from werkzeug.datastructures import MultiDict - - -def test_form_should_have_errors_when_duplicate_service_is_added(client): - def _get_form_names(): - return ['some.service', 'more.names'] - form = AddServiceForm(_get_form_names, - formdata=MultiDict([('name', 'some service')])) - form.validate() - assert {'name': ['This service name is already in use']} == form.errors diff --git a/tests/app/main/views/test_add_service.py b/tests/app/main/views/test_add_service.py index 6a178cc2b..703e8ab7a 100644 --- a/tests/app/main/views/test_add_service.py +++ b/tests/app/main/views/test_add_service.py @@ -1,6 +1,5 @@ from flask import url_for, session -from unittest.mock import ANY -import app + from app.utils import is_gov_user @@ -17,9 +16,7 @@ def test_non_gov_user_cannot_see_add_service_button( def test_get_should_render_add_service_template( - logged_in_client, - api_user_active, - mocker, + logged_in_client ): response = logged_in_client.get(url_for('main.add_service')) assert response.status_code == 200 @@ -29,7 +26,6 @@ def test_get_should_render_add_service_template( def test_should_add_service_and_redirect_to_tour_when_no_services( app_, logged_in_client, - mocker, mock_create_service, mock_create_service_template, mock_get_services_with_no_services, @@ -69,7 +65,6 @@ def test_should_add_service_and_redirect_to_tour_when_no_services( def test_should_add_service_and_redirect_to_dashboard_when_existing_service( app_, logged_in_client, - mocker, mock_create_service, mock_create_service_template, mock_get_services, @@ -93,9 +88,7 @@ def test_should_add_service_and_redirect_to_dashboard_when_existing_service( def test_should_return_form_errors_when_service_name_is_empty( - logged_in_client, - mocker, - api_user_active, + logged_in_client ): response = logged_in_client.post(url_for('main.add_service'), data={}) assert response.status_code == 200 @@ -104,24 +97,16 @@ def test_should_return_form_errors_when_service_name_is_empty( def test_should_return_form_errors_with_duplicate_service_name_regardless_of_case( logged_in_client, - 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'}) + response = logged_in_client.post(url_for('main.add_service'), data={'name': 'SERVICE ONE'}) assert response.status_code == 200 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 def test_non_whitelist_user_cannot_access_create_service_page( logged_in_client, - mock_login, mock_get_non_govuser, api_nongov_user_active, ): @@ -132,7 +117,6 @@ def test_non_whitelist_user_cannot_access_create_service_page( def test_non_whitelist_user_cannot_create_service( logged_in_client, - mock_login, mock_get_non_govuser, api_nongov_user_active, ): 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):