code review feedback

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():
     """

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]

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

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):

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')

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',

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'}

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()

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')

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(