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

[Bugfix] add RoomCallLocator to the types for CallWithChatComposite #4398

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

realjesset
Copy link

What

missing the RoomCallLocator from CallAndChatLocator in AzureCommunicationCallWithChatAdapter.ts

Why

adds the RoomCallLocator to the CallAndChatLocator types

How Tested

ran up an instance of the CallWithChatComposite and it works, it was working without this change, but added types to make it consistent.

Process & policy checklist

  • I have updated the project documentation to reflect my changes if necessary.
  • I have read the CONTRIBUTING documentation.

Is this a breaking change?

  • This change causes current functionality to break.

@realjesset realjesset requested review from a team as code owners April 3, 2024 12:00
@realjesset realjesset changed the title fix: add RoomCallLocator to the types for CallWithChatComposite [Bug Fix] add RoomCallLocator to the types for CallWithChatComposite Apr 3, 2024
@realjesset realjesset changed the title [Bug Fix] add RoomCallLocator to the types for CallWithChatComposite [Bugfix] add RoomCallLocator to the types for CallWithChatComposite Apr 3, 2024
@mgamis-msft
Copy link
Contributor

mgamis-msft commented Apr 4, 2024

Hi @realjesset, thanks for being proactive and creating this PR. We did not intend for Rooms call to have chat. Technically, it should be possible to add a chat thread to a Rooms call. I will try this out myself to test if this works. I'm wondering if you were able to make this work?

@realjesset
Copy link
Author

Hi @realjesset, thanks for being proactive and creating this PR.

Hello @mgamis-msft! No worries at all, I use this library heavily, its my pleasure!

We did not intend for Rooms call to have chat. Technically, it should be possible to add a chat thread to a Rooms call. I will try this out myself to test if this works. I'm wondering if you were able to make this work?

so passing the roomId to the locator just works out of the box, apart from it missing types. As for the chat, once you've created a chat thread with a topic name then you can pass it like this and it works:

// ...rest of your args for adapter
locator: {
  callLocator: {
    // @ts-expect-error - this is a bug in the types
    roomId: room.id,
  },
  chatThreadId: room.threadId, // threadId we create via `${endpoint}/chat/threads?api-version=2024-03-15-preview` endpoint
},

@mgamis-msft
Copy link
Contributor

mgamis-msft commented Apr 4, 2024

Hi @realjesset, thanks for being proactive and creating this PR.

Hello @mgamis-msft! No worries at all, I use this library heavily, its my pleasure!

We did not intend for Rooms call to have chat. Technically, it should be possible to add a chat thread to a Rooms call. I will try this out myself to test if this works. I'm wondering if you were able to make this work?

so passing the roomId to the locator just works out of the box, apart from it missing types. As for the chat, once you've created a chat thread with a topic name then you can pass it like this and it works:

// ...rest of your args for adapter
locator: {
  callLocator: {
    // @ts-expect-error - this is a bug in the types
    roomId: room.id,
  },
  chatThreadId: room.threadId, // threadId we create via `${endpoint}/chat/threads?api-version=2024-03-15-preview` endpoint
},

Cool! That is great to hear to have such a heavy user.

I guess the concern here is someone who is not invited to the room can be added to the chat thread. The Rooms API intends to add chat in the future and have a secure way of inviting someone to both a room and a chat thread. They will most likely provide the chat thread id for us which we can add to the locator.
Your code change here is spot on but at this time we aren't prepared to officially add it into our API for security reasons.
You are just a little bit ahead of us :P

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.

2 participants