Opened 9 years ago

Closed 6 years ago

Last modified 6 years ago

#4979 closed (fixed)

Admin does not allow removal of an image from an ImageField after it has been set, even if null=True and blank=True

Reported by: Rob van der Linde Owned by: nobody
Component: contrib.admin Version: master
Severity: Keywords: ImageField
Cc: robillard.etienne@…, tjurewicz@…, Mikhail Korobov Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:

Description

In my model, I have the following field:

photo = models.ImageField("Photo", upload_to="images", null=True, blank=True)

I am able to upload an image using the admin tool no problem, I am also able to change it to another image at a later stage, but I am not able to remove the image (e.g. set the field back to NULL), even though my field does allow null values. This might also apply to FileField fields.

Attachments (2)

4979-admin-file-deletion.diff (3.9 KB) - added by Daniel Pope <dan@…> 8 years ago.
Patch for admin filefield deletion
4979-admin-file-deletion.2.diff (4.1 KB) - added by Daniel Pope <dan@…> 8 years ago.
Admin filefield deletion (updated for style)

Download all attachments as: .zip

Change History (18)

comment:1 Changed 9 years ago by mariarchi

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

As far as I know, image removal is just not yet implemented in built-in admin.
There was a solution around on how to fix it in wiki on this website (checkbox "delete" appears near each image).

comment:2 Changed 9 years ago by anonymous

Cc: robillard.etienne@… added

comment:3 Changed 9 years ago by James Bennett

Resolution: duplicate
Status: newclosed

Duplicate of #2983.

comment:4 Changed 8 years ago by rodrigo.cr@…

Resolution: duplicate
Status: closedreopened

This is not a duplicate of 2983. The issue here is setting a previously filled in ImageField (or FileField) to blank, not the automated deletion of files when new ones are uploaded.

comment:5 Changed 8 years ago by Jeff Anderson

Triage Stage: UnreviewedAccepted

Changed 8 years ago by Daniel Pope <dan@…>

Patch for admin filefield deletion

comment:6 Changed 8 years ago by Daniel Pope <dan@…>

Has patch: set
Patch needs improvement: set

I've added an initial patch - needs a bit of cleaning up as I've made a bit of a mess wrt PEP8 so I will reupload in a moment.

In this patch I've taken the approach of False indicating a file explicitly unset, None indicating no new value. I don't particularly like that approach, but FileField is special-cased at the model, form and form field level so it defaults to preserving the current value. A cleaner approach might be to remove the special case that makes it default to preserving a value, and have FileField explicitly return the same file. But that would mean more upheaval, less compatibility.

Changed 8 years ago by Daniel Pope <dan@…>

Admin filefield deletion (updated for style)

comment:7 Changed 8 years ago by Daniel Pope <dan@…>

Patch needs improvement: unset

comment:8 Changed 8 years ago by Jacob

Patch needs improvement: set

This aproach is fine with me, but overloading False to mean "delete the file" smells bad to me. A relatively easy change that'd remove (some of) the smell would be to use a sentinel object (i.e. DeleteFile = object()) instead of False.

comment:9 Changed 8 years ago by Daniel Pope <dan@…>

To offer as much compatibility as possible for anyone using ModelAdmin hooks I thought it best to preserve the ability to do

if form.cleaned_data['file']:

So I suppose that could be achieved like this

class DeleteFileType(object):
    def __nonzero__(self):
        return False

DeleteFile = DeleteFileType()

comment:10 Changed 8 years ago by Jacob

You know, maybe we're over-thinking this a bit. Why not leave cleaned_data['file'] alone, and just deal with the checkbox as a separate bit of data (cleaned_data['__delete_file'] or somesuch)? Anyone using ModelAdmin hooks will need to update 'em anyway to deal with the new features, so there's not a big change here.

comment:11 Changed 8 years ago by Alex Gaynor

Jacob: unless I'm missing something I think there's a second ticket(in the 7000s I think) that uses a MultiValueField to pretty much do that.

comment:12 Changed 8 years ago by Alex Gaynor

#7048 for reference.

comment:13 Changed 7 years ago by trent

Cc: tjurewicz@… added

comment:14 Changed 6 years ago by anonymous

Cc: Mikhail Korobov added

comment:15 Changed 6 years ago by David Cramer

Resolution: fixed
Status: reopenedclosed

This appears to have been fixed in [13968]

comment:16 Changed 6 years ago by Jannis Leidel

Indeed, thanks.

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