From ebb4a37f8d08152409f4052d7673327c40c7fe79 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Mon, 22 May 2023 09:12:12 -0700 Subject: [PATCH] fix more skips --- app/celery/research_mode_tasks.py | 33 ++++----- app/delivery/send_to_providers.py | 2 +- tests/app/celery/test_research_mode_tasks.py | 60 ++++++++--------- tests/app/dao/test_provider_details_dao.py | 70 -------------------- tests/app/delivery/test_send_to_providers.py | 56 ++-------------- 5 files changed, 46 insertions(+), 175 deletions(-) diff --git a/app/celery/research_mode_tasks.py b/app/celery/research_mode_tasks.py index 62344a41a..a72e16fef 100644 --- a/app/celery/research_mode_tasks.py +++ b/app/celery/research_mode_tasks.py @@ -5,6 +5,7 @@ from requests import HTTPError, request from app.celery.process_ses_receipts_tasks import process_ses_results from app.config import QueueNames +from app.dao.notifications_dao import get_notification_by_id from app.models import SMS_TYPE temp_fail = "2028675303" @@ -16,8 +17,8 @@ perm_fail_email = "perm-fail@simulator.notify" temp_fail_email = "temp-fail@simulator.notify" -def send_sms_response(provider, reference, to): - body = sns_callback(reference, to) +def send_sms_response(provider, reference): + body = sns_callback(reference) headers = {"Content-type": "application/json"} make_request(SMS_TYPE, provider, body, headers) @@ -59,25 +60,17 @@ def make_request(notification_type, provider, data, headers): return response.json() -def sns_callback(notification_id, to): - raise Exception("Need to update for SNS callback format along with test_send_to_providers") +def sns_callback(notification_id): + notification = get_notification_by_id(notification_id) - # example from mmg_callback - # if to.strip().endswith(temp_fail): - # # status: 4 - expired (temp failure) - # status = "4" - # elif to.strip().endswith(perm_fail): - # # status: 5 - rejected (perm failure) - # status = "5" - # else: - # # status: 3 - delivered - # status = "3" - - # return json.dumps({"reference": "mmg_reference", - # "CID": str(notification_id), - # "MSISDN": to, - # "status": status, - # "deliverytime": "2016-04-05 16:01:07"}) + # This will only work if all notifications, including successful ones, are in the notifications table + # If we decide to delete successful notifications, we will have to get this from notifications history + print(f"NOTIFICATION_ID {str(notification_id)} status {notification.status} dtime {notification.completed_at}") + return json.dumps({ + "CID": str(notification_id), + "status": notification.status, + # "deliverytime": notification.completed_at + }) def ses_notification_callback(reference): diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index 380ec7b4d..cd4a766a3 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -61,7 +61,7 @@ def send_sms_to_provider(notification): ) if service.research_mode or notification.key_type == KEY_TYPE_TEST: update_notification_to_sending(notification, provider) - send_sms_response(provider.name, str(notification.id), notification.to) + send_sms_response(provider.name, str(notification.id)) else: try: diff --git a/tests/app/celery/test_research_mode_tasks.py b/tests/app/celery/test_research_mode_tasks.py index 5bc401d54..6c9bad953 100644 --- a/tests/app/celery/test_research_mode_tasks.py +++ b/tests/app/celery/test_research_mode_tasks.py @@ -12,6 +12,7 @@ from app.celery.research_mode_tasks import ( sns_callback, ) from app.config import QueueNames +from app.models import NOTIFICATION_DELIVERED, NOTIFICATION_FAILED, Notification from tests.conftest import Matcher dvla_response_file_matcher = Matcher( @@ -20,24 +21,33 @@ dvla_response_file_matcher = Matcher( ) -@pytest.mark.skip(reason="Re-enable when SMS receipts exist") -def test_make_sns_callback(notify_api, rmock): +def test_make_sns_callback(notify_api, rmock, mocker): endpoint = "http://localhost:6011/notifications/sms/sns" + get_notification_by_id = mocker.patch('app.celery.research_mode_tasks.get_notification_by_id') + n = Notification() + n.id = 1234 + n.status = NOTIFICATION_DELIVERED + get_notification_by_id.return_value = n rmock.request( "POST", endpoint, json={"status": "success"}, status_code=200) - send_sms_response("sns", "1234", "2028675309") + send_sms_response("sns", "1234") assert rmock.called assert rmock.request_history[0].url == endpoint - assert json.loads(rmock.request_history[0].text)['MSISDN'] == '2028675309' + assert json.loads(rmock.request_history[0].text)['status'] == 'delivered' -@pytest.mark.skip(reason="Re-enable when SMS receipts exist") def test_callback_logs_on_api_call_failure(notify_api, rmock, mocker): endpoint = "http://localhost:6011/notifications/sms/sns" + get_notification_by_id = mocker.patch('app.celery.research_mode_tasks.get_notification_by_id') + n = Notification() + n.id = 1234 + n.status = NOTIFICATION_FAILED + get_notification_by_id.return_value = n + rmock.request( "POST", endpoint, @@ -46,12 +56,12 @@ def test_callback_logs_on_api_call_failure(notify_api, rmock, mocker): mock_logger = mocker.patch('app.celery.tasks.current_app.logger.error') with pytest.raises(HTTPError): - send_sms_response("mmg", "1234", "07700900001") + send_sms_response("sns", "1234") assert rmock.called assert rmock.request_history[0].url == endpoint mock_logger.assert_called_once_with( - 'API POST request on http://localhost:6011/notifications/sms/mmg failed with status 500' + 'API POST request on http://localhost:6011/notifications/sms/sns failed with status 500' ) @@ -65,31 +75,13 @@ def test_make_ses_callback(notify_api, mocker): assert mock_task.apply_async.call_args[0][0][0] == ses_notification_callback(some_ref) -@pytest.mark.skip(reason="Re-enable when SNS delivery receipts exist") -def test_delievered_sns_callback(): - phone_number = "2028675309" - data = json.loads(sns_callback("1234", phone_number)) - assert data['MSISDN'] == phone_number - assert data['status'] == "3" - assert data['reference'] == "sns_reference" - assert data['CID'] == "1234" - - -@pytest.mark.skip(reason="Re-enable when SNS delivery receipts exist") -def test_perm_failure_sns_callback(): - phone_number = "2028675302" - data = json.loads(sns_callback("1234", phone_number)) - assert data['MSISDN'] == phone_number - assert data['status'] == "5" - assert data['reference'] == "sns_reference" - assert data['CID'] == "1234" - - -@pytest.mark.skip(reason="Re-enable when SNS delivery receipts exist") -def test_temp_failure_sns_callback(): - phone_number = "2028675303" - data = json.loads(sns_callback("1234", phone_number)) - assert data['MSISDN'] == phone_number - assert data['status'] == "4" - assert data['reference'] == "sns_reference" +def test_delivered_sns_callback(mocker): + get_notification_by_id = mocker.patch('app.celery.research_mode_tasks.get_notification_by_id') + n = Notification() + n.id = 1234 + n.status = NOTIFICATION_DELIVERED + get_notification_by_id.return_value = n + + data = json.loads(sns_callback("1234")) + assert data['status'] == "delivered" assert data['CID'] == "1234" diff --git a/tests/app/dao/test_provider_details_dao.py b/tests/app/dao/test_provider_details_dao.py index 1ca40cedd..e55935c13 100644 --- a/tests/app/dao/test_provider_details_dao.py +++ b/tests/app/dao/test_provider_details_dao.py @@ -193,44 +193,6 @@ def test_get_sms_providers_for_update_returns_nothing_if_recent_updates(restore_ assert not resp -@pytest.mark.skip(reason="Reenable if/when we add a second SMS provider") -@pytest.mark.parametrize(['starting_priorities', 'expected_priorities'], [ - ({'sns': 50, 'other': 50}, {'sns': 40, 'other': 60}), - ({'sns': 0, 'other': 20}, {'sns': 0, 'other': 30}), # lower bound respected - ({'sns': 50, 'other': 100}, {'sns': 40, 'other': 100}), # upper bound respected - - # document what happens if they have unexpected values outside of the 0 - 100 range (due to manual setting from - # the admin app). the code never causes further issues, but sometimes doesn't actively reset the vaues to 0-100. - ({'sns': 150, 'other': 50}, {'sns': 140, 'other': 60}), - ({'sns': 50, 'other': 150}, {'sns': 40, 'other': 100}), - - ({'sns': -100, 'other': 50}, {'sns': 0, 'other': 60}), - ({'sns': 50, 'other': -100}, {'sns': 40, 'other': -90}), -]) -def test_reduce_sms_provider_priority_adjusts_provider_priorities( - mocker, - restore_provider_details, - notify_user, - starting_priorities, - expected_priorities, -): - mock_adjust = mocker.patch('app.dao.provider_details_dao._adjust_provider_priority') - - sns = get_provider_details_by_identifier('sns') - other = get_provider_details_by_identifier('other') - - sns.priority = starting_priorities['sns'] - other.priority = starting_priorities['other'] - # need to update these manually to avoid triggering the `onupdate` clause of the updated_at column - ProviderDetails.query.filter(ProviderDetails.notification_type == 'sms').update({'updated_at': datetime.min}) - - # switch away from sns. currently both 50/50 - dao_reduce_sms_provider_priority('sns', time_threshold=timedelta(minutes=10)) - - mock_adjust.assert_any_call(other, expected_priorities['other']) - mock_adjust.assert_any_call(sns, expected_priorities['sns']) - - def test_reduce_sms_provider_priority_does_nothing_if_providers_have_recently_changed( mocker, restore_provider_details, @@ -255,38 +217,6 @@ def test_reduce_sms_provider_priority_does_nothing_if_there_is_only_one_active_p assert mock_adjust.called is False -@pytest.mark.skip(reason="Reenable if/when we add a second SMS provider") -@pytest.mark.parametrize('existing_sns, existing_other, new_sns, new_other', [ - (50, 50, 60, 40), # not just 50/50 - 60/40 specifically - (65, 35, 60, 40), # doesn't overshoot if there's less than 10 difference - (0, 100, 10, 90), # only adjusts by 10 - (100, 100, 90, 90), # it tries to fix weird data - it will reduce both if needs be -]) -def test_adjust_provider_priority_back_to_resting_points_updates_all_providers( - restore_provider_details, - mocker, - existing_sns, - existing_other, - new_sns, - new_other -): - sns = get_provider_details_by_identifier('sns') - other = get_provider_details_by_identifier('other') - sns.priority = existing_sns - other.priority = existing_other - - mock_adjust = mocker.patch('app.dao.provider_details_dao._adjust_provider_priority') - mock_get_providers = mocker.patch('app.dao.provider_details_dao._get_sms_providers_for_update', return_value=[ - sns, other - ]) - - dao_adjust_provider_priority_back_to_resting_points() - - mock_get_providers.assert_called_once_with(timedelta(hours=1)) - mock_adjust.assert_any_call(sns, new_sns) - mock_adjust.assert_any_call(other, new_other) - - def test_adjust_provider_priority_back_to_resting_points_does_nothing_if_theyre_already_at_right_values( restore_provider_details, mocker, diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index c5bd57648..21e854c26 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -45,36 +45,6 @@ def setup_function(_function): send_to_providers.provider_cache.clear() -@pytest.mark.skip(reason="Reenable when we have more than 1 SMS provider") -def test_provider_to_use_should_return_random_provider(mocker, notify_db_session): - sns = get_provider_details_by_identifier('sns') - other = get_provider_details_by_identifier('other') - sns.priority = 60 - other.priority = 40 - mock_choices = mocker.patch('app.delivery.send_to_providers.random.choices', return_value=[sns]) - - ret = send_to_providers.provider_to_use('sms', international=True) - - mock_choices.assert_called_once_with([sns, other], weights=[60, 40]) - assert ret.name == 'sns' - - -@pytest.mark.skip(reason="Reenable when we have more than 1 SMS provider") -def test_provider_to_use_should_cache_repeated_calls(mocker, notify_db_session): - mock_choices = mocker.patch( - 'app.delivery.send_to_providers.random.choices', - wraps=send_to_providers.random.choices, - ) - - results = [ - send_to_providers.provider_to_use('sms', international=False) - for _ in range(10) - ] - - assert all(result == results[0] for result in results) - assert len(mock_choices.call_args_list) == 1 - - @pytest.mark.parametrize('international_provider_priority', ( # Since there’s only one international provider it should always # be used, no matter what its priority is set to @@ -93,18 +63,6 @@ def test_provider_to_use_should_only_return_sns_for_international( assert ret.name == 'sns' -@pytest.mark.skip(reason="Reenable when we have more than 1 SMS provider") -def test_provider_to_use_should_only_return_active_providers(mocker, restore_provider_details): - sns = get_provider_details_by_identifier('sns') - other = get_provider_details_by_identifier('other') - sns.active = False - other.active = True - - ret = send_to_providers.provider_to_use('sms') - - assert ret.name == 'other' - - def test_provider_to_use_raises_if_no_active_providers(mocker, restore_provider_details): sns = get_provider_details_by_identifier('sns') sns.active = False @@ -261,7 +219,7 @@ def test_should_call_send_sms_response_task_if_research_mode( notify_db_session, sample_service, sample_notification, mocker, research_mode, key_type ): mocker.patch('app.aws_sns_client.send_sms') - mocker.patch('app.delivery.send_to_providers.send_sms_response') + send_sms_response = mocker.patch('app.delivery.send_to_providers.send_sms_response') if research_mode: sample_service.research_mode = True @@ -275,8 +233,8 @@ def test_should_call_send_sms_response_task_if_research_mode( ) assert not aws_sns_client.send_sms.called - app.delivery.send_to_providers.send_sms_response.assert_called_once_with( - 'sns', str(sample_notification.id), sample_notification.to + send_sms_response.assert_called_once_with( + 'sns', str(sample_notification.id) ) persisted_notification = notifications_dao.get_notification_by_id(sample_notification.id) @@ -288,17 +246,15 @@ def test_should_call_send_sms_response_task_if_research_mode( assert not persisted_notification.personalisation -@pytest.mark.skip(reason="Needs updating when we get SMS delivery receipts done") def test_should_have_sending_status_if_fake_callback_function_fails(sample_notification, mocker): mocker.patch('app.delivery.send_to_providers.send_sms_response', side_effect=HTTPError) sample_notification.key_type = KEY_TYPE_TEST - with pytest.raises(HTTPError): send_to_providers.send_sms_to_provider( sample_notification ) - assert sample_notification.status == 'sending' + assert sample_notification.status == 'sent' assert sample_notification.sent_by == 'sns' @@ -389,14 +345,14 @@ def test_send_email_to_provider_should_call_research_mode_task_response_task_if_ reference = uuid.uuid4() mocker.patch('app.uuid.uuid4', return_value=reference) mocker.patch('app.aws_ses_client.send_email') - mocker.patch('app.delivery.send_to_providers.send_email_response') + send_email_response = mocker.patch('app.delivery.send_to_providers.send_email_response') send_to_providers.send_email_to_provider( notification ) assert not app.aws_ses_client.send_email.called - app.delivery.send_to_providers.send_email_response.assert_called_once_with(str(reference), 'john@smith.com') + send_email_response.assert_called_once_with(str(reference), 'john@smith.com') persisted_notification = Notification.query.filter_by(id=notification.id).one() assert persisted_notification.to == 'john@smith.com' assert persisted_notification.template_id == sample_email_template.id