Opened 6 weeks ago

Closed 4 weeks ago

#36038 closed Cleanup/optimization (wontfix)

Added a test case for the display_for_field function when a FileField is passed.

Reported by: Antoliny Owned by: Antoliny
Component: contrib.admin Version: dev
Severity: Normal Keywords: display_for_field, FileFIeld
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by Antoliny)

def display_for_field(value, field, empty_value_display):
    from django.contrib.admin.templatetags.admin_list import _boolean_icon
    ...
    elif isinstance(field, (models.IntegerField, models.FloatField)):
        return formats.number_format(value)
    elif isinstance(field, models.FileField) and value:
        return format_html('<a href="{}">{}</a>', value.url, value)
    ...

There is no test case for the display_for_field function when a FileField object is passed. --> admin_utils test code

When the FileField handling was added in the above function, I found that the test was conducted indirectly through the FileWidget readonly attribute. --> test code

However, I believe it would be better to have a direct test case for when a FileField is passed, considering that display_for_field function is globally accessible.

Change History (9)

comment:1 by Antoliny, 6 weeks ago

Owner: set to Antoliny

comment:2 by Antoliny, 6 weeks ago

Description: modified (diff)

comment:3 by Antoliny, 6 weeks ago

Description: modified (diff)

comment:4 by Natalia Bidart, 6 weeks ago

Component: Utilitiescontrib.admin
Triage Stage: UnreviewedAccepted
Version: 5.1dev

Thank you Antoliny! Missing tests are always welcomed.

comment:5 by Antoliny, 6 weeks ago

comment:6 by Antoliny, 6 weeks ago

Has patch: set

comment:7 by Sarah Boyce, 5 weeks ago

You're right that this is already covered by existing tests and this PR would add double cover.
The existing assert is "strong" enough that changes to display_for_field for FileField would make the existing test fail.
I would prefer we add tests where we are missing coverage or in parallel with a ticket for which the tests add value. Otherwise, it feels unnecessary

comment:8 by Sarah Boyce, 5 weeks ago

Patch needs improvement: set

comment:9 by Sarah Boyce, 4 weeks ago

Resolution: wontfix
Status: assignedclosed
Note: See TracTickets for help on using tickets.
Back to Top