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

PoC Add JSONBoundField to serializers (Fixes #4999) #5042

Merged
merged 3 commits into from Apr 27, 2017
Merged

PoC Add JSONBoundField to serializers (Fixes #4999) #5042

merged 3 commits into from Apr 27, 2017

Conversation

READ10
Copy link

@READ10 READ10 commented Apr 2, 2017

Per issue #4999, JSONFields are not rendered properly in the DRF
browsable API HTML forms. This patch attempts to fix that behavior by
introducing a JSONBoundField helper similar to the NestedBoundField
helper.

Description

I experienced #4999 today and put together this patch as an attempt to fix it. I'd appreciate any feedback as to whether this approach makes sense as this is my first dive into the DRF code. I could particularly use some guidance on what tests, documentation, etc. I should add.

./runtests.py reports all tests pass. tox reports failures in py35-djangomaster and py36-djangomaster, but those are present in my environment without this patch also. All other tox tests pass.

@xordoquy xordoquy added the Bug label Apr 4, 2017
@xordoquy xordoquy added this to the 3.6.3 Release milestone Apr 4, 2017
@xordoquy
Copy link
Collaborator

xordoquy commented Apr 4, 2017

Looks good to me. Need to think a bit about how this plays with the JSONField's binary argument.

@@ -82,6 +83,12 @@ def as_form_field(self):
return self.__class__(self._field, value, self.errors, self._prefix)


class JSONBoundField(BoundField):
def as_form_field(self):
value = json.dumps(self.value)
Copy link
Member

@tomchristie tomchristie Apr 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this display in a textarea or an input? Ideally an indented style and a textarea would probably be nice.

Copy link
Author

@READ10 READ10 Apr 7, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. Right now it does input, but I can see if I can make it do indented + textarea.

@tomchristie
Copy link
Member

tomchristie commented Apr 7, 2017

This does seem pretty valid, yup.
Good call on making sure this works okay together the binary argument, @xordoquy. That needs confirmation before we press on with this.

@READ10
Copy link
Author

READ10 commented Apr 13, 2017

I'm going to be out of the office until Apr. 22, and I've been trying to wrap some other stuff up before I go, so this has gotten pushed down the priority queue. I'll take it up again when I get back.

Dave Allan added 3 commits Apr 26, 2017
Per issue #4999, JSONFields are not rendered properly in the DRF
browsable API HTML forms.  This patch attempts to fix that behavior by
introducing a JSONBoundField helper similar to the NestedBoundField
helper.
@READ10
Copy link
Author

READ10 commented Apr 26, 2017

I've updated my branch, I think addressing everybody's feedback so far. The updated code should play nicely with binary=True, although additional testing from someone who uses binary would be helpful. I've also changed the form to use a text area and indent the JSON.

@tomchristie
Copy link
Member

tomchristie commented Apr 27, 2017

Nice improvement that, thanks!

@tomchristie tomchristie merged commit 80d0ee5 into encode:master Apr 27, 2017
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