COR-2038 change fetch product image URL logic in catalog cron sync #190

LiorGingi · 2022-04-25T09:00:43Z

No description provided.

DanielxC

Nice work!

DanielxC · 2022-05-02T14:25:09Z

Model/Sync/Catalog/Data.php

-                        if (isset($attr['type']) && $attr['type'] === 'image') {
-                            $baseUrl = $this->yotpoCoreConfig->getBaseUrl(UrlInterface::URL_TYPE_MEDIA);
-                            $data = $data ? $baseUrl . 'catalog/product' . $data : "";
+                        if ($key == 'group_name' && $value) {


This case can be added to the switch case with inner if condition

Can you please explain how?

Please see this suggestion comment:
https://github.com/YotpoLtd/magento2-module-core/pull/190/files#r867898027

DanielxC · 2022-05-02T14:29:28Z

Model/Sync/Catalog/Data.php

+                        if ($key == 'group_name' && $value) {
+                            $value = strtolower($value);
+                            $value = str_replace(' ', '_', $value);
+                            $value = preg_replace('/[^A-Za-z0-9_-]/', '-', $value);


What does this line do? It seems like everything regex search

It replaces everything that is not alphanumeric, underscore, or dash with a dash.

Thanks for the explanation. Is this the expected behaviour? do we want it to replace !@#$%^&*()+= these for example to dash?

Model/Sync/Catalog/Data.php

DanielxC · 2022-05-02T14:42:26Z

Model/Sync/Catalog/Data.php

+
+                            $method = $attr['method'];
+                            $itemValue = $this->$method($item, $configKey);
+                            $value = $itemValue ?: ($method == 'getProductPrice' ? 0.00 : $itemValue);


Maybe this can be reduced to one oneliner condition intead of nested condition

Model/Sync/Catalog/Data.php

…og/Helper/Image

DanielxC · 2022-05-09T11:11:22Z

Model/Sync/Catalog/Data.php

-
-                        if (isset($attr['type']) && $attr['type'] === 'url') {
-                            $data = $item->getProductUrl();
+                switch ($attributeKey) {


Suggested change

switch ($attributeKey) {

$attributeDataValue = '';

$attributeDetailsAttributeCode = '';

$attributeDetailsMethod = $attributeDetails['method'];

if (isset($attributeDetails['attr_code']) && $attributeDetails['attr_code']) {

$attributeDetailsAttributeCode = $attributeDetails['attr_code'];

}

$itemValue = $this->$attributeDetailsMethod($item, $attributeDetailsAttributeCode);

switch ($attributeKey) {

case 'gtins':

$attributeDataValue = $this->prepareGtinsData($attributeDetails, $item);

break;

case 'custom_properties':

$attributeDataValue = $this->prepareCustomProperties($attributeDetails, $item);

break;

case 'is_discontinued':

$attributeDataValue = false;

break;

case 'url':

$attributeDataValue = $item->getProductUrl();

break;

case 'image_url':

$attributeDataValue = $this->getProductImageUrl($item);

break;

case 'group_name':

if ($itemValue) {

$attributeDataValue = substr((string)$itemValue, 0, 100);

$attributeDataValue = strtolower($attributeDataValue);

$attributeDataValue = str_replace(' ', '_', $attributeDataValue);

$attributeDataValue = preg_replace('/[^A-Za-z0-9_-]/', '-', $attributeDataValue);

}

break;

default:

if (!$attributeDetails['default'] && isset($attributeDetails['method']) && $attributeDetails['method']) {

if ($itemValue) {

$attributeDataValue = $itemValue;

} elseif ($attributeDetailsMethod == 'getProductPrice') {

$attributeDataValue = 0.00;

} else {

$attributeDataValue = $itemValue;

}

}

}

$itemAttributesData[$attributeKey] = $attributeDataValue;

if (($attributeKey == 'custom_properties' || $attributeKey == 'gtins') && !$attributeDataValue) {

unset($itemAttributesData[$attributeKey]);

}

I don't agree with this change. You've basically extracted what we had in the default clause, but it's not relevant for all of the action types.
It looks better for sure, but it degregates performance.

I disagree with that. Because default clause shouldn't check for other cases. Its a bad practice.
But I agree on the performance concern. I created a new branch with a refactor commit so it will be better performance wise (and easier for you to read than a suggestion). Let me know what you think. We can also talk about it in a call.
7f554dc

DanielxC · 2022-05-09T11:20:18Z

Model/Sync/Catalog/Data.php

+                            if ($itemValue) {
+                                $value = $itemValue;
+                            } elseif ($method == 'getProductPrice') {
+                                $value = 0.00;
+                            } else {
+                                $value = $itemValue;
+                            }


I think we changed the logic now no? Isn't it should be:

Suggested change

if ($itemValue) {

$value = $itemValue;

} elseif ($method == 'getProductPrice') {

$value = 0.00;

} else {

$value = $itemValue;

}

if ($method == 'getProductPrice') {

$value = 0.00;

} else {

$value = $itemValue;

}

I think that your suggestion is the one changing the logic.
The one I wrote broke down the one-liner we had there.
According to what you wrote, all getProductPrice will get a value of 0.00.

You are right, my bad. maybe lets change it then to:

if (!$itemValue && $method == 'getProductPrice') { $value = 0.00; } else { $value = $itemValue; }

the three ifs condition is confusing. wdyt?

LiorGingi changed the title ~~COR-2038 fetch product image URL~~ COR-2038 change fetch product image URL logic in catalog cron sync Apr 25, 2022

DanielxC suggested changes May 2, 2022

View reviewed changes

LiorGingi added 2 commits May 8, 2022 00:38

enhance(Sync/Catalog/Data):added getting image URL from Magento/Catal…

da5d2b6

…og/Helper/Image

style(Sync/Catalog/Data#attributeMapping):changed else-if to switch-case

1203c01

LiorGingi force-pushed the COR-2038-fetch-product-image-url branch from 6d8150b to 1203c01 Compare May 7, 2022 21:39

DanielxC reviewed May 9, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

COR-2038 change fetch product image URL logic in catalog cron sync #190

COR-2038 change fetch product image URL logic in catalog cron sync #190

LiorGingi commented Apr 25, 2022

DanielxC left a comment

DanielxC May 2, 2022

LiorGingi May 7, 2022

DanielxC May 9, 2022

DanielxC May 2, 2022

LiorGingi May 7, 2022 •

edited

Loading

DanielxC May 9, 2022 •

edited

Loading

DanielxC May 2, 2022

DanielxC May 9, 2022

LiorGingi May 9, 2022

DanielxC May 11, 2022

DanielxC May 9, 2022

LiorGingi May 9, 2022

DanielxC May 11, 2022

-                switch ($attributeKey) {
+                $attributeDataValue = '';
+                $attributeDetailsAttributeCode = '';
+                $attributeDetailsMethod = $attributeDetails['method'];
+                if (isset($attributeDetails['attr_code']) && $attributeDetails['attr_code']) {
+                    $attributeDetailsAttributeCode = $attributeDetails['attr_code'];
+                }
+                $itemValue = $this->$attributeDetailsMethod($item, $attributeDetailsAttributeCode);
+                switch ($attributeKey) {
+                    case 'gtins':
+                        $attributeDataValue = $this->prepareGtinsData($attributeDetails, $item);
+                        break;
+                    case 'custom_properties':
+                        $attributeDataValue = $this->prepareCustomProperties($attributeDetails, $item);
+                        break;
+                    case 'is_discontinued':
+                        $attributeDataValue = false;
+                        break;
+                    case 'url':
+                        $attributeDataValue = $item->getProductUrl();
+                        break;
+                    case 'image_url':
+                        $attributeDataValue = $this->getProductImageUrl($item);
+                        break;
+                    case 'group_name':
+                        if ($itemValue) {
+                            $attributeDataValue = substr((string)$itemValue, 0, 100);
+                            $attributeDataValue = strtolower($attributeDataValue);
+                            $attributeDataValue = str_replace(' ', '_', $attributeDataValue);
+                            $attributeDataValue = preg_replace('/[^A-Za-z0-9_-]/', '-', $attributeDataValue);
+                        }
+                        break;
+                    default:
+                        if (!$attributeDetails['default'] && isset($attributeDetails['method']) && $attributeDetails['method']) {
+                            if ($itemValue) {
+                                $attributeDataValue = $itemValue;
+                            } elseif ($attributeDetailsMethod == 'getProductPrice') {
+                                $attributeDataValue = 0.00;
+                            } else {
+                                $attributeDataValue = $itemValue;
+                            }
+                        }
+                }
+                $itemAttributesData[$attributeKey] = $attributeDataValue;
+                if (($attributeKey == 'custom_properties' || $attributeKey == 'gtins') && !$attributeDataValue) {
+                    unset($itemAttributesData[$attributeKey]);
+                }

COR-2038 change fetch product image URL logic in catalog cron sync #190

Are you sure you want to change the base?

COR-2038 change fetch product image URL logic in catalog cron sync #190

Conversation

LiorGingi commented Apr 25, 2022

DanielxC 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

LiorGingi May 7, 2022 • edited Loading

Choose a reason for hiding this comment

DanielxC May 9, 2022 • edited Loading

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

LiorGingi May 7, 2022 •

edited

Loading

DanielxC May 9, 2022 •

edited

Loading