PayPal Express payments fail as tries to remove stock twice

[maderlock]

Preconditions

1. Magento 2.1.0 under nginx, php7 and mysql5.7
2. Paypal Express enabled with sandbox account
3. Configurable products set up with inventory enabled

Steps to reproduce

1. Add configurable product to basket more with qty for more than 1/2 remaining stock of that option (e.g. 3 left for option, so add qty of 2)
2. Checkout normally (do not use PayPal express from cart)
3. Select PayPal express from payment methods, enter details and submit back to site

Expected result

1. Success page

Actual result

1. Sent to review page with error message "We can't place the order."

Purchasing for an amount less than half works. Also, going to paypal express directly from the cart works. This is just for the checkout payment method.

From stepping through the code, it seems that after placing the order the quote is saved. On this save, all products are re-added to the cart. As the qty has already been removed from stock, this fails and an exception is thrown that there is not enough of the product. Although this isn't logged anywhere, on \Magento\Quote\Model\Quote line 1628 the exception is thrown along the lines of "Not enough stock for product blah". Thus, any qty that is more than 1/2 the remaining stock fails.

It then returns twice the stock back, so that the stock ends up too high. In some ways this is even more of a problem than PayPal failing, as we have a client that now has to do a whole new stock take!

I'm struggling to follow the code that is being run here. Why are all the products added to the quote again? Why are the buyRequests objects rather than the bare qtys that seem to exist when run at other times? Why is reset_count set on the request when the condition on \Magento\Quote\Model\Quote\Item\Processor::process never seems to fire?

[maderlock]

For anyone using PayPal this is a show-stopper.

[maderlock]

For now, I've disabled saving quote items once they are in an inactive quote. This seems like a non-optimal solution, but as a hack it does the job - PayPal Express orders can go through now without throwing an error, redirecting to the review step and putting incorrect stock back into the inventory.

I've added a plugin for \Magento\Quote\Model\Quote\Item\CartItemPersister containing:

use Magento\Quote\Api\Data\CartInterface;
use Magento\Quote\Api\Data\CartItemInterface;

class CartItemPersister
{
    public function aroundSave(
        \Magento\Quote\Model\Quote\Item\CartItemPersister $persister,
        \closure $closure,
        CartInterface $quote, CartItemInterface $item
    )
    {
        // If the quote is not active, then skip this item save (probably after order placement)
        if ($quote->getIsActive() === false) {
            return;
        }

        // Else call as usual
        $closure($quote, $item);
    }
}

[pboisvert]

@asemenenko Can you check into this one from the PayPal side and assign to another team if the issue relates to inventory?

[srenon]

Just a side note : PayPal Express seem to be processing most things twice. I was working on a module that apply additional fees to checkout total and when checking out using paypal the fee was applying twice. I track it down to collect() executing more times during paypal checkout vs other checkout method.

[iivashchenko]

Hi @maderlock, thanks for reporting. I've linked this issue to internal ticket MAGETWO-58676.

[phirunson]

Hi, not sure what the progress on this is, but here is my take. Using Magento EE 2.1.0

Have been affected by this as well and tracked it down on our project to the "Skip Order Review Step" function with Paypal Express. If enabled, this occurs:

• When the order is placed, the controller called is \Magento\Paypal\Controller\Express\AbstractExpress\ReturnAction::execute.
• This method calls \Magento\Paypal\Controller\Express\AbstractExpress::_initCheckout, which then calls \Magento\Paypal\Controller\Express\AbstractExpress::_getQuote which then calls \Magento\Checkout\Model\Session::getQuote. At this point, the current quote is saved onto the \Magento\Checkout\Model\Session object.
• The initial \Magento\Paypal\Controller\Express\AbstractExpress\ReturnAction::execute method then calls \Magento\Paypal\Model\Express\Checkout::returnFromPaypal, which saves the quote using $quote->collectTotals(); $this->quoteRepository->save($quote);
• The request is then forwarded to \Magento\Paypal\Controller\Express\AbstractExpress\PlaceOrder::execute. This is important because this continues the php session instead of creating a new session.
• This then calls \Magento\Paypal\Controller\Express\AbstractExpress::_initCheckout, which retrieves the current quote from the \Magento\Checkout\Model\Session object's stored variable $_quote.
• This is problematic as the quote has had collectTotals() called, and has been saved. This comment on \Magento\Checkout\Model\Session:229 indicates that a quote should be reloaded if this occurs:

/*
 * We mast to create new quote object, because collectTotals()
 * can to create links with other objects.
 */

• What seems to happen is that when \Magento\Paypal\Model\Express\Checkout::place is called, the quote has the links created when collectTotals() was called, and the items in the quote try to be added to the cart, which fails because there is not enough inventory left in stock, and throwing an exception

Our current solution is having an override on the \Magento\Paypal\Model\Express\Checkout::place method that sets the $_quote variable on that class to a newly retrieved quote (Already had this class overridden for a different purpose. It may be possible to do this as a plugin, but the $_quote object is protected, and there doesn't appear to be a way of setting via a public method, so may need to do override):

    public function place($token, $shippingMethodCode = null)
    {
        //This is a workaround to fix an issue where quote being saved twice in a single call causes the cart to
        //think items are being added. Reloads the quote.
        $this->_quote = $this->quoteRepository->get($this->_quote->getId());
        return parent::place($token, $shippingMethodCode);
    }

This works because the repository unsets it's internal variables after saving a quote, so calling get() will retrieve the quote again from db. This solution is not ideal, but works.

I think a possible solution is that there should be a way of clearing \Magento\Checkout\Model\Session::$_quote to be null without destroying the session so that a call to \Magento\Checkout\Model\Session::getQuote will retrieve a new object, or have an argument that forces getQuote() to retrieve the object from scratch.
That way, at \Magento\Paypal\Controller\Express\AbstractExpress\ReturnAction:34, before forwarding to placeOrder, the \Magento\Checkout\Model\Session::$_quote can be reset to null so that the order is placed correctly

[iivashchenko]

@phirunson, thank you for your investigation. The internal ticket is still opened. Link to your solution has been attached to the ticket. I will leave comments as soon as there is progress on the issue.

[pobrassard]

Hi,

Is it possible that this issue it not only with PayPal Express. I'm getting the same problem with other payments method not integrated by default in Magento.

[tobias-forkel]

Thanks @phirunson I was able to fix the issue in Magento CE 2.1.1 as follows:

• 1. Create extension Company_Paypal to rewrite a model
• 2. Rewrite app/code/Company/Paypal/Model/Express/Checkout.php
• 3. Extend from \Magento\Paypal\Model\Express\Checkout

<?php
/**
 * Copyright © 2016 Magento. All rights reserved.
 * See COPYING.txt for license details.
 */
namespace Company\Paypal\Model\Express;

class Checkout extends \Magento\Paypal\Model\Express\Checkout
{

• 4. Copy method place from /vendor/magento/module-paypal/Model/Express/Checkout.php
• 5. Replace $this->_quote->collectTotals()

         $this->ignoreAddressValidation();
-        $this->_quote->collectTotals();
+        $this->_quote = $this->quoteRepository->get($this->_quote->getId());
         $order = $this->quoteManagement->submit($this->_quote);

• 6. You may have to copy method ignoreAddressValidation from /vendor/magento/module-paypal/Model/Express/Checkout.php as well

[maderlock]

Any progress on MAGETWO-58676?

[iivashchenko]

@maderlock, no, so far. I'll notice you about any changes of an issue state.

[sergiojovanig]

I can confirm this bug in Magento CE 2.1.3.

This bug is critical and it was reported at Aug '16, 6 months ago...

Thanks to @phirunson I could fix the bug.

[iivashchenko]

@sjovanig, it is fixed in the development branch and going to be ported to Magento CE 2.1.x right after delivering to mainline.

[staffrob]

Facing this issue in 2.1.4

[alena-marchenko]

Hi @maderlock

Fix for MAGETWO-58676 is merged to develop branch, closing the issue.
Thank you.

[andidhouse]

This is closed in February and still present at 2.1.7 - wow.

Paypal is useless with this bug. I am not shure how PayPal as your partner will react on a major bug like this. For us goodby PayPal for now as magento team does not solve this since month.

[ebaschiera]

Fixed in 2.1.8. (but do a couple of tests before upgrading: #10499 )

[andidhouse]

@orlangur and now?

#10499

The new version 2.1.8 is again full of bugs regarding PayPal, reindexing, updating etc. etc.

And now? Close the M2 Shops and go for another system or what do you propose? Staying with these kind of bugs makes no sense at all. What do you think users of M2 should do now?

[orlangur]

@ebaschiera @andidhouse #10499 does not seem to be easily reproducible on clean Magento 2 installation. Having affected shop instance in your hand it should be not too hard to locate the buggy code with xdebug.

@andidhouse "full of bugs" sounds a bit harsh. I saw the problem with indexing performance degradation, but what do you mean by "bugs regarding updating"?

[andidhouse]

I really wonder sometimes why you guys test on a clean install only?
A lot of problems come from updates and most users are not new users but update the system.

Here are only some bugs reported - if you search here on GitHub i am shure to find 10+ more.
Also i am shure that in the next time the bugs regarding 2.1.8 and the update to it will rise here.

Is is really that hard to make it more stable and test more before launching a new version?
I mean the people here are not reporting bugs for fun...

#10586
#10578
#10565
#10561
#10535
#10530
#10531
#10517

[orlangur]

I've checked links up to #10561 and looks like this is just a list of random bugs without any analysis.

#10586 - people saw such errors before
#10578 - bug with en_GB locale date format, not update-related
#10565 - already fixed by a Community member, not update-related
#10561 - problem with third-party extensions or specific instance O.o

By "clean install" not the 2.1.8 installation from scratch is usually meant but any installation without third-party modules. For example, if you get clean 2.1.7 with official sample data and then upgrade it to 2.1.8 revealing some problem, it's a valid bug scenario.