Opened 11 years ago

Closed 10 years ago

Last modified 7 years ago

#7753 closed (fixed)

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

Reported by: ElliottM Owned by: nobody
Component: Forms Version: master
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


Consider the following snippet of a 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, - NullBooleanSelect widget - Booleanfield field, with the NullBooleanfield defined right below.

Attachments (3)

nullbool_clean.diff (1.5 KB) - added by ElliottM 11 years ago.
fixed a tabbing thing
nulbool_clean2.diff (1.5 KB) - added by ElliottM 11 years ago.
replaced lists with tuples
7753.nullboolean.diff (2.0 KB) - added by Julien Phalip 10 years ago.
Same patch with, hopefully, stronger tests

Download all attachments as: .zip

Change History (10)

Changed 11 years ago by ElliottM

Attachment: nullbool_clean.diff added

fixed a tabbing thing

comment:1 Changed 11 years ago by Alex Gaynor

Triage Stage: UnreviewedAccepted

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

Changed 11 years ago by ElliottM

Attachment: nulbool_clean2.diff added

replaced lists with tuples

comment:2 Changed 11 years ago by Julien Phalip

Triage Stage: AcceptedReady for checkin

comment:3 Changed 10 years ago by Malcolm Tredinnick

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

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.

Changed 10 years ago by Julien Phalip

Attachment: 7753.nullboolean.diff added

Same patch with, hopefully, stronger tests

comment:4 Changed 10 years ago by Julien Phalip

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.

comment:5 Changed 10 years ago by Alex Gaynor

Resolution: fixed
Status: newclosed

Fixed in r8661.

comment:6 Changed 10 years ago by Jacob

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

comment:7 Changed 7 years ago by Jacob

milestone: 1.0

Milestone 1.0 deleted

Note: See TracTickets for help on using tickets.
Back to Top