Opened 3 months ago

Last modified 3 months ago

#31558 assigned New feature

Support the use of boolean attribute on properties in the admin.

Reported by: Alexandre Poitevin Owned by: Alexandre Poitevin
Component: contrib.admin Version: master
Severity: Normal Keywords:
Cc: Hasan Ramezani Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by felixxm)

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 (10)

comment:1 Changed 3 months ago by felixxm

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 Changed 3 months ago by Hasan Ramezani

Owner: changed from nobody to Hasan Ramezani
Status: newassigned

comment:3 Changed 3 months ago 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.

  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 Changed 3 months ago by Hasan Ramezani

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

comment:5 Changed 3 months ago by Hasan Ramezani

Owner: set to Alexandre Poitevin
Status: newassigned

comment:6 Changed 3 months ago 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.

comment:7 Changed 3 months ago 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.

comment:8 Changed 3 months ago 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.

comment:9 Changed 3 months ago 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.

comment:10 Changed 3 months ago 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.

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