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.

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

First draft changes to adapt the plugin #232

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ogomezba
Copy link
Contributor

@ogomezba ogomezba commented Aug 13, 2024

Changes required by the prestashop plugin to adapt to multiprice search engines.

Decisions

  • I've decided to keep the language as we did for the feed URLs but remove the currency.
  • We are not saving the currencies for the search engine anywhere as we use all currencies for all languages. Because we can obtain the list of currencies at any moment, this allows us to dynamically populate prices without keeping track of currencies anywhere.
  • We were saving the search engine Hash IDs per language-currency. We could keep this but it doesn't go well with the previous decision of dynamically obtaining the currencies at indexation time. Hence, I've also changed the format of the hash ID configs.

Open Items / Questions

  • How are we going to handle the retrocompatibility/deployment?
  • Is there any update needed in the script?
  • Is there any update needed due to changes in the Installation search engine mapping?
  • How are we going to determine which currency symbol to use? Does it have any impact on the plugin?
  • Is there any update on the response to the Doomanager create store request? How does the SE field change?

@ogomezba ogomezba self-assigned this Aug 13, 2024
@ogomezba ogomezba linked an issue Aug 13, 2024 that may be closed by this pull request
@@ -62,6 +62,7 @@

foreach (Language::getLanguages(true, $context->shop->id) as $lang) {
$lang = Tools::strtoupper($lang['iso_code']);
//TODO: Is this still needed?
$currency = dfTools::getCurrencyForLanguage($lang);
Copy link
Contributor

Choose a reason for hiding this comment

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

esto creo que es para cuando se hace una petición a la config del plugin que aparezca todos los idiomas y monedas que el cliente tiene configurados. Creo que está bien mantenerlo

*
* @return string
*/
public function getHashId($id_lang, $id_currency)
public function getHashId($id_lang)
Copy link
Contributor

Choose a reason for hiding this comment

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

creo que hay que tener cuidado con este cambio porque para los clientes que hagan una instalación nueva si que se les va a crear un search engine por idioma pero para los antiguos tenemos que seguir siendo capaces de buscar el hashid teniendo en cuenta la moneda. Esta función tiene que ser retrocompatible con una instalación "antigua" por lo que no podemos hacer este cambio directamente.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sí, lo mismo que comentaba para Magento, la idea de las PRs era verificar que la nueva forma de funcionar era viable. Cuando tengamos claro cómo lo vamos a hacer retrocompatible en general, el código deberá poder distinguir entre los dos casos.

echo ($product_price && $onsale_price && $product_price != $onsale_price)
? Tools::convertPrice($onsale_price, $currency) : '';

echo json_encode($prices) . TXT_SEPARATOR;
Copy link
Contributor

Choose a reason for hiding this comment

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

Te dejo por aquí lo que comentó José de cómo deberíamos devolver los precios en el caso de un fichero de texto:

id|title|pricesxxx|t1|price_EUR=33/sale_price_USD=20/price_rupia=33000/sale_price_EUR=22
```


$prices = [];

//TODO: What do we do when the prices should not be indexed?
Copy link
Contributor

Choose a reason for hiding this comment

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

ahora mismo si el show_price está deshabilitado lo que hace es mandar el campo price a false? o es que no se manda directamente ese campo en el csv?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahora mismo, sí el show_price está deshabilitado, no se manda el campo directamente

@@ -498,16 +496,19 @@ function array_merge_by_id_product($array1 = [], $array2 = [])
$product_price = dfTools::get_price($product_id, $cfg_prices_w_taxes);
$onsale_price = dfTools::get_onsale_price($product_id, $cfg_prices_w_taxes);

if (!$row['show_price']) {
$product_price = false;
$onsale_price = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

aquí pasa un poco igual que antes, no se si deberíamos sacar esta parte a una función y dependiendo de si el cliente esté utilizando el nuevo sistema que calcule el precio de la forma nueva pero que si es de los antiguos se mantenga el código antiguo

@@ -264,10 +262,13 @@ private function getAvailability($product)
}
}

private function getPrice($product, $salePrice = false)
private function getPrices($product)
Copy link
Contributor

Choose a reason for hiding this comment

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

y aquí lo mismo también

@ogomezba ogomezba force-pushed the ogomezba/dooplugins/614/cambios-multicurrency branch from b7f0984 to cce9947 Compare August 22, 2024 11:30
$lang_code = $lang['language_code'];
$feed_url = $this->buildFeedUrl($shopId, $lang['iso_code']);
$store_data['search_engines'][] = [
'language' => $lang_code,
Copy link
Contributor

Choose a reason for hiding this comment

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

habría que revisar cómo hacer con las monedas para poder generar las combinaciones de idiomas - monedas - search engine en el installation
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Prestashop] Analysis of search engine with multi currency: plugin
2 participants