make 500 error provider switches also check for recent changes

moving the logic and the test from switch provider on slow delivery to
dao reduce sms provider priority

app/celery/scheduled_tasks.py

@@ -35,7 +35,6 @@ from app.dao.notifications_dao import (
 )
 from app.dao.provider_details_dao import dao_reduce_sms_provider_priority
 from app.dao.users_dao import delete_codes_older_created_more_than_a_day_ago
-from app.dao.provider_details_dao import get_provider_details_by_notification_type
 from app.models import (
     Job,
     JOB_STATUS_IN_PROGRESS,
@@ -111,13 +110,6 @@ def switch_current_sms_provider_on_slow_delivery():
     in the last ten minutes. If both providers are slow, don't do anything. If we changed the providers in the
     last ten minutes, then don't update them again either.
     """
-    providers = get_provider_details_by_notification_type('sms')
-    # if something updated recently, don't update again. If the updated_at is null, set it to min time
-    # just to prevent errors
-    if any((provider.updated_at or datetime.min) > datetime.utcnow() - timedelta(minutes=10) for provider in providers):
-        current_app.logger.info("Slow delivery notifications provider switched less than 10 minutes ago.")
-        return
-
     slow_delivery_notifications = is_delivery_slow_for_providers(
         threshold=0.3,
         created_at=datetime.utcnow() - timedelta(minutes=10),

app/dao/provider_details_dao.py

@@ -1,4 +1,4 @@
-from datetime import datetime
+from datetime import datetime, timedelta
 
 from notifications_utils.timezones import convert_utc_to_bst
 from sqlalchemy import asc, desc, func
@@ -36,30 +36,47 @@ def dao_get_provider_versions(provider_id):
 
 @transactional
 def dao_reduce_sms_provider_priority(identifier):
-    # TODO: do we want to hold off on reducing priority if we've already adjusted priorities recently?
-    # do we want to do anything differently between slow delivery vs 500s?
-
     # get current priority of both providers
     q = ProviderDetails.query.filter(
         ProviderDetails.notification_type == 'sms',
         ProviderDetails.active
-    ).with_for_update()
+    ).with_for_update().all()
 
     providers = {provider.identifier: provider for provider in q}
     other_identifier = get_alternative_sms_provider(identifier)
 
+    # if something updated recently, don't update again. If the updated_at is null, treat it as min time
+    if any((provider.updated_at or datetime.min) > datetime.utcnow() - timedelta(minutes=10) for provider in q):
+        current_app.logger.info("Not adjusting providers, providers updated less than 10 minutes ago.")
+        return
+
     reduced_provider = providers[identifier]
     increased_provider = providers[other_identifier]
+    pre_reduction_priority = reduced_provider.priority
+    pre_increase_priority = increased_provider.priority
 
     # always keep values between 0 and 100
     reduced_provider.priority = max(0, reduced_provider.priority - 10)
     increased_provider.priority = min(100, increased_provider.priority + 10)
 
+    current_app.logger.info('Adjusting provider priority - {} going from {} to {}'.format(
+        reduced_provider.identifier,
+        reduced_provider.priority,
+        pre_reduction_priority
+    ))
+    current_app.logger.info('Adjusting provider priority - {} going from {} to {}'.format(
+        increased_provider.identifier,
+        increased_provider.priority,
+        pre_increase_priority
+    ))
+
     # Automatic update so set as notify user
     notify_user = get_user_by_id(current_app.config['NOTIFY_USER_ID'])
     reduced_provider.created_by_id = notify_user.id
     increased_provider.created_by_id = notify_user.id
 
+    # update without commit so that both rows can be changed without ending the transaction
+    # and releasing the for_update lock
     _update_provider_details_without_commit(reduced_provider)
     _update_provider_details_without_commit(increased_provider)
 

tests/app/celery/test_scheduled_tasks.py

@@ -21,7 +21,7 @@ from app.celery.scheduled_tasks import (
 from app.config import QueueNames, TaskNames
 from app.dao.jobs_dao import dao_get_job_by_id
 from app.dao.notifications_dao import dao_get_scheduled_notifications
-from app.dao.provider_details_dao import get_provider_details_by_notification_type, get_provider_details_by_identifier
+from app.dao.provider_details_dao import get_provider_details_by_identifier
 from app.models import (
     JOB_STATUS_IN_PROGRESS,
     JOB_STATUS_ERROR,
@@ -122,21 +122,6 @@ def test_switch_current_sms_provider_on_slow_delivery_switches_when_one_provider
     mock_reduce.assert_called_once_with('firetext')
 
 
-@freeze_time('2017-05-01 14:00:00')
-def test_switch_current_sms_provider_on_slow_delivery_does_nothing_if_recent_changes(
-    mocker,
-    restore_provider_details,
-):
-    mock_is_slow = mocker.patch('app.celery.scheduled_tasks.is_delivery_slow_for_providers')
-    mock_reduce = mocker.patch('app.celery.scheduled_tasks.dao_reduce_sms_provider_priority')
-    get_provider_details_by_identifier('mmg').updated_at = datetime(2017, 5, 1, 13, 51)
-
-    switch_current_sms_provider_on_slow_delivery()
-
-    assert mock_is_slow.called is False
-    assert mock_reduce.called is False
-
-
 @freeze_time('2017-05-01 14:00:00')
 @pytest.mark.parametrize('is_slow_dict', [
     {'mmg': False, 'firetext': False},
@@ -151,7 +136,6 @@ def test_switch_current_sms_provider_on_slow_delivery_does_nothing_if_no_need(
     mock_reduce = mocker.patch('app.celery.scheduled_tasks.dao_reduce_sms_provider_priority')
     get_provider_details_by_identifier('mmg').updated_at = datetime(2017, 5, 1, 13, 51)
 
-
     switch_current_sms_provider_on_slow_delivery()
 
     assert mock_reduce.called is False

tests/app/dao/test_provider_details_dao.py

@@ -185,6 +185,21 @@ def test_reduce_sms_provider_priority_adds_rows_to_history_table(
     assert updated_provider_history_rows[0].priority == 90
 
 
+@freeze_time('2017-05-01 14:00:00')
+def test_reduce_sms_provider_priority_does_nothing_if_providers_have_recently_changed(
+    mocker,
+    restore_provider_details,
+):
+    mock_is_slow = mocker.patch('app.celery.scheduled_tasks.is_delivery_slow_for_providers')
+    mock_reduce = mocker.patch('app.celery.scheduled_tasks.dao_reduce_sms_provider_priority')
+    get_provider_details_by_identifier('mmg').updated_at = datetime(2017, 5, 1, 13, 51)
+
+    dao_reduce_sms_provider_priority('firetext')
+
+    assert mock_is_slow.called is False
+    assert mock_reduce.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')