From 01a3df6edce08e3cf0cca42b60dcf95127155497 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 25 Jan 2021 14:03:16 +0000 Subject: [PATCH] Update _delete_template_cache_for_service to delete all template version cache and not just the one ending in "None" Update all methods that were previous calling @cache.delete('service-{service-id}-template-None') to instead call _delete_template_cache_for_service Remove call to get service templates, it's not needed since all template version cache is being deleted. --- app/notify_client/service_api_client.py | 32 +++++++-------- package.json | 2 +- tests/app/main/views/test_service_settings.py | 2 + .../notify_client/test_service_api_client.py | 41 ++++++++++--------- 4 files changed, 41 insertions(+), 36 deletions(-) diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index a3dfbcc40..7fa74f5b8 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -7,12 +7,7 @@ from app.notify_client import NotifyAdminAPIClient, _attach_current_user, cache class ServiceAPIClient(NotifyAdminAPIClient): def _delete_template_cache_for_service(self, service_id): - templates_for_service = self.get_service_templates(service_id)['data'] - if templates_for_service: - redis_client.delete(*[ - f"service-{service_id}-template-{x['id']}-version-None" - for x in templates_for_service - ]) + redis_client.delete_cache_keys_by_pattern(f"service-{service_id}-template-*") @cache.delete('user-{user_id}') def create_service( @@ -109,12 +104,12 @@ class ServiceAPIClient(NotifyAdminAPIClient): 'go_live_user', 'go_live_at', 'rate_limit', - 'notes', } if disallowed_attributes: raise TypeError('Not allowed to update service attributes: {}'.format( ", ".join(disallowed_attributes) )) + endpoint = "/service/{0}".format(service_id) return self.post(endpoint, data) @@ -143,7 +138,9 @@ class ServiceAPIClient(NotifyAdminAPIClient): def archive_service(self, service_id, cached_service_user_ids): if cached_service_user_ids: redis_client.delete(*map('user-{}'.format, cached_service_user_ids)) - return self.post('/service/{}/archive'.format(service_id), data=None) + ret = self.post('/service/{}/archive'.format(service_id), data=None) + self._delete_template_cache_for_service(str(service_id)) + return ret @cache.delete('service-{service_id}') def suspend_service(self, service_id): @@ -191,7 +188,6 @@ class ServiceAPIClient(NotifyAdminAPIClient): return self.post(endpoint, data) @cache.delete('service-{service_id}-templates') - @cache.delete('service-{service_id}-template-{id_}-version-None') @cache.delete('service-{service_id}-template-{id_}-versions') def update_service_template( self, id_, name, type_, content, service_id, subject=None, process_type=None @@ -216,40 +212,44 @@ class ServiceAPIClient(NotifyAdminAPIClient): }) data = _attach_current_user(data) endpoint = "/service/{0}/template/{1}".format(service_id, id_) + self._delete_template_cache_for_service(service_id) return self.post(endpoint, data) @cache.delete('service-{service_id}-templates') - @cache.delete('service-{service_id}-template-{id_}-version-None') @cache.delete('service-{service_id}-template-{id_}-versions') def redact_service_template(self, service_id, id_): - return self.post( + ret = self.post( "/service/{}/template/{}".format(service_id, id_), _attach_current_user( {'redact_personalisation': True} ), ) + self._delete_template_cache_for_service(service_id) + return ret @cache.delete('service-{service_id}-templates') - @cache.delete('service-{service_id}-template-{template_id}-version-None') @cache.delete('service-{service_id}-template-{template_id}-versions') def update_service_template_sender(self, service_id, template_id, reply_to): data = { 'reply_to': reply_to, } data = _attach_current_user(data) - return self.post( + ret = self.post( "/service/{0}/template/{1}".format(service_id, template_id), data ) + self._delete_template_cache_for_service(service_id) + return ret @cache.delete('service-{service_id}-templates') - @cache.delete('service-{service_id}-template-{template_id}-version-None') @cache.delete('service-{service_id}-template-{template_id}-versions') def update_service_template_postage(self, service_id, template_id, postage): - return self.post( + ret = self.post( "/service/{0}/template/{1}".format(service_id, template_id), _attach_current_user({'postage': postage}) ) + self._delete_template_cache_for_service(service_id) + return ret @cache.set('service-{service_id}-template-{template_id}-version-{version}') def get_service_template(self, service_id, template_id, version=None): @@ -301,7 +301,6 @@ class ServiceAPIClient(NotifyAdminAPIClient): ]) @cache.delete('service-{service_id}-templates') - @cache.delete('service-{service_id}-template-{template_id}-version-None') @cache.delete('service-{service_id}-template-{template_id}-versions') def delete_service_template(self, service_id, template_id): """ @@ -312,6 +311,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): 'archived': True } data = _attach_current_user(data) + self._delete_template_cache_for_service(service_id) return self.post(endpoint, data=data) def is_service_name_unique(self, service_id, name, email_from): diff --git a/package.json b/package.json index 61b310a78..afab8009c 100644 --- a/package.json +++ b/package.json @@ -23,9 +23,9 @@ "@babel/preset-env": "7.4.2", "del": "5.1.0", "diff-dom": "2.5.1", + "govuk_frontend_toolkit": "8.1.0", "govuk-elements-sass": "3.1.2", "govuk-frontend": "2.13.0", - "govuk_frontend_toolkit": "8.1.0", "gulp": "4.0.0", "gulp-add-src": "1.0.0", "gulp-babel": "8.0.0", diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index f6f0d05aa..d06fa78e6 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -3976,10 +3976,12 @@ def test_archive_service_after_confirm( mock_get_service_and_organisation_counts, mock_get_organisations_and_services_for_user, mock_get_users_by_service, + mock_get_service_templates, user, ): mocked_fn = mocker.patch('app.service_api_client.post') redis_delete_mock = mocker.patch('app.notify_client.service_api_client.redis_client.delete') + mocker.patch('app.notify_client.service_api_client.redis_client.delete_cache_keys_by_pattern') client_request.login(user) page = client_request.post( 'main.archive_service', diff --git a/tests/app/notify_client/test_service_api_client.py b/tests/app/notify_client/test_service_api_client.py index 4a2ef83b8..80b276e2a 100644 --- a/tests/app/notify_client/test_service_api_client.py +++ b/tests/app/notify_client/test_service_api_client.py @@ -12,7 +12,7 @@ FAKE_TEMPLATE_ID = uuid4() def test_client_posts_archived_true_when_deleting_template(mocker): mocker.patch('app.notify_client.current_user', id='1') - + mock_redis_delete_by_pattern = mocker.patch('app.extensions.RedisClient.delete_cache_keys_by_pattern') expected_data = { 'archived': True, 'created_by': '1' @@ -21,9 +21,12 @@ def test_client_posts_archived_true_when_deleting_template(mocker): client = ServiceAPIClient() mock_post = mocker.patch('app.notify_client.service_api_client.ServiceAPIClient.post') + mocker.patch('app.notify_client.service_api_client.ServiceAPIClient.get', + return_value={'data': {'id': str(FAKE_TEMPLATE_ID)}}) client.delete_service_template(SERVICE_ONE_ID, FAKE_TEMPLATE_ID) mock_post.assert_called_once_with(expected_url, data=expected_data) + assert call(f'service-{SERVICE_ONE_ID}-template-*') in mock_redis_delete_by_pattern.call_args_list def test_client_gets_service(mocker): @@ -436,27 +439,22 @@ def test_deletes_service_cache( ]), ('update_service_template', [FAKE_TEMPLATE_ID, 'foo', 'sms', 'bar', SERVICE_ONE_ID], [ 'service-{}-template-{}-versions'.format(SERVICE_ONE_ID, FAKE_TEMPLATE_ID), - 'service-{}-template-{}-version-None'.format(SERVICE_ONE_ID, FAKE_TEMPLATE_ID), 'service-{}-templates'.format(SERVICE_ONE_ID), ]), ('redact_service_template', [SERVICE_ONE_ID, FAKE_TEMPLATE_ID], [ 'service-{}-template-{}-versions'.format(SERVICE_ONE_ID, FAKE_TEMPLATE_ID), - 'service-{}-template-{}-version-None'.format(SERVICE_ONE_ID, FAKE_TEMPLATE_ID), 'service-{}-templates'.format(SERVICE_ONE_ID), ]), ('update_service_template_sender', [SERVICE_ONE_ID, FAKE_TEMPLATE_ID, 'foo'], [ 'service-{}-template-{}-versions'.format(SERVICE_ONE_ID, FAKE_TEMPLATE_ID), - 'service-{}-template-{}-version-None'.format(SERVICE_ONE_ID, FAKE_TEMPLATE_ID), 'service-{}-templates'.format(SERVICE_ONE_ID), ]), ('update_service_template_postage', [SERVICE_ONE_ID, FAKE_TEMPLATE_ID, 'first'], [ 'service-{}-template-{}-versions'.format(SERVICE_ONE_ID, FAKE_TEMPLATE_ID), - 'service-{}-template-{}-version-None'.format(SERVICE_ONE_ID, FAKE_TEMPLATE_ID), 'service-{}-templates'.format(SERVICE_ONE_ID), ]), ('delete_service_template', [SERVICE_ONE_ID, FAKE_TEMPLATE_ID], [ 'service-{}-template-{}-versions'.format(SERVICE_ONE_ID, FAKE_TEMPLATE_ID), - 'service-{}-template-{}-version-None'.format(SERVICE_ONE_ID, FAKE_TEMPLATE_ID), 'service-{}-templates'.format(SERVICE_ONE_ID), ]), ('archive_service', [SERVICE_ONE_ID, []], [ @@ -471,25 +469,32 @@ def test_deletes_caches_when_modifying_templates( method, extra_args, expected_cache_deletes, - mock_get_service_templates, ): mocker.patch('app.notify_client.current_user', id='1') mock_redis_delete = mocker.patch('app.extensions.RedisClient.delete') + mock_redis_delete_by_pattern = mocker.patch('app.extensions.RedisClient.delete_cache_keys_by_pattern') mock_request = mocker.patch('notifications_python_client.base.BaseAPIClient.request') getattr(service_api_client, method)(*extra_args) assert mock_redis_delete.call_args_list == [call(x) for x in expected_cache_deletes] assert len(mock_request.call_args_list) == 1 + if method != 'create_service_template': + # no deletes for template cach on create_service_template + assert len(mock_redis_delete_by_pattern.call_args_list) == 1 + assert mock_redis_delete_by_pattern.call_args_list[0] == call(f'service-{SERVICE_ONE_ID}-template-*') -def test_deletes_cached_users_when_archiving_service(mocker): - mock_redis_delete = mocker.patch('app.notify_client.service_api_client.redis_client.delete') - mocker.patch('notifications_python_client.base.BaseAPIClient.request') +def test_deletes_cached_users_when_archiving_service(mocker, mock_get_service_templates): + mock_redis_delete = mocker.patch('app.extensions.RedisClient.delete') + mock_redis_delete_by_pattern = mocker.patch('app.extensions.RedisClient.delete_cache_keys_by_pattern') + + mocker.patch('notifications_python_client.base.BaseAPIClient.request', return_value={'data': ""}) service_api_client.archive_service(SERVICE_ONE_ID, ["my-user-id1", "my-user-id2"]) assert call('user-my-user-id1', 'user-my-user-id2') in mock_redis_delete.call_args_list + assert call(f'service-{SERVICE_ONE_ID}-template-*') in mock_redis_delete_by_pattern.call_args_list def test_client_gets_guest_list(mocker): @@ -525,30 +530,28 @@ def test_client_doesnt_delete_service_template_cache_when_none_exist( mocker.patch('app.notify_client.current_user', id='1') mocker.patch('notifications_python_client.base.BaseAPIClient.request') mock_redis_delete = mocker.patch('app.extensions.RedisClient.delete') + mock_redis_delete_by_pattern = mocker.patch('app.extensions.RedisClient.delete_cache_keys_by_pattern') service_api_client.update_reply_to_email_address(SERVICE_ONE_ID, uuid4(), 'foo@bar.com') assert len(mock_redis_delete.call_args_list) == 1 assert mock_redis_delete.call_args_list[0] == call('service-{}'.format(SERVICE_ONE_ID)) + assert len(mock_redis_delete_by_pattern.call_args_list) == 1 + def test_client_deletes_service_template_cache_when_service_is_updated( app_, mock_get_user, - mock_get_service_templates, mocker ): mocker.patch('app.notify_client.current_user', id='1') mocker.patch('notifications_python_client.base.BaseAPIClient.request') mock_redis_delete = mocker.patch('app.extensions.RedisClient.delete') + mock_redis_delete_by_pattern = mocker.patch('app.extensions.RedisClient.delete_cache_keys_by_pattern') service_api_client.update_reply_to_email_address(SERVICE_ONE_ID, uuid4(), 'foo@bar.com') - assert len(mock_redis_delete.call_args_list) == 2 - assert mock_redis_delete.call_args_list[1] == call('service-{}'.format(SERVICE_ONE_ID)) - - templates_to_delete = mock_redis_delete.call_args_list[0][0] - assert len(templates_to_delete) == 6 - for template_key in templates_to_delete: - assert template_key.startswith(f'service-{SERVICE_ONE_ID}-template') - assert template_key.endswith('version-None') + assert len(mock_redis_delete.call_args_list) == 1 + assert mock_redis_delete.call_args_list[0] == call(f'service-{SERVICE_ONE_ID}') + assert mock_redis_delete_by_pattern.call_args_list[0] == call(f'service-{SERVICE_ONE_ID}-template-*')