Introduce single unified UpdateProductCommand #30031

zuk3975 · 2022-10-17T12:35:01Z

Questions	Answers
Branch?	develop
Description?	related POC's #29999, #30009, #30025. Current approach having multiple CQRS commands for product update (commands like UpdateProductBasicInformationCommand, UpdateProductPricesCommand, UpdateProductOptionsCommand, UpdateProductDetailsCommand, UpdateProductSeoCommand) has some flaws - user saves product once but it is actually fetched and saved couple times and the product update hooks fires multiple times too. This PR introduces single command for all these general properties update (except for all the association commands like setting categories, suppliers, features etc.). Integrated 2 commands for now: basic information and options. The new product_commands_builder (which builds the new command) tags are commented out, so it doesn't affect the product page yet. This approach should eventually replace the currently existing commands, but for now we are still keeping the old multi-command approach working in product page until this new one is fully implemented.
Type?	refacto
Category?	BO
BC breaks?	no
Deprecations?	no
Fixed ticket?
Related PRs
How to test?	see bellow
Possible impacts?

How to test
Locally it can be tested by uncomenting tags of product_commands_builder (in Resources/config/services/core/form/command_builder.yml prestashop.core.form.command_builder.product.update_product_commands_builder) and making sure that the UpdateProductCommand appears in the list of generated commands in Core/Form/IdentifiableObject/DataHandler/ProductFormDataHandler::update(). Also related behat scenarios can be run using these commands in terminal:

./vendor/bin/behat -c tests/Integration/Behaviour/behat.yml -s update_product --tags update-basic-information
./vendor/bin/behat -c tests/Integration/Behaviour/behat.yml -s update_product --tags update-multi-shop-basic-information
./vendor/bin/behat -c tests/Integration/Behaviour/behat.yml -s update_product --tags update-options
./vendor/bin/behat -c tests/Integration/Behaviour/behat.yml -s update_product --tags update-multi-shop-options

jolelievre

Thanks @zuk3975

A few comments but most of them are about naming and namespaces (and suggestions for additional tests ^^), but the generic principle of the whole PR seems good to me!

src/Adapter/Product/Update/ProductBasicInformationPropertyFiller.php

src/Adapter/Product/Update/ProductOptionsPropertyFiller.php

jolelievre · 2022-10-17T16:56:28Z

src/Adapter/Product/Update/ProductUpdatablePropertyFiller.php

+use PrestaShop\PrestaShop\Core\Domain\Product\Command\UpdateProductCommand;
+use Product;
+
+class ProductUpdatablePropertyFiller implements ProductUpdatablePropertyFillerInterface


You should add a description for this class explaining how it handles all the sub fillers

I'm thinking the naming is a bit too long maybe (same for the interface and related variables), maybe ProductFiller and ProductFillerInterface::fillProperties would be enough?

I would still like to emphasize that it returns the properties for update 🤔
So either a class or a method could mention the updatable props. (maybe the method?). Or you think thats not necessary?

jolelievre · 2022-10-17T16:58:30Z

src/Adapter/Product/Update/ProductOptionsPropertyFiller.php

+use PrestaShop\PrestaShop\Core\Domain\Product\Command\UpdateProductCommand;
+use Product;
+
+class ProductOptionsPropertyFiller implements ProductUpdatablePropertyFillerInterface


Please add descriptions on all the new class, even a simple one at least explaining which sub domain this filler handles

Also, I think these classes should be unit tested, their responsibility is quite simple and scope:

fill the fields

return array indicating which fields have been update

This makes them perfect candidates for unit testing, the two you handled don't have that many specific rules (although link_rewrite is not that intuitive) but other like SEO and Prices sub domains have some twisted rules sometimes which would be worth testing to avoid regressions.

It may be a bit in doublons with the behat tests, but it's not the same thing. Behat tests will validate the behaviour, while unit tests here would test our implementation.

src/Adapter/Product/Update/ProductUpdatablePropertyFillerInterface.php

...aviour/Features/Context/Domain/Product/UpdateProduct/AbstractUpdateOptionsFeatureContext.php

...n/Behaviour/Features/Context/Domain/Product/UpdateProduct/OptionsAssertionFeatureContext.php

...viour/Features/Context/Domain/Product/UpdateProduct/UpdateBasicInformationFeatureContext.php

...tion/Behaviour/Features/Context/Domain/Product/UpdateProduct/UpdateOptionsFeatureContext.php

src/Core/Form/IdentifiableObject/CommandBuilder/Product/UpdateProductCommandsBuilder.php

zuk3975 · 2022-10-19T11:54:10Z

tests/Unit/Adapter/Product/Update/Filler/ProductOptionsPropertyFillerTest.php

+            $product,
+            $command,
+            [
+                'available_for_order',


ping @jolelievre Idk if we should care to check if new value from command changes the previous one?
bcuz thats why this test case seems abit strange (by default $product->show_price and $product->available_for_order is true. So when we set AvailableForOrder to true, we are still updating the field (even though we are updating it with the same value). Same happens with other fields, its just more visible with show_price and available_for_order, because the $show_price value is the only one that is not updated if its not changed.

Not sure I understand everything 😅 we should discuss this live

…e for now

jolelievre

Thanks, @zuk3975

The whole architecture is fine for me, the only comments are about naming, we should discuss this live after next standup it'll be more efficient than via review comments

jolelievre · 2022-10-24T16:03:08Z

classes/shop/Shop.php

@@ -397,7 +397,7 @@ public static function initialize()
        );

        $isAllShop = 'all' === $id_shop;
-        $isApiInUse = defined('_PS_API_IN_USE_') && _PS_API_IN_USE_;
+        $isApiInUse = defined('_PS_API_IN_USE_') && _PS_API_IN_USE_ === true;


I'm not sure why this was needed since it's only use for Webservice context and the only value it can have so far true anyway

PrestaShop/webservice/dispatcher.php

Line 32 in 31bdccd

define('_PS_API_IN_USE_', true);

Is it really in your PR's scope?

oh, no I fixed it in another one. Forgot here, thanks 👍

jolelievre · 2022-10-24T16:05:38Z

src/Adapter/Product/Update/Filler/ProductBasicInformationPropertyFiller.php

+/**
+ * Fills product properties which can be considered as a basic product information
+ */
+class ProductBasicInformationPropertyFiller implements ProductUpdatablePropertyFillerInterface


Suggested change

class ProductBasicInformationPropertyFiller implements ProductUpdatablePropertyFillerInterface

class BasicInformationFiller implements ProductFillerInterface

I'm thinking of ways to reduce all the namings which are quite long:

the method name in the interface is already fillUpdatableProperties so the UpdateableProperty part can be removed in class names and interface

since we already are in product namespace having product prefix everywhere is a bit redundant (although I kept it in ProductFillterInterface to avoid confusion with future potential FillerInterface used for other entites

jolelievre · 2022-10-24T16:09:13Z

src/Adapter/Product/Update/Filler/ProductOptionsPropertyFiller.php

+/**
+ * Fills product properties which can be considered as product options
+ */
+class ProductOptionsPropertyFiller implements ProductUpdatablePropertyFillerInterface


Suggested change

class ProductOptionsPropertyFiller implements ProductUpdatablePropertyFillerInterface

class OptionsFiller implements ProductFillerInterface

jolelievre · 2022-10-24T16:09:37Z

src/Adapter/Product/Update/Filler/ProductUpdatablePropertyFiller.php

+ *
+ * All the internal property fillers are split just to contain less code and be more readable (because the Product contains many of properties).
+ */
+class ProductUpdatablePropertyFiller implements ProductUpdatablePropertyFillerInterface


Suggested change

class ProductUpdatablePropertyFiller implements ProductUpdatablePropertyFillerInterface

class ProductFiller implements ProductFillerInterface

jolelievre · 2022-10-24T16:09:51Z

src/Adapter/Product/Update/Filler/ProductUpdatablePropertyFillerInterface.php

+/**
+ * Responsible for filling up the Product with the properties which have to be updated
+ */
+interface ProductUpdatablePropertyFillerInterface


Suggested change

interface ProductUpdatablePropertyFillerInterface

interface ProductFillerInterface

jolelievre · 2022-10-24T16:12:46Z

src/PrestaShopBundle/Resources/config/services/adapter/product.yml

+  prestashop.adapter.product.update.product_updatable_property_filler:
+    class: PrestaShop\PrestaShop\Adapter\Product\Update\ProductUpdatablePropertyFiller
+    arguments:
+      - !tagged core.product_updatable_property_filler


I made some other comments regarding the interface naming, this tag renaming would make even more sense if the interface is renamed

jolelievre · 2022-10-24T16:18:54Z

tests/Unit/Adapter/Product/Update/Filler/ProductBasicInformationPropertyFillerTest.php

+use PrestaShop\PrestaShop\Adapter\Product\Update\Filler\ProductUpdatablePropertyFillerInterface;
+use PrestaShop\PrestaShop\Adapter\Tools;
+
+class ProductBasicInformationPropertyFillerTest extends PropertyFillerTestCase


Suggested change

class ProductBasicInformationPropertyFillerTest extends PropertyFillerTestCase

class BasicInformationFillerTest extends ProductFillerTestCase

jolelievre · 2022-10-24T16:19:11Z

tests/Unit/Adapter/Product/Update/Filler/PropertyFillerTestCase.php

+use PrestaShop\PrestaShop\Core\Domain\Shop\ValueObject\ShopConstraint;
+use Product;
+
+abstract class PropertyFillerTestCase extends TestCase


Suggested change

abstract class PropertyFillerTestCase extends TestCase

abstract class ProductFillerTestCase extends TestCase

jolelievre · 2022-10-24T16:21:59Z

tests/Unit/Adapter/Product/Update/Filler/PropertyFillerTestCase.php

+    abstract public function getDataForTestFillsUpdatableProperties(): iterable;
+
+    abstract public function getFiller(): ProductUpdatablePropertyFillerInterface;


Suggested change

abstract public function getDataForTestFillsUpdatableProperties(): iterable;

abstract public function getFiller(): ProductUpdatablePropertyFillerInterface;

abstract protected function getDataForTestFillsUpdatableProperties(): iterable;

abstract protected function getFiller(): ProductUpdatablePropertyFillerInterface;

These two could and should be protected no? Maybe getDataForTestFillsUpdatableProperties is a bit different since a data provider so I understood we may be forced to keep it public But for getFiller it seems we don't need to have it public, or am I missing something?

jolelievre · 2022-10-24T16:32:06Z

tests/Unit/Adapter/Product/Update/Filler/ProductOptionsPropertyFillerTest.php

+            $product,
+            $command,
+            [
+                'available_for_order',


Not sure I understand everything 😅 we should discuss this live

This reverts commit 1d1306a.

src/PrestaShopBundle/Resources/config/services/core/form/command_builder.yml

jolelievre

Thanks @zuk3975

FabienPapet · 2022-10-25T11:59:52Z

src/PrestaShopBundle/Resources/config/services/adapter/product.yml

+  prestashop.adapter.product.update.filler.product_filler:
+    class: PrestaShop\PrestaShop\Adapter\Product\Update\Filler\ProductFiller
+    arguments:
+      - !tagged core.product_filler


I Think the tag name should start by prestashop (like prestashop.core.product_filler)

I was insipired by core.product_command_builder so I didn't think too much.
I think its a question to @jolelievre (I see other older services using prestashop prefix like prestashop.core.command_bus) maybe indeed we shoul rename these 2 new concepts with the prestahop prefix too?

When we use core it implicitly relates to prestashop core, but I guess we can be more explicit on this one I don't mind you can replace it if you prefer

I don't care either, so as its not a blocker I prefer not to change anything now. But we can still do it when migrating other commands if decided

FabienPapet · 2022-10-25T14:42:20Z

src/Adapter/Product/Update/Filler/BasicInformationFiller.php

+/**
+ * Fills product properties which can be considered as a basic product information
+ */
+class BasicInformationFiller implements ProductFillerInterface


I don't understand the whole point of having *FillerInterface files and classes.
Why don't we fetch the product and map data inside ?

purely just to keep them small I guess. Cuz we will have like 5-6 of those classes (and we already had all the logic taken from dedicated command handlers that used it before).
Do you think its an overengineering?

The idea was indeed to avoid having one mega filler with 4000 lines of code, and to be able to work on remaining pieces more easily in multiple PRs. But maybe at the end this should be all done inside one, we could refacto this and merge them all into one We can discuss this tomorrow I guess

jolelievre · 2022-10-26T09:07:27Z

Thanks @zuk3975

zuk3975 requested a review from a team as a code owner October 17, 2022 12:35

prestonBot added develop Branch Refactoring Type: Refactoring labels Oct 17, 2022

jolelievre reviewed Oct 17, 2022

View reviewed changes

zuk3975 force-pushed the unify-product-command-final branch from 0be5a5b to 65e1e11 Compare October 18, 2022 14:17

zuk3975 commented Oct 19, 2022

View reviewed changes

zuk3975 added 16 commits October 19, 2022 16:27

introduce UpdateProductCommand with basic info and related filler

bbbf625

commandsBuilder for basic info

c1f28b5

add Option properties ot command and a filler

d61cb73

fill options in commands builder

592bb4c

behat split assertions and additional context for options

819b3ab

split behat contexts for updateBasicInformation

adebb0b

comment out product_commands_builder so it doesnt affect product pag…

6318d66

…e for now

fix yml style

7906ad0

introduce filler ns

8811651

add missing interfaces and some docs

8d7a16e

fix error code

72e38f4

rm unnecessary checks in commands builder

6c8f58b

unit test for UpdateProductCommandsBuilder

0a4ea42

unit for ProductBasicInformationfiller

d35acf1

add option property filler unit and some reusable methods

a8b9a36

assert expected product after filling properties

133d5a7

zuk3975 force-pushed the unify-product-command-final branch from e65014a to 133d5a7 Compare October 19, 2022 13:27

fix "Right side of && is always true." stan error

1d1306a

jolelievre reviewed Oct 24, 2022

View reviewed changes

zuk3975 added 2 commits October 25, 2022 10:13

Revert " fix "Right side of && is always true." stan error"

ec04bc9

This reverts commit 1d1306a.

rename fillers

9987796

zuk3975 force-pushed the unify-product-command-final branch from 4a56bb6 to 0c17d4c Compare October 25, 2022 08:13

jolelievre reviewed Oct 25, 2022

View reviewed changes

src/PrestaShopBundle/Resources/config/services/core/form/command_builder.yml Outdated Show resolved Hide resolved

add back the condition check

5e0e8f2

zuk3975 force-pushed the unify-product-command-final branch from 0c17d4c to 5e0e8f2 Compare October 25, 2022 09:09

clean filler tests and add test case for condition check

07e2bd8

zuk3975 force-pushed the unify-product-command-final branch from e386a01 to 07e2bd8 Compare October 25, 2022 09:26

jolelievre approved these changes Oct 25, 2022

View reviewed changes

FabienPapet reviewed Oct 25, 2022

View reviewed changes

FabienPapet approved these changes Oct 26, 2022

View reviewed changes

jolelievre merged commit 18be425 into PrestaShop:develop Oct 26, 2022

This was referenced Oct 26, 2022

UpdateProductCommand unification - prices related properties handling #30122

Merged

UpdateProductCommand unification - SEO related properties handling #30143

Merged

This was referenced Nov 3, 2022

UpdateProductCommand unification - Details related properties handling #30197

Merged

UpdateProductCommand unification - Shipping related properties handling #30228

Merged

matks added the migration symfony migration project label Nov 9, 2022

matks added this to the 8.1.0 milestone Nov 9, 2022

zuk3975 mentioned this pull request Nov 21, 2022

UpdateProductCommand unification - handle stock properties #30360

Merged

M0rgan01 mentioned this pull request Jul 27, 2023

Option "Force update of friendly URL" doesn't work #33432

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce single unified UpdateProductCommand #30031

Introduce single unified UpdateProductCommand #30031

zuk3975 commented Oct 17, 2022

jolelievre left a comment

jolelievre Oct 17, 2022

jolelievre Oct 17, 2022

zuk3975 Oct 18, 2022 •

edited

jolelievre Oct 17, 2022

jolelievre Oct 17, 2022

zuk3975 Oct 19, 2022

jolelievre Oct 24, 2022

jolelievre left a comment

jolelievre Oct 24, 2022

zuk3975 Oct 25, 2022

jolelievre Oct 24, 2022

jolelievre Oct 24, 2022

jolelievre Oct 24, 2022

jolelievre Oct 24, 2022

jolelievre Oct 24, 2022

jolelievre Oct 24, 2022

jolelievre Oct 24, 2022

jolelievre Oct 24, 2022

jolelievre Oct 24, 2022

jolelievre Oct 24, 2022

jolelievre left a comment

FabienPapet Oct 25, 2022

zuk3975 Oct 25, 2022 •

edited

jolelievre Oct 25, 2022

zuk3975 Oct 26, 2022

FabienPapet Oct 25, 2022

zuk3975 Oct 25, 2022

jolelievre Oct 25, 2022

jolelievre commented Oct 26, 2022

	class ProductBasicInformationPropertyFiller implements ProductUpdatablePropertyFillerInterface
	class BasicInformationFiller implements ProductFillerInterface

	class ProductOptionsPropertyFiller implements ProductUpdatablePropertyFillerInterface
	class OptionsFiller implements ProductFillerInterface

	class ProductUpdatablePropertyFiller implements ProductUpdatablePropertyFillerInterface
	class ProductFiller implements ProductFillerInterface

	interface ProductUpdatablePropertyFillerInterface
	interface ProductFillerInterface

	class ProductBasicInformationPropertyFillerTest extends PropertyFillerTestCase
	class BasicInformationFillerTest extends ProductFillerTestCase

	abstract class PropertyFillerTestCase extends TestCase
	abstract class ProductFillerTestCase extends TestCase

		abstract public function getDataForTestFillsUpdatableProperties(): iterable;

		abstract public function getFiller(): ProductUpdatablePropertyFillerInterface;

Introduce single unified UpdateProductCommand #30031

Introduce single unified UpdateProductCommand #30031

Conversation

zuk3975 commented Oct 17, 2022

jolelievre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuk3975 Oct 18, 2022 • edited

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

jolelievre 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

Choose a reason for hiding this comment

jolelievre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuk3975 Oct 25, 2022 • edited

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

jolelievre commented Oct 26, 2022

zuk3975 Oct 18, 2022 •

edited

zuk3975 Oct 25, 2022 •

edited