From 7a7a9ae85425b6c96ca9531878ac3262c213df71 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Mon, 3 Dec 2018 17:19:38 +0000 Subject: [PATCH] Cache service data retention in Redis Adds caching for service data retention. This removes separate API client methods to retrieve individual data retention records by id or type in favor of a single method that fetches and caches all retention settings configured for the service. This makes it much easier to invalidate cache when settings change. Lookup by id or type is provided by helper methods in the service model. --- app/main/views/jobs.py | 8 ++--- app/main/views/notifications.py | 5 ++-- app/models/service.py | 11 ++++++- app/notify_client/service_api_client.py | 9 ++---- tests/app/main/views/test_activity.py | 26 ++++++++-------- tests/app/main/views/test_jobs.py | 30 ++++++++++--------- tests/app/main/views/test_send.py | 2 +- tests/app/main/views/test_service_settings.py | 15 ++++++---- tests/conftest.py | 28 ++--------------- 9 files changed, 62 insertions(+), 72 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 04cc17751..ee8eeb102 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -226,8 +226,8 @@ def get_notifications(service_id, message_type, status_override=None): service_data_retention_days = None if message_type is not None: - service_data_retention_days = service_api_client.get_service_data_retention_by_notification_type( - service_id, message_type + service_data_retention_days = current_service.get_data_retention_by_type( + message_type ).get('days_of_retention', current_app.config['ACTIVITY_STATS_LIMIT_DAYS']) if request.path.endswith('csv') and current_user.has_permissions('view_activity'): @@ -406,8 +406,8 @@ def get_job_partials(job, template): counts=_get_job_counts(job), status=filter_args['status'] ) - service_data_retention_days = service_api_client.get_service_data_retention_by_notification_type( - current_service.id, template['template_type'] + service_data_retention_days = current_service.get_data_retention_by_type( + template['template_type'] ).get('days_of_retention', current_app.config['ACTIVITY_STATS_LIMIT_DAYS']) return { diff --git a/app/main/views/notifications.py b/app/main/views/notifications.py index dee53af1c..22251775a 100644 --- a/app/main/views/notifications.py +++ b/app/main/views/notifications.py @@ -30,7 +30,6 @@ from app import ( format_date_numeric, job_api_client, notification_api_client, - service_api_client, ) from app.main import main from app.notify_client.api_key_api_client import KEY_TYPE_TEST @@ -198,8 +197,8 @@ def download_notifications_csv(service_id): filter_args = parse_filter_args(request.args) filter_args['status'] = set_status_filters(filter_args) - service_data_retention_days = service_api_client.get_service_data_retention_by_notification_type( - service_id, filter_args.get('message_type')[0] + service_data_retention_days = current_service.get_data_retention_by_type( + filter_args.get('message_type')[0] ).get('days_of_retention', current_app.config['ACTIVITY_STATS_LIMIT_DAYS']) return Response( stream_with_context( diff --git a/app/models/service.py b/app/models/service.py index 8a04d79e5..4ed78ca50 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -268,7 +268,16 @@ class Service(): return service_api_client.get_service_data_retention(self.id) def get_data_retention_item(self, id): - return service_api_client.get_service_data_retention_by_id(self.id, id) + return next( + (dr for dr in self.data_retention if dr['id'] == id), + None + ) + + def get_data_retention_by_type(self, notification_type): + return next( + (dr for dr in self.data_retention if dr['notification_type'] == notification_type), + {} + ) @property def email_branding_id(self): diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index 63a5e1e9f..7af14425f 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -488,6 +488,7 @@ class ServiceAPIClient(NotifyAdminAPIClient): } return self.post("/service/{}/delivery-receipt-api".format(service_id), data) + @cache.delete('service-{service_id}-data-retention') def create_service_data_retention(self, service_id, notification_type, days_of_retention): data = { "notification_type": notification_type, @@ -496,20 +497,16 @@ class ServiceAPIClient(NotifyAdminAPIClient): return self.post("/service/{}/data-retention".format(service_id), data) + @cache.delete('service-{service_id}-data-retention') def update_service_data_retention(self, service_id, data_retention_id, days_of_retention): data = { "days_of_retention": days_of_retention } return self.post("/service/{}/data-retention/{}".format(service_id, data_retention_id), data) + @cache.set('service-{service_id}-data-retention') def get_service_data_retention(self, service_id): return self.get("/service/{}/data-retention".format(service_id)) - def get_service_data_retention_by_notification_type(self, service_id, notification_type): - return self.get("/service/{}/data-retention/notification-type/{}".format(service_id, notification_type)) - - def get_service_data_retention_by_id(self, service_id, data_retention_id): - return self.get("service/{}/data-retention/{}".format(service_id, data_retention_id)) - service_api_client = ServiceAPIClient() diff --git a/tests/app/main/views/test_activity.py b/tests/app/main/views/test_activity.py index 8c77c6031..1a028867b 100644 --- a/tests/app/main/views/test_activity.py +++ b/tests/app/main/views/test_activity.py @@ -89,7 +89,7 @@ def test_can_show_notifications( service_one, mock_get_notifications, mock_get_service_statistics, - mock_get_service_data_retention_by_notification_type, + mock_get_service_data_retention, mock_has_no_jobs, user, extra_args, @@ -229,7 +229,7 @@ def test_link_to_download_notifications( fake_uuid, mock_get_notifications, mock_get_service_statistics, - mock_get_service_data_retention_by_notification_type, + mock_get_service_data_retention, mock_has_no_jobs, user, query_parameters, @@ -265,7 +265,7 @@ def test_letters_with_status_virus_scan_failed_shows_a_failure_description( logged_in_client, service_one, mock_get_service_statistics, - mock_get_service_data_retention_by_notification_type, + mock_get_service_data_retention, ): mock_get_notifications( mocker, @@ -295,7 +295,7 @@ def test_should_not_show_preview_link_for_precompiled_letters_in_virus_states( logged_in_client, service_one, mock_get_service_statistics, - mock_get_service_data_retention_by_notification_type, + mock_get_service_data_retention, letter_status, ): mock_get_notifications( @@ -318,7 +318,7 @@ def test_should_not_show_preview_link_for_precompiled_letters_in_virus_states( def test_shows_message_when_no_notifications( client_request, mock_get_service_statistics, - mock_get_service_data_retention_by_notification_type, + mock_get_service_data_retention, mock_get_notifications_with_no_notifications, ): @@ -380,7 +380,7 @@ def test_search_recipient_form( logged_in_client, mock_get_notifications, mock_get_service_statistics, - mock_get_service_data_retention_by_notification_type, + mock_get_service_data_retention, initial_query_arguments, form_post_data, expected_search_box_label, @@ -422,7 +422,7 @@ def test_should_show_notifications_for_a_service_with_next_previous( active_user_with_permissions, mock_get_notifications_with_previous_next, mock_get_service_statistics, - mock_get_service_data_retention_by_notification_type, + mock_get_service_data_retention, mocker, ): response = logged_in_client.get(url_for( @@ -503,7 +503,7 @@ def test_html_contains_notification_id( active_user_with_permissions, mock_get_notifications, mock_get_service_statistics, - mock_get_service_data_retention_by_notification_type, + mock_get_service_data_retention, mocker, ): response = logged_in_client.get(url_for( @@ -523,7 +523,7 @@ def test_html_contains_links_for_failed_notifications( client_request, active_user_with_permissions, mock_get_service_statistics, - mock_get_service_data_retention_by_notification_type, + mock_get_service_data_retention, mocker, ): mock_get_notifications(mocker, active_user_with_permissions, noti_status='technical-failure') @@ -544,7 +544,7 @@ def test_redacts_templates_that_should_be_redacted( mocker, active_user_with_permissions, mock_get_service_statistics, - mock_get_service_data_retention_by_notification_type, + mock_get_service_data_retention, ): mock_get_notifications( mocker, @@ -577,7 +577,7 @@ def test_big_numbers_and_search_dont_show_for_letters( mock_get_notifications, active_user_with_permissions, mock_get_service_statistics, - mock_get_service_data_retention_by_notification_type, + mock_get_service_data_retention, message_type, tablist_visible, search_bar_visible @@ -609,7 +609,7 @@ def test_sending_status_hint_does_not_include_status_for_letters( service_one, active_user_with_permissions, mock_get_service_statistics, - mock_get_service_data_retention_by_notification_type, + mock_get_service_data_retention, message_type, status, expected_hint_status, @@ -637,7 +637,7 @@ def test_should_expected_hint_for_letters( service_one, active_user_with_permissions, mock_get_service_statistics, - mock_get_service_data_retention_by_notification_type, + mock_get_service_data_retention, mocker, fake_uuid, is_precompiled_letter, diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 4d7c008b7..98729d3f4 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -181,7 +181,7 @@ def test_should_show_page_for_one_job( mock_get_job, mocker, mock_get_notifications, - mock_get_service_data_retention_by_notification_type, + mock_get_service_data_retention, fake_uuid, status_argument, expected_api_call, @@ -229,11 +229,11 @@ def test_should_show_page_for_one_job_with_flexible_data_retention( mock_get_job, mocker, mock_get_notifications, - mock_get_service_data_retention_by_notification_type, + mock_get_service_data_retention, fake_uuid, ): - mock_get_service_data_retention_by_notification_type.side_effect = [{"days_of_retention": 10}] + mock_get_service_data_retention.side_effect = [[{"days_of_retention": 10, "notification_type": "sms"}]] page = client_request.get( 'main.view_job', service_id=SERVICE_ONE_ID, @@ -251,7 +251,7 @@ def test_get_jobs_should_tell_user_if_more_than_one_page( mock_get_job, mock_get_service_template, mock_get_notifications_with_previous_next, - mock_get_service_data_retention_by_notification_type, + mock_get_service_data_retention, ): response = logged_in_client.get(url_for( 'main.view_job', @@ -273,7 +273,7 @@ def test_should_show_job_in_progress( mock_get_job_in_progress, mocker, mock_get_notifications, - mock_get_service_data_retention_by_notification_type, + mock_get_service_data_retention, fake_uuid, ): @@ -293,7 +293,7 @@ def test_should_show_letter_job( client_request, mock_get_service_letter_template, mock_get_job, - mock_get_service_data_retention_by_notification_type, + mock_get_service_data_retention, fake_uuid, active_user_with_permissions, mocker, @@ -349,7 +349,7 @@ def test_should_show_letter_job_with_banner_after_sending_before_1730( mock_get_service_letter_template, mock_get_job, mock_get_notifications, - mock_get_service_data_retention_by_notification_type, + mock_get_service_data_retention, fake_uuid, ): @@ -372,7 +372,7 @@ def test_should_show_letter_job_with_banner_when_there_are_multiple_CSV_rows( mock_get_service_letter_template, mock_get_job_in_progress, mock_get_notifications, - mock_get_service_data_retention_by_notification_type, + mock_get_service_data_retention, fake_uuid, ): @@ -395,7 +395,7 @@ def test_should_show_letter_job_with_banner_after_sending_after_1730( mock_get_service_letter_template, mock_get_job, mock_get_notifications, - mock_get_service_data_retention_by_notification_type, + mock_get_service_data_retention, fake_uuid, ): @@ -418,7 +418,7 @@ def test_should_show_scheduled_job( active_user_with_permissions, mock_get_service_template, mock_get_scheduled_job, - mock_get_service_data_retention_by_notification_type, + mock_get_service_data_retention, mocker, mock_get_notifications, fake_uuid, @@ -486,7 +486,7 @@ def test_should_show_updates_for_one_job_as_json( mock_get_notifications, mock_get_service_template, mock_get_job, - mock_get_service_data_retention_by_notification_type, + mock_get_service_data_retention, mocker, fake_uuid, ): @@ -523,7 +523,7 @@ def test_should_show_letter_job_with_first_class_if_notifications_are_first_clas client_request, mock_get_service_letter_template, mock_get_job, - mock_get_service_data_retention_by_notification_type, + mock_get_service_data_retention, fake_uuid, active_user_with_permissions, mocker, @@ -547,14 +547,16 @@ def test_should_show_letter_job_with_first_class_if_notifications_are_first_clas @freeze_time("2016-01-01 11:09:00.061258") def test_should_show_letter_job_with_first_class_if_no_notifications( client_request, + service_one, mock_get_service_letter_template, mock_get_job, fake_uuid, mock_get_notifications_with_no_notifications, - mock_get_service_data_retention_by_notification_type, + mock_get_service_data_retention, mocker ): - mocker.patch('app.main.views.jobs.current_service', postage='first') + + service_one['postage'] = 'first' page = client_request.get( 'main.view_job', diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 8d92f80a9..77e7ee13f 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -2002,7 +2002,7 @@ def test_create_job_should_call_api( mock_get_job, mock_get_notifications, mock_get_service_template, - mock_get_service_data_retention_by_notification_type, + mock_get_service_data_retention, mocker, fake_uuid, when diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 391051cdb..611ab24e5 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -3327,6 +3327,9 @@ def test_show_service_data_retention( mock_get_service_data_retention, ): + + mock_get_service_data_retention.return_value[0]['days_of_retention'] = 5 + response = logged_in_platform_admin_client.get(url_for('main.data_retention', service_id=service_one['id'])) page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') rows = page.select('tbody tr') @@ -3366,12 +3369,12 @@ def test_update_service_data_retention( logged_in_platform_admin_client, service_one, fake_uuid, - mock_get_service_data_retention_by_id, + mock_get_service_data_retention, mock_update_service_data_retention, ): response = logged_in_platform_admin_client.post(url_for('main.edit_data_retention', service_id=service_one['id'], - data_retention_id=fake_uuid), + data_retention_id=str(fake_uuid)), data={'days_of_retention': 5} ) assert response.status_code == 302 @@ -3385,7 +3388,7 @@ def test_update_service_data_retention_return_validation_error_for_negative_days logged_in_platform_admin_client, service_one, fake_uuid, - mock_get_service_data_retention_by_id, + mock_get_service_data_retention, mock_update_service_data_retention, ): response = logged_in_platform_admin_client.post(url_for('main.edit_data_retention', @@ -3397,7 +3400,7 @@ def test_update_service_data_retention_return_validation_error_for_negative_days page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') error_message = page.find('span', class_='error-message').text.strip() assert error_message == 'Must be between 3 and 90' - assert mock_get_service_data_retention_by_id.called + assert mock_get_service_data_retention.called assert not mock_update_service_data_retention.called @@ -3405,8 +3408,10 @@ def test_update_service_data_retention_populates_form( logged_in_platform_admin_client, service_one, fake_uuid, - mock_get_service_data_retention_by_id, + mock_get_service_data_retention, ): + + mock_get_service_data_retention.return_value[0]['days_of_retention'] = 5 response = logged_in_platform_admin_client.get(url_for('main.edit_data_retention', service_id=service_one['id'], data_retention_id=fake_uuid) diff --git a/tests/conftest.py b/tests/conftest.py index 0d964d14b..9309b9710 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -536,14 +536,6 @@ def mock_get_service_statistics(mocker, api_user_active): return mocker.patch('app.service_api_client.get_service_statistics', side_effect=_get) -@pytest.fixture(scope='function') -def mock_get_service_data_retention_by_notification_type(mocker, api_user_active): - def _get(service_id, notification_type): - return {} - - return mocker.patch('app.service_api_client.get_service_data_retention_by_notification_type', side_effect=_get) - - @pytest.fixture(scope='function') def mock_get_detailed_services(mocker, fake_uuid): service_one = service_json( @@ -2831,11 +2823,11 @@ def normalize_spaces(input): @pytest.fixture(scope='function') def mock_get_service_data_retention(mocker): - data = {"id": str(fake_uuid), - "service_id": str(fake_uuid), + data = {"id": str(sample_uuid()), + "service_id": str(sample_uuid()), "service_name": "service name", "notification_type": "email", - "days_of_retention": 5, + "days_of_retention": 7, "created_at": datetime.now(), "updated_at": None, } @@ -2843,20 +2835,6 @@ def mock_get_service_data_retention(mocker): return_value=[data]) -@pytest.fixture(scope='function') -def mock_get_service_data_retention_by_id(mocker): - data = {"id": str(fake_uuid), - "service_id": str(fake_uuid), - "service_name": "service name", - "notification_type": "email", - "days_of_retention": 5, - "created_at": datetime.now(), - "updated_at": None, - } - return mocker.patch('app.service_api_client.get_service_data_retention_by_id', - return_value=data) - - @pytest.fixture(scope='function') def mock_create_service_data_retention(mocker): return mocker.patch('app.service_api_client.create_service_data_retention')