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.

Sign up for GitHub

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

Jump to bottom

Fixed #35173 -- Reallowed filtering with __isnull on foreign keys not listed in list_filters. #17844

Merged
merged 1 commit into from Feb 15, 2024
Merged

Fixed #35173 -- Reallowed filtering with __isnull on foreign keys not listed in list_filters. #17844

merged 1 commit into from Feb 15, 2024

Conversation

Hisham-Pak
Copy link
Contributor

@Hisham-Pak Hisham-Pak force-pushed the ticket_35173 branch 2 times, most recently from 111ffff to 6fd450b Compare February 9, 2024 13:39
@nessita
Copy link
Contributor

nessita commented Feb 9, 2024

Hello @Hisham-Pak, thank you for this pull request.

I don't think this is the right approach: as far as I see this branch is just restoring the whole if guard that was removed in https://code.djangoproject.com/changeset/f80669d2f5a5f1db9e9b73ca893fefba34f955e7, which sounds like a disproportionate solution for the reported problem.

Ideally, the fix would only enlarge/modify the current if guard to cover for the missing case. Or, the branch should come up with a solution that would replace the current if branch, but without resorting to having two apparently independent if guards that are really duplicating some of the checks.

Does this make sense? cc/ @sarahboyce

@Hisham-Pak
Copy link
Contributor Author

Hisham-Pak commented Feb 9, 2024 •
edited

Hi @nessita, thanks for the review. Currently I was not able to find a way to combine these checks in one if because of a break in between:

if not getattr(field, "path_infos", None):
    # This is not a relational field, so further parts
    # must be transforms.
    break

Will update this if I found a way around. And yes I think not prev_field could be removed from second if.

@nessita
Copy link
Contributor

nessita commented Feb 9, 2024

Hi @nessita, thanks for the review. Currently I was not able to find a way to combine these checks in one if because of a break in between:

if not getattr(field, "path_infos", None):
    # This is not a relational field, so further parts
    # must be transforms.
    break

Will update this if I found a way around. And yes I think not prev_field could be removed from second if.

Thank you for the response. If the break is causing the issue, you could consider reverting the current if to what it had before the regression commit but in a manner that the fixed issue in PR #17581, which has already a regression test, keeps passing.

@Hisham-Pak
Copy link
Contributor Author

@nessita, I implemented the changes you suggested. Let me know if it looks good.

@Hisham-Pak Hisham-Pak force-pushed the ticket_35173 branch 2 times, most recently from 28ef15e to d699eda Compare February 11, 2024 17:47
sarahboyce
sarahboyce approved these changes Feb 13, 2024
View reviewed changes
Copy link
Contributor

@sarahboyce sarahboyce 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 working on this @Hisham-Pak

nessita
nessita reviewed Feb 14, 2024
View reviewed changes
django/contrib/admin/options.py Outdated
model._meta.auto_field is None
or part not in getattr(prev_field, "to_fields", [])
)
and (field.is_relation or not getattr(field, "primary_key", None))
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a question about this: the not getattr(field, "primary_key", None), what is it covering exactly? is it asking for whether there is primary_key field? or if the primaty_key field is None? what happens if the value of primary_key is falsy?

I change the code to be:

Suggested change
and (field.is_relation or not getattr(field, "primary_key", None))
and (field.is_relation or not field.primary_key)

and no test fails, so I think that we can either simplify the code or add tests ensuring the have the more robust guard here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not getattr(field, "primary_key", None) was checking for not(hasattr(field, "primary_key") and field.primary_key). But I think this was not necessary as all fields inherit from Field class which already has primary_key attribute. The only exception I can think is of reverse relations and for that field.is_relation is already set to True.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @Hisham-Pak, would you have time to extend the test case or add a new one with such case so we evaluate if we need to further tweak the if guard?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we need any further tweaking. It already is looking good.

@Hisham-Pak @nessita
Fixed #35173 -- Fixed ModelAdmin.lookup_allowed() for lookups on fore…
ebbab6f 
…ign keys when not included in ModelAdmin.list_filters.

Regression in f80669d.

Thanks Sarah Boyce for the review.
nessita
nessita approved these changes Feb 14, 2024
View reviewed changes
Copy link
Contributor

@nessita nessita 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 @Hisham-Pak! I made small changes and tweaked the release notes.

@nessita nessita merged commit 8db593d into django:main Feb 15, 2024
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nessita nessita nessita approved these changes

@sarahboyce sarahboyce sarahboyce approved these changes

@harshitbudhiraja harshitbudhiraja harshitbudhiraja approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@Hisham-Pak @nessita @sarahboyce @harshitbudhiraja