Code

Opened 7 years ago

Closed 3 years ago

Last modified 3 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: robvdl Owned by: nobody
Component: contrib.admin Version: master
Severity: Keywords: ImageField
Cc: robillard.etienne@…, tjurewicz@…, kmike 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@…> 5 years ago.
Patch for admin filefield deletion
4979-admin-file-deletion.2.diff (4.1 KB) - added by Daniel Pope <dan@…> 5 years ago.
Admin filefield deletion (updated for style)

Download all attachments as: .zip

Change History (18)

comment:1 Changed 7 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 7 years ago by anonymous

  • Cc robillard.etienne@… added

comment:3 Changed 7 years ago by ubernostrum

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

Duplicate of #2983.

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

  • Resolution duplicate deleted
  • Status changed from closed to reopened

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 6 years ago by programmerq

  • Triage Stage changed from Unreviewed to Accepted

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

Patch for admin filefield deletion

comment:6 Changed 5 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 5 years ago by Daniel Pope <dan@…>

Admin filefield deletion (updated for style)

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

  • Patch needs improvement unset

comment:8 Changed 5 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 5 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 5 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 5 years ago by Alex

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 5 years ago by Alex

#7048 for reference.

comment:13 Changed 4 years ago by trent

  • Cc tjurewicz@… added

comment:14 Changed 4 years ago by anonymous

  • Cc kmike added

comment:15 Changed 3 years ago by dcramer

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

This appears to have been fixed in [13968]

comment:16 Changed 3 years ago by jezdez

Indeed, thanks.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.