Skip to content
This repository has been archived by the owner on Sep 8, 2021. It is now read-only.

Fix calculation to apply taxes after discount #19

Merged
merged 1 commit into from
Nov 14, 2018

Conversation

mraerino
Copy link
Contributor

@mraerino mraerino commented Nov 2, 2018

Fixes #16

This PR replicates netlify/gocommerce#155 for the JS lib.
The tests have been synced manually to verify similar behavior.

@mraerino mraerino changed the title Fix calculation to apply taxes after discount WIP: Fix calculation to apply taxes after discount Nov 12, 2018
@mraerino mraerino force-pushed the fix/calculation-discounts-taxes branch from eda3bb7 to 7319eb9 Compare November 13, 2018 17:22
Introduces the field "NetTotal" for orders and price items.
This field represents the net amount of a single item or an order
that taxes are calculated based on. While the "subtotal" amount is
not discounted, the "NetTotal" is.
This is especially important when prices are expected to include
taxes. In these cases discounts are applied to the original price
and then the net price is calculated.

The test values are synced with those in GoCommerce.
@mraerino mraerino force-pushed the fix/calculation-discounts-taxes branch from 7319eb9 to 2271c04 Compare November 13, 2018 17:22
@mraerino mraerino changed the title WIP: Fix calculation to apply taxes after discount Fix calculation to apply taxes after discount Nov 13, 2018
@mraerino
Copy link
Contributor Author

mraerino commented Nov 13, 2018

@bcomnes this has been updated to do the same as what has been refined in my GoCommerce PR. Description about tests etc still applies.
Ready for review 😊

@bcomnes
Copy link
Contributor

bcomnes commented Nov 14, 2018

Triaged this today. Will review soon.

@bcomnes bcomnes self-requested a review November 14, 2018 19:56
Copy link
Contributor

@bcomnes bcomnes left a comment

Choose a reason for hiding this comment

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

LGTM

@mraerino versionwise, would this be a breaking change? Also, do we need to enforce any kind of version requirements between the client and server?

@mraerino
Copy link
Contributor Author

mraerino commented Nov 14, 2018

@bcomnes
this change is not connected to any api code. it is only needed for a price preview before issuing the order.

the only thing that is changed and not added in this fix is the behavior of the tax amount field. but since the behavior was wrong before (considering any taxation system in US or Europe) it should rather be classified as a bug fix than a breaking change.
If you want to make it a minor version bump then do it because of the added field net_total.

@bcomnes
Copy link
Contributor

bcomnes commented Nov 14, 2018

OK sounds good.

@bcomnes bcomnes merged commit f69c350 into netlify:master Nov 14, 2018
@bcomnes
Copy link
Contributor

bcomnes commented Nov 14, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants