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

Product combination price edit : Use blur instead of onkeyup to avoid price flip #25749

Merged

Conversation

sowbiba
Copy link
Contributor

@sowbiba sowbiba commented Aug 31, 2021

Questions Answers
Branch? 1.7.7.x
Description? Use blur to wait until user ends typing before round the price
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #25718 & #25753
How to test? See #25718 & #25753
Possible impacts? Test with ecotax

This change is Reviewable

@sowbiba sowbiba requested a review from a team as a code owner August 31, 2021 08:15
@prestonBot prestonBot added 1.7.7.x Branch Bug fix Type: Bug fix labels Aug 31, 2021
matks
matks previously approved these changes Aug 31, 2021
@sowbiba sowbiba added this to the 1.7.7.8 milestone Aug 31, 2021
jolelievre
jolelievre previously approved these changes Aug 31, 2021
Copy link
Contributor

@jolelievre jolelievre left a comment

Choose a reason for hiding this comment

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

Thanks @sowbiba

Seems good to me, I would just like to have the approval from @PierreRambaud since he worked on the number formatted part

@PierreRambaud
Copy link
Contributor

PierreRambaud commented Aug 31, 2021

I approve but I don't know if this trigger is still needed:
https://github.com/PrestaShop/PrestaShop/blob/develop/admin-dev/themes/default/js/bundle/product/form.js#L1969$

@sowbiba
Copy link
Contributor Author

sowbiba commented Aug 31, 2021

I approve but I don't know if this trigger is still needed:
https://github.com/PrestaShop/PrestaShop/blob/develop/admin-dev/themes/default/js/bundle/product/form.js#L1969$

$(document).on('blur', '.combination-form .attribute_priceTI', function() { ==> TI edit

TI edit => Impact on TE
                           => get TI input value
                              compute TE value
                              update TE input
                              Impact on TI
                                  => compute TI value (depending on TE)
                                     update TI input

this is weird to me, we assume to (maybe) override the value edited

@sowbiba sowbiba dismissed stale reviews from jolelievre and matks via 86cac09 August 31, 2021 11:22
@@ -1991,7 +1976,7 @@ var priceCalculation = (function() {
var impactPriceTI = this.getImpactTIInputValue(obj);
var impactPriceTE = this.computePriceTaxExcluded(impactPriceTI);

this.updateImpactTEInput(impactPriceTE, obj);
this.getImpactTEInput(obj).val(impactPriceTE);
Copy link
Contributor

Choose a reason for hiding this comment

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

Careful, you are changing the behaviour here, as the removed updateImpactTEInput function used to update both fields tax excluded and included (and recompute tax included at the same time)

Copy link
Contributor

Choose a reason for hiding this comment

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

The whole behaviour is quite touchy and prone to many errors To avoid any risk I would have kept the update* methods, and you can simply remove the change trigger code I guess

Also you need to be careful because not only the combination form is updated, the combination list also needs to be updated and it's not handled in this file. The list update is handled here

updateFinalPrice($(input.parents('tr')[0]));

It relies on the change event so it might break this feature, if you only wanted to remove the change trigger for "clean code" the DON'T 😅. Because this whole code is a mess and a slight change can break many unexpected things

As long as your bug is fixed you should try to minimize the modifications as much as possible. This code is bound to be removed once the product page is redone in v8 anyway, so now is not the time to do overkill changes unless you plan on releasing a 1.7.9 next week.. ^^

@sowbiba sowbiba force-pushed the fix-product-combination-price-round branch from 86cac09 to 66332b7 Compare August 31, 2021 11:55
jolelievre
jolelievre previously approved these changes Aug 31, 2021
Copy link
Contributor

@jolelievre jolelievre left a comment

Choose a reason for hiding this comment

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

Thanks @sowbiba

PierreRambaud
PierreRambaud previously approved these changes Aug 31, 2021
@PierreRambaud PierreRambaud added the Waiting for QA Status: action required, waiting for test feedback label Aug 31, 2021
@jolelievre jolelievre linked an issue Aug 31, 2021 that may be closed by this pull request
NeOMakinG
NeOMakinG previously approved these changes Aug 31, 2021
Copy link
Contributor

@jolelievre jolelievre left a comment

Choose a reason for hiding this comment

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

Thank you @atomiix for fixing my mistakes ^^

@khouloudbelguith
Copy link
Contributor

Hi @sowbiba, @atomiix,

I checked with @Robin-Fischer-PS
It is ok ✔️

About the rounding price, we have old issues:
#9855
#12078

But before PS1.7.7.5, we need to refresh the page to show the error and now the rounding bug is displayed when we click outside because of blur (The event is on blur. As soon as you leave the field, the calculation is done)

And about the ecotax issue, we have old issues
#9972
#24373

So it seems that the ecotax is not working since before.

Thanks!

@khouloudbelguith khouloudbelguith added QA ✔️ Status: check done, code approved and removed Waiting for QA Status: action required, waiting for test feedback labels Sep 2, 2021
@PierreRambaud PierreRambaud merged commit 5c86a98 into PrestaShop:1.7.7.x Sep 2, 2021
@PierreRambaud
Copy link
Contributor

Thank you @sowbiba and @atomiix

@sowbiba sowbiba deleted the fix-product-combination-price-round branch September 7, 2021 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.7.7.x Branch Bug fix Type: Bug fix QA ✔️ Status: check done, code approved
Projects
None yet
8 participants