Opened 10 months ago

Closed 9 months 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, 10 months ago

Owner: set to Antoliny

comment:2 by Antoliny, 10 months ago

Description: modified (diff)

comment:3 by Antoliny, 10 months ago

Description: modified (diff)

comment:4 by Natalia Bidart, 10 months ago

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

Thank you Antoliny! Missing tests are always welcomed.

comment:6 by Antoliny, 10 months ago

Has patch: set

comment:7 by Sarah Boyce, 10 months 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, 10 months ago

Patch needs improvement: set

comment:9 by Sarah Boyce, 9 months ago

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