Phone provider refactoring (#1713)

# What this PR does
This PR moves phone notification logic into separate object PhoneBackend
and introduces PhoneProvider interface to hide actual implementation of
external phone services provider. It should allow add new phone
providers just by implementing one class (See SimplePhoneProvider for
example).
# Why 
[Asterisk PR](#1282) showed that
our phone notification system is not flexible. However this is one of
the most frequent community questions - how to add "X" phone provider.
Also, this refactoring move us one step closer to unifying all
notification backends, since with PhoneBackend all phone notification
logic is collected in one place and independent from concrete
realisation.
# Highligts
1. PhoneBackend object - contains all phone notifications business
logic.
2. PhoneProvider - interface to  external phone services provider.
3. TwilioPhoneProvider and SimplePhoneProvider - two examples of
PhoneProvider implementation.
4. PhoneCallRecord and SMSRecord models. I introduced these models to
keep phone notification limits logic decoupled from external providers.
Existing TwilioPhoneCall and TwilioSMS objects will be migrated to the
new table to not to reset limits counter. To be able to receive status
callbacks and gather from Twilio TwilioPhoneCall and TwilioSMS still
exists, but they are linked to PhoneCallRecord and SMSRecord via fk, to
not to leat twilio logic into core code.

---------

Co-authored-by: Yulia Shanyrova <[email protected]>

Author: Konstantinov-Innokentii

CHANGELOG.md

@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## Unreleased
 
+### Changed
+
+- Phone provider refactoring
+
 ### Fixed
 
 - Improve plugin authentication by @vadimkerr ([#1995](https://github.com/grafana/oncall/pull/1995))

engine/apps/alerts/constants.py

@@ -2,7 +2,7 @@ class ActionSource:
     (
         SLACK,
         WEB,
-        TWILIO,
+        PHONE,
         TELEGRAM,
     ) = range(4)
 

engine/apps/alerts/incident_appearance/templaters/phone_call_templater.py

@@ -1,5 +1,5 @@
 from apps.alerts.incident_appearance.templaters.alert_templater import AlertTemplater
-from common.utils import clean_markup, escape_for_twilio_phone_call
+from common.utils import clean_markup
 
 
 class AlertPhoneCallTemplater(AlertTemplater):
@@ -14,14 +14,11 @@ def _postformat(self, templated_alert):
         return templated_alert
 
     def _postformat_pipeline(self, text):
-        return self._escape(clean_markup(self._slack_format_for_phone_call(text))) if text is not None else text
+        return clean_markup(self._slack_format_for_phone_call(text)).replace('"', "") if text is not None else text
 
     def _slack_format_for_phone_call(self, data):
         sf = self.slack_formatter
         sf.user_mention_format = "{}"
         sf.channel_mention_format = "#{}"
         sf.hyperlink_mention_format = "{title}"
         return sf.format(data)
-
-    def _escape(self, data):
-        return escape_for_twilio_phone_call(data)

engine/apps/alerts/tasks/notify_user.py

@@ -9,7 +9,7 @@
 from apps.alerts.constants import NEXT_ESCALATION_DELAY
 from apps.alerts.signals import user_notification_action_triggered_signal
 from apps.base.messaging import get_messaging_backend_from_id
-from apps.base.utils import live_settings
+from apps.phone_notifications.phone_backend import PhoneBackend
 from common.custom_celery_tasks import shared_dedicated_queue_retry_task
 
 from .task_logger import task_logger
@@ -224,8 +224,6 @@ def notify_user_task(
     autoretry_for=(Exception,), retry_backoff=True, max_retries=1 if settings.DEBUG else None
 )
 def perform_notification(log_record_pk):
-    SMSMessage = apps.get_model("twilioapp", "SMSMessage")
-    PhoneCall = apps.get_model("twilioapp", "PhoneCall")
     UserNotificationPolicy = apps.get_model("base", "UserNotificationPolicy")
     TelegramToUserConnector = apps.get_model("telegram", "TelegramToUserConnector")
     UserNotificationPolicyLogRecord = apps.get_model("base", "UserNotificationPolicyLogRecord")
@@ -259,20 +257,12 @@ def perform_notification(log_record_pk):
         return
 
     if notification_channel == UserNotificationPolicy.NotificationChannel.SMS:
-        SMSMessage.send_sms(
-            user,
-            alert_group,
-            notification_policy,
-            is_cloud_notification=live_settings.GRAFANA_CLOUD_NOTIFICATIONS_ENABLED,
-        )
+        phone_backend = PhoneBackend()
+        phone_backend.notify_by_sms(user, alert_group, notification_policy)
 
     elif notification_channel == UserNotificationPolicy.NotificationChannel.PHONE_CALL:
-        PhoneCall.make_call(
-            user,
-            alert_group,
-            notification_policy,
-            is_cloud_notification=live_settings.GRAFANA_CLOUD_NOTIFICATIONS_ENABLED,
-        )
+        phone_backend = PhoneBackend()
+        phone_backend.notify_by_call(user, alert_group, notification_policy)
 
     elif notification_channel == UserNotificationPolicy.NotificationChannel.TELEGRAM:
         TelegramToUserConnector.notify_user(user, alert_group, notification_policy)

engine/apps/alerts/tests/test_alert_group.py

@@ -38,7 +38,7 @@ def test_render_for_phone_call(
     )
 
     expected_verbose_name = (
-        f"You are invited to check an incident from Grafana OnCall. "
+        f"to check an incident from Grafana OnCall. "
         f"Alert via {alert_receive_channel.verbal_name} - Grafana with title TestAlert triggered 1 times"
     )
     rendered_text = AlertGroupPhoneCallRenderer(alert_group).render()

engine/apps/api/serializers/organization.py

@@ -1,3 +1,4 @@
+from dataclasses import asdict
 from datetime import timedelta
 
 import humanize
@@ -7,6 +8,7 @@
 from rest_framework import fields, serializers
 
 from apps.base.models import LiveSetting
+from apps.phone_notifications.phone_provider import get_phone_provider
 from apps.slack.models import SlackTeamIdentity
 from apps.slack.tasks import resolve_archived_incidents_for_organization, unarchive_incidents_for_organization
 from apps.user_management.models import Organization
@@ -112,14 +114,16 @@ def get_limits(self, obj):
         return obj.notifications_limit_web_report(user)
 
     def get_env_status(self, obj):
+        # deprecated in favour of ConfigAPIView.
+        # All new env statuses should be added there
         LiveSetting.populate_settings_if_needed()
 
         telegram_configured = not LiveSetting.objects.filter(name__startswith="TELEGRAM", error__isnull=False).exists()
-        twilio_configured = not LiveSetting.objects.filter(name__startswith="TWILIO", error__isnull=False).exists()
-
+        phone_provider_config = get_phone_provider().flags
         return {
             "telegram_configured": telegram_configured,
-            "twilio_configured": twilio_configured,
+            "twilio_configured": phone_provider_config.configured,  # keep for backward compatibility
+            "phone_provider": asdict(phone_provider_config),
         }
 
     def get_stats(self, obj):

engine/apps/api/serializers/user.py

@@ -11,11 +11,11 @@
 from apps.base.models import UserNotificationPolicy
 from apps.base.utils import live_settings
 from apps.oss_installation.utils import cloud_user_identity_status
-from apps.twilioapp.utils import check_phone_number_is_valid
 from apps.user_management.models import User
 from apps.user_management.models.user import default_working_hours
 from common.api_helpers.custom_fields import TeamPrimaryKeyRelatedField
 from common.api_helpers.mixins import EagerLoadingMixin
+from common.api_helpers.utils import check_phone_number_is_valid
 from common.timezones import TimeZoneField
 
 from .custom_serializers import DynamicFieldsModelSerializer

engine/apps/api/tests/test_user.py

@@ -17,6 +17,7 @@
     RBACPermission,
 )
 from apps.base.models import UserNotificationPolicy
+from apps.phone_notifications.exceptions import FailedToFinishVerification
 from apps.schedules.models import CustomOnCallShift, OnCallScheduleWeb
 from apps.user_management.models.user import default_working_hours
 
@@ -471,7 +472,7 @@ def test_user_get_other_verification_code(
 
     client = APIClient()
     url = reverse("api-internal:user-get-verification-code", kwargs={"pk": admin.public_primary_key})
-    with patch("apps.twilioapp.phone_manager.PhoneManager.send_verification_code", return_value=Mock()):
+    with patch("apps.phone_notifications.phone_backend.PhoneBackend.send_verification_sms", return_value=Mock()):
         response = client.get(url, format="json", **make_user_auth_headers(tester, token))
 
     assert response.status_code == expected_status
@@ -486,7 +487,7 @@ def test_validation_of_verification_code(
     client = APIClient()
     url = reverse("api-internal:user-verify-number", kwargs={"pk": user.public_primary_key})
     with patch(
-        "apps.twilioapp.phone_manager.PhoneManager.verify_phone_number", return_value=(True, None)
+        "apps.phone_notifications.phone_backend.PhoneBackend.verify_phone_number", return_value=True
     ) as verify_phone_number:
         url_with_token = f"{url}?token=some_token"
         r = client.put(url_with_token, format="json", **make_user_auth_headers(user, token))
@@ -504,6 +505,24 @@ def test_validation_of_verification_code(
         assert verify_phone_number.call_count == 1
 
 
+@pytest.mark.django_db
+def test_verification_code_provider_exception(
+    make_organization_and_user_with_plugin_token,
+    make_user_auth_headers,
+):
+    organization, user, token = make_organization_and_user_with_plugin_token()
+    client = APIClient()
+    url = reverse("api-internal:user-verify-number", kwargs={"pk": user.public_primary_key})
+    with patch(
+        "apps.phone_notifications.phone_backend.PhoneBackend.verify_phone_number",
+        side_effect=FailedToFinishVerification,
+    ) as verify_phone_number:
+        url_with_token = f"{url}?token=some_token"
+        r = client.put(url_with_token, format="json", **make_user_auth_headers(user, token))
+        assert r.status_code == 503
+        assert verify_phone_number.call_count == 1
+
+
 @pytest.mark.django_db
 @pytest.mark.parametrize(
     "role,expected_status",
@@ -561,7 +580,7 @@ def test_user_verify_another_phone(
     client = APIClient()
     url = reverse("api-internal:user-verify-number", kwargs={"pk": other_user.public_primary_key})
 
-    with patch("apps.twilioapp.phone_manager.PhoneManager.verify_phone_number", return_value=(True, None)):
+    with patch("apps.phone_notifications.phone_backend.PhoneBackend.verify_phone_number", return_value=True):
         response = client.put(f"{url}?token=12345", format="json", **make_user_auth_headers(tester, token))
 
     assert response.status_code == expected_status
@@ -686,7 +705,7 @@ def test_admin_can_detail_users(
     assert response.status_code == status.HTTP_200_OK
 
 
-@patch("apps.twilioapp.phone_manager.PhoneManager.send_verification_code", return_value=Mock())
+@patch("apps.phone_notifications.phone_backend.PhoneBackend.send_verification_sms", return_value=Mock())
 @pytest.mark.django_db
 def test_admin_can_get_own_verification_code(
     mock_verification_start,
@@ -702,7 +721,7 @@ def test_admin_can_get_own_verification_code(
     assert response.status_code == status.HTTP_200_OK
 
 
-@patch("apps.twilioapp.phone_manager.PhoneManager.send_verification_code", return_value=Mock())
+@patch("apps.phone_notifications.phone_backend.PhoneBackend.send_verification_sms", return_value=Mock())
 @pytest.mark.django_db
 def test_admin_can_get_another_user_verification_code(
     mock_verification_start,
@@ -719,7 +738,7 @@ def test_admin_can_get_another_user_verification_code(
     assert response.status_code == status.HTTP_200_OK
 
 
-@patch("apps.twilioapp.phone_manager.PhoneManager.verify_phone_number", return_value=(True, None))
+@patch("apps.phone_notifications.phone_backend.PhoneBackend.verify_phone_number", return_value=True)
 @pytest.mark.django_db
 def test_admin_can_verify_own_phone(
     mocked_verification_check,
@@ -734,7 +753,7 @@ def test_admin_can_verify_own_phone(
     assert response.status_code == status.HTTP_200_OK
 
 
-@patch("apps.twilioapp.phone_manager.PhoneManager.verify_phone_number", return_value=(True, None))
+@patch("apps.phone_notifications.phone_backend.PhoneBackend.verify_phone_number", return_value=True)
 @pytest.mark.django_db
 def test_admin_can_verify_another_user_phone(
     mocked_verification_check,
@@ -912,7 +931,7 @@ def test_user_can_detail_users(
     assert response.status_code == status.HTTP_403_FORBIDDEN
 
 
-@patch("apps.twilioapp.phone_manager.PhoneManager.send_verification_code", return_value=Mock())
+@patch("apps.phone_notifications.phone_backend.PhoneBackend.send_verification_sms", return_value=Mock())
 @pytest.mark.django_db
 def test_user_can_get_own_verification_code(
     mock_verification_start, make_organization_and_user_with_plugin_token, make_user_auth_headers
@@ -926,7 +945,7 @@ def test_user_can_get_own_verification_code(
     assert response.status_code == status.HTTP_200_OK
 
 
-@patch("apps.twilioapp.phone_manager.PhoneManager.send_verification_code", return_value=Mock())
+@patch("apps.phone_notifications.phone_backend.PhoneBackend.send_verification_sms", return_value=Mock())
 @pytest.mark.django_db
 def test_user_cant_get_another_user_verification_code(
     mock_verification_start,
@@ -944,7 +963,7 @@ def test_user_cant_get_another_user_verification_code(
     assert response.status_code == status.HTTP_403_FORBIDDEN
 
 
-@patch("apps.twilioapp.phone_manager.PhoneManager.verify_phone_number", return_value=(True, None))
+@patch("apps.phone_notifications.phone_backend.PhoneBackend.verify_phone_number", return_value=True)
 @pytest.mark.django_db
 def test_user_can_verify_own_phone(
     mocked_verification_check, make_organization_and_user_with_plugin_token, make_user_auth_headers
@@ -958,7 +977,7 @@ def test_user_can_verify_own_phone(
     assert response.status_code == status.HTTP_200_OK
 
 
-@patch("apps.twilioapp.phone_manager.PhoneManager.verify_phone_number", return_value=(True, None))
+@patch("apps.phone_notifications.phone_backend.PhoneBackend.verify_phone_number", return_value=True)
 @pytest.mark.django_db
 def test_user_cant_verify_another_user_phone(
     mocked_verification_check,
@@ -1218,7 +1237,7 @@ def test_viewer_cant_detail_users(
     assert response.status_code == status.HTTP_403_FORBIDDEN
 
 
-@patch("apps.twilioapp.phone_manager.PhoneManager.send_verification_code", return_value=Mock())
+@patch("apps.phone_notifications.phone_backend.PhoneBackend.send_verification_sms", return_value=Mock())
 @pytest.mark.django_db
 def test_viewer_cant_get_own_verification_code(
     mock_verification_start, make_organization_and_user_with_plugin_token, make_user_auth_headers
@@ -1232,7 +1251,7 @@ def test_viewer_cant_get_own_verification_code(
     assert response.status_code == status.HTTP_403_FORBIDDEN
 
 
-@patch("apps.twilioapp.phone_manager.PhoneManager.send_verification_code", return_value=Mock())
+@patch("apps.phone_notifications.phone_backend.PhoneBackend.send_verification_sms", return_value=Mock())
 @pytest.mark.django_db
 def test_viewer_cant_get_another_user_verification_code(
     mock_verification_start,
@@ -1250,7 +1269,7 @@ def test_viewer_cant_get_another_user_verification_code(
     assert response.status_code == status.HTTP_403_FORBIDDEN
 
 
-@patch("apps.twilioapp.phone_manager.PhoneManager.verify_phone_number", return_value=(True, None))
+@patch("apps.phone_notifications.phone_backend.PhoneBackend.verify_phone_number", return_value=True)
 @pytest.mark.django_db
 def test_viewer_cant_verify_own_phone(
     mocked_verification_check, make_organization_and_user_with_plugin_token, make_user_auth_headers
@@ -1264,7 +1283,7 @@ def test_viewer_cant_verify_own_phone(
     assert response.status_code == status.HTTP_403_FORBIDDEN
 
 
-@patch("apps.twilioapp.phone_manager.PhoneManager.verify_phone_number", return_value=(True, None))
+@patch("apps.phone_notifications.phone_backend.PhoneBackend.verify_phone_number", return_value=True)
 @pytest.mark.django_db
 def test_viewer_cant_verify_another_user_phone(
     mocked_verification_check,
@@ -1340,9 +1359,7 @@ def test_forget_own_number(
 
     client = APIClient()
     url = reverse("api-internal:user-forget-number", kwargs={"pk": user.public_primary_key})
-    with patch(
-        "apps.twilioapp.phone_manager.PhoneManager.notify_about_changed_verified_phone_number", return_value=None
-    ):
+    with patch("apps.phone_notifications.phone_backend.PhoneBackend._notify_disconnected_number", return_value=None):
         response = client.put(url, None, format="json", **make_user_auth_headers(user, token))
         assert response.status_code == expected_status
 
@@ -1390,9 +1407,7 @@ def test_forget_other_number(
 
     client = APIClient()
     url = reverse("api-internal:user-forget-number", kwargs={"pk": admin_primary_key})
-    with patch(
-        "apps.twilioapp.phone_manager.PhoneManager.notify_about_changed_verified_phone_number", return_value=None
-    ):
+    with patch("apps.phone_notifications.phone_backend.PhoneBackend._notify_disconnected_number", return_value=None):
         response = client.put(url, None, format="json", **make_user_auth_headers(other_user, token))
         assert response.status_code == expected_status
 
@@ -1574,8 +1589,8 @@ def test_check_availability_other_user(make_organization_and_user_with_plugin_to
     assert response.status_code == status.HTTP_200_OK
 
 
-@patch("apps.twilioapp.phone_manager.PhoneManager.send_verification_code", return_value=Mock())
-@patch("apps.twilioapp.phone_manager.PhoneManager.verify_phone_number", return_value=(True, None))
+@patch("apps.phone_notifications.phone_backend.PhoneBackend.send_verification_sms", return_value=Mock())
+@patch("apps.phone_notifications.phone_backend.PhoneBackend.verify_phone_number", return_value=True)
 @patch(
     "apps.api.throttlers.GetPhoneVerificationCodeThrottlerPerUser.get_throttle_limits",
     return_value=(1, 10 * 60),
@@ -1616,8 +1631,8 @@ def test_phone_number_verification_flow_ratelimit_per_user(
     assert response.status_code == status.HTTP_429_TOO_MANY_REQUESTS
 
 
-@patch("apps.twilioapp.phone_manager.PhoneManager.send_verification_code", return_value=Mock())
-@patch("apps.twilioapp.phone_manager.PhoneManager.verify_phone_number", return_value=(True, None))
+@patch("apps.phone_notifications.phone_backend.PhoneBackend.send_verification_sms", return_value=Mock())
+@patch("apps.phone_notifications.phone_backend.PhoneBackend.verify_phone_number", return_value=True)
 @patch(
     "apps.api.throttlers.GetPhoneVerificationCodeThrottlerPerOrg.get_throttle_limits",
     return_value=(1, 10 * 60),
@@ -1659,7 +1674,7 @@ def test_phone_number_verification_flow_ratelimit_per_org(
     assert response.status_code == status.HTTP_429_TOO_MANY_REQUESTS
 
 
-@patch("apps.twilioapp.phone_manager.PhoneManager.send_verification_code", return_value=True)
+@patch("apps.phone_notifications.phone_backend.PhoneBackend.send_verification_sms", return_value=Mock())
 @pytest.mark.parametrize(
     "recaptcha_testing_pass,expected_status",
     [
@@ -1686,7 +1701,7 @@ def test_phone_number_verification_recaptcha(
         response = client.get(url, format="json", **request_headers)
         assert response.status_code == expected_status
         if expected_status == status.HTTP_200_OK:
-            mock_verification_start.assert_called_once_with()
+            mock_verification_start.assert_called_once_with(user)
         else:
             mock_verification_start.assert_not_called()