Opened 14 years ago
Last modified 2 years 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: | dev |
Severity: | Normal | Keywords: | |
Cc: | jdunck@…, Solvik, wilburb, net147, Zach Borboa, vmspike@…, direx, Prateem Shrestha, Paolo Dina, Scott Stevens, Ülgen Sarıkavak | 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 (26)
comment:1 by , 14 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 14 years ago
Type: | → Bug |
---|
comment:3 by , 14 years ago
Severity: | → Normal |
---|
comment:4 by , 14 years ago
Cc: | added |
---|
comment:5 by , 14 years ago
Cc: | added |
---|---|
Easy pickings: | unset |
UI/UX: | unset |
comment:6 by , 13 years ago
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 by , 13 years ago
Cc: | added |
---|
comment:8 by , 13 years ago
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 by , 12 years ago
Cc: | added |
---|
comment:10 by , 10 years ago
Cc: | added |
---|
comment:12 by , 10 years ago
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 itsget_readonly_fields
.
- Another possibility is to use a custom
formset
that overridesBaseModelFormSet._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?
comment:13 by , 8 years ago
Cc: | added |
---|---|
Version: | 1.2 → master |
5 years old ticket and no decision...
Suppose to change version to master. Has the same issue with django 1.9.
follow-ups: 15 16 23 comment:14 by , 8 years ago
I can confirm this for 1.10.1.
My two cents, I consider the current behavior (passing the top-level object as the obj
parameter) to be The Wrong Thing. An Inline Admin should get its own model.
This bug significantly hinders the ability of get_readonly_fields()
to be useful in inline admins because you can only mark a field as read-only for every instance of the admin. Which I could just do with readonly_fields
.
comment:15 by , 8 years ago
Replying to Jamie Bliss:
My two cents, I consider the current behavior (passing the top-level object as the
obj
parameter) to be The Wrong Thing. An Inline Admin should get its own model.
This bug significantly hinders the ability of
get_readonly_fields()
to be useful in inline admins because you can only mark a field as read-only for every instance of the admin. Which I could just do withreadonly_fields
.
I agree completely. Actually my current work is hindered by it.
comment:16 by , 7 years ago
Cc: | added |
---|
Replying to Jamie Bliss:
My two cents, I consider the current behavior (passing the top-level object as the
obj
parameter) to be The Wrong Thing. An Inline Admin should get its own model.
Full ACK. And this is still the current behavior in Django 2.0 (and master).
comment:18 by , 7 years ago
Cc: | added |
---|
comment:19 by , 6 years ago
I'm using Django 2.0.7 and have the same issue. However, I've a dumb solution to this (untested).
I'm building an account-transaction admin page. As one would expected, the AccountAdmin will have TransactionStackedInlines. Undoubtably, we use the inline form to add new transaction with most fields editable. And for existing transaction, we want them as read-only.
Since, existing transactions look better in tabular inline (not all fields displayed), while new transaction is better in stacked form, I created two inlines. Here is my codes:
@admin.register(models.Account) class AccountAdmin(admin.ModelAdmin): inlines = [ inlines.TransactionTabularInline, inlines.TransactionStackedInline, ]
And for the two inlines, I want that the top, tabular inline has no "add new" action, while the bottom stacked inline shows nothing but a new transaction:
class TransactionTabularInline(admin.TabularInline): model = models.Transaction extra = 1 # extra must be 1 to make the magic work max_num = 0 # somehow this disable the "add" because max_num > extra fields = (contains only a handful of fields) can_delete = False show_change_link = True # allow viewing the full entry in another page readonly_fields = [ the list of 'fields' ] class TransactionStackedInline(admin.StackedInline): model = models.Transaction extra = 0 def get_queryset(self, request): queryset = super().get_queryset(request) return queryset.none() # no existing records will appears
There will be an arrow on each row in tabular inline, which points to TransactionAdmin, in which get_readonly_fields
will work as every one expected.
I think you can use two (stacked) inlines, one for existing records with more readonly_fields
, and one without readonly but return an empty query_set.
I found queryset.none()
from django's code, when users has no permission on the object. So, I suppose this is "okay" when I do not want to display existing records for my own reason.
comment:20 by , 6 years ago
Cc: | added |
---|
This also affects has_<value>_permission
calls, and actually causes the wrong object to be checked for permissions.
I feel as though this increases the severity, since it no longer just affects what fields are read-only, but is now a failure to enforce permissions correctly (and could allow object permissions where it's expected to be prohibited).
I'm unsure if we need a new bug for this, or to expand the scope of this existing ticket (rename/etc.).
follow-up: 22 comment:21 by , 6 years ago
Given the change it would involve, I think this will probably need to be resolved as a documentation issue.
That they're passed the parent object means that get_readonly_fields()
&co as they appear on an inline have the same signature but a different semantics from when they appear on the normal ModelAdmin. As such, they should be documented separately in the InlineModelAdmin ref.
Similar differences are already documented for the various InlineModelAdmin versions of the `has_<value>_permission()` methods.
On the actual issue, could it be resolved with permissions? If you have add
but not change
on the inline model (possibly via custom has_add_permission()
and has_change_permission()
methods) do you not get the allow creating new objects, immutible existing objects
from the title?
comment:22 by , 5 years ago
Nice solution, this worked in my scenario.
Replying to Carlton Gibson:
On the actual issue, could it be resolved with permissions? If you have
add
but notchange
on the inline model (possibly via customhas_add_permission()
andhas_change_permission()
methods) do you not get theallow creating new objects, immutible existing objects
from the title?
comment:23 by , 5 years ago
Replying to Jamie Bliss:
My two cents, I consider the current behavior (passing the top-level object as the
obj
parameter) to be The Wrong Thing. An Inline Admin should get its own model.
I need to agree with you and inform that this still exists in Django 3.0.2
comment:24 by , 4 years ago
This issue is also affecting my current project. I expected obj
within get_readonly_fields()
to be the inline instance, and not the parent.
I have a simplified permissions system that does not check per object permissions, and cannot implement the workaround Carlton suggested.
comment:25 by , 3 years ago
One can selectively disable certain fields on the add_fields method of the associated formset as a workaround:
class MyFormSet(BaseInlineFormSet): def add_fields(self, form, index): super().add_fields(form, index) if form.initial: form.fields['my_field'].disabled = True class My_Inline(admin.TabularInline): model = MyModel formset = MyFormSet
comment:26 by , 2 years ago
Cc: | added |
---|
The
obj
passed toget_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.