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

Fix Fedex tracking error #8220

Closed
wants to merge 2 commits into from
Closed

Fix Fedex tracking error #8220

wants to merge 2 commits into from

Conversation

casconed
Copy link

closes #8219

Preconditions

  1. Magento 2.1.2
  2. PHP 7
  3. Fedex configured in Sandbox mode with appropriate credentials

Steps to reproduce

  1. Create Fedex Shipment from the Admin
  2. Use tracking 122816215025810
  3. Click tracking link to open popup window

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Jan 20, 2017

CLA assistant check
All committers have signed the CLA.

@@ -1571,7 +1571,7 @@ private function processTrackingDetails(\stdClass $trackInfo)
];

if (!empty($trackInfo->ShipTimestamp)) {
$datetime = \DateTime::createFromFormat(\DateTime::ISO8601, $trackInfo->ShipTimestamp);
$datetime = \DateTime::createFromFormat('U', strtotime($trackInfo->ShipTimestamp));
Copy link
Contributor

Choose a reason for hiding this comment

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

According to FedEx API docs, date time should be with time zone, but sometimes API returns it without timezone. This fix is incorrect.

Copy link
Author

@casconed casconed Jan 23, 2017

Choose a reason for hiding this comment

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

This fix accounts for all possibilities, time zone or otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

If a timezone is not specified, the time will be parsed according to configured timezone for Magento store, but I think it's more correctly to parse time in that case with UTC timezone.

Copy link
Author

Choose a reason for hiding this comment

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

thanks for clarifying - i'll make that change.

Copy link
Contributor

@joni-jones joni-jones Jan 23, 2017

Choose a reason for hiding this comment

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

You don't need to do any changes because this issue already fixed by Magento team and will be delivered soon. Anyway, thanks for fix and your time.

Copy link
Contributor

Choose a reason for hiding this comment

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

UPD FedEx support has confirmed, if date time doesn't contain timezone, when date time in UTC.

@joni-jones
Copy link
Contributor

Rejecting your PR because this issue fixed in the scope of internal ticket MAGETWO-63215.

@joni-jones joni-jones added Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Progress: reject labels Jan 30, 2017
@casconed
Copy link
Author

Great, so when will that be merged?

@joni-jones
Copy link
Contributor

This issue fixed and now all flow for tracking numbers should be covered by functional tests. As soon as functional tests will be finished, this fix will be merged.

@okorshenko
Copy link
Contributor

@casconed Thank you for your contribution to Magento 2 project. This issue already fixed and on the way to develop branch. We will update you in #8219
Closing this PR.

@okorshenko okorshenko closed this Feb 7, 2017
magento-devops-reposync-svc pushed a commit that referenced this pull request May 6, 2023
Mftf test cleanup and tag cloud group
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Shipping Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Progress: reject
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants