Phone provider refactoring #1713

Konstantinov-Innokentii · 2023-04-09T16:18:38Z

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 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

PhoneBackend object - contains all phone notifications business logic.
PhoneProvider - interface to external phone services provider.
TwilioPhoneProvider and SimplePhoneProvider - two examples of PhoneProvider implementation.
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.

Remove a.py

iskhakov · 2023-04-26T05:09:01Z

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


    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)


Is that expected that we don't escape < > & anymore?

https://github.com/grafana/oncall/pull/1713/files#diff-007b962c05ebca7bac34170b3d6fa519225c2bc6d652371dfc27ae018f942b73L193-L195

Yes, this escape was Twilio Related, so it's moved to TwilioPhoneProvider https://github.com/grafana/oncall/pull/1713/files#diff-764f1941b04b97b1517990791c24bc6446e623f0326b95c44060dd4ec42afd75R26

iskhakov

looks good

iskhakov · 2023-04-26T05:14:12Z

engine/apps/phone_notifications/models/phone_call.py

+
+
+# Duplicate to avoid circular import
+class TwilioCallStatuses:


This will be removed after migration, right?

Yes, I'm keeping it to be able to receive Gather and call statuses(digits from phone call) for calls which were made shortly before migration.

iskhakov · 2023-04-26T05:24:57Z

engine/apps/twilioapp/gather.py

+
+
+def get_gather_url():
+    return create_engine_url(reverse("twilioapp:gather"), "https://pretty-mosh-97.loca.lt")


hardcoded url

iskhakov · 2023-04-26T05:26:45Z

engine/apps/twilioapp/migrations/0003_auto_20230408_0711.py

+            database_operations=database_operations,
+            state_operations=state_operations
+        )
+    ]


Consider using https://docs.djangoproject.com/en/3.2/ref/models/options/#db-table instead of renaming tables to avoid data migrations

vadimkerr

Great refactoring, looks like it will make adding new providers so much easier 👍

vadimkerr · 2023-04-27T09:14:19Z

engine/apps/phone_notifications/models/phone_call.py

+    )
+
+
+class OnCallPhoneCall(models.Model):


nit: maybe remove "OnCall" from the class name so it's just PhoneCall?

I tried just PhoneCall name, but it doesn't works
For example we have this method in ProveProvider interface. If I rename OnCallPhoneCall to PhoneCall, then I should rename oncall_phone_call parameter name to phone_call which makes no sense in function singnature. On the other side I don't like PhoneCall model name with oncall_phone_call parameter, since it is confusing.

def make_notification_call(self, number, message, oncall_phone_call): ....

Same issue we have with TwilioPhoneCall model. Is has field oncall_phone_call. Renaming it to just phone call leads to lines like twilio_phone_call.phone_call.
I'm not 100% happy with OnCallPhoneCall name, but having just PhoneCall introduces more confusion.

@vadimkerr what do you think about PhoneCallRecord instead of OnCallPhoneCall. Also it sounds well: twilio_phone_call.phone_call_record ( at least better that twilio_phone_call.oncall_phone_call)

vadimkerr · 2023-04-27T09:14:37Z

engine/apps/phone_notifications/models/sms.py

+    )
+
+
+class OnCallSMS(models.Model):


nit: Maybe just SMS?

vadimkerr · 2023-04-27T09:16:50Z

engine/settings/base.py

+# Used in get_phone_provider function to dynamically load current provider.
+PHONE_PROVIDERS = {
+    "twilio": "apps.twilioapp.phone_provider.TwilioPhoneProvider",
+    "simple": "apps.phone_notifications.simple_phone_provider.SimplePhoneProvider",


This should probably be removed so it doesn't show up as an available option for PHONE_PROVIDER

joeyorlando · 2023-04-27T14:58:42Z

engine/engine/management/commands/verify_phone.py

could this still be useful?

Actually it is useful only for slack now. We process events triggered from slack synchronously, instead of running celery tasks. It is needed to make re-rendering of slack message faster, when button is pressed. We don't use this system for other chatops integrations, but ideally we should. Just keeping it consistent, waiting when we will refactor messaging backends.

joeyorlando

this is great! LGTM.

few minor questions/comments:

do we need the new get_verification_call endpoint?
I see you have "refactor tests" listed as a todo, +1 on this

joeyorlando · 2023-04-27T15:04:07Z

engine/apps/twilioapp/gather.py

+
+
+def get_gather_message():
+    return "Press 1 to acknowledge, 2 to resolve, 3 to silence to 30 minutes"


nit move "1", "2", "3" into variables and reference those in the 3 places in this file. ex:

ACK_DIGIT = "1" RESOLVE_DIGIT = "2" SILENCE_DIGIT = "3" def get_gather_message(): return f"Press {ACK_DIGIT} to acknowledge, {RESOLVE_DIGIT} to resolve, {SILENCE_DIGIT} to silence to 30 minutes"

same thing for the process_digit and process_gather_data functions

joeyorlando · 2023-04-27T15:07:28Z

engine/apps/phone_notifications/phone_provider.py

🎉🎉🎉 this is really clean, nicely done

joeyorlando · 2023-04-27T15:12:21Z

engine/apps/phone_notifications/phone_backend.py

I assume the majority of this logic was just relocated from Twilio specific code?

joeyorlando · 2023-04-27T15:45:44Z

engine/apps/twilioapp/status_callback.py

+
+
+def get_call_status_callback_url():
+    return create_engine_url(reverse("twilioapp:call_status_events"), "https://pretty-mosh-97.loca.lt")


hardcoded URL

joeyorlando · 2023-04-27T15:45:50Z

engine/apps/twilioapp/status_callback.py

+
+
+def get_sms_status_callback_url():
+    return create_engine_url(reverse("twilioapp:sms_status_events"), "https://pretty-mosh-97.loca.lt")


hardcoded URL

joeyorlando · 2023-04-27T15:46:22Z

engine/apps/twilioapp/twilio_client.py

should this file be deleted?

Yep, I'm still using it as a reference while working on PR, will delete it before merge, thanks!

joeyorlando · 2023-04-27T15:47:29Z

engine/apps/api/views/user.py

+    @action(
+        detail=True,
+        methods=["get"],
+        throttle_classes=[GetPhoneVerificationCodeThrottlerPerUser, GetPhoneVerificationCodeThrottlerPerOrg],
+    )
+    def get_verification_call(self, request, pk):
+        logger.info("get_verification_code_via_call: validating reCAPTCHA code")
+        valid = check_recaptcha_internal_api(request, "mobile_verification_code")
+        if not valid:
+            logger.warning(f"get_verification_code_via_call: invalid reCAPTCHA validation")
+            return Response("failed reCAPTCHA check", status=status.HTTP_400_BAD_REQUEST)
+        logger.info('get_verification_code_via_call: pass reCAPTCHA validation"')
+
+        user = self.get_object()
+        phone_backend = PhoneBackend()
+        try:
+            phone_backend.make_verification_call(user)
+        except NumberAlreadyVerified:
+            return Response("Phone number already verified", status=status.HTTP_400_BAD_REQUEST)
+        except FailedToStartVerification:
+            return Response("Something went wrong while calling", status=status.HTTP_500_INTERNAL_SERVER_ERROR)


new endpoint?

Yep, it is introduced keeping in mind that some providers can't sent sms, like asterisk. Twilio is also capable of verifying via call, but I didn't supported it to not overcomplicate refactoring.

# Conflicts: # engine/apps/api/views/user.py

…er settings

…tificator

Many countries are introducing different requirements for SMS senders to register and/or use alpha numeric ids, short codes or regional numbers or face being blocked. The changes in this PR will give us more flexibility by allowing us to map to different resources in Twilio based on the phone number we are trying to reach. For this first implementation the selection is made based on country code of the recipient. Verification and phone calls were given the same treatment although the immediate need is for SMS. Senders with no country code set can be used as catch-all defaults. This also falls back to the configured live settings/environment variables if not configured. Possible future additions: - Move through list of trying multiple senders before failing notification - Easily expanded to allow per-organization or per-user resources to let users and tenants configure their own Twilio - Add UI + replace live settings so users can configure their own settings - More selection criteria if needed TODO: - [x] Add+Fix Tests - [x] Verify changes are compatible with #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 <yulia.shanyrova@grafana.com>

Konstantinov-Innokentii added 2 commits March 25, 2023 15:28

Remove unused "created_for_slack" method

f1e9b30

Initial commit for PhoneProvider refactoring

2c8f83b

Konstantinov-Innokentii requested a review from a team April 9, 2023 16:18

Konstantinov-Innokentii changed the title ~~Phone notificator~~ Phone provider refactoring Apr 9, 2023

Delete a.py

bbd1430

Remove a.py

Konstantinov-Innokentii marked this pull request as draft April 9, 2023 16:21

Konstantinov-Innokentii added 4 commits April 25, 2023 22:41

Fixed receiving phone_call and sms notifications

ab928fa

Cleam twilioapp utils

e0cc0b1

Clean some comments

81178a6

Comments and logging

0ccd284

mderynck force-pushed the dev branch from c50f4f0 to cdb62f2 Compare April 25, 2023 22:40

Konstantinov-Innokentii added 6 commits April 26, 2023 12:09

Polishing

678a23d

Remove non-related changed

f66242f

Return AllowOnlyTwilio permission

b53558e

Reduce twilio status callbacks

3b99554

Keep status and sid

21d589e

Remove unised SMSMessage model

56d6ccd

iskhakov reviewed Apr 26, 2023

View reviewed changes

iskhakov approved these changes Apr 26, 2023

View reviewed changes

Konstantinov-Innokentii added 5 commits April 26, 2023 13:55

Fix migration dependencies

9ffaa24

Merge dev

7fd7600

Keep TwilioLogRecord for backward compalibility

8dd39f9

Count oss call in limits

26ebc43

Count oss call in limits

3b8479e

vadimkerr approved these changes Apr 27, 2023

View reviewed changes

joeyorlando reviewed Apr 27, 2023

View reviewed changes

joeyorlando approved these changes Apr 27, 2023

View reviewed changes

Konstantinov-Innokentii added 3 commits May 5, 2023 10:56

Rename OnCallPhoneCall/SMS to PhoneCall/SMSRecord

381aa54

Rewrite twilio phone call tests

4d79515

Fix existing tests

bb5b505

Konstantinov-Innokentii and others added 4 commits May 22, 2023 13:35

Enable cloud calls only in OSS

6147dfd

Comments polishing

81c2490

Merge branch 'dev' into phone_notificator

2eca39e

# Conflicts: # engine/apps/api/views/user.py

verification with call and send test sms button have been added to Us…

a513a0e

…er settings

Ukochka requested a review from a team May 22, 2023 14:11

Konstantinov-Innokentii added 5 commits May 23, 2023 12:55

Handle ProviderNotSupports in verification API

163aeb3

Fixes

a38f4a8

Merge branch 'dev' into phone_notificator

4f16a9b

Comment out simple phone provider

7f0e6d4

Merge remote-tracking branch 'origin/phone_notificator' into phone_no…

b9a6402

…tificator

Ukochka approved these changes May 23, 2023

View reviewed changes

Konstantinov-Innokentii added 11 commits May 23, 2023 17:53

Ignore migration linter

2ea310b

Fix tests

24d9eb0

Update CHANGELOG.md

3298392

Fix tests

7acb39c

Fix tests

3987336

Fix tests

9b71018

Fix MockPhoneProvider

e65a3c3

Make migrations backward compatible

0585099

Merge branch 'dev' into phone_notificator

7bebc85

Fix tests

2e336f9

Merge remote-tracking branch 'origin/phone_notificator' into phone_no…

0a227d4

…tificator

Konstantinov-Innokentii added this pull request to the merge queue May 24, 2023

Merged via the queue into dev with commit 1f786e8 May 24, 2023

Konstantinov-Innokentii deleted the phone_notificator branch May 24, 2023 06:32

mderynck added a commit that referenced this pull request May 24, 2023

Merge dev + #1713 phone refactoring

bd5af75

Konstantinov-Innokentii mentioned this pull request May 31, 2023

Implement Asterisk phone provider #1282

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Phone provider refactoring #1713

Phone provider refactoring #1713

Konstantinov-Innokentii commented Apr 9, 2023 •

edited

Loading

iskhakov Apr 26, 2023 •

edited

Loading

Konstantinov-Innokentii Apr 26, 2023 •

edited

Loading

iskhakov left a comment

iskhakov Apr 26, 2023

Konstantinov-Innokentii Apr 26, 2023 •

edited

Loading

iskhakov Apr 26, 2023

Konstantinov-Innokentii Apr 26, 2023

iskhakov Apr 26, 2023

vadimkerr left a comment

vadimkerr Apr 27, 2023

Konstantinov-Innokentii May 2, 2023 •

edited

Loading

Konstantinov-Innokentii May 2, 2023

vadimkerr Apr 27, 2023

vadimkerr Apr 27, 2023

joeyorlando Apr 27, 2023

Konstantinov-Innokentii May 5, 2023

joeyorlando left a comment

joeyorlando Apr 27, 2023

joeyorlando Apr 27, 2023

joeyorlando Apr 27, 2023

Konstantinov-Innokentii Apr 27, 2023

joeyorlando Apr 27, 2023

joeyorlando Apr 27, 2023

joeyorlando Apr 27, 2023

Konstantinov-Innokentii Apr 27, 2023

joeyorlando Apr 27, 2023

Konstantinov-Innokentii Apr 27, 2023



		# Duplicate to avoid circular import
		class TwilioCallStatuses:



		def get_gather_url():
		return create_engine_url(reverse("twilioapp:gather"), "https://pretty-mosh-97.loca.lt")



		def get_gather_message():
		return "Press 1 to acknowledge, 2 to resolve, 3 to silence to 30 minutes"



		def get_call_status_callback_url():
		return create_engine_url(reverse("twilioapp:call_status_events"), "https://pretty-mosh-97.loca.lt")



		def get_sms_status_callback_url():
		return create_engine_url(reverse("twilioapp:sms_status_events"), "https://pretty-mosh-97.loca.lt")

Phone provider refactoring #1713

Phone provider refactoring #1713

Conversation

Konstantinov-Innokentii commented Apr 9, 2023 • edited Loading

What this PR does

Why

Highligts

iskhakov Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

Konstantinov-Innokentii Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

iskhakov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Konstantinov-Innokentii Apr 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vadimkerr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Konstantinov-Innokentii May 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeyorlando left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Konstantinov-Innokentii commented Apr 9, 2023 •

edited

Loading

iskhakov Apr 26, 2023 •

edited

Loading

Konstantinov-Innokentii Apr 26, 2023 •

edited

Loading

Konstantinov-Innokentii Apr 26, 2023 •

edited

Loading

Konstantinov-Innokentii May 2, 2023 •

edited

Loading