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 c5c5b14f..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(); @@ -357,17 +358,29 @@ 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 }, + ); + }); + + 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 c1e623e5..ee1a5766 100644 --- a/apps/backend/src/pantries/pantries.controller.ts +++ b/apps/backend/src/pantries/pantries.controller.ts @@ -1,11 +1,11 @@ import { + BadRequestException, Body, Controller, Get, Param, ParseIntPipe, Patch, - Put, Post, Query, Req, @@ -34,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 { @@ -382,11 +383,21 @@ export class PantriesController { } @Roles(Role.ADMIN) - @Put('/:pantryId/volunteers') + @Patch('/:pantryId/volunteers') async updatePantryVolunteers( @Param('pantryId', ParseIntPipe) pantryId: number, - @Body('volunteerIds') volunteerIds: number[], + @Body(new ValidationPipe()) + body: UpdatePantryVolunteersDto, ): Promise { - return this.pantriesService.updatePantryVolunteers(pantryId, volunteerIds); + 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 820e20a1..0fe7af21 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,166 @@ 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, { + addVolunteerIds: [7], + removeVolunteerIds: [], + }); + 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, { + addVolunteerIds: [], + removeVolunteerIds: [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, { + addVolunteerIds: [8], + removeVolunteerIds: [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('replaces volunteer set', async () => { - const williamId = Number(await getVolunteerId('william.m@volunteer.org')); - await service.updatePantryVolunteers(1, [williamId]); + it('silently ignores adding an already-assigned volunteer', async () => { + await service.updatePantryVolunteers(1, { + addVolunteerIds: [6], + removeVolunteerIds: [], + }); 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 === 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, { + addVolunteerIds: [], + removeVolunteerIds: [8], + }); + const pantryAfter = await testDataSource + .getRepository(Pantry) + .findOne({ where: { pantryId: 1 }, relations: ['volunteers'] }); + expect(pantryBefore?.volunteers).toEqual(pantryAfter?.volunteers); + }); + + it('throws BadRequestException for duplicate IDs in addVolunteerIds', async () => { + await expect( + service.updatePantryVolunteers(1, { + addVolunteerIds: [7, 7], + removeVolunteerIds: [], + }), + ).rejects.toThrow( + new BadRequestException('addVolunteerIds contains duplicate values'), + ); + }); + + it('throws BadRequestException for duplicate IDs in removeVolunteerIds', async () => { + await expect( + 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, { + addVolunteerIds: [6], + removeVolunteerIds: [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, { + addVolunteerIds: [6], + removeVolunteerIds: [], + }), ).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, { + 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 () => { - const adminId = Number(await getVolunteerId('john.smith@ssf.org')); await expect( - service.updatePantryVolunteers(1, [adminId]), - ).rejects.toThrow(BadRequestException); + 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 b7304150..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 { @@ -332,7 +333,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 +346,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 +420,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 +466,37 @@ export class PantriesService { async updatePantryVolunteers( pantryId: number, - volunteerIds: number[], + body: UpdatePantryVolunteersDto, ): Promise { + const { addVolunteerIds = [], removeVolunteerIds = [] } = body; validateId(pantryId, 'Pantry'); - volunteerIds.forEach((id) => validateId(id, 'Volunteer')); + + 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( + ', ', + )}`, + ); + } + + const allVolunteerIds = [...addSet, ...removeSet]; + allVolunteerIds.forEach((id) => validateId(id, 'Volunteer')); const pantry = await this.repo.findOne({ where: { pantryId }, @@ -479,25 +507,32 @@ export class PantriesService { throw new NotFoundException(`Pantry with ID ${pantryId} not found`); } - const users = await Promise.all( - volunteerIds.map((id) => this.usersService.findOne(id)), - ); - - if (users.length !== volunteerIds.length) { - throw new NotFoundException('One or more users not found'); - } + 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( - `Users ${nonVolunteers + `User(s) ${nonVolunteers .map((user) => user.id) .join(', ')} are not volunteers`, ); } - pantry.volunteers = users; + const currentVolunteers = pantry.volunteers ?? []; + const filteredVolunteers = currentVolunteers.filter( + (v) => !removeSet.has(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.spec.ts b/apps/backend/src/users/users.service.spec.ts index 5e13fe7b..b0d1f599 100644 --- a/apps/backend/src/users/users.service.spec.ts +++ b/apps/backend/src/users/users.service.spec.ts @@ -278,4 +278,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 f72d9fae..ab54e606 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', ); @@ -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; +}