Conversation
…idation, NotFoundError
| import { Exclude, Expose } from 'class-transformer'; | ||
| import { CurrentUserWithoutTokens } from '../../auth/dtos/current-user.dto'; | ||
| import { Project } from '../../project/project.entity'; | ||
| import { UserSimpleShowDto } from 'src/user/dtos/user.show.dto'; |
There was a problem hiding this comment.
replace with relative import
| } from 'class-transformer'; | ||
| import { IsOptional } from 'class-validator'; | ||
| import * as sanitizeHtml from 'sanitize-html'; | ||
| import { ExposeType } from 'src/utils/decorators/expose-type.decorator'; |
There was a problem hiding this comment.
replace with relative import
| import { IsEmailVerifiedGuard } from 'src/auth/is-email-verified.guard'; | ||
| import { EnrollmentRequestFromLeaderDto } from 'src/enrollment/dtos/enrollment-request.dto'; | ||
| import { RequestWithUser } from 'src/utils/request-with-user'; | ||
|
|
There was a problem hiding this comment.
replace all with relative import
| EnrollmentRequestShowDto, | ||
| EnrollmentRequestsShowDto, | ||
| } from 'src/enrollment/dtos/enrollment-request.show.dto'; | ||
|
|
There was a problem hiding this comment.
lots of relative import in user service, modify them all please
…llment management
…m/team120/Universiteams into feat/user-send-enrollment-request
Co-authored-by: NicoAntonelli <niconelli2@gmail.com>
… requests and invitations simpler methods
src/project/project.controller.ts
Outdated
| @UseGuards(...IsEmailVerifiedGuard) | ||
| @ApiCookieAuth() | ||
| @Put(':id/invitation/accept') | ||
| async acceptEnrollRequest( |
There was a problem hiding this comment.
We might need to modify this method name to 'acceptEnrollInvitation' in order to match what is being called from the service (better readability)
JAcciarri
left a comment
JAcciarri
left a comment
There was a problem hiding this comment.
Half Pull request reviewed, I will continue later 👍
src/project/project.controller.ts
Outdated
| @UseGuards(...IsEmailVerifiedGuard) | ||
| @ApiCookieAuth() | ||
| @Put(':id/invitation/decline') | ||
| async declineEnrollRequest( |
There was a problem hiding this comment.
Same here, We might need to modify this method name to 'declineEnrollInvitation' in order to match what is being called from the service and better readability
| throw new BadRequest( | ||
| 'Este usuario ya ha sido invitado para inscribirse en este proyecto', | ||
| ); | ||
| } |
There was a problem hiding this comment.
I believe this validation will never enter as per our logic, an admin won't see invitation button if user has been already invited. Having said that, we could keep it, double verification from backend is always good
| enrollment.id, | ||
| enrollRequestAdminDto.message, | ||
| action === 'approve' ? 'approved' : 'rejected', | ||
| ); |
There was a problem hiding this comment.
We are asking twice for action to equal 'approve', can this be avoided?
Additionally, do we really need last parameter passed? It is only being used by debugger.
In case we really want it to be passed, can't we make a const action only one time and then pass it wherever needed - We would be passing a RequestState.Accepted or RequestState.Rejected instead of hardcoded string
There was a problem hiding this comment.
If I remove this parameter and directly use RequestState for the debugger message (inside the private function), this would go against the suggestion we took based on your feedback at the last meet, which was that there were unnecessarily nested conditions. Passing this parameter solved that problem since it eliminates the need to make nested ternaries (in this case, for the debug message).
| enrollment.id, | ||
| enrollRequestAdminDto.message, | ||
| action === 'accept' ? 'accepted' : 'declined', | ||
| ); |
There was a problem hiding this comment.
all of the above comments apply as well here :)
| } | ||
|
|
||
| private async getUser(userId: number): Promise<User> { | ||
| const user = await this.userRepository.findOne({ |
There was a problem hiding this comment.
I'm feeling it's not a good practice to create methods under Project service that are handling other entities. Responsibility for getting one user shouldn't be under UserService? Or in case we need to use userRepository I feel like we could go ahead and just use it under ProjectService methods themselves
There was a problem hiding this comment.
I changed the functio name to getUserWithInvitations to make it clear that they are functions associated with enrollments that will only be used in this file and not in userService. However, if that is not enough, I can move them to UserService and make them public so that they can be used from ProjectService, even if they are not used there.
…d fix leftJoin wrong selection key
…er's full name and add missing leftJoin
…er's full name and add missing leftJoin
3 participants
Enrollment invitations from a user admin to a potential interested user