From ed2b217a082a3738bbc6232de8abc9e3c4ace593 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 27 Jul 2020 12:11:33 +0200 Subject: [PATCH 1/4] Remove duplication in oauth actions We were writing the same code twice, with the only difference being the text "Sign up" in the sign_up action, and "Sign in" in the sign_in action. Note we're renaming the `omniauth.info_*` I18n keys so we don't need to add new exceptions to the `ignore_unused` list, and so it's consistent with all the other keys under the `omniauth` key. --- app/views/devise/_omniauth_form.html.erb | 134 ++++++--------------- app/views/devise/sessions/new.html.erb | 2 +- app/views/users/registrations/new.html.erb | 2 +- config/locales/en/general.yml | 5 +- config/locales/es/general.yml | 5 +- 5 files changed, 48 insertions(+), 100 deletions(-) diff --git a/app/views/devise/_omniauth_form.html.erb b/app/views/devise/_omniauth_form.html.erb index 9010330c4..d820e5e4e 100644 --- a/app/views/devise/_omniauth_form.html.erb +++ b/app/views/devise/_omniauth_form.html.erb @@ -1,103 +1,49 @@ <% if feature?(:twitter_login) || feature?(:facebook_login) || feature?(:google_login) %> - - <% if current_page?(new_user_session_path) %> -
-
-
- <%= t("omniauth.info_sign_in") %> -
-
- - <% if feature? :twitter_login %> -
- <%= link_to t("omniauth.twitter.name"), user_twitter_omniauth_authorize_path, - title: t("omniauth.twitter.sign_in"), - class: "button-twitter button expanded", - method: :post %> -
- <% end %> - - <% if feature? :facebook_login %> -
- <%= link_to t("omniauth.facebook.name"), user_facebook_omniauth_authorize_path, - title: t("omniauth.facebook.sign_in"), - class: "button-facebook button expanded", - method: :post %> -
- <% end %> - - <% if feature? :google_login %> -
- <%= link_to t("omniauth.google_oauth2.name"), user_google_oauth2_omniauth_authorize_path, - title: t("omniauth.google_oauth2.sign_in"), - class: "button-google button expanded", - method: :post %> -
- <% end %> - - <% if feature? :wordpress_login %> -
- <%= link_to t("omniauth.wordpress_oauth2.name"), user_wordpress_oauth2_omniauth_authorize_path, - title: t("omniauth.wordpress_oauth2.sign_in"), - class: "button-wordpress button expanded", - method: :post %> -
- <% end %> - -
- <%= t("omniauth.or_fill") %> +
+
+
+ <%= t("omniauth.info.#{action}") %>
- <% elsif current_page?(new_user_registration_path) %> -
-
-
- <%= t("omniauth.info_sign_up") %> -
+ <% if feature? :twitter_login %> +
+ <%= link_to t("omniauth.twitter.name"), user_twitter_omniauth_authorize_path, + title: t("omniauth.twitter.#{action}"), + class: "button-twitter button expanded", + method: :post %>
+ <% end %> - <% if feature? :twitter_login %> -
- <%= link_to t("omniauth.twitter.name"), user_twitter_omniauth_authorize_path, - title: t("omniauth.twitter.sign_up"), - class: "button-twitter button expanded", - method: :post %> -
- <% end %> - - <% if feature? :facebook_login %> -
- <%= link_to t("omniauth.facebook.name"), user_facebook_omniauth_authorize_path, - title: t("omniauth.facebook.sign_up"), - class: "button-facebook button expanded", - method: :post %> -
- <% end %> - - <% if feature? :google_login %> -
- <%= link_to t("omniauth.google_oauth2.name"), user_google_oauth2_omniauth_authorize_path, - title: t("omniauth.google_oauth2.sign_up"), - class: "button-google button expanded", - method: :post %> - -
- <% end %> - - <% if feature? :wordpress_login %> -
- <%= link_to t("omniauth.wordpress_oauth2.name"), user_wordpress_oauth2_omniauth_authorize_path, - title: t("omniauth.wordpress_oauth2.sign_up"), - class: "button-wordpress button expanded", - method: :post %> -
- <% end %> - -
- <%= t("omniauth.or_fill") %> + <% if feature? :facebook_login %> +
+ <%= link_to t("omniauth.facebook.name"), user_facebook_omniauth_authorize_path, + title: t("omniauth.facebook.#{action}"), + class: "button-facebook button expanded", + method: :post %>
+ <% end %> + + <% if feature? :google_login %> +
+ <%= link_to t("omniauth.google_oauth2.name"), user_google_oauth2_omniauth_authorize_path, + title: t("omniauth.google_oauth2.#{action}"), + class: "button-google button expanded", + method: :post %> +
+ <% end %> + + <% if feature? :wordpress_login %> +
+ <%= link_to t("omniauth.wordpress_oauth2.name"), user_wordpress_oauth2_omniauth_authorize_path, + title: t("omniauth.wordpress_oauth2.#{action}"), + class: "button-wordpress button expanded", + method: :post %> +
+ <% end %> + +
+ <%= t("omniauth.or_fill") %>
- <% end %> - +
<% end %> diff --git a/app/views/devise/sessions/new.html.erb b/app/views/devise/sessions/new.html.erb index d0d219c62..85ae3d320 100644 --- a/app/views/devise/sessions/new.html.erb +++ b/app/views/devise/sessions/new.html.erb @@ -1,7 +1,7 @@ <% provide :title do %><%= t("devise_views.sessions.new.title") %><% end %>

<%= t("devise_views.sessions.new.title") %>

-<%= render "devise/omniauth_form" %> +<%= render "devise/omniauth_form", action: "sign_in" %>

<%= sanitize(t("devise_views.shared.links.signup", diff --git a/app/views/users/registrations/new.html.erb b/app/views/users/registrations/new.html.erb index f296d3b69..7dafa264f 100644 --- a/app/views/users/registrations/new.html.erb +++ b/app/views/users/registrations/new.html.erb @@ -1,7 +1,7 @@ <% provide :title do %><%= t("devise_views.users.registrations.new.title") %><% end %>

<%= t("devise_views.users.registrations.new.title") %>

-<%= render "devise/omniauth_form" %> +<%= render "devise/omniauth_form", action: "sign_up" %> <%= form_for(resource, as: resource_name, url: registration_path(resource_name)) do |f| %> <%= render "shared/errors", resource: resource %> diff --git a/config/locales/en/general.yml b/config/locales/en/general.yml index 24fb4fe6f..23dc76513 100644 --- a/config/locales/en/general.yml +++ b/config/locales/en/general.yml @@ -299,8 +299,9 @@ en: sign_in: Sign in with Twitter sign_up: Sign up with Twitter name: Twitter - info_sign_in: "Sign in with:" - info_sign_up: "Sign up with:" + info: + sign_in: "Sign in with:" + sign_up: "Sign up with:" or_fill: "Or fill the following form:" proposals: create: diff --git a/config/locales/es/general.yml b/config/locales/es/general.yml index 92de921d8..dda320025 100644 --- a/config/locales/es/general.yml +++ b/config/locales/es/general.yml @@ -299,8 +299,9 @@ es: sign_in: Entra con Twitter sign_up: Regístrate con Twitter name: Twitter - info_sign_in: "Entra con:" - info_sign_up: "Regístrate con:" + info: + sign_in: "Entra con:" + sign_up: "Regístrate con:" or_fill: "O rellena el siguiente formulario:" proposals: create: From b1c2a4a9f2fcbbb1686265666c237e70f39d8b2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 27 Jul 2020 12:29:19 +0200 Subject: [PATCH 2/4] Make it easier to add new omniauth buttons Since we're using the exact same logic for all existing buttons, we can just get the list of available ones and loop through them. --- app/helpers/settings_helper.rb | 9 ++++++ app/views/devise/_omniauth_form.html.erb | 35 +++--------------------- 2 files changed, 13 insertions(+), 31 deletions(-) diff --git a/app/helpers/settings_helper.rb b/app/helpers/settings_helper.rb index b03675c2c..7280f71f2 100644 --- a/app/helpers/settings_helper.rb +++ b/app/helpers/settings_helper.rb @@ -1,4 +1,13 @@ module SettingsHelper + def oauth_logins + [ + (:twitter if feature?(:twitter_login)), + (:facebook if feature?(:facebook_login)), + (:google_oauth2 if feature?(:google_login)), + (:wordpress_oauth2 if feature?(:wordpress_login)) + ].compact + end + def feature?(name) setting["feature.#{name}"].presence || setting["process.#{name}"].presence end diff --git a/app/views/devise/_omniauth_form.html.erb b/app/views/devise/_omniauth_form.html.erb index d820e5e4e..516e32cbb 100644 --- a/app/views/devise/_omniauth_form.html.erb +++ b/app/views/devise/_omniauth_form.html.erb @@ -6,38 +6,11 @@
- <% if feature? :twitter_login %> + <% oauth_logins.each do |login| %>
- <%= link_to t("omniauth.twitter.name"), user_twitter_omniauth_authorize_path, - title: t("omniauth.twitter.#{action}"), - class: "button-twitter button expanded", - method: :post %> -
- <% end %> - - <% if feature? :facebook_login %> -
- <%= link_to t("omniauth.facebook.name"), user_facebook_omniauth_authorize_path, - title: t("omniauth.facebook.#{action}"), - class: "button-facebook button expanded", - method: :post %> -
- <% end %> - - <% if feature? :google_login %> -
- <%= link_to t("omniauth.google_oauth2.name"), user_google_oauth2_omniauth_authorize_path, - title: t("omniauth.google_oauth2.#{action}"), - class: "button-google button expanded", - method: :post %> -
- <% end %> - - <% if feature? :wordpress_login %> -
- <%= link_to t("omniauth.wordpress_oauth2.name"), user_wordpress_oauth2_omniauth_authorize_path, - title: t("omniauth.wordpress_oauth2.#{action}"), - class: "button-wordpress button expanded", + <%= link_to t("omniauth.#{login}.name"), send("user_#{login}_omniauth_authorize_path"), + title: t("omniauth.#{login}.#{action}"), + class: "button-#{login.to_s.delete_suffix("_oauth2")} button expanded", method: :post %>
<% end %> From b7b05b55feb86281bfc9fdeacbe59e1ddd1a770e Mon Sep 17 00:00:00 2001 From: Julian Herrero Date: Sat, 25 Jul 2020 11:21:41 +0700 Subject: [PATCH 3/4] Show Wordpress login button if it's the only one enabled --- app/views/devise/_omniauth_form.html.erb | 2 +- spec/system/users_auth_spec.rb | 97 ++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 1 deletion(-) diff --git a/app/views/devise/_omniauth_form.html.erb b/app/views/devise/_omniauth_form.html.erb index 516e32cbb..8cbe25844 100644 --- a/app/views/devise/_omniauth_form.html.erb +++ b/app/views/devise/_omniauth_form.html.erb @@ -1,4 +1,4 @@ -<% if feature?(:twitter_login) || feature?(:facebook_login) || feature?(:google_login) %> +<% if oauth_logins.any? %>
diff --git a/spec/system/users_auth_spec.rb b/spec/system/users_auth_spec.rb index 4b38b80ec..63663ca7c 100644 --- a/spec/system/users_auth_spec.rb +++ b/spec/system/users_auth_spec.rb @@ -100,6 +100,103 @@ describe "Users" do end context "OAuth authentication" do + context "Form buttons" do + before do + Setting["feature.facebook_login"] = false + Setting["feature.twitter_login"] = false + Setting["feature.google_login"] = false + Setting["feature.wordpress_login"] = false + end + + scenario "No button will appear if all features are disabled" do + visit new_user_registration_path + + expect(page).not_to have_link "Twitter" + expect(page).not_to have_link "Facebook" + expect(page).not_to have_link "Google" + expect(page).not_to have_link "Wordpress" + + visit new_user_session_path + + expect(page).not_to have_link "Twitter" + expect(page).not_to have_link "Facebook" + expect(page).not_to have_link "Google" + expect(page).not_to have_link "Wordpress" + end + + scenario "Twitter login button will appear if feature is enabled" do + Setting["feature.twitter_login"] = true + + visit new_user_registration_path + + expect(page).to have_link "Twitter" + expect(page).not_to have_link "Facebook" + expect(page).not_to have_link "Google" + expect(page).not_to have_link "Wordpress" + + visit new_user_session_path + + expect(page).to have_link "Twitter" + expect(page).not_to have_link "Facebook" + expect(page).not_to have_link "Google" + expect(page).not_to have_link "Wordpress" + end + + scenario "Facebook login button will appear if feature is enabled" do + Setting["feature.facebook_login"] = true + + visit new_user_registration_path + + expect(page).not_to have_link "Twitter" + expect(page).to have_link "Facebook" + expect(page).not_to have_link "Google" + expect(page).not_to have_link "Wordpress" + + visit new_user_session_path + + expect(page).not_to have_link "Twitter" + expect(page).to have_link "Facebook" + expect(page).not_to have_link "Google" + expect(page).not_to have_link "Wordpress" + end + + scenario "Google login button will appear if feature is enabled" do + Setting["feature.google_login"] = true + + visit new_user_registration_path + + expect(page).not_to have_link "Twitter" + expect(page).not_to have_link "Facebook" + expect(page).to have_link "Google" + expect(page).not_to have_link "Wordpress" + + visit new_user_session_path + + expect(page).not_to have_link "Twitter" + expect(page).not_to have_link "Facebook" + expect(page).to have_link "Google" + expect(page).not_to have_link "Wordpress" + end + + scenario "Wordpress login button will appear if feature is enabled" do + Setting["feature.wordpress_login"] = true + + visit new_user_registration_path + + expect(page).not_to have_link "Twitter" + expect(page).not_to have_link "Facebook" + expect(page).not_to have_link "Google" + expect(page).to have_link "Wordpress" + + visit new_user_session_path + + expect(page).not_to have_link "Twitter" + expect(page).not_to have_link "Facebook" + expect(page).not_to have_link "Google" + expect(page).to have_link "Wordpress" + end + end + context "Twitter" do let(:twitter_hash) { { provider: "twitter", uid: "12345", info: { name: "manuela" }} } let(:twitter_hash_with_email) { { provider: "twitter", uid: "12345", info: { name: "manuela", email: "manuelacarmena@example.com" }} } From 1b211716025154b8684d83ee9fd7004a0f8321a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 27 Jul 2020 13:50:50 +0200 Subject: [PATCH 4/4] Remove duplication in CSS for omniauth buttons We were adding the same styles five times. --- app/assets/stylesheets/layout.scss | 45 +++++++----------------------- 1 file changed, 10 insertions(+), 35 deletions(-) diff --git a/app/assets/stylesheets/layout.scss b/app/assets/stylesheets/layout.scss index 4177b9d3d..6dbf15537 100644 --- a/app/assets/stylesheets/layout.scss +++ b/app/assets/stylesheets/layout.scss @@ -1548,6 +1548,16 @@ table { line-height: $line-height * 2; padding: 0; position: relative; + + &::before { + font-family: "icons" !important; + font-size: rem-calc(24); + left: 0; + line-height: $line-height * 2; + padding: 0 rem-calc(20); + position: absolute; + top: 0; + } } .button.button-twitter { @@ -1557,13 +1567,6 @@ table { &::before { color: #45b0e3; content: "f"; - font-family: "icons" !important; - font-size: rem-calc(24); - left: 0; - line-height: $line-height * 2; - padding: 0 rem-calc(20); - position: absolute; - top: 0; } } @@ -1600,13 +1603,6 @@ table { &::before { color: #3b5998; content: "A"; - font-family: "icons" !important; - font-size: rem-calc(24); - left: 0; - line-height: $line-height * 2; - padding: 0 rem-calc(20); - position: absolute; - top: 0; } } @@ -1643,13 +1639,6 @@ table { &::before { color: #de4c34; content: "B"; - font-family: "icons" !important; - font-size: rem-calc(24); - left: 0; - line-height: $line-height * 2; - padding: 0 rem-calc(20); - position: absolute; - top: 0; } } @@ -1660,13 +1649,6 @@ table { &::before { color: #2f2f33; content: "J"; - font-family: "icons" !important; - font-size: rem-calc(24); - left: 0; - line-height: $line-height * 2; - padding: 0 rem-calc(20); - position: absolute; - top: 0; } } @@ -1677,13 +1659,6 @@ table { &::before { color: #08c; content: "1"; - font-family: "icons" !important; - font-size: rem-calc(24); - left: 0; - line-height: $line-height * 2; - padding: 0 rem-calc(20); - position: absolute; - top: 0; } }