[Django] #32336: Allow readonly callables defined in get_readonly_fields
Django
-----------------------------------------+--------------------------
Reporter: Tim McCurrach | Owner: nobody
Type: Bug | Status: assigned
Component: contrib.admin | Version: 3.1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-----------------------------------------+--------------------------
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).
--
Ticket URL: <https://code.djangoproject.com/ticket/32336>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Tim McCurrach | Owner: Tim McCurrach
Type: Bug | Status: assigned
Component: contrib.admin | Version: 3.1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim McCurrach):
* owner: nobody => Tim McCurrach
--
Ticket URL: <https://code.djangoproject.com/ticket/32336#comment:1>
Django
Reporter: Tim McCurrach | Owner: Tim McCurrach
Component: contrib.admin | Version: 3.1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim McCurrach):
PR is on the way soon
--
Ticket URL: <https://code.djangoproject.com/ticket/32336#comment:2>
Django
Reporter: Tim McCurrach | Owner: Tim McCurrach
Component: contrib.admin | Version: 3.1
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:
> 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:
=== 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
-------------------------------+-----------------------------------------
Reporter: Tim McCurrach | Owner: Tim McCurrach
Component: contrib.admin | Version: 3.1
Severity: Normal | Resolution: wontfix
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 Mariusz Felisiak):
* status: assigned => closed
* type: Bug => New feature
* resolution: => wontfix
Comment:
Functions are not a supported in `ModelAdmin.readonly_fields`, see
[https://docs.djangoproject.com/en/3.1/ref/contrib/admin/#django.contrib.admin.ModelAdmin.readonly_fields
docs]:
> ''"A read-only field can not only display data from a model’s field, it
can also display the output of a model’s method or a method of the
ModelAdmin class itself."''
I agree with Tim that it's not worth complexity.
--
Ticket URL: <https://code.djangoproject.com/ticket/32336#comment:4>