From f6367f22783f322cdcd189485cb50a03d54f3b0d Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 14 Feb 2019 14:25:31 +0000 Subject: [PATCH 1/6] move (non-api) clients (inc redis) from app/__init__.py to extensions when clients are defined in app/__init__.py, it increases the chance of cyclical imports. By moving module level client singletons out to a separate extensions file, we stop cyclical imports, but keep the same code flow - the clients are still initialised in `create_app` in `__init__.py`. The redis client in particular is no longer separate - previously redis was set up on the `NotifyAdminAPIClient` base class, but now there's one singleton in `app.extensions`. This was done so that we can access redis from outside of the existing clients. --- app/__init__.py | 20 +++++++++++-------- app/extensions.py | 11 ++++++++++ app/main/views/feedback.py | 8 ++------ app/main/views/platform_admin.py | 2 +- app/main/views/service_settings.py | 2 +- app/notify_client/__init__.py | 5 ----- app/notify_client/cache.py | 8 +++++--- app/notify_client/job_api_client.py | 4 ++-- .../template_folder_api_client.py | 3 ++- .../test_email_branding_client.py | 12 +++++------ tests/app/notify_client/test_job_client.py | 6 +++--- .../test_letter_branding_client.py | 12 +++++------ .../notify_client/test_service_api_client.py | 8 ++++---- .../test_template_folder_client.py | 12 +++++------ tests/app/notify_client/test_user_client.py | 6 +++--- 15 files changed, 64 insertions(+), 55 deletions(-) create mode 100644 app/extensions.py diff --git a/app/__init__.py b/app/__init__.py index 8f45d5b1d..0a2ef8724 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -25,9 +25,6 @@ from functools import partial from notifications_python_client.errors import HTTPError from notifications_utils import logging, request_helper, formatters -from notifications_utils.clients.antivirus.antivirus_client import AntivirusClient -from notifications_utils.clients.zendesk.zendesk_client import ZendeskClient -from notifications_utils.clients.statsd.statsd_client import StatsdClient from notifications_utils.recipients import ( validate_phone_number, InvalidPhoneError, @@ -42,6 +39,12 @@ from werkzeug.local import LocalProxy from app import proxy_fix from app.config import configs from app.asset_fingerprinter import asset_fingerprinter +from app.extensions import ( + antivirus_client, + statsd_client, + zendesk_client, + redis_client, +) from app.models.service import Service from app.models.user import AnonymousUser from app.navigation import ( @@ -75,9 +78,6 @@ from app.utils import get_logo_cdn_domain, id_safe login_manager = LoginManager() csrf = CSRFProtect() -antivirus_client = AntivirusClient() -statsd_client = StatsdClient() -zendesk_client = ZendeskClient() # The current service attached to the request stack. @@ -116,8 +116,7 @@ def create_app(application): proxy_fix, request_helper, - # Internal API clients - antivirus_client, + # API clients api_key_api_client, billing_api_client, complaint_api_client, @@ -140,8 +139,10 @@ def create_app(application): user_api_client, # External API clients + antivirus_client, statsd_client, zendesk_client, + redis_client ): client.init_app(application) @@ -153,6 +154,9 @@ def create_app(application): login_manager.session_protection = None login_manager.anonymous_user = AnonymousUser + # make sure we handle unicode correctly + redis_client.redis_store.decode_responses = True + from app.main import main as main_blueprint application.register_blueprint(main_blueprint) diff --git a/app/extensions.py b/app/extensions.py new file mode 100644 index 000000000..935dfd7c4 --- /dev/null +++ b/app/extensions.py @@ -0,0 +1,11 @@ +from notifications_utils.clients.antivirus.antivirus_client import ( + AntivirusClient, +) +from notifications_utils.clients.redis.redis_client import RedisClient +from notifications_utils.clients.statsd.statsd_client import StatsdClient +from notifications_utils.clients.zendesk.zendesk_client import ZendeskClient + +antivirus_client = AntivirusClient() +statsd_client = StatsdClient() +zendesk_client = ZendeskClient() +redis_client = RedisClient() diff --git a/app/main/views/feedback.py b/app/main/views/feedback.py index 8b7b36f65..bfba526bb 100644 --- a/app/main/views/feedback.py +++ b/app/main/views/feedback.py @@ -4,12 +4,8 @@ import pytz from flask import abort, redirect, render_template, request, session, url_for from flask_login import current_user -from app import ( - convert_to_boolean, - current_service, - service_api_client, - zendesk_client, -) +from app import convert_to_boolean, current_service, service_api_client +from app.extensions import zendesk_client from app.main import main from app.main.forms import Feedback, Problem, SupportType, Triage diff --git a/app/main/views/platform_admin.py b/app/main/views/platform_admin.py index d2eebb061..04ab5ae2b 100644 --- a/app/main/views/platform_admin.py +++ b/app/main/views/platform_admin.py @@ -8,12 +8,12 @@ from notifications_python_client.errors import HTTPError from requests import RequestException from app import ( - antivirus_client, complaint_api_client, letter_jobs_client, platform_stats_api_client, service_api_client, ) +from app.extensions import antivirus_client from app.main import main from app.main.forms import DateFilterForm, PDFUploadForm, ReturnedLettersForm from app.statistics_utils import ( diff --git a/app/main/views/service_settings.py b/app/main/views/service_settings.py index 69b477de2..0febfa2a5 100644 --- a/app/main/views/service_settings.py +++ b/app/main/views/service_settings.py @@ -23,8 +23,8 @@ from app import ( organisations_client, service_api_client, user_api_client, - zendesk_client, ) +from app.extensions import zendesk_client from app.main import main from app.main.forms import ( BrandingOptionsEmail, diff --git a/app/notify_client/__init__.py b/app/notify_client/__init__.py index bcd9f8fb9..ffcf55e23 100644 --- a/app/notify_client/__init__.py +++ b/app/notify_client/__init__.py @@ -2,7 +2,6 @@ from flask_login import current_user from flask import has_request_context, request, abort from notifications_python_client.base import BaseAPIClient from notifications_python_client import __version__ -from notifications_utils.clients.redis.redis_client import RedisClient def _attach_current_user(data): @@ -14,8 +13,6 @@ def _attach_current_user(data): class NotifyAdminAPIClient(BaseAPIClient): - redis_client = RedisClient() - def __init__(self): super().__init__("a" * 73, "b") @@ -24,8 +21,6 @@ class NotifyAdminAPIClient(BaseAPIClient): self.service_id = app.config['ADMIN_CLIENT_USER_NAME'] self.api_key = app.config['ADMIN_CLIENT_SECRET'] self.route_secret = app.config['ROUTE_SECRET_KEY_1'] - self.redis_client.init_app(app) - self.redis_client.redis_store.decode_responses = True def generate_headers(self, api_token): headers = { diff --git a/app/notify_client/cache.py b/app/notify_client/cache.py index 85bcd54ab..36d769df2 100644 --- a/app/notify_client/cache.py +++ b/app/notify_client/cache.py @@ -4,6 +4,8 @@ from datetime import timedelta from functools import wraps from inspect import signature +from app.extensions import redis_client + TTL = int(timedelta(days=7).total_seconds()) @@ -38,11 +40,11 @@ def set(key_format): @wraps(client_method) def new_client_method(client_instance, *args, **kwargs): redis_key = _make_key(key_format, client_method, args, kwargs) - cached = client_instance.redis_client.get(redis_key) + cached = redis_client.get(redis_key) if cached: return json.loads(cached.decode('utf-8')) api_response = client_method(client_instance, *args, **kwargs) - client_instance.redis_client.set( + redis_client.set( redis_key, json.dumps(api_response), ex=TTL, @@ -60,7 +62,7 @@ def delete(key_format): @wraps(client_method) def new_client_method(client_instance, *args, **kwargs): redis_key = _make_key(key_format, client_method, args, kwargs) - client_instance.redis_client.delete(redis_key) + redis_client.delete(redis_key) return client_method(client_instance, *args, **kwargs) return new_client_method diff --git a/app/notify_client/job_api_client.py b/app/notify_client/job_api_client.py index e835ac601..b6e8fd97d 100644 --- a/app/notify_client/job_api_client.py +++ b/app/notify_client/job_api_client.py @@ -1,5 +1,6 @@ from collections import defaultdict +from app.extensions import redis_client from app.notify_client import NotifyAdminAPIClient, _attach_current_user, cache @@ -96,8 +97,7 @@ class JobApiClient(NotifyAdminAPIClient): return bool(self.get_jobs(service_id)['data']) def create_job(self, job_id, service_id, scheduled_for=None): - - self.redis_client.set( + redis_client.set( 'has_jobs-{}'.format(service_id), b'true', ex=cache.TTL, diff --git a/app/notify_client/template_folder_api_client.py b/app/notify_client/template_folder_api_client.py index 23044259a..d273b4dc8 100644 --- a/app/notify_client/template_folder_api_client.py +++ b/app/notify_client/template_folder_api_client.py @@ -1,3 +1,4 @@ +from app.extensions import redis_client from app.notify_client import NotifyAdminAPIClient, cache @@ -35,7 +36,7 @@ class TemplateFolderAPIClient(NotifyAdminAPIClient): }) if template_ids: - self.redis_client.delete(*map( + redis_client.delete(*map( 'template-{}-version-None'.format, template_ids, )) diff --git a/tests/app/notify_client/test_email_branding_client.py b/tests/app/notify_client/test_email_branding_client.py index bfef92971..ef1b48587 100644 --- a/tests/app/notify_client/test_email_branding_client.py +++ b/tests/app/notify_client/test_email_branding_client.py @@ -9,11 +9,11 @@ def test_get_email_branding(mocker, fake_uuid): return_value={'foo': 'bar'} ) mock_redis_get = mocker.patch( - 'app.notify_client.RedisClient.get', + 'app.extensions.RedisClient.get', return_value=None, ) mock_redis_set = mocker.patch( - 'app.notify_client.RedisClient.set', + 'app.extensions.RedisClient.set', ) EmailBrandingClient().get_email_branding(fake_uuid) mock_get.assert_called_once_with( @@ -33,11 +33,11 @@ def test_get_all_email_branding(mocker): return_value={'email_branding': [1, 2, 3]} ) mock_redis_get = mocker.patch( - 'app.notify_client.RedisClient.get', + 'app.extensions.RedisClient.get', return_value=None, ) mock_redis_set = mocker.patch( - 'app.notify_client.RedisClient.set', + 'app.extensions.RedisClient.set', ) EmailBrandingClient().get_all_email_branding() mock_get.assert_called_once_with( @@ -56,7 +56,7 @@ def test_create_email_branding(mocker): 'domain': 'sample.com', 'brand_type': 'org'} mock_post = mocker.patch('app.notify_client.email_branding_client.EmailBrandingClient.post') - mock_redis_delete = mocker.patch('app.notify_client.RedisClient.delete') + mock_redis_delete = mocker.patch('app.extensions.RedisClient.delete') EmailBrandingClient().create_email_branding( logo=org_data['logo'], name=org_data['name'], text=org_data['text'], colour=org_data['colour'], domain=org_data['domain'], brand_type='org' @@ -75,7 +75,7 @@ def test_update_email_branding(mocker, fake_uuid): 'domain': 'sample.com', 'brand_type': 'org'} mock_post = mocker.patch('app.notify_client.email_branding_client.EmailBrandingClient.post') - mock_redis_delete = mocker.patch('app.notify_client.RedisClient.delete') + mock_redis_delete = mocker.patch('app.extensions.RedisClient.delete') EmailBrandingClient().update_email_branding( branding_id=fake_uuid, logo=org_data['logo'], name=org_data['name'], text=org_data['text'], colour=org_data['colour'], domain=org_data['domain'], brand_type='org') diff --git a/tests/app/notify_client/test_job_client.py b/tests/app/notify_client/test_job_client.py index 58c457f35..b12322168 100644 --- a/tests/app/notify_client/test_job_client.py +++ b/tests/app/notify_client/test_job_client.py @@ -10,7 +10,7 @@ def test_client_creates_job_data_correctly(mocker, fake_uuid): job_id = fake_uuid service_id = fake_uuid mocker.patch('app.notify_client.current_user', id='1') - mock_redis_set = mocker.patch('app.notify_client.RedisClient.set') + mock_redis_set = mocker.patch('app.extensions.RedisClient.set') expected_data = { "id": job_id, @@ -330,7 +330,7 @@ def test_has_jobs_sets_cache( 'app.notify_client.job_api_client.JobApiClient.get', return_value={'data': job_data} ) - mock_redis_set = mocker.patch('app.notify_client.RedisClient.set') + mock_redis_set = mocker.patch('app.extensions.RedisClient.set') JobApiClient().has_jobs(fake_uuid) @@ -359,7 +359,7 @@ def test_has_jobs_returns_from_cache( 'app.notify_client.job_api_client.JobApiClient.get' ) mock_redis_get = mocker.patch( - 'app.notify_client.RedisClient.get', + 'app.extensions.RedisClient.get', return_value=cache_value, ) diff --git a/tests/app/notify_client/test_letter_branding_client.py b/tests/app/notify_client/test_letter_branding_client.py index 232d55fa8..a6e9db49d 100644 --- a/tests/app/notify_client/test_letter_branding_client.py +++ b/tests/app/notify_client/test_letter_branding_client.py @@ -8,8 +8,8 @@ def test_get_letter_branding(mocker, fake_uuid): 'app.notify_client.letter_branding_client.LetterBrandingClient.get', return_value={'foo': 'bar'} ) - mock_redis_get = mocker.patch('app.notify_client.RedisClient.get', return_value=None) - mock_redis_set = mocker.patch('app.notify_client.RedisClient.set') + mock_redis_get = mocker.patch('app.extensions.RedisClient.get', return_value=None) + mock_redis_set = mocker.patch('app.extensions.RedisClient.set') LetterBrandingClient().get_letter_branding(fake_uuid) @@ -24,8 +24,8 @@ def test_get_letter_branding(mocker, fake_uuid): def test_get_all_letter_branding(mocker): mock_get = mocker.patch('app.notify_client.letter_branding_client.LetterBrandingClient.get', return_value=[1, 2, 3]) - mock_redis_get = mocker.patch('app.notify_client.RedisClient.get', return_value=None) - mock_redis_set = mocker.patch('app.notify_client.RedisClient.set') + mock_redis_get = mocker.patch('app.extensions.RedisClient.get', return_value=None) + mock_redis_set = mocker.patch('app.extensions.RedisClient.set') LetterBrandingClient().get_all_letter_branding() @@ -42,7 +42,7 @@ def test_create_letter_branding(mocker): new_branding = {'filename': 'uuid-test', 'name': 'my letters', 'domain': 'example.com'} mock_post = mocker.patch('app.notify_client.letter_branding_client.LetterBrandingClient.post') - mock_redis_delete = mocker.patch('app.notify_client.RedisClient.delete') + mock_redis_delete = mocker.patch('app.extensions.RedisClient.delete') LetterBrandingClient().create_letter_branding( filename=new_branding['filename'], name=new_branding['name'], domain=new_branding['domain'] @@ -59,7 +59,7 @@ def test_update_letter_branding(mocker, fake_uuid): branding = {'filename': 'uuid-test', 'name': 'my letters', 'domain': 'example.com'} mock_post = mocker.patch('app.notify_client.letter_branding_client.LetterBrandingClient.post') - mock_redis_delete = mocker.patch('app.notify_client.RedisClient.delete') + mock_redis_delete = mocker.patch('app.extensions.RedisClient.delete') LetterBrandingClient().update_letter_branding( branding_id=fake_uuid, filename=branding['filename'], name=branding['name'], domain=branding['domain']) diff --git a/tests/app/notify_client/test_service_api_client.py b/tests/app/notify_client/test_service_api_client.py index f809259db..f8e58dec4 100644 --- a/tests/app/notify_client/test_service_api_client.py +++ b/tests/app/notify_client/test_service_api_client.py @@ -324,7 +324,7 @@ def test_returns_value_from_cache( ): mock_redis_get = mocker.patch( - 'app.notify_client.RedisClient.get', + 'app.extensions.RedisClient.get', return_value=cache_value, ) mock_api_get = mocker.patch( @@ -332,7 +332,7 @@ def test_returns_value_from_cache( return_value={'data_from': 'api'}, ) mock_redis_set = mocker.patch( - 'app.notify_client.RedisClient.set', + 'app.extensions.RedisClient.set', ) assert client_method(*extra_args) == expected_return_value @@ -375,7 +375,7 @@ def test_deletes_service_cache( extra_kwargs, ): mocker.patch('app.notify_client.current_user', id='1') - mock_redis_delete = mocker.patch('app.notify_client.RedisClient.delete') + mock_redis_delete = mocker.patch('app.extensions.RedisClient.delete') mock_request = mocker.patch('notifications_python_client.base.BaseAPIClient.request') getattr(client, method)(*extra_args, **extra_kwargs) @@ -423,7 +423,7 @@ def test_deletes_caches_when_modifying_templates( expected_cache_deletes, ): mocker.patch('app.notify_client.current_user', id='1') - mock_redis_delete = mocker.patch('app.notify_client.RedisClient.delete') + mock_redis_delete = mocker.patch('app.extensions.RedisClient.delete') mock_request = mocker.patch('notifications_python_client.base.BaseAPIClient.request') getattr(service_api_client, method)(*extra_args) diff --git a/tests/app/notify_client/test_template_folder_client.py b/tests/app/notify_client/test_template_folder_client.py index 3ab808930..bfd615e2f 100644 --- a/tests/app/notify_client/test_template_folder_client.py +++ b/tests/app/notify_client/test_template_folder_client.py @@ -9,7 +9,7 @@ from app.notify_client.template_folder_api_client import TemplateFolderAPIClient @pytest.mark.parametrize('parent_id', [uuid.uuid4(), None]) def test_create_template_folder_calls_correct_api_endpoint(mocker, parent_id): - mock_redis_delete = mocker.patch('app.notify_client.RedisClient.delete') + mock_redis_delete = mocker.patch('app.extensions.RedisClient.delete') some_service_id = uuid.uuid4() expected_url = '/service/{}/template-folder'.format(some_service_id) @@ -26,8 +26,8 @@ def test_create_template_folder_calls_correct_api_endpoint(mocker, parent_id): def test_get_template_folders_calls_correct_api_endpoint(mocker): - mock_redis_get = mocker.patch('app.notify_client.RedisClient.get', return_value=None) - mock_redis_set = mocker.patch('app.notify_client.RedisClient.set') + mock_redis_get = mocker.patch('app.extensions.RedisClient.get', return_value=None) + mock_redis_set = mocker.patch('app.extensions.RedisClient.set') mock_api_get = mocker.patch( 'app.notify_client.NotifyAdminAPIClient.get', return_value={'template_folders': {'a': 'b'}} @@ -50,7 +50,7 @@ def test_get_template_folders_calls_correct_api_endpoint(mocker): def test_move_templates_and_folders(mocker): - mock_redis_delete = mocker.patch('app.notify_client.RedisClient.delete') + mock_redis_delete = mocker.patch('app.extensions.RedisClient.delete') mock_api_post = mocker.patch('app.notify_client.NotifyAdminAPIClient.post') some_service_id = uuid.uuid4() @@ -106,7 +106,7 @@ def test_move_templates_and_folders_to_root(mocker): def test_update_template_folder_calls_correct_api_endpoint(mocker): - mock_redis_delete = mocker.patch('app.notify_client.RedisClient.delete') + mock_redis_delete = mocker.patch('app.extensions.RedisClient.delete') some_service_id = uuid.uuid4() template_folder_id = uuid.uuid4() @@ -124,7 +124,7 @@ def test_update_template_folder_calls_correct_api_endpoint(mocker): def test_delete_template_folder_calls_correct_api_endpoint(mocker): - mock_redis_delete = mocker.patch('app.notify_client.RedisClient.delete') + mock_redis_delete = mocker.patch('app.extensions.RedisClient.delete') some_service_id = uuid.uuid4() template_folder_id = uuid.uuid4() diff --git a/tests/app/notify_client/test_user_client.py b/tests/app/notify_client/test_user_client.py index c3b93cd2b..f76338e75 100644 --- a/tests/app/notify_client/test_user_client.py +++ b/tests/app/notify_client/test_user_client.py @@ -210,7 +210,7 @@ def test_returns_value_from_cache( ): mock_redis_get = mocker.patch( - 'app.notify_client.RedisClient.get', + 'app.extensions.RedisClient.get', return_value=cache_value, ) mock_api_get = mocker.patch( @@ -218,7 +218,7 @@ def test_returns_value_from_cache( return_value={'data': 'from api'}, ) mock_redis_set = mocker.patch( - 'app.notify_client.RedisClient.set', + 'app.extensions.RedisClient.set', ) mock_model = mocker.patch( 'app.models.user.User.__init__', @@ -262,7 +262,7 @@ def test_deletes_user_cache( extra_kwargs, ): mocker.patch('app.notify_client.current_user', id='1') - mock_redis_delete = mocker.patch('app.notify_client.RedisClient.delete') + mock_redis_delete = mocker.patch('app.extensions.RedisClient.delete') mock_request = mocker.patch('notifications_python_client.base.BaseAPIClient.request') getattr(client, method)(*extra_args, **extra_kwargs) From f6513613d3a2365b3de87ead226a1397f880b1c7 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 15 Feb 2019 10:26:56 +0000 Subject: [PATCH 2/6] bump utils to bring in redis changes also set redis url locally to be localhost. redis is disabled by default so this won't do anything unless you set REDIS_ENABLED=1 as an environment variable --- app/config.py | 2 ++ requirements-app.txt | 2 +- requirements.txt | 8 ++++---- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/app/config.py b/app/config.py index 27926e011..47620b253 100644 --- a/app/config.py +++ b/app/config.py @@ -97,6 +97,8 @@ class Development(Config): ASSET_PATH = '/static/' + REDIS_URL = 'redis://localhost:6379/0' + class Test(Development): DEBUG = True diff --git a/requirements-app.txt b/requirements-app.txt index 07a25835f..4b2101f14 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -23,4 +23,4 @@ awscli-cwlogs>=1.4,<1.5 # Putting upgrade on hold due to v1.0.0 using sha512 instead of sha1 by default itsdangerous==0.24 # pyup: <1.0.0 -git+https://github.com/alphagov/notifications-utils.git@31.0.0#egg=notifications-utils==31.0.0 +git+https://github.com/alphagov/notifications-utils.git@31.2.0#egg=notifications-utils==31.2.0 diff --git a/requirements.txt b/requirements.txt index 629d88224..f1b58d0cf 100644 --- a/requirements.txt +++ b/requirements.txt @@ -25,13 +25,13 @@ awscli-cwlogs>=1.4,<1.5 # Putting upgrade on hold due to v1.0.0 using sha512 instead of sha1 by default itsdangerous==0.24 # pyup: <1.0.0 -git+https://github.com/alphagov/notifications-utils.git@31.0.0#egg=notifications-utils==31.0.0 +git+https://github.com/alphagov/notifications-utils.git@31.2.0#egg=notifications-utils==31.2.0 ## The following requirements were added by pip freeze: -awscli==1.16.100 +awscli==1.16.105 bleach==3.0.2 boto3==1.6.16 -botocore==1.12.90 +botocore==1.12.95 certifi==2018.11.29 chardet==3.0.4 Click==7.0 @@ -48,7 +48,7 @@ jdcal==1.4 Jinja2==2.10 jmespath==0.9.3 lml==0.0.9 -lxml==4.3.0 +lxml==4.3.1 MarkupSafe==1.1.0 mistune==0.8.4 monotonic==1.5 From 1dcba53daf5ef338fb1b1880611ef7fc3fad1bef Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 15 Feb 2019 10:27:38 +0000 Subject: [PATCH 3/6] add clear cache platform admin page a form that allows you to clear entries from the cache for all of either users, services or templates. It'll tell you the largest amount of keys deleted, since there are multiple keys associated with each model. --- app/main/forms.py | 12 +++++ app/main/views/platform_admin.py | 48 ++++++++++++++++++- app/navigation.py | 5 ++ .../views/platform-admin/_base_template.html | 1 + .../views/platform-admin/clear-cache.html | 21 ++++++++ 5 files changed, 85 insertions(+), 2 deletions(-) create mode 100644 app/templates/views/platform-admin/clear-cache.html diff --git a/app/main/forms.py b/app/main/forms.py index 399705c24..6f6ad8447 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1281,3 +1281,15 @@ class TemplateAndFoldersSelectionForm(Form): required_for_ops('add-new-template'), Optional(), ], required_message='Select the type of template you want to add') + + +class ClearCacheForm(StripWhitespaceForm): + model_type = RadioField( + 'What do you want to clear today', + choices=[ + ('user', 'Users'), + ('service', 'Services'), + ('template', 'Templates') + ], + validators=[DataRequired()] + ) diff --git a/app/main/views/platform_admin.py b/app/main/views/platform_admin.py index 04ab5ae2b..14a6110cd 100644 --- a/app/main/views/platform_admin.py +++ b/app/main/views/platform_admin.py @@ -13,9 +13,9 @@ from app import ( platform_stats_api_client, service_api_client, ) -from app.extensions import antivirus_client +from app.extensions import antivirus_client, redis_client from app.main import main -from app.main.forms import DateFilterForm, PDFUploadForm, ReturnedLettersForm +from app.main.forms import DateFilterForm, PDFUploadForm, ReturnedLettersForm, ClearCacheForm from app.statistics_utils import ( get_formatted_percentage, get_formatted_percentage_two_dp, @@ -281,6 +281,50 @@ def platform_admin_letter_validation_preview(): ) +@main.route("/platform-admin/clear-cache", methods=['GET', 'POST']) +@login_required +@user_is_platform_admin +def clear_cache(): + # note: `service-{uuid}-templates` cache is cleared for both services and templates. + CACHE_KEYS = { + 'user': [ + 'user-????????-????-????-????-????????????', + ], + 'service': [ + 'has_jobs-????????-????-????-????-????????????', + 'service-????????-????-????-????-????????????', + 'service-????????-????-????-????-????????????-templates', + 'service-????????-????-????-????-????????????-data-retention', + 'service-????????-????-????-????-????????????-template-folders', + ], + 'template': [ + 'service-????????-????-????-????-????????????-templates', + 'template-????????-????-????-????-????????????-version-*', + 'template-????????-????-????-????-????????????-versions', + ], + 'email_branding': [ + 'email_branding', + 'email_branding-????????-????-????-????-????????????', + ] + } + form = ClearCacheForm() + + if form.validate_on_submit(): + to_delete = form.model_type.data + + num_deleted = max( + redis_client.delete_cache_keys_by_pattern(pattern) + for pattern in CACHE_KEYS[to_delete] + ) + + flash('Removed {} {} objects from redis'.format(num_deleted, to_delete)) + + return render_template( + 'views/platform-admin/clear-cache.html', + form=form + ) + + def sum_service_usage(service): total = 0 for notification_type in service['statistics'].keys(): diff --git a/app/navigation.py b/app/navigation.py index befb5e046..e83ff3800 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -74,6 +74,7 @@ class HeaderNavigation(Navigation): }, 'platform-admin': { 'add_organisation', + 'clear_cache', 'create_email_branding', 'create_letter_branding', 'email_branding', @@ -302,6 +303,7 @@ class MainNavigation(Navigation): 'check_notification', 'choose_template', 'choose_template_to_copy', + 'clear_cache', 'confirm_redact_template', 'conversation_reply', 'copy_template', @@ -399,6 +401,7 @@ class MainNavigation(Navigation): 'check_notification_preview', 'choose_account', 'choose_service', + 'clear_cache', 'confirm_edit_organisation_name', 'conversation_reply_with_template', 'conversation_updates', @@ -573,6 +576,7 @@ class CaseworkNavigation(Navigation): 'choose_account', 'choose_service', 'choose_template_to_copy', + 'clear_cache', 'confirm_edit_organisation_name', 'confirm_redact_template', 'conversation', @@ -809,6 +813,7 @@ class OrgNavigation(Navigation): 'choose_service', 'choose_template', 'choose_template_to_copy', + 'clear_cache', 'confirm_redact_template', 'conversation', 'conversation_reply', diff --git a/app/templates/views/platform-admin/_base_template.html b/app/templates/views/platform-admin/_base_template.html index 71d617dfc..90022f895 100644 --- a/app/templates/views/platform-admin/_base_template.html +++ b/app/templates/views/platform-admin/_base_template.html @@ -24,6 +24,7 @@ ('Email Complaints', url_for('main.platform_admin_list_complaints')), ('Returned letters', url_for('main.platform_admin_returned_letters')), ('Letter validation preview', url_for('main.platform_admin_letter_validation_preview')), + ('Clear cache', url_for('main.clear_cache')), ] %}
  • diff --git a/app/templates/views/platform-admin/clear-cache.html b/app/templates/views/platform-admin/clear-cache.html new file mode 100644 index 000000000..b4d89030f --- /dev/null +++ b/app/templates/views/platform-admin/clear-cache.html @@ -0,0 +1,21 @@ +{% extends "views/platform-admin/_base_template.html" %} +{% from "components/form.html" import form_wrapper %} +{% from "components/radios.html" import radios %} +{% from "components/page-footer.html" import page_footer %} + +{% block per_page_title %} + Clear Cache +{% endblock %} + +{% block platform_admin_content %} + +

    + Clear Redis Cache +

    + + {% call form_wrapper() %} + {{ radios(form.model_type) }} + {{ page_footer('Clear') }} + {% endcall %} + +{% endblock %} From 89bfdf27ce0fb0750bc9931bfee891d8988bdd47 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 15 Feb 2019 11:13:53 +0000 Subject: [PATCH 4/6] add tests --- app/main/views/platform_admin.py | 7 +++- app/navigation.py | 1 - .../views/platform-admin/clear-cache.html | 2 +- tests/app/main/views/test_platform_admin.py | 41 ++++++++++++++++++- 4 files changed, 47 insertions(+), 4 deletions(-) diff --git a/app/main/views/platform_admin.py b/app/main/views/platform_admin.py index 14a6110cd..63f4bc96c 100644 --- a/app/main/views/platform_admin.py +++ b/app/main/views/platform_admin.py @@ -15,7 +15,12 @@ from app import ( ) from app.extensions import antivirus_client, redis_client from app.main import main -from app.main.forms import DateFilterForm, PDFUploadForm, ReturnedLettersForm, ClearCacheForm +from app.main.forms import ( + ClearCacheForm, + DateFilterForm, + PDFUploadForm, + ReturnedLettersForm, +) from app.statistics_utils import ( get_formatted_percentage, get_formatted_percentage_two_dp, diff --git a/app/navigation.py b/app/navigation.py index e83ff3800..a3fb7247e 100644 --- a/app/navigation.py +++ b/app/navigation.py @@ -303,7 +303,6 @@ class MainNavigation(Navigation): 'check_notification', 'choose_template', 'choose_template_to_copy', - 'clear_cache', 'confirm_redact_template', 'conversation_reply', 'copy_template', diff --git a/app/templates/views/platform-admin/clear-cache.html b/app/templates/views/platform-admin/clear-cache.html index b4d89030f..5cdc11b0a 100644 --- a/app/templates/views/platform-admin/clear-cache.html +++ b/app/templates/views/platform-admin/clear-cache.html @@ -10,7 +10,7 @@ {% block platform_admin_content %}

    - Clear Redis Cache + Clear Cache

    {% call form_wrapper() %} diff --git a/tests/app/main/views/test_platform_admin.py b/tests/app/main/views/test_platform_admin.py index 719f83ef3..22de66a6d 100644 --- a/tests/app/main/views/test_platform_admin.py +++ b/tests/app/main/views/test_platform_admin.py @@ -2,7 +2,7 @@ import datetime import re import uuid from functools import partial -from unittest.mock import ANY +from unittest.mock import ANY, call import pytest import requests_mock @@ -858,3 +858,42 @@ def test_letter_validation_preview_doesnt_call_template_preview_when_file_doesnt page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') assert page.find('div', class_='banner-dangerous').text.strip() == "Document didn't pass the virus scan" + + +def test_clear_cache_shows_form(client_request, platform_admin_user, mocker): + redis = mocker.patch('app.main.views.platform_admin.redis_client') + client_request.login(platform_admin_user) + + 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 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): + 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) + + assert redis.delete_cache_keys_by_pattern.call_args_list == [ + call('service-????????-????-????-????-????????????-templates'), + call('template-????????-????-????-????-????????????-version-*'), + call('template-????????-????-????-????-????????????-versions'), + ] + + flash_banner = page.find('div', class_='banner-dangerous') + assert flash_banner.text.strip() == 'Removed 3 template objects from redis' + + +def test_clear_cache_requires_option(client_request, platform_admin_user,mocker): + redis = mocker.patch('app.main.views.platform_admin.redis_client') + client_request.login(platform_admin_user) + + page = client_request.post('main.clear_cache', _data={}, _expected_status=200) + + assert normalize_spaces(page.find('span', class_='error-message').text) == 'Not a valid choice' + assert not redis.delete_cache_keys_by_pattern.called From b062a5a13f3b447ed548f416e18e404109301b06 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 18 Feb 2019 12:05:51 +0000 Subject: [PATCH 5/6] make banner green (default) instead of red (dangerous) --- app/main/views/platform_admin.py | 2 +- tests/app/main/views/test_platform_admin.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/main/views/platform_admin.py b/app/main/views/platform_admin.py index 63f4bc96c..c14c78117 100644 --- a/app/main/views/platform_admin.py +++ b/app/main/views/platform_admin.py @@ -322,7 +322,7 @@ def clear_cache(): for pattern in CACHE_KEYS[to_delete] ) - flash('Removed {} {} objects from redis'.format(num_deleted, to_delete)) + flash('Removed {} {} objects from redis'.format(num_deleted, to_delete), 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 22de66a6d..7fba52400 100644 --- a/tests/app/main/views/test_platform_admin.py +++ b/tests/app/main/views/test_platform_admin.py @@ -885,11 +885,11 @@ def test_clear_cache_submits_and_tells_you_how_many_things_were_deleted(client_r call('template-????????-????-????-????-????????????-versions'), ] - flash_banner = page.find('div', class_='banner-dangerous') + flash_banner = page.find('div', class_='banner-default') assert flash_banner.text.strip() == 'Removed 3 template objects from redis' -def test_clear_cache_requires_option(client_request, platform_admin_user,mocker): +def test_clear_cache_requires_option(client_request, platform_admin_user, mocker): redis = mocker.patch('app.main.views.platform_admin.redis_client') client_request.login(platform_admin_user) From bfefb115ed33f21cefdbb81b5a86b3fada33fff0 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 19 Feb 2019 11:35:34 +0000 Subject: [PATCH 6/6] move choices from form to view so that when updating, you don't have to update two separate places --- app/main/forms.py | 5 ----- app/main/views/platform_admin.py | 31 +++++++++++++++++++------------ 2 files changed, 19 insertions(+), 17 deletions(-) diff --git a/app/main/forms.py b/app/main/forms.py index 6f6ad8447..525e20737 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -1286,10 +1286,5 @@ class TemplateAndFoldersSelectionForm(Form): class ClearCacheForm(StripWhitespaceForm): model_type = RadioField( 'What do you want to clear today', - choices=[ - ('user', 'Users'), - ('service', 'Services'), - ('template', 'Templates') - ], validators=[DataRequired()] ) diff --git a/app/main/views/platform_admin.py b/app/main/views/platform_admin.py index c14c78117..1029bba3c 100644 --- a/app/main/views/platform_admin.py +++ b/app/main/views/platform_admin.py @@ -1,5 +1,6 @@ import itertools import re +from collections import OrderedDict from datetime import datetime from flask import abort, flash, redirect, render_template, request, url_for @@ -291,28 +292,34 @@ def platform_admin_letter_validation_preview(): @user_is_platform_admin def clear_cache(): # note: `service-{uuid}-templates` cache is cleared for both services and templates. - CACHE_KEYS = { - 'user': [ + CACHE_KEYS = OrderedDict([ + ('user', [ 'user-????????-????-????-????-????????????', - ], - 'service': [ + ]), + ('service', [ 'has_jobs-????????-????-????-????-????????????', 'service-????????-????-????-????-????????????', 'service-????????-????-????-????-????????????-templates', 'service-????????-????-????-????-????????????-data-retention', 'service-????????-????-????-????-????????????-template-folders', - ], - 'template': [ + ]), + ('template', [ 'service-????????-????-????-????-????????????-templates', 'template-????????-????-????-????-????????????-version-*', 'template-????????-????-????-????-????????????-versions', - ], - 'email_branding': [ + ]), + ('email_branding', [ 'email_branding', 'email_branding-????????-????-????-????-????????????', - ] - } + ]), + ('letter_branding', [ + 'letter_branding', + 'letter_branding-????????-????-????-????-????????????', + ]) + ]) + form = ClearCacheForm() + form.model_type.choices = [(key, key.replace('_', ' ').title()) for key in CACHE_KEYS] if form.validate_on_submit(): to_delete = form.model_type.data @@ -321,8 +328,8 @@ def clear_cache(): redis_client.delete_cache_keys_by_pattern(pattern) for pattern in CACHE_KEYS[to_delete] ) - - flash('Removed {} {} objects from redis'.format(num_deleted, to_delete), category='default') + msg = 'Removed {} {} object{} from redis' + flash(msg.format(num_deleted, to_delete, 's' if num_deleted != 1 else ''), category='default') return render_template( 'views/platform-admin/clear-cache.html',