Skip to content
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

Updated NestedBoundField to also handle empty string when rendering its form #3677

Merged
merged 2 commits into from Nov 27, 2015

Conversation

Ernest0x
Copy link
Contributor

@Ernest0x Ernest0x commented Nov 26, 2015

If a NestedBoundField field has a value of None and is inside another NestedBoundField field, it will have its value converted to an empty string while the form of its enclosing field is being rendered. So, NestedBoundField fields with an empty string value must be handled the same way as NestedBoundField fields with a None value.

…ts form

If a NestedBoundField field has a value of `None` and is inside another NestedBoundField field, it will have its value converted to an empty string while the form of its enclosing field is being rendered. So, NestedBoundField fields with an empty string value must be handled the same way as NestedBoundField fields with a `None` value.
@tomchristie
Copy link
Member

tomchristie commented Nov 26, 2015

Could you give a simple example, demonstrating exactly what gets rendered incorrectly, and how this fix affects it?

@Ernest0x
Copy link
Contributor Author

Ernest0x commented Nov 26, 2015

Could you give a simple example, demonstrating exactly what gets rendered incorrectly, and how this fix affects it?

Of course. I have made the following #3464 (comment)

@tomchristie
Copy link
Member

tomchristie commented Nov 26, 2015

Okay. That's something, tho a minimal code example would still be helpful.

@andrewdodd
Copy link

andrewdodd commented Nov 26, 2015

I agree, I'd love to see a basic example of how this occurs.

@Ernest0x
Copy link
Contributor Author

Ernest0x commented Nov 26, 2015

Ok, here is code that reproduces exactly the error that this pull request fixes:

from rest_framework import serializers
from rest_framework.renderers import HTMLFormRenderer

class Level2NestedSerializer(serializers.Serializer):
    some_field = serializers.CharField()

class Level1NestedSerializer(serializers.Serializer):
    nested2_field = Level2NestedSerializer(allow_null=True)
    another_field = serializers.CharField()

class Level0NestedSerializer(serializers.Serializer):
    nested1_field = Level1NestedSerializer()

serializer = Level0NestedSerializer(data={
    'nested1_field': {
        'nested2_field': None,
        'another_field': 'test'
    }
})

serializer.is_valid()

renderer = HTMLFormRenderer()
for field in serializer:
    rendered = renderer.render_field(field, {})

@tomchristie
Copy link
Member

tomchristie commented Nov 26, 2015

Okay, and how does the HTML form render before/after the fix. (Include screenshots if you're feeling generous! 😎 )

@Ernest0x
Copy link
Contributor Author

Ernest0x commented Nov 26, 2015

Okay, and how does the HTML form render before/after the fix. (Include screenshots if you're feeling generous! )

Before the fix, it does not render at all. It crashes with a traceback.
After the fix, it renders ok. For the nested field with None value, an empty form is rendered.

@tomchristie tomchristie added this to the 3.3.2 Release milestone Nov 27, 2015
@tomchristie
Copy link
Member

tomchristie commented Nov 27, 2015

Would you consider pulling together a test case? Failing that we might consider merging in any case and adding that as an outstanding issue.

@Ernest0x
Copy link
Contributor Author

Ernest0x commented Nov 27, 2015

Would you consider pulling together a test case?

Sure. Added in this pull request.

tomchristie added a commit that referenced this pull request Nov 27, 2015
Updated NestedBoundField to also handle empty string when rendering its form
@tomchristie tomchristie merged commit 8dea1ae into encode:master Nov 27, 2015
3 checks passed
@tomchristie
Copy link
Member

tomchristie commented Nov 27, 2015

Good stuff, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants