545-Replace req user with req userEntity in the user.controller.ts #579
Conversation
# Conflicts: # src/user/user.controller.ts
src/user/user.controller.ts
Outdated
| this.userService.updateUser({ lastActiveAt: new Date() }, user.user.id); | ||
| return user as GetUserDto; | ||
| await this.userService.updateUser({ lastActiveAt: new Date() }, req['userEntity'].id); | ||
| return (await this.userService.getUserProfile(req['userEntity'].id)).userDto; |
There was a problem hiding this comment.
the lack of await here was intentional - we async update the users lastActiveAt after they have called their user profile.
i don't think we need the new getUserProfile function here, the change required is only
async getUserByFirebaseId(@Req() req: Request): Promise<UserEntity> {
const user = req['userEntity'];
this.userService.updateUser({ lastActiveAt: new Date() }, user.id);
return formatUserObject(user);
There was a problem hiding this comment.
Sorry to double back on this @SangilYun. @annarhughes I think we do need the getUserProfile here. As far as I understand, this endpoint is how we get all the user details to populate frontend state. Do you have a different understanding? This ticket was to ensure we still returned the same information but didn't have to grab it in the firebaseAuthGuard and mean that the GetUserDto is passed everywhere. There might be a further refactor at a later date where we break down how we grab the profile but this isn't yet implemented.
There was a problem hiding this comment.
Anna is away so i'll update you on this @SangilYun - sorry for the delay. I would to keep the service method getUserProfile. The reason this change still works is because the firebaseAuthguard is still pulling too much data. If you look at getUserByFirebaseId which the firebaseAuthguard calls, it;s getting subscriptions, sessions, partnerAdmin data. We just don't need that on every request. So I'd like the getUserProfile to be the function that adds all this contextual data. Does that make sense? Then after this change, I can return to the authGuard and only pull the data from the user repository. This means we can drop all the complex module dependencies. I hope that makes sense and sorry for the confusion in the team!
There was a problem hiding this comment.
@eleanorreem Appreciate the feedback! I've added the service function back in, please have a look :)
There was a problem hiding this comment.
Thanks so much for this contribution @SangilYun 🌟 i've just added a couple of comments to review!
src/user/user.controller.ts
Outdated
| @@ -84,7 +83,7 @@ export class UserController { | |||
| @Patch() | |||
| @UseGuards(FirebaseAuthGuard) | |||
| async updateUser(@Body() updateUserDto: UpdateUserDto, @Req() req: Request) { | |||
There was a problem hiding this comment.
It'd be good to add the return type : Promise<UserEntity> to the updateUser function here and in the controller!
|
@annarhughes Thank you for reviewing! I've made the updates, please take a look! |
|
Brilliant thanks! |
Issue link / number:
#545
What changes did you make?
Replace req["user"] with req["userEntity"] in the user.controller.ts
Why did you make the changes?
To use the pure UserEntity schema/types for all internal functions.
Did you run tests?
Yes, cypress and unit tests ⭐️