Opened 16 years ago

Closed 16 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: dev
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: no UI/UX: no

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

Download all attachments as: .zip

Change History (27)

comment:1 by Brian Rosner, 16 years ago

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

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

Owner: changed from nobody to Brian Rosner

comment:4 by Brian Rosner, 16 years ago

Status: newassigned

by oyvind @…, 16 years ago

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

comment:6 by Alex Gaynor, 16 years ago

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 by oyvind@…, 16 years ago

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

by oyvind@…, 16 years ago

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

by Øyvind Saltvik <oyvind@…>, 16 years ago

Same patch with tests

comment:8 by Øyvind Saltvik <oyvind@…>, 16 years ago

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??

by Brian Rosner, 16 years ago

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

by Brian Rosner, 16 years ago

special cased to FileField

comment:9 by oyvind@…, 16 years ago

Like the spechialcased solution, less backwardas compatibility issues too.

by Øyvind Saltvik <oyvind@…>, 16 years ago

Testing for takes_initial instead of using isinstance for the FileField

by Øyvind Saltvik <oyvind@…>, 16 years ago

Fixed a silly typo

by Øyvind Saltvik <oyvind@…>, 16 years ago

Fixed a problem with combofield and filefield

comment:10 by Øyvind Saltvik <oyvind@…>, 16 years ago

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 by oyvind@…, 16 years ago

does not allow, i meant

by Øyvind Saltvik <oyvind@…>, 16 years ago

MultiValueField works too, is this overkill?

comment:12 by jkocherhans, 16 years ago

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 by jkocherhans, 16 years ago

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 by Jacob, 16 years ago

Resolution: fixed
Status: closedreopened

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

comment:15 by Jacob, 16 years ago

Resolution: fixed
Status: reopenedclosed

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

comment:16 by Brian Rosner, 16 years ago

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 by Thomas Steinacher <tom@…>, 16 years ago

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

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