From 019133e290f4fd00508b4a9e56a28aa8f57d0338 Mon Sep 17 00:00:00 2001 From: Michal Plebanski Date: Thu, 9 Sep 2021 17:26:13 +0200 Subject: [PATCH 01/10] Add AFS e2e tests to cover update scenario --- .../application-flagged-sets.controller.ts | 26 +-- .../application-flagged-sets.service.ts | 4 +- ...application-flagged-set-pagination-meta.ts | 7 + .../application-flagged-set-resolve.dto.ts | 18 ++ .../dto/application-flagged-set.dto.ts | 29 +-- .../paginated-application-flagged-set.dto.ts | 11 ++ .../application-flagged-set.entity.ts | 2 +- ...ed-application-flagged-set-query-params.ts | 16 ++ backend/core/test/afs/afs.e2e-spec.ts | 181 ++++++++++++++++++ 9 files changed, 242 insertions(+), 52 deletions(-) create mode 100644 backend/core/src/application-flagged-sets/dto/application-flagged-set-pagination-meta.ts create mode 100644 backend/core/src/application-flagged-sets/dto/application-flagged-set-resolve.dto.ts create mode 100644 backend/core/src/application-flagged-sets/dto/paginated-application-flagged-set.dto.ts create mode 100644 backend/core/src/application-flagged-sets/paginated-application-flagged-set-query-params.ts diff --git a/backend/core/src/application-flagged-sets/application-flagged-sets.controller.ts b/backend/core/src/application-flagged-sets/application-flagged-sets.controller.ts index ac68179a73..08a63fd51c 100644 --- a/backend/core/src/application-flagged-sets/application-flagged-sets.controller.ts +++ b/backend/core/src/application-flagged-sets/application-flagged-sets.controller.ts @@ -11,33 +11,17 @@ import { ValidationPipe, } from "@nestjs/common" import { Request as ExpressRequest } from "express" -import { ApiBearerAuth, ApiOperation, ApiProperty, ApiTags } from "@nestjs/swagger" +import { ApiBearerAuth, ApiOperation, ApiTags } from "@nestjs/swagger" import { ResourceType } from "../auth/decorators/resource-type.decorator" import { OptionalAuthGuard } from "../auth/guards/optional-auth.guard" import { AuthzGuard } from "../auth/guards/authz.guard" import { defaultValidationPipeOptions } from "../shared/default-validation-pipe-options" import { mapTo } from "../shared/mapTo" -import { Expose } from "class-transformer" -import { IsUUID } from "class-validator" -import { ValidationsGroupsEnum } from "../shared/types/validations-groups-enum" import { ApplicationFlaggedSetsService } from "./application-flagged-sets.service" -import { PaginationQueryParams } from "../shared/dto/pagination.dto" -import { - ApplicationFlaggedSetDto, - ApplicationFlaggedSetResolveDto, - PaginatedApplicationFlaggedSetDto, -} from "./dto/application-flagged-set.dto" - -export class PaginatedApplicationFlaggedSetQueryParams extends PaginationQueryParams { - @Expose() - @ApiProperty({ - type: String, - example: "listingId", - required: true, - }) - @IsUUID(4, { groups: [ValidationsGroupsEnum.default] }) - listingId: string -} +import { ApplicationFlaggedSetDto } from "./dto/application-flagged-set.dto" +import { PaginatedApplicationFlaggedSetDto } from "./dto/paginated-application-flagged-set.dto" +import { ApplicationFlaggedSetResolveDto } from "./dto/application-flagged-set-resolve.dto" +import { PaginatedApplicationFlaggedSetQueryParams } from "./paginated-application-flagged-set-query-params" @Controller("/applicationFlaggedSets") @ApiTags("applicationFlaggedSets") diff --git a/backend/core/src/application-flagged-sets/application-flagged-sets.service.ts b/backend/core/src/application-flagged-sets/application-flagged-sets.service.ts index c4933c306d..4caceddaca 100644 --- a/backend/core/src/application-flagged-sets/application-flagged-sets.service.ts +++ b/backend/core/src/application-flagged-sets/application-flagged-sets.service.ts @@ -1,5 +1,4 @@ import { Inject, Injectable, NotFoundException, Scope } from "@nestjs/common" -import { PaginatedApplicationFlaggedSetQueryParams } from "./application-flagged-sets.controller" import { AuthzService } from "../auth/services/authz.service" import { ApplicationFlaggedSet } from "./entities/application-flagged-set.entity" import { InjectRepository } from "@nestjs/typeorm" @@ -13,12 +12,13 @@ import { } from "typeorm" import { paginate } from "nestjs-typeorm-paginate" import { Application } from "../applications/entities/application.entity" -import { ApplicationFlaggedSetResolveDto } from "./dto/application-flagged-set.dto" import { REQUEST } from "@nestjs/core" import { Request as ExpressRequest } from "express" import { User } from "../auth/entities/user.entity" import { FlaggedSetStatus } from "./types/flagged-set-status-enum" import { Rule } from "./types/rule-enum" +import { ApplicationFlaggedSetResolveDto } from "./dto/application-flagged-set-resolve.dto" +import { PaginatedApplicationFlaggedSetQueryParams } from "./paginated-application-flagged-set-query-params" @Injectable({ scope: Scope.REQUEST }) export class ApplicationFlaggedSetsService { diff --git a/backend/core/src/application-flagged-sets/dto/application-flagged-set-pagination-meta.ts b/backend/core/src/application-flagged-sets/dto/application-flagged-set-pagination-meta.ts new file mode 100644 index 0000000000..10c66f2512 --- /dev/null +++ b/backend/core/src/application-flagged-sets/dto/application-flagged-set-pagination-meta.ts @@ -0,0 +1,7 @@ +import { PaginationMeta } from "../../shared/dto/pagination.dto" +import { Expose } from "class-transformer" + +export class ApplicationFlaggedSetPaginationMeta extends PaginationMeta { + @Expose() + totalFlagged: number +} diff --git a/backend/core/src/application-flagged-sets/dto/application-flagged-set-resolve.dto.ts b/backend/core/src/application-flagged-sets/dto/application-flagged-set-resolve.dto.ts new file mode 100644 index 0000000000..7cfeae050c --- /dev/null +++ b/backend/core/src/application-flagged-sets/dto/application-flagged-set-resolve.dto.ts @@ -0,0 +1,18 @@ +import { Expose, Type } from "class-transformer" +import { ArrayMaxSize, IsArray, IsDefined, IsUUID, ValidateNested } from "class-validator" +import { ValidationsGroupsEnum } from "../../shared/types/validations-groups-enum" +import { IdDto } from "../../shared/dto/id.dto" + +export class ApplicationFlaggedSetResolveDto { + @Expose() + @IsUUID(4, { groups: [ValidationsGroupsEnum.default] }) + afsId: string + + @Expose() + @IsDefined({ groups: [ValidationsGroupsEnum.default] }) + @IsArray({ groups: [ValidationsGroupsEnum.default] }) + @ArrayMaxSize(512, { groups: [ValidationsGroupsEnum.default] }) + @ValidateNested({ groups: [ValidationsGroupsEnum.default], each: true }) + @Type(() => IdDto) + applications: IdDto[] +} diff --git a/backend/core/src/application-flagged-sets/dto/application-flagged-set.dto.ts b/backend/core/src/application-flagged-sets/dto/application-flagged-set.dto.ts index 4817fa1272..65c1ffa728 100644 --- a/backend/core/src/application-flagged-sets/dto/application-flagged-set.dto.ts +++ b/backend/core/src/application-flagged-sets/dto/application-flagged-set.dto.ts @@ -1,10 +1,9 @@ import { OmitType } from "@nestjs/swagger" import { ApplicationFlaggedSet } from "../entities/application-flagged-set.entity" import { Expose, Type } from "class-transformer" -import { ArrayMaxSize, IsArray, IsDefined, IsUUID, ValidateNested } from "class-validator" +import { ValidateNested } from "class-validator" import { ValidationsGroupsEnum } from "../../shared/types/validations-groups-enum" import { ApplicationDto } from "../../applications/dto/application.dto" -import { PaginationFactory, PaginationMeta } from "../../shared/dto/pagination.dto" import { IdDto } from "../../shared/dto/id.dto" export class ApplicationFlaggedSetDto extends OmitType(ApplicationFlaggedSet, [ @@ -27,29 +26,3 @@ export class ApplicationFlaggedSetDto extends OmitType(ApplicationFlaggedSet, [ @Type(() => IdDto) listing: IdDto } - -export class ApplicationFlaggedSetPaginationMeta extends PaginationMeta { - @Expose() - totalFlagged: number -} - -export class PaginatedApplicationFlaggedSetDto extends PaginationFactory( - ApplicationFlaggedSetDto -) { - @Expose() - meta: ApplicationFlaggedSetPaginationMeta -} - -export class ApplicationFlaggedSetResolveDto { - @Expose() - @IsUUID(4, { groups: [ValidationsGroupsEnum.default] }) - afsId: string - - @Expose() - @IsDefined({ groups: [ValidationsGroupsEnum.default] }) - @IsArray({ groups: [ValidationsGroupsEnum.default] }) - @ArrayMaxSize(512, { groups: [ValidationsGroupsEnum.default] }) - @ValidateNested({ groups: [ValidationsGroupsEnum.default], each: true }) - @Type(() => IdDto) - applications: IdDto[] -} diff --git a/backend/core/src/application-flagged-sets/dto/paginated-application-flagged-set.dto.ts b/backend/core/src/application-flagged-sets/dto/paginated-application-flagged-set.dto.ts new file mode 100644 index 0000000000..6a352fc361 --- /dev/null +++ b/backend/core/src/application-flagged-sets/dto/paginated-application-flagged-set.dto.ts @@ -0,0 +1,11 @@ +import { PaginationFactory } from "../../shared/dto/pagination.dto" +import { Expose } from "class-transformer" +import { ApplicationFlaggedSetPaginationMeta } from "./application-flagged-set-pagination-meta" +import { ApplicationFlaggedSetDto } from "./application-flagged-set.dto" + +export class PaginatedApplicationFlaggedSetDto extends PaginationFactory( + ApplicationFlaggedSetDto +) { + @Expose() + meta: ApplicationFlaggedSetPaginationMeta +} diff --git a/backend/core/src/application-flagged-sets/entities/application-flagged-set.entity.ts b/backend/core/src/application-flagged-sets/entities/application-flagged-set.entity.ts index c0463f767d..c3568d7b87 100644 --- a/backend/core/src/application-flagged-sets/entities/application-flagged-set.entity.ts +++ b/backend/core/src/application-flagged-sets/entities/application-flagged-set.entity.ts @@ -24,7 +24,7 @@ export class ApplicationFlaggedSet extends AbstractEntity { @Type(() => Date) resolvedTime?: Date | null - @ManyToOne(() => User, { eager: true, nullable: true, cascade: true }) + @ManyToOne(() => User, { eager: true, nullable: true, cascade: false }) @JoinColumn() @Expose() @ValidateNested({ groups: [ValidationsGroupsEnum.default] }) diff --git a/backend/core/src/application-flagged-sets/paginated-application-flagged-set-query-params.ts b/backend/core/src/application-flagged-sets/paginated-application-flagged-set-query-params.ts new file mode 100644 index 0000000000..8068126294 --- /dev/null +++ b/backend/core/src/application-flagged-sets/paginated-application-flagged-set-query-params.ts @@ -0,0 +1,16 @@ +import { PaginationQueryParams } from "../shared/dto/pagination.dto" +import { Expose } from "class-transformer" +import { ApiProperty } from "@nestjs/swagger" +import { IsUUID } from "class-validator" +import { ValidationsGroupsEnum } from "../shared/types/validations-groups-enum" + +export class PaginatedApplicationFlaggedSetQueryParams extends PaginationQueryParams { + @Expose() + @ApiProperty({ + type: String, + example: "listingId", + required: true, + }) + @IsUUID(4, { groups: [ValidationsGroupsEnum.default] }) + listingId: string +} diff --git a/backend/core/test/afs/afs.e2e-spec.ts b/backend/core/test/afs/afs.e2e-spec.ts index 176e1b73ec..36285b60b3 100644 --- a/backend/core/test/afs/afs.e2e-spec.ts +++ b/backend/core/test/afs/afs.e2e-spec.ts @@ -20,6 +20,7 @@ import { ApplicationFlaggedSet } from "../../src/application-flagged-sets/entiti import { getTestAppBody } from "../lib/get-test-app-body" import { FlaggedSetStatus } from "../../src/application-flagged-sets/types/flagged-set-status-enum" import { Rule } from "../../src/application-flagged-sets/types/rule-enum" +import { ApplicationDto } from "../../src/applications/dto/application.dto" // Cypress brings in Chai types for the global expect, but we want to use jest // expect here so we need to re-declare it. @@ -34,6 +35,8 @@ describe("ApplicationFlaggedSets", () => { let afsRepository: Repository let householdMembersRepository: Repository let listing1Id: string + let updateApplication + let getAfsesForListingId const setupDb = async () => { await householdMembersRepository.createQueryBuilder().delete().execute() @@ -79,6 +82,24 @@ describe("ApplicationFlaggedSets", () => { const listings = await supertest(app.getHttpServer()).get("/listings").expect(200) listing1Id = listings.body.items[0].id await setupDb() + + updateApplication = async (application: ApplicationDto) => { + return ( + await supertest(app.getHttpServer()) + .put(`/applications/${application.id}`) + .send(application) + .set(...setAuthorization(adminAccessToken)) + .expect(200) + ).body + } + + getAfsesForListingId = async (listingId) => { + return ( + await supertest(app.getHttpServer()) + .get(`/applicationFlaggedSets?listingId=${listingId}`) + .set(...setAuthorization(adminAccessToken)) + ).body + } }) it(`should mark two similar application as flagged`, async () => { @@ -184,6 +205,166 @@ describe("ApplicationFlaggedSets", () => { expect(resolvedAfs.applications.filter((app) => app.markedAsDuplicate === false).length).toBe(1) }) + it(`should take application edits into account (application toggles between conflicting and non conflicting in an AFS of 2 apps)`, async () => { + const app1Seed = getTestAppBody(listing1Id) + const app2Seed = getTestAppBody(listing1Id) + + // Two applications do not conflict by any rule at this point + app2Seed.applicant.emailAddress = "another@email.com" + app2Seed.applicant.firstName = "AnotherFirstName" + const apps = [] + + await Promise.all( + [app1Seed, app2Seed].map(async (payload) => { + const appRes = await supertest(app.getHttpServer()) + .post("/applications/submit") + .send(payload) + .expect(201) + apps.push(appRes.body) + }) + ) + + let afses = await getAfsesForListingId(listing1Id) + + expect(afses.body.meta.totalFlagged).toBe(0) + expect(afses.body.items.length).toBe(0) + + const [app1, app2] = apps + + // Applications conflict by email rule + app2.applicant.emailAddress = app1Seed.applicant.emailAddress + await updateApplication(app2) + + afses = await getAfsesForListingId(listing1Id) + + expect(afses.body.meta.totalFlagged).toBe(1) + expect(afses.body.items[0].applications.map((app) => app.id)).toContain(app2.id) + expect(afses.body.items[0].applications.map((app) => app.id)).toContain(app1.id) + + // Applications do not conflict by any rule + app2.applicant.emailAddress = app2Seed.applicant.emailAddress + await updateApplication(app2) + + afses = await getAfsesForListingId(listing1Id) + + expect(afses.body.meta.totalFlagged).toBe(0) + expect(afses.body.items.length).toBe(0) + }) + + it(`should take application edits into account (application toggles between conflicting and non conflicting in an AFS of 3 apps)`, async () => { + const app1Seed = getTestAppBody(listing1Id) + const app2Seed = getTestAppBody(listing1Id) + const app3Seed = getTestAppBody(listing1Id) + + // Three applications do not conflict by any rule at this point + app2Seed.applicant.emailAddress = "another@email.com" + app2Seed.applicant.firstName = "AnotherFirstName" + app3Seed.applicant.emailAddress = "third@email.com" + app3Seed.applicant.firstName = "ThirdFirstName" + const apps = [] + + await Promise.all( + [app1Seed, app2Seed, app3Seed].map(async (payload) => { + const appRes = await supertest(app.getHttpServer()) + .post("/applications/submit") + .send(payload) + .expect(201) + apps.push(appRes.body) + }) + ) + + let afses = await getAfsesForListingId(listing1Id) + + expect(afses.body.meta.totalFlagged).toBe(0) + expect(afses.body.items.length).toBe(0) + + // eslint-disable-next-line + let [app1, app2, app3] = apps + + // Applications conflict by email rule + app2.applicant.emailAddress = app1Seed.applicant.emailAddress + app3.applicant.emailAddress = app1Seed.applicant.emailAddress + await updateApplication(app2) + app3 = await updateApplication(app3) + expect(app3.flagged).toBe(true) + + afses = await getAfsesForListingId(listing1Id) + + expect(afses.body.meta.totalFlagged).toBe(1) + expect(afses.body.items[0].applications.map((app) => app.id)).toContain(app1.id) + expect(afses.body.items[0].applications.map((app) => app.id)).toContain(app2.id) + expect(afses.body.items[0].applications.map((app) => app.id)).toContain(app3.id) + + // Application 3 do not conflict with others now + app3.applicant.emailAddress = app3Seed.applicant.emailAddress + app3 = await updateApplication(app3) + expect(app3.flagged).toBe(false) + + afses = await getAfsesForListingId(listing1Id) + + expect(afses.body.meta.totalFlagged).toBe(1) + expect(afses.body.items.length).toBe(1) + expect(afses.body.items[0].applications.map((app) => app.id)).not.toContain(app3.id) + }) + + it(`should take application edits into account (application toggles between conflicting and non conflicting in an AFS of 3 apps, AFS already resolved)`, async () => { + const app1Seed = getTestAppBody(listing1Id) + const app2Seed = getTestAppBody(listing1Id) + const app3Seed = getTestAppBody(listing1Id) + + // Three applications conflict by email rule + app2Seed.applicant.firstName = "AnotherFirstName" + app3Seed.applicant.firstName = "ThirdFirstName" + const apps = [] + + await Promise.all( + [app1Seed, app2Seed, app3Seed].map(async (payload) => { + const appRes = await supertest(app.getHttpServer()) + .post("/applications/submit") + .send(payload) + .expect(201) + apps.push(appRes.body) + }) + ) + + // eslint-disable-next-line + let [app1, app2, app3] = apps + expect(app3.markedAsDuplicate).toBe(false) + + const afses = await getAfsesForListingId(listing1Id) + const afsToBeResolved = afses.items[0] + + const resolveRes = await supertest(app.getHttpServer()) + .post(`/applicationFlaggedSets/resolve`) + .send({ afsId: afsToBeResolved.id, applications: [{ id: app3.id }] }) + .set(...setAuthorization(adminAccessToken)) + .expect(201) + expect(resolveRes.body.resolvedTime).not.toBe(null) + expect(resolveRes.body.resolvingUser).not.toBe(null) + expect(resolveRes.body.status).toBe(FlaggedSetStatus.resolved) + + app3 = await updateApplication(app3) + expect(app3.markedAsDuplicate).toBe(true) + + // App3 now does not conflict with any other applications + app3.applicant.emailAddress = "third@email.com" + app3 = await updateApplication(app3) + expect(app3.markedAsDuplicate).toBe(false) + expect(app3.flagged).toBe(false) + + const previouslyResolvedAfs = ( + await supertest(app.getHttpServer()) + .get(`/applicationFlaggedSets/${afsToBeResolved.id}`) + .set(...setAuthorization(adminAccessToken)) + ).body + expect(resolveRes.body.resolvedTime).toBe(null) + expect(resolveRes.body.resolvingUser).toBe(null) + expect(previouslyResolvedAfs.status).toBe(FlaggedSetStatus.flagged) + expect(previouslyResolvedAfs.applications.map((app) => app.id)).toContain(app1.id) + expect(previouslyResolvedAfs.applications.map((app) => app.id)).toContain(app2.id) + expect(previouslyResolvedAfs.applications.map((app) => app.id)).not.toContain(app3.id) + }) + afterEach(async () => { await setupDb() jest.clearAllMocks() From 6bf0a415aa7324225bb57d1916329ead68a693e6 Mon Sep 17 00:00:00 2001 From: Michal Plebanski Date: Thu, 9 Sep 2021 17:29:00 +0200 Subject: [PATCH 02/10] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7dfdef4236..f8ef39044b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -108,6 +108,7 @@ All notable changes to this project will be documented in this file. The format - Fixes flakiness in authz.e2e-spec.ts related to logged in user trying to GET /applications which did not belong to him (sorting of UUID is not deterministic, so the user should fetch by specying a query param userId = self) [#1575](https://github.com/bloom-housing/bloom/pull/1575) - Fixed ListingsService.retrieve `view` query param not being optional in autogenerated client (it should be) [#1575](https://github.com/bloom-housing/bloom/pull/1575) - updated DTOs to omit entities and use DTOs for application-method, user-roles, user, listing and units-summary ([#1679](https://github.com/bloom-housing/bloom/pull/1679)) + - makes application flagged sets module take applications edits into account (e.g. a leasing agent changes something in the application) ([#1810](https://github.com/bloom-housing/bloom/pull/1810)) ### General From fd7daca90237647f6cece0ccd45fd402c22ba845 Mon Sep 17 00:00:00 2001 From: Michal Plebanski Date: Tue, 14 Sep 2021 16:24:40 +0200 Subject: [PATCH 03/10] Add constraint that leasing agent can only resolve AFS when listing is closed --- .../application-flagged-sets.service.ts | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/backend/core/src/application-flagged-sets/application-flagged-sets.service.ts b/backend/core/src/application-flagged-sets/application-flagged-sets.service.ts index 4caceddaca..d412843fb4 100644 --- a/backend/core/src/application-flagged-sets/application-flagged-sets.service.ts +++ b/backend/core/src/application-flagged-sets/application-flagged-sets.service.ts @@ -1,14 +1,14 @@ -import { Inject, Injectable, NotFoundException, Scope } from "@nestjs/common" +import { BadRequestException, Inject, Injectable, NotFoundException, Scope } from "@nestjs/common" import { AuthzService } from "../auth/services/authz.service" import { ApplicationFlaggedSet } from "./entities/application-flagged-set.entity" import { InjectRepository } from "@nestjs/typeorm" import { Brackets, DeepPartial, + EntityManager, + getManager, Repository, SelectQueryBuilder, - getManager, - EntityManager, } from "typeorm" import { paginate } from "nestjs-typeorm-paginate" import { Application } from "../applications/entities/application.entity" @@ -19,6 +19,7 @@ import { FlaggedSetStatus } from "./types/flagged-set-status-enum" import { Rule } from "./types/rule-enum" import { ApplicationFlaggedSetResolveDto } from "./dto/application-flagged-set-resolve.dto" import { PaginatedApplicationFlaggedSetQueryParams } from "./paginated-application-flagged-set-query-params" +import { ListingStatus } from "../listings/types/listing-status-enum" @Injectable({ scope: Scope.REQUEST }) export class ApplicationFlaggedSetsService { @@ -71,11 +72,16 @@ export class ApplicationFlaggedSetsService { const transApplicationsRepository = transactionalEntityManager.getRepository(Application) const afs = await transAfsRepository.findOne({ where: { id: dto.afsId }, - relations: ["applications"], + relations: ["applications", "listing"], }) if (!afs) { throw new NotFoundException() } + + if (afs.listing.status !== ListingStatus.closed) { + throw new BadRequestException("Listing must be closed before resolving any duplicates.") + } + // eslint-disable-next-line @typescript-eslint/no-unnecessary-type-assertion afs.resolvingUser = this.request.user as User afs.resolvedTime = new Date() From 8d6ac3df2b8c4b639e4794fed5d7ae7ae92c1719 Mon Sep 17 00:00:00 2001 From: Michal Plebanski Date: Wed, 15 Sep 2021 15:41:16 +0200 Subject: [PATCH 04/10] Make application updates trigger AFS recalculations --- CHANGELOG.md | 1 + .../application-flagged-sets.service.ts | 40 ++++++ .../src/applications/applications.service.ts | 52 ++++--- backend/core/test/afs/afs.e2e-spec.ts | 132 ++++++++++-------- 4 files changed, 149 insertions(+), 76 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f8ef39044b..1bdcfb37a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ All notable changes to this project will be documented in this file. The format - Added: - Add POST /users/invite endpoint and extend PUT /users/confirm with optional password change ([#1801](https://github.com/bloom-housing/bloom/pull/1801)) + - Changes to applications done through `PUT /applications/:id` are now reflected in AFS ([#1810](https://github.com/bloom-housing/bloom/pull/1810)) ## Frontend diff --git a/backend/core/src/application-flagged-sets/application-flagged-sets.service.ts b/backend/core/src/application-flagged-sets/application-flagged-sets.service.ts index d412843fb4..59cc5aea59 100644 --- a/backend/core/src/application-flagged-sets/application-flagged-sets.service.ts +++ b/backend/core/src/application-flagged-sets/application-flagged-sets.service.ts @@ -121,6 +121,46 @@ export class ApplicationFlaggedSetsService { } } + async onApplicationUpdate( + newApplication: Application, + transactionalEntityManager: EntityManager + ) { + const transApplicationsRepository = transactionalEntityManager.getRepository(Application) + newApplication.markedAsDuplicate = false + await transApplicationsRepository.save(newApplication) + + const transAfsRepository = transactionalEntityManager.getRepository(ApplicationFlaggedSet) + let afses = await transAfsRepository + .createQueryBuilder("afs") + .leftJoinAndSelect("afs.applications", "applications") + .leftJoinAndSelect("afs.listing", "listing") + .where("listing.id = :listingId", { listingId: newApplication.listing.id }) + .getMany() + afses = afses.filter( + (afs) => afs.applications.findIndex((app) => app.id === newApplication.id) !== -1 + ) + const afsesToBeSaved: Array = [] + const afsesToBeRemoved: Array = [] + for (const afs of afses) { + afs.status = FlaggedSetStatus.flagged + afs.resolvedTime = null + afs.resolvingUser = null + const applicationIndex = afs.applications.findIndex( + (application) => application.id === newApplication.id + ) + afs.applications.splice(applicationIndex, 1) + if (afs.applications.length > 1) { + afsesToBeSaved.push(afs) + } else { + afsesToBeRemoved.push(afs) + } + } + await transAfsRepository.save(afsesToBeSaved) + await transAfsRepository.remove(afsesToBeRemoved) + + await this.onApplicationSave(newApplication, transactionalEntityManager) + } + async fetchDuplicatesMatchingRule( transactionalEntityManager: EntityManager, application: Application, diff --git a/backend/core/src/applications/applications.service.ts b/backend/core/src/applications/applications.service.ts index 559ee5c243..1ac341b621 100644 --- a/backend/core/src/applications/applications.service.ts +++ b/backend/core/src/applications/applications.service.ts @@ -9,7 +9,7 @@ import { import { Application } from "./entities/application.entity" import { ApplicationCreateDto, ApplicationUpdateDto } from "./dto/application.dto" import { InjectRepository } from "@nestjs/typeorm" -import { getManager, QueryFailedError, Repository } from "typeorm" +import { QueryFailedError, Repository } from "typeorm" import { paginate, Pagination } from "nestjs-typeorm-paginate" import { PaginatedApplicationListQueryParams } from "./applications.controller" import { ApplicationFlaggedSetsService } from "../application-flagged-sets/application-flagged-sets.service" @@ -99,8 +99,7 @@ export class ApplicationsService { throw new BadRequestException("Listing is not open for application submission.") } await this.authorizeUserAction(this.req.user, applicationCreateDto, authzActions.submit) - const application = await this._create(applicationCreateDto) - return application + return await this._create(applicationCreateDto) } async create(applicationCreateDto: ApplicationCreateDto) { @@ -130,8 +129,19 @@ export class ApplicationsService { id: application.id, }) - await this.repository.save(application) - return application + return await this.repository.manager.transaction( + "SERIALIZABLE", + async (transactionalEntityManager) => { + const applicationsRepository = transactionalEntityManager.getRepository(Application) + const newApplication = await applicationsRepository.save(application) + await this.applicationFlaggedSetsService.onApplicationUpdate( + application, + transactionalEntityManager + ) + + return await applicationsRepository.findOne({ id: newApplication.id }) + } + ) } async delete(applicationId: string) { @@ -190,18 +200,22 @@ export class ApplicationsService { } private async _createApplication(applicationCreateDto: ApplicationUpdateDto) { - return await getManager().transaction("SERIALIZABLE", async (transactionalEntityManager) => { - const applicationsRepository = transactionalEntityManager.getRepository(Application) - const application = await applicationsRepository.save({ - ...applicationCreateDto, - user: this.req.user, - }) - await this.applicationFlaggedSetsService.onApplicationSave( - application, - transactionalEntityManager - ) - return application - }) + return await this.repository.manager.transaction( + "SERIALIZABLE", + async (transactionalEntityManager) => { + const applicationsRepository = transactionalEntityManager.getRepository(Application) + const application = await applicationsRepository.save({ + ...applicationCreateDto, + user: this.req.user, + }) + await this.applicationFlaggedSetsService.onApplicationSave( + application, + transactionalEntityManager + ) + + return await applicationsRepository.findOne({ id: application.id }) + } + ) } private async _create(applicationCreateDto: ApplicationUpdateDto) { @@ -240,7 +254,9 @@ export class ApplicationsService { } } - const listing = await this.listingsService.findOne(application.listing.id) + // Listing is not eagerly joined on application entity so let's use the one provided with + // create dto + const listing = await this.listingsService.findOne(applicationCreateDto.listing.id) if (application.applicant.emailAddress) { await this.emailService.confirmation(listing, application, applicationCreateDto.appUrl) } diff --git a/backend/core/test/afs/afs.e2e-spec.ts b/backend/core/test/afs/afs.e2e-spec.ts index 36285b60b3..10269fda0e 100644 --- a/backend/core/test/afs/afs.e2e-spec.ts +++ b/backend/core/test/afs/afs.e2e-spec.ts @@ -9,9 +9,6 @@ import { ListingsModule } from "../../src/listings/listings.module" import { EmailService } from "../../src/shared/email/email.service" import { getUserAccessToken } from "../utils/get-user-access-token" import { setAuthorization } from "../utils/set-authorization-helper" -// Use require because of the CommonJS/AMD style export. -// See https://www.typescriptlang.org/docs/handbook/modules.html#export--and-import--require -import dbOptions = require("../../ormconfig.test") import { Repository } from "typeorm" import { Application } from "../../src/applications/entities/application.entity" import { HouseholdMember } from "../../src/applications/entities/household-member.entity" @@ -21,6 +18,11 @@ import { getTestAppBody } from "../lib/get-test-app-body" import { FlaggedSetStatus } from "../../src/application-flagged-sets/types/flagged-set-status-enum" import { Rule } from "../../src/application-flagged-sets/types/rule-enum" import { ApplicationDto } from "../../src/applications/dto/application.dto" +import { Listing } from "../../src/listings/entities/listing.entity" +import { ListingStatus } from "../../src/listings/types/listing-status-enum" +// Use require because of the CommonJS/AMD style export. +// See https://www.typescriptlang.org/docs/handbook/modules.html#export--and-import--require +import dbOptions = require("../../ormconfig.test") // Cypress brings in Chai types for the global expect, but we want to use jest // expect here so we need to re-declare it. @@ -34,8 +36,10 @@ describe("ApplicationFlaggedSets", () => { let applicationsRepository: Repository let afsRepository: Repository let householdMembersRepository: Repository + let listingsRepository: Repository let listing1Id: string let updateApplication + let getApplication let getAfsesForListingId const setupDb = async () => { @@ -56,7 +60,7 @@ describe("ApplicationFlaggedSets", () => { AuthModule, ListingsModule, ApplicationsModule, - TypeOrmModule.forFeature([ApplicationFlaggedSet, Application, HouseholdMember]), + TypeOrmModule.forFeature([ApplicationFlaggedSet, Application, HouseholdMember, Listing]), ThrottlerModule.forRoot({ ttl: 60, limit: 5, @@ -77,10 +81,15 @@ describe("ApplicationFlaggedSets", () => { householdMembersRepository = app.get>( getRepositoryToken(HouseholdMember) ) + listingsRepository = app.get>(getRepositoryToken(Listing)) + const listing = (await listingsRepository.find({ take: 1 }))[0] + await listingsRepository.save({ + ...listing, + status: ListingStatus.closed, + }) adminAccessToken = await getUserAccessToken(app, "admin@example.com", "abcdef") - const listings = await supertest(app.getHttpServer()).get("/listings").expect(200) - listing1Id = listings.body.items[0].id + listing1Id = listing.id await setupDb() updateApplication = async (application: ApplicationDto) => { @@ -93,11 +102,21 @@ describe("ApplicationFlaggedSets", () => { ).body } + getApplication = async (id: string) => { + return ( + await supertest(app.getHttpServer()) + .get(`/applications/${id}`) + .set(...setAuthorization(adminAccessToken)) + .expect(200) + ).body + } + getAfsesForListingId = async (listingId) => { return ( await supertest(app.getHttpServer()) .get(`/applicationFlaggedSets?listingId=${listingId}`) .set(...setAuthorization(adminAccessToken)) + .expect(200) ).body } }) @@ -214,20 +233,18 @@ describe("ApplicationFlaggedSets", () => { app2Seed.applicant.firstName = "AnotherFirstName" const apps = [] - await Promise.all( - [app1Seed, app2Seed].map(async (payload) => { - const appRes = await supertest(app.getHttpServer()) - .post("/applications/submit") - .send(payload) - .expect(201) - apps.push(appRes.body) - }) - ) + for (const payload of [app1Seed, app2Seed]) { + const appRes = await supertest(app.getHttpServer()) + .post("/applications/submit") + .send(payload) + .expect(201) + apps.push(appRes.body) + } let afses = await getAfsesForListingId(listing1Id) - expect(afses.body.meta.totalFlagged).toBe(0) - expect(afses.body.items.length).toBe(0) + expect(afses.meta.totalFlagged).toBe(0) + expect(afses.items.length).toBe(0) const [app1, app2] = apps @@ -237,9 +254,9 @@ describe("ApplicationFlaggedSets", () => { afses = await getAfsesForListingId(listing1Id) - expect(afses.body.meta.totalFlagged).toBe(1) - expect(afses.body.items[0].applications.map((app) => app.id)).toContain(app2.id) - expect(afses.body.items[0].applications.map((app) => app.id)).toContain(app1.id) + expect(afses.meta.totalFlagged).toBe(1) + expect(afses.items[0].applications.map((app) => app.id).includes(app1.id)).toBe(true) + expect(afses.items[0].applications.map((app) => app.id).includes(app2.id)).toBe(true) // Applications do not conflict by any rule app2.applicant.emailAddress = app2Seed.applicant.emailAddress @@ -247,8 +264,8 @@ describe("ApplicationFlaggedSets", () => { afses = await getAfsesForListingId(listing1Id) - expect(afses.body.meta.totalFlagged).toBe(0) - expect(afses.body.items.length).toBe(0) + expect(afses.meta.totalFlagged).toBe(0) + expect(afses.items.length).toBe(0) }) it(`should take application edits into account (application toggles between conflicting and non conflicting in an AFS of 3 apps)`, async () => { @@ -263,20 +280,18 @@ describe("ApplicationFlaggedSets", () => { app3Seed.applicant.firstName = "ThirdFirstName" const apps = [] - await Promise.all( - [app1Seed, app2Seed, app3Seed].map(async (payload) => { - const appRes = await supertest(app.getHttpServer()) - .post("/applications/submit") - .send(payload) - .expect(201) - apps.push(appRes.body) - }) - ) + for (const payload of [app1Seed, app2Seed, app3Seed]) { + const appRes = await supertest(app.getHttpServer()) + .post("/applications/submit") + .send(payload) + .expect(201) + apps.push(appRes.body) + } let afses = await getAfsesForListingId(listing1Id) - expect(afses.body.meta.totalFlagged).toBe(0) - expect(afses.body.items.length).toBe(0) + expect(afses.meta.totalFlagged).toBe(0) + expect(afses.items.length).toBe(0) // eslint-disable-next-line let [app1, app2, app3] = apps @@ -286,25 +301,23 @@ describe("ApplicationFlaggedSets", () => { app3.applicant.emailAddress = app1Seed.applicant.emailAddress await updateApplication(app2) app3 = await updateApplication(app3) - expect(app3.flagged).toBe(true) afses = await getAfsesForListingId(listing1Id) - expect(afses.body.meta.totalFlagged).toBe(1) - expect(afses.body.items[0].applications.map((app) => app.id)).toContain(app1.id) - expect(afses.body.items[0].applications.map((app) => app.id)).toContain(app2.id) - expect(afses.body.items[0].applications.map((app) => app.id)).toContain(app3.id) + expect(afses.meta.totalFlagged).toBe(1) + expect(afses.items[0].applications.map((app) => app.id).includes(app1.id)).toBe(true) + expect(afses.items[0].applications.map((app) => app.id).includes(app2.id)).toBe(true) + expect(afses.items[0].applications.map((app) => app.id).includes(app3.id)).toBe(true) // Application 3 do not conflict with others now app3.applicant.emailAddress = app3Seed.applicant.emailAddress app3 = await updateApplication(app3) - expect(app3.flagged).toBe(false) afses = await getAfsesForListingId(listing1Id) - expect(afses.body.meta.totalFlagged).toBe(1) - expect(afses.body.items.length).toBe(1) - expect(afses.body.items[0].applications.map((app) => app.id)).not.toContain(app3.id) + expect(afses.meta.totalFlagged).toBe(1) + expect(afses.items.length).toBe(1) + expect(afses.items[0].applications.map((app) => app.id).includes(app3.id)).toBe(false) }) it(`should take application edits into account (application toggles between conflicting and non conflicting in an AFS of 3 apps, AFS already resolved)`, async () => { @@ -317,15 +330,13 @@ describe("ApplicationFlaggedSets", () => { app3Seed.applicant.firstName = "ThirdFirstName" const apps = [] - await Promise.all( - [app1Seed, app2Seed, app3Seed].map(async (payload) => { - const appRes = await supertest(app.getHttpServer()) - .post("/applications/submit") - .send(payload) - .expect(201) - apps.push(appRes.body) - }) - ) + for (const payload of [app1Seed, app2Seed, app3Seed]) { + const appRes = await supertest(app.getHttpServer()) + .post("/applications/submit") + .send(payload) + .expect(201) + apps.push(appRes.body) + } // eslint-disable-next-line let [app1, app2, app3] = apps @@ -343,26 +354,26 @@ describe("ApplicationFlaggedSets", () => { expect(resolveRes.body.resolvingUser).not.toBe(null) expect(resolveRes.body.status).toBe(FlaggedSetStatus.resolved) - app3 = await updateApplication(app3) + app3 = await getApplication(app3.id) expect(app3.markedAsDuplicate).toBe(true) // App3 now does not conflict with any other applications app3.applicant.emailAddress = "third@email.com" app3 = await updateApplication(app3) expect(app3.markedAsDuplicate).toBe(false) - expect(app3.flagged).toBe(false) const previouslyResolvedAfs = ( await supertest(app.getHttpServer()) .get(`/applicationFlaggedSets/${afsToBeResolved.id}`) .set(...setAuthorization(adminAccessToken)) + .expect(200) ).body - expect(resolveRes.body.resolvedTime).toBe(null) - expect(resolveRes.body.resolvingUser).toBe(null) + expect(previouslyResolvedAfs.resolvedTime).toBe(null) + expect(previouslyResolvedAfs.resolvingUser).toBe(null) expect(previouslyResolvedAfs.status).toBe(FlaggedSetStatus.flagged) - expect(previouslyResolvedAfs.applications.map((app) => app.id)).toContain(app1.id) - expect(previouslyResolvedAfs.applications.map((app) => app.id)).toContain(app2.id) - expect(previouslyResolvedAfs.applications.map((app) => app.id)).not.toContain(app3.id) + expect(previouslyResolvedAfs.applications.map((app) => app.id).includes(app1.id)).toBe(true) + expect(previouslyResolvedAfs.applications.map((app) => app.id).includes(app2.id)).toBe(true) + expect(previouslyResolvedAfs.applications.map((app) => app.id).includes(app3.id)).toBe(false) }) afterEach(async () => { @@ -371,6 +382,11 @@ describe("ApplicationFlaggedSets", () => { }) afterAll(async () => { + const modifiedListing = await listingsRepository.findOne({ id: listing1Id }) + await listingsRepository.save({ + ...modifiedListing, + status: ListingStatus.active, + }) await app.close() }) }) From 06dafc6149716cf50647dde76dd836b4c88c9825 Mon Sep 17 00:00:00 2001 From: Michal Plebanski Date: Thu, 9 Sep 2021 17:26:13 +0200 Subject: [PATCH 05/10] Add AFS e2e tests to cover update scenario --- .../application-flagged-sets.controller.ts | 26 +-- .../application-flagged-sets.service.ts | 4 +- ...application-flagged-set-pagination-meta.ts | 7 + .../application-flagged-set-resolve.dto.ts | 18 ++ .../dto/application-flagged-set.dto.ts | 29 +-- .../paginated-application-flagged-set.dto.ts | 11 ++ .../application-flagged-set.entity.ts | 2 +- ...ed-application-flagged-set-query-params.ts | 16 ++ backend/core/test/afs/afs.e2e-spec.ts | 181 ++++++++++++++++++ 9 files changed, 242 insertions(+), 52 deletions(-) create mode 100644 backend/core/src/application-flagged-sets/dto/application-flagged-set-pagination-meta.ts create mode 100644 backend/core/src/application-flagged-sets/dto/application-flagged-set-resolve.dto.ts create mode 100644 backend/core/src/application-flagged-sets/dto/paginated-application-flagged-set.dto.ts create mode 100644 backend/core/src/application-flagged-sets/paginated-application-flagged-set-query-params.ts diff --git a/backend/core/src/application-flagged-sets/application-flagged-sets.controller.ts b/backend/core/src/application-flagged-sets/application-flagged-sets.controller.ts index ac68179a73..08a63fd51c 100644 --- a/backend/core/src/application-flagged-sets/application-flagged-sets.controller.ts +++ b/backend/core/src/application-flagged-sets/application-flagged-sets.controller.ts @@ -11,33 +11,17 @@ import { ValidationPipe, } from "@nestjs/common" import { Request as ExpressRequest } from "express" -import { ApiBearerAuth, ApiOperation, ApiProperty, ApiTags } from "@nestjs/swagger" +import { ApiBearerAuth, ApiOperation, ApiTags } from "@nestjs/swagger" import { ResourceType } from "../auth/decorators/resource-type.decorator" import { OptionalAuthGuard } from "../auth/guards/optional-auth.guard" import { AuthzGuard } from "../auth/guards/authz.guard" import { defaultValidationPipeOptions } from "../shared/default-validation-pipe-options" import { mapTo } from "../shared/mapTo" -import { Expose } from "class-transformer" -import { IsUUID } from "class-validator" -import { ValidationsGroupsEnum } from "../shared/types/validations-groups-enum" import { ApplicationFlaggedSetsService } from "./application-flagged-sets.service" -import { PaginationQueryParams } from "../shared/dto/pagination.dto" -import { - ApplicationFlaggedSetDto, - ApplicationFlaggedSetResolveDto, - PaginatedApplicationFlaggedSetDto, -} from "./dto/application-flagged-set.dto" - -export class PaginatedApplicationFlaggedSetQueryParams extends PaginationQueryParams { - @Expose() - @ApiProperty({ - type: String, - example: "listingId", - required: true, - }) - @IsUUID(4, { groups: [ValidationsGroupsEnum.default] }) - listingId: string -} +import { ApplicationFlaggedSetDto } from "./dto/application-flagged-set.dto" +import { PaginatedApplicationFlaggedSetDto } from "./dto/paginated-application-flagged-set.dto" +import { ApplicationFlaggedSetResolveDto } from "./dto/application-flagged-set-resolve.dto" +import { PaginatedApplicationFlaggedSetQueryParams } from "./paginated-application-flagged-set-query-params" @Controller("/applicationFlaggedSets") @ApiTags("applicationFlaggedSets") diff --git a/backend/core/src/application-flagged-sets/application-flagged-sets.service.ts b/backend/core/src/application-flagged-sets/application-flagged-sets.service.ts index c4933c306d..4caceddaca 100644 --- a/backend/core/src/application-flagged-sets/application-flagged-sets.service.ts +++ b/backend/core/src/application-flagged-sets/application-flagged-sets.service.ts @@ -1,5 +1,4 @@ import { Inject, Injectable, NotFoundException, Scope } from "@nestjs/common" -import { PaginatedApplicationFlaggedSetQueryParams } from "./application-flagged-sets.controller" import { AuthzService } from "../auth/services/authz.service" import { ApplicationFlaggedSet } from "./entities/application-flagged-set.entity" import { InjectRepository } from "@nestjs/typeorm" @@ -13,12 +12,13 @@ import { } from "typeorm" import { paginate } from "nestjs-typeorm-paginate" import { Application } from "../applications/entities/application.entity" -import { ApplicationFlaggedSetResolveDto } from "./dto/application-flagged-set.dto" import { REQUEST } from "@nestjs/core" import { Request as ExpressRequest } from "express" import { User } from "../auth/entities/user.entity" import { FlaggedSetStatus } from "./types/flagged-set-status-enum" import { Rule } from "./types/rule-enum" +import { ApplicationFlaggedSetResolveDto } from "./dto/application-flagged-set-resolve.dto" +import { PaginatedApplicationFlaggedSetQueryParams } from "./paginated-application-flagged-set-query-params" @Injectable({ scope: Scope.REQUEST }) export class ApplicationFlaggedSetsService { diff --git a/backend/core/src/application-flagged-sets/dto/application-flagged-set-pagination-meta.ts b/backend/core/src/application-flagged-sets/dto/application-flagged-set-pagination-meta.ts new file mode 100644 index 0000000000..10c66f2512 --- /dev/null +++ b/backend/core/src/application-flagged-sets/dto/application-flagged-set-pagination-meta.ts @@ -0,0 +1,7 @@ +import { PaginationMeta } from "../../shared/dto/pagination.dto" +import { Expose } from "class-transformer" + +export class ApplicationFlaggedSetPaginationMeta extends PaginationMeta { + @Expose() + totalFlagged: number +} diff --git a/backend/core/src/application-flagged-sets/dto/application-flagged-set-resolve.dto.ts b/backend/core/src/application-flagged-sets/dto/application-flagged-set-resolve.dto.ts new file mode 100644 index 0000000000..7cfeae050c --- /dev/null +++ b/backend/core/src/application-flagged-sets/dto/application-flagged-set-resolve.dto.ts @@ -0,0 +1,18 @@ +import { Expose, Type } from "class-transformer" +import { ArrayMaxSize, IsArray, IsDefined, IsUUID, ValidateNested } from "class-validator" +import { ValidationsGroupsEnum } from "../../shared/types/validations-groups-enum" +import { IdDto } from "../../shared/dto/id.dto" + +export class ApplicationFlaggedSetResolveDto { + @Expose() + @IsUUID(4, { groups: [ValidationsGroupsEnum.default] }) + afsId: string + + @Expose() + @IsDefined({ groups: [ValidationsGroupsEnum.default] }) + @IsArray({ groups: [ValidationsGroupsEnum.default] }) + @ArrayMaxSize(512, { groups: [ValidationsGroupsEnum.default] }) + @ValidateNested({ groups: [ValidationsGroupsEnum.default], each: true }) + @Type(() => IdDto) + applications: IdDto[] +} diff --git a/backend/core/src/application-flagged-sets/dto/application-flagged-set.dto.ts b/backend/core/src/application-flagged-sets/dto/application-flagged-set.dto.ts index 4817fa1272..65c1ffa728 100644 --- a/backend/core/src/application-flagged-sets/dto/application-flagged-set.dto.ts +++ b/backend/core/src/application-flagged-sets/dto/application-flagged-set.dto.ts @@ -1,10 +1,9 @@ import { OmitType } from "@nestjs/swagger" import { ApplicationFlaggedSet } from "../entities/application-flagged-set.entity" import { Expose, Type } from "class-transformer" -import { ArrayMaxSize, IsArray, IsDefined, IsUUID, ValidateNested } from "class-validator" +import { ValidateNested } from "class-validator" import { ValidationsGroupsEnum } from "../../shared/types/validations-groups-enum" import { ApplicationDto } from "../../applications/dto/application.dto" -import { PaginationFactory, PaginationMeta } from "../../shared/dto/pagination.dto" import { IdDto } from "../../shared/dto/id.dto" export class ApplicationFlaggedSetDto extends OmitType(ApplicationFlaggedSet, [ @@ -27,29 +26,3 @@ export class ApplicationFlaggedSetDto extends OmitType(ApplicationFlaggedSet, [ @Type(() => IdDto) listing: IdDto } - -export class ApplicationFlaggedSetPaginationMeta extends PaginationMeta { - @Expose() - totalFlagged: number -} - -export class PaginatedApplicationFlaggedSetDto extends PaginationFactory( - ApplicationFlaggedSetDto -) { - @Expose() - meta: ApplicationFlaggedSetPaginationMeta -} - -export class ApplicationFlaggedSetResolveDto { - @Expose() - @IsUUID(4, { groups: [ValidationsGroupsEnum.default] }) - afsId: string - - @Expose() - @IsDefined({ groups: [ValidationsGroupsEnum.default] }) - @IsArray({ groups: [ValidationsGroupsEnum.default] }) - @ArrayMaxSize(512, { groups: [ValidationsGroupsEnum.default] }) - @ValidateNested({ groups: [ValidationsGroupsEnum.default], each: true }) - @Type(() => IdDto) - applications: IdDto[] -} diff --git a/backend/core/src/application-flagged-sets/dto/paginated-application-flagged-set.dto.ts b/backend/core/src/application-flagged-sets/dto/paginated-application-flagged-set.dto.ts new file mode 100644 index 0000000000..6a352fc361 --- /dev/null +++ b/backend/core/src/application-flagged-sets/dto/paginated-application-flagged-set.dto.ts @@ -0,0 +1,11 @@ +import { PaginationFactory } from "../../shared/dto/pagination.dto" +import { Expose } from "class-transformer" +import { ApplicationFlaggedSetPaginationMeta } from "./application-flagged-set-pagination-meta" +import { ApplicationFlaggedSetDto } from "./application-flagged-set.dto" + +export class PaginatedApplicationFlaggedSetDto extends PaginationFactory( + ApplicationFlaggedSetDto +) { + @Expose() + meta: ApplicationFlaggedSetPaginationMeta +} diff --git a/backend/core/src/application-flagged-sets/entities/application-flagged-set.entity.ts b/backend/core/src/application-flagged-sets/entities/application-flagged-set.entity.ts index c0463f767d..c3568d7b87 100644 --- a/backend/core/src/application-flagged-sets/entities/application-flagged-set.entity.ts +++ b/backend/core/src/application-flagged-sets/entities/application-flagged-set.entity.ts @@ -24,7 +24,7 @@ export class ApplicationFlaggedSet extends AbstractEntity { @Type(() => Date) resolvedTime?: Date | null - @ManyToOne(() => User, { eager: true, nullable: true, cascade: true }) + @ManyToOne(() => User, { eager: true, nullable: true, cascade: false }) @JoinColumn() @Expose() @ValidateNested({ groups: [ValidationsGroupsEnum.default] }) diff --git a/backend/core/src/application-flagged-sets/paginated-application-flagged-set-query-params.ts b/backend/core/src/application-flagged-sets/paginated-application-flagged-set-query-params.ts new file mode 100644 index 0000000000..8068126294 --- /dev/null +++ b/backend/core/src/application-flagged-sets/paginated-application-flagged-set-query-params.ts @@ -0,0 +1,16 @@ +import { PaginationQueryParams } from "../shared/dto/pagination.dto" +import { Expose } from "class-transformer" +import { ApiProperty } from "@nestjs/swagger" +import { IsUUID } from "class-validator" +import { ValidationsGroupsEnum } from "../shared/types/validations-groups-enum" + +export class PaginatedApplicationFlaggedSetQueryParams extends PaginationQueryParams { + @Expose() + @ApiProperty({ + type: String, + example: "listingId", + required: true, + }) + @IsUUID(4, { groups: [ValidationsGroupsEnum.default] }) + listingId: string +} diff --git a/backend/core/test/afs/afs.e2e-spec.ts b/backend/core/test/afs/afs.e2e-spec.ts index 176e1b73ec..36285b60b3 100644 --- a/backend/core/test/afs/afs.e2e-spec.ts +++ b/backend/core/test/afs/afs.e2e-spec.ts @@ -20,6 +20,7 @@ import { ApplicationFlaggedSet } from "../../src/application-flagged-sets/entiti import { getTestAppBody } from "../lib/get-test-app-body" import { FlaggedSetStatus } from "../../src/application-flagged-sets/types/flagged-set-status-enum" import { Rule } from "../../src/application-flagged-sets/types/rule-enum" +import { ApplicationDto } from "../../src/applications/dto/application.dto" // Cypress brings in Chai types for the global expect, but we want to use jest // expect here so we need to re-declare it. @@ -34,6 +35,8 @@ describe("ApplicationFlaggedSets", () => { let afsRepository: Repository let householdMembersRepository: Repository let listing1Id: string + let updateApplication + let getAfsesForListingId const setupDb = async () => { await householdMembersRepository.createQueryBuilder().delete().execute() @@ -79,6 +82,24 @@ describe("ApplicationFlaggedSets", () => { const listings = await supertest(app.getHttpServer()).get("/listings").expect(200) listing1Id = listings.body.items[0].id await setupDb() + + updateApplication = async (application: ApplicationDto) => { + return ( + await supertest(app.getHttpServer()) + .put(`/applications/${application.id}`) + .send(application) + .set(...setAuthorization(adminAccessToken)) + .expect(200) + ).body + } + + getAfsesForListingId = async (listingId) => { + return ( + await supertest(app.getHttpServer()) + .get(`/applicationFlaggedSets?listingId=${listingId}`) + .set(...setAuthorization(adminAccessToken)) + ).body + } }) it(`should mark two similar application as flagged`, async () => { @@ -184,6 +205,166 @@ describe("ApplicationFlaggedSets", () => { expect(resolvedAfs.applications.filter((app) => app.markedAsDuplicate === false).length).toBe(1) }) + it(`should take application edits into account (application toggles between conflicting and non conflicting in an AFS of 2 apps)`, async () => { + const app1Seed = getTestAppBody(listing1Id) + const app2Seed = getTestAppBody(listing1Id) + + // Two applications do not conflict by any rule at this point + app2Seed.applicant.emailAddress = "another@email.com" + app2Seed.applicant.firstName = "AnotherFirstName" + const apps = [] + + await Promise.all( + [app1Seed, app2Seed].map(async (payload) => { + const appRes = await supertest(app.getHttpServer()) + .post("/applications/submit") + .send(payload) + .expect(201) + apps.push(appRes.body) + }) + ) + + let afses = await getAfsesForListingId(listing1Id) + + expect(afses.body.meta.totalFlagged).toBe(0) + expect(afses.body.items.length).toBe(0) + + const [app1, app2] = apps + + // Applications conflict by email rule + app2.applicant.emailAddress = app1Seed.applicant.emailAddress + await updateApplication(app2) + + afses = await getAfsesForListingId(listing1Id) + + expect(afses.body.meta.totalFlagged).toBe(1) + expect(afses.body.items[0].applications.map((app) => app.id)).toContain(app2.id) + expect(afses.body.items[0].applications.map((app) => app.id)).toContain(app1.id) + + // Applications do not conflict by any rule + app2.applicant.emailAddress = app2Seed.applicant.emailAddress + await updateApplication(app2) + + afses = await getAfsesForListingId(listing1Id) + + expect(afses.body.meta.totalFlagged).toBe(0) + expect(afses.body.items.length).toBe(0) + }) + + it(`should take application edits into account (application toggles between conflicting and non conflicting in an AFS of 3 apps)`, async () => { + const app1Seed = getTestAppBody(listing1Id) + const app2Seed = getTestAppBody(listing1Id) + const app3Seed = getTestAppBody(listing1Id) + + // Three applications do not conflict by any rule at this point + app2Seed.applicant.emailAddress = "another@email.com" + app2Seed.applicant.firstName = "AnotherFirstName" + app3Seed.applicant.emailAddress = "third@email.com" + app3Seed.applicant.firstName = "ThirdFirstName" + const apps = [] + + await Promise.all( + [app1Seed, app2Seed, app3Seed].map(async (payload) => { + const appRes = await supertest(app.getHttpServer()) + .post("/applications/submit") + .send(payload) + .expect(201) + apps.push(appRes.body) + }) + ) + + let afses = await getAfsesForListingId(listing1Id) + + expect(afses.body.meta.totalFlagged).toBe(0) + expect(afses.body.items.length).toBe(0) + + // eslint-disable-next-line + let [app1, app2, app3] = apps + + // Applications conflict by email rule + app2.applicant.emailAddress = app1Seed.applicant.emailAddress + app3.applicant.emailAddress = app1Seed.applicant.emailAddress + await updateApplication(app2) + app3 = await updateApplication(app3) + expect(app3.flagged).toBe(true) + + afses = await getAfsesForListingId(listing1Id) + + expect(afses.body.meta.totalFlagged).toBe(1) + expect(afses.body.items[0].applications.map((app) => app.id)).toContain(app1.id) + expect(afses.body.items[0].applications.map((app) => app.id)).toContain(app2.id) + expect(afses.body.items[0].applications.map((app) => app.id)).toContain(app3.id) + + // Application 3 do not conflict with others now + app3.applicant.emailAddress = app3Seed.applicant.emailAddress + app3 = await updateApplication(app3) + expect(app3.flagged).toBe(false) + + afses = await getAfsesForListingId(listing1Id) + + expect(afses.body.meta.totalFlagged).toBe(1) + expect(afses.body.items.length).toBe(1) + expect(afses.body.items[0].applications.map((app) => app.id)).not.toContain(app3.id) + }) + + it(`should take application edits into account (application toggles between conflicting and non conflicting in an AFS of 3 apps, AFS already resolved)`, async () => { + const app1Seed = getTestAppBody(listing1Id) + const app2Seed = getTestAppBody(listing1Id) + const app3Seed = getTestAppBody(listing1Id) + + // Three applications conflict by email rule + app2Seed.applicant.firstName = "AnotherFirstName" + app3Seed.applicant.firstName = "ThirdFirstName" + const apps = [] + + await Promise.all( + [app1Seed, app2Seed, app3Seed].map(async (payload) => { + const appRes = await supertest(app.getHttpServer()) + .post("/applications/submit") + .send(payload) + .expect(201) + apps.push(appRes.body) + }) + ) + + // eslint-disable-next-line + let [app1, app2, app3] = apps + expect(app3.markedAsDuplicate).toBe(false) + + const afses = await getAfsesForListingId(listing1Id) + const afsToBeResolved = afses.items[0] + + const resolveRes = await supertest(app.getHttpServer()) + .post(`/applicationFlaggedSets/resolve`) + .send({ afsId: afsToBeResolved.id, applications: [{ id: app3.id }] }) + .set(...setAuthorization(adminAccessToken)) + .expect(201) + expect(resolveRes.body.resolvedTime).not.toBe(null) + expect(resolveRes.body.resolvingUser).not.toBe(null) + expect(resolveRes.body.status).toBe(FlaggedSetStatus.resolved) + + app3 = await updateApplication(app3) + expect(app3.markedAsDuplicate).toBe(true) + + // App3 now does not conflict with any other applications + app3.applicant.emailAddress = "third@email.com" + app3 = await updateApplication(app3) + expect(app3.markedAsDuplicate).toBe(false) + expect(app3.flagged).toBe(false) + + const previouslyResolvedAfs = ( + await supertest(app.getHttpServer()) + .get(`/applicationFlaggedSets/${afsToBeResolved.id}`) + .set(...setAuthorization(adminAccessToken)) + ).body + expect(resolveRes.body.resolvedTime).toBe(null) + expect(resolveRes.body.resolvingUser).toBe(null) + expect(previouslyResolvedAfs.status).toBe(FlaggedSetStatus.flagged) + expect(previouslyResolvedAfs.applications.map((app) => app.id)).toContain(app1.id) + expect(previouslyResolvedAfs.applications.map((app) => app.id)).toContain(app2.id) + expect(previouslyResolvedAfs.applications.map((app) => app.id)).not.toContain(app3.id) + }) + afterEach(async () => { await setupDb() jest.clearAllMocks() From ed7e4ca4bcb7514945e283bd4d942a4f9eb71be8 Mon Sep 17 00:00:00 2001 From: Michal Plebanski Date: Thu, 9 Sep 2021 17:29:00 +0200 Subject: [PATCH 06/10] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 45612b2c9d..179bec81f2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -124,6 +124,7 @@ All notable changes to this project will be documented in this file. The format - Fixes flakiness in authz.e2e-spec.ts related to logged in user trying to GET /applications which did not belong to him (sorting of UUID is not deterministic, so the user should fetch by specying a query param userId = self) [#1575](https://github.com/bloom-housing/bloom/pull/1575) - Fixed ListingsService.retrieve `view` query param not being optional in autogenerated client (it should be) [#1575](https://github.com/bloom-housing/bloom/pull/1575) - updated DTOs to omit entities and use DTOs for application-method, user-roles, user, listing and units-summary ([#1679](https://github.com/bloom-housing/bloom/pull/1679)) + - makes application flagged sets module take applications edits into account (e.g. a leasing agent changes something in the application) ([#1810](https://github.com/bloom-housing/bloom/pull/1810)) ### General From b98daa95501ca31e4c06620d9a9984dc6412ee40 Mon Sep 17 00:00:00 2001 From: Michal Plebanski Date: Tue, 14 Sep 2021 16:24:40 +0200 Subject: [PATCH 07/10] Add constraint that leasing agent can only resolve AFS when listing is closed --- .../application-flagged-sets.service.ts | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/backend/core/src/application-flagged-sets/application-flagged-sets.service.ts b/backend/core/src/application-flagged-sets/application-flagged-sets.service.ts index 4caceddaca..d412843fb4 100644 --- a/backend/core/src/application-flagged-sets/application-flagged-sets.service.ts +++ b/backend/core/src/application-flagged-sets/application-flagged-sets.service.ts @@ -1,14 +1,14 @@ -import { Inject, Injectable, NotFoundException, Scope } from "@nestjs/common" +import { BadRequestException, Inject, Injectable, NotFoundException, Scope } from "@nestjs/common" import { AuthzService } from "../auth/services/authz.service" import { ApplicationFlaggedSet } from "./entities/application-flagged-set.entity" import { InjectRepository } from "@nestjs/typeorm" import { Brackets, DeepPartial, + EntityManager, + getManager, Repository, SelectQueryBuilder, - getManager, - EntityManager, } from "typeorm" import { paginate } from "nestjs-typeorm-paginate" import { Application } from "../applications/entities/application.entity" @@ -19,6 +19,7 @@ import { FlaggedSetStatus } from "./types/flagged-set-status-enum" import { Rule } from "./types/rule-enum" import { ApplicationFlaggedSetResolveDto } from "./dto/application-flagged-set-resolve.dto" import { PaginatedApplicationFlaggedSetQueryParams } from "./paginated-application-flagged-set-query-params" +import { ListingStatus } from "../listings/types/listing-status-enum" @Injectable({ scope: Scope.REQUEST }) export class ApplicationFlaggedSetsService { @@ -71,11 +72,16 @@ export class ApplicationFlaggedSetsService { const transApplicationsRepository = transactionalEntityManager.getRepository(Application) const afs = await transAfsRepository.findOne({ where: { id: dto.afsId }, - relations: ["applications"], + relations: ["applications", "listing"], }) if (!afs) { throw new NotFoundException() } + + if (afs.listing.status !== ListingStatus.closed) { + throw new BadRequestException("Listing must be closed before resolving any duplicates.") + } + // eslint-disable-next-line @typescript-eslint/no-unnecessary-type-assertion afs.resolvingUser = this.request.user as User afs.resolvedTime = new Date() From eaa959a6f6fdebe7942ae5f0e261c01a6de37496 Mon Sep 17 00:00:00 2001 From: Michal Plebanski Date: Wed, 15 Sep 2021 15:41:16 +0200 Subject: [PATCH 08/10] Make application updates trigger AFS recalculations --- CHANGELOG.md | 1 + .../application-flagged-sets.service.ts | 40 ++++++ .../src/applications/applications.service.ts | 52 ++++--- backend/core/test/afs/afs.e2e-spec.ts | 132 ++++++++++-------- 4 files changed, 149 insertions(+), 76 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 179bec81f2..2aac659b6b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ All notable changes to this project will be documented in this file. The format - Add POST /users/invite endpoint and extend PUT /users/confirm with optional password change ([#1801](https://github.com/bloom-housing/bloom/pull/1801)) - Add `isPartner` filter to GET /user/list endpoint ([#1830](https://github.com/bloom-housing/bloom/pull/1830)) - Add logic for connecting newly created user account to existing applications (matching based on applicant.emailAddress) ([#1807](https://github.com/bloom-housing/bloom/pull/1807)) + - Changes to applications done through `PUT /applications/:id` are now reflected in AFS ([#1810](https://github.com/bloom-housing/bloom/pull/1810)) ## Frontend diff --git a/backend/core/src/application-flagged-sets/application-flagged-sets.service.ts b/backend/core/src/application-flagged-sets/application-flagged-sets.service.ts index d412843fb4..59cc5aea59 100644 --- a/backend/core/src/application-flagged-sets/application-flagged-sets.service.ts +++ b/backend/core/src/application-flagged-sets/application-flagged-sets.service.ts @@ -121,6 +121,46 @@ export class ApplicationFlaggedSetsService { } } + async onApplicationUpdate( + newApplication: Application, + transactionalEntityManager: EntityManager + ) { + const transApplicationsRepository = transactionalEntityManager.getRepository(Application) + newApplication.markedAsDuplicate = false + await transApplicationsRepository.save(newApplication) + + const transAfsRepository = transactionalEntityManager.getRepository(ApplicationFlaggedSet) + let afses = await transAfsRepository + .createQueryBuilder("afs") + .leftJoinAndSelect("afs.applications", "applications") + .leftJoinAndSelect("afs.listing", "listing") + .where("listing.id = :listingId", { listingId: newApplication.listing.id }) + .getMany() + afses = afses.filter( + (afs) => afs.applications.findIndex((app) => app.id === newApplication.id) !== -1 + ) + const afsesToBeSaved: Array = [] + const afsesToBeRemoved: Array = [] + for (const afs of afses) { + afs.status = FlaggedSetStatus.flagged + afs.resolvedTime = null + afs.resolvingUser = null + const applicationIndex = afs.applications.findIndex( + (application) => application.id === newApplication.id + ) + afs.applications.splice(applicationIndex, 1) + if (afs.applications.length > 1) { + afsesToBeSaved.push(afs) + } else { + afsesToBeRemoved.push(afs) + } + } + await transAfsRepository.save(afsesToBeSaved) + await transAfsRepository.remove(afsesToBeRemoved) + + await this.onApplicationSave(newApplication, transactionalEntityManager) + } + async fetchDuplicatesMatchingRule( transactionalEntityManager: EntityManager, application: Application, diff --git a/backend/core/src/applications/applications.service.ts b/backend/core/src/applications/applications.service.ts index 7aa8ead8a3..9ffc688957 100644 --- a/backend/core/src/applications/applications.service.ts +++ b/backend/core/src/applications/applications.service.ts @@ -9,7 +9,7 @@ import { import { Application } from "./entities/application.entity" import { ApplicationCreateDto, ApplicationUpdateDto } from "./dto/application.dto" import { InjectRepository } from "@nestjs/typeorm" -import { getManager, QueryFailedError, Repository } from "typeorm" +import { QueryFailedError, Repository } from "typeorm" import { paginate, Pagination } from "nestjs-typeorm-paginate" import { PaginatedApplicationListQueryParams } from "./applications.controller" import { ApplicationFlaggedSetsService } from "../application-flagged-sets/application-flagged-sets.service" @@ -100,8 +100,7 @@ export class ApplicationsService { throw new BadRequestException("Listing is not open for application submission.") } await this.authorizeUserAction(this.req.user, applicationCreateDto, authzActions.submit) - const application = await this._create(applicationCreateDto) - return application + return await this._create(applicationCreateDto) } async create(applicationCreateDto: ApplicationCreateDto) { @@ -131,8 +130,19 @@ export class ApplicationsService { id: application.id, }) - await this.repository.save(application) - return application + return await this.repository.manager.transaction( + "SERIALIZABLE", + async (transactionalEntityManager) => { + const applicationsRepository = transactionalEntityManager.getRepository(Application) + const newApplication = await applicationsRepository.save(application) + await this.applicationFlaggedSetsService.onApplicationUpdate( + application, + transactionalEntityManager + ) + + return await applicationsRepository.findOne({ id: newApplication.id }) + } + ) } async delete(applicationId: string) { @@ -191,18 +201,22 @@ export class ApplicationsService { } private async _createApplication(applicationCreateDto: ApplicationUpdateDto) { - return await getManager().transaction("SERIALIZABLE", async (transactionalEntityManager) => { - const applicationsRepository = transactionalEntityManager.getRepository(Application) - const application = await applicationsRepository.save({ - ...applicationCreateDto, - user: this.req.user, - }) - await this.applicationFlaggedSetsService.onApplicationSave( - application, - transactionalEntityManager - ) - return application - }) + return await this.repository.manager.transaction( + "SERIALIZABLE", + async (transactionalEntityManager) => { + const applicationsRepository = transactionalEntityManager.getRepository(Application) + const application = await applicationsRepository.save({ + ...applicationCreateDto, + user: this.req.user, + }) + await this.applicationFlaggedSetsService.onApplicationSave( + application, + transactionalEntityManager + ) + + return await applicationsRepository.findOne({ id: application.id }) + } + ) } private async _create(applicationCreateDto: ApplicationUpdateDto) { @@ -241,7 +255,9 @@ export class ApplicationsService { } } - const listing = await this.listingsService.findOne(application.listing.id) + // Listing is not eagerly joined on application entity so let's use the one provided with + // create dto + const listing = await this.listingsService.findOne(applicationCreateDto.listing.id) if (application.applicant.emailAddress) { await this.emailService.confirmation(listing, application, applicationCreateDto.appUrl) } diff --git a/backend/core/test/afs/afs.e2e-spec.ts b/backend/core/test/afs/afs.e2e-spec.ts index 36285b60b3..10269fda0e 100644 --- a/backend/core/test/afs/afs.e2e-spec.ts +++ b/backend/core/test/afs/afs.e2e-spec.ts @@ -9,9 +9,6 @@ import { ListingsModule } from "../../src/listings/listings.module" import { EmailService } from "../../src/shared/email/email.service" import { getUserAccessToken } from "../utils/get-user-access-token" import { setAuthorization } from "../utils/set-authorization-helper" -// Use require because of the CommonJS/AMD style export. -// See https://www.typescriptlang.org/docs/handbook/modules.html#export--and-import--require -import dbOptions = require("../../ormconfig.test") import { Repository } from "typeorm" import { Application } from "../../src/applications/entities/application.entity" import { HouseholdMember } from "../../src/applications/entities/household-member.entity" @@ -21,6 +18,11 @@ import { getTestAppBody } from "../lib/get-test-app-body" import { FlaggedSetStatus } from "../../src/application-flagged-sets/types/flagged-set-status-enum" import { Rule } from "../../src/application-flagged-sets/types/rule-enum" import { ApplicationDto } from "../../src/applications/dto/application.dto" +import { Listing } from "../../src/listings/entities/listing.entity" +import { ListingStatus } from "../../src/listings/types/listing-status-enum" +// Use require because of the CommonJS/AMD style export. +// See https://www.typescriptlang.org/docs/handbook/modules.html#export--and-import--require +import dbOptions = require("../../ormconfig.test") // Cypress brings in Chai types for the global expect, but we want to use jest // expect here so we need to re-declare it. @@ -34,8 +36,10 @@ describe("ApplicationFlaggedSets", () => { let applicationsRepository: Repository let afsRepository: Repository let householdMembersRepository: Repository + let listingsRepository: Repository let listing1Id: string let updateApplication + let getApplication let getAfsesForListingId const setupDb = async () => { @@ -56,7 +60,7 @@ describe("ApplicationFlaggedSets", () => { AuthModule, ListingsModule, ApplicationsModule, - TypeOrmModule.forFeature([ApplicationFlaggedSet, Application, HouseholdMember]), + TypeOrmModule.forFeature([ApplicationFlaggedSet, Application, HouseholdMember, Listing]), ThrottlerModule.forRoot({ ttl: 60, limit: 5, @@ -77,10 +81,15 @@ describe("ApplicationFlaggedSets", () => { householdMembersRepository = app.get>( getRepositoryToken(HouseholdMember) ) + listingsRepository = app.get>(getRepositoryToken(Listing)) + const listing = (await listingsRepository.find({ take: 1 }))[0] + await listingsRepository.save({ + ...listing, + status: ListingStatus.closed, + }) adminAccessToken = await getUserAccessToken(app, "admin@example.com", "abcdef") - const listings = await supertest(app.getHttpServer()).get("/listings").expect(200) - listing1Id = listings.body.items[0].id + listing1Id = listing.id await setupDb() updateApplication = async (application: ApplicationDto) => { @@ -93,11 +102,21 @@ describe("ApplicationFlaggedSets", () => { ).body } + getApplication = async (id: string) => { + return ( + await supertest(app.getHttpServer()) + .get(`/applications/${id}`) + .set(...setAuthorization(adminAccessToken)) + .expect(200) + ).body + } + getAfsesForListingId = async (listingId) => { return ( await supertest(app.getHttpServer()) .get(`/applicationFlaggedSets?listingId=${listingId}`) .set(...setAuthorization(adminAccessToken)) + .expect(200) ).body } }) @@ -214,20 +233,18 @@ describe("ApplicationFlaggedSets", () => { app2Seed.applicant.firstName = "AnotherFirstName" const apps = [] - await Promise.all( - [app1Seed, app2Seed].map(async (payload) => { - const appRes = await supertest(app.getHttpServer()) - .post("/applications/submit") - .send(payload) - .expect(201) - apps.push(appRes.body) - }) - ) + for (const payload of [app1Seed, app2Seed]) { + const appRes = await supertest(app.getHttpServer()) + .post("/applications/submit") + .send(payload) + .expect(201) + apps.push(appRes.body) + } let afses = await getAfsesForListingId(listing1Id) - expect(afses.body.meta.totalFlagged).toBe(0) - expect(afses.body.items.length).toBe(0) + expect(afses.meta.totalFlagged).toBe(0) + expect(afses.items.length).toBe(0) const [app1, app2] = apps @@ -237,9 +254,9 @@ describe("ApplicationFlaggedSets", () => { afses = await getAfsesForListingId(listing1Id) - expect(afses.body.meta.totalFlagged).toBe(1) - expect(afses.body.items[0].applications.map((app) => app.id)).toContain(app2.id) - expect(afses.body.items[0].applications.map((app) => app.id)).toContain(app1.id) + expect(afses.meta.totalFlagged).toBe(1) + expect(afses.items[0].applications.map((app) => app.id).includes(app1.id)).toBe(true) + expect(afses.items[0].applications.map((app) => app.id).includes(app2.id)).toBe(true) // Applications do not conflict by any rule app2.applicant.emailAddress = app2Seed.applicant.emailAddress @@ -247,8 +264,8 @@ describe("ApplicationFlaggedSets", () => { afses = await getAfsesForListingId(listing1Id) - expect(afses.body.meta.totalFlagged).toBe(0) - expect(afses.body.items.length).toBe(0) + expect(afses.meta.totalFlagged).toBe(0) + expect(afses.items.length).toBe(0) }) it(`should take application edits into account (application toggles between conflicting and non conflicting in an AFS of 3 apps)`, async () => { @@ -263,20 +280,18 @@ describe("ApplicationFlaggedSets", () => { app3Seed.applicant.firstName = "ThirdFirstName" const apps = [] - await Promise.all( - [app1Seed, app2Seed, app3Seed].map(async (payload) => { - const appRes = await supertest(app.getHttpServer()) - .post("/applications/submit") - .send(payload) - .expect(201) - apps.push(appRes.body) - }) - ) + for (const payload of [app1Seed, app2Seed, app3Seed]) { + const appRes = await supertest(app.getHttpServer()) + .post("/applications/submit") + .send(payload) + .expect(201) + apps.push(appRes.body) + } let afses = await getAfsesForListingId(listing1Id) - expect(afses.body.meta.totalFlagged).toBe(0) - expect(afses.body.items.length).toBe(0) + expect(afses.meta.totalFlagged).toBe(0) + expect(afses.items.length).toBe(0) // eslint-disable-next-line let [app1, app2, app3] = apps @@ -286,25 +301,23 @@ describe("ApplicationFlaggedSets", () => { app3.applicant.emailAddress = app1Seed.applicant.emailAddress await updateApplication(app2) app3 = await updateApplication(app3) - expect(app3.flagged).toBe(true) afses = await getAfsesForListingId(listing1Id) - expect(afses.body.meta.totalFlagged).toBe(1) - expect(afses.body.items[0].applications.map((app) => app.id)).toContain(app1.id) - expect(afses.body.items[0].applications.map((app) => app.id)).toContain(app2.id) - expect(afses.body.items[0].applications.map((app) => app.id)).toContain(app3.id) + expect(afses.meta.totalFlagged).toBe(1) + expect(afses.items[0].applications.map((app) => app.id).includes(app1.id)).toBe(true) + expect(afses.items[0].applications.map((app) => app.id).includes(app2.id)).toBe(true) + expect(afses.items[0].applications.map((app) => app.id).includes(app3.id)).toBe(true) // Application 3 do not conflict with others now app3.applicant.emailAddress = app3Seed.applicant.emailAddress app3 = await updateApplication(app3) - expect(app3.flagged).toBe(false) afses = await getAfsesForListingId(listing1Id) - expect(afses.body.meta.totalFlagged).toBe(1) - expect(afses.body.items.length).toBe(1) - expect(afses.body.items[0].applications.map((app) => app.id)).not.toContain(app3.id) + expect(afses.meta.totalFlagged).toBe(1) + expect(afses.items.length).toBe(1) + expect(afses.items[0].applications.map((app) => app.id).includes(app3.id)).toBe(false) }) it(`should take application edits into account (application toggles between conflicting and non conflicting in an AFS of 3 apps, AFS already resolved)`, async () => { @@ -317,15 +330,13 @@ describe("ApplicationFlaggedSets", () => { app3Seed.applicant.firstName = "ThirdFirstName" const apps = [] - await Promise.all( - [app1Seed, app2Seed, app3Seed].map(async (payload) => { - const appRes = await supertest(app.getHttpServer()) - .post("/applications/submit") - .send(payload) - .expect(201) - apps.push(appRes.body) - }) - ) + for (const payload of [app1Seed, app2Seed, app3Seed]) { + const appRes = await supertest(app.getHttpServer()) + .post("/applications/submit") + .send(payload) + .expect(201) + apps.push(appRes.body) + } // eslint-disable-next-line let [app1, app2, app3] = apps @@ -343,26 +354,26 @@ describe("ApplicationFlaggedSets", () => { expect(resolveRes.body.resolvingUser).not.toBe(null) expect(resolveRes.body.status).toBe(FlaggedSetStatus.resolved) - app3 = await updateApplication(app3) + app3 = await getApplication(app3.id) expect(app3.markedAsDuplicate).toBe(true) // App3 now does not conflict with any other applications app3.applicant.emailAddress = "third@email.com" app3 = await updateApplication(app3) expect(app3.markedAsDuplicate).toBe(false) - expect(app3.flagged).toBe(false) const previouslyResolvedAfs = ( await supertest(app.getHttpServer()) .get(`/applicationFlaggedSets/${afsToBeResolved.id}`) .set(...setAuthorization(adminAccessToken)) + .expect(200) ).body - expect(resolveRes.body.resolvedTime).toBe(null) - expect(resolveRes.body.resolvingUser).toBe(null) + expect(previouslyResolvedAfs.resolvedTime).toBe(null) + expect(previouslyResolvedAfs.resolvingUser).toBe(null) expect(previouslyResolvedAfs.status).toBe(FlaggedSetStatus.flagged) - expect(previouslyResolvedAfs.applications.map((app) => app.id)).toContain(app1.id) - expect(previouslyResolvedAfs.applications.map((app) => app.id)).toContain(app2.id) - expect(previouslyResolvedAfs.applications.map((app) => app.id)).not.toContain(app3.id) + expect(previouslyResolvedAfs.applications.map((app) => app.id).includes(app1.id)).toBe(true) + expect(previouslyResolvedAfs.applications.map((app) => app.id).includes(app2.id)).toBe(true) + expect(previouslyResolvedAfs.applications.map((app) => app.id).includes(app3.id)).toBe(false) }) afterEach(async () => { @@ -371,6 +382,11 @@ describe("ApplicationFlaggedSets", () => { }) afterAll(async () => { + const modifiedListing = await listingsRepository.findOne({ id: listing1Id }) + await listingsRepository.save({ + ...modifiedListing, + status: ListingStatus.active, + }) await app.close() }) }) From 87d4ff179dbfd799ed5ffd164dbdf849b1614bce Mon Sep 17 00:00:00 2001 From: Michal Plebanski Date: Tue, 28 Sep 2021 14:37:05 +0200 Subject: [PATCH 09/10] Improve query efficiency in onApplicationUpdate AFS module --- .../application-flagged-sets.service.ts | 42 +++++++++++++++---- .../application-flagged-set.entity.ts | 2 +- 2 files changed, 35 insertions(+), 9 deletions(-) diff --git a/backend/core/src/application-flagged-sets/application-flagged-sets.service.ts b/backend/core/src/application-flagged-sets/application-flagged-sets.service.ts index 59cc5aea59..8cdfb7db04 100644 --- a/backend/core/src/application-flagged-sets/application-flagged-sets.service.ts +++ b/backend/core/src/application-flagged-sets/application-flagged-sets.service.ts @@ -7,6 +7,9 @@ import { DeepPartial, EntityManager, getManager, + getMetadataArgsStorage, + In, + QueryRunner, Repository, SelectQueryBuilder, } from "typeorm" @@ -121,6 +124,29 @@ export class ApplicationFlaggedSetsService { } } + private async _getAfsesContainingApplicationId( + queryRunnery: QueryRunner, + applicationId: string + ): Promise> { + const metadataArgsStorage = getMetadataArgsStorage().findJoinTable( + ApplicationFlaggedSet, + "applications" + ) + const applicationsJunctionTableName = metadataArgsStorage.name + const afsMetadata = queryRunnery.connection.getMetadata(ApplicationFlaggedSet) + const applicationsMetadata = queryRunnery.connection.getMetadata(Application) + const query = ` + SELECT DISTINCT afs.id FROM ${afsMetadata.tableName} afs + INNER JOIN ( + SELECT DISTINCT application_flagged_set_id, applications_id FROM ${applicationsJunctionTableName} + WHERE ${applicationsJunctionTableName}.applications_id = $1 + ) matching_afs ON afs.id = matching_afs.application_flagged_set_id + LEFT JOIN ${applicationsJunctionTableName} ON matching_afs.application_flagged_set_id = ${applicationsJunctionTableName}.application_flagged_set_id + LEFT JOIN ${applicationsMetadata.tableName} applications ON applications.id = ${applicationsJunctionTableName}.applications_id + ` + return await queryRunnery.query(query, [applicationId]) + } + async onApplicationUpdate( newApplication: Application, transactionalEntityManager: EntityManager @@ -130,15 +156,15 @@ export class ApplicationFlaggedSetsService { await transApplicationsRepository.save(newApplication) const transAfsRepository = transactionalEntityManager.getRepository(ApplicationFlaggedSet) - let afses = await transAfsRepository - .createQueryBuilder("afs") - .leftJoinAndSelect("afs.applications", "applications") - .leftJoinAndSelect("afs.listing", "listing") - .where("listing.id = :listingId", { listingId: newApplication.listing.id }) - .getMany() - afses = afses.filter( - (afs) => afs.applications.findIndex((app) => app.id === newApplication.id) !== -1 + + const afsIds = await this._getAfsesContainingApplicationId( + transAfsRepository.queryRunner, + newApplication.id ) + const afses = await transAfsRepository.find({ + where: { id: In(afsIds.map((afs) => afs.id)) }, + relations: ["applications"], + }) const afsesToBeSaved: Array = [] const afsesToBeRemoved: Array = [] for (const afs of afses) { diff --git a/backend/core/src/application-flagged-sets/entities/application-flagged-set.entity.ts b/backend/core/src/application-flagged-sets/entities/application-flagged-set.entity.ts index c3568d7b87..3f7c36ef73 100644 --- a/backend/core/src/application-flagged-sets/entities/application-flagged-set.entity.ts +++ b/backend/core/src/application-flagged-sets/entities/application-flagged-set.entity.ts @@ -37,7 +37,7 @@ export class ApplicationFlaggedSet extends AbstractEntity { status: FlaggedSetStatus @ManyToMany(() => Application) - @JoinTable() + @JoinTable({ name: "application_flagged_set_applications_applications" }) @Expose() @ValidateNested({ groups: [ValidationsGroupsEnum.default], each: true }) applications: Application[] From 38ecb81653a7e825f9b38e443521c84d5972d62c Mon Sep 17 00:00:00 2001 From: Michal Plebanski Date: Wed, 29 Sep 2021 16:55:52 +0200 Subject: [PATCH 10/10] refactor(backend): simplify AFS filtering by applicationId query --- .../application-flagged-sets.service.ts | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/backend/core/src/application-flagged-sets/application-flagged-sets.service.ts b/backend/core/src/application-flagged-sets/application-flagged-sets.service.ts index 8cdfb7db04..0388288756 100644 --- a/backend/core/src/application-flagged-sets/application-flagged-sets.service.ts +++ b/backend/core/src/application-flagged-sets/application-flagged-sets.service.ts @@ -127,22 +127,15 @@ export class ApplicationFlaggedSetsService { private async _getAfsesContainingApplicationId( queryRunnery: QueryRunner, applicationId: string - ): Promise> { + ): Promise> { const metadataArgsStorage = getMetadataArgsStorage().findJoinTable( ApplicationFlaggedSet, "applications" ) const applicationsJunctionTableName = metadataArgsStorage.name - const afsMetadata = queryRunnery.connection.getMetadata(ApplicationFlaggedSet) - const applicationsMetadata = queryRunnery.connection.getMetadata(Application) const query = ` - SELECT DISTINCT afs.id FROM ${afsMetadata.tableName} afs - INNER JOIN ( - SELECT DISTINCT application_flagged_set_id, applications_id FROM ${applicationsJunctionTableName} - WHERE ${applicationsJunctionTableName}.applications_id = $1 - ) matching_afs ON afs.id = matching_afs.application_flagged_set_id - LEFT JOIN ${applicationsJunctionTableName} ON matching_afs.application_flagged_set_id = ${applicationsJunctionTableName}.application_flagged_set_id - LEFT JOIN ${applicationsMetadata.tableName} applications ON applications.id = ${applicationsJunctionTableName}.applications_id + SELECT DISTINCT application_flagged_set_id FROM ${applicationsJunctionTableName} + WHERE applications_id = $1 ` return await queryRunnery.query(query, [applicationId]) } @@ -162,7 +155,7 @@ export class ApplicationFlaggedSetsService { newApplication.id ) const afses = await transAfsRepository.find({ - where: { id: In(afsIds.map((afs) => afs.id)) }, + where: { id: In(afsIds.map((afs) => afs.application_flagged_set_id)) }, relations: ["applications"], }) const afsesToBeSaved: Array = []