Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#27563 closed Cleanup/optimization (fixed)

Move "Apply limit_choices_to" code from BaseModelForm to fields_for_model()

Reported by: Jon Dufresne Owned by: nobody
Component: Forms Version: 1.11
Severity: Release blocker Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I frequently use fields_for_model() to generate form fields dynamically on a model form. The form may have different fields based on the request, system settings, or other input. Using fields_for_model() is very convenient to build these dynamic fields. For the most part, it is built consistently had the field originally been defined on Meta.fields.

One shortfall I noticed, limit_choices_to is not applied to form field querysets the way it typically is for ModelForms. To make the fields_for_model() function more convenient and consistent with fields generated by ModelForm, I'd like to suggest moving the "Apply limit_choices_to" code from BaseModelForm to fields_for_model().

Currently here: https://github.com/django/django/blob/3507d4e773aa9ff2336e7230ba231c4ba6eb568f/django/forms/models.py#L294-L300

Suggested location: https://github.com/django/django/blob/3507d4e773aa9ff2336e7230ba231c4ba6eb568f/django/forms/models.py#L172-L173

Change History (13)

comment:1 by Jon Dufresne, 7 years ago

Has patch: set

comment:2 by Claude Paroz, 7 years ago

Triage Stage: UnreviewedReady for checkin

Looks good!

comment:3 by Jon Dufresne, 7 years ago

Resolution: fixed
Status: newclosed

Not sure why my commit didn't close the ticket. If anyone is aware of something I did wrong, please let me know so I can change my workflow.

Version 0, edited 7 years ago by Jon Dufresne (next)

comment:4 by Tim Graham, 7 years ago

Has patch: unset
Resolution: fixed
Severity: NormalRelease blocker
Status: closednew
Triage Stage: Ready for checkinAccepted
Version: master1.11

Reopening per the regression reported in #27937.

comment:5 by Simon Charette, 7 years ago

It looks like something is wrong with ModelChoiceField.__deepcopy__(), it should be assigning self.queryset.all() to make sure _result_cache isn't shared between instances.

comment:6 by Tim Graham, 7 years ago

Has patch: set

Thanks Simon. I created a PR with your suggestion.

comment:7 by GitHub <noreply@…>, 7 years ago

In 44f9241c:

Refs #27563 -- Fixed ModelChoiceField.deepcopy() so forms don't share a queryset cache.

Thanks Luke Benstead for the report Simon Charettes for the fix.

comment:8 by Tim Graham <timograham@…>, 7 years ago

In a956169:

[1.11.x] Refs #27563 -- Fixed ModelChoiceField.deepcopy() so forms don't share a queryset cache.

Thanks Luke Benstead for the report Simon Charettes for the fix.

Backport of 44f9241c48e28823b140bc4ec7515f5a88b88c32 from master

comment:9 by Tim Graham, 7 years ago

Resolution: fixed
Status: newclosed

comment:10 by drunkard, 7 years ago

This change breaks inheriting from django.forms.models.ModelForm directly.

I'm upgrading to python3.6 + django-1.11.2, and gets wrong with this commit. I'm using dynamic limit_choices_to function to get user specific filter expr, one of my function is like:

def limit_export_branch():                                                     
    user = current_user()                                                         
    if user:                                                                      
        bs = user.userprofile.branches_with_perms(['add_export', 'change_export'])
        return Q(pk__in=bs.values_list('pk', flat=True),                       
                 businesses__name=BUSINESS_BROADBAND)                          
    return Q(pk=-1)  # deny to avoid info leak

It just called once during develop server init, and won't be call while open a USER EDIT FORM, but it work in django admin edit page, by reading the code, django admin build form using modelform_factory(), with this commit reverted, my customized form works just fine. So, I think it's a regression.

Or, the usage of ModelForm changed? but newest doc not mentioned.

comment:11 by drunkard, 7 years ago

Resolution: fixed
Status: closednew

comment:12 by Tim Graham, 7 years ago

Resolution: fixed
Status: newclosed

This change is released, so please open a new ticket at this point. I'm not sure if what you provided is sufficient to reproduce the issue. A sample project or a test would be helpful.

comment:13 by Tim Graham, 7 years ago

The new ticket is #28345.

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