I want to be informed about every item being out of stock #17

bartoszpietrzak1994 · 2018-06-12T09:44:48Z

    Scenario: Reordering previously placed order when one of items is out of stock
        Given the product "Angel T-Shirt" is out of stock
        When I browse my orders
        And I click reorder button next to the order "#00000666"
        Then I should be on my cart summary page
        And I should be notified that product "Angel T-Shirt" is out of stock
        And I should be notified that previous order total was "$59.00"
        And I should see exactly 2 notifications

    Scenario: Reordering previously placed order when several items are out of stock
        Given the product "Angel T-Shirt" is out of stock
        Given the product "Awesome Mug" is out of stock
        When I browse my orders
        And I click reorder button next to the order "#00000666"
        Then I should be on my cart summary page
        And I should be notified that products "Angel T-Shirt", "Awesome Mug" are out of stock
        And I should be notified that previous order total was "$59.00"
        And I should see exactly 2 notifications

Scenarios fulfilled

GSadee · 2018-06-12T10:11:25Z

features/being_notified_about_changes_in_order.feature

+        Given the store operates on a single channel in the "United States" named "Web"
+        And the store has a product "Angel T-Shirt" priced at "$39.00"
+        And there are 25 units of product "Angel T-Shirt" available in the inventory
+        And this product is tracked by the inventory


I guess that there is no need for this step, because the product is set as tracked in the previous step

GSadee · 2018-06-12T10:11:47Z

features/being_notified_about_changes_in_order.feature

+    @ui
+    Scenario: Reordering previously placed order when several items are out of stock
+        Given the product "Angel T-Shirt" is out of stock
+        Given the product "Awesome Mug" is out of stock


GSadee · 2018-06-12T10:14:17Z

features/reordering_previously_placed_order.feature

@@ -25,38 +27,15 @@ Feature: Reordering previously placed order
        Then I should be on my cart summary page
        And I should see "Angel T-Shirt" with quantity 1 in my cart
        And my cart total should be "$19.00"
+        And I should see exactly 0 notifications


And I should not see any notifications or sth similar?

GSadee · 2018-06-12T10:19:15Z

spec/ReorderEligibility/ItemsOutOfStockEligibilityCheckerSpec.php

+
+final class ItemsOutOfStockEligibilityCheckerSpec extends ObjectBehavior
+{
+    function let(ReorderEligibilityConstraintMessageFormatterInterface $reorderEligibilityConstraintMessageFormatter)


Missing return types here and in other functions of this class

GSadee · 2018-06-12T10:21:51Z

src/Factory/OrderFactory.php

@@ -75,6 +76,13 @@ private function copyOrderItemsToReorder(OrderInterface $order, OrderInterface $

        /** @var OrderItemInterface $orderItem */
        foreach ($orderItems as $orderItem) {
+


Unnecessary blank line

GSadee · 2018-06-12T10:24:34Z

src/ReorderEligibility/ReorderEligibilityChecker.php

+     * @param OrderInterface $reorder
+     * @return mixed
+     */
+    public function check(OrderInterface $order, OrderInterface $reorder);


Missing return type

GSadee · 2018-06-12T10:24:48Z

src/ReorderEligibility/ReorderEligibilityChecker.php

+     * @param OrderInterface $order
+     * @param OrderInterface $reorder
+     * @return mixed
+     */


Unnecessary docblock

src/ReorderEligibility/ReorderEligibilityConstraintMessageFormatter.php

+        $message = '';
+
+        if (count($messageParameters) === 1) {
+            $message = array_pop($messageParameters);


GSadee · 2018-06-12T10:26:50Z

src/Resources/translations/flashes.en.yml

-        promotion_not_enabled: 'Some promotions are no longer enabled. It may have affected order total.'
+        items_price_changed: 'Prices of products: %product_names% have changed, which have affected order total.'
+        promotion_not_enabled: 'Following promotions: %promotion_names% are no longer enabled, which have affected order total.'
+        items_out_of_stock: 'Following items: %order_items% are out of stock. It may have affected order total.'


Wrong order of keys

GSadee · 2018-06-12T12:00:48Z

src/Factory/OrderFactory.php

@@ -75,6 +76,12 @@ private function copyOrderItemsToReorder(OrderInterface $order, OrderInterface $

        /** @var OrderItemInterface $orderItem */
        foreach ($orderItems as $orderItem) {
+            /** @var ProductVariantInterface $productVariant */
+            $productVariant = $orderItem->getVariant();
+            if (!($productVariant->isTracked() && $productVariant->isInStock())) {


IMO this if should look like: if ($productVariant->isTracked() && !$productVariant->isInStock())

Zales0123 · 2018-06-13T07:29:39Z

src/ReorderEligibility/ItemsOutOfStockEligibilityChecker.php

+            'message' => 'sylius.reorder.items_out_of_stock',
+            'parameters' => [
+                '%order_items%' => $this->reorderEligibilityConstraintMessageFormatter->format($variantsOutOfStock),
+            ]


I'm wondering does ReorderEligibilityChecker should return a serialized flash message... It's against SRP principle for me. What I would expect from such a service is returning boolean flag or event better - throwing an exception if something is wrong. Then these exceptions can be caught where the main checker is used and passed to some "notifier" that would create a flash message basing on an exception.

Zales0123 · 2018-06-13T07:30:12Z

src/ReorderEligibility/ItemsOutOfStockEligibilityChecker.php

+            /** @var ProductVariantInterface $productVariant */
+            $productVariant = $orderItem->getVariant();
+            if ($productVariant->isTracked() && !$productVariant->isInStock()) {
+                array_push($variantsOutOfStock, $orderItem->getVariantName());


The preferred way to add something to an array is $variantsOutOfStock[] = $orderItem->getVariantName()

Zales0123 · 2018-06-13T07:34:33Z

src/ReorderEligibility/ReorderItemPricesEligibilityChecker.php

+        $orderVariantsWithChangedPrice = [];
+
+        foreach (array_keys($orderVariantNamesToTotal) as $variantName) {
+            if (!array_key_exists($variantName, $reorderVariantNamesToTotal)) {


!isset($variantName, $reorderVariantNamesToTotal)

I want to check if the key exists in the array, not if the value for given key is set.

Zales0123 · 2018-06-15T14:03:54Z

src/ReorderEligibility/ReorderEligibilityCheckerResponse.php

+
+namespace Sylius\CustomerReorderPlugin\ReorderEligibility;
+
+class ReorderEligibilityCheckerResponse


I think we've misunderstood about this class a little bit. What I was thinking about was:

Class EligibilityCheckerResult which contains some message what has gone wrong (and maybe a status? success/failure?)

Each checkers returns array/collection of EligibilityCheckerResults

Composite checker merges all responses into returns them to the controller

We need one more service that would convert each result into the flash message

And that's all. I didn't dig deep into these ReorderEligibilityCheckerResponseProcessors but I think they're just not needed. I think we should discuss it in live conversation.

GSadee · 2018-06-19T09:42:07Z

spec/Reorder/ReordererSpec.php

+        $this->reorder($order, $channel);
+    }
+
+    function it_checks_if_price_of_any_item_has_changed(


Try to keep the order of parameters from let method here and in other method in spec files

GSadee · 2018-06-19T09:46:21Z

spec/ReorderEligibility/ReorderItemPricesEligibilityCheckerSpec.php

@@ -51,7 +52,8 @@ function it_returns_empty_array_when_prices_are_the_same(
        $secondOrderItem->getVariantName()->willReturn('test_variant_name_02');
        $secondOrderItem->getUnitPrice()->willReturn(100);

-        $this->check($order, $reorder)->shouldReturn([]);
+        $response = $this->check($order, $reorder);


Why did you change it?

GSadee · 2018-06-19T09:46:52Z

spec/ReorderEligibility/ReorderItemPricesEligibilityCheckerSpec.php

-            'parameters' => [
-                '%product_names%' => 'test_variant_name_01, test_variant_name_02'
-            ]
+        $response = $this->check($order, $reorder);


GSadee · 2018-06-19T10:03:52Z

Thanks Bartek 🤘

bartoszpietrzak1994 added 11 commits June 12, 2018 11:41

Behat tests up and passing

7e29310

Improved behat tests and flash messages

dc622bb

Additional notifications count check

bd42714

Slight steps refactoring

f542dbe

Additional test + improvements

09026d1

First part of remaining eligibility checks specs

de1c170

Remaining specs

81d49c5

Build fixes

93e0fb4

Rebasing with master + fixes

fefa87c

Additional build fixes

6dfdb0d

Self review fixes

4c45e62

GSadee reviewed Jun 12, 2018

View reviewed changes

bartoszpietrzak1994 force-pushed the i-want-to-be-informed-about-every-item-being-out-of-stock branch from 535bce6 to ac1131a Compare June 12, 2018 11:54

GSadee reviewed Jun 12, 2018

View reviewed changes

bartoszpietrzak1994 force-pushed the i-want-to-be-informed-about-every-item-being-out-of-stock branch 2 times, most recently from 98331f7 to 0c0c3e2 Compare June 12, 2018 12:20

Review fixes

eef1d4d

bartoszpietrzak1994 force-pushed the i-want-to-be-informed-about-every-item-being-out-of-stock branch from 0c0c3e2 to eef1d4d Compare June 12, 2018 12:29

GSadee approved these changes Jun 13, 2018

View reviewed changes

Zales0123 reviewed Jun 13, 2018

View reviewed changes

bartoszpietrzak1994 added 4 commits June 13, 2018 16:35

Implementation of EligibilityCheckerResponseProcessors

e7bde86

Fixed specs

5c649a5

Remaining specs and existing fixes

6eb0101

Review fixes

3986a2f

Zales0123 suggested changes Jun 15, 2018

View reviewed changes

bartoszpietrzak1994 added 3 commits June 18, 2018 14:21

Review fixes

56477e4

Spec fixes and remaining tests

6101150

Remaining processor spec test

f56872b

GSadee approved these changes Jun 19, 2018

View reviewed changes

GSadee merged commit 86313fe into Sylius:master Jun 19, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

I want to be informed about every item being out of stock #17

I want to be informed about every item being out of stock #17

bartoszpietrzak1994 commented Jun 12, 2018

GSadee Jun 12, 2018

GSadee Jun 12, 2018

GSadee Jun 12, 2018

GSadee Jun 12, 2018

GSadee Jun 12, 2018

GSadee Jun 12, 2018

GSadee Jun 12, 2018

This comment was marked as outdated.

GSadee Jun 12, 2018

GSadee Jun 12, 2018

Zales0123 Jun 13, 2018

Zales0123 Jun 13, 2018

Zales0123 Jun 13, 2018

bartoszpietrzak1994 Jun 15, 2018

Zales0123 Jun 15, 2018

GSadee Jun 19, 2018

GSadee Jun 19, 2018

GSadee Jun 19, 2018

GSadee commented Jun 19, 2018

		@@ -75,6 +76,13 @@ private function copyOrderItemsToReorder(OrderInterface $order, OrderInterface $

		/** @var OrderItemInterface $orderItem */
		foreach ($orderItems as $orderItem) {


		namespace Sylius\CustomerReorderPlugin\ReorderEligibility;

		class ReorderEligibilityCheckerResponse

I want to be informed about every item being out of stock #17

I want to be informed about every item being out of stock #17

Conversation

bartoszpietrzak1994 commented Jun 12, 2018

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

This comment was marked as outdated.

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

GSadee commented Jun 19, 2018