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

fix: listing list dupes in pagination #1140

Merged
merged 4 commits into from
Apr 14, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
1 change: 1 addition & 0 deletions backend/core/src/listings/dto/filter-type-to-field-map.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ type keysWithMappedField = Exclude<
>

export const filterTypeToFieldMap: Record<keysWithMappedField, string> = {
id: "listings.id",
status: "listings.status",
name: "listings.name",
isVerified: "listings.isVerified",
Expand Down
10 changes: 10 additions & 0 deletions backend/core/src/listings/dto/listing-filter-params.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,16 @@ import { Region } from "../../property/types/region-enum"

// add other listing filter params here
export class ListingFilterParams extends BaseFilter {
@Expose()
@ApiProperty({
type: String,
example: "FAB1A3C6-965E-4054-9A48-A282E92E9426",
required: false,
})
@IsOptional({ groups: [ValidationsGroupsEnum.default] })
@IsString({ groups: [ValidationsGroupsEnum.default] })
[ListingFilterKeys.id]?: string;

@Expose()
@ApiProperty({
type: String,
Expand Down
20 changes: 7 additions & 13 deletions backend/core/src/listings/listings.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,24 +51,18 @@ export class ListingsService {

public async list(params: ListingsQueryParams): Promise<Pagination<Listing>> {
const getOrderByCondition = (params: ListingsQueryParams): OrderByCondition => {
if (!params.orderBy) {
// Default to ordering by applicationDates (i.e. applicationDueDate
// and applicationOpenDate) if no orderBy param is specified.
return {
"listings.applicationDueDate": "ASC",
"listings.applicationOpenDate": "DESC",
}
}
switch (params.orderBy) {
case OrderByFieldsEnum.mostRecentlyUpdated:
return { "listings.updated_at": "DESC" }
case OrderByFieldsEnum.applicationDates:
return {
"listings.applicationDueDate": "ASC",
}
case undefined:
// Default to ordering by applicationDates (i.e. applicationDueDate
// and applicationOpenDate) if no orderBy param is specified.
return {
"listings.applicationDueDate": "ASC",
"listings.applicationOpenDate": "DESC",
"listings.name": "ASC",
Copy link
Collaborator

Choose a reason for hiding this comment

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

🥳

}
default:
throw new HttpException(
Expand All @@ -79,18 +73,18 @@ export class ListingsService {
}
}

const orderBy = getOrderByCondition(params)
// Inner query to get the sorted listing ids of the listings to display
// TODO(avaleske): Only join the tables we need for the filters that are applied.
const innerFilteredQuery = this.listingRepository
.createQueryBuilder("listings")
.select("listings.id", "listings_id")
.leftJoin("listings.property", "property")
.leftJoin("listings.leasingAgents", "leasingAgents")
.leftJoin("property.buildingAddress", "buildingAddress")
.leftJoin("listings.reservedCommunityType", "reservedCommunityType")
.leftJoin("listings.features", "listing_features")
.groupBy("listings.id")
.orderBy(getOrderByCondition(params))
.orderBy(orderBy)

if (params.filter) {
addFilters<Array<ListingFilterParams>, typeof filterTypeToFieldMap>(
Expand Down Expand Up @@ -120,7 +114,7 @@ export class ListingsService {
// (WHERE params are the values passed to andWhere() that TypeORM escapes
// and substitues for the `:paramName` placeholders in the WHERE clause.)
.setParameters(innerFilteredQuery.getParameters())
.orderBy(getOrderByCondition(params))
.orderBy(orderBy)
.getMany()

listings = await this.addUnitSummariesToListings(listings)
Expand Down
5 changes: 2 additions & 3 deletions backend/core/src/listings/tests/listings.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -492,16 +492,15 @@ describe("ListingsService", () => {
})

describe("ListingsService.list sorting", () => {
it("defaults to ordering by application dates when no orderBy param is set", async () => {
it("defaults to ordering by name when no orderBy param is set", async () => {
mockListingsRepo.createQueryBuilder
.mockReturnValueOnce(mockInnerQueryBuilder)
.mockReturnValueOnce(mockQueryBuilder)

await service.list({})

const expectedOrderByArgument = {
"listings.applicationDueDate": "ASC",
"listings.applicationOpenDate": "DESC",
"listings.name": "ASC",
}

// The inner query must be ordered so that the ordering applies across all pages (if pagination is requested)
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// The names of supported filters on /listings
export enum ListingFilterKeys {
id = "id",
status = "status",
name = "name",
isVerified = "isVerified",
Expand Down
14 changes: 8 additions & 6 deletions backend/core/test/listings/listings.e2e-spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,9 @@ describe("Listings", () => {
})

it("should modify property related fields of a listing and return a modified value", async () => {
const res = await supertest(app.getHttpServer()).get("/listings").expect(200)
const res = await supertest(app.getHttpServer())
.get("/listings?orderBy=applicationDates")
.expect(200)

const listing: ListingDto = { ...res.body.items[0] }

Expand Down Expand Up @@ -519,22 +521,22 @@ describe("Listings", () => {
})
})

it("defaults to sorting listings by applicationDueDate, then applicationOpenDate", async () => {
it("defaults to sorting listings by name", async () => {
const res = await supertest(app.getHttpServer()).get(`/listings?limit=all`).expect(200)
const listings = res.body.items

// The Coliseum seed has the soonest applicationDueDate (1 day in the future)
expect(listings[0].name).toBe("Test: Coliseum")
expect(listings[0].name).toBe("Medical Center Village")

// Triton and "Default, No Preferences" share the next-soonest applicationDueDate
// (5 days in the future). Between the two, Triton 1 appears first because it has
// the closer applicationOpenDate.
const secondListing = listings[1]
expect(secondListing.name).toBe("Test: Triton 1")
expect(secondListing.name).toBe("Melrose Square Homes")
const thirdListing = listings[2]
expect(thirdListing.name).toBe("Test: Triton 2")
expect(thirdListing.name).toBe("New Center Commons")
const fourthListing = listings[3]
expect(fourthListing.name).toBe("Test: Default, No Preferences")
expect(fourthListing.name).toBe("New Center Pavilion")

const secondListingAppDueDate = new Date(secondListing.applicationDueDate)
const thirdListingAppDueDate = new Date(thirdListing.applicationDueDate)
Expand Down
12 changes: 6 additions & 6 deletions sites/partners/lib/hooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ interface UseSingleApplicationDataProps extends PaginationProps {
type UseUserListProps = PaginationProps

type UseListingsDataProps = PaginationProps & {
userId?: string
listingIds?: string[]
}

export function useSingleListingData(listingId: string) {
Expand All @@ -40,22 +40,22 @@ export function useSingleListingData(listingId: string) {
}
}

export function useListingsData({ page, limit, userId }: UseListingsDataProps) {
export function useListingsData({ page, limit, listingIds }: UseListingsDataProps) {
const params = {
page,
limit,
view: "base",
}

// filter if logged user is an agent
if (typeof userId !== undefined) {
if (listingIds !== undefined) {
Object.assign(params, {
filter: [
{
$comparison: EnumListingFilterParamsComparison["="],
leasingAgents: userId,
$comparison: EnumListingFilterParamsComparison["IN"],
id: listingIds.join(","),
},
],
view: "base",
})
}

Expand Down
7 changes: 4 additions & 3 deletions sites/partners/pages/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,7 @@ export default function ListingsList() {
{
headerName: t("listings.listingName"),
field: "name",
sortable: true,
sort: "asc",
sortable: false,
filter: false,
resizable: true,
cellRenderer: "ListingsLink",
Expand Down Expand Up @@ -137,7 +136,9 @@ export default function ListingsList() {
const { listingDtos, listingsLoading } = useListingsData({
page: currentPage,
limit: itemsPerPage,
userId: !isAdmin ? profile?.id : undefined,
listingIds: !isAdmin
? profile?.leasingAgentInListings?.map((listing) => listing.id)
: undefined,
})

return (
Expand Down