Django

Code

Ticket #7753 (closed: fixed)

Opened 5 months ago

Last modified 3 months ago

NullBooleanField does not clean data correctly when widget=forms.HiddenInput

Reported by: ElliottM Assigned to: nobody
Milestone: 1.0 Component: Forms
Version: SVN Keywords:
Cc: Triage Stage: Accepted
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 1

Description

Consider the following snippet of a forms.py file: 

class TestForm(forms.Form):
    null_bool=forms.NullBooleanField(widget=forms.HiddenInput, initial=True)

In django.newforms.widgets, the NullBooleanSelect? widget normalizes all the input to either True, False, or None. Up until now, the nullBooleanField.clean() method has expected the input to be in one of these three forms.

However, when a HiddenInput? widget is used, that normalization is bypassed, and the NullBooleanfield?.clean() method receives a string. In the case I meantioned above, the string would be 'True'. In the (non-null) BooleanField?.clean() method, specific tests are done for this, but they're not done in the NullBooleanField?.clean() method, and I believe they should be.

A patch is included, with tests.

For convenience, http://code.djangoproject.com/browser/django/trunk/django/newforms/widgets.py#L226 - NullBooleanSelect? widget http://code.djangoproject.com/browser/django/trunk/django/newforms/fields.py#L560 - Booleanfield field, with the NullBooleanfield? defined right below.

Attachments

nullbool_clean.diff (1.5 kB) - added by ElliottM on 07/15/08 00:12:42.
fixed a tabbing thing
nulbool_clean2.diff (1.5 kB) - added by ElliottM on 07/15/08 00:27:08.
replaced lists with tuples
7753.nullboolean.diff (2.0 kB) - added by julien on 08/26/08 10:01:42.
Same patch with, hopefully, stronger tests

Change History

07/15/08 00:12:42 changed by ElliottM

  • attachment nullbool_clean.diff added.

fixed a tabbing thing

07/15/08 00:12:57 changed by Alex

  • needs_better_patch changed.
  • stage changed from Unreviewed to Accepted.
  • needs_tests changed.
  • needs_docs changed.

You should use tuples for the groups(ie (True, 'True')) in place of lists, otherwise this looks good by me.

07/15/08 00:27:08 changed by ElliottM

  • attachment nulbool_clean2.diff added.

replaced lists with tuples

07/18/08 19:19:46 changed by julien

  • stage changed from Accepted to Ready for checkin.

08/23/08 16:27:28 changed by mtredinnick

  • needs_better_patch set to 1.
  • stage changed from Ready for checkin to Accepted.

I don't really like the test addition here. It's a bit fragile, since it's basically testing that the internal representation doesn't change. Instead, what should be tested is that when a field of this type is rendered as a hidden field, we correctly re-process that value back to True or False, as appropriate. In other words, the internal representation is irrelevant. What's important is that we round-trip the data correctly. So I think we can have a better test here before checking this in.

08/26/08 10:01:42 changed by julien

  • attachment 7753.nullboolean.diff added.

Same patch with, hopefully, stronger tests

08/26/08 10:07:38 changed by julien

I gave it a go at writing stronger tests. But you may disagree with those... Those tests don't do the full round-trip with transparent values (as, I agree with Malcolm, it would be ideal). But at least they assert the values both at the rendering and the cleaning stages.

08/28/08 10:06:48 changed by Alex

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

Fixed in r8661.

08/28/08 10:06:59 changed by jacob

(In [8661]) Fixed #7753: clean NullBooleanField correctly when using HiddenInput. Thanks to julien and ElliottM.

Add/Change #7753 (NullBooleanField does not clean data correctly when widget=forms.HiddenInput)




Change Properties
Action