Code

Opened 3 years ago

Closed 3 years ago

#15907 closed Bug (fixed)

Generic inlines ignoring the exclude information from a custom form.

Reported by: leonelfreire Owned by: nobody
Component: Forms Version: 1.3
Severity: Normal Keywords: form, exclude, generic, inline
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Suppose we have the following models:

from django.db import models
from django.contrib.contenttypes.models import ContentType
from django.contrib.contenttypes import generic

class MainModel(models.Model):
    field1 = models.CharField(max_length=255)

class Inline1(models.Model):
    main_model     = models.ForeignKey(MainModel)
    inline1_field1 = models.CharField(max_length=255)
    inline1_field2 = models.CharField(max_length=255)

class Inline2(models.Model):
    content_type   = models.ForeignKey(ContentType)
    object_id      = models.PositiveIntegerField()
    content_object = generic.GenericForeignKey()
    inline2_field1 = models.CharField(max_length=255)
    inline2_field2 = models.CharField(max_length=255)


and this admin configuration:

from django import forms
from django.contrib.contenttypes import generic
from django.contrib import admin

from bug.models import MainModel, Inline1, Inline2

class Inline1Form(forms.models.ModelForm):
    class Meta:
        model   = Inline1
        exclude = ('inline1_field2', )

class Inline2Form(forms.models.ModelForm):
    class Meta:
        model   = Inline2
        exclude = ('inline2_field2',)

class Inline1Inline(admin.StackedInline):
    model = Inline1
    form  = Inline1Form
    extra = 1

class Inline2Inline(generic.GenericStackedInline):
    model = Inline2
    form  = Inline2Form
    extra = 1

class MainModelAdmin(admin.ModelAdmin):
    inlines = (Inline1Inline, Inline2Inline)

admin.site.register(MainModel, MainModelAdmin)

Then, the exclude attribute from the inner Meta class from Inline2Form is not used in the final Meta class (look the comments):

# Django 1.3 (file: django/contrib/contenttypes/generic.py, line: 362)
def generic_inlineformset_factory(model, form=ModelForm,
                                  formset=BaseGenericInlineFormSet,
                                  ct_field="content_type", fk_field="object_id",
                                  fields=None, exclude=None,
                                  extra=3, can_order=False, can_delete=True,
                                  max_num=None,
                                  formfield_callback=lambda f: f.formfield()):
    """
    Returns an ``GenericInlineFormSet`` for the given kwargs.

    You must provide ``ct_field`` and ``object_id`` if they different from the
    defaults ``content_type`` and ``object_id`` respectively.
    """
    opts = model._meta
    # Avoid a circular import.
    from django.contrib.contenttypes.models import ContentType
    # if there is no field called `ct_field` let the exception propagate
    ct_field = opts.get_field(ct_field)
    if not isinstance(ct_field, models.ForeignKey) or ct_field.rel.to != ContentType:
        raise Exception("fk_name '%s' is not a ForeignKey to ContentType" % ct_field)
    fk_field = opts.get_field(fk_field) # let the exception propagate
    if exclude is not None:
        exclude = list(exclude)
        exclude.extend([ct_field.name, fk_field.name])
    else:
    	##################################################################
	##################################################################
	# ct_field.name and fk_field.name are always in the exclude list #
	##################################################################
	##################################################################
        exclude = [ct_field.name, fk_field.name]
    FormSet = modelformset_factory(model, form=form,
                                   formfield_callback=formfield_callback,
                                   formset=formset,
                                   extra=extra, can_delete=can_delete, can_order=can_order,
                                   fields=fields, exclude=exclude, max_num=max_num)
    FormSet.ct_field = ct_field
    FormSet.ct_fk_field = fk_field
    return FormSet


# Django 1.3 (file: django/forms/models.py, line: 370)
def modelform_factory(model, form=ModelForm, fields=None, exclude=None,
                       formfield_callback=None):
    # Create the inner Meta class. FIXME: ideally, we should be able to
    # construct a ModelForm without creating and passing in a temporary
    # inner class.

    # Build up a list of attributes that the Meta object will have.
    attrs = {'model': model}
    if fields is not None:
        attrs['fields'] = fields
    if exclude is not None:
    	###########################################################################################
    	###########################################################################################
    	# For an generic inline, exclude is never None (at least ct_field.name and fk_field.name) #
    	###########################################################################################
    	###########################################################################################
        attrs['exclude'] = exclude

    # If parent form class already has an inner Meta, the Meta we're
    # creating needs to inherit from the parent's inner meta.
    #######################################################################################################
    #######################################################################################################
    # I think the problem is here: we are not "mixing" attrs['exclude'] with form.Meta.exclude.           #
    # attrs['exclude'] is overwriting the attribute form.Meta.exclude and the final Meta class is created #
    # without the exclude information from the Meta class of the form.                                    #
    # I think that, if the generic inline does not provide an exclude list by itself, but have a custom   #
    # form that do this, the exclude from the Meta of this form should be honored.                        #
    #######################################################################################################
    #######################################################################################################
    parent = (object,)
    if hasattr(form, 'Meta'):
        parent = (form.Meta, object)
    Meta = type('Meta', parent, attrs)

    # Give this new form class a reasonable name.
    class_name = model.__name__ + 'Form'

    # Class attributes for the new form class.
    form_class_attrs = {
        'Meta': Meta,
        'formfield_callback': formfield_callback
    }

    return ModelFormMetaclass(class_name, (form,), form_class_attrs)

I've attached the project that I made in order to test this (if this help).

Attachments (7)

generic_inline_bug.tar.bz2 (3.2 KB) - added by leonelfreire 3 years ago.
generic_inline_error.png (19.9 KB) - added by leonelfreire 3 years ago.
15907_test_case_at_r16105.diff (2.7 KB) - added by prestontimmons 3 years ago.
Test case to demonstrate bug
15907_patch.diff (1.0 KB) - added by leonelfreire 3 years ago.
Patch that fixes this bug.
15907_patch_at_r16141.diff (3.4 KB) - added by prestontimmons 3 years ago.
Patch combined with tests
15907_at_16452.diff (3.2 KB) - added by prestontimmons 3 years ago.
Patch with fix moved to generic_inlineformset_factory
15907.generic-inline-admin-fix.diff (8.4 KB) - added by prestontimmons 3 years ago.

Download all attachments as: .zip

Change History (16)

Changed 3 years ago by leonelfreire

Changed 3 years ago by leonelfreire

Changed 3 years ago by prestontimmons

Test case to demonstrate bug

comment:1 Changed 3 years ago by prestontimmons

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

I verified this bug occurs and added a patch with a test case.

Changed 3 years ago by leonelfreire

Patch that fixes this bug.

comment:2 Changed 3 years ago by leonelfreire

  • Has patch set

Added a patch that fixes this for me (revision: 16105).

Last edited 3 years ago by leonelfreire (previous) (diff)

Changed 3 years ago by prestontimmons

Patch combined with tests

comment:3 follow-up: Changed 3 years ago by prestontimmons

I combined the patches with tests and fix into one. Leonel, your patch had lines to combine the fields attribute. I couldn't reproduce a case where fields ever showed up wrong, so I left that out. If there's a bug I'm missing it'd be cool to fix that as well.

comment:4 in reply to: ↑ 3 Changed 3 years ago by anonymous

Replying to prestontimmons:

I combined the patches with tests and fix into one. Leonel, your patch had lines to combine the fields attribute. I couldn't reproduce a case where fields ever showed up wrong, so I left that out. If there's a bug I'm missing it'd be cool to fix that as well.

OK. Thanks for the fix. ;-)

comment:5 Changed 3 years ago by prestontimmons

  • Patch needs improvement set
  • UI/UX unset

The attached patch is a bad fix. It conflicts with behavior expected in #8999, which depends on the exclude argument to modelformset_factory trumping exclude information in the ModelForm.Meta.

A better place to fix this is in generic_inlineformset_factory. When exclude information isn't passed in, combine the default contenttype fields with the model form meta fields.

Changed 3 years ago by prestontimmons

Patch with fix moved to generic_inlineformset_factory

comment:6 Changed 3 years ago by prestontimmons

  • Patch needs improvement unset

Changed 3 years ago by prestontimmons

comment:7 Changed 3 years ago by prestontimmons

This new patch moves the changes into GenericInlineAdmin, and is parallel to changes in the patch for #14496 to ModelAdmin and InlineAdmin, providing consistency for how exclude is handled for all inlines.

comment:8 Changed 3 years ago by jezdez

  • Triage Stage changed from Accepted to Ready for checkin

comment:9 Changed 3 years ago by jezdez

  • Resolution set to fixed
  • Status changed from new to closed

In [16603]:

Fixed #15907 -- Fixed another conflict between the ModelForm exclude and the GenericInline. Thanks, leonelfreire and prestontimmons.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


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

 
Note: See TracTickets for help on using tickets.