Opened 17 years ago

Closed 9 years ago

Last modified 9 years ago

#2445 closed New feature (fixed)

[patch] allow callable values for limit_choices_to

Reported by: michael@… Owned by: Tim Graham <timograham@…>
Component: Core (Other) Version: dev
Severity: Normal Keywords: sprint2013
Cc: zeraien@…, gary.wilson@…, django@…, davidgrant@…, mpjung@…, hr.bjarni+django@…, Odin Hørthe Omdal, remco@…, joseph.spiros@…, mmitar@…, leo@…, Chris Chambers, russamos, semenov@…, andreterra@…, stj, dschruth, danny.adair@…, Steven Bisseker, philipe.rp@…, kmike84@…, christopher.r.adams@…, someuniquename@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Allow limit_choices_to to test for and invoke callable value arguments. This would allow filtering on dynamically determined values. It enhances the possibilities for customising the admin interface. This may be related to Ticket 2193 (sounds similar at any rate).

For example:

def assigned_tasks():
   return get_assigned_tasks_id_list(blah, blah)

class TimeRecord(models.Model):
   task = models.ForeignKey(Task, limit_choices_to = {'id__in': assigned_tasks})

Attachments (6)

query_value.diff (578 bytes) - added by michael@… 17 years ago.
test callable(value) in db/models/query.py
evallimitchoices.diff (4.8 KB) - added by michael@… 17 years ago.
Patch to allow evaluation of callables in limit_choices_to
choices_func.diff (2.1 KB) - added by Dmitri Fedortchenko <zeraien@…> 16 years ago.
This patch is still a bit unpredictable with edit_inline objects, but works with normal relationships.
choices_func.2.diff (2.1 KB) - added by Dmitri Fedortchenko <zeraien@…> 16 years ago.
Accidentally included some debug print statements in the previous patch...whoops
choices_func_newforms-admin.patch (2.5 KB) - added by David Grant <davidgrant@…> 15 years ago.
patch for newforms-admin branch. Just one line is different. Still doesn't work with inlines
choices_for_foo.diff (5.8 KB) - added by stj 12 years ago.
patch against r15803

Download all attachments as: .zip

Change History (71)

Changed 17 years ago by michael@…

Attachment: query_value.diff added

test callable(value) in db/models/query.py

comment:1 Changed 17 years ago by anonymous

Cc: gary.wilson@… added

comment:2 Changed 17 years ago by Adrian Holovaty

I agree with the concept, but the patch is incorrect -- the check for callable() should be made only on the limit_choices_to values, not in the code that parses *every* query. Could you have another look?

comment:3 Changed 17 years ago by Adrian Holovaty

Summary: [patch] allow callable values for limit_choices_toallow callable values for limit_choices_to

Removing [patch] from the subject, because the patch is invalid.

comment:4 Changed 17 years ago by Stefano J. Attardi <django@…>

Cc: django@… added

comment:5 Changed 17 years ago by michael@…

I took another look - I only have limited familiarity with the code.

The value of limit_choices_to is passed to complex_filter() in three places:

django/contrib/admin/views/main.py
django/db/models/manipulators.py
django/db/models/fields/__init__.py

I believe that calls to complex_filter() are the only places limit_choices_to is actually used.

The limit_choices_to is always obtained via referencing some fragment such as .rel.limit_choices_to
The "rel" class needs a method (hopefully in some base class) that evaluates its' limit_choices_to and returns a new hashmap:

    def get_limit_choices_to(self):
        limiters = {}
        if self.limit_choices_to:
            for id in self.limit_choices_to:
                value = self.limit_choices_to[id]
                if callable(value):
                    value = value()
                limiters[id] = value
        return limiters

The existing direct references to ref.limit_choices_to need to change to ref.get_limit_choices_to()

Does this sound like I'm on the right track? I'm proceeding on these lines for now.

Changed 17 years ago by michael@…

Attachment: evallimitchoices.diff added

Patch to allow evaluation of callables in limit_choices_to

comment:6 Changed 17 years ago by michael@…

Summary: allow callable values for limit_choices_to[patch] allow callable values for limit_choices_to

I attached a new patch that attempts to implement callables in limit_choices_to without involving query.py. I've put [patch] back in the header. The new patch is larger - I'm open to suggestions on how it might be more simply achieved.

comment:7 Changed 16 years ago by Gary Wilson <gary.wilson@…>

Needs documentation: set
Needs tests: set
Triage Stage: UnreviewedDesign decision needed

I like the idea (touched on in #1891) of getting rid of limit_choices_to in favor of a choices that accepts QuerySets or callables.

comment:8 Changed 16 years ago by webograph <webograph@…>

i think that callables should be passed a reference to self, at least if they accept it -- think of the following model:

class Mother(models.Model):
    firstborn = models.ForeignKey('Child', limit_choices_to={'mother':lambda me: me})
    
class Child(models.Model):
    mother = models.ForeignKey('Mother', related_name='children')

comment:9 Changed 16 years ago by jkocherhans

Has patch: unset
Needs documentation: unset
Needs tests: unset
Triage Stage: Design decision neededAccepted

We discussed this in Chicago and I think the plan is to allow some special methods to generate the choices.

class SomeOtherModel(models.Model):
    name = models.CharField(max_length=255)

class MyModel(models.Model):
    test = models.ForeignKey(SomeOtherModel):

    def choices_for_test(self):
        """Hook for providing custom choices. Returns a queryset or an iterable of choices tuples."""
        return SomeOtherModel.objects.all()

comment:10 Changed 16 years ago by Dmitri Fedortchenko <zeraien@…>

Cc: zeraien@… added

Is anyone still working on this?

I've been seeking this functionality so I wrote a patch that actually implements jkocherhanss way of dealing with this.

By defining a function in your model called choices_forFIELDNAME you can limit the choices using data from the current row...

The patch also allows for using QuerySets for the choices attribute for any field...

I am still working on getting it to work with edit_inline, but it works perfectly with normal models.

Changed 16 years ago by Dmitri Fedortchenko <zeraien@…>

Attachment: choices_func.diff added

This patch is still a bit unpredictable with edit_inline objects, but works with normal relationships.

Changed 16 years ago by Dmitri Fedortchenko <zeraien@…>

Attachment: choices_func.2.diff added

Accidentally included some debug print statements in the previous patch...whoops

comment:11 Changed 16 years ago by Jannis Leidel

Has patch: set

comment:12 Changed 16 years ago by Dmitri Fedortchenko <zeraien@…>

Patch needs improvement: set

Patch needs work since it does not work with edit inlines.

Changed 15 years ago by David Grant <davidgrant@…>

patch for newforms-admin branch. Just one line is different. Still doesn't work with inlines

comment:13 Changed 15 years ago by David Grant <davidgrant@…>

Cc: davidgrant@… added

What's the problem with inlines? I've been trying to figure this out all day and I can't seem to figure out what's going on. It seem to be calling the choices_for* on out-of-date instances. The only thing I can think of is that the code in models/base.py isn't being called again, so there is an old choices_for* method attached to the field.

comment:14 Changed 15 years ago by David Grant <davidgrant@…>

I think the problem that appears with inlines is in attaching this method to the model's field (see content of patch). The choices_for__somefield method should not be a Field method it should be a model method (which is what it looks like anyways). This business of attaching it to the field needs to go away and the choices_for__somefield method needs to be called to filter out the choices for somefield within the context of some instance of the model (the self that is passed in).

This problem goes away if you always create an instance of the class that has the limited-choice field in it before calling the choices method. Then the choices choices_for__somefield gets reattached to the field and when you ask the field what it's choices are (via get_choices method) you get what you expect...but this is not how inlines work right now, where an instance of the inlined object is not always instantiated before looking at the field's choices, hence you will get the choices of the last-instantiated child object! But re-laod the page again and it works.

Only an instance of the model you are editing is instantiated (the non-inlined model) right away, which is why this hack works when editing the inlined model in non-in-lined mode (editing the child directly rather than through the parent).

comment:15 Changed 15 years ago by Alex Gaynor

David's approach looks to be correct, however related fields don't actually have a limit_choices_to parameter, so I think we should combine this and #1891 and remove limit_choices_to entirely, and allow callables and querysets for choices, as this does.

comment:16 Changed 15 years ago by Matias Surdi <matiassurdi@…>

Do yo know if this patch is going to be applied to trunk in any momment?

comment:17 Changed 15 years ago by Michael P. Jung

Cc: mpjung@… added

comment:18 Changed 15 years ago by Marc Fargas

Needs documentation: set
Needs tests: set

The patch is missing tests and documentation.

comment:19 Changed 14 years ago by hejsan

Cc: hr.bjarni+django@… added

comment:20 Changed 14 years ago by hejsan

This is quite important, without this functionality one is not able to make choices context sensitive without some major hack.

The last proposed solution, to allow for a choices_for_BLA() function is definitely the way to go, that is the only way that allows you to check your self variables and make the choices accordingly in a dynamic way.

Please someone smarter than me fix the choices_func_newforms-admin.patch patch to work for inlines also :)

comment:21 Changed 14 years ago by Odin Hørthe Omdal

Cc: Odin Hørthe Omdal added

comment:22 Changed 13 years ago by Remco Wendt

Cc: remco@… added

comment:23 Changed 13 years ago by Joseph Spiros

Cc: joseph.spiros@… added

I'm not sure if this covers every use case, or if this is just a stupid hack that shouldn't be considered a proper solution to this problem, but what I've done is create classes that implement add_to_query (instances of which can be supplied to, I believe, any method that also accepts a Q object), and which dynamically create and add appropriate Q objects to the query upon invocation. My use case for the moment is limiting the choice of ContentTypes on the content type ForeignKey used by a GenericForeignKey. See ContentTypeLimiter and subclasses in my project.


comment:24 Changed 13 years ago by Greg Turner

By the way, limit_choices_to can indeed be a callable (the documentation example says limit_choices_to=datetime.date.now), but doesn't include self.

comment:25 Changed 13 years ago by Mitar

Cc: mmitar@… added

comment:26 Changed 13 years ago by banyanleo

Cc: leo@… added

comment:27 Changed 12 years ago by Chris Chambers

Cc: Chris Chambers added

comment:28 in reply to:  23 Changed 12 years ago by anentropic

Replying to jspiros:

I'm not sure if this covers every use case, or if this is just a stupid hack that shouldn't be considered a proper solution to this problem, but what I've done is create classes that implement add_to_query (instances of which can be supplied to, I believe, any method that also accepts a Q object), and which dynamically create and add appropriate Q objects to the query upon invocation. My use case for the moment is limiting the choice of ContentTypes on the content type ForeignKey used by a GenericForeignKey. See ContentTypeLimiter and subclasses in my project.

That's really clever! I'm not clear how, or if it's possible, to access the current instance from within a *Limiter class though?

comment:29 Changed 12 years ago by russamos

Cc: russamos added

comment:30 Changed 12 years ago by Ilya Semenov

Cc: semenov@… added

comment:31 Changed 12 years ago by Andy Terra

Cc: andreterra@… added

Changed 12 years ago by stj

Attachment: choices_for_foo.diff added

patch against r15803

comment:32 Changed 12 years ago by stj

Cc: stj added

Spent some time at the PyCon 2011 sprint session on this and came up with a new implementation.

The choices_for_FOO method needs to return a QuerySet though in this implementation is no check for that.
In a small test application I tested this with InlineModelAdmin and found no bugs.

comment:33 Changed 12 years ago by dschruth

Cc: dschruth added

comment:34 Changed 12 years ago by Łukasz Rekucki

Type: enhancementNew feature

comment:35 Changed 12 years ago by Łukasz Rekucki

Severity: normalNormal

comment:36 Changed 12 years ago by danny.adair@…

Cc: danny.adair@… added

comment:37 Changed 12 years ago by anonymous

Easy pickings: unset
Resolution: fixed
Status: newclosed

comment:38 Changed 12 years ago by Karen Tracey

Resolution: fixed
Status: closedreopened

I don't know if the previous update was a mistake or spam, but this is not fixed. Using r16279 specifying a callable for limit_choices_to raises a TypeError _filter_or_exclude() argument after ** must be a dictionary.

comment:39 Changed 12 years ago by Steven Bisseker

Cc: Steven Bisseker added
UI/UX: unset

comment:40 Changed 12 years ago by Steven Bisseker

UI/UX: set

comment:41 Changed 11 years ago by Ben Davis

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

Unsetting "needs test" and "needs documentation", as the latest patch seems to have both.

comment:42 Changed 11 years ago by myusuf3

Patch needs improvement: set

The newest patch no longer applies cleanly.

Setting patch needs improvement.

comment:43 Changed 11 years ago by Felipe 'chronos' Prenholato

Cc: philipe.rp@… added

comment:44 Changed 11 years ago by Mikhail Korobov

Cc: kmike84@… added

comment:45 Changed 10 years ago by Aymeric Augustin

Status: reopenednew

comment:46 Changed 10 years ago by Sébastien Fievet

Keywords: sprint2013 added
Owner: changed from nobody to Sébastien Fievet
Status: newassigned

I'll update the patch from @stj so it apply to master.

comment:47 Changed 10 years ago by Sébastien Fievet

comment:48 Changed 10 years ago by Jannis Leidel

UI/UX: unset

Repeating what I said on IRC, I think adding a field specific option into the model definition is a bad idea and doesn't have precedence in the model API. Instead I would think making it possible to pass a callable into the choices parameter of the field as mentioned by Gary a couple of years ago, makes more sense.

comment:49 Changed 10 years ago by Jannis Leidel

This is related to #13181

comment:50 Changed 10 years ago by Christopher Adams

Cc: christopher.r.adams@… added
Owner: changed from Sébastien Fievet to Christopher Adams

comment:51 Changed 10 years ago by Christopher Adams

Owner: Christopher Adams deleted
Status: assignednew

I just submitted a pull request for this ticket at https://github.com/django/django/pull/1600.

  • 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.
  • I included some new tests. All old test cases run successfully.

comment:52 Changed 10 years ago by Christopher Adams

Patch needs improvement: unset
Version: master

comment:53 Changed 9 years ago by Evstifeev Roman

Cc: someuniquename@… added

comment:54 Changed 9 years ago by Christopher Adams

Owner: set to Christopher Adams
Status: newassigned

comment:55 Changed 9 years ago by Christopher Adams

Triage Stage: AcceptedReady for checkin

I submitted a new pull request that should implement this feature:
https://github.com/django/django/pull/2233

All unit tests run on my local dev box. Added new unit test (admin_views.tests.LimitChoicesToTest) for the new feature.

Summary:

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.

comment:56 Changed 9 years ago by Christopher Adams

Owner: Christopher Adams deleted
Status: assignednew
Triage Stage: Ready for checkinAccepted

comment:57 Changed 9 years ago by Tim Graham

Patch needs improvement: set

I've left some comments for improvement on the pull request.

comment:58 Changed 9 years ago by Christopher Adams

Owner: set to Christopher Adams
Status: newassigned

Thanks Tim. I'll make the changes as requested.

comment:59 Changed 9 years ago by Christopher Adams

Owner: Christopher Adams deleted
Patch needs improvement: unset
Status: assignednew

The changes requested by @timo have been made at 5d4b7a1. I updated the pull request. See https://github.com/django/django/pull/2233.

commit 5d4b7a1c466174bfe05f32545652519446354c25
Author: Christopher Adams <christopher.r.adams@…>
Date: Sun Feb 9 12:02:03 2014 -0500

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.

comment:60 Changed 9 years ago by Tim Graham <timograham@…>

Owner: set to Tim Graham <timograham@…>
Resolution: fixed
Status: newclosed

In eefc88feefec0c3685bfb102714530b751b4ae90:

Fixed #2445 -- Allowed limit_choices_to attribute to be a callable.

ForeignKey or ManyToManyField attribute limit_choices_to can now
be a callable that returns either a Q object or a dict.

Thanks michael at actrix.gen.nz for the original suggestion.

comment:61 Changed 9 years ago by Daniel Hahler

Thanks! This looks very useful.

I wonder if it could be changed to pass along any request object to the callback, which would allow to look at request.user and then limit the choices to objects that the current user owns?

From what I understand the request object may not be available, e.g. when used via management commands, where None would be passed to the callback then.

comment:62 Changed 9 years ago by Tim Graham

Not really, the request is generally not available in a form.

comment:63 Changed 9 years ago by Christopher Adams

I know some people make the request available in thread local storage using middleware (see http://stackoverflow.com/questions/1057252/how-do-i-access-the-request-object-or-any-other-variable-in-a-forms-clean-met#answer-1057418). This effectively hacks Django to make its behavior similar to frameworks that make the request object globally available, such as Flask. Note however that many people believe this pattern is a bad practice.

comment:64 Changed 9 years ago by Baptiste Mispelon <bmispelon@…>

In bfb11b95626f39e2f5e18d97d7761c6f93dcc1a9:

Fixed #23795 -- Fixed a regression in custom form fields

Custom form fields having a queryset attribute but no
limit_choices_to could no longer be used in ModelForms.

Refs #2445.

Thanks to artscoop for the report.

comment:65 Changed 9 years ago by Baptiste Mispelon <bmispelon@…>

In 606c57a132a1e1e680853c014efc41110ee75a80:

[1.7.x] Fixed #23795 -- Fixed a regression in custom form fields

Custom form fields having a queryset attribute but no
limit_choices_to could no longer be used in ModelForms.

Refs #2445.

Thanks to artscoop for the report.

Backport of bfb11b95626f39e2f5e18d97d7761c6f93dcc1a9 from master.

Conflicts:

django/forms/fields.py

Note: See TracTickets for help on using tickets.
Back to Top