-
-
Notifications
You must be signed in to change notification settings - Fork 6.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NullBooleanField should use forms.NullBooleanField just like django. And BooleanField should support default=True #7722
Comments
Hello, Thank you for the steps to reproduce that issue. Due to the title, I'm a bit confused. Which
In the current example that you provided, both of the fields are converted to That is the representation of the serializer:
TestBooleanSerializer():
id = IntegerField(label='ID', read_only=True)
acknowledged = BooleanField(allow_null=True, required=False)
autoRenewing = BooleanField(allow_null=True, label='AutoRenewing', required=False) Even though both Currently, that behavior can be provided with the following change in your sample code: class TestBoolean(models.Model):
CHECKLIST_OPTIONS = (
(None, 'Unknown'),
(True, 'Yes'),
(False, 'No'),
)
acknowledged = models.BooleanField(null=True, blank=True)
autoRenewing = models.NullBooleanField()
class TestBooleanSerializer(serializers.ModelSerializer):
autoRenewing = serializers.NullBooleanField()
class Meta:
model = TestBoolean
fields = '__all__' So do you think that this should be the default behavior when |
So @pythonwood is referring to And yes, it looks valid to me. I suspect that this broke for use when we moved away from using the deprecated model field, and didn't introduce some behaviour to switch between the correct form field type, depending on if the serializer field is nullable or not. Blergh. |
Actually it's less clear to me now if we ever used to render this correctly. We don't use Django's form rendering for the browsable API, instead we have our own templates. Our rendering for boolean fields is determined here... django-rest-framework/rest_framework/renderers.py Lines 293 to 295 in c69e2e4
We probably need some explicit override in the |
I understand. So for Either way, would it be okay for you if I work on this issue? |
I don't know. Possibly a bit odd for a template named |
I understand and I agree on that one (: So I will check the following multiple options to change this behavior:
Those are the options that I could come up with. Please let me know if I'm missing anything here. Thank you for your help @tomchristie . |
Okay, so "I saw that model BooleanFields are generated as ChoiceFields if there is a choices parameter on the model field definition" doesn't actually help here, because that only applies in the case where |
I had a little looked into drf source. because form/json data render and parser need to modify both, Maybe it take a lot patch ? |
Which part of the parser needs to be modified? I could not see it. Here, I was working on the approach of overwriting the rendering template for boolean fields with allow_null=True. I have added some tests and I was planning to add more tests for nested fields. It would be nice to get some early-feedback. That was the desired behavior, right? With the following model class TestBoolean(models.Model):
CHECKLIST_OPTIONS = (
(None, 'Unknown'),
(True, 'Yes'),
(False, 'No'),
)
boolean_choices_field = models.BooleanField(null=True, choices=CHECKLIST_OPTIONS)
nullable_boolean_field_1 = models.BooleanField(null=True)
nullable_boolean_field_2 = models.BooleanField(null=True)
nullable_boolean_field_3 = models.BooleanField(null=True)
null_boolean_field_1 = models.NullBooleanField()
null_boolean_field_2 = models.NullBooleanField()
null_boolean_field_3 = models.NullBooleanField()
regular_boolean_field = models.BooleanField()
class TestBooleanSerializer(serializers.ModelSerializer):
class Meta:
model = TestBoolean
fields = '__all__' class AnotherSerializer(serializers.Serializer):
nullable_boolean_field_1 = serializers.BooleanField(allow_null=True)
nullable_boolean_field_2 = serializers.BooleanField(allow_null=True)
nullable_boolean_field_3 = serializers.BooleanField(allow_null=True)
null_boolean_field_1 = serializers.NullBooleanField()
null_boolean_field_2 = serializers.NullBooleanField()
null_boolean_field_3 = serializers.NullBooleanField()
regular_boolean_field = serializers.BooleanField() With this |
Yeah, you do it. It is good enought for my case (It take "Unknow" became null is right, thanks). Hope to good case in nest mode and hope to merge soon. |
Render BooleanFields with allow_null=True as HTML select rather than as HTML checkbox in Browsable API encode#7722.
Render BooleanFields with allow_null=True as HTML select rather than as HTML checkbox in Browsable API encode#7722.
Render BooleanFields with allow_null=True as HTML select rather than as HTML checkbox in Browsable API encode#7722.
Render BooleanFields with allow_null=True as HTML select rather than as HTML checkbox in Browsable API encode#7722.
For anyone having trouble with parsing querystring params in regards to
|
Sorry, is it True or None? |
@merwok Ah yes that's right, I meant None. I made an edit now. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Keep open |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Keep 🤖 |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Should be fixed |
can you try and review the related PR of this issue? |
Checklist
master
branch of Django REST framework.Steps to reproduce
run shell command
boolean/models.py
add app in forboolean/settings.py
modify forboolean/urls.py
run shell command
stdout
Expected behavior
django doc https://docs.djangoproject.com/en/3.1/ref/models/fields/#booleanfield
class BooleanField(**options)¶
A true/false field.
The default form widget for this field is CheckboxInput, or NullBooleanSelect if null=True.
The default value of BooleanField is None when Field.default isn’t defined.
Actual behavior
django rest framework doc
When using HTML encoded form input be aware that omitting a value will always be treated as setting a field to False, even if it has a default=True option specified. This is because HTML checkbox inputs represent the unchecked state by omitting the value, so REST framework treats omission as if it is an empty checkbox input.
Note that Django 2.1 removed the blank kwarg from models.BooleanField. Prior to Django 2.1 models.BooleanField fields were always blank=True. Thus since Django 2.1 default serializers.BooleanField instances will be generated without the required kwarg (i.e. equivalent to required=True) whereas with previous versions of Django, default BooleanField instances will be generated with a required=False option. If you want to control this behaviour manually, explicitly declare the BooleanField on the serializer class, or use the extra_kwargs option to set the required flag.
The text was updated successfully, but these errors were encountered: