diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index 2f3f76cc8..4c918fa66 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -127,6 +127,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): return self.post('/service/{}/resume'.format(service_id), data=None) @cache.delete('service') + @cache.delete('user', key_from_args=[1]) def remove_user_from_service(self, service_id, user_id): """ Remove a user from a service diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index c9344649a..12d20a129 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -36,10 +36,12 @@ class UserApiClient(NotifyAdminAPIClient): user_data = self.post("/user", data) return User(user_data['data'], max_failed_login_count=self.max_failed_login_count) - def get_user(self, id): - url = "/user/{}".format(id) - user_data = self.get(url) - return User(user_data['data'], max_failed_login_count=self.max_failed_login_count) + def get_user(self, user_id): + return User(self._get_user(user_id)['data'], max_failed_login_count=self.max_failed_login_count) + + @cache.set('user') + def _get_user(self, user_id): + return self.get("/user/{}".format(user_id)) def get_user_by_email(self, email_address): user_data = self.get('/user/email', params={'email': email_address}) @@ -59,6 +61,7 @@ class UserApiClient(NotifyAdminAPIClient): users.append(User(user, max_failed_login_count=self.max_failed_login_count)) return users + @cache.delete('user') def update_user_attribute(self, user_id, **kwargs): data = dict(kwargs) disallowed_attributes = set(data.keys()) - ALLOWED_ATTRIBUTES @@ -72,17 +75,20 @@ class UserApiClient(NotifyAdminAPIClient): user_data = self.post(url, data=data) return User(user_data['data'], max_failed_login_count=self.max_failed_login_count) + @cache.delete('user') def reset_failed_login_count(self, user_id): url = "/user/{}/reset-failed-login-count".format(user_id) user_data = self.post(url, data={}) return User(user_data['data'], max_failed_login_count=self.max_failed_login_count) + @cache.delete('user') def update_password(self, user_id, password): data = {"_password": password} url = "/user/{}/update-password".format(user_id) user_data = self.post(url, data=data) return User(user_data['data'], max_failed_login_count=self.max_failed_login_count) + @cache.delete('user') def verify_password(self, user_id, password): try: url = "/user/{}/verify/password".format(user_id) @@ -112,6 +118,7 @@ class UserApiClient(NotifyAdminAPIClient): endpoint = '/user/{0}/email-already-registered'.format(user_id) self.post(endpoint, data=data) + @cache.delete('user') def check_verify_code(self, user_id, code, code_type): data = {'code_type': code_type, 'code': code} endpoint = '/user/{}/verify/code'.format(user_id) @@ -148,16 +155,19 @@ class UserApiClient(NotifyAdminAPIClient): return [User(data) for data in resp['data']] @cache.delete('service') + @cache.delete('user', key_from_args=[1]) def add_user_to_service(self, service_id, user_id, permissions): # permissions passed in are the combined admin roles, not db permissions endpoint = '/service/{}/users/{}'.format(service_id, user_id) data = [{'permission': x} for x in translate_permissions_from_admin_roles_to_db(permissions)] self.post(endpoint, data=data) + @cache.delete('user', key_from_args=[1]) def add_user_to_organisation(self, org_id, user_id): resp = self.post('/organisations/{}/users/{}'.format(org_id, user_id), data={}) return User(resp['data'], max_failed_login_count=self.max_failed_login_count) + @cache.delete('user') def set_user_permissions(self, user_id, service_id, permissions): # permissions passed in are the combined admin roles, not db permissions data = [{'permission': x} for x in translate_permissions_from_admin_roles_to_db(permissions)] @@ -176,12 +186,15 @@ class UserApiClient(NotifyAdminAPIClient): def activate_user(self, user): if user.state == 'pending': - url = "/user/{}/activate".format(user.id) - user_data = self.post(url, data=None) + user_data = self._activate_user(user.id) return User(user_data['data'], max_failed_login_count=self.max_failed_login_count) else: return user + @cache.delete('user') + def _activate_user(self, user_id): + return self.post("/user/{}/activate".format(user_id), data=None) + def send_change_email_verification(self, user_id, new_email): endpoint = '/user/{}/change-email-verification'.format(user_id) data = {'email': new_email} diff --git a/tests/app/notify_client/test_service_api_client.py b/tests/app/notify_client/test_service_api_client.py index aa137ca46..539ec4a70 100644 --- a/tests/app/notify_client/test_service_api_client.py +++ b/tests/app/notify_client/test_service_api_client.py @@ -237,7 +237,5 @@ def test_deletes_service_cache( getattr(client, method)(*extra_args, **extra_kwargs) - assert mock_redis_delete.call_args_list == [ - call('service-{}'.format(SERVICE_ONE_ID)) - ] + assert call('service-{}'.format(SERVICE_ONE_ID)) in mock_redis_delete.call_args_list assert len(mock_request.call_args_list) == 1 diff --git a/tests/app/notify_client/test_user_client.py b/tests/app/notify_client/test_user_client.py index 2a56f66ea..338fbccb3 100644 --- a/tests/app/notify_client/test_user_client.py +++ b/tests/app/notify_client/test_user_client.py @@ -1,11 +1,13 @@ from unittest.mock import call import pytest -from tests.conftest import SERVICE_ONE_ID +from tests.conftest import SERVICE_ONE_ID, api_user_pending, fake_uuid -from app import user_api_client +from app import service_api_client, user_api_client from app.notify_client.models import User +user_id = fake_uuid() + def test_client_gets_all_users_for_service( mocker, @@ -157,3 +159,110 @@ def test_client_converts_admin_permissions_to_db_permissions_on_add_to_service(a {'permission': 'send_letters'}, {'permission': 'view_activity'}, ], key=lambda x: x['permission']) + + +@pytest.mark.parametrize( + ( + 'expected_cache_get_calls,' + 'cache_value,' + 'expected_api_calls,' + 'expected_cache_set_calls,' + 'expected_return_value,' + ), + [ + ( + [ + call('user-{}'.format(user_id)) + ], + b'{"data": "from cache"}', + [], + [], + 'from cache', + ), + ( + [ + call('user-{}'.format(user_id)) + ], + None, + [ + call('/user/{}'.format(user_id)) + ], + [ + call( + 'user-{}'.format(user_id), + '{"data": "from api"}', + ex=86400 + ) + ], + 'from api', + ), + ] +) +def test_returns_value_from_cache( + app_, + mocker, + expected_cache_get_calls, + cache_value, + expected_return_value, + expected_api_calls, + expected_cache_set_calls, +): + + mock_redis_get = mocker.patch( + 'app.notify_client.RedisClient.get', + return_value=cache_value, + ) + mock_api_get = mocker.patch( + 'app.notify_client.NotifyAdminAPIClient.get', + return_value={'data': 'from api'}, + ) + mock_redis_set = mocker.patch( + 'app.notify_client.RedisClient.set', + ) + mock_model = mocker.patch( + 'app.notify_client.models.User.__init__', + return_value=None, + ) + + user_api_client.get_user(user_id) + + mock_model.assert_called_once_with( + expected_return_value, + max_failed_login_count=10, + ) + assert mock_redis_get.call_args_list == expected_cache_get_calls + assert mock_api_get.call_args_list == expected_api_calls + assert mock_redis_set.call_args_list == expected_cache_set_calls + + +@pytest.mark.parametrize('client, method, extra_args, extra_kwargs', [ + (user_api_client, 'add_user_to_service', [SERVICE_ONE_ID, fake_uuid(), []], {}), + (user_api_client, 'update_user_attribute', [user_id], {}), + (user_api_client, 'reset_failed_login_count', [user_id], {}), + (user_api_client, 'update_user_attribute', [user_id], {}), + (user_api_client, 'update_password', [user_id, 'hunter2'], {}), + (user_api_client, 'verify_password', [user_id, 'hunter2'], {}), + (user_api_client, 'check_verify_code', [user_id, '', ''], {}), + (user_api_client, 'add_user_to_service', [SERVICE_ONE_ID, user_id, []], {}), + (user_api_client, 'add_user_to_organisation', [fake_uuid(), user_id], {}), + (user_api_client, 'set_user_permissions', [user_id, SERVICE_ONE_ID, []], {}), + (user_api_client, 'activate_user', [api_user_pending(fake_uuid())], {}), + (service_api_client, 'remove_user_from_service', [SERVICE_ONE_ID, user_id], {}), +]) +def test_deletes_user_cache( + app_, + mock_get_user, + mocker, + client, + method, + extra_args, + extra_kwargs, +): + mocker.patch('app.notify_client.current_user', id='1') + mock_redis_delete = mocker.patch('app.notify_client.RedisClient.delete') + mock_request = mocker.patch('notifications_python_client.base.BaseAPIClient.request') + + getattr(client, method)(*extra_args, **extra_kwargs) + + assert call('user-{}'.format(user_id)) in mock_redis_delete.call_args_list + assert len(mock_request.call_args_list) == 1