From c1a1763d47a147d8f8356557576e6cb9fffe35b6 Mon Sep 17 00:00:00 2001 From: Justin Wang Date: Thu, 24 Sep 2026 22:05:54 -0400 Subject: [PATCH 01/10] Add POST /recommendations endpoint Lets foster coordinators recommend a Chameleon animal to a volunteer before a formal match exists. New recommendations default to isActive: true and the endpoint returns 200 with the created row. - CreateRecommendationDTO validates volunteerId and chameleonAnimalId as positive integers; the controller re-checks with validateId so the guard holds when the handler is called directly - 404 when the volunteer does not exist, via VolunteersService.existsById - validateId now rejects non-integers, which the "valid integer" rule needs - Registers RecommendationsModule on the app module Co-Authored-By: Claude Opus 5 --- apps/backend/src/app.module.ts | 2 + .../createRecommendation.dto.ts | 15 ++++ .../recommendations.controller.spec.ts | 73 ++++++++++++++++++- .../recommendations.controller.ts | 37 +++++++++- .../recommendations/recommendations.module.ts | 3 +- .../recommendations.service.spec.ts | 58 ++++++++++++++- .../recommendations.service.ts | 16 ++++ .../src/utils/validation.utils.spec.ts | 12 +++ apps/backend/src/utils/validation.utils.ts | 2 +- .../src/volunteers/volunteers.service.spec.ts | 24 +++++- .../src/volunteers/volunteers.service.ts | 19 +---- 11 files changed, 238 insertions(+), 23 deletions(-) create mode 100644 apps/backend/src/recommendations/createRecommendation.dto.ts diff --git a/apps/backend/src/app.module.ts b/apps/backend/src/app.module.ts index faede2748..cf4db389b 100644 --- a/apps/backend/src/app.module.ts +++ b/apps/backend/src/app.module.ts @@ -3,6 +3,7 @@ import { TypeOrmModule } from '@nestjs/typeorm'; import { ConfigModule, ConfigService } from '@nestjs/config'; import typeorm from './config/typeorm'; import { CognitoModule } from './aws/cognito/cognito.module'; +import { RecommendationsModule } from './recommendations/recommendations.module'; @Module({ imports: [ @@ -16,6 +17,7 @@ import { CognitoModule } from './aws/cognito/cognito.module'; configService.getOrThrow('typeorm'), }), CognitoModule, + RecommendationsModule, ], }) export class AppModule {} diff --git a/apps/backend/src/recommendations/createRecommendation.dto.ts b/apps/backend/src/recommendations/createRecommendation.dto.ts new file mode 100644 index 000000000..ef1126f27 --- /dev/null +++ b/apps/backend/src/recommendations/createRecommendation.dto.ts @@ -0,0 +1,15 @@ +import { IsInt, IsPositive } from 'class-validator'; + +/** + * Request body for POST /recommendations. A recommendation points a + * volunteer at an animal in Chameleon before any formal match exists. + */ +export class CreateRecommendationDTO { + @IsInt() + @IsPositive() + volunteerId!: number; + + @IsInt() + @IsPositive() + chameleonAnimalId!: number; +} diff --git a/apps/backend/src/recommendations/recommendations.controller.spec.ts b/apps/backend/src/recommendations/recommendations.controller.spec.ts index 748a2c3fc..e9c7a1bbe 100644 --- a/apps/backend/src/recommendations/recommendations.controller.spec.ts +++ b/apps/backend/src/recommendations/recommendations.controller.spec.ts @@ -1,17 +1,35 @@ import { Test, TestingModule } from '@nestjs/testing'; +import { BadRequestException, NotFoundException } from '@nestjs/common'; import { RecommendationsController } from './recommendations.controller'; import { RecommendationsService } from './recommendations.service'; +import { Recommendation } from './recommendations.entity'; +import { CreateRecommendationDTO } from './createRecommendation.dto'; +import { VolunteersService } from '../volunteers/volunteers.service'; describe('RecommendationsController', () => { let controller: RecommendationsController; + let recommendationsService: { create: jest.Mock }; + let volunteersService: { existsById: jest.Mock }; + + const body: CreateRecommendationDTO = { + volunteerId: 7, + chameleonAnimalId: 42, + }; beforeEach(async () => { + recommendationsService = { create: jest.fn() }; + volunteersService = { existsById: jest.fn() }; + const module: TestingModule = await Test.createTestingModule({ controllers: [RecommendationsController], providers: [ { provide: RecommendationsService, - useValue: {}, + useValue: recommendationsService, + }, + { + provide: VolunteersService, + useValue: volunteersService, }, ], }).compile(); @@ -24,4 +42,57 @@ describe('RecommendationsController', () => { it('should be defined', () => { expect(controller).toBeDefined(); }); + + describe('createRecommendation', () => { + it('returns the created recommendation when the volunteer exists', async () => { + const recommendation = { ...body, isActive: true } as Recommendation; + volunteersService.existsById.mockResolvedValue(true); + recommendationsService.create.mockResolvedValue(recommendation); + + const result = await controller.createRecommendation(body); + + expect(result).toBe(recommendation); + expect(volunteersService.existsById).toHaveBeenCalledWith(7); + expect(recommendationsService.create).toHaveBeenCalledWith(body); + }); + + it('throws NotFoundException when the volunteer does not exist', async () => { + volunteersService.existsById.mockResolvedValue(false); + + await expect(controller.createRecommendation(body)).rejects.toThrow( + NotFoundException, + ); + expect(recommendationsService.create).not.toHaveBeenCalled(); + }); + + it.each([ + ['volunteerId is missing', { chameleonAnimalId: 42 }], + ['chameleonAnimalId is missing', { volunteerId: 7 }], + ['the body is missing', undefined], + [ + 'volunteerId is not an integer', + { volunteerId: 1.5, chameleonAnimalId: 42 }, + ], + [ + 'chameleonAnimalId is not an integer', + { volunteerId: 7, chameleonAnimalId: 1.5 }, + ], + [ + 'volunteerId is not positive', + { volunteerId: 0, chameleonAnimalId: 42 }, + ], + [ + 'chameleonAnimalId is not positive', + { volunteerId: 7, chameleonAnimalId: -1 }, + ], + ])('throws BadRequestException when %s', async (_case, invalidBody) => { + await expect( + controller.createRecommendation( + invalidBody as unknown as CreateRecommendationDTO, + ), + ).rejects.toThrow(BadRequestException); + expect(volunteersService.existsById).not.toHaveBeenCalled(); + expect(recommendationsService.create).not.toHaveBeenCalled(); + }); + }); }); diff --git a/apps/backend/src/recommendations/recommendations.controller.ts b/apps/backend/src/recommendations/recommendations.controller.ts index ed58b1def..fd8ccbba2 100644 --- a/apps/backend/src/recommendations/recommendations.controller.ts +++ b/apps/backend/src/recommendations/recommendations.controller.ts @@ -1,7 +1,40 @@ -import { Controller } from '@nestjs/common'; +import { + Body, + Controller, + HttpCode, + HttpStatus, + NotFoundException, + Post, +} from '@nestjs/common'; import { RecommendationsService } from './recommendations.service'; +import { Recommendation } from './recommendations.entity'; +import { CreateRecommendationDTO } from './createRecommendation.dto'; +import { VolunteersService } from '../volunteers/volunteers.service'; +import { validateId } from '../utils/validation.utils'; +// @ApiTags('Recommendations') +// @ApiBearerAuth() @Controller('recommendations') export class RecommendationsController { - constructor(private recommendationsService: RecommendationsService) {} + constructor( + private recommendationsService: RecommendationsService, + private volunteersService: VolunteersService, + ) {} + + @Post() + @HttpCode(HttpStatus.OK) + async createRecommendation( + @Body() body: CreateRecommendationDTO, + ): Promise { + validateId(body?.volunteerId, 'volunteer'); + validateId(body?.chameleonAnimalId, 'Chameleon animal'); + + if (!(await this.volunteersService.existsById(body.volunteerId))) { + throw new NotFoundException( + `Volunteer with ID ${body.volunteerId} not found`, + ); + } + + return this.recommendationsService.create(body); + } } diff --git a/apps/backend/src/recommendations/recommendations.module.ts b/apps/backend/src/recommendations/recommendations.module.ts index af7c536b8..3dd562311 100644 --- a/apps/backend/src/recommendations/recommendations.module.ts +++ b/apps/backend/src/recommendations/recommendations.module.ts @@ -3,9 +3,10 @@ import { TypeOrmModule } from '@nestjs/typeorm'; import { Recommendation } from './recommendations.entity'; import { RecommendationsController } from './recommendations.controller'; import { RecommendationsService } from './recommendations.service'; +import { VolunteersModule } from '../volunteers/volunteers.module'; @Module({ - imports: [TypeOrmModule.forFeature([Recommendation])], + imports: [TypeOrmModule.forFeature([Recommendation]), VolunteersModule], controllers: [RecommendationsController], providers: [RecommendationsService], exports: [RecommendationsService], diff --git a/apps/backend/src/recommendations/recommendations.service.spec.ts b/apps/backend/src/recommendations/recommendations.service.spec.ts index 5424b2ecd..6afede206 100644 --- a/apps/backend/src/recommendations/recommendations.service.spec.ts +++ b/apps/backend/src/recommendations/recommendations.service.spec.ts @@ -5,14 +5,20 @@ import { Recommendation } from './recommendations.entity'; describe('RecommendationsService', () => { let service: RecommendationsService; + let repo: { create: jest.Mock; save: jest.Mock }; beforeEach(async () => { + repo = { + create: jest.fn((entity) => entity), + save: jest.fn((entity) => Promise.resolve(entity)), + }; + const module: TestingModule = await Test.createTestingModule({ providers: [ RecommendationsService, { provide: getRepositoryToken(Recommendation), - useValue: {}, + useValue: repo, }, ], }).compile(); @@ -23,4 +29,54 @@ describe('RecommendationsService', () => { it('should be defined', () => { expect(service).toBeDefined(); }); + + describe('create', () => { + it('saves a recommendation for the volunteer and animal', async () => { + const result = await service.create({ + volunteerId: 7, + chameleonAnimalId: 42, + }); + + expect(repo.create).toHaveBeenCalledWith({ + volunteerId: 7, + chameleonAnimalId: 42, + isActive: true, + }); + expect(repo.save).toHaveBeenCalledWith({ + volunteerId: 7, + chameleonAnimalId: 42, + isActive: true, + }); + expect(result).toEqual({ + volunteerId: 7, + chameleonAnimalId: 42, + isActive: true, + }); + }); + + it('defaults new recommendations to active', async () => { + const result = await service.create({ + volunteerId: 1, + chameleonAnimalId: 2, + }); + + expect(result.isActive).toBe(true); + }); + + it('returns the saved entity from the repository', async () => { + const saved = { + volunteerId: 7, + chameleonAnimalId: 42, + isActive: true, + } as Recommendation; + repo.save.mockResolvedValue(saved); + + const result = await service.create({ + volunteerId: 7, + chameleonAnimalId: 42, + }); + + expect(result).toBe(saved); + }); + }); }); diff --git a/apps/backend/src/recommendations/recommendations.service.ts b/apps/backend/src/recommendations/recommendations.service.ts index 72688a007..a54b124a0 100644 --- a/apps/backend/src/recommendations/recommendations.service.ts +++ b/apps/backend/src/recommendations/recommendations.service.ts @@ -2,6 +2,7 @@ import { Injectable } from '@nestjs/common'; import { InjectRepository } from '@nestjs/typeorm'; import { Repository } from 'typeorm'; import { Recommendation } from './recommendations.entity'; +import { CreateRecommendationDTO } from './createRecommendation.dto'; @Injectable() export class RecommendationsService { @@ -9,4 +10,19 @@ export class RecommendationsService { @InjectRepository(Recommendation) private repo: Repository, ) {} + + /** + * Creates a recommendation for the given volunteer and Chameleon animal. + * (volunteer_id, chameleon_animal_id) is the primary key, so recommending + * the same animal again reactivates the existing row rather than + * duplicating it. + */ + create({ + volunteerId, + chameleonAnimalId, + }: CreateRecommendationDTO): Promise { + return this.repo.save( + this.repo.create({ volunteerId, chameleonAnimalId, isActive: true }), + ); + } } diff --git a/apps/backend/src/utils/validation.utils.spec.ts b/apps/backend/src/utils/validation.utils.spec.ts index 6b3908403..9ac7b4b8d 100644 --- a/apps/backend/src/utils/validation.utils.spec.ts +++ b/apps/backend/src/utils/validation.utils.spec.ts @@ -11,4 +11,16 @@ describe('validateId', () => { new BadRequestException('Invalid User ID'), ); }); + + it('should throw BadRequestException for a non-integer ID', () => { + expect(() => validateId(1.5, 'User')).toThrow( + new BadRequestException('Invalid User ID'), + ); + }); + + it('should throw BadRequestException for a missing ID', () => { + expect(() => validateId(undefined as unknown as number, 'User')).toThrow( + new BadRequestException('Invalid User ID'), + ); + }); }); diff --git a/apps/backend/src/utils/validation.utils.ts b/apps/backend/src/utils/validation.utils.ts index 5c4f14a78..a02ac8d94 100644 --- a/apps/backend/src/utils/validation.utils.ts +++ b/apps/backend/src/utils/validation.utils.ts @@ -1,7 +1,7 @@ import { BadRequestException } from '@nestjs/common'; export function validateId(id: number, entityName: string): void { - if (!id || id < 1) { + if (!Number.isInteger(id) || id < 1) { throw new BadRequestException(`Invalid ${entityName} ID`); } } diff --git a/apps/backend/src/volunteers/volunteers.service.spec.ts b/apps/backend/src/volunteers/volunteers.service.spec.ts index fda546818..20330c8ad 100644 --- a/apps/backend/src/volunteers/volunteers.service.spec.ts +++ b/apps/backend/src/volunteers/volunteers.service.spec.ts @@ -5,14 +5,17 @@ import { FosterVolunteer } from './volunteers.entity'; describe('VolunteersService', () => { let service: VolunteersService; + let repo: { existsBy: jest.Mock }; beforeEach(async () => { + repo = { existsBy: jest.fn() }; + const module: TestingModule = await Test.createTestingModule({ providers: [ VolunteersService, { provide: getRepositoryToken(FosterVolunteer), - useValue: {}, + useValue: repo, }, ], }).compile(); @@ -23,4 +26,23 @@ describe('VolunteersService', () => { it('should be defined', () => { expect(service).toBeDefined(); }); + + describe('existsById', () => { + it('returns true when a volunteer with the id exists', async () => { + repo.existsBy.mockResolvedValue(true); + + const result = await service.existsById(7); + + expect(result).toBe(true); + expect(repo.existsBy).toHaveBeenCalledWith({ volunteerId: 7 }); + }); + + it('returns false when no volunteer with the id exists', async () => { + repo.existsBy.mockResolvedValue(false); + + const result = await service.existsById(7); + + expect(result).toBe(false); + }); + }); }); diff --git a/apps/backend/src/volunteers/volunteers.service.ts b/apps/backend/src/volunteers/volunteers.service.ts index 1c6f17a61..a44b9f7e0 100644 --- a/apps/backend/src/volunteers/volunteers.service.ts +++ b/apps/backend/src/volunteers/volunteers.service.ts @@ -10,20 +10,7 @@ export class VolunteersService { private repo: Repository, ) {} - // Example service functions - // find(email: string) { - // return this.repo.find({ where: { email } }); - // } - - // async update(id: number, attrs: Partial) { - // const user = await this.findOne(id); - - // if (!user) { - // throw new NotFoundException('User not found'); - // } - - // Object.assign(user, attrs); - - // return this.repo.save(user); - // } + existsById(id: number): Promise { + return this.repo.existsBy({ volunteerId: id }); + } } From fff3cbecaa549e3f58a8f3ec77cef9b0e8ee885b Mon Sep 17 00:00:00 2001 From: Justin Wang Date: Sat, 26 Sep 2026 11:07:23 -0400 Subject: [PATCH 02/10] Fix recommendations boot failure and duplicate-POST race Code review caught that registering RecommendationsModule made the app fail to boot. It pulls in VolunteersModule, the first forFeature module reachable from AppModule, so autoLoadEntities handed TypeORM FosterVolunteer without FosterCoordinator - which its assignedCoordinator relation targets - and metadata building threw before app.listen. No test caught it because none stand up the module graph. - VolunteersModule now imports CoordinatorsModule, so it is self-contained wherever it is registered rather than relying on the app module - recommendations.module.spec asserts the entity graph reachable from the module is closed under relations; it fails without the fix above - Service upserts on the composite key instead of save(), so two coordinators recommending the same animal at once cannot race into a primary key violation surfacing as a 500 - Pin the 200 status the ticket requires, so dropping @HttpCode fails Co-Authored-By: Claude Opus 5 --- .../recommendations.controller.spec.ts | 17 ++++- .../recommendations.module.spec.ts | 73 +++++++++++++++++++ .../recommendations.service.spec.ts | 72 +++++++++--------- .../recommendations.service.ts | 15 ++-- .../src/volunteers/volunteers.module.ts | 6 +- 5 files changed, 141 insertions(+), 42 deletions(-) create mode 100644 apps/backend/src/recommendations/recommendations.module.spec.ts diff --git a/apps/backend/src/recommendations/recommendations.controller.spec.ts b/apps/backend/src/recommendations/recommendations.controller.spec.ts index e9c7a1bbe..0d8d8a4eb 100644 --- a/apps/backend/src/recommendations/recommendations.controller.spec.ts +++ b/apps/backend/src/recommendations/recommendations.controller.spec.ts @@ -1,5 +1,10 @@ import { Test, TestingModule } from '@nestjs/testing'; -import { BadRequestException, NotFoundException } from '@nestjs/common'; +import { + BadRequestException, + HttpStatus, + NotFoundException, +} from '@nestjs/common'; +import { HTTP_CODE_METADATA } from '@nestjs/common/constants'; import { RecommendationsController } from './recommendations.controller'; import { RecommendationsService } from './recommendations.service'; import { Recommendation } from './recommendations.entity'; @@ -95,4 +100,14 @@ describe('RecommendationsController', () => { expect(recommendationsService.create).not.toHaveBeenCalled(); }); }); + + // The ticket specifies 200, not Nest's default 201 for POST. + it('responds 200 rather than the default 201 for a POST', () => { + expect( + Reflect.getMetadata( + HTTP_CODE_METADATA, + RecommendationsController.prototype.createRecommendation, + ), + ).toBe(HttpStatus.OK); + }); }); diff --git a/apps/backend/src/recommendations/recommendations.module.spec.ts b/apps/backend/src/recommendations/recommendations.module.spec.ts new file mode 100644 index 000000000..2d0286ade --- /dev/null +++ b/apps/backend/src/recommendations/recommendations.module.spec.ts @@ -0,0 +1,73 @@ +import { DynamicModule } from '@nestjs/common'; +import { getMetadataArgsStorage } from 'typeorm'; +import { RecommendationsModule } from './recommendations.module'; + +/** + * Collects the entity names every TypeOrmModule.forFeature() reachable from + * the given module registers. forFeature exposes each entity as a repository + * provider whose token is the string `${EntityName}Repository`. + */ +function registeredEntityNames( + mod: unknown, + seen = new Set(), + found = new Set(), +): Set { + if (!mod || seen.has(mod)) return found; + seen.add(mod); + + const dynamic = mod as Partial; + const staticModule = dynamic.module ?? mod; + + for (const provider of dynamic.providers ?? []) { + const token = (provider as { provide?: unknown }).provide; + if (typeof token === 'string' && token.endsWith('Repository')) { + found.add(token.slice(0, -'Repository'.length)); + } + } + + const imports = [ + ...((Reflect.getMetadata('imports', staticModule) as unknown[]) ?? []), + ...(dynamic.imports ?? []), + ]; + for (const imported of imports) registeredEntityNames(imported, seen, found); + + return found; +} + +/** Resolves the entity a relation points at, e.g. `() => FosterCoordinator`. */ +function relationTargetName(type: unknown): string | null { + const resolved = typeof type === 'function' ? type(undefined) : type; + if (typeof resolved === 'string') return resolved; + if (typeof resolved === 'function') return resolved.name; + return null; +} + +describe('RecommendationsModule', () => { + /** + * `autoLoadEntities` hands TypeORM only the entities that a reachable + * forFeature() registered. If one of them has a relation to an entity whose + * module was never imported, TypeORM throws "Entity metadata for X#y was not + * found" while building metadata and the app dies on boot. No other test + * catches that, because none of them stand the module graph up. + */ + it('registers every entity that its registered entities relate to', () => { + const registered = registeredEntityNames(RecommendationsModule); + expect(registered).toContain('Recommendation'); + + const missing = getMetadataArgsStorage() + .relations.filter( + (relation) => + typeof relation.target === 'function' && + registered.has(relation.target.name), + ) + .map((relation) => ({ + from: `${(relation.target as { name: string }).name}#${ + relation.propertyName + }`, + to: relationTargetName(relation.type), + })) + .filter(({ to }) => to !== null && !registered.has(to)); + + expect(missing).toEqual([]); + }); +}); diff --git a/apps/backend/src/recommendations/recommendations.service.spec.ts b/apps/backend/src/recommendations/recommendations.service.spec.ts index 6afede206..fab06703a 100644 --- a/apps/backend/src/recommendations/recommendations.service.spec.ts +++ b/apps/backend/src/recommendations/recommendations.service.spec.ts @@ -5,12 +5,18 @@ import { Recommendation } from './recommendations.entity'; describe('RecommendationsService', () => { let service: RecommendationsService; - let repo: { create: jest.Mock; save: jest.Mock }; + let repo: { upsert: jest.Mock; findOneByOrFail: jest.Mock }; + + const saved = { + volunteerId: 7, + chameleonAnimalId: 42, + isActive: true, + } as Recommendation; beforeEach(async () => { repo = { - create: jest.fn((entity) => entity), - save: jest.fn((entity) => Promise.resolve(entity)), + upsert: jest.fn().mockResolvedValue({ identifiers: [] }), + findOneByOrFail: jest.fn().mockResolvedValue(saved), }; const module: TestingModule = await Test.createTestingModule({ @@ -31,52 +37,48 @@ describe('RecommendationsService', () => { }); describe('create', () => { - it('saves a recommendation for the volunteer and animal', async () => { - const result = await service.create({ - volunteerId: 7, - chameleonAnimalId: 42, - }); + it('upserts the recommendation as active on the composite key', async () => { + await service.create({ volunteerId: 7, chameleonAnimalId: 42 }); - expect(repo.create).toHaveBeenCalledWith({ - volunteerId: 7, - chameleonAnimalId: 42, - isActive: true, - }); - expect(repo.save).toHaveBeenCalledWith({ - volunteerId: 7, - chameleonAnimalId: 42, - isActive: true, - }); - expect(result).toEqual({ - volunteerId: 7, - chameleonAnimalId: 42, - isActive: true, - }); + expect(repo.upsert).toHaveBeenCalledWith( + { volunteerId: 7, chameleonAnimalId: 42, isActive: true }, + ['volunteerId', 'chameleonAnimalId'], + ); }); it('defaults new recommendations to active', async () => { - const result = await service.create({ - volunteerId: 1, - chameleonAnimalId: 2, - }); + await service.create({ volunteerId: 7, chameleonAnimalId: 42 }); - expect(result.isActive).toBe(true); + const [values] = repo.upsert.mock.calls[0]; + expect(values.isActive).toBe(true); }); - it('returns the saved entity from the repository', async () => { - const saved = { + it('returns the persisted recommendation', async () => { + const result = await service.create({ volunteerId: 7, chameleonAnimalId: 42, - isActive: true, - } as Recommendation; - repo.save.mockResolvedValue(saved); + }); - const result = await service.create({ + expect(result).toBe(saved); + expect(repo.findOneByOrFail).toHaveBeenCalledWith({ volunteerId: 7, chameleonAnimalId: 42, }); + }); - expect(result).toBe(saved); + it('reads the row back only after the upsert has resolved', async () => { + const order: string[] = []; + repo.upsert.mockImplementation(async () => { + order.push('upsert'); + }); + repo.findOneByOrFail.mockImplementation(async () => { + order.push('read'); + return saved; + }); + + await service.create({ volunteerId: 7, chameleonAnimalId: 42 }); + + expect(order).toEqual(['upsert', 'read']); }); }); }); diff --git a/apps/backend/src/recommendations/recommendations.service.ts b/apps/backend/src/recommendations/recommendations.service.ts index a54b124a0..b920bd43c 100644 --- a/apps/backend/src/recommendations/recommendations.service.ts +++ b/apps/backend/src/recommendations/recommendations.service.ts @@ -15,14 +15,19 @@ export class RecommendationsService { * Creates a recommendation for the given volunteer and Chameleon animal. * (volunteer_id, chameleon_animal_id) is the primary key, so recommending * the same animal again reactivates the existing row rather than - * duplicating it. + * duplicating it. This upserts rather than save()-ing so that two + * coordinators recommending the same animal at once can't race into a + * primary key violation. */ - create({ + async create({ volunteerId, chameleonAnimalId, }: CreateRecommendationDTO): Promise { - return this.repo.save( - this.repo.create({ volunteerId, chameleonAnimalId, isActive: true }), - ); + await this.repo.upsert({ volunteerId, chameleonAnimalId, isActive: true }, [ + 'volunteerId', + 'chameleonAnimalId', + ]); + + return this.repo.findOneByOrFail({ volunteerId, chameleonAnimalId }); } } diff --git a/apps/backend/src/volunteers/volunteers.module.ts b/apps/backend/src/volunteers/volunteers.module.ts index d3fef67b4..da2094f82 100644 --- a/apps/backend/src/volunteers/volunteers.module.ts +++ b/apps/backend/src/volunteers/volunteers.module.ts @@ -3,9 +3,13 @@ import { TypeOrmModule } from '@nestjs/typeorm'; import { FosterVolunteer } from './volunteers.entity'; import { VolunteersController } from './volunteers.controller'; import { VolunteersService } from './volunteers.service'; +import { CoordinatorsModule } from '../coordinators/coordinators.module'; @Module({ - imports: [TypeOrmModule.forFeature([FosterVolunteer])], + // FosterVolunteer relates to FosterCoordinator, so CoordinatorsModule has to + // be registered too or autoLoadEntities leaves TypeORM unable to build that + // relation's metadata. + imports: [TypeOrmModule.forFeature([FosterVolunteer]), CoordinatorsModule], controllers: [VolunteersController], providers: [VolunteersService], exports: [VolunteersService], From 0919162eb5c81ce456717b07cb961bb9e5a63e0f Mon Sep 17 00:00:00 2001 From: Justin Wang Date: Sun, 27 Sep 2026 13:38:37 -0400 Subject: [PATCH 03/10] Remove recommendations module spec Co-Authored-By: Claude Opus 5.5 (1M context) --- .../recommendations.module.spec.ts | 73 ------------------- 1 file changed, 73 deletions(-) delete mode 100644 apps/backend/src/recommendations/recommendations.module.spec.ts diff --git a/apps/backend/src/recommendations/recommendations.module.spec.ts b/apps/backend/src/recommendations/recommendations.module.spec.ts deleted file mode 100644 index 2d0286ade..000000000 --- a/apps/backend/src/recommendations/recommendations.module.spec.ts +++ /dev/null @@ -1,73 +0,0 @@ -import { DynamicModule } from '@nestjs/common'; -import { getMetadataArgsStorage } from 'typeorm'; -import { RecommendationsModule } from './recommendations.module'; - -/** - * Collects the entity names every TypeOrmModule.forFeature() reachable from - * the given module registers. forFeature exposes each entity as a repository - * provider whose token is the string `${EntityName}Repository`. - */ -function registeredEntityNames( - mod: unknown, - seen = new Set(), - found = new Set(), -): Set { - if (!mod || seen.has(mod)) return found; - seen.add(mod); - - const dynamic = mod as Partial; - const staticModule = dynamic.module ?? mod; - - for (const provider of dynamic.providers ?? []) { - const token = (provider as { provide?: unknown }).provide; - if (typeof token === 'string' && token.endsWith('Repository')) { - found.add(token.slice(0, -'Repository'.length)); - } - } - - const imports = [ - ...((Reflect.getMetadata('imports', staticModule) as unknown[]) ?? []), - ...(dynamic.imports ?? []), - ]; - for (const imported of imports) registeredEntityNames(imported, seen, found); - - return found; -} - -/** Resolves the entity a relation points at, e.g. `() => FosterCoordinator`. */ -function relationTargetName(type: unknown): string | null { - const resolved = typeof type === 'function' ? type(undefined) : type; - if (typeof resolved === 'string') return resolved; - if (typeof resolved === 'function') return resolved.name; - return null; -} - -describe('RecommendationsModule', () => { - /** - * `autoLoadEntities` hands TypeORM only the entities that a reachable - * forFeature() registered. If one of them has a relation to an entity whose - * module was never imported, TypeORM throws "Entity metadata for X#y was not - * found" while building metadata and the app dies on boot. No other test - * catches that, because none of them stand the module graph up. - */ - it('registers every entity that its registered entities relate to', () => { - const registered = registeredEntityNames(RecommendationsModule); - expect(registered).toContain('Recommendation'); - - const missing = getMetadataArgsStorage() - .relations.filter( - (relation) => - typeof relation.target === 'function' && - registered.has(relation.target.name), - ) - .map((relation) => ({ - from: `${(relation.target as { name: string }).name}#${ - relation.propertyName - }`, - to: relationTargetName(relation.type), - })) - .filter(({ to }) => to !== null && !registered.has(to)); - - expect(missing).toEqual([]); - }); -}); From e5d347e9198af0cee1e42049aa1738c4f2c8067b Mon Sep 17 00:00:00 2001 From: Justin Wang Date: Sun, 27 Sep 2026 13:38:38 -0400 Subject: [PATCH 04/10] Remove explanatory comments Co-Authored-By: Claude Opus 5.5 (1M context) --- .../src/recommendations/createRecommendation.dto.ts | 4 ---- .../recommendations/recommendations.controller.spec.ts | 1 - .../src/recommendations/recommendations.service.ts | 8 -------- apps/backend/src/volunteers/volunteers.module.ts | 3 --- 4 files changed, 16 deletions(-) diff --git a/apps/backend/src/recommendations/createRecommendation.dto.ts b/apps/backend/src/recommendations/createRecommendation.dto.ts index ef1126f27..0976d2321 100644 --- a/apps/backend/src/recommendations/createRecommendation.dto.ts +++ b/apps/backend/src/recommendations/createRecommendation.dto.ts @@ -1,9 +1,5 @@ import { IsInt, IsPositive } from 'class-validator'; -/** - * Request body for POST /recommendations. A recommendation points a - * volunteer at an animal in Chameleon before any formal match exists. - */ export class CreateRecommendationDTO { @IsInt() @IsPositive() diff --git a/apps/backend/src/recommendations/recommendations.controller.spec.ts b/apps/backend/src/recommendations/recommendations.controller.spec.ts index 0d8d8a4eb..6652dc263 100644 --- a/apps/backend/src/recommendations/recommendations.controller.spec.ts +++ b/apps/backend/src/recommendations/recommendations.controller.spec.ts @@ -101,7 +101,6 @@ describe('RecommendationsController', () => { }); }); - // The ticket specifies 200, not Nest's default 201 for POST. it('responds 200 rather than the default 201 for a POST', () => { expect( Reflect.getMetadata( diff --git a/apps/backend/src/recommendations/recommendations.service.ts b/apps/backend/src/recommendations/recommendations.service.ts index b920bd43c..655e2fe95 100644 --- a/apps/backend/src/recommendations/recommendations.service.ts +++ b/apps/backend/src/recommendations/recommendations.service.ts @@ -11,14 +11,6 @@ export class RecommendationsService { private repo: Repository, ) {} - /** - * Creates a recommendation for the given volunteer and Chameleon animal. - * (volunteer_id, chameleon_animal_id) is the primary key, so recommending - * the same animal again reactivates the existing row rather than - * duplicating it. This upserts rather than save()-ing so that two - * coordinators recommending the same animal at once can't race into a - * primary key violation. - */ async create({ volunteerId, chameleonAnimalId, diff --git a/apps/backend/src/volunteers/volunteers.module.ts b/apps/backend/src/volunteers/volunteers.module.ts index da2094f82..5353e5f2f 100644 --- a/apps/backend/src/volunteers/volunteers.module.ts +++ b/apps/backend/src/volunteers/volunteers.module.ts @@ -6,9 +6,6 @@ import { VolunteersService } from './volunteers.service'; import { CoordinatorsModule } from '../coordinators/coordinators.module'; @Module({ - // FosterVolunteer relates to FosterCoordinator, so CoordinatorsModule has to - // be registered too or autoLoadEntities leaves TypeORM unable to build that - // relation's metadata. imports: [TypeOrmModule.forFeature([FosterVolunteer]), CoordinatorsModule], controllers: [VolunteersController], providers: [VolunteersService], From 1c7edd7be9d4b41df2568f085cbc45e7a139bf60 Mon Sep 17 00:00:00 2001 From: Justin Wang Date: Sun, 27 Sep 2026 13:38:44 -0400 Subject: [PATCH 05/10] Restore example service functions in VolunteersService Co-Authored-By: Claude Opus 5.5 (1M context) --- .../src/volunteers/volunteers.service.ts | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/apps/backend/src/volunteers/volunteers.service.ts b/apps/backend/src/volunteers/volunteers.service.ts index a44b9f7e0..ead6333f6 100644 --- a/apps/backend/src/volunteers/volunteers.service.ts +++ b/apps/backend/src/volunteers/volunteers.service.ts @@ -10,6 +10,23 @@ export class VolunteersService { private repo: Repository, ) {} + // Example service functions + // find(email: string) { + // return this.repo.find({ where: { email } }); + // } + + // async update(id: number, attrs: Partial) { + // const user = await this.findOne(id); + + // if (!user) { + // throw new NotFoundException('User not found'); + // } + + // Object.assign(user, attrs); + + // return this.repo.save(user); + // } + existsById(id: number): Promise { return this.repo.existsBy({ volunteerId: id }); } From 498dea5b57a0b44fd845e59c2f431b5376ceb1a9 Mon Sep 17 00:00:00 2001 From: Justin Wang Date: Sun, 27 Sep 2026 13:14:08 -0400 Subject: [PATCH 06/10] Respect @Column names in PluralNamingStrategy Co-Authored-By: Claude Opus 5.5 (1M context) --- .../strategies/plural-naming.strategy.spec.ts | 17 +++++++++++++++++ .../src/strategies/plural-naming.strategy.ts | 4 ++-- 2 files changed, 19 insertions(+), 2 deletions(-) create mode 100644 apps/backend/src/strategies/plural-naming.strategy.spec.ts diff --git a/apps/backend/src/strategies/plural-naming.strategy.spec.ts b/apps/backend/src/strategies/plural-naming.strategy.spec.ts new file mode 100644 index 000000000..bb6673169 --- /dev/null +++ b/apps/backend/src/strategies/plural-naming.strategy.spec.ts @@ -0,0 +1,17 @@ +import { PluralNamingStrategy } from './plural-naming.strategy'; + +describe('PluralNamingStrategy', () => { + const strategy = new PluralNamingStrategy(); + + describe('columnName', () => { + it('uses the name given in @Column', () => { + expect(strategy.columnName('volunteerId', 'volunteer_id')).toBe( + 'volunteer_id', + ); + }); + + it('falls back to the property name', () => { + expect(strategy.columnName('phone', '')).toBe('phone'); + }); + }); +}); diff --git a/apps/backend/src/strategies/plural-naming.strategy.ts b/apps/backend/src/strategies/plural-naming.strategy.ts index 1a7c0a156..c1acbf17e 100644 --- a/apps/backend/src/strategies/plural-naming.strategy.ts +++ b/apps/backend/src/strategies/plural-naming.strategy.ts @@ -8,8 +8,8 @@ export class PluralNamingStrategy return userSpecifiedName || targetName.toLowerCase() + 's'; // Pluralize the table name } - columnName(propertyName: string): string { - return propertyName; + columnName(propertyName: string, customName: string | undefined): string { + return customName || propertyName; } relationName(propertyName: string): string { From f3d9709933ec16bd7686003a14ba87e3fdd31b34 Mon Sep 17 00:00:00 2001 From: Justin Wang Date: Tue, 29 Sep 2026 17:45:44 -0400 Subject: [PATCH 07/10] Respond to review comments Co-Authored-By: Claude Opus 5.5 --- .../recommendations.controller.spec.ts | 53 ++++++++----------- .../recommendations.controller.ts | 31 +++++------ .../strategies/plural-naming.strategy.spec.ts | 17 ------ .../src/strategies/plural-naming.strategy.ts | 8 --- .../src/volunteers/volunteers.service.spec.ts | 34 +++++++----- .../src/volunteers/volunteers.service.ts | 31 +++++------ 6 files changed, 72 insertions(+), 102 deletions(-) delete mode 100644 apps/backend/src/strategies/plural-naming.strategy.spec.ts diff --git a/apps/backend/src/recommendations/recommendations.controller.spec.ts b/apps/backend/src/recommendations/recommendations.controller.spec.ts index 6652dc263..87605f0a3 100644 --- a/apps/backend/src/recommendations/recommendations.controller.spec.ts +++ b/apps/backend/src/recommendations/recommendations.controller.spec.ts @@ -1,10 +1,5 @@ import { Test, TestingModule } from '@nestjs/testing'; -import { - BadRequestException, - HttpStatus, - NotFoundException, -} from '@nestjs/common'; -import { HTTP_CODE_METADATA } from '@nestjs/common/constants'; +import { BadRequestException, NotFoundException } from '@nestjs/common'; import { RecommendationsController } from './recommendations.controller'; import { RecommendationsService } from './recommendations.service'; import { Recommendation } from './recommendations.entity'; @@ -14,16 +9,16 @@ import { VolunteersService } from '../volunteers/volunteers.service'; describe('RecommendationsController', () => { let controller: RecommendationsController; let recommendationsService: { create: jest.Mock }; - let volunteersService: { existsById: jest.Mock }; + let volunteersService: { findActiveOrFail: jest.Mock }; - const body: CreateRecommendationDTO = { + const body = { volunteerId: 7, chameleonAnimalId: 42, - }; + } as CreateRecommendationDTO; beforeEach(async () => { recommendationsService = { create: jest.fn() }; - volunteersService = { existsById: jest.fn() }; + volunteersService = { findActiveOrFail: jest.fn() }; const module: TestingModule = await Test.createTestingModule({ controllers: [RecommendationsController], @@ -49,31 +44,36 @@ describe('RecommendationsController', () => { }); describe('createRecommendation', () => { - it('returns the created recommendation when the volunteer exists', async () => { + it('returns the created recommendation when the volunteer is active', async () => { const recommendation = { ...body, isActive: true } as Recommendation; - volunteersService.existsById.mockResolvedValue(true); + volunteersService.findActiveOrFail.mockResolvedValue({}); recommendationsService.create.mockResolvedValue(recommendation); const result = await controller.createRecommendation(body); expect(result).toBe(recommendation); - expect(volunteersService.existsById).toHaveBeenCalledWith(7); + expect(volunteersService.findActiveOrFail).toHaveBeenCalledWith(7); expect(recommendationsService.create).toHaveBeenCalledWith(body); }); - it('throws NotFoundException when the volunteer does not exist', async () => { - volunteersService.existsById.mockResolvedValue(false); + it.each([ + ['does not exist', new NotFoundException()], + ['is not active', new BadRequestException()], + ])( + 'does not create a recommendation when the volunteer %s', + async (_case, error) => { + volunteersService.findActiveOrFail.mockRejectedValue(error); - await expect(controller.createRecommendation(body)).rejects.toThrow( - NotFoundException, - ); - expect(recommendationsService.create).not.toHaveBeenCalled(); - }); + await expect(controller.createRecommendation(body)).rejects.toThrow( + error, + ); + expect(recommendationsService.create).not.toHaveBeenCalled(); + }, + ); it.each([ ['volunteerId is missing', { chameleonAnimalId: 42 }], ['chameleonAnimalId is missing', { volunteerId: 7 }], - ['the body is missing', undefined], [ 'volunteerId is not an integer', { volunteerId: 1.5, chameleonAnimalId: 42 }, @@ -96,17 +96,8 @@ describe('RecommendationsController', () => { invalidBody as unknown as CreateRecommendationDTO, ), ).rejects.toThrow(BadRequestException); - expect(volunteersService.existsById).not.toHaveBeenCalled(); + expect(volunteersService.findActiveOrFail).not.toHaveBeenCalled(); expect(recommendationsService.create).not.toHaveBeenCalled(); }); }); - - it('responds 200 rather than the default 201 for a POST', () => { - expect( - Reflect.getMetadata( - HTTP_CODE_METADATA, - RecommendationsController.prototype.createRecommendation, - ), - ).toBe(HttpStatus.OK); - }); }); diff --git a/apps/backend/src/recommendations/recommendations.controller.ts b/apps/backend/src/recommendations/recommendations.controller.ts index fd8ccbba2..c66488e63 100644 --- a/apps/backend/src/recommendations/recommendations.controller.ts +++ b/apps/backend/src/recommendations/recommendations.controller.ts @@ -1,18 +1,12 @@ -import { - Body, - Controller, - HttpCode, - HttpStatus, - NotFoundException, - Post, -} from '@nestjs/common'; +import { Body, Controller, HttpStatus, Post } from '@nestjs/common'; +import { ApiOperation, ApiResponse, ApiTags } from '@nestjs/swagger'; import { RecommendationsService } from './recommendations.service'; import { Recommendation } from './recommendations.entity'; import { CreateRecommendationDTO } from './createRecommendation.dto'; import { VolunteersService } from '../volunteers/volunteers.service'; import { validateId } from '../utils/validation.utils'; -// @ApiTags('Recommendations') +@ApiTags('Recommendations') // @ApiBearerAuth() @Controller('recommendations') export class RecommendationsController { @@ -22,18 +16,21 @@ export class RecommendationsController { ) {} @Post() - @HttpCode(HttpStatus.OK) + @ApiOperation({ + summary: 'Recommend a Chameleon Animal to an active Volunteer', + }) + @ApiResponse({ + status: HttpStatus.CREATED, + description: 'The created or reactivated recommendation', + type: Recommendation, + }) async createRecommendation( @Body() body: CreateRecommendationDTO, ): Promise { - validateId(body?.volunteerId, 'volunteer'); - validateId(body?.chameleonAnimalId, 'Chameleon animal'); + validateId(body.volunteerId, 'Volunteer'); + validateId(body.chameleonAnimalId, 'Chameleon Animal'); - if (!(await this.volunteersService.existsById(body.volunteerId))) { - throw new NotFoundException( - `Volunteer with ID ${body.volunteerId} not found`, - ); - } + await this.volunteersService.findActiveOrFail(body.volunteerId); return this.recommendationsService.create(body); } diff --git a/apps/backend/src/strategies/plural-naming.strategy.spec.ts b/apps/backend/src/strategies/plural-naming.strategy.spec.ts deleted file mode 100644 index bb6673169..000000000 --- a/apps/backend/src/strategies/plural-naming.strategy.spec.ts +++ /dev/null @@ -1,17 +0,0 @@ -import { PluralNamingStrategy } from './plural-naming.strategy'; - -describe('PluralNamingStrategy', () => { - const strategy = new PluralNamingStrategy(); - - describe('columnName', () => { - it('uses the name given in @Column', () => { - expect(strategy.columnName('volunteerId', 'volunteer_id')).toBe( - 'volunteer_id', - ); - }); - - it('falls back to the property name', () => { - expect(strategy.columnName('phone', '')).toBe('phone'); - }); - }); -}); diff --git a/apps/backend/src/strategies/plural-naming.strategy.ts b/apps/backend/src/strategies/plural-naming.strategy.ts index c1acbf17e..03599d836 100644 --- a/apps/backend/src/strategies/plural-naming.strategy.ts +++ b/apps/backend/src/strategies/plural-naming.strategy.ts @@ -7,12 +7,4 @@ export class PluralNamingStrategy tableName(targetName: string, userSpecifiedName: string | undefined): string { return userSpecifiedName || targetName.toLowerCase() + 's'; // Pluralize the table name } - - columnName(propertyName: string, customName: string | undefined): string { - return customName || propertyName; - } - - relationName(propertyName: string): string { - return propertyName; - } } diff --git a/apps/backend/src/volunteers/volunteers.service.spec.ts b/apps/backend/src/volunteers/volunteers.service.spec.ts index 20330c8ad..b769d7e1f 100644 --- a/apps/backend/src/volunteers/volunteers.service.spec.ts +++ b/apps/backend/src/volunteers/volunteers.service.spec.ts @@ -1,14 +1,15 @@ import { Test, TestingModule } from '@nestjs/testing'; +import { BadRequestException, NotFoundException } from '@nestjs/common'; import { getRepositoryToken } from '@nestjs/typeorm'; import { VolunteersService } from './volunteers.service'; import { FosterVolunteer } from './volunteers.entity'; describe('VolunteersService', () => { let service: VolunteersService; - let repo: { existsBy: jest.Mock }; + let repo: { findOneBy: jest.Mock }; beforeEach(async () => { - repo = { existsBy: jest.fn() }; + repo = { findOneBy: jest.fn() }; const module: TestingModule = await Test.createTestingModule({ providers: [ @@ -27,22 +28,31 @@ describe('VolunteersService', () => { expect(service).toBeDefined(); }); - describe('existsById', () => { - it('returns true when a volunteer with the id exists', async () => { - repo.existsBy.mockResolvedValue(true); + describe('findActiveOrFail', () => { + it('returns the volunteer when they are active', async () => { + const volunteer = { volunteerId: 7, active: true } as FosterVolunteer; + repo.findOneBy.mockResolvedValue(volunteer); - const result = await service.existsById(7); + const result = await service.findActiveOrFail(7); - expect(result).toBe(true); - expect(repo.existsBy).toHaveBeenCalledWith({ volunteerId: 7 }); + expect(result).toBe(volunteer); + expect(repo.findOneBy).toHaveBeenCalledWith({ volunteerId: 7 }); }); - it('returns false when no volunteer with the id exists', async () => { - repo.existsBy.mockResolvedValue(false); + it('throws NotFoundException when no volunteer with the id exists', async () => { + repo.findOneBy.mockResolvedValue(null); - const result = await service.existsById(7); + await expect(service.findActiveOrFail(7)).rejects.toThrow( + NotFoundException, + ); + }); + + it('throws BadRequestException when the volunteer is not active', async () => { + repo.findOneBy.mockResolvedValue({ volunteerId: 7, active: false }); - expect(result).toBe(false); + await expect(service.findActiveOrFail(7)).rejects.toThrow( + BadRequestException, + ); }); }); }); diff --git a/apps/backend/src/volunteers/volunteers.service.ts b/apps/backend/src/volunteers/volunteers.service.ts index ead6333f6..51a7440f6 100644 --- a/apps/backend/src/volunteers/volunteers.service.ts +++ b/apps/backend/src/volunteers/volunteers.service.ts @@ -1,4 +1,8 @@ -import { Injectable } from '@nestjs/common'; +import { + BadRequestException, + Injectable, + NotFoundException, +} from '@nestjs/common'; import { InjectRepository } from '@nestjs/typeorm'; import { Repository } from 'typeorm'; import { FosterVolunteer } from './volunteers.entity'; @@ -10,24 +14,17 @@ export class VolunteersService { private repo: Repository, ) {} - // Example service functions - // find(email: string) { - // return this.repo.find({ where: { email } }); - // } + async findActiveOrFail(id: number): Promise { + const volunteer = await this.repo.findOneBy({ volunteerId: id }); - // async update(id: number, attrs: Partial) { - // const user = await this.findOne(id); + if (!volunteer) { + throw new NotFoundException(`Volunteer with ID ${id} not found`); + } - // if (!user) { - // throw new NotFoundException('User not found'); - // } + if (!volunteer.active) { + throw new BadRequestException(`Volunteer with ID ${id} is not active`); + } - // Object.assign(user, attrs); - - // return this.repo.save(user); - // } - - existsById(id: number): Promise { - return this.repo.existsBy({ volunteerId: id }); + return volunteer; } } From a6b7aa1347c65e24d27231ba175c1e186ab5bdea Mon Sep 17 00:00:00 2001 From: Justin Wang Date: Sat, 3 Oct 2026 10:41:25 -0400 Subject: [PATCH 08/10] Address second round of PR review comments - Document RecommendationsService.create and VolunteersService.findActiveOrFail with summary, context, @param, @returns and @throws - Assert findOneBy is called in the not-found and inactive findActiveOrFail tests - Drop controller tests for malformed IDs, which the DTO's class-validator decorators already cover - Replace the redundant "defaults to active" service test with one for reactivating an inactive recommendation Co-Authored-By: Claude Opus 5.5 --- .../recommendations.controller.spec.ts | 29 ------------------- .../recommendations.service.spec.ts | 16 +++++++--- .../recommendations.service.ts | 13 +++++++++ .../src/volunteers/volunteers.service.spec.ts | 2 ++ .../src/volunteers/volunteers.service.ts | 10 +++++++ 5 files changed, 37 insertions(+), 33 deletions(-) diff --git a/apps/backend/src/recommendations/recommendations.controller.spec.ts b/apps/backend/src/recommendations/recommendations.controller.spec.ts index 87605f0a3..c4cd7ab77 100644 --- a/apps/backend/src/recommendations/recommendations.controller.spec.ts +++ b/apps/backend/src/recommendations/recommendations.controller.spec.ts @@ -70,34 +70,5 @@ describe('RecommendationsController', () => { expect(recommendationsService.create).not.toHaveBeenCalled(); }, ); - - it.each([ - ['volunteerId is missing', { chameleonAnimalId: 42 }], - ['chameleonAnimalId is missing', { volunteerId: 7 }], - [ - 'volunteerId is not an integer', - { volunteerId: 1.5, chameleonAnimalId: 42 }, - ], - [ - 'chameleonAnimalId is not an integer', - { volunteerId: 7, chameleonAnimalId: 1.5 }, - ], - [ - 'volunteerId is not positive', - { volunteerId: 0, chameleonAnimalId: 42 }, - ], - [ - 'chameleonAnimalId is not positive', - { volunteerId: 7, chameleonAnimalId: -1 }, - ], - ])('throws BadRequestException when %s', async (_case, invalidBody) => { - await expect( - controller.createRecommendation( - invalidBody as unknown as CreateRecommendationDTO, - ), - ).rejects.toThrow(BadRequestException); - expect(volunteersService.findActiveOrFail).not.toHaveBeenCalled(); - expect(recommendationsService.create).not.toHaveBeenCalled(); - }); }); }); diff --git a/apps/backend/src/recommendations/recommendations.service.spec.ts b/apps/backend/src/recommendations/recommendations.service.spec.ts index fab06703a..22da5603d 100644 --- a/apps/backend/src/recommendations/recommendations.service.spec.ts +++ b/apps/backend/src/recommendations/recommendations.service.spec.ts @@ -46,11 +46,19 @@ describe('RecommendationsService', () => { ); }); - it('defaults new recommendations to active', async () => { - await service.create({ volunteerId: 7, chameleonAnimalId: 42 }); + it('reactivates an inactive recommendation for the same pair', async () => { + // The existing (7, 42) row is inactive. Upserting on the composite key + // overwrites it with isActive: true rather than inserting a duplicate. + const result = await service.create({ + volunteerId: 7, + chameleonAnimalId: 42, + }); - const [values] = repo.upsert.mock.calls[0]; - expect(values.isActive).toBe(true); + expect(repo.upsert).toHaveBeenCalledWith( + expect.objectContaining({ isActive: true }), + ['volunteerId', 'chameleonAnimalId'], + ); + expect(result.isActive).toBe(true); }); it('returns the persisted recommendation', async () => { diff --git a/apps/backend/src/recommendations/recommendations.service.ts b/apps/backend/src/recommendations/recommendations.service.ts index 655e2fe95..e3eaba93e 100644 --- a/apps/backend/src/recommendations/recommendations.service.ts +++ b/apps/backend/src/recommendations/recommendations.service.ts @@ -11,6 +11,19 @@ export class RecommendationsService { private repo: Repository, ) {} + /** + * Recommends a Chameleon Animal to a Volunteer. + * + * (volunteerId, chameleonAnimalId) is the primary key, so recommending the + * same animal again reactivates the existing row instead of duplicating it. + * This upserts rather than save()-ing so that two concurrent requests for + * the same pair can't race into a primary key violation. Does not check + * that the Volunteer exists or is active; callers are expected to. + * + * @param dto - The Volunteer and Chameleon Animal IDs to link. + * @returns The active recommendation as persisted. + * @throws {EntityNotFoundError} If the row can't be read back after the upsert. + */ async create({ volunteerId, chameleonAnimalId, diff --git a/apps/backend/src/volunteers/volunteers.service.spec.ts b/apps/backend/src/volunteers/volunteers.service.spec.ts index b769d7e1f..dacbe6a45 100644 --- a/apps/backend/src/volunteers/volunteers.service.spec.ts +++ b/apps/backend/src/volunteers/volunteers.service.spec.ts @@ -45,6 +45,7 @@ describe('VolunteersService', () => { await expect(service.findActiveOrFail(7)).rejects.toThrow( NotFoundException, ); + expect(repo.findOneBy).toHaveBeenCalledWith({ volunteerId: 7 }); }); it('throws BadRequestException when the volunteer is not active', async () => { @@ -53,6 +54,7 @@ describe('VolunteersService', () => { await expect(service.findActiveOrFail(7)).rejects.toThrow( BadRequestException, ); + expect(repo.findOneBy).toHaveBeenCalledWith({ volunteerId: 7 }); }); }); }); diff --git a/apps/backend/src/volunteers/volunteers.service.ts b/apps/backend/src/volunteers/volunteers.service.ts index 51a7440f6..7cdb79660 100644 --- a/apps/backend/src/volunteers/volunteers.service.ts +++ b/apps/backend/src/volunteers/volunteers.service.ts @@ -14,6 +14,16 @@ export class VolunteersService { private repo: Repository, ) {} + /** + * Fetches a Volunteer by ID, requiring that they are active. + * + * Returns the Volunteer so callers can reuse it instead of fetching again. + * + * @param id - The Volunteer's ID. + * @returns The active Volunteer. + * @throws {NotFoundException} If no Volunteer with the ID exists. + * @throws {BadRequestException} If the Volunteer is not active. + */ async findActiveOrFail(id: number): Promise { const volunteer = await this.repo.findOneBy({ volunteerId: id }); From 5f15894b9a77e36f6f6b909c9967825da1621f43 Mon Sep 17 00:00:00 2001 From: Justin Wang Date: Sat, 3 Oct 2026 10:44:47 -0400 Subject: [PATCH 09/10] remove comment --- .../backend/src/recommendations/recommendations.service.spec.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/apps/backend/src/recommendations/recommendations.service.spec.ts b/apps/backend/src/recommendations/recommendations.service.spec.ts index 22da5603d..d60296250 100644 --- a/apps/backend/src/recommendations/recommendations.service.spec.ts +++ b/apps/backend/src/recommendations/recommendations.service.spec.ts @@ -47,8 +47,6 @@ describe('RecommendationsService', () => { }); it('reactivates an inactive recommendation for the same pair', async () => { - // The existing (7, 42) row is inactive. Upserting on the composite key - // overwrites it with isActive: true rather than inserting a duplicate. const result = await service.create({ volunteerId: 7, chameleonAnimalId: 42, From 901de524f6fed0156446e19b39d7c4cd64f76921 Mon Sep 17 00:00:00 2001 From: Justin Wang Date: Sun, 4 Oct 2026 10:43:00 -0400 Subject: [PATCH 10/10] Address third round of PR review comments - Move CreateRecommendationDTO into recommendations/dto/ and rename to kebab-case - Collapse volunteer-missing/inactive controller tests into one Co-Authored-By: Claude Opus 5.5 --- .../create-recommendation.dto.ts} | 0 .../recommendations.controller.spec.ts | 25 ++++++++----------- .../recommendations.controller.ts | 2 +- .../recommendations.service.ts | 2 +- 4 files changed, 12 insertions(+), 17 deletions(-) rename apps/backend/src/recommendations/{createRecommendation.dto.ts => dto/create-recommendation.dto.ts} (100%) diff --git a/apps/backend/src/recommendations/createRecommendation.dto.ts b/apps/backend/src/recommendations/dto/create-recommendation.dto.ts similarity index 100% rename from apps/backend/src/recommendations/createRecommendation.dto.ts rename to apps/backend/src/recommendations/dto/create-recommendation.dto.ts diff --git a/apps/backend/src/recommendations/recommendations.controller.spec.ts b/apps/backend/src/recommendations/recommendations.controller.spec.ts index c4cd7ab77..5149a8e40 100644 --- a/apps/backend/src/recommendations/recommendations.controller.spec.ts +++ b/apps/backend/src/recommendations/recommendations.controller.spec.ts @@ -1,9 +1,9 @@ import { Test, TestingModule } from '@nestjs/testing'; -import { BadRequestException, NotFoundException } from '@nestjs/common'; +import { NotFoundException } from '@nestjs/common'; import { RecommendationsController } from './recommendations.controller'; import { RecommendationsService } from './recommendations.service'; import { Recommendation } from './recommendations.entity'; -import { CreateRecommendationDTO } from './createRecommendation.dto'; +import { CreateRecommendationDTO } from './dto/create-recommendation.dto'; import { VolunteersService } from '../volunteers/volunteers.service'; describe('RecommendationsController', () => { @@ -56,19 +56,14 @@ describe('RecommendationsController', () => { expect(recommendationsService.create).toHaveBeenCalledWith(body); }); - it.each([ - ['does not exist', new NotFoundException()], - ['is not active', new BadRequestException()], - ])( - 'does not create a recommendation when the volunteer %s', - async (_case, error) => { - volunteersService.findActiveOrFail.mockRejectedValue(error); + it('does not create a recommendation when the volunteer does not exist or is not active', async () => { + const error = new NotFoundException(); + volunteersService.findActiveOrFail.mockRejectedValue(error); - await expect(controller.createRecommendation(body)).rejects.toThrow( - error, - ); - expect(recommendationsService.create).not.toHaveBeenCalled(); - }, - ); + await expect(controller.createRecommendation(body)).rejects.toThrow( + error, + ); + expect(recommendationsService.create).not.toHaveBeenCalled(); + }); }); }); diff --git a/apps/backend/src/recommendations/recommendations.controller.ts b/apps/backend/src/recommendations/recommendations.controller.ts index c66488e63..3ddf5ec42 100644 --- a/apps/backend/src/recommendations/recommendations.controller.ts +++ b/apps/backend/src/recommendations/recommendations.controller.ts @@ -2,7 +2,7 @@ import { Body, Controller, HttpStatus, Post } from '@nestjs/common'; import { ApiOperation, ApiResponse, ApiTags } from '@nestjs/swagger'; import { RecommendationsService } from './recommendations.service'; import { Recommendation } from './recommendations.entity'; -import { CreateRecommendationDTO } from './createRecommendation.dto'; +import { CreateRecommendationDTO } from './dto/create-recommendation.dto'; import { VolunteersService } from '../volunteers/volunteers.service'; import { validateId } from '../utils/validation.utils'; diff --git a/apps/backend/src/recommendations/recommendations.service.ts b/apps/backend/src/recommendations/recommendations.service.ts index e3eaba93e..7dfa3be98 100644 --- a/apps/backend/src/recommendations/recommendations.service.ts +++ b/apps/backend/src/recommendations/recommendations.service.ts @@ -2,7 +2,7 @@ import { Injectable } from '@nestjs/common'; import { InjectRepository } from '@nestjs/typeorm'; import { Repository } from 'typeorm'; import { Recommendation } from './recommendations.entity'; -import { CreateRecommendationDTO } from './createRecommendation.dto'; +import { CreateRecommendationDTO } from './dto/create-recommendation.dto'; @Injectable() export class RecommendationsService {