Skip to content

[NEW] Add avatar support to custom oauth client and server#9045

Closed
geekgonecrazy wants to merge 15 commits intodevelopfrom
new/add-avatar-support-custom-oauth
Closed

[NEW] Add avatar support to custom oauth client and server#9045
geekgonecrazy wants to merge 15 commits intodevelopfrom
new/add-avatar-support-custom-oauth

Conversation

@geekgonecrazy
Copy link
Contributor

@geekgonecrazy geekgonecrazy commented Dec 8, 2017

Brings in avatar from oauth as an option in profile.

Ideally... it would actually try to set it. But oauth skips the avatar set step. Once that is fixed the custom oauth will be presented as an avatar option and will auto set.

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-9045 December 8, 2017 04:00 Inactive
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-9045 December 8, 2017 05:36 Inactive
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-9045 December 8, 2017 06:21 Inactive
@geekgonecrazy
Copy link
Contributor Author

So I wanted to make logging in with oauth set avatar... but apparently logging in via a service and logging in via oauth travel down two separate paths...

Which means a lot of the logic we are applying is actually getting skipped when logging in with oauth.

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-9045 December 8, 2017 19:19 Inactive
@RocketChat RocketChat deleted a comment Dec 8, 2017
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-9045 December 8, 2017 19:31 Inactive
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-9045 December 8, 2017 20:43 Inactive
@geekgonecrazy
Copy link
Contributor Author

I'll save resolving oauth skipping normal user checks for another PR. This at least adds it as an option in avatar suggestion.

@geekgonecrazy geekgonecrazy changed the title [NEW] Add avatar support to custom oauth [NEW] Add avatar support to custom oauth client and server Dec 8, 2017
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-9045 December 12, 2017 03:38 Inactive
@rodrigok
Copy link
Member

@geekgonecrazy can you fix the conflicts?

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-9045 January 22, 2018 00:01 Inactive
@rodrigok rodrigok temporarily deployed to rocket-chat-pr-9045 March 15, 2018 19:35 Inactive
@rodrigok rodrigok temporarily deployed to rocket-chat-pr-9045 March 15, 2018 19:36 Inactive
@rodrigok rodrigok temporarily deployed to rocket-chat-pr-9045 March 15, 2018 19:37 Inactive
@rodrigok rodrigok temporarily deployed to rocket-chat-pr-9045 March 15, 2018 19:49 Inactive
@rodrigok rodrigok temporarily deployed to rocket-chat-pr-9045 March 15, 2018 19:49 Inactive
return ['gravatar', 'facebook', 'google', 'github', 'gitlab', 'linkedIn', 'twitter']
.map((service) => {

if (suggestions.avatars) {
Copy link
Member

Choose a reason for hiding this comment

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

Why remove the list of services? That will remove the suggestions for login to get the avatar

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes it use any provider instead of limiting to just these

Copy link
Member

Choose a reason for hiding this comment

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

But them you remove the suggestions for login, is that expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah... I see what you mean. So this will get list of suggestions as well as show for them to link their account.

So best way to handle is probably to add in the suggestions list to return even those that aren't already linked.

I'll take another look to see if can make that happen.

Copy link
Contributor

Choose a reason for hiding this comment

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

@geekgonecrazy can you take another look at fixing this?

@rodrigok rodrigok added this to the 0.63.0 milestone Mar 15, 2018
return ['gravatar', 'facebook', 'google', 'github', 'gitlab', 'linkedIn', 'twitter']
.map((service) => {

if (suggestions.avatars) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@geekgonecrazy can you take another look at fixing this?

@mjovanovic0
Copy link
Contributor

Not related to this issue but similar. Maybe support PR that I create long ago for custom avatar sources? #7929

@rodrigok rodrigok modified the milestones: 0.64.0, 0.66.0 May 22, 2018
@geekgonecrazy
Copy link
Contributor Author

Not sure how to handle this or I'd finish.

@theorenck theorenck modified the milestones: 0.66.0, Short-term Jul 31, 2018
@geekgonecrazy geekgonecrazy deleted the new/add-avatar-support-custom-oauth branch April 13, 2019 04:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

Comments