Code

Opened 4 years ago

Closed 3 years ago

Last modified 3 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 3 years ago.
14496.custom-admin-form-exclude.alternative.diff (6.0 KB) - added by julien 3 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 4 years ago by dmoisset

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

comment:2 Changed 3 years ago by julien

  • Component changed from Contrib apps to django.contrib.admin

comment:3 Changed 3 years ago by julien

  • Severity set to Normal
  • Type set to Bug

Changed 3 years ago by julien

comment:4 Changed 3 years ago by julien

  • 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 Changed 3 years ago by julien

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 Changed 3 years ago by prestontimmons

  • 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 Changed 3 years ago by jezdez

  • Triage Stage changed from Accepted to Ready for checkin

comment:8 Changed 3 years ago by jezdez

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

In [16602]:

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

comment:9 Changed 3 years ago by anonymous

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

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.