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 , 10 years ago
Summary: | Is there mistake? → Add test for untested condition in admin_list template tag. |
---|---|
Triage Stage: | Unreviewed → Accepted |
Type: | Bug → Cleanup/optimization |
comment:3 by , 8 years ago
Cc: | 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 , 8 years ago
Attachment: | 24580.diff added |
---|
comment:4 by , 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 , 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(' ')
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.
What problem are you seeing? I don't think your suggestion is correct as
f
is a model field instance whileManyToOneRel
is a "related field" so I don't think that condition would ever evaluate toTrue
. 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.