#1800 Delivery Status Callbacks: VA Profile lookup failures

app/celery/common.py

@@ -26,7 +26,10 @@ def handle_max_retries_exceeded(
     notification_id: str,
     method_name: str,
 ) -> str:
-    """Handles sms/email deliver requests that exceeded the retry maximum, updates Notification status"""
+    """
+    Handles sms/email deliver requests that exceeded the retry maximum.  Updates the Notification status.
+    """
+
     current_app.logger.critical('%s: Notification %s failed by exceeding retry limits', method_name, notification_id)
     message = (
         'RETRY FAILED: Max retries reached. '

app/celery/contact_information_tasks.py

@@ -2,6 +2,7 @@
 from app import notify_celery, va_profile_client
 from app.celery.common import can_retry, handle_max_retries_exceeded
 from app.celery.exceptions import AutoRetryException
+from app.celery.service_callback_tasks import check_and_queue_callback_task
 from app.feature_flags import FeatureFlag, is_feature_enabled
 from app.va.identifier import IdentifierType
 from app.va.va_profile import VAProfileRetryableException, VAProfileNonRetryableException, NoContactInfoException
@@ -27,7 +28,7 @@ def lookup_contact_info(
     self,
     notification_id,
 ):
-    current_app.logger.info(f'Looking up contact information for notification_id:{notification_id}.')
+    current_app.logger.info('Looking up contact information for notification_id: %s.', notification_id)
 
     notification = get_notification_by_id(notification_id)
     va_profile_id = notification.recipient_identifiers[IdentifierType.VA_PROFILE_ID.value]
@@ -55,18 +56,20 @@ def lookup_contact_info(
             raise AutoRetryException(f'Found {type(e).__name__}, autoretrying...', e, e.args)
         else:
             msg = handle_max_retries_exceeded(notification_id, 'lookup_contact_info')
+            check_and_queue_callback_task(notification)
             raise NotificationTechnicalFailureException(msg)
     except NoContactInfoException as e:
         message = (
             f"Can't proceed after querying VA Profile for contact information for {notification_id}. "
             'Stopping execution of following tasks. Notification has been updated to permanent-failure.'
         )
-        current_app.logger.warning(f'{e.__class__.__name__} - {str(e)}: ' + message)
-        self.request.chain = None
+        current_app.logger.warning('%s - %s:  %s', e.__class__.__name__, str(e), message)
 
         update_notification_status_by_id(
             notification_id, NOTIFICATION_PERMANENT_FAILURE, status_reason=e.failure_reason
         )
+        check_and_queue_callback_task(notification)
+        raise NotificationPermanentFailureException(message) from e
 
     except (VAProfileIDNotFoundException, VAProfileNonRetryableException) as e:
         current_app.logger.exception(e)
@@ -77,6 +80,7 @@ def lookup_contact_info(
         update_notification_status_by_id(
             notification_id, NOTIFICATION_PERMANENT_FAILURE, status_reason=e.failure_reason
         )
+        check_and_queue_callback_task(notification)
         raise NotificationPermanentFailureException(message) from e
 
     else:

app/celery/lookup_va_profile_id_task.py

@@ -58,6 +58,7 @@ def lookup_va_profile_id(
             raise AutoRetryException('Found MpiRetryableException, autoretrying...', e, e.args)
         else:
             msg = handle_max_retries_exceeded(notification_id, 'lookup_va_profile_id')
+            check_and_queue_callback_task(notification)
             raise NotificationTechnicalFailureException(msg)
 
     except (
@@ -74,7 +75,6 @@ def lookup_va_profile_id(
             'Stopping execution of following tasks. Notification has been updated to permanent-failure.'
         )
         current_app.logger.warning(message)
-        self.request.chain = None
         notifications_dao.update_notification_status_by_id(
             notification_id, NOTIFICATION_PERMANENT_FAILURE, status_reason=e.failure_reason
         )
@@ -87,9 +87,9 @@ def lookup_va_profile_id(
             'Notification has been updated to technical-failure'
         )
         current_app.logger.exception(message)
-
         status_reason = e.failure_reason if hasattr(e, 'failure_reason') else 'Unknown error from MPI'
         notifications_dao.update_notification_status_by_id(
             notification_id, NOTIFICATION_TECHNICAL_FAILURE, status_reason=status_reason
         )
+        check_and_queue_callback_task(notification)
         raise NotificationTechnicalFailureException(message) from e

app/celery/service_callback_tasks.py

@@ -313,13 +313,16 @@ def check_and_queue_callback_task(
         service_id=notification.service_id, notification_status=notification.status
     )
 
-    # if a row of info is found
     if service_callback_api:
         # build dictionary for notification
         notification_data = create_delivery_status_callback_data(notification, service_callback_api, payload)
         send_delivery_status_to_service.apply_async(
             [service_callback_api.id, str(notification.id), notification_data], queue=QueueNames.CALLBACKS
         )
+    else:
+        current_app.logger.debug(
+            'No callbacks found for notification %s and service %s.', notification.id, notification.service_id
+        )
 
 
 def _check_and_queue_complaint_callback_task(

tests/app/celery/test_contact_information_tasks.py

@@ -26,20 +26,13 @@
 notification_id = str(uuid.uuid4())
 
 
-@pytest.fixture(scope='function')
-def notification():
-    recipient_identifier = RecipientIdentifier(
-        notification_id=notification_id, id_type=IdentifierType.VA_PROFILE_ID.value, id_value=EXAMPLE_VA_PROFILE_ID
+def test_should_get_email_address_and_update_notification(client, mocker, sample_template, sample_notification):
+    template = sample_template(template_type=EMAIL_TYPE)
+    notification = sample_notification(
+        template=template,
+        recipient_identifiers=[{'id_type': IdentifierType.VA_PROFILE_ID.value, 'id_value': EXAMPLE_VA_PROFILE_ID}],
     )
 
-    notification = Notification(id=notification_id)
-    notification.recipient_identifiers.set(recipient_identifier)
-    notification.notification_type = EMAIL_TYPE
-
-    return notification
-
-
-def test_should_get_email_address_and_update_notification(client, mocker, notification):
     mocked_get_notification_by_id = mocker.patch(
         'app.celery.contact_information_tasks.get_notification_by_id', return_value=notification
     )
@@ -58,8 +51,11 @@ def test_should_get_email_address_and_update_notification(client, mocker, notifi
     assert notification.to == 'test@test.org'
 
 
-def test_should_get_phone_number_and_update_notification(client, mocker, notification):
-    notification.notification_type = SMS_TYPE
+def test_should_get_phone_number_and_update_notification(client, mocker, sample_notification):
+    notification = sample_notification(
+        recipient_identifiers=[{'id_type': IdentifierType.VA_PROFILE_ID.value, 'id_value': EXAMPLE_VA_PROFILE_ID}]
+    )
+    assert notification.notification_type == SMS_TYPE
     mocked_get_notification_by_id = mocker.patch(
         'app.celery.contact_information_tasks.get_notification_by_id', return_value=notification
     )
@@ -78,9 +74,19 @@ def test_should_get_phone_number_and_update_notification(client, mocker, notific
     assert notification.to == '+15555555555'
 
 
-def test_should_not_retry_on_non_retryable_exception(client, mocker, notification):
+def test_should_not_retry_on_non_retryable_exception(client, mocker, sample_template, sample_notification):
+    template = sample_template(template_type=EMAIL_TYPE)
+    notification = sample_notification(
+        template=template,
+        recipient_identifiers=[{'id_type': IdentifierType.VA_PROFILE_ID.value, 'id_value': EXAMPLE_VA_PROFILE_ID}],
+    )
+
     mocker.patch('app.celery.contact_information_tasks.get_notification_by_id', return_value=notification)
 
+    mocked_check_and_queue_callback_task = mocker.patch(
+        'app.celery.contact_information_tasks.check_and_queue_callback_task',
+    )
+
     mocked_va_profile_client = mocker.Mock(VAProfileClient)
 
     exception = VAProfileNonRetryableException
@@ -91,51 +97,92 @@ def test_should_not_retry_on_non_retryable_exception(client, mocker, notificatio
         'app.celery.contact_information_tasks.update_notification_status_by_id'
     )
 
-    with pytest.raises(Exception) as exc_info:
+    with pytest.raises(NotificationPermanentFailureException) as exc_info:
         lookup_contact_info(notification.id)
 
-    assert exc_info.type is NotificationPermanentFailureException
     mocked_va_profile_client.get_email.assert_called_with(EXAMPLE_VA_PROFILE_ID)
 
     mocked_update_notification_status_by_id.assert_called_with(
         notification.id, NOTIFICATION_PERMANENT_FAILURE, status_reason=exception.failure_reason
     )
+    mocked_check_and_queue_callback_task.assert_called_once_with(notification)
 
 
 @pytest.mark.parametrize('exception_type', (Timeout, VAProfileRetryableException))
-def test_should_retry_on_retryable_exception(client, mocker, notification, exception_type):
+def test_should_retry_on_retryable_exception(client, mocker, sample_template, sample_notification, exception_type):
+    template = sample_template(template_type=EMAIL_TYPE)
+    notification = sample_notification(
+        template=template,
+        recipient_identifiers=[{'id_type': IdentifierType.VA_PROFILE_ID.value, 'id_value': EXAMPLE_VA_PROFILE_ID}],
+    )
     mocker.patch('app.celery.contact_information_tasks.get_notification_by_id', return_value=notification)
 
     mocked_va_profile_client = mocker.Mock(VAProfileClient)
     mocked_va_profile_client.get_email = mocker.Mock(side_effect=exception_type('some error'))
     mocker.patch('app.celery.contact_information_tasks.va_profile_client', new=mocked_va_profile_client)
 
-    with pytest.raises(Exception) as exc_info:
+    with pytest.raises(AutoRetryException) as exc_info:
         lookup_contact_info(notification.id)
 
-    assert exc_info.type is AutoRetryException
     mocked_va_profile_client.get_email.assert_called_with(EXAMPLE_VA_PROFILE_ID)
 
 
-def test_should_retry_on_timeout(client, mocker, notification):
+@pytest.mark.parametrize('notification_type', (SMS_TYPE, EMAIL_TYPE))
+@pytest.mark.parametrize('v3_enabled', (True, False))
+def test_lookup_contact_info_should_retry_on_timeout(
+    client, mocker, sample_template, sample_notification, notification_type, v3_enabled
+):
+    template = sample_template(template_type=notification_type)
+    notification = sample_notification(
+        template=template,
+        recipient_identifiers=[{'id_type': IdentifierType.VA_PROFILE_ID.value, 'id_value': EXAMPLE_VA_PROFILE_ID}],
+    )
+
     mocker.patch('app.celery.contact_information_tasks.get_notification_by_id', return_value=notification)
+    mocker.patch('app.celery.contact_information_tasks.is_feature_enabled', return_value=v3_enabled)
 
     mocked_va_profile_client = mocker.Mock(VAProfileClient)
-    mocked_va_profile_client.get_email = mocker.Mock(side_effect=Timeout('Request timed out'))
+
+    if notification_type == SMS_TYPE and v3_enabled:
+        mocked_va_profile_client.get_telephone_from_profile_v3 = mocker.Mock(side_effect=Timeout('Request timed out'))
+    elif notification_type == SMS_TYPE and not v3_enabled:
+        mocked_va_profile_client.get_telephone = mocker.Mock(side_effect=Timeout('Request timed out'))
+    elif notification_type == EMAIL_TYPE and v3_enabled:
+        mocked_va_profile_client.get_email_from_profile_v3 = mocker.Mock(side_effect=Timeout('Request timed out'))
+    elif notification_type == EMAIL_TYPE and not v3_enabled:
+        mocked_va_profile_client.get_email = mocker.Mock(side_effect=Timeout('Request timed out'))
+
     mocker.patch('app.celery.contact_information_tasks.va_profile_client', new=mocked_va_profile_client)
 
-    with pytest.raises(Exception) as exc_info:
+    with pytest.raises(AutoRetryException) as exc_info:
         lookup_contact_info(notification.id)
 
-    assert exc_info.type is AutoRetryException
-
     assert exc_info.value.args[0] == 'Found Timeout, autoretrying...'
     assert isinstance(exc_info.value.args[1], Timeout)
     assert str(exc_info.value.args[1]) == 'Request timed out'
-    mocked_va_profile_client.get_email.assert_called_with(EXAMPLE_VA_PROFILE_ID)
+
+    if notification_type == SMS_TYPE and v3_enabled:
+        mocked_va_profile_client.get_telephone_from_profile_v3.assert_called_with(
+            notification.recipient_identifiers[IdentifierType.VA_PROFILE_ID.value]
+        )
+    elif notification_type == SMS_TYPE and not v3_enabled:
+        mocked_va_profile_client.get_telephone.assert_called_with(EXAMPLE_VA_PROFILE_ID)
+    elif notification_type == EMAIL_TYPE and v3_enabled:
+        mocked_va_profile_client.get_email_from_profile_v3.assert_called_with(
+            notification.recipient_identifiers[IdentifierType.VA_PROFILE_ID.value]
+        )
+    elif notification_type == EMAIL_TYPE and not v3_enabled:
+        mocked_va_profile_client.get_email.assert_called_with(EXAMPLE_VA_PROFILE_ID)
 
 
-def test_should_update_notification_to_technical_failure_on_max_retries(client, mocker, notification):
+def test_should_update_notification_to_technical_failure_on_max_retries(
+    client, mocker, sample_template, sample_notification
+):
+    template = sample_template(template_type=EMAIL_TYPE)
+    notification = sample_notification(
+        template=template,
+        recipient_identifiers=[{'id_type': IdentifierType.VA_PROFILE_ID.value, 'id_value': EXAMPLE_VA_PROFILE_ID}],
+    )
     mocker.patch('app.celery.contact_information_tasks.get_notification_by_id', return_value=notification)
 
     mocked_va_profile_client = mocker.Mock(VAProfileClient)
@@ -146,44 +193,47 @@ def test_should_update_notification_to_technical_failure_on_max_retries(client,
         'app.celery.contact_information_tasks.handle_max_retries_exceeded'
     )
 
-    with pytest.raises(Exception) as exc_info:
+    with pytest.raises(NotificationTechnicalFailureException) as exc_info:
         lookup_contact_info(notification.id)
 
-    assert exc_info.type is NotificationTechnicalFailureException
     mocked_va_profile_client.get_email.assert_called_with(EXAMPLE_VA_PROFILE_ID)
 
     mocked_handle_max_retries_exceeded.assert_called_once()
 
 
-def test_should_update_notification_to_permanent_failure_on_no_contact_info_exception(client, mocker, notification):
+def test_should_update_notification_to_permanent_failure_on_no_contact_info_exception(
+    client, mocker, sample_template, sample_notification
+):
+    template = sample_template(template_type=EMAIL_TYPE)
+    notification = sample_notification(
+        template=template,
+        recipient_identifiers=[{'id_type': IdentifierType.VA_PROFILE_ID.value, 'id_value': EXAMPLE_VA_PROFILE_ID}],
+    )
     mocker.patch('app.celery.contact_information_tasks.get_notification_by_id', return_value=notification)
 
     mocked_va_profile_client = mocker.Mock(VAProfileClient)
     exception = NoContactInfoException
     mocked_va_profile_client.get_email = mocker.Mock(side_effect=exception)
     mocker.patch('app.celery.contact_information_tasks.va_profile_client', new=mocked_va_profile_client)
 
+    mocked_check_and_queue_callback_task = mocker.patch(
+        'app.celery.contact_information_tasks.check_and_queue_callback_task',
+    )
+
     mocked_update_notification_status_by_id = mocker.patch(
         'app.celery.contact_information_tasks.update_notification_status_by_id'
     )
 
-    # This explains the use of "type" below:
-    # https://docs.python.org/3.10/library/unittest.mock.html#unittest.mock.PropertyMock
-    mocked_request = mocker.Mock()
-    mocked_chain = mocker.PropertyMock()
-    mocked_chain.return_value = ['some-task-to-be-executed-next']
-    type(mocked_request).chain = mocked_chain
-    mocker.patch('celery.app.task.Task.request', new=mocked_request)
-
-    lookup_contact_info(notification.id)
+    with pytest.raises(NotificationPermanentFailureException) as exc_info:
+        lookup_contact_info(notification.id)
 
     mocked_va_profile_client.get_email.assert_called_with(EXAMPLE_VA_PROFILE_ID)
 
     mocked_update_notification_status_by_id.assert_called_with(
         notification.id, NOTIFICATION_PERMANENT_FAILURE, status_reason=exception.failure_reason
     )
 
-    mocked_chain.assert_called_with(None)
+    mocked_check_and_queue_callback_task.assert_called_once_with(notification)
 
 
 @pytest.mark.parametrize(
@@ -195,7 +245,12 @@ def test_should_update_notification_to_permanent_failure_on_no_contact_info_exce
             NOTIFICATION_TECHNICAL_FAILURE,
             RETRIES_EXCEEDED,
         ),
-        (NoContactInfoException, False, NOTIFICATION_PERMANENT_FAILURE, NoContactInfoException.failure_reason),
+        (
+            NoContactInfoException,
+            NotificationPermanentFailureException,
+            NOTIFICATION_PERMANENT_FAILURE,
+            NoContactInfoException.failure_reason,
+        ),
         (
             VAProfileNonRetryableException,
             NotificationPermanentFailureException,
@@ -205,14 +260,31 @@ def test_should_update_notification_to_permanent_failure_on_no_contact_info_exce
     ],
 )
 def test_exception_sets_failure_reason_if_thrown(
-    client, mocker, notification, exception, throws_additional_exception, notification_status, exception_reason
+    client,
+    mocker,
+    sample_template,
+    sample_notification,
+    exception,
+    throws_additional_exception,
+    notification_status,
+    exception_reason,
 ):
+    template = sample_template(template_type=EMAIL_TYPE)
+    notification = sample_notification(
+        template=template,
+        recipient_identifiers=[{'id_type': IdentifierType.VA_PROFILE_ID.value, 'id_value': EXAMPLE_VA_PROFILE_ID}],
+    )
     mocker.patch('app.celery.contact_information_tasks.get_notification_by_id', return_value=notification)
 
     mocked_va_profile_client = mocker.Mock(VAProfileClient)
     mocked_va_profile_client.get_email = mocker.Mock(side_effect=exception)
     mocker.patch('app.celery.contact_information_tasks.va_profile_client', new=mocked_va_profile_client)
     mocker.patch('app.celery.contact_information_tasks.can_retry', return_value=False)
+
+    mocked_check_and_queue_callback_task = mocker.patch(
+        'app.celery.contact_information_tasks.check_and_queue_callback_task',
+    )
+
     if exception_reason == RETRIES_EXCEEDED:
         mocker_handle_max_retries_exceeded = mocker.patch(
             'app.celery.contact_information_tasks.handle_max_retries_exceeded'
@@ -232,3 +304,5 @@ def test_exception_sets_failure_reason_if_thrown(
             mocked_update_notification_status_by_id.assert_called_once_with(
                 notification.id, notification_status, status_reason=exception_reason
             )
+
+    mocked_check_and_queue_callback_task.assert_called_once_with(notification)