BC-8193 - Expanding and registering BBB #5354

MartinSchuhmacher · 2024-11-25T14:38:41Z

Description

Start implementing BBB on boards.

Links to Tickets or other pull requests

BC-8193
hpi-schul-cloud/nuxt-client#3469
hpi-schul-cloud/dof_app_deploy#1061

Changes

scope for VC service
api tests for new VC scopes
VC element on board
target models for VC

Approval for review

DEV: If api was changed - generate-client:server was executed in vue frontend and changes were tested and put in a PR with the same branch name.
QA: In addition to review, the code has been manually tested (if manual testing is possible)
All points were discussed with the ticket creator, support-team or product owner. The code upholds all quality guidelines from the PR-template.

Notice: Please remove the WIP label if the PR is ready to review, otherwise nobody will review it.

apps/server/src/shared/domain/interface/video-conference-scope.enum.ts

apps/server/src/modules/video-conference/service/video-conference.service.ts

Loki-Afro · 2024-12-02T12:56:59Z

keep code and pr in english

uidp · 2024-12-05T15:17:06Z

apps/server/src/modules/board/controller/mapper/content-element-response.factory.spec.ts

@@ -65,6 +67,14 @@ describe(ContentElementResponseFactory.name, () => {
 		expect(result).toBeInstanceOf(DeletedElementResponse);
 	});

+	it('should return instance of DeletedElementResponse', () => {


VideoConferenceElementResponse?

right, will fix that

apps/server/src/modules/board/controller/mapper/video-conference-element-response.mapper.ts

uidp · 2024-12-05T15:24:50Z

apps/server/src/modules/board/service/internal/board-node-delete-hooks.service.ts

@@ -93,4 +97,8 @@ export class BoardNodeDeleteHooksService {
 			await this.contextExternalToolService.deleteContextExternalTool(linkedTool);
 		}
 	}
+
+	async afterDeleteVideoConferenceElement(videoConferenceElement: VideoConferenceElement): Promise<void> {
+		await this.filesStorageClientAdapterService.deleteFilesOfParent(videoConferenceElement.id);


Are there files associated with video conference elements?

during implementation I somewhen had the thought that they would have...now I can not explain how that could be xD
so will remove that, you're right 👍

uidp · 2024-12-05T15:31:25Z

apps/server/src/modules/board/testing/entity/video-conference-element-entity.factory.ts

+		title: `video conference element #${sequence}`,
+		position: 0,
+		children: [],
+		url: `url #${sequence}`,


Can we maybe write a proper URL format, like e.g. https://example.org/#{sequence}?

you are right, that is not very quality like, will change that

uidp · 2024-12-05T15:46:59Z

apps/server/src/modules/video-conference/service/video-conference.service.ts

+
+				return {
+					scopeId: element.id,
+					scopeName: 'video-conference-element',


Why not use an enum or union type for this? Or is type safety not relevant here?

not too sure about it tbh, will investigate on that

seems to be fine with the values from VideoConferenceScope enum, so I will use that for the VC scopeName also in course context

uidp · 2024-12-05T15:53:50Z

apps/server/src/modules/video-conference/service/video-conference.service.ts

+				};
+			}
+			case VideoConferenceScope.VIDEO_CONFERENCE_ELEMENT: {
+				const element = (await this.boardNodeService.findById(scopeId)) as VideoConferenceElement;


We can use findByClassAndId(VideoConferenceElement, scopId) to avoid the cast and have the type checked.

thx for the hint 👍

uidp · 2024-12-05T16:08:25Z

apps/server/src/shared/domain/entity/video-conference.entity.ts

@@ -4,6 +4,8 @@ import { BaseEntityWithTimestamps } from './base.entity';
 export enum TargetModels {
 	COURSES = 'courses',
 	EVENTS = 'events',
+	ROOMS = 'rooms',
+	VIDEO_CONFERENCE_ELEMENTS = 'video-conference-elements',


Quiet long name. Although precise. Maybe we can come up with something shorter. On the other hand we don't know if there will be other elements using VC. An what if we have to implement other VC services other than BBB? 😵‍💫

I also found myself in that rabbit hole when I thought about the naming. I thought about BBB_ELEMENTS but that was little to precise and not fitting with the rest of the services for VC. Otherwise something like BOARD_ELEMENTS could fit, but that would maybe be little too general. Whats your opinion on that?

Looks like we should stick to 'video-conference-elements'. Otherwise it would be too unspecific.

EzzatOmar

The video-conference.controller.ts apis could have better status codes.

Start Conference (PUT :scope/:scopeId/start):
Current: HttpStatus.OK
Better: HttpStatus.CREATED (201) if conference created, HttpStatus.NO_CONTENT (204) if already running.

Join Conference (GET :scope/:scopeId/join):
Current: HttpStatus.OK
Better: HttpStatus.OK (200) is good, but use HttpStatus.NOT_FOUND (404) if conference not running.

Get Conference Info (GET :scope/:scopeId/info):
Current: HttpStatus.OK
Better: HttpStatus.OK (200) is good, but use HttpStatus.NOT_FOUND (404) if no info available.

End Conference (GET :scope/:scopeId/end):
Current: HttpStatus.OK
Better: HttpStatus.NO_CONTENT (204) if successfully ended, HttpStatus.NOT_FOUND (404) if conference not found.

Good PR. Nothing drastic.

General thoughts...
The video conference module is doing too much. It's too big for doing so little. It should not have so much knowledge about the other modules. In essence, what we actually are doing in this module is managing the big blue button server. We are creating instances and returning links. I'm not even sure if it makes sense to know for which context it is used or not. I mean what kind of difference does it make if the conference is used for a room versus for a board? All we need to do is to check if the person is able to create or if the person can join/delete. So basically only the authorization is needed. I think the title does not even need to be pulled from the scoped resource. It can just be an api argument. Whoever is able to create a conference should also be able to set the title and this can be delegated to the front end.

The authorization on the other hand needs to be done. In case we separate the authorization part like in rooms, where we introduce a roomMembership module, we could delete complexity.

EzzatOmar · 2024-12-11T13:11:04Z

apps/server/src/modules/board/controller/mapper/video-conference-element-response.mapper.ts

+		return result;
+	}
+
+	canMap(element: VideoConferenceElement): boolean {


maybe use type unknown as you want to check the element type

good point 👍 maybe I change that for all the other types as well

EzzatOmar · 2024-12-11T13:15:14Z

apps/server/src/modules/board/domain/types/content-element-type.enum.ts

here is another example of our inconsistency of enum value naming. (camel case, kebab case, ...)

We might want to address that in the future @CeEv

EzzatOmar · 2024-12-11T13:18:58Z

apps/server/src/modules/board/domain/video-conference-element.do.ts

why do we need a fallback for url and title ... ?? ''.

Is there a reason not to trust the type system?

no, agree, if we use types we should trust on that

EzzatOmar · 2024-12-11T13:23:34Z

...artridge/common-cartridge-client/card-client/cards-api-client/models/content-element-type.ts

-    EXTERNAL_TOOL: 'externalTool',
-    COLLABORATIVE_TEXT_EDITOR: 'collaborativeTextEditor',
-    DELETED: 'deleted'
+	FILE: 'file',


would be nice to have this list sorted A->Z

not sure how this is sorted by openapi to be honest, would not like to handle that file manually

apps/server/src/modules/board/domain/video-conference-element.do.ts

apps/server/src/modules/video-conference/service/video-conference.service.ts

sonarqubecloud · 2024-12-16T08:10:38Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

implementing board videoconf in VC service with tests

c0095fc

MartinSchuhmacher self-assigned this Nov 25, 2024

recent state for implementing board authorization in VC

0ddbb76

uidp reviewed Nov 26, 2024

View reviewed changes

apps/server/src/shared/domain/interface/video-conference-scope.enum.ts Outdated Show resolved Hide resolved

implementing VC element and room + VC element scope

777e81b

uidp reviewed Nov 27, 2024

View reviewed changes

apps/server/src/modules/video-conference/service/video-conference.service.ts Outdated Show resolved Hide resolved

MartinSchuhmacher and others added 3 commits November 27, 2024 11:28

adjusting VC scope to board element

1e2e125

Merge branch 'main' into BC-8193-start-bbb

8bd7dba

adding VC response and node

23be7be

MartinSchuhmacher changed the title ~~BC-8193 - Erweiterung und Registrierung BBB~~ BC-8193 - Expanding and registering BBB Dec 2, 2024

MartinSchuhmacher and others added 8 commits December 2, 2024 20:13

Merge branch 'main' into BC-8193-start-bbb

2a5dd3c

adding more tests

7ffc545

Merge branch 'main' into BC-8193-start-bbb

e110ae3

adjusting VC service

633d4af

adding api tests for VC scopes

0dea59c

Merge branch 'main' into BC-8193-start-bbb

232ab4e

fixing rename issues

d8b752a

adding unit tests for coverage

b27c068

MartinSchuhmacher requested a review from uidp December 5, 2024 06:37

MartinSchuhmacher mentioned this pull request Dec 5, 2024

BC-8193 - Expanding and registering BBB hpi-schul-cloud/nuxt-client#3469

Merged

3 tasks

Merge branch 'main' into BC-8193-start-bbb

647dbd2

uidp reviewed Dec 5, 2024

View reviewed changes

uidp and others added 6 commits December 5, 2024 20:58

add school id indexes

802ad7a

Merge branch 'main' into BC-8193-start-bbb

ab4ea5a

adding yellow to room color selector

48c38b7

repositioning of yellow color in room color picker

d42ec95

adjusting after review feedback

1d536ed

Merge branch 'main' into BC-8193-start-bbb

dc7c768

EzzatOmar reviewed Dec 11, 2024

View reviewed changes

MartinSchuhmacher and others added 9 commits December 11, 2024 18:45

removing url property from VC element

66b7a98

implementing review feedback

d830df8

Merge branch 'main' into BC-8193-start-bbb

07a775e

fixing merge conflicts

b20cc2d

adding more tests for coverage

9c7ed6e

seting type alias for better maintainability

013c5f6

Merge branch 'main' into BC-8193-start-bbb

7b305ce

implementing FEATURE_COLUMN_BOARD_VIDEOCONFERENCE_ENABLED for FE usage

594e25e

adding missing dev activation for FE env

59c42f5

MartinSchuhmacher mentioned this pull request Dec 13, 2024

BC-8193 - Expanding and registering BBB hpi-schul-cloud/dof_app_deploy#1061

Merged

1 task

uidp reviewed Dec 13, 2024

View reviewed changes

apps/server/src/modules/video-conference/service/video-conference.service.ts Outdated Show resolved Hide resolved

uidp approved these changes Dec 13, 2024

View reviewed changes

apps/server/src/modules/video-conference/service/video-conference.service.ts Outdated Show resolved Hide resolved

MartinSchuhmacher and others added 2 commits December 13, 2024 20:47

fixing typo

b177a9c

Merge branch 'main' into BC-8193-start-bbb

1c69034

MartinSchuhmacher merged commit 0b4dacc into main Dec 16, 2024
76 checks passed

MartinSchuhmacher deleted the BC-8193-start-bbb branch December 16, 2024 12:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BC-8193 - Expanding and registering BBB #5354

BC-8193 - Expanding and registering BBB #5354

MartinSchuhmacher commented Nov 25, 2024 •

edited

Loading

Loki-Afro commented Dec 2, 2024

uidp Dec 5, 2024

MartinSchuhmacher Dec 10, 2024

uidp Dec 5, 2024

MartinSchuhmacher Dec 10, 2024

uidp Dec 5, 2024

MartinSchuhmacher Dec 10, 2024

uidp Dec 5, 2024

MartinSchuhmacher Dec 10, 2024

MartinSchuhmacher Dec 10, 2024 •

edited

Loading

uidp Dec 5, 2024

MartinSchuhmacher Dec 10, 2024

uidp Dec 5, 2024

MartinSchuhmacher Dec 10, 2024

uidp Dec 11, 2024

EzzatOmar Dec 11, 2024

EzzatOmar left a comment

EzzatOmar Dec 11, 2024

MartinSchuhmacher Dec 11, 2024

EzzatOmar Dec 11, 2024

EzzatOmar Dec 11, 2024

MartinSchuhmacher Dec 11, 2024

EzzatOmar Dec 11, 2024

MartinSchuhmacher Dec 11, 2024

sonarqubecloud bot commented Dec 16, 2024

BC-8193 - Expanding and registering BBB #5354

BC-8193 - Expanding and registering BBB #5354

Conversation

MartinSchuhmacher commented Nov 25, 2024 • edited Loading

Description

Links to Tickets or other pull requests

Changes

Approval for review

Loki-Afro commented Dec 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MartinSchuhmacher Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EzzatOmar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Dec 16, 2024

Quality Gate passed

MartinSchuhmacher commented Nov 25, 2024 •

edited

Loading

MartinSchuhmacher Dec 10, 2024 •

edited

Loading