Skip to content
This repository has been archived by the owner on May 28, 2023. It is now read-only.

Bugfix/original_price not calculated correctly due to tax bug #464

Merged
merged 1 commit into from
Jun 18, 2020

Conversation

didkan
Copy link
Contributor

@didkan didkan commented Jun 5, 2020

Fixes #463

Use normal equality operator instead of strict equality operator to catch both null and undefined values for sourcePriceInclTax.
This makes sure we get proper calculation of original_price and its variants.

@pkarw pkarw requested a review from gibkigonzo June 5, 2020 08:15
Copy link
Contributor

@gibkigonzo gibkigonzo left a comment

Choose a reason for hiding this comment

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

@didkan
Copy link
Contributor Author

didkan commented Jun 5, 2020

Exactly.
There is similar code in vuestorefront, but that code doesn't even try to fall back on config.tax.sourcePriceIncludesTax. I suppose that means it is mandatory to put the sourcePriceIncludesTax in config.storeViews.xx.tax instead.
Not sure if this is by design or a mistake. I suppose I would prefer both vuestorefront and vuestorefront-api to handle that config option in the same way.
Here is the code part in vuestorefront:
https://github.com/DivanteLtd/vue-storefront/blob/master/core/modules/catalog/store/tax/actions.ts#L52-L61

@pkarw
Copy link
Contributor

pkarw commented Jun 6, 2020

@gibkigonzo we need to port it to storefornt-api and the tax.ts in the vue-storefront project as well I guess

@gibkigonzo
Copy link
Contributor

@pkarw yes, we need to port it in storefront-api, but not in vsf
@didkan storeView in vsf is prepared before app creation. currentStoreView() doesn't return config but already merged config.storeViews.xx.tax with global config.tax. https://github.com/DivanteLtd/vue-storefront/blob/master/core/lib/multistore.ts#L67 https://github.com/DivanteLtd/vue-storefront/blob/master/core/lib/multistore.ts#L18-L34 So if sourcePriceIncludesTax will not be defined in config.storeViews.xx.tax then it will not override config.tax. I think that we shouldn't build config for multistore in TaxProxy in first place, because it is not TaxProxy responsibility to build config. This require bigger refactor, because there are more places where config is build.

@gibkigonzo gibkigonzo merged commit 4c06d23 into vuestorefront:develop Jun 18, 2020
This was referenced Jun 19, 2020
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.

3 participants