Dynamic shipping tax class calculation #192

aboritskiy · 2020-12-21T22:43:37Z

Please make sure these boxes are checked before submitting your PR - thank you!

[V] Pull request is based against develop branch
README.md reflects changes (if applicable)
New files contain a license header

Issue

This PR fixes issue # .

Proposed changes

this is rebased and reworked version of #176
major changes comparing to the original pull request:

it is based on 0.4.1
I covered all code style comments from the original PR
reworked the feature to allow compatibility with backend orders.

sprankhub · 2020-12-22T09:41:51Z

Thanks a lot @aboritskiy! Will have a closer look ASAP.

aboritskiy · 2020-12-22T11:54:36Z

@sprankhub

composer self-update
Updating to version 2.0.8 (stable channel).
   Downloading (100%)
Use composer self-update --rollback to return to version 1.9.2

it looks like travis tries to work with composer 2.x, I gave a quick look through the code, I don't get why it happens cause you already have --1 flag here: https://github.com/firegento/firegento-magesetup2/blob/develop/Test/script/travis_before_install.sh#L22, will try to give a closer look into that later as well.

Model/Config.php

aboritskiy · 2020-12-22T12:56:30Z

here is the explanation of composer failure:

https://github.com/composer/composer/blob/master/CHANGELOG.md

1.10.2 2020-04-09
Added --1 flag to self-update command which can be added to automated self-update runs to make sure it won't automatically jump to 2.0 once that is released
Fixed path repository symlinks being made relative when the repo url is defined as absolute paths
Fixed potential issues when using "composer ..." in scripts and composer/composer was also required in the project
Fixed 1.10.0 regression when downloading GitHub archives from non-API URLs
Fixed handling of malformed info in fund command
Fixed Symfony5 compatibility issues in a few commands

i.e. --1 flag is not supported by composer version that is in the base image/config of travis, this causes composer to upgrade itself to 2.x which then breaks magento installation. I don't think I can help here further cause it then depends on travis config that is not visible to me as external.

sprankhub · 2020-12-22T16:13:43Z

Thanks @aboritskiy. Composer is not the issue, our implemented fix works. Restarted the build and we are almost green :-) Could you please fix the code style issues from https://travis-ci.com/github/firegento/firegento-magesetup2/jobs/463710485?

sprankhub

Thanks a lot for your valuable work, @aboritskiy! I left some comments and hope that they make sense to you :) If anything is unclear, tell me.

Model/Config.php

Plugin/Tax/Config/ShippingTaxPlugin.php

aboritskiy · 2020-12-23T15:18:38Z

after a bit more testing I realized this solution is working incorrectly for b2b cases, i.e. when customer has VAT ID set and is getting VAT removed. I wasn't able to fix it right quick, cause there is not enough information in the scope of this pluin to do calculation properly. I have to rework this again. Looks like I also missed couple of edge cases with type hints, typehints are a very minor thing comparing to the b2b issue. This requires further reworking and testing, I will proceed on this the next days and either update this pull-request or submit new one.

I'm not sure if we should keep this one open, for me it will help to see an overview of feedback, so I'd keep it open, but if for you, @sprankhub, it is not cool somehow - feel free to decline this pull-request.

sprankhub · 2020-12-24T11:57:46Z

Thanks for your feedback, @aboritskiy! Happy to keep that open until the new version is implemented.

Merry Christmas!

aboritskiy · 2020-12-29T16:17:19Z

ok, here is a new version, turns out I had to add another completely new feature here to let Magento substract VAT correctly for b2b customers. There is very little left from original approach to be honest. Let's see what Travis says about code style.

look through original code-review comments and cleanup code style, providing a bit more strict, yet more meaningful typhints

also this dependency was causing an exception: report.CRITICAL: Missing required argument $country of FireGento\MageSetup\Model\Config.

otherwise the calculation doesn't have access to the quote, and doesn't work in admin section. I can't say it is nicest possible implementaion, but looking at magento core code, I don't see how to achieve the same effect better, cause magento implies that shipping tax class is a static thing.

regenerated the en_US file, using n98-magerun2.phar i18n:collect-phrases -o vendor/firegento/magesetup2/i18n/en_US.csv vendor/firegento/magesetup2/ also made sure tranlsation keys are on the matching lines between en_US and de_DE files.

in short: add a custom address total processor that will act shortly before tax total processor (see sorting order) and figure out the product tax class with highest tax rate among current cart items. Later on the calculated value is used as shipping tax class.

this commit allows proper calculation when cross-border trade is enabled. In short: * price for end-customers should stay the same no matter the tax * for business customers, VAT should be removed. * VAT amount should be calculated based on destination country default tax rate and NOT default store tax rate like in magento default. Current implementation is hacky and brings duplicaiton, this is mostly caused by the location of the code we need to override here. Target code is located in: Magento\Tax\Model\Calculation\AbstractCalculator that class is inherited by whole bunch of classes: Magento\Tax\Model\Calculation\AbstractAggregateCalculator Magento\Tax\Model\Calculation\RowBaseCalculator Magento\Tax\Model\Calculation\TotalBaseCalculator Magento\Tax\Model\Calculation\UnitBaseCalculator Vertex\Tax\Model\Calculation\VertexCalculator most interesting for us here is RowBaseCalculator and TotalBaseCalculator. We also override UnitBaseCalculator, though I'm not sure if that is needed. I do not override VertexCalculator for 2 reasons: * I didn't test this * somebody who uses Vertex platform will not bother implementing proper tax calculation in magento itself, as Vertex will calculate tax. Possible future improvement here is to rewrite implementation so that required change is not duplicated and injected either as Plugin or at least as Trait. Leaving current implementation is also considered a good option though, cause it heavily relies on initialized dependencies of `AbstractCalculator` some of those dependencies are tricky to get initialized in the plugin, e.g. `storeId` or `getAddressRateRequest()`. More generally I think this solution can be implemented best by patching magento core, author is actually surprised why magento does not support such calculation mechanics out of the box.

move total calculation override to a trait, to reduce code duplication, I'm not using plugins here cause methods that are being overriden are proteced and plugin is not possible. Also adding necessary config flags to control new features

regenerated the en_US file, using n98-magerun2.phar i18n:collect-phrases -o vendor/firegento/magesetup2/i18n/en_US.csv vendor/firegento/magesetup2/ also made sure tranlsation keys are on the matching lines between en_US and de_DE files.

remove typed properties, those are only supported by php7.4

aboritskiy · 2020-12-29T19:03:42Z

Ok, looks like I was able to resolve all code style issues, even those that are also present in develop branch.

sprankhub

Thanks a lot for the update, @aboritskiy! To be honest, I did not test this, but just checked the code and left some comments. If you could add some integration tests, this would be awesome. However, this is not a blocker for me to merge this. It would just help to merge this in good conscience :-)

sprankhub · 2021-01-03T09:29:07Z

etc/config.xml

@@ -13,5 +13,10 @@
                <including_shipping_costs>0</including_shipping_costs>
            </price>
        </catalog>
+        <tax>
+            <calculation>


Should we add classes/dynamic_shipping_tax_class for consistency here as well?

ehm, not really.

I placed it right next to magento's standard tax/calculation/cross_border_trade_enabled

I think I was not clear enough. My idea was to also add a default value for classes/dynamic_shipping_tax_class. IMHO, it makes sense to define a default value for each config value.

etc/di.xml

sprankhub · 2021-01-03T12:45:51Z

etc/adminhtml/system.xml

+            <group id="calculation">
+                <field id="cross_border_trade_advanced_enabled" translate="label" type="select" sortOrder="71" showInDefault="1" showInWebsite="1" showInStore="0">
+                    <label>Enable Cross Border Trade Advanced</label>
+                    <comment>When catalog price includes tax, enable this setting to fix the price for end-customers, but allow business customers to have VAT substracted.</comment>


This is quite a new feature. I am fine with adding it to MageSetup, but I would also like to have the OK from @frostblogNet...

sprankhub · 2021-01-03T12:47:30Z

etc/magesetup.xml

@@ -22,6 +22,8 @@
            <tax__calculation__shipping_includes_tax>1</tax__calculation__shipping_includes_tax>
            <tax__calculation__apply_after_discount>1</tax__calculation__apply_after_discount>
            <tax__calculation__discount_tax>1</tax__calculation__discount_tax>
+            <tax__calculation__cross_border_trade_enabled>0</tax__calculation__cross_border_trade_enabled>


Why has this been added? Doesn't this mean we change this setting during our setup? I think we should not touch this during our setup, because it is not required for any market (especially not for the German market).

well, I don't have strong opinion here - can remove that as well.

Please remove this for the reasons I mentioned above.

sprankhub · 2021-01-03T12:53:10Z

Model/System/Config.php

+     */
+    public function isDynamicShippingTaxClassActive()
+    {
+        return (bool)$this->scopeConfig->getValue(


Can we use isSetFlag?

any thinking behind this? we should probably change other methods as well to match that

Since isSetFlag does exactly what we want, I think it just makes sense to use it. I did not check the old code, but IMHO, we should use it where a bool is retrieved from the config. If you would not like to change this, fine for me.

sprankhub · 2021-01-03T12:53:16Z

Model/System/Config.php

+     */
+    public function isAdvancedCrossBorderTradeActive()
+    {
+        return (bool)$this->scopeConfig->getValue(


Can we use isSetFlag?

Model/Tax/Calculation/AdvancedCrossBorderCalculator.php

sprankhub · 2021-02-18T09:24:00Z

@aboritskiy any update on this? Thanks!

niekappartmedia · 2021-02-18T09:39:24Z

@aboritskiy I have currently installed your pull request. Works like a charm exactly what we need. Only thing is that the shipping price is not changed with this tax class on the shipping method selection block. See screenshots:

In the first screen it should also be 16,35 incl tax.

aboritskiy · 2021-02-18T21:48:12Z

I'm very sorry, but something not nice happened and I have to remove this from public domain.
I might come back with pull-request again after some time.

sprankhub self-requested a review December 22, 2020 09:41

aboritskiy commented Dec 22, 2020

View reviewed changes

Model/Config.php Show resolved Hide resolved

sprankhub requested changes Dec 23, 2020

View reviewed changes

Petar and others added 14 commits December 29, 2020 19:38

Add dynamic shipping tax calculation

4d62e52

Fix review issues and travis errors

45fbf42

Use method to get configuration

3f37502

chore: fix code-style

4417341

look through original code-review comments and cleanup code style, providing a bit more strict, yet more meaningful typhints

fix: remove not used dependency

c97fc24

also this dependency was causing an exception: report.CRITICAL: Missing required argument $country of FireGento\MageSetup\Model\Config.

fix: homogenize i18n files

e72271d

regenerated the en_US file, using n98-magerun2.phar i18n:collect-phrases -o vendor/firegento/magesetup2/i18n/en_US.csv vendor/firegento/magesetup2/ also made sure tranlsation keys are on the matching lines between en_US and de_DE files.

chore: finalization of features and code cleanup

bb4c0d8

move total calculation override to a trait, to reduce code duplication, I'm not using plugins here cause methods that are being overriden are proteced and plugin is not possible. Also adding necessary config flags to control new features

fix: homogenize i18n files

ab22d6b

regenerated the en_US file, using n98-magerun2.phar i18n:collect-phrases -o vendor/firegento/magesetup2/i18n/en_US.csv vendor/firegento/magesetup2/ also made sure tranlsation keys are on the matching lines between en_US and de_DE files.

chore: code-style cleanup

4c592e5

chore: php 7.2 compatibility

66a5f22

remove typed properties, those are only supported by php7.4

chore: fix code-style

1559664

aboritskiy requested a review from sprankhub December 29, 2020 19:03

sprankhub reviewed Jan 3, 2021

View reviewed changes

sprankhub requested a review from frostblogNet January 9, 2021 20:50

aboritskiy closed this Feb 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamic shipping tax class calculation #192

Dynamic shipping tax class calculation #192

aboritskiy commented Dec 21, 2020

sprankhub commented Dec 22, 2020

aboritskiy commented Dec 22, 2020

aboritskiy commented Dec 22, 2020

sprankhub commented Dec 22, 2020

sprankhub left a comment

aboritskiy commented Dec 23, 2020

sprankhub commented Dec 24, 2020

aboritskiy commented Dec 29, 2020

aboritskiy commented Dec 29, 2020

sprankhub left a comment

sprankhub Jan 3, 2021

aboritskiy Jan 5, 2021

sprankhub Jan 9, 2021

sprankhub Jan 3, 2021

sprankhub Jan 3, 2021

aboritskiy Jan 5, 2021

sprankhub Jan 9, 2021

sprankhub Jan 3, 2021

aboritskiy Jan 5, 2021

sprankhub Jan 9, 2021

sprankhub Jan 3, 2021

sprankhub commented Feb 18, 2021

niekappartmedia commented Feb 18, 2021 •

edited

Loading

aboritskiy commented Feb 18, 2021

Dynamic shipping tax class calculation #192

Dynamic shipping tax class calculation #192

Conversation

aboritskiy commented Dec 21, 2020

Issue

Proposed changes

sprankhub commented Dec 22, 2020

aboritskiy commented Dec 22, 2020

aboritskiy commented Dec 22, 2020

sprankhub commented Dec 22, 2020

sprankhub left a comment

Choose a reason for hiding this comment

aboritskiy commented Dec 23, 2020

sprankhub commented Dec 24, 2020

aboritskiy commented Dec 29, 2020

aboritskiy commented Dec 29, 2020

sprankhub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sprankhub commented Feb 18, 2021

niekappartmedia commented Feb 18, 2021 • edited Loading

aboritskiy commented Feb 18, 2021

niekappartmedia commented Feb 18, 2021 •

edited

Loading