OpenMage / magento-lts

Official OpenMage LTS codebase | Migrate easily from Magento Community Edition in minutes! Download the source code for free or contribute to OpenMage LTS | Security vulnerability patches, bug fixes, performance improvements and more.
https://www.openmage.org
Open Software License 3.0
863 stars 438 forks source link

Fixed UPS Rest API bugs #3976

Closed fballiano closed 1 month ago

fballiano commented 1 month ago

Let's use this PR to fix all the bugs we can find regarding the new UPS Rest API.

ragnese commented 1 month ago

Where is the code that displays the message for the adminhtml form item here?

Screenshot 2024-05-07 at 3 09 40 PM
fballiano commented 1 month ago

it's in app/code/core/Mage/Usa/etc/system.xml

fballiano commented 1 month ago

@ragnese what about we include this one https://github.com/magento/magento2/pull/38673/files?

ragnese commented 1 month ago

@ragnese what about we include this one https://github.com/magento/magento2/pull/38673/files?

Interesting. So the UPS API doesn't work if you mix metric and imperial units? I guess we probably should include that, then.

fballiano commented 1 month ago

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

ragnese commented 1 month ago

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 )

fballiano commented 1 month ago

@ragnese done

ragnese commented 1 month ago

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.

fballiano commented 1 month ago

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

fballiano commented 1 month ago

also, please leave a code review in order to speed up the merge, I can't always force my hands on merging

fballiano commented 1 month ago

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.

fballiano commented 1 month ago

Today I deployed this patch to production for a customer, tests:

fballiano commented 1 month ago

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.

mingjyou commented 1 month ago

seems like free UPS shipping method do now work.

fballiano commented 1 month ago

@mingjyou please provide more info so that we can test this

mingjyou commented 1 month ago

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: @.***>