Opened 10 years ago

Closed 10 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 @… 10 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@… 10 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@…> 10 years ago.
Same patch with tests
pass_initial_through_with_tests_2.diff (41.2 KB) - added by Brian Rosner 10 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 10 years ago.
special cased to FileField
pass_initial_through_with_tests_4.diff (8.4 KB) - added by Øyvind Saltvik <oyvind@…> 10 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@…> 10 years ago.
Fixed a silly typo
pass_initial_through_with_tests_5.diff (9.9 KB) - added by Øyvind Saltvik <oyvind@…> 10 years ago.
Fixed a problem with combofield and filefield
pass_initial_through_with_tests_mulitvaluefield.diff (15.2 KB) - added by Øyvind Saltvik <oyvind@…> 10 years ago.
MultiValueField works too, is this overkill?

Download all attachments as: .zip

Change History (27)

comment:1 Changed 10 years ago by Brian Rosner

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 10 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 10 years ago by Brian Rosner

Owner: changed from nobody to Brian Rosner

comment:4 Changed 10 years ago by Brian Rosner

Status: newassigned

Changed 10 years ago by oyvind @…

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

comment:6 Changed 10 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 10 years ago by oyvind@…

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

Changed 10 years ago by oyvind@…

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

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

Same patch with tests

comment:8 Changed 10 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 10 years ago by Brian Rosner

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

Changed 10 years ago by Brian Rosner

special cased to FileField

comment:9 Changed 10 years ago by oyvind@…

Like the spechialcased solution, less backwardas compatibility issues too.

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

Testing for takes_initial instead of using isinstance for the FileField

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

Fixed a silly typo

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

Fixed a problem with combofield and filefield

comment:10 Changed 10 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 10 years ago by oyvind@…

does not allow, i meant

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

MultiValueField works too, is this overkill?

comment:12 Changed 10 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 10 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 10 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 10 years ago by Jacob

Resolution: fixed
Status: reopenedclosed

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

comment:16 Changed 10 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 10 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 10 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