#2172 - Retry Push for certain HTTP status codes

app/celery/provider_tasks.py

@@ -296,6 +296,9 @@ def deliver_push(
         retries = celery_task.request.retries
 
         if retries < max_retries:
+            if 'icn' in payload:
+                payload['icn'] = '<redacted>'
+
             current_app.logger.warning(
                 'Push notification retrying: %s, max retries: %s, retries: %s', payload, max_retries, retries
             )

app/internal/rest.py

@@ -57,7 +57,8 @@
     else:
         response_body = {generic: request.json}
 
+    # TODO - will be addressed in TEAM-1487
     return response_body, status_code
 
 
 @internal_blueprint.route('/502', methods=['POST', 'GET'])

app/mobile_app/__init__.py

@@ -1,2 +1,2 @@
-from .mobile_app_types import MobileAppType, DEAFULT_MOBILE_APP_TYPE  # noqa
+from .mobile_app_types import MobileAppType, DEFAULT_MOBILE_APP_TYPE  # noqa
 from .mobile_app import MobileApp  # noqa

app/mobile_app/mobile_app_registry.py

@@ -25,7 +25,7 @@ def get_app(
         try:
             return self._registry[app_type]
         except KeyError:
-            self.logger.warning('Attempted to use an app that is not initialized: %s', app_type)
+            self.logger.exception('Attempted to use an app that is not initialized: %s', app_type)
             raise
 
     def get_registered_apps(self) -> List[MobileAppType]:

app/mobile_app/mobile_app_types.py

@@ -11,4 +11,4 @@ def values() -> List['MobileAppType']:
         return list(x.value for x in MobileAppType)
 
 
-DEAFULT_MOBILE_APP_TYPE = MobileAppType.VA_FLAGSHIP_APP
+DEFAULT_MOBILE_APP_TYPE = MobileAppType.VA_FLAGSHIP_APP

app/v2/notifications/rest_push.py

@@ -9,7 +9,7 @@
 from app.celery.provider_tasks import deliver_push
 from app.config import QueueNames
 from app.constants import PUSH_TYPE
-from app.mobile_app import DEAFULT_MOBILE_APP_TYPE, MobileAppType
+from app.mobile_app import DEFAULT_MOBILE_APP_TYPE, MobileAppType
 from app.schema_validation import validate
 from app.utils import get_public_notify_type_text
 from app.v2.errors import BadRequestError
@@ -34,31 +34,31 @@ def push_notification_helper(schema: dict):
     the Flask "request" instance.
     """
     if not authenticated_service.has_permissions(PUSH_TYPE):
+        public_notify_type = get_public_notify_type_text(PUSH_TYPE, plural=True)
+
         raise BadRequestError(
-            message='Service is not allowed to send {}'.format(
-                get_public_notify_type_text(PUSH_TYPE, plural=True),
-            ),
+            message=f'Service is not allowed to send {public_notify_type}',
             status_code=403,
         )
 
     validated_payload = validate_push_payload(schema)
 
     vetext_formatted_payload = vetext_client.format_for_vetext(validated_payload)
 
-    try:
-        current_app.logger.debug(
-            'Attempting to call deliver_push celery task with validated payload: %s',
-            vetext_formatted_payload,
-        )
+    current_app.logger.debug(
+        'Attempting to call deliver_push celery task with validated payload: %s',
+        vetext_formatted_payload,
+    )
 
+    try:
         # Choosing to use the email queue for push to limit the number of empty queues
         deliver_push.apply_async(
             args=(vetext_formatted_payload,),
             queue=QueueNames.SEND_EMAIL,
         )
     except (CeleryError, OperationalError):
         current_app.logger.exception('Failed to enqueue deliver_push request')
-        response = jsonify(result='error', message='VA Notify service impaired, please try again'), 502
+        response = jsonify(result='error', message='VA Notify service impaired, please try again'), 503
     else:
         response = jsonify(result='success'), 201
     # Flask turns the tuple into a json and status_code
@@ -84,7 +84,7 @@ def validate_push_payload(schema: dict[str, str]) -> V2PushPayload:
         if 'mobile_app' in req_json:
             app_sid = mobile_app_registry.get_app(MobileAppType[req_json['mobile_app']]).sid
         else:
-            app_sid = mobile_app_registry.get_app(DEAFULT_MOBILE_APP_TYPE).sid
+            app_sid = mobile_app_registry.get_app(DEFAULT_MOBILE_APP_TYPE).sid
     except (KeyError, TypeError) as e:
         current_app.logger.warning('Push request failed validation due to mobile app setup: %s', e)
         raise BadRequestError(message=str(e), status_code=400)

app/va/vetext/client.py

@@ -33,23 +33,20 @@ def init_app(
 
     @staticmethod
     def format_for_vetext(payload: V2PushPayload) -> dict[str, str]:
-        if payload.personalisation is not None:
+        if payload.personalisation:
             payload.personalisation = {f'%{k.upper()}%': v for k, v in payload.personalisation.items()}
 
+        formatted_payload = {
+            'appSid': payload.app_sid,
+            'templateSid': payload.template_id,
+            'personalization': payload.personalisation,
+        }
+
         if payload.is_broadcast():
-            formatted_payload = {
-                'appSid': payload.app_sid,
-                'templateSid': payload.template_id,
-                'topicSid': payload.topic_sid,
-                'personalization': payload.personalisation,
-            }
+            formatted_payload['topicSid'] = payload.topic_sid
         else:
-            formatted_payload = {
-                'appSid': payload.app_sid,
-                'icn': payload.icn,
-                'templateSid': payload.template_id,
-                'personalization': payload.personalisation,
-            }
+            formatted_payload['icn'] = payload.icn
+
         return formatted_payload
 
     def send_push_notification(
@@ -68,12 +65,10 @@ def send_push_notification(
                 f'{self.base_url}/mobile/push/send', auth=self.auth, json=payload, timeout=self.TIMEOUT
             )
 
-            self.logger.info(
-                'VEText response: %s for payload %s',
-                response.json() if response.ok else response.status_code,
-                payload,
+            self.logger.info('VEText response: %s', response.json() if response.ok else response.status_code)
+            self.logger.debug(
+                'VEText response: %s for payload %s', response.json() if response.ok else response.status_code, payload
             )
-            self.logger.info('VEText response text %s', response.text)
             response.raise_for_status()
         except requests.exceptions.ReadTimeout:
             # Discussion with VEText: read timeouts are still processed, so no need to retry
@@ -91,15 +86,19 @@ def send_push_notification(
             elif e.response.status_code == 400:
                 self._decode_bad_request_response(e)
             else:
-                payload['icn'] = '<redacted>'
+                if 'icn' in payload:
+                    payload['icn'] = '<redacted>'
+
                 self.logger.exception(
                     'Status: %s - Not retrying - payload: %s',
                     e.response.status_code,
                     payload,
                 )
                 raise NonRetryableException from e
         except requests.RequestException as e:
-            payload['icn'] = '<redacted>'
+            if 'icn' in payload:
+                payload['icn'] = '<redacted>'
+
             self.logger.exception(
                 'Exception raised sending push notification. Not retrying - payload: %s',
                 payload,
@@ -128,9 +127,9 @@ def _decode_bad_request_response(
             payload = http_exception.response.json()
             field = payload.get('idType')
             message = payload.get('error')
-            self.logger.warning('Bad response from VEText: %s with field: ', message, field)
+            self.logger.error('Bad response from VEText: %s with field: ', message, field)
             raise NonRetryableException from http_exception
         except Exception:
             message = http_exception.response.text
-            self.logger.warning('Bad response from VEText: %s', message)
+            self.logger.error('Bad response from VEText: %s', message)
             raise NonRetryableException from http_exception

tests/app/celery/test_provider_tasks.py

@@ -1,5 +1,6 @@
 from requests import HTTPError, Response
 from requests.exceptions import ConnectTimeout, RequestException
+from app.mobile_app.mobile_app_types import MobileAppType
 import pytest
 
 from app.celery.exceptions import AutoRetryException, NonRetryableException
@@ -19,7 +20,6 @@
     NotificationTechnicalFailureException,
     InvalidProviderException,
 )
-from app.mobile_app import DEAFULT_MOBILE_APP_TYPE
 from app.models import Notification
 from app.v2.errors import RateLimitError
 from collections import namedtuple
@@ -501,14 +501,17 @@ def test_deliver_push_happy_path_icn(
     client,
     rmock,
 ):
+    url = f'{client.application.config["VETEXT_URL"]}/mobile/push/send'
+
     rmock.register_uri(
         'POST',
-        f'{client.application.config["VETEXT_URL"]}/mobile/push/send',
+        url,
         json={'message': 'success'},
         status_code=201,
     )
+
     formatted_payload = {
-        'appSid': DEAFULT_MOBILE_APP_TYPE,
+        'appSid': f'{MobileAppType.VA_FLAGSHIP_APP}_SID',
         'templateSid': '2222',
         'icn': '3333',
         'personalization': {'%MSG_ID': '4444'},
@@ -517,19 +520,27 @@ def test_deliver_push_happy_path_icn(
     # Should run without exceptions
     deliver_push(formatted_payload)
 
+    assert rmock.called
+    assert rmock.request_history[0].method == 'POST'
+    assert rmock.request_history[0].url == url
+    assert rmock.request_history[0].json()
+
 
 def test_deliver_push_happy_path_topic(
     client,
     rmock,
 ):
+    url = f'{client.application.config["VETEXT_URL"]}/mobile/push/send'
+
     rmock.register_uri(
         'POST',
-        f'{client.application.config["VETEXT_URL"]}/mobile/push/send',
+        url,
         json={'message': 'success'},
         status_code=201,
     )
+
     formatted_payload = {
-        'appSid': DEAFULT_MOBILE_APP_TYPE,
+        'appSid': f'{MobileAppType.VA_FLAGSHIP_APP}_SID',
         'templateSid': '2222',
         'topicSid': '3333',
         'personalization': {'%MSG_ID': '4444'},
@@ -538,6 +549,11 @@ def test_deliver_push_happy_path_topic(
     # Should run without exceptions
     deliver_push(formatted_payload)
 
+    assert rmock.called
+    assert rmock.request_history[0].method == 'POST'
+    assert rmock.request_history[0].url == url
+    assert rmock.request_history[0].json()
+
 
 @pytest.mark.parametrize(
     'test_exception, status_code',
@@ -558,13 +574,16 @@ def test_deliver_push_retryable_exception(
 ):
     if status_code is not None:
         test_exception.response.status_code = status_code
+
+    url = f'{client.application.config["VETEXT_URL"]}/mobile/push/send'
+
     rmock.register_uri(
         'POST',
-        f'{client.application.config["VETEXT_URL"]}/mobile/push/send',
+        url,
         exc=test_exception,
     )
     formatted_payload = {
-        'appSid': DEAFULT_MOBILE_APP_TYPE,
+        'appSid': f'{MobileAppType.VA_FLAGSHIP_APP}_SID',
         'templateSid': '2222',
         'icn': '3333',
         'personalization': {'%MSG_ID': '4444'},
@@ -573,6 +592,11 @@ def test_deliver_push_retryable_exception(
     with pytest.raises(AutoRetryException):
         deliver_push(formatted_payload)
 
+    assert rmock.called
+    assert rmock.request_history[0].method == 'POST'
+    assert rmock.request_history[0].url == url
+    assert rmock.request_history[0].json()
+
 
 @pytest.mark.parametrize(
     'test_exception, status_code',
@@ -591,17 +615,26 @@ def test_deliver_push_nonretryable_exception(
 ):
     if status_code is not None:
         test_exception.response.status_code = status_code
+
+    url = f'{client.application.config["VETEXT_URL"]}/mobile/push/send'
+
     rmock.register_uri(
         'POST',
-        f'{client.application.config["VETEXT_URL"]}/mobile/push/send',
+        url,
         exc=test_exception,
     )
+
     formatted_payload = {
-        'appSid': DEAFULT_MOBILE_APP_TYPE,
+        'appSid': f'{MobileAppType.VA_FLAGSHIP_APP}_SID',
         'templateSid': '2222',
         'icn': '3333',
         'personalization': {'%MSG_ID': '4444'},
     }
 
     with pytest.raises(NonRetryableException):
         deliver_push(formatted_payload)
+
+    assert rmock.called
+    assert rmock.request_history[0].method == 'POST'
+    assert rmock.request_history[0].url == url
+    assert rmock.request_history[0].json()

tests/app/integration/test_push_notification.py

@@ -138,4 +138,4 @@ def test_mobile_app_push_notification_celery_exception(
     )
 
     assert response.json.get('result') == 'error'
-    assert response.status_code == 502
+    assert response.status_code == 503

tests/app/v2/notifications/test_push_broadcast_notifications.py

@@ -132,4 +132,4 @@ def test_celery_returns_502(self, client, sample_api_key, sample_service, mocker
         mocker.patch('app.v2.notifications.rest_push.deliver_push.apply_async', side_effect=side_effect)
         service = sample_service(service_permissions=[PUSH_TYPE])
         response = post_send_push_broadcast_notification(client, sample_api_key(service), PUSH_BROADCAST_REQUEST)
-        assert response.status_code == 502
+        assert response.status_code == 503

tests/app/v2/notifications/test_push_notifications.py

@@ -168,4 +168,4 @@ def test_celery_returns_502(self, client, sample_api_key, sample_service, mocker
         mocker.patch('app.v2.notifications.rest_push.deliver_push.apply_async', side_effect=side_effect)
         service = sample_service(service_permissions=[PUSH_TYPE])
         response = post_send_notification(client, sample_api_key(service), PUSH_TYPE, PUSH_REQUEST)
-        assert response.status_code == 502
+        assert response.status_code == 503

tests/app/va/vetext/test_client.py

@@ -8,7 +8,7 @@
 from requests_mock import ANY
 
 from app.celery.exceptions import NonRetryableException, RetryableException
-from app.mobile_app import DEAFULT_MOBILE_APP_TYPE
+from app.mobile_app import DEFAULT_MOBILE_APP_TYPE
 from app.va.identifier import IdentifierType
 from app.va.vetext import VETextClient
 from app.v2.dataclasses import V2PushPayload
@@ -162,36 +162,36 @@ def test_raises_bad_request_exception_when_400_error_not_json(self, rmock, test_
 class TestFormatVetextPayload:
     def test_format_happy_path_push(self):
         payload = V2PushPayload(
-            DEAFULT_MOBILE_APP_TYPE,
+            DEFAULT_MOBILE_APP_TYPE,
             'any_template_id',
             personalisation={'foo_1': 'bar', 'baz_two': 'abc', 'tmp': '123'},
             icn='some_icn',
         )
         formatted_payload = VETextClient.format_for_vetext(payload)
         expected_formatted_payload = {
-            'appSid': DEAFULT_MOBILE_APP_TYPE,
+            'appSid': DEFAULT_MOBILE_APP_TYPE,
             'icn': 'some_icn',
             'personalization': {'%FOO_1%': 'bar', '%BAZ_TWO%': 'abc', '%TMP%': '123'},
             'templateSid': 'any_template_id',
         }
         assert formatted_payload == expected_formatted_payload
 
     def test_format_happy_path_broadcast(self):
-        payload = V2PushPayload(DEAFULT_MOBILE_APP_TYPE, 'any_template_id', topic_sid='some_topic_sid')
+        payload = V2PushPayload(DEFAULT_MOBILE_APP_TYPE, 'any_template_id', topic_sid='some_topic_sid')
         formatted_payload = VETextClient.format_for_vetext(payload)
         expected_formatted_payload = {
-            'appSid': DEAFULT_MOBILE_APP_TYPE,
+            'appSid': DEFAULT_MOBILE_APP_TYPE,
             'personalization': None,
             'templateSid': 'any_template_id',
             'topicSid': 'some_topic_sid',
         }
         assert formatted_payload == expected_formatted_payload
 
     def test_format_personalisation_happy_path(self):
-        payload = V2PushPayload(DEAFULT_MOBILE_APP_TYPE, 'any_template_id')
+        payload = V2PushPayload(DEFAULT_MOBILE_APP_TYPE, 'any_template_id')
         formatted_payload = VETextClient.format_for_vetext(payload)
         expected_formatted_payload = {
-            'appSid': DEAFULT_MOBILE_APP_TYPE,
+            'appSid': DEFAULT_MOBILE_APP_TYPE,
             'icn': None,
             'personalization': None,
             'templateSid': 'any_template_id',
@@ -205,7 +205,7 @@ def sample_vetext_push_payload(self, test_vetext_client):
         """Defaults to ICN (normal push, not broadcast)"""
 
         def _wrapper(
-            mobile_app: str = DEAFULT_MOBILE_APP_TYPE,
+            mobile_app: str = DEFAULT_MOBILE_APP_TYPE,
             template_id: str = 'any_template_id',
             icn: str | None = 'some_icn',
             topic_sid: str | None = None,