[Django] #33703: ModelAdmin.get_fields wrong work with ModelAdmin.fieldsets statement
Django
-------------------------------------+-------------------------------------
Reporter: Maxim | Owner: nobody
Danilov |
Type: | Status: new
Uncategorized |
Component: | Version: 4.0
contrib.admin |
Severity: Normal | Keywords: admin, modeladmin
Triage Stage: | Has patch: 1
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
-------------------------------------+-------------------------------------
almost all my issue is "not fixed", but i try
I found a bug: ModelAdmin.get_fields works wrong with fieldset statement.
How to reproduce it:
{{{
from django.db import models
from django.contrib.admin import ModelAdmin
class MyModel(models.Model):
title = models.CharField('Meta Title of object', max_length=80,
blank=True)
slug = models.CharField('Meta Slug of object', max_length=80,
blank=True)
class MyModelAdmin1(ModelAdmin):
fields = ('title', )
class MyModelAdmin2(ModelAdmin):
fieldset = ('title', )
print(MyModelAdmin1(MyModel, None).get_fields(None))
print(MyModelAdmin2(MyModel, None).get_fields(None))
}}}
save as test.py, in new or old project.
python manage.py test output:
{{{
('title',)
['title', 'slug']
Found xxx test(s).
...
}}}
Problem is nor directly in def get_fields.
{{{
def get_fields(self, request, obj=None):
"""
Hook for specifying fields.
"""
if self.fields:
return self.fields
# _get_form_for_get_fields() is implemented in subclasses.
form = self._get_form_for_get_fields(request, obj)
return [*form.base_fields, *self.get_readonly_fields(request,
obj)]
}}}
But in call of self._get_form_for_get_fields
{{{
def _get_form_for_get_fields(self, request, obj):
return self.get_form(request, obj, fields=None)
}}}
The author of _get_form_for_get_fields has forgotten how self.get_form
handles "fields":
{{{
def get_form(self, request, obj=None, change=False, **kwargs):
"""
Return a Form class for use in the admin add view. This is used by
add_view and change_view.
"""
if 'fields' in kwargs:
fields = kwargs.pop('fields')
else:
fields = flatten_fieldsets(self.get_fieldsets(request, obj))
...
}}}
I see two possibility to fix it:
or
{{{
def _get_form_for_get_fields(self, request, obj):
return self.get_form(request, obj)
}}}
or
{{{
def get_form(self, request, obj=None, change=False, **kwargs):
"""
Return a Form class for use in the admin add view. This is used by
add_view and change_view.
"""
fields = kwargs.pop('fields', None) or []
if not fields:
fields = flatten_fieldsets(self.get_fieldsets(request, obj))
...
}}}
p.s.
this issue is easy to create, we already have 'contrib.admin' in
"component select" in Issue-Creation-Form, unlike, for example,
'contrib.gis'.
For 'contrib.gis' we suddenly use GIS in uppercase.
--
Ticket URL: <https://code.djangoproject.com/ticket/33703>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Maxim Danilov | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.admin | Version: 4.0
Severity: Normal | Resolution:
Keywords: admin, modeladmin | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by Maxim Danilov):
* Attachment "test.py" added.
example which works for me on django 4.01
Django
-----------------------------------+--------------------------------------
Reporter: Maxim Danilov | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.admin | Version: 4.0
Severity: Normal | Resolution:
Keywords: admin, modeladmin | Triage Stage: Unreviewed
Needs tests: 0 | Patch needs improvement: 0
-----------------------------------+--------------------------------------
Changes (by Maxim Danilov):
* has_patch: 1 => 0
* easy: 1 => 0
Old description:
New description:
How to reproduce it:
class MyModelAdmin1(ModelAdmin):
I thought we had two options to fix this:
{{{
def _get_form_for_get_fields(self, request, obj):
return self.get_form(request, obj)
}}}
or
{{{
def get_form(self, request, obj=None, change=False, **kwargs):
"""
Return a Form class for use in the admin add view. This is used by
add_view and change_view.
"""
fields = kwargs.pop('fields', None) or []
if not fields:
fields = flatten_fieldsets(self.get_fieldsets(request, obj))
...
}}}
BUT! My solution not works, we receive a recursion:
{{{
in get_fields
call self._get_form_for_get_fields
in _get_form_for_get_fields
call self.get_form
in get_form
call self.get_fieldsets
in get_fieldsets
call self.get_fields (go to 1. step)
}}}
p.s.
this issue is easy to create, we already have 'contrib.admin' in
"component select" in Issue-Creation-Form, unlike, for example,
'contrib.gis'.
For 'contrib.gis' we suddenly use GIS in uppercase.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/33703#comment:1>
Django
-----------------------------------+--------------------------------------
Reporter: Maxim Danilov | Owner: nobody
Component: contrib.admin | Version: 4.0
Severity: Normal | Resolution: invalid
Keywords: admin, modeladmin | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+--------------------------------------
* status: new => closed
* resolution: => invalid
Comment:
Thanks for the report, however as far as I'm aware `get_fields()` works as
intended and
[https://docs.djangoproject.com/en/stable/ref/contrib/admin/#django.contrib.admin.ModelAdmin.get_fields
documented], it's a hook which allows specifying fields dynamically. It
returns `ModelAdmin.fields` or base fields and readonly-fields, if not
specified.
It shouldn't check fieldsets definition, but the other way around,
fieldsets should check specified fields. That's why you're getting an
infinite recursion. Moreover `fields=None` was added intentionally in
23e1b59cf2ad6a75637dd0273973e657e48e317e.
--
Ticket URL: <https://code.djangoproject.com/ticket/33703#comment:2>
Django
-----------------------------------+--------------------------------------
Reporter: Maxim Danilov | Owner: nobody
Component: contrib.admin | Version: 4.0
Severity: Normal | Resolution:
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+--------------------------------------
* status: closed => new
* resolution: invalid =>
Comment:
documentation:
----
ModelAdmin.get_fields
The get_fields method is given the HttpRequest and the obj being edited
(or None on an add form) and is expected to return a list of fields, as
described above in the ModelAdmin.fields section.
ModelAdmin.fields
Use the fields option to make simple layout changes in the forms on the
“add” and “change” pages such as showing only a subset of available fields
For more complex layout needs, see the fieldsets option.
----
I use only .fieldsets, without .fields definition. ModelAdmin.get_fields
give me a wrong list.
Why you set my ticket as invalid?
--
Ticket URL: <https://code.djangoproject.com/ticket/33703#comment:3>
Django
-----------------------------------+--------------------------------------
Reporter: Maxim Danilov | Owner: nobody
Component: contrib.admin | Version: 4.0
Severity: Normal | Resolution: invalid
Keywords: admin, modeladmin | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------------+--------------------------------------
* status: new => closed
* resolution: => invalid
Comment:
Exactly as you quoted the docs:
> The get_fields method is given the HttpRequest and the obj being edited
(or None on an add form) and is expected to **return a list of fields, as
described above in the ModelAdmin.fields** section.
and `ModelAdmin.fields`:
> **Use the fields option to make simple layout changes** in the forms on
the “add” and “change” pages such as showing only a subset of available
fields **For more complex layout needs, see the fieldsets option.**
You didn't use `ModelAdmin.fields` or overwrite `ModelAdmin.get_fields()`,
so `ModelAdmin.get_fields()` returned base fields and readonly-fields.
`ModelAdmin.get_fields()` is not described as a tool for getting displayed
fields, it's a hook to overwrite displayed fields when you need a
conditional logic based on a request and you don't use `fieldsets` for a
complex layout, not the other way around.
You can raise your doubts on the DevelopersMailingList to reach a wider
audience and see what other think, if you don't agree. Thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/33703#comment:4>