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

Implement backend pagination for GET /listings #133

Merged
merged 11 commits into from
Jul 1, 2021

Conversation

anders-schneider
Copy link

Issue

Addresses #56

  • This change addresses the issue in full

Description

This change changes the parameters and return type for GET /listings such that results are paginated. The parameters now include page and limit fields, and the returned object includes both an array of Listings and metadata about e.g. which page of listings this represents, how many total listings there are, how many listings per page, etc.

The pagination implementation uses skip and take when building the SQL query, and those correspond to SQL's OFFSET and LIMIT, respectively. typeorm actually sends two queries as a result: a first to ask for all the IDs of listings that are in the specified range, and a second to get all of the data (including all of the data in JOINed tables). (And, this change adds a third SELECT COUNT(*) FROM listings; query in order to populate the pagination metadata, but that should be fast.) The hope is that, although this change means more SQL queries to the database, the overall time to retrieve listing data is faster since we don't have to retrieve all of the JOINed data in a single query.

This change also adds a couple tests and fixes a couple places in the frontend so that they understand the new paginated-listings return type.

Type of change

  • New feature (non-breaking change which adds functionality)

How Can This Be Tested/Reviewed?

$ cd backend/core
$ yarn test:e2e:local
$ yarn test listing

Also, if you run the code locally and point your browser to localhost:3100/listings, you should be able to see the new paginated listings result.

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

@willrlin
Copy link

Also, change the title of the PR to imperative and remove your username, as if it were the first line of the CL.

@anders-schneider anders-schneider changed the title Aeschneider backend listings pagination Implement backend pagination for GET /listings Jun 30, 2021
@anders-schneider
Copy link
Author

Thanks for the comments @willrlin ! I closed the conversations that I felt were resolved, and left the ones that you may have follow-up on open - let me know? And I changed the PR title, thanks for catching that!

…CityOfDetroit/bloom into aeschneider-backend-listings-pagination
Copy link

@abbiefarr abbiefarr left a comment

Choose a reason for hiding this comment

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

This change seems to break both the partner portal:
image

and the resident portal:
image

For the partner portal, I'm hoping it's an easy fix in sites/partners/lib/hooks.ts. I have to look more into the issue with the resident portal.

@abbiefarr
Copy link

Also I believe that whenever the backend api changes, we need to regenerate the client with yarn generate:client

@abbiefarr
Copy link

I fixed the errors with the public and partner portals, with the default values they display all the listings as they did before. I also ran the client library codegen, which generated some changes I expected and others I didn't expect and can't really explain.

PTAL at the newest commit, the issues were with some objects being "undefined", but I'm pretty unfamiliar with typescript so I'm pretty skeptical of the code I wrote.

Copy link

@willrlin willrlin left a comment

Choose a reason for hiding this comment

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

Seems fine to me. As for the codegen, hopefully it was just a bit out of date. Maybe we should set a presubmit check to ensure that the generated version matches the PR version?

sites/public/pages/index.tsx Outdated Show resolved Hide resolved
@abbiefarr abbiefarr merged commit 61d7270 into main Jul 1, 2021
@abbiefarr abbiefarr deleted the aeschneider-backend-listings-pagination branch July 1, 2021 17:35
avaleske added a commit that referenced this pull request Aug 5, 2021
@avaleske avaleske mentioned this pull request Aug 5, 2021
14 tasks
avaleske added a commit that referenced this pull request Aug 9, 2021
avaleske added a commit that referenced this pull request Aug 9, 2021
seanmalbert pushed a commit that referenced this pull request Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants