From 0fdda016e21ba37bad19fda6cb5344a3ae34188a Mon Sep 17 00:00:00 2001 From: David McDonald Date: Fri, 22 May 2020 17:12:00 +0100 Subject: [PATCH] Refactor archive to do one sync delete of all keys rather than many Otherwise we could be waiting on 50 sync calls to redis to happen. This way we do it in one sync call and follow the pattern of https://github.com/alphagov/notifications-admin/blob/b98f4561fabc00ed1e9abcedfdee29005cdf3e5d/app/notify_client/organisations_api_client.py#L56 --- app/main/views/service_settings.py | 6 +++++- app/notify_client/service_api_client.py | 8 +++----- tests/app/notify_client/test_service_api_client.py | 9 +++++++++ 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index d2ca667e5..234a382f3 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -302,7 +302,11 @@ def archive_service(service_id): ): abort(403) if request.method == 'POST': - service_api_client.archive_service(service_id, current_service.active_users) + # We need to purge the cache for the services users as otherwise, although they will have had their permissions + # removed in the DB, they would still have permissions in the cache to view/edit/manage this service + cached_service_user_ids = [user.id for user in current_service.active_users] + + service_api_client.archive_service(service_id, cached_service_user_ids) flash( '‘{}’ was deleted'.format(current_service.name), 'default_with_tick', diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index 7f118fec0..9c7c9f20f 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -130,11 +130,9 @@ class ServiceAPIClient(NotifyAdminAPIClient): @cache.delete('service-{service_id}') @cache.delete('service-{service_id}-templates') - def archive_service(self, service_id, service_users): - # We need to purge the cache for the services users as otherwise, although they will have had their permissions - # removed in the DB, they would still have permissions in the cache to view/edit/manage this service - for user in service_users: - cache.delete(f'user-{user.id}') + def archive_service(self, service_id, cached_service_user_ids): + if cached_service_user_ids: + cache.delete(*map('user-{}'.format, cached_service_user_ids)) return self.post('/service/{}/archive'.format(service_id), data=None) @cache.delete('service-{service_id}') diff --git a/tests/app/notify_client/test_service_api_client.py b/tests/app/notify_client/test_service_api_client.py index 281af2ca0..f3500d1b2 100644 --- a/tests/app/notify_client/test_service_api_client.py +++ b/tests/app/notify_client/test_service_api_client.py @@ -479,3 +479,12 @@ def test_deletes_caches_when_modifying_templates( assert mock_redis_delete.call_args_list == list(map(call, expected_cache_deletes)) assert len(mock_request.call_args_list) == 1 + + +def test_deletes_cached_users_when_archiving_service(mocker): + mock_redis_delete = mocker.patch('app.notify_client.service_api_client.cache.delete') + mocker.patch('notifications_python_client.base.BaseAPIClient.request') + + service_api_client.archive_service(SERVICE_ONE_ID, ["my-user-id1", "my-user-id2"]) + + assert mock_redis_delete.call_args_list == [call('user-my-user-id1', 'user-my-user-id2')]