[Inventory] Holding inventory units during checkout #6033

Arminek · 2016-09-08T13:18:46Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Related tickets
License	MIT

gabiudrescu · 2016-09-08T20:22:46Z

are you sure you want to do this as a standard behavior of Sylius? because reserving the stock of a product for the user only for adding it to cart is a pretty big commitment, from business point of view.

also, it's not clear for me from these scenarios how the units get released? only by acquisition? how can I, as an admin, cancel these reservations?

lchrusciel · 2016-09-09T07:15:36Z

src/Sylius/Component/Core/Inventory/Updater/OnHoldQuantityUpdater.php

+    {
+        foreach ($order->getItems() as $orderItem) {
+            /** @var OrderItemInterface $orderItem */
+            /** @var ProductVariantInterface $variant */


Is redundant, as getVariant is typehinted.

michalmarcinkowski · 2016-09-14T20:40:50Z

@gabiudrescu we decided to have the following default behavior:

Reserve inventory when order is placed (checkout completed)
When the order is cancelled the reserved units are released (to be implemented in coming weeks)
After the order is paid the inventory is decreased

Later we will think about mechanisms like releasing reserved units when order is not paid after X minutes, but right now it complicates a lot. E.g. you can have offline payment method (e.g. cash on delivery) and you don't want to release the reserved units after X minutes. What is more we need to figure out what will happen when the reserved units are released and after that someone will pay for the order that have products which are out of stock... Let's discuss that later :)

michalmarcinkowski

We should figure out better naming for these services.

michalmarcinkowski · 2016-09-14T20:42:22Z

features/inventory/checkout_inventory/holding_inventory_units_during_checkout_process.feature

+    Scenario: Holding inventory units
+        Given this user has added 3 products "Iron Maiden T-Shirt" to the cart
+        And this user bought those products
+        When I view all variants of the product "Iron Maiden T-Shirt"


I think that When I view variants of product "Iron Maiden T-Shirt" will be more appropriate step.

michalmarcinkowski · 2016-09-14T20:43:40Z

features/inventory/checkout_inventory/holding_inventory_units_during_checkout_process.feature

+        Given this user has added 3 products "Iron Maiden T-Shirt" to the cart
+        And this user bought those products
+        When I view all variants of the product "Iron Maiden T-Shirt"
+        Then I should know that 3 units of this product is hold


Then 3 units of this product should be on hold And 5 units of this product should be on hand

michalmarcinkowski · 2016-09-14T20:45:33Z

features/inventory/checkout_inventory/holding_inventory_units_during_checkout_process.feature

+    Scenario: Release hold units after order has been paid
+        Given this user has added 3 products "Iron Maiden T-Shirt" to the cart
+        And this user bought this product
+        When I view the summary of this order made by "sylius@example.com"


When I view the summary of this order

michalmarcinkowski · 2016-09-14T20:46:55Z

features/inventory/checkout_inventory/holding_inventory_units_during_checkout_process.feature

+        When I view the summary of this order made by "sylius@example.com"
+        And I mark the order of "sylius@example.com" as a paid
+        And I view all variants of the product "Iron Maiden T-Shirt"
+        Then I should not know about on hold quantity of this product


Then 2 units of this product should be on hand And there should be no units of this product on hold

@michalmarcinkowski And ~~there should be~~ no units of this product on hold?

michalmarcinkowski · 2016-09-14T20:47:30Z

features/inventory/checkout_inventory/holding_inventory_units_during_checkout_process.feature

+        Given this user has added 3 products "Iron Maiden T-Shirt" to the cart
+        And this user bought this product
+        When I view the summary of this order made by "sylius@example.com"
+        And I mark the order of "sylius@example.com" as a paid


... as a paid

michalmarcinkowski · 2016-09-14T20:49:09Z

src/Sylius/Behat/Page/Admin/ProductVariant/IndexPage.php

+
+            return $quantity === $onHandQuantity;
+        } catch (ElementNotFoundException $exception) {
+            return false;


Should we return false when the element is not on page?

IMO in those kind of methods yes.

We should return false only if that element is not rendered if on hand quantity is equal to 0.

Looking at src/Sylius/Bundle/AdminBundle/Resources/views/ProductVariant/Field/inventory.html.twig we can see, that those elements are always rendered, so we should not catch that exception.

michalmarcinkowski · 2016-09-14T20:51:11Z

src/Sylius/Component/Core/Inventory/Handler/PaidOrderInventoryHandler.php

+/**
+ * @author Arkadiusz Krakowiak <arkadiusz.krakowiak@lakion.com>
+ */
+final class PaidOrderInventoryHandler implements PaidOrderInventoryHandlerInterface


Why the name of the class is coupled with order state (PaidOrder)?

michalmarcinkowski · 2016-09-14T20:51:52Z

src/Sylius/Component/Core/Inventory/Handler/PaidOrderInventoryHandler.php

+
+    public function __construct()
+    {
+        $this->decreasingQuantityUpdaters = new PriorityQueue();


Another composite? Looks like overhead for this use case...

pamil · 2016-09-15T11:41:46Z

features/inventory/checkout_inventory/holding_inventory_units_during_checkout_process.feature

+        When I view the summary of this order made by "sylius@example.com"
+        And I mark the order of "sylius@example.com" as a paid
+        And I view all variants of the product "Iron Maiden T-Shirt"
+        Then I should not know about on hold quantity of this product


@michalmarcinkowski And ~~there should be~~ no units of this product on hold?

pamil · 2016-09-15T11:42:31Z

src/Sylius/Behat/Context/Ui/CheckoutContext.php

@@ -170,7 +170,7 @@ public function iAmAtTheCheckoutAddressingStep()
    }

    /**
-     * @Given /^(this user) bought this product$/
+     * @Given /^(this user) bought (?:this|those) product(?:|s)$/


(?:this product|those products)?

pamil · 2016-09-15T11:43:58Z

src/Sylius/Behat/Context/Ui/Shop/CartContext.php

@@ -276,6 +276,16 @@ public function thisUserHasProductInTheCart(ShopUserInterface $user, ProductInte
    }

    /**
+     * @Given /^(this user) has(?:| added) (\d+) (products "[^"]+") (?:to|in) the cart$/


What about expanding this regexp?

/^(this user) has (\d+) (products "[^"]+") in the cart$/ /^(this user) added (\d+) (products "[^"]+") to the cart$/

pamil · 2016-09-15T11:48:21Z

src/Sylius/Behat/Page/Admin/ProductVariant/IndexPage.php

+
+            return $quantity === $onHandQuantity;
+        } catch (ElementNotFoundException $exception) {
+            return false;


We should return false only if that element is not rendered if on hand quantity is equal to 0.

pamil · 2016-09-15T11:48:29Z

src/Sylius/Behat/Page/Admin/ProductVariant/IndexPage.php

+
+            return $quantity === $onHoldQuantity;
+        } catch (ElementNotFoundException $exception) {
+            return false;


Same here (https://github.com/Sylius/Sylius/pull/6033/files#r78837272).

Should throw exception when element not found.

pamil · 2016-09-15T11:51:59Z

src/Sylius/Component/Core/Inventory/Handler/PaidOrderInventoryHandler.php

+
+    public function __construct()
+    {
+        $this->decreasingQuantityUpdaters = new PriorityQueue();


pamil · 2016-09-15T12:05:42Z

src/Sylius/Component/Core/Inventory/Handler/PaidOrderInventoryHandler.php

+        foreach ($this->decreasingQuantityUpdaters as $decreasingQuantityUpdater) {
+            try {
+                $decreasingQuantityUpdater->decrease($order);
+            } catch (\InvalidArgumentException $exception) {


It shouldn't catch general exceptions.

pamil · 2016-09-15T12:06:10Z

src/Sylius/Component/Core/Inventory/Handler/PaidOrderInventoryHandler.php

+    public function handle(OrderInterface $order)
+    {
+        if ($this->decreasingQuantityUpdaters->isEmpty()) {
+            throw new HandleException(self::class, 'There is no decreasing updaters to handle this request.');


HandleException says nothing, as we can get a stacktrace from any exception, but it does not say what came wrong.

pamil · 2016-09-15T12:06:27Z

src/Sylius/Component/Core/Inventory/Handler/PaidOrderInventoryHandlerInterface.php

+/**
+ * @author Arkadiusz Krakowiak <arkadiusz.krakowiak@lakion.com>
+ */
+interface PaidOrderInventoryHandlerInterface


PaidOrder -> Order

pamil · 2016-09-15T12:06:57Z

src/Sylius/Component/Core/Inventory/Handler/PaidOrderInventoryHandlerInterface.php

+/**
+ * @author Arkadiusz Krakowiak <arkadiusz.krakowiak@lakion.com>
+ */
+interface PaidOrderInventoryHandlerInterface


Anyway, Handler seems to be a too general name. What should it do?

pamil · 2016-09-15T12:07:25Z

src/Sylius/Component/Core/Inventory/Updater/DecreasingQuantityUpdaterInterface.php

@@ -16,7 +16,7 @@
 /**
 * @author Anna Walasek <anna.walasek@lakion.com>
 */
-interface OnHandQuantityUpdaterInterface
+interface DecreasingQuantityUpdaterInterface


Why is it Decreasing? What should it do?

pamil · 2016-09-15T12:08:34Z

src/Sylius/Component/Core/Inventory/Updater/IncreasingQuantityUpdaterInterface.php

+/**
+ * @author Arkadiusz Krakowiak <arkadiusz.krakowiak@lakion.com>
+ */
+interface IncreasingQuantityUpdaterInterface


Seems redundant, has one method with one argument, same as DecreasingQuantity..., why won't you merge them?

pamil · 2016-09-15T12:09:56Z

src/Sylius/Component/Core/spec/Inventory/Updater/OnHoldQuantityUpdaterSpec.php

+        $orderItem->getQuantity()->willReturn(10);
+        $orderItems = new ArrayCollection();
+        $orderItems->add($orderItem->getWrappedObject());
+        $order->getItems()->willReturn($orderItems);


$order->getItems()->willReturn([$orderItem]);

instead of these three lines.

Same for every ArrayCollection in specs.

pamil

GitHub does not allow to add request changes review without any text here...

michalmarcinkowski · 2016-09-15T14:49:45Z

src/Sylius/Behat/Context/Ui/Admin/ManagingProductVariantsContext.php

+    public function thereShouldBeNoUnitsOfThisProductOnHold(ProductInterface $product)
+    {
+        Assert::false(
+            $this->indexPage->hasOnHoldQuantity($product->getFirstVariant(), 1),


Why checking for 1? If 0 is not displayed we ca simply check if there is onHold label or not.

michalmarcinkowski · 2016-09-15T14:50:50Z

src/Sylius/Behat/Page/Admin/ProductVariant/IndexPage.php

+
+            return $quantity === $onHoldQuantity;
+        } catch (ElementNotFoundException $exception) {
+            return false;


Should throw exception when element not found.

michalmarcinkowski · 2016-09-15T14:52:49Z

src/Sylius/Component/Core/Inventory/Updater/OnHandQuantityUpdater.php

+            $variant = $orderItem->getVariant();
+
+            if ($variant->isTracked()) {
+


michalmarcinkowski · 2016-09-15T14:55:20Z

src/Sylius/Component/Core/spec/Inventory/Updater/OnHoldQuantityUpdaterSpec.php

+    ) {
+        $productVariant->getOnHold()->willReturn(10);
+        $productVariant->isTracked()->willReturn(true);
+        $productVariant->getName()->willReturn('t-shirt');


Do we need it?

michalmarcinkowski · 2016-09-15T14:55:41Z

src/Sylius/Component/Core/spec/Inventory/Updater/OnHoldQuantityUpdaterSpec.php

+    ) {
+        $productVariant->getOnHold()->willReturn(5);
+        $productVariant->isTracked()->willReturn(true);
+        $productVariant->getName()->willReturn('t-shirt');


Same as above.

Yep we are using Assert library so i need this to pass information about the name of variant before the exception will be thrown.

FYI for name we should use something like "Start wars T-Shirt" to make it closer to real data :)

michalmarcinkowski · 2016-09-15T14:56:32Z

src/Sylius/Component/Core/spec/Inventory/Updater/OnHoldQuantityUpdaterSpec.php

+        $productVariant->setOnHold(5)->shouldBeCalled();
+
+        $this->decrease($order);
+    }


Missing spec for decreasing onHold of two different items.

pamil · 2016-09-15T15:29:35Z

src/Sylius/Behat/Page/Admin/ProductVariant/IndexPageInterface.php

+     *
+     * @return bool
+     */
+    public function hasOnHoldLabelFor(ProductVariantInterface $productVariant);


This method should be removed.

What do you think about refactoring hasOn*QuantityFor(ProductVariantInterface $productVariant, $quantity) : bool to getOn*QuantityFor(ProductVariantInterface $productVariant) : int and comparing it in the context?

This way we can also show nicer exceptions.

Also, this separates templating logic (do not show onHold if onHold = 0) as the other shops may implement it differently.

pamil · 2016-09-15T15:41:41Z

src/Sylius/Bundle/AdminBundle/Resources/views/ProductVariant/Field/inventory.html.twig

@@ -1,10 +1,10 @@
 {% if data.isTracked %}
 <div class="ui {{ data.onHand > 0 ? 'green' : 'red' }} icon label">
    <i class="cube icon"></i>
-    {{ data.onHand }} {{ 'sylius.ui.available_on_hand'|trans }}
+    <span id="sylius-{{ data.id }}-on-hand-quantity">{{ data.onHand }}</span> {{ 'sylius.ui.available_on_hand'|trans }}


id="sylius-{{ data.id }}-on-hand-quantity" -> class="sylius-on-hand-quantity" data-product-variant-id="{{ data.id }}"? These non-semantic IDs made only for Behat looks quite bad to me :/

michalmarcinkowski · 2016-09-16T11:50:13Z

Good job Arek! 👍

Arminek changed the title ~~[Inventory] Holding inventory units during checkout~~ [WIP][Inventory] Holding inventory units during checkout Sep 8, 2016

Arminek force-pushed the inventory-hold branch from 19f1c6d to 8b8fe77 Compare September 8, 2016 13:32

pjedrzejewski added the New Feature label Sep 9, 2016

lchrusciel reviewed Sep 9, 2016
View reviewed changes

Arminek force-pushed the inventory-hold branch 3 times, most recently from d6b9f09 to 4a4231c Compare September 13, 2016 17:58

Arminek changed the title ~~[WIP][Inventory] Holding inventory units during checkout~~ [Inventory] Holding inventory units during checkout Sep 13, 2016

michalmarcinkowski suggested changes Sep 14, 2016

View reviewed changes

pamil reviewed Sep 15, 2016

View reviewed changes

pamil suggested changes Sep 15, 2016

View reviewed changes

Arminek force-pushed the inventory-hold branch 2 times, most recently from f613632 to 049455b Compare September 15, 2016 13:40

michalmarcinkowski suggested changes Sep 15, 2016

View reviewed changes

Arminek force-pushed the inventory-hold branch from 049455b to 74fe73e Compare September 15, 2016 15:21

pamil reviewed Sep 15, 2016

View reviewed changes

Arminek force-pushed the inventory-hold branch 4 times, most recently from ba12d2c to 95a777d Compare September 16, 2016 09:56

Arminek added 4 commits September 16, 2016 12:54

[Inventory] Holding inventory during checkout feature

11d7aae

[Inventory] Create on hold quantity updater service

268b981

[Behat] Inventory feature implementation

d6d8b1a

Review fixes

08fcd5e

Arminek force-pushed the inventory-hold branch from 95a777d to 08fcd5e Compare September 16, 2016 10:59

michalmarcinkowski merged commit 03723d3 into Sylius:master Sep 16, 2016

		$variant = $orderItem->getVariant();

		if ($variant->isTracked()) {

[Inventory] Holding inventory units during checkout #6033

[Inventory] Holding inventory units during checkout #6033

Conversation

Arminek commented Sep 8, 2016

gabiudrescu commented Sep 8, 2016

Choose a reason for hiding this comment

michalmarcinkowski commented Sep 14, 2016

michalmarcinkowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalmarcinkowski Sep 14, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pamil Sep 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pamil Sep 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pamil Sep 15, 2016 • edited Loading

Choose a reason for hiding this comment

pamil Sep 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pamil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalmarcinkowski commented Sep 16, 2016

michalmarcinkowski Sep 14, 2016 •

edited

Loading

pamil Sep 15, 2016 •

edited

Loading

pamil Sep 15, 2016 •

edited

Loading

pamil Sep 15, 2016 •

edited

Loading

pamil Sep 15, 2016 •

edited

Loading