From f2f0e5a0f10f60bd43e00436579df9dd2341b782 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 15 Aug 2023 14:50:41 -0700 Subject: [PATCH] code review feedback --- app/celery/scheduled_tasks.py | 8 --- app/dao/provider_details_dao.py | 53 ------------------- app/delivery/send_to_providers.py | 4 +- app/models.py | 9 ---- tests/app/celery/test_nightly_tasks.py | 4 -- tests/app/celery/test_provider_tasks.py | 18 ------- .../app/clients/test_performance_platform.py | 4 -- tests/app/dao/test_email_branding_dao.py | 10 ---- tests/app/dao/test_provider_details_dao.py | 53 ------------------- tests/app/delivery/test_send_to_providers.py | 4 +- 10 files changed, 2 insertions(+), 165 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index d3e76e584..bfbe4eca3 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -28,9 +28,6 @@ from app.dao.jobs_dao import ( find_missing_row_for_job, ) from app.dao.notifications_dao import notifications_not_yet_sent -from app.dao.provider_details_dao import ( - dao_adjust_provider_priority_back_to_resting_points, -) from app.dao.services_dao import ( dao_find_services_sending_to_tv_numbers, dao_find_services_with_high_failure_rates, @@ -85,11 +82,6 @@ def delete_invitations(): raise -@notify_celery.task(name='tend-providers-back-to-middle') -def tend_providers_back_to_middle(): - dao_adjust_provider_priority_back_to_resting_points() - - @notify_celery.task(name='check-job-status') def check_job_status(): """ diff --git a/app/dao/provider_details_dao.py b/app/dao/provider_details_dao.py index 02352f3e3..410ee9dba 100644 --- a/app/dao/provider_details_dao.py +++ b/app/dao/provider_details_dao.py @@ -74,59 +74,6 @@ def _get_sms_providers_for_update(time_threshold): return q -@autocommit -def dao_reduce_sms_provider_priority(identifier, *, time_threshold): - """ - Will reduce a chosen sms provider's priority, and increase the other provider's priority by 10 points each. - If either provider has been updated in the last `time_threshold`, then it won't take any action. - """ - # amount_to_reduce_by = 10 - providers_list = _get_sms_providers_for_update(time_threshold) - - if len(providers_list) < 2: - current_app.logger.info("Not adjusting providers, number of active providers is less than 2.") - return - - # TODO right now we only have one provider. Remove this? - # providers = {provider.identifier: provider for provider in providers_list} - # other_identifier = get_alternative_sms_provider(identifier) - # - # reduced_provider = providers[identifier] - # increased_provider = providers[other_identifier] - # - # # always keep values between 0 and 100 - # reduced_provider_priority = max(0, reduced_provider.priority - amount_to_reduce_by) - # increased_provider_priority = min(100, increased_provider.priority + amount_to_reduce_by) - # - # _adjust_provider_priority(reduced_provider, reduced_provider_priority) - # _adjust_provider_priority(increased_provider, increased_provider_priority) - - -@autocommit -def dao_adjust_provider_priority_back_to_resting_points(): - """ - Provided that neither SMS provider has been modified in the last hour, move both providers by 10 percentage points - each towards their defined resting points (set in SMS_PROVIDER_RESTING_POINTS in config.py). - """ - # TODO we only have one provider. Remove? - # amount_to_reduce_by = 10 - # time_threshold = timedelta(hours=1) - # - # providers = _get_sms_providers_for_update(time_threshold) - # - # for provider in providers: - # target = current_app.config['SMS_PROVIDER_RESTING_POINTS'][provider.identifier] - # current = provider.priority - # - # if current != target: - # if current > target: - # new_priority = max(target, provider.priority - amount_to_reduce_by) - # else: - # new_priority = min(target, provider.priority + amount_to_reduce_by) - # - # _adjust_provider_priority(provider, new_priority) - - def get_provider_details_by_notification_type(notification_type, supports_international=False): filters = [ProviderDetails.notification_type == notification_type] diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 79cefc2fb..caef65ef9 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -1,5 +1,5 @@ import random -from datetime import datetime, timedelta +from datetime import datetime from urllib import parse from cachetools import TTLCache, cached @@ -18,7 +18,6 @@ from app.celery.research_mode_tasks import ( from app.dao.email_branding_dao import dao_get_email_branding_by_id from app.dao.notifications_dao import dao_update_notification from app.dao.provider_details_dao import ( - dao_reduce_sms_provider_priority, get_provider_details_by_notification_type, ) from app.exceptions import NotificationTechnicalFailureException @@ -82,7 +81,6 @@ def send_sms_to_provider(notification): except Exception as e: notification.billable_units = template.fragment_count dao_update_notification(notification) - dao_reduce_sms_provider_priority(provider.name, time_threshold=timedelta(minutes=1)) raise e else: notification.billable_units = template.fragment_count diff --git a/app/models.py b/app/models.py index 184294487..145367af5 100644 --- a/app/models.py +++ b/app/models.py @@ -679,10 +679,7 @@ class ServiceInboundApi(db.Model, Versioned): @property def bearer_token(self): - # Column is non-nullable - # if self._bearer_token: return encryption.decrypt(self._bearer_token) - # return None @bearer_token.setter def bearer_token(self, bearer_token): @@ -719,10 +716,7 @@ class ServiceCallbackApi(db.Model, Versioned): @property def bearer_token(self): - # There is a non-null constraint on this column - # if self._bearer_token: return encryption.decrypt(self._bearer_token) - # return None @bearer_token.setter def bearer_token(self, bearer_token): @@ -777,10 +771,7 @@ class ApiKey(db.Model, Versioned): @property def secret(self): - # Column is non-nullable - # if self._secret: return encryption.decrypt(self._secret) - # return None @secret.setter def secret(self, secret): diff --git a/tests/app/celery/test_nightly_tasks.py b/tests/app/celery/test_nightly_tasks.py index 263a4739b..4ceb328f0 100644 --- a/tests/app/celery/test_nightly_tasks.py +++ b/tests/app/celery/test_nightly_tasks.py @@ -325,10 +325,6 @@ def test_delete_notifications_task_calls_task_for_services_that_have_sent_notifi ]) -def delete_notifications_by_service_and_type(id, param, param1): - pass - - def test_cleanup_unfinished_jobs(mocker): mock_s3 = mocker.patch('app.celery.nightly_tasks.remove_csv_object') mock_dao_archive = mocker.patch('app.celery.nightly_tasks.dao_archive_job') diff --git a/tests/app/celery/test_provider_tasks.py b/tests/app/celery/test_provider_tasks.py index 84382ef42..e5599bf65 100644 --- a/tests/app/celery/test_provider_tasks.py +++ b/tests/app/celery/test_provider_tasks.py @@ -42,24 +42,6 @@ def test_should_add_to_retry_queue_if_notification_not_found_in_deliver_sms_task app.celery.provider_tasks.deliver_sms.retry.assert_called_with(queue="retry-tasks", countdown=0) -def test_send_sms_should_not_switch_providers_on_non_provider_failure( - sample_notification, - mocker -): - mocker.patch( - 'app.delivery.send_to_providers.send_sms_to_provider', - side_effect=Exception("Non Provider Exception") - ) - mock_dao_reduce_sms_provider_priority = mocker.patch( - 'app.delivery.send_to_providers.dao_reduce_sms_provider_priority' - ) - mocker.patch('app.celery.provider_tasks.deliver_sms.retry') - - deliver_sms(sample_notification.id) - - assert mock_dao_reduce_sms_provider_priority.called is False - - def test_should_retry_and_log_warning_if_SmsClientResponseException_for_deliver_sms_task(sample_notification, mocker): mocker.patch( 'app.delivery.send_to_providers.send_sms_to_provider', diff --git a/tests/app/clients/test_performance_platform.py b/tests/app/clients/test_performance_platform.py index f90fbafdb..b25073e57 100644 --- a/tests/app/clients/test_performance_platform.py +++ b/tests/app/clients/test_performance_platform.py @@ -60,10 +60,6 @@ def test_should_raise_for_status(perf_client): perf_client.send_stats_to_performance_platform({'dataType': 'foo'}) -def generate_payload_id(payload, param): - pass - - def test_generate_payload_id(): payload = {'_timestamp': '2023-01-01 00:00:00', 'service': 'my_service', 'group_name': 'group_name', 'dataType': 'dataType', 'period': 'period'} diff --git a/tests/app/dao/test_email_branding_dao.py b/tests/app/dao/test_email_branding_dao.py index 6daa0b8cd..537f5ce57 100644 --- a/tests/app/dao/test_email_branding_dao.py +++ b/tests/app/dao/test_email_branding_dao.py @@ -6,16 +6,6 @@ from app.dao.email_branding_dao import ( from app.models import EmailBranding from tests.app.db import create_email_branding -# def test_get_email_branding_options_gets_all_email_branding(notify_db_session): -# email_branding_1 = create_email_branding(name='test_email_branding_1') -# email_branding_2 = create_email_branding(name='test_email_branding_2') -# -# email_branding = dao_get_email_branding_options() -# -# assert len(email_branding) == 2 -# assert email_branding_1 == email_branding[0] -# assert email_branding_2 == email_branding[1] - def test_get_email_branding_by_id_gets_correct_email_branding(notify_db_session): email_branding = create_email_branding() diff --git a/tests/app/dao/test_provider_details_dao.py b/tests/app/dao/test_provider_details_dao.py index 73335e8fa..85ad35e19 100644 --- a/tests/app/dao/test_provider_details_dao.py +++ b/tests/app/dao/test_provider_details_dao.py @@ -8,9 +8,7 @@ from app import notification_provider_clients from app.dao.provider_details_dao import ( _adjust_provider_priority, _get_sms_providers_for_update, - dao_adjust_provider_priority_back_to_resting_points, dao_get_provider_stats, - dao_reduce_sms_provider_priority, dao_update_provider_details, get_alternative_sms_provider, get_provider_details_by_identifier, @@ -196,57 +194,6 @@ def test_get_sms_providers_for_update_returns_nothing_if_recent_updates(restore_ assert not resp -def test_reduce_sms_provider_priority_does_nothing_if_providers_have_recently_changed( - mocker, - restore_provider_details, -): - mock_get_providers = mocker.patch('app.dao.provider_details_dao._get_sms_providers_for_update', return_value=[]) - mock_adjust = mocker.patch('app.dao.provider_details_dao._adjust_provider_priority') - - dao_reduce_sms_provider_priority('sns', time_threshold=timedelta(minutes=5)) - - mock_get_providers.assert_called_once_with(timedelta(minutes=5)) - assert mock_adjust.called is False - - -def test_reduce_sms_provider_priority_does_nothing_if_there_is_only_one_active_provider( - mocker, - restore_provider_details, -): - mock_adjust = mocker.patch('app.dao.provider_details_dao._adjust_provider_priority') - - dao_reduce_sms_provider_priority('sns', time_threshold=timedelta(minutes=5)) - - assert mock_adjust.called is False - - -def test_adjust_provider_priority_back_to_resting_points_does_nothing_if_theyre_already_at_right_values( - restore_provider_details, - mocker, -): - sns = get_provider_details_by_identifier('sns') - sns.priority = 100 - - mock_adjust = mocker.patch('app.dao.provider_details_dao._adjust_provider_priority') - mocker.patch('app.dao.provider_details_dao._get_sms_providers_for_update', return_value=[sns]) - - dao_adjust_provider_priority_back_to_resting_points() - - assert mock_adjust.called is False - - -def test_adjust_provider_priority_back_to_resting_points_does_nothing_if_no_providers_to_update( - restore_provider_details, - mocker, -): - mock_adjust = mocker.patch('app.dao.provider_details_dao._adjust_provider_priority') - mocker.patch('app.dao.provider_details_dao._get_sms_providers_for_update', return_value=[]) - - dao_adjust_provider_priority_back_to_resting_points() - - assert mock_adjust.called is False - - @freeze_time('2018-06-28 12:00') def test_dao_get_provider_stats(notify_db_session): service_1 = create_service(service_name='1') diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 4e2631b79..d4fd63e22 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -1,7 +1,7 @@ import json import uuid from collections import namedtuple -from datetime import datetime, timedelta +from datetime import datetime from unittest.mock import ANY import pytest @@ -595,7 +595,6 @@ def test_should_set_notification_billable_units_and_reduces_provider_priority_if mocker, ): mocker.patch('app.aws_sns_client.send_sms', side_effect=Exception()) - mock_reduce = mocker.patch('app.delivery.send_to_providers.dao_reduce_sms_provider_priority') sample_notification.billable_units = 0 assert sample_notification.sent_by is None @@ -608,7 +607,6 @@ def test_should_set_notification_billable_units_and_reduces_provider_priority_if assert 1 == 1 assert sample_notification.billable_units == 1 - mock_reduce.assert_called_once_with('sns', time_threshold=timedelta(minutes=1)) def test_should_send_sms_to_international_providers(