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

[MM-51499] Implement API endpoint to start calls #437

Closed
wants to merge 1 commit into from
Closed

Conversation

streamer45
Copy link
Collaborator

Summary

PR proposes a new API endpoint to start calls. This action becomes effectively decoupled from joining as a call can be now started prior to any client connecting to it. This opens interesting opportunities for automation and integration (e.g. calendar schedule). On top of the new changes there's some minor refactoring mainly to extract common application logic and keep it away from the API layer.

Fixes #309

To note, this PR does not alter the way clients join calls. The two actions (start and join) are still coupled when connecting to calls. On this front more care is needed to avoid breaking changes and introducing more racy behaviour. I am still not entirely sure what the best approach should be but will defer to a future PR.

@cpoile At this point I am mostly looking for a quick pass to start a discussion on whether any of this makes sense. As this would mark the first attempt at supporting a public API for Calls, I'd like to take this step with some extra caution. Some still open questions I'd like to go through:

  • We should at least have some basic documentation for the endpoints and expected data format. Not sure whether in this repository itself (may be easier to keep up to date) or the in official docs.
  • Should we version the API in any way?
  • Should we also provide a sample client or extend the main Golang one?

Example

Start a call

Request body (JSON):

{
  "channel_id": "", // required
  "thread_id": "", // optional
  "title": "" // optional
}
curl -v -X POST -H 'Authorization: Bearer xxx' \
  http://localhost:8065/plugins/com.mattermost.calls/calls/start -d '{"channel_id": "3gpph3ezpp83fqndj456fd31oa"}'

End a call

Empty request body.

curl -v -X POST -H 'Authorization: Bearer xxx' \
  http://localhost:8065/plugins/com.mattermost.calls/calls/3gpph3ezpp83fqndj456fd31oa/end

Ticket Link

https://mattermost.atlassian.net/browse/MM-49965
https://mattermost.atlassian.net/browse/MM-51499

@streamer45 streamer45 added Do Not Merge Should not be merged until this label is removed Work In Progress Not yet ready for review Docs Needed Requires documentation labels May 29, 2023
@streamer45 streamer45 requested a review from cpoile May 29, 2023 12:02
@streamer45 streamer45 self-assigned this May 29, 2023
@codecov-commenter
Copy link

Codecov Report

Patch coverage has no change and project coverage change: -0.14 ⚠️

Comparison is base (b1cdaa0) 5.51% compared to head (e414fed) 5.37%.

Additional details and impacted files
@@           Coverage Diff            @@
##            main    #437      +/-   ##
========================================
- Coverage   5.51%   5.37%   -0.14%     
========================================
  Files         22      23       +1     
  Lines       4028    4131     +103     
========================================
  Hits         222     222              
- Misses      3793    3896     +103     
  Partials      13      13              
Impacted Files Coverage Δ
server/api.go 1.49% <0.00%> (+0.12%) ⬆️
server/call.go 0.00% <0.00%> (ø)
server/channel_state.go 42.37% <ø> (ø)
server/plugin.go 0.00% <0.00%> (ø)
server/websocket.go 0.00% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@cpoile cpoile left a comment

Choose a reason for hiding this comment

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

Looks fine to me! I left a couple thoughts for your consideration.

We should at least have some basic documentation for the endpoints and expected data format. Not sure whether in this repository itself (may be easier to keep up to date) or the in official docs.

If we make the client a part of this repo, then the docs should go here too, imo.

Should we version the API in any way?

If we publish a client, sure, makes sense.

Should we also provide a sample client or extend the main Golang one?

If we want to publish a client, I think it's easiest to do it in this repo. We could follow what we did in the old Playbooks repo.

Comment on lines +55 to +57
if post.RootId != "" {
return nil, fmt.Errorf("thread is not a root post")
}
Copy link
Member

Choose a reason for hiding this comment

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

Should we get the root post and use that going forward -- we could assume that the user meant to start a call in the thread that has that post (even if they didn't know the actual rootId)?

@@ -465,83 +465,23 @@ func (p *Plugin) handleLeave(us *session, userID, connID, channelID string) erro
return nil
}

func (p *Plugin) handleJoin(userID, connID, channelID, title, threadID string) error {
p.LogDebug("handleJoin", "userID", userID, "connID", connID, "channelID", channelID)
func (p *Plugin) handleJoin(userID, connID string, req CallStartRequest) error {
Copy link
Member

Choose a reason for hiding this comment

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

It might be a little confusing to have CallStartRequest as a param to handleJoin -- even if the data is the same. What do you think?

Comment on lines +481 to +482
} else if prevState.Call == nil {
if err := p.handleCallStarted(state.Call, req, channel); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

The addUserSession -> handleCallStarted flow is a bit confusing to me... If I were doing it from scratch, I would want to check if there's an ongoing call, if not then start one. That seems a bit more clear?

@streamer45
Copy link
Collaborator Author

Closing as it will be much easier to create it from scratch after all the changes, especially SQL store.

@streamer45 streamer45 closed this Jun 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Do Not Merge Should not be merged until this label is removed Docs Needed Requires documentation Work In Progress Not yet ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trigger start a call from mattermost API
3 participants