From 6c8fea1ee8cb4000cb914ecdf9f9bb582c13c9ca Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 19 Apr 2018 10:26:17 +0100 Subject: [PATCH 1/6] Remove splatting on get template methods MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This `*params` argument seems to be copy/pasted boilerplate. It’s not used by any consumers of this client, and makes it harder to write a decorator for this function. --- app/notify_client/service_api_client.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index 76c09c9ed..b4c1e8b36 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -199,7 +199,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): data ) - def get_service_template(self, service_id, template_id, version=None, *params): + def get_service_template(self, service_id, template_id, version=None): """ Retrieve a service template. """ @@ -208,9 +208,9 @@ class ServiceAPIClient(NotifyAdminAPIClient): template_id=template_id) if version: endpoint = '{base}/version/{version}'.format(base=endpoint, version=version) - return self.get(endpoint, *params) + return self.get(endpoint) - def get_service_template_versions(self, service_id, template_id, *params): + def get_service_template_versions(self, service_id, template_id): """ Retrieve a list of versions for a template """ @@ -218,15 +218,15 @@ class ServiceAPIClient(NotifyAdminAPIClient): service_id=service_id, template_id=template_id ) - return self.get(endpoint, *params) + return self.get(endpoint) - def get_service_templates(self, service_id, *params): + def get_service_templates(self, service_id): """ Retrieve all templates for service. """ endpoint = '/service/{service_id}/template'.format( service_id=service_id) - return self.get(endpoint, *params) + return self.get(endpoint) def count_service_templates(self, service_id, template_type=None): return len([ From 6101e5da4394dbf2b0dba1dc8fe08ccbe5585261 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 19 Apr 2018 10:23:21 +0100 Subject: [PATCH 2/6] Rewrite cache decorator to reference args by name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `@cache.delete('user', 'user_id')` is easier to read and understand than `@cache.delete('user', key_from_args=[1])`. This will become even more apparent if we have to start doing stuff like `key_from_args=[1, 5]`, which is a lot more opaque than just saying `'service_id', 'template_id'`. It does make the implementation a bit more complex, but I’m not too worried about that because: - the tests are solid - it’s nicely encapsulated --- app/notify_client/cache.py | 40 +++++++++++++++---- app/notify_client/invite_api_client.py | 4 +- app/notify_client/organisations_api_client.py | 2 +- app/notify_client/service_api_client.py | 38 +++++++++--------- app/notify_client/user_api_client.py | 22 +++++----- 5 files changed, 65 insertions(+), 41 deletions(-) diff --git a/app/notify_client/cache.py b/app/notify_client/cache.py index bc356bbb0..674f4fdb4 100644 --- a/app/notify_client/cache.py +++ b/app/notify_client/cache.py @@ -1,27 +1,51 @@ import json +from contextlib import suppress from datetime import timedelta from functools import wraps +from inspect import signature TTL = int(timedelta(hours=24).total_seconds()) -def _make_key(prefix, args, key_from_args): +def _get_argument(argument_name, args, kwargs, client_method): - if key_from_args is None: - key_from_args = [0] + with suppress(KeyError): + return kwargs[argument_name] + with suppress(ValueError): + return args[list(signature(client_method).parameters).index(argument_name) - 1] + + raise TypeError("{}() takes no argument called '{}'".format( + client_method.__name__, argument_name + )) + + +def list_of_strings(list_of_stuff): + return list(map(str, filter(None, list_of_stuff))) + + +def _make_key(prefix, key_from_args, local_variables): return '-'.join( - [prefix] + [args[index] for index in key_from_args] + [ + local_variables['prefix'] + ] + list_of_strings( + _get_argument( + argument_name, + local_variables['args'], + local_variables['kwargs'], + local_variables['client_method'] + ) for argument_name in key_from_args + ) ) -def set(prefix, key_from_args=None): +def set(prefix, *key_from_args): def _set(client_method): @wraps(client_method) def new_client_method(client_instance, *args, **kwargs): - redis_key = _make_key(prefix, args, key_from_args) + redis_key = _make_key(prefix, key_from_args, locals()) cached = client_instance.redis_client.get(redis_key) if cached: return json.loads(cached.decode('utf-8')) @@ -37,13 +61,13 @@ def set(prefix, key_from_args=None): return _set -def delete(prefix, key_from_args=None): +def delete(prefix, *key_from_args): def _delete(client_method): @wraps(client_method) def new_client_method(client_instance, *args, **kwargs): - redis_key = _make_key(prefix, args, key_from_args) + redis_key = _make_key(prefix, key_from_args, locals()) client_instance.redis_client.delete(redis_key) return client_method(client_instance, *args, **kwargs) diff --git a/app/notify_client/invite_api_client.py b/app/notify_client/invite_api_client.py index 2cfe0946c..45e6840f1 100644 --- a/app/notify_client/invite_api_client.py +++ b/app/notify_client/invite_api_client.py @@ -44,8 +44,8 @@ class InviteApiClient(NotifyAdminAPIClient): self.post(url='/service/{0}/invite/{1}'.format(service_id, invited_user_id), data=data) - @cache.delete('service') - @cache.delete('user', key_from_args=[1]) + @cache.delete('service', 'service_id') + @cache.delete('user', 'invited_user_id') def accept_invite(self, service_id, invited_user_id): data = {'status': 'accepted'} self.post(url='/service/{0}/invite/{1}'.format(service_id, invited_user_id), diff --git a/app/notify_client/organisations_api_client.py b/app/notify_client/organisations_api_client.py index 02a2cb6e3..6bab54e5a 100644 --- a/app/notify_client/organisations_api_client.py +++ b/app/notify_client/organisations_api_client.py @@ -27,7 +27,7 @@ class OrganisationsClient(NotifyAdminAPIClient): def get_service_organisation(self, service_id): return self.get(url="/service/{}/organisation".format(service_id)) - @cache.delete('service') + @cache.delete('service', 'service_id') def update_service_organisation(self, service_id, org_id): data = { 'service_id': service_id diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index b4c1e8b36..f379849ec 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -9,7 +9,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): def __init__(self): super().__init__("a" * 73, "b") - @cache.delete('user', key_from_args=[4]) + @cache.delete('user', 'user_id') def create_service( self, service_name, @@ -34,7 +34,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): data = _attach_current_user(data) return self.post("/service", data)['data']['id'] - @cache.set('service') + @cache.set('service', 'service_id') def get_service(self, service_id): return self._get_service(service_id, detailed=False, today_only=False) @@ -74,7 +74,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): params_dict['only_active'] = True return self.get_services(params_dict) - @cache.delete('service') + @cache.delete('service', 'service_id') def update_service( self, service_id, @@ -115,20 +115,20 @@ class ServiceAPIClient(NotifyAdminAPIClient): def update_service_with_properties(self, service_id, properties): return self.update_service(service_id, **properties) - @cache.delete('service') + @cache.delete('service', 'service_id') def archive_service(self, service_id): return self.post('/service/{}/archive'.format(service_id), data=None) - @cache.delete('service') + @cache.delete('service', 'service_id') def suspend_service(self, service_id): return self.post('/service/{}/suspend'.format(service_id), data=None) - @cache.delete('service') + @cache.delete('service', 'service_id') def resume_service(self, service_id): return self.post('/service/{}/resume'.format(service_id), data=None) - @cache.delete('service') - @cache.delete('user', key_from_args=[1]) + @cache.delete('service', 'service_id') + @cache.delete('user', 'user_id') def remove_user_from_service(self, service_id, user_id): """ Remove a user from a service @@ -267,7 +267,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): def get_whitelist(self, service_id): return self.get(url='/service/{}/whitelist'.format(service_id)) - @cache.delete('service') + @cache.delete('service', 'service_id') def update_whitelist(self, service_id, data): return self.put(url='/service/{}/whitelist'.format(service_id), data=data) @@ -305,7 +305,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): '/service/{}/inbound-sms/summary'.format(service_id) ) - @cache.delete('service') + @cache.delete('service', 'service_id') def create_service_inbound_api(self, service_id, url, bearer_token, user_id): data = { "url": url, @@ -314,7 +314,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): } return self.post("/service/{}/inbound-api".format(service_id), data) - @cache.delete('service') + @cache.delete('service', 'service_id') def update_service_inbound_api(self, service_id, url, bearer_token, user_id, inbound_api_id): data = { "url": url, @@ -346,7 +346,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): ) ) - @cache.delete('service') + @cache.delete('service', 'service_id') def add_reply_to_email_address(self, service_id, email_address, is_default=False): return self.post( "/service/{}/email-reply-to".format(service_id), @@ -356,7 +356,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): } ) - @cache.delete('service') + @cache.delete('service', 'service_id') def update_reply_to_email_address(self, service_id, reply_to_email_id, email_address, is_default=False): return self.post( "/service/{}/email-reply-to/{}".format( @@ -375,7 +375,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): def get_letter_contact(self, service_id, letter_contact_id): return self.get("/service/{}/letter-contact/{}".format(service_id, letter_contact_id)) - @cache.delete('service') + @cache.delete('service', 'service_id') def add_letter_contact(self, service_id, contact_block, is_default=False): return self.post( "/service/{}/letter-contact".format(service_id), @@ -385,7 +385,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): } ) - @cache.delete('service') + @cache.delete('service', 'service_id') def update_letter_contact(self, service_id, letter_contact_id, contact_block, is_default=False): return self.post( "/service/{}/letter-contact/{}".format( @@ -411,7 +411,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): "/service/{}/sms-sender/{}".format(service_id, sms_sender_id) ) - @cache.delete('service') + @cache.delete('service', 'service_id') def add_sms_sender(self, service_id, sms_sender, is_default=False, inbound_number_id=None): data = { "sms_sender": sms_sender, @@ -421,7 +421,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): data["inbound_number_id"] = inbound_number_id return self.post("/service/{}/sms-sender".format(service_id), data=data) - @cache.delete('service') + @cache.delete('service', 'service_id') def update_sms_sender(self, service_id, sms_sender_id, sms_sender, is_default=False): return self.post( "/service/{}/sms-sender/{}".format(service_id, sms_sender_id), @@ -438,7 +438,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): ) )['data'] - @cache.delete('service') + @cache.delete('service', 'service_id') def update_service_callback_api(self, service_id, url, bearer_token, user_id, callback_api_id): data = { "url": url, @@ -448,7 +448,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): data['bearer_token'] = bearer_token return self.post("/service/{}/delivery-receipt-api/{}".format(service_id, callback_api_id), data) - @cache.delete('service') + @cache.delete('service', 'service_id') def create_service_callback_api(self, service_id, url, bearer_token, user_id): data = { "url": url, diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index 12d20a129..e5fbcfae6 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -39,7 +39,7 @@ class UserApiClient(NotifyAdminAPIClient): 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') + @cache.set('user', 'user_id') def _get_user(self, user_id): return self.get("/user/{}".format(user_id)) @@ -61,7 +61,7 @@ class UserApiClient(NotifyAdminAPIClient): users.append(User(user, max_failed_login_count=self.max_failed_login_count)) return users - @cache.delete('user') + @cache.delete('user', 'user_id') def update_user_attribute(self, user_id, **kwargs): data = dict(kwargs) disallowed_attributes = set(data.keys()) - ALLOWED_ATTRIBUTES @@ -75,20 +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') + @cache.delete('user', 'user_id') 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') + @cache.delete('user', 'user_id') 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') + @cache.delete('user', 'user_id') def verify_password(self, user_id, password): try: url = "/user/{}/verify/password".format(user_id) @@ -118,7 +118,7 @@ class UserApiClient(NotifyAdminAPIClient): endpoint = '/user/{0}/email-already-registered'.format(user_id) self.post(endpoint, data=data) - @cache.delete('user') + @cache.delete('user', 'user_id') def check_verify_code(self, user_id, code, code_type): data = {'code_type': code_type, 'code': code} endpoint = '/user/{}/verify/code'.format(user_id) @@ -154,20 +154,20 @@ class UserApiClient(NotifyAdminAPIClient): resp = self.get(endpoint) return [User(data) for data in resp['data']] - @cache.delete('service') - @cache.delete('user', key_from_args=[1]) + @cache.delete('service', 'service_id') + @cache.delete('user', 'user_id') 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]) + @cache.delete('user', 'user_id') 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') + @cache.delete('user', 'user_id') 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)] @@ -191,7 +191,7 @@ class UserApiClient(NotifyAdminAPIClient): else: return user - @cache.delete('user') + @cache.delete('user', 'user_id') def _activate_user(self, user_id): return self.post("/user/{}/activate".format(user_id), data=None) From cea7a027e3fb8c60cc09dd98d12a5a814cc8cf26 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 19 Apr 2018 10:24:35 +0100 Subject: [PATCH 3/6] Add caching of templates in Redis MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A lot of the frequently-used pages in the admin app rely on the API to get templates. So this commit adds three new caches: - a single template version (including a key without a version number, which is the current version) - all the templates for a service - all versions of a template The first will be the most crucial for performance, but there’s not much cost to adding the other two. --- app/notify_client/cache.py | 5 +- app/notify_client/service_api_client.py | 17 +++ .../notify_client/test_service_api_client.py | 119 +++++++++++++++++- 3 files changed, 137 insertions(+), 4 deletions(-) diff --git a/app/notify_client/cache.py b/app/notify_client/cache.py index 674f4fdb4..7d622e11e 100644 --- a/app/notify_client/cache.py +++ b/app/notify_client/cache.py @@ -12,9 +12,12 @@ def _get_argument(argument_name, args, kwargs, client_method): with suppress(KeyError): return kwargs[argument_name] - with suppress(ValueError): + with suppress(ValueError, IndexError): return args[list(signature(client_method).parameters).index(argument_name) - 1] + with suppress(KeyError): + return signature(client_method).parameters[argument_name].default + raise TypeError("{}() takes no argument called '{}'".format( client_method.__name__, argument_name )) diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index f379849ec..a11adde39 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -139,6 +139,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): data = _attach_current_user({}) return self.delete(endpoint, data) + @cache.delete('templates', 'service_id') def create_service_template(self, name, type_, content, service_id, subject=None, process_type='normal'): """ Create a service template. @@ -158,6 +159,9 @@ class ServiceAPIClient(NotifyAdminAPIClient): endpoint = "/service/{0}/template".format(service_id) return self.post(endpoint, data) + @cache.delete('templates', 'service_id') + @cache.delete('template', 'id_') + @cache.delete('template_versions', 'id_') def update_service_template(self, id_, name, type_, content, service_id, subject=None, process_type=None): """ Update a service template. @@ -181,6 +185,9 @@ class ServiceAPIClient(NotifyAdminAPIClient): endpoint = "/service/{0}/template/{1}".format(service_id, id_) return self.post(endpoint, data) + @cache.delete('templates', 'service_id') + @cache.delete('template', 'id_') + @cache.delete('template_versions', 'id_') def redact_service_template(self, service_id, id_): return self.post( "/service/{}/template/{}".format(service_id, id_), @@ -189,6 +196,9 @@ class ServiceAPIClient(NotifyAdminAPIClient): ), ) + @cache.delete('templates', 'service_id') + @cache.delete('template', 'template_id') + @cache.delete('template_versions', 'template_id') def update_service_template_sender(self, service_id, template_id, reply_to): data = { 'reply_to': reply_to, @@ -199,6 +209,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): data ) + @cache.set('template', 'template_id', 'version') def get_service_template(self, service_id, template_id, version=None): """ Retrieve a service template. @@ -210,6 +221,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): endpoint = '{base}/version/{version}'.format(base=endpoint, version=version) return self.get(endpoint) + @cache.set('template_versions', 'template_id') def get_service_template_versions(self, service_id, template_id): """ Retrieve a list of versions for a template @@ -220,6 +232,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): ) return self.get(endpoint) + @cache.set('templates', 'service_id') def get_service_templates(self, service_id): """ Retrieve all templates for service. @@ -228,6 +241,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): service_id=service_id) return self.get(endpoint) + # This doesn’t need caching because it calls through to a method which is cached def count_service_templates(self, service_id, template_type=None): return len([ template for template in @@ -238,6 +252,9 @@ class ServiceAPIClient(NotifyAdminAPIClient): ) ]) + @cache.delete('templates', 'service_id') + @cache.delete('template', 'template_id') + @cache.delete('template_versions', 'template_id') 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_service_api_client.py b/tests/app/notify_client/test_service_api_client.py index 354d08f16..7b55817c1 100644 --- a/tests/app/notify_client/test_service_api_client.py +++ b/tests/app/notify_client/test_service_api_client.py @@ -6,10 +6,12 @@ from tests.conftest import SERVICE_ONE_ID, fake_uuid from app import invite_api_client, service_api_client, user_api_client from app.notify_client.service_api_client import ServiceAPIClient +FAKE_TEMPLATE_ID = fake_uuid() + def test_client_posts_archived_true_when_deleting_template(mocker): - service_id = fake_uuid - template_id = fake_uuid + service_id = fake_uuid() + template_id = fake_uuid() mocker.patch('app.notify_client.current_user', id='1') expected_data = { @@ -139,6 +141,8 @@ def test_client_returns_count_of_service_templates( @pytest.mark.parametrize( ( + 'client_method,' + 'extra_args,' 'expected_cache_get_calls,' 'cache_value,' 'expected_api_calls,' @@ -147,6 +151,8 @@ def test_client_returns_count_of_service_templates( ), [ ( + service_api_client.get_service, + [SERVICE_ONE_ID], [ call('service-{}'.format(SERVICE_ONE_ID)) ], @@ -156,6 +162,8 @@ def test_client_returns_count_of_service_templates( {'data_from': 'cache'}, ), ( + service_api_client.get_service, + [SERVICE_ONE_ID], [ call('service-{}'.format(SERVICE_ONE_ID)) ], @@ -172,10 +180,72 @@ def test_client_returns_count_of_service_templates( ], {'data_from': 'api'}, ), + ( + service_api_client.get_service_template, + [SERVICE_ONE_ID, FAKE_TEMPLATE_ID], + [ + call('template-{}'.format(FAKE_TEMPLATE_ID)) + ], + b'{"data_from": "cache"}', + [], + [], + {'data_from': 'cache'}, + ), + ( + service_api_client.get_service_template, + [SERVICE_ONE_ID, FAKE_TEMPLATE_ID], + [ + call('template-{}'.format(FAKE_TEMPLATE_ID)) + ], + None, + [ + call('/service/{}/template/{}'.format(SERVICE_ONE_ID, FAKE_TEMPLATE_ID)) + ], + [ + call( + 'template-{}'.format(FAKE_TEMPLATE_ID), + '{"data_from": "api"}', + ex=86400 + ) + ], + {'data_from': 'api'}, + ), + ( + service_api_client.get_service_template, + [SERVICE_ONE_ID, FAKE_TEMPLATE_ID, 1], + [ + call('template-{}-1'.format(FAKE_TEMPLATE_ID)) + ], + b'{"data_from": "cache"}', + [], + [], + {'data_from': 'cache'}, + ), + ( + service_api_client.get_service_template, + [SERVICE_ONE_ID, FAKE_TEMPLATE_ID, 1], + [ + call('template-{}-1'.format(FAKE_TEMPLATE_ID)) + ], + None, + [ + call('/service/{}/template/{}/version/1'.format(SERVICE_ONE_ID, FAKE_TEMPLATE_ID)) + ], + [ + call( + 'template-{}-1'.format(FAKE_TEMPLATE_ID), + '{"data_from": "api"}', + ex=86400 + ) + ], + {'data_from': 'api'}, + ), ] ) def test_returns_value_from_cache( mocker, + client_method, + extra_args, expected_cache_get_calls, cache_value, expected_return_value, @@ -195,7 +265,7 @@ def test_returns_value_from_cache( 'app.notify_client.RedisClient.set', ) - assert service_api_client.get_service(SERVICE_ONE_ID) == expected_return_value + assert client_method(*extra_args) == expected_return_value assert mock_redis_get.call_args_list == expected_cache_get_calls assert mock_api_get.call_args_list == expected_api_calls @@ -240,3 +310,46 @@ def test_deletes_service_cache( assert call('service-{}'.format(SERVICE_ONE_ID)) in mock_redis_delete.call_args_list assert len(mock_request.call_args_list) == 1 + + +@pytest.mark.parametrize('method, extra_args, expected_cache_deletes', [ + ('create_service_template', ['name', 'type_', 'content', SERVICE_ONE_ID], [ + 'templates-{}'.format(SERVICE_ONE_ID), + ]), + ('update_service_template', [FAKE_TEMPLATE_ID, 'foo', 'sms', 'bar', SERVICE_ONE_ID], [ + 'templates-{}'.format(SERVICE_ONE_ID), + 'template-{}'.format(FAKE_TEMPLATE_ID), + 'template_versions-{}'.format(FAKE_TEMPLATE_ID), + ]), + ('redact_service_template', [SERVICE_ONE_ID, FAKE_TEMPLATE_ID], [ + 'templates-{}'.format(SERVICE_ONE_ID), + 'template-{}'.format(FAKE_TEMPLATE_ID), + 'template_versions-{}'.format(FAKE_TEMPLATE_ID), + ]), + ('update_service_template_sender', [SERVICE_ONE_ID, FAKE_TEMPLATE_ID, 'foo'], [ + 'templates-{}'.format(SERVICE_ONE_ID), + 'template-{}'.format(FAKE_TEMPLATE_ID), + 'template_versions-{}'.format(FAKE_TEMPLATE_ID), + ]), + ('delete_service_template', [SERVICE_ONE_ID, FAKE_TEMPLATE_ID], [ + 'templates-{}'.format(SERVICE_ONE_ID), + 'template-{}'.format(FAKE_TEMPLATE_ID), + 'template_versions-{}'.format(FAKE_TEMPLATE_ID), + ]), +]) +def test_deletes_caches_when_modifying_templates( + app_, + mock_get_user, + mocker, + method, + extra_args, + expected_cache_deletes, +): + 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(service_api_client, method)(*extra_args) + + assert mock_redis_delete.call_args_list == list(map(call, expected_cache_deletes)) + assert len(mock_request.call_args_list) == 1 From b28e8691a648bf15783c1edd0b6ab1d8be4cab2a Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 19 Apr 2018 14:01:45 +0100 Subject: [PATCH 4/6] Revert "Remove keyword args from call to create service" This reverts commit bde696cf56d86fc78ecb2b80df7e710d730ec24b. The caching decorator supports keyword arguments now. --- app/main/views/add_service.py | 12 ++++++------ tests/app/main/views/test_add_service.py | 24 ++++++++++++------------ 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/app/main/views/add_service.py b/app/main/views/add_service.py index f1978247f..45314ad20 100644 --- a/app/main/views/add_service.py +++ b/app/main/views/add_service.py @@ -29,12 +29,12 @@ def _create_service(service_name, organisation_type, email_from, form): free_sms_fragment_limit = current_app.config['DEFAULT_FREE_SMS_FRAGMENT_LIMITS'].get(organisation_type) try: service_id = service_api_client.create_service( - service_name, - organisation_type, - current_app.config['DEFAULT_SERVICE_LIMIT'], - True, - session['user_id'], - email_from, + service_name=service_name, + organisation_type=organisation_type, + message_limit=current_app.config['DEFAULT_SERVICE_LIMIT'], + restricted=True, + user_id=session['user_id'], + email_from=email_from, ) session['service_id'] = service_id diff --git a/tests/app/main/views/test_add_service.py b/tests/app/main/views/test_add_service.py index d63cbd7b5..0ed602f9c 100644 --- a/tests/app/main/views/test_add_service.py +++ b/tests/app/main/views/test_add_service.py @@ -43,12 +43,12 @@ def test_should_add_service_and_redirect_to_tour_when_no_services( ) assert mock_get_services_with_no_services.called mock_create_service.assert_called_once_with( - 'testing the post', - 'local', - app_.config['DEFAULT_SERVICE_LIMIT'], - True, - api_user_active.id, - 'testing.the.post' + service_name='testing the post', + organisation_type='local', + message_limit=app_.config['DEFAULT_SERVICE_LIMIT'], + restricted=True, + user_id=api_user_active.id, + email_from='testing.the.post' ) mock_create_service_template.assert_called_once_with( 'Example text message template', @@ -95,12 +95,12 @@ def test_should_add_service_and_redirect_to_dashboard_when_existing_service( ) assert mock_get_services.called mock_create_service.assert_called_once_with( - 'testing the post', - organisation_type, - app_.config['DEFAULT_SERVICE_LIMIT'], - True, - api_user_active.id, - 'testing.the.post' + service_name='testing the post', + organisation_type=organisation_type, + message_limit=app_.config['DEFAULT_SERVICE_LIMIT'], + restricted=True, + user_id=api_user_active.id, + email_from='testing.the.post' ) mock_create_or_update_free_sms_fragment_limit.assert_called_once_with(101, free_allowance) assert len(mock_create_service_template.call_args_list) == 0 From 06de94f1c5f61d3049f4b4c0ebf23b666adc64a0 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 20 Apr 2018 16:32:02 +0100 Subject: [PATCH 5/6] Rewrite cache decorator to use format string MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is easier to read than having to understand the arguments 1…n of the cache decorator are ‘magic’, and gives us more flexibility about how the cache keys are formatted, eg being able to add words in the middle of them. Also changes the key format for all templates to be `service-{service_id}-templates` instead of `templates-{service_id}` because then it’s clearer what the ID represents. --- app/notify_client/cache.py | 35 ++++------ app/notify_client/invite_api_client.py | 4 +- app/notify_client/organisations_api_client.py | 2 +- app/notify_client/service_api_client.py | 70 +++++++++---------- app/notify_client/user_api_client.py | 22 +++--- .../notify_client/test_service_api_client.py | 38 +++++----- 6 files changed, 80 insertions(+), 91 deletions(-) diff --git a/app/notify_client/cache.py b/app/notify_client/cache.py index 7d622e11e..8bdd6e38c 100644 --- a/app/notify_client/cache.py +++ b/app/notify_client/cache.py @@ -7,13 +7,14 @@ from inspect import signature TTL = int(timedelta(hours=24).total_seconds()) -def _get_argument(argument_name, args, kwargs, client_method): +def _get_argument(argument_name, client_method, args, kwargs): with suppress(KeyError): return kwargs[argument_name] with suppress(ValueError, IndexError): - return args[list(signature(client_method).parameters).index(argument_name) - 1] + argument_index = list(signature(client_method).parameters).index(argument_name) + return args[argument_index - 1] # -1 because `args` doesn’t include `self` with suppress(KeyError): return signature(client_method).parameters[argument_name].default @@ -23,32 +24,20 @@ def _get_argument(argument_name, args, kwargs, client_method): )) -def list_of_strings(list_of_stuff): - return list(map(str, filter(None, list_of_stuff))) +def _make_key(key_format, client_method, args, kwargs): + return key_format.format(**{ + argument_name: _get_argument(argument_name, client_method, args, kwargs) + for argument_name in list(signature(client_method).parameters) + }) -def _make_key(prefix, key_from_args, local_variables): - return '-'.join( - [ - local_variables['prefix'] - ] + list_of_strings( - _get_argument( - argument_name, - local_variables['args'], - local_variables['kwargs'], - local_variables['client_method'] - ) for argument_name in key_from_args - ) - ) - - -def set(prefix, *key_from_args): +def set(key_format): def _set(client_method): @wraps(client_method) def new_client_method(client_instance, *args, **kwargs): - redis_key = _make_key(prefix, key_from_args, locals()) + redis_key = _make_key(key_format, client_method, args, kwargs) cached = client_instance.redis_client.get(redis_key) if cached: return json.loads(cached.decode('utf-8')) @@ -64,13 +53,13 @@ def set(prefix, *key_from_args): return _set -def delete(prefix, *key_from_args): +def delete(key_format): def _delete(client_method): @wraps(client_method) def new_client_method(client_instance, *args, **kwargs): - redis_key = _make_key(prefix, key_from_args, locals()) + redis_key = _make_key(key_format, client_method, args, kwargs) client_instance.redis_client.delete(redis_key) return client_method(client_instance, *args, **kwargs) diff --git a/app/notify_client/invite_api_client.py b/app/notify_client/invite_api_client.py index 45e6840f1..ba7434134 100644 --- a/app/notify_client/invite_api_client.py +++ b/app/notify_client/invite_api_client.py @@ -44,8 +44,8 @@ class InviteApiClient(NotifyAdminAPIClient): self.post(url='/service/{0}/invite/{1}'.format(service_id, invited_user_id), data=data) - @cache.delete('service', 'service_id') - @cache.delete('user', 'invited_user_id') + @cache.delete('service-{service_id}') + @cache.delete('user-{invited_user_id}') def accept_invite(self, service_id, invited_user_id): data = {'status': 'accepted'} self.post(url='/service/{0}/invite/{1}'.format(service_id, invited_user_id), diff --git a/app/notify_client/organisations_api_client.py b/app/notify_client/organisations_api_client.py index 6bab54e5a..ce7d39a4b 100644 --- a/app/notify_client/organisations_api_client.py +++ b/app/notify_client/organisations_api_client.py @@ -27,7 +27,7 @@ class OrganisationsClient(NotifyAdminAPIClient): def get_service_organisation(self, service_id): return self.get(url="/service/{}/organisation".format(service_id)) - @cache.delete('service', 'service_id') + @cache.delete('service-{service_id}') def update_service_organisation(self, service_id, org_id): data = { 'service_id': service_id diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index a11adde39..11c70a481 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -9,7 +9,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): def __init__(self): super().__init__("a" * 73, "b") - @cache.delete('user', 'user_id') + @cache.delete('user-{user_id}') def create_service( self, service_name, @@ -34,7 +34,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): data = _attach_current_user(data) return self.post("/service", data)['data']['id'] - @cache.set('service', 'service_id') + @cache.set('service-{service_id}') def get_service(self, service_id): return self._get_service(service_id, detailed=False, today_only=False) @@ -74,7 +74,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): params_dict['only_active'] = True return self.get_services(params_dict) - @cache.delete('service', 'service_id') + @cache.delete('service-{service_id}') def update_service( self, service_id, @@ -115,20 +115,20 @@ class ServiceAPIClient(NotifyAdminAPIClient): def update_service_with_properties(self, service_id, properties): return self.update_service(service_id, **properties) - @cache.delete('service', 'service_id') + @cache.delete('service-{service_id}') def archive_service(self, service_id): return self.post('/service/{}/archive'.format(service_id), data=None) - @cache.delete('service', 'service_id') + @cache.delete('service-{service_id}') def suspend_service(self, service_id): return self.post('/service/{}/suspend'.format(service_id), data=None) - @cache.delete('service', 'service_id') + @cache.delete('service-{service_id}') def resume_service(self, service_id): return self.post('/service/{}/resume'.format(service_id), data=None) - @cache.delete('service', 'service_id') - @cache.delete('user', 'user_id') + @cache.delete('service-{service_id}') + @cache.delete('user-{user_id}') def remove_user_from_service(self, service_id, user_id): """ Remove a user from a service @@ -139,7 +139,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): data = _attach_current_user({}) return self.delete(endpoint, data) - @cache.delete('templates', 'service_id') + @cache.delete('service-{service_id}-templates') def create_service_template(self, name, type_, content, service_id, subject=None, process_type='normal'): """ Create a service template. @@ -159,9 +159,9 @@ class ServiceAPIClient(NotifyAdminAPIClient): endpoint = "/service/{0}/template".format(service_id) return self.post(endpoint, data) - @cache.delete('templates', 'service_id') - @cache.delete('template', 'id_') - @cache.delete('template_versions', 'id_') + @cache.delete('service-{service_id}-templates') + @cache.delete('template-{id_}-version-None') + @cache.delete('template-{id_}-versions') def update_service_template(self, id_, name, type_, content, service_id, subject=None, process_type=None): """ Update a service template. @@ -185,9 +185,9 @@ class ServiceAPIClient(NotifyAdminAPIClient): endpoint = "/service/{0}/template/{1}".format(service_id, id_) return self.post(endpoint, data) - @cache.delete('templates', 'service_id') - @cache.delete('template', 'id_') - @cache.delete('template_versions', 'id_') + @cache.delete('service-{service_id}-templates') + @cache.delete('template-{id_}-version-None') + @cache.delete('template-{id_}-versions') def redact_service_template(self, service_id, id_): return self.post( "/service/{}/template/{}".format(service_id, id_), @@ -196,9 +196,9 @@ class ServiceAPIClient(NotifyAdminAPIClient): ), ) - @cache.delete('templates', 'service_id') - @cache.delete('template', 'template_id') - @cache.delete('template_versions', 'template_id') + @cache.delete('service-{service_id}-templates') + @cache.delete('template-{template_id}-version-None') + @cache.delete('template-{template_id}-versions') def update_service_template_sender(self, service_id, template_id, reply_to): data = { 'reply_to': reply_to, @@ -209,7 +209,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): data ) - @cache.set('template', 'template_id', 'version') + @cache.set('template-{template_id}-version-{version}') def get_service_template(self, service_id, template_id, version=None): """ Retrieve a service template. @@ -221,7 +221,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): endpoint = '{base}/version/{version}'.format(base=endpoint, version=version) return self.get(endpoint) - @cache.set('template_versions', 'template_id') + @cache.set('template-{template_id}-versions') def get_service_template_versions(self, service_id, template_id): """ Retrieve a list of versions for a template @@ -232,7 +232,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): ) return self.get(endpoint) - @cache.set('templates', 'service_id') + @cache.set('service-{service_id}-templates') def get_service_templates(self, service_id): """ Retrieve all templates for service. @@ -252,9 +252,9 @@ class ServiceAPIClient(NotifyAdminAPIClient): ) ]) - @cache.delete('templates', 'service_id') - @cache.delete('template', 'template_id') - @cache.delete('template_versions', 'template_id') + @cache.delete('service-{service_id}-templates') + @cache.delete('template-{template_id}-version-None') + @cache.delete('template-{template_id}-versions') def delete_service_template(self, service_id, template_id): """ Set a service template's archived flag to True @@ -284,7 +284,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): def get_whitelist(self, service_id): return self.get(url='/service/{}/whitelist'.format(service_id)) - @cache.delete('service', 'service_id') + @cache.delete('service-{service_id}') def update_whitelist(self, service_id, data): return self.put(url='/service/{}/whitelist'.format(service_id), data=data) @@ -322,7 +322,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): '/service/{}/inbound-sms/summary'.format(service_id) ) - @cache.delete('service', 'service_id') + @cache.delete('service-{service_id}') def create_service_inbound_api(self, service_id, url, bearer_token, user_id): data = { "url": url, @@ -331,7 +331,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): } return self.post("/service/{}/inbound-api".format(service_id), data) - @cache.delete('service', 'service_id') + @cache.delete('service-{service_id}') def update_service_inbound_api(self, service_id, url, bearer_token, user_id, inbound_api_id): data = { "url": url, @@ -363,7 +363,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): ) ) - @cache.delete('service', 'service_id') + @cache.delete('service-{service_id}') def add_reply_to_email_address(self, service_id, email_address, is_default=False): return self.post( "/service/{}/email-reply-to".format(service_id), @@ -373,7 +373,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): } ) - @cache.delete('service', 'service_id') + @cache.delete('service-{service_id}') def update_reply_to_email_address(self, service_id, reply_to_email_id, email_address, is_default=False): return self.post( "/service/{}/email-reply-to/{}".format( @@ -392,7 +392,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): def get_letter_contact(self, service_id, letter_contact_id): return self.get("/service/{}/letter-contact/{}".format(service_id, letter_contact_id)) - @cache.delete('service', 'service_id') + @cache.delete('service-{service_id}') def add_letter_contact(self, service_id, contact_block, is_default=False): return self.post( "/service/{}/letter-contact".format(service_id), @@ -402,7 +402,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): } ) - @cache.delete('service', 'service_id') + @cache.delete('service-{service_id}') def update_letter_contact(self, service_id, letter_contact_id, contact_block, is_default=False): return self.post( "/service/{}/letter-contact/{}".format( @@ -428,7 +428,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): "/service/{}/sms-sender/{}".format(service_id, sms_sender_id) ) - @cache.delete('service', 'service_id') + @cache.delete('service-{service_id}') def add_sms_sender(self, service_id, sms_sender, is_default=False, inbound_number_id=None): data = { "sms_sender": sms_sender, @@ -438,7 +438,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): data["inbound_number_id"] = inbound_number_id return self.post("/service/{}/sms-sender".format(service_id), data=data) - @cache.delete('service', 'service_id') + @cache.delete('service-{service_id}') def update_sms_sender(self, service_id, sms_sender_id, sms_sender, is_default=False): return self.post( "/service/{}/sms-sender/{}".format(service_id, sms_sender_id), @@ -455,7 +455,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): ) )['data'] - @cache.delete('service', 'service_id') + @cache.delete('service-{service_id}') def update_service_callback_api(self, service_id, url, bearer_token, user_id, callback_api_id): data = { "url": url, @@ -465,7 +465,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): data['bearer_token'] = bearer_token return self.post("/service/{}/delivery-receipt-api/{}".format(service_id, callback_api_id), data) - @cache.delete('service', 'service_id') + @cache.delete('service-{service_id}') def create_service_callback_api(self, service_id, url, bearer_token, user_id): data = { "url": url, diff --git a/app/notify_client/user_api_client.py b/app/notify_client/user_api_client.py index e5fbcfae6..c0e3a28ef 100644 --- a/app/notify_client/user_api_client.py +++ b/app/notify_client/user_api_client.py @@ -39,7 +39,7 @@ class UserApiClient(NotifyAdminAPIClient): 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', 'user_id') + @cache.set('user-{user_id}') def _get_user(self, user_id): return self.get("/user/{}".format(user_id)) @@ -61,7 +61,7 @@ class UserApiClient(NotifyAdminAPIClient): users.append(User(user, max_failed_login_count=self.max_failed_login_count)) return users - @cache.delete('user', 'user_id') + @cache.delete('user-{user_id}') def update_user_attribute(self, user_id, **kwargs): data = dict(kwargs) disallowed_attributes = set(data.keys()) - ALLOWED_ATTRIBUTES @@ -75,20 +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', 'user_id') + @cache.delete('user-{user_id}') 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', 'user_id') + @cache.delete('user-{user_id}') 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', 'user_id') + @cache.delete('user-{user_id}') def verify_password(self, user_id, password): try: url = "/user/{}/verify/password".format(user_id) @@ -118,7 +118,7 @@ class UserApiClient(NotifyAdminAPIClient): endpoint = '/user/{0}/email-already-registered'.format(user_id) self.post(endpoint, data=data) - @cache.delete('user', 'user_id') + @cache.delete('user-{user_id}') def check_verify_code(self, user_id, code, code_type): data = {'code_type': code_type, 'code': code} endpoint = '/user/{}/verify/code'.format(user_id) @@ -154,20 +154,20 @@ class UserApiClient(NotifyAdminAPIClient): resp = self.get(endpoint) return [User(data) for data in resp['data']] - @cache.delete('service', 'service_id') - @cache.delete('user', 'user_id') + @cache.delete('service-{service_id}') + @cache.delete('user-{user_id}') 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', 'user_id') + @cache.delete('user-{user_id}') 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', 'user_id') + @cache.delete('user-{user_id}') 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)] @@ -191,7 +191,7 @@ class UserApiClient(NotifyAdminAPIClient): else: return user - @cache.delete('user', 'user_id') + @cache.delete('user-{user_id}') def _activate_user(self, user_id): return self.post("/user/{}/activate".format(user_id), data=None) diff --git a/tests/app/notify_client/test_service_api_client.py b/tests/app/notify_client/test_service_api_client.py index 7b55817c1..60748c818 100644 --- a/tests/app/notify_client/test_service_api_client.py +++ b/tests/app/notify_client/test_service_api_client.py @@ -184,7 +184,7 @@ def test_client_returns_count_of_service_templates( service_api_client.get_service_template, [SERVICE_ONE_ID, FAKE_TEMPLATE_ID], [ - call('template-{}'.format(FAKE_TEMPLATE_ID)) + call('template-{}-version-None'.format(FAKE_TEMPLATE_ID)) ], b'{"data_from": "cache"}', [], @@ -195,7 +195,7 @@ def test_client_returns_count_of_service_templates( service_api_client.get_service_template, [SERVICE_ONE_ID, FAKE_TEMPLATE_ID], [ - call('template-{}'.format(FAKE_TEMPLATE_ID)) + call('template-{}-version-None'.format(FAKE_TEMPLATE_ID)) ], None, [ @@ -203,7 +203,7 @@ def test_client_returns_count_of_service_templates( ], [ call( - 'template-{}'.format(FAKE_TEMPLATE_ID), + 'template-{}-version-None'.format(FAKE_TEMPLATE_ID), '{"data_from": "api"}', ex=86400 ) @@ -214,7 +214,7 @@ def test_client_returns_count_of_service_templates( service_api_client.get_service_template, [SERVICE_ONE_ID, FAKE_TEMPLATE_ID, 1], [ - call('template-{}-1'.format(FAKE_TEMPLATE_ID)) + call('template-{}-version-1'.format(FAKE_TEMPLATE_ID)) ], b'{"data_from": "cache"}', [], @@ -225,7 +225,7 @@ def test_client_returns_count_of_service_templates( service_api_client.get_service_template, [SERVICE_ONE_ID, FAKE_TEMPLATE_ID, 1], [ - call('template-{}-1'.format(FAKE_TEMPLATE_ID)) + call('template-{}-version-1'.format(FAKE_TEMPLATE_ID)) ], None, [ @@ -233,7 +233,7 @@ def test_client_returns_count_of_service_templates( ], [ call( - 'template-{}-1'.format(FAKE_TEMPLATE_ID), + 'template-{}-version-1'.format(FAKE_TEMPLATE_ID), '{"data_from": "api"}', ex=86400 ) @@ -314,27 +314,27 @@ def test_deletes_service_cache( @pytest.mark.parametrize('method, extra_args, expected_cache_deletes', [ ('create_service_template', ['name', 'type_', 'content', SERVICE_ONE_ID], [ - 'templates-{}'.format(SERVICE_ONE_ID), + 'service-{}-templates'.format(SERVICE_ONE_ID), ]), ('update_service_template', [FAKE_TEMPLATE_ID, 'foo', 'sms', 'bar', SERVICE_ONE_ID], [ - 'templates-{}'.format(SERVICE_ONE_ID), - 'template-{}'.format(FAKE_TEMPLATE_ID), - 'template_versions-{}'.format(FAKE_TEMPLATE_ID), + 'service-{}-templates'.format(SERVICE_ONE_ID), + 'template-{}-version-None'.format(FAKE_TEMPLATE_ID), + 'template-{}-versions'.format(FAKE_TEMPLATE_ID), ]), ('redact_service_template', [SERVICE_ONE_ID, FAKE_TEMPLATE_ID], [ - 'templates-{}'.format(SERVICE_ONE_ID), - 'template-{}'.format(FAKE_TEMPLATE_ID), - 'template_versions-{}'.format(FAKE_TEMPLATE_ID), + 'service-{}-templates'.format(SERVICE_ONE_ID), + 'template-{}-version-None'.format(FAKE_TEMPLATE_ID), + 'template-{}-versions'.format(FAKE_TEMPLATE_ID), ]), ('update_service_template_sender', [SERVICE_ONE_ID, FAKE_TEMPLATE_ID, 'foo'], [ - 'templates-{}'.format(SERVICE_ONE_ID), - 'template-{}'.format(FAKE_TEMPLATE_ID), - 'template_versions-{}'.format(FAKE_TEMPLATE_ID), + 'service-{}-templates'.format(SERVICE_ONE_ID), + 'template-{}-version-None'.format(FAKE_TEMPLATE_ID), + 'template-{}-versions'.format(FAKE_TEMPLATE_ID), ]), ('delete_service_template', [SERVICE_ONE_ID, FAKE_TEMPLATE_ID], [ - 'templates-{}'.format(SERVICE_ONE_ID), - 'template-{}'.format(FAKE_TEMPLATE_ID), - 'template_versions-{}'.format(FAKE_TEMPLATE_ID), + 'service-{}-templates'.format(SERVICE_ONE_ID), + 'template-{}-version-None'.format(FAKE_TEMPLATE_ID), + 'template-{}-versions'.format(FAKE_TEMPLATE_ID), ]), ]) def test_deletes_caches_when_modifying_templates( From 222a67959aafd2dbb0598ee18dce0644031304ec Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 20 Apr 2018 17:29:06 +0100 Subject: [PATCH 6/6] Add tests for all templates and template versions --- .../notify_client/test_service_api_client.py | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/tests/app/notify_client/test_service_api_client.py b/tests/app/notify_client/test_service_api_client.py index 60748c818..ef4a9d318 100644 --- a/tests/app/notify_client/test_service_api_client.py +++ b/tests/app/notify_client/test_service_api_client.py @@ -240,6 +240,66 @@ def test_client_returns_count_of_service_templates( ], {'data_from': 'api'}, ), + ( + service_api_client.get_service_templates, + [SERVICE_ONE_ID], + [ + call('service-{}-templates'.format(SERVICE_ONE_ID)) + ], + b'{"data_from": "cache"}', + [], + [], + {'data_from': 'cache'}, + ), + ( + service_api_client.get_service_templates, + [SERVICE_ONE_ID], + [ + call('service-{}-templates'.format(SERVICE_ONE_ID)) + ], + None, + [ + call('/service/{}/template'.format(SERVICE_ONE_ID)) + ], + [ + call( + 'service-{}-templates'.format(SERVICE_ONE_ID), + '{"data_from": "api"}', + ex=86400 + ) + ], + {'data_from': 'api'}, + ), + ( + service_api_client.get_service_template_versions, + [SERVICE_ONE_ID, FAKE_TEMPLATE_ID], + [ + call('template-{}-versions'.format(FAKE_TEMPLATE_ID)) + ], + b'{"data_from": "cache"}', + [], + [], + {'data_from': 'cache'}, + ), + ( + service_api_client.get_service_template_versions, + [SERVICE_ONE_ID, FAKE_TEMPLATE_ID], + [ + call('template-{}-versions'.format(FAKE_TEMPLATE_ID)) + ], + None, + [ + call('/service/{}/template/{}/versions'.format(SERVICE_ONE_ID, FAKE_TEMPLATE_ID)) + ], + [ + call( + 'template-{}-versions'.format(FAKE_TEMPLATE_ID), + '{"data_from": "api"}', + ex=86400 + ) + ], + {'data_from': 'api'}, + ), ] ) def test_returns_value_from_cache(