Skip to content
/ django Public

Fixed #28384 -- Fixed ModelAdmin.lookup_allowed() for OneToOneField primary keys and nested relations.#16661

Merged
felixxm merged 1 commit intodjango:mainfrom
sarahboyce:ticket_28384_lookup_disallowed_admin
Mar 28, 2023
Merged

Fixed #28384 -- Fixed ModelAdmin.lookup_allowed() for OneToOneField primary keys and nested relations.#16661
felixxm merged 1 commit intodjango:mainfrom
sarahboyce:ticket_28384_lookup_disallowed_admin

Conversation

@sarahboyce
Copy link
Contributor

@sarahboyce sarahboyce commented Mar 18, 2023

Ticket: https://code.djangoproject.com/ticket/28384

Notes:

I am not 100% sure whether there is something I am missing from this comment

            # It is allowed to filter on values that would be found from local
            # model anyways. For example, if you filter on employee__department__id,
            # then the id value would be found already from employee__department_id.

I tried to include stuff around it in the test case but I don't really understand what it is getting at

The test itself is copied from the ticket, there is a test above test_lookup_allowed_onetoone that is very similar and can be tweaked to include this case but not sure if we want this to be in a separate test

@timgraham
Copy link
Member

timgraham commented Mar 18, 2023

I believe the case the comment refers to is when "restaurant__place__name" is disallowed (assuming place has a name field), "restaurant__place__id" should be allowed (even when other fields on place are disllowed) since "restaurant__place_id" is allowed.

@sarahboyce sarahboyce force-pushed the ticket_28384_lookup_disallowed_admin branch 2 times, most recently from 7bc55f6 to 460a690 Compare March 18, 2023 11:09
@sarahboyce
Copy link
Contributor Author

sarahboyce commented Mar 18, 2023

Thank you Tim that helps! ⭐

What I have is breaking a couple of things because I am now too strict with which filters I allow

(this is why I have these very random additions to some list_filter definitions)

Updating that this patch needs improvement, but if anyone has any clever ideas, that would be very helpful 👍

@sarahboyce sarahboyce force-pushed the ticket_28384_lookup_disallowed_admin branch 2 times, most recently from 7ca0bcc to 874dcb6 Compare March 18, 2023 20:52
@sarahboyce
Copy link
Contributor Author

sarahboyce commented Mar 18, 2023

Ok I have something that is good enough 👍 it's ready for a review

felixxm
felixxm reviewed Mar 21, 2023
View reviewed changes
@sarahboyce sarahboyce force-pushed the ticket_28384_lookup_disallowed_admin branch 2 times, most recently from 84b2348 to 705438b Compare March 27, 2023 16:42
@felixxm
Copy link
Member

felixxm commented Mar 28, 2023

@sarahboyce Thanks 👍

@felixxm felixxm force-pushed the ticket_28384_lookup_disallowed_admin branch from 705438b to 93814d9 Compare March 28, 2023 07:22
@felixxm felixxm force-pushed the ticket_28384_lookup_disallowed_admin branch from 93814d9 to 45ecd9a Compare March 28, 2023 07:26
@felixxm
Copy link
Member

felixxm commented Mar 28, 2023

buildbot, test on selenium.

@felixxm felixxm merged commit 45ecd9a into django:main Mar 28, 2023
@felixxm felixxm changed the title Fixed #28384 -- Fixed ModelAdmin.lookup_allowed() for foreign keys as primary key Fixed #28384 -- Fixed ModelAdmin.lookup_allowed() for OneToOneField primary keys and nested relations. Mar 28, 2023
@jedie jedie mentioned this pull request Dec 22, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@felixxm felixxm felixxm left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@sarahboyce @timgraham @felixxm