Code

Opened 7 years ago

Closed 2 years ago

#5893 closed New feature (fixed)

FilePathField doesn't support folders

Reported by: Alex Owned by: Alex
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 7 years ago.
diff.2.diff (3.9 KB) - added by Alex 7 years ago.
Updated to have files by default, but no folders
diff.3.diff (3.7 KB) - added by Alex 7 years ago.
Cleaned up the if/elif brackets, also appended folders first
diff.4.diff (3.8 KB) - added by Alex 7 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 7 years ago.
Cleaned up the if brackets again
allow_folders.diff (7.8 KB) - added by Alex 6 years ago.
Fixed a few typos
allow_folders.2.diff (7.8 KB) - added by Alex 6 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 6 years ago.
Updated patch: newforms -> forms
allow_folders.4.diff (11.8 KB) - added by Alex 5 years ago.
allow_folders.5.diff (11.8 KB) - added by Alex 5 years ago.
stupid literals
filepath-folders.diff (7.7 KB) - added by Alex 5 years ago.
filepath-folders.2.diff (10.3 KB) - added by Alex 5 years ago.
Resolves issues.
filepath-folders.3.diff (11.0 KB) - added by Alex 5 years ago.
Merge conflicts!
filepath-flolders-final.diff (8.9 KB) - added by Alex 2 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 7 years ago by Alex

comment:1 Changed 7 years ago by SmileyChris

  • Keywords enhancement added; new-forms, FilePathField removed
  • Needs documentation set
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Design decision needed
  • Version changed from SVN to newforms-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 7 years ago by Alex

Updated to have files by default, but no folders

Changed 7 years ago by Alex

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

Changed 7 years ago by Alex

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

Changed 7 years ago by Alex

Cleaned up the if brackets again

comment:2 Changed 7 years ago by brosner

  • Component changed from Admin interface to django.newforms
  • Version changed from newforms-admin to SVN

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

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

Changed 6 years ago by Alex

Fixed a few typos

Changed 6 years ago by Alex

Now fix_os_paths is applied to the output of the new tests

Changed 6 years ago by Alex

Updated patch: newforms -> forms

comment:4 Changed 6 years ago by Alex

  • Owner changed from nobody to Alex

Changed 5 years ago by Alex

Changed 5 years ago by Alex

stupid literals

Changed 5 years ago by Alex

comment:5 Changed 5 years ago by Alex

  • Owner changed from Alex to kmtracy
  • Triage Stage changed from Design decision needed to Accepted

comment:6 Changed 5 years ago by kmtracey

  • Owner changed from kmtracy to kmtracey

Put this on my list to look at.

comment:7 Changed 5 years ago by kmtracey

  • Owner changed from kmtracey to Alex
  • 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 5 years ago by kmtracey

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

Changed 5 years ago by Alex

Resolves issues.

comment:9 Changed 5 years ago by tiliv

  • Cc tonightslastsong@… added

Changed 5 years ago by Alex

Merge conflicts!

comment:10 Changed 3 years ago by gabrielhurley

  • Severity set to Normal
  • Type set to New feature

comment:11 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:12 Changed 2 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

Changed 2 years ago by Alex

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

comment:13 Changed 2 years ago by Alex

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

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.

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.