Migrating attribute value page #32475

JevgenijVisockij · 2023-05-08T07:31:21Z

Questions	Answers
Branch?	develop
Description?	Migrates attribute value form to symfony
Type?	refacto
Category?	BO
BC breaks?	no
Deprecations?	no
How to test?	CI green and UI tests green, functional QA will be done in the last PR
UI Tests	https://github.com/jolelievre/ga.tests.ui.pr/actions/runs/7786548308
Fixed ticket?	Fix #33126

Blocked by PrestaShop/blockwishlist#226

zuk3975 · 2023-05-08T07:59:33Z

behats failing

matthieu-rolland

there's a conflict, maybe it's the cause of the behat failure? I let you check 👍

JevgenijVisockij · 2023-06-28T07:29:43Z

Blocked by PrestaShop/blockwishlist#226 since bug in blockwishlist causes exceptino during attribute value deletion.

admin-dev/themes/new-theme/js/pages/attribute/form/attribute-form-map.ts

src/PrestaShopBundle/Form/Admin/Sell/Catalog/AttributeType.php

src/Adapter/Attribute/CommandHandler/AddAttributeHandler.php

src/Adapter/Attribute/QueryHandler/GetAttributeForEditingHandler.php

src/Adapter/Attribute/Repository/AttributeRepository.php

src/Core/Domain/AttributeGroup/Attribute/QueryResult/EditableAttribute.php

src/PrestaShopBundle/Form/Admin/Sell/Catalog/AttributeType.php

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

tests/Integration/Behaviour/Features/Context/Domain/AttributeFeatureContext.php

matks · 2023-08-28T15:27:13Z

Argh 😞 a rebase is needed

matthieu-rolland · 2023-09-25T07:20:26Z

src/PrestaShopBundle/Controller/Admin/Sell/Catalog/AttributeController.php

@@ -306,9 +357,15 @@ private function getErrorMessages()
            'Admin.Notifications.Error'
        );

+        $wrongValueMessage = $this->trans(
+            'A field is invalid.',


Do we have an existing wording for when a field has a wrong value but we don't know what field it is ?

Not sure, but you can also "explode" the exception array here based on the const from the AttributeConstraintException class which would allow you to use specific error messages for each field instead

Hlavtox · 2023-11-24T22:58:57Z

Ping @JevgenijVisockij @matthieu-rolland

install-dev/data/xml/feature_flag.xml

jolelievre

Thanks @matthieu-rolland

Sorry for the HUUUGGE review, especially since most of the code was not done by you but there are many things that don't follow the conventions and should be improved I went through a thorough review though, so I shouldn't have any other comments for this one

admin-dev/themes/new-theme/js/pages/attribute/form/attribute-form-map.ts

install-dev/data/xml/feature_flag.xml

src/Adapter/Attribute/CommandHandler/EditAttributeHandler.php

tests/Integration/Behaviour/Features/Scenario/Attribute/attribute_management.feature

jolelievre · 2023-12-29T12:31:51Z

tests/Integration/Behaviour/Features/Context/Domain/AttributeGroupFeatureContext.php

@@ -56,7 +56,7 @@ public function createAttributeGroup(string $reference, TableNode $node): void
    {
        $data = $this->localizeByRows($node);

-        $attributeGroupId = $this->createAttributeGroupUsingCommand($data['name'], $data['public_name'], $data['type']);
+        $attributeGroupId = $this->createAttributeGroupUsingCommand([1 => $data['name']], [1 => $data['public_name']], $data['type']);


I don't understand why this was modified with hard coded values, but the initial implementation that handled localized values more accurately was better as it really allowed checking the multi-lang fields And the implementation was cleaner

well it was failing because it formerly didn't use the CQRS command, and then when using the CQRS command the expected format was not the same, I'll see how I can have it working with previous test implementation and current CQRS command

You should use multi-lang values like name[en-US] like in many other behat tests, coupled with the localizeByRows and similar methods this should be doable

This still doesn't rely on localized values 🤔

jolelievre · 2023-12-29T12:32:26Z

tests/Integration/Behaviour/Features/Scenario/AttributeGroup/attribute_group_management.feature

    And language with iso code "en" is the default one
-    And language "language2" with locale "fr-FR" exists
+    And I create attribute group "attributeGroup1" with specified properties:
+      | name        | Color        |


Previous implementation with actual localized values was better, this should be refactored/reverted a bit

this one appeared after rebase... I was not sure which one to keep

I don't know this is worth looking into it to understand why this was changed

But the modification is on this PR so it's unlikely this was done in another PR But the attribute group is a multi lang entity, so is its command so it should be tested with multiple languages

tests/Integration/Behaviour/behat.yml

src/Adapter/Attribute/CommandHandler/AddAttributeHandler.php

src/Adapter/Attribute/CommandHandler/EditAttributeHandler.php

src/Adapter/Attribute/Validate/AttributeValidator.php

src/Adapter/AttributeGroup/CommandHandler/EditAttributeGroupHandler.php

src/Adapter/AttributeGroup/QueryHandler/GetAttributeGroupForEditingHandler.php

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

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

tests/Integration/Behaviour/behat.yml

jolelievre · 2024-01-15T14:44:53Z

tests/Integration/Behaviour/Features/Scenario/AttributeGroup/attribute_group_management.feature

    And language with iso code "en" is the default one
-    And language "language2" with locale "fr-FR" exists
+    And I create attribute group "attributeGroup1" with specified properties:
+      | name        | Color        |


I don't know this is worth looking into it to understand why this was changed

But the modification is on this PR so it's unlikely this was done in another PR But the attribute group is a multi lang entity, so is its command so it should be tested with multiple languages

jolelievre · 2024-01-15T14:47:00Z

tests/Integration/Behaviour/Features/Context/Domain/AttributeGroupFeatureContext.php

@@ -56,7 +56,7 @@ public function createAttributeGroup(string $reference, TableNode $node): void
    {
        $data = $this->localizeByRows($node);

-        $attributeGroupId = $this->createAttributeGroupUsingCommand($data['name'], $data['public_name'], $data['type']);
+        $attributeGroupId = $this->createAttributeGroupUsingCommand([1 => $data['name']], [1 => $data['public_name']], $data['type']);


You should use multi-lang values like name[en-US] like in many other behat tests, coupled with the localizeByRows and similar methods this should be doable

jolelievre · 2024-01-22T08:48:52Z

tests/Integration/Behaviour/Features/Context/Domain/AttributeFeatureContext.php

+    /**
+     * @var int default shop id from configs
+     */
+    private $defaultShopId;
+
+    public function __construct()
+    {
+        $this->defaultShopId = CommonFeatureContext::getContainer()->get('prestashop.adapter.legacy.configuration')->get('PS_SHOP_DEFAULT');
+    }


Suggested change

/**

* @var int default shop id from configs

*/

private $defaultShopId;

public function __construct()

{

$this->defaultShopId = CommonFeatureContext::getContainer()->get('prestashop.adapter.legacy.configuration')->get('PS_SHOP_DEFAULT');

}

matks

Good for me

Nothing to add to @jolelievre review 👍

Just provided 2 little suggestions not blocking

matks · 2024-01-31T10:05:35Z

src/Adapter/Attribute/CommandHandler/EditAttributeHandler.php

+
+        if (null !== $command->getLocalizedValue()) {
+            $attribute->name = $command->getLocalizedValue();
+            //$propertiesToUpdate['name'] = array_keys($command->getLocalizedValue());


WIP? Or commented code to delete?

matks · 2024-01-31T10:08:17Z

src/Adapter/Domain/LocalizedObjectModelTrait.php

+
+trait LocalizedObjectModelTrait
+{
+    protected function fillLocalizedValues(ObjectModel $product, string $propertyName, array $localizedValues, array &$updatableProperties): void


Yes PR #34892 has been merged and file

PrestaShop/src/Adapter/Domain/LocalizedObjectModelTrait.php

Line 35 in d26dbeb

protected function fillLocalizedValues(ObjectModel $product, string $propertyName, array $localizedValues, array &$updatableProperties): void

exists on develop

…eatureContext.php Co-authored-by: Jonathan Lelievre <jo.lelievre@gmail.com>

…uteHandler

Co-authored-by: Jonathan Lelievre <jo.lelievre@gmail.com>

tests/Integration/Behaviour/behat.yml

…he others and handle partial update and better validation

…d redirection to All shops context has been fixed

…ndle multishop association field

JevgenijVisockij requested a review from a team as a code owner May 8, 2023 07:31

prestonBot added develop Branch Refactoring Type: Refactoring labels May 8, 2023

JevgenijVisockij mentioned this pull request May 8, 2023

[WIP] Migrating attribute page #32150

Closed

matthieu-rolland reviewed Jun 19, 2023

View reviewed changes

JevgenijVisockij force-pushed the migrate-view-attribute-value-fixed branch from 2823965 to 16cb986 Compare June 28, 2023 07:34

jolelievre reviewed Jun 28, 2023

View reviewed changes

jolelievre mentioned this pull request Jun 28, 2023

Attribute delete fix PrestaShop/blockwishlist#226

Merged

This was referenced Jul 6, 2023

Migration SF - Controllers #33135

Open

Finish Migrating Catalog > Attributes & Features > Attribute Group > Add / edit attribute value #33126

Closed

lartist force-pushed the migrate-view-attribute-value-fixed branch 3 times, most recently from aeb98d4 to 01ec887 Compare August 11, 2023 14:29

matthieu-rolland force-pushed the migrate-view-attribute-value-fixed branch from 01ec887 to b1864b4 Compare September 19, 2023 13:44

matthieu-rolland reviewed Sep 25, 2023

View reviewed changes

matthieu-rolland force-pushed the migrate-view-attribute-value-fixed branch 3 times, most recently from 1a512e1 to f2e66d7 Compare December 21, 2023 15:39

matthieu-rolland force-pushed the migrate-view-attribute-value-fixed branch from 383eb34 to 7cb7834 Compare December 28, 2023 16:28

M0rgan01 added this to the 9.0.0 milestone Dec 29, 2023

M0rgan01 reviewed Dec 29, 2023

View reviewed changes

install-dev/data/xml/feature_flag.xml Outdated Show resolved Hide resolved

jolelievre requested changes Dec 29, 2023

View reviewed changes

jolelievre reviewed Jan 15, 2024

View reviewed changes

jolelievre reviewed Jan 22, 2024

View reviewed changes

matthieu-rolland force-pushed the migrate-view-attribute-value-fixed branch from 26a0cf5 to 5db7330 Compare January 30, 2024 15:08

matks reviewed Jan 31, 2024

View reviewed changes

Matthieu Rolland and others added 16 commits February 5, 2024 15:39

make behat tests congruents with attribute create/edit refactors

6dbdbb8

add shop assocition tests for attribute creation and update

6681348

add invalid color behat test

55fad31

rename accesors, setters and properties for attribute names

6fb011a

Update tests/Integration/Behaviour/Features/Context/Domain/AttributeF…

cc16015

…eatureContext.php Co-authored-by: Jonathan Lelievre <jo.lelievre@gmail.com>

fix bugs related to name/value refactor

049d89e

put back attribute group in behat config file

dafc527

check invalid color exception

32542c8

remove update method from AttributeRepository (partial update exists)

143ef64

remove final and don't extend AbstractObjectModelHandler in AddAttrib…

a63f550

…uteHandler

Update src/Adapter/Attribute/CommandHandler/AddAttributeHandler.php

e91cc7b

Co-authored-by: Jonathan Lelievre <jo.lelievre@gmail.com>

use property promotion in attribute edit handler and validator

d9908e3

fix lang related behat bugs

6df6ed8

cs fixer

618852d

Handle attribute partial update tests in behat

f6d93e6

Correctly rename all value fields into name

1cab0e0

jolelievre force-pushed the migrate-view-attribute-value-fixed branch 2 times, most recently from 4cc2a7c to 332494d Compare February 5, 2024 15:06

jolelievre previously approved these changes Feb 5, 2024

View reviewed changes

tests/Integration/Behaviour/behat.yml Show resolved Hide resolved

jolelievre added 4 commits February 5, 2024 16:16

Improve attribute groups command handles to be more consistent with t…

efea104

…he others and handle partial update and better validation

Improve shop context building, it inclides associated shopIds now, an…

056cd41

…d redirection to All shops context has been fixed

Update integration of attribute and attribute group form to better ha…

469a69b

…ndle multishop association field

Update CSV export column name

68ab72a

jolelievre dismissed their stale review via 68ab72a February 5, 2024 15:16

jolelievre force-pushed the migrate-view-attribute-value-fixed branch from 332494d to 68ab72a Compare February 5, 2024 15:16

jolelievre approved these changes Feb 5, 2024

View reviewed changes

M0rgan01 approved these changes Feb 5, 2024

View reviewed changes

ps-jarvis added the Waiting for QA Status: action required, waiting for test feedback label Feb 5, 2024

jolelievre removed the Waiting for QA Status: action required, waiting for test feedback label Feb 5, 2024

jolelievre merged commit 25e0dda into PrestaShop:develop Feb 5, 2024
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrating attribute value page #32475

Migrating attribute value page #32475

JevgenijVisockij commented May 8, 2023 •

edited by jolelievre

zuk3975 commented May 8, 2023

matthieu-rolland left a comment

JevgenijVisockij commented Jun 28, 2023

matks commented Aug 28, 2023

matthieu-rolland Sep 25, 2023

jolelievre Dec 29, 2023

Hlavtox commented Nov 24, 2023

jolelievre left a comment

jolelievre Dec 29, 2023

matthieu-rolland Dec 29, 2023 •

edited

jolelievre Jan 15, 2024

jolelievre Jan 31, 2024

jolelievre Dec 29, 2023

matthieu-rolland Dec 29, 2023

jolelievre Jan 15, 2024

jolelievre Jan 15, 2024

jolelievre Jan 15, 2024

jolelievre Jan 22, 2024

matks left a comment

matks Jan 31, 2024

matks Jan 31, 2024

Migrating attribute value page #32475

Migrating attribute value page #32475

Conversation

JevgenijVisockij commented May 8, 2023 • edited by jolelievre

zuk3975 commented May 8, 2023

matthieu-rolland left a comment

Choose a reason for hiding this comment

JevgenijVisockij commented Jun 28, 2023

matks commented Aug 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hlavtox commented Nov 24, 2023

jolelievre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthieu-rolland Dec 29, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JevgenijVisockij commented May 8, 2023 •

edited by jolelievre

matthieu-rolland Dec 29, 2023 •

edited