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

FIX: price impact of new combination is always a percentage even if the checkbox was unchecked #18397

Conversation

atm-florianm
Copy link
Contributor

@atm-florianm atm-florianm commented Aug 11, 2021

FIX cf. forum thread

When you create a new product combination, even if you leave the "Percentage variation" checkbox empty, the price impact is considered a percentage.

This is caused by the attribute variation_price_percentage of ProductCombination being either an array or a boolean depending on whether PRODUIT_MULTIPRICES is used: it seems to me that in the lines I changed, it was set as an array when it should have been a boolean and vice versa.

My changes fix the problem as it was described, however, I am not very familiar with the multiprice feature, so I’d gladly wait for someone more experienced to take a second look.

@@ -137,9 +137,10 @@
// for conf PRODUIT_MULTIPRICES
if ($conf->global->PRODUIT_MULTIPRICES) {
$level_price_impact = array_map('price2num', $level_price_impact);
$level_price_impact_percent = array(1 => $price_impact_percent);
Copy link
Member

@eldy eldy Aug 17, 2021

Choose a reason for hiding this comment

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

Why only the key 1 is filled? Later we used the array with index [$i] where $i is the price level.
Can you check if this is ok ?

$level_price_impact_percent = array();
for ($i = 1; $i <= $conf->global->PRODUIT_MULTIPRICES_LIMIT; $i++) {
    $level_price_impact_percent = array($i, $price_impact_percent);
} 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ll check this with @atm-john next time I see him, because I am not very familiar with multiprice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I (finally) discussed it with John: seems OK at first look, but we want to review it once more to be sure.

@eldy eldy added the Discussion Some questions or discussions are opened and wait answers of author or other people to be processed label Aug 17, 2021
@eldy
Copy link
Member

eldy commented Apr 10, 2023

Does this bug still true with v17 ?

@eldy eldy added the Bug need more information This bug needs more information to be processed label Apr 10, 2023
@eldy
Copy link
Member

eldy commented Aug 26, 2023

@atm-florianm can we close this ? Or bug still exists in v18 ?

@eldy
Copy link
Member

eldy commented Sep 27, 2023

It seems this PR was set with "Discussion" or "PR to fix" tag, but no answer nor correction was provided since a long time. So request is automatically closed.
Please reopened and add a comment if you think this is an error.

@eldy eldy closed this Sep 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug need more information This bug needs more information to be processed Discussion Some questions or discussions are opened and wait answers of author or other people to be processed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants