-
Notifications
You must be signed in to change notification settings - Fork 33
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
Extract images and team domain from identity only auth #33
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
I'll leave it open to give @ueberauth/developers a chance to look at it.
@ueberauth/developers do we bump versions in this case?
@tajchumber I'm still fairly new to elixir, are my changes idiomatic? |
@ibash don't worry about it :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for getting involved @ibash and submitting these improvements, some quick feedback 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks very good, I have one change request and one question.
For now the code looks either in the user
or identity
to find image_urls
and team_image_urls
, I am not fully into the slack API, but would it be possible that some image_urls
would be in the user
and some other image_urls
would be in the identity
and with this solution we would only parse some of them?
@Hanspagh I don't think so, because the |
When using sign in with slack the team domain and image urls are provided. This pulls them out into info and credentials.
@Hanspagh @doomspork updated! |
Looks good to me @doomspork ? |
When using sign in with slack the team domain and image urls are provided. This pulls them out into info and credentials.