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 Mariusz Felisiak)

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 Mariusz Felisiak, 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 siteSupport the use of boolean attribute on properties in the admin.
Triage Stage: UnreviewedAccepted
Type: BugNew feature
Version: 3.0master

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.

comment:2 by Hasan Ramezani, 4 years ago

Owner: changed from nobody to Hasan Ramezani
Status: newassigned

comment:3 by Alexandre Poitevin, 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:

  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.

  1. 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 Hasan Ramezani, 4 years ago

Cc: Hasan Ramezani added
Owner: Hasan Ramezani removed
Status: assignednew

comment:5 by Hasan Ramezani, 4 years ago

Owner: set to Alexandre Poitevin
Status: newassigned

comment:6 by Mariusz Felisiak, 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 Alexandre Poitevin, 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 Mariusz Felisiak, 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 Alexandre Poitevin, 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 Alexandre Poitevin, 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 Mariusz Felisiak, 3 years ago

Owner: Alexandre Poitevin removed
Status: assignednew

comment:12 by Anvansh Singh, 16 months ago

Owner: set to Anvansh Singh
Status: newassigned

I'm picking this up, will try to send a patch for this soon.

comment:13 by Mariusz Felisiak, 16 months ago

Has patch: set
Needs documentation: set
Patch needs improvement: set
Last edited 14 months ago by Anvansh Singh (previous) (diff)

comment:14 by Mariusz Felisiak, 14 months ago

comment:15 by Mariusz Felisiak, 14 months ago

Needs documentation: unset
Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:16 by Mariusz Felisiak <felisiak.mariusz@…>, 14 months ago

Resolution: fixed
Status: assignedclosed

In 225328e:

Fixed #31558 -- Added support for boolean attribute on properties in ModelAdmin.list_display.

Note: See TracTickets for help on using tickets.
Back to Top