Repository navigation
MSPCA-17: Approve Volunteer Endpoint - #10
shreeyaadhikari wants to merge 12 commits into
Conversation
…d tests, add volunteer status enum
…m default, migration fix
dburkhart07
left a comment
dburkhart07
left a comment
There was a problem hiding this comment.
all very small things, looking really good so far!!! 🦓
| * @param volunteer - The Volunteer to check. | ||
| * @throws {BadRequestException} If the Volunteer's status is not Active. | ||
| */ | ||
| export function checkVolunteerActive(volunteer: FosterVolunteer): void { |
There was a problem hiding this comment.
i think since we are dealing with volunteers, this helper can be put in the volunteers service (along with its tests), and any other service that wants to check this can import the VolunteerService. The validation utils is for highly generalized helper functions that many controller or service functions may all need access to.
| async approve(id: number): Promise<FosterVolunteer> { | ||
| const volunteer = await this.findByIdOrFail(id); | ||
|
|
||
| if (volunteer.status === VolunteerStatus.ACTIVE) { |
There was a problem hiding this comment.
dont think we need this here. we only really need one check for the volunteer's status, which should be whether or not they are pending
| } | ||
|
|
||
| if (volunteer.status !== VolunteerStatus.PENDING) { | ||
| throw new BadRequestException(`Only pending Volunteers can be approved`); |
There was a problem hiding this comment.
should be a conflict exception. will need to update the documentation on the controller based on this and the above comment
| * @param value - The raw string. | ||
| * @returns The HTML-escaped string. | ||
| */ | ||
| export function escapeHtml(value: string): string { |
There was a problem hiding this comment.
this is really cool, i didnt know that we needed this (seems good for defense against strange edits people make to fields). do you think we could abstract this file into the utils file, and import it from there? let's also add some tests for it in there.
| repo.save.mockImplementation(async (v) => v); | ||
|
|
||
| const result = await service.approve(1); | ||
|
|
There was a problem hiding this comment.
let's check repo.FindByIdOrFail was also called with the right parameter
| }); | ||
| }); | ||
|
|
||
| it('throws ConflictException and does not resend the email if already active', async () => { |
There was a problem hiding this comment.
this can be combined with the test below. i think throws ConflictException and does not send an email if the volunteer is not pending
|
|
||
| await expect(service.approve(999)).rejects.toThrow( | ||
| new NotFoundException('Volunteer with ID 999 not found'), | ||
| ); |
There was a problem hiding this comment.
lets make sure findByIdOrFail was called here
ℹ️ Issue
Closes MSPCA-17
This PR is built on top of MSPCA-7 which adds the
VolunteerStatusenum this ticket depends on.📝 Description
When a foster volunteer creates an account, it starts as Pending. This PR lets foster coordinators approve a pending volunteer. Approving sets the account to Active and emails the volunteer that they've been approved and can now log in. It also adds a reusable
checkVolunteerActivehelper for future endpoints that should only work for active volunteers.Changes:
✔️ Verification
Ran yarn test. All service and controller tests pass
🏕️ (Optional) Future Work / Notes