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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add GraphQL register/login #3879

Merged
merged 17 commits into from Oct 15, 2019

Conversation

@kalanyuz
Copy link
Contributor

kalanyuz commented Aug 31, 2019

Description of what you did:

Added a GraphQL login/register query/mutation to users-permissions plugin.

My PR is a:

  • 馃挜 Breaking change
  • 馃悰 Bug fix #3922 fix #2546
  • 馃拝 Enhancement
  • 馃殌 New feature

Main update on the:

  • Admin
  • Documentation
  • Framework
  • Plugin

Manual testing done on the following databases:

  • Not applicable
  • MongoDB
  • MySQL
  • Postgres
  • SQLite
kalanyuz added 2 commits Aug 31, 2019
@kalanyuz kalanyuz changed the title Add GraphQL login Add GraphQL register/login Aug 31, 2019
@derrickmehaffy

This comment has been minimized.

Copy link
Contributor

derrickmehaffy commented Aug 31, 2019

@kalanyuz Can you please add End2End tests and also add documentation please.

@kalanyuz

This comment has been minimized.

Copy link
Contributor Author

kalanyuz commented Sep 1, 2019

@kalanyuz Can you please add End2End tests and also add documentation please.

@derrickmehaffy Added a test per request. However I noticed that there isn't a documentation for graphql usage for this plugin. Isn't it because of its self-documentation aspect?

@alexandrebodin

This comment has been minimized.

Copy link
Collaborator

alexandrebodin commented Sep 2, 2019

Thansk for this PR. We will be testing it asap

@dhniels

This comment has been minimized.

Copy link

dhniels commented Sep 8, 2019

nice job everyone, cant wait to see this merged!

@itsmepetrov

This comment has been minimized.

Copy link

itsmepetrov commented Sep 16, 2019

Thanks for your PR @kalanyuz, but I think login should be mutation

Copy link
Collaborator

alexandrebodin left a comment

Looking good just a few changes remaining

});
});
});
});

This comment has been minimized.

Copy link
@alexandrebodin

alexandrebodin Sep 16, 2019

Collaborator

Can you please add error tests too ? thanks !

This comment has been minimized.

Copy link
@kalanyuz

kalanyuz Sep 29, 2019

Author Contributor

Which error cases would you like to test?
Since this is based on core controller functions. Would only null checking cases be enough?

This comment has been minimized.

Copy link
@kalanyuz

kalanyuz Sep 30, 2019

Author Contributor

Couldn't really create meaningful error tests because GraphQL because it either terminates internally when a variable is missing or can't return requested fields.

@alexandrebodin

This comment has been minimized.

Copy link
Collaborator

alexandrebodin commented Sep 26, 2019

Hi @kalanyuz will you be able to do the updates ?

@alexandrebodin alexandrebodin removed this from the 3.0.0-beta.17 milestone Sep 26, 2019
@kalanyuz

This comment has been minimized.

Copy link
Contributor Author

kalanyuz commented Sep 29, 2019

@alexandrebodin Yep. However my hands are a bit full right now so please wait a couple more days.

kalanyuz added 3 commits Sep 30, 2019
Copy link
Collaborator

alexandrebodin left a comment

Almost done :)

@z-ax

This comment has been minimized.

Copy link

z-ax commented Oct 1, 2019

Oh, can't wait for this! 馃挜

@kalanyuz

This comment has been minimized.

Copy link
Contributor Author

kalanyuz commented Oct 1, 2019

@alexandrebodin Removed :)

@alexandrebodin

This comment has been minimized.

Copy link
Collaborator

alexandrebodin commented Oct 1, 2019

@kalanyuz You still are getting some bugs in the tests ;)

@kalanyuz

This comment has been minimized.

Copy link
Contributor Author

kalanyuz commented Oct 1, 2019

@alexandrebodin Apparently beta .16 requires adding plugin: 'users-permissions' in the graphql file even though the file is in the same plugin directory. This behavior didn't exist in .15.

@alexandrebodin

This comment has been minimized.

Copy link
Collaborator

alexandrebodin commented Oct 1, 2019

@alexandrebodin Apparently beta .16 requires adding plugin: 'users-permissions' in the graphql file even though the file is in the same plugin directory. This behavior didn't exist in .15.

Yes this is the only way to accurately know with resolver / resolverOf to target as you could target an other pluginb resolver for example. Thanks for the update will be testing once everything is green before merging

@alexandrebodin alexandrebodin added this to the 3.0.0-beta.17 milestone Oct 4, 2019
context.request.body = _.toPlainObject(options.input);

await strapi.plugins['users-permissions'].controllers.auth.register(context);
let output = context.body.toJSON ? context.body.toJSON() : context.body;

This comment has been minimized.

Copy link
@alexandrebodin

alexandrebodin Oct 4, 2019

Collaborator

You need to check if the output is an error and throw in that case. same in login

This comment has been minimized.

Copy link
@kalanyuz

kalanyuz Oct 9, 2019

Author Contributor

@alexandrebodin Isn't this kind of redundant? If it has errors from Auth controller this will return the error from that controller correctly with messages and error codes (confirmed this myself). These resolvers were intended only for relaying Auth controller's output so isn't dissecting the error output to throw the same thing out repetitive?

This comment has been minimized.

Copy link
@Kriskator

Kriskator Oct 9, 2019

I just added this code to my project in extensions/user-permissions/config/schema-graphql and it works very fine, I hope this will be part of the core soon.
I can confirm the missing error output as described. What I'm wondering is, why there will be no error thrown at line 185 (await strapi...). I compared it to updateUser mutation and when there is an error (e.g. not existing id) it already throws here:

await strapi.plugins['users-permissions'].controllers.user.update(context);

with "message": "entry.notFound" error response.
So no need to check context.body for an error. I think its best to keep it that way to stay in line. But right now I don't see where to go in order to achieve similar behaviour.

This comment has been minimized.

Copy link
@Kriskator

Kriskator Oct 9, 2019

Ok, difference is that e.g. updateUser throws an error by bookshelf (if used) when there is an invalid database query like the id doesn't exist, whereas strapi returns or sets ctx.badRequest() (or other errors, see @hapi/boom docs) which will not throw an error but return an HTTP-friendly error object. So GraphQL resolvers have to handle it by themselves?
It would be great to hook it somewhere and convert it to an ApolloError somehow...

This comment has been minimized.

Copy link
@alexandrebodin

alexandrebodin Oct 14, 2019

Collaborator

As saif by Kriskator, when using register or login the controller might return a badRequest error this will not be caught but will set the ctx.body to this error. this is why you need to check for it so you can throw an ApolloError

This comment has been minimized.

Copy link
@kalanyuz

kalanyuz Oct 15, 2019

Author Contributor

@alexandrebodin Would this be viable?

This comment has been minimized.

Copy link
@Kriskator

Kriskator Oct 15, 2019

ApolloErrors aren't used in whole project.
I did it this way:

if (output.isBoom) {
  throw new Error(output.data[0].messages[0].message);
}

This comment has been minimized.

Copy link
@Kriskator

Kriskator Oct 15, 2019

Oh, just saw the update, so now you can throw an ApolloError :)

This comment has been minimized.

Copy link
@alexandrebodin

alexandrebodin Oct 15, 2019

Collaborator

If you can just put this util in a function at the top of the file ;) thanks !

This comment has been minimized.

Copy link
@kalanyuz

kalanyuz Oct 15, 2019

Author Contributor

@alexandrebodin here you go :)

@alexandrebodin

This comment has been minimized.

Copy link
Collaborator

alexandrebodin commented Oct 8, 2019

@kalanyuz Hopin you can finish the work you are so close ! :D

alexandrebodin and others added 3 commits Oct 14, 2019
context.request.body = _.toPlainObject(options.input);

await strapi.plugins['users-permissions'].controllers.auth.register(context);
let output = context.body.toJSON ? context.body.toJSON() : context.body;

This comment has been minimized.

Copy link
@alexandrebodin

alexandrebodin Oct 15, 2019

Collaborator

If you can just put this util in a function at the top of the file ;) thanks !

kalanyuz added 2 commits Oct 15, 2019
Copy link
Collaborator

alexandrebodin left a comment

LGTM ! Great work !

@alexandrebodin alexandrebodin merged commit edbff44 into strapi:master Oct 15, 2019
1 check was pending
1 check was pending
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can鈥檛 perform that action at this time.