[SSF-211] promote volunteer to admin#187
Conversation
jxuistrying
changed the title
|
Also backend tests are passing for me so not sure why you're having issues. |
dburkhart07
left a comment
dburkhart07
left a comment
There was a problem hiding this comment.
few questions pending yurika's clarification, but still some good things to work on. make sure all these updates you make you make to the tests too!
| @Roles(Role.ADMIN) | ||
| async promoteToAdmin( | ||
| @Param('id', ParseIntPipe) id: number, | ||
| @Body() dto: UpdateUserRoleDto, |
There was a problem hiding this comment.
This may be a very rare case where we do not need a body. We know we are already promoting the volunteer to admin, so we shouldn't need to verify any other data. Can we delete this and the dto, and the check in the controller?
| return this.usersService.update(id, dto); | ||
| } | ||
|
|
||
| @Patch('/:id/role') |
There was a problem hiding this comment.
nit: Can we rename this to something more descriptive, perhaps @Patch('/:id/promote-volunteer)
| async promoteToAdmin( | ||
| @Param('id', ParseIntPipe) id: number, | ||
| @Body() dto: UpdateUserRoleDto, | ||
| ): Promise<User> { |
There was a problem hiding this comment.
Most of our patch api calls will return void you'll see, let's do the same here and update the service function and all tests.
| [userId], | ||
| ); | ||
|
|
||
| if (user.userCognitoSub) { |
There was a problem hiding this comment.
This is a required field, it will aways exist
| ); | ||
| } | ||
|
|
||
| const queryRunner = this.dataSource.createQueryRunner(); |
There was a problem hiding this comment.
I don't think we need any of this queryRunner, and we should be able to just set the volunteers to [] and save that. I think that may have been missed in the ticket, so can you do that? Should just be able to do this.repo.save instead (like all our other service functions).
| if (user.userCognitoSub) { | ||
| await this.authService.addUserToGroup(user.email, 'admin'); | ||
| await this.authService.removeUserFromGroup(user.email, 'volunteer'); | ||
| } |
There was a problem hiding this comment.
Can we wrap all 3 service calls in one single transaction? Look at many of our other service functions (one example would be the create function in the donations service) to understand how we go about using a transactionManager. The boilerplate code should be identical to that.
| AdminCreateUserCommand, | ||
| } from '@aws-sdk/client-cognito-identity-provider'; | ||
|
|
||
| import { |
There was a problem hiding this comment.
nit: include these imports in the same one as above
| } | ||
| } | ||
|
|
||
| async addUserToGroup(username: string, groupName: string): Promise<void> { |
There was a problem hiding this comment.
@Yurika-Kan now that we have this function, should we be calling it for when we create a new volunteer as well? what about pantry and fm?
3 participants
ℹ️ Issue
Closes [SSF-211]
📝 Description
✔️ Verification
used frontend to test promoting a volunteer to admin and checked the network tab to see user role updated to admin
🏕️ (Optional) Future Work / Notes
my yarn tests are failing currently but im not sure how to fix it yet.