Skip to content

Master social login#953

Merged
daronco merged 11 commits intomasterfrom
master-social-login
Oct 24, 2017
Merged

Master social login#953
daronco merged 11 commits intomasterfrom
master-social-login

Conversation

@RafaFP
Copy link
Contributor

@RafaFP RafaFP commented Oct 17, 2017

Adding Facebook and Google authentication Login to Mconf login

TODO: add to signin options and add facebook
      also test it

Conflicts:
	app/assets/stylesheets/app/registrations/new.scss
	app/views/registrations/_signup_form.html.haml
	app/views/registrations/new.html.haml
	config/locales/en/mconf_com.yml
	config/locales/pt-br/mconf_com.yml
TODO: finish it (still need the apitokens and routes) and create tests
Social login is disabled if key or secret are missing for authentication methods
This prevents a lot of errors on sign_in due to redirecting to :back
@RafaFP RafaFP requested a review from daronco October 17, 2017 19:53
@RafaFP RafaFP self-assigned this Oct 17, 2017
# otherwise AhoyEmail will change links and break tests
MCONF_EMAIL_TRACK_CLICKED=false
MCONF_EMAIL_TRACK_OPENED=true

Copy link
Member

Choose a reason for hiding this comment

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

Do we really need configs set for tests? If not you can just remove them from here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. We need them so that devise can initialize the omniauth

# define the creation for social omniauth
def self.from_omniauth(access_token)
data = access_token.info
user = User.where(email: data['email']).first
Copy link
Member

Choose a reason for hiding this comment

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

User.find_by(email: data['email'])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Completely forgot I could do that. Fixed.

@@ -0,0 +1,18 @@
#social-login-section
- if (Rails.application.config.omniauth_google_key.present? && Rails.application.config.omniauth_google_secret.present?) || (Rails.application.config.omniauth_facebook_key.present? && Rails.application.config.omniauth_facebook_secret.present?)
Copy link
Member

Choose a reason for hiding this comment

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

Just make mathods to encapsulate this. My suggestion is Site.current.social_login_enabled?, that returns true if at least one of the social login options is enabled, and Site.current.social_login_enabled?("google") to check for a single method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Looks much better now

.env.development Outdated
# Omniauth for google and facebook need API keys.
# More instructions can be found on https://github.com/omniauth/omniauth/wiki/List-of-Strategies
# Then add the following:
# MCONF_OMNIAUTH_GOOGLE_KEY = 'yourgooglekey'
Copy link
Member

Choose a reason for hiding this comment

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

Usually env vars are defined without using spaces around the =.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed that

register: "Create a new account"
resend_confirmation_email: "Resend confirmation email"
social_login:
social_facebook: "Login with Facebook Account"
Copy link
Member

Choose a reason for hiding this comment

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

"Log in with Facebook"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

resend_confirmation_email: "Resend confirmation email"
social_login:
social_facebook: "Login with Facebook Account"
social_google: "Login with Google Account"
Copy link
Member

Choose a reason for hiding this comment

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

"Log in with Google"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

register: "Criar uma nova conta"
resend_confirmation_email: "Reenviar e-mail de confirmação"
social_login:
social_facebook: "Acessar com a conta Facebook"
Copy link
Member

Choose a reason for hiding this comment

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

"Entrar com o Facebook"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

resend_confirmation_email: "Reenviar e-mail de confirmação"
social_login:
social_facebook: "Acessar com a conta Facebook"
social_google: "Acessar com a conta Google"
Copy link
Member

Choose a reason for hiding this comment

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

"Entrar com o Google"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

def self.from_omniauth(access_token)
data = access_token.info
user = User.where(email: data['email']).first
user = User.find_by_email(data['email'])
Copy link
Member

Choose a reason for hiding this comment

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

This is an old syntax, it's better to use find_by(email: data['email']).

@@ -1,15 +1,16 @@
#social-login-section
- if (Rails.application.config.omniauth_google_key.present? && Rails.application.config.omniauth_google_secret.present?) || (Rails.application.config.omniauth_facebook_key.present? && Rails.application.config.omniauth_facebook_secret.present?)
- if Site.current.social_login_enabled?
Copy link
Member

Choose a reason for hiding this comment

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

We have a current_site helper that caches the site, it's usually better to use it in views.

@daronco daronco merged commit 2c91d04 into master Oct 24, 2017
daronco added a commit that referenced this pull request Oct 24, 2017
@daronco daronco deleted the master-social-login branch November 24, 2017 17:53
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.

2 participants