Added error handling for pinpoint errors (#2430)

app/celery/provider_tasks.py

@@ -16,8 +16,14 @@
     MalwareDetectedException,
     MalwareScanInProgressException,
     NotificationTechnicalFailureException,
+    PinpointConflictException,
+    PinpointValidationException,
+)
+from app.models import (
+    NOTIFICATION_PERMANENT_FAILURE,
+    NOTIFICATION_TECHNICAL_FAILURE,
+    Notification,
 )
-from app.models import NOTIFICATION_TECHNICAL_FAILURE, Notification
 from app.notifications.callbacks import _check_and_queue_callback_task
 from celery import Task
 
@@ -109,6 +115,12 @@ def _deliver_sms(self, notification_id):
         current_app.logger.error(f"Cannot send notification {notification_id}, got an invalid direct file url.")
         update_notification_status_by_id(notification_id, NOTIFICATION_TECHNICAL_FAILURE)
         _check_and_queue_callback_task(notification)
+    except (PinpointConflictException, PinpointValidationException) as e:
+        # As this is due to Pinpoint errors, we are NOT retrying the notification
+        # We are only warning on the error, and not logging an error
+        current_app.logger.warning("Pinpoint error: {}".format(e))
+        update_notification_status_by_id(notification_id, NOTIFICATION_PERMANENT_FAILURE)
+        _check_and_queue_callback_task(notification)
     except Exception:
         try:
             current_app.logger.exception("SMS notification delivery for id: {} failed".format(notification_id))

app/clients/sms/aws_pinpoint.py

@@ -4,6 +4,7 @@
 import phonenumbers
 
 from app.clients.sms import SmsClient, SmsSendingVehicles
+from app.exceptions import PinpointConflictException, PinpointValidationException
 
 
 class AwsPinpointClient(SmsClient):
@@ -69,8 +70,9 @@ def send_sms(self, to, content, reference, multi=True, sender=None, template_id=
                 if e.response.get("Reason") == "DESTINATION_PHONE_NUMBER_OPTED_OUT":
                     opted_out = True
                 else:
-                    raise e
-
+                    raise PinpointConflictException(e)
+            except self._client.exceptions.ValidationException as e:
+                raise PinpointValidationException(e)
             except Exception as e:
                 self.statsd_client.incr("clients.pinpoint.error")
                 raise e

app/exceptions.py

@@ -25,3 +25,15 @@ class InvalidUrlException(Exception):
 
 class DocumentDownloadException(Exception):
     pass
+
+
+class PinpointConflictException(Exception):
+    def __init__(self, original_exception):
+        self.original_exception = original_exception
+        super().__init__(str(original_exception))
+
+
+class PinpointValidationException(Exception):
+    def __init__(self, original_exception):
+        self.original_exception = original_exception
+        super().__init__(str(original_exception))

tests/app/celery/test_provider_tasks.py

@@ -8,7 +8,11 @@
 from app.celery import provider_tasks
 from app.celery.provider_tasks import deliver_email, deliver_sms, deliver_throttled_sms
 from app.clients.email.aws_ses import AwsSesClientException
-from app.exceptions import NotificationTechnicalFailureException
+from app.exceptions import (
+    NotificationTechnicalFailureException,
+    PinpointConflictException,
+    PinpointValidationException,
+)
 from celery.exceptions import MaxRetriesExceededError
 
 sms_methods = [
@@ -120,6 +124,46 @@ def test_should_go_into_technical_error_if_exceeds_retries_on_deliver_sms_task(
     queued_callback.assert_called_once_with(sample_notification)
 
 
+class TestErrorHandling:
+    @pytest.mark.parametrize("sms_method,sms_method_name", sms_methods)
+    def test_should_not_retry_pinpoint_conflict(
+        self,
+        sample_notification,
+        mocker,
+        sms_method,
+        sms_method_name,
+    ):
+        mocker.patch(
+            "app.delivery.send_to_providers.send_sms_to_provider",
+            side_effect=PinpointConflictException("hello"),
+        )
+        queued_callback = mocker.patch("app.celery.provider_tasks._check_and_queue_callback_task")
+
+        sms_method(sample_notification.id)
+
+        assert sample_notification.status == "permanent-failure"
+        queued_callback.assert_called_once_with(sample_notification)
+
+    @pytest.mark.parametrize("sms_method,sms_method_name", sms_methods)
+    def test_should_not_retry_pinpoint_validation(
+        self,
+        sample_notification,
+        mocker,
+        sms_method,
+        sms_method_name,
+    ):
+        mocker.patch(
+            "app.delivery.send_to_providers.send_sms_to_provider",
+            side_effect=PinpointValidationException("hello"),
+        )
+        queued_callback = mocker.patch("app.celery.provider_tasks._check_and_queue_callback_task")
+
+        sms_method(sample_notification.id)
+
+        assert sample_notification.status == "permanent-failure"
+        queued_callback.assert_called_once_with(sample_notification)
+
+
 def test_should_go_into_technical_error_if_exceeds_retries_on_deliver_email_task(sample_notification, mocker):
     mocker.patch(
         "app.delivery.send_to_providers.send_email_to_provider",

tests/app/clients/test_aws_pinpoint.py

@@ -2,6 +2,7 @@
 
 from app import aws_pinpoint_client
 from app.clients.sms import SmsSendingVehicles
+from app.exceptions import PinpointConflictException, PinpointValidationException
 from tests.conftest import set_config_values
 
 
@@ -77,16 +78,43 @@ def test_send_sms_returns_raises_error_if_there_is_no_valid_number_is_found(noti
     assert "No valid numbers found for SMS delivery" in str(excinfo.value)
 
 
-def test_handles_opted_out_numbers(notify_api, mocker, sample_template):
-    conflict_error = aws_pinpoint_client._client.exceptions.ConflictException(
-        error_response={"Reason": "DESTINATION_PHONE_NUMBER_OPTED_OUT"}, operation_name="send_text_message"
-    )
-    mocker.patch("app.aws_pinpoint_client._client.send_text_message", side_effect=conflict_error)
+class TestErrorHandling:
+    def test_handles_opted_out_numbers(self, notify_api, mocker, sample_template):
+        conflict_error = aws_pinpoint_client._client.exceptions.ConflictException(
+            error_response={"Reason": "DESTINATION_PHONE_NUMBER_OPTED_OUT"}, operation_name="send_text_message"
+        )
+        mocker.patch("app.aws_pinpoint_client._client.send_text_message", side_effect=conflict_error)
 
-    to = "6135555555"
-    content = "foo"
-    reference = "ref"
-    assert aws_pinpoint_client.send_sms(to, content, reference=reference, template_id=sample_template.id) == "opted_out"
+        to = "6135555555"
+        content = "foo"
+        reference = "ref"
+        assert aws_pinpoint_client.send_sms(to, content, reference=reference, template_id=sample_template.id) == "opted_out"
+
+    def test_raises_PinpointConflictException(self, notify_api, mocker, sample_template):
+        conflict_error = aws_pinpoint_client._client.exceptions.ConflictException(
+            error_response={"Reason": "REGISTRATION_NOT_COMPLETE"}, operation_name="send_text_message"
+        )
+        mocker.patch("app.aws_pinpoint_client._client.send_text_message", side_effect=conflict_error)
+
+        to = "6135555555"
+        content = "foo"
+        reference = "ref"
+        with pytest.raises(PinpointConflictException) as excinfo:
+            aws_pinpoint_client.send_sms(to, content, reference=reference, template_id=sample_template.id)
+            assert excinfo.original_exception == conflict_error
+
+    def test_raises_PinpointValidationException(self, notify_api, mocker, sample_template):
+        conflict_error = aws_pinpoint_client._client.exceptions.ValidationException(
+            error_response={"Reason": "DestinationCountryBlocked"}, operation_name="send_text_message"
+        )
+        mocker.patch("app.aws_pinpoint_client._client.send_text_message", side_effect=conflict_error)
+
+        to = "6135555555"
+        content = "foo"
+        reference = "ref"
+        with pytest.raises(PinpointValidationException) as excinfo:
+            aws_pinpoint_client.send_sms(to, content, reference=reference, template_id=sample_template.id)
+            assert excinfo.original_exception == conflict_error
 
 
 @pytest.mark.serial