From e3baa9ba3512d745c82d4a199a9f517b57e092f4 Mon Sep 17 00:00:00 2001 From: David McDonald Date: Mon, 21 Sep 2020 14:04:10 +0100 Subject: [PATCH] Step 1 of renaming cache keys for templates We want to change cache keys for templates and broadcasts to include their service ID. So cache keys should change from `template-{template_id}-versions` to `service-{service_id}-template-{template_id}-versions`. The first step of this which needs to be deployed as a change first is to delete both keys when updating service templates (even if they key is not yet set). This means that when we release code in the next PR to start setting the new key, we won't run into a case where either the old or the new key can remain set with stale data. --- app/notify_client/broadcast_message_api_client.py | 2 ++ app/notify_client/service_api_client.py | 10 ++++++++++ .../notify_client/test_broadcast_message_client.py | 12 ++++++++++-- tests/app/notify_client/test_service_api_client.py | 10 ++++++++++ 4 files changed, 32 insertions(+), 2 deletions(-) diff --git a/app/notify_client/broadcast_message_api_client.py b/app/notify_client/broadcast_message_api_client.py index bd288183f..eeb3bc63e 100644 --- a/app/notify_client/broadcast_message_api_client.py +++ b/app/notify_client/broadcast_message_api_client.py @@ -32,6 +32,7 @@ class BroadcastMessageAPIClient(NotifyAdminAPIClient): return self.get(f'/service/{service_id}/broadcast-message/{broadcast_message_id}') @cache.delete('broadcast-message-{broadcast_message_id}') + @cache.delete('service-{service_id}-broadcast-message-{broadcast_message_id}') def update_broadcast_message(self, *, service_id, broadcast_message_id, data): self.post( f'/service/{service_id}/broadcast-message/{broadcast_message_id}', @@ -39,6 +40,7 @@ class BroadcastMessageAPIClient(NotifyAdminAPIClient): ) @cache.delete('broadcast-message-{broadcast_message_id}') + @cache.delete('service-{service_id}-broadcast-message-{broadcast_message_id}') def update_broadcast_message_status(self, status, *, service_id, broadcast_message_id): data = _attach_current_user({ 'status': status, diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index 277720ce0..0a8b9e771 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -192,6 +192,8 @@ class ServiceAPIClient(NotifyAdminAPIClient): @cache.delete('service-{service_id}-templates') @cache.delete('template-{id_}-version-None') @cache.delete('template-{id_}-versions') + @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 ): @@ -220,6 +222,8 @@ class ServiceAPIClient(NotifyAdminAPIClient): @cache.delete('service-{service_id}-templates') @cache.delete('template-{id_}-version-None') @cache.delete('template-{id_}-versions') + @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( "/service/{}/template/{}".format(service_id, id_), @@ -231,6 +235,8 @@ class ServiceAPIClient(NotifyAdminAPIClient): @cache.delete('service-{service_id}-templates') @cache.delete('template-{template_id}-version-None') @cache.delete('template-{template_id}-versions') + @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, @@ -244,6 +250,8 @@ class ServiceAPIClient(NotifyAdminAPIClient): @cache.delete('service-{service_id}-templates') @cache.delete('template-{template_id}-version-None') @cache.delete('template-{template_id}-versions') + @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( "/service/{0}/template/{1}".format(service_id, template_id), @@ -302,6 +310,8 @@ class ServiceAPIClient(NotifyAdminAPIClient): @cache.delete('service-{service_id}-templates') @cache.delete('template-{template_id}-version-None') @cache.delete('template-{template_id}-versions') + @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): """ Set a service template's archived flag to True diff --git a/tests/app/notify_client/test_broadcast_message_client.py b/tests/app/notify_client/test_broadcast_message_client.py index 7f87ef949..eac8e6c0e 100644 --- a/tests/app/notify_client/test_broadcast_message_client.py +++ b/tests/app/notify_client/test_broadcast_message_client.py @@ -1,3 +1,5 @@ +from unittest.mock import call + from app.notify_client.broadcast_message_api_client import ( BroadcastMessageAPIClient, ) @@ -70,7 +72,10 @@ def test_update_broadcast_message(mocker): '/service/12345/broadcast-message/67890', data={'abc': 'def'}, ) - mock_redis_delete.assert_called_once_with('broadcast-message-67890') + mock_redis_delete.assert_has_calls([ + call('service-12345-broadcast-message-67890'), + call('broadcast-message-67890'), + ]) def test_update_broadcast_message_status(mocker): @@ -89,4 +94,7 @@ def test_update_broadcast_message_status(mocker): '/service/12345/broadcast-message/67890/status', data={'created_by': '1', 'status': 'cancelled'}, ) - mock_redis_delete.assert_called_once_with('broadcast-message-67890') + mock_redis_delete.assert_has_calls([ + call('service-12345-broadcast-message-67890'), + call('broadcast-message-67890'), + ]) diff --git a/tests/app/notify_client/test_service_api_client.py b/tests/app/notify_client/test_service_api_client.py index 7bcc5a7aa..e172b9569 100644 --- a/tests/app/notify_client/test_service_api_client.py +++ b/tests/app/notify_client/test_service_api_client.py @@ -435,26 +435,36 @@ def test_deletes_service_cache( 'service-{}-templates'.format(SERVICE_ONE_ID), ]), ('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), 'template-{}-versions'.format(FAKE_TEMPLATE_ID), 'template-{}-version-None'.format(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), 'template-{}-versions'.format(FAKE_TEMPLATE_ID), 'template-{}-version-None'.format(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), 'template-{}-versions'.format(FAKE_TEMPLATE_ID), 'template-{}-version-None'.format(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), 'template-{}-versions'.format(FAKE_TEMPLATE_ID), 'template-{}-version-None'.format(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), 'template-{}-versions'.format(FAKE_TEMPLATE_ID), 'template-{}-version-None'.format(FAKE_TEMPLATE_ID), 'service-{}-templates'.format(SERVICE_ONE_ID),