[Inventory] Inventory back to stock after order cancellation #6192

tuka217 · 2016-09-23T07:58:27Z

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

michalmarcinkowski · 2016-09-23T08:05:40Z

features/inventory/verify_teserved_inventory_is_back_in_stock_on_order_cancelling.feature

+
+    @todo
+    Scenario: Verify the reserved inventory is back in stock after order cancellation by customer
+        Given the customer canceled this order


Given the order "#22" was canceled

And if we still prefer british english - cancelled :)

michalmarcinkowski · 2016-09-23T08:06:06Z

features/inventory/verify_teserved_inventory_is_back_in_stock_on_order_cancelling.feature

+    Scenario: Verify the reserved inventory is back in stock after order cancellation by customer
+        Given the customer canceled this order
+        When I view all variants of the product "T-shirt banana"
+        And this variant should have 0 items on hold


I would also add a check for onHand here.

michalmarcinkowski · 2016-09-23T08:06:55Z

features/inventory/verify_teserved_inventory_is_back_in_stock_on_order_cancelling.feature

+
+    @todo
+    Scenario: Verify the reserved inventory is back in stock after order cancellation
+        When I cancel this order


When I cancel order XXX

michalmarcinkowski · 2016-09-23T08:08:35Z

features/inventory/verify_teserved_inventory_is_back_in_stock_on_order_cancelling.feature

+    Scenario: Verify the reserved inventory is back in stock after order cancellation
+        When I cancel this order
+        And I view all variants of the product "T-shirt banana"
+        And this variant should have 0 items on hold


this variant is unknown :)

michalmarcinkowski · 2016-09-23T08:09:19Z

We should also add scenarios when we buy more that one item, to verify all items are back in stock.

NoResponseMate · 2016-09-26T09:34:39Z

src/Sylius/Behat/Context/Setup/OrderContext.php

@@ -440,7 +441,10 @@ public function thisOrderIsAlreadyPaid(OrderInterface $order)

    /**
     * @Given /^the customer canceled (this order)$/


Since you're double elling stuff, might as well do it here and in the method's name.

NoResponseMate · 2016-09-26T09:37:34Z

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

+            }
+
+            if ('paid' === $order->getPaymentState()) {
+


Unnecessary empty line.

NoResponseMate · 2016-09-26T09:39:38Z

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

+                }
+
+                Assert::greaterThanEq(
+                    ($variant->getOnHold() - $orderItem->getQuantity()),


You're using ::getOnHold/Hand a lot. How 'bout putting those in variables?

NoResponseMate · 2016-09-26T09:48:42Z

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

+                continue;
+            }
+
+            if ('cancelled' === $order->getState() && 'paid' !== $order->getPaymentState()) {


It seems that the 2nd check will always be fulfilled as you check for the opposite a couple of lines up.

michalmarcinkowski · 2016-09-27T11:55:20Z

features/inventory/verify_reserved_inventory_is_back_in_stock_on_order_cancelling.feature

+        And I am logged in as an administrator
+
+    @ui
+    Scenario: Verify the reserved inventory is back in stock after order cancellation


after cancellation of a new order

michalmarcinkowski · 2016-09-27T11:55:52Z

features/inventory/verify_reserved_inventory_is_back_in_stock_on_order_cancelling.feature

+        And this variant should have 0 items on hold
+
+    @ui
+    Scenario: Verify the reserved inventory and quantity of product's items is back in stock after order cancellation


after cancellation of a paid order

michalmarcinkowski · 2016-09-27T12:05:42Z

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

+                continue;
+            }
+        }
+    }


This method should be splitted into 2: hold and release.

hold method will have the same logic as previous decrease in OnHoldQuantityUpdater (see this) and should be called on checkout complete transition (no need to check the order state in the method).

release method should be called on before cancel transition and have the following logic: If order payment state is paid then onHand++ otherwise onHold--.

Ok, but when we decrease onHold and onHand when order go from new to paid? Now it is done by OrderInventoryHandler which should be removed.

Arminek · 2016-09-27T14:05:44Z

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

+                    continue;
+                }
+
+                Assert::greaterThanEq(


How about creating a custom exception something like InventoryOperationException?

In next iteration

pamil · 2016-09-28T12:30:23Z

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

+
+            return;
+        }
+        $this->release($order);


Missing a blank line above.

pamil · 2016-09-28T12:33:34Z

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

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


Not needed blank line.

pamil · 2016-09-28T12:34:40Z

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

+
+            if ($variant->isTracked()) {
+
+                Assert::greaterThanEq(


The more specific exception than \InvalidArgumentException would be nicer to work with.

As I wrote above in next iteration

pamil · 2016-09-28T12:39:19Z

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

+     */
+    public function hold(OrderInterface $order)
+    {
+        /** @var OrderItemInterface $orderItem */


foreach ($order->getItems() as $orderItem) { $variant = $orderItem->getVariant(); if ($variant->isTracked()) { // ... } }

is used quite often in this class, we might extract it to method returning an array of all tracked order item or a method executing a callback on every tracked order item.

Something like this:

foreach ($this->getTrackedOrderItems($order) as $orderItem) { // ... }

Or just use the early continue to lower the nesting level:

foreach ($order->getItems() as $orderItem) { $variant = $orderItem->getVariant(); if (!$variant->isTracked()) { continue; } // ... }

Early continue is fine, but only for hold and giveBack

pamil · 2016-09-28T12:42:13Z

src/Sylius/Component/Core/spec/Inventory/Operator/OrderInventoryOperatorSpec.php

+
+        $this->shouldThrow(\InvalidArgumentException::class)->during('sell', [$order]);
+    }
+}


Missing blank line at the end.

pamil · 2016-09-28T12:42:46Z

src/Sylius/Component/Core/spec/Inventory/Operator/OrderInventoryOperatorSpec.php

+        $variant->getOnHold()->willReturn(10);
+        $variant->getOnHand()->willReturn(10);
+
+


Doubled blank line.

pamil · 2016-09-28T12:43:19Z

src/Sylius/Component/Core/spec/Inventory/Operator/OrderInventoryOperatorSpec.php

+ *
+ * @author Anna Walasek <anna.walasek@lakion.com>
+ */
+final class OrderInventoryOperatorSpec extends ObjectBehavior


It misses specs for ignoring variants that aren't tracked.

…their implementation

[CoreBundle] Add callbacks with OrderInventoryOperator in state_machines and add its declaration in services.xml

…ntityUpdater

michalmarcinkowski · 2016-09-28T20:14:13Z

Great work Ania! 👍

michalmarcinkowski · 2016-09-28T20:20:40Z

features/inventory/verify_reserved_inventory_is_back_in_stock_on_order_cancelling.feature

+        Given there is a customer "john.doe@gmail.com" that placed an order "#00000022"
+        And the customer bought 3 items of "Green" variant of product "T-shirt banana"
+        And the customer chose "Free" shipping method to "United States" with "Cash on Delivery" payment
+        And I cancelled the order "#00000022"


And the order "#00000022" was cancelled - is more apropriate

michalmarcinkowski · 2016-09-28T20:24:09Z

src/Sylius/Behat/Context/Setup/OrderContext.php

+     * @Given /^(this order) was cancelled$/
+     * @Given the order :order was cancelled
+     * @Given I cancelled the order :order
+     * @Given /^I cancelled (this order)$/


"Given I did something" steps should be removed

[Inventory] Inventory back to stock after order cancellation

michalmarcinkowski reviewed Sep 23, 2016

View reviewed changes

tuka217 force-pushed the order-cancelation-inventory branch 4 times, most recently from aece5a4 to 8537054 Compare September 23, 2016 15:47

michalmarcinkowski added the New Feature label Sep 26, 2016

tuka217 force-pushed the order-cancelation-inventory branch 2 times, most recently from 42556a3 to abe7331 Compare September 26, 2016 09:26

tuka217 changed the title ~~[WIP][Inventory] Inventory back to stock after order cancellation~~ [Inventory] Inventory back to stock after order cancellation Sep 26, 2016

tuka217 force-pushed the order-cancelation-inventory branch from abe7331 to 25400f0 Compare September 26, 2016 09:31

NoResponseMate reviewed Sep 26, 2016

View reviewed changes

tuka217 force-pushed the order-cancelation-inventory branch 2 times, most recently from b957964 to ca4dea3 Compare September 26, 2016 10:47

michalmarcinkowski reviewed Sep 27, 2016

View reviewed changes

michalmarcinkowski suggested changes Sep 27, 2016

View reviewed changes

Arminek reviewed Sep 27, 2016

View reviewed changes

tuka217 force-pushed the order-cancelation-inventory branch from ca4dea3 to 0e76d71 Compare September 28, 2016 12:26

pamil reviewed Sep 28, 2016

View reviewed changes

tuka217 and others added 3 commits September 28, 2016 15:00

[feature] Add scenarios (back to stock after order cancellation) and …

ff9c0de

…their implementation

[Core] Introduce OrderInventoryOperator

40ab110

[CoreBundle] Add callbacks with OrderInventoryOperator in state_machines and add its declaration in services.xml

[Core] Remove OrderInventoryHandler, OnHandQuantityUpdater, OnHoldQua…

5fc3919

…ntityUpdater

tuka217 force-pushed the order-cancelation-inventory branch from 0e76d71 to 5fc3919 Compare September 28, 2016 13:01

michalmarcinkowski merged commit 4a6e557 into Sylius:master Sep 28, 2016

michalmarcinkowski reviewed Sep 28, 2016

View reviewed changes

michalmarcinkowski mentioned this pull request Sep 28, 2016

cancel a paid order also refund #6144

Closed

tuka217 mentioned this pull request Sep 29, 2016

[Inventory] Minor fixes for back to stock after order cancellation #6244

Merged

pamil pushed a commit to pamil/Sylius that referenced this pull request May 7, 2019

Merge pull request Sylius#6192 from tuka217/order-cancelation-inventory

7c75a6b

[Inventory] Inventory back to stock after order cancellation

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Inventory] Inventory back to stock after order cancellation #6192

[Inventory] Inventory back to stock after order cancellation #6192

tuka217 commented Sep 23, 2016

michalmarcinkowski Sep 23, 2016

CoderMaggie Sep 23, 2016

michalmarcinkowski Sep 23, 2016

michalmarcinkowski Sep 23, 2016

michalmarcinkowski Sep 23, 2016 •

edited

Loading

michalmarcinkowski Sep 23, 2016

michalmarcinkowski commented Sep 23, 2016

NoResponseMate Sep 26, 2016

NoResponseMate Sep 26, 2016

NoResponseMate Sep 26, 2016

NoResponseMate Sep 26, 2016

michalmarcinkowski Sep 27, 2016 •

edited

Loading

michalmarcinkowski Sep 27, 2016

michalmarcinkowski Sep 27, 2016

tuka217 Sep 27, 2016 •

edited

Loading

Arminek Sep 27, 2016

tuka217 Sep 28, 2016

pamil Sep 28, 2016

pamil Sep 28, 2016

pamil Sep 28, 2016

tuka217 Sep 28, 2016

pamil Sep 28, 2016 •

edited

Loading

tuka217 Sep 28, 2016

pamil Sep 28, 2016

pamil Sep 28, 2016

pamil Sep 28, 2016 •

edited

Loading

michalmarcinkowski commented Sep 28, 2016

michalmarcinkowski Sep 28, 2016

michalmarcinkowski Sep 28, 2016

		@@ -440,7 +441,10 @@ public function thisOrderIsAlreadyPaid(OrderInterface $order)

		/**
		* @Given /^the customer canceled (this order)$/

		$variant = $orderItem->getVariant();

		if ($variant->isTracked()) {

		$variant->getOnHold()->willReturn(10);
		$variant->getOnHand()->willReturn(10);

[Inventory] Inventory back to stock after order cancellation #6192

[Inventory] Inventory back to stock after order cancellation #6192

Conversation

tuka217 commented Sep 23, 2016

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 Sep 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalmarcinkowski commented Sep 23, 2016

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 Sep 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuka217 Sep 27, 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

pamil Sep 28, 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 Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

michalmarcinkowski commented Sep 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalmarcinkowski Sep 23, 2016 •

edited

Loading

michalmarcinkowski Sep 27, 2016 •

edited

Loading

tuka217 Sep 27, 2016 •

edited

Loading

pamil Sep 28, 2016 •

edited

Loading

pamil Sep 28, 2016 •

edited

Loading