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

Checkout forms should be able to specify which line items to purchase. #947

Closed
brandonkelly opened this issue Aug 1, 2019 · 15 comments
Closed
Assignees
Labels

Comments

@brandonkelly
Copy link
Member

Checkout forms need a way to instruct Commerce to only purchase certain line items & quantities, ignoring anything else that was added to the cart elsewhere.

Scenario A – unexpected cart merge:

  1. Guest adds items to cart.
  2. Guest clicks “Checkout”.
  3. Checkout form gives guest an opportunity to log-in; guest is now a logged in user.
  4. Carts::getCart() is called (doesn’t matter how); a previous cart is merged into current cart; new line items added automatically.
  5. User completes checkout without realizing they were purchasing additional items.

Scenario B – two browser tabs:

  1. [Tab 1] Logged-in user adds items to cart.
  2. [Tab 1] User clicks “Checkout”.
  3. [Tab 2] User adds new line item to cart.
  4. [Tab 1] User overlooks that the cart form hasn’t been updated; completes checkout without realizing they were purchasing additional items.

Both of these could be avoided if the commerce/payments/pay action let you specify which line items IDs should actually be purchased, and at which quantities. For example:

For example:

<form method="post" action="">
  <input type="hidden" name="action" value="commerce/payments/pay">
  <input type="hidden" name="lineItems" value="{{ cart.lineItemInfo }}">
  <!-- ... -->
</form>

lineItemInfo would map to a getLineItemInfo() method on craft\commerce\elements\Order:

public function getLineItemInfo(): string
{
    $info= [];
    foreach ($this->getLineItems() as $lineItem) {
        $info[] = $lineItem->id . ':' . $lineItem->qty;
    }
    return implode(',', $info);
}

If lineItems is supplied and the active cart contains additional line items/quantities, the commerce/payments/pay action could just move all of the excess over to a new cart, and store its ID in a new session variable. Once the current order is complete, that new cart could be brought in as the current session‘s cart.

@bossanova808
Copy link
Contributor

I agree with the two scenarios being issues, although scenario B I am much less worried about as anything in that area is the result of explicit user add-to-cart actions.

Scenario A is implicit/hidden behaviour, which I personally think is the real issue - something happening to a users cart as a by-product of another action (in this case logging in), which they might not expect.

I'm not quite sure how this would work in practise with this flow:

View Cart -> (if logged out, otherwise skip to shipping) Login/Guest -> Shipping -> Payment -> Complete

As in, if this is something specifically on the Payment step, isn't it already too late? The items would have already been merged earlier (i.e. as they move to shipping after logging in).

So I presume the intention is that we save those 'fixed' line items earlier in the checkout somehow (cookie? Or will there be a Craft mechanism for this?), and keep passing them along?

Just trying to think this through!

@lukeholder
Copy link
Member

@bossanova808 @brandonkelly @echantigny

I think the core issue here is this "The cart changed on me unexpectedly during checkout".

But the issue can not only be brought about because of a cart merge, but because of other system rules that can change the cart during recalculation:

  • A sale no longer available to be applied (sale date expired while things are in the cart)
  • Discounts could no longer be applying if you change a qty below a minimum qty condition.
  • Custom PHP code that modifies the cart based on business rules.

To solve for the unexpected cart changing issue, we could add better flash/notice messages returned to the customer when things change in the cart. i.e "Sale X prices were updated, sale has finished.", or "Previous items in your cart have been added to this cart". (Maybe the new order audit log system coming in Commerce 3 could be used to accomplish this feature.)

As for the exact issue of merging the previous items as an issue during checkout, I propose the following solution:

If the developer does not want the cart contents to change (like with a merge) during the request, they should submit all line items that they want to be in the cart on that request. Doing this would use the standard commerce/cart/update-cart controller action to update line items, and it would use a new replaceLineItems flag. Together they would replace the line items being updated on the order, ignoring any current ones (that could have been merged earlier in the request).

By doing this on all pages the developer deems " checkout pages", including the payment page, the order contents can not change out from beneath the customer. Again, this doesn't solve the issue of the cart generally changing in unexpected ways during recalculation but does control what is in the cart explicitly.

Thoughts, please.

@echantigny
Copy link
Contributor

echantigny commented Aug 2, 2019

@lukeholder

Scenario A could potentially lead a user to by things he didn't expect, I agree. Maybe we could let the user decide? If, at any point, you are meeting a guest cart with a user cart, you could provide a flow for us to use to ask the user if he wants to merge the items or not? The dev could also make that decision for them and provide proper feedback. In my case, I have an order review before you go to pay for it doesn't affect me that much. My client's customers are more angry to loose their carts than anything else since the site's launch.

Scenario B is definitely a "flash message" situation. But those need to be very specific and it could lead to confusion of they are not. Also, those flash need to be set in the json response when the cart is fully Ajax, like mine.

Thanks for looking into this. I really hope we can find a way to get commerce to work like all the dev wants, but mostly, to let our clients keep their customers/sales.

@lukeholder
Copy link
Member

lukeholder commented Aug 7, 2019

OK, so currently on the develop branch is the following:

If the customer is a registered user they may expect to be able to log into another computer and continue their cart from a previous session. If the user arrives at the store page logged-in, without a cart in session the most recent cart belonging to that user is restored to the session.

If the user logs in, but already has a guest cart in session, this does not happen automatically.

When retrieving the current cart you can optionally tell the system to merge in the line items from a previous session in 2 ways:

  1. Submit the mergeCarts parameter in either the commerce/cart/get-cart ajax controller action or the commerce/cart/update-cart controller action.

<input type="hidden" name="mergeCarts" value="commerce/cart/update-cart">

  1. Instead of calling {% set cart = craft.commerce.cart.cart %} in your twig template, call craft.commerce.cart.mergedCart.

Please note, using the above 2 methods will only merge previous carts of a logged-in user and only carts that belong to that user.

Calling craft.commetce.cart.mergedCart while the user is a guest behaves the same way as craft.commetce.cart.cart, so there is no harm in using it on most cart pages.

Having said that, you might not want to use it on final checkout pages so the customers don't get confused seeing new items in the cart before payment.

Before merging, you may want to show the user what will be merged.

You could do this by showing the other cart’s contents from the previous session with:

{% set otherCarts = craft.orders.isCompleted(false).user(currentUser).id('not ' ~ cart.id).all()

The above query gets all carts for the current user, excluding the current cart.

@eric-chantigny
Copy link

@lukeholder Some extra questions here:

1- When we call the cart with mergedCart, will it get rid of the old carts after the merge?
2- If we ask the user if he wants to merge in old carts, and he says no, is there a way to tell the system to get rid of the old carts so we don't ask him over and over again?
3- I use Ajax for everything related to the cart, so hopefully, I can easily get the "otherCarts" that way too, instead of Twig?

I will soon update my local copy to this latest dev version, but I want to make sure that I don't go too deep down the rabbit hole.

Thanks

@lukeholder
Copy link
Member

lukeholder commented Aug 7, 2019

@eric-chantigny

1 - Yes it soft deletes them like any other element, but garbage collection still needs to run to delete them fully from the database.

2 - Not with a controller action but you could call {% do craft.app.elements.deleteElementById(otherCart.id) %}

3 - On page load, you would load the other carts order numbers with twig into your JS, then you can get each cart over ajax with the commerce/cart/get-cart' passing each orderNumber` param to get each cart.

Let me know how you go.

@lukeholder
Copy link
Member

We could look to add a delete cart ajax action if a valid cart number belonging to the current user is supplied.

@echantigny
Copy link
Contributor

@lukeholder

I feel like this is getting really complex though. I would've loved to see a true/false config setting saying "merge carts", like it was first done. That way, we could just activate that if we want that behavior. I liked the way you had it last week.

I'll see if I can implement this without adding any more issues.

Thanks for the work!

@lukeholder
Copy link
Member

@eric-chantigny If you want it the way it was last week (on the develop branch), simply include mergeCarts in all your ajax requests and it will work the exact same way. And if you are using twig, switch all craft.commerce.carts.cart to craft.commerce.carts.mergedCart.

@echantigny
Copy link
Contributor

@lukeholder

Thanks for the clarification. I missed that option.

@lukeholder
Copy link
Member

@bossanova808 Welcome your input

@echantigny
Copy link
Contributor

@lukeholder

I tested this with the craft.commerce.carts.mergedCart and mergeCarts as defaults and everything seems to be working.

I need to update the Polling code to take into account the ajax calls to update the cart, which currently triggers the polling update each time, but that should be easy enough setting top level JS variables.

One thing that would be great to have is a flag on the json and twig cart object when we call MergedCart (or ajax mergeCarts) to let us know if there was actually anything merged in or it was only 1 cart. Just a boolean flag would be enough. That way, we could let our customers know if anything was merged since their last visit. In my case, this would be useful because, for the first few weeks, some very old carts will start merging in (the lost ones).

@bossanova808
Copy link
Contributor

@lukeholder Looks like if one wants no merging at all (i.e. current behaviour) then just always use {% set cart = craft.commerce.cart.cart %} ...so it's nice to still have that option.

Given we do complex service orders, the merging thing is tricky for us but I will have a proper look next week...

@lukeholder
Copy link
Member

@bossanova808 yeah all these changes are backwards compatible so we are going ahead with the release with these changes in 2.1.11

@eric-chantigny
Copy link

@lukeholder Any chance you can add that flag (see above) before pushing the change to the master branch?

Otherwise, I'll code something in my template to verify if there's any user carts.cart that will merge before I call carts.mergedCart, but I feel like that will take a bigger toll on performance.

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

No branches or pull requests

5 participants