Code

Opened 6 years ago

Closed 4 years ago

#7048 closed (fixed)

Support clearing FileFields with ModelForms

Reported by: jarrow Owned by: carljm
Component: Forms Version: master
Severity: Keywords: sprintSep2010
Cc: anball@…, code@…, david@…, msaelices@…, tahooie@…, digitaljhelms@…, andy@…, taavi@…, flosch@…, valera.grishin@…, liangent@…, nicola.murino@…, michelts@…, jdunck@…, drmeers@…, sebleier@…, herbert.poul@…, flavio.curella@…, aribao@…, dan.fairs@…, tjurewicz@…, mortas.11@…, brent.tubbs@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Currently one can set blank=True in the model. This works in the admin: It let's you save entries without the file. But once you submit a file, you cannot get rid of it!

So blank=True on FileFields currently means "blank first, then set forever". Which is a not the most common use case IMHO ;)

There already is a snippet implementing a RemovableFileField: http://www.djangosnippets.org/snippets/636/

If the deletion of the file from disk isn't desired I would like to at least contribute a patch to clear the field. I'm filing this for newforms-admin so it doesn't get outdated right away.

Any thoughts on this are greatly appreciated!

Attachments (7)

DeletableFileFields-00_code.diff (17.7 KB) - added by jarrow 6 years ago.
First version of the patch, no test fixes yet, docs are separate
DeletableFileFields-00_docs.txt (1.2 KB) - added by jarrow 6 years ago.
Docs as a simple text file as the docs refactor is going to land anyway …
DeletableFileFields-01.diff (19.5 KB) - added by jarrow 5 years ago.
Updated patch to r9381, included docs, checking blank=True before deleting (against forged POST data)
DeletableFileFields-02.diff (19.6 KB) - added by jarrow 5 years ago.
Moved documentation to the correct file
7048_r13753.diff (26.8 KB) - added by carljm 4 years ago.
7048_r13817.diff (26.8 KB) - added by carljm 4 years ago.
use hyphenated clear-checkbox name to better avoid name collisions
7048_r13926.diff (26.8 KB) - added by jezdez 4 years ago.
UI updates and a few other issues fixed

Download all attachments as: .zip

Change History (61)

comment:1 Changed 6 years ago by garcia_marc

  • Keywords fs-rf added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

I think that I asked it in the developers list, and the idea is not spend time in FileFields because they are being refactored. Marking this ticket as fs-rf to be considered.

comment:2 Changed 6 years ago by Gulopine

  • Keywords fs-rf removed

The filestorage refactor does indeed make it easier to delete files from within Python, but it doesn't extend to the admin interface at all. Removing the fs-rf keyword.

comment:3 Changed 6 years ago by uptimebox

  • Owner changed from nobody to uptimebox
  • Status changed from new to assigned

comment:4 Changed 6 years ago by isagalaev

  • Keywords yandex-sprint added

comment:5 Changed 6 years ago by Jonas

  • Cc code@… added

I'm very interested in a solution to this problem and would love to help out. Let me know if I can do something.

comment:6 Changed 6 years ago by jacob

  • milestone set to 1.0
  • Triage Stage changed from Unreviewed to Accepted
  • Version changed from newforms-admin to SVN

comment:7 Changed 6 years ago by DavidReynolds

  • Cc david@… added

comment:8 Changed 6 years ago by jarrow

  • Keywords nfa-someday yandex-sprint removed
  • Owner changed from uptimebox to jarrow
  • Status changed from assigned to new

Claiming the ticket as there hasn't been any activity in the last month. If there exists a patch from the Yandex sprint now would be a good time to post it :)

comment:9 Changed 6 years ago by isagalaev

Looks like we didn't get to it after all. Feel free to take it, thanks!

comment:10 Changed 6 years ago by garcia_marc

I don't have a patch for it, but this feature is working in a field that subclasses FileField on the stdimage application. So it should be easy to extract from there, and create a patch. Probably the patch will need to be improved later, but at least is a starting point.

stdimage is available at http://code.google.com/p/django-stdimage/

comment:11 Changed 6 years ago by jarrow

Cool, thx! I also got code from brosner. I will probably use both sources to compile a patch :)

Changed 6 years ago by jarrow

First version of the patch, no test fixes yet, docs are separate

Changed 6 years ago by jarrow

Docs as a simple text file as the docs refactor is going to land anyway ...

comment:12 Changed 6 years ago by jarrow

  • Has patch set
  • Needs tests set

After talking to brosner on #django-dev we decided to implement this on the ModelForm level. The attached patch introduces file deletion functionality for ModelForms which can be customized via its Meta class (see attached docs).

Work still to be done:

  • Check admin CSS
    • at least for the RTL design this is bound to break
    • what about inlines?
  • Fix regression tests
  • Write new tests ;)

Notes on the patch:

  • I had to add some special admin CSS to align everything properly. That can only be achieved with a fixed padding to the left ...
  • For the DeletableFileField I started with a MultiValueField till i had overwritten every piece of code in it ... now it just sublcasses Field. The widget still subclasses MultiWidget though.
  • The display of the current file moved from the admin to ModelForm. It just doesn't make sense to provide a delete option if the presence of a file isn't visible ;)

If I missed anything in this short description let me know. Please test and tell me what breaks I'll keep the patch up to date :)

comment:13 Changed 6 years ago by msaelices

  • Cc msaelices@… added

comment:14 Changed 6 years ago by brosner

  • milestone changed from 1.0 to post-1.0

After seeing what is really involved in this ticket. jarrow and I decided it is best to avoid this pre-1.0. There is just too much design decisions that need to be made to make this work the right way. I personally appreciate jarrow's work. This will certainly make it post-1.0. :)

comment:15 Changed 6 years ago by jarrow

Just a note to myself: Maybe the CSS could be shorter as I introduced <br/>s after writing the style rules. Haven't checked though. The <br/>s are there for a sensible default display without CSS when using for example as_table().

comment:16 Changed 6 years ago by jarrow

#6405 is about making MultiValueField pass through initial data. Maybe this could be closed if a deletable FileField is the only use case.

comment:17 Changed 5 years ago by tahooie

  • Cc tahooie@… added

Changed 5 years ago by jarrow

Updated patch to r9381, included docs, checking blank=True before deleting (against forged POST data)

comment:18 Changed 5 years ago by jarrow

The attached patch seems to work fine. The tests still need fixing though. I wanted to wait for some feedback on the code and approach :) RTL CSS also not done yet. But there seems to be an admin redesign forthcoming anyway.

Changed 5 years ago by jarrow

Moved documentation to the correct file

comment:19 Changed 5 years ago by jarrow

With the current default values for the ModelForm Meta class options the admin shows the delete checkbox and removes the file from disk. It does not remove empty directories. To enable this in the admin one currently needs to do something like this:

class TestModelAdmin(admin.ModelAdmin):
    def get_form(self, request, obj=None, **kwargs):
        defaults = {
            "files_delete_empty_dirs": True,
        }
        defaults.update(kwargs)
        return super(TestModelAdmin, self).get_form(request, obj, **defaults)

Maybe a settings option to make deletion of empty dirs the default would be nice. Or we could add a files_delete_empty_dirs option to the ModelAdmin class and pass it to the ModelForm (like we do with exclude etc.).

comment:20 Changed 5 years ago by digitaljhelms

  • Cc digitaljhelms@… added

comment:21 Changed 5 years ago by andybak

  • Cc andy@… added

comment:22 Changed 5 years ago by anonymous

  • Cc taavi@… added

comment:23 Changed 5 years ago by brosner

  • Owner changed from jarrow to brosner
  • Status changed from new to assigned

comment:24 Changed 5 years ago by anonymous

  • Cc flosch@… added

comment:25 Changed 5 years ago by kmtracey

  • Summary changed from Support clearing FileFields in the admin if blank=True to Support clearing FileFields with ModelForms

Updated summary to reflect scope of the ticket -- discussion and patch indicate the solution here will not be admin-specific.

comment:26 Changed 5 years ago by kmtracey

(Oh and forgot to mention assuming the wider scope, #10299 has been closed as a dupe.)

comment:27 Changed 5 years ago by Valera_Grishin

  • Cc valera.grishin@… added

comment:28 Changed 5 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:29 Changed 5 years ago by liangent

  • Cc liangent@… added

a problem...

if I created a subclass of ModelForm, then added some FileField(s) which are not in the Model, the latest patch will raise FieldDoesNotExist: Xxx has no field named 'xxx' .

comment:30 Changed 5 years ago by mrts

  • milestone set to 1.1

Seems to be in scope for 1.1 according to Version1.1Features.

comment:31 Changed 5 years ago by jacob

It looks like this patch metastasized and grew some unrelated features -- to wit, nothing about this ticket has anything to do with deleting files (?) and directories (!) as a side-effect of clearing a file field. That's a separate discussion that we haven't had yet, and we're certainly not sneaking in new behavior like that this late in the release cycle.

comment:32 Changed 5 years ago by jarrow

The deletion of files from disk was already in the code that Brian gave me. So I thought that it was in scope for the ticket. I also wouldn't call the deletion of files and directories unrelated. I would expect the admin to be able to get rid of uploaded files if it has the option to clear the field. But we could of course disable this by default. Sure the delete directories feature sounds scary, that's why it is off by default.

Brian wanted to review the ticket. I hope he finds the time to have a look at this (he seems to be rather busy at the moment ...).

comment:33 Changed 5 years ago by Alex

I'd like to suggest, in the strongest terms possible, that this shouldn't be a Meta option, it should be an option on the Field. Meta is for formwide options.

comment:34 Changed 5 years ago by Alex

  • milestone changed from 1.1 to 1.2

comment:35 Changed 5 years ago by anonymous

  • Cc nicola.murino@… added

comment:36 Changed 5 years ago by anonymous

  • Cc anball@… added

comment:37 Changed 5 years ago by michelts

  • Cc michelts@… added

comment:38 Changed 5 years ago by anonymous

  • Cc jdunck@… added

comment:39 Changed 5 years ago by DrMeers

  • Cc drmeers@… added

comment:40 Changed 4 years ago by anonymous

  • Cc sebleier@… added

comment:41 Changed 4 years ago by kahless

  • Cc herbert.poul@… added

comment:42 Changed 4 years ago by gogna

  • Cc flavio.curella@… added

comment:43 Changed 4 years ago by tolano

  • Cc aribao@… added

comment:44 Changed 4 years ago by ubernostrum

  • milestone 1.2 deleted

1.2 is feature-frozen, moving this feature request off the milestone.

comment:45 Changed 4 years ago by danfairs

  • Cc dan.fairs@… added

comment:46 Changed 4 years ago by trent

  • Cc tjurewicz@… added

comment:47 Changed 4 years ago by carljm

  • Owner changed from brosner to carljm
  • Status changed from assigned to new

Picking this up at the sprints. Going to rework the patch to remove file-deletion entirely (that can be a separate ticket/discussion) and keep things out of the form Meta per Alex's comment.

comment:48 Changed 4 years ago by carljm

  • Component changed from django.contrib.admin to Forms

Changed 4 years ago by carljm

comment:49 Changed 4 years ago by carljm

Added a new patch with these design choices:

  • No file deleting.
  • Clearability should be the default behavior of a forms.FileField(required=False) - and thus also models.FileField(blank=True).formfield(). It's a bug that FileFields are currently not clearable. Thus, no Meta options or extra field arguments are needed.
  • This involves a minor backwards incompatibility in form rendering (a possible checkbox where there was not one previously), which is documented in the 1.3 release notes.
  • It doesn't make sense to have a clearable file input that doesn't display the current value of the field, so that behavior is moved from AdminFileWidget to ClearableFileInput (which AdminFileWidget inherits).
  • The HTML rendering of the ClearableFileInput is customizable by subclassing and overriding some class attributes.
  • Contradictory input (checking the clear checkbox and simultaneously uploading a new file) results in a form validation error.

All of these design choices were checked in person with at least one core committer at the sprint, mostly with Malcolm.

For reviewers more comfortable with git, here is the github compare URL for my working branch for this patch: http://github.com/carljm/django/compare/master...ticket_7048

comment:50 Changed 4 years ago by carljm

  • Needs tests unset
  • Status changed from new to assigned

comment:51 Changed 4 years ago by carljm

  • Keywords sprintSep2010 added

Changed 4 years ago by carljm

use hyphenated clear-checkbox name to better avoid name collisions

comment:52 Changed 4 years ago by Althalus

  • Cc mortas.11@… added

comment:53 Changed 4 years ago by btubbs

  • Cc brent.tubbs@… added

Changed 4 years ago by jezdez

UI updates and a few other issues fixed

comment:54 Changed 4 years ago by jezdez

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

(In [13968]) Fixed #7048 -- Added ClearableFileInput widget to clear file fields. Thanks for report and patch, jarrow and Carl Meyer.

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.