Refactor WorkspaceMemberDto transpilation

[prastoin]

Introduction

Following #11914

closing twentyhq/core-team-issues#1011

[github-actions]

TODOs/FIXMEs:

• // TODO Refactor to be transpiled to WorkspaceMember instead: packages/twenty-server/src/engine/core-modules/user/user.resolver.ts
• // TODO Refactor to be transpiled to WorkspaceMember instead: packages/twenty-server/src/engine/core-modules/user/user.resolver.ts
• // TODO Refactor should not throw ? typed as nullable ?: packages/twenty-server/src/engine/core-modules/user/user.resolver.ts
• // TODO: Fix typing disrepency between Entity and DTO: packages/twenty-server/src/engine/core-modules/user/user.resolver.ts

Generated by 🚫 dangerJS against cc86019

[greptile-apps]

PR Summary

This PR consolidates workspace member DTO transpilation by introducing a new WorkspaceMemberTranspiler service and removing the redundant DeletedWorkspaceMemberTranspiler.

• Introduced WorkspaceMemberTranspiler service in /packages/twenty-server/src/engine/core-modules/user/services/workspace-member-transpiler.service.ts to handle both active and deleted workspace member transformations
• Refactored UserResolver to use the new transpiler service with improved error handling and type safety
• Added fromRoleEntityToRoleDto utility in /packages/twenty-server/src/engine/metadata-modules/role/utils/fromRoleEntityToRoleDto.util.ts for consistent role mapping
• Modified PermissionsService to support a new UserWorkspacePermissions type and prepare for V2 permissions
• Removed redundant DeletedWorkspaceMemberTranspiler service and its references in UserModule

_{7 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings | Greptile}

[greptile-apps]

style: Consider adding @WorkspaceIsNullable() decorator if avatarUrl should be optional. Remove the comment after decision is made.

[prastoin]

Hey @charlesBochet I've been facing few assertion within the code expecting this field to be nullable, do you please have more information regarding this ?

[charlesBochet]

Sure!

Main concern: have only one value (null or '') to represent an "empty" string in DB. This simplifies our code.

Previous vision: use ''
New vision: use null

Why this change: NULLs don't take disk space in postgres + it's easier to make unicity constraint where not null

So, avatarUrl should become Nullable but is not for now. The migration effort hasn't started yet and it might be too ambitious to tackle it now. In the current codebase, avatarUrl is of type TEXT and should not be nullable.

Note: this null state is also true for JSON ({} vs null}, ARRAY ([] vs null) and TEXT ('' vs null). Composite Fields should respect their subField types, which is not the case at all yet

[prastoin]

Thanks for the clarification,
I think we should tackle all of the raised points pretty soon to avoid building any legacy on them, or at least update their typing through the applications to start bullet proofing the application to these about to become nullable fields.
Because from my understanding some eng has started coding having this in mind, leading to sometimes assertions checking the nullability of the data and sometimes not. Not knowing which assertion side should be considered as legacy, in order to prevent such mind gym, for the workspace entity we could add string | null | '' to the avatarUrl typing

Lets discuss this when you have some free time

[github-actions]

🚀 Preview Environment Ready!

Your preview environment is available at: http://bore.pub:22340

This environment will automatically shut down when the PR is closed or after 5 hours.

[prastoin]

I still want to implement integration tests on these

[charlesBochet]

Thanks @prastoin, the PR seems reasonable, let's add integration test on the resolver indeed!