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

[Admin][Core] Fix calculation issues on order show #5169

Merged

Conversation

Zales0123
Copy link
Member

@Zales0123 Zales0123 commented Jun 3, 2016

Q A
Bug fix? yes
New feature? yes
BC breaks? yes
Related tickets
License MIT

This PR some issues on new admin order show page and introduce new logic of shipping promotions:

  • use methods introduced in this PR in order show page on admin module
  • introduced order shipping promotion adjustment, that replaces order promotion usage in shipping discount action
  • fix shipping taxes applying (shipping promotions has not been included)

- introduced ORDER_SHIPPING_PROMOTION_ADJUSTMENT to separate shipping promotions from order promotions
- apply shipping tax based on shipping total AND shipping promotions
@Zales0123 Zales0123 changed the title Fix calculation issues on order show [Admin][Core] Fix calculation issues on order show Jun 3, 2016
@pjedrzejewski
Copy link
Member

Not including neutral tax adjustments in the tax total is not entirely correct. Even if the taxes are included in price, we need to display the total tax amount correctly - including that tax.

@@ -25,3 +28,12 @@ Feature: Seeing taxes of order items
Then the order's shipping total should be "€0.00"
And the order's tax total should be "€32.20"
And the order's total should be "€252.20"

@ui
Scenario: Seeing included in price taxes of order items are not counted in taxes total
Copy link
Contributor

Choose a reason for hiding this comment

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

Please extract this to a separate feature file.

  1. We should not mix included and excluded tax rates within one channel
  2. The background becomes too big

@michalmarcinkowski
Copy link
Contributor

We also should have scenarios for aggregating order promotion and tax adjustments.

@Zales0123
Copy link
Member Author

@michalmarcinkowski Totally agreed 👍 However, maybe we could introduce them in separate PR, as it's similar but still different problem and we don't want to delay this PR?

@pjedrzejewski pjedrzejewski merged commit 7ad7929 into Sylius:master Jun 3, 2016
@pjedrzejewski
Copy link
Member

Thanks Mateusz! 👍

@Zales0123 Zales0123 deleted the fix-calculation-issues-on-order-show branch October 28, 2016 13:39
pamil pushed a commit to pamil/Sylius that referenced this pull request May 7, 2019
…on-order-show

[Admin][Core] Fix calculation issues on order show
pamil pushed a commit to pamil/Sylius that referenced this pull request May 7, 2019
…on-order-show

[Admin][Core] Fix calculation issues on order show
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants