diff --git a/app/dao/provider_details_dao.py b/app/dao/provider_details_dao.py index ffa50c23e..bb130ffe1 100644 --- a/app/dao/provider_details_dao.py +++ b/app/dao/provider_details_dao.py @@ -2,8 +2,10 @@ from datetime import datetime from notifications_utils.timezones import convert_utc_to_bst from sqlalchemy import asc, desc, func +from flask import current_app from app.dao.dao_utils import transactional +from app.dao.users_dao import get_user_by_id from app.models import FactBilling, ProviderDetails, ProviderDetailsHistory, SMS_TYPE, User from app import db @@ -34,6 +36,9 @@ 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', @@ -41,11 +46,22 @@ def dao_reduce_sms_provider_priority(identifier): ).with_for_update() providers = {provider.identifier: provider for provider in q} - other = get_alternative_sms_provider(identifier) + other_identifier = get_alternative_sms_provider(identifier) + + reduced_provider = providers[identifier] + increased_provider = providers[other_identifier] # always keep values between 0 and 100 - providers[identifier].priority = max(0, providers[identifier].priority - 10) - providers[other].priority = min(100, providers[other].priority + 10) + reduced_provider.priority = max(0, reduced_provider.priority - 10) + increased_provider.priority = min(100, increased_provider.priority + 10) + + # 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_provider_details_without_commit(reduced_provider) + _update_provider_details_without_commit(increased_provider) def get_provider_details_by_notification_type(notification_type, supports_international=False): @@ -60,6 +76,13 @@ def get_provider_details_by_notification_type(notification_type, supports_intern @transactional def dao_update_provider_details(provider_details): + _update_provider_details_without_commit(provider_details) + + +def _update_provider_details_without_commit(provider_details): + """ + Doesn't commit, for when you need to control the database transaction manually + """ provider_details.version += 1 provider_details.updated_at = datetime.utcnow() history = ProviderDetailsHistory.from_original(provider_details) diff --git a/tests/app/dao/test_provider_details_dao.py b/tests/app/dao/test_provider_details_dao.py index 2ef98014d..a985510e2 100644 --- a/tests/app/dao/test_provider_details_dao.py +++ b/tests/app/dao/test_provider_details_dao.py @@ -2,6 +2,7 @@ import pytest from datetime import datetime from freezegun import freeze_time +from sqlalchemy.sql import desc from app.models import ProviderDetails, ProviderDetailsHistory from app import clients @@ -11,7 +12,6 @@ from app.dao.provider_details_dao import ( get_provider_details_by_notification_type, dao_update_provider_details, dao_get_provider_stats, - dao_get_provider_versions, dao_reduce_sms_provider_priority, ) from tests.app.db import ( @@ -137,9 +137,13 @@ def test_get_alternative_sms_provider_fails_if_unrecognised(): ]) def test_reduce_sms_provider_priority_switches_provider( notify_db_session, + mocker, + restore_provider_details, + sample_user, starting_priorities, - expected_priorities + expected_priorities, ): + mocker.patch('app.dao.provider_details_dao.get_user_by_id', return_value=sample_user) mmg = get_provider_details_by_identifier('mmg') firetext = get_provider_details_by_identifier('firetext') @@ -151,6 +155,8 @@ def test_reduce_sms_provider_priority_switches_provider( assert firetext.priority == expected_priorities['firetext'] assert mmg.priority == expected_priorities['mmg'] + assert mmg.created_by is sample_user + assert firetext.created_by is sample_user def test_reduce_sms_provider_priority_adds_rows_to_history_table( @@ -158,45 +164,25 @@ def test_reduce_sms_provider_priority_adds_rows_to_history_table( restore_provider_details, sample_user ): - raise NotImplementedError - # mocker.patch('app.provider_details.switch_providers.get_user_by_id', return_value=sample_user) - # provider_history_rows = ProviderDetailsHistory.query.filter( - # ProviderDetailsHistory.id == mmg.id - # ).order_by( - # desc(ProviderDetailsHistory.version) - # ).all() + mocker.patch('app.dao.provider_details_dao.get_user_by_id', return_value=sample_user) + mmg = get_provider_details_by_identifier('mmg') + provider_history_rows = ProviderDetailsHistory.query.filter( + ProviderDetailsHistory.id == mmg.id + ).order_by( + desc(ProviderDetailsHistory.version) + ).all() - # dao_toggle_sms_provider(mmg.identifier) + dao_reduce_sms_provider_priority(mmg.identifier) - # updated_provider_history_rows = ProviderDetailsHistory.query.filter( - # ProviderDetailsHistory.id == mmg.id - # ).order_by( - # desc(ProviderDetailsHistory.version) - # ).all() + updated_provider_history_rows = ProviderDetailsHistory.query.filter( + ProviderDetailsHistory.id == mmg.id + ).order_by( + desc(ProviderDetailsHistory.version) + ).all() - # assert len(updated_provider_history_rows) - len(provider_history_rows) == 1 - # assert updated_provider_history_rows[0].version - provider_history_rows[0].version == 1 - - -def test_reduce_sms_provider_priority_records_notify_user( - restore_provider_details, - sample_user, - mocker -): - raise NotImplementedError - # mocker.patch('app.provider_details.switch_providers.get_user_by_id', return_value=sample_user) - - # current_provider = get_current_provider('sms') - # dao_toggle_sms_provider(current_provider.identifier) - # new_provider = get_current_provider('sms') - - # assert current_provider.identifier != new_provider.identifier - # assert new_provider.created_by.id == sample_user.id - # assert new_provider.created_by_id == sample_user.id - - -def test_can_get_all_provider_history(notify_db_session): - assert len(dao_get_provider_versions('mmg')) == 1 + assert len(updated_provider_history_rows) - len(provider_history_rows) == 1 + assert updated_provider_history_rows[0].version - provider_history_rows[0].version == 1 + assert updated_provider_history_rows[0].priority == 90 @freeze_time('2018-06-28 12:00')