From 8f9ade7a8b775b8497e525adf0c50ea58a574159 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 14 Jun 2019 11:10:57 +0100 Subject: [PATCH] Cache organisations, not just domains in Redis MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This should make the ‘All organisations’ page load a lil’ bit quicker. Still worth caching the domains separately so the response is smaller when we only care about domains. This is because the code that uses the domains is part of the sign up flow, so it’s really important that it’s snappy. --- app/main/views/platform_admin.py | 7 ++- app/notify_client/organisations_api_client.py | 3 + tests/app/main/views/test_platform_admin.py | 34 ++++++++--- .../notify_client/test_organisation_client.py | 58 +++++++++++++++++-- 4 files changed, 87 insertions(+), 15 deletions(-) diff --git a/app/main/views/platform_admin.py b/app/main/views/platform_admin.py index 47dddc90f..a5f335bc2 100644 --- a/app/main/views/platform_admin.py +++ b/app/main/views/platform_admin.py @@ -417,7 +417,12 @@ def clear_cache(): ('letter_branding', [ 'letter_branding', 'letter_branding-????????-????-????-????-????????????', - ]) + ]), + ('organisation', [ + 'organisations', + 'domains', + 'live-service-and-organisation-counts', + ]), ]) form = ClearCacheForm() diff --git a/app/notify_client/organisations_api_client.py b/app/notify_client/organisations_api_client.py index ecec12359..c51982cdd 100644 --- a/app/notify_client/organisations_api_client.py +++ b/app/notify_client/organisations_api_client.py @@ -7,6 +7,7 @@ from app.notify_client import NotifyAdminAPIClient, _attach_current_user, cache class OrganisationsClient(NotifyAdminAPIClient): + @cache.set('organisations') def get_organisations(self): return self.get(url='/organisations') @@ -30,6 +31,7 @@ class OrganisationsClient(NotifyAdminAPIClient): return None raise error + @cache.delete('organisations') def create_organisation(self, name): data = { "name": name @@ -37,6 +39,7 @@ class OrganisationsClient(NotifyAdminAPIClient): return self.post(url="/organisations", data=data) @cache.delete('domains') + @cache.delete('organisations') def update_organisation(self, org_id, **kwargs): return self.post(url="/organisations/{}".format(org_id), data=kwargs) diff --git a/tests/app/main/views/test_platform_admin.py b/tests/app/main/views/test_platform_admin.py index 2e0b5a765..ac05a3e7e 100644 --- a/tests/app/main/views/test_platform_admin.py +++ b/tests/app/main/views/test_platform_admin.py @@ -900,24 +900,42 @@ def test_clear_cache_shows_form(client_request, platform_admin_user, mocker): assert page.select('input[type=radio]')[0]['value'] == 'user' assert page.select('input[type=radio]')[1]['value'] == 'service' assert page.select('input[type=radio]')[2]['value'] == 'template' + assert page.select('input[type=radio]')[3]['value'] == 'email_branding' + assert page.select('input[type=radio]')[4]['value'] == 'letter_branding' + assert page.select('input[type=radio]')[5]['value'] == 'organisation' assert not redis.delete_cache_keys_by_pattern.called -def test_clear_cache_submits_and_tells_you_how_many_things_were_deleted(client_request, platform_admin_user, mocker): +@pytest.mark.parametrize('model_type, expected_calls, expected_confirmation', ( + ('template', [ + call('service-????????-????-????-????-????????????-templates'), + call('template-????????-????-????-????-????????????-version-*'), + call('template-????????-????-????-????-????????????-versions'), + ], 'Removed 3 template objects from redis'), + ('organisation', [ + call('organisations'), + call('domains'), + call('live-service-and-organisation-counts'), + ], 'Removed 3 organisation objects from redis'), +)) +def test_clear_cache_submits_and_tells_you_how_many_things_were_deleted( + client_request, + platform_admin_user, + mocker, + model_type, + expected_calls, + expected_confirmation, +): redis = mocker.patch('app.main.views.platform_admin.redis_client') redis.delete_cache_keys_by_pattern.side_effect = [0, 3, 1] client_request.login(platform_admin_user) - page = client_request.post('main.clear_cache', _data={'model_type': 'template'}, _expected_status=200) + page = client_request.post('main.clear_cache', _data={'model_type': model_type}, _expected_status=200) - assert redis.delete_cache_keys_by_pattern.call_args_list == [ - call('service-????????-????-????-????-????????????-templates'), - call('template-????????-????-????-????-????????????-version-*'), - call('template-????????-????-????-????-????????????-versions'), - ] + assert redis.delete_cache_keys_by_pattern.call_args_list == expected_calls flash_banner = page.find('div', class_='banner-default') - assert flash_banner.text.strip() == 'Removed 3 template objects from redis' + assert flash_banner.text.strip() == expected_confirmation def test_clear_cache_requires_option(client_request, platform_admin_user, mocker): diff --git a/tests/app/notify_client/test_organisation_client.py b/tests/app/notify_client/test_organisation_client.py index 7839cbdad..d0cc36612 100644 --- a/tests/app/notify_client/test_organisation_client.py +++ b/tests/app/notify_client/test_organisation_client.py @@ -7,6 +7,7 @@ from app import organisations_client @pytest.mark.parametrize( ( + 'client_method,' 'expected_cache_get_calls,' 'cache_value,' 'expected_api_calls,' @@ -15,13 +16,14 @@ from app import organisations_client ), [ ( + 'get_domains', [ - call('domains') + call('domains'), ], b""" [ - {"domains": ["a", "b", "c"]}, - {"domains": ["c", "d", "e"]} + {"name": "org 1", "domains": ["a", "b", "c"]}, + {"name": "org 2", "domains": ["c", "d", "e"]} ] """, [], @@ -29,19 +31,62 @@ from app import organisations_client ['a', 'b', 'c', 'd', 'e'], ), ( + 'get_domains', [ - call('domains') + call('domains'), + call('organisations'), ], None, [ call(url='/organisations') ], [ + call( + 'organisations', + '[{"domains": ["x", "y", "z"]}]', + ex=604800, + ), call( 'domains', '["x", "y", "z"]', ex=604800 - ) + ), + ], + 'from api', + ), + ( + 'get_organisations', + [ + call('organisations'), + ], + b""" + [ + {"name": "org 1", "domains": ["a", "b", "c"]}, + {"name": "org 2", "domains": ["c", "d", "e"]} + ] + """, + [], + [], + [ + {"name": "org 1", "domains": ["a", "b", "c"]}, + {"name": "org 2", "domains": ["c", "d", "e"]} + ], + ), + ( + 'get_organisations', + [ + call('organisations'), + ], + None, + [ + call(url='/organisations') + ], + [ + call( + 'organisations', + '[{"domains": ["x", "y", "z"]}]', + ex=604800, + ), ], 'from api', ), @@ -50,6 +95,7 @@ from app import organisations_client def test_returns_value_from_cache( app_, mocker, + client_method, expected_cache_get_calls, cache_value, expected_return_value, @@ -71,7 +117,7 @@ def test_returns_value_from_cache( 'app.extensions.RedisClient.set', ) - organisations_client.get_domains() + getattr(organisations_client, client_method)() assert mock_redis_get.call_args_list == expected_cache_get_calls assert mock_api_get.call_args_list == expected_api_calls