fix more skips

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):
+def send_sms_response(provider, reference):
-    body = sns_callback(reference, to)
+    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):
+def sns_callback(notification_id):
-    raise Exception("Need to update for SNS callback format along with test_send_to_providers")
+    notification = get_notification_by_id(notification_id)
 
-    # example from mmg_callback
+    # This will only work if all notifications, including successful ones, are in the notifications table
-    # if to.strip().endswith(temp_fail):
+    # If we decide to delete successful notifications, we will have to get this from notifications history
-    #     # status: 4 - expired (temp failure)
+    print(f"NOTIFICATION_ID {str(notification_id)} status {notification.status} dtime {notification.completed_at}")
-    #     status = "4"
+    return json.dumps({
-    # elif to.strip().endswith(perm_fail):
+        "CID": str(notification_id),
-    #     # status: 5 - rejected (perm failure)
+        "status": notification.status,
-    #     status = "5"
+        # "deliverytime": notification.completed_at
-    # 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"})
 
 
 def ses_notification_callback(reference):

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:

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, mocker):
-def test_make_sns_callback(notify_api, rmock):
     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_delivered_sns_callback(mocker):
-def test_delievered_sns_callback():
+    get_notification_by_id = mocker.patch('app.celery.research_mode_tasks.get_notification_by_id')
-    phone_number = "2028675309"
+    n = Notification()
-    data = json.loads(sns_callback("1234", phone_number))
+    n.id = 1234
-    assert data['MSISDN'] == phone_number
+    n.status = NOTIFICATION_DELIVERED
-    assert data['status'] == "3"
+    get_notification_by_id.return_value = n
-    assert data['reference'] == "sns_reference"
+
-    assert data['CID'] == "1234"
+    data = json.loads(sns_callback("1234"))
-
+    assert data['status'] == "delivered"
-
-@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"
     assert data['CID'] == "1234"

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,

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(
+    send_sms_response.assert_called_once_with(
-        'sns', str(sample_notification.id), sample_notification.to
+        '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