Opened 14 years ago

Closed 13 years ago

Last modified 13 years ago

#14496 closed Bug (fixed)

Conflict between ModelForm.Meta.exclude and ModelAdmin.readonly attributes

Reported by: msgre_valise Owned by: nobody
Component: contrib.admin Version: 1.2
Severity: Normal Keywords: ModelForm ModelAdmin readonly_fields exclude conflict
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

I have problem with contrib.admin application. Consider we have demo application with this three files:

demo/models.py:

    from django.db import models

    class Demo(models.Model):
        name = models.CharField(max_length=100)
        note = models.TextField(blank=True, null=True)

demo/admin.py:

    from django.contrib import admin
    from models import Demo
    from forms import DemoAdminForm

    class DemoAdmin(admin.ModelAdmin):
        readonly_fields = ('name', )
        form = DemoAdminForm

    admin.site.register(Demo, DemoAdmin)

demo/forms.py:

    from django import forms
    from models import Demo

    class DemoAdminForm(forms.ModelForm):
        class Meta:
            model = Demo
            exclude = ('note', )

If I go to administration, and edit some Demo object, I see two fields: editable Note and readonly Name. But! I should see only readonly Name (because I use own ModelForm class which excluded note field). Try to comment line "readonly_fields = ('name', )" at admin.py file. You will see, that in this situation (no readonly attribute on ModelAdmin) Django correctly accept Meta class on ModelForm and exclude field Note.

I think that there is a problem in the way, how Django construct ModelForm class. It don't respect Meta classes in Form, if there is at same time readonly attribute on ModelAdmin.

Attachments (2)

14496.custom-admin-form-exclude.diff (6.8 KB ) - added by Julien Phalip 14 years ago.
14496.custom-admin-form-exclude.alternative.diff (6.0 KB ) - added by Julien Phalip 14 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 by Daniel F Moisset, 14 years ago

Triage Stage: UnreviewedAccepted

comment:2 by Julien Phalip, 14 years ago

Component: Contrib appsdjango.contrib.admin

comment:3 by Julien Phalip, 14 years ago

Severity: Normal
Type: Bug

by Julien Phalip, 14 years ago

comment:4 by Julien Phalip, 14 years ago

Easy pickings: unset
Has patch: set

The reason this is happening is that django.forms.models.modelform_factory() systematically overrides the form's Meta.exclude attribute with the exclude kwarg that it receives. So, in this case, DemoAdminForm.Meta.exclude is overridden by ('name', ) which comes from the DemoAdmin.readonly_fields declaration, and the "note" field is therefore included in the form.

modelform_factory's behaviour in itself is actually correct, but the problem is that ModelAdmin.get_form() does not take it properly into account.

Like the reporter I think it makes sense to assume that, if a custom form is provided, then that form's exclude option should be considered and those fields should be excluded from the generated form, as well as the fields defined in ModelAdmin.exclude and ModelAdmin.readonly_fields. The attached patch rectifies this behaviour both for ModelAdmin.get_form() and InlineModelAdmin.get_formset().

comment:5 by Julien Phalip, 14 years ago

I've had second thoughts about this and posted an alternative patch.

I still think this ticket is a valid bug. The confusion mostly comes from the fact that defining readonly_fields makes the ModelAdmin ignore the ModelForm's Meta.exclude option even if the ModelAdmin doesn't define its own exclude. Both patches fix that bug.

However, while the first patch systematically considers the ModelForm.Meta.exclude option, the new patch only considers it if ModelAdmin doesn't define its own exclude. If ModelAdmin does define exclude, then it takes precedence. This is a way of transferring control from ModelForm to ModelAdmin, which seems like a more explicit way of handling things.

comment:6 by Preston Timmons, 13 years ago

UI/UX: unset

The patch and tests look good. It'll be nice to have the precedence of exclude options defined and documented.

I also added a parallel patch for generic inlines at #15907.

comment:7 by Jannis Leidel, 13 years ago

Triage Stage: AcceptedReady for checkin

comment:8 by Jannis Leidel, 13 years ago

Resolution: fixed
Status: newclosed

In [16602]:

Fixed #14496 -- Fixed conflict between ModelForm exclude and ModelAdmin readonly values. Thanks, Julien Phalip.

comment:9 by anonymous, 13 years ago

In case someone wants code to work around this, put this in your ModelAdmin subclass:

    def get_form(self, request, obj=None, **kwargs):
        """Work around https://code.djangoproject.com/ticket/14496 (not fixed in Django 1.3)"""
        form = super(OffcutAdminRestricted, self).get_form(request, obj, **kwargs)
        for e in self.form._meta.exclude:
            if e in form.base_fields:
                del form.base_fields[e]
                form._meta.exclude.append(e)
        return form
Note: See TracTickets for help on using tickets.
Back to Top