Code

Opened 6 years ago

Closed 4 years ago

Last modified 3 years ago

#9336 closed (fixed)

CheckboxInput should render 'False' value as unchecked

Reported by: bthomas Owned by: nobody
Component: Forms Version: 1.0
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Currently, BooleanField will accept a value of 'False' and convert it to a boolean False in cleaned_data, for the case where it accepts data from a hidden field. However, if it is rendered as a CheckboxInput widget again, it will be rendered as checked, as the string 'False' from the initial data is converted to a True value.

Failed example:
    w.render('is_cool', 'False')
Expected:
    u'<input type="checkbox" name="is_cool" />'
Got:
    u'<input checked="checked" type="checkbox" name="is_cool" value="False" />'

This was happening to me in a strange case of a form validating for the preview stage of FormPreview, but failing validation after the POST, but there are probably other cases where someone might want to render a hidden value of 'False' into a CheckboxInput.

Attachments (2)

checkbox_false.diff (1.8 KB) - added by bthomas 6 years ago.
Accept string versions of True and False, with regression tests
9336.diff (1.5 KB) - added by bthomas 5 years ago.
Sanitize input correctly in value_from_datadict

Download all attachments as: .zip

Change History (14)

Changed 6 years ago by bthomas

Accept string versions of True and False, with regression tests

comment:1 Changed 6 years ago by bthomas

  • Cc cpawlukosky@… added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 6 years ago by mtredinnick

I'd prefer to fix it so that nothing in Django is passing such a string into widgets. Checkbox widgets take booleans. It's not hard to pass in the right type. We should take advantage of Python's strong typing in this situation, not avoid it.

So I'm -1 on this change, preferring to fix the root problem instead.

comment:3 Changed 6 years ago by bthomas

  • Patch needs improvement set

It seems like CheckboxInput has always received a string, not a boolean, as its value input. A checked box submits a value of "on", and bool("on") is True. An unchecked box submits nothing, thus False. You can test this with a form containing a checkbox. If the checkbox is selected, and the form raises a ValidationError, the checkbox is rendered with a value of "on" like so:

<input checked="checked" type="checkbox" name="is_cool" value="on" />

but it should just be:

<input checked="checked" type="checkbox" name="is_cool" />

I think I agree with you that the data should be cleaned before being passed to render(), though I'm not sure of the best way to do it. Calling the field's clean() method won't work for invalid data. Something else needs to happen to the data in BoundField.as_widget before being passed to widget.render()

comment:4 Changed 5 years ago by jacob

  • Triage Stage changed from Unreviewed to Accepted

comment:5 Changed 5 years ago by bthomas

  • Cc cpawlukosky@… removed
  • Patch needs improvement unset

The patch on #9473 is following the same approach here. I don't think there's any reason to not fix this the way I've implemented it. If someone wants to re-implement field validation so that data is always sanitized separately, that seems more like a feature or improvement.

comment:6 Changed 5 years ago by bthomas

  • milestone set to 1.1

comment:7 follow-up: Changed 5 years ago by russellm

  • Patch needs improvement set

I may be missing something here, but I concur with Malcolm. This isn't the same fix as #9473 - that patch deals with the value_from_datadict function, which needs to be liberal on what it accepts based on the various widgets that could be used to render booleans. This patch deals with the init and render methods, which should both be dealing with cleansed data that we can reliably know to be boolean.

@bthomas: your original report indicated that this was a problem with FormPreview. If you could provide an example of how this problem manifests, we could work out if this is a problem with FormPreview, or if there is some other problem at work here.

comment:8 Changed 5 years ago by jacob

  • milestone changed from 1.1 to 1.2

Changed 5 years ago by bthomas

Sanitize input correctly in value_from_datadict

comment:9 in reply to: ↑ 7 Changed 5 years ago by bthomas

  • Patch needs improvement unset

Replying to russellm:

I may be missing something here, but I concur with Malcolm. This isn't the same fix as #9473 - that patch deals with the value_from_datadict function, which needs to be liberal on what it accepts based on the various widgets that could be used to render booleans. This patch deals with the init and render methods, which should both be dealing with cleansed data that we can reliably know to be boolean.

Quite correct, of course. I've attached a patch that fixes value_from_datadict instead

comment:10 Changed 4 years ago by jkocherhans

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

(In [12556]) Fixed #9336. Changed CheckboxInput to render 'True' and 'False' input strings as checked or not instead of as a value attribute. Thanks, bthomas.

comment:11 Changed 4 years ago by jkocherhans

(In [12557]) [1.1.X] Fixed #9336. Changed CheckboxInput to render 'True' and 'False' input strings as checked or not instead of as a value attribute. Backport of r12556 from trunk.

comment:12 Changed 3 years ago by jacob

  • milestone 1.2 deleted

Milestone 1.2 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.