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 backward Relay pagination #1046

Conversation

tcleonard
Copy link
Collaborator

New version of the pull request #1010 (to solve issue #1009) which adds unit tests.

@tcleonard
Copy link
Collaborator Author

Not sure what is causing those test failures... it fails in an import in django rest framework (and the import is totally valid).
Tests are passing locally.

@zbyte64
Copy link
Collaborator

zbyte64 commented Oct 19, 2020

This is the relevant part I see from the test failure:

 .tox/py37-django111/lib/python3.7/site-packages/rest_framework/fields.py:15: in <module>
      from django.core.validators import (
  E   ImportError: cannot import name 'ProhibitNullCharactersValidator' from 'django.core.validators' (/home/runner/work/graphene-django/graphene-django/.tox/py37-django111/lib/python3.7/site-packages/django/core/validators.py)

DRF wants to import a validator from Django that doesn't exist in Django 1.11

@tcleonard
Copy link
Collaborator Author

Indeed, which is not related to anything I have changed... Maybe DRF simply decided to drop support for older versions of Django and we might need to do the same here.

@jkimbo
Copy link
Member

jkimbo commented Oct 19, 2020

@tcleonard yep DRF dropped support for Django 1.11. Fixed the tests here: #1047

@tcleonard
Copy link
Collaborator Author

thanks @jkimbo, merged your changes and the CI now passes

Copy link
Collaborator

@zbyte64 zbyte64 left a comment

Choose a reason for hiding this comment

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

Thank you for this

@vecchp
Copy link

vecchp commented Dec 8, 2020

@zbyte64 is there anything that needs to be done to get this in?

@tcleonard
Copy link
Collaborator Author

I don't think so... for me it's ready to merge.. 🤷

Copy link
Contributor

@pcraciunoiu pcraciunoiu left a comment

Choose a reason for hiding this comment

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

Makes sense to me. Just one change I'd suggest.

graphene_django/fields.py Show resolved Hide resolved

def test_should_query_with_low_max_limit(self, graphene_settings):
"""
When doing backward pagination (using last) in combination with a max limit higher than the number of objects
Copy link
Contributor

Choose a reason for hiding this comment

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

In this case, max limit is lower than the number of objects, right?

It looks like this test covers the same case as this one, which is great!

@pcraciunoiu
Copy link
Contributor

@jkimbo any chance we could get this merged?

Because graphene-django doesn't support true cursor pagination, the most reliable way to get recent items first is to go backwards, and this bug gets in the way of that.

@zbyte64 zbyte64 merged commit 454b740 into graphql-python:master Dec 23, 2020
@jkimbo
Copy link
Member

jkimbo commented Dec 23, 2020

Thanks @zbyte64

1 similar comment
@tcleonard
Copy link
Collaborator Author

Thanks @zbyte64

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.

5 participants