From 186202cc9d9fd694ea9f425a803c10aa6928ddb4 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 7 Aug 2017 11:30:25 +0100 Subject: [PATCH 1/5] [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): From 57808b1d846e6b7031dbda47dcb2f657c564ded4 Mon Sep 17 00:00:00 2001 From: Pete Herlihy Date: Fri, 4 Aug 2017 14:31:08 +0100 Subject: [PATCH 2/5] Updated the video to the new one --- app/templates/views/signedout.html | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/templates/views/signedout.html b/app/templates/views/signedout.html index d5c3fc071..4ea1bad59 100644 --- a/app/templates/views/signedout.html +++ b/app/templates/views/signedout.html @@ -107,7 +107,7 @@
- +
From 7e8c76bbc519d8a041595c86197133ef94b50e72 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 8 Aug 2017 11:13:08 +0100 Subject: [PATCH 3/5] Let the API error when the service name is a duplicate. - Remove the name_func from the AddServiceForm - Catch the HTTPError and set the error on the form --- app/main/forms.py | 23 ----------- app/main/views/add_service.py | 52 +++++++++++++++--------- tests/app/main/test_add_service_form.py | 11 ----- tests/app/main/views/test_add_service.py | 24 +++-------- 4 files changed, 37 insertions(+), 73 deletions(-) delete mode 100644 tests/app/main/test_add_service_form.py diff --git a/app/main/forms.py b/app/main/forms.py index fafe580da..5f6560716 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -4,7 +4,6 @@ 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 @@ -223,28 +222,6 @@ 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 - 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): def __init__(self, names_func, *args, **kwargs): diff --git a/app/main/views/add_service.py b/app/main/views/add_service.py index fde726af0..49886ed4e 100644 --- a/app/main/views/add_service.py +++ b/app/main/views/add_service.py @@ -1,4 +1,3 @@ - from flask import ( render_template, redirect, @@ -11,6 +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,30 @@ 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 + + +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']) @@ -64,20 +80,16 @@ def add_service(): 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) - service_id = form.service_id + email_from = email_safe(form.name.data) + service_name = form.name.data + + 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 ca5fcaa10..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 -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,22 +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_duplicate_service, ): - response = logged_in_client.post(url_for('main.add_service'), data={'name': 'SERVICE TWO'}) - print(response.status_code) - assert response.status_code == 400 - assert response.message == "Duplicate service 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) - assert mock_create_duplicate_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, ): @@ -130,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, ): From dbbd70d0ef7f2361c2817a245d253398517d9a8c Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 8 Aug 2017 14:29:29 +0100 Subject: [PATCH 4/5] Revert change to package-lock.json --- package-lock.json | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/package-lock.json b/package-lock.json index 6544fc159..8abfbd0d6 100644 --- a/package-lock.json +++ b/package-lock.json @@ -97,6 +97,12 @@ } } }, + "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", @@ -3760,6 +3766,12 @@ "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", From 98595d8d80b9bf6274a854cf6618577a850af0a7 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 8 Aug 2017 15:04:45 +0100 Subject: [PATCH 5/5] Added an else: raise --- app/main/views/add_service.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/main/views/add_service.py b/app/main/views/add_service.py index 49886ed4e..fc7f43a22 100644 --- a/app/main/views/add_service.py +++ b/app/main/views/add_service.py @@ -52,6 +52,8 @@ def _create_service(service_name, email_from, form): 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):