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.

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

New feature: Added support for UPS Rest APIs #3878

Merged
merged 56 commits into from
May 7, 2024
Merged

New feature: Added support for UPS Rest APIs #3878

merged 56 commits into from
May 7, 2024

Conversation

fballiano
Copy link
Contributor

@fballiano fballiano commented Mar 6, 2024

3rd of June 2024 UPS will shut down their SOAP APIs as stated on their website here:
https://developer.ups.com/oauth-developer-guide?loc=en_US

All the websites using UPS will stop working on that date

This PR is aims to port what M2 did to implement UPS Rest APIs with:
https://experienceleague.adobe.com/docs/commerce-knowledge-base/kb/troubleshooting/known-issues-patches-attached/ups-shipping-method-integration-migration-from-soap-to-restful-api.html

Most of the changes in M2 as in these 2 files:

@github-actions github-actions bot added Template : admin Relates to admin template Component: Usa Relates to Mage_Usa Component: Adminhtml Relates to Mage_Adminhtml labels Mar 6, 2024
@fballiano fballiano marked this pull request as draft March 6, 2024 23:48
@fballiano
Copy link
Contributor Author

I have something working

Screenshot 2024-03-10 alle 18 12 21

@fballiano
Copy link
Contributor Author

it kinda works now:

Screenshot 2024-03-12 alle 21 00 40

at least until creating an order, in the backend is shown correctly:
Screenshot 2024-03-12 alle 21 01 57

I don't know what other tests are needed after the order.

@fballiano
Copy link
Contributor Author

@Flyingmana I think we should add a notification in the the backend for all the users about this

@ragnese
Copy link
Contributor

ragnese commented Apr 30, 2024

Don't we also need to do getTracking for Rest?

Co-authored-by: Rob Agnese <rob@hubindustrial.com>
@fballiano
Copy link
Contributor Author

About this PR, I'm waiting for some tests results by a customer, it seems to be working fine (for sure it's not perfect but it is what it is).

Since 3rd June is near the corner, in case I don't get any feedback from the community here, I'll still merge this PR and release 20.7.0 with a note about this particular feature.

And after june we'll remove the code about XML and re-release.

@fballiano
Copy link
Contributor Author

It's 7th may, regardless of the tests results I think it's time this PR gets merged and released to give merchants a few weeks to test it out and give feedback.

I'll merge and release 20.7.0 with a clear statement about this UPS module.

Let's hope for the best.

@fballiano fballiano merged commit 77c0df5 into OpenMage:main May 7, 2024
14 checks passed
@fballiano fballiano deleted the upsrest2 branch May 7, 2024 08:13
@fballiano fballiano changed the title [HELP NEEDED] Added support for UPS Rest APIs New feature: Added support for UPS Rest APIs May 7, 2024
@ragnese
Copy link
Contributor

ragnese commented May 7, 2024

I know it's a little late now, but I just thought of something while I was working on bringing this change into our production system: Should we have used the "live" URLs (onlinetools.ups.com) for the defaults in config.xml, instead of the testing URLs (wwwcie.ups.com)?

Do you think it would be worth doing a quick patch for that?

@fballiano
Copy link
Contributor Author

@ragnese if you configure it in "live" mode it uses the live URL right? I think it does

@fballiano
Copy link
Contributor Author

I've received this:

Screenshot 2024-05-07 alle 18 00 08

@fballiano
Copy link
Contributor Author

if I click here:

Screenshot 2024-05-07 alle 18 08 28

I get

Screenshot 2024-05-07 alle 18 08 16

@ragnese
Copy link
Contributor

ragnese commented May 7, 2024

First about the config params: The way we have it now, it'll use what's set in the config if it's there. Then it'll fallback to either the live or default URLs if the config value is falsey.

I'll look at the tracking error. It looks like the request is actually returning an error response and we're just not handling that correctly. So, we need to handle the bad response better, but we also need to figure out what the actual issue is with the request. I'm looking into it now.

@fballiano
Copy link
Contributor Author

what if we remove the configuration of the API URLs? I don't even know what it was done that way :-\

@ragnese
Copy link
Contributor

ragnese commented May 7, 2024

I agree that making them configurable was always a weird choice. But, that could turn into a breaking change for anyone who might be pointing to some custom proxy to escape a firewall or something... who knows what weird things people may be doing...

On the tracking error, it looks like you might have the error response string in that screenshot, but it's cut off (the "#1" line of the stack trace). Could you post what that response actually says? I bet it's that we don't have tracking permission on the auth. I noticed that the Admin panel config for the clientId and clientSecret list the permissions to acquire when setting up a UPS app, but it doesn't list "tracking" as one of the needed permissions, so that's probably it.

@fballiano
Copy link
Contributor Author

@ragnese the error is

Array ( [code] => TV0011 [message] => Missing transactionSrc )

@ragnese
Copy link
Contributor

ragnese commented May 7, 2024

Okay, that's an easy fix. We just need to add any kind of identifier that is "us". For other code I've used, we used our UPS merchantId, but since that's not something we ask for in the config, we could just put something like "OpenMage" and I think that'll be fine.

@fballiano
Copy link
Contributor Author

@ragnese I started to post something at #3976 let's use that new PR for the new addition :-)

@sebadiginet
Copy link

HI

There is one bug/problem - currency change is not working. Our calculation from UPS we get in PLN. We have same result in debug shipping_ups.log in PLN and EUR currency websites so its proper answer from API UPS. But checkout in website EUR currency we have not available any shipping method by UPS. Normally its working without problem with exchange made in magento for both previous version of UPS and for Fedex.

@sebadiginet
Copy link

Probably i found reason - line in Ups.php - $allowedCurrencies = Mage::app()->getStore()->getAvailableCurrencyCodes(); is wrong because it should be $allowedCurrencies = Mage::getModel('directory/currency')->getConfigAllowCurrencies();
Store view can have other currencies allowed then response from UPS. Calculation should be made by global allowed currencies for openmage. In such correction its working

@iamniels
Copy link
Contributor

I ran into a bug, I got this error:

Code: 110548
Message: A shipment cannot have a KGS/IN or LBS/CM or OZS/CM as its unit of measurements

I checked the request and it contained:

"Package": [
                {
                    "PackagingType": {
                        "Code": "00",
                        "Description": "Packaging"
                    },
                    "Dimensions": {
                        "UnitOfMeasurement": {
                            "Code": "IN",
                            "Description": "Inches"
                        },
                        "Length": "5",
                        "Width": "5",
                        "Height": "5"
                    },
                    "PackageWeight": {
                        "UnitOfMeasurement": {
                            "Code": "KGS"
                        },
                        "Weight": "1.265"
                    }
                }

By removing the Dimensions the error has been resolved.

@sebadiginet
Copy link

I got error today when try to configure firecheckout extension :
looks that error with that line in Ups.php : $methods[$methodCode] = $methodData->getText();

Fatal error: Uncaught Error: Call to a member function getText() on string in /opt/lampp/htdocs/hpc/app/code/core/Mage/Usa/Model/Shipping/Carrier/Ups.php:1344 Stack trace: #0 /opt/lampp/htdocs/hpc/app/code/local/TM/FireCheckout/Model/System/Config/Source/Shipping/Allmethods.php(31): Mage_Usa_Model_Shipping_Carrier_Ups->getAllowedMethods() #1 /opt/lampp/htdocs/hpc/app/code/core/Mage/Adminhtml/Block/System/Config/Form.php(463): TM_FireCheckout_Model_System_Config_Source_Shipping_Allmethods->toOptionArray(false) #2 /opt/lampp/htdocs/hpc/app/code/core/Mage/Adminhtml/Block/System/Config/Form.php(229): Mage_Adminhtml_Block_System_Config_Form->initFields(Object(Varien_Data_Form_Element_Fieldset), Object(Mage_Core_Model_Config_Element), Object(Mage_Core_Model_Config_Element)) #3 /opt/lampp/htdocs/hpc/app/code/core/Mage/Adminhtml/Block/System/Config/Form.php(164): Mage_Adminhtml_Block_System_Config_Form->_initGroup(Object(Varien_Data_Form), Object(Mage_Core_Model_Config_Element), Object(Mage_Core_Model_Config_Element)) #4 /opt/lam in /opt/lampp/htdocs/hpc/app/code/core/Mage/Usa/Model/Shipping/Carrier/Ups.php on line 1344

@fballiano
Copy link
Contributor Author

@sebadiginet already solved here: #4013

please open new issues because it's much more difficult to track comments on old PRs

@sebadiginet
Copy link

There is possibility to add estimated delivery time for new version of ups ? I have it ready working for fedex also with newer wsdl RateService_v31.wsdl which support new shipping methods like fedex regional - i can share with openmage to make upgrade for new version. Its described for example here https://magento.stackexchange.com/questions/147208/ups-api-estimated-delivery-date

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Adminhtml Relates to Mage_Adminhtml Component: Usa Relates to Mage_Usa documentation help wanted phpstan Template : admin Relates to admin template
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants