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

Sofia/magento2_support_issues #334

Merged
merged 7 commits into from
Jul 29, 2024
Merged

Conversation

sofia-doofinder
Copy link
Member

Required by: https://github.com/doofinder/support/issues/2572
Required by: https://github.com/doofinder/support/issues/2569

Estamos teniendo algunos problemas con los update on save de magento.

  • Al indexar variantes llega que el tipo es simple y por lo tanto lo trata como un producto simple y le asigna su propio group_id por lo que se pierde la agrupación. Se ha modificado para que si se actualiza un producto variante se mande el update on save de su producto padre para que se actualice el padre y sus variantes.

  • Tenemos un nombramiento mezclado del cron job, en algunos sitios se llamaba como doofinder_config_config/update_on_save/cron_expression y en otros crontab/default/jobs/doofinder_update_on_save/schedule/cron_expr, se estaban creando dos entradas en base de datos cuando en realidad sólo necesitamos uno.
    image

  • Hay una incongruencia entre lo que indexamos en el df_variants_information y lo que después se indexa en la variante, en el producto padre estamos utilizando el label del atributo y en la variante el code, por lo que después no funciona el botón del add to cart correctamente. Se ha modificado para que a itemsTransformation le llegue este code y poder utilizarlo.

@sofia-doofinder sofia-doofinder changed the title Sofia/mixing cron naming Sofia/magento2_support_issues Jul 25, 2024
@sofia-doofinder sofia-doofinder requested review from pedromcp90 and removed request for davidmolinacano July 25, 2024 13:48
@sofia-doofinder sofia-doofinder self-assigned this Jul 25, 2024
$itemId = (int)$product->getId();

$parentProduct = $this->configurableProductType->getParentIdsByChild($itemId);
if (count($parentProduct) > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Es posible que un cambio en un producto hijo afecte al padre? Quizá cambiando el precio del grupo o algo así? Si fuera así, tendría sentido que realmente se enviaran al update on save no el primer padre sino todos los que contienen al item?

Copy link
Member Author

Choose a reason for hiding this comment

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

me parece buena idea @ogomezba, meto en base de datos todos los padres relacionados con ese producto, no sólo el primero.

Copy link
Contributor

@davidmolinacano davidmolinacano left a comment

Choose a reason for hiding this comment

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

Makes sense to me! 👏 🏆

Comment on lines 120 to 123
$parentProduct = $this->configurableProductType->getParentIdsByChild($itemId);
if (count($parentProduct) > 0) {
$itemsToInsert = $parentProduct;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Genial! Solamente un pequeño comentario, quizá es más claro llamar a la variable parentProducts ya que pueden ser varios?

Suggested change
$parentProduct = $this->configurableProductType->getParentIdsByChild($itemId);
if (count($parentProduct) > 0) {
$itemsToInsert = $parentProduct;
}
$parentProducts = $this->configurableProductType->getParentIdsByChild($itemId);
if (count($parentProducts) > 0) {
$itemsToInsert = $parentProducts;
}

Copy link
Contributor

@brunovesar brunovesar left a comment

Choose a reason for hiding this comment

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

Good job,
I have a couple of comments

@@ -53,7 +53,7 @@ public function uninstall(SchemaSetupInterface $setup, ModuleContextInterface $c
}
//remove cron entries
$collection = $this->collectionFactory->create();
$collection->addFieldToFilter('path', 'crontab/default/jobs/doofinder_update_on_save/schedule/cron_expr');
$collection->addFieldToFilter('path', 'doofinder_config_config/update_on_save/cron_expression');
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this key "doofinder_config_config" intended? why there are 2 config?

Copy link
Member Author

Choose a reason for hiding this comment

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

For some reason we were using two keys differents for the same approach, I've been doing some tests and seems that the valid is the defined here
image

so the other was inserted in the database but never used, however when uninstalling we were deleting the cronjob for the non-working but keeping the correct one.. so to avoid confusing I've keept the one that seems to be working and removed the other.

Comment on lines 119 to 123
$itemsToInsert = [$itemId];
$parentProduct = $this->configurableProductType->getParentIdsByChild($itemId);
if (count($parentProduct) > 0) {
$itemsToInsert = $parentProduct;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If has parents, we're not inserting the item?
Shouldn't we do an append?

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly, the issue here is that when a user edit a variant product now we're storing only the variant but not the parent, so the request is send to dooplugins and when we are going to manage the variant it is taken as a simple product instead of a variant and it is managed as an simple product, which mean that it is exclueded of the configurable product and if the customer has the feed configured as group variants now the variants appear in the search... It is because for magento a variant is a simple product but included in a group, it is quite weired, so to avoid this case when a variant product is updated we store its parent to be updated, and in dooplugins when we update this product we update it and its variants, so the product that was edited will be updated in the feed also.

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense then!
Maybe we could add a small comment alongside these lines to leave evidence of this behavior

/* When updating a product it's children are also updated, so in this case we don't need to specify the itemId */

Copy link
Member Author

Choose a reason for hiding this comment

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

great! thank you :)

Copy link
Member

@pedromcp90 pedromcp90 left a comment

Choose a reason for hiding this comment

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

Buen trabajo!

@sofia-doofinder sofia-doofinder merged commit cf5a59a into master Jul 29, 2024
@sofia-doofinder sofia-doofinder deleted the Sofia/mixing_cron_naming branch July 29, 2024 13:14
@sofia-doofinder sofia-doofinder mentioned this pull request Jul 29, 2024
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.

5 participants