[Django] #27563: Move "Apply limit_choices_to" code from BaseModelForm to fields_for_model()
Django
------------------------------------------------+------------------------
Reporter: Jon Dufresne | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Forms | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
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 `ModelForm`s. 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
--
Ticket URL: <https://code.djangoproject.com/ticket/27563>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Jon Dufresne | Owner: nobody
Cleanup/optimization |
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jon Dufresne):
* has_patch: 0 => 1
Comment:
[https://github.com/django/django/pull/7651 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/27563#comment:1>
Django
Reporter: Jon Dufresne | Owner: nobody
Cleanup/optimization |
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Claude Paroz):
* stage: Unreviewed => Ready for checkin
Comment:
Looks good!
--
Ticket URL: <https://code.djangoproject.com/ticket/27563#comment:2>
Django
Reporter: Jon Dufresne | Owner: nobody
Cleanup/optimization |
Component: Forms | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jon Dufresne):
* status: new => closed
* resolution: => fixed
Comment:
Not sure why my
[https://github.com/django/django/commit/6abd6c598ea23e0a962c87b0075aa2f79f9ead36
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27563#comment:3>
Django
Reporter: Jon Dufresne | Owner: nobody
Type: Cleanup/optimization | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* status: closed => new
* severity: Normal => Release blocker
* version: master => 1.11
* has_patch: 1 => 0
* resolution: fixed =>
* stage: Ready for checkin => Accepted
Comment:
Reopening per the regression reported in #27937.
--
Ticket URL: <https://code.djangoproject.com/ticket/27563#comment:4>
Django
Reporter: Jon Dufresne | Owner: nobody
Type: Cleanup/optimization | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Simon Charette):
It looks like something is wrong with
[https://github.com/django/django/blob/e7033e00f8e1ba2ffe538e56c5088a7e94c2e45d/django/forms/models.py#L1158
ModelChoiceField.__deepcopy__()], it should be assigning
`self.queryset.all()` to make sure `_result_cache` isn't shared between
instances.
--
Ticket URL: <https://code.djangoproject.com/ticket/27563#comment:5>
Django
Reporter: Jon Dufresne | Owner: nobody
Type: Cleanup/optimization | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* has_patch: 0 => 1
Comment:
Thanks Simon. I created a [https://github.com/django/django/pull/8178 PR]
with your suggestion.
--
Ticket URL: <https://code.djangoproject.com/ticket/27563#comment:6>
Django
Reporter: Jon Dufresne | Owner: nobody
Type: Cleanup/optimization | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by GitHub <noreply@…>):
In [changeset:"44f9241c48e28823b140bc4ec7515f5a88b88c32" 44f9241c]:
{{{
#!CommitTicketReference repository=""
revision="44f9241c48e28823b140bc4ec7515f5a88b88c32"
Refs #27563 -- Fixed ModelChoiceField.__deepcopy__() so forms don't share
a queryset cache.
Thanks Luke Benstead for the report Simon Charettes for the fix.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27563#comment:7>
Django
Reporter: Jon Dufresne | Owner: nobody
Type: Cleanup/optimization | Status: new
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"a95616944b9cb58c2547d1f08590b3ad3a142fe2" a956169]:
{{{
#!CommitTicketReference repository=""
revision="a95616944b9cb58c2547d1f08590b3ad3a142fe2"
[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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27563#comment:8>
Django
Reporter: Jon Dufresne | Owner: nobody
Component: Forms | Version: 1.11
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* status: new => closed
* resolution: => fixed
--
Ticket URL: <https://code.djangoproject.com/ticket/27563#comment:9>
Django
Reporter: Jon Dufresne | Owner: nobody
Component: Forms | Version: 1.11
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by drunkard):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27563#comment:10>
Django
Reporter: Jon Dufresne | Owner: nobody
Type: Cleanup/optimization | Status: new
Severity: Release blocker | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by drunkard):
* status: closed => new
* resolution: fixed =>
--
Ticket URL: <https://code.djangoproject.com/ticket/27563#comment:11>
Django
Reporter: Jon Dufresne | Owner: nobody
Component: Forms | Version: 1.11
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* status: new => closed
* resolution: => fixed
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27563#comment:12>
Django
Reporter: Jon Dufresne | Owner: nobody
Component: Forms | Version: 1.11
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
The new ticket is #28345.
--
Ticket URL: <https://code.djangoproject.com/ticket/27563#comment:13>