Skip to content
This repository has been archived by the owner on Aug 30, 2021. It is now read-only.

0.4.0 - refactor_user #559

Closed
wants to merge 1 commit into from
Closed

0.4.0 - refactor_user #559

wants to merge 1 commit into from

Conversation

sielay
Copy link

@sielay sielay commented May 12, 2015

Hi. It took time, but this version IS aligned to 0.4.0 and it would be quite clever to merge/agree on it before any further changes to users. I don't remember such a nasty merge so far ;)

This changes introduce major changes:

  • there is no providerData or additionalProviderData in user objet
  • each identity including email(s) and username(s) are stored in one array, and contain own data
  • tokens are being updated on next successful login

This includes:

  • user explorer for users with admin role in /admin/users
  • updates to oauth accounts and emails

This pass grunt test for me and work in currently developer project.

This was tested against actual integration with facebook, google+, twitter, linkedin and github.

@sielay
Copy link
Author

sielay commented May 13, 2015

Added field confirmed to identity. Can be used to validate emails.

@simison
Copy link
Member

simison commented May 13, 2015

You should use tabs instead of spaces for indentation (yeah..). Consider installing http://editorconfig.org/

@sielay
Copy link
Author

sielay commented May 14, 2015

@simison is that agreed standard for the project? (how?!)

@simison
Copy link
Member

simison commented May 14, 2015

Yeah, if you look at the surrounding code, it's all tabs.

https://github.com/meanjs/mean/blob/master/.editorconfig#L11

@sielay
Copy link
Author

sielay commented May 14, 2015

Blah

Ok. Will align and update PR.

@simison
Copy link
Member

simison commented May 14, 2015

Atom has this nice little feature.

@ilanbiala
Copy link
Member

I'll take a look once the spacing is fixed.

@ilanbiala ilanbiala added this to the 0.4.0 milestone May 15, 2015
@sielay sielay force-pushed the v0.4.0 branch 2 times, most recently from f1b8047 to 1417925 Compare May 18, 2015 21:31
@sielay
Copy link
Author

sielay commented May 18, 2015

Tabs added

@sielay
Copy link
Author

sielay commented May 19, 2015

Ok, updated. Didn't realised formatter made new conflict ;) Hope you are not against few spaces here and there. Don't like cluttered code.

@@ -5,9 +5,10 @@
*/
var passport = require('passport'),
FacebookStrategy = require('passport-facebook').Strategy,
users = require('../../controllers/users.server.controller');
User = require('mongoose').model('User'),
users = require('./../../controllers/users.server.controller');
Copy link
Member

Choose a reason for hiding this comment

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

the ./ isn't necessary here

Copy link
Member

Choose a reason for hiding this comment

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

What do you think of

path = require('path'),
users = require(path.resolve('./modules/users/server/controllers/users.server.controller')),

?

I find it always hard to try track down those .../../.....'s. ;-)

Copy link
Member

Choose a reason for hiding this comment

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

It's pretty clear for me, but adding a resolve doesn't make much sense when it's just a one time thing and you can check the file tree in Sublime/Atom etc. to verify it.

@ilanbiala
Copy link
Member

Left some comments, looks good mostly.

@sielay
Copy link
Author

sielay commented May 21, 2015

@ilanbiala cheers, will review over weekend

@sielay
Copy link
Author

sielay commented May 27, 2015

@ilanbiala Updated and left few comments. PS you always learn something new - like reverse while ;)

@ilanbiala
Copy link
Member

@lirantal do you think 0.4.0 can be merged in? Most of the remaining issues in the 0.4.0 milestone are documentation and also not necessary for initial release, so I think now is a good time.

@mleanos
Copy link
Member

mleanos commented Jun 30, 2015

@lirantal Thanks for responding so quickly. I got side-tracked yesterday.

I would love to help merge 0.4 into master. Is there anything in particular that I can work on? This will be my first contribution, so some direction might be in order.

On a side note, I pulled the most recent 0.4.0 branch down locally a few hours ago, and I'm having some significant issues. I'm about to create a couple issues to track them.

All in all, I'm very happy with the 0.4.0 branch. You all have done a wonderful job with this project. Thanks much!

@sielay
Copy link
Author

sielay commented Jun 30, 2015

@mleanos tell me, if you have problems with that PR. This week I will be releasing my project basing on it with very high test coverage (it will be under CPAL license, but with option for MIT merge backs to this particular project).

@lirantal
Copy link
Member

@ilanbiala this PR is making changes across 30+ files and is in the very core of user handling so I would definitely think it's safer to just merge 0.4 into master and then merge this PR in.

@mleanos I'll take a look at the state of 0.4 now and we'll see how to co-ordinate the effort.

@mleanos
Copy link
Member

mleanos commented Jun 30, 2015

@lirantal I agree this should be merged after the master branch has been merged with 0.4. Makes most sense.

Is there a conversation that's driving the coordination of getting master merged with 0.4?

@lirantal
Copy link
Member

@mleanos let's take this discussion in #402 further to not hi-jack this PR thread

@lirantal
Copy link
Member

@sielay we can't auto merge it due to conflicts, can you look what's going on there and updated this PR so it's ready for a merge?

@lirantal lirantal mentioned this pull request Jul 20, 2015
14 tasks
@rhutchison
Copy link
Contributor

@lirantal @ilanbiala @sielay I do not like that the username provider is under the identities object. A user should only have 1 username - why can't this be left outside of the identities?

Additionally, you have no support for primary email address. I think this is important for "forgot" emails. Right now, it looks like you just select the first e-mail in the list.

@ilanbiala
Copy link
Member

@rhutchison what new structure are you thinking of? I'm open to improvements.

@rhutchison
Copy link
Contributor

@ilanbiala

  • user.username should be left as-is
  • user.emails should be an array of email objects
    • email should be:
      • email address
      • isPrimary (this could be eliminated if we keep the primary email as user.primaryEmail)
      • anything that is needed for "confirmed email"
  • user.identities should be user.providers and only contain third-party providers

@rhutchison
Copy link
Contributor

Also, this PR has an admin feature. We need to have a decision on #615 (comment) before we proceed with a soltuion.

@lirantal
Copy link
Member

@rhutchison sounds reasonable though I admit I did not review this PR properly.
@ilanbiala will you take it with @rhutchison and @sielay to resolve what we want to do and continue with the correct PRs?

@ilanbiala
Copy link
Member

Yes, but @rhutchison are we waiting for #615 or can we proceed without it?

@rhutchison
Copy link
Contributor

@ilanbiala @lirantal if @sielay can pull out his admin code and fix the user model / supporting methods then this could be merged ahead of #615.

@sielay please let us know your thoughts and if you would be able to make the necessary updates.

@sielay
Copy link
Author

sielay commented Jul 23, 2015

@rhutchison what is difference between email and for instance twitter id? It's identity. In most of ACL systems you will use schema to differ them. You can use helpers to filter them. Blocking multiple usernames has sense.

@lirantal lirantal mentioned this pull request Jul 24, 2015
@ilanbiala
Copy link
Member

@rhutchison @sielay can we resolve this PR or will it need to be part of 0.5.0?

@ilanbiala
Copy link
Member

@lirantal @sielay @rhutchison judging by how much room for improvement and discussion there still is in this PR, I suggest we push it off to 0.5.0 and we'll release 0.5.0 whenever this is resolved. If it happens in a week, great, if not then 0.5.0 will not be too far off anyway, but this will probably hold up 0.4.0.

@lirantal lirantal modified the milestones: 0.5.0, 0.4.0 Jul 29, 2015
@lirantal
Copy link
Member

I agree, updated the issue.

@lirantal lirantal mentioned this pull request Jul 29, 2015
8 tasks
@sielay
Copy link
Author

sielay commented Aug 3, 2015

Can you give timeline for 0.5.0? Honestly I am so covered by work, that I am not able to rebase it soon.

@lirantal
Copy link
Member

lirantal commented Aug 3, 2015

I don't have any timeline for 0.5.0, sorry, but maybe you'll have some time in the future.

@lirantal
Copy link
Member

lirantal commented Aug 3, 2015

Closing this PR as for now we're unable to get there.

@lirantal lirantal closed this Aug 3, 2015
@lirantal lirantal self-assigned this Aug 3, 2015
@mleanos mleanos modified the milestones: Backlog, 0.5.0 Oct 2, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants