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 pagination for FindAll method #427

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dmaksimov
Copy link
Contributor

@dmaksimov dmaksimov commented Oct 30, 2021

The FindAll method was returning on the wrong results when using pagination.

The $pageNumber argument was being passed directly to the query as the startPosition instead of determining what the startPosition should be based on the $pageNumber and $pageSize.

This PR determines the correct startPosition for the query.

The $pageNumber argument default has been updated to 1 since that makes more sense. When thinking about pages we think of them as starting at 1 not 0.


Example

Say you had 25 items and were wanting to return them all in pages of 10.

Page 0

$dataService->FindAll('Item', 0, 10);

Result:

select * from Item startPosition 0 maxResults 10
/* returns items 1-10 */

Expected:

select * from Item startPosition 1 maxResults 10
/* returns items 1-10 */

(this is redundant and the same as page 1)

Page 1

$dataService->FindAll('Item', 1, 10);

Result:

select * from Item startPosition 1 maxResults 10
/* returns items 1-10 */

Expected:

select * from Item startPosition 1 maxResults 10
/* returns items 1-10 */

Page 2

$dataService->FindAll('Item', 2, 10);

Result:

select * from Item startPosition 2 maxResults 10
/* returns items 2-11 */

Expected:

select * from Item startPosition 11 maxResults 10
/* returns items 11-20 */

Page 3

$dataService->FindAll('Item', 3, 10);

Result:

select * from Item startPosition 3 maxResults 10
/* returns items 3-12 */

Expected:

select * from Item startPosition 21 maxResults 10
/* returns items 21-25 */

@LiamAshdown
Copy link

#473 Can this be merged please?

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.

2 participants