From 45c3e3c277582b5a757a13b7ea387e422ec793a5 Mon Sep 17 00:00:00 2001 From: Ryan Ahearn Date: Wed, 30 Nov 2022 13:50:49 -0500 Subject: [PATCH] Remove unused `is_delivery_slow_for_providers` method --- app/dao/notifications_dao.py | 63 +------------ .../notification_dao/test_notification_dao.py | 93 ------------------- 2 files changed, 2 insertions(+), 154 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index de0780c8d..47db5aba7 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -1,6 +1,4 @@ from datetime import datetime, timedelta -from itertools import groupby -from operator import attrgetter from botocore.exceptions import ClientError from flask import current_app @@ -16,14 +14,14 @@ from notifications_utils.timezones import ( convert_local_timezone_to_utc, convert_utc_to_local_timezone, ) -from sqlalchemy import and_, asc, desc, func, or_, union +from sqlalchemy import asc, desc, func, or_, union from sqlalchemy.orm import joinedload from sqlalchemy.orm.exc import NoResultFound from sqlalchemy.sql import functions from sqlalchemy.sql.expression import case from werkzeug.datastructures import MultiDict -from app import create_uuid, db, statsd_client +from app import create_uuid, db from app.dao.dao_utils import autocommit from app.letters.utils import LetterPDFNotFound, find_letter_pdf_in_s3 from app.models import ( @@ -32,7 +30,6 @@ from app.models import ( KEY_TYPE_TEST, LETTER_TYPE, NOTIFICATION_CREATED, - NOTIFICATION_DELIVERED, NOTIFICATION_PENDING, NOTIFICATION_PENDING_VIRUS_CHECK, NOTIFICATION_PERMANENT_FAILURE, @@ -44,7 +41,6 @@ from app.models import ( FactNotificationStatus, Notification, NotificationHistory, - ProviderDetails, ) from app.utils import ( escape_special_characters, @@ -453,61 +449,6 @@ def dao_timeout_notifications(cutoff_time, limit=100000): return notifications -def is_delivery_slow_for_providers( - created_at, - threshold, - delivery_time, -): - """ - Returns a dict of providers and whether they are currently slow or not. eg: - { - 'mmg': True, - 'firetext': False - } - """ - slow_notification_counts = db.session.query( - ProviderDetails.identifier, - case( - [( - Notification.status == NOTIFICATION_DELIVERED, - (Notification.updated_at - Notification.sent_at) >= delivery_time - )], - else_=(datetime.utcnow() - Notification.sent_at) >= delivery_time - ).label("slow"), - func.count().label('count') - ).select_from( - ProviderDetails - ).outerjoin( - Notification, and_( - Notification.notification_type == SMS_TYPE, - Notification.sent_by == ProviderDetails.identifier, - Notification.created_at >= created_at, - Notification.sent_at.isnot(None), - Notification.status.in_([NOTIFICATION_DELIVERED, NOTIFICATION_PENDING, NOTIFICATION_SENDING]), - Notification.key_type != KEY_TYPE_TEST - ) - ).filter( - ProviderDetails.notification_type == 'sms', - ProviderDetails.active - ).order_by( - ProviderDetails.identifier - ).group_by( - ProviderDetails.identifier, - "slow" - ) - - slow_providers = {} - for provider, rows in groupby(slow_notification_counts, key=attrgetter('identifier')): - rows = list(rows) - total_notifications = sum(row.count for row in rows) - slow_notifications = sum(row.count for row in rows if row.slow) - - slow_providers[provider] = (slow_notifications / total_notifications >= threshold) - statsd_client.gauge(f'slow-delivery.{provider}.ratio', slow_notifications / total_notifications) - - return slow_providers - - @autocommit def dao_update_notifications_by_reference(references, update_dict): updated_count = Notification.query.filter( diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index dc217a5c0..b0dfd8482 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -25,7 +25,6 @@ from app.dao.notifications_dao import ( get_notifications_for_job, get_notifications_for_service, get_service_ids_with_notifications_on_date, - is_delivery_slow_for_providers, notifications_not_yet_sent, update_notification_status_by_id, update_notification_status_by_reference, @@ -36,12 +35,9 @@ from app.models import ( KEY_TYPE_TEAM, KEY_TYPE_TEST, NOTIFICATION_DELIVERED, - NOTIFICATION_PENDING, - NOTIFICATION_SENDING, NOTIFICATION_SENT, NOTIFICATION_STATUS_TYPES, NOTIFICATION_STATUS_TYPES_FAILED, - NOTIFICATION_TEMPORARY_FAILURE, SMS_TYPE, Job, Notification, @@ -928,95 +924,6 @@ def test_should_exclude_test_key_notifications_by_default( assert len(all_notifications) == 1 -@pytest.mark.parametrize( - "normal_sending,slow_sending,normal_delivered,slow_delivered,threshold,expected_result", - [ - (0, 0, 0, 0, 0.1, False), - (1, 0, 0, 0, 0.1, False), - (1, 1, 0, 0, 0.1, True), - (0, 0, 1, 1, 0.1, True), - (1, 1, 1, 1, 0.5, True), - (1, 1, 1, 1, 0.6, False), - (45, 5, 45, 5, 0.1, True), - ] -) -@freeze_time("2018-12-04 12:00:00.000000") -def test_is_delivery_slow_for_providers( - notify_db_session, - sample_template, - normal_sending, - slow_sending, - normal_delivered, - slow_delivered, - threshold, - expected_result -): - normal_notification = partial( - create_notification, - template=sample_template, - sent_by='sns', - sent_at=datetime.now(), - updated_at=datetime.now() - ) - - slow_notification = partial( - create_notification, - template=sample_template, - sent_by='sns', - sent_at=datetime.now() - timedelta(minutes=5), - updated_at=datetime.now() - ) - - for _ in range(normal_sending): - normal_notification(status='sending') - for _ in range(slow_sending): - slow_notification(status='sending') - for _ in range(normal_delivered): - normal_notification(status='delivered') - for _ in range(slow_delivered): - slow_notification(status='delivered') - - result = is_delivery_slow_for_providers(datetime.utcnow(), threshold, timedelta(minutes=4)) - assert result == { - 'firetext': False, - 'mmg': False, - 'sns': expected_result - } - - -@pytest.mark.skip(reason="Needs updating for TTS: Failing for unknown reason") -@pytest.mark.parametrize("options,expected_result", [ - ({"status": NOTIFICATION_DELIVERED, "sent_by": "mmg"}, True), - ({"status": NOTIFICATION_PENDING, "sent_by": "mmg"}, True), - ({"status": NOTIFICATION_SENDING, "sent_by": "mmg"}, True), - - ({"status": NOTIFICATION_TEMPORARY_FAILURE, "sent_by": "mmg"}, False), - ({"status": NOTIFICATION_DELIVERED, "sent_by": "mmg", "sent_at": None}, False), - ({"status": NOTIFICATION_DELIVERED, "sent_by": "mmg", "key_type": KEY_TYPE_TEST}, False), - ({"status": NOTIFICATION_SENDING, "sent_by": "firetext"}, False), - ({"status": NOTIFICATION_DELIVERED, "sent_by": "firetext"}, False), - -]) -@freeze_time("2018-12-04 12:00:00.000000") -def test_delivery_is_delivery_slow_for_providers_filters_out_notifications_it_should_not_count( - notify_db_session, - sample_template, - options, - expected_result -): - create_slow_notification_with = { - "template": sample_template, - "sent_at": datetime.now() - timedelta(minutes=5), - "updated_at": datetime.now(), - } - create_slow_notification_with.update(options) - create_notification( - **create_slow_notification_with - ) - result = is_delivery_slow_for_providers(datetime.utcnow(), 0.1, timedelta(minutes=4)) - assert result['mmg'] == expected_result - - def test_dao_get_notifications_by_recipient(sample_template): recipient_to_search_for = {