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

WIP: 6375 - cumulation percentual discounts leads to wrong calculation #419

Closed
wants to merge 6 commits into from
Closed

WIP: 6375 - cumulation percentual discounts leads to wrong calculation #419

wants to merge 6 commits into from

Conversation

michaelkeiluweit
Copy link
Contributor

@michaelkeiluweit michaelkeiluweit commented Jun 23, 2016

https://bugs.oxid-esales.com/view.php?id=6375

The commit f3847ec adds the method Price::addDiscount, because the method name setDiscount is misleading. As the commit messages says: "introduced the method Price::addDiscount as the body of setDiscount adds a discount to an array. But set implies that only one discount can be set.". Therefore I added the tag "@deprecated" to the method header of Price::setDiscount.
If the commit is not conform, please feel free to skip this commit.

@michaelkeiluweit
Copy link
Contributor Author

@ Developer: Please consider also this (private) comment from @keywan-ghadami-oxid https://bugs.oxid-esales.com/view.php?id=6375#c11654

@robertblank
Copy link
Contributor

The question is, if the bug is a bug or if it is a feature.
If you apply 2 discounts of 10% to a 100 EUR worth product, would you expect :

  1. the discounts to be added (20%) and subtracted from the product price. The total would be 80 EUR.
  2. the first discount to be subtracted from the product price (90 EUR) and the second discount subtracted from 90 EUR. The total would be 81 EUR.

Normally we have a "total" line, if some calculations are done and this line is missing after applying each discount. see screenshot This would suggests, that there are no calculations done.
Additionally a "10% discount coupon" means "10% off your basket". In case 2) this is true for the first coupon, but not for the second, which would mean "9% off your basket". The user would loose 1% and could be annoyed. If the users has 2 different discounts e.g. "10% discount" and "20% discount" even the order of applying the discounts matters, so things could really get complicated to explain.

I would like to wait for some comments from the community upon this topic before accepting this PR.

@michaelkeiluweit
Copy link
Contributor Author

michaelkeiluweit commented Jun 28, 2016

Yesterday evening I decided to change the bug to a feature request. Therefore I need to

  • revert the changes on the tests
  • implement an option if the discount is an additive or multiplicative discount
  • add new tests to cover the then new additive discounts

So please do not merge the PR for the next time

@michaelkeiluweit michaelkeiluweit changed the title 6375 - cumulation percentual discounts leads to wrong calculation WIP: 6375 - cumulation percentual discounts leads to wrong calculation Jun 28, 2016
@floeschie
Copy link

Can we close this pull request and you create a new one later? I would like to get rid of stuff which is not ready for merge or work in progress. WIP stuff should stay in the author's personal repo clone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants