Opened 5 years ago

Closed 7 months ago

Last modified 7 months ago

#14497 closed New feature (fixed)

ModelAdmin.readonly_fields isn't graceful with filefields.

Reported by: Keryn Knight <keryn@…> Owned by: paulcollins
Component: contrib.admin Version: 1.2
Severity: Normal Keywords: feature admin readonly filefield
Cc: kez.knight@…, adam@…, thepapermen, riccardo.magliocchetti@…, cmawebsite@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: yes

Description

Currently, if supplying a FileField (and presumably, an ImageField or anything subclassing FileField) in the list of readonly_fields on a ModelAdmin instance, the value stored against the database (the path to its location on the filesystem or storage backend) is displayed as plain text.

It would be neater if instead, it displayed an HTML link of path, as it does when displaying the standard file upload widget in a non-readonly context (though excepting the file upload field itself). This would allow for more intuitive handling of admin access to read-only forms with additional data (eg: downloading uploaded PDFs, which is the use-case I'm facing at the moment.)

Looking at the source, I think the place to tackle this would be AdminReadonlyField.contents() in source:django/trunk/django/contrib/admin/helpers.py but that is just a cursory glance.

Obviously, low priority feature request, rather than a bug.

Attachments (4)

admin-filefield-readonly-display.diff (819 bytes) - added by adamjforster 4 years ago.
admin-filefield-readonly-display_with_tests.diff (3.4 KB) - added by paulcollins 3 years ago.
14497.admin-filewidget-readonly.diff (3.7 KB) - added by julien 3 years ago.
14497-admin-filewidget-readonly.diff (4.0 KB) - added by rm_ 17 months ago.
Updated to apply to a recent git master, also simplified a bit by reusing current widgetadmin instead of creating a new one

Download all attachments as: .zip

Change History (40)

comment:1 Changed 5 years ago by dmoisset

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

I think that an issue here is that the file stored in a filefield is not necessarily mapped to an URL (and even if it is, it's not obvious which URL)

comment:2 Changed 5 years ago by rasca

  • Component changed from Contrib apps to django.contrib.admin
  • Triage Stage changed from Unreviewed to Accepted

I can confirm it shows as plain text.

The URL problem dmoisset mentions would also apply to the widget used when it's not a readonly_field.

comment:3 Changed 5 years ago by rasca

Related #14643

comment:4 Changed 4 years ago by julien

  • Severity set to Normal
  • Type set to Cleanup/optimization

comment:5 Changed 4 years ago by julien

  • Type changed from Cleanup/optimization to New feature

comment:6 Changed 4 years ago by adamjforster

  • Easy pickings unset
  • Owner changed from nobody to adamjforster
  • Status changed from new to assigned

Changed 4 years ago by adamjforster

comment:7 Changed 4 years ago by adamjforster

  • Cc adam@… added
  • Has patch set

I have added a patch that outputs readonly FileField (and it's subclasses such as ImageField) values as links to the download file.

comment:8 Changed 4 years ago by julien

  • UI/UX set

comment:9 Changed 4 years ago by julien

  • Needs tests set

comment:10 Changed 4 years ago by adamjforster

I'm not sure how to test this, can someone else take a look?

comment:11 Changed 4 years ago by akaariai

You might want to check out the admin's Selenium test cases found from django/contrib/admin/tests.py. Seems like the perfect fit for testing this.

comment:12 Changed 3 years ago by javiersanchez@…

Let me suggest a small adding. Usually is not necessary to show the complete path, but just the file name. This is trivial by "import os.path" and applying "os.path.basename" as below:

            return mark_safe(u'<a href="%s">%s</a>'
                % (escape(value.url),
                    os.path.basename(force_unicode(value))))
Last edited 7 months ago by timgraham (previous) (diff)

comment:13 Changed 3 years ago by paulcollins

  • Needs tests unset
  • Owner changed from adamjforster to paulcollins
  • Status changed from assigned to new

@adamjforster I'm grabbing this ticket since you called out for some help writing tests. Please feel free to grab it back if you'd like. I removed your check on for value.file, because it causes an IO exception (no such file) during the unittests. I figured that testing for the actual existence of the file wasn't really needed to generate the link to the file.

@akaariai I've added the Selenium tests, but I'm not sure if that's the fastest way to test that the field is rendering correctly in the admin. Suggestions?

@javiersanchez I can update the patch for this but that would make this particular interface inconsistent with the read/write interface. In the current version the link is rendered the same way, though it's rendered by different code.

comment:14 Changed 3 years ago by paulcollins

After some discussion with Julien I removed the Selenium tests and did basically the same thing with the django.test.client

Last edited 3 years ago by paulcollins (previous) (diff)

Changed 3 years ago by paulcollins

comment:15 Changed 3 years ago by paulcollins

Updated because julien was getting a unit test failure on his osx machine. Is there a bug with assertContains(html=True)?
If this passes on his machine then there might be an issue in there. More when I hear back from julien.

Changed 3 years ago by julien

comment:16 follow-up: Changed 3 years ago by julien

  • Patch needs improvement set

Thank you, Paul. I realized the test was failing for me because of the hardcoded "media/" in the url path. Using the string variable STORAGE_URL fixes that (see patch above).

I've also noticed that the same display logic is used on the changelist when a file field is in list_display. It'd be great to write an extra test to cover that test. Other than that, the patch looks great!

comment:17 in reply to: ↑ 16 Changed 3 years ago by paulcollins

  • Triage Stage changed from Accepted to Design decision needed

Replying to julien:

I've also noticed that the same display logic is used on the changelist when a file field is in list_display.


Uhoh. So this is something I completely missed. It seems that display_for_field is implicitly expected to just return a text value, not an HTML value.
If a read only file field is listed in the list_display then in the admin the link to edit that object vanishes. The HTML ends up looking like <a href='link to admin page'><a href='link to file'>filepath/filename</a></a>. Worse, if it's the only thing listed in list_display it leaves the end user no obvious way to go and actually edit the object.

An icky feeling fix would be to go and put the special case logic for this in contrib.admin.helpers.AdminReadonlyField.contents which is what's being called to display it on the modify object page. After some discussion with julien though, I'm kicking this back to DDN. Maybe at some point the AdminReadonlyField class will be refactored to allow for this kind of flexibility.

comment:18 Changed 3 years ago by thepapermen

  • Cc thepapermen added

comment:19 Changed 3 years ago by anonymous

+1
this feature would be very helpful

comment:20 follow-up: Changed 3 years ago by zout

+1
Weird it isn't there, please include this in the next minor release

comment:21 in reply to: ↑ 20 Changed 3 years ago by anonymous

+1!
Really want to have this feature available!

comment:22 Changed 2 years ago by aaugustin

  • Triage Stage changed from Design decision needed to Accepted

comment:23 Changed 2 years ago by rm_

  • Cc riccardo.magliocchetti@… added

comment:24 Changed 2 years ago by paulcollins

@aaugustin
So I see you've marked this as accepted. I can dig back into this issue again and see if my horrible hack solution would even still apply here. Assuming it does is that the direction I should move this in?

comment:25 Changed 2 years ago by aaugustin

I marked this ticket as "accepted" when we removed "design decision needed"; it seemed more appropriate than closing it. But I haven't looked at the patch.

Changed 17 months ago by rm_

Updated to apply to a recent git master, also simplified a bit by reusing current widgetadmin instead of creating a new one

comment:26 Changed 7 months ago by synotna

What needs to happen for this to finally make it into a release?

comment:27 Changed 7 months ago by timgraham

Submit a pull request on GitHub and uncheck "Patch needs improvement" so the ticket appears in the review queue. Then find a friend to review the patch and mark the ticket as "Ready for checkin".

comment:29 Changed 7 months ago by rm_

  • Patch needs improvement unset

comment:30 Changed 7 months ago by collinanderson

  • Cc cmawebsite@… added

comment:31 Changed 7 months ago by collinanderson

  • Triage Stage changed from Accepted to Ready for checkin

Thanks!

comment:32 Changed 7 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 2be621e44c1b5b68c895383f3e20b1f17ddb447a:

Fixed #14497 -- Improved admin widget for "read only" FileFields

Based on patch by Adam J Forster, Paul Collins, and Julien.

comment:33 Changed 7 months ago by Tim Graham <timograham@…>

In 4957b8a40615a082c9218c7c662fb8fccf46b14f:

[1.8.x] Fixed #14497 -- Improved admin widget for "read only" FileFields

Based on patch by Adam J Forster, Paul Collins, and Julien.

Backport of 2be621e44c1b5b68c895383f3e20b1f17ddb447a from master

comment:34 Changed 7 months ago by Tim Graham <timograham@…>

In 07cfe1bd822f4108b5029aef1615a17000d0b0dc:

Refs #14497 -- Handled empty readonly admin FileFields

comment:35 Changed 7 months ago by Tim Graham <timograham@…>

In 343c0875338128dad162d4806fe908fb31404d14:

[1.8.x] Refs #14497 -- Handled empty readonly admin FileFields

Backport of 07cfe1bd822f4108b5029aef1615a17000d0b0dc from master

comment:36 Changed 7 months ago by collinanderson

I also discovered that this takes precedence over list_display_links, but I think I like the new behavior.

Note: See TracTickets for help on using tickets.
Back to Top