From 8396412ce12b5465c3cc5b1d5435ab7f21a9881e Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 21 Feb 2022 10:37:51 +0000 Subject: [PATCH 1/3] Report all cache keys that were deleted This will make it easier to add another test / feature to clear all the cache keys. It's debatable which of "sum" and "max" is useful: - "max" is a better (although still not accurate) indicator of the number of "things" affected e.g. templates, services, etc. - "sum" makes sense in places where "max" doesn't e.g. when we clear the "organisations" group, which doesn't equate to individual orgs. Using "sum() ... across" seems like a reasonable compromise and makes it clear that we're iterating over different kinds of keys. While the pluralisation is nice, I don't think it's worth the effort to make it work for both "object(s)" and "format(s)". --- app/main/views/platform_admin.py | 12 ++++++----- tests/app/main/views/test_platform_admin.py | 22 ++++++++++----------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/app/main/views/platform_admin.py b/app/main/views/platform_admin.py index 37d97e9e2..1cfbeab97 100644 --- a/app/main/views/platform_admin.py +++ b/app/main/views/platform_admin.py @@ -423,14 +423,16 @@ def clear_cache(): form.model_type.choices = [(key, key.replace('_', ' ').title()) for key in CACHE_KEYS] if form.validate_on_submit(): - to_delete = form.model_type.data + group_key = form.model_type.data + group = CACHE_KEYS[group_key] - num_deleted = max( + num_deleted = sum( redis_client.delete_cache_keys_by_pattern(pattern) - for pattern in CACHE_KEYS[to_delete] + for pattern in group ) - msg = 'Removed {} {} object{} from redis' - flash(msg.format(num_deleted, to_delete, 's' if num_deleted != 1 else ''), category='default') + + msg = f'Removed {num_deleted} {group_key} objects across {len(group)} key formats' + flash(msg, category='default') return render_template( 'views/platform-admin/clear-cache.html', diff --git a/tests/app/main/views/test_platform_admin.py b/tests/app/main/views/test_platform_admin.py index 4c19466e5..305df1106 100644 --- a/tests/app/main/views/test_platform_admin.py +++ b/tests/app/main/views/test_platform_admin.py @@ -745,14 +745,10 @@ def test_clear_cache_shows_form( @pytest.mark.parametrize('model_type, expected_calls, expected_confirmation', ( ('template', [ - # Returns 101 call('service-????????-????-????-????-????????????-templates'), - # Returns 102 call('service-????????-????-????-????-????????????-template-????????-????-????-????-????????????-version-*'), - # Returns 103 call('service-????????-????-????-????-????????????-template-????????-????-????-????-????????????-versions'), - # 103 shown here because it’s the `max` of the 3 counts - ], 'Removed 103 template objects from redis'), + ], 'Removed 6 template objects across 3 key formats'), ('service', [ call('has_jobs-????????-????-????-????-????????????'), call('service-????????-????-????-????-????????????'), @@ -761,16 +757,16 @@ def test_clear_cache_shows_form( call('service-????????-????-????-????-????????????-template-folders'), call('service-????????-????-????-????-????????????-returned-letters-statistics'), call('service-????????-????-????-????-????????????-returned-letters-summary'), - ], 'Removed 107 service objects from redis'), + ], 'Removed 14 service objects across 7 key formats'), ('organisation', [ call('organisations'), call('domains'), call('live-service-and-organisation-counts'), call('organisation-????????-????-????-????-????????????-name'), - ], 'Removed 104 organisation objects from redis'), + ], 'Removed 8 organisation objects across 4 key formats'), ('broadcast', [ call('service-????????-????-????-????-????????????-broadcast-message-????????-????-????-????-????????????'), - ], 'Removed 101 broadcast objects from redis'), + ], 'Removed 2 broadcast objects across 1 key formats'), )) def test_clear_cache_submits_and_tells_you_how_many_things_were_deleted( client_request, @@ -781,12 +777,14 @@ def test_clear_cache_submits_and_tells_you_how_many_things_were_deleted( expected_confirmation, ): redis = mocker.patch('app.main.views.platform_admin.redis_client') - # The way this is set up means the first time `delete_cache_keys_by_pattern` - # is called it will return `101`, the second time it will return `102`, etc - redis.delete_cache_keys_by_pattern.side_effect = [101, 102, 103, 104, 105, 106, 107, 108, 109] + redis.delete_cache_keys_by_pattern.return_value = 2 client_request.login(platform_admin_user) - page = client_request.post('main.clear_cache', _data={'model_type': model_type}, _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 == expected_calls From 16a14cd6428b404777d5fac7c5de56560ffedc95 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 21 Feb 2022 11:19:50 +0000 Subject: [PATCH 2/3] Rewrite test for clear cache radio buttons This was missing an existing option to clear for broadcasts. --- tests/app/main/views/test_platform_admin.py | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/tests/app/main/views/test_platform_admin.py b/tests/app/main/views/test_platform_admin.py index 305df1106..e965e7667 100644 --- a/tests/app/main/views/test_platform_admin.py +++ b/tests/app/main/views/test_platform_admin.py @@ -734,13 +734,18 @@ def test_clear_cache_shows_form( page = client_request.get('main.clear_cache') - 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 + radios = {el['value'] for el in page.select('input[type=radio]')} + + assert radios == { + 'user', + 'service', + 'template', + 'email_branding', + 'letter_branding', + 'organisation', + 'broadcast' + } @pytest.mark.parametrize('model_type, expected_calls, expected_confirmation', ( From ebbfd204726946a44da8b6c4b78fb98e9ed99194 Mon Sep 17 00:00:00 2001 From: Ben Thorner Date: Mon, 21 Feb 2022 11:28:15 +0000 Subject: [PATCH 3/3] Make it easy to clear cache for all key formats Having to submit the form for each choice separately slowed us down during an incident where Redis was unavailable and came back with stale data, which we had to clear manually. Note: we don't want to use the "flush" feature in case there are other keys in Redis, which may not be safe to remove. --- app/main/forms.py | 6 +++++- app/main/views/platform_admin.py | 19 ++++++++++++++----- tests/app/main/views/test_platform_admin.py | 14 ++++++-------- 3 files changed, 25 insertions(+), 14 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index e76955779..f6f7412a2 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -2445,10 +2445,14 @@ class TemplateAndFoldersSelectionForm(Form): class ClearCacheForm(StripWhitespaceForm): - model_type = GovukRadiosField( + model_type = GovukCheckboxesField( 'What do you want to clear today', ) + def validate_model_type(self, field): + if not field.data: + raise ValidationError('Select at least one option') + class GoLiveNotesForm(StripWhitespaceForm): request_to_go_live_notes = TextAreaField( diff --git a/app/main/views/platform_admin.py b/app/main/views/platform_admin.py index 1cfbeab97..10e43fb4b 100644 --- a/app/main/views/platform_admin.py +++ b/app/main/views/platform_admin.py @@ -420,18 +420,27 @@ def clear_cache(): ]) form = ClearCacheForm() - form.model_type.choices = [(key, key.replace('_', ' ').title()) for key in CACHE_KEYS] + + form.model_type.choices = [ + (key, key.replace('_', ' ').title()) for key in CACHE_KEYS + ] if form.validate_on_submit(): - group_key = form.model_type.data - group = CACHE_KEYS[group_key] + group_keys = form.model_type.data + groups = map(CACHE_KEYS.get, group_keys) + patterns = list(itertools.chain(*groups)) num_deleted = sum( redis_client.delete_cache_keys_by_pattern(pattern) - for pattern in group + for pattern in patterns + ) + + msg = ( + f'Removed {num_deleted} objects ' + f'across {len(patterns)} key formats ' + f'for {", ".join(group_keys)}' ) - msg = f'Removed {num_deleted} {group_key} objects across {len(group)} key formats' flash(msg, category='default') return render_template( diff --git a/tests/app/main/views/test_platform_admin.py b/tests/app/main/views/test_platform_admin.py index e965e7667..fb4cba007 100644 --- a/tests/app/main/views/test_platform_admin.py +++ b/tests/app/main/views/test_platform_admin.py @@ -735,7 +735,7 @@ def test_clear_cache_shows_form( page = client_request.get('main.clear_cache') assert not redis.delete_cache_keys_by_pattern.called - radios = {el['value'] for el in page.select('input[type=radio]')} + radios = {el['value'] for el in page.select('input[type=checkbox]')} assert radios == { 'user', @@ -753,8 +753,8 @@ def test_clear_cache_shows_form( call('service-????????-????-????-????-????????????-templates'), call('service-????????-????-????-????-????????????-template-????????-????-????-????-????????????-version-*'), call('service-????????-????-????-????-????????????-template-????????-????-????-????-????????????-versions'), - ], 'Removed 6 template objects across 3 key formats'), - ('service', [ + ], 'Removed 6 objects across 3 key formats for template'), + (['service', 'organisation'], [ call('has_jobs-????????-????-????-????-????????????'), call('service-????????-????-????-????-????????????'), call('service-????????-????-????-????-????????????-templates'), @@ -762,16 +762,14 @@ def test_clear_cache_shows_form( call('service-????????-????-????-????-????????????-template-folders'), call('service-????????-????-????-????-????????????-returned-letters-statistics'), call('service-????????-????-????-????-????????????-returned-letters-summary'), - ], 'Removed 14 service objects across 7 key formats'), - ('organisation', [ call('organisations'), call('domains'), call('live-service-and-organisation-counts'), call('organisation-????????-????-????-????-????????????-name'), - ], 'Removed 8 organisation objects across 4 key formats'), + ], 'Removed 22 objects across 11 key formats for service, organisation'), ('broadcast', [ call('service-????????-????-????-????-????????????-broadcast-message-????????-????-????-????-????????????'), - ], 'Removed 2 broadcast objects across 1 key formats'), + ], 'Removed 2 objects across 1 key formats for broadcast'), )) def test_clear_cache_submits_and_tells_you_how_many_things_were_deleted( client_request, @@ -807,7 +805,7 @@ def test_clear_cache_requires_option( page = client_request.post('main.clear_cache', _data={}, _expected_status=200) - assert normalize_spaces(page.find('span', class_='govuk-error-message').text) == 'Error: Select an option' + assert normalize_spaces(page.find('span', class_='govuk-error-message').text) == 'Error: Select at least one option' assert not redis.delete_cache_keys_by_pattern.called