[Django] #21753: allow generic editing views to use `form_class` and `fields` together
Django
-------------------------+-------------------------------------------------
Reporter: | Owner: gabejackson
gabejackson | Status: new
Type: New | Version: master
feature | Keywords: generic edit views form_class
Component: Forms | fields ModelForm
Severity: Normal | Has patch: 1
Triage Stage: | UI/UX: 0
Unreviewed |
Easy pickings: 0 |
-------------------------+-------------------------------------------------
The addition of the `fields` property to ModelFormMixin for generic form
views allows to set fields for the given `model` as documented here:
https://docs.djangoproject.com/en/1.6/ref/class-based-views/mixins-
editing/#django.views.generic.edit.ModelFormMixin.fields
This allows the following:
{{{
class Author(models.Model):
name = models.CharField(max_length=60)
url = models.CharField(max_length=60)
class AuthorUpdate(UpdateView):
model = Author
fields = ['name']
}}}
Which will result in a form with only the field `name`.
Unfortunately, it is currently impossible to use the `fields` in
combination with the `form_class` attribute. This means you cannot
override fields with custom ModelForms:
{{{
class Author(models.Model):
name = models.CharField(max_length=60)
url = models.CharField(max_length=60)
class AuthorForm(ModelForm):
# Customization ...
class Meta:
model = Author
class AuthorUpdate(UpdateView):
model = Author
form_class = AuthorForm
fields = ['name']
}}}
This will result in a form with all fields of Author.
This is due to the implementation of get_form_class doing this:
{{{
def get_form_class(self):
"""
Returns the form class to use in this view.
"""
if self.form_class:
return self.form_class
else:
if self.model is not None:
# If a model has been explicitly provided, use it
model = self.model
elif hasattr(self, 'object') and self.object is not None:
# If this view is operating on a single object, use
# the class of that object
model = self.object.__class__
else:
# Try to get a queryset and extract the model class
# from that
model = self.get_queryset().model
if self.fields is None:
warnings.warn("Using ModelFormMixin (base class of %s) without
"
"the 'fields' attribute is deprecated." %
self.__class__.__name__,
PendingDeprecationWarning)
return model_forms.modelform_factory(model, fields=self.fields)
}}}
It is apparent that `fields` gets ignored if `form_class` is set.
I attached a patch including a test case and fix for this issue/feature.
This could be classified as a bug, since no warning is raised when using
`form_class` combined with `fields`.
--
Ticket URL: <https://code.djangoproject.com/ticket/21753>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: gabejackson | Owner:
Type: New feature | gabejackson
Component: Forms | Status: new
Severity: Normal | Version: master
Keywords: generic edit views | Resolution:
form_class fields ModelForm | Triage Stage:
Has patch: 1 | Unreviewed
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by gabejackson):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
To be specific about customization in the ModelForm: Customization means
either: defining fields in Meta, adding further base classes, overriding
ModelForm methods (for instance __init__ for to add a django-crispy-form
helper and layout).
There is a discussion point here which was discussed with timograham in
#django-dev:
Should the behavior be:
a) Respect the fields in form_class's Meta even if fields is defined in
the View
b) Respect fields of the View even if fields of the form_class's Meta is
set
timograham's opinion on this is a) "I would expect fields to be used on
the view only if they aren't specified on the form"
My opinion is that fields of the View should always be respected since
they allow more specific functionality of the form within the view,
specifically removing fields for custom views where data is added to the
create Model, say in form_valid. A simple use case for this is the
following model:
{{{
class Voucher(models.Model):
user = models.ForeignKey(get_user_model())
amount = models.DecimalField(...)
class VoucherForm(ModelForm):
user = autocomplete_light.GenericModelChoiceField(
label='User',
widget=autocomplete_light.ChoiceWidget(
autocomplete='AutocompleteUser',
autocomplete_js_attributes={
'minimum_characters': 1,
'placeholder': u'Start typing to find a user'
},
)
)
class Meta:
model = Voucher
def clean_amount(self):
amount = self.cleaned_data['amount']
if amount < 20:
raise forms.ValidationError("amount must be greater than 20")
return amount
def __init__(self, *args, **kwargs):
super(VoucherForm, self).__init__(*args, **kwargs)
self.helper = FormHelper()
self.helper.form_tag = False
self.helper.layout = Layout(.....)
class VoucherCreateView(CreateView):
model = Voucher
form_class = VoucherForm
class VoucherCreateForUserView(CreateView):
model = Voucher
form_class = VoucherForm
fields = ('amount',)
def form_valid(self, form):
self.object = form.save(commit=False)
# Get user from somewhere...
self.object.user =
get_user_model().objects.get(pk=self.kwargs['user_id'])
self.object.save()
return HttpResponseRedirect(self.get_success_url())
}}}
With the current approach (as in Django 1.6) or according to opinion a)
you must create a ModelForm for each of these use cases redefining
Meta.fields.
timo and I agree that there at least but be some kind of warning when
using form_class and fields together.
--
Ticket URL: <https://code.djangoproject.com/ticket/21753#comment:1>
Django
Reporter: gabejackson | Owner:
Type: New feature | gabejackson
Component: Forms | Status: new
Severity: Normal | Version: master
Keywords: generic edit views | Resolution:
form_class fields ModelForm | Triage Stage:
Has patch: 1 | Unreviewed
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by mjtamlyn):
I would suggest this should only give a warning, possibly even
`ImproperlyConfigured`, not change the form. From options a) and b) above
it is not clear what the "correct" approach is - and either has the
potential to confuse. I do not want class based views to fall into a trap
of allowing myriad hooks to customise the form class, like `form.Meta`
allows with fields. The reason why `fields` was introduced on these views
is to allow you to ensure security when using an auto generated
`form_class`. If you're setting the `form_class`, you can obviously set up
the `form_class` correctly.
Personally, I'd rather get rid of automatic `form_class` generation all
together, but that's another story.
--
Ticket URL: <https://code.djangoproject.com/ticket/21753#comment:2>
Django
Reporter: gabejackson | Owner:
Type: New feature | gabejackson
Severity: Normal | Version: master
Keywords: generic edit views | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by mjtamlyn):
* component: Forms => Generic views
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/21753#comment:3>
Django
Reporter: gabejackson | Owner:
Type: New feature | gabejackson
Component: Generic views | Status: new
Severity: Normal | Version: master
Keywords: generic edit views | Resolution:
form_class fields ModelForm | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by gabejackson):
Main argument from discussion with mjtamyln: "one of the common criticisms
of our CBVs is that they have too many customisation points and do too
much magic (or implicit behaviour). Let's not add any more"
Generally fatter forms and thinner views are recommended. The above use-
case should be solved by subclassing ModelForm and adapting the fields,
then use the new ModelForm in the View.
Dynamic form creation dependent of request data can be done by passing
data to Form.__init__() and adapting the fields there. Alternatively one
can change the form_class in get_form_class by doing something like:
return SuperForm if self.request.user.is_superuser else NormalForm.
It should be noted in the docs that `fields` was added to View from a
security point of view, not for customization of forms.
The question remains if using form_class and fields together should raise
an ImproperlyConfigured or just fail silently. Compare this to the
approach of using queryset vs model in a View: This will fail silently
preferring queryset over model. This seems logical because queryset allows
more fine-grained control. Does the same hold true for fields and
form_class? One might expect that fields will just work with form_class,
or one might make the reverse assumption that fields defined in form_class
will override what is in fields of the View.
to be discussed.
--
Ticket URL: <https://code.djangoproject.com/ticket/21753#comment:4>
Django
`form_class` and `fields` are specified
-------------------------------------+-------------------------------------
Reporter: gabejackson | Owner:
Type: | gabejackson
Cleanup/optimization | Status: new
Component: Generic views | Version: master
Severity: Normal | Resolution:
Keywords: generic edit views | Triage Stage: Accepted
form_class fields ModelForm | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by timo):
* needs_better_patch: 0 => 1
* type: New feature => Cleanup/optimization
Comment:
I would like to see `ImproperlyConfigured` as I don't see any benefit to
failing silently.
--
Ticket URL: <https://code.djangoproject.com/ticket/21753#comment:5>
Django
`form_class` and `fields` are specified
-------------------------------------+-------------------------------------
Reporter: gabejackson | Owner:
Cleanup/optimization | Status: assigned
Component: Generic views | Version: master
Severity: Normal | Resolution:
Keywords: generic edit views | Triage Stage: Accepted
form_class fields ModelForm | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: new => assigned
* owner: gabejackson => berkerpeksag
--
Ticket URL: <https://code.djangoproject.com/ticket/21753#comment:6>
Django
`form_class` and `fields` are specified
-------------------------------------+-------------------------------------
Reporter: gabejackson | Owner:
Type: | berkerpeksag
Cleanup/optimization | Status: assigned
Component: Generic views | Version: master
Severity: Normal | Resolution:
Keywords: generic edit views | Triage Stage: Accepted
form_class fields ModelForm | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by berkerpeksag):
* needs_better_patch: 1 => 0
Comment:
https://github.com/django/django/pull/3518
--
Ticket URL: <https://code.djangoproject.com/ticket/21753#comment:7>
Django
`form_class` and `fields` are specified
-------------------------------------+-------------------------------------
Reporter: gabejackson | Owner:
Type: | berkerpeksag
Component: Generic views | Version: master
Keywords: generic edit views | Triage Stage: Accepted
form_class fields ModelForm | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"343162410f9270012e4fdd8396d542b39cc63269"]:
{{{
#!CommitTicketReference repository=""
revision="343162410f9270012e4fdd8396d542b39cc63269"
Fixed #21753 -- Raised exception when both `form_class` and `fields` are
specified.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21753#comment:8>