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

Fixed #30449 -- Fixed RelatedFieldListFilter/RelatedOnlyFieldListFilter to respect model's Meta.ordering. #11400

Merged
merged 2 commits into from Aug 15, 2019

Conversation

zeyneloz
Copy link
Contributor

Check #30449

Refactored admin.RelatedFieldListFilter and admin.RelatedOnlyFieldListFilter
to respect ordering defined in related model admin. Refactored
Field.get_choices to fallback to default model ordering.

Copy link

@gauravtoshniwal1989 gauravtoshniwal1989 left a comment

Choose a reason for hiding this comment

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

Minor comments around checking the existence of ordering before applying it on the QuerySet in get_choices.
PS: Please forgive my naivety if it seems like that. It's my first review on the Django project.

django/db/models/fields/__init__.py Show resolved Hide resolved
django/db/models/fields/reverse_related.py Outdated Show resolved Hide resolved
Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

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

@zeyneloz Thanks for this patch 👍 I left some comment. Can you also split your changes into two commits

  • 1st with adding get_field_ordering() hook, and
  • 2nd with fix and tests.

django/db/models/fields/reverse_related.py Show resolved Hide resolved
tests/admin_filters/models.py Outdated Show resolved Hide resolved
tests/admin_filters/models.py Outdated Show resolved Hide resolved
tests/admin_filters/models.py Outdated Show resolved Hide resolved
tests/admin_filters/tests.py Outdated Show resolved Hide resolved
tests/admin_filters/tests.py Show resolved Hide resolved
tests/admin_filters/tests.py Outdated Show resolved Hide resolved
tests/admin_filters/tests.py Show resolved Hide resolved
tests/admin_filters/tests.py Show resolved Hide resolved
@zeyneloz
Copy link
Contributor Author

@felixxm Thanks for the comments. I tried to avoid adding extra models for tests but I had to add two new ones otherwise it would have broken existing test cases. Can you review the new commits, If everything is okay, I can revamp the commits then.

Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

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

@zeyneloz Thanks! Please add release notes (2.2.5.txt) and take a look at tests in an alternative PR #11661 they are simpler, looks sufficient and don't add new models.

@zeyneloz
Copy link
Contributor Author

@zeyneloz Thanks! Please add release notes (2.2.5.txt) and take a look at tests in an alternative PR #11661 they are simpler, looks sufficient and don't add new models.

Thanks @felixxm . I added the release note.
I also checked alternative PR but there are not enough tests in it. I had to introduce new models to cover all test cases, without breaking existing ones (and to make it more readable/predictable). Anybody is more than welcome to revamp the tests of this PR if they can make them without new models, since I don't have any free time to look into it this week.

@felixxm
Copy link
Member

felixxm commented Aug 15, 2019

@zeyneloz Thanks! I changed tests and moved a regression fix to a separate commit.

@felixxm felixxm changed the title Fixed #30449 -- Fixed ordering of admin.RelatedFieldListFilter. Fixed #30449 -- Fixed RelatedFieldListFilter/RelatedOnlyFieldListFilter to respect model's Meta.ordering. Aug 15, 2019
zeyneloz and others added 2 commits August 15, 2019 10:29
…er to respect model's Meta.ordering.

Regression in 6d4e5fe.

Co-Authored-By: Mariusz Felisiak <felisiak.mariusz@gmail.com>
@felixxm felixxm merged commit 8289fc5 into django:master Aug 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants