Fix unexpected error about not enough stocks while completing the payment in the Admin Panel

[jakubtobiasz]

Q	A
Branch?	1.12
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Related tickets	#16160
License	MIT

[github-actions]

Bunnyshell Preview Environment deployed

It will be automatically stopped in 4 hours.

Use the command /bns:start to start it if it's stopped.

Component	Endpoints
mailhog	https://mailhog-mwdahs.bunnyenv.com/
php	https://store-mwdahs.bunnyenv.com/

Available commands:

• /bns:stop to stop the environment
• /bns:deploy to redeploy the environment
• /bns:delete to remove the environment

[Wojdylak]

I'm not sure, but I feel that we should only check onHold. However, what happens if someone orders a product variant and, before completing this order, the administrator activates tracking for this variant?

[jakubtobiasz]

This logic comes from the \Sylius\Component\Core\Inventory\Operator\OrderInventoryOperator::sell method.

<?php

// ...
Assert::greaterThanEq(
                ($variant->getOnHold() - $orderItem->getQuantity()),
                0,
                sprintf(
                    'Not enough units to decrease on hold quantity from the inventory of a variant "%s".',
                    $variant->getName(),
                ),
            );

            Assert::greaterThanEq(
                ($variant->getOnHand() - $orderItem->getQuantity()),
                0,
                sprintf(
                    'Not enough units to decrease on hand quantity from the inventory of a variant "%s".',
                    $variant->getName(),
                ),
            );
// ...

So both values are checked, same as in the example above. If you hold a variant, the onHand value is not decreased.

1. There're 4 socks in stock (onHand: 4, onHold: 0)
2. You buy 2 socks with using the offline payment method (onHand: 4, onHold 2)
3. As an admin you complete the payment (onHand: 2, onHold: 0)

[jakubtobiasz]

However, what happens if someone orders a product variant and, before completing this order, the administrator activates tracking for this variant?

I guess we have to think about it, as currently it'll throw an exception. We'll discuss it on the daily meeting, and create a task if needed.

[diimpp]

This onHand/onHold pre-sell check logic feels important enough to varant it's own checker.

For example, why not add new method canBeSold to

Sylius/src/Sylius/Component/Core/Inventory/Operator/OrderInventoryOperator.php

Line 52 in a0ad0d1

public function sell(OrderInterface $order): void

or make brand new OrderInventoryChecker

[oallain]

I think it needs a bit of work.
Because the

Sylius/src/Sylius/Component/Core/Inventory/Operator/OrderInventoryOperator.php

Line 62 in 7735d06

Assert::greaterThanEq(

or

Sylius/src/Sylius/Component/Core/Inventory/Operator/OrderInventoryOperator.php

Line 62 in 7735d06

Assert::greaterThanEq(

Assertions introduce a bug into Sylius.

Let me explain:

• On a Variant, stock management is activated
• A customer places an order with this variant
• An administrator deactivates stock management on this variant
• On actions, such as payment, order cancellation, etc., we get a 500 error because the assertion is not correct.

[diimpp]

Just in case, can service's argument be removed BC wise?

[jakubtobiasz]

That's debatable. In theory, it can "break" something in terms of the system might start behaving differently. On the other hand,

• we never bothered about it, as our hands could be tied too much and make Sylius development harder
• our documentation doesn't mention it
• Symfony documentation also doesn't mention anything about it

[diimpp]

Something happened with this file,

• file name is not right
• feature/background parts are identical to finalizing_order_payment.feature aside from inventory line.

And the "Angel T-Shirt" product's inventory has become tracked with 1 items

Can this line be moved from background to scenario, so original Finalizing order payment and this feature file could be merged?

Otherwise, please rename file and feature accordingly.

[jakubtobiasz]

Right, I wanted to just reproduce the bug, I forgot to merge/rename that file.

[diimpp]

This onHand/onHold pre-sell check logic feels important enough to varant it's own checker.

For example, why not add new method canBeSold to

Sylius/src/Sylius/Component/Core/Inventory/Operator/OrderInventoryOperator.php

Line 52 in a0ad0d1

public function sell(OrderInterface $order): void

or make brand new OrderInventoryChecker

[Rafikooo]

I'm closing this ticket as the problem was resolved here: #16307