Closed (fixed)
Project:
Commerce Core
Version:
7.x-1.x-dev
Component:
Cart
Priority:
Normal
Category:
Feature request
Assigned:
Issue tags:
Reporter:
Created:
13 May 2010 at 03:19 UTC
Updated:
10 Dec 2014 at 12:00 UTC
Jump to comment: Most recent
Comments
Comment #1
alpritt commentedComment #2
alpritt commentedI got over excited about using github for the first time and pushed my changes before it was really working!
The code that I committed is probably fine, but there is a static caching issue that's causing multiple orders to get created. Basically
commerce_cart_product_add()doesn't like being called more than once per page view unless an order already exists.I'm not quite sure where the best place to reset the cache is at the moment. Also calling
commerce_cart_orders_reset()isn't enough, sincecommerce_cart_order_load()collects its order fromcommerce_order_load()which has its own entity cache.There seems to be a lot of flexibility for where to clear these caches, but I'm not sure on the best design decision.
Comment #3
rszrama commentedHey Alan, thanks for jumping in on this. : )
After reviewing your code, I realized there's going to be a simpler way. In reality, all you really need to do is update the $order->customer_uid to match the logged in user's uid. This will convert it to an authenticated shopping cart, and you won't have to worry with creating a new order / deleting the old one. One of the advantages of the new system of handling shopping carts over Ubercart. : )
I did go ahead and commit your documentation patch. It should show up on Ohloh after they refresh for you to claim if you aren't already registered there.
http://www.ohloh.net/p/drupalcommerce/contributors
Comment #4
rszrama commented(Forgot to update the status.)
Comment #5
alpritt commentedThat essentially IS what happens in Ubercart isn't it? Unless I'm missing something, you still then have to handle the 'intelligent merging' if the authenticated user has already got something in their cart. If you swap over the IDs I think you'd have to do the same juggling of the old authenticated cart as happens in Ubercart's uc_cart_login_update().
Comment #6
rszrama commentedAhh, you're right - I forgot about the fact that we had to merge with the existing order. My bad. In that case, I'll get right on reviewing your patches. ; )
Comment #7
rszrama commentedComment #8
rszrama commentedTook another look at this during our code sprint... I wonder if it wouldn't be easier to just add the line items to the authenticated cart manually by tweaking its line items array instead of finding the line item IDs and using the full commerce_cart_product_add()?
I did pull your code as is into my branch and will revisit this issue for further testing / resolution.
Comment #9
rszrama commentedAlan, I'm pretty sure I have your latest code at the moment, and I noticed that upon converting the anonymous cart to an authenticated cart, it deleted my anonymous order and recreated it for the user account even though no merge needed to happen. We need to smartly handle the merge so data isn't unnecessarily lost. So, at a minimum, I'd like to see this patch changed so it only deletes if necessary (i.e. if the user didn't have an authenticated cart already, you could just use the approach of switching the order's customer_uid). Further discussion would then center around what to do with any other data on the anonymous order... for example, maybe there's address data in there already? We wouldn't want to lose that... so we might do some sort of merge where the anonymous order's data (since it's the most recent) takes precedence over data in the authenticated order. Make sense?
Comment #10
rszrama commentedAlso, before you do any further work, pull from my repository to get a fix to only operate if an anonymous order actually existed.
See: http://github.com/rszrama/drupalcommerce/commit/7f97b6a1f283b6148e629c50...
Comment #11
alpritt commentedoh, sorry, I'm not dead; I just somehow managed to miss your reply to this thread. I'll try to give this another look in a few days time. I think what you say above all makes some sense, but I'll dig into the code again before I try to clarify or suggest anything.
Comment #12
rszrama commentedAhh, great - I was just about to dive into this myself but will work elsewhere until I hear from you. : )
Comment #13
alpritt commented"I wonder if it wouldn't be easier to just add the line items to the authenticated cart manually by tweaking its line items array instead of finding the line item IDs and using the full commerce_cart_product_add()?"
If we manually merged the array I don't believe we have any way of judging scenarios where the same product is present in both anonymous and authenticated carts. So, for example, I think we would end up with:
1x | Drupal Mug | $10
1x | Drupal Mug | $10
instead of what we currently get, which is:
2x Drupal Mug $20
which wouldn't be a good change.
Of course, it's probably more likely that they actually only wanted one item in the first place and what they'd actually prefer to see is
1x | Drupal Mug | $10
But hopefully you disagree about that, because I imagine it will be a pain to code that logic.
In any case, we probably need a drupal_set_message() asking them to review the contents of their cart when a merge occurs.
----
"I'd like to see this patch changed so it only deletes if necessary (i.e. if the user didn't have an authenticated cart already, you could just use the approach of switching the order's customer_uid)."
I think the way it works now means we're pointlessly eating anonymous order_ids and then giving them a new one when the merge happens, so I agree this is a good idea for that reason alone.
----
What data do we need to make sure we preserve. Just the stuff under $order->data?
Comment #14
rszrama commentedI just noticed an access bug when someone logs in on a checkout URL. It is no longer a valid URL since it depends on the Order ID. If the user is on a checkout URL, we should redirect them to the new URL for the correct order.
Comment #15
alpritt commented@14: I'm trying to work out the design choice here. Why do we have the order id present in the URL at all, rather than always grabbing it from the session/db? That would be more consistent with how we grab the correct order for the cart and would avoid this bug and simplify this 'router' stuff.
Comment #16
rszrama commentedWell, the problem is the order ID for a shopping cart will only be in the session if the Cart module is actually being used. The Checkout module needs to be independent of the Cart module to allow for non-shopping cart scenarios (i.e. one off donation forms, pre-filled invoices that a customer simply comes on-site to pay, etc.). The only way to effectively do this is to keep the order ID for the Checkout module part of the URL. This is why the Cart module and Checkout module both have their own page routing functions - the Cart module looks in the session for a cart order ID and redirects to the Checkout URL using the specific ID while the Checkout module looks at the checkout URL for an order ID and then performs validation / redirection as necessary for that order and the current user.
Comment #17
alpritt commentedhmm, but if the checkout module knows enough to do validation / redirection I would have though it would have enough information to work out the correct order (while keeping the cart independent). Unless there is a scenario where there might be more than one order from the user in checkout at the same time. Or an in-checkout order that the user hasn't actually initiated. But would that be sensible?
I may have a play with the code to see if what I'm thinking makes sense then. I've read through the code and understand what you're saying and still think this could be designed slightly cleaner. If not, I'll learn something at least!
Comment #18
rszrama commentedIn January, we determined the best thing to do is more than likely going to be to keep the anonymous shopping cart and make that the new authenticated cart. In other words, instead of merging like we are now, we should just update the uid of the anonymous cart. This will avoid unexpected items being in the cart suddenly when you login during checkout and fix the URL problem mentioned above.
We're undecided what to do with the existing cart. We determined it should be good enough for now to leave the old cart order in place... so technically it will become the user's shopping cart again upon checkout completion. Perhaps we can simply add in a hook_commerce_cart_convert() that lets modules react when an anonymous cart order is converted to an authenticated cart. A site that didn't want to preserve old carts can cancel them at that point in time.
Comment #19
rszrama commentedAlrighty, changed the behavior, made it an API, and added / documented the hook. I'm going to go through now and clean up some of the other user login logic...
https://github.com/rszrama/drupalcommerce/commit/d2493453edca664d7ccfe1a...
Comment #20
pcambraI think we should add functional tests for this?
Comment #21
rszrama commented+1
Comment #24
jasonrichardsmith@gmail.com commentedThank you, for the hook.
I have successfully implemented it, and wrote a post about it:
http://jasonrichardsmith.org/blog/drupal-commerce-merge-anonymous-carts-...