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

Do not render spree/shared/social if no more authentication methods available for user #7

Merged
merged 1 commit into from Apr 14, 2016

Conversation

Projects
None yet
4 participants
@westonganger
Copy link
Contributor

commented Apr 8, 2016

No description provided.

@@ -3,7 +3,7 @@
<h2><%= Spree.t(:sign_in_through_one_of_these_services) %></h2>
<% end %>

<% Spree::AuthenticationMethod.available_for(@spree_user).each do |method| %>
<% if Spree::AuthenticationMethod.available_for(@spree_user).each do |method| %>

This comment has been minimized.

Copy link
@jarednorman

jarednorman Apr 11, 2016

Member

.each returns the original collection, so this condition will never be falsy.

@jarednorman

This comment has been minimized.

Copy link
Member

commented Apr 11, 2016

Perhaps I'm reading this incorrectly, but I don't see how this PR changes anything.

@tvdeyen

This comment has been minimized.

Copy link
Member

commented Apr 11, 2016

Thanks for your contribution, but Jared is right. An empty collection will still be true.

You probably want to put this in a .present? block first.

Also, please add tests.

@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2016

It is still showing the Facebook icon even though I have already used that method of authentication and exists? returns false. See the attached image for reference. I will add the test shortly.
screenshot-127 0 0 1 3000 2016-04-11 08-06-01

spree.spree_user_omniauth_authorize_url(provider: method.provider),
title: Spree.t(:sign_in_with, provider: method.provider)) if method.active %>

<% if Spree::AuthenticationMethod.available_for(spree_current_user).exists? %>

This comment has been minimized.

Copy link
@jarednorman

jarednorman Apr 12, 2016

Member

Adding an outer conditional doesn't change the behaviour as far as I can tell. If there aren't any, the loop never runs, and since this partial isn't rendered if there aren't any, it's extra not needed.

<% end %>

<% if Spree::AuthenticationMethod.available_for(spree_current_user).exists? %>

This comment has been minimized.

Copy link
@jarednorman

jarednorman Apr 12, 2016

Member

I'm fairly sure .exists? will run an additional query that won't hit the query cache when .available_for is used inside the partial. .present? or something would be preferable.

@jarednorman

This comment has been minimized.

Copy link
Member

commented Apr 12, 2016

Thanks for the contribution. Left a couple of small comments, but beyond those, this looks good to merge to me.

Do not render spree/shared/social if no more authentication methods a…
…vailable for user.

add tests, fix spree/shared/_social if condition.

fix incorrect use of @spree_user.
@westonganger

This comment has been minimized.

Copy link
Contributor Author

commented Apr 12, 2016

I cleaned this up and squashed it into one commit. I think the main problem why it was showing the icons was because it was calling @spree_user which should have been spree_current_user

@jarednorman

This comment has been minimized.

Copy link
Member

commented Apr 12, 2016

Looking good. The specs that are failing are the same as the ones on master (likely due to admin changes which I will look into shortly) so I think this is good to merge.

@jhawthorn

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2016

Thank you

@jhawthorn jhawthorn merged commit d685e84 into solidusio-contrib:master Apr 14, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.