-
Notifications
You must be signed in to change notification settings - Fork 493
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
transfer call #292
transfer call #292
Conversation
@@ -156,5 +162,4 @@ async def handle_ws_message(self, message) -> Optional[PhoneCallWebsocketAction] | |||
|
|||
def mark_terminated(self): | |||
super().mark_terminated() | |||
asyncio.create_task(self.telephony_client.end_call(self.twilio_sid)) | |||
|
|||
# asyncio.create_task(self.telephony_client.end_call(self.twilio_sid)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is commented out because it immediately disconnects the transferred call if its left in.
response = self.twilio_client.calls(twilio_sid).update(status="completed")
would love some insight here towards a solution here. I thought about using redis to set some kind of conditional flag telling it not to disconnect the call if its being transferred.
any maintainers willing to point me in the right direction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup agreed, i think there should be a boolean flag for whether or not the call should be completely disconnected (this is useful in cases where we want to end the ws but not the call eg transfer or dtmf)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 we can maybe leverage the active
prop on StreamingConversation
here. if active
, we don't end the call
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how do you want the TransferCall
action to communicate with StreamingConversation
to keep the conversation active
when its being transferred?
elif data["event"] == "stop":
self.logger.debug(f"Media WS: Received event 'stop': {message}")
self.logger.debug("Stopping...")
return PhoneCallWebsocketAction.CLOSE_WEBSOCKET
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a good first pass thank you @sethgw – we'll make some changes before merging
vocode/streaming/action/factory.py
Outdated
from vocode.streaming.models.actions import ActionType | ||
|
||
|
||
class ActionFactory: | ||
def create_action(self, action_type: str) -> BaseAction: | ||
if action_type == ActionType.NYLAS_SEND_EMAIL: | ||
return NylasSendEmail(should_respond=True) | ||
elif action_type == ActionType.TRANSFER_CALL: | ||
return TransferCall(to_phone="+15555555555") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should change this to support an arbitrary transfer call number
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think the correct move here is to add an actionconfig (with params you can set). as a workaround to get the pr in for now, what do you think about including this in the TransferCallParameters?
super().__init__(**kwargs) | ||
self.to_phone = to_phone | ||
# TODO: need to support all config managers | ||
self.config_manager = RedisConfigManager() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use the ConversationStateManager here?
or even better just use the TwilioPhoneCallAction
https://github.com/vocodedev/vocode-python/blob/e4cd86b9bca32bb3d65b6e4e4b50caf948775d87/vocode/streaming/action/phone_call_action.py#L31
to grab the sid
@@ -156,5 +162,4 @@ async def handle_ws_message(self, message) -> Optional[PhoneCallWebsocketAction] | |||
|
|||
def mark_terminated(self): | |||
super().mark_terminated() | |||
asyncio.create_task(self.telephony_client.end_call(self.twilio_sid)) | |||
|
|||
# asyncio.create_task(self.telephony_client.end_call(self.twilio_sid)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup agreed, i think there should be a boolean flag for whether or not the call should be completely disconnected (this is useful in cases where we want to end the ws but not the call eg transfer or dtmf)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just merged in the ActionConfig change: #300
can't commit to your fork so here's a patch to use the new config object here. Also updated it so you don't need the config manager (twilio_sid is already in the TwilioCallAction): https://gist.github.com/ajar98/e8b8fc31d2348fdd285a30b5004d052b
1001f04
to
85cb994
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be g2g just one last fix needed here!
@ajar98 I went ahead and made the http request non-blocking |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good, thanks @sethgw !
* transfer call * action factory * patch * get sid * comment for follow up * non-blocking
a simple action that will transfer a call #276