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

Fix discount computing to take combination into account #11645

Merged
merged 1 commit into from Dec 13, 2018

Conversation

Projects
None yet
6 participants
@jolelievre
Copy link
Contributor

jolelievre commented Dec 6, 2018

Questions Answers
Branch? develop
Description? Set discount on a product which has variation depending on the combination The discount table only computes based on the default combination It does not update when you change the attributes The controller simply did not use the id_product_attribute when computing the price
Type? bug fix
Category? FO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #9872
How to test? Set a discount of 30% for a product with combination Set a different price on one color When you switch color the discount table has to update

This change is Reviewable

@jolelievre jolelievre added this to the 1.7.6.0 milestone Dec 6, 2018

@marionf marionf self-assigned this Dec 7, 2018

@gudmund1
Copy link

gudmund1 left a comment

Hi, I tested this on my store (prestashop 1.7.3.2) but it did not fix the issue for me. The discount price still only displays the value for default combination.

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

@marionf marionf removed their assignment Dec 12, 2018

@jolelievre

This comment has been minimized.

Copy link
Contributor

jolelievre commented Dec 12, 2018

Hi @gudmund1 if you tried to import this change in another version I can't assure you it will work, there may be other changes in the code that makes it work. This fix is targeted for the develop branch (coming 1.7.6 version).
Did you also copy the getIdProductAttributeByRequestOrGroup function?

In any case I wouldn't advise you to modify the core code in PrestaShop unless you are very sure of what you do, as it can cause side effects and it will be harder for you to upgrade your version.

@gudmund1

This comment has been minimized.

Copy link

gudmund1 commented Dec 12, 2018

Did you also copy the getIdProductAttributeByRequestOrGroup function?

Hi, I applied the listed changes to the productcontroller.php file. When it didn't work I undid the changes. Do you mean this function needs to be updated elsewhere?

I "upgraded" to 1.7.3.2 about 6 months ago and it has nearly ruined my business. I will not be upgrading again any time soon, especially not to fix bugs like these. If I had to do that, I would just give up and switch to Woocommerce. And bear in mind that this functionality was present and working fine in 1.5. So I'm determined to get this functionality back, without going through the nightmare of an upgrade again. If you could help me do that I would be very appreciative.

@jolelievre

This comment has been minimized.

Copy link
Contributor

jolelievre commented Dec 12, 2018

It's just that the method getIdProductAttributeByRequestOrGroup was not available in 1.7.3 and was introduced in the 1.7.5 development, so if you just copy the diff from this PR it can not work.

You can try and copy the method in your code to make this fix work, but again I strongly advise against doing so. Because this method can itself have dependencies on other method or bug fixes so nothing guaranty that this will work.

I understand that the upgrade can be difficult in some cases, especially if you upgraded form a quite low version. But we have several feedbacks of agencies who use our "One click upgrade" module and manage to upgrade their shop as the new versions are released.

In any case when performing an upgrade (or any other fix or code modification) it is highly recommended to perform it not on your production site, but rather first on a copy of your site (both code and database) to check if the upgrade/modification process goes smoothly.

@gudmund1

This comment has been minimized.

Copy link

gudmund1 commented Dec 13, 2018

I understand. Thank you for explaining. I accept the risks, after all I've been hacking prestashop for nearly 10 years with only a few disasters :-)
Could you perhaps point me in the right direction? Which file from version 1.7.5.x contains this function? I will study it and see if I need help from a developer to achieve it.
Thank you

@eternoendless

This comment has been minimized.

Copy link
Member

eternoendless commented Dec 13, 2018

Thank you @jolelievre

@eternoendless eternoendless merged commit e9b88af into PrestaShop:develop Dec 13, 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
@jolelievre

This comment has been minimized.

Copy link
Contributor

jolelievre commented Dec 13, 2018

@gudmund1 ok if you are sure of what you do, the function is in the same file you can find it in the develop branch here:

private function getIdProductAttributeByRequestOrGroup()

it seems to be linked to, at least, the following functions as well, but don't hesitate to search for more dependencies just in case:

  • getIdProductAttributeByGroup
  • tryToGetAvailableIdProductAttribute

However I prefer to repeat this, not for you gudmund1 but for any other person who would come and read this PR:

I REALLY STRONGLY advise AGAINST picking pieces of code from PR destined to others versions of PrestaShop, keep in mind that these fixes were developed in the context of a specific version and integrating them in another can cause unexpected side effects.

Besides modifying the core code may block you in the upgrade process which is a loss, new versions of PrestaShop integrate new features, improvements, optimizations and most important security fixes. It really is a loss not to upgrade PrestaShop regularly and we are working every day in improving and making easier this upgrade process with good feedbacks from both developers and agencies.

Well, that being said @gudmund1 I hope the information I gave you will help, don't hesitate to ask if you have other questions, and most of all be careful ;)

@gudmund1

This comment has been minimized.

Copy link

gudmund1 commented Dec 13, 2018

thank you @jolelievre! I've been waiting for this for so long. Just to know that there is a solution is great, even if I can't achieve it in 1.7.3.2. Thank you so much. I will put the results here.

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