-
-
Notifications
You must be signed in to change notification settings - Fork 435
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
Fixed UPS Rest API bugs #3976
Fixed UPS Rest API bugs #3976
Conversation
it's in app/code/core/Mage/Usa/etc/system.xml |
@ragnese what about we include this one https://github.com/magento/magento2/pull/38673/files? |
Co-authored-by: Rob Agnese <rob@hubindustrial.com>
Interesting. So the UPS API doesn't work if you mix metric and imperial units? I guess we probably should include that, then. |
it doesn't work (I had the same problem since I selected "LBS" because the system was mixing KGS and INCHES and UPS didn't like that |
Fair enough. Then we should definitely pull it in. (Though, it's pretty funny to think of a 5 cm x 5 cm x 5 cm package :p ) |
@ragnese done |
Do you think this is worth pushing out as another point release as it is now? On the one hand, it would be nice to wait until we're pretty sure everything works well and only have one more release to fix the UPS REST issues. But, on the other hand, it doesn't seem like a good idea to let the tracking stuff stay broken longer than necessary. Personally, I think the broken tracking is serious enough that this should be merged sooner rather than later and then just tackle future bugs/issues as they come. |
I would wait a few more days, there are people testing this patch so there's no super hurry to do another release immediately, every communication has warnings about this. let's give it a few more days and if everything seems stable we'll go |
also, please leave a code review in order to speed up the merge, I can't always force my hands on merging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We just need to address the comment about the "activity" if-statement.
I'll now merge this since we had no ulterior feedback (my customer say this 20.7 + this PR work fine) and we'll have to release it publicly very soon. |
Today I deployed this patch to production for a customer, tests:
|
my mistake, the production Rest API account wasn't setup correctly and didn't have "tracking" access enabled, enabling it fixed everything. I think I can confirm everything works correctly. I'll post an update in the next few days if there's something worth alerting you all. |
seems like free UPS shipping method do now work. |
@mingjyou please provide more info so that we can test this |
Free Method: UPS Ground ( seems like do not allow to choose free method )
Minimum Order Amount for Free Shipping: 200
do not show free ups ground shipping when check out On Wednesday, May 22, 2024 at 12:21:14 PM EDT, Fabrizio Balliano ***@***.***> wrote:
@mingjyou please provide more info so that we can test this
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Let's use this PR to fix all the bugs we can find regarding the new UPS Rest API.