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(GraphQL): improve hasNext/hasPrevious #4231

Merged

Conversation

2 participants
@aldeed
Copy link
Member

aldeed commented May 3, 2018

Resolves #4230
Impact: minor
Type: bugfix

Issue

The current implementation of hasNextPage and hasPreviousPage for GraphQL connections has two limitations:

  • When forward paginating, hasPreviousPage is always false, and when backward paginating, hasNextPage is always false
  • When the response contains exactly as many items as you request, we set hasNextPage or hasPreviousPage to true without checking for sure to see if it's true.

Changes

hasPreviousPage and hasNextPage should always be correct now. It does extra database queries to set correct values.

Breaking changes

None

Testing

Do catalogItems or tags queries with various before, after, first, and last values.
Ensure that if hasPreviousPage is true, there really is a previous page of results. Ensure that if hasNextPage is true, there really is a next page of results.

@aldeed aldeed requested a review from mikemurray May 3, 2018

@aldeed aldeed self-assigned this May 3, 2018

@aldeed aldeed added this to the Democrat milestone May 3, 2018

@mikemurray
Copy link
Member

mikemurray left a comment

LGTM. Tested with a real implementation and these updates not only seem to work well, but also reduce the complexity of the client-side logic related to pagination

@mikemurray mikemurray merged commit 4c980e2 into release-1.12.0 May 3, 2018

9 of 10 checks passed

ci/circleci: snyk-security Your tests failed on CircleCI
Details
WIP ready for review
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: docker-build Your tests passed on CircleCI!
Details
ci/circleci: docker-push Your tests passed on CircleCI!
Details
ci/circleci: dockerfile-lint Your tests passed on CircleCI!
Details
ci/circleci: eslint Your tests passed on CircleCI!
Details
ci/circleci: test-app Your tests passed on CircleCI!
Details
ci/circleci: test-unit Your tests passed on CircleCI!
Details
security/snyk No dependency changes
Details

@mikemurray mikemurray deleted the fix-4230-aldeed-improve-has-more-pages-logic branch May 3, 2018

@spencern spencern referenced this pull request May 31, 2018

Merged

Release 1.12.0 #4287

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.