UPS Option to include TAX in rate #11707

gwharton · 2017-10-24T21:57:58Z

The current UPS Module uses the UPS Rate excluding TAX to use as the rate charged to the customer during checkout.

This modification adds an additional config option so you can choose whether to request and include TAX information from UPS in the rate or not.

This modification is especially useful for businesses who are not TAX registered and cannot claim back the TAX charged by UPS. This allows them to pass on the TAX costs to the customer as part of the overall shipping rate.

Description

Fixed Issues (if relevant)

magento/magento2#<issue_number>: Issue title
...

Manual testing scenarios

...
...

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)

The current UPS Module uses the UPS Rate excluding TAX to use as the rate charged to the customer during checkout. This modification adds an additional config option so you can choose whether to request and include TAX information from UPS in the rate or not. This modification is especially useful for businesses who are not TAX registered and cannot claim back the TAX charged by UPS. This allows them to pass on the TAX costs to the customer as part of the overall shipping rate.

magento-cicd2 · 2017-10-24T21:58:01Z

All committers have signed the CLA.

Remove blank line that was added by mistake

This pull request adds enough lines of code to the function to trigger the excessive method length check in PHPMD. The majority of the code in the function is not mine and I do not want to change stuff for the sake of passing this check. My goal was to make the changes with as little impact on the existing code base as possible.

gwharton · 2017-11-08T11:07:01Z

I branched 2.2-develop for this fix, but if approved can it be merged into 2.3-develop aswell so future releases get the changes.

misha-kotov · 2018-01-08T18:44:15Z

Hi @gwharton , thanks for your contribution. A couple things:

Include TAX in Rate should be changed to Include Tax in Rate in the admin panel configuraitons
Can you clarify where in the admin panel config this new setting would appear?
What's name of the new response field that will be returned in addition to the existing GrandTotal ?

Changed admin panel label from TAX to Tax

gwharton · 2018-01-13T12:24:47Z

The admin panel setting appears below the "Enable Negotiated Rates" setting, but only when UPS Type is set to "United Parcel Service XML". Getting TAX information (and negotiated rates) are not supported in the non XML service type.

gwharton · 2018-01-13T12:31:49Z

Here are the responses received for the following combinations with negotiated rates set

For a UK-UK shipment (taxes applicable)

Include Tax in rate = NO

    <NegotiatedRates>
      <NetSummaryCharges>
        <GrandTotal>
          <CurrencyCode>GBP</CurrencyCode>
          <MonetaryValue>9.35</MonetaryValue>
        </GrandTotal>
      </NetSummaryCharges>
    </NegotiatedRates>

Include Tax in rate = YES

    <NegotiatedRates>
      <TaxCharges>
        <Type>VAT</Type>
        <MonetaryValue>1.87</MonetaryValue>
      </TaxCharges>
      <NetSummaryCharges>
        <GrandTotal>
          <CurrencyCode>GBP</CurrencyCode>
          <MonetaryValue>9.35</MonetaryValue>
        </GrandTotal>
        <TotalChargesWithTaxes>
          <CurrencyCode>GBP</CurrencyCode>
          <MonetaryValue>11.22</MonetaryValue>
        </TotalChargesWithTaxes>
      </NetSummaryCharges>
    </NegotiatedRates>

For a UK-USA shipment (taxes NOT applicable)

Include Tax in rate = NO

    <NegotiatedRates>
      <NetSummaryCharges>
        <GrandTotal>
          <CurrencyCode>GBP</CurrencyCode>
          <MonetaryValue>44.37</MonetaryValue>
        </GrandTotal>
      </NetSummaryCharges>
    </NegotiatedRates>

Include Tax in rate = YES

    <NegotiatedRates>
      <NetSummaryCharges>
        <GrandTotal>
          <CurrencyCode>GBP</CurrencyCode>
          <MonetaryValue>44.37</MonetaryValue>
        </GrandTotal>
      </NetSummaryCharges>
    </NegotiatedRates>

gwharton · 2018-01-13T12:35:36Z

And the same responses received without negotiated rates set

For a UK-UK shipment (taxes applicable)

Include Tax in rate = NO

    <TransportationCharges>
      <CurrencyCode>GBP</CurrencyCode>
      <MonetaryValue>6.45</MonetaryValue>
    </TransportationCharges>
    <ServiceOptionsCharges>
      <CurrencyCode>GBP</CurrencyCode>
      <MonetaryValue>0.00</MonetaryValue>
    </ServiceOptionsCharges>
    <TotalCharges>
      <CurrencyCode>GBP</CurrencyCode>
      <MonetaryValue>6.45</MonetaryValue>
    </TotalCharges>

Include Tax in rate = YES

    <TransportationCharges>
      <CurrencyCode>GBP</CurrencyCode>
      <MonetaryValue>6.45</MonetaryValue>
    </TransportationCharges>
    <ServiceOptionsCharges>
      <CurrencyCode>GBP</CurrencyCode>
      <MonetaryValue>0.00</MonetaryValue>
    </ServiceOptionsCharges>
    <TaxCharges>
      <Type>VAT</Type>
      <MonetaryValue>1.29</MonetaryValue>
    </TaxCharges>
    <TotalCharges>
      <CurrencyCode>GBP</CurrencyCode>
      <MonetaryValue>6.45</MonetaryValue>
    </TotalCharges>
    <TotalChargesWithTaxes>
      <CurrencyCode>GBP</CurrencyCode>
      <MonetaryValue>7.74</MonetaryValue>
    </TotalChargesWithTaxes>

For a UK-USA shipment (taxes NOT applicable)

Include Tax in rate = NO

    <TransportationCharges>
      <CurrencyCode>GBP</CurrencyCode>
      <MonetaryValue>35.16</MonetaryValue>
    </TransportationCharges>
    <ServiceOptionsCharges>
      <CurrencyCode>GBP</CurrencyCode>
      <MonetaryValue>0.00</MonetaryValue>
    </ServiceOptionsCharges>
    <TotalCharges>
      <CurrencyCode>GBP</CurrencyCode>
      <MonetaryValue>35.16</MonetaryValue>
    </TotalCharges>

Include Tax in rate = YES

    <TransportationCharges>
      <CurrencyCode>GBP</CurrencyCode>
      <MonetaryValue>35.16</MonetaryValue>
    </TransportationCharges>
    <ServiceOptionsCharges>
      <CurrencyCode>GBP</CurrencyCode>
      <MonetaryValue>0.00</MonetaryValue>
    </ServiceOptionsCharges>
    <TotalCharges>
      <CurrencyCode>GBP</CurrencyCode>
      <MonetaryValue>35.16</MonetaryValue>
    </TotalCharges>

gwharton · 2018-01-13T12:44:09Z

The code change checks for the presence of the TaxCharges field. If the field is present and the option to include tax in rate is set to Yes, then the MonetaryValue of the TaxCharges field is added to the rate.

There is an alternative method to get the total including tax as if tax is applicable to the rate then the additional field "TotalChargesWithTaxes" could be used instead of summing the two fields to obtain the total including tax, but I chose not to use this option to minimise the amount of code changes. Let me know if this method is preferred and I will update.

misha-kotov · 2018-01-19T23:12:23Z

@gwharton sorry for the delay. I think this will all work, but I'm wondering if it's better for naming consistency to name "total" and "total with taxes" fields like this:

Negotiated rates = ON, Include Tax in Rate = ON
GrandTotal
GrantTotalWithTaxes -> update this name

Negotiated rates = OFF, , Include Tax in Rate = ON
TotalCharges
TotalChargesWithTaxes

One last thing: I'd prefer to change the admin panel setting name slightly. Currently the name Include Tax in Total is confusing bc it can be interpreted as "the tax value will be included in the exiting 'total' field".
I propose to call it something like Request Tax-Inclusive Rate with hint text of "When available, this value is returned in addition to totals without tax".

Updated admin panel label to "Request Tax-Inclusive Rate" Added comment "When applicable, taxes (sales tax, VAT etc.) are included in the rate". The above comment seems more appropriate than the one suggested as "When available, this value is returned in addition to totals without tax" would mean nothing to the end user. All he knows about/cares about is that the end total includes any taxes or not.

…aths. Previously the routine obtained the base cost, and then if taxes existed, it added the tax on to get the total. Now the function checks for the presence of the "TotalChargesWithTaxes" field. If present and the "Request Tax-Inclusive Rate" option is set, then the total is obtained from the "TotalChargesWithTaxes" field. If not present, or the "Request Tax-Inclusive Rate" option is not set, then the total is obtained from the original field which does not include tax. Also moved the setting of the "responseCurrencyCode" variable into the same logic. This ensures that the correct response currency code is always set correctly based on the value returned from UPS. Previously "responseCurrencyCode" would only have ever been set correctly if negotiated rates were never used.

gwharton · 2018-01-23T12:01:29Z

@misha-kotov I have updated the labelling on the admin panel to be inline with your comments, although I have proposed a slightly different comment to the suggested one.

I have also changed the logic to get rid of the maths. There is no point in obtaining the base charge, then the taxes and adding the two together, when the total including tax is also returned from UPS and can be used directly.

I have also fixed a problem with the setting of the responseCurrencyCode which was not working correctly when negotiated rates were used, and was made worse by my original commit.

With regard to the naming of the fields in the response, these are the field names that are returned by UPS in their API and are beyond our control I am afraid. They're not consistent, but we are stuck with them.

Updated admin panel label and comment in i18n

okorshenko · 2018-01-25T20:47:36Z

Hi @gwharton thank you for update. Could you please cover your improvement with integration tests so that we can proceed with PR acceptance
Thank you

misha-kotov · 2018-01-25T20:58:29Z

@gwharton thanks for your edits. Now that you changed the math calculations, can you provide examples of how it looks now? The 4 combinations/examples you gave above were very useful.

gwharton · 2018-01-26T10:55:59Z

@misha-kotov There are 8 possible routes through the code. I will summarise all 8 here.

Option 1

Negotiated Rates Option set to "NO"
Request Tax Inclusive Rate Option set to "NO"
Shipping source and destination means tax is applicable to shipment (ex UK -> UK)