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

Add Django form mutations #217

Merged
merged 14 commits into from Jun 5, 2018

Conversation

grantmcconnaughey
Copy link

@grantmcconnaughey grantmcconnaughey commented Jul 18, 2017

This PR adds support for mutations based on Django forms. It's pretty similar to the serializer mutations over in #186. I based most of the changes off of that PR so the code should look pretty similar after being merged.

Here's how to use them:

class MyForm(forms.Form):
    name = forms.CharField()

class MyMutation(FormMutation):
    class Meta:
        form_class = MyForm

The mutation will have one argument called 'input'. That input will expect a name.

There is also a mutation that supports ModelForms. This mutation will look up the DjangoObjectType for the forms model and return it after the form saves.

class Pet(models.Model):
    name = models.CharField()

class PetForm(forms.ModelForm):
    class Meta:
        model = Pet
        fields = ('name',)

class PetMutation(ModelFormMutation):
    class Meta:
        form_class = PetForm

To-Do

  • Documentation

@coveralls
Copy link

coveralls commented Jul 18, 2017

Coverage Status

Coverage decreased (-0.9%) to 91.852% when pulling 257c69b on grantmcconnaughey:form_mutations into 0588f89 on graphql-python:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.9%) to 91.852% when pulling 257c69b on grantmcconnaughey:form_mutations into 0588f89 on graphql-python:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.9%) to 91.852% when pulling 257c69b on grantmcconnaughey:form_mutations into 0588f89 on graphql-python:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.9%) to 91.852% when pulling 257c69b on grantmcconnaughey:form_mutations into 0588f89 on graphql-python:master.

@coveralls
Copy link

coveralls commented Jul 18, 2017

Coverage Status

Coverage increased (+0.2%) to 92.91% when pulling 37ec06d on grantmcconnaughey:form_mutations into 0588f89 on graphql-python:master.

@grantmcconnaughey grantmcconnaughey changed the title WIP - Add Django form-based mutations Add Django form-based mutations Jul 18, 2017
@grantmcconnaughey grantmcconnaughey changed the title Add Django form-based mutations Add Django form mutations Jul 18, 2017
@coveralls
Copy link

coveralls commented Jul 18, 2017

Coverage Status

Coverage increased (+0.1%) to 92.819% when pulling 6c5771e on grantmcconnaughey:form_mutations into 0588f89 on graphql-python:master.

@patrick91
Copy link
Member

That looks good! @syrusakbary do you have time to check this? :)

@coveralls
Copy link

coveralls commented Jul 24, 2017

Coverage Status

Coverage increased (+0.1%) to 92.481% when pulling 6cde395 on grantmcconnaughey:form_mutations into cec1a84 on graphql-python:master.

@spockNinja
Copy link
Contributor

This change looks great.

There was some work towards translating form fields already done in form_converter.py. It looks like it is only currently being used for filter fields.

I think I like having it in /forms/converter though. So how about we drop the old file, make sure it didn't have anything the new one doesn't, and fix the filter-fields to import from this file.

@coveralls
Copy link

coveralls commented Oct 2, 2017

Coverage Status

Coverage decreased (-0.3%) to 92.681% when pulling 463ce68 on grantmcconnaughey:form_mutations into f35e445 on graphql-python:master.

@coveralls
Copy link

coveralls commented Oct 2, 2017

Coverage Status

Coverage increased (+0.1%) to 93.13% when pulling bf7ad7e on grantmcconnaughey:form_mutations into f35e445 on graphql-python:master.

@coveralls
Copy link

coveralls commented Oct 26, 2017

Coverage Status

Coverage increased (+0.1%) to 93.13% when pulling bf7ad7e on grantmcconnaughey:form_mutations into f35e445 on graphql-python:master.

@grantmcconnaughey
Copy link
Author

grantmcconnaughey commented Oct 26, 2017

I just re-ran the build on this PR. I believe it is compatible with Graphene/Graphene-Django 2.0.

@syrusakbary
Copy link
Member

I'm loving this PR. I need to take some time to review it.
I think might be better to use names like DjangoFormMutation that doesn't collide with other libraries.

Also might be useful to have an utility DjangoFormInputObjectType (sorry if too verbose :P), that permits the mapping from Django Forms to InputObjectType, like:

class MyInput(DjangoFormInputObjectType):
    class Meta:
        form = MyForm

Which is equivalent to write by hand:

class MyInput(InputObjectType):
    field1 = ...
    field2 = ...

Thoughts?

@grantmcconnaughey
Copy link
Author

grantmcconnaughey commented Nov 16, 2017

Thanks, Syrus! I like the idea of creating an InputObjectType each time. It's a little more verbose, but it will allow users to name it whatever they want and easily add new fields that aren't already on the form. I think removing a bit of the magic here is a good thing.

I'll whip up those changes for this PR.


@classmethod
def mutate_and_get_payload(cls, root, info, **input):
form = cls._meta.form_class(data=input)
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on some of the other requests coming in on the SerializerMutation stuff, I have a few suggestions here for extensibility.

  1. Add a get_form classmethod that can be overridden to do any custom form logic.
  2. Add a get_form_kwargs classmethod for a simpler override use case where you just want to send some extra kwargs to the form class.

@coveralls
Copy link

coveralls commented Nov 29, 2017

Coverage Status

Coverage increased (+0.3%) to 93.346% when pulling 6d7a0d0 on grantmcconnaughey:form_mutations into f35e445 on graphql-python:master.

@grantmcconnaughey
Copy link
Author

Hey guys, I just pushed some changes to this PR. They include...

  • Added Django prefix to mutation class names
  • The ability to override get_form and get_form_kwargs
  • Improved default return_field_name for DjangoModelFormMutation. It defaults to the camel-cased model name. So User defaults to 'user' and MyOtherModel defaults to myOtherModel
  • Handle updates with model forms. I've added an id field. If that's passed into a DjangoModelFormMutation then we'll grab the object and pass into the instance kwarg. Example

The last thing I haven't added is the DjangoFormInputObjectType. I'm inheriting from ClientIDMutation which handles creating the Input type itself. Perhaps I shouldn't be inheriting from ClientIDMutation? It looks like that might be relay-specific. What do you two think?

@coveralls
Copy link

coveralls commented Nov 29, 2017

Coverage Status

Coverage increased (+0.3%) to 93.346% when pulling c3938d1 on grantmcconnaughey:form_mutations into f35e445 on graphql-python:master.

@spockNinja
Copy link
Contributor

This PR is looking fantastic.

I think you're right, we probably shouldn't inherit from ClientIDMutation when this functionality isn't relay specific. I think going down the path that Syrus suggested with the DjangoFormInputObjectType is a solid approach here.

Also note that I believe in 2.0, Input now goes under Arguments. (Input was kept around in ClientIDMutation, but the base Mutation expects Arguments) So we might call it DjangoFormArgumentsType?

@Eraldo
Copy link

Eraldo commented Dec 9, 2017

Awesome.. this looks very promising. :)
@spockNinja "DjangoFormArgumentsType" sounds clear to me.

@maarcingebala
Copy link

Hey @grantmcconnaughey, your work on this PR is great, I'd love to have it in graphene-django. I think that this feature is essential in any larger Django project that aims at having comprehensive API while keeping the codebase clean and DRY.

@caffodian
Copy link

Thanks, this looks awesome and full of great ideas. Is there any chance of work being done on this to bring it up into 2.0?

Copy link

@fw-developer fw-developer left a comment

Choose a reason for hiding this comment

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

this is look great!
form will make mutate easy way!

@syrusakbary
Copy link
Member

I want to merge this PR but I want to do some changes before it, such as:

  • Resolve conflicts
  • Refactor the DjangoFormMutation into DjangoFormInputObjectType (so it's reusable across different mutations)

I will open another PR adding the changes on top of this PR :)

@syrusakbary syrusakbary mentioned this pull request Jun 5, 2018
@syrusakbary
Copy link
Member

The refactor from DjangoFormMutation into DjangoFormInputObjectType was not as easy as I thought.

As I want to make a release but not block this feature, I will merge #450 (once tests pass), while making clear in the docs that the API is experimental, so people can start trying it.

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

Successfully merging this pull request may close these issues.

None yet

9 participants