Skip to content
This repository has been archived by the owner on Jan 31, 2023. It is now read-only.

Specifying Page on Search #58

Closed
confused-Techie opened this issue Oct 27, 2022 · 6 comments
Closed

Specifying Page on Search #58

confused-Techie opened this issue Oct 27, 2022 · 6 comments
Labels
help wanted Extra attention is needed

Comments

@confused-Techie
Copy link
Owner

If a Search is executed that only has a single page of results, and the query passes page=1 as a non-programmer would, then they are given a not found result, since the database is trying to find the 1st page of results from a zero-indexed position.

This should be resolved in a such a way to allow this to happen.

Or otherwise to investigate further with how Atom.io implements this feature and mirror that functionality.

@Digitalone1
Copy link
Contributor

Can you report where is the code of that issue? Thanks.

@confused-Techie
Copy link
Owner Author

Can you report where is the code of that issue? Thanks.

Whatever change fixes should likely be made in the other locations we handle pagination. But as for what the issue was originally created for getPackagesSearch

You can see where the total_pages is defined in ./src/handlers/package_handler.js L272.

But additional pagination occurs in getPackages as well

@confused-Techie confused-Techie added the help wanted Extra attention is needed label Nov 13, 2022
@Digitalone1
Copy link
Contributor

I'm taking a look at it.

The issue might be in the simpleSearch?

if (page !== 1) {
  offset = page * paginated_amount;
}

Let's set paginated_amount as 10. page is 1 by default, so for 1 you get limit 10 and offset 0. Right.

But for page 2 you get limit 10 and offset 20? Shouldn't be limit 10 and offset 10? So:

if (page > 1) {
  offset = (page - 1) * paginated_amount;
}

@confused-Techie
Copy link
Owner Author

@Digitalone1 ya know I think this is it! Great find, I'll fix that today, but pretty sure that would be the root of the issue. Don't know how I didn't catch that

@Digitalone1
Copy link
Contributor

@confused-Techie This has been fixed, right?

@confused-Techie
Copy link
Owner Author

@confused-Techie This has been fixed, right?

I'll probably double check in a moment, but pretty sure initial testing from what you proposed fixed this issue

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants