Opened 10 years ago

Last modified 8 years 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):

According to the ticket's flags, the next step(s) to move this issue forward are:

  • To provide a patch by sending a pull request. Claim the ticket when you start working so that someone else doesn't duplicate effort. Before sending a pull request, review your work against the patch review checklist. Check the "Has patch" flag on the ticket after sending a pull request and include a link to the pull request in the ticket comment when making that update. The usual format is: [https://github.com/django/django/pull/#### PR].

Change History (6)

comment:1 by Tim Graham, 10 years ago

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 by None, 10 years ago

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

comment:3 by Adonys Alea Boffill, 8 years ago

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.

by Tim Graham, 8 years ago

Attachment: 24580.diff added

comment:4 by Tim Graham, 8 years ago

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 by Adonys Alea Boffill, 8 years ago

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