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

Recalculate totals on order change #17769

Open
Tracked by #19445
Hlavtox opened this issue Feb 20, 2020 · 6 comments
Open
Tracked by #19445

Recalculate totals on order change #17769

Hlavtox opened this issue Feb 20, 2020 · 6 comments
Labels
CO Category: Core Feature Type: New Feature Needs Specs Status: issue needs to be specified Order Component: Which BO section is concerned

Comments

@Hlavtox
Copy link
Contributor

Hlavtox commented Feb 20, 2020

Hi guys, I was browsing the code for the migrated admin order page and there is still a lot of code that could (and does) cause trouble.

Whatever changes you do, add or remove products, add or remove discounts, it adds or subtracts the amounts from the order and invoice. But this creates room for errors, negative totals, rounding errors, because it works with rounded values. (Like #13809)

I don't know if you guys had a discussion about this, but I think it would be good (if there is time) to change to logic to completely recalculate the order every time something changes. This would eliminate all the trouble there is.

@khouloudbelguith
Copy link
Contributor

Hi @Hlavtox,

Thanks for your report.
Ping @PrestaShop/prestashop-product-team, @PrestaShop/prestashop-core-developers what do you think?

Thanks!

@khouloudbelguith khouloudbelguith added CO Category: Core Improvement Type: Improvement Order Component: Which BO section is concerned Needs Specs Status: issue needs to be specified Waiting for dev Status: action required, waiting for tech feedback Waiting for PM Status: action required, waiting for product feedback labels Feb 21, 2020
@Hlavtox Hlavtox changed the title [PROPOSAL] Recalculate totals on order change Recalculate totals on order change Feb 21, 2020
@arouiadib
Copy link
Contributor

arouiadib commented Feb 25, 2020

Hello,
I totally agree with you @Hlavtox.
I managed to reproduce #13809 and #11059, and they are caused by the rounded values. In fact, subtracting a rounded up value from an non-rounded value give negative result, which is invalid from Object point of view.
But I think this problem of rounding has already a ticket somewhere here in Github, I just don't find it.

Wdyt?

  • We use a workaround for the two mentioned issues? like round() or setting to zero totals that are negative?
    or
  • We gather all tickets about rounding, and have a better idea about the whole situation?

Thank you very much for your time guys :)

@colinegin
Copy link

Second option is what we aim for but as you can imagine this is very complex. We have already started gathering all issues about rounding, you can find the Price & taxes epic here : #9703

Of course, this needs a deep functional and technical study before we can really start working on it.
This is a priority topic for 178.

@Hlavtox
Copy link
Contributor Author

Hlavtox commented Feb 25, 2020

@arouiadib @colinegin

Personally, I wouldn't rush it and settle for some “hackjobs”. Even if we temporarily fix it by (value < 0, then value = 0), it still does not solve the issue of rounding...

Better to think it through. It waited a few years, it can wait for some more time.

Proper tax and discount calculations in FO are more important. We (merchants) can always tell a customer: “Make a new order, we don't change them.” 👍

@arouiadib
Copy link
Contributor

Perfect :), thank you very much @colinegin @Hlavtox

I suggest closing this issue and continuing discussion there in #9703. wdy?

@colinegin
Copy link

Hi guys,

just for your information, I just created a new EPIC dedicated to rounding issues. You can find it here : #19445

Have a nice evening ;)

@marionf marionf added Feature Type: New Feature and removed Improvement Type: Improvement Waiting for PM Status: action required, waiting for product feedback Waiting for dev Status: action required, waiting for tech feedback labels Dec 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CO Category: Core Feature Type: New Feature Needs Specs Status: issue needs to be specified Order Component: Which BO section is concerned
Projects
None yet
Development

No branches or pull requests

5 participants