#11358 Fix full tax summary display breakdown

[adrian-martinez-interactiv4]

Full Tax Summary display wrong numbers.

Description

Full tax summary breakdown fails, as explained in #11358, solution taken from proposed gist: https://gist.github.com/heyqule/7c830137715544ff95baba50a5ed3d80

Fixed Issues (if relevant)

1. Full Tax Summary display wrong numbers. #11358: Full Tax Summary display wrong numbers.

Manual testing scenarios

As explained in #11358

Contribution checklist

• Pull request has a meaningful description of its purpose
• All commits are accompanied by meaningful commit messages
• All new or changed code is covered with unit/integration tests (if applicable)
• All automated tests passed successfully (all builds on Travis CI are green)

[fooman]

thanks @adrian-martinez-interactiv4 this looks to be an important fix to get right. Unfortunately the proposed changes do make a range of tests fail - https://travis-ci.org/magento/magento2/jobs/287781682.

It seems that these changes do have some side effects which either need to be mitigated or we would need to confirm that the original tests are buggy as well. In particular results like https://travis-ci.org/magento/magento2/jobs/287781682#L1115 need to be looked into more closely.

Can you please take a look at these test results and let us know what you think.

[adrian-martinez-interactiv4]

@fooman for sure, I just tried to apply the proposed patch and crossed fingers, but a deeper look shows that actually tests are ok, and they are crashing because this fix doesn't allow compound tax rules by priority anymore (splits them and results are as if they were calculated one after another, what is not the same). I'll update you with more info soon.

[adrian-martinez-interactiv4]

Hi again @fooman , good news and bad news.

The good news are that I have finally achieved the goal of split rate amounts correctly, store them and retrieve them to be displayed in cart and checkout summary, and covered them with tests by adapting the existing tests, adding the expected calculated amount per rate to checks where needed.

The bad new is I've had to add set/get amount methods to a couple interfaces marked as @api:
This one to deal with amount in tax rate:

And I was forced to edit this one, because tax details are retrieved via REST api, and due to nature of the api that uses \Magento\Framework\Reflection\DataObjectProcessor::buildOutputDataArray to transform the objects to array using its interfaces, this change was needed to make the ws return rate amount values:

Codacy is complaining about a single variable, but all travis build is successful:

Can you take a quick look and let me know what you think? Thank you.

[adrian-martinez-interactiv4]

Hi @fooman , any news about this?

[fooman]

@adrian-martinez-interactiv4 thanks for the ping. I am still gathering my thoughts on this one.

What I think so far: Well done! that's indeed good news.

You've made a couple of changes to frontend template files - would you mind confirming why these were needed?

Also looking at the test changes would we need 1 additional scenario that covers a Canadian setup (ie to cover both compound vs cumulative) or was this simply a case of no existing test coverage for this at all?

The interface change - I am digging into this a bit deeper if I can think of something to avoid it I'll let you know. If the interface changes stay this will have create a backwards incompatible change and with that will impact if we can easily backport this / in which version this might land. The better approach is probably to make the api impacting changes as outlined here.

[adrian-martinez-interactiv4]

Hi @fooman , I'll try to answer all your questions. I need your advice for the interface part, it's explained at the end of this comment.

About the templates, app/code/Magento/Tax/view/frontend/web/template/checkout/cart/totals/tax.html and app/code/Magento/Tax/view/frontend/web/template/checkout/summary/tax.html have analog changes, so I'll center the explanation on one of them. Having a result data like the following:

Changes are explained this way:

About the tests, I think its covered by the following integration tests:
\Magento\Tax\Model\TaxCalculationTest::multiRulesTotalBasedDataProvider
\Magento\Tax\Model\TaxCalculationTest::multiRulesRowBasedDataProvider
\Magento\Tax\Model\TaxCalculationTest::multiRulesUnitBasedDataProvider

All of them are based in \Magento\Tax\Model\TaxCalculationTest::getBaseQuoteResult method, that aggregates both cummulative and compound rules:

About the interfaces, I already knew this page: http://devdocs.magento.com/guides/v2.0/contributor-guide/backward-compatible-development/#introduction-of-a-method-to-a-class-or-interface. In first place I refused to create a new interface, since new added methods really should belong to the specified @api interfaces. I'm ready to do it in another way, but I need some advice about what approach to take:

• If I created a new interface, I'd have to make up a new name for it: logic name for interface is original one; given GrandTotalRatesInterface, would I be forced to create a GrandTotalRatesAmountInterface? I think it may depend on the answer to the next question.
• If so, once created, I have two choices:
  • Redefine old interface methods in the new interface, add new methods to new interface, mark old interface as @deprecated
  • Define only new methods in new interface, and make old interface extend new interface (I really think this is not really an option, but I wanted to ask just in case I'm wrong)

If new interface is going to replace old one, and I'm going to replace references to old interfaces in di.xml files and elsewhere it may appear, it should have a representative name, that is already taken by the old interface...

Can you bring some light to all of this? Maybe I am missing something and I'd like to learn how am I supposed to do it properly. Thanks in advance.

[fooman]

@adrian-martinez-interactiv4 thanks a lot for the explanations and outlining the changes. This all looks great to me. In regards to the interface changes.

For GrandTotalRatesInterface you are proposing to make this an ExtensibleDataInterface but your other code changes don't make use of setExtensionAttributes() or getExtensionAttributes(). I am okay with this change just wanted to check if this is deliberate.

If so we can use the following name GrandTotalRatesExtensibleInterface.

Define only new methods in new interface, and make old interface extend new interface (I really think this is not really an option, but I wanted to ask just in case I'm wrong)

This approach seems the best to me for the moment. It allows us to introduce the new one with the least amount of changes (api/deprecation notices) - however it still leaves the other option open (my gut feeling is that there might be further changes coming to the tax module - see for example here).

AppliedTaxRateInterface could be AppliedTaxRateWithAmountInterface

[orlangur]

Closing due to inactivity. Feel free to reach me out anytime later if you wish to continue work on this pull request and it will be reopened.