Opened 4 years ago
Closed 14 months ago
#31558 closed New feature (fixed)
Support the use of boolean attribute on properties in the admin.
Reported by: | Alexandre Poitevin | Owned by: | Anvansh Singh |
---|---|---|---|
Component: | contrib.admin | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | Hasan Ramezani | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
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…)
Change History (16)
comment:1 by , 4 years ago
Description: | modified (diff) |
---|---|
Summary: | Boolean attribute for a property getter of a model doesn’t active the “on/off” icon in the admin site → Support the use of boolean attribute on properties in the admin. |
Triage Stage: | Unreviewed → Accepted |
Type: | Bug → New feature |
Version: | 3.0 → master |
comment:2 by , 4 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:3 by , 4 years ago
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:
- 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.
- 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/
).
comment:4 by , 4 years ago
Cc: | added |
---|---|
Owner: | removed |
Status: | assigned → new |
comment:5 by , 4 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:6 by , 4 years ago
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.
comment:7 by , 4 years ago
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.
comment:8 by , 4 years ago
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.
comment:9 by , 4 years ago
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
.
comment:10 by , 4 years ago
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.
comment:11 by , 3 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:12 by , 16 months ago
Owner: | set to |
---|---|
Status: | new → assigned |
I'm picking this up, will try to send a patch for this soon.
comment:13 by , 16 months ago
Has patch: | set |
---|---|
Needs documentation: | set |
Patch needs improvement: | set |
comment:15 by , 14 months ago
Needs documentation: | unset |
---|---|
Patch needs improvement: | unset |
Triage Stage: | Accepted → Ready for checkin |
Thanks. This is definitely a new feature (see similar ticket #30259 for
admin_order_field
support). We can support it but when using theproperty()
function and not with decorator, that's why I updated the ticket description.