Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

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

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
da5d2b6
enhance(Sync/Catalog/Data):added getting image URL from Magento/Catal…
LiorGingi Apr 25, 2022
1203c01
style(Sync/Catalog/Data#attributeMapping):changed else-if to switch-case
LiorGingi Apr 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
115 changes: 70 additions & 45 deletions Model/Sync/Catalog/Data.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace Yotpo\Core\Model\Sync\Catalog;

use Magento\Catalog\Helper\Image as CatalogImageHelper;
use Magento\CatalogInventory\Model\StockRegistry;
use Magento\Framework\Exception\LocalizedException;
use Magento\Framework\Exception\NoSuchEntityException;
Expand All @@ -20,6 +21,11 @@
*/
class Data extends Main
{
/**
* @var CatalogImageHelper
*/
private $catalogImageHelper;

/**
* @var YotpoCoreConfig
*/
Expand Down Expand Up @@ -53,7 +59,7 @@ class Data extends Main
/**
* @var array<string, array>
*/
protected $mappingAttributes = [
protected $attributesMapping = [
'row_id' => [
'default' => 1,
'attr_code' => 'row_id'
Expand Down Expand Up @@ -167,6 +173,7 @@ class Data extends Main

/**
* Data constructor.
* @param CatalogImageHelper $catalogImageHelper
* @param YotpoCoreConfig $yotpoCoreConfig
* @param YotpoResource $yotpoResource
* @param Configurable $resourceConfigurable
Expand All @@ -177,6 +184,7 @@ class Data extends Main
* @param YotpoCoreCatalogLogger $yotpoCatalogLogger
*/
public function __construct(
CatalogImageHelper $catalogImageHelper,
YotpoCoreConfig $yotpoCoreConfig,
YotpoResource $yotpoResource,
Configurable $resourceConfigurable,
Expand All @@ -186,13 +194,14 @@ public function __construct(
StockRegistry $stockRegistry,
YotpoCoreCatalogLogger $yotpoCatalogLogger
) {
$this->catalogImageHelper = $catalogImageHelper;
$this->yotpoCoreConfig = $yotpoCoreConfig;
$this->yotpoResource = $yotpoResource;
$this->resourceConfigurable = $resourceConfigurable;
$this->productRepository = $productRepository;
$this->collectionFactory = $collectionFactory;
$this->stockRegistry = $stockRegistry;
$this->mappingAttributes['row_id']['attr_code'] = $this->yotpoCoreConfig->getEavRowIdFieldName();
$this->attributesMapping['row_id']['attr_code'] = $this->yotpoCoreConfig->getEavRowIdFieldName();
$this->logger = $yotpoCatalogLogger;
parent::__construct($resourceConnection);
}
Expand Down Expand Up @@ -369,65 +378,69 @@ protected function prepareOptions($syncItems, $configIds, $productObjects)
*/
public function attributeMapping(Product $item)
{
$itemArray = [];
$mapAttributes = $this->mappingAttributes;
$itemAttributesData = [];

foreach ($mapAttributes as $key => $attr) {
foreach ($this->attributesMapping as $attributeKey => $attributeDetails) {
try {
if ($key === 'gtins') {
$value = $this->prepareGtinsData($attr, $item);
} elseif ($key === 'custom_properties') {
$value = $this->prepareCustomProperties($attr, $item);
} elseif ($key === 'is_discontinued') {
$value = false;
} else {
if ($attr['default']) {
$data = $item->getData($attr['attr_code']);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

case 'gtins':
$value = $this->prepareGtinsData($attributeDetails, $item);
break;
case 'custom_properties':
$value = $this->prepareCustomProperties($attributeDetails, $item);
break;
case 'is_discontinued':
$value = false;
break;
case 'url':
$value = $item->getProductUrl();
break;
case 'image_url':
$value = $this->getProductImageUrl($item);
break;
default:
if (!$attributeDetails['default'] && isset($attributeDetails['method']) && $attributeDetails['method']) {

$configKey = isset($attributeDetails['attr_code']) && $attributeDetails['attr_code'] ?
$attributeDetails['attr_code'] : '';

$method = $attributeDetails['method'];
$itemValue = $this->$method($item, $configKey);
if ($itemValue) {
$value = $itemValue;
} elseif ($method == 'getProductPrice') {
$value = 0.00;
} else {
$value = $itemValue;
}
Comment on lines +409 to +415
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

} else {
$value = '';
}

if (isset($attr['type']) && $attr['type'] === 'image') {
$baseUrl = $this->yotpoCoreConfig->getBaseUrl(UrlInterface::URL_TYPE_MEDIA);
$data = $data ? $baseUrl . 'catalog/product' . $data : "";
if ($attributeKey == 'group_name' && $value) {
$value = substr((string)$value, 0, 100);
$value = strtolower($value);
$value = str_replace(' ', '_', $value);
$value = preg_replace('/[^A-Za-z0-9_-]/', '-', $value);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@LiorGingi LiorGingi May 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@DanielxC DanielxC May 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
$value = $data;
} elseif (isset($attr['method']) && $attr['method']) {

$configKey = isset($attr['attr_code']) && $attr['attr_code'] ?
$attr['attr_code'] : '';

$method = $attr['method'];
$itemValue = $this->$method($item, $configKey);
$value = $itemValue ?: ($method == 'getProductPrice' ? 0.00 : $itemValue);
} else {
$value = '';
}
if ($key == 'group_name' && $value) {
$value = strtolower($value);
$value = str_replace(' ', '_', $value);
$value = preg_replace('/[^A-Za-z0-9_-]/', '-', $value);
$value = substr((string)$value, 0, 100);
}
}
$itemArray[$key] = $value;
if (($key == 'custom_properties' || $key == 'gtins') && !$value) {
unset($itemArray[$key]);

DanielxC marked this conversation as resolved.
Show resolved Hide resolved
$itemAttributesData[$attributeKey] = $value;
if (($attributeKey == 'custom_properties' || $attributeKey == 'gtins') && !$value) {
unset($itemAttributesData[$attributeKey]);
}
} catch (\Exception $e) {
$this->logger->info(
__(
'Exception raised within attributeMapping - $key: %1, $attr: %2 Exception Message: %3',
$key,
$attr,
$attributeKey,
$attributeDetails,
$e->getMessage()
)
);
}
}

return $itemArray;
return $itemAttributesData;
}

/**
Expand Down Expand Up @@ -618,4 +631,16 @@ private function filterNotConfigurableProducts($productIds)

return $filteredProductIds;
}

/**
* Get Product Image from product object
*
* @param Product $product
* @param string $imageId
* @return string|null
*/
private function getProductImageUrl($product, $imageId = 'product_page_image_large')
{
return $this->catalogImageHelper->init($product, $imageId)->getUrl();
}
}