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

@@ -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 {

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(

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):

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()

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,

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',

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

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)

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')