BC-8168 - Implementing video conferences in FE and remaining issues #5420
Conversation
| } | ||
|
|
||
| /* istanbul ignore next */ | ||
| throw new Error(`Unsupported board reference type ${context.type as string}`); |
There was a problem hiding this comment.
maybe throwing a BadRequestException?
| return features; | ||
| } | ||
|
|
||
| if (context.type === BoardExternalReferenceType.Room) { |
There was a problem hiding this comment.
I'm wondering what BoardExternalReferenceType.User is.
Will that case ever happen?
There was a problem hiding this comment.
BoardExternalReferenceType.User is for the MediaBoard which exists in the context of a user. So that is theoretically possible i think.
| @@ -52,7 +52,7 @@ export class BoardNodeAuthorizableService implements AuthorizationLoaderService | |||
| const rootIds = boardNodes.map((node) => node.rootId); | |||
| const parentIds = boardNodes.map((node) => node.parentId).filter((defined) => defined) as EntityId[]; | |||
| const boardNodeMap = await this.getBoardNodeMap([...rootIds, ...parentIds]); | |||
| const promises = boardNodes.map((boardNode) => { | |||
| const promises = boardNodes.map(async (boardNode) => { | |||
There was a problem hiding this comment.
you only need async when using await. a normal fn can happily return a promise
There was a problem hiding this comment.
Well that was a fix for a linter warning. But yes, I agree. Doesn't have to be awaited.
There was a problem hiding this comment.
But we can re-write the function using async / await when we eliminate the then. Maybe that's more readable.
There was a problem hiding this comment.
Updated the code accordingly.
|
|
||
| return board; | ||
| return { board, features }; |
There was a problem hiding this comment.
this seems to be the simplest solution to just add the features in the initial response.
we should not really follow strict rest as we are building an interactive web app and not a static directory
There was a problem hiding this comment.
Do you mean adding it to ColumnBoard? That would mean altering the domain object.
|
|
||
| if ( | ||
| this.isVideoConferenceEnabledForCourse(course.features) || | ||
| (await this.isVideoConferenceEnabledForSchool(course.school.id)) || |
There was a problem hiding this comment.
you will get the feature pushed into the array, even if only the school has VC activated, but the course has not, did I see that right? That would not be correct, as we could have the case, that VC are activated for the whole school, but not for the specific course. Could you validate my thought?
| if (context.type === BoardExternalReferenceType.Room) { | ||
| const room = await this.roomService.getSingleRoom(context.id); | ||
|
|
||
| if ((await this.isVideoConferenceEnabledForSchool(room.schoolId)) || this.isVideoConferenceEnabledForConfig()) { |
There was a problem hiding this comment.
also here we get the feature in both cases, even if its only activated for the instance, but not for the school, that does not sound right
There was a problem hiding this comment.
Good point, we should be able to deactivate the feature for the school even if it's enabled on the instance right?
There was a problem hiding this comment.
correct, and it should (for the example above) be able to be deactivated on course level, even if its activated on a school (and therefore on the instance)
|
Description
Implementing video conferences in FE and remaining issues
Links to Tickets or other pull requests
BC-8168
hpi-schul-cloud/nuxt-client#3480
Approval for review
generate-client:serverwas executed in vue frontend and changes were tested and put in a PR with the same branch name.