Django

Code

Ticket #6302 (closed: fixed)

Opened 6 months ago

Last modified 4 months ago

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

Reported by: sdistefano@gmail.com Assigned to: brosner
Milestone: Component: django.newforms
Version: SVN Keywords: ModelForm FileField required
Cc: tom@eggdrop.ch Triage Stage: Accepted
Has patch: 0 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 0

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

newforms-admin_filefield_initial_fix.diff (0.9 kB) - added by oyvind @saltvik.no on 01/09/08 04:13:32.
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@saltvik.no on 01/09/08 07:36:04.
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@saltvik.no> on 01/09/08 10:36:03.
Same patch with tests
pass_initial_through_with_tests_2.diff (41.2 kB) - added by brosner on 01/09/08 16:53:10.
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 on 01/09/08 18:16:22.
special cased to FileField?
pass_initial_through_with_tests_4.diff (8.4 kB) - added by Øyvind Saltvik <oyvind@saltvik.no> on 01/11/08 18:37:59.
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@saltvik.no> on 01/13/08 11:08:00.
Fixed a silly typo
pass_initial_through_with_tests_5.diff (9.9 kB) - added by Øyvind Saltvik <oyvind@saltvik.no> on 01/15/08 11:15:13.
Fixed a problem with combofield and filefield
pass_initial_through_with_tests_mulitvaluefield.diff (15.2 kB) - added by Øyvind Saltvik <oyvind@saltvik.no> on 01/16/08 05:06:35.
MultiValueField? works too, is this overkill?

Change History

01/02/08 12:12:22 changed by brosner

  • needs_better_patch changed.
  • needs_tests changed.
  • needs_docs changed.

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.

01/08/08 16:00:02 changed 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.

01/08/08 18:57:13 changed by brosner

  • owner changed from nobody to brosner.

01/08/08 18:57:22 changed by brosner

  • status changed from new to assigned.

01/08/08 19:41:16 changed by brosner

The discussion on django-dev: http://groups.google.com/group/django-developers/browse_frm/thread/68bd9110af6ab591

01/09/08 04:13:32 changed by oyvind @saltvik.no

  • attachment newforms-admin_filefield_initial_fix.diff added.

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

01/09/08 04:17:52 changed 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.

01/09/08 04:29:09 changed by oyvind@saltvik.no

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

01/09/08 07:36:04 changed by oyvind@saltvik.no

  • attachment pass_initial_through_messy_example.diff added.

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

01/09/08 10:36:03 changed by Øyvind Saltvik <oyvind@saltvik.no>

  • attachment pass_initial_through_with_tests.diff added.

Same patch with tests

01/09/08 10:44:43 changed by Øyvind Saltvik <oyvind@saltvik.no>

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

01/09/08 16:53:10 changed by brosner

  • attachment pass_initial_through_with_tests_2.diff added.

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

01/09/08 18:16:22 changed by brosner

  • attachment pass_initial_through_with_tests_3.diff added.

special cased to FileField?

01/09/08 18:50:03 changed by oyvind@saltvik.no

Like the spechialcased solution, less backwardas compatibility issues too.

01/11/08 18:37:59 changed by Øyvind Saltvik <oyvind@saltvik.no>

  • attachment pass_initial_through_with_tests_4.diff added.

Testing for takes_initial instead of using isinstance for the FileField?

01/13/08 11:08:00 changed by Øyvind Saltvik <oyvind@saltvik.no>

  • attachment pass_initial_through_with_tests_4.2.diff added.

Fixed a silly typo

01/15/08 11:15:13 changed by Øyvind Saltvik <oyvind@saltvik.no>

  • attachment pass_initial_through_with_tests_5.diff added.

Fixed a problem with combofield and filefield

01/15/08 11:20:24 changed by Øyvind Saltvik <oyvind@saltvik.no>

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.

01/15/08 17:34:17 changed by oyvind@saltvik.no

does not allow, i meant

01/16/08 05:06:35 changed by Øyvind Saltvik <oyvind@saltvik.no>

  • attachment pass_initial_through_with_tests_mulitvaluefield.diff added.

MultiValueField? works too, is this overkill?

01/17/08 11:39:37 changed by jkocherhans

  • 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.

01/17/08 12:03:22 changed by jkocherhans

  • status changed from assigned to closed.
  • resolution set to fixed.

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

01/18/08 09:44:48 changed by jacob

  • status changed from closed to reopened.
  • resolution deleted.

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

01/18/08 09:53:20 changed by jacob

  • status changed from reopened to closed.
  • resolution set to fixed.

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

01/18/08 10:06:53 changed 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. :)

03/10/08 09:58:56 changed by Thomas Steinacher <tom@eggdrop.ch>

  • cc set to tom@eggdrop.ch.
  • status changed from closed to reopened.
  • resolution deleted.

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().

03/10/08 10:10:58 changed by brosner

  • status changed from reopened to closed.
  • resolution set to fixed.

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/Change #6302 (FileField/ImageField still requires a value when ModelForm is called when instance != None)




Change Properties
Action