Opened 2 years ago

Last modified 9 months ago

#24580 new Cleanup/optimization

Add test for untested condition in admin_list template tag.

Reported by: None Owned by: None
Component: contrib.admin Version: 1.8
Severity: Normal Keywords:
Cc: aaboffill@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In file django/contrib/admin/templatetags/admin_list.py is it:

                if isinstance(f.rel, models.ManyToOneRel):  # <-- Is it OK?
                    field_val = getattr(result, f.name)
                    if field_val is None:
                        result_repr = EMPTY_CHANGELIST_VALUE
                    else:
                        result_repr = field_val
                else:
                    result_repr = display_for_field(value, f)
                if isinstance(f, (models.DateField, models.TimeField, models.ForeignKey)):
                    row_classes.append('nowrap')

I think isinstance(f.rel, models.ManyToOneRel) must be replaced to isinstance(f, models.ManyToOneRel):

Attachments (1)

24580.diff (1.1 KB) - added by Tim Graham 9 months ago.

Download all attachments as: .zip

Change History (6)

comment:1 Changed 2 years ago by Tim Graham

Summary: Is there mistake?Add test for untested condition in admin_list template tag.
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

What problem are you seeing? I don't think your suggestion is correct as f is a model field instance while ManyToOneRel is a "related field" so I don't think that condition would ever evaluate to True. Checking the coverage report, it appears that branch is being executed, however, no tests fail when I make the suggested change, so we may want to add some assertions to the relevant tests.

comment:2 Changed 2 years ago by None

Object f may have no attribute rel and it will raise exception.

comment:3 Changed 9 months ago by Adonys Alea Boffill

Cc: aaboffill@… added

This ticket seems fixed. There are almost 2 years since this ticket was created, and Django has continually evolved. For example, the code fragment shown in the ticket's description has actually some little changes.

               # if isinstance(f.rel, models.ManyToOneRel):  <= old line...
               if isinstance(f.remote_field, models.ManyToOneRel):
                    field_val = getattr(result, f.name)
                    if field_val is None:
                        result_repr = empty_value_display
                    else:
                        result_repr = field_val
                else:
                    result_repr = display_for_field(value, f, empty_value_display)
                if isinstance(f, (models.DateField, models.TimeField, models.ForeignKey)):
                    row_classes.append('nowrap')


I analysed the tests where this fragment of code is involved and I've the following result:

  • Admin tests where the condition if isinstance(f.remote_field, models.ManyToOneRel) is used:
Test Name Number of Tests
admin_changelist 10
admin_views 48
admin_widgets 1
  • Admin tests where the condition if isinstance(f.remote_field, models.ManyToOneRel) returns true:
Test Name Number of Tests
admin_changelist 7
admin_views 13
admin_widgets 1
  • Admin tests where the condition if isinstance(f.remote_field, models.ManyToOneRel) returns false:
Test Name Number of Tests
admin_changelist 10
admin_views 48
admin_widgets 1

This condition if isinstance(f.remote_field, models.ManyToOneRel) in admin_list template tag works good. I think that there are not other ticket related specifically with that, and the Django admin items_for_result method works fine. Is still necessary create other test specifically for that condition??
I think that this ticket could be closed as fixed.

Changed 9 months ago by Tim Graham

Attachment: 24580.diff added

comment:4 Changed 9 months ago by Tim Graham

I don't see any test failures with the attached patch applied. The idea is to add test assertions to show why that code is needed, if it is.

comment:5 Changed 9 months ago by Adonys Alea Boffill

Umm!! now I understand your point. Well, I think that there are not a way to prove that this code introduce any difference to final result, because both case end up making a force_text to a model instance.

  • Situation 1:

The condition if isinstance(f.remote_field, models.ManyToOneRel) returns true and field_val is not None, then result_repr would be a model instance, see https://github.com/django/django/blob/master/django/contrib/admin/templatetags/admin_list.py#L245. In any other place where result_repr is used, is used as format string parameter or to receive a new value.

-- Here: https://github.com/django/django/blob/master/django/contrib/admin/templatetags/admin_list.py#L251

result_repr = mark_safe('&nbsp;')

-- Here: https://github.com/django/django/blob/master/django/contrib/admin/templatetags/admin_list.py#L260-L285

link_or_text = result_repr
#----------------------------------------------------
yield format_html('<{}{}>{}</{}>',
                              table_tag,
                              row_class,
                              link_or_text,
                              table_tag)

-- Here: https://github.com/django/django/blob/master/django/contrib/admin/templatetags/admin_list.py#L279

             link_or_text = format_html(
                    '<a href="{}"{}>{}</a>',
                    url,
                    format_html(
                        ' data-popup-opener="{}"', value
                    ) if cl.is_popup else '',
                    result_repr)

-- Here: https://github.com/django/django/blob/master/django/contrib/admin/templatetags/admin_list.py#L294

result_repr = mark_safe(force_text(bf.errors) + force_text(bf))

-- Here: https://github.com/django/django/blob/master/django/contrib/admin/templatetags/admin_list.py#L295

yield format_html('<td{}>{}</td>', row_class, result_repr)
  • Situation 2:

Use only result_repr = display_for_field(value, f, empty_value_display). The final result is the same, because value is a model instance returned by lookup_field method in https://github.com/django/django/blob/master/django/contrib/admin/utils.py#L278-L304, when isinstance(f.remote_field, models.ManyToOneRel) is true. Then, the display_for_field method will call display_for_value in https://github.com/django/django/blob/master/django/contrib/admin/utils.py#L421 and display_for_value finally will return return force_text(value) in https://github.com/django/django/blob/master/django/contrib/admin/utils.py#L440.

In both cases if is None the empty_value_display will be shown.

My conclusions is that we can use result_repr = display_for_field(value, f, empty_value_display) directly, If you are agree Tim, I can make the change and ensure that every tests runs fine again.

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