Opened 9 years ago

Closed 9 years ago

#6302 closed (fixed)

FileField/ImageField still requires a value when ModelForm is called when instance != None

Reported by: sdistefano@… Owned by: Brian Rosner
Component: Forms Version: master
Severity: Keywords: ModelForm FileField required
Cc: tom@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

This would make the users upload a new picture every time they change the form. A sensible default would be valued, and a custom handler to decide specific behavior is, I think, a must.

Attachments (9)

newforms-admin_filefield_initial_fix.diff (956 bytes) - added by oyvind @… 9 years ago.
Possible fix, not sure if this is the correct way to do this
pass_initial_through_messy_example.diff (4.4 KB) - added by oyvind@… 9 years ago.
Patch against newforms-admin, don't like all parts of it
pass_initial_through_with_tests.diff (5.7 KB) - added by Øyvind Saltvik <oyvind@…> 9 years ago.
Same patch with tests
pass_initial_through_with_tests_2.diff (41.2 KB) - added by Brian Rosner 9 years ago.
this might be overkill, but initial data is now passed to field.clean. should FileField be special cased?
pass_initial_through_with_tests_3.diff (8.4 KB) - added by Brian Rosner 9 years ago.
special cased to FileField
pass_initial_through_with_tests_4.diff (8.4 KB) - added by Øyvind Saltvik <oyvind@…> 9 years ago.
Testing for takes_initial instead of using isinstance for the FileField
pass_initial_through_with_tests_4.2.diff (8.1 KB) - added by Øyvind Saltvik <oyvind@…> 9 years ago.
Fixed a silly typo
pass_initial_through_with_tests_5.diff (9.9 KB) - added by Øyvind Saltvik <oyvind@…> 9 years ago.
Fixed a problem with combofield and filefield
pass_initial_through_with_tests_mulitvaluefield.diff (15.2 KB) - added by Øyvind Saltvik <oyvind@…> 9 years ago.
MultiValueField works too, is this overkill?

Download all attachments as: .zip

Change History (27)

comment:1 Changed 9 years ago by Brian Rosner

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

Can you please provide more detailed information on how actually this is not working? It is probably a user error in this case and should be brought up in django-users. As long as the FileField or ImageField has a value it will be marked as not required.

comment:2 Changed 9 years ago by Brian Rosner

Ok, I have verified this is occuring and it is not user error. The FieldField model field tries to set its formfield to require=False when there is initial data. Well, that worked well for for_for_instance , but now that ModelForm calls formfield before it gets the initial data from the model instance it will never be marked as False. I will have a patch whipped up shortly.

comment:3 Changed 9 years ago by Brian Rosner

Owner: changed from nobody to Brian Rosner

comment:4 Changed 9 years ago by Brian Rosner

Status: newassigned

Changed 9 years ago by oyvind @…

Possible fix, not sure if this is the correct way to do this

comment:6 Changed 9 years ago by Alex Gaynor

I haven't looked at the functionality but from oyvind's I would just say it should probably check that initial isn't none before setting required to false.

comment:7 Changed 9 years ago by oyvind@…

Just noticed that myself, but testing for empty values makes it fail for some reason.

Changed 9 years ago by oyvind@…

Patch against newforms-admin, don't like all parts of it

Changed 9 years ago by Øyvind Saltvik <oyvind@…>

Same patch with tests

comment:8 Changed 9 years ago by Øyvind Saltvik <oyvind@…>

Forgot to say, latest patch is against trunk.

Seems to be working

Should value_from_datadict handle initial too instead of making a own value_from_initial on the !Input! widget??

Changed 9 years ago by Brian Rosner

this might be overkill, but initial data is now passed to field.clean. should FileField be special cased?

Changed 9 years ago by Brian Rosner

special cased to FileField

comment:9 Changed 9 years ago by oyvind@…

Like the spechialcased solution, less backwardas compatibility issues too.

Changed 9 years ago by Øyvind Saltvik <oyvind@…>

Testing for takes_initial instead of using isinstance for the FileField

Changed 9 years ago by Øyvind Saltvik <oyvind@…>

Fixed a silly typo

Changed 9 years ago by Øyvind Saltvik <oyvind@…>

Fixed a problem with combofield and filefield

comment:10 Changed 9 years ago by Øyvind Saltvik <oyvind@…>

Latest patch does allow FileFields with MultiValueField , I and brosner think this is a edge case.

Since takes_initial can be added to fields this is left as a exercise to the user.

comment:11 Changed 9 years ago by oyvind@…

does not allow, i meant

Changed 9 years ago by Øyvind Saltvik <oyvind@…>

MultiValueField works too, is this overkill?

comment:12 Changed 9 years ago by jkocherhans

Triage Stage: UnreviewedAccepted

I'm going to check in http://code.djangoproject.com/attachment/ticket/6302/pass_initial_through_with_tests_3.diff mostly as is. Please open another ticket for fixing ComboField, etc. I'm not entirely sure about the takes_initial thing yet. It seems like over-engineering, but more isinstance checks make me cringe as well.

comment:13 Changed 9 years ago by jkocherhans

Resolution: fixed
Status: assignedclosed

(In [7021]) Fixed #6302. FileField no longer requires a value if one already exists. Thanks Brian Rosner and Øyvind Saltvik.

comment:14 Changed 9 years ago by Jacob

Resolution: fixed
Status: closedreopened

This fix appears not to work with ImageFields; I'm writing a test and fix.

comment:15 Changed 9 years ago by Jacob

Resolution: fixed
Status: reopenedclosed

(In [7025]) Fixed #6302 for ImageFields.

comment:16 Changed 9 years ago by Brian Rosner

Ah, good catch Jacob. I looked at the code you committed and could have sworn that was in my patch, but turns out for some reason I removed it from my new patch that special cased FileField. Should have had a test for ImageField. :)

comment:17 Changed 9 years ago by Thomas Steinacher <tom@…>

Cc: tom@… added
Resolution: fixed
Status: closedreopened

Still doesn't work for me in latest SVN. I am using RemovableImageField from the following code:
http://www.djangosnippets.org/snippets/469/

In RemovableFileFormField's init method, kwargs doesn't contain an initial attribute.

This worked perfectly with form_for_instance().

comment:18 Changed 9 years ago by Brian Rosner

Resolution: fixed
Status: reopenedclosed

If you looked at the code that was committed you would have seen that the initial data is not passed in through the kwargs in __init__. It is passed to the field clean method. Any further problems should really be put in a new ticket as this ticket as described was fixed. Thanks.

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