[Django] #24580: Is there mistake?
Django
-------------------------------+------------------------
Reporter: liminspace | Owner: liminspace
Type: Bug | Status: new
Component: contrib.admin | Version: 1.8
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+------------------------
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)`:
--
Ticket URL: <https://code.djangoproject.com/ticket/24580>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: liminspace | Owner:
Type: | liminspace
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
Changes (by timgraham):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
* type: Bug => Cleanup/optimization
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24580#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: liminspace | Owner:
Type: | liminspace
Component: contrib.admin | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by liminspace):
Object `f` may have no attribute `rel` and it will raise exception.
--
Ticket URL: <https://code.djangoproject.com/ticket/24580#comment:2>
Django
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
Easy pickings: 0 | UI/UX: 0
Changes (by Adonys Alea Boffill):
* cc: aaboffill@… (added)
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24580#comment:3>
Django
--------------------------------------+------------------------------------
Reporter: None | Owner: None
Component: contrib.admin | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* Attachment "24580.diff" added.
Django
--------------------------------------+------------------------------------
Reporter: None | Owner: None
Component: contrib.admin | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/24580#comment:4>
Django
--------------------------------------+------------------------------------
Reporter: None | Owner: None
Component: contrib.admin | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 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>