[MM-51499] Implement API endpoint to start calls #437
Conversation
streamer45
added
Do Not Merge
Codecov ReportPatch coverage has no change and project coverage change:
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
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
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.
| if post.RootId != "" { | ||
| return nil, fmt.Errorf("thread is not a root post") | ||
| } |
There was a problem hiding this comment.
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 { | |||
There was a problem hiding this comment.
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?
| } else if prevState.Call == nil { | ||
| if err := p.handleCallStarted(state.Call, req, channel); err != nil { |
There was a problem hiding this comment.
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?
|
Closing as it will be much easier to create it from scratch after all the changes, especially SQL store. |
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:
Example
Start a call
Request body (JSON):
{ "channel_id": "", // required "thread_id": "", // optional "title": "" // optional }End a call
Empty request body.
curl -v -X POST -H 'Authorization: Bearer xxx' \ http://localhost:8065/plugins/com.mattermost.calls/calls/3gpph3ezpp83fqndj456fd31oa/endTicket Link
https://mattermost.atlassian.net/browse/MM-49965
https://mattermost.atlassian.net/browse/MM-51499