From 52a5da4d175ed5739e21fd8283b094e45faccb77 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Thu, 11 Feb 2021 12:02:42 +0000 Subject: [PATCH 1/3] Handle exception is org name already exists Previously this would return a 500 error, as the 400 exception was not handled from the API [1]. Note that: - We tend to rely on exception messages to identify the error that occurred [2][3], with services being a notable deviation [4]. I've used the exception message approach, as this is more granular and broadly consistent with the rest of the app. - There is already code to cover this scenario when a user changes the name of an existing organisation or service, but the mechanism is different [5][6]. It makes sense to just get any error from the call to try and create the organisation. - The API mock is based on one for services [7], but I've chosen to have it inline with the test, since we're unlikely to reuse it, and it's clearer to have the test setup as part of the test. [1]: https://github.com/alphagov/notifications-api/blob/8f99da525dad3bf653a4e1f9e4a7b7b689219d78/app/organisation/rest.py#L34-L47 [2]: https://github.com/alphagov/notifications-admin/blob/70b606a2d4cb03f2bd9f3a5742a40970a928e696/app/main/views/manage_users.py#L166 [3]: https://github.com/alphagov/notifications-admin/blob/70b606a2d4cb03f2bd9f3a5742a40970a928e696/app/main/views/templates.py#L499 [4]: https://github.com/alphagov/notifications-admin/blob/70b606a2d4cb03f2bd9f3a5742a40970a928e696/app/main/views/add_service.py#L30 [5]: https://github.com/alphagov/notifications-admin/blob/70b606a2d4cb03f2bd9f3a5742a40970a928e696/app/main/views/service_settings.py#L102-L104 [6]: https://github.com/alphagov/notifications-admin/blob/70b606a2d4cb03f2bd9f3a5742a40970a928e696/app/main/views/organisations.py#L264-L266 [7]: https://github.com/alphagov/notifications-admin/blob/0abc143147c98ac50c059729eb5c1a4755d607fa/tests/conftest.py#L590-L606 --- app/main/views/organisations.py | 15 ++++++--- .../views/organisations/test_organisations.py | 31 +++++++++++++++++++ 2 files changed, 42 insertions(+), 4 deletions(-) diff --git a/app/main/views/organisations.py b/app/main/views/organisations.py index 52978bd6d..50c679d50 100644 --- a/app/main/views/organisations.py +++ b/app/main/views/organisations.py @@ -62,10 +62,17 @@ def add_organisation(): form = NewOrganisationForm() if form.validate_on_submit(): - return redirect(url_for( - '.organisation_settings', - org_id=Organisation.create_from_form(form).id, - )) + try: + return redirect(url_for( + '.organisation_settings', + org_id=Organisation.create_from_form(form).id, + )) + except HTTPError as e: + msg = 'Organisation name already exists' + if e.status_code == 400 and msg in e.message: + form.name.errors.append("This organisation name is already in use") + else: + raise e return render_template( 'views/organisations/add-organisation.html', diff --git a/tests/app/main/views/organisations/test_organisations.py b/tests/app/main/views/organisations/test_organisations.py index bb8f78b38..bfd7b6c7c 100644 --- a/tests/app/main/views/organisations/test_organisations.py +++ b/tests/app/main/views/organisations/test_organisations.py @@ -194,6 +194,37 @@ def test_create_new_organisation_fails_with_incorrect_input( assert error_message in page.select_one('.govuk-error-message').text +def test_create_new_organisation_fails_with_duplicate_name( + client_request, + platform_admin_user, + mocker, +): + def _create(**_kwargs): + json_mock = Mock(return_value={'message': 'Organisation name already exists'}) + resp_mock = Mock(status_code=400, json=json_mock) + http_error = HTTPError(response=resp_mock, message="Default message") + raise http_error + + mocker.patch( + 'app.organisations_client.create_organisation', + side_effect=_create + ) + + client_request.login(platform_admin_user) + page = client_request.post( + '.add_organisation', + _data={ + 'name': 'Existing org', + 'organisation_type': 'local', + 'crown_status': 'non-crown', + }, + _expected_status=200, + ) + + error_message = 'This organisation name is already in use' + assert error_message in page.select_one('.govuk-error-message').text + + @pytest.mark.parametrize('organisation_type, organisation, expected_status', ( ('nhs_gp', None, 200), ('central', None, 403), From 00cc67f8133f150cb6d5624c2eff6f1b29ca605c Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 15 Feb 2021 11:41:26 +0000 Subject: [PATCH 2/3] Inline duplicate service fixture with test Similarly to the previous commit, this fixture is only used once, so can benefit from being inline with its test. --- tests/app/main/views/test_add_service.py | 14 +++++++++++++- tests/conftest.py | 19 ------------------- 2 files changed, 13 insertions(+), 20 deletions(-) diff --git a/tests/app/main/views/test_add_service.py b/tests/app/main/views/test_add_service.py index 5c4e24e25..11ef3dc0a 100644 --- a/tests/app/main/views/test_add_service.py +++ b/tests/app/main/views/test_add_service.py @@ -1,5 +1,6 @@ import pytest from flask import session, url_for +from notifications_python_client.errors import HTTPError from app.utils import is_gov_user from tests import organisation_json @@ -309,9 +310,20 @@ def test_add_service_fails_if_service_name_fails_validation( def test_should_return_form_errors_with_duplicate_service_name_regardless_of_case( client_request, - mock_create_duplicate_service, mock_get_organisation_by_domain, + mocker, ): + def _create(**_kwargs): + json_mock = mocker.Mock(return_value={'message': {'name': ["Duplicate service name"]}}) + resp_mock = mocker.Mock(status_code=400, json=json_mock) + http_error = HTTPError(response=resp_mock, message="Default message") + raise http_error + + mocker.patch( + 'app.service_api_client.create_service', + side_effect=_create + ) + page = client_request.post( 'main.add_service', _data={ diff --git a/tests/conftest.py b/tests/conftest.py index 98e2a551e..ae7fb8ab3 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -587,25 +587,6 @@ 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, - organisation_type, - 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 5c2cdf6250ecb61c5935918bbd63e04edeb63c12 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 15 Feb 2021 13:54:52 +0000 Subject: [PATCH 3/3] Remove redundant import of Mock and ANY It's conventional to use the "mocker" fixture to access these. --- .../main/views/organisations/test_organisations.py | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/tests/app/main/views/organisations/test_organisations.py b/tests/app/main/views/organisations/test_organisations.py index bfd7b6c7c..32d35683f 100644 --- a/tests/app/main/views/organisations/test_organisations.py +++ b/tests/app/main/views/organisations/test_organisations.py @@ -1,5 +1,3 @@ -from unittest.mock import ANY, Mock - import pytest from bs4 import BeautifulSoup from flask import url_for @@ -102,7 +100,7 @@ def test_page_to_create_new_organisation( ('radio', 'organisation_type', 'other'), ('radio', 'crown_status', 'crown'), ('radio', 'crown_status', 'non-crown'), - ('hidden', 'csrf_token', ANY), + ('hidden', 'csrf_token', mocker.ANY), ] @@ -200,8 +198,8 @@ def test_create_new_organisation_fails_with_duplicate_name( mocker, ): def _create(**_kwargs): - json_mock = Mock(return_value={'message': 'Organisation name already exists'}) - resp_mock = Mock(status_code=400, json=json_mock) + json_mock = mocker.Mock(return_value={'message': 'Organisation name already exists'}) + resp_mock = mocker.Mock(status_code=400, json=json_mock) http_error = HTTPError(response=resp_mock, message="Default message") raise http_error @@ -1079,7 +1077,7 @@ def test_update_organisation_domains_when_domain_already_exists( client_request.login(user) mocker.patch('app.organisations_client.update_organisation', side_effect=HTTPError( - response=Mock( + response=mocker.Mock( status_code=400, json={'result': 'error', 'message': 'Domain already exists'} ), @@ -1229,7 +1227,7 @@ def test_confirm_update_organisation_with_name_already_in_use( mocker.patch( 'app.organisations_client.update_organisation_name', side_effect=HTTPError( - response=Mock( + response=mocker.Mock( status_code=400, json={'result': 'error', 'message': 'Organisation name already exists'} ),