[Backport] Fix DHL Quotes for Domestic Shipments when Content Type is set to Non-Document

[gwharton]

Original PR (*)

1. PR Fix DHL Quotes for Domestic Shipments when Content Type is set to Non-Document #19487

Description (*)

1. Updated label for Content Type configuration field to make it clear what it is.
2. Changed Allowable Methods (Non Doc and Doc) to always be displayed no
   matter what the content type mode is. Makes no sense to display one or the other.
3. Changed the isDutiable function to only look at whether a shipment is
   domestic or not. Previously it would return dutiable for everything
   if the Content Type mode is non document (which causes problems for
   domestic shipments in Non Document mode). Fixes DHL Shipping Quotes fail for Domestic Shipments when Content Mode is "Non Documents" #19485

Fixed Issues (if relevant)

1. DHL Shipping Quotes fail for Domestic Shipments when Content Mode is "Non Documents" #19485:DHL Shipping Quotes fail for Domestic Shipments when Content Mode is "Non Documents"

Manual testing scenarios (*)

1. Set Shipment Source to UK, SW1 1AA
2. Setup DHL module to obtain online quotes
3. Set Content Type to "Non Document"
4. Obtain quotes for destinations
   4.1 UK, SW1 1AA (UK -> UK - Domestic - Documents - Non dutiable)
   4.2 FR, 75001 (UK -> EU - Domestic - Documents - Non dutiable)
   4.3 NO, 4032 (UK -> Europe - Intl - Non Documents - Dutiable)
   4.4 US, 90210 (UK -> World - Intl - Non Documents - Dutiable)
5. Ensure quotes are received for all of the above regions.

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)

[magento-engcom-team]

Hi @gwharton. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento-engcom-team give me test instance - deploy test instance based on PR changes
• @magento-engcom-team give me 2.2-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

[magento-engcom-team]

Hi @orlangur, thank you for the review.
ENGCOM-3602 has been created to process this Pull Request

[sivaschenko]

Hello @gwharton , can you please add a unit test fix from the original PR to this pull request.

[gwharton]

Just waiting on the travis/codacy checks. I'll address any failures and then thats it.

There are some changes in the way the 2.2 and 2.3 DHL module operates. In 2.2 the shipment request xml document generated by the Carrier model is different depending on which region the shipment originates from, whereas in 2.3 the DHL API was updated and the request xml document is the same regardless of source region, but now contains subtle differences depending on whether the shipment is domestic or international.

There are also additional features in the DHL API present in 2.3 which are not present in 2.2 and do not require testing.

I began by discarding the existing 2.2 tests and replacing with 2.3 tests, and then working back, removing/changing content as required.

This has means that I had to make some substantial changes to the tests when compared to the 2.3 codebase, however it is broadly inline and tests the same things in the same way, albeit with the 2.3 specific parts removed, and alterations for testing source region.

[soleksii]

Hi @gwharton ! I've checked PR and during testing, we faced an issue.

Problem: When we select some Non-document allowed methods:

it returns all the methods when creating an order(for example: France, ZIP code:75001, UK -> FR )

Is it the right behavior?

Thanks!

[gwharton]

@stoleksiy It's really confusing I know.

When you request UK->UK the module classes this as a domestic shipment. The services returned will be filtered through both the "Documents Allowed Methods" and "Non Documents Allowed Methods" setting.

When you request UK->EU the module classes this as a domestic shipment. The services returned will be filtered through both the "Documents Allowed Methods" and "Non Documents Allowed Methods" setting.

When you request UK->World (Non EU) the module classes this as a non domestic shipment. If the "Content Type (Non Domestic)" setting is set to "Documents", then the services returned will be filtered through the "Documents Allowed Methods" setting. If the "Content Type (Non Domestic)" setting is set to "Non Documents", then the services returned will be filtered through the "Non Documents Allowed Methods" setting.

So I can see in your test case, you requested UK -> EU. The setting "Content Type (Non Domestic)" would be ignored (The module classes this as domestic) and the services that match those selected in "Documents Allowed Methods" and "Non Documents Allowed Methods" would be shown. In your case, you weren't filtering any out any Non Document methods as they are all selected by default.

It is super confusing I agree. With regard to the DHL module the words "Domestic" and "Non Domestic" actually mean whether the shipment starts and ends in the EU, and the module classes all of these shipments as domestic.

You have the additional confusion when sending Non Domestic (argh, EU -> Non EU) where the same service may be offered twice by DHL, i.e you get offered Express 12:00 as Document type and Express 12:00 as Non Document type. This is the reason for the "Content Type (Non Domestic)" setting.

I agree that the problem is the definition and use of the word "Domestic" is confusing. The module defines it as everything within the EU is domestic or shipments within the same country are domestic, but that does not match what a users definition of what Domestic means and leads to this confusion. This is why the note was added below the setting to try to explain what the setting means. It also states that "Shipments within the EU are classed as domestic". Not ideal I know.

Perhaps in the short term the Phrase "Content Type (Non Domestic)" could be changed in the frontend. But really, the module needs to be reworked in the code to get rid of the "domestic" setting altogether.

Let me know if you need me to action this either now (in this PR, and create a new associated PR for 2.3, as this change is already live in 2.3) or propose further changes in a new issue.

[orlangur]

@stoleksiy this is a backport, by the way. Do we have the same behavior you observe on PR branch on 2.3-develop now?

[gwharton]

@orlangur Yes this is already in 2.3 Note that the behaviour seen in this testing is the expected behaviour (although not optimal). See my previous comments.

[soleksii]

@gwharton Thanks for explanation!
I think it can be marked as QA passed. You could propose further changes in new pull requests (for 2.2 and 2.3 versions).

[soleksii]

✔️ QA Passed

[orlangur]

Thanks @gwharton, I didn't even try to read it unless it would be unavoidable :)

[m2-assistant]

Hi @gwharton, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.