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

Check if Combination id is set before deleting Associations from data… #14043

Merged
merged 2 commits into from Jul 2, 2019

Conversation

@kazeno
Copy link
Contributor

commented May 31, 2019

…base. Otherwise it will remove all products without attributes from all carts.

Questions Answers
Branch? develop
Description? Checks if Combination id is set before removing its data from the DB. If not set returns false.
Type? bug fix
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #14041
How to test? Call (new Combination())->deleteAssociations() . Before the patch it will remove all products without combinations from all existing carts, which is an unintended consequence of the API. After the patch it won't.

This change is Reviewable

Check if Combination id is set before deleting Associations from data…
…base. Otherwise it will remove all products without attributes from all carts.

@kazeno kazeno requested a review from PrestaShop/prestashop-core-developers as a code owner May 31, 2019

@prestonBot

This comment has been minimized.

Copy link
Collaborator

commented May 31, 2019

Hello @kazeno!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

@prestonBot prestonBot added the Bug label May 31, 2019

Show resolved Hide resolved classes/Combination.php Outdated
Update classes/Combination.php
Co-Authored-By: GoT <PierreRambaud@users.noreply.github.com>

@prestonBot prestonBot added the develop label Jun 7, 2019

@sarahdib

This comment has been minimized.

Copy link

commented Jun 11, 2019

Hello @kazeno

Thank you for your contribution :)
can you provide me a module to test the PR ?

Thank You

@kazeno

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

Hello @kazeno

Thank you for your contribution :)
can you provide me a module to test the PR ?

Thank You

Hi, do you mean an integration test suite?

@sarahdib

This comment has been minimized.

Copy link

commented Jun 12, 2019

@kazeno no just a module who is using (new Combination())->deleteAssociations() to test this fix.

Thank you

@kazeno

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2019

I've created a proof of concept module, you can find it here:
https://gist.github.com/kazeno/f943337df1c7e8d6415e86757cf2c785
Just put the code inside modules/issue14041/issue14041.php and install the module, it will replicate the issue any time the module's configuration is accessed.

@sarahdib sarahdib added QA ✔️ and removed waiting for QA labels Jun 27, 2019

@sarahdib sarahdib added this to the 1.7.7.0 milestone Jun 27, 2019

@matks

This comment has been minimized.

Copy link
Contributor

commented Jul 2, 2019

Thank you @kazeno

@matks matks merged commit 1c3ddd0 into PrestaShop:develop Jul 2, 2019

2 checks passed

PrettyCI Code formatting
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kazeno kazeno deleted the kazeno:combinations-fix branch Jul 2, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.