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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 24 additions & 0 deletions features/order/managing_orders/handling_order_payment/sps_253.feature
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
@managing_orders
Feature: Finalizing order payment
In order to mark order's payment state as complete
As an Administrator
I want to be able to finalize payment

Background:
Given the store operates on a single channel in "United States"
And the store has a product "Angel T-Shirt"
And the store ships everywhere for Free
And the store allows paying with "Cash on Delivery"
And the "Angel T-Shirt" product's inventory has become tracked with 1 items
And there is a customer "lucy@teamlucifer.com" that placed an order "#00000666"
And the customer bought 1 "Angel T-Shirt" products
And the customer "Lucifer Morningstar" addressed it to "Seaside Fwy", "90802" "Los Angeles" in the "United States" with identical billing address
And the customer chose "Free" shipping method with "Cash on Delivery" payment
And I am logged in as an administrator

@ui
Scenario: Finalizing order's payment
Given I view the summary of the order "#00000666"
When I mark this order as paid
Then I should be notified that the order's payment has been successfully completed
And it should have payment state "Completed"
26 changes: 19 additions & 7 deletions src/Sylius/Bundle/CoreBundle/EventListener/PaymentPreCompleteListener.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,10 @@

use Sylius\Bundle\ResourceBundle\Event\ResourceControllerEvent;
use Sylius\Component\Core\Model\PaymentInterface;
use Sylius\Component\Inventory\Checker\AvailabilityCheckerInterface;
use Sylius\Component\Core\Model\ProductVariantInterface;

final class PaymentPreCompleteListener
{
public function __construct(
private AvailabilityCheckerInterface $availabilityChecker,
) {
}

public function checkStockAvailability(ResourceControllerEvent $event): void
{
/** @var PaymentInterface $payment */
Expand All @@ -33,7 +28,7 @@ public function checkStockAvailability(ResourceControllerEvent $event): void
foreach ($orderItems as $orderItem) {
$variant = $orderItem->getVariant();

if (!$this->availabilityChecker->isStockSufficient($variant, $orderItem->getQuantity())) {
if (!$this->canBeCompleted($variant, $orderItem->getQuantity())) {
$event->setMessageType('error');
$event->setMessage('sylius.resource.payment.cannot_be_completed');
$event->setMessageParameters(['%productVariantCode%' => $variant->getCode()]);
Expand All @@ -43,4 +38,21 @@ public function checkStockAvailability(ResourceControllerEvent $event): void
}
}
}

private function canBeCompleted(ProductVariantInterface $variant, int $quantity): bool
{
if (!$variant->isTracked()) {
return true;
}

if ($variant->getOnHold() - $quantity < 0) {
return false;
}

if ($variant->getOnHand() - $quantity < 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return false;
}

return true;
}
}
1 change: 0 additions & 1 deletion src/Sylius/Bundle/CoreBundle/Resources/config/services/listeners.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,6 @@
</service>

<service id="Sylius\Bundle\CoreBundle\EventListener\PaymentPreCompleteListener">
<argument type="service" id="Sylius\Component\Inventory\Checker\AvailabilityCheckerInterface" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<tag name="kernel.event_listener" event="sylius.payment.pre_complete" method="checkStockAvailability" />
</service>

Expand Down