diff --git a/apps/backend/src/pantries/pantries.controller.spec.ts b/apps/backend/src/pantries/pantries.controller.spec.ts index c5c5b14f..eb5aa283 100644 --- a/apps/backend/src/pantries/pantries.controller.spec.ts +++ b/apps/backend/src/pantries/pantries.controller.spec.ts @@ -357,17 +357,23 @@ describe('PantriesController', () => { }); describe('updatePantryVolunteers', () => { - it('should overwrite the set of volunteers assigned to a pantry', async () => { + it('should call pantriesService.updatePantryVolunteers with add and remove lists', async () => { const pantryId = 1; - const volunteerIds = [10, 11, 12]; + const addVolunteerIds = [10, 11, 12]; + const removeVolunteerIds = [1, 2]; mockPantriesService.updatePantryVolunteers.mockResolvedValue(undefined); - await controller.updatePantryVolunteers(pantryId, volunteerIds); + await controller.updatePantryVolunteers( + pantryId, + addVolunteerIds, + removeVolunteerIds, + ); expect(mockPantriesService.updatePantryVolunteers).toHaveBeenCalledWith( pantryId, - volunteerIds, + addVolunteerIds, + removeVolunteerIds, ); }); }); diff --git a/apps/backend/src/pantries/pantries.controller.ts b/apps/backend/src/pantries/pantries.controller.ts index c1e623e5..f1df4cfc 100644 --- a/apps/backend/src/pantries/pantries.controller.ts +++ b/apps/backend/src/pantries/pantries.controller.ts @@ -5,7 +5,6 @@ import { Param, ParseIntPipe, Patch, - Put, Post, Query, Req, @@ -382,11 +381,16 @@ export class PantriesController { } @Roles(Role.ADMIN) - @Put('/:pantryId/volunteers') + @Patch('/:pantryId/volunteers') async updatePantryVolunteers( @Param('pantryId', ParseIntPipe) pantryId: number, - @Body('volunteerIds') volunteerIds: number[], + @Body('addVolunteerIds') addVolunteerIds: number[], + @Body('removeVolunteerIds') removeVolunteerIds: number[], ): Promise { - return this.pantriesService.updatePantryVolunteers(pantryId, volunteerIds); + return this.pantriesService.updatePantryVolunteers( + pantryId, + addVolunteerIds, + removeVolunteerIds, + ); } } diff --git a/apps/backend/src/pantries/pantries.service.spec.ts b/apps/backend/src/pantries/pantries.service.spec.ts index 820e20a1..f3912934 100644 --- a/apps/backend/src/pantries/pantries.service.spec.ts +++ b/apps/backend/src/pantries/pantries.service.spec.ts @@ -805,7 +805,8 @@ describe('PantriesService', () => { const saved = await testDataSource.getRepository(Pantry).findOne({ where: { pantryName: 'No Volunteer Pantry' }, }); - await testDataSource.getRepository(Pantry).update(saved!.pantryId, { + if (!saved) throw new Error('Pantry not found after creation'); + await testDataSource.getRepository(Pantry).update(saved.pantryId, { status: ApplicationStatus.APPROVED, }); @@ -827,42 +828,98 @@ describe('PantriesService', () => { }); describe('updatePantryVolunteers', () => { - const getVolunteerId = async (email: string) => - ( - await testDataSource.query( - `SELECT user_id FROM users WHERE email = $1 LIMIT 1`, - [email], - ) - )[0].user_id; + it('adds volunteers to a pantry', async () => { + await service.updatePantryVolunteers(1, [7], []); + const pantry = await testDataSource + .getRepository(Pantry) + .findOne({ where: { pantryId: 1 }, relations: ['volunteers'] }); + expect(pantry?.volunteers?.map((v) => v.id)).toContain(7); + }); + + it('removes volunteers from a pantry', async () => { + await service.updatePantryVolunteers(1, [], [6]); + const pantry = await testDataSource + .getRepository(Pantry) + .findOne({ where: { pantryId: 1 }, relations: ['volunteers'] }); + expect(pantry?.volunteers?.map((v) => v.id)).not.toContain(6); + }); + + it('adds and removes volunteers in a single request', async () => { + await service.updatePantryVolunteers(1, [8], [6, 9]); + const pantry = await testDataSource + .getRepository(Pantry) + .findOne({ where: { pantryId: 1 }, relations: ['volunteers'] }); + const ids = pantry?.volunteers?.map((v) => v.id); + expect(ids).toContain(8); + expect(ids).not.toContain(6); + expect(ids).not.toContain(9); + }); + + it('silently ignores adding an already-assigned volunteer', async () => { + await service.updatePantryVolunteers(1, [6], []); + const pantry = await testDataSource + .getRepository(Pantry) + .findOne({ where: { pantryId: 1 }, relations: ['volunteers'] }); + const ids = pantry?.volunteers?.map((v) => v.id); + expect(ids?.filter((id) => id === 6)).toHaveLength(1); + }); + + it('silently ignores removing a volunteer not assigned to the pantry', async () => { + const pantryBefore = await testDataSource + .getRepository(Pantry) + .findOne({ where: { pantryId: 1 }, relations: ['volunteers'] }); + await service.updatePantryVolunteers(1, [], [8]); + const pantryAfter = await testDataSource + .getRepository(Pantry) + .findOne({ where: { pantryId: 1 }, relations: ['volunteers'] }); + expect(pantryBefore?.volunteers).toEqual(pantryAfter?.volunteers); + }); - it('replaces volunteer set', async () => { - const williamId = Number(await getVolunteerId('william.m@volunteer.org')); - await service.updatePantryVolunteers(1, [williamId]); + it('handles duplicate IDs in addVolunteerIds without constraint violation', async () => { + await expect( + service.updatePantryVolunteers(1, [7, 7], []), + ).resolves.not.toThrow(); const pantry = await testDataSource .getRepository(Pantry) .findOne({ where: { pantryId: 1 }, relations: ['volunteers'] }); - expect(pantry?.volunteers).toHaveLength(1); - expect(pantry?.volunteers?.[0].id).toBe(williamId); + const ids = pantry?.volunteers?.map((v) => v.id); + expect(ids?.filter((id) => id === 7)).toHaveLength(1); + }); + + it('handles duplicate IDs in removeVolunteerIds', async () => { + await expect( + service.updatePantryVolunteers(1, [], [6, 6]), + ).resolves.not.toThrow(); + const pantry = await testDataSource + .getRepository(Pantry) + .findOne({ where: { pantryId: 1 }, relations: ['volunteers'] }); + expect(pantry?.volunteers?.map((v) => v.id)).not.toContain(6); + }); + + it('throws BadRequestException when same ID appears in both add and remove lists', async () => { + await expect(service.updatePantryVolunteers(1, [6], [6])).rejects.toThrow( + new BadRequestException( + 'The following ID(s) appear in both the add and remove lists: 6', + ), + ); }); it('throws NotFoundException when pantry not found', async () => { - const williamId = Number(await getVolunteerId('william.m@volunteer.org')); await expect( - service.updatePantryVolunteers(9999, [williamId]), + service.updatePantryVolunteers(9999, [6], []), ).rejects.toThrow(NotFoundException); }); - it('throws NotFoundException when volunteer id does not exist', async () => { - await expect(service.updatePantryVolunteers(1, [99999])).rejects.toThrow( - NotFoundException, - ); + it('throws NotFoundException when volunteer ID does not exist', async () => { + await expect( + service.updatePantryVolunteers(1, [99999], []), + ).rejects.toThrow(new NotFoundException('User 99999 not found')); }); it('throws BadRequestException when user is not a volunteer', async () => { - const adminId = Number(await getVolunteerId('john.smith@ssf.org')); - await expect( - service.updatePantryVolunteers(1, [adminId]), - ).rejects.toThrow(BadRequestException); + await expect(service.updatePantryVolunteers(1, [1], [])).rejects.toThrow( + new BadRequestException('User(s) 1 are not volunteers'), + ); }); }); }); diff --git a/apps/backend/src/pantries/pantries.service.ts b/apps/backend/src/pantries/pantries.service.ts index b7304150..6846d38f 100644 --- a/apps/backend/src/pantries/pantries.service.ts +++ b/apps/backend/src/pantries/pantries.service.ts @@ -332,7 +332,7 @@ export class PantriesService { pantryMessage.subject, pantryMessage.bodyHTML, ); - } catch (error) { + } catch { throw new InternalServerErrorException( 'Failed to send pantry application submitted confirmation email to representative', ); @@ -345,7 +345,7 @@ export class PantriesService { adminMessage.subject, adminMessage.bodyHTML, ); - } catch (error) { + } catch { throw new InternalServerErrorException( 'Failed to send new pantry application notification email to SSF', ); @@ -419,7 +419,7 @@ export class PantriesService { message.subject, message.bodyHTML, ); - } catch (error) { + } catch { throw new InternalServerErrorException( 'Failed to send pantry account approved notification email to representative', ); @@ -465,10 +465,25 @@ export class PantriesService { async updatePantryVolunteers( pantryId: number, - volunteerIds: number[], + addVolunteerIds: number[], + removeVolunteerIds: number[], ): Promise { validateId(pantryId, 'Pantry'); - volunteerIds.forEach((id) => validateId(id, 'Volunteer')); + + const overlap = addVolunteerIds.filter((id) => + removeVolunteerIds.includes(id), + ); + if (overlap.length > 0) { + throw new BadRequestException( + `The following ID(s) appear in both the add and remove lists: ${overlap.join( + ', ', + )}`, + ); + } + + const uniqueAddIds = [...new Set(addVolunteerIds)]; + const allVolunteerIds = [...uniqueAddIds, ...removeVolunteerIds]; + allVolunteerIds.forEach((id) => validateId(id, 'Volunteer')); const pantry = await this.repo.findOne({ where: { pantryId }, @@ -480,24 +495,32 @@ export class PantriesService { } const users = await Promise.all( - volunteerIds.map((id) => this.usersService.findOne(id)), + allVolunteerIds.map((id) => this.usersService.findOne(id)), ); - if (users.length !== volunteerIds.length) { - throw new NotFoundException('One or more users not found'); - } - const nonVolunteers = users.filter((user) => user.role !== Role.VOLUNTEER); if (nonVolunteers.length > 0) { throw new BadRequestException( - `Users ${nonVolunteers + `User(s) ${nonVolunteers .map((user) => user.id) .join(', ')} are not volunteers`, ); } - pantry.volunteers = users; + const usersToAdd = users.filter((u) => uniqueAddIds.includes(u.id)); + + const currentVolunteers = pantry.volunteers ?? []; + const filteredVolunteers = currentVolunteers.filter( + (v) => !removeVolunteerIds.includes(v.id), + ); + + const existingVolunteerIds = new Set(filteredVolunteers.map((v) => v.id)); + const volunteersToAdd = usersToAdd.filter( + (u) => !existingVolunteerIds.has(u.id), + ); + + pantry.volunteers = [...filteredVolunteers, ...volunteersToAdd]; await this.repo.save(pantry); } diff --git a/apps/backend/src/users/users.service.ts b/apps/backend/src/users/users.service.ts index f72d9fae..a9676553 100644 --- a/apps/backend/src/users/users.service.ts +++ b/apps/backend/src/users/users.service.ts @@ -81,7 +81,7 @@ export class UsersService { message.subject, message.bodyHTML, ); - } catch (error) { + } catch { throw new InternalServerErrorException( 'Failed to send account created notification email to volunteer', );