[Django] #31558: Boolean attribute for a property getter of a model doesn’t active the “on/off” icon in the admin site
Django
“on/off” icon in the admin site
----------------------------------------------+------------------------
Reporter: Alexandre Poitevin | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 3.0
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 |
----------------------------------------------+------------------------
Example (adapted from the polls tutorial):
{{{
class Question(models.Model):
DEFINITION_OF_RECENT = {"days": 1}
text = models.CharField(max_length=255)
publication_date = models.DateTimeField('Date of publication')
@property
def was_published_recently(self):
timenow = timezone.now()
recent = timenow - timedelta(**self.DEFINITION_OF_RECENT)
return recent <= self.publication_date <= timenow
was_published_recently.fget.admin_order_field = 'publication_date'
was_published_recently.fget.short_description = 'Published Recently?'
was_published_recently.fget.boolean = True
}}}
There is no problem with `short_description` nor `admin_order_filed`, but
`boolean` does not act as expected.
(I hesitated between Bug and New feature for this ticket…)
--
Ticket URL: <https://code.djangoproject.com/ticket/31558>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
------------------------------------+------------------------------------
Reporter: Alexandre Poitevin | Owner: nobody
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by felixxm):
* type: Bug => New feature
* version: 3.0 => master
* stage: Unreviewed => Accepted
Old description:
> Example (adapted from the polls tutorial):
> {{{
> class Question(models.Model):
>
> DEFINITION_OF_RECENT = {"days": 1}
>
> text = models.CharField(max_length=255)
> publication_date = models.DateTimeField('Date of publication')
>
> @property
> def was_published_recently(self):
> timenow = timezone.now()
> recent = timenow - timedelta(**self.DEFINITION_OF_RECENT)
> return recent <= self.publication_date <= timenow
>
> was_published_recently.fget.admin_order_field = 'publication_date'
> was_published_recently.fget.short_description = 'Published Recently?'
> was_published_recently.fget.boolean = True
> }}}
>
> There is no problem with `short_description` nor `admin_order_filed`, but
> `boolean` does not act as expected.
>
> (I hesitated between Bug and New feature for this ticket…)
New description:
Example (adapted from the polls tutorial):
{{{
class Question(models.Model):
DEFINITION_OF_RECENT = {"days": 1}
text = models.CharField(max_length=255)
publication_date = models.DateTimeField('Date of publication')
def was_published_recently(self):
timenow = timezone.now()
recent = timenow - timedelta(**self.DEFINITION_OF_RECENT)
return recent <= self.publication_date <= timenow
was_published_recently.admin_order_field = 'publication_date'
was_published_recently.short_description = 'Published Recently?'
was_published_recently.boolean = True
was_published_recently = property(was_published_recently)
}}}
There is no problem with `short_description` nor `admin_order_filed`, but
`boolean` does not act as expected.
(I hesitated between Bug and New feature for this ticket…)
--
Comment:
Thanks. This is definitely a new feature (see similar ticket #30259 for
`admin_order_field` support). We can support it but when using the
`property()` function and not with decorator, that's why I updated the
ticket description.
--
Ticket URL: <https://code.djangoproject.com/ticket/31558#comment:1>
Django
Reporter: Alexandre Poitevin | Owner: Hasan
| Ramezani
Type: New feature | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Hasan Ramezani):
* owner: nobody => Hasan Ramezani
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/31558#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Alexandre Poitevin | Owner: Hasan
| Ramezani
Type: New feature | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Alexandre Poitevin):
Thanks for accept my ticket.
I was hoping to contribute and make the pull request myself, but I’m a new
to Django, so…
Anyway I would like to say just 2 things:
1. I don’t understand what is the difference between the decorator and
calling explicitly `property`.
I saw a similar statement in the documentation. But in fact, the effect is
the same:
{{{
>>> class C:
... def m(self): ...
... m.attr = 42
... m = property(m)
...
>>> C.m.attr
Traceback (most recent call last):
...
AttributeError: 'property' object has no attribute 'attr'
>>> C.m.fget.attr
42
>>> class C:
... @property
... def m(self): ...
... m.fget.attr = 42
...
>>> C.m.attr
Traceback (most recent call last):
...
AttributeError: 'property' object has no attribute 'attr'
>>> C.m.fget.attr
42
}}}
So the original method is stored as a `fget` attribute of the property,
with or without the decorator, so I don’t see why this feature couldn’t be
achieve with the decorator.
2. After navigating in the source code and made a little work of
debugging, I found two functions that may the ones requiring modification
to enable this feature:
`contrib.admin.templatetags.items_for_result`
`contrib.admin.utils.lookup_fields`
In the second:
{{{
# For non-field values, the value is either a method, property or
# returned via a callable.
if callable(name):
attr = name
value = attr(obj)
}}}
And in the first :
{{{
try:
f, attr, value = lookup_field(field_name, result,
cl.model_admin)
except ObjectDoesNotExist:
result_repr = empty_value_display
else:
empty_value_display = getattr(attr, 'empty_value_display',
empty_value_display)
if f is None or f.auto_created:
if field_name == 'action_checkbox':
row_classes = ['action-checkbox']
boolean = getattr(attr, 'boolean', False) # <= HERE !
result_repr = display_for_value(value,
empty_value_display, boolean)
}}}
By the way, this one (`items_for_result`) has no unit tests, or at least I
didn’t see anyone (`grep -nr "items_for_result" tests/`).
--
Ticket URL: <https://code.djangoproject.com/ticket/31558#comment:3>
Django
Reporter: Alexandre Poitevin | Owner: (none)
Type: New feature | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Hasan Ramezani):
* cc: Hasan Ramezani (added)
* owner: Hasan Ramezani => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/31558#comment:4>
Django
Reporter: Alexandre Poitevin | Owner: Alexandre
| Poitevin
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Hasan Ramezani):
* owner: (none) => Alexandre Poitevin
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/31558#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Alexandre Poitevin | Owner: Alexandre
| Poitevin
Type: New feature | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by felixxm):
`fget` is a function for getting an attribute value, it's an
implementation detail of `property()`. Setting variable on a function is
hacky and cannot be recommended in our docs, IMO.
--
Ticket URL: <https://code.djangoproject.com/ticket/31558#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Alexandre Poitevin | Owner: Alexandre
| Poitevin
Type: New feature | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Alexandre Poitevin):
Setting a variable function is already what you find in the tutorial…
{{{
class Question(models.Model):
# ...
def was_published_recently(self):
now = timezone.now()
return now - datetime.timedelta(days=1) <= self.pub_date <= now
was_published_recently.admin_order_field = 'pub_date'
was_published_recently.boolean = True
was_published_recently.short_description = 'Published recently?'
}}}
See: https://docs.djangoproject.com/en/3.0/intro/tutorial07/#id8
And in https://docs.djangoproject.com/en/3.0/ref/contrib/admin/ :
> Elements of list_display can also be properties. Please note however,
that due to the way properties work in Python, setting short_description
or admin_order_field on a property is only possible when using the
property() function and not with the @property decorator.
>
> For example:
{{{
class Person(models.Model):
first_name = models.CharField(max_length=50)
last_name = models.CharField(max_length=50)
def my_property(self):
return self.first_name + ' ' + self.last_name
my_property.short_description = "Full name of the person"
my_property.admin_order_field = 'last_name'
full_name = property(my_property)
class PersonAdmin(admin.ModelAdmin):
list_display = ('full_name',)
}}}
End of citation.
And this is what I was referring earlier. You can’t set the attribute on
property, but you can set it to its `fget`. There’s no problem about using
the decorator. We just have do update this part of the documentation.
--
Ticket URL: <https://code.djangoproject.com/ticket/31558#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Alexandre Poitevin | Owner: Alexandre
| Poitevin
Type: New feature | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by felixxm):
Tutorial doesn't use `fget`. Again, variables defined in properties must
be known before creating a `property`, that's why setting variable for
decorated property raises `AttributeError`. If you really believe that we
should encourage users for override `fget` you can start a discussion on
DevelopersMailingList.
--
Ticket URL: <https://code.djangoproject.com/ticket/31558#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Alexandre Poitevin | Owner: Alexandre
| Poitevin
Type: New feature | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Alexandre Poitevin):
Not now, because in fact it is a separate debate.
The reason it began, is the modification of my ticket (no grudge
intended).
I’m gonna work on a PR. I think the good place to put the corresponding
unit test is `tests/modeladmin/tests_actions.py`.
--
Ticket URL: <https://code.djangoproject.com/ticket/31558#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Alexandre Poitevin | Owner: Alexandre
| Poitevin
Type: New feature | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Alexandre Poitevin):
So I do really think I’ve tracked down the reason why it works with
`short_description` and `admin_order_field`, but not with `boolean`.
I don’t know if this is the good place to make some kind of code review
(maybe in the dev mailist?), but anyway as a reference (at least for me):
For `short_description`, the code is in
`contrib.admin.utils.label_for_fields()`:
{{{
if hasattr(attr, "short_description"):
label = attr.short_description
elif (isinstance(attr, property) and
hasattr(attr, "fget") and
hasattr(attr.fget, "short_description")):
label = attr.fget.short_description
elif callable(attr):
if attr.__name__ == "<lambda>":
label = "--"
else:
label = pretty_name(attr.__name__)
else:
label = pretty_name(name)
}}}
The `property` case is properly handled.
For `admin_order_field`, it’s in
`contrib.admin.views.main.get_ordering_fields()`:
{{{
if callable(field_name):
attr = field_name
elif hasattr(self.model_admin, field_name):
attr = getattr(self.model_admin, field_name)
else:
attr = getattr(self.model, field_name)
if isinstance(attr, property) and hasattr(attr, 'fget'):
attr = attr.fget
return getattr(attr, 'admin_order_field', None)
}}}
Same thing.
But for `boolean`, it’s in
`contrib.admin.templatetags.admin_list.items_for_result()`:
{{{
boolean = getattr(attr, 'boolean', False)
result_repr = display_for_value(value,
empty_value_display, boolean)
}}}
No special handling for properties…
And `items_for_result()` has no test to begin with.
The implementation of the feature seems easy, but I’m not sure about how
to write a good test for that.
I also think that the retrieving of these models’ fields “metadata” should
be refactored to appear in one_place only.
There is already a `lookup_field()` in `contrib.admin.utils` but it seems
it’s not enough for `property` (and again no unit tests):
{{{
def lookup_field(name, obj, model_admin=None):
opts = obj._meta
try:
f = _get_non_gfk_field(opts, name)
except (FieldDoesNotExist, FieldIsAForeignKeyColumnName):
# For non-field values, the value is either a method, property or
# returned via a callable.
if callable(name):
attr = name
value = attr(obj)
elif hasattr(model_admin, name) and name != '__str__':
attr = getattr(model_admin, name)
value = attr(obj)
else:
attr = getattr(obj, name)
if callable(attr):
value = attr()
else:
value = attr
f = None
else:
attr = None
value = getattr(obj, name)
return f, attr, value
}}}
I think (lots of “I think”) that I need to write tests for this before
anything else.
I just need some orientations in order to take care of this.
--
Ticket URL: <https://code.djangoproject.com/ticket/31558#comment:10>
Django
Reporter: Alexandre Poitevin | Owner: (none)
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* owner: Alexandre Poitevin => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/31558#comment:11>
Django
Reporter: Alexandre Poitevin | Owner: Anvansh
| Singh
Type: New feature | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Anvansh Singh):
* owner: (none) => Anvansh Singh
* status: new => assigned
Comment:
I'm picking this up, will try to send a patch for this soon.
--
Ticket URL: <https://code.djangoproject.com/ticket/31558#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Alexandre Poitevin | Owner: Anvansh
| Singh
Type: New feature | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
* needs_docs: 0 => 1
Comment:
[https://github.com/django/django/pull/17092 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/31558#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: Alexandre Poitevin | Owner: Anvansh
| Singh
Type: New feature | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Mariusz Felisiak):
[https://github.com/django/django/pull/17263 New PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/31558#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: Alexandre Poitevin | Owner: Anvansh
| Singh
Type: New feature | Status: assigned
Component: contrib.admin | Version: dev
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 1 => 0
* stage: Accepted => Ready for checkin
* needs_docs: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/31558#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: Alexandre Poitevin | Owner: Anvansh
| Singh
Component: contrib.admin | Version: dev
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak <felisiak.mariusz@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"225328efd9814f2e922ee77fb48a3eea7428c397" 225328e]:
{{{
#!CommitTicketReference repository=""
revision="225328efd9814f2e922ee77fb48a3eea7428c397"
Fixed #31558 -- Added support for boolean attribute on properties in
ModelAdmin.list_display.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31558#comment:16>