Fix product import for Product page v2. #28923

MeKeyCool · 2022-06-30T15:35:59Z

Questions	Answers
Branch?	8.0.x
Description?	Product page v2 display an error after a product import.
Type?	bug fix
Category?	BO
BC breaks?	no
Deprecations?	no
Fixed ticket?	Fixes #28169.
Related PRs	Na.
How to test?	Cf. #28169
Possible impacts?	Product creation.

❓ for @PrestaShop/prestashop-core-developers , the solution to create a new Product to use Product::getDynamicProductType() function according Product state isn't completely satisfying. But with @atomiix we didn't find an easy solution that won't imply deep refactoring. If anyone as a better solution please share :)

In order to fix product import for Product page v2, we

Remove default empty supplier creation during import.

Import calls legacy controller that creates some weird data with 0 ids (associative tables). Inducing wrong behaviors when trying to get consistent data with product page v2.
Fix getProductType for ProductType::TYPE_UNDEFINED

ProductType::TYPE_UNDEFINED is kept for legacy shop compatibility (existing databases, import old datas, ...) but a new function as been implemented to manage data consistency : Product::getDynamicProductType(). We searched a way to build the ProductType from CQRS Query request while product isn't instantiated yet and it's data are not satisfying new required structure.

matks

Import calls legacy controller that creates some weird data with 0 ids (associative tables). Inducing wrong behaviors when trying to get consistent data with product page v2.

PrestaShop 8.0.0 allows users to use product page v1 or v2. So what will happen for users of product page v1?

jolelievre

Thanks @MeKeyCool

One test should be added, and I'm not convinced about the modification in import, seems like it induces a regression

jolelievre · 2022-07-07T19:32:17Z

src/Adapter/Product/Repository/ProductRepository.php

@@ -301,6 +301,11 @@ public function getProductType(ProductId $productId): ProductType
            ));
        }

+        if ($result['product_type'] === ProductType::TYPE_UNDEFINED) {


That's a good initiative to make this method more resilient to absent data in the DB! I was afraid that maybe we were actually relying on the fact that this exception needs to be thrown in some cases but it doesn't seem to be the case.

To validate this new behavior and make sure we don't include some regression later we should enforce this with a test. This could be tested in behat test by creating a Product with an undefined type maybe in an integration test

After some investigations, I found those tests : https://github.com/PrestaShop/PrestaShop/blob/develop/tests/Integration/Behaviour/Features/Scenario/Product/legacy_product_type.feature
Isn't it enough ? What test is lacking for you ?

This test was already present before your fix, and yet a bug was happening when we requested the product's suppliers using the GetProductSupplierOptions query Which means this use case was not tested (trying to ftehc the suppliers while the type is undefined)

So our test coverage didn't handle this buggy use case, meaning the use case wasn't present and should be added, and you're right it should probably be tested in the feature you mentioned

Basically, you can add a test that tries and get the suppliers, if you remove your fix it should fail but with the fix it should be fine now

Just to be precise, this fix is not fixing initial problem, when we investigated initial issue, we found two different problems.

I shared a behat test but I am not well experienced with it so I expect you correct me if I did anything wrong ^^'

jolelievre · 2022-07-07T19:33:20Z

controllers/admin/AdminImportController.php

@@ -2104,17 +2104,14 @@ protected function productImportOne($info, $default_language_id, $id_lang, $forc
                $id_product_supplier = (int) ProductSupplier::getIdByProductAndSupplier((int) $product->id, 0, (int) $product->id_supplier);
                if ($id_product_supplier) {
                    $product_supplier = new ProductSupplier($id_product_supplier);
-                } else {
-                    $product_supplier = new ProductSupplier();
+                    $product_supplier->id_product = (int) $product->id;


I'm not sure about this modification, it means that importing a new supplier that doesn't exist will never work anymore so in most cases import of product supplier will remain empty..

@jolelievre that's actually what happens when you create a product with product page v1 when you don't specify a supplier, that's why we made that choice.

Hmm but I'm not sure it is the same use case 🤔 In product page we are editing a product which can have an existing ProductSupplier, in this case we are importing a product, so most of the time a new product so the chances are high that the ProductSupplier will never exist for this couple of id_product/id_supplier meaning if it's not present the association is never created

I have close to zero knowledge about the import feature, but it really feels like in this case the ProductSupplier can only be updated when it exists and can never created be created by import

@jolelievre I'm not sure to understand why it's different?
In product page you can also create a product without defining a product supplier so to me it's like importing without specifying a product supplier?
Also, if I remember well, you can have a product supplier during import, you just have to specify which one

I think the main thing to check here is if !empty($product->id_supplier) to make sure we don't create an association to a non existing supplier

But if the association doesn't exist (and that's the purpose of ProductSupplier::getIdByProductAndSupplier) we still need to create it (and not update it)

@jolelievre you're right, I didn't get it right 😅

I am a bit lost with this issue so I may be out of the subject.
But @jolelievre , isn't the test you are asking for done at line 2103 ?

if (!$validateOnly && !empty($product->id) && property_exists($product, 'supplier_reference')) {

Hmm, I'm not talking about the reference value but about the actual $product->id_supplier which stores the relation between the product and its default supplier

What I mean in this comment is:

with your modification, you only create the ProductSupplier when $id_product_supplier is not empty

$id_product_supplier is initialized with ProductSupplier::getIdByProductAndSupplier which searches for a relationship between the product and $product->id_supplier, meaning it will only return a value if the supplier is already associated to the product (ProductSupplier represents the association table between product and supplier)

so if the relationship isn't found (because it's not in DB) you don't create the ProductSupplier, meaning you don't create the relationship

so an inexistent relationship will never be created

so with you modify you can only modify/update an existing relationship

TLDR: if the relationship doesn't exist yet it's never created so the import is now unable to create a new relationship (it was able to do so with the previous code)

So that's why I'm blocking this modification because it introduces a regression

Now besides this modification which is not appropriate I discussed with @atomiix who worked with you on this bug fix and it seems like there was a bug because of this because a ProductSupplier instance was created but the id_supplier was 0 which is not accepted in the product page v2 because it tries to get a non-existent supplier (indeed no supplier can have an ID 0) But the problem comes from the fact that we created a ProductSuplier relationship with id_supplier being 0

This should be prevented, and to do so we need to check that $product->id_supplier is different from 0 before we try and create the ProductSupplier (and we don't even need to look for a relationship with ProductSupplier::getIdByProductAndSupplier since it doesn't make sense)

TLDR: you should add && !empty($product->id_supplier) on Line 2103, currently the test checks the supplier_reference value not the ID itself

MeKeyCool · 2022-07-20T05:59:06Z

Import calls legacy controller that creates some weird data with 0 ids (associative tables). Inducing wrong behaviors when trying to get consistent data with product page v2.

PrestaShop 8.0.0 allows users to use product page v1 or v2. So what will happen for users of product page v1?

@matks , do you mean that you suspect I didn't test enough ?
Would you be satisfied f I add some behat tests as suggested by @jolelievre ?
If you have some specific scenario in your mind, please share ^^

tests/Integration/Behaviour/Features/Context/LegacyProductFeatureContext.php

atomiix · 2022-08-23T08:27:19Z

src/Adapter/Product/Repository/ProductRepository.php

-            return new ProductType($result['product_type']);
+        // Older products that were created before product page v2, might have no type, so we determine it dynamically
+        // According Phpstan, "Strict comparison using === between mixed and '' will always evaluate to false." so we use == instead of ===
+        if (empty($result['product_type']) || ($result['product_type'] == ProductType::TYPE_UNDEFINED)) {


I think you should annotate the $result variable instead of using ==:

/** @var array<string, string> $result */ $result = $this->connection->createQueryBuilder() ...

Lol, in fact I was completely out of subject, the point was that ProductType::TYPE_UNDEFINED = '' (empty string) so empty($result['product_type']) already test this case 😅

atomiix

There's at least the method productAsToBeEditable that needs to be updated and I think productShouldBeEditable is a better name to be consistent with the other *should be* methods

atomiix · 2022-08-23T16:04:56Z

tests/Integration/Behaviour/Features/Context/LegacyProductFeatureContext.php

+     * @Then product :productName has to be editable
+     */
+    public function productAsToBeEditable($productName)


Suggested change

* @Then product :productName has to be editable

*/

public function productAsToBeEditable($productName)

* @Then product :productName should be editable

*/

public function productShouldBeEditable($productName)

atomiix · 2022-08-23T16:07:58Z

tests/Integration/Behaviour/Features/Scenario/Product/legacy_product_type.feature

+    Then there is a product "undefined_product" with name "Undefined Product"
+    And product "undefined_product" type should be standard
+    And product "undefined_product" persisted type should be undefined
+    And product "undefined_product" dynamic type should be standard
+    And product "Undefined Product" has to be editable


I know it's not what has been done with the tests just above but it make more sense because there is a product "undefined_product" with name "Undefined Product" doesn't check something, it sets something.

Suggested change

Then there is a product "undefined_product" with name "Undefined Product"

And product "undefined_product" type should be standard

And product "undefined_product" persisted type should be undefined

And product "undefined_product" dynamic type should be standard

And product "Undefined Product" has to be editable

And there is a product "undefined_product" with name "Undefined Product"

Then product "undefined_product" type should be standard

And product "undefined_product" persisted type should be undefined

And product "undefined_product" dynamic type should be standard

And product "Undefined Product" should be editable

atomiix · 2022-08-23T16:08:31Z

src/Adapter/Product/Repository/ProductRepository.php

@@ -309,7 +309,7 @@ public function getProductType(ProductId $productId): ProductType
                $productId->getValue()
            ));
        }
-
+        


Suggested change

- Remove default empty supplier creation during import. - Fix getProductType for `ProductType::TYPE_UNDEFINED`

jolelievre

Thanks @MeKeyCool

sallemiines · 2022-09-07T09:00:25Z

I will check this pr

sallemiines · 2022-09-08T13:57:37Z

Hello @MeKeyCool

The issu is solved
I run the automated tests OK
In the BO > Advanced settings > Import csv product without supplier
Go to BO > Catalog > V2 Products
Edit the imported product
The product is edited successfully
Pass an order with an imported product
Delete the product
In the BO > Advanced settings > Import csv product with supplier
Go to BO > Catalog > V2 Products
Edit the imported product
The product is edited successfully

PR-28923.mp4

Thank you !

matthieu-rolland · 2022-09-08T15:42:11Z

good job @MeKeyCool and @sLorenzini !

MeKeyCool requested a review from a team as a code owner June 30, 2022 15:35

prestonBot added 8.0.x Branch Bug fix Type: Bug fix labels Jun 30, 2022

jolelievre linked an issue Jun 30, 2022 that may be closed by this pull request

BO - An exception is displayed in the product page V2 when importing a product without supplier #28169

Closed

2 tasks

matks reviewed Jul 6, 2022

View reviewed changes

jolelievre requested changes Jul 7, 2022

View reviewed changes

MeKeyCool force-pushed the bugfix/28169_fix_product_page_v2_after_import branch 3 times, most recently from 2b986a3 to b91f71f Compare July 26, 2022 08:33

MeKeyCool requested review from atomiix, jolelievre and matks July 27, 2022 07:44

FabienPapet reviewed Jul 29, 2022

View reviewed changes

tests/Integration/Behaviour/Features/Context/LegacyProductFeatureContext.php Outdated Show resolved Hide resolved

MeKeyCool force-pushed the bugfix/28169_fix_product_page_v2_after_import branch from b91f71f to cbc3407 Compare August 1, 2022 07:54

Add behat test for product legacy import

8f42c99

MeKeyCool force-pushed the bugfix/28169_fix_product_page_v2_after_import branch from cbc3407 to 0b651bb Compare August 17, 2022 09:47

MeKeyCool requested a review from FabienPapet August 17, 2022 09:47

MeKeyCool force-pushed the bugfix/28169_fix_product_page_v2_after_import branch 2 times, most recently from 0652e8d to 35e7a8e Compare August 18, 2022 08:22

atomiix reviewed Aug 23, 2022

View reviewed changes

MeKeyCool force-pushed the bugfix/28169_fix_product_page_v2_after_import branch 2 times, most recently from b4000d6 to 1e24d7a Compare August 23, 2022 12:46

atomiix requested changes Aug 23, 2022

View reviewed changes

Fix product import for Product page v2.

5690e58

- Remove default empty supplier creation during import. - Fix getProductType for `ProductType::TYPE_UNDEFINED`

MeKeyCool force-pushed the bugfix/28169_fix_product_page_v2_after_import branch from 1e24d7a to 5690e58 Compare August 24, 2022 13:09

jolelievre approved these changes Aug 24, 2022

View reviewed changes

MeKeyCool requested review from atomiix and removed request for FabienPapet and matks August 24, 2022 23:10

atomiix approved these changes Aug 25, 2022

View reviewed changes

atomiix added the Waiting for QA Status: action required, waiting for test feedback label Aug 25, 2022

atomiix added this to the 8.0.0 milestone Aug 25, 2022

sLorenzini self-assigned this Sep 8, 2022

sLorenzini added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Sep 8, 2022

matthieu-rolland merged commit 20b3298 into PrestaShop:8.0.x Sep 8, 2022

MeKeyCool deleted the bugfix/28169_fix_product_page_v2_after_import branch September 8, 2022 16:29

khouloudbelguith assigned sallemiines Sep 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix product import for Product page v2. #28923

Fix product import for Product page v2. #28923

MeKeyCool commented Jun 30, 2022 •

edited

Loading

matks left a comment

jolelievre left a comment

jolelievre Jul 7, 2022

MeKeyCool Jul 21, 2022

jolelievre Jul 22, 2022

MeKeyCool Jul 26, 2022

jolelievre Jul 7, 2022

atomiix Jul 8, 2022

jolelievre Jul 11, 2022

atomiix Jul 11, 2022

jolelievre Jul 13, 2022

atomiix Jul 13, 2022

MeKeyCool Jul 20, 2022

jolelievre Jul 22, 2022

jolelievre Jul 22, 2022

jolelievre Jul 22, 2022

MeKeyCool commented Jul 20, 2022

atomiix Aug 23, 2022

MeKeyCool Aug 23, 2022

atomiix left a comment

atomiix Aug 23, 2022

atomiix Aug 23, 2022

atomiix Aug 23, 2022

jolelievre left a comment

sallemiines commented Sep 7, 2022

sallemiines commented Sep 8, 2022

matthieu-rolland commented Sep 8, 2022

Fix product import for Product page v2. #28923

Fix product import for Product page v2. #28923

Conversation

MeKeyCool commented Jun 30, 2022 • edited Loading

matks left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MeKeyCool commented Jul 20, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atomiix 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

jolelievre left a comment

Choose a reason for hiding this comment

sallemiines commented Sep 7, 2022

sallemiines commented Sep 8, 2022

matthieu-rolland commented Sep 8, 2022

MeKeyCool commented Jun 30, 2022 •

edited

Loading