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
feat(external videos): back end improvements #12504
Conversation
Refactor the external videos collection, moving the logic and functionalities outside of /imports/api/meetings to a new location in /external-videos/server/modifiers in order to decrease the coupling between the functionalities, favoring the maintenance.
Checks permission of start, stop and update external video messages and only broadcast it if the sender is presenter (unless it's system messages).
Revoked extra frontend permission checks and removed dependency from external video stop Meteor's system call.
Move all external video's system stop control events to akka-apps: - on presenter change - on presenter leave - on screenshare start
A media state may have more than 2 states. Moving this property back to integer so it can be extended.
|
||
const externalVideoUrl = body.externalVideoUrl; | ||
const user = Users.findOne({ meetingId: meetingId, userId: userId }) | ||
const { userId } = header; |
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 don't check if userId
and externalVideoUrl
are strings neither here nor in akka-apps, we just start using them (unless I am missing to read some spot)
check(meetingId, String); | ||
check(userId, String); | ||
check(requesterUserId, String); | ||
check(externalVideoUrl, String); |
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 check here but not on startExternalVideo
. Is this sufficient?
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 pushed those checks to the modifier but the userId
is missing, indeed.
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.
Tested locally and it worked well.
The check for string on share vs on consuming the url looks odd but it's still checked. We may tweak things in future.
Cherry picked some improvements we've been doing to the external videos feature.
Kudos to @frankemax and @Arthurk12