Skip to content

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

Closed
wants to merge 3 commits into from

2 participants

@adamsc64
adamsc64 commented Feb 3, 2014
  • 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 test class using limit_choices_to on ModelForm.
@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 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 test class using ``limit_choices_to`` on ModelForm.
f822e2e
@adamsc64
adamsc64 commented Feb 3, 2014

This is a new pull request with fixes as requested by @freakboy3742 in the comment to tests/admin_views/models.py in [#1600]. I removed the offending change to the old unit test.

Additionally, this new pull request includes a rebase with the latest upstream from django/django:master (there were a few changes that needed to be made to my old branch to make it work).

@timgraham timgraham commented on an outdated diff Feb 6, 2014
django/forms/models.py
@@ -324,6 +324,16 @@ def __init__(self, data=None, files=None, auto_id='id_%s', prefix=None,
self._validate_unique = False
super(BaseModelForm, self).__init__(data, files, auto_id, prefix, object_data,
error_class, label_suffix, empty_permitted)
+ # This form instance should now have its own set of fields in
@timgraham
Django member
timgraham added a note Feb 6, 2014

I would simply say "Apply limit_choices_to to each field."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on an outdated diff Feb 6, 2014
docs/ref/models/fields.txt
staff_member = models.ForeignKey(User, limit_choices_to={'is_staff': True})
causes the corresponding field on the ``ModelForm`` to list only ``Users``
- that have ``is_staff=True``.
-
- Instead of a dictionary this can also be a :class:`Q object
- <django.db.models.Q>` for more :ref:`complex queries
- <complex-lookups-with-q>`. However, if ``limit_choices_to`` is a :class:`Q
- object <django.db.models.Q>` then it will only have an effect on the
- choices available in the admin when the field is not listed in
- ``raw_id_fields`` in the ``ModelAdmin`` for the model.
+ that have ``is_staff=True``. This may be helpful in the Django admin.
+
+ The callable form can be helpful when used in conjunction with the Python
@timgraham
Django member
timgraham added a note Feb 6, 2014

can be helpful, for instance, when ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on an outdated diff Feb 6, 2014
docs/ref/models/fields.txt
- <django.db.models.Q>` for more :ref:`complex queries
- <complex-lookups-with-q>`. However, if ``limit_choices_to`` is a :class:`Q
- object <django.db.models.Q>` then it will only have an effect on the
- choices available in the admin when the field is not listed in
- ``raw_id_fields`` in the ``ModelAdmin`` for the model.
+ that have ``is_staff=True``. This may be helpful in the Django admin.
+
+ The callable form can be helpful when used in conjunction with the Python
+ ``datetime`` module to limit selections by date range. For example::
+
+ limit_choices_to = lambda: {'pub_date__lte': datetime.date.utcnow()}
+
+ If ``limit_choices_to`` is or returns a :class:`Q object
+ <django.db.models.Q>`, which is useful for :ref:`complex queries
+ <complex-lookups-with-q>`, then it will only have an effect on the choices
+ available in the admin when the field is not listed in ``raw_id_fields`` in
@timgraham
Django member
timgraham added a note Feb 6, 2014

would like a link for raw_id_fields... :attr:`~django.contrib.admin.ModelAdmin.raw_id_fields`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on an outdated diff Feb 6, 2014
docs/ref/models/fields.txt
+
+ The callable form can be helpful when used in conjunction with the Python
+ ``datetime`` module to limit selections by date range. For example::
+
+ limit_choices_to = lambda: {'pub_date__lte': datetime.date.utcnow()}
+
+ If ``limit_choices_to`` is or returns a :class:`Q object
+ <django.db.models.Q>`, which is useful for :ref:`complex queries
+ <complex-lookups-with-q>`, then it will only have an effect on the choices
+ available in the admin when the field is not listed in ``raw_id_fields`` in
+ the ``ModelAdmin`` for the model.
+
+.. note::
+
+ If a callable is used for ``limit_choices_to``, it will be invoked every time a
+ new form is instantiated. It may also be invoked when a model is validated using
@timgraham
Django member
timgraham added a note Feb 6, 2014

is validated, for example by management commands or the admin.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on an outdated diff Feb 6, 2014
docs/ref/models/fields.txt
+
+ limit_choices_to = lambda: {'pub_date__lte': datetime.date.utcnow()}
+
+ If ``limit_choices_to`` is or returns a :class:`Q object
+ <django.db.models.Q>`, which is useful for :ref:`complex queries
+ <complex-lookups-with-q>`, then it will only have an effect on the choices
+ available in the admin when the field is not listed in ``raw_id_fields`` in
+ the ``ModelAdmin`` for the model.
+
+.. note::
+
+ If a callable is used for ``limit_choices_to``, it will be invoked every time a
+ new form is instantiated. It may also be invoked when a model is validated using
+ various management commands, as well as in the admin. The admin constructs
+ querysets to validate its form inputs in various edge cases multiple times,
+ so there is a possibility your callable may be invoked that many times.
@timgraham
Django member
timgraham added a note Feb 6, 2014

"that many times" -> "several times"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham and 1 other commented on an outdated diff Feb 6, 2014
docs/ref/models/fields.txt
+ limit_choices_to = lambda: {'pub_date__lte': datetime.date.utcnow()}
+
+ If ``limit_choices_to`` is or returns a :class:`Q object
+ <django.db.models.Q>`, which is useful for :ref:`complex queries
+ <complex-lookups-with-q>`, then it will only have an effect on the choices
+ available in the admin when the field is not listed in ``raw_id_fields`` in
+ the ``ModelAdmin`` for the model.
+
+.. note::
+
+ If a callable is used for ``limit_choices_to``, it will be invoked every time a
+ new form is instantiated. It may also be invoked when a model is validated using
+ various management commands, as well as in the admin. The admin constructs
+ querysets to validate its form inputs in various edge cases multiple times,
+ so there is a possibility your callable may be invoked that many times.
+ Make sure there are no undesired side effects to this behavior in your case.
@timgraham
Django member
timgraham added a note Feb 6, 2014

"no undesired side effects in your callable."?

An example of something that might go wrong might be helpful.

@adamsc64
adamsc64 added a note Feb 9, 2014

Ok, hmm. I don't think there's anything especially that could go wrong when using a callable for limit_choices_to that would be significantly different from what could go wrong when using the callable form for arguments in other places in Django. So I'll just leave the side effects warning out, unless you especially want it there. Just let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham and 1 other commented on an outdated diff Feb 6, 2014
tests/admin_views/tests.py
@@ -53,8 +53,9 @@
Report, MainPrepopulated, RelatedPrepopulated, UnorderedObject,
Simple, UndeletableObject, UnchangeableObject, Choice, ShortMessage,
Telegram, Pizza, Topping, FilteredManager, City, Restaurant, Worker,
- ParentWithDependentChildren)
+ ParentWithDependentChildren, StumpJoke)
@timgraham
Django member
timgraham added a note Feb 6, 2014

unused import (recommend you check your code with flake8)

@adamsc64
adamsc64 added a note Feb 9, 2014

Thanks for the recommendation about flake8. I hadn't heard of that before. Will integrate that into my workflow, great help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on an outdated diff Feb 6, 2014
tests/admin_views/forms.py
@@ -9,3 +10,10 @@ def clean_username(self):
if username == 'customform':
raise forms.ValidationError('custom form error')
return username
+
+
+class StumpJokeForm(forms.ModelForm):
+
+ class Meta:
+ model = StumpJoke
+ exclude = []
@timgraham
Django member
timgraham added a note Feb 6, 2014

I think it's more canonical to use fields = '__all__'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on an outdated diff Feb 6, 2014
tests/admin_views/forms.py
@@ -9,3 +10,10 @@ def clean_username(self):
if username == 'customform':
raise forms.ValidationError('custom form error')
return username
+
+
+class StumpJokeForm(forms.ModelForm):
@timgraham
Django member
timgraham added a note Feb 6, 2014

This looks like the wrong place for the test as this is just testing ModelForm, not the admin. That said, you probably need tests for the admin portion of this patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on the diff Feb 6, 2014
docs/ref/models/fields.txt
@@ -1074,21 +1074,38 @@ define the details of how the relation works.
.. attribute:: ForeignKey.limit_choices_to
@timgraham
Django member
timgraham added a note Feb 6, 2014

You need to include a ".. versionchanged:: 1.7" note to describe the new behavior. The change should also be mentioned in the 1.7 release notes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham
Django member

This looks like a good start Chris. I'll give it another look after you address my comments, thanks!

@adamsc64 adamsc64 Made changes for #2445 requested by @timgraham on Feb 6, 2014.
- Moved ModelForm test of new ``limit_choices_to`` functionality to
  ``./tests/model_forms/*`` instead of keeping them in
  ``./tests/admin_views/*``.
- Added new test in admin_views for ``limit_choices_to`` callable
  functionality for Django Admin.
- Updated the release notes for Django 1.7 to include a note about the
  new behavior for ``limit_choices_to``.
- Added 'versionchanged' notification for ``limit_choices_to`` in the
  documentation for Django 1.7.
- Added explicit link to ``raw_id_fields`` in the changes to the
  ``ForeignKey.limit_choices_to`` documentation.
- Defined ModelForm for tests using more canonical 'fields' instead of
  'exclude'.
- Refactored datetime import statement for model_forms test.
- Phrased changes to documentation in a better way where requested.
- Added missing indentation for 'note' section of proposed changes to
  documentation.
- Simplified inline code comments where requested.
- Removed unused imports.
- Refs #2445.
5d4b7a1
@timgraham timgraham commented on an outdated diff Feb 11, 2014
tests/admin_views/tests.py
+class LimitChoicesToInAdminTest(TestCase):
+ urls = "admin_views.urls"
+ fixtures = ['admin-views-users.xml']
+
+ def setUp(self):
+ self.client.login(username='super', password='secret')
+
+ def tearDown(self):
+ self.client.logout()
+
+ def test_limit_choices_to_as_callable(self):
+ """Test for ticket 2445 changes to admin."""
+ character1 = Character(username='threepwood')
+ character2 = Character(username='marley')
+
+ character1.last_action = datetime.datetime.today() + datetime.timedelta(days=1)
@timgraham
Django member
timgraham added a note Feb 11, 2014

is there a reason you didn't use Character.objects.create() and set all attributes there?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on an outdated diff Feb 11, 2014
tests/admin_views/tests.py
+ self.client.logout()
+
+ def test_limit_choices_to_as_callable(self):
+ """Test for ticket 2445 changes to admin."""
+ character1 = Character(username='threepwood')
+ character2 = Character(username='marley')
+
+ character1.last_action = datetime.datetime.today() + datetime.timedelta(days=1)
+ character1.save()
+
+ character2.last_action = datetime.datetime.today() - datetime.timedelta(days=1)
+ character2.save()
+
+ response = self.client.get('/test_admin/admin/admin_views/stumpjoke/add/')
+ # The allowed option should appear twice; the limited option should not appear.
+ self.assertEqual(response.content.count('threepwood'), 2)
@timgraham
Django member
timgraham added a note Feb 11, 2014

instead of character1 and character2 I would use threepwood and marley and then use threepwood.name, etc. instead of repeating the string

@timgraham
Django member
timgraham added a note Feb 11, 2014

please check this test on Python 3

Traceback (most recent call last):
  File "/home/tim/code/django/tests/admin_views/tests.py", line 3735, in test_limit_choices_to_as_callable
    self.assertEqual(response.content.count('threepwood'), 2)
TypeError: expected an object with the buffer interface
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on an outdated diff Feb 11, 2014
tests/model_forms/models.py
@@ -10,6 +10,7 @@
import os
import tempfile
+import datetime
@timgraham
Django member
timgraham added a note Feb 11, 2014

alphabetize imports (put before os)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on an outdated diff Feb 11, 2014
tests/model_forms/tests.py
@@ -1878,3 +1885,34 @@ class Form2(forms.Form):
self.assertEqual(list(type(str('NewForm'), (ModelForm, Mixin, Form), {})().fields.keys()), ['name'])
self.assertEqual(list(type(str('NewForm'), (ModelForm, Form, Mixin), {})().fields.keys()), ['name', 'age'])
self.assertEqual(list(type(str('NewForm'), (ModelForm, Form), {'age': None})().fields.keys()), ['name'])
+
+
+class LimitChoicesToTest(TestCase):
+ """
+ Tests the functionality of ``limit_choices_to``.
+ """
+ def setUp(self):
+ self.user1 = Character(username='threepwood')
@timgraham
Django member
timgraham added a note Feb 11, 2014

same thing with .create() and easier names to remember

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@adamsc64 adamsc64 Simplified unit tests for #2445 as per comments by @timgraham on Feb …
…10, 2014.

- Used one-statement create() method invocation instead of multi-line
  attribute assignment.
- Renamed symbol names in unit tests to be more descriptive.
- Used attribute data instead of raw strings when asserting test success
  condition.
- Converted response.content (bytes object in Python 3) into type 'str' to
  assert successful test condition using str.count method.
- Alphabetized import statements.
acab55c
@timgraham timgraham commented on the diff Feb 11, 2014
tests/admin_views/tests.py
+ def tearDown(self):
+ self.client.logout()
+
+ def test_limit_choices_to_as_callable(self):
+ """Test for ticket 2445 changes to admin."""
+ threepwood = Character.objects.create(
+ username='threepwood',
+ last_action=datetime.datetime.today() + datetime.timedelta(days=1),
+ )
+ marley = Character.objects.create(
+ username='marley',
+ last_action=datetime.datetime.today() - datetime.timedelta(days=1),
+ )
+ response = self.client.get('/test_admin/admin/admin_views/stumpjoke/add/')
+ # The allowed option should appear twice; the limited option should not appear.
+ self.assertEqual(str(response.content).count(threepwood.username), 2)
@timgraham
Django member
timgraham added a note Feb 11, 2014

We can actually use assertContains and assertNotContains to simplify things here. I'm making the change and committing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham
Django member

merged in eefc88f, thanks!

@timgraham timgraham closed this Feb 11, 2014
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.