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 for #6578 Breaks Carts for New Customers (Reopen #290) #306

Open
fredericboivin opened this issue Oct 20, 2015 · 10 comments
Open

Fix for #6578 Breaks Carts for New Customers (Reopen #290) #306

fredericboivin opened this issue Oct 20, 2015 · 10 comments

Comments

@fredericboivin
Copy link

Previous issue: #290

The concerned issue is closed and I'm not sure if anyone is watching so I'm opening a new one to be sure :

Problem :

Cart are not merged if when a user logs in. Previous guest_cart is not merged with the signed_in cart and the the guest_cart is in limbo since it was not merged and the guest_token is destroyed on log out so you can't access it again

Steps :

  1. Log in
  2. Add item update README to note installing and running migrations #2 to cart
  3. Log out
  4. Add item Improvements to the user/show page. My account index. #1 to cart as a guest
  5. Log in

Expected results :
Cart contain Item #1 and Item #2 since the carts should be merged

Results :

Guest cart is discarded so you only have Item #2 in your cart

Gemfile
https://gist.github.com/dangerdogz/5563861ca6a1d4d0649f

Gemfile.lock
https://gist.github.com/dangerdogz/bf19cf07bf569360afd8

Regression introduced in commit 336b0e4

Suggested solution :

The offending commit intend to fix the following problem :

  1. Checkout as a guest
  2. Checkout as a logged in user

The guest user ends up in the user account because since guest_token is cleared on logout, it's not resetted after the guest checkout and the logged in user reuse the same token thus associating both order to the user

In my opinion, it's somewhat an edge case since you have to do the steps on the same computer in the correct order and the fix ends breaking an essential checkout flow (shop, log in, countinue)

Thus, I suggest that we revert the offending commit and fix the original problem in one of the following ways :

  1. Check the order status before the association so we avoid associating completed guest order from a previous session with other users (not sure about this solution)
Warden::Manager.after_set_user except: :fetch do |user, auth, opts|
  if auth.cookies.signed[:guest_token].present?
    if user.is_a?(Spree::User)
      Spree::Order.where(email: user.email, guest_token: auth.cookies.signed[:guest_token], user_id: nil).each do |order|
        order.associate_user!(user) unless order.completed?
      end
    end
  end
end
  1. We could destroy the guest_token after a successful guest checkout since the "guest session" is complete (no cart, order is done, no reason to maintain a guest session since it's only purpose is to shop and then checkout and it's useless after the process is completed)

I'll make the pull request once a solution is selected, feel free to chime in if you have a better way to fix it, I'm not that much familiar with this part of Spree

@alexandre025
Copy link

Same issue. It's really annoying when we login during the checkout process

@Wooobee
Copy link

Wooobee commented Dec 30, 2015

Same for me

@JoeStanton
Copy link

This is a major regression that's just bitten us :(

@damianlegawiec
Copy link
Member

@JoeStanton I know your pain guys - adding to our priorities list to sort this thing out

@mvz
Copy link
Contributor

mvz commented Mar 29, 2016

@damianlegawiec see wuboy0307@76e890f for a possible fix.

@mvz
Copy link
Contributor

mvz commented Mar 29, 2016

Even better: use the logic of #find_order_by_token_or_user from Spree::Core::ControllerHelpers::Order (i.e., order must be incomplete, and have same currency, token, store id).

@damianlegawiec
Copy link
Member

@mvz looks good, can you submit a PR?

@mvz
Copy link
Contributor

mvz commented Mar 29, 2016

@damianlegawiec sure.

@mvz
Copy link
Contributor

mvz commented Apr 1, 2016

@damianlegawiec done. Looks like CircleCI is having trouble building anything, but Travis seems happy 😄.

@damianlegawiec
Copy link
Member

@mvz yeah, Circle recently has some serious problems :/ Thank you!

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

No branches or pull requests

6 participants