Add test for untested condition in admin_list template tag.

[None]

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):

[Tim Graham]

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.

[None]

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

[Adonys Alea Boffill]

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.

[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.

[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(' ')

-- 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.