-
-
Notifications
You must be signed in to change notification settings - Fork 449
Add Fedex REST API rate and tracking functionality, to co-exist with Fedex WSDL #4374
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
base: main
Are you sure you want to change the base?
Conversation
Fedex is currently in transition from WSDL to REST API. This change allows configurable usage of WSDL or REST API. Note: uses shipstream/fedex-rest-sdk, but currently it's not updated with fedex tracking API schema, so need to use a patch branch for it. Also composer installing shipstream/fedex-rest-sdk requires PHP 8.1+, so I've to change php version as well for this commit.
Someone using FEDEX to test it? |
|
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.
Pull Request Overview
This PR adds FedEx REST API functionality to co-exist alongside the existing FedEx WSDL implementation, allowing users to switch between API types as needed.
- Adds a REST API SDK dependency and configuration options for REST API credentials
- Implements REST API rate calculation and shipment tracking functionality
- Maintains backward compatibility with existing WSDL implementation while supporting PHP 8.1+ requirement
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.
Show a summary per file
File | Description |
---|---|
composer.json | Adds FedEx REST SDK dependency and updates PHP version requirement to 8.1.28 |
app/code/core/Mage/Usa/etc/system.xml | Adds configuration fields for REST API toggle and tracking credentials |
app/code/core/Mage/Usa/etc/config.xml | Adds encrypted backend models for new REST API tracking credentials |
app/code/core/Mage/Usa/Model/Shipping/Carrier/Fedex.php | Implements REST API rate and tracking methods with conditional logic |
README.md | Adds contributor entry |
.all-contributorsrc | Adds contributor configuration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
@@ -138,7 +139,7 @@ | |||
"openmage/composer-plugin": true | |||
}, | |||
"platform": { | |||
"php": "7.4" | |||
"php": "8.1.28" |
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.
The platform PHP version should be specified as a minimum version (e.g., "8.1") rather than a specific patch version ("8.1.28") to allow flexibility with newer compatible patch releases.
"php": "8.1.28" | |
"php": "8.1" |
Copilot uses AI. Check for mistakes.
<comment>Fedex RestAPI need a seperate project for tracking, it's different with project that collect rate.</comment> | ||
</rest_track_key> | ||
<rest_track_secrete translate="label"> | ||
<label>RestAPI Track Secrete</label> |
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.
The field name 'rest_track_secrete' contains a spelling error. It should be 'rest_track_secret' (without the 'e' at the end).
<label>RestAPI Track Secrete</label> | |
<rest_track_secret translate="label"> | |
<label>RestAPI Track Secret</label> |
Copilot uses AI. Check for mistakes.
<comment>Fedex RestAPI need a seperate project for tracking, it's different with project that collect rate.</comment> | ||
</rest_track_key> | ||
<rest_track_secrete translate="label"> | ||
<label>RestAPI Track Secrete</label> |
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.
The label contains a spelling error. 'Secrete' should be 'Secret'.
Copilot uses AI. Check for mistakes.
<show_in_default>1</show_in_default> | ||
<show_in_website>1</show_in_website> | ||
<show_in_store>0</show_in_store> | ||
<comment>Fedex RestAPI need a seperate project for tracking, it's different with project that collect rate.</comment> |
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.
The comment contains a spelling error. 'seperate' should be 'separate'.
Copilot uses AI. Check for mistakes.
@@ -126,6 +126,8 @@ | |||
<meter_number backend_model="adminhtml/system_config_backend_encrypted"/> | |||
<key backend_model="adminhtml/system_config_backend_encrypted"/> | |||
<password backend_model="adminhtml/system_config_backend_encrypted"/> | |||
<rest_track_key backend_model="adminhtml/system_config_backend_encrypted"/> | |||
<rest_track_secrete backend_model="adminhtml/system_config_backend_encrypted"/> |
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.
The configuration key 'rest_track_secrete' contains a spelling error. It should be 'rest_track_secret' to match standard terminology.
Copilot uses AI. Check for mistakes.
if ($this->getConfigData('use_rest_api')) { | ||
$this->_getRestApiTracking($tracking); | ||
} | ||
else{ |
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.
Missing space before opening brace. Should be 'else {' for consistency with PHP coding standards.
Copilot uses AI. Check for mistakes.
new RequestedPackageLineItem( | ||
weight: new Weight( | ||
units: $this->getConfigData('unit_of_measure'), | ||
value: $this->getTotalNumOfBoxes($request->getPackageWeight()), |
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.
Using 'getTotalNumOfBoxes' with weight parameter suggests incorrect usage. This method typically calculates number of boxes, but the weight should be the actual package weight, not number of boxes.
Copilot uses AI. Check for mistakes.
if($storeCurrencyCode != $currencyCode){ | ||
$currencyStore = Mage::getModel('directory/currency')->load($storeCurrencyCode); | ||
$currencyCurrent = Mage::getModel('directory/currency')->load($currencyCode); | ||
$rate = round($rate / $currencyStore->getRate($currencyCurrent)); |
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.
Currency conversion logic is incorrect. Should multiply by the rate, not divide. The correct formula should be: $rate = round($rate * $currencyStore->getRate($currencyCurrent)).
Copilot uses AI. Check for mistakes.
|
||
// Handle delivery date and time | ||
if($trackInfo->dateAndTimes){ | ||
if($trackInfo->dateAndTimes[0]->type == 'ACTUAL_DELIVERY' || $trackInfo->dateAndTimes[0]->type == 'ACTUAL_PICKUP' || |
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.
The condition checks for 'ESTIMATED_DELIVERY' twice (lines 1926), which appears to be a copy-paste error. The second occurrence should likely be 'ESTIMATED_PICKUP'.
Copilot uses AI. Check for mistakes.
Figured I would try the Copilot reviewer here. FedEx will be retiring Web Services on 6/1/26 (per a recent reminder email), which I suppose was pushed back from the original May '24 date, and as @xqiu mentioned, new accounts can potentially no longer access the SOAP API. I'll keep an eye on this one and do some testing myself in the coming weeks, but if any other maintainer has an account (and the time 😄) to test it would be great to push this through. |
Description (*)
Add Fedex REST API rate and tracking functionality, to co-exist with Fedex WSDL.
The reason is that Fedex WSDL still functions. But Fedex starts to require some accounts to use REST API (in my actual case), and stopped one of my accounts to use WSDL.
Fixed Issues (if relevant)
Manual testing scenarios (*)
Questions or comments
The change requires PHP 8.1 and above, as the package shipstream/fedex-rest-sdk requires.