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

Fixed UPS Rest API bugs #3976

Merged
merged 11 commits into from
May 12, 2024
Merged

Fixed UPS Rest API bugs #3976

merged 11 commits into from
May 12, 2024

Conversation

fballiano
Copy link
Contributor

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

@ragnese
Copy link
Contributor

ragnese commented May 7, 2024

Where is the code that displays the message for the adminhtml form item here?
Screenshot 2024-05-07 at 3 09 40 PM

@fballiano
Copy link
Contributor Author

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

@fballiano
Copy link
Contributor Author

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

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

ragnese commented May 7, 2024

@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
Copy link
Contributor Author

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
Copy link
Contributor

ragnese commented May 7, 2024

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
Copy link
Contributor Author

@ragnese done

@ragnese
Copy link
Contributor

ragnese commented May 8, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@fballiano fballiano requested a review from ragnese May 8, 2024 12:27
Copy link
Contributor

@ragnese ragnese left a 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.

@fballiano
Copy link
Contributor Author

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 fballiano merged commit cda6275 into OpenMage:main May 12, 2024
17 checks passed
@fballiano fballiano deleted the upsrest3 branch May 12, 2024 08:53
@fballiano
Copy link
Contributor Author

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

  • post deploy, without configuration change -> everything ok (still using UPS XML APIs)
  • post deploy, after switching to Rest API -> shipping API works ok, tracking doesn't work, I get [result] => {"response":{"errors":[{"code":"250002","message":"Invalid Authentication Information."}]}}

@fballiano
Copy link
Contributor Author

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
Copy link

seems like free UPS shipping method do now work.

@fballiano
Copy link
Contributor Author

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

@mingjyou
Copy link

mingjyou commented May 22, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Usa Relates to Mage_Usa
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants