Fix to not reuse spree_current_user as @user
#131
Conversation
yono
changed the title
@user@user
|
@yono Sorry we haven't had a chance to look at this, would you like to reopen it? |
|
Thanks! |
|
@yono 👏 thank you for this PR! Can you add a spec that verifies this fix? That would be great. |
|
@spaghetticode Thank you for your review! I added a spec and rearranged specs. |
|
@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! |
|
@spaghetticode I added body of commit message for 3353d58 and fad9001 |
|
@yono This looks good to me. One last thing, some changes have occurred in master, could you rebase with master? |
Although `@user` and `spree_current_user` refer to the same record on the DB, they must be kept two different Ruby objects in order to avoid annoyances when the user update process fails. For example, consider a custom user edit page that shows both the user email in the header and the edit form allows email customization. When submitting the form with an invalid email, this email will be redisplayed both in the header and in the form.
When updating the user information, after `@user.save` succeeds, `spree_current_user` information may be stale. For this reason the object must be reloaded in order to get the updates from the DB.
Original implementation
@user ||= spree_current_usercontaminatesspree_current_userwhen failed to save@user.It affects customized view like display
spree_current_user.email.So I fix to load new
Spree::Userinstance.