Opened 9 years ago

Closed 5 years ago

#5893 closed New feature (fixed)

FilePathField doesn't support folders

Reported by: Alex Gaynor Owned by: Alex Gaynor
Component: Forms Version: master
Severity: Normal Keywords: enhancement
Cc: tonightslastsong@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

FilePathField(both the model field and the form field) only allows for selecting files, not folders. I have attached a fix to the problem which adds allow_files and allow_folders options, both of which default to true. This applies to both trunk and newforms-admin(which hasn't updated the FilePathField to use newforms, for whatever reason).

Attachments (14)

diff.diff (3.9 KB) - added by Alex Gaynor 9 years ago.
diff.2.diff (3.9 KB) - added by Alex Gaynor 9 years ago.
Updated to have files by default, but no folders
diff.3.diff (3.7 KB) - added by Alex Gaynor 9 years ago.
Cleaned up the if/elif brackets, also appended folders first
diff.4.diff (3.8 KB) - added by Alex Gaynor 9 years ago.
Added an assert to make sure either allow_files or allow_folders is true
diff.5.diff (3.7 KB) - added by Alex Gaynor 9 years ago.
Cleaned up the if brackets again
allow_folders.diff (7.8 KB) - added by Alex Gaynor 9 years ago.
Fixed a few typos
allow_folders.2.diff (7.8 KB) - added by Alex Gaynor 9 years ago.
Now fix_os_paths is applied to the output of the new tests
allow_folders.3.diff (7.7 KB) - added by Alex Gaynor 8 years ago.
Updated patch: newforms -> forms
allow_folders.4.diff (11.8 KB) - added by Alex Gaynor 8 years ago.
allow_folders.5.diff (11.8 KB) - added by Alex Gaynor 8 years ago.
stupid literals
filepath-folders.diff (7.7 KB) - added by Alex Gaynor 7 years ago.
filepath-folders.2.diff (10.3 KB) - added by Alex Gaynor 7 years ago.
Resolves issues.
filepath-folders.3.diff (11.0 KB) - added by Alex Gaynor 7 years ago.
Merge conflicts!
filepath-flolders-final.diff (8.9 KB) - added by Alex Gaynor 5 years ago.
Final patch (note to others, please don't review/commit; this is an example patch for a "contributing to django" event).

Download all attachments as: .zip

Change History (27)

Changed 9 years ago by Alex Gaynor

Attachment: diff.diff added

comment:1 Changed 9 years ago by Chris Beaven

Keywords: enhancement added; new-forms FilePathField removed
Needs documentation: set
Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedDesign decision needed
Version: SVNnewforms-admin

Interesting proposal. My suggestions (apart from the need for docs & tests):

  • change allow_folders to default to False (to keep the current default behaviour)
  • append folders before files to choices

Patch should also be applied against newforms-admin, which is a bit of a problem since this field hasn't been updated to newforms yet - #5894.

Changed 9 years ago by Alex Gaynor

Attachment: diff.2.diff added

Updated to have files by default, but no folders

Changed 9 years ago by Alex Gaynor

Attachment: diff.3.diff added

Cleaned up the if/elif brackets, also appended folders first

Changed 9 years ago by Alex Gaynor

Attachment: diff.4.diff added

Added an assert to make sure either allow_files or allow_folders is true

Changed 9 years ago by Alex Gaynor

Attachment: diff.5.diff added

Cleaned up the if brackets again

comment:2 Changed 9 years ago by Brian Rosner

Component: Admin interfacedjango.newforms
Version: newforms-adminSVN

This does not apply to only newforms-admin. This is a newforms and trunk problem that newforms-admin will inherit if it is approved for inclusion.

comment:3 Changed 9 years ago by Alex Gaynor

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

Changed 9 years ago by Alex Gaynor

Attachment: allow_folders.diff added

Fixed a few typos

Changed 9 years ago by Alex Gaynor

Attachment: allow_folders.2.diff added

Now fix_os_paths is applied to the output of the new tests

Changed 8 years ago by Alex Gaynor

Attachment: allow_folders.3.diff added

Updated patch: newforms -> forms

comment:4 Changed 8 years ago by Alex Gaynor

Owner: changed from nobody to Alex Gaynor

Changed 8 years ago by Alex Gaynor

Attachment: allow_folders.4.diff added

Changed 8 years ago by Alex Gaynor

Attachment: allow_folders.5.diff added

stupid literals

Changed 7 years ago by Alex Gaynor

Attachment: filepath-folders.diff added

comment:5 Changed 7 years ago by Alex Gaynor

Owner: changed from Alex Gaynor to kmtracy
Triage Stage: Design decision neededAccepted

comment:6 Changed 7 years ago by Karen Tracey

Owner: changed from kmtracy to Karen Tracey

Put this on my list to look at.

comment:7 Changed 7 years ago by Karen Tracey

Owner: changed from Karen Tracey to Alex Gaynor
Patch needs improvement: set

Hey Alex -- First, the file containing the new test was just converted to unit tests. Can you convert the new test?

Also I'm not sure about the asserts. For the model field I'd expect that contradiction to be be caught during validation. I see there are a couple of asserts in that file for auto field stuff, but it seems most errors of this sort are caught instead in get_validation_errors in django/core/management/validate.py. Is there some reason you went with an assert here instead?

For the form field, there are other such nonsensical argument combos that we just don't check for. The user is left with a form that can't ever validate (e.g. IntegerField with max_value lower than min_value), but that's what they asked for. Perhaps not as helpful as we could be, but using assert to catch invalid arg combos passed into Django just doesn't seem to fit in with the rest of the code...is there some reason this case is special and should be handled differently than other similar cases?

comment:8 Changed 7 years ago by Karen Tracey

Oh, and the doc changes need versionadded flags, don't they?

Changed 7 years ago by Alex Gaynor

Attachment: filepath-folders.2.diff added

Resolves issues.

comment:9 Changed 7 years ago by Tim Valenta

Cc: tonightslastsong@… added

Changed 7 years ago by Alex Gaynor

Attachment: filepath-folders.3.diff added

Merge conflicts!

comment:10 Changed 6 years ago by Gabriel Hurley

Severity: Normal
Type: New feature

comment:11 Changed 5 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:12 Changed 5 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

Changed 5 years ago by Alex Gaynor

Final patch (note to others, please don't review/commit; this is an example patch for a "contributing to django" event).

comment:13 Changed 5 years ago by Alex Gaynor

Resolution: fixed
Status: newclosed

In [17925]:

Fixed #5893 -- Added a flag to FilePathField to allow listing folders, in addition to regular files. Thank you to Brian Rosner, for encouraging me to first contribute to Django 4 years ago.

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