#24580: Add test for untested condition in admin_list template tag.
--------------------------------------+------------------------------------
Reporter: None | Owner: None
Type: Cleanup/optimization | Status: new
Component: contrib.admin | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (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(' ')
}}}
-- 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24580#comment:5>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/django-updates/068.109c821c646095172c443a46fc3bf809%40djangoproject.com.
For more options, visit https://groups.google.com/d/optout.
