search

solidusio / solidus_auth_devise

🔑 Devise authentication for your Solidus store.
http://solidus.io
BSD 3-Clause "New" or "Revised" License
52 stars 124 forks source link

Fix to not reuse spree_current_user as `@user` #131

Closed yono closed 5 years ago

yono commented 5 years ago

Original implementation @user ||= spree_current_user contaminates spree_current_user when failed to save @user.

It affects customized view like display spree_current_user.email.

So I fix to load new Spree::User instance.

jacobherrington commented 5 years ago

@yono Sorry we haven't had a chance to look at this, would you like to reopen it?

yono commented 5 years ago

@jacobherrington

Thanks! I'm sorry that my English isn't good. Please let me know if you have any questions.

spaghetticode commented 5 years ago

@yono 👏 thank you for this PR! Can you add a spec that verifies this fix? That would be great.

yono commented 5 years ago

@spaghetticode Thank you for your review! I added a spec and rearranged specs.

spaghetticode commented 5 years ago

@yono thank you for adding specs, I think that the code of this PR is now complete 🎉

One last thing, I think that commit messages for 3a420d and 2ddf9a should be improved. I think it's a good practice to explain why changes happen, otherwise future readers may struggle to find the reasoning behind these changes.

You can check our contributing guidelines for more details, there are a few links on how to write great commit messages.

Again, thank you for your time!

yono commented 5 years ago

@spaghetticode I added body of commit message for https://github.com/solidusio/solidus_auth_devise/pull/131/commits/3353d58a4bc6cb4c7f32424f0322bd2c3a4c03ce and https://github.com/solidusio/solidus_auth_devise/pull/131/commits/fad9001f10f409a09432768281a6041feac9f35a

jacobherrington commented 5 years ago

@yono This looks good to me. One last thing, some changes have occurred in master, could you rebase with master?