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