From 7ce4b4e82ced9b1e5e87821fd1cbed86e89b110c Mon Sep 17 00:00:00 2001 From: amywng <147568742+amywng@users.noreply.github.com> Date: Wed, 25 Mar 2026 11:11:37 -0400 Subject: [PATCH 1/2] update volunteers endpoint --- .../src/pantries/pantries.controller.spec.ts | 14 ++- .../src/pantries/pantries.controller.ts | 12 +- .../src/pantries/pantries.service.spec.ts | 103 ++++++++++++++---- apps/backend/src/pantries/pantries.service.ts | 47 ++++++-- apps/backend/src/users/users.service.ts | 2 +- 5 files changed, 134 insertions(+), 44 deletions(-) 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', ); From 566e242f981c6dfa419a29cfceb35cc397601045 Mon Sep 17 00:00:00 2001 From: amywng <147568742+amywng@users.noreply.github.com> Date: Mon, 30 Mar 2026 10:18:52 -0400 Subject: [PATCH 2/2] comments --- .../dtos/update-pantry-volunteers-dto.ts | 16 +++ .../src/pantries/pantries.controller.spec.ts | 17 ++- .../src/pantries/pantries.controller.ts | 21 ++-- .../src/pantries/pantries.service.spec.ts | 118 ++++++++++++++---- apps/backend/src/pantries/pantries.service.ts | 42 ++++--- apps/backend/src/users/users.service.spec.ts | 13 ++ apps/backend/src/users/users.service.ts | 14 +++ .../src/utils/validation.utils.spec.ts | 21 +++- apps/backend/src/utils/validation.utils.ts | 4 + 9 files changed, 213 insertions(+), 53 deletions(-) create mode 100644 apps/backend/src/pantries/dtos/update-pantry-volunteers-dto.ts diff --git a/apps/backend/src/pantries/dtos/update-pantry-volunteers-dto.ts b/apps/backend/src/pantries/dtos/update-pantry-volunteers-dto.ts new file mode 100644 index 00000000..99b943f1 --- /dev/null +++ b/apps/backend/src/pantries/dtos/update-pantry-volunteers-dto.ts @@ -0,0 +1,16 @@ +import { IsArray, IsInt, IsOptional } from 'class-validator'; +import { Type } from 'class-transformer'; + +export class UpdatePantryVolunteersDto { + @IsOptional() + @IsArray() + @IsInt({ each: true }) + @Type(() => Number) + addVolunteerIds?: number[]; + + @IsOptional() + @IsArray() + @IsInt({ each: true }) + @Type(() => Number) + removeVolunteerIds?: number[]; +} diff --git a/apps/backend/src/pantries/pantries.controller.spec.ts b/apps/backend/src/pantries/pantries.controller.spec.ts index eb5aa283..f3a75d09 100644 --- a/apps/backend/src/pantries/pantries.controller.spec.ts +++ b/apps/backend/src/pantries/pantries.controller.spec.ts @@ -22,6 +22,7 @@ import { ApplicationStatus } from '../shared/types'; import { User } from '../users/users.entity'; import { AuthenticatedRequest } from '../auth/authenticated-request'; import { UpdatePantryApplicationDto } from './dtos/update-pantry-application.dto'; +import { BadRequestException } from '@nestjs/common'; const mockPantriesService = mock(); const mockOrdersService = mock(); @@ -364,16 +365,22 @@ describe('PantriesController', () => { mockPantriesService.updatePantryVolunteers.mockResolvedValue(undefined); - await controller.updatePantryVolunteers( - pantryId, + await controller.updatePantryVolunteers(pantryId, { addVolunteerIds, removeVolunteerIds, - ); + }); expect(mockPantriesService.updatePantryVolunteers).toHaveBeenCalledWith( pantryId, - addVolunteerIds, - removeVolunteerIds, + { addVolunteerIds, removeVolunteerIds }, + ); + }); + + it('should throw BadRequestException if neither ID lists are given', async () => { + await expect(controller.updatePantryVolunteers(1, {})).rejects.toThrow( + new BadRequestException( + 'At least one of addVolunteerIds or removeVolunteerIds must be provided', + ), ); }); }); diff --git a/apps/backend/src/pantries/pantries.controller.ts b/apps/backend/src/pantries/pantries.controller.ts index f1df4cfc..ee1a5766 100644 --- a/apps/backend/src/pantries/pantries.controller.ts +++ b/apps/backend/src/pantries/pantries.controller.ts @@ -1,4 +1,5 @@ import { + BadRequestException, Body, Controller, Get, @@ -33,6 +34,7 @@ import { CheckOwnership, pipeNullable } from '../auth/ownership.decorator'; import { Public } from '../auth/public.decorator'; import { AuthenticatedRequest } from '../auth/authenticated-request'; import { UpdatePantryApplicationDto } from './dtos/update-pantry-application.dto'; +import { UpdatePantryVolunteersDto } from './dtos/update-pantry-volunteers-dto'; @Controller('pantries') export class PantriesController { @@ -384,13 +386,18 @@ export class PantriesController { @Patch('/:pantryId/volunteers') async updatePantryVolunteers( @Param('pantryId', ParseIntPipe) pantryId: number, - @Body('addVolunteerIds') addVolunteerIds: number[], - @Body('removeVolunteerIds') removeVolunteerIds: number[], + @Body(new ValidationPipe()) + body: UpdatePantryVolunteersDto, ): Promise { - return this.pantriesService.updatePantryVolunteers( - pantryId, - addVolunteerIds, - removeVolunteerIds, - ); + const { addVolunteerIds, removeVolunteerIds } = body; + if ( + (!addVolunteerIds || addVolunteerIds.length === 0) && + (!removeVolunteerIds || removeVolunteerIds.length === 0) + ) { + throw new BadRequestException( + 'At least one of addVolunteerIds or removeVolunteerIds must be provided', + ); + } + return this.pantriesService.updatePantryVolunteers(pantryId, body); } } diff --git a/apps/backend/src/pantries/pantries.service.spec.ts b/apps/backend/src/pantries/pantries.service.spec.ts index f3912934..0fe7af21 100644 --- a/apps/backend/src/pantries/pantries.service.spec.ts +++ b/apps/backend/src/pantries/pantries.service.spec.ts @@ -829,7 +829,10 @@ describe('PantriesService', () => { describe('updatePantryVolunteers', () => { it('adds volunteers to a pantry', async () => { - await service.updatePantryVolunteers(1, [7], []); + await service.updatePantryVolunteers(1, { + addVolunteerIds: [7], + removeVolunteerIds: [], + }); const pantry = await testDataSource .getRepository(Pantry) .findOne({ where: { pantryId: 1 }, relations: ['volunteers'] }); @@ -837,7 +840,10 @@ describe('PantriesService', () => { }); it('removes volunteers from a pantry', async () => { - await service.updatePantryVolunteers(1, [], [6]); + await service.updatePantryVolunteers(1, { + addVolunteerIds: [], + removeVolunteerIds: [6], + }); const pantry = await testDataSource .getRepository(Pantry) .findOne({ where: { pantryId: 1 }, relations: ['volunteers'] }); @@ -845,7 +851,10 @@ describe('PantriesService', () => { }); it('adds and removes volunteers in a single request', async () => { - await service.updatePantryVolunteers(1, [8], [6, 9]); + await service.updatePantryVolunteers(1, { + addVolunteerIds: [8], + removeVolunteerIds: [6, 9], + }); const pantry = await testDataSource .getRepository(Pantry) .findOne({ where: { pantryId: 1 }, relations: ['volunteers'] }); @@ -856,7 +865,10 @@ describe('PantriesService', () => { }); it('silently ignores adding an already-assigned volunteer', async () => { - await service.updatePantryVolunteers(1, [6], []); + await service.updatePantryVolunteers(1, { + addVolunteerIds: [6], + removeVolunteerIds: [], + }); const pantry = await testDataSource .getRepository(Pantry) .findOne({ where: { pantryId: 1 }, relations: ['volunteers'] }); @@ -868,36 +880,45 @@ describe('PantriesService', () => { const pantryBefore = await testDataSource .getRepository(Pantry) .findOne({ where: { pantryId: 1 }, relations: ['volunteers'] }); - await service.updatePantryVolunteers(1, [], [8]); + await service.updatePantryVolunteers(1, { + addVolunteerIds: [], + removeVolunteerIds: [8], + }); const pantryAfter = await testDataSource .getRepository(Pantry) .findOne({ where: { pantryId: 1 }, relations: ['volunteers'] }); expect(pantryBefore?.volunteers).toEqual(pantryAfter?.volunteers); }); - it('handles duplicate IDs in addVolunteerIds without constraint violation', async () => { + it('throws BadRequestException for duplicate IDs in addVolunteerIds', async () => { await expect( - service.updatePantryVolunteers(1, [7, 7], []), - ).resolves.not.toThrow(); - 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 === 7)).toHaveLength(1); + service.updatePantryVolunteers(1, { + addVolunteerIds: [7, 7], + removeVolunteerIds: [], + }), + ).rejects.toThrow( + new BadRequestException('addVolunteerIds contains duplicate values'), + ); }); - it('handles duplicate IDs in removeVolunteerIds', async () => { + it('throws BadRequestException for 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); + service.updatePantryVolunteers(1, { + addVolunteerIds: [], + removeVolunteerIds: [6, 6], + }), + ).rejects.toThrow( + new BadRequestException('removeVolunteerIds contains duplicate values'), + ); }); it('throws BadRequestException when same ID appears in both add and remove lists', async () => { - await expect(service.updatePantryVolunteers(1, [6], [6])).rejects.toThrow( + await expect( + service.updatePantryVolunteers(1, { + addVolunteerIds: [6], + removeVolunteerIds: [6], + }), + ).rejects.toThrow( new BadRequestException( 'The following ID(s) appear in both the add and remove lists: 6', ), @@ -906,20 +927,67 @@ describe('PantriesService', () => { it('throws NotFoundException when pantry not found', async () => { await expect( - service.updatePantryVolunteers(9999, [6], []), + service.updatePantryVolunteers(9999, { + addVolunteerIds: [6], + removeVolunteerIds: [], + }), ).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')); + service.updatePantryVolunteers(1, { + addVolunteerIds: [99999], + removeVolunteerIds: [], + }), + ).rejects.toThrow(new NotFoundException('Users not found: 99999')); + }); + + it('throws NotFoundException when some volunteer IDs do not exist', async () => { + await expect( + service.updatePantryVolunteers(1, { + addVolunteerIds: [7, 99999], + removeVolunteerIds: [], + }), + ).rejects.toThrow(new NotFoundException('Users not found: 99999')); }); it('throws BadRequestException when user is not a volunteer', async () => { - await expect(service.updatePantryVolunteers(1, [1], [])).rejects.toThrow( + await expect( + service.updatePantryVolunteers(1, { + addVolunteerIds: [1], + removeVolunteerIds: [], + }), + ).rejects.toThrow( + new BadRequestException('User(s) 1 are not volunteers'), + ); + }); + + it('throws BadRequestException when some users are not volunteers in a mixed list', async () => { + await expect( + service.updatePantryVolunteers(1, { + addVolunteerIds: [6, 1], + removeVolunteerIds: [], + }), + ).rejects.toThrow( new BadRequestException('User(s) 1 are not volunteers'), ); }); + + it('handles pantry with empty volunteers array', async () => { + const pantryId = 4; + const pantryBefore = await testDataSource + .getRepository(Pantry) + .findOne({ where: { pantryId }, relations: ['volunteers'] }); + expect(pantryBefore?.volunteers).toHaveLength(0); + await service.updatePantryVolunteers(pantryId, { + addVolunteerIds: [7], + removeVolunteerIds: [], + }); + const pantryAfter = await testDataSource + .getRepository(Pantry) + .findOne({ where: { pantryId }, relations: ['volunteers'] }); + expect(pantryAfter?.volunteers?.map((v) => v.id)).toContain(7); + }); }); }); diff --git a/apps/backend/src/pantries/pantries.service.ts b/apps/backend/src/pantries/pantries.service.ts index 6846d38f..622eba90 100644 --- a/apps/backend/src/pantries/pantries.service.ts +++ b/apps/backend/src/pantries/pantries.service.ts @@ -12,7 +12,7 @@ import { In, Repository } from 'typeorm'; import { Pantry } from './pantries.entity'; import { Order } from '../orders/order.entity'; import { User } from '../users/users.entity'; -import { validateId } from '../utils/validation.utils'; +import { hasDuplicates, validateId } from '../utils/validation.utils'; import { ApplicationStatus } from '../shared/types'; import { PantryApplicationDto } from './dtos/pantry-application.dto'; import { Role } from '../users/types'; @@ -23,6 +23,7 @@ import { UsersService } from '../users/users.service'; import { UpdatePantryApplicationDto } from './dtos/update-pantry-application.dto'; import { emailTemplates, SSF_PARTNER_EMAIL } from '../emails/emailTemplates'; import { EmailsService } from '../emails/email.service'; +import { UpdatePantryVolunteersDto } from './dtos/update-pantry-volunteers-dto'; @Injectable() export class PantriesService { @@ -465,14 +466,27 @@ export class PantriesService { async updatePantryVolunteers( pantryId: number, - addVolunteerIds: number[], - removeVolunteerIds: number[], + body: UpdatePantryVolunteersDto, ): Promise { + const { addVolunteerIds = [], removeVolunteerIds = [] } = body; validateId(pantryId, 'Pantry'); - const overlap = addVolunteerIds.filter((id) => - removeVolunteerIds.includes(id), - ); + if (hasDuplicates(addVolunteerIds)) { + throw new BadRequestException( + 'addVolunteerIds contains duplicate values', + ); + } + + if (hasDuplicates(removeVolunteerIds)) { + throw new BadRequestException( + 'removeVolunteerIds contains duplicate values', + ); + } + + const addSet = new Set(addVolunteerIds); + const removeSet = new Set(removeVolunteerIds); + + const overlap = [...addSet].filter((id) => removeSet.has(id)); if (overlap.length > 0) { throw new BadRequestException( `The following ID(s) appear in both the add and remove lists: ${overlap.join( @@ -481,8 +495,7 @@ export class PantriesService { ); } - const uniqueAddIds = [...new Set(addVolunteerIds)]; - const allVolunteerIds = [...uniqueAddIds, ...removeVolunteerIds]; + const allVolunteerIds = [...addSet, ...removeSet]; allVolunteerIds.forEach((id) => validateId(id, 'Volunteer')); const pantry = await this.repo.findOne({ @@ -494,11 +507,12 @@ export class PantriesService { throw new NotFoundException(`Pantry with ID ${pantryId} not found`); } - const users = await Promise.all( - allVolunteerIds.map((id) => this.usersService.findOne(id)), - ); + const users = await this.usersService.findByIds(allVolunteerIds); + const usersToAdd = users.filter((u) => addSet.has(u.id)); - const nonVolunteers = users.filter((user) => user.role !== Role.VOLUNTEER); + const nonVolunteers = usersToAdd.filter( + (user) => user.role !== Role.VOLUNTEER, + ); if (nonVolunteers.length > 0) { throw new BadRequestException( @@ -508,11 +522,9 @@ export class PantriesService { ); } - const usersToAdd = users.filter((u) => uniqueAddIds.includes(u.id)); - const currentVolunteers = pantry.volunteers ?? []; const filteredVolunteers = currentVolunteers.filter( - (v) => !removeVolunteerIds.includes(v.id), + (v) => !removeSet.has(v.id), ); const existingVolunteerIds = new Set(filteredVolunteers.map((v) => v.id)); diff --git a/apps/backend/src/users/users.service.spec.ts b/apps/backend/src/users/users.service.spec.ts index ac38ff75..08ff2e25 100644 --- a/apps/backend/src/users/users.service.spec.ts +++ b/apps/backend/src/users/users.service.spec.ts @@ -273,4 +273,17 @@ describe('UsersService', () => { expect(result).toEqual([]); }); }); + + describe('findByIds', () => { + it('findByIds success', async () => { + const found = await service.findByIds([1, 2]); + expect(found.map((u) => u.id)).toEqual([1, 2]); + }); + + it('findByIds with some non-existent IDs throws NotFoundException', async () => { + await expect(service.findByIds([1, 9999])).rejects.toThrow( + new NotFoundException('Users not found: 9999'), + ); + }); + }); }); diff --git a/apps/backend/src/users/users.service.ts b/apps/backend/src/users/users.service.ts index a9676553..ab54e606 100644 --- a/apps/backend/src/users/users.service.ts +++ b/apps/backend/src/users/users.service.ts @@ -102,6 +102,20 @@ export class UsersService { return user; } + async findByIds(userIds: number[]): Promise { + userIds.forEach((id) => validateId(id, 'User')); + + const users = await this.repo.findBy({ id: In(userIds) }); + + if (users.length !== userIds.length) { + const foundIds = users.map((u) => u.id); + const missingIds = userIds.filter((id) => !foundIds.includes(id)); + throw new NotFoundException(`Users not found: ${missingIds.join(', ')}`); + } + + return users; + } + async update(id: number, dto: UpdateUserInfoDto): Promise { validateId(id, 'User'); diff --git a/apps/backend/src/utils/validation.utils.spec.ts b/apps/backend/src/utils/validation.utils.spec.ts index 2d2905d0..ef1ea9b8 100644 --- a/apps/backend/src/utils/validation.utils.spec.ts +++ b/apps/backend/src/utils/validation.utils.spec.ts @@ -2,7 +2,12 @@ import { BadRequestException, InternalServerErrorException, } from '@nestjs/common'; -import { sanitizeUrl, validateEnv, validateId } from './validation.utils'; +import { + hasDuplicates, + sanitizeUrl, + validateEnv, + validateId, +} from './validation.utils'; describe('validateId', () => { it('should not throw an error for a valid ID', () => { @@ -74,3 +79,17 @@ describe('sanitizeUrl', () => { ); }); }); + +describe('hasDuplicates', () => { + it('returns true for array with duplicates', () => { + expect(hasDuplicates([1, 1, 2])).toBeTruthy(); + }); + + it('returns false for array with no duplicates', () => { + expect(hasDuplicates([1, 2, 3])).toBeFalsy(); + }); + + it('returns false for empty array', () => { + expect(hasDuplicates([])).toBeFalsy(); + }); +}); diff --git a/apps/backend/src/utils/validation.utils.ts b/apps/backend/src/utils/validation.utils.ts index 700f757e..e86d4f77 100644 --- a/apps/backend/src/utils/validation.utils.ts +++ b/apps/backend/src/utils/validation.utils.ts @@ -39,3 +39,7 @@ export function sanitizeUrl(url: string): string | null { return null; } } + +export function hasDuplicates(arr: number[]): boolean { + return new Set(arr).size !== arr.length; +}