From 313d39415d1d7922ae72778d5f615aad41c302cc Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Tue, 19 May 2020 13:49:17 +0100 Subject: [PATCH] Catch errors when user register from invite API gives an error if it tries to add a user to a service and that user is already a member of the service. This situation shouldn't occur - admin checks if an invited user is a member of a service before calling API, but we have seen this error occurring when there are two requests processing at the same time. This change catches the errors from API if a user is already a member of a service and redirects the user to the service dashboard so that they don't see an error page. --- app/models/user.py | 18 ++++++++---- tests/app/main/views/test_accept_invite.py | 32 +++++++++++++++++++++ tests/app/main/views/test_verify.py | 33 ++++++++++++++++++++++ 3 files changed, 77 insertions(+), 6 deletions(-) diff --git a/app/models/user.py b/app/models/user.py index 8f4650578..669240867 100644 --- a/app/models/user.py +++ b/app/models/user.py @@ -407,12 +407,18 @@ class User(JSONModel, UserMixin): session['current_session_id'] = self.current_session_id def add_to_service(self, service_id, permissions, folder_permissions): - user_api_client.add_user_to_service( - service_id, - self.id, - permissions, - folder_permissions, - ) + try: + user_api_client.add_user_to_service( + service_id, + self.id, + permissions, + folder_permissions, + ) + except HTTPError as exception: + if exception.status_code == 400 and 'already part of service' in exception.message: + pass + else: + raise exception def add_to_organisation(self, organisation_id): user_api_client.add_user_to_organisation( diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index 45ab72622..d779beae4 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/tests/app/main/views/test_accept_invite.py @@ -11,6 +11,7 @@ from tests.conftest import ( USER_ONE_ID, create_active_caseworking_user, create_active_user_with_permissions, + create_api_user_active, normalize_spaces, ) @@ -196,6 +197,37 @@ def test_existing_user_of_service_get_redirected_to_signin( assert mock_accept_invite.call_count == 1 +def test_accept_invite_redirects_if_api_raises_an_error_that_they_are_already_part_of_the_service( + client, + mocker, + api_user_active, + sample_invite, + mock_accept_invite, + mock_get_service, + mock_get_users_by_service +): + sample_invite['email_address'] = api_user_active['email_address'] + + # This mock needs to return a user with a different ID to the invited user so that + # `existing_user in Users(invited_user.service)` returns False and the right code path is tested + mocker.patch('app.user_api_client.get_user_by_email', return_value=create_api_user_active(with_unique_id=True)) + mocker.patch('app.invite_api_client.check_token', return_value=sample_invite) + + mocker.patch('app.user_api_client.add_user_to_service', side_effect=HTTPError( + response=Mock( + status_code=400, + json={ + "result": "error", + "message": {f"User id: {api_user_active['id']} already part of service id: {SERVICE_ONE_ID}"} + }, + ), + message=f"User id: {api_user_active['id']} already part of service id: {SERVICE_ONE_ID}" + )) + + response = client.get(url_for('main.accept_invite', token='thisisnotarealtoken'), follow_redirects=False) + assert response.location == url_for('main.service_dashboard', service_id=SERVICE_ONE_ID, _external=True) + + def test_existing_signed_out_user_accept_invite_redirects_to_sign_in( client, service_one, diff --git a/tests/app/main/views/test_verify.py b/tests/app/main/views/test_verify.py index f143d382a..d0bbe6256 100644 --- a/tests/app/main/views/test_verify.py +++ b/tests/app/main/views/test_verify.py @@ -1,10 +1,14 @@ import json import uuid +from unittest.mock import Mock from bs4 import BeautifulSoup +from flask import session as flask_session from flask import url_for from itsdangerous import SignatureExpired +from notifications_python_client.errors import HTTPError +from app.main.views.verify import activate_user from tests.conftest import normalize_spaces @@ -165,3 +169,32 @@ def test_verify_redirects_to_sign_in_if_not_logged_in( assert response.location == url_for('main.sign_in', _external=True) assert response.status_code == 302 + + +def test_activate_user_redirects_to_service_dashboard_if_user_already_belongs_to_service( + mocker, + client, + service_one, + sample_invite, + api_user_active, + mock_login, +): + mocker.patch('app.user_api_client.add_user_to_service', side_effect=HTTPError( + response=Mock( + status_code=400, + json={ + "result": "error", + "message": {f"User id: {api_user_active['id']} already part of service id: {service_one['id']}"} + }, + ), + message=f"User id: {api_user_active['id']} already part of service id: {service_one['id']}" + )) + + # Can't use `with client.session_transaction()...` here since activate_session is not a view function + flask_session['invited_user'] = sample_invite + + response = activate_user(api_user_active['id']) + + assert response.location == url_for('main.service_dashboard', service_id=service_one['id']) + + flask_session.pop('invited_user')