Migrate attribute group edit #31502

JevgenijVisockij · 2023-02-23T07:40:45Z

Questions	Answers
Branch?	develop
Description?	Migrates attribute group form to symfony
Type?	refacto
Category?	BO
BC breaks?	no
Deprecations?	no
How to test?	Turn on attributes in experimental feature, the edit and add buttons in attribute groups list should now lead to new symfony form. Form should allow to create new attribute groups and edit existing ones, should work with multishop. IMPORTANT: Meta Title, URL and Indexable fields in attribute group from come from faceted search module and will not appear in this form. Those will need to be added on faceted search module side once this PR is merged. UI Tests: https://github.com/matthieu-rolland/ga.tests.ui.pr/actions/runs/6232728862
Fixed ticket?	#10508

matthieu-rolland

integration tests are red, they must be updated due to your migration I guess 👍

misclick I wanted to comment-review

src/Core/Domain/AttributeGroup/Command/AddAttributeGroupCommand.php

zuk3975 · 2023-03-02T09:35:49Z

src/Core/AttributeGroup/AttributeGroupSettings.php

+    public const ATTRIBUTE_GROUP_TYPE_SELECT = 'select';
+
+    public const ATTRIBUTE_GROUP_TYPE_RADIO = 'radio';
+
+    public const ATTRIBUTE_GROUP_TYPE_COLOR = 'color';


should introduce VO AttributeGroupType which would also validate itself in constructor

Indeed it can be interesting to have such VOs that ensures the limited choices of this field

Good idea! Implemented it

zuk3975 · 2023-03-02T09:38:26Z

src/Adapter/AttributeGroup/QueryHandler/GetAttributeGroupForEditingHandler.php

+        try {
+            $attributeGroup = new \AttributeGroup($attributeGroupId->getValue());
+        } catch (PrestaShopException $e) {
+            throw new AttributeGroupException('Failed to create new attribute group', 0, $e);
+        }
+
+        if ($attributeGroup->id !== $attributeGroupId->getValue()) {
+            throw new AttributeGroupNotFoundException(sprintf('Attribute group with id "%s" was not found.', $attributeGroupId->getValue()));
+        }
+
+        return $attributeGroup;


inject PrestaShop\PrestaShop\Adapter\AttributeGroup\Repository\AttributeGroupRepository
and a new method if it doesn't yet exist get(AttributeGroupId $attributeGroupId, ShopId $shopId): AttributeGroup and use it here instead

Oh right I didn't remember but a repository for this entity already exists

PrestaShop/src/Adapter/AttributeGroup/Repository/AttributeGroupRepository.php

Line 39 in d7ed7d6

class AttributeGroupRepository extends AbstractMultiShopObjectModelRepository

So you should use and improve it to match your needs here (creation, update, get entity), most of the code you are using here is actually factorized and mutualized vie the abstract module repository helper methods like getObjectModel

zuk3975 · 2023-03-02T09:40:54Z

src/Adapter/AttributeGroup/CommandHandler/EditAttributeGroupHandler.php

+            throw new AttributeGroupConstraintException('Invalid attribute group data', AttributeGroupConstraintException::INVALID_NAME);
+        }
+
+        if (false === $attributeGroup->update()) {


Im pretty sure this could also be used from repository. as well as the validations (same as in other subdomains like product) object model validator can check the fields it already has some methods encapsulated with correct exception throws etc.

Indeed you should use an ObjectModel repository here as well

src/Core/Domain/AttributeGroup/Query/GetAttributeGroupForEditing.php

Hlavtox · 2023-03-02T09:49:25Z

src/PrestaShopBundle/Resources/views/Admin/Sell/Catalog/AttributeGroup/create.html.twig

+ * @license   https://opensource.org/licenses/OSL-3.0 Open Software License (OSL 3.0)
+ *#}
+
+{% set layoutTitle = 'Add a new attribute group'|trans({}, 'Admin.Catalog.attribute') %}


Please move the layoutTitle to controller and name it New attribute to keep consistent with #31322, the wording is validated.

Good call, so @JevgenijVisockij this should be a template variable set in the controller when you are rendering the page

Hlavtox · 2023-03-02T09:49:50Z

src/PrestaShopBundle/Resources/views/Admin/Sell/Catalog/AttributeGroup/edit.html.twig

+ * @license   https://opensource.org/licenses/OSL-3.0 Open Software License (OSL 3.0)
+ *#}
+
+{% set layoutTitle = 'Edit: %name%'|trans({'%name%': attributeGroupName}, 'Admin.Actions') %}


Please move the layoutTitle to controller and name it Editing attribute %name% to keep consistent with #31322, the wording is validated.

Agreed, except it should be Aditing attribute group %name% here, this PR targets the AttributeGroup entity not the Attribute one

Hlavtox · 2023-03-02T09:50:43Z

src/PrestaShopBundle/Resources/views/Admin/Sell/Catalog/AttributeGroup/Blocks/form.html.twig

+
+  <div class="card">
+    <h3 class="card-header">
+      {{ 'Attribute Group'|trans({}, 'Admin.Catalog.Attribute') }}


{{ 'Attribute'|trans({}, 'Admin.Catalog.Attribute') }}

I think this new wording is more adapted, to understand the difference between Attribute group and its contained Attributes

So to clarify it should stay as Attribute group right?

@jolelievre @JevgenijVisockij @l-delin It has always been Attribute and Attribute value and I think it should stay like that.

It's called Attribute group only in the code and in the database.

zuk3975

Can we wait for @jolelievre review aswell? Im kinda not sure which things we want to require and which not.
I added couple comments which would follow the same principle as we recently done with product page, but don't rush with them.
I have couple doubts, e.g. if we need to use shop constraint for GetAttributeGroupCommand, or we should use the ShopId? because query will always need to get the values only from one shop, so the shopGroup and allShops constraints would become useless (futhermore the attributeGroup doesn't have "default shop id" in database as product does, so what would be the value of shopId when allshops or group shop constraint is given? (it would end up taking first random shop then?) . Anyway I think it may be nice to have a meetup and align before we start requesting different things. It would also apply to other migration PR's

zuk3975 · 2023-03-06T12:44:25Z

src/Adapter/AttributeGroup/CommandHandler/AddAttributeGroupHandler.php

+            throw new AttributeGroupConstraintException('Invalid attribute group data', AttributeGroupConstraintException::INVALID_NAME);
+        }
+
+        if (false === $attributeGroup->add()) {


this part should also be moved to a new repository method "create({all the required props}): Product"

zuk3975 · 2023-03-06T12:44:49Z

src/Core/Domain/AttributeGroup/Command/EditAttributeGroupCommand.php

+     *
+     * @throws AttributeGroupConstraintException
+     */
+    public function __construct(int $attributeGroupId, array $localizedNames, array $localizedPublicNames, string $type, array $shopAssociation = [])


split to multiline

src/Core/Domain/AttributeGroup/ValueObject/AttributeGroupId.php

src/Core/Form/IdentifiableObject/DataProvider/AttributeGroupFormDataProvider.php

zuk3975 · 2023-03-06T12:52:56Z

src/PrestaShopBundle/Resources/config/services/bundle/form/form_type.yml

@@ -537,6 +537,12 @@ services:

  PrestaShopBundle\Form\Admin\Type\AmountCurrencyType:

+  PrestaShopBundle\Form\Admin\Sell\Catalog\AttributeGroupType:
+    public: true


im pretty sure it can also be autowired and private

Private for sure, autowire why not it won't bring much since you still need to manually define $isMultistoreEnabled But it can anticipate future modifications

Romeved public true, but is autwire needed? there defaults autowire: true at the top of the file

oh if there is default autowire:true then its all good (didn't see that)

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

zuk3975

Ok so after brief discussion with @jolelievre you could address my comments. It is better if we implement those repositories/validators and shopId explicitly

jolelievre

Hi @JevgenijVisockij

actually unlike what I though initially and what @zuk3975 suggested, I don't think we need a shopId or a ShopConstraint inside the commands/queries for this Entity.

It's not like for product where the shop context changes the entity content completely (because many things are editable based on shop context and override many values). For this entity it's actually much simpler it only handles an association between the entity and the shops, but the content doesn't change. So you only need to handle in update (and potentially creation) to edit the list of associated shops.

jolelievre · 2023-03-06T14:17:44Z

admin-dev/themes/new-theme/js/pages/attribute-group/index.ts

+    ],
+  );
+
+  new window.prestashop.component.ChoiceTree('#attribute_group_shop_association').enableAutoCheckChildren();


Not blocking but could you move these selectors into a separate mapping file, please? It makes it easier to centralize them and modify them when a refacto/modification is needed

src/Adapter/AttributeGroup/CommandHandler/AddAttributeGroupHandler.php

zuk3975 · 2023-03-06T14:49:07Z

Hi @JevgenijVisockij

actually unlike what I though initially and what @zuk3975 suggested, I don't think we need a shopId or a ShopConstraint inside the commands/queries for this Entity.

It's not like for product where the shop context changes the entity content completely (because many things are editable based on shop context and override many values). For this entity it's actually much simpler it only handles an association between the entity and the shops, but the content doesn't change. So you only need to handle in update (and potentially creation) to edit the list of associated shops.

Ok sorry for confusion, I agree.
Attribute_group_shop only contains associations (so either group is assigned to shop or not, but values between shops are always the same) 🙇

jolelievre

Thanks @JevgenijVisockij

sorry for all the comments again 😅

jolelievre · 2023-03-06T14:25:36Z

src/Adapter/AttributeGroup/CommandHandler/EditAttributeGroupHandler.php

+            throw new AttributeGroupConstraintException('Invalid attribute group data', AttributeGroupConstraintException::INVALID_NAME);
+        }
+
+        if (false === $attributeGroup->update()) {


Indeed you should use an ObjectModel repository here as well

jolelievre · 2023-03-06T14:28:28Z

src/Adapter/AttributeGroup/QueryHandler/GetAttributeGroupForEditingHandler.php

+        try {
+            $attributeGroup = new \AttributeGroup($attributeGroupId->getValue());
+        } catch (PrestaShopException $e) {
+            throw new AttributeGroupException('Failed to create new attribute group', 0, $e);
+        }
+
+        if ($attributeGroup->id !== $attributeGroupId->getValue()) {
+            throw new AttributeGroupNotFoundException(sprintf('Attribute group with id "%s" was not found.', $attributeGroupId->getValue()));
+        }
+
+        return $attributeGroup;


Oh right I didn't remember but a repository for this entity already exists

PrestaShop/src/Adapter/AttributeGroup/Repository/AttributeGroupRepository.php

Line 39 in d7ed7d6

class AttributeGroupRepository extends AbstractMultiShopObjectModelRepository

So you should use and improve it to match your needs here (creation, update, get entity), most of the code you are using here is actually factorized and mutualized vie the abstract module repository helper methods like getObjectModel

jolelievre · 2023-03-06T14:30:28Z

src/Core/AttributeGroup/AttributeGroupSettings.php

+    public const ATTRIBUTE_GROUP_TYPE_SELECT = 'select';
+
+    public const ATTRIBUTE_GROUP_TYPE_RADIO = 'radio';
+
+    public const ATTRIBUTE_GROUP_TYPE_COLOR = 'color';


Indeed it can be interesting to have such VOs that ensures the limited choices of this field

src/Core/Domain/AttributeGroup/Command/AddAttributeGroupCommand.php

src/Core/Domain/AttributeGroup/Command/EditAttributeGroupCommand.php

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

src/Core/Domain/AttributeGroup/CommandHandler/EditAttributeGroupHandlerInterface.php

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

…roupFeatureContext.php Updated array definition in docs Co-authored-by: Julius Žukauskas <31609858+zuk3975@users.noreply.github.com>

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

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

src/PrestaShopBundle/Resources/config/services/bundle/form/form_type.yml

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

boherm

LGTM, thanks @JevgenijVisockij!
Just a small comment but I don't block your PR. ;)

boherm · 2023-09-18T08:12:10Z

src/Adapter/AttributeGroup/CommandHandler/AddAttributeGroupHandler.php

+    private $attributeGroupRepository;
+
+    public function __construct(AttributeGroupRepository $attributeGroupRepository)
+    {
+        $this->attributeGroupRepository = $attributeGroupRepository;
+    }


As we are in develop branch and we drop some PHP versions oldies, you can use constructor property promotion here (and in other handler classes) to simplify some lines.
Anyway, I'm not blocking your PR for this, but it can be a must have now in develop branch PRs! ;)

MatShir · 2023-09-19T07:44:16Z

Test UI and let's go !!!!!!

MatShir · 2023-09-19T14:35:04Z

@matthieu-rolland UI test are 🔴 :/

matthieu-rolland · 2023-09-19T15:33:32Z

and it's a merge ! thank you @JevgenijVisockij !

matks · 2023-09-19T19:16:51Z

7 months until merge 😄

JevgenijVisockij requested a review from a team as a code owner February 23, 2023 07:40

prestonBot added develop Branch Refactoring Type: Refactoring labels Feb 23, 2023

JevgenijVisockij changed the title ~~Migrate attribute view~~ Migrate attribute group edit Feb 23, 2023

JevgenijVisockij force-pushed the migrate-attribute-view branch from f622b04 to e01842f Compare February 23, 2023 13:49

matthieu-rolland previously approved these changes Feb 23, 2023

View reviewed changes

zuk3975 reviewed Mar 2, 2023

View reviewed changes

src/Core/Domain/AttributeGroup/Command/AddAttributeGroupCommand.php Show resolved Hide resolved

zuk3975 reviewed Mar 2, 2023

View reviewed changes

src/Core/Domain/AttributeGroup/Query/GetAttributeGroupForEditing.php Show resolved Hide resolved

Hlavtox reviewed Mar 2, 2023

View reviewed changes

zuk3975 requested changes Mar 2, 2023

View reviewed changes