Add BaseFailed exceptions for phone_notificator #2074

Konstantinov-Innokentii · 2023-06-01T05:06:07Z

What this PR does

Introduces BaseFailed exception for phone_notificator.

Why

We need to somehow distinguish errors we want to be notified - like network errors or invalid twilio credentials (I will call them "real" errors) and errors we want to share with user, but don't want to be paged ( I will call them "fake" errors).
To do that I added "graceful_msg" to all Failed... exceptions. If details field is present - it mean we can return 400 code with the message, if not - 500 code. So, "real" errors will raise Failed... exception, while "fake" will add "graceful_msg".

TODO

handle exceptions handled here #2065

Checklist

Unit, integration, and e2e (if applicable) tests updated
Documentation added (or pr:no public docs PR label added if not required)
CHANGELOG.md updated (or pr:no changelog PR label added if not required)

…s' into phone_notificator_granular_errors

joeyorlando · 2023-06-01T09:49:05Z

engine/apps/api/views/user.py

@@ -650,3 +647,10 @@ def check_availability(self, request, pk):
        user = self.get_object()
        warnings = check_user_availability(user=user, team=request.user.current_team)
        return Response(data={"warnings": warnings}, status=status.HTTP_200_OK)
+
+
+def handle_phone_notificator_failed(exc: BaseFailed) -> Response:


Suggested change

def handle_phone_notificator_failed(exc: BaseFailed) -> Response:

def handle_phone_notification_failed(exc: BaseFailed) -> Response:

joeyorlando · 2023-06-01T09:49:22Z

engine/apps/phone_notifications/exceptions.py

+       graceful_msg: string with some details about exception which can be exposed to caller.
+    """
+
+    def __init__(self, graceful_msg=None):


where is graceful_msg being set?

nowhere yet. but it's expected to be set in concrete providers, me or @mderynck will add this

Konstantinov-Innokentii · 2023-06-02T05:54:04Z

@mderynck could you please add unittests simular to what @joeyorlando did in his PR
Otherwise LGTM, thanks for tackling that issue @mderynck and @joeyorlando !

engine/apps/twilioapp/phone_provider.py

joeyorlando · 2023-06-05T06:29:58Z

engine/apps/twilioapp/phone_provider.py

+    """
+    Errors we will raise without graceful messages:
+
+    20404 - We should not be requesting missing resources
+    30808 - Unknown error, likely on the carrier side
+    30007, 32017 - Blocked or filtered, Intermediary / Carrier Analytics blocked call
+                due to poor reputation score on the telephone number:
+        * We need to register our number or sender with the analytics provider or carrier for that jurisdiction
+    """
+
+    def _get_graceful_msg(self, e, number):
+        if e.code in (30003, 30005):
+            return f"Destination handset {number} is unreachable"
+        elif e.code == 30004:
+            return f"Sending message to {number} is blocked"
+        elif e.code == 30006:
+            return f"Cannot send to {number} is landline or unreachable carrier"
+        elif e.code == 30410:
+            return f"Provider for {number} is experiencing timeouts"
+        elif e.code == 60200:
+            return f"{number} is incorrectly formatted"
+        elif e.code in (21215, 60410, 60605):
+            return f"Verification to {number} is blocked"
+        elif e.code == 60203:
+            return f"Max verification attempts for {number} reached"
+        return None


joeyorlando · 2023-06-05T06:30:15Z

engine/apps/twilioapp/tests/test_twilio_provider.py

+@pytest.mark.django_db
+@pytest.mark.parametrize(
+    "twilio_code,expected_exception,graceful_msg,provider_method,mock_method",
+    [
+        (60200, FailedToMakeCall, True, lambda p: p.make_call(TEST_NUMBER, TEST_MESSAGE), "_call_create"),
+        (30808, FailedToMakeCall, False, lambda p: p.make_call(TEST_NUMBER, TEST_MESSAGE), "_call_create"),
+        (None, FailedToMakeCall, False, lambda p: p.make_call(TEST_NUMBER, TEST_MESSAGE), "_call_create"),
+        (30410, FailedToMakeCall, True, lambda p: p.make_notification_call(TEST_NUMBER, TEST_MESSAGE), "_call_create"),
+        (30808, FailedToMakeCall, False, lambda p: p.make_notification_call(TEST_NUMBER, TEST_MESSAGE), "_call_create"),
+        (None, FailedToMakeCall, False, lambda p: p.make_notification_call(TEST_NUMBER, TEST_MESSAGE), "_call_create"),
+        (30004, FailedToSendSMS, True, lambda p: p.send_sms(TEST_NUMBER, TEST_MESSAGE), "_messages_create"),
+        (30808, FailedToSendSMS, False, lambda p: p.send_sms(TEST_NUMBER, TEST_MESSAGE), "_messages_create"),
+        (None, FailedToSendSMS, False, lambda p: p.send_sms(TEST_NUMBER, TEST_MESSAGE), "_messages_create"),
+        (
+            30006,
+            FailedToSendSMS,
+            True,
+            lambda p: p.send_notification_sms(TEST_NUMBER, TEST_MESSAGE),
+            "_messages_create",
+        ),
+        (
+            30808,
+            FailedToSendSMS,
+            False,
+            lambda p: p.send_notification_sms(TEST_NUMBER, TEST_MESSAGE),
+            "_messages_create",
+        ),
+        (
+            None,
+            FailedToSendSMS,
+            False,
+            lambda p: p.send_notification_sms(TEST_NUMBER, TEST_MESSAGE),
+            "_messages_create",
+        ),
+        (
+            60203,
+            FailedToFinishVerification,
+            True,
+            lambda p: p.finish_verification(TEST_NUMBER, TEST_CODE),
+            "_verify_sender",
+        ),
+        (
+            30808,
+            FailedToFinishVerification,
+            False,
+            lambda p: p.finish_verification(TEST_NUMBER, TEST_CODE),
+            "_verify_sender",
+        ),
+        (
+            None,
+            FailedToFinishVerification,
+            False,
+            lambda p: p.finish_verification(TEST_NUMBER, TEST_CODE),
+            "_verify_sender",
+        ),
+    ],
+)
+@mock.patch("apps.twilioapp.phone_provider.TwilioPhoneProvider._normalize_phone_number", return_value=(TEST_NUMBER, 1))
+def test_twilio_provider_exceptions(
+    mocked_normalize, twilio_code, expected_exception, graceful_msg, provider_method, mock_method
+):
+    provider = TwilioPhoneProvider()
+
+    with mock.patch(f"apps.twilioapp.phone_provider.TwilioPhoneProvider.{mock_method}") as twilio_mock:
+        twilio_mock.side_effect = TwilioRestException(500, "", code=twilio_code)
+        with pytest.raises(expected_exception) as exc:
+            provider_method(provider)
+            if graceful_msg:
+                assert len(exc.value.graceful_msg) > 0
+            else:
+                assert exc.value.graceful_msg is None
+        twilio_mock.assert_called_once()


Add BaseFailed exceptions for phone_notificator

8008969

Konstantinov-Innokentii requested a review from a team June 1, 2023 05:06

Konstantinov-Innokentii mentioned this pull request Jun 1, 2023

Better handle "Verification delivery attempt blocked" and "Geo Permission configuration is not permitting call" exceptions from Twilio #2065

Closed

3 tasks

Better handle phone_notificator exc in internal api

39dc38c

Konstantinov-Innokentii added pr:no changelog pr:no public docs Added to a PR that does not require public documentation updates labels Jun 1, 2023

Konstantinov-Innokentii added 3 commits June 1, 2023 13:23

Merge branch 'dev' into phone_notificator_granular_errors

091b04f

Fix tests

3721594

Merge remote-tracking branch 'origin/phone_notificator_granular_error…

39c26f3

…s' into phone_notificator_granular_errors

joeyorlando reviewed Jun 1, 2023

View reviewed changes

mderynck added 2 commits June 1, 2023 10:44

Add commonly encountered error codes for Twilio

0e4806d

Add graceful message for other calls

ffd466e

joeyorlando reviewed Jun 2, 2023

View reviewed changes

engine/apps/twilioapp/phone_provider.py Outdated Show resolved Hide resolved

Fix raise/return in phone provider, add tests

118bfb6

joeyorlando reviewed Jun 5, 2023

View reviewed changes

joeyorlando approved these changes Jun 5, 2023

View reviewed changes

Konstantinov-Innokentii added this pull request to the merge queue Jun 8, 2023

Merged via the queue into dev with commit c7f8ac3 Jun 8, 2023

Konstantinov-Innokentii deleted the phone_notificator_granular_errors branch June 8, 2023 06:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add BaseFailed exceptions for phone_notificator #2074

Add BaseFailed exceptions for phone_notificator #2074

Konstantinov-Innokentii commented Jun 1, 2023 •

edited

Loading

joeyorlando Jun 1, 2023

joeyorlando Jun 1, 2023

Konstantinov-Innokentii Jun 1, 2023

Konstantinov-Innokentii commented Jun 2, 2023

joeyorlando Jun 5, 2023

joeyorlando Jun 5, 2023

	def handle_phone_notificator_failed(exc: BaseFailed) -> Response:
	def handle_phone_notification_failed(exc: BaseFailed) -> Response:

Add BaseFailed exceptions for phone_notificator #2074

Add BaseFailed exceptions for phone_notificator #2074

Conversation

Konstantinov-Innokentii commented Jun 1, 2023 • edited Loading

What this PR does

Why

TODO

Checklist

joeyorlando Jun 1, 2023

Choose a reason for hiding this comment

joeyorlando Jun 1, 2023

Choose a reason for hiding this comment

Konstantinov-Innokentii Jun 1, 2023

Choose a reason for hiding this comment

Konstantinov-Innokentii commented Jun 2, 2023

joeyorlando Jun 5, 2023

Choose a reason for hiding this comment

joeyorlando Jun 5, 2023

Choose a reason for hiding this comment

Konstantinov-Innokentii commented Jun 1, 2023 •

edited

Loading