Skip to content
This repository has been archived by the owner on Jun 8, 2023. It is now read-only.

feat: optionally return all raw email addresses for user #4

Merged
merged 2 commits into from Feb 7, 2019

Conversation

jaredscheib
Copy link

@jaredscheib jaredscheib commented Feb 6, 2019

Adopted in part from jaredhanson#27


if (self._allRawEmails) {
profile.emails = json.map(function (email) {
var mappedEmail = Object.assign(email, { value: email.email });

Choose a reason for hiding this comment

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

How do you know which email is the primary one? Should we add type to these? See: http://www.passportjs.org/docs/profile/

Copy link
Author

Choose a reason for hiding this comment

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

here's a sample json object:

[ { primary: true,
    verified: true,
    visibility: 'private',
    value: 'bob.pineapple@gmail.com' },
  { primary: false,
    verified: true,
    visibility: null,
    value: 'pineapple@users.noreply.github.com' },
  { primary: false,
    verified: true,
    visibility: null,
    value: 'bob@pineapple.com' } ]

Choose a reason for hiding this comment

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

Ah, is this what the response looks like? [ { "email": "octocat@github.com", "verified": true, "primary": true, "visibility": "public" } ]

Choose a reason for hiding this comment

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

Cool, sounds good.

Copy link
Author

Choose a reason for hiding this comment

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

yep, exactly

Copy link
Author

Choose a reason for hiding this comment

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

my map just transforms these objects to have a value key, which seems to be a convention across passport addons

Choose a reason for hiding this comment

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

Should we add a test then?

Copy link
Author

Choose a reason for hiding this comment

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

added tests!

Copy link

@ultrabluewolf ultrabluewolf left a comment

Choose a reason for hiding this comment

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

LGTM, but it would be great to add a test as well.

Copy link

@ultrabluewolf ultrabluewolf left a comment

Choose a reason for hiding this comment

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

:shipit:

@jaredscheib jaredscheib merged commit b39119d into master Feb 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants