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 pagination to user following/follower lists #9323

Merged

Conversation

pidgezero-one
Copy link
Contributor

@pidgezero-one pidgezero-one commented May 22, 2024

Closes #9283

Adds pagination support to people/:username/followers and people/:username/following

Technical

  • I added results_per_page as a mandatory variable in the template instead of optional, so that the page size given to the macro will always match the page size used for calculating the offset in the DB query
  • An easier way to solve this issue would have just been to pass a slice of follows to the template, but I don't think returning the whole DB contents would be solving an issue of performance as well as returning a constant 25 or fewer rows does.
  • I added limits and offsets to the methods in core/follows.py, but I made these params optional so as to not break other parts of the code that use get_following. I looked in other parts of the codebase to verify that None and 0 are appropriate defaults.
  • Also removed some unused imports and replaced some spacing in the HTML file that was loading weirdly in my code editor

Testing

I inserted dummy data into the follows table to verify that both lists loaded correctly at 0, 1-25, and 26+ entries, and verified that the pagination UI loads the correct section of the list depending on the page (http://localhost:8080/people/openlibrary/followers, http://localhost:8080/people/openlibrary/followers?page=1, http://localhost:8080/people/openlibrary/followers?page=2).

Screenshot

These are zoomed out.

Following list, 25 entries in total:
image

Following list, 26 entries:
image
image

Following list, empty:
image

Followers list, 25 entries in total:
image

Followers list, 26 entries:
image
image

Followers list, empty:
image

Stakeholders

@mekarpeles

Attribution Disclaimer: By proposing this pull request, I affirm to have made a best-effort and exercised my discretion to make sure relevant sections of this code which substantially leverage code suggestions, code generation, or code snippets from sources (e.g. Stack Overflow, GitHub) have been annotated with basic attribution so reviewers & contributors may have confidence and access to the correct context to evaluate and use this code.

@mekarpeles
Copy link
Member

This is a great start @pidgezero-one, great work so far.

You might consider running it using gitpod which may also give you some more detail about anything that may be going wrong and make it easier to test!

@mekarpeles mekarpeles self-assigned this May 23, 2024
i = web.input(page=1)
page_size = 25
page = 1 if not i.page.isdigit() else max(1, int(i.page))
offset = (page - 1) * page_size
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the page is also used to calculate the offset, I validate it here and pass it into the template instead of defining in the template. The number of values being passed to the template is quite large, and this might not be totally necessary, but doing it this way lets me force invalid page values in the query param to default to page 1 (i.e. ?page=0, ?page=-1, ?page=aaaa all default to the first page) before using it to calculate the offset.

@pidgezero-one pidgezero-one marked this pull request as ready for review May 25, 2024 01:37
@scottbarnes scottbarnes added the Needs: Review This issue/PR needs to be reviewed in order to be closed or merged (see comments). [managed] label May 28, 2024
@mekarpeles mekarpeles merged commit c8203dd into internetarchive:master May 29, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Review This issue/PR needs to be reviewed in order to be closed or merged (see comments). [managed]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Paginate Followers & Following pages
3 participants