Merge pull request #2022 from alphagov/redis-cache-user

Cache `GET /user` response in Redis

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

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):
+    def get_user(self, user_id):
-        url = "/user/{}".format(id)
+        return User(self._get_user(user_id)['data'], max_failed_login_count=self.max_failed_login_count)
-        user_data = self.get(url)
+
-        return User(user_data['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._activate_user(user.id)
-            user_data = self.post(url, data=None)
             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}

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 == [
+    assert call('service-{}'.format(SERVICE_ONE_ID)) in mock_redis_delete.call_args_list
-        call('service-{}'.format(SERVICE_ONE_ID))
-    ]
     assert len(mock_request.call_args_list) == 1

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