Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add initial filtering capability and tests to listings service #18

Merged
merged 16 commits into from
Jun 24, 2021
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions backend/core/src/listings/listings.controller.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import { Test, TestingModule } from "@nestjs/testing"
import { ListingsController } from "./listings.controller"
import { PassportModule } from "@nestjs/passport"
import { ListingsService } from "./listings.service"
import { AuthzService } from "../auth/authz.service"
import { CacheModule } from "@nestjs/common"

// Cypress brings in Chai types for the global expect, but we want to use jest
// expect here so we need to re-declare it.
// see: https://github.com/cypress-io/cypress/issues/1319#issuecomment-593500345
declare const expect: jest.Expect

describe("Listings Controller", () => {
let controller: ListingsController

beforeEach(async () => {
const module: TestingModule = await Test.createTestingModule({
imports: [
PassportModule,
CacheModule.register({
ttl: 60,
max: 2,
}),
],
providers: [
{ provide: AuthzService, useValue: {} },
{ provide: ListingsService, useValue: {} },
],
controllers: [ListingsController],
}).compile()

controller = module.get<ListingsController>(ListingsController)
})

it("should be defined", () => {
expect(controller).toBeDefined()
})
})
32 changes: 29 additions & 3 deletions backend/core/src/listings/listings.controller.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,40 @@ import {
ValidationPipe,
} from "@nestjs/common"
import { ListingsService } from "./listings.service"
import { ApiBearerAuth, ApiOperation, ApiTags } from "@nestjs/swagger"
import { ApiBearerAuth, ApiOperation, ApiProperty, ApiTags } from "@nestjs/swagger"
import { ListingCreateDto, ListingDto, ListingUpdateDto } from "./dto/listing.dto"
import { ResourceType } from "../auth/decorators/resource-type.decorator"
import { OptionalAuthGuard } from "../auth/guards/optional-auth.guard"
import { AuthzGuard } from "../auth/guards/authz.guard"
import { ApiImplicitQuery } from "@nestjs/swagger/dist/decorators/api-implicit-query.decorator"
import { mapTo } from "../shared/mapTo"
import { defaultValidationPipeOptions } from "../shared/default-validation-pipe-options"
import { Expose } from "class-transformer"
import { IsOptional, IsString } from "class-validator"
import { ValidationsGroupsEnum } from "../shared/types/validations-groups-enum"

//TODO extend query params to add paginations
export class ListingsListQueryParams {
@Expose()
@ApiProperty({
type: String,
example: "Fox Creek",
required: false,
description: "The neighborhood to filter by",
})
@IsOptional({ groups: [ValidationsGroupsEnum.default] })
@IsString({ groups: [ValidationsGroupsEnum.default] })
neighborhood?: string

@Expose()
@ApiProperty({
type: String,
required: false,
})
@IsOptional({ groups: [ValidationsGroupsEnum.default] })
@IsString({ groups: [ValidationsGroupsEnum.default] })
jsonpath?: string
}

@Controller("listings")
@ApiTags("listings")
Expand All @@ -40,8 +66,8 @@ export class ListingsController {
type: String,
})
@UseInterceptors(CacheInterceptor)
public async getAll(@Query("jsonpath") jsonpath?: string): Promise<ListingDto[]> {
return mapTo(ListingDto, await this.listingsService.list(jsonpath))
async list(@Query() queryParams: ListingsListQueryParams): Promise<ListingDto[]> {
return mapTo(ListingDto, await this.listingsService.list(queryParams))
}

@Post()
Expand Down
67 changes: 67 additions & 0 deletions backend/core/src/listings/listings.service.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
import { Test, TestingModule } from "@nestjs/testing"
import { ListingsService } from "./listings.service"
import { getRepositoryToken } from "@nestjs/typeorm"
import { Listing } from "./entities/listing.entity"

// Cypress brings in Chai types for the global expect, but we want to use jest
// expect here so we need to re-declare it.
// see: https://github.com/cypress-io/cypress/issues/1319#issuecomment-593500345
declare const expect: jest.Expect

let service: ListingsService
const mockListings = [{ id: "asdf1" }, { id: "asdf2" }]
const mockQueryBuilder = {
leftJoinAndSelect: jest.fn().mockReturnThis(),
orderBy: jest.fn().mockReturnThis(),
andWhere: jest.fn().mockReturnThis(),
getMany: jest.fn().mockReturnValue(mockListings),
}
const mockListingsRepo = { createQueryBuilder: jest.fn().mockReturnValue(mockQueryBuilder) }

describe("ListingsService", () => {
beforeEach(async () => {
process.env.APP_SECRET = "SECRET"
const module: TestingModule = await Test.createTestingModule({
providers: [
ListingsService,
{
provide: getRepositoryToken(Listing),
useValue: mockListingsRepo,
},
],
}).compile()

service = module.get(ListingsService)
})

afterEach(() => {
jest.clearAllMocks()
})

it("should be defined", () => {
expect(service).toBeDefined()
})

describe("getListingsList", () => {
it("should not add a WHERE clause if no filters are applied", async () => {
const listings = await service.list({})

expect(listings).toEqual(mockListings)
expect(mockQueryBuilder.andWhere).toHaveBeenCalledTimes(0)
})

it("should add a WHERE clause if the neighborhood filter is applied", async () => {
const expectedNeighborhood = "Fox Creek"

const listings = await service.list({ neighborhood: expectedNeighborhood })

expect(listings).toEqual(mockListings)
expect(mockQueryBuilder.andWhere).toHaveBeenCalledWith(
"property.neighborhood = :neighborhood",
{
neighborhood: expectedNeighborhood,
}
)
})
})
})
39 changes: 24 additions & 15 deletions backend/core/src/listings/listings.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,15 @@ import { Listing } from "./entities/listing.entity"
import { ListingCreateDto, ListingUpdateDto } from "./dto/listing.dto"
import { InjectRepository } from "@nestjs/typeorm"
import { Repository } from "typeorm"
import { ListingsListQueryParams } from "./listings.controller"

@Injectable()
export class ListingsService {
constructor(@InjectRepository(Listing) private readonly repository: Repository<Listing>) {}

private getQueryBuilder() {
return Listing.createQueryBuilder("listings")
return this.repository

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the switch here and elsewhere to use this.repository instead of Listing?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Listing is using the entity object directly, but this.repository is the injected entity. That lets me swap it with a mock in the tests.

.createQueryBuilder("listings")
.leftJoinAndSelect("listings.leasingAgents", "leasingAgents")
.leftJoinAndSelect("listings.preferences", "preferences")
.leftJoinAndSelect("listings.property", "property")
Expand All @@ -20,27 +22,34 @@ export class ListingsService {
.leftJoinAndSelect("units.amiChart", "amiChart")
}

public async list(jsonpath?: string): Promise<Listing[]> {
let listings = await this.getQueryBuilder()
.orderBy({
"listings.id": "DESC",
"units.max_occupancy": "ASC",
"preferences.ordinal": "ASC",
})
.getMany()
public async list(params: ListingsListQueryParams): Promise<Listing[]> {
const query = this.getQueryBuilder().orderBy({
"listings.id": "DESC",
"units.maxOccupancy": "ASC",
"preferences.ordinal": "ASC",
})

if (params.neighborhood) {
// This works because there's a 1:1 relationship between properties and listings.
// If that weren't true (for example, if we filtered on a unit's fields), we couldn't
// use this type of where clause.
query.andWhere("property.neighborhood = :neighborhood", { neighborhood: params.neighborhood })
}

let listings = await query.getMany()

if (jsonpath) {
listings = jp.query(listings, jsonpath)
if (params.jsonpath) {
listings = jp.query(listings, params.jsonpath)
}
return listings
}

async create(listingDto: ListingCreateDto) {
return Listing.save(listingDto)
return this.repository.save(listingDto)
}

async update(listingDto: ListingUpdateDto) {
const listing = await Listing.findOneOrFail({
const listing = await this.repository.findOneOrFail({
where: { id: listingDto.id },
relations: ["property"],
})
Expand All @@ -58,10 +67,10 @@ export class ListingsService {
}

async delete(listingId: string) {
const listing = await Listing.findOneOrFail({
const listing = await this.repository.findOneOrFail({
where: { id: listingId },
})
return await Listing.remove(listing)
return await this.repository.remove(listing)
}

async findOne(listingId: string) {
Expand Down