From 99b99ca65632816a49b2369b3e1920a95b6e793c Mon Sep 17 00:00:00 2001 From: psachmann Date: Tue, 17 Oct 2023 16:56:31 +0200 Subject: [PATCH] EW-561 code coverage, better searching and pagination --- src/global.d.ts | 4 + .../person/api/person.controller.spec.ts | 21 +++-- src/modules/person/api/person.controller.ts | 4 +- src/modules/person/api/person.uc.spec.ts | 29 ++++--- src/modules/person/api/person.uc.ts | 2 +- .../person/domain/person.service.spec.ts | 30 ++++--- src/modules/person/domain/person.service.ts | 4 +- .../person.repo.integration-spec.ts | 62 +++++++++------ src/modules/person/persistence/person.repo.ts | 6 +- .../person.scope.integration-spec.ts | 61 ++++++++++++++ .../person/persistence/person.scope.ts | 8 +- ...bal-pagination-headers.interceptor.spec.ts | 79 +++++++++++++++++++ .../global-pagination-headers.interceptor.ts | 2 +- src/shared/persistence/scope-base.ts | 4 +- 14 files changed, 248 insertions(+), 68 deletions(-) create mode 100644 src/modules/person/persistence/person.scope.integration-spec.ts create mode 100644 src/shared/paging/global-pagination-headers.interceptor.spec.ts diff --git a/src/global.d.ts b/src/global.d.ts index eafe2804e..cba2e28a0 100644 --- a/src/global.d.ts +++ b/src/global.d.ts @@ -5,3 +5,7 @@ declare type Result = { ok: true; value: T } | { ok: false; error: declare type Persisted = WasPersisted extends true ? T : Option; declare type Counted = [T[], number]; + +declare type Findable = { + [P in keyof T]?: T[P] extends string ? string | RegExp : T[P]; +}; diff --git a/src/modules/person/api/person.controller.spec.ts b/src/modules/person/api/person.controller.spec.ts index 2b1fb9283..41b90b8a2 100644 --- a/src/modules/person/api/person.controller.spec.ts +++ b/src/modules/person/api/person.controller.spec.ts @@ -17,6 +17,7 @@ import { PersonenkontextUc } from './personenkontext.uc.js'; import { CreatePersonenkontextBodyParams } from './create-personenkontext.body.params.js'; import { CreatedPersonenkontextDto } from './created-personenkontext.dto.js'; import { Jahrgangsstufe, Personenstatus, Rolle } from '../domain/personenkontext.enums.js'; +import { PagedResponse } from '../../../shared/paging/index.js'; describe('PersonController', () => { let module: TestingModule; @@ -153,7 +154,6 @@ describe('PersonController', () => { lokalisierung: '', vertrauensstufe: TrustLevel.TRUSTED, }; - const person2: PersonResponse = { id: faker.string.uuid(), name: { @@ -167,20 +167,27 @@ describe('PersonController', () => { lokalisierung: '', vertrauensstufe: TrustLevel.TRUSTED, }; - const mockPersondatensatz1: PersonenDatensatz = { person: person1, }; const mockPersondatensatz2: PersonenDatensatz = { person: person2, }; - const mockPersondatensatz: PersonenDatensatz[] = [mockPersondatensatz1, mockPersondatensatz2]; + const mockPersondatensatz: PagedResponse = new PagedResponse({ + offset: 0, + limit: 10, + total: 2, + items: [mockPersondatensatz1, mockPersondatensatz2], + }); + personUcMock.findAll.mockResolvedValue(mockPersondatensatz); - const result: PersonenDatensatz[] = await personController.findPersons(queryParams); + + const result: PagedResponse = await personController.findPersons(queryParams); + expect(personUcMock.findAll).toHaveBeenCalledTimes(1); - expect(result.at(0)?.person.referrer).toEqual(queryParams.referrer); - expect(result.at(0)?.person.name.vorname).toEqual(queryParams.vorname); - expect(result.at(0)?.person.name.familienname).toEqual(queryParams.familienname); + expect(result.items.at(0)?.person.referrer).toEqual(queryParams.referrer); + expect(result.items.at(0)?.person.name.vorname).toEqual(queryParams.vorname); + expect(result.items.at(0)?.person.name.familienname).toEqual(queryParams.familienname); expect(result).toEqual(mockPersondatensatz); }); }); diff --git a/src/modules/person/api/person.controller.ts b/src/modules/person/api/person.controller.ts index 7892a152c..654e5dea3 100644 --- a/src/modules/person/api/person.controller.ts +++ b/src/modules/person/api/person.controller.ts @@ -86,7 +86,7 @@ export class PersonController { } @Get() - @ApiOkResponse({ description: 'The persons were successfully returned.' }) + @ApiOkResponse({ description: 'The persons were successfully returned.', type: Array }) @ApiUnauthorizedResponse({ description: 'Not authorized to get persons.' }) @ApiForbiddenResponse({ description: 'Insufficient permissions to get persons.' }) @ApiInternalServerErrorResponse({ description: 'Internal server error while getting all persons.' }) @@ -96,7 +96,7 @@ export class PersonController { PersonenQueryParams, FindPersonDatensatzDTO, ); - const persons: Paged = await this.uc.findAll(personDatensatzDTO); + const persons: Paged = await this.personUc.findAll(personDatensatzDTO); const response: PagedResponse = new PagedResponse(persons); return response; diff --git a/src/modules/person/api/person.uc.spec.ts b/src/modules/person/api/person.uc.spec.ts index dda267ad6..6eb332b32 100644 --- a/src/modules/person/api/person.uc.spec.ts +++ b/src/modules/person/api/person.uc.spec.ts @@ -11,6 +11,7 @@ import { faker } from '@faker-js/faker'; import { PersonDo } from '../domain/person.do.js'; import { PersonenDatensatz } from './personendatensatz.js'; import { KeycloakUserService } from '../../keycloak-administration/index.js'; +import { Paged } from '../../../shared/paging/index.js'; describe('PersonUc', () => { let module: TestingModule; @@ -145,21 +146,29 @@ describe('PersonUc', () => { it('should find all persons that match with query param', async () => { const firstPerson: PersonDo = DoFactory.createPerson(true); const secondPerson: PersonDo = DoFactory.createPerson(true); - const persons: PersonDo[] = [firstPerson, secondPerson]; + const persons: Paged> = { + offset: 0, + limit: 10, + total: 2, + items: [firstPerson, secondPerson], + }; + personServiceMock.findAllPersons.mockResolvedValue(persons); - const result: PersonenDatensatz[] = await personUc.findAll(personDTO); - expect(result).toHaveLength(2); - expect(result.at(0)?.person.name.vorname).toEqual(firstPerson.firstName); - expect(result.at(0)?.person.name.familienname).toEqual(firstPerson.lastName); - expect(result.at(1)?.person.name.vorname).toEqual(secondPerson.firstName); - expect(result.at(1)?.person.name.familienname).toEqual(secondPerson.lastName); + + const result: Paged = await personUc.findAll(personDTO); + + expect(result.items).toHaveLength(2); + expect(result.items.at(0)?.person.name.vorname).toEqual(firstPerson.firstName); + expect(result.items.at(0)?.person.name.familienname).toEqual(firstPerson.lastName); + expect(result.items.at(1)?.person.name.vorname).toEqual(secondPerson.firstName); + expect(result.items.at(1)?.person.name.familienname).toEqual(secondPerson.lastName); }); it('should return an empty array when no matching persons are found', async () => { - const emptyResult: PersonDo[] = []; + const emptyResult: Paged> = { offset: 0, limit: 0, total: 0, items: [] }; personServiceMock.findAllPersons.mockResolvedValue(emptyResult); - const result: PersonenDatensatz[] = await personUc.findAll(personDTO); - expect(result).toEqual([]); + const result: Paged = await personUc.findAll(personDTO); + expect(result.items).toEqual([]); }); }); }); diff --git a/src/modules/person/api/person.uc.ts b/src/modules/person/api/person.uc.ts index facd351e7..2640e8d08 100644 --- a/src/modules/person/api/person.uc.ts +++ b/src/modules/person/api/person.uc.ts @@ -55,9 +55,9 @@ export class PersonUc { public async findAll(personDto: FindPersonDatensatzDto): Promise> { const personDo: PersonDo = this.mapper.map(personDto, FindPersonDatensatzDto, PersonDo); const result: Paged> = await this.personService.findAllPersons( + personDo, personDto.offset, personDto.limit, - personDo, ); if (result.total === 0) { diff --git a/src/modules/person/domain/person.service.spec.ts b/src/modules/person/domain/person.service.spec.ts index ac280a419..752a289c1 100644 --- a/src/modules/person/domain/person.service.spec.ts +++ b/src/modules/person/domain/person.service.spec.ts @@ -8,6 +8,7 @@ import { DoFactory } from '../../../../test/utils/do-factory.js'; import { PersonRepo } from '../persistence/person.repo.js'; import { PersonDo } from './person.do.js'; import { PersonService } from './person.service.js'; +import { Paged } from '../../../shared/paging/index.js'; describe('PersonService', () => { let module: TestingModule; @@ -123,26 +124,29 @@ describe('PersonService', () => { describe('findAllPersons', () => { it('should get all persons that match', async () => { - const firstPerson: PersonDo = DoFactory.createPerson(false); - const secondPerson: PersonDo = DoFactory.createPerson(false); - const persons: PersonDo[] = [ - firstPerson as unknown as PersonDo, - secondPerson as unknown as PersonDo, - ]; - personRepoMock.findAll.mockResolvedValue(persons); + const firstPerson: PersonDo = DoFactory.createPerson(true); + const secondPerson: PersonDo = DoFactory.createPerson(true); + const persons: Counted> = [[firstPerson, secondPerson], 2]; + + personRepoMock.findBy.mockResolvedValue(persons); mapperMock.map.mockReturnValue(persons as unknown as Dictionary); + const personDoWithQueryParam: PersonDo = DoFactory.createPerson(false); - const result: PersonDo[] = await personService.findAllPersons(personDoWithQueryParam); - expect(result).toHaveLength(2); + const result: Paged> = await personService.findAllPersons(personDoWithQueryParam, 0, 10); + + expect(result.items).toHaveLength(2); }); it('should return an empty list of persons ', async () => { const person: PersonDo = DoFactory.createPerson(false); - personRepoMock.findAll.mockResolvedValue([]); + + personRepoMock.findBy.mockResolvedValue([[], 0]); mapperMock.map.mockReturnValue(person as unknown as Dictionary); - const result: PersonDo[] = await personService.findAllPersons(person); - expect(result).toBeInstanceOf(Array); - expect(result).toHaveLength(0); + + const result: Paged> = await personService.findAllPersons(person); + + expect(result.items).toBeInstanceOf(Array); + expect(result.items).toHaveLength(0); }); }); }); diff --git a/src/modules/person/domain/person.service.ts b/src/modules/person/domain/person.service.ts index 147da2c4e..c7875f3d1 100644 --- a/src/modules/person/domain/person.service.ts +++ b/src/modules/person/domain/person.service.ts @@ -30,9 +30,9 @@ export class PersonService { } public async findAllPersons( - offset: Option, - limit: Option, personDo: Partial>, + offset?: number, + limit?: number, ): Promise>> { const scope: PersonScope = new PersonScope() .findBy({ diff --git a/src/modules/person/persistence/person.repo.integration-spec.ts b/src/modules/person/persistence/person.repo.integration-spec.ts index be3a13573..2f0daaadb 100644 --- a/src/modules/person/persistence/person.repo.integration-spec.ts +++ b/src/modules/person/persistence/person.repo.integration-spec.ts @@ -8,6 +8,8 @@ import { PersonDo } from '../domain/person.do.js'; import { PersonPersistenceMapperProfile } from './person-persistence.mapper.profile.js'; import { PersonEntity } from './person.entity.js'; import { PersonRepo } from './person.repo.js'; +import { PersonScope } from './person.scope.js'; +import { ScopeOperator } from '../../../shared/persistence/scope.enums.js'; describe('PersonRepo', () => { let module: TestingModule; @@ -132,31 +134,45 @@ describe('PersonRepo', () => { }); describe('findAll', () => { - it('should find all persons from database', async () => { - const props: Partial> = { - referrer: 'referrer_value', - firstName: 'first name', - lastName: 'last name', - isInformationBlocked: false, - }; - const personDo1: PersonDo = DoFactory.createPerson(false, props); - const personDo2: PersonDo = DoFactory.createPerson(false, props); - await em.persistAndFlush(mapper.map(personDo1, PersonDo, PersonEntity)); - await em.persistAndFlush(mapper.map(personDo2, PersonDo, PersonEntity)); - const personDoFromQueryParam: PersonDo = DoFactory.createPerson(false, props); - const result: PersonDo[] = await sut.findAll(personDoFromQueryParam); - expect(result).not.toBeNull(); - expect(result).toHaveLength(2); - await expect(em.find(PersonEntity, {})).resolves.toHaveLength(2); + describe('when persons match the query', () => { + it('should return all matching persons', async () => { + const props: Partial> = { + referrer: 'referrer_value', + firstName: 'first name', + lastName: 'last name', + isInformationBlocked: false, + }; + const personDo1: PersonDo = DoFactory.createPerson(false, props); + const personDo2: PersonDo = DoFactory.createPerson(false, props); + + await em.persistAndFlush(mapper.map(personDo1, PersonDo, PersonEntity)); + await em.persistAndFlush(mapper.map(personDo2, PersonDo, PersonEntity)); + + const [result]: Counted> = await sut.findBy( + new PersonScope().findBy( + { + firstName: props.firstName, + lastName: props.lastName, + }, + ScopeOperator.AND, + ), + ); + + expect(result).not.toBeNull(); + expect(result).toHaveLength(2); + await expect(em.find(PersonEntity, {})).resolves.toHaveLength(2); + }); }); - it('should return an empty list', async () => { - const props: Partial> = {}; - const personDoFromQueryParam: PersonDo = DoFactory.createPerson(false, props); - const result: PersonDo[] = await sut.findAll(personDoFromQueryParam); - expect(result).not.toBeNull(); - expect(result).toHaveLength(0); - await expect(em.find(PersonEntity, {})).resolves.toHaveLength(0); + describe('when no person matches the query', () => { + it('should return an empty list', async () => { + const [result]: Counted> = await sut.findBy(new PersonScope()); + + expect(result).not.toBeNull(); + expect(result).toHaveLength(0); + + await expect(em.find(PersonEntity, {})).resolves.toHaveLength(0); + }); }); }); }); diff --git a/src/modules/person/persistence/person.repo.ts b/src/modules/person/persistence/person.repo.ts index 66ab91876..5d1401408 100644 --- a/src/modules/person/persistence/person.repo.ts +++ b/src/modules/person/persistence/person.repo.ts @@ -25,7 +25,7 @@ export class PersonRepo { } public async findById(id: string): Promise>> { - const person: Option = await this.em.findOne(PersonEntity, { id }); + const person: Option = await this.em.findOne(this.entityName, { id }); if (person) { return this.mapper.map(person, PersonEntity, PersonDo); } @@ -33,7 +33,7 @@ export class PersonRepo { } public async findByReferrer(referrer: string): Promise>> { - const person: Option = await this.em.findOne(PersonEntity, { referrer }); + const person: Option = await this.em.findOne(this.entityName, { referrer }); if (person) { return this.mapper.map(person, PersonEntity, PersonDo); } @@ -63,7 +63,7 @@ export class PersonRepo { } private async update(personDo: PersonDo): Promise> { - let person: Option> = await this.em.findOne(PersonEntity, { id: personDo.id }); + let person: Option> = await this.em.findOne(this.entityName, { id: personDo.id }); if (person) { person.assign(this.mapper.map(personDo, PersonDo, PersonEntity)); } else { diff --git a/src/modules/person/persistence/person.scope.integration-spec.ts b/src/modules/person/persistence/person.scope.integration-spec.ts new file mode 100644 index 000000000..ce6b6b6b2 --- /dev/null +++ b/src/modules/person/persistence/person.scope.integration-spec.ts @@ -0,0 +1,61 @@ +import { Mapper } from '@automapper/core'; +import { getMapperToken } from '@automapper/nestjs'; +import { MikroORM } from '@mikro-orm/core'; +import { EntityManager } from '@mikro-orm/postgresql'; +import { Test, TestingModule } from '@nestjs/testing'; +import { ConfigTestModule, DatabaseTestModule, DoFactory, MapperTestModule } from '../../../../test/utils/index.js'; +import { PersonDo } from '../domain/person.do.js'; +import { PersonPersistenceMapperProfile } from './person-persistence.mapper.profile.js'; +import { PersonEntity } from './person.entity.js'; +import { PersonScope } from './person.scope.js'; +import { ScopeOrder } from '../../../shared/persistence/scope.enums.js'; + +describe('PersonScope', () => { + let module: TestingModule; + let orm: MikroORM; + let em: EntityManager; + let mapper: Mapper; + + beforeAll(async () => { + module = await Test.createTestingModule({ + imports: [ConfigTestModule, DatabaseTestModule.forRoot({ isDatabaseRequired: true }), MapperTestModule], + providers: [PersonPersistenceMapperProfile], + }).compile(); + orm = module.get(MikroORM); + em = module.get(EntityManager); + mapper = module.get(getMapperToken()); + + await DatabaseTestModule.setupDatabase(orm); + }, 30 * 1_000); + + afterAll(async () => { + await module.close(); + }, 30 * 1_000); + + beforeEach(async () => { + await DatabaseTestModule.clearDatabase(orm); + }); + + describe('findBy', () => { + describe('when filtering for persons', () => { + beforeEach(async () => { + const persons: PersonEntity[] = Array.from({ length: 110 }, (_v: unknown, i: number) => + mapper.map(DoFactory.createPerson(false, { firstName: `John #${i}` }), PersonDo, PersonEntity), + ); + + await em.persistAndFlush(persons); + }); + + it('should return found persons', async () => { + const scope: PersonScope = new PersonScope() + .findBy({ firstName: new RegExp('John #1') }) + .sortBy('firstName', ScopeOrder.ASC) + .paged(10, 10); + const [persons, total]: Counted = await scope.executeQuery(em); + + expect(total).toBe(21); + expect(persons).toHaveLength(10); + }); + }); + }); +}); diff --git a/src/modules/person/persistence/person.scope.ts b/src/modules/person/persistence/person.scope.ts index 734dc8a84..dbc383f1a 100644 --- a/src/modules/person/persistence/person.scope.ts +++ b/src/modules/person/persistence/person.scope.ts @@ -3,9 +3,9 @@ import { ScopeBase, ScopeOperator } from '../../../shared/persistence/index.js'; import { PersonEntity } from './person.entity.js'; type FindProps = { - firstName?: string; - lastName?: string; - birthDate?: Date; + firstName: string; + lastName: string; + birthDate: Date; }; export class PersonScope extends ScopeBase { @@ -13,7 +13,7 @@ export class PersonScope extends ScopeBase { return PersonEntity; } - public findBy(findProps: FindProps, operator: ScopeOperator = ScopeOperator.AND): this { + public findBy(findProps: Findable, operator: ScopeOperator = ScopeOperator.AND): this { this.findByInternal( { firstName: findProps.firstName, diff --git a/src/shared/paging/global-pagination-headers.interceptor.spec.ts b/src/shared/paging/global-pagination-headers.interceptor.spec.ts new file mode 100644 index 000000000..4f946a050 --- /dev/null +++ b/src/shared/paging/global-pagination-headers.interceptor.spec.ts @@ -0,0 +1,79 @@ +import { Observable, from, lastValueFrom } from 'rxjs'; +import { DeepMocked, createMock } from '@golevelup/ts-jest'; +import { CallHandler, ExecutionContext } from '@nestjs/common'; +import { GlobalPaginationHeadersInterceptor } from './global-pagination-headers.interceptor.js'; +import { Response } from 'express'; +import { HttpArgumentsHost } from '@nestjs/common/interfaces/index.js'; +import { PagedResponse } from './paged.response.js'; + +describe('GlobalPaginationHeadersInterceptor', () => { + const sut: GlobalPaginationHeadersInterceptor = new GlobalPaginationHeadersInterceptor(); + let responseMock: DeepMocked; + let contextMock: DeepMocked; + let callHandlerMock: DeepMocked; + + describe('intercept', () => { + describe('when intercepting a paged response', () => { + beforeEach(() => { + responseMock = createMock(); + contextMock = createMock({ + switchToHttp: () => + createMock({ + getResponse: () => responseMock, + }), + }); + callHandlerMock = createMock({ + handle: () => from([new PagedResponse({ offset: 0, limit: 0, total: 0, items: [] })]), + }); + }); + + it('should set pagination headers for the response', async () => { + const observable: Observable = sut.intercept(contextMock, callHandlerMock); + + // is needed to execute the observable pipeline + await lastValueFrom(observable); + + expect(responseMock.setHeader).toBeCalledTimes(3); + expect(responseMock.setHeader).toBeCalledWith('pagination-offset', 0); + expect(responseMock.setHeader).toBeCalledWith('pagination-limit', 0); + expect(responseMock.setHeader).toBeCalledWith('pagination-total', 0); + }); + + it('should change response type to list', async () => { + const observable: Observable = sut.intercept(contextMock, callHandlerMock); + + await expect(lastValueFrom(observable)).resolves.toStrictEqual([]); + }); + }); + + describe('when intercepting a non paged response', () => { + beforeEach(() => { + responseMock = createMock(); + contextMock = createMock({ + switchToHttp: () => + createMock({ + getResponse: () => responseMock, + }), + }); + callHandlerMock = createMock({ + handle: () => from([null]), + }); + }); + + it('should not set any headers', async () => { + const observable: Observable = sut.intercept(contextMock, callHandlerMock); + + // is needed to execute the observable pipeline + await lastValueFrom(observable); + + expect(responseMock.setHeader).toBeCalledTimes(0); + }); + + it('should not change response type', async () => { + const observable: Observable = sut.intercept(contextMock, callHandlerMock); + + await expect(lastValueFrom(observable)).resolves.toBeNull(); + }); + }); + }); +}); diff --git a/src/shared/paging/global-pagination-headers.interceptor.ts b/src/shared/paging/global-pagination-headers.interceptor.ts index dd451ea40..7ea4959d3 100644 --- a/src/shared/paging/global-pagination-headers.interceptor.ts +++ b/src/shared/paging/global-pagination-headers.interceptor.ts @@ -21,8 +21,8 @@ export class GlobalPaginationHeadersInterceptor implements NestInterceptor { } private static setPaginationHeaders(response: Response, payload: PagedResponse): void { - response.setHeader('pagination-total', payload.total); response.setHeader('pagination-offset', payload.offset); response.setHeader('pagination-limit', payload.limit); + response.setHeader('pagination-total', payload.total); } } diff --git a/src/shared/persistence/scope-base.ts b/src/shared/persistence/scope-base.ts index 6dba8c16a..fef11d9b5 100644 --- a/src/shared/persistence/scope-base.ts +++ b/src/shared/persistence/scope-base.ts @@ -11,7 +11,7 @@ export abstract class ScopeBase { private limit: Option; - public abstract get entityName(): EntityName; + protected abstract get entityName(): EntityName; public async executeQuery(em: EntityManager): Promise> { const qb: QueryBuilder = em.createQueryBuilder(this.entityName); @@ -41,7 +41,7 @@ export abstract class ScopeBase { return this; } - protected findByInternal(props: Partial, operator: ScopeOperator): this { + protected findByInternal(props: Findable, operator: ScopeOperator): this { const query: QBFilterQuery = { [operator]: Object.keys(props) .filter((key: string) => props[key] !== undefined)