changed_data works incorrectly with BooleanField and NullBooleanField

[Alexander Lyabah]

if you are using BooleanField with attribute show_hidden_initial=True, the form will show inverse changed_data. If field is not changed it will show as changed, and added to form.changed_data.

[Russell Keith-Magee]

I don't see why this needs to be on the v1.1 milestone at this late stage in the development cycle.

As I understand the problem, worst case, it will be an edge case annoyance. The only place I can see this being a problem is with a BooleanField in an inline model in the admin that has a callable initial value. Of course, it's possible that there might be an easier way to stimulate this problem, but the report doesn't provide any details on what that way might be. Sample models/actions/code would be extraordinarily helpful.

Also - I'm not particularly inspired by the fact that the proposed fix breaks any number of tests in the Django test suite.

[Alexander Lyabah]

this is my little example, for better describing.

from django import forms
from django.shortcuts import render_to_response
class InForm(forms.Form):
    name = forms.CharField(show_hidden_initial=True)
    is_a = forms.BooleanField(show_hidden_initial=True, required=False)
def index(request):
    if request.method =='POST':
        form = InForm(request.POST)
        if form.is_valid():
            print form.changed_data 
    else:
        form = InForm(initial = {'name':'HI','is_a':False})
    return render_to_response('index.html',{'text':'HI ALL','form':form})

if just submit displayed form, "print form.changed_data" shows that the 'is_a' in changed_data array. But it wasn't change.

As I see, the problem in clean data of initial hidden input. So, I prepare a new patch. And I want to get your diagnosis.

[Alex Gaynor]

Please do not mark your own patches as RFC, further a ticket with "Needs tests" cannot possibly be RFC.

[Alexander Lyabah]

ok. sorry.

[punteney]

updated patch and test case

[punteney]

Updated the patch to do the fix at the CheckBoxInput widget _has_changed function level as it was a problem with the string 'False" being evaluated as a boolean and returning as True. Also added a test to check for this issue

[Margie Roginski]

I just want to mention that I have also encountered this bug and I don't think it is just a corner case in the admin as described by Russelm. I believe this is a general bug with the case where a BooleanField has show_hidden_initial set to True. When the HiddenInput widget for this field is rendered based on POST data and the POST data contains no data for the field (which occurs when the check box is unchecked), the HiddenInput is rendered with the string "False" as its value. In this case. when CheckboxInput's _has_changed() method is run to compare initial with data, intial is set to "False" and data is set to False (the python False value, not the string "False"). In the released code _has_changed() looks like this:

        return bool(initial) != bool(data)

With initial set to "False" and data set to python's False value, this returns True, which is incorrect. This is a situation where the user has not changed the value of the field, yet due to the rendering of the HiddenInput with the string "False", has_changed() will return True.

The attached patch does seem to work for me (the one called "show_hidden_initial_boolean.diff").

Margie

[Margie Roginski]

I've just encountered this problem again, but in a slightly different way. I have a BooleanField in my model and it is has value False. When I create the corresponding BooleanField in my modelForm, the initial value sent out in the hidden input is '0'. In other words, request.POST contains

u'initial-requireResultOnClose': [u'0']

If I then check the checkbox, the field comes back with 'on', like this:

u'requireResultOnClose': [u'on']

The _has_changed() method for CheckBoxInput() returns True when it compares u'0' and u'on' due to the fact that bool(u'0') is True, but it should really return False.

I have changed _has_changed() for CheckBoxInput() to look like this, which seems to work form my cases:

def _has_changed(self, initial, data):

# Sometimes data or initial could be None or u which should be the
# same thing as False.
if initial in ('False', '0'):

initial = False

return bool(initial) != bool(data)

In fields.py, BooleanField's clean() method does something similar, so I'm hoping this is the right direction. In general it seems very difficult to compare all these values such as False, 'False', u'0', u'1', True, 'True' and get it right. The hidden initial stuff adds an extra complication due to the fact that the hidden input is rendered through the hidden_widget() widget, which uses different values than those used by the CheckBoxInput. IE, in the CheckBoxInput widget when data is posted, the value 'on' indicates it is checked and no input at all in the post data indicates it is unchecked. But when using the CheckBoxInput's hidden_widget() widget, you see values u'0' and u'1', and it seems that these are really not taken into account in the current code base, probably due to the fact that few people are using the show_hidden_initial functionality.

[Ilya Semenov]

I agree with @margieroginski. The current show_hidden_input implementation is just broken when it comes to booleans/checkboxes. @russellm, this isn't an "edge case annoyance...in the admin" at all. The show_hidden_input logic is very useful when you allow to edit a single (shared) model instance to many users simultaneously; they won't be posting "old" data and rewrite all changes made by the other collaborators. You shouldn't underestimate the importance, as this is actually a very common use case - that would be useful even in this Django bug tracker, since that's not a rare situation when someone's changes to Summary or Triage Stage is erroneously "reverted" by the next poster - a perfect example where hidden inputs should have been added.

I found this ticket because I first developed my own generic implementation of exactly the same logic - I was saving pristine values along with the form and then checking them on submit. Then I discovered the show_hidden_input parameter and rewrote my app to use it instead. The funny thing is that in my implementation, I didn't have the discussed bug since I was testing on a form which had BooleanFields from the very beginning so I specifically added a workaround. You can imagine my disappointment when my code suddenly broke when switched to the "original" Django solution.

In general it seems very difficult to compare all these values such as False, 'False', u'0', u'1', True, 'True' and get it right.

I can't agree with that. Overestimating the complexity of the problem is no better than underestimating its importance :)
There's no such things as 'False'. How did you get that? When a python value is rendered, it gets through force_unicode() and False can only become '0'. Therefore, the logic is very simple:
1) no data, '' and '0' is False
2) all the rest is True

The bug can be traced to this code in forms.Form:

def _get_changed_data(self):
    # ...
    for name, field in self.fields.items():
        # ...
        if field.widget._has_changed(initial_value, data_value):
            self._changed_data.append(name)

The initial_data here is taken from hidden_widget (which is filled with '0' for False when a form is first rendered) which is then casted to True by bool() inside CheckboxInput._has_changed.
Therefore, the two possible fixes could be:
1) make CheckboxInput._has_changed smarter and treat initial='0' as False
2) add a custom BooleanField.hidden_widget (so called BooleanHiddenInput) which would store '' for False instead of '0'

I prefer the second approach as it allows to get rid of hard coded comparisons against string '0'.

[Ilya Semenov]

Attached a patch against Django 1.1.1 release which adds a HiddenBooleanInput widget and sets it as a hidden_widget for BooleanField. That did the trick for me and I attached it because it might be useful for the other people, but this is of course far from a final fix. I see the following concerns:

1) This is only tested with ModelForm and models.BooleanField, i.e. when the initial form data can be 0 and 1 (not False and True) - which is weird by the way (why would a models.BooleanField return integers instead of booleans?)

2) NullBooleanField appears to be desperately broken in the similar manner. At least, NullBooleanSelect._has_changed has a simple logic of comparing bools (copy-pasted from CheckboxInput), without any regards to the fact that there might be None's as well.

[Aymeric Augustin]

Change UI/UX from NULL to False.

[Aymeric Augustin]

Change Easy pickings from NULL to False.

[Claude Paroz]

I just committed [d11038acb2ea2f59a1d00a41b553f] which should have fixed this issue. I didn't handle the '0' case, as I think that we should stick with real boolean value here.
NullBooleanSelect issue should have been treated in #11860.