[Order][OrderItem] Immutable product/variant names #8874

GSadee · 2017-10-19T13:50:37Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Related tickets	fixes #8628
License	MIT

It replaces a PR (#8667) created by @johnrisby using his squashed commits.

stefandoorn · 2017-10-19T13:54:54Z

It currently covers product + variant information. It would make sense to me also to write the underlying product option values with the variant. For my project they say way more than the variant name / code, as we have 1000s of variants per products and they just have something generated. The underlying naming says way more to the client and is also printed on invoices. Would be great to have that also added.

The basics now already implemented are also a must, so it's great to see it moving on :)

johnrisby · 2017-10-20T07:33:16Z

Thanks @GSadee!

GSadee · 2017-10-20T07:37:05Z

@johnrisby no problem and of course I hope you don't mind that I continue your work 😃

johnrisby · 2017-10-20T07:54:53Z

Not at all @GSadee, it's great. That's the whole point!

lchrusciel · 2017-10-24T08:12:21Z

app/migrations/Version20170913125128.php

+        $this->addSql('ALTER TABLE sylius_order_item ADD immutable_product_name VARCHAR(255), ADD immutable_product_code VARCHAR(255), ADD immutable_variant_name VARCHAR(255), ADD immutable_variant_code VARCHAR(255)');
+
+        $this->addSql('UPDATE sylius_order_item 
+                        INNER JOIN sylius_order AS o ON sylius_order_item.order_id = o.id 


It would much easier to follow the SQL logic if we would use some more appropriate names for aliases rather than abbreviate the table name

lchrusciel · 2017-10-24T08:13:33Z

app/migrations/Version20170913125128.php

+                        INNER JOIN sylius_product_variant AS pv ON sylius_order_item.variant_id = pv.id 
+                        INNER JOIN sylius_product_variant_translation AS pt ON pt.translatable_id = pv.id 
+                        INNER JOIN sylius_order AS ol ON pt.locale = ol.locale_code 
+                        SET sylius_order_item.immutable_variant_name =  pt.name,  sylius_order_item.immutable_variant_code = pv.code


Is it really worthy to add another SQL statement?

lchrusciel · 2017-10-24T08:15:42Z

src/Sylius/Behat/Page/Admin/Order/ShowPage.php

-            return 1 === count($rows);
+            foreach ($rows as $row) {
+                $field = $this->tableAccessor->getFieldFromRow($table, $row, 'item');
+                $name = $field->find('css', '.sylius-product-name');


Shouldn't we assert that the field would exist?

Not in this case, it would change the behaviour of this method.

lchrusciel · 2017-10-24T08:16:29Z

src/Sylius/Bundle/AdminBundle/Resources/views/Order/Show/Summary/_item.html.twig

@@ -6,7 +6,7 @@
 {% set taxAdjustment = constant('Sylius\\Component\\Core\\Model\\AdjustmentInterface::TAX_ADJUSTMENT') %}

 {% set variant = item.variant %}
-{% set product = variant.product %}
+{% set product = item.variant.product %}


What's the difference?

lchrusciel · 2017-10-24T08:19:08Z

src/Sylius/Bundle/AdminBundle/Resources/views/Product/_info.html.twig

-        <a href="{{ path('sylius_admin_product_update', {'id': product.id}) }}" class="sylius-product-name" title="{{ product.name }}">{{ product.name }}</a>
-        <span class="sub header sylius-product-variant-code" title="{{ variant.code }}">
-            {{ variant.code }}
+        <div class="sylius-product-name" title="{{ item.immutableProductName }}">{{ item.immutableProductName }}</div>


Do we need to add immutable prefix? It is set to order item, so it shouldn't change anyway.

Just thinking

lchrusciel · 2017-10-24T08:19:48Z

src/Sylius/Bundle/CoreBundle/Resources/config/doctrine/model/OrderItem.orm.xml

@@ -17,6 +17,11 @@
                                      http://doctrine-project.org/schemas/orm/doctrine-mapping.xsd">

    <mapped-superclass name="Sylius\Component\Core\Model\OrderItem" table="sylius_order_item">
+        <field name="immutableProductName" column="immutable_product_name" type="string" nullable="true"/>


I'm not sure if the product name on order item should be nullable. It is pretty important data

lchrusciel · 2017-10-24T08:20:36Z

src/Sylius/Component/Core/Model/OrderItem.php

+    /**
+     * {@inheritdoc}
+     */
+    public function setImmutableProductCode(?string $immutableProductCode): void


Maybe we could skip this new setter?

pamil · 2017-10-24T11:25:25Z

app/migrations/Version20170913125128.php

+                        INNER JOIN sylius_product AS p ON pv.product_id = p.id  
+                        INNER JOIN sylius_product_translation AS pt ON pt.translatable_id = p.id 
+                        INNER JOIN sylius_order AS ol ON pt.locale = ol.locale_code 
+                        SET sylius_order_item.product_name =  pt.name


Doubled space after = sign 🎉

pamil · 2017-10-25T13:48:21Z

app/migrations/Version20170913125128.php

+                        INNER JOIN sylius_product_variant AS pv ON sylius_order_item.variant_id = pv.id 
+                        INNER JOIN sylius_product_variant_translation AS pvt ON pvt.translatable_id = pv.id 
+                        INNER JOIN sylius_order AS ol ON pvt.locale = ol.locale_code 
+                        SET sylius_order_item.variant_name =  pvt.name


pamil · 2017-10-25T13:50:01Z

app/migrations/Version20170913125128.php

+                        INNER JOIN sylius_product_variant AS pv ON sylius_order_item.variant_id = pv.id 
+                        INNER JOIN sylius_product AS p ON pv.product_id = p.id  
+                        INNER JOIN sylius_product_translation AS pt ON pt.translatable_id = p.id 
+                        INNER JOIN sylius_order AS ol ON pt.locale = ol.locale_code 


Do we need to join sylius_order for the second time? Isn't WHERE enough then?

pamil · 2017-10-25T14:12:18Z

src/Sylius/Component/Core/Model/OrderItem.php

+    /**
+     * {@inheritdoc}
+     */
+    public function setVariant(?ProductVariantInterface $variant): void


Hmm, what if a product name changes after the product was added to the cart but before the checkout was completed?

Good point 👍 What is your suggestion? Should I set immutable names on checkout complete transition?

I've created OrderImmutableNamesSetter which is called on checkout complete transition.

lchrusciel · 2017-10-25T14:13:46Z

app/migrations/Version20170913125128.php

+        $this->addSql('ALTER TABLE sylius_order_item ADD product_name VARCHAR(255), ADD variant_name VARCHAR(255)');
+
+        $this->addSql('UPDATE sylius_order_item 
+                        INNER JOIN sylius_order AS o ON sylius_order_item.order_id = o.id 


Can we not abbreviate rest of table aliases. It is not easy to understand what happened here

pamil · 2017-10-26T13:53:32Z

src/Sylius/Component/Core/Order/OrderImmutableNamesSetterInterface.php

+    /**
+     * @param OrderInterface $order
+     */
+    public function setNames(OrderInterface $order): void;


This can be changed later (up to v1.1.0 release), so it's not a big deal, only this interface name looks weird.

OrderImmutableNamesSetterInterface::setNames -> OrderItemProductNamesSetterInterface::__invoke / OrderFinalizer::__invoke / something else?

pjedrzejewski · 2017-10-30T08:10:00Z

src/Sylius/Bundle/CoreBundle/Resources/config/app/state_machine/sylius_order_checkout.yml

@@ -63,3 +63,7 @@ winzou_state_machine:
                    on: ["complete"]
                    do: ["@sylius.state_resolver.order_shipping", "resolve"]
                    args: ["object"]
+                sylius_set_order_immutable_names:


We should move it to the main order state machine. This is not really specific to checkout, it should happen whenver we create a new order (cart -> new transition).

pjedrzejewski · 2017-10-30T11:40:10Z

Thank you John & Grzesiu, nice work guys! /cc @johnrisby

johnrisby · 2017-10-30T11:45:40Z

Great stuff. Sorry I've not contributed more to this recently but I've got a lot on. Thanks for picking up the baton Grzesiu!

GSadee added the New Feature label Oct 19, 2017

GSadee added this to the 1.1 milestone Oct 19, 2017

pjedrzejewski requested a review from pamil October 24, 2017 06:20

lchrusciel reviewed Oct 24, 2017

View reviewed changes

GSadee force-pushed the immutable-product-name branch from 2609a6a to 61f2419 Compare October 24, 2017 10:36

pamil reviewed Oct 25, 2017

View reviewed changes

lchrusciel reviewed Oct 25, 2017

View reviewed changes

GSadee force-pushed the immutable-product-name branch 2 times, most recently from 620a8fe to 3966d36 Compare October 26, 2017 06:49

GSadee changed the title ~~[Order][OrderItem] Immutable product/variant name and code~~ [Order][OrderItem] Immutable product/variant names Oct 26, 2017

GSadee mentioned this pull request Oct 26, 2017

#8628 Added immutable product/variant names and codes on OrderItem #8667

Closed

pamil approved these changes Oct 26, 2017

View reviewed changes

GSadee force-pushed the immutable-product-name branch from 8005059 to b3be998 Compare October 26, 2017 15:16

pamil added Feature New feature proposals. and removed New Feature labels Oct 27, 2017

pjedrzejewski requested changes Oct 30, 2017

View reviewed changes

johnrisby and others added 9 commits October 30, 2017 09:12

Sylius#8628 Added immutable product/variant names and codes on OrderItem

f21c034

[OrderItem] Refactor adding immutable product/variant names and codes

7f35652

[Behat][Order] Add scenarios for viewing items with proper names

3da58c3

[Order] Add displaying immutable product/variant names and codes in ui

3b0fb95

Fix unit tests by changing fixtures order

1bafcbe

[Order] Remove immutable product/variant code

1f9f443

[Order] Remove immutable prefix

003096c

Fixes after PR review

cdc9221

[Order] Add order product/variant names setter

bb52a56

GSadee force-pushed the immutable-product-name branch from b3be998 to bb52a56 Compare October 30, 2017 08:18

pjedrzejewski merged commit 78fd015 into Sylius:master Oct 30, 2017

GSadee deleted the immutable-product-name branch October 30, 2017 11:45

pamil mentioned this pull request Dec 4, 2017

[Order] Flattening #5534

Closed

stefandoorn mentioned this pull request Dec 4, 2017

[RFC] Immutable product variant option name / values #9005

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Order][OrderItem] Immutable product/variant names #8874

[Order][OrderItem] Immutable product/variant names #8874

GSadee commented Oct 19, 2017 •

edited

Loading

stefandoorn commented Oct 19, 2017

johnrisby commented Oct 20, 2017

GSadee commented Oct 20, 2017

johnrisby commented Oct 20, 2017

lchrusciel Oct 24, 2017

lchrusciel Oct 24, 2017

lchrusciel Oct 24, 2017

GSadee Oct 24, 2017

lchrusciel Oct 24, 2017

lchrusciel Oct 24, 2017

lchrusciel Oct 24, 2017

lchrusciel Oct 24, 2017

pamil Oct 24, 2017

pamil Oct 25, 2017

pamil Oct 25, 2017

pamil Oct 25, 2017

GSadee Oct 26, 2017

GSadee Oct 26, 2017

lchrusciel Oct 25, 2017

pamil Oct 26, 2017

pjedrzejewski Oct 30, 2017

pjedrzejewski commented Oct 30, 2017

johnrisby commented Oct 30, 2017

[Order][OrderItem] Immutable product/variant names #8874

[Order][OrderItem] Immutable product/variant names #8874

Conversation

GSadee commented Oct 19, 2017 • edited Loading

stefandoorn commented Oct 19, 2017

johnrisby commented Oct 20, 2017

GSadee commented Oct 20, 2017

johnrisby commented Oct 20, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjedrzejewski commented Oct 30, 2017

johnrisby commented Oct 30, 2017

GSadee commented Oct 19, 2017 •

edited

Loading