BC-7971 - introduce room members module #5291

EzzatOmar · 2024-10-15T05:08:52Z

Description

Each room needs to be linked with users including permissions.
The users should be batched into groups using the groups module.
This PR also add 2 new roles ROLE_VIEWER & ROLE_EDITOR.

Links to Tickets or other pull requests

BC-7971
Doc Page

Changes

Adds a new nest module "room-member-module".
This module is used by room module to manage members.

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.

…th room module

Metauriel · 2024-10-15T12:25:36Z

apps/server/src/modules/room-member/repo/room-member.repo.ts

+export class RoomMemberRepo {
+	constructor(private readonly em: EntityManager) {}
+
+	async findById(id: EntityId): Promise<RoomMemberEntity | null> {


The repo should return a RoomMember (DO) instead of the RoomMemberEntity.
There already is the DO, as well as the mapper, but both are so far unused. The Repo should be responsible for mapping the entity to a DO before returning it, therefore removing the entity from the interface of the repository entirely, making the repo the only class that knows about the Entity.

Metauriel · 2024-10-15T12:28:01Z

apps/server/src/modules/room-member/service/room-member.service.ts

+import { RoleRepo } from '@shared/repo';
+import { Action, AuthorizationService } from '@src/modules/authorization';
+import { Group, GroupService, GroupTypes } from '@src/modules/group';
+import { GroupEntity, GroupEntityTypes, GroupUserEmbeddable } from '@src/modules/group/entity';


forbidden import. The Module should not expose its entities, and you should not import something deeper than the index file of the module that denotes its public parts.

Metauriel · 2024-10-15T12:47:34Z

apps/server/src/modules/room-member/service/room-member.service.ts

+			id: new ObjectId().toHexString(),
+		});
+		const savedGroup = await this.groupService.save(newGroup);
+		const groupEntity = new GroupEntity({


this mapping shouldnt be part of the logic. If its absolutely necessary to do, it needs to be done in the datalayer, using a defined entity mapper. Here in the service, you should only work with the domain objects, Group in this case.

You will probably run into the issue that the RoomMemberRepo in turn will also not be allowed to know about and use these Entities and Mappers. Generally we decided to use ids instead of full DO representations to keep the modules and domains seperated, and not having to import anything that shouldnt be shared, and instead orchestrate multiple services to get all the data together that we need.

thinking this through, im not sure what the ideal solution is. I see two possible approaches.

The first one, is to use an id instead of a reference in the DO, and simply use it as an inbetween step to fetching the group next. That is definetly clean dependencywise, but also feels pretty cluncy.

The second approach is to always have the group populated in the RoomMembers object, and pushing the responsability for mapping this into the repository (since the repository should create Domain Objects).
Again, the goal should be to not knowing about the GroupEntity or the GroupDomainMapper. The cleanest would be to have only the id in the entity, and after loading the entity calling the group service from the Repo to fetch the DO for the Group, and then creating the GroupMembers DO with it. The disadvantage here is that we need two calls to the database.

a more "pragmatic" approach would be to break some rules, and simply import the things you need to do the mapping in the RoomMembers Repo. At least it would retain the borders and responsabilities of the layers, though it would mean a very close coupling to the group module. (and a break of our import rules)

An in-between solution could be to still do the populate in the database command, but without knowing what it means, and then fetching the DO from the other module via its id, relying on the identitymap to avoid another call to the database. But I dont know exactly if and how that could work, as I think the populate requires at least the entity to know the other entity, thus already breaking module borders... so I think this one seems unfeasable for now.

For this PR, im fine with either solution, as long as layer responsabilities are kept. But for the future we should figure out how to deal with such a case, where "id only" might not suffice as a hard rule.

Metauriel · 2024-10-15T12:48:34Z

apps/server/src/modules/room-member/service/room-member.service.ts

+		return this.roomMembersRepo.save(roomMemberEntity);
+	}
+
+	public async hasAuthorization(roomId: EntityId, user: User, action: Action) {


there shoudnt be a need for this function, generally the uc is calling the authorization service directly.

Metauriel · 2024-10-15T12:53:21Z

apps/server/src/modules/room-member/service/room-member.service.ts

+
+	public async addMemberToRoom(roomId: EntityId, user: User, roleName: RoleName) {
+		const role = await this.roleRepo.findByName(roleName);
+		const groupUser = new GroupUserEmbeddable({ role, user });


this is a very data centric way of doing this. I think it would be more readable to simply fetch the DO (or even recieve it as argument), call something like addMember(userid, role) on it, and then call repo.save.

That way, instanciating data object can be hidden within the repo, or the DO if it requires it as well.

Metauriel · 2024-10-15T12:58:12Z

apps/server/src/modules/room/api/room.uc.ts

+		});
+		const authorizedRooms = rooms.data.filter((room) => permittedRoomSet.has(room.id));
+
+		// TODO: must find a way to pagainate correctly over the authorized roomIds


I dont know how you would ever do that. But loading all rooms and then filtering seems extremely inefficient anyways, as you would have to load ALL rooms in the entire instance, which after a couple of years will be a LOT.

I would instead turn this around, first fetching all RoomMember entries that contain the user, then fetching all these rooms from the roomService by their ids, observing pagination.

Metauriel · 2024-10-15T13:02:04Z

apps/server/src/modules/room/api/room.uc.ts

-
+		const user = await this.authorizationService.getUserWithPermissions(userId);
+		// NOTE: currently only teacher are allowed to create rooms. Could not find simpler way to check this.
+		this.authorizationService.checkOneOfPermissions(user, [Permission.TASK_DASHBOARD_TEACHER_VIEW_V3]);


surely you can use a permission that has something to do with the operation we are doing?
Either introduce new permissions like ROOM_CREATE, or at the very least use something connected like COURSE_CREATE, assuming that everyone that may create a course is also allowed to create a room. though a new permission is most definetly cleaner.

apps/server/src/modules/room-member/do/room-member.do.ts

Metauriel · 2024-10-15T13:27:44Z

apps/server/src/modules/room/api/room.uc.ts

-
-		// TODO check authorization
+		const user = await this.authorizationService.getUserWithPermissions(userId);
+		const hasAuthorization = await this.roomMemberService.hasAuthorization(roomId, user, Action.read);


use authorisationService instead

- Return do in room member service - Move authorization from room-member to room module - fix imports

Metauriel · 2024-10-18T08:15:52Z

apps/server/src/modules/room-member/do/room-member.do.ts

+	}
+
+	public removeMember(userId: ObjectId): void {
+		this.props.members = this.props.members.filter((member) => member.userId !== userId);


I couldnt find any code in the data layer that transforms this back into the usergroup. How do you make sure changes made here are saved into the database?

I guess either you should have an actual reference to a groupDO in here that you update, or the changes made to this array need to be mapped back into the group somewhere.

Metauriel · 2024-10-18T08:27:27Z

apps/server/src/modules/room-member/repo/room-member.repo.ts

+	constructor(private readonly em: EntityManager, private readonly groupRepo: GroupRepo) {}
+
+	async findByUserId(userId: EntityId): Promise<RoomMember[]> {
+		const groups = await this.groupRepo.findGroups({ userId, groupTypes: [GroupTypes.ROOM] });


use the groupService instead. the groupRepo is private and should not be used outside the group module. It also shouldnt make a difference for you, since the repo also returns DOs.

Metauriel · 2024-10-18T08:50:14Z

apps/server/src/modules/room-member/repo/room-member.repo.ts

+		const roomEntities = await this.em.find(RoomMemberEntity, scope.query);
+		const roomMembers = await Promise.all(
+			roomEntities.map(async (roomEntity) => {
+				const group = groups.data.find((g) => g.id === roomEntity.userGroupId.toHexString());


This seems really complicated and strange. You already have the group DOs, and then you map them back to entities, in order to map the whole thing back to DOs. Why that step?

Is it just to populate the users? most of the time we wont need the users, so I would be in favor of just not populating them, and load them later when they are needed. For example, all authorisation can be done without populating (only need ids), all interactions with the main page of rooms can be done (doesnt need the user list at all, just the list of boards and such). Only interactions with the usermanagement of the rooms needs more information on the users.

So I would simply add the group itself to the DO with the information it contains (userIds), and not bother trying to populate more information into it that might not be needed for most operations. I think that also allows you to significantly reduce dependecies.

I hope to have simplified some of the data transformations.
Due to the nature of module boundaries we have different db queries.

In general I see map, filter, reduce very readable . I don't like the Promise.All things which are removed.

But maybe I don't understand oop & ddd.

Metauriel · 2024-10-18T08:51:39Z

apps/server/src/modules/group/index.ts

@@ -2,3 +2,5 @@ export { GroupModule } from './group.module';
 export { GroupConfig } from './group.config';
 export * from './domain';
 export { GroupService } from './service';
+export * from './repo';


the repo and entity are private, and shall not be exported.

Metauriel · 2024-10-18T08:53:30Z

apps/server/src/modules/room-member/repo/room-member.repo.ts

+import { EntityManager, ObjectId } from '@mikro-orm/mongodb';
+import { Injectable } from '@nestjs/common';
+import { EntityId } from '@shared/domain/types';
+import { Group, GroupRepo, GroupTypes, GroupDomainMapper } from '@src/modules/group';


GroupRepo and GroupDomainMapper are private. We should try to find a way to not need them here. If we reallly really do, we should talk in the ARC chapter about how to avoid that, and for now do a deep import with a comment and ticket to change it.

I reversed the exposure of the model internal things. Repo, Entity, Mapper.

Metauriel · 2024-10-18T08:55:13Z

apps/server/src/modules/room-member/service/room-member.service.ts

+	constructor(
+		private readonly roomMembersRepo: RoomMemberRepo,
+		private readonly groupService: GroupService,
+		private readonly roleRepo: RoleRepo,


use RoleService instead

Metauriel · 2024-10-18T08:56:11Z

apps/server/src/modules/room-member/service/room-member.service.ts

+		private readonly roomMembersRepo: RoomMemberRepo,
+		private readonly groupService: GroupService,
+		private readonly roleRepo: RoleRepo,
+		private readonly em: EntityManager


remove this, domain layer should never be tied to datalayer details.

Metauriel · 2024-10-18T08:58:12Z

apps/server/src/modules/room/api/room.uc.ts

+		private readonly roomService: RoomService,
+		private readonly roomMemberService: RoomMemberService,
+		private readonly authorizationService: AuthorizationService,
+		private readonly roomMembersRepo: RoomMemberRepo


use a service instad.

Metauriel · 2024-10-18T09:04:12Z

apps/server/src/modules/room/room.module.ts


 @Module({
-	imports: [CqrsModule],
-	providers: [RoomRepo, RoomService],
+	imports: [CqrsModule, RoomMemberModule, GroupModule, RoleModule, AuthorizationModule],


wasnt the whole purpose of having a seperate roomMemberModule to keep it out of the room Module? where did you run into issues, and need it in the roomModule? we should see if thats avoidable, otherwise we should think about combining them into one module, since it kind of defeats the point...

Metauriel · 2024-10-18T09:07:32Z

apps/server/src/modules/room/domain/do/room-authorizable.ts

+import { AuthorizableObject, DomainObject } from '@shared/domain/domain-object';
+import { RoomMember } from '@src/modules/room-member';
+
+export interface RoomAuthorizableProps extends AuthorizableObject {


this should probably be part of the RoomMembers module, since for authorization we pretty much only need the members. Maybe we dont even need a seperate class, but can just use the RoomMembers DO to authorize rooms

Yes moved to RoomMember

EzzatOmar · 2024-10-21T19:25:41Z

I made the RoomMember Module very dumb. Only 2 hard references.
The DO Layer is responsible to check for data consistence. I have implemented 2 more methods in the group service to support our use case.

Making the RoomMember dumb made the coding much easier. But the drawback is more network latency due to module boundaries.

Note: Tests are not done yet.

Metauriel

I like this structure a lot better! still some cleaning up to do obviously (the room authorizable can be removed right?), but this is the right direction.

Regarding the promise.all, it allows for a bit better performance, since different database queries can happen in parallel. If you like you can check if there is some potential there, but I dont consider that necessary for this PR.

Metauriel · 2024-10-22T13:15:28Z

apps/server/src/modules/room/room.module.ts


 @Module({
-	imports: [CqrsModule],
+	imports: [CqrsModule, RoomMemberModule, AuthorizationModule],


are these references still needed?

sonarqubecloud · 2024-10-23T11:22:01Z

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 SonarCloud

EzzatOmar · 2024-10-23T11:37:37Z

Tests included.
Cleanup fixed.

uidp

Generally I'm not happy with the entity name RoomMember because it suggests that this is about a single room member instead of a group of members assigned to a single room. Maybe it would be better to name it RoomMembership?

uidp · 2024-10-23T13:49:00Z

apps/server/src/modules/group/controller/mapper/group-response.mapper.ts

@@ -17,6 +17,7 @@ import { PeriodResponse } from '../dto/response/period.response';
 const typeMapping: Record<GroupTypes, GroupTypeResponse> = {
 	[GroupTypes.CLASS]: GroupTypeResponse.CLASS,
 	[GroupTypes.COURSE]: GroupTypeResponse.COURSE,
+	[GroupTypes.ROOM]: GroupTypeResponse.ROOM,
 	[GroupTypes.OTHER]: GroupTypeResponse.OTHER,
 };


BTW I'm wondering why we aren't using the domain enum for the group type in the response. That should actually be possible and valid IMHO. But that's not a subject of this PR.

uidp · 2024-10-23T13:52:44Z

apps/server/src/modules/group/entity/group.entity.ts

@@ -9,6 +9,7 @@ import { GroupValidPeriodEmbeddable } from './group-valid-period.embeddable';
 export enum GroupEntityTypes {
 	CLASS = 'class',
 	COURSE = 'course',
+	ROOM = 'room',
 	OTHER = 'other',
 }



Also similar to the response above we could use the GroupType enum from the domain here. Again maybe not in this PR.

uidp · 2024-10-23T13:54:14Z

apps/server/src/modules/group/repo/group-domain.mapper.ts

 	[GroupEntityTypes.OTHER]: GroupTypes.OTHER,
 };

 export const GroupTypesToGroupEntityTypesMapping: Record<GroupTypes, GroupEntityTypes> = {
 	[GroupTypes.CLASS]: GroupEntityTypes.CLASS,
 	[GroupTypes.COURSE]: GroupEntityTypes.COURSE,
+	[GroupTypes.ROOM]: GroupEntityTypes.ROOM,
 	[GroupTypes.OTHER]: GroupEntityTypes.OTHER,
 };



This kind of mapping wouldn't be necessary when we used GroupType enum from domain.

uidp · 2024-10-23T14:02:15Z

apps/server/src/modules/group/service/group.service.ts

+	public async addUserToGroup(groupId: EntityId, userId: EntityId, roleName: RoleName): Promise<void> {
+		const role = await this.roleService.findByName(roleName);
+		if (!role.id) throw new BadRequestException('Role has no id.');
+		const group = await this.findById(groupId);


Use the repo (see above)?

uidp · 2024-10-23T14:02:31Z

apps/server/src/modules/group/service/group.service.ts

+			organizationId,
+		});
+
+		await this.save(group);


For persistence operations it's maybe better to use the repo instead of the service methods. This can help prevent unwanted side effects.

One of the points discussed where that room members should not have access to the group internals.
That is why we have to use the do layer here.

uidp · 2024-10-23T14:27:23Z

apps/server/src/modules/room-member/repo/room-member.repo.ts

+	constructor(private readonly em: EntityManager) {}
+
+	async findByRoomId(roomId: EntityId): Promise<RoomMember | null> {
+		const roomMemberEntities = await this.em.findOne(RoomMemberEntity, { roomId });


singular roomMemberEntity?

uidp · 2024-10-23T14:31:34Z

apps/server/src/modules/room-member/service/room-member.service.ts

+export class RoomMemberService {
+	constructor(
+		private readonly groupService: GroupService,
+		private readonly roomMembersRepo: RoomMemberRepo,


singular roomMemberRepo? like the name of the class?

uidp · 2024-10-23T14:33:00Z

apps/server/src/modules/room-member/service/room-member.service.ts

+		return roomMember;
+	}
+
+	private static buildRoomMemberAuthorizable(


Why static?
Also we should maybe put the private methods below the public methods.

this method is free of side effects

That is why I put that as static to indicate that

uidp · 2024-10-23T14:40:29Z

apps/server/src/modules/room-member/service/room-member.service.ts

+
+	public async deleteRoomMember(roomId: EntityId) {
+		const roomMember = await this.roomMembersRepo.findByRoomId(roomId);
+		if (roomMember === null) return;


Maybe roomMember == null is more safe? I know "typescript-wise" this should be null but I personally would prefer a more general check for existence.

uidp · 2024-10-23T15:04:03Z

apps/server/src/shared/domain/interface/rolename.enum.ts

@@ -9,6 +9,8 @@ export enum RoleName {
 	DEMOTEACHER = 'demoTeacher',
 	EXPERT = 'expert',
 	HELPDESK = 'helpdesk',
+	ROOM_VIEWER = 'room_viewer',
+	ROOM_EDITOR = 'room_editor',


Is snake_case the right convention for enum values?

Yes you are right. The keys can be reversed.

But the values are already written. Hmm. but we can migrate them.

The Room Member module manages the association between users and rooms, handling permissions and roles within rooms. This module is designed to be injected into the Room module for managing user access and roles within rooms. Added: * RoomMember Module * Room roles and permissions * Rule for Rooms * Group service: new type 'room' * Group service: new service methods * Migration: insert 2 new roles and 2 new permisisons Changed: * Room Module runs authorization in UC * On new Room also a RoomMember and a Group is created with default user

EzzatOmar added 6 commits October 7, 2024 00:48

wip: added new roles and service to add user groups, tests broken.

786ce74

update authorization rule for room-members, connect to room member wi…

66b10bc

…th room module

Merge branch 'main' into BC-7971-room-members

91fe96e

fix broken do mapper tests

199aed9

remove docs

6d085a3

Merge branch 'main' into BC-7971-room-members

ac210c1

Metauriel requested changes Oct 15, 2024

View reviewed changes

EzzatOmar added 5 commits October 16, 2024 10:52

add group type = room

c2c60db

- Add group type "room" to group module.

d1c367c

- Return do in room member service - Move authorization from room-member to room module - fix imports

Merge branch 'main' into BC-7971-room-members

48ae35f

fix code coverage

fb26f78

Merge branch 'main' into BC-7971-room-members

a04929d

Metauriel requested changes Oct 18, 2024

View reviewed changes

EzzatOmar added 14 commits October 21, 2024 10:01

Merge branch 'main' into BC-7971-room-members

1ae2f1e

add group create service

72af51c

add groupservice.addUserToGroup method

11dac30

update nest module, remove not needed imports

eca91f3

add entity and do for room member

bdb8a35

add mapper for room member

e8bc6a8

add room member repo

f58d452

remove old mapper

d633c98

add room member rule

cc7d5b5

add authorization rule to room member

97eb5a9

use new structure in room member service

ca1eaed

put authorization in room uc

e3590ec

reverse group repo exposure

b6f00b9

Merge branch 'main' into BC-7971-room-members

0d6143f

Metauriel reviewed Oct 22, 2024

View reviewed changes

EzzatOmar added 8 commits October 22, 2024 20:53

fix tests

4a9f7c4

Merge branch 'main' into BC-7971-room-members

c6fab6b

add missing tests and cleanup

da969dc

Merge branch 'main' into BC-7971-room-members

100f17d

reverse authz order to make sure 404 is returned

0b5dd82

Merge branch 'main' into BC-7971-room-members

7835361

update migration backup

74f4f10

fix lint err

23988d4

Metauriel approved these changes Oct 23, 2024

View reviewed changes

EzzatOmar enabled auto-merge (squash) October 23, 2024 14:06

EzzatOmar merged commit bbca0b9 into main Oct 23, 2024
78 checks passed

EzzatOmar deleted the BC-7971-room-members branch October 23, 2024 14:47

uidp reviewed Oct 23, 2024

View reviewed changes

BC-7971 - introduce room members module #5291

BC-7971 - introduce room members module #5291

Conversation

EzzatOmar commented Oct 15, 2024 • edited Loading

Description

Links to Tickets or other pull requests

Changes

Approval for review

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

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

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 commented Oct 21, 2024

Metauriel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Oct 23, 2024

Quality Gate passed

EzzatOmar commented Oct 23, 2024

uidp 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

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 commented Oct 15, 2024 •

edited

Loading