#7753 closed (fixed)
NullBooleanField does not clean data correctly when widget=forms.HiddenInput
Reported by: | ElliottM | Owned by: | nobody |
---|---|---|---|
Component: | Forms | Version: | dev |
Severity: | Keywords: | ||
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
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 (3)
Change History (10)
by , 16 years ago
Attachment: | nullbool_clean.diff added |
---|
comment:1 by , 16 years ago
Triage Stage: | Unreviewed → Accepted |
---|
You should use tuples for the groups(ie (True, 'True')) in place of lists, otherwise this looks good by me.
comment:2 by , 16 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:3 by , 16 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → 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.
by , 16 years ago
Attachment: | 7753.nullboolean.diff added |
---|
Same patch with, hopefully, stronger tests
comment:4 by , 16 years ago
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.
fixed a tabbing thing