Code

Opened 3 months ago

Last modified 3 months ago

#21753 new Cleanup/optimization

Generic editing views should raise ImproperlyConfigured if both `form_class` and `fields` are specified

Reported by: gabejackson Owned by: gabejackson
Component: Generic views Version: master
Severity: Normal Keywords: generic edit views form_class fields ModelForm
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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.

Attachments (2)

0001-Allow-generic-editing-views-to-use-form_class-and-fi.patch (5.5 KB) - added by gabejackson 3 months ago.
0002-Allow-generic-editing-views-to-use-form_class-and-fi.patch (5.5 KB) - added by gabejackson 3 months ago.
PEP8 remove whitespace

Download all attachments as: .zip

Change History (7)

Changed 3 months ago by gabejackson

PEP8 remove whitespace

comment:1 Changed 3 months ago by gabejackson

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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.

comment:2 Changed 3 months ago 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.

comment:3 Changed 3 months ago by mjtamlyn

  • Component changed from Forms to Generic views
  • Triage Stage changed from Unreviewed to Accepted

comment:4 Changed 3 months ago 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.

Last edited 3 months ago by mjtamlyn (previous) (diff)

comment:5 Changed 3 months ago by timo

  • Patch needs improvement set
  • Summary changed from allow generic editing views to use `form_class` and `fields` together to Generic editing views should raise ImproperlyConfigured if both `form_class` and `fields` are specified
  • Type changed from New feature to Cleanup/optimization

I would like to see ImproperlyConfigured as I don't see any benefit to failing silently.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from gabejackson to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.