Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Asterisk phone provider #1282

Closed
wants to merge 3 commits into from
Closed

Implement Asterisk phone provider #1282

wants to merge 3 commits into from

Conversation

vucong2409
Copy link

What this PR does

This PR adds support for Asterisk call center beside Twilio.

Which issue(s) this PR fixes

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated

@vucong2409 vucong2409 requested review from a team as code owners February 3, 2023 10:51
@CLAassistant
Copy link

CLAassistant commented Feb 3, 2023

CLA assistant check
All committers have signed the CLA.

@vucong2409 vucong2409 changed the title feat: implement Asterisk phone provider Implement Asterisk phone provider Feb 3, 2023
Makefile Outdated
@@ -54,7 +54,7 @@ ifeq ($(DB),$(SQLITE_PROFILE))
endif

define run_docker_compose_command
$(DOCKER_COMPOSE_ENV_VARS) docker compose -f $(DOCKER_COMPOSE_FILE) $(1)
$(DOCKER_COMPOSE_ENV_VARS) docker-compose -f $(DOCKER_COMPOSE_FILE) $(1)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing that out! I've reverted that change.

Refactor phone_manager for asterisk support
@dzift
Copy link

dzift commented Feb 21, 2023

Hello! can you show an example of a working configuration for testing? When I send alerts, oncall requires twilio to confirm numbers, but I'm trying to integrate with asterisk.

@badhop
Copy link

badhop commented Mar 3, 2023

Hello! can you show an example of a working configuration for testing? When I send alerts, oncall requires twilio to confirm numbers, but I'm trying to integrate with asterisk.

Also have the same problem. Need twilio to confirm telephone number.

@Git-general-InternalIT
Copy link

Hello! can you show an example of a working configuration for testing? When I send alerts, oncall requires twilio to confirm numbers, but I'm trying to integrate with asterisk.

Also have the same problem. Need twilio to confirm telephone number.

In Env Variables -> PHONE_PROVIDER, need set to Asterisk

@badhop
Copy link

badhop commented Mar 3, 2023

Hello! can you show an example of a working configuration for testing? When I send alerts, oncall requires twilio to confirm numbers, but I'm trying to integrate with asterisk.

Also have the same problem. Need twilio to confirm telephone number.

In Env Variables -> PHONE_PROVIDER, need set to Asterisk

Already set. When you try verify phone number it sends request to asterisk, but without verification code.

@vucong2409
Copy link
Author

vucong2409 commented Mar 7, 2023

Hi guys,
Sorry for the late reply
Currently, I sent a message containing OTP via alertMessage variable (you can see that in engine/apps/twilioapp/asterisk_client.py) and expect Asterisk to have some sort of TTS set up to read it.

For example, I have a dial plan set up like below. I use https://github.com/zaf/asterisk-googletts for TTS.

[oncall]
exten => 12345, 1, Answer()
           =>  12345, n, agi(googletts.agi,${alertMessage},en)
           =>  12345, n, Hangup()

@dzift
Copy link

dzift commented Mar 22, 2023

Hi all! Please tell me how to solve the problem with verify phone number?

@Konstantinov-Innokentii
Copy link
Member

Konstantinov-Innokentii commented Mar 25, 2023

Hi, @vucong2409. First of all - sorry for the late reply. Second - thanks for the PR.
I want to provide asterisk feature for OSS and make phone providers little more "robust".
I believe, that abstraction on phone numbers should be a level higher, than a clients. I experimenting with the PhoneNotificator interface, which will hide all interactions with phone providers form the core code (mainly from alerts)
Now I'm working on that, keeping in mind you asterisk changes, so it will be great to collaborate with you.

Konstantinov-Innokentii added a commit that referenced this pull request May 24, 2023
# 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
Copy link
Member

Hi everyone. I merged #1713 with refactoring of phone provider system, which should make implementing asterisk much simpler. Waiting for contributions :)

@Konstantinov-Innokentii
Copy link
Member

Konstantinov-Innokentii commented Jun 9, 2023

We are still waiting for volunteers to finish this PR or make a new one with Asterisk integration, feel free to contact me to sync up on that!

@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the pr:stale Added to a PR that has been deemed "stale". Managed by the actions/stale GitHub Action label Jul 10, 2023
@github-actions
Copy link
Contributor

This pull request has been automatically closed because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot closed this Aug 10, 2023
@joeyorlando joeyorlando reopened this Aug 16, 2023
@joeyorlando joeyorlando removed the pr:stale Added to a PR that has been deemed "stale". Managed by the actions/stale GitHub Action label Aug 16, 2023
@joeyorlando
Copy link
Collaborator

closing this PR as it needs to be refactored to use the new phone provider interface. It's not currently on the core team's roadmap to add this provider, but if anyone would like to open a new PR, implemented using the new interface, feel free and we will definitely review 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants