Opened 5 years ago

Last modified 9 months ago

#25306 new New feature

Allow a limit_choices_to callable to accept the current model instance

Reported by: Miles Hutson Owned by: nobody
Component: Forms Version: master
Severity: Normal Keywords:
Cc: Matthijs Kooijman Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Real life problem: I have a site with two models Person and Storyteller. Storytellers can tell stories (represented by a TextField) about a Person. This relationship is represented by a ForeignKey from Storyteller to Person. An admin can choose a feature story about a Person. This is represented by a foreign key from Person to Storyteller. The change view for a given Person should only display Storytellers that have a foreign key to that person. There isn't an easy way to do this currently.

Change History (5)

comment:1 Changed 5 years ago by Tim Graham

How about using AdminSite.form and customizing the foreign key's queryset in form.__init__() as described in Changing the queryset.

comment:2 Changed 5 years ago by Miles Hutson

Thanks for the pointer Tim! I've been digging into the admin code, because I honestly hadn't needed to before. I'm sure that what you mentioned will end up being what I have to do. But part of the point of Django admin seems to be that people don't end up having to dive into the code for an admin interface when they're building a website. Hence why there are options like blank and limit_choices_to on fields. The suggestion in this feature request is that model fields have something similar to limit_choices_to. Perhaps this parameter could be passed a function. When a ForeignKey is populated in a change view, this function could be passed the relevant instance, and could return a queryset to filter on.

Perhaps this could be done by adding the behavior described to limit_choices_to? I believe this would involve modifying the __init__ method of forms.models.BaseModelForm.

Last edited 5 years ago by Tim Graham (previous) (diff)

comment:3 Changed 5 years ago by Miles Hutson

Anyone who googles this and finds themselves in the same rut, here's a useful stopgap. Override the ModelAdmin's form with a form subclassed off of this.

class LimitChoicesBaseForm(ModelForm):
    limit_choices_methods = []

    def __init__(self, data=None, files=None, auto_id='id_%s', prefix=None,
                 initial=None, error_class=ErrorList, label_suffix=None,
                 empty_permitted=False, instance=None):
        super(LimitChoicesBaseForm, self).__init__(data=data, files=files, auto_id=auto_id, prefix=prefix,
                                                   initial=initial, error_class=error_class, label_suffix=label_suffix,
                                                   empty_permitted=empty_permitted, instance=instance)
        for method in self.limit_choices_methods:
            formfield = self.fields[method['field_name']]
            if hasattr(formfield, 'queryset'):
                filter_q_object = method['method'](instance)
                formfield.queryset = formfield.queryset.filter(filter_q_object)

comment:4 Changed 5 years ago by Tim Graham

Component: contrib.adminForms
Summary: Should be an easier way to restrict the choices for a ForeignKeyAllow a limit_choices_to callable to accept the current model instance
Triage Stage: UnreviewedAccepted
Version: 1.8master

It might be possible to allow passing the instance to limit_choices_to, but I'm not sure. Accepting on the basis of investigating the idea. See #2445 for related work.

comment:5 Changed 9 months ago by Matthijs Kooijman

Cc: Matthijs Kooijman added

I'm running into this same problem. The proposed solution of letting limit_choices_to accept a model instance seems reasonable to me, but on closer inspection (see below) probably not feasible, unfortunately.

I guess this will need some compatibility check (to support both the old argumentless version as well as a version with an instance argument). It seems checking this using e.g. utils.inspect.func_supports_parameter() and using a kwarg in the call seems reasonable, or checking with utils.inspect.method_has_no_args() and using a positional argument?

It seems that limit_choices_to is resolved in only three places:
$ git grep callable.*limit_choices_to
contrib/admin/widgets.py: if callable(limit_choices_to):
db/models/fields/related.py: if callable(self.remote_field.limit_choices_to):
forms/models.py: if callable(self.limit_choices_to):

The first one is used by the ForeignKeyRawIdWidget and ManyToManyRawIdWidget. It is probably hard to get the model instance there, but the value is only used to provide a "related" link that shows related objects for a raw id widget. Given this widget is already quite spartan, it might be ok to not have these choices limited (e.g. passing None as the instance and let the callable decide what to do).

The second and third one are part of the get_limit_choices_to() methods on RelatedField and ModelChoiceField

For ModelChoiceField, it seems the instance can be passed by BaseModelForm.__init__ through apply_limit_choices_to_to_formfield. This leaves one path through django.forms.fields_for_model, but only when called with apply_limit_choices_to=True which (though it is the default value) does not seem to happen within the Django codebase (except for tests).

For RelatedField, there are two paths. ForeignKey.validate has the instance, so it can simply pass that on. The last path is through Field.get_choices, which has no access to the model instance (and is again called from a few places without access to an instance AFAICS).

Looking at the above, there are quite some places where there is no instance available, so that would probably require signficant refactoring to move the resolution of limit_choices_to up in the call hierarchy where sufficient context is available (and it would not fix it for get_choices, which is probably a public API).

Alternatively, you could make sure to pass the instance wherever it is available and None elsewhere, as a best effort better-than-nothing approach, but I suspect that that would only lead to inconsistency and the illusion of really supporting this, which might not even be better than nothing.

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