Repository navigation
MSPCA-2: get volunteer by id and update volunteer by id - #2
shreeyaadhikari wants to merge 26 commits into
Conversation
… sa/mspca-2-get-volunteer-by-id-and-update-volunteer-by-id
dburkhart07
left a comment
There was a problem hiding this comment.
A lot of just small nits and general style guidelines that we will follow throughout development. Logic looks really great so far. Ty Shreeya!!!! 🐼
… sa/mspca-2-get-volunteer-by-id-and-update-volunteer-by-id
dburkhart07
left a comment
There was a problem hiding this comment.
all very small things, but looking really good! hopefully should be good to go after these changes!!
… sa/mspca-2-get-volunteer-by-id-and-update-volunteer-by-id
dburkhart07
left a comment
There was a problem hiding this comment.
can you remove the columnName and relationName functions in the plural naming strategy? after that its good so ill approve
… sa/mspca-2-get-volunteer-by-id-and-update-volunteer-by-id
| * @returns The volunteer with the given ID | ||
| * @throws NotFoundException if no volunteer exists with the given ID | ||
| */ | ||
| async getVolunteerById(id: number): Promise<FosterVolunteer> { |
There was a problem hiding this comment.
i see an existing findByIdOrFail(id) on main here @dburkhart07 should we use that method instead? it's the same method/impl with a diff name~
There was a problem hiding this comment.
yeah, in hindsight i didnt think that entire service method would get implemented by another ticket, but it did end up happening. now that we have it, it's a good helper to use, and should be used here as well. let's rather change findByIdOrFail to return it with the assignedCoordinator relation though, and update the tests
There was a problem hiding this comment.
heads up that my #7 is merged now & i already implemented the findByIdOrFail with loading relations: ['assignedCoordinator'] there.
mspca/apps/backend/src/volunteers/volunteers.service.ts
Lines 26 to 37 in 9c642fc
so after pulling main's latest ver, u'd just need to replace the duplicate refs to findOneBy in getVolunteerById & updateVolunteerById to call findByIdOrFail instead.
| * @throws NotFoundException if no volunteer exists with the given ID | ||
| */ | ||
| async getVolunteerById(id: number): Promise<FosterVolunteer> { | ||
| const volunteer = await this.repo.findOneBy({ volunteerId: id }); |
There was a problem hiding this comment.
findOneBy loads no relations so assignedCoordinator comes back missing, should we pass it in as relations: ['assignedCoordinator']? @dburkhart07
There was a problem hiding this comment.
ticket says
Build PATCH /volunteers/:id endpoint. Address, city, zipcode, homebase, residentAnimals, notes, and fosterType should all be editable fields in the DTO.
but dto also exposes firstName, lastName, phone, secondaryPhone, email as editable fields - want to double check that's intended.. @dburkhart07
There was a problem hiding this comment.
still waiting on client communication for what fields we want to be editable. for right now, let's just say all the ones currently implemented here (there are a few noneditable fields that i already had removed), and we will limit scope later on as we get more information. good catch though, sorry about the inconsistency
Yurika-Kan
left a comment
There was a problem hiding this comment.
after addressing #2 (comment), everything will be aligned & it will lgtm <333
ℹ️ Issue
Closes MSPCA-2
📝 Description
Adds the ability for foster coordinators to look up a single volunteer's full details by ID and update their info without overwriting the entire record.
Briefly list the changes made to the code:
✔️ Verification
Ran yarn test. All service and controller tests pass for VolunteersService and VolunteersController.
🏕️ (Optional) Future Work / Notes
Did you notice anything ugly during the course of this ticket? Any bugs, design challenges, or unexpected behavior? Write it down so we can clean it up in a future ticket!