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-53433] Use JobID to differentiate bot initiated sessions #462

Merged
merged 5 commits into from
Aug 31, 2023

Conversation

streamer45
Copy link
Collaborator

@streamer45 streamer45 commented Jun 28, 2023

Summary

With the introduction of new job types (e.g. transcriptions) we need a way to properly identify bot initiated sessions. So far we assumed all bot connections were recording related but that won't always be the case going forward. The idea is to pass a ContextID JobID parameter that univocally identifies a resource/job type.

Related PR

mattermost/calls-recorder#38

Ticket Link

https://mattermost.atlassian.net/browse/MM-53433

@streamer45 streamer45 added 2: Dev Review Requires review by a core committer Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) labels Jun 28, 2023
@streamer45 streamer45 added this to the v0.18.0 / MM 8.1 (ESR) milestone Jun 28, 2023
@streamer45 streamer45 requested a review from cpoile June 28, 2023 22:49
@streamer45 streamer45 self-assigned this Jun 28, 2023
Comment on lines 25 to 34
export type CallsClientJoinData = {
channelID: string;
title?: string;
threadID?: string;

// Calls bot only
// contextID is the id used to track the context of the bot connection to
// a call (e.g. recording, transcription).
contextID?: string;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this could fit in https://github.com/mattermost/calls-common since it's part of the public API. For this PR though it's practically unnecessary.

@codecov-commenter
Copy link

codecov-commenter commented Jun 28, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.05% ⚠️

Comparison is base (5c059da) 5.62% compared to head (a1e7e53) 5.57%.
Report is 11 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##            main    #462      +/-   ##
========================================
- Coverage   5.62%   5.57%   -0.05%     
========================================
  Files         21      21              
  Lines       4178    4213      +35     
========================================
  Hits         235     235              
- Misses      3926    3961      +35     
  Partials      17      17              
Files Changed Coverage Δ
server/job_service.go 0.00% <0.00%> (ø)
server/recording_api.go 0.00% <0.00%> (ø)
server/session.go 0.00% <0.00%> (ø)
server/websocket.go 0.00% <0.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@streamer45
Copy link
Collaborator Author

Will have to update the calls-recorder image we use in the Gitlab pipeline to make e2e pass.

@streamer45 streamer45 added 3: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Jun 30, 2023
@streamer45 streamer45 changed the title [MM-53433] Use ContextID to differentiate bot initiated sessions [MM-53433] Use JobID to differentiate bot initiated sessions Aug 28, 2023
@streamer45
Copy link
Collaborator Author

@cpoile Heads up, I applied the changes as suggested in mattermost/rtcd#115 (comment)

@streamer45 streamer45 removed the Do Not Merge/Awaiting PR Awaiting another pull request before merging (e.g. server changes) label Aug 31, 2023
@streamer45 streamer45 merged commit 177eded into main Aug 31, 2023
@streamer45 streamer45 deleted the MM-53433 branch August 31, 2023 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants