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

feature: calculate shipping tax using cart items #1283

Merged
merged 34 commits into from
Mar 5, 2020
Merged

feature: calculate shipping tax using cart items #1283

merged 34 commits into from
Mar 5, 2020

Conversation

lukadschaak
Copy link
Contributor

@lukadschaak lukadschaak commented Feb 14, 2020

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Fixed tickets -

Moin!
@dpfaffenbauer , do you remember the shipping tax calculation in germany? We spoke about that in the training and you implemented some stuff.

This PR implements the possibility to select a service, that will calculate the tax amount of the shipping price. In some countries the shipping price will have fix tax rate. In other countries, for example in Germany, the tax amount for the shipping cost is variable. It depends on the tax rate of the cart items. When all Items have the same tax rate, the shipping tax is the same (7% or 19%). If there are items with different tax rates in cart, it gets complicated.

With this Feature you can implement your own service and define your own tax calculation. Also the two ways of calculation are pre-implemented.

The Code is WIP. First of all i want to know if @dpfaffenbauer wants this feature in core and if you are happy with this way of implementing this feature. If yes i would finish the code and try to add tests.

On the following screen you see the combobox i added.

cart-based-taxation

@dpfaffenbauer
Copy link
Member

I'd much rather go a step further and would introduce shipping calculation strategies, where you can register services, these services gets displayed in a combobox and you can choose between them, wdyt?

@lukadschaak
Copy link
Contributor Author

Like we start with our two strategies, and register them for example as service with the symfony tagging system? Then people are able to register their own service, which will then be displayed in the combobox?!
I think that sounds really nice. So every Country can select/implement his fitting service.

@dpfaffenbauer
Copy link
Member

yep

@dpfaffenbauer
Copy link
Member

BTW: If you are not sure how to implement it, we have several examples of how that is done in CoreShop. eg. CoreShop Index

@lukadschaak
Copy link
Contributor Author

I will take a look at the current implementations and update this PR asap.

@dpfaffenbauer dpfaffenbauer changed the title WIP: [feature] calculate shipping tax using cart items [WIP] feature: calculate shipping tax using cart items Feb 18, 2020
@lukadschaak
Copy link
Contributor Author

What is the reason of adding the fields 'Stores' and 'Tax Rate' (see Screenshot of PR description) in ExtJS via panel.down("fieldset").add in a seperate item.js inside the CoreBundle?

src/CoreShop/Bundle/CoreBundle/Resources/public/pimcore/js/shipping/carrier/item.js

@dpfaffenbauer
Copy link
Member

cause the shipping bundle doesn't know anything about stores nor taxes. only the core-bundle does

@lukadschaak lukadschaak changed the title [WIP] feature: calculate shipping tax using cart items feature: calculate shipping tax using cart items Feb 25, 2020
@lukadschaak
Copy link
Contributor Author

General question. I'm thinking about naming and maybe ShippingTaxCaluculation would be more appropiate than ShippingTaxStrategy?!

@dpfaffenbauer
Copy link
Member

ShippingTaxCalculationStrategy?

@lukadschaak
Copy link
Contributor Author

Sounds good. I will do it

@dpfaffenbauer
Copy link
Member

Is this ready to be reviewed and tested?

FYI: Travis currently fails cause Pimcore 6.5.2 is broken...

Copy link
Member

@dpfaffenbauer dpfaffenbauer left a comment

Choose a reason for hiding this comment

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

Just realised, this cannot be finished yet. Carrier Price Calculation also knows about tax and needs to use the tax-calc service as well, right?

@lukadschaak
Copy link
Contributor Author

Just realised, this cannot be finished yet. Carrier Price Calculation also knows about tax and needs to use the tax-calc service as well, right?

Not sure where else the Carrier Price Calculation takes place. I just know of the CartProcessors which listen to cart updates. But yes, The Carrier price calculation always depends on these tax-calc services i think.

@dpfaffenbauer dpfaffenbauer added this to the 2.2.0 milestone Mar 3, 2020
dpfaffenbauer
dpfaffenbauer previously approved these changes Mar 3, 2020
@dpfaffenbauer
Copy link
Member

@solverat could you review that as well pls?

@dpfaffenbauer dpfaffenbauer merged commit abb157e into coreshop:master Mar 5, 2020
@dpfaffenbauer
Copy link
Member

big thanks @lukadschaak

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

Successfully merging this pull request may close these issues.

2 participants