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 #1811: take limit_choices_to into account with FK #6371

Merged
merged 6 commits into from Jan 8, 2019

Conversation

Projects
None yet
5 participants
@adrienbrunet
Copy link
Contributor

adrienbrunet commented Dec 20, 2018

Closes #1811

I created a small repo to test this fix and it works as expected. It is based on #5358 proposal. (I've done nothing other than formatting the code)

Tests are passing but some more tests for this particular bit should be added, hence the WIP in the title.

(If anyone wants to help...)

@adrienbrunet

This comment has been minimized.

Copy link
Contributor Author

adrienbrunet commented Dec 21, 2018

Finally I took some time to add a simple test for this.
I don't know if it's enough or if we should add more. It's not really elegant though.
Advices welcome.

@xordoquy
Copy link
Collaborator

xordoquy left a comment

Would it be possible to move the test within the test_relations_pk.py file ?

Show resolved Hide resolved rest_framework/relations.py Outdated
@adrienbrunet

This comment has been minimized.

Copy link
Contributor Author

adrienbrunet commented Dec 21, 2018

I moved my tests to test_relations_pk file and updated the code in relations.py to filter only when limit_choices_to is defined. I'm all ears if you have any other requests.

@adrienbrunet adrienbrunet changed the title WIP: Issue #1811: take limit_choices_to into account with FK Issue #1811: take limit_choices_to into account with FK Dec 27, 2018

@adrienbrunet adrienbrunet changed the title Issue #1811: take limit_choices_to into account with FK Fix #1811: take limit_choices_to into account with FK Dec 28, 2018

Show resolved Hide resolved rest_framework/relations.py Outdated
@tomchristie

This comment has been minimized.

Copy link
Member

tomchristie commented Jan 8, 2019

Thanks for this. Looks like it's working towards a resolution.

@adrienbrunet

This comment has been minimized.

Copy link
Contributor Author

adrienbrunet commented Jan 8, 2019

Thanks for this. Looks like it's working towards a resolution.

What better feeling than fixing a 5-year old issue ? 😁

What do you think about the latest changes ?

@@ -266,6 +266,9 @@ def get_relation_kwargs(field_name, relation_info):
# If this field is read-only, then return early.
# No further keyword arguments are valid.
return kwargs
limit_choices_to = model_field.get_limit_choices_to()
if limit_choices_to:
kwargs['queryset'] = kwargs['queryset'].filter(**limit_choices_to)

This comment has been minimized.

@tomchristie

tomchristie Jan 8, 2019

Member

I think this block should probably go above the if has_through_model: switch. (L252) Since there's a couple of cases where the queryset keyword argument gets popped off.

This comment has been minimized.

@tomchristie

tomchristie Jan 8, 2019

Member

Or else use if queryset in kwargs and limit_choices_to:, which would also do the trick.

This comment has been minimized.

@adrienbrunet

adrienbrunet Jan 8, 2019

Author Contributor

You're right. I did not think it through. I just wanted to avoid extra work putting it after the read_only but forgot that queryset can be popped.

Maybe another solution would be to add a check like 'queryset' in kwargs?

This comment has been minimized.

@adrienbrunet

adrienbrunet Jan 8, 2019

Author Contributor

I was too slow to post my comment and did not see yours. :/

@tomchristie

This comment has been minimized.

Copy link
Member

tomchristie commented Jan 8, 2019

Nice one! I think there's one last bit to fix here - making sure that the change still works in cases where the queryset keyword argument doesn't exist anymore.

Requested change has been made.

@adrienbrunet

This comment has been minimized.

Copy link
Contributor Author

adrienbrunet commented Jan 8, 2019

I'm switching back my solution to the previous one and adding the queryset in kwargs check. Would you like a rebase to avoid all this back and forth commits ?

@tomchristie

This comment has been minimized.

Copy link
Member

tomchristie commented Jan 8, 2019

@adrianmester No need to switch it - looks good as is.

And no to the rebase - I'll just use Squash and merge.

@tomchristie

This comment has been minimized.

Copy link
Member

tomchristie commented Jan 8, 2019

What better feeling than fixing a 5-year old issue ? 😁

Neat, yup. It's the oldest open issue on our list!

https://github.com/encode/django-rest-framework/issues?page=6&q=is%3Aissue+is%3Aopen

@adrienbrunet

This comment has been minimized.

Copy link
Contributor Author

adrienbrunet commented Jan 8, 2019

Allright. That's good for me. It was a pleasure to be part of the team for the last half hour or so 😃

@tomchristie

This comment has been minimized.

Copy link
Member

tomchristie commented Jan 8, 2019

Boom 💥

@tomchristie tomchristie merged commit e3bd4b9 into encode:master Jan 8, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@adrienbrunet adrienbrunet deleted the adrienbrunet:issue-1811 branch Jan 8, 2019

@ulgens

This comment has been minimized.

Copy link

ulgens commented Jan 18, 2019

https://docs.djangoproject.com/en/2.1/ref/models/fields/#django.db.models.ForeignKey.limit_choices_to

It's possible that limit_choices_to can get a Q object, but this PR doesn't consider that and expects for a dictionary.

(boom is denied 😄 )

@ulgens

This comment has been minimized.

Copy link

ulgens commented Jan 18, 2019

It's possible that limit_choices_to can get a Q object, but this PR doesn't consider that and expects for a dictionary.

A different version, which consider Q objects: usetaptap@607059d

@vasdee vasdee referenced this pull request Feb 24, 2019

Closed

limit_choices_to and Q objects #6470

4 of 6 tasks complete
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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.