HOTFIX - Updated Permission Logic (#2007)

.talismanrc

@@ -1,23 +1,24 @@
 fileignoreconfig:
-  - filename: README.md
-    checksum: b2cbbb8508af49abccae8b35b317f7ce09215f3508430ed31440add78f450e5a
-  - filename: app/notifications/process_notifications.py
-    checksum: ae4e31c6eb56d91ec80ae09d13baf4558cf461c65f08893b93fee43f036a17a7
-  - filename: tests/app/notifications/test_process_notifications_for_profile_v3.py
-    checksum: 4e15e63d349635131173ffdd7aebcd547621db08de877ef926d3a41fde72d065
-  - filename: poetry.lock
-    checksum: 34d12acdf749363555c31add4e7e7afa9e2a27afd792bd98c85f331b87bd7112
-  - filename: app/template/rest.py
-    checksum: 1e5bdac8bc694d50f8f656dec127dd036b7b1b5b6156e3282d3411956c71ba0b
-  - filename: app/service/rest.py
-    checksum: b42aefd1ae0e6ea76e75db4cf14d425facd0941943b17f7ba2e41f850ad1ec23
-  - filename: app/celery/contact_information_tasks.py
-    checksum: 80d0acf88bafb1358583016b9e143f4523ef1160d6eacdc9754ca68859b90eae
-  - filename: lambda_functions/pinpoint_callback/pinpoint_callback_lambda.py
-    checksum: 7bd4900e14b1fa789bbb2568b8a8d7a400e3c8350ba32fb44cc0b5b66a2df037
-  - filename: lambda_functions/ses_callback/ses_callback_lambda.py
-    checksum: b20c36921290a9609f158784e2a3278c36190887e6054ea548004a67675fd79b
-  - filename: scripts/trigger_task.py
-    checksum: 0e9d244dbe285de23fc84bb643407963dacf7d25a3358373f01f6272fb217778
-
+- filename: README.md
+  checksum: b2cbbb8508af49abccae8b35b317f7ce09215f3508430ed31440add78f450e5a
+- filename: app/celery/contact_information_tasks.py
+  checksum: 80d0acf88bafb1358583016b9e143f4523ef1160d6eacdc9754ca68859b90eae
+- filename: app/notifications/process_notifications.py
+  checksum: ae4e31c6eb56d91ec80ae09d13baf4558cf461c65f08893b93fee43f036a17a7
+- filename: app/service/rest.py
+  checksum: b42aefd1ae0e6ea76e75db4cf14d425facd0941943b17f7ba2e41f850ad1ec23
+- filename: app/template/rest.py
+  checksum: 1e5bdac8bc694d50f8f656dec127dd036b7b1b5b6156e3282d3411956c71ba0b
+- filename: lambda_functions/pinpoint_callback/pinpoint_callback_lambda.py
+  checksum: 7bd4900e14b1fa789bbb2568b8a8d7a400e3c8350ba32fb44cc0b5b66a2df037
+- filename: lambda_functions/ses_callback/ses_callback_lambda.py
+  checksum: b20c36921290a9609f158784e2a3278c36190887e6054ea548004a67675fd79b
+- filename: poetry.lock
+  checksum: 34d12acdf749363555c31add4e7e7afa9e2a27afd792bd98c85f331b87bd7112
+- filename: scripts/trigger_task.py
+  checksum: 0e9d244dbe285de23fc84bb643407963dacf7d25a3358373f01f6272fb217778
+- filename: tests/app/conftest.py
+  checksum: a80aa727586db82ed1b50bdb81ddfe1379e649a9dfc1ece2c36047486b41b83d
+- filename: tests/app/notifications/test_process_notifications_for_profile_v3.py
+  checksum: 4e15e63d349635131173ffdd7aebcd547621db08de877ef926d3a41fde72d065
 version: "1.0"

app/celery/contact_information_tasks.py

@@ -69,29 +69,24 @@ def lookup_contact_info(
 
     notification = get_notification_by_id(notification_id)
     recipient_identifier = notification.recipient_identifiers[IdentifierType.VA_PROFILE_ID.value]
-    should_send = notification.default_send
 
     try:
         if is_feature_enabled(FeatureFlag.VA_PROFILE_V3_COMBINE_CONTACT_INFO_AND_PERMISSIONS_LOOKUP):
             result = get_profile_result(notification, recipient_identifier)
-            recipient = result.recipient
-            should_send = result.communication_allowed
-            permission_message = result.permission_message
+            notification.to = result.recipient
+            if not result.communication_allowed:
+                handle_communication_not_allowed(notification, recipient_identifier, result.permission_message)
+            # Otherwise, this communication is allowed. We will update the notification below and continue the chain.
         else:
-            recipient = get_recipient(
+            notification.to = get_recipient(
                 notification.notification_type,
                 notification_id,
                 recipient_identifier,
             )
+        dao_update_notification(notification)
     except Exception as e:
         handle_lookup_contact_info_exception(self, notification, recipient_identifier, e)
 
-    notification.to = recipient
-    dao_update_notification(notification)
-
-    if not should_send:
-        handle_communication_not_allowed(notification, recipient_identifier, permission_message)
-
 
 def get_recipient(
     notification_type: str,
@@ -203,15 +198,23 @@ def handle_lookup_contact_info_exception(
             recipient_identifier.id_value,
             notification.id,
         )
-
-        return None if notification.default_send else 'No recipient opt-in found for explicit preference'
+        if not notification.default_send:
+            update_notification_status_by_id(
+                notification_id=notification.id,
+                status=NOTIFICATION_PERMANENT_FAILURE,
+                status_reason='No recipient opt-in found for explicit preference',
+            )
+            raise e
+        else:
+            # Means the default_send is True and this does not require an explicit opt-in
+            return None
     else:
         current_app.logger.exception(f'Unhandled exception for notification {notification.id}: {e}')
         raise e
 
 
 def handle_communication_not_allowed(
-    notification: Notification, recipient_identifier: RecipientIdentifier, permission_message: str
+    notification: Notification, recipient_identifier: RecipientIdentifier, permission_message: str | None = None
 ):
     """
     Handles the scenario where communication is not allowed for a given notification.
@@ -224,17 +227,16 @@ def handle_communication_not_allowed(
     Raises:
         NotificationPermanentFailureException: If the recipient has declined permission to receive notifications.
     """
-    if is_feature_enabled(FeatureFlag.VA_PROFILE_V3_COMBINE_CONTACT_INFO_AND_PERMISSIONS_LOOKUP):
-        current_app.logger.info(
-            'Permission denied for recipient %s for notification %s',
-            recipient_identifier.id_value,
-            notification.id,
-        )
-        reason = permission_message if permission_message is not None else 'Contact preferences set to false'
-        update_notification_status_by_id(notification.id, NOTIFICATION_PREFERENCES_DECLINED, status_reason=reason)
-
-        message = f'The recipient for notification {notification.id} has declined permission to receive notifications.'
-        current_app.logger.info(message)
-
-        check_and_queue_callback_task(notification)
-        raise NotificationPermanentFailureException(message)
+    current_app.logger.info(
+        'Permission denied for recipient %s for notification %s',
+        recipient_identifier.id_value,
+        notification.id,
+    )
+    reason = permission_message if permission_message is not None else 'Contact preferences set to false'
+    update_notification_status_by_id(notification.id, NOTIFICATION_PREFERENCES_DECLINED, status_reason=reason)
+
+    message = f'The recipient for notification {notification.id} has declined permission to receive notifications.'
+    current_app.logger.info(message)
+
+    check_and_queue_callback_task(notification)
+    raise NotificationPermanentFailureException(message)

app/celery/lookup_recipient_communication_permissions_task.py

@@ -96,7 +96,7 @@ def recipient_has_given_permission(
 
     try:
         is_allowed = va_profile_client.get_is_communication_allowed(
-            identifier, communication_item.va_profile_item_id, notification_id, notification_type
+            identifier, communication_item.va_profile_item_id, notification_id, notification_type, default_send_flag
         )
     except VAProfileRetryableException as e:
         if can_retry(task.request.retries, task.max_retries, notification_id):

app/va/va_profile/va_profile_client.py

@@ -129,7 +129,8 @@ def get_telephone(self, va_profile_id: RecipientIdentifier) -> str:
                 and sorted_telephones[0].get('phoneNumber')
             ):
                 self.statsd_client.incr('clients.va-profile.get-telephone.success')
-            return f"+{sorted_telephones[0]['countryCode']}{sorted_telephones[0]['areaCode']}{sorted_telephones[0]['phoneNumber']}"
+                # https://en.wikipedia.org/wiki/E.164 format
+                return f"+{sorted_telephones[0]['countryCode']}{sorted_telephones[0]['areaCode']}{sorted_telephones[0]['phoneNumber']}"
 
         self.statsd_client.incr('clients.va-profile.get-telephone.failure')
         self._raise_no_contact_info_exception(self.PHONE_BIO_TYPE, va_profile_id, contact_info.get(self.TX_AUDIT_ID))
@@ -175,7 +176,7 @@ def get_telephone_with_permission(
             Property communication_allowed is true when VA Profile service indicates that the recipient has allowed communication.
             Property permission_message may contain an error message if the permission check encountered an exception.
         """
-        profile = self.get_profile(va_profile_id)
+        profile: Profile = self.get_profile(va_profile_id)
         communication_allowed = notification.default_send
         permission_message = None
         try:
@@ -184,7 +185,7 @@ def get_telephone_with_permission(
             )
         except CommunicationItemNotFoundException:
             self.logger.info('Communication item for recipient %s not found', va_profile_id)
-            permission_message = f'V3 Profile - No recipient opt-in found for explicit preference, falling back to default send: {notification.default_send} (Recipient Identifier {va_profile_id.id_value})'
+            permission_message = 'No recipient opt-in found for explicit preference'
 
         contact_info: ContactInformation = profile.get('contactInformation', {})
 
@@ -235,7 +236,7 @@ def get_email_with_permission(
             )
         except CommunicationItemNotFoundException:
             self.logger.info('Communication item for recipient %s not found', va_profile_id)
-            permission_message = f'V3 Profile - No recipient opt-in found for explicit preference, falling back to default send: {notification.default_send} (Recipient Identifier {va_profile_id.id_value})'
+            permission_message = 'No recipient opt-in found for explicit preference'
 
         contact_info: ContactInformation = profile.get('contactInformation', {})
         sorted_emails = sorted(
@@ -257,6 +258,7 @@ def get_is_communication_allowed(
         communication_item_id: str,
         notification_id: str,
         notification_type: str,
+        default_send: bool,
     ) -> bool:
         """
         Determine if communication is allowed for a given recipient, communication item, and notification type.
@@ -277,14 +279,21 @@ def get_is_communication_allowed(
         communication_permissions: CommunicationPermissions = self.get_profile(recipient_id).get(
             'communicationPermissions', {}
         )
+        communication_channel = CommunicationChannel(VA_NOTIFY_TO_VA_PROFILE_NOTIFICATION_TYPES[notification_type])
         for perm in communication_permissions:
             if (
-                perm['communicationChannelName'] == VA_NOTIFY_TO_VA_PROFILE_NOTIFICATION_TYPES[notification_type]
+                perm['communicationChannelId'] == communication_channel.id
                 and perm['communicationItemId'] == communication_item_id
             ):
                 self.statsd_client.incr('clients.va-profile.get-communication-item-permission.success')
-                assert isinstance(perm['allowed'], bool)
-                return perm['allowed']
+                # if default send is true and allowed is false, return false
+                # if default send is true and allowed is true, return true
+                # if default send is false, default to what it finds
+                permission: bool | None = perm['allowed']
+                if permission is not None:
+                    return perm['allowed']
+                else:
+                    return default_send
 
         self.logger.debug(
             'V3 Profile -- Recipient %s did not have permission for communication item %s and channel %s for notification %s',
@@ -326,8 +335,14 @@ def get_is_communication_allowed_from_profile(
                 and perm['communicationItemId'] == notification.va_profile_item_id
             ):
                 self.statsd_client.incr('clients.va-profile.get-communication-item-permission.success')
-                assert isinstance(perm['allowed'], bool)
-                return perm['allowed']
+                # if default send is true and allowed is false, return false
+                # if default send is true and allowed is true, return true
+                # if default send is false, default to what it finds
+                permission: bool | None = perm['allowed']
+                if permission is not None:
+                    return perm['allowed']
+                else:
+                    return notification.default_send
 
         self.logger.debug(
             'V3 Profile -- did not have permission for communication item %s and channel %s',

tests/app/celery/test_contact_information_tasks.py

@@ -1,7 +1,11 @@
-import pytest
 import uuid
+
+import pytest
+from requests import Timeout
+
 from app.celery.common import RETRIES_EXCEEDED
 from app.celery.contact_information_tasks import lookup_contact_info
+from app.celery.lookup_recipient_communication_permissions_task import lookup_recipient_communication_permissions
 from app.celery.exceptions import AutoRetryException
 from app.exceptions import NotificationTechnicalFailureException, NotificationPermanentFailureException
 from app.models import (
@@ -18,7 +22,7 @@
     VAProfileNonRetryableException,
     VAProfileRetryableException,
 )
-from requests import Timeout
+from app.va.va_profile.va_profile_client import CommunicationChannel
 
 
 EXAMPLE_VA_PROFILE_ID = '135'
@@ -314,3 +318,66 @@ def test_exception_sets_failure_reason_if_thrown(
             )
 
     mocked_check_and_queue_callback_task.assert_called_once_with(notification)
+
+
+@pytest.mark.parametrize(
+    'default_send, user_set, expected',
+    [
+        # If the user has set a preference, we always go with that and override default_send
+        [True, True, True],
+        [True, False, False],
+        [False, True, True],
+        [False, False, False],
+        # If the user has not set a preference, go with the default_send
+        [True, None, True],
+        [False, None, False],
+    ],
+)
+@pytest.mark.parametrize('notification_type', [CommunicationChannel.EMAIL, CommunicationChannel.TEXT])
+def test_get_email_or_sms_with_permission_utilizes_default_send(
+    mock_va_profile_client,
+    mock_va_profile_response,
+    sample_communication_item,
+    sample_notification,
+    sample_template,
+    default_send,
+    user_set,
+    expected,
+    notification_type,
+    mocker,
+):
+    # Test each combo, ensuring contact info responds with expected result
+    channel = EMAIL_TYPE if notification_type == CommunicationChannel.EMAIL else SMS_TYPE
+    profile = mock_va_profile_response['profile']
+    communication_item = sample_communication_item(default_send)
+    template = sample_template(template_type=channel, communication_item_id=communication_item.id)
+
+    notification = sample_notification(
+        template=template,
+        gen_type=channel,
+        recipient_identifiers=[{'id_type': IdentifierType.VA_PROFILE_ID.value, 'id_value': '1234'}],
+    )
+
+    profile['communicationPermissions'][0]['allowed'] = user_set
+    profile['communicationPermissions'][0]['communicationItemId'] = notification.va_profile_item_id
+    profile['communicationPermissions'][0]['communicationChannelId'] = notification_type.id
+
+    mocker.patch('app.va.va_profile.va_profile_client.VAProfileClient.get_profile', return_value=profile)
+
+    if default_send:
+        # Leaving this logic so it's easier to understand
+        if user_set or user_set is None:
+            # Implicit + user has not opted out
+            assert lookup_contact_info(notification.id) is None
+        else:
+            # Implicit + user has opted out
+            with pytest.raises(NotificationPermanentFailureException):
+                lookup_recipient_communication_permissions(notification.id)
+    else:
+        if user_set:
+            # Explicit + User has opted in
+            assert lookup_recipient_communication_permissions(notification.id) is None
+        else:
+            # Explicit + User has not defined opted in
+            with pytest.raises(NotificationPermanentFailureException):
+                lookup_recipient_communication_permissions(notification.id)

tests/app/conftest.py

@@ -1,5 +1,7 @@
+from datetime import datetime, timedelta
 import json
 import os
+from random import randint, randrange
 from typing import List, Union
 from uuid import UUID, uuid4
 
@@ -83,9 +85,9 @@
     UserServiceRoles,
     WEBHOOK_CHANNEL_TYPE,
 )
-from datetime import datetime, timedelta
+from app.va.va_profile import VAProfileClient
+
 from flask import current_app, url_for
-from random import randint, randrange
 from sqlalchemy import delete, update, select, Table
 from sqlalchemy.orm import scoped_session
 from sqlalchemy.orm.session import make_transient
@@ -103,6 +105,7 @@
 
 # Tests only run against email/sms. API also considers letters
 TEMPLATE_TYPES = [SMS_TYPE, EMAIL_TYPE]
+MOCK_VA_PROFILE_URL = 'http://mock.vaprofile.va.gov'
 
 
 def json_compare(a, b) -> bool:
@@ -2431,6 +2434,34 @@ def _dynamodb_insert(items_to_insert: list):
         dynamodb_mock.delete_item(Key={'participant_id': item['participant_id'], 'payment_id': item['payment_id']})
 
 
+@pytest.fixture(scope='function')
+def mock_va_profile_client(mocker, notify_api):
+    with notify_api.app_context():
+        mock_logger = mocker.Mock()
+        mock_ssl_key_path = 'some_key.pem'
+        mock_ssl_cert_path = 'some_cert.pem'
+        mock_statsd_client = mocker.Mock()
+        mock_va_profile_token = mocker.Mock()
+
+        client = VAProfileClient()
+        client.init_app(
+            logger=mock_logger,
+            va_profile_url=MOCK_VA_PROFILE_URL,
+            ssl_cert_path=mock_ssl_cert_path,
+            ssl_key_path=mock_ssl_key_path,
+            va_profile_token=mock_va_profile_token,
+            statsd_client=mock_statsd_client,
+        )
+
+        return client
+
+
+@pytest.fixture(scope='function')
+def mock_va_profile_response():
+    with open('tests/app/va/va_profile/mock_response.json', 'r') as f:
+        return json.load(f)
+
+
 #######################################################################################################################
 #                                                                                                                     #
 #                                                 SESSION-SCOPED                                                      #