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.

Sign up for GitHub

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

Jump to bottom

Dynamic shipping tax class calculation #192

Closed
wants to merge 14 commits into from
Closed

Dynamic shipping tax class calculation #192

wants to merge 14 commits into from

Commits on Dec 29, 2020

  1. Add dynamic shipping tax calculation

    Petar authored and Anton Boritskiy committed Dec 29, 2020
    Configuration menu
    Copy the full SHA
    4d62e52 View commit details
    Browse the repository at this point in the history

  2. Fix review issues and travis errors

    Petar authored and Anton Boritskiy committed Dec 29, 2020
    Configuration menu
    Copy the full SHA
    45fbf42 View commit details
    Browse the repository at this point in the history

  3. Use method to get configuration

    Petar authored and Anton Boritskiy committed Dec 29, 2020
    Configuration menu
    Copy the full SHA
    3f37502 View commit details
    Browse the repository at this point in the history

  4. chore: fix code-style 

    look through original code-review comments and cleanup code style,
providing a bit more strict, yet more meaningful typhints
    Anton Boritskiy committed Dec 29, 2020
    Configuration menu
    Copy the full SHA
    4417341 View commit details
    Browse the repository at this point in the history

  5. fix: remove not used dependency 

    also this dependency was causing an exception:

    report.CRITICAL: Missing required argument $country of FireGento\MageSetup\Model\Config.
    Anton Boritskiy committed Dec 29, 2020
    Configuration menu
    Copy the full SHA
    c97fc24 View commit details
    Browse the repository at this point in the history

  6. fix: move the shipping tax calculation one level up in call-stack 

    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.
    Anton Boritskiy committed Dec 29, 2020
    Configuration menu
    Copy the full SHA
    540b020 View commit details
    Browse the repository at this point in the history

  7. fix: homogenize i18n files 

    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.
    Anton Boritskiy committed Dec 29, 2020
    Configuration menu
    Copy the full SHA
    e72271d View commit details
    Browse the repository at this point in the history

  8. feat: WIP trying out different approach 

    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.
    Anton Boritskiy committed Dec 29, 2020
    Configuration menu
    Copy the full SHA
    1a64779 View commit details
    Browse the repository at this point in the history

  9. feat: add advanced cross-border trade feature. 

    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.
    Anton Boritskiy committed Dec 29, 2020
    Configuration menu
    Copy the full SHA
    543f02a View commit details
    Browse the repository at this point in the history

  10. chore: finalization of features and code cleanup 

    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
    Anton Boritskiy committed Dec 29, 2020
    Configuration menu
    Copy the full SHA
    bb4c0d8 View commit details
    Browse the repository at this point in the history

  11. fix: homogenize i18n files 

    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.
    Anton Boritskiy committed Dec 29, 2020
    Configuration menu
    Copy the full SHA
    ab22d6b View commit details
    Browse the repository at this point in the history

  12. chore: code-style cleanup

    Anton Boritskiy committed Dec 29, 2020
    Configuration menu
    Copy the full SHA
    4c592e5 View commit details
    Browse the repository at this point in the history

  13. chore: php 7.2 compatibility 

    remove typed properties, those are only supported by php7.4
    Anton Boritskiy committed Dec 29, 2020
    Configuration menu
    Copy the full SHA
    66a5f22 View commit details
    Browse the repository at this point in the history

  14. chore: fix code-style

    Anton Boritskiy committed Dec 29, 2020
    Configuration menu
    Copy the full SHA
    1559664 View commit details
    Browse the repository at this point in the history