#32336: Allow readonly callables defined in get_readonly_fields
-------------------------------+-----------------------------------------
Reporter: Tim McCurrach | Owner: Tim McCurrach
Type: Bug | Status: assigned
Component: contrib.admin | Version: 3.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+-----------------------------------------
Changes (by Tim McCurrach):
* has_patch: 0 => 1
Old description:
> Currently if you have a `get_readonly_fields` that defines a function
> within its scope, and then uses it such as the following:
>
> {{{
> def get_readonly_fields(self, request, obj=None):
> def my_field(obj):
> return "This is readonly"
> return self.readonly_fields + (my_field, )
> }}}
> This will lead to an error:
>
> {{{
> File "...lib/python3.8/site-packages/django/forms/models.py", line 265,
> in __new__
> message = message % (', '.join(missing_fields),
> TypeError: sequence item 0: expected str instance, function found
> }}}
>
> === Motivation ===
> Whilst the above may seem a bit of an edge case, the reason I came across
> it was wanting to use the `request` object for a readonly field. Readonly
> fields can be callables that accept only one parameter (obj). If you want
> a readonly field that behaves differently based on the request object,
> you need to do something like this:
>
> {{{
> def my_field(request, obj):
> # something here
>
> class MyAdmin(admin.ModelAdmin):
> def get_readonly_fields(self, request, obj=None):
> readonly_field = functools.partial(my_field, request)
> return (readonly_field,)
> }}}
>
> But this obviously leads to the same error. Changing the behaviour of a
> specific readonly field based on the user seems like a reasonable use-
> case to cater for.
>
> === Cause of the error ===
> - `get_readonly_fields` is called several times per change view request.
> - Taking the example above, the value `my_field` stored in `fields` and
> `excluded` (local variables in `ModelAdmin.get_form`) refer to different
> objects since they come from separate calls to `get_readonly_fields`.
> - This is a problem further down the line when the ModelForm is being
> generated. At the end of `forms.models.fields_for_model` we return values
> in `fields` that are not in `exclude` but since there are different
> versions of `my_field` in both, it ends up not being filtered out when it
> should be.
>
> === Proposed Solution ===
> Cache the return value of `get_readonly_fields` in some instance variable
> `self._readonly_fields`, and reset this value at the beginning of every
> request. This way the various variables that store readonly fields will
> all be referring to the same objects. For `InlineModelAdmin`s the value
> of `self._readonly_fields` would need to be set to `None` on
> instantiation (which happens for every new change view request).
New description:
Currently if you have a `get_readonly_fields` that defines a function
within its scope, and then uses it such as the following:
{{{
def get_readonly_fields(self, request, obj=None):
def my_field(obj):
return "This is readonly"
return self.readonly_fields + (my_field, )
}}}
This will lead to an error:
{{{
File "...lib/python3.8/site-packages/django/forms/models.py", line 265, in
__new__
message = message % (', '.join(missing_fields),
TypeError: sequence item 0: expected str instance, function found
}}}
=== Motivation ===
Whilst the above may seem a bit of an edge case, the reason I came across
it was wanting to use the `request` object for a readonly field. Readonly
fields can be callables that accept only one parameter (obj). If you want
a readonly field that behaves differently based on the request object, you
need to do something like this:
{{{
def my_field(request, obj):
# something here
class MyAdmin(admin.ModelAdmin):
def get_readonly_fields(self, request, obj=None):
readonly_field = functools.partial(my_field, request)
return (readonly_field,)
}}}
But this obviously leads to the same error. Changing the behaviour of a
specific readonly field based on the user seems like a reasonable use-case
to cater for.
=== Cause of the error ===
- `get_readonly_fields` is called several times per change view request.
- Taking the example above, the value `my_field` stored in `fields` and
`excluded` (local variables in `ModelAdmin.get_form`) refer to different
objects since they come from separate calls to `get_readonly_fields`.
- This is a problem further down the line when the ModelForm is being
generated. At the end of `forms.models.fields_for_model` we return values
in `fields` that are not in `exclude` but since there are different
versions of `my_field` in both, it ends up not being filtered out when it
should be.
=== Proposed Solution ===
Cache the return value of `get_readonly_fields` in some instance variable
`self._readonly_fields`, and reset this value at the beginning of every
request. This way the various variables that store readonly fields will
all be referring to the same objects. For `InlineModelAdmin`s the value of
`self._readonly_fields` would need to be set to `None` on instantiation
(which happens for every new change view request).
=== Tests ===
I'm not sure if this needs a separate test. I have adapted `PostAdmin`
(used in lots of admin tests, in particular the ones that test
readonly_fields) to cater for this case. So although no tests are added,
there are plenty of failing tests before the fix.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/32336#comment:3>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/071.ad757b1cd899d399c11db40a7000f5d5%40djangoproject.com.
