FEATURE: merge images from child to configurable #24
Conversation
…e product by akeneo import, plus define which image attributes should be used for product image in Oro
There was a problem hiding this comment.
Hi @sylvainraye ,
thank you for your contributions, check my comments below please
Entity/AkeneoSettings.php
Outdated
| private $akeneoMergeImageToParent = false; | ||
|
|
||
| /** | ||
| * @var ParameterBagisAkeneoMergeImageToParent |
Entity/AkeneoSettings.php
Outdated
| * | ||
| * @return $this | ||
| */ | ||
| public function setAkeneoAttributesImageList($akeneoAttributesImageList) |
There was a problem hiding this comment.
Add typehint and return type please
Form/Type/AkeneoSettingsType.php
Outdated
| ] | ||
| ); | ||
|
|
||
| $transportData['akeneoAttributesList'] = str_replace(' ', '', $transportData['akeneoAttributesList']); | ||
| $transportData['akeneoAttributesImageList'] = str_replace(' ', '', $transportData['akeneoAttributesImageList']); |
There was a problem hiding this comment.
Requires validation like line above, see my comments in #22
| /** | ||
| * @var array|null | ||
| */ | ||
| private $attributesImageFilter = null; |
There was a problem hiding this comment.
Let's make it an array by default
| } | ||
| foreach ($item['values'] as $code => &$values) { | ||
|
|
||
| if (null === $this->attributesImageFilter || in_array($code, $this->attributesImageFilter)) { |
There was a problem hiding this comment.
@sylvainraye what's the reason to add a separate field to filter image attributes?
There was a problem hiding this comment.
Not sure that List of image attributes of Akeneo to use as main product picture. matches the current behavior.
This condition will skip images, but what if we have three different images for different purposes?
Main image configured in https://github.com/oroinc/OroAkeneoBundle/blob/3.1/ImportExport/DataConverter/ProductImageDataConverter.php#L15 and https://github.com/oroinc/OroAkeneoBundle/blob/3.1/ImportExport/Strategy/ProductImageImportStrategy.php#L72
There was a problem hiding this comment.
what's the reason to add a separate field to filter image attributes?
-> In akeneo you can have many pictures for different use cases: for print, for web, for anything else. In product, those image may be at product or product model level. Currently, the connector import all pictures of a product and set them as product image which is totally a non wanted behavior and may conduct to unexpected behavior. Example, a customer may have a shop running on Magento and on OroCommerce and may need to specify three different attributes for the different sizes in Magento, while in OroCommerce only one attribute could be used. It's also relevant for project migration (for one shop system to an other), well there are many use cases indeed. Here, we give the flexibility to set which attributes should be defined as product image.
Not sure that List of image attributes of Akeneo to use as main product picture. matches the current behavior.the term main picture is probably not appropriate, the logic here set images as Product images. It doesn't affect if an image will be a main image or a small one or else. I changed the wording. The current logic of your connector is that it takes the first image it found and set it as main product image. Which is also open for discussion regarding this logic. Hw it's an other topic that the PR doesn't cover.
Integration/AkeneoTransport.php
Outdated
| */ | ||
| public function getCategories(int $pageSize) | ||
| { | ||
| return $this->client->getCategoryApi()->all($pageSize); | ||
| $categoryTreeChannel = null; |
There was a problem hiding this comment.
Create a separate PR for this fix please
| && !isset($this->proceededParent[$identifier]) | ||
| ) { | ||
|
|
||
| $this->proceededParent[$identifier] = true; |
There was a problem hiding this comment.
Can you add some description how to get the same identifier imported twice here?
| price_list: | ||
| label: 'Pricelist' | ||
| merge_image: | ||
| label: 'Merge images from children products to parent' |
There was a problem hiding this comment.
Update this description please
children=simple
parent=configurable
| tooltip: 'This field enables you to apply filters to sync only the attributes you want. Values must be attribute code, separated with a semi-colon. IMPORTANT: if not defined before to save the integration, all attributes will be imported.' | ||
| akeneo_attribute_image_list: | ||
| label: 'Image Attributes Filter' | ||
| tooltip: 'List of image attributes of Akeneo to use as main product picture.' |
There was a problem hiding this comment.
Separator and validation descriptions are missing
… specific channel
…ier in loop hw it's open to discussion. Cleanup code
sylvainraye
changed the title
|
Hello, @sylvainraye , form updates are missing, so it's not possible to create an integration. Can we merge this PR with #22 also, to decrease code review and QA time? |
|
Fixed by 2a65477 |
No description provided.