Skip to content

Fixed #2445 -- ``limit_choices_to`` attribute can now be a callable. #1600

Closed
wants to merge 1 commit into from

3 participants

@adamsc64
adamsc64 commented Sep 7, 2013
  • ForeignKey or ManyToManyField attribute limit_choices_to can now be a callable that returns either a Q object or a dict.
  • The callable will be invoked at ModelForm initialization time.
  • Admin form behavior modified to handle new functionality.
  • Admin widget behavior modified to handle new functionality.
  • Updated Django documentation field reference section.
  • Added unit tests for limit_choices_to on ModelForms.
  • Tweaked unit tests for Admin to use some callables for limit_choices_to.
@adamsc64 adamsc64 Fixed #2445 -- ``limit_choices_to`` attribute can now be a callable.
- ForeignKey or ManyToManyField attribute ``limit_choices_to`` can now
  be a callable that returns either a ``Q`` object or a dict.
- The callable should be invoked at ModelForm initialization time.
- Admin form behavior modified to handle new functionality.
- Admin widget behavior modified to handle new functionality.
- Updated Django documentation field reference section.
- Added unit tests for ``limit_choices_to`` on ModelForms.
- Tweaked unit tests for Admin to use some callables for
  ``limit_choices_to``.
0f9d45b
@czpython
czpython commented Sep 8, 2013

👍

@freakboy3742 freakboy3742 commented on the diff Sep 30, 2013
tests/admin_views/models.py
@@ -127,7 +128,7 @@ class Meta:
@python_2_unicode_compatible
class Thing(models.Model):
title = models.CharField(max_length=20)
- color = models.ForeignKey(Color, limit_choices_to={'warm': True})
+ color = models.ForeignKey(Color, limit_choices_to=lambda: {'warm': True})
@freakboy3742
Django member
freakboy3742 added a note Sep 30, 2013

Why does this change the existing test? Generally, changing old tests indicates that you're introducing a potential regression, because old behavior isn't going to be tested any more.

@adamsc64
adamsc64 added a note Feb 1, 2014

Russell,

Good point, apologies for that. This was just an extra attempt at mixing the new functionality in with the old, but in retrospect it is a bad idea. I will re-submit the pull request without modifying the old test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@freakboy3742 freakboy3742 commented on the diff Sep 30, 2013
django/contrib/admin/widgets.py
@@ -171,7 +171,10 @@ def render(self, name, value, attrs=None):
return mark_safe(''.join(output))
def base_url_parameters(self):
- return url_params_from_lookup_dict(self.rel.limit_choices_to)
+ limit_choices_to = self.rel.limit_choices_to
+ if callable(limit_choices_to):
+ limit_choices_to = limit_choices_to()
+ return url_params_from_lookup_dict(limit_choices_to)
@freakboy3742
Django member
freakboy3742 added a note Sep 30, 2013

Isn't this inconsistent with allowing limit_choices_to() to return a Q object?

@adamsc64
adamsc64 added a note Feb 1, 2014

Russell,

You are absolutely correct -- Django does not allow Q objects for the widgets ManyToManyRawIdWidget and ForeignKeyRawIdWidget. I don't exactly know why this limitation is there, however it was recently documented by @evildmp [Daniele Procida] at 79cc666. As you can see, his changes to the Django documentation for limit_choices_to specify this limitation.

So this is a limitation that exists regardless of whether limit_choices_to can be a callable or not. In other words, I don't think it's a new limitation I am introducing in this pull request.

Background: The methods base_url_parameters() as well as url_params_from_lookup_dict() are only invoked when these widget types are used. Use of these widgets does not support using Q objects for limit_choices_to, I believe perhaps because they involve raw ids.

I don't know what the solution to this problem is, as it's probably hard to translate a Q object into a set of key-value parameters (see url_params_from_lookup_dict() function for example). But I believe solving this problem is beyond the scope of this ticket. What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@adamsc64 adamsc64 closed this Feb 1, 2014
@adamsc64 adamsc64 deleted the adamsc64:ticket_2445 branch Feb 1, 2014
@adamsc64
adamsc64 commented Feb 3, 2014

I re-opened another pull request [#2233] with:
1. Rebase from django/django:master (there were a few changes that needed to be made to my branch).
2. Removed the offending unit test change in tests/admin_views/models.py, called out by @freakboy3742 in the above comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.