Opened 4 years ago

Last modified 7 months ago

#15602 new Bug

Using get_readonly_fields and StackedInline/TabularInline admin objects doesn't allow creating new objects, immutible existing objects

Reported by: bradwhittington Owned by: nobody
Component: contrib.admin Version: 1.2
Severity: Normal Keywords:
Cc: jdunck@…, Solvik, wilburb, net147, zachborboa Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I am using a pattern described here, on a StackedInline admin class, AKA:

    def get_readonly_fields(self, request, obj = None):
        if obj:
            return self.readonly_fields+('text','author','private')
        return self.readonly_fields

But it seems to currently be broken (in 1.2.5), I get all fields repeated twice per object, and the add new object fields are all marked read-only, as if obj is always a non-None value. If I throw logging into that method (log.debug(obj==None)), I am seeing (in an example with one inline object already created):

2011-03-14 00:23:52 SAST+0000 - DEBUG - task.admin - False
2011-03-14 00:23:52 SAST+0000 - DEBUG - task.admin - True
2011-03-14 00:23:52 SAST+0000 - DEBUG - task.admin - False
2011-03-14 00:23:52 SAST+0000 - DEBUG - task.admin - False

Change History (12)

comment:1 Changed 4 years ago by kmtracey

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

The obj passed to get_readonly_fields defined in an inline ModelAdmin looks to be the parent object, not the object for the inline itself. This looks to be similar to #15424.

comment:2 Changed 4 years ago by lukeplant

  • Type set to Bug

comment:3 Changed 4 years ago by lukeplant

  • Severity set to Normal

comment:4 Changed 4 years ago by jdunck

  • Cc jdunck@… added

comment:5 Changed 4 years ago by Solvik

  • Cc Solvik added
  • Easy pickings unset
  • UI/UX unset

comment:6 Changed 4 years ago by nivhaa@…

Happens the same to me, but I'm working with 1.3... can't use get_readonly_fields on an inline models when I'm having two inlines in the same change_view page. "Please correct the errors below" is shown above, without any error shown below. when trying to debug, somewhere along the road, the formset gets this error for each inline object "This field is required" - but as I said, it's not shown in the page

comment:7 Changed 4 years ago by wilburb

  • Cc wilburb added

comment:8 Changed 4 years ago by julien

The core issue here, as pointed out by Karen, is that obj is the parent object, not a child object from the inline. Note that the same issue affects get_readonly_fields(), get_fieldsets() and get_prepopulated_fields(): https://code.djangoproject.com/browser/django/trunk/django/contrib/admin/options.py?rev=17065#L1080

Those methods are called as each inline formset is instantiated, not as each individual form within the formsets is instantiated. The inline formsets are instantiated in the admin view, whereas the individual inline forms are instantiated later during the template-rendering stage, at the same time as the related objects are fetched from the database via the corresponding inline's queryset:

https://code.djangoproject.com/browser/django/trunk/django/contrib/admin/templates/admin/change_form.html?rev=17065#L60
https://code.djangoproject.com/browser/django/trunk/django/contrib/admin/helpers.py?rev=17065#L209

The three methods get_readonly_fields(), get_fieldsets() and get_prepopulated_fields() all need to be passed the HttpRequest object. Yet, by the stage at which the inline forms are instantiated the request object isn't available.

To sum up, if we want to actually pass the inline's individual object to those methods, then some pretty significant refactoring will be necessary. Maybe this is a good time to rethink the way inlines are managed in the admin.

comment:9 Changed 3 years ago by net147

  • Cc net147 added

comment:10 Changed 8 months ago by zachborboa

  • Cc zachborboa added

comment:11 Changed 7 months ago by dji

#24185 is a duplicate of this.

comment:12 Changed 7 months ago by dji

I've done some additional research, and the problem actually goes much deeper than this. Not only do the results of get_readonly_fields get passed to the InlineAdminFormSet object and others in django.contrib.admin.helpers that actually render the formset (including the readonly fields), they also get passed into the formset validation logic.

The way formsets work is to take a single form class and instantiate it multiple times; an inline model formset uses a constructed ModelForm subclass. (see django.forms.models.modelformset_factory)

Now, the results of get_readonly_fields are added to this constructed form class as the Meta.exclude list, the list of fields that are excluded from validation. Because this same class is instantiated multiple times by the formset, every instance of the form must have the same fields excluded. So, as it stands now, every instance must have the same readonly fields - because you want to exclude at least these of fields from validation. Decisions can't be made on a per-form/per-inline-model-instance basis.

The only real "solutions" that come to mind both involve _underscore methods, which is exactly what you're not supposed to do:

  • The simplest is to allow the custom model form class to be created with exclude=(), and let the individual forms in the formset be instantiated. Then we'd tweak each form's _meta.exclude as we wanted to include the results of its get_readonly_fields.
  • Another possibility is to use a custom formset that overrides BaseModelFormSet._construct_form, the method that model formsets use to get each model instance in the queryset and to instantiate each of their individual forms. Instead of instantiating the formset's normal form, we'd have to check the readonly fields on the instance, create a new model form class for that particular exclude list, and instantiate a single instance of that class. Whew.

I suppose the ideal solution would be to allow a model form's exclude to be callable with the form's instance, but that's outside the scope of this ticket I imagine.

Of course none of this solves the earlier problem of what to do with the request, but in comparison that seems like a fairly simple problem to have - worst case you send it along when you instantiate the InlineAdminForm wrapper or bind up a functools.partial version of get_readonly_fields with the request in it. But perhaps there's even a cleaner way, I didn't look into this too much as it depends on the larger problem.

So, I guess in conclusion - how bad is it considered to mess with a form's _meta after it's been instantiated? Would that be a deal-breaker even if it fixed this?

Last edited 7 months ago by dji (previous) (diff)
Note: See TracTickets for help on using tickets.
Back to Top