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

CO: add old product ID on actionProductAdd hook when we duplicate product #10533

Merged
merged 3 commits into from Dec 14, 2018

Conversation

Projects
None yet
5 participants
@duGuillaume
Copy link
Contributor

duGuillaume commented Sep 20, 2018

Questions Answers
Branch? develop
Description? during product duplication it was impossible to retrieve old product id and use it on actionProductAdd hook
Category? CO

This change is Reviewable

duGuillaume and others added some commits Sep 20, 2018

@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Nov 20, 2018

In the case we do not duplicate a product, it would be great to at least have a entry about the original product.
This should be done in the third occurrence I found in AdminProductController:

Hook::exec('actionProductAdd', array('id_product' => (int) $this->object->id, 'product' => $this->object));

In this case, as we don't duplicate a product, the value could be null.

Also, could we have the key as old_id_product? It would follow the parameters of actionShopDataDuplication, with the parameter starting with old_.

@duGuillaume

This comment has been minimized.

Copy link
Contributor

duGuillaume commented Nov 21, 2018

I wanted to respect the naming chart set by the existant variable named $id_product_old = $product->id; created at line 521

is set like this in all duplicates functions like
public static function duplicateAttributes($id_product_old, $id_product_new) or public static function duplicateSuppliers($id_product_old, $id_product_new) for exemple

How i need to change this PR ? By naming $id_product_old to $old_id_product everywhere or i put 'old_id_product' => $id_product_old as third param ?

@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Nov 21, 2018

Your point seems valid, let's keep it as you did.

However, would you mind updating the third call to the hook? There is one call without the parameter.
Adding 'id_product_old' => null would be fine.

@duGuillaume

This comment has been minimized.

Copy link
Contributor

duGuillaume commented Nov 21, 2018

should i change in
tests/resources/module/pscsx32412/override/controllers/admin/AdminProductsController.php l.673 and l.1797
and
tests/resources/ModulesOverrideInstallUninstallTest/AdminProductsController.php l.421 and l.1549 ?

@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Nov 21, 2018

No, these files are used for tests regarding overrides only

Guillaume Durand
@duGuillaume

This comment has been minimized.

Copy link
Contributor

duGuillaume commented Nov 21, 2018

ok, done

@mickaelandrieu

This comment has been minimized.

Copy link
Contributor

mickaelandrieu commented Dec 10, 2018

Hello @Quetzacoalt91,

I think we can merge it: do you agree?

Mickaël

@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Dec 11, 2018

Just asking for a QA review

@marionf marionf added QA ✔️ and removed waiting for QA labels Dec 12, 2018

@Quetzacoalt91 Quetzacoalt91 merged commit fb0964b into PrestaShop:develop Dec 14, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Dec 14, 2018

Thank you @duGuillaume

@Quetzacoalt91 Quetzacoalt91 added this to the 1.7.6.0 milestone Dec 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment