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

UPS Rest API: handling cases where a single service is returned by UPS #4044

Merged
merged 1 commit into from
Jun 20, 2024
Merged

UPS Rest API: handling cases where a single service is returned by UPS #4044

merged 1 commit into from
Jun 20, 2024

Conversation

fballiano
Copy link
Contributor

Everything is explained in #4043

Fixes #4043

@github-actions github-actions bot added the Component: Usa Relates to Mage_Usa label Jun 17, 2024
@kiatng
Copy link
Contributor

kiatng commented Jun 19, 2024

Ref issue #4043, the property "RatedShipment" is usually an array, but if there is only one item in it, then it is an object.

isset($arr['Service']) looks for the key "Service" in $arr (JSON object) and return true if it's truthy. If the key doesn't exist, it returns false.

@F1Red5 proposal is_string(array_key_first($arr)) returns true on "RatedShipment" originally is a JSON object, it doesn't check that the object has the property "Service".

Based on this, I think testing "RatedShipment" for an object is may be better as it doesn't assume what keys $arr has. But may be it's necessary to check for the key "Service", so I don't know. I defer the decision to the PR author @fballiano.

@fballiano
Copy link
Contributor Author

the code object of this PR parses a maximum of 10 records, I would never trade readability and understandability of code for such a minor improvement in this context. if we were parsing millions then we should be having this talk, but having this talk now for this specific thing, in my opinion, doesn't make any sense. we should have this lever of participation with the other million things we should be doing instead of wasting energy for this.

I've personally tested all of the UPS implementation, @iamniels tested that this PR works for him (because he submitted it actually) so I really cannot understand all of this chat.

Copy link
Contributor

@kiatng kiatng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fballiano thanks for the clarification and your efforts!

@fballiano fballiano merged commit 365aece into OpenMage:main Jun 20, 2024
17 checks passed
@fballiano fballiano deleted the ups_single_service branch June 20, 2024 16:13
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.

UPS REST _parseRestResponse doesn't return rates when only one service is available for a destination
5 participants