Conversation
dburkhart07
left a comment
dburkhart07
left a comment
There was a problem hiding this comment.
some funny dsa adjustments! 🎱 🦀 📱 👑
| allVolunteerIds.map((id) => this.usersService.findOne(id)), | ||
| ); | ||
|
|
||
| if (users.length !== volunteerIds.length) { |
| async updatePantryVolunteers( | ||
| @Param('pantryId', ParseIntPipe) pantryId: number, | ||
| @Body('volunteerIds') volunteerIds: number[], | ||
| @Body('addVolunteerIds') addVolunteerIds: number[], |
There was a problem hiding this comment.
Should these both be required? Thinking we only need 1 to execute this. Should adjust tests accordingly.
| volunteerIds.forEach((id) => validateId(id, 'Volunteer')); | ||
|
|
||
| const overlap = addVolunteerIds.filter((id) => | ||
| removeVolunteerIds.includes(id), |
There was a problem hiding this comment.
Can we make removeVolunteerIds also a set for easier iteration? Both of these arrays are looped through a ton, so I think they should immediately be converted to sets, and have that used for every other check (this will reduce many computations below from O(n) -> O(1)). Hooray for Leetcode!!
| pantryId: number, | ||
| volunteerIds: number[], | ||
| addVolunteerIds: number[], | ||
| removeVolunteerIds: number[], |
There was a problem hiding this comment.
While we are on the topic of sets, on conversion, we should validate at the start that there are no duplicates within our respective lists (cant remove a volunteer twice and cant add one twice), and add a test for this.
| @@ -480,24 +495,32 @@ export class PantriesService { | |||
| } | |||
|
|
|||
| const users = await Promise.all( | |||
There was a problem hiding this comment.
Can we actually change this to use findBy({ id: In(allVolunteerIds) }) so that we only need to do one query select?
| throw new NotFoundException('One or more users not found'); | ||
| } | ||
|
|
||
| const nonVolunteers = users.filter((user) => user.role !== Role.VOLUNTEER); |
There was a problem hiding this comment.
We don't need to check that the volunteer users being removed are volunteers (we already know they are), so we can just check the ones being added.
2 participants
ℹ️ Issue
Closes SSF-171
📝 Description
pantries.controller.ts✔️ Verification
Checked with Postman that adding/removing worked and threw expected exceptions when needed. Verified written tests passed