Code

Opened 6 years ago

Closed 6 years ago

Last modified 3 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: UI/UX:

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)

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

Download all attachments as: .zip

Change History (10)

Changed 6 years ago by ElliottM

fixed a tabbing thing

comment:1 Changed 6 years ago by Alex

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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

Changed 6 years ago by ElliottM

replaced lists with tuples

comment:2 Changed 6 years ago by julien

  • Triage Stage changed from Accepted to Ready for checkin

comment:3 Changed 6 years ago by mtredinnick

  • Patch needs improvement set
  • Triage 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.

Changed 6 years ago by julien

Same patch with, hopefully, stronger tests

comment:4 Changed 6 years ago 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.

comment:5 Changed 6 years ago by Alex

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

Fixed in r8661.

comment:6 Changed 6 years ago by jacob

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

comment:7 Changed 3 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

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.