Code

Opened 6 years ago

Closed 6 years ago

#6302 closed (fixed)

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

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

Download all attachments as: .zip

Change History (27)

comment:1 Changed 6 years ago by brosner

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

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

  • Owner changed from nobody to brosner

comment:4 Changed 6 years ago by brosner

  • Status changed from new to assigned

Changed 6 years ago by oyvind @…

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

comment:6 Changed 6 years ago by Alex

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

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

Changed 6 years ago by oyvind@…

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

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

Same patch with tests

comment:8 Changed 6 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 6 years ago by brosner

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

Changed 6 years ago by brosner

special cased to FileField

comment:9 Changed 6 years ago by oyvind@…

Like the spechialcased solution, less backwardas compatibility issues too.

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

Testing for takes_initial instead of using isinstance for the FileField

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

Fixed a silly typo

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

Fixed a problem with combofield and filefield

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

does not allow, i meant

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

MultiValueField works too, is this overkill?

comment:12 Changed 6 years ago by jkocherhans

  • Triage Stage changed from Unreviewed to Accepted

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

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

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

comment:14 Changed 6 years ago by jacob

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

comment:15 Changed 6 years ago by jacob

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

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

comment:16 Changed 6 years ago by brosner

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

  • Cc tom@… added
  • Resolution fixed deleted
  • Status changed from closed to reopened

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

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

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.

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.