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.

Sign up for GitHub

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

Jump to bottom

Move where clauses and pagination to subquery, so filtered results return all listing data #271

Merged
merged 13 commits into from
Jul 27, 2021
Merged

Move where clauses and pagination to subquery, so filtered results return all listing data #271

merged 13 commits into from
Jul 27, 2021

Conversation

avaleske
Copy link

@avaleske avaleske commented Jul 26, 2021 •
edited
Loading

Move where clauses and pagination to subquery, so filtered results return all listing data

Issue #258, #151

Addresses #258, #151

  • This change addresses the issue in full
  • This change addresses only certain aspects of the issue
  • This change is a dependency for another issue
  • This change has a dependency from another issue

Description

Moves WHERE clauses and pagination to a subquery, so filtered results return all listing data.

Previously, if we filtered on units or preferences, only the units or preferences that match the query were returned. Now, everything about a building that contains matching units is returned

Also:

  • The subquery gets a list of matching ids by only joining the tables necessary for filtering, before joining everything together
  • Fixes a bug where meta.totalItems was the total number of listings, not the total number of listings that match the filters
  • Refactors pagination logic to use the same subquery as the filters (Filtering and pagination don't work together #151)
  • Updates tests to match, and test meta details
  • Moves sorting logic back into the query (this comment no longer applies now that we're not using take() and skip()

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Prototype/POC (not to merge)
  • This change is a refactor/address technical debt
  • This change requires a documentation update
  • This change requires a SQL Script

How Can This Be Tested/Reviewed?

Test with paths like:

  • /listings?filter[$comparison]=%3C%3E&filter[neighborhood]=Foster%20City&page=1&limit=90
  • /listings?filter[$comparison]=%3C%3E&filter[neighborhood]=Foster%20City&page=2&limit=2
  • /listings?filter[$comparison]==&filter[neighborhood]=Foster%20City&page=1&limit=90
  • /listings

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have reviewed the changes in a desktop view
  • I have reviewed the changes in a mobile view
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have assigned reviewers
  • I have updated the changelog to include a description of my changes

@avaleske avaleske changed the title Avaleske/move where clauses to subquery Move where clauses and pagination to subquery Jul 26, 2021
@avaleske avaleske changed the title Move where clauses and pagination to subquery Move where clauses and pagination to subquery, so filtered results return all listing data Jul 26, 2021
@avaleske avaleske marked this pull request as ready for review July 26, 2021 23:26
@avaleske avaleske linked an issue Jul 26, 2021 that may be closed by this pull request
Refactor listings query so filters on related entities still return all listing data #258
Closed
@avaleske avaleske added epic: filtering Tickets related to Filtering M8 labels Jul 26, 2021
@avaleske avaleske added this to the M8 milestone Jul 26, 2021
anders-schneider
anders-schneider reviewed Jul 27, 2021
View reviewed changes
Copy link

@anders-schneider anders-schneider left a comment

Choose a reason for hiding this comment

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

Thanks for doing this! Two questions below.

backend/core/src/listings/listings.service.ts Outdated Show resolved Hide resolved
backend/core/src/listings/listings.service.ts Outdated Show resolved Hide resolved
backend/core/src/listings/listings.service.ts Outdated Show resolved Hide resolved
anders-schneider
anders-schneider approved these changes Jul 27, 2021
View reviewed changes
Copy link

@anders-schneider anders-schneider left a comment

Choose a reason for hiding this comment

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

LGTM!

@avaleske avaleske merged commit fd423fb into main Jul 27, 2021
@avaleske avaleske mentioned this pull request Jul 27, 2021
Closed
@avaleske avaleske linked an issue Jul 27, 2021 that may be closed by this pull request
Filtering and pagination don't work together #151
Closed
seanmalbert pushed a commit that referenced this pull request Jun 23, 2022
@avaleske
Move where clauses and pagination to subquery, so filtered results re…
132e44f 
…turn all listing data (#271)

* inner query works on its own, but fails when placed in outer query

* add param for inner query params

* add support for where parameters to filtered query

* updates pagination to use inner query

* Refactor pagination, update type for comparison param

* fix comment spacing

* fix tests

* update changelog

* address comment nits and remove units from inner join for now

* reorder outer query builder logic, and use .getParameters() instead of passing inner parameters out manually

* fix tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@willrlin willrlin willrlin approved these changes

@anders-schneider anders-schneider anders-schneider approved these changes

@abbiefarr abbiefarr Awaiting requested review from abbiefarr

Assignees
No one assigned
Labels
epic: filtering Tickets related to Filtering
Projects
None yet
Milestone
M8
3 participants
@avaleske @willrlin @anders-schneider