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

Fix missing tax states from loops #2470

Open
wants to merge 7 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@marsender
Contributor

marsender commented Jan 30, 2018

This PR allows states to be taken into account in tax rules.
Without this, if you are in a country like US with multiple states, the state filter is not used and the tax is added for each state.
The problem occurs for any tax rules applied to a country with states.
Thanks

@roadster31

This comment has been minimized.

Contributor

roadster31 commented Feb 8, 2018

Could you please give a description of your PR ?

@marsender

This comment has been minimized.

Contributor

marsender commented Feb 8, 2018

This PR allows states to be taken into account in tax rules.
Without this, if you are in a country like US with multiple states, the state filter is not used and the tax is added for each state.
The problem occurs for any tax rules applied to a country with states.
Thanks

@roadster31

This comment has been minimized.

Contributor

roadster31 commented Feb 12, 2018

In this PR, you're changing the signature of several public methods in core/lib/Thelia/Model/ProductSaleElements.php and core/lib/Thelia/Model/Product.php, which causes à BC break.

Maybe you can add additional method parameters at end of parameter list, with a default value, or use new methods (e.g. getTaxedPriceWithState) and keep old ones with the same signature.

What do you think ?

@marsender

This comment has been minimized.

Contributor

marsender commented Feb 12, 2018

Hi,
I saw that and have added this change: a7d793c
in order to set a null default value: State $state = null
But sorry, I forgot to add it in the PR

@roadster31

This comment has been minimized.

Contributor

roadster31 commented Oct 25, 2018

Hi,

Even with your last changes, the signature of the method is modified, as parameters are not in the same order;

public function getTaxedPrice(Country $country, $price) becomes public function getTaxedPrice(Country $country, State $state = null, $price = 0).

The following signature don't introduce a compatibility break, as method parameters appear in the same order, and the new $state parameter has a default value :

public function getTaxedPrice(Country $country, $price, State $state = null).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment