From 9c03438a53d39d2250d2f682dcef82f1a629785e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 3 Mar 2020 17:12:45 +0000 Subject: [PATCH 1/5] Add an endpoint to return returned letter stats MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently the dashboard in the admin app pull the entire returned letter summary for a service to calculate how many letters have been returned in the last seven days. Adding a separate endpoint for this purpose is better because: - it’s a more efficient query - it’s less data to send down the pipe - it gives us a place to return the complete datetime, so the dashboard can be more precise about when the most recent report was --- app/dao/returned_letters_dao.py | 20 +++++++ app/service/rest.py | 23 +++++++- tests/app/dao/test_returned_letters_dao.py | 66 +++++++++++++++++++++- tests/app/service/test_rest.py | 14 +++++ 4 files changed, 121 insertions(+), 2 deletions(-) diff --git a/app/dao/returned_letters_dao.py b/app/dao/returned_letters_dao.py index 09dc95e00..c7a445156 100644 --- a/app/dao/returned_letters_dao.py +++ b/app/dao/returned_letters_dao.py @@ -13,6 +13,7 @@ from app.models import ( Template, User, ) +from app.utils import midnight_n_days_ago def _get_notification_ids_for_references(references): @@ -50,6 +51,25 @@ def insert_or_update_returned_letters(references): db.session.connection().execute(stmt) +def fetch_returned_letter_count(service_id): + return db.session.query( + func.count(ReturnedLetter.notification_id).label('returned_letter_count'), + ).filter( + ReturnedLetter.service_id == service_id, + ReturnedLetter.reported_at > midnight_n_days_ago(7), + ).one() + + +def fetch_most_recent_returned_letter(service_id): + return db.session.query( + ReturnedLetter.reported_at, + ).filter( + ReturnedLetter.service_id == service_id, + ).order_by( + desc(ReturnedLetter.reported_at) + ).first() + + def fetch_returned_letter_summary(service_id): return db.session.query( func.count(ReturnedLetter.notification_id).label('returned_letter_count'), diff --git a/app/service/rest.py b/app/service/rest.py index d0c46607d..2ea5a0e53 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -31,8 +31,10 @@ from app.dao.fact_notification_status_dao import ( from app.dao.inbound_numbers_dao import dao_allocate_number_for_service from app.dao.organisation_dao import dao_get_organisation_by_service_id from app.dao.returned_letters_dao import ( + fetch_most_recent_returned_letter, + fetch_returned_letter_count, fetch_returned_letter_summary, - fetch_returned_letters + fetch_returned_letters, ) from app.dao.service_data_retention_dao import ( fetch_service_data_retention, @@ -946,6 +948,25 @@ def check_if_reply_to_address_already_in_use(service_id, email_address): ) +@service_blueprint.route('//returned-letter-statistics', methods=['GET']) +def returned_letter_statistics(service_id): + + most_recent = fetch_most_recent_returned_letter(service_id) + + if not most_recent: + return jsonify({ + 'returned_letter_count': 0, + 'most_recent_report': None, + }) + + count = fetch_returned_letter_count(service_id) + + return jsonify({ + 'returned_letter_count': count.returned_letter_count, + 'most_recent_report': most_recent.reported_at.strftime(DATETIME_FORMAT_NO_TIMEZONE), + }) + + @service_blueprint.route('//returned-letter-summary', methods=['GET']) def returned_letter_summary(service_id): results = fetch_returned_letter_summary(service_id) diff --git a/tests/app/dao/test_returned_letters_dao.py b/tests/app/dao/test_returned_letters_dao.py index 17312ddfa..368f89175 100644 --- a/tests/app/dao/test_returned_letters_dao.py +++ b/tests/app/dao/test_returned_letters_dao.py @@ -1,14 +1,22 @@ +import uuid from datetime import datetime, timedelta, date from freezegun import freeze_time from app.dao.returned_letters_dao import ( insert_or_update_returned_letters, + fetch_most_recent_returned_letter, + fetch_returned_letter_count, fetch_returned_letter_summary, fetch_returned_letters ) from app.models import ReturnedLetter, NOTIFICATION_RETURNED_LETTER -from tests.app.db import create_notification, create_notification_history, create_returned_letter +from tests.app.db import ( + create_notification, + create_notification_history, + create_returned_letter, + create_service, +) def test_insert_or_update_returned_letters_inserts(sample_letter_template): @@ -90,6 +98,62 @@ def test_insert_or_update_returned_letters_with_duplicates_in_reference_list(sam assert x.notification_id in [notification_1.id, notification_2.id] +def test_get_returned_letter_count(sample_service): + # Before 7 days – don’t count + create_returned_letter( + sample_service, + reported_at=datetime(2001, 1, 1) + ) + create_returned_letter( + sample_service, + reported_at=datetime(2010, 11, 1, 23, 59, 59), + ) + # In the last 7 days – count + create_returned_letter( + sample_service, + reported_at=datetime(2010, 11, 2, 0, 0, 0), + ) + create_returned_letter( + sample_service, + reported_at=datetime(2010, 11, 8, 10, 0), + ) + create_returned_letter( + sample_service, + reported_at=datetime(2010, 11, 8, 10, 0), + ) + # Different service – don’t count + create_returned_letter( + create_service(service_id=uuid.uuid4(), service_name='Other service'), + reported_at=datetime(2010, 11, 8, 10, 0), + ) + + with freeze_time('2010-11-08 10:10'): + result = fetch_returned_letter_count(sample_service.id) + + assert result.returned_letter_count == 3 + + +def test_fetch_most_recent_returned_letter_for_service(sample_service): + # Older + create_returned_letter( + sample_service, + reported_at=datetime(2009, 9, 9, 9, 9), + ) + # Newer + create_returned_letter( + sample_service, + reported_at=datetime(2010, 10, 10, 10, 10), + ) + # Newest, but different service + create_returned_letter( + create_service(service_id=uuid.uuid4(), service_name='Other service'), + reported_at=datetime(2011, 11, 11, 11, 11), + ) + result = fetch_most_recent_returned_letter(sample_service.id) + + assert str(result.reported_at) == '2010-10-10' + + def test_get_returned_letter_summary(sample_service): now = datetime.utcnow() create_returned_letter(sample_service, reported_at=now) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 5d55b2572..fc643520c 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -3382,6 +3382,20 @@ def test_get_monthly_notification_data_by_service(mocker, admin_request): assert response == [] +@freeze_time('2019-12-11 13:30') +def test_get_returned_letter_statistics(admin_request, sample_service): + create_returned_letter(sample_service, reported_at=datetime.utcnow() - timedelta(days=3)) + create_returned_letter(sample_service, reported_at=datetime.utcnow() - timedelta(days=2)) + create_returned_letter(sample_service, reported_at=datetime.utcnow() - timedelta(days=1)) + + response = admin_request.get('service.returned_letter_statistics', service_id=sample_service.id) + + assert response == { + 'returned_letter_count': 3, + 'most_recent_report': '2019-12-10 00:00:00.000000' + } + + @freeze_time('2019-12-11 13:30') def test_get_returned_letter_summary(admin_request, sample_service): create_returned_letter(sample_service, reported_at=datetime.utcnow() - timedelta(days=3)) From 8c47d07845857199afe7c59c03765ea951b25248 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 5 Mar 2020 10:00:30 +0000 Subject: [PATCH 2/5] Optimise method to prevent redundant counting If we know that the most recently returned letter was reported more than 7 days ago then we know, without having to go to the database again, that the count of returned letters in the last 7 days is 0. --- app/service/rest.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/app/service/rest.py b/app/service/rest.py index 2ea5a0e53..9e2bef9fc 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -122,7 +122,7 @@ from app.schemas import ( email_data_request_schema ) from app.user.users_schema import post_set_permissions_schema -from app.utils import pagination_links +from app.utils import midnight_n_days_ago, pagination_links service_blueprint = Blueprint('service', __name__) @@ -959,6 +959,16 @@ def returned_letter_statistics(service_id): 'most_recent_report': None, }) + most_recent_reported_at = datetime.combine( + most_recent.reported_at, datetime.min.time() + ) + + if most_recent_reported_at < midnight_n_days_ago(7): + return jsonify({ + 'returned_letter_count': 0, + 'most_recent_report': most_recent.reported_at.strftime(DATETIME_FORMAT_NO_TIMEZONE), + }) + count = fetch_returned_letter_count(service_id) return jsonify({ From 70b2afa12415aaecdd4d358e0651665b06db24de Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 5 Mar 2020 10:04:20 +0000 Subject: [PATCH 3/5] =?UTF-8?q?Rename=20method=20to=20be=20clear=20it?= =?UTF-8?q?=E2=80=99s=20recent-only?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- app/dao/returned_letters_dao.py | 2 +- app/service/rest.py | 4 ++-- tests/app/dao/test_returned_letters_dao.py | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/dao/returned_letters_dao.py b/app/dao/returned_letters_dao.py index c7a445156..270435af9 100644 --- a/app/dao/returned_letters_dao.py +++ b/app/dao/returned_letters_dao.py @@ -51,7 +51,7 @@ def insert_or_update_returned_letters(references): db.session.connection().execute(stmt) -def fetch_returned_letter_count(service_id): +def fetch_recent_returned_letter_count(service_id): return db.session.query( func.count(ReturnedLetter.notification_id).label('returned_letter_count'), ).filter( diff --git a/app/service/rest.py b/app/service/rest.py index 9e2bef9fc..b542e2fd0 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -32,7 +32,7 @@ from app.dao.inbound_numbers_dao import dao_allocate_number_for_service from app.dao.organisation_dao import dao_get_organisation_by_service_id from app.dao.returned_letters_dao import ( fetch_most_recent_returned_letter, - fetch_returned_letter_count, + fetch_recent_returned_letter_count, fetch_returned_letter_summary, fetch_returned_letters, ) @@ -969,7 +969,7 @@ def returned_letter_statistics(service_id): 'most_recent_report': most_recent.reported_at.strftime(DATETIME_FORMAT_NO_TIMEZONE), }) - count = fetch_returned_letter_count(service_id) + count = fetch_recent_returned_letter_count(service_id) return jsonify({ 'returned_letter_count': count.returned_letter_count, diff --git a/tests/app/dao/test_returned_letters_dao.py b/tests/app/dao/test_returned_letters_dao.py index 368f89175..e85fb9e7f 100644 --- a/tests/app/dao/test_returned_letters_dao.py +++ b/tests/app/dao/test_returned_letters_dao.py @@ -6,7 +6,7 @@ from freezegun import freeze_time from app.dao.returned_letters_dao import ( insert_or_update_returned_letters, fetch_most_recent_returned_letter, - fetch_returned_letter_count, + fetch_recent_returned_letter_count, fetch_returned_letter_summary, fetch_returned_letters ) @@ -128,7 +128,7 @@ def test_get_returned_letter_count(sample_service): ) with freeze_time('2010-11-08 10:10'): - result = fetch_returned_letter_count(sample_service.id) + result = fetch_recent_returned_letter_count(sample_service.id) assert result.returned_letter_count == 3 From 02c824a980db5b3c11294de0eca663452bdb2da3 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 5 Mar 2020 10:18:10 +0000 Subject: [PATCH 4/5] Test for scenario when no returned letters exist --- tests/app/service/test_rest.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index fc643520c..4c3ee2b78 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -3396,6 +3396,26 @@ def test_get_returned_letter_statistics(admin_request, sample_service): } +def test_get_returned_letter_statistics_with_no_returned_letters( + mocker, + admin_request, + sample_service, +): + count_mock = mocker.patch( + 'app.service.rest.fetch_recent_returned_letter_count', + ) + + assert admin_request.get( + 'service.returned_letter_statistics', + service_id=sample_service.id, + ) == { + 'returned_letter_count': 0, + 'most_recent_report': None, + } + + assert count_mock.called is False + + @freeze_time('2019-12-11 13:30') def test_get_returned_letter_summary(admin_request, sample_service): create_returned_letter(sample_service, reported_at=datetime.utcnow() - timedelta(days=3)) From 6fd92c493146b77268575eea52baf0f97b25f745 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 5 Mar 2020 14:31:51 +0000 Subject: [PATCH 5/5] Test for letters not returned recently MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If you have some returned letters, but none in the last 7 days, we don’t count how many you have in the last 7 days. But we should test to make sure we’re not going to the database again. --- tests/app/service/test_rest.py | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 4c3ee2b78..423c5c1a5 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -3396,6 +3396,30 @@ def test_get_returned_letter_statistics(admin_request, sample_service): } +@freeze_time('2019-12-11 13:30') +def test_get_returned_letter_statistics_with_old_returned_letters( + mocker, + admin_request, + sample_service, +): + create_returned_letter(sample_service, reported_at=datetime.utcnow() - timedelta(days=8)) + create_returned_letter(sample_service, reported_at=datetime.utcnow() - timedelta(days=800)) + + count_mock = mocker.patch( + 'app.service.rest.fetch_recent_returned_letter_count', + ) + + assert admin_request.get( + 'service.returned_letter_statistics', + service_id=sample_service.id, + ) == { + 'returned_letter_count': 0, + 'most_recent_report': '2019-12-03 00:00:00.000000', + } + + assert count_mock.called is False + + def test_get_returned_letter_statistics_with_no_returned_letters( mocker, admin_request,