From 28b03fe748a586f6687650223094a20a380e9930 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Mon, 13 Aug 2018 17:04:40 +0100 Subject: [PATCH 1/3] Add limit_days argument to statistics API client method Allows getting notification counts for a given number of days to support services with custom data retention periods (admin dashboard page should still display counts for the last 7 days, while the notifications page displays all stored notifications). --- app/notify_client/service_api_client.py | 6 ++++-- tests/app/notify_client/test_service_api_client.py | 14 ++++++++++---- tests/conftest.py | 2 +- 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index b2d2f79b5..1f5e74d16 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -41,8 +41,10 @@ class ServiceAPIClient(NotifyAdminAPIClient): """ return self.get('/service/{0}'.format(service_id)) - def get_service_statistics(self, service_id, today_only): - return self.get('/service/{0}/statistics'.format(service_id), params={'today_only': today_only})['data'] + def get_service_statistics(self, service_id, today_only, limit_days=None): + return self.get('/service/{0}/statistics'.format(service_id), params={ + 'today_only': today_only, 'limit_days': limit_days + })['data'] def get_services(self, params_dict=None): """ diff --git a/tests/app/notify_client/test_service_api_client.py b/tests/app/notify_client/test_service_api_client.py index 7bc4afd4f..92dac3626 100644 --- a/tests/app/notify_client/test_service_api_client.py +++ b/tests/app/notify_client/test_service_api_client.py @@ -35,15 +35,21 @@ def test_client_gets_service(mocker): mock_get.assert_called_once_with('/service/foo') -@pytest.mark.parametrize('today_only', [True, False]) -def test_client_gets_service_statistics(mocker, today_only): +@pytest.mark.parametrize('today_only, limit_days', [ + (True, None), + (False, None), + (False, 30), +]) +def test_client_gets_service_statistics(mocker, today_only, limit_days): client = ServiceAPIClient() mock_get = mocker.patch.object(client, 'get', return_value={'data': {'a': 'b'}}) - ret = client.get_service_statistics('foo', today_only) + ret = client.get_service_statistics('foo', today_only, limit_days) assert ret == {'a': 'b'} - mock_get.assert_called_once_with('/service/foo/statistics', params={'today_only': today_only}) + mock_get.assert_called_once_with('/service/foo/statistics', params={ + 'today_only': today_only, 'limit_days': limit_days + }) def test_client_only_updates_allowed_attributes(mocker): diff --git a/tests/conftest.py b/tests/conftest.py index eba37a8c0..dc642a0b5 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -523,7 +523,7 @@ def mock_get_international_service(mocker, api_user_active): @pytest.fixture(scope='function') def mock_get_service_statistics(mocker, api_user_active): - def _get(service_id, today_only): + def _get(service_id, today_only, limit_days=None): return { 'email': {'requested': 0, 'delivered': 0, 'failed': 0}, 'sms': {'requested': 0, 'delivered': 0, 'failed': 0}, From 52d48ad67909e27787a0d8b85a926d16ac9b16a6 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Mon, 13 Aug 2018 17:06:16 +0100 Subject: [PATCH 2/3] Add API client method to fetch service data retention by type --- app/notify_client/service_api_client.py | 3 +++ tests/app/main/views/test_activity.py | 12 ++++++++++++ tests/conftest.py | 8 ++++++++ 3 files changed, 23 insertions(+) diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index 1f5e74d16..73ac7b6f3 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -500,5 +500,8 @@ class ServiceAPIClient(NotifyAdminAPIClient): 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)) diff --git a/tests/app/main/views/test_activity.py b/tests/app/main/views/test_activity.py index 74d2e0c6b..5a0c2e397 100644 --- a/tests/app/main/views/test_activity.py +++ b/tests/app/main/views/test_activity.py @@ -70,6 +70,7 @@ def test_can_show_notifications( service_one, mock_get_notifications, mock_get_service_statistics, + mock_get_service_data_retention_by_notification_type, mock_has_no_jobs, user, extra_args, @@ -194,6 +195,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_has_no_jobs, user, query_parameters, @@ -229,6 +231,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_notifications( mocker, @@ -258,6 +261,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, letter_status, ): mock_get_notifications( @@ -280,6 +284,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_notifications_with_no_notifications, ): @@ -341,6 +346,7 @@ def test_search_recipient_form( logged_in_client, mock_get_notifications, mock_get_service_statistics, + mock_get_service_data_retention_by_notification_type, initial_query_arguments, form_post_data, expected_search_box_label, @@ -382,6 +388,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, mocker, ): response = logged_in_client.get(url_for( @@ -462,6 +469,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, mocker, ): response = logged_in_client.get(url_for( @@ -482,6 +490,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_notifications( mocker, @@ -514,6 +523,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, message_type, tablist_visible, search_bar_visible @@ -543,6 +553,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, message_type, hint_status_visible, mocker @@ -570,6 +581,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, mocker, fake_uuid, is_precompiled_letter, diff --git a/tests/conftest.py b/tests/conftest.py index dc642a0b5..2683c6533 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -533,6 +533,14 @@ 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( From e3a0adc293aed90d535cc83161895215a75bb55a Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Mon, 13 Aug 2018 17:07:49 +0100 Subject: [PATCH 3/3] Use per-service data retention period to display notifications page Uses the configured service data retention page to display retention period length, notification counts and fetch notifications from the API on the notifications page. --- app/main/views/jobs.py | 17 ++++++++++++++--- app/templates/views/activity/notifications.html | 2 +- app/templates/views/notifications.html | 2 +- tests/app/main/views/test_activity.py | 2 +- 4 files changed, 17 insertions(+), 6 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 10c8a91e4..9d0b22ab0 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -216,6 +216,11 @@ def get_notifications(service_id, message_type, status_override=None): abort(404) 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, 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'): return Response( generate_notifications_csv( @@ -224,7 +229,7 @@ def get_notifications(service_id, message_type, status_override=None): page_size=5000, template_type=[message_type], status=filter_args.get('status'), - limit_days=current_app.config['ACTIVITY_STATS_LIMIT_DAYS'] + limit_days=service_data_retention_days ), mimetype='text/csv', headers={ @@ -235,7 +240,7 @@ def get_notifications(service_id, message_type, status_override=None): page=page, template_type=[message_type] if message_type else [], status=filter_args.get('status'), - limit_days=current_app.config['ACTIVITY_STATS_LIMIT_DAYS'], + limit_days=service_data_retention_days, to=request.form.get('to', ''), ) url_args = { @@ -262,13 +267,18 @@ def get_notifications(service_id, message_type, status_override=None): download_link = None return { + 'service_data_retention_days': service_data_retention_days, 'counts': render_template( 'views/activity/counts.html', status=request.args.get('status'), status_filters=get_status_filters( current_service, message_type, - service_api_client.get_service_statistics(service_id, today_only=False) + service_api_client.get_service_statistics( + service_id, + today_only=False, + limit_days=service_data_retention_days + ) ) ), 'notifications': render_template( @@ -277,6 +287,7 @@ def get_notifications(service_id, message_type, status_override=None): notifications['notifications'] )), page=page, + limit_days=service_data_retention_days, prev_page=prev_page, next_page=next_page, status=request.args.get('status'), diff --git a/app/templates/views/activity/notifications.html b/app/templates/views/activity/notifications.html index a4bc2a3c8..d2b8e3465 100644 --- a/app/templates/views/activity/notifications.html +++ b/app/templates/views/activity/notifications.html @@ -11,7 +11,7 @@ notifications, caption="Recent activity", caption_visible=False, - empty_message='No messages found  (messages are kept for 7 days)'|safe, + empty_message='No messages found  (messages are kept for {} days)'.format(limit_days)|safe, field_headings=['Recipient', 'Status'], field_headings_visible=False ) %} diff --git a/app/templates/views/notifications.html b/app/templates/views/notifications.html index 6f361d708..41c89b5d6 100644 --- a/app/templates/views/notifications.html +++ b/app/templates/views/notifications.html @@ -58,7 +58,7 @@

Download this report   - Data available for 7 days + Data available for {{ partials.service_data_retention_days }} days

{% endif %} diff --git a/tests/app/main/views/test_activity.py b/tests/app/main/views/test_activity.py index 5a0c2e397..2dfc2a198 100644 --- a/tests/app/main/views/test_activity.py +++ b/tests/app/main/views/test_activity.py @@ -145,7 +145,7 @@ def test_can_show_notifications( **extra_args )) json_content = json.loads(json_response.get_data(as_text=True)) - assert json_content.keys() == {'counts', 'notifications'} + assert json_content.keys() == {'counts', 'notifications', 'service_data_retention_days'} @pytest.mark.parametrize('user, query_parameters, expected_download_link', [