Skip to content
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

Fix issue when using Safari to authenticate #46

Merged
merged 1 commit into from
Jun 14, 2019

Conversation

VitaliyAdamkov
Copy link
Contributor

@AlessioRocco
Copy link
Contributor

AlessioRocco commented Jun 7, 2019

Just a question, why you chose r: rand instead of t: Time.now.to_i?

@VitaliyAdamkov
Copy link
Contributor Author

Just a question, why you chose r: rand instead of t: Time.now.to_i?

I was not choosing, just shared other man's approach, because it succeeded :)

@AlessioRocco
Copy link
Contributor

@VitaliyAdamkov can you please rebase on master and remove this commit 05d3a3a?

@VitaliyAdamkov
Copy link
Contributor Author

@VitaliyAdamkov can you please rebase on master and remove this commit 05d3a3a?

done

spree.send("spree_user_#{method.provider}_omniauth_authorize_path"),
title: I18n.t('spree.sign_in_with', provider: method.provider)) if method.active %>
spree.send("spree_user_#{method.provider}_omniauth_authorize_path", r: rand),
title: Spree.t(:sign_in_with, provider: method.provider)) if method.active %>
Copy link
Member

Choose a reason for hiding this comment

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

@VitaliyAdamkov I like this change, removing I18n from views is an improvement 👍
But Spree.t has been deprecated in Solidus (see issue solidusio/solidus/issues/2773) and we're trying to remove it from the extensions as well. Can you please change this to something like t('spree.sign_in_with' ... ?

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

Copy link
Member

@spaghetticode spaghetticode Jun 14, 2019

Choose a reason for hiding this comment

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

@VitaliyAdamkov thank you 👍.
I have one last request, can you please squash the two commits into a single one? I think this would make git history cleaner and tidier... thank you!

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

Copy link
Member

@spaghetticode spaghetticode left a comment

Choose a reason for hiding this comment

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

👍 thank you @VitaliyAdamkov

@AlessioRocco AlessioRocco merged commit 9b72d22 into solidusio-contrib:master Jun 14, 2019
@AlessioRocco
Copy link
Contributor

Merged 🎉 thanks @VitaliyAdamkov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants