[UNRESOLVED] Refresh user providerData when the user log again

[junit38]

Refresh the user providers's data or the users additional providers's data
when he log again with a social account to maintin the accuracy of the data.

Fixes #511

[junit38]

I've done the PR again to match wanted conditions.

[codydaig]

@junit38 Be sure that the tests pass. You can run grunt test locally as well. You've got spacing errors being caught. https://travis-ci.org/meanjs/mean/jobs/92890473

[mleanos]

@junit38 Any thoughts on this comment, from the other PR?

#1062 (comment)

[junit38]

The test pass locally but I could'nt understand why the error occur is the last travis test. Any idea ?

Maybe it's because this is not the last version of the repository.
Is the test base on the last release ?

[codydaig]

@junit38 The tests run the code that you currently have in that branch. You can rebase your branch and push and it will rerun the tests.

[lirantal]

@codydaig @ilanbiala guys did you notice we're not getting the coverage report anymore? or this is only because the build here failed?

[ilanbiala]

@codydaig @lirantal not sure that it stopped because I haven't noticed on other PRs, maybe it's just for this build?

[lirantal]

Let's keep an eye open.

[codydaig]

@lirantal #1091 I think it's just because the build failed, that it didn't reach the coverage report.

[lirantal]

Ok, thanks @codydaig, and @ilanbiala.

[mleanos]

@junit38 You can close, and then immediately re-open this PR to initiate the build process again.

Also, I think we should only update the provider token.

See my comments here, #1062 (comment)

[ilanbiala]

Reran the build, so just check the status.

[lirantal]

we have a coverage drop - @junit38 can you add relevant tests please?

[rhutchison]

Anyone is welcome to chime in, I have not tested this, but based on my limited knowledge, won't this potentially overwrite 1 provider with another?

If I initially register with facebook, later with another provider (lets use twitter) - facebook is allocated in user.providerData and twitter in user.additionalProvidersData. If I then login with twitter, it looks like twitter might overwrite the facebook provider data? I do not see any checks to validate the provider I am authenticating with matches the providerData allocated to user.providerData

https://github.com/meanjs/mean/pull/1070/files#diff-6586a849345903e73352845be92306cfR177

[lirantal]

Guys please mind the coverage - if there's something that can be done about it then please be sure to add relevant tests.

[mleanos]

@rhutchison You're right. This will overwrite the existing providerData field, in the use case you described; and the provider field isn't updated, so there will be a mismatch there.

This PR still needs some work. We only want to update the provider data, for the provider that is being used to login.

@junit38 Your addition takes into account when the user isn't logged in. There is already similar logic to what you will need, for when the user is already logged in. This should be a good source to base your update from.. https://github.com/meanjs/mean/blob/master/modules/users/server/controllers/users/users.authentication.server.controller.js#L181-L202

I would actually prefer that we don't update any data other than the Authentication Token coming from the provider. As a user, I wouldn't want my data automatically pulled over from my provider into my MEANJS account. Once my account is created, and I've been using it, then the data in my MEANJS account should be treated as the master data. I would like to see a feature that "Syncs Provider Data".

[mleanos]

@junit38 Are you planning on addressing the issues raised here?

[junit38]

Yes I'm on it. Did not notify the changement before.

[ilanbiala]

@junit38 can you add some sort of tests to this?

[junit38]

What kind of test are you thinking about ?

[ilanbiala]

Anything that will prevent coverage from decreasing, and anything that will make sure this functionality continues to work in the future. Coveralls currently is not working for us, so I'm working on getting that back up. In the mean time, try setting up some simple tests.

[mleanos]

I made this comment in the last PR for this fix. Anyone have thoughts on this?
#1062 (comment)

[lirantal]

@mleanos what does it mean to update just the user's token? or more specifically where in the code do you think there are issues?

@ilanbiala it'll probably be harder to add tests here because it integrates with 3rd party services, so at least let's have a few users do manual testing before we merge.

@junit38 can you squash these commits to one?

[ilanbiala]

@lirantal we actually can't even see the line coverage for this file: https://coveralls.io/builds/5033109/source?filename=..%2Fmodules%2Fusers%2Fserver%2Fcontrollers%2Fusers%2Fusers.authentication.server.controller.js. Any thoughts on how to fix this? Some of the files on Coveralls work, and some don't.

[mleanos]

@lirantal I was mistaken that the original PR for this fix updated the User profile data; rather it was only updating the providerData. Furthermore, it looks like this PR has been modified to only update the Access & Refresh token's. @junit38 Is that right?

Since I was wrong in my interpretation of what data was getting updated, I'm now think we do want to update the providerData, or additionalProvidersData, with all the data coming from the Providers response. I apologize for the confusion.

[lirantal]

OK so @junit38 care to comment on @mleanos's ?

[junit38]

Yes that's right. I can restore the previous behaviour.

[lirantal]

@junit38 any chance to get more input from you on this PR to finalize it so we can merge it please?
cc @mleanos @codydaig

[lirantal]

No reply, closing.
Please re-open if you come back at this again.