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

added onUserUpdate hook #9042

Closed

Conversation

@gerwinbrunner
Copy link
Contributor

@gerwinbrunner gerwinbrunner commented Aug 24, 2017

I found a missing feature documented in the code here:
https://github.com/meteor/meteor/blob/master/packages/accounts-base/accounts_server.js#L1433

So I just implemend it in the way the onCreateUser hook was done.

@apollo-cla
Copy link

@apollo-cla apollo-cla commented Aug 24, 2017

@gerwinbrunner: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@sebakerckhof
Copy link
Contributor

@sebakerckhof sebakerckhof commented Aug 25, 2017

It's weird to me that onCreateUser would be called for every created user, while onUpdateUser would only be called for users logging in via oAuth. If this is a useful feature, I'd at least give it a more descriptive name along the lines of 'onExternalLogin`

@merlinpatt
Copy link
Collaborator

@merlinpatt merlinpatt commented Aug 25, 2017

I agree that it should probably be called onExternalLogin to be more clear. But I'm not sure what benefit this provides. There isn't a way to pass options through to the user doc like you can with onCreateUser.

onCreateUser allows you to pass options in via Accounts.createUser(options). But, as far as I can tell, Meteor.loginWith<ExternalService> has no means to pass options to this new function onExternalLogin. And these should share the same features

@gerwinbrunner
Copy link
Contributor Author

@gerwinbrunner gerwinbrunner commented Aug 28, 2017

@sebakerckhof Thanks for the input... I just went with what was in the comments of the original code. I'll change the name.

@merlinpatt The purpose for this is to be able to update profile (or other data) after an OAuth login.
That information is passed by the OAuth service. An example would be to change the users country of residence if he changed that info in the original OAuth service.

Copy link
Member

@hwillson hwillson left a comment

Thanks for working on this @gerwinbrunner! I've added a few minor style comments to this review. Other than those, I think we should get some tests in place for these changes. Would you mind adding tests for this? See the accounts - updateOrCreateUserFromExternalService - Facebook test for a related example. Thanks again!

@@ -119,6 +119,22 @@ export class AccountsServer extends AccountsCommon {

this._onCreateUserHook = func;
}

This comment has been minimized.

@hwillson

hwillson Sep 5, 2017
Member

Can you adjust this so there is only one blank line before and after the onExternalLogin function?

@@ -1433,15 +1449,22 @@ Ap.updateOrCreateUserFromExternalService = function (
// We *don't* process options (eg, profile) for update, but we do replace

This comment has been minimized.

@hwillson

hwillson Sep 5, 2017
Member

This comment needs to change since we're now providing a way to process options. Please remove the XXX line, and consider updating this comment to better reflect the new functionality. Actually, you could consider just removing this comment entirely - now that we're addressing options, the code here pretty much speaks for itself.

@hwillson
Copy link
Member

@hwillson hwillson commented Sep 18, 2017

Hi @gerwinbrunner - just touching base on this. Have you had a chance to review my comments?

@gerwinbrunner
Copy link
Contributor Author

@gerwinbrunner gerwinbrunner commented Sep 18, 2017

@hwillson
Copy link
Member

@hwillson hwillson commented Oct 4, 2017

@gerwinbrunner Have you had a chance to look at this again? Let us know if you'll have some time to wrap this up. Thanks!

@hwillson hwillson self-assigned this Oct 11, 2017
@hwillson hwillson dismissed their stale review Oct 16, 2017

I've made the requested changes, so they should be reviewed by someone else.

@hwillson
Copy link
Member

@hwillson hwillson commented Oct 16, 2017

All outstanding items here should now be addressed, so this is ready for a final review.

@abernix abernix added this to the Package Patches milestone Oct 18, 2017
@benjamn
Copy link
Member

@benjamn benjamn commented Oct 19, 2017

Rebased and pushed to devel as 20f698a

@benjamn benjamn closed this Oct 19, 2017
@fgm
Copy link
Contributor

@fgm fgm commented Nov 2, 2017

AFAICS this actually closes #7213 from 06/2016, and its followup meteor/meteor-feature-requests#68

@hwillson
Copy link
Member

@hwillson hwillson commented Nov 2, 2017

Good catch - thanks @fgm!

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

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.