diff --git a/app/main/views/platform_admin.py b/app/main/views/platform_admin.py index 50917c819..eb7eb7647 100644 --- a/app/main/views/platform_admin.py +++ b/app/main/views/platform_admin.py @@ -368,6 +368,9 @@ def platform_admin_returned_letters(): try: letter_jobs_client.submit_returned_letters(references) + redis_client.delete_cache_keys_by_pattern( + 'service-????????-????-????-????-????????????-returned-letters-statistics' + ) redis_client.delete_cache_keys_by_pattern( 'service-????????-????-????-????-????????????-returned-letters-summary' ) diff --git a/app/models/service.py b/app/models/service.py index 20c841c33..c053438cc 100644 --- a/app/models/service.py +++ b/app/models/service.py @@ -1,8 +1,4 @@ -from datetime import datetime, timedelta - -from dateutil.parser import parse from flask import abort, current_app -from notifications_utils.timezones import local_timezone from werkzeug.utils import cached_property from app.models import JSONModel @@ -668,30 +664,25 @@ class Service(JSONModel): if test: yield BASE + '_incomplete' + tag + @cached_property + def returned_letter_statistics(self): + return service_api_client.get_returned_letter_statistics(self.id) + @cached_property def returned_letter_summary(self): return service_api_client.get_returned_letter_summary(self.id) @property - def most_recent_returned_letter_report(self): - if not self.returned_letter_summary: - return None - return parse( - self.returned_letter_summary[0]['reported_at'] + " 00:00:00" - ).replace(tzinfo=local_timezone) + def count_of_returned_letters_in_last_7_days(self): + return self.returned_letter_statistics['returned_letter_count'] @property - def count_of_returned_letters_in_last_7_days(self): - seven_days_ago = ( - datetime.now() - timedelta(days=7) - ).replace( - hour=0, minute=0, second=0 - ) - return sum( - report['returned_letter_count'] - for report in self.returned_letter_summary - if parse(report['reported_at'] + " 00:00:00") >= seven_days_ago - ) + def date_of_most_recent_returned_letter_report(self): + return self.returned_letter_statistics['most_recent_report'] + + @property + def has_returned_letters(self): + return bool(self.date_of_most_recent_returned_letter_report) @property def contact_lists(self): diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index b89e5c196..b0ad13904 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -569,6 +569,10 @@ class ServiceAPIClient(NotifyAdminAPIClient): def get_service_data_retention(self, service_id): return self.get("/service/{}/data-retention".format(service_id)) + @cache.set('service-{service_id}-returned-letters-statistics') + def get_returned_letter_statistics(self, service_id): + return self.get("service/{}/returned-letter-statistics".format(service_id)) + @cache.set('service-{service_id}-returned-letters-summary') def get_returned_letter_summary(self, service_id): return self.get("service/{}/returned-letter-summary".format(service_id)) diff --git a/app/templates/views/dashboard/_inbox.html b/app/templates/views/dashboard/_inbox.html index a73ee296c..901b23195 100644 --- a/app/templates/views/dashboard/_inbox.html +++ b/app/templates/views/dashboard/_inbox.html @@ -17,7 +17,7 @@ {% endif %} {% endif %} - {% if current_service.returned_letter_summary %} + {% if current_service.has_returned_letters %} {% endif %} diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index c5631ce2a..45ab72622 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/tests/app/main/views/test_accept_invite.py @@ -149,7 +149,7 @@ def test_accepting_invite_removes_invite_from_session( mock_get_billable_units, mock_get_free_sms_fragment_limit, mock_get_inbound_sms_summary, - mock_get_returned_letter_summary_with_no_returned_letters, + mock_get_returned_letter_statistics_with_no_returned_letters, fake_uuid, user, landing_page_title, @@ -477,7 +477,7 @@ def test_new_invited_user_verifies_and_added_to_service( mock_get_service_statistics, mock_get_usage, mock_get_free_sms_fragment_limit, - mock_get_returned_letter_summary_with_no_returned_letters, + mock_get_returned_letter_statistics_with_no_returned_letters, mock_create_event, mocker, ): diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index 208cd1e20..34d29dbd7 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -143,7 +143,7 @@ def test_get_started( mock_get_usage, mock_get_free_sms_fragment_limit, mock_get_inbound_sms_summary, - mock_get_returned_letter_summary_with_no_returned_letters, + mock_get_returned_letter_statistics_with_no_returned_letters, ): mocker.patch( 'app.template_statistics_client.get_template_statistics_for_service', @@ -168,7 +168,7 @@ def test_get_started_is_hidden_once_templates_exist( mock_get_usage, mock_get_free_sms_fragment_limit, mock_get_inbound_sms_summary, - mock_get_returned_letter_summary_with_no_returned_letters, + mock_get_returned_letter_statistics_with_no_returned_letters, ): mocker.patch( 'app.template_statistics_client.get_template_statistics_for_service', @@ -194,7 +194,7 @@ def test_inbound_messages_not_visible_to_service_without_permissions( mock_get_usage, mock_get_free_sms_fragment_limit, mock_get_inbound_sms_summary, - mock_get_returned_letter_summary_with_no_returned_letters, + mock_get_returned_letter_statistics_with_no_returned_letters, ): service_one['permissions'] = [] @@ -219,7 +219,7 @@ def test_inbound_messages_shows_count_of_messages_when_there_are_messages( mock_get_usage, mock_get_free_sms_fragment_limit, mock_get_inbound_sms_summary, - mock_get_returned_letter_summary_with_no_returned_letters, + mock_get_returned_letter_statistics_with_no_returned_letters, ): service_one['permissions'] = ['inbound_sms'] page = client_request.get( @@ -246,7 +246,7 @@ def test_inbound_messages_shows_count_of_messages_when_there_are_no_messages( mock_get_usage, mock_get_free_sms_fragment_limit, mock_get_inbound_sms_summary_with_no_messages, - mock_get_returned_letter_summary_with_no_returned_letters, + mock_get_returned_letter_statistics_with_no_returned_letters, ): service_one['permissions'] = ['inbound_sms'] page = client_request.get( @@ -488,7 +488,7 @@ def test_returned_letters_not_visible_if_service_has_no_returned_letters( mock_get_usage, mock_get_free_sms_fragment_limit, mock_get_inbound_sms_summary, - mock_get_returned_letter_summary_with_no_returned_letters, + mock_get_returned_letter_statistics_with_no_returned_letters, ): page = client_request.get( 'main.service_dashboard', @@ -511,21 +511,11 @@ def test_returned_letters_shows_count_of_recently_returned_letters( mock_get_inbound_sms_summary, ): mocker.patch( - 'app.service_api_client.get_returned_letter_summary', - return_value=[ - # Today (should be counted) - { - 'returned_letter_count': 1000, 'reported_at': '2020-01-10' - }, - # Just within the last 7 days (should be counted) - { - 'returned_letter_count': 3000, 'reported_at': '2020-01-3' - }, - # Just after the last 7 days (should not be counted) - { - 'returned_letter_count': 2000, 'reported_at': '2020-01-2' - }, - ], + 'app.service_api_client.get_returned_letter_statistics', + return_value={ + 'returned_letter_count': 4000, + 'most_recent_report': '2020-01-10', + }, ) page = client_request.get( 'main.service_dashboard', @@ -540,29 +530,29 @@ def test_returned_letters_shows_count_of_recently_returned_letters( ) -@pytest.mark.parametrize('reporting_date, expected_message', ( - ('2020-02-02', ( +@pytest.mark.parametrize('reporting_date, count, expected_message', ( + ('2020-02-02', 1, ( '1 returned letter latest report today' )), - ('2020-02-01', ( + ('2020-02-01', 1, ( '1 returned letter latest report yesterday' )), - ('2020-01-31', ( + ('2020-01-31', 1, ( '1 returned letter latest report 2 days ago' )), - ('2020-01-26', ( + ('2020-01-26', 1, ( '1 returned letter latest report 7 days ago' )), - ('2020-01-25', ( + ('2020-01-25', 0, ( '0 returned letters latest report 8 days ago' )), - ('2020-01-01', ( + ('2020-01-01', 0, ( '0 returned letters latest report 1 month ago' )), - ('2019-09-09', ( + ('2019-09-09', 0, ( '0 returned letters latest report 4 months ago' )), - ('2010-10-10', ( + ('2010-10-10', 0, ( '0 returned letters latest report 9 years ago' )), )) @@ -579,15 +569,15 @@ def test_returned_letters_only_counts_recently_returned_letters( mock_get_free_sms_fragment_limit, mock_get_inbound_sms_summary_with_no_messages, reporting_date, + count, expected_message, ): mocker.patch( - 'app.service_api_client.get_returned_letter_summary', - return_value=[ - { - 'returned_letter_count': 1, 'reported_at': reporting_date - }, - ], + 'app.service_api_client.get_returned_letter_statistics', + return_value={ + 'returned_letter_count': count, + 'most_recent_report': reporting_date, + }, ) page = client_request.get( 'main.service_dashboard', @@ -609,7 +599,7 @@ def test_should_show_recent_templates_on_dashboard( mock_get_usage, mock_get_free_sms_fragment_limit, mock_get_inbound_sms_summary, - mock_get_returned_letter_summary_with_no_returned_letters, + mock_get_returned_letter_statistics_with_no_returned_letters, ): mock_template_stats = mocker.patch('app.template_statistics_client.get_template_statistics_for_service', return_value=copy.deepcopy(stub_template_stats)) @@ -663,7 +653,7 @@ def test_should_not_show_recent_templates_on_dashboard_if_only_one_template_used mock_get_usage, mock_get_free_sms_fragment_limit, mock_get_inbound_sms_summary, - mock_get_returned_letter_summary_with_no_returned_letters, + mock_get_returned_letter_statistics_with_no_returned_letters, stats, ): mock_template_stats = mocker.patch( @@ -818,7 +808,7 @@ def test_should_show_upcoming_jobs_on_dashboard( mock_get_usage, mock_get_free_sms_fragment_limit, mock_get_inbound_sms_summary, - mock_get_returned_letter_summary_with_no_returned_letters, + mock_get_returned_letter_statistics_with_no_returned_letters, ): page = client_request.get( 'main.service_dashboard', @@ -875,7 +865,7 @@ def test_correct_font_size_for_big_numbers( mock_get_jobs, mock_get_usage, mock_get_free_sms_fragment_limit, - mock_get_returned_letter_summary_with_no_returned_letters, + mock_get_returned_letter_statistics_with_no_returned_letters, service_one, permissions, totals, @@ -912,7 +902,7 @@ def test_should_not_show_jobs_on_dashboard_for_users_with_uploads_page( mock_get_usage, mock_get_free_sms_fragment_limit, mock_get_inbound_sms_summary, - mock_get_returned_letter_summary_with_no_returned_letters, + mock_get_returned_letter_statistics_with_no_returned_letters, ): page = client_request.get( 'main.service_dashboard', @@ -1107,7 +1097,7 @@ def test_menu_send_messages( mock_get_usage, mock_get_inbound_sms_summary, mock_get_free_sms_fragment_limit, - mock_get_returned_letter_summary_with_no_returned_letters, + mock_get_returned_letter_statistics_with_no_returned_letters, ): service_one['permissions'] = ['email', 'sms', 'letter', 'upload_letters'] @@ -1143,7 +1133,7 @@ def test_menu_send_messages_when_service_does_not_have_upload_letters_permission mock_get_usage, mock_get_inbound_sms_summary, mock_get_free_sms_fragment_limit, - mock_get_returned_letter_summary_with_no_returned_letters, + mock_get_returned_letter_statistics_with_no_returned_letters, ): with app_.test_request_context(): resp = _test_dashboard_menu( @@ -1168,7 +1158,7 @@ def test_menu_manage_service( mock_get_service_statistics, mock_get_usage, mock_get_inbound_sms_summary, - mock_get_returned_letter_summary_with_no_returned_letters, + mock_get_returned_letter_statistics_with_no_returned_letters, mock_get_free_sms_fragment_limit, ): with app_.test_request_context(): @@ -1200,7 +1190,7 @@ def test_menu_manage_api_keys( mock_get_service_statistics, mock_get_usage, mock_get_inbound_sms_summary, - mock_get_returned_letter_summary_with_no_returned_letters, + mock_get_returned_letter_statistics_with_no_returned_letters, mock_get_free_sms_fragment_limit, ): with app_.test_request_context(): @@ -1230,7 +1220,7 @@ def test_menu_all_services_for_platform_admin_user( mock_get_service_statistics, mock_get_usage, mock_get_inbound_sms_summary, - mock_get_returned_letter_summary_with_no_returned_letters, + mock_get_returned_letter_statistics_with_no_returned_letters, mock_get_free_sms_fragment_limit, ): with app_.test_request_context(): @@ -1263,7 +1253,7 @@ def test_route_for_service_permissions( mock_get_usage, mock_get_free_sms_fragment_limit, mock_get_inbound_sms_summary, - mock_get_returned_letter_summary_with_no_returned_letters, + mock_get_returned_letter_statistics_with_no_returned_letters, ): with app_.test_request_context(): validate_route_permission( @@ -1317,7 +1307,7 @@ def test_service_dashboard_updates_gets_dashboard_totals( mock_get_usage, mock_get_free_sms_fragment_limit, mock_get_inbound_sms_summary, - mock_get_returned_letter_summary_with_no_returned_letters, + mock_get_returned_letter_statistics_with_no_returned_letters, ): mocker.patch('app.main.views.dashboard.get_dashboard_totals', return_value={ 'email': {'requested': 123, 'delivered': 0, 'failed': 0}, @@ -1500,7 +1490,7 @@ def test_org_breadcrumbs_do_not_show_if_service_has_no_org( mock_get_jobs, mock_get_usage, mock_get_free_sms_fragment_limit, - mock_get_returned_letter_summary_with_no_returned_letters, + mock_get_returned_letter_statistics_with_no_returned_letters, ): page = client_request.get('main.service_dashboard', service_id=SERVICE_ONE_ID) @@ -1515,7 +1505,7 @@ def test_org_breadcrumbs_do_not_show_if_user_is_not_an_org_member( active_caseworking_user, client_request, mock_get_template_folders, - mock_get_returned_letter_summary_with_no_returned_letters, + mock_get_returned_letter_statistics_with_no_returned_letters, ): # active_caseworking_user is not an org member @@ -1538,7 +1528,7 @@ def test_org_breadcrumbs_show_if_user_is_a_member_of_the_services_org( mock_get_jobs, mock_get_usage, mock_get_free_sms_fragment_limit, - mock_get_returned_letter_summary_with_no_returned_letters, + mock_get_returned_letter_statistics_with_no_returned_letters, active_user_with_permissions, client_request, ): @@ -1566,7 +1556,7 @@ def test_org_breadcrumbs_do_not_show_if_user_is_a_member_of_the_services_org_but mock_get_jobs, mock_get_usage, mock_get_free_sms_fragment_limit, - mock_get_returned_letter_summary_with_no_returned_letters, + mock_get_returned_letter_statistics_with_no_returned_letters, active_user_with_permissions, client_request, ): @@ -1591,7 +1581,7 @@ def test_org_breadcrumbs_show_if_user_is_platform_admin( mock_get_jobs, mock_get_usage, mock_get_free_sms_fragment_limit, - mock_get_returned_letter_summary_with_no_returned_letters, + mock_get_returned_letter_statistics_with_no_returned_letters, platform_admin_user, platform_admin_client, ): @@ -1623,7 +1613,7 @@ def test_should_show_usage_on_dashboard( mock_get_jobs, mock_get_usage, mock_get_free_sms_fragment_limit, - mock_get_returned_letter_summary_with_no_returned_letters, + mock_get_returned_letter_statistics_with_no_returned_letters, permissions, ): service_one['permissions'] = permissions diff --git a/tests/app/main/views/test_platform_admin.py b/tests/app/main/views/test_platform_admin.py index a157e3e46..84ce28a63 100644 --- a/tests/app/main/views/test_platform_admin.py +++ b/tests/app/main/views/test_platform_admin.py @@ -706,9 +706,10 @@ def test_platform_admin_submit_returned_letters(mocker, platform_admin_client): ) mock_client.assert_called_once_with(['REF1', 'REF2']) - redis.delete_cache_keys_by_pattern.assert_called_once_with( - 'service-????????-????-????-????-????????????-returned-letters-summary' - ) + assert redis.delete_cache_keys_by_pattern.call_args_list == [ + call('service-????????-????-????-????-????????????-returned-letters-statistics'), + call('service-????????-????-????-????-????????????-returned-letters-summary'), + ] assert response.status_code == 302 assert response.location == url_for('main.platform_admin_returned_letters', _external=True) diff --git a/tests/app/main/views/test_sign_out.py b/tests/app/main/views/test_sign_out.py index 2d1be4c2f..fdb604acf 100644 --- a/tests/app/main/views/test_sign_out.py +++ b/tests/app/main/views/test_sign_out.py @@ -31,7 +31,7 @@ def test_sign_out_user( mock_get_usage, mock_get_free_sms_fragment_limit, mock_get_inbound_sms_summary, - mock_get_returned_letter_summary_with_no_returned_letters, + mock_get_returned_letter_statistics_with_no_returned_letters ): with client_request.session_transaction() as session: assert session.get('user_id') is not None diff --git a/tests/app/notify_client/test_service_api_client.py b/tests/app/notify_client/test_service_api_client.py index de7e731be..ef8dfc33d 100644 --- a/tests/app/notify_client/test_service_api_client.py +++ b/tests/app/notify_client/test_service_api_client.py @@ -335,6 +335,25 @@ def test_client_returns_count_of_service_templates( ], {'data_from': 'api'}, ), + ( + service_api_client.get_returned_letter_statistics, + [SERVICE_ONE_ID], + [ + call('service-{}-returned-letters-statistics'.format(SERVICE_ONE_ID)) + ], + None, + [ + call('service/{}/returned-letter-statistics'.format(SERVICE_ONE_ID)) + ], + [ + call( + 'service-{}-returned-letters-statistics'.format(SERVICE_ONE_ID), + '{"data_from": "api"}', + ex=604800, + ) + ], + {'data_from': 'api'}, + ), ] ) def test_returns_value_from_cache( diff --git a/tests/conftest.py b/tests/conftest.py index 1510ccf1f..fa31dc42f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3496,6 +3496,17 @@ def mock_template_preview(mocker): mocker.patch('app.template_previews.TemplatePreview.from_utils_template', return_value=example_response) +@pytest.fixture(scope='function') +def mock_get_returned_letter_statistics_with_no_returned_letters(mocker): + return mocker.patch( + 'app.service_api_client.get_returned_letter_statistics', + return_value={ + 'returned_letter_count': 0, + 'most_recent_report': None, + }, + ) + + def create_api_user_active(with_unique_id=False): return { 'id': str(uuid4()) if with_unique_id else sample_uuid(),