add zvonok integration #2339
add zvonok integration #2339
Conversation
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
thank you 🙏 do you mind adding trailing newlines to the new files + adding back the trailing newlines to modified files?
There was a problem hiding this comment.
LGTM, great work. Left some comments.
Besides that comments it is needed to:
- Remove links to non-english docs
- Add paragraph about this phone provider to https://grafana.com/docs/oncall/latest/open-source/#twilio-setup. It's in the docs folder.
|
|
||
| class ZvonokPhoneCall(ProviderPhoneCall, models.Model): | ||
| class Meta: | ||
| db_table = "zvonok_phonecall" |
There was a problem hiding this comment.
There is no need to specify db_table here. It was specified for PhoneCallRecord only for migration purposes.
engine/apps/zvonok/urls.py
Outdated
| from django.urls import path | ||
| from .views import CallStatusCallback, HealthCheck | ||
|
|
||
| app_name = "zvonok" |
There was a problem hiding this comment.
How app_name is used? Could it be removed?
engine/apps/zvonok/views.py
Outdated
|
|
||
|
|
||
| class HealthCheck(APIView): | ||
| def get(self, request): |
There was a problem hiding this comment.
Is this view required by zvonok? If not, let's remove it
engine/settings/base.py
Outdated
| } | ||
| PHONE_PROVIDER = os.environ.get("PHONE_PROVIDER", default="twilio") | ||
|
|
||
| if PHONE_PROVIDER == "zvonok": |
There was a problem hiding this comment.
I believe we don't need this check, because currently we have no way to make it work as expected.
PHONE_PROVIDER value can be changed via UI, but at this point it still will be twilio, since value is changed through live_settings mechanism and db, not via env.
So, it makes no sense to check value of settings.PHONE_PROVIDER, but live_settings.PHONE_PROVIDER should be checked. However it's impossible, since django checks settings files before apps are loaded. You can try and will receive:
django.core.exceptions.AppRegistryNotReady: Apps aren't loaded yet. Exception.
Also, I think that force-disabling GRAFANA_CLOUD_NOTIFICATIONS_ENABLED is not good idea, since someone might want to switch to cloud notifications after zvonok is configured.
engine/settings/base.py
Outdated
| @@ -233,6 +233,7 @@ class DatabaseTypes: | |||
| "fcm_django", | |||
| "django_dbconn_retry", | |||
| "apps.phone_notifications", | |||
| "apps.zvonok" | |||
There was a problem hiding this comment.
Could you please move enabling of zvonok app to line 685, where we plug-in apps required only for oss?
engine/engine/urls.py
Outdated
| @@ -67,6 +67,11 @@ | |||
| path("api/internal/v1/", include("apps.oss_installation.urls", namespace="oss_installation")), | |||
| ] | |||
|
|
|||
| if settings.PHONE_PROVIDER == "zvonok": | |||
There was a problem hiding this comment.
It is needed to check live_settings.PHONE_PROVIDER here. If it won't work ( I'm not remember if apps are loaded before urls, you can simply enable zvonok urls if settings.IS_OPEN_SOURCE (line 65)
Fixed |
Added integration with zvonok.com service.
Features:
To process the call status, it is required to add a postback with the GET method on the side of the zvonok.com service with the following format (more info here):
${ONCALL_BASE_URL}/zvonok/call_status_events?campaign_id={ct_campaign_id}&call_id={ct_call_id}&status={ct_status}&user_choice={ct_user_choice}The names of the transmitted parameters can be redefined through environment variables.