Free shipping items applied to all FedEx methods.

AirmanAJK commented 7 years ago

When collecting shipping rates for FedEx where the products have some items designated as free shipping and others not, the reduced rate for shipping is applied to all available FedEx methods instead of just the configured method.

Preconditions

This applies to all Magento versions, including the most recent code in the develop branch. The PHP/MySQL versions are irrelevant since it's a simple bug in the code.

Steps to reproduce

Create two simple products that can be added to the shopping cart.
Create a Cart Price Rule that will designate one of the products as free shipping but not the other.
Configure FedEx as as an enabled shipping method with Ground configured as the Free Method.
Add both products to the shopping cart and enter a zip code to generate an estimated shipping rate.

Expected result

The resulting rates will only have a reduced value for FedEx Ground as it was designated as the free method. The other methods like Priority Overnight should not be reduced by the free shipping product.

Actual result

All methods are reduced in addition to the expected method.

This is happening in the function _updateFreeMethodQuote in the following file: \vendor\magento\module-shipping\Model\Carrier\AbstractCarrier.php

The intention was to collect the original shipping rates based on the total weight of all products, regardless of any free shipping status. After these "full price" rates are collected, a second request is made to get the rates using a total weight that is reduced by the free product's weight (stored in the data collection of the request as free_method_weight). Only the configured shipping method (ie Ground) should be updated in the original rates results with the new price.

The problem is that the first action performed in the function _getQuotes() is to create a new instance of the _result property. This REPLACES all of the original rates collected with the new reduced rates. This is because the function _updateFreeMethodQuote() also calls _getQuotes() to collect the rates.

My band-aid fix is to alter the part of the function _updateFreeMethodQuote with this: $result = $this->_getQuotes();

To this instead:

$tempResult = $this->_result;
$result = $this->_getQuotes();
$this->_result = $tempResult;

This preserves the rates of the original request while still allowing the second request to be performed.

AirmanAJK commented 7 years ago

At a second glance, it seems the real problem is in this file: \vendor\magento\module-fedex\Model\Carrier.php since in its implementation of _getQuotes() it sets $this->_result directly instead of just returning a result object like the other carriers do.

AirmanAJK commented 7 years ago

Here is my suggested fix inside \Magento\Fedex\Model\Carrier

public function collectRates(\Magento\Quote\Model\Quote\Address\RateRequest $request) {
    if (!$this->canCollectRates()) {
        return $this->getErrorMessage();
    }

    $this->setRequest($request);
    $this->_result = $this->_getQuotes();
    $this->_updateFreeMethodQuote($request);

    return $this->getResult();
}

protected function _getQuotes() {
    $result = $this->_rateFactory->create();
    // make separate request for Smart Post method
    $allowedMethods = explode(',', $this->getConfigData('allowed_methods'));
    if (in_array(self::RATE_REQUEST_SMARTPOST, $allowedMethods)) {
        $response = $this->_doRatesRequest(self::RATE_REQUEST_SMARTPOST);
        $preparedSmartpost = $this->_prepareRateResponse($response);
        if (!$preparedSmartpost->getError()) {
            $result->append($preparedSmartpost);
        }
    }
    // make general request for all methods
    $response = $this->_doRatesRequest(self::RATE_REQUEST_GENERAL);
    $preparedGeneral = $this->_prepareRateResponse($response);
    if (!$preparedGeneral->getError()
        || $result->getError() && $preparedGeneral->getError()
        || empty($result->getAllRates())
    ) {
        $result->append($preparedGeneral);
    }

    return $result;
}

pboisvert commented 7 years ago

@antboiko can you get this one reviewed?

RomanKrut commented 7 years ago

@AirmanAJK Hi, can you please provide information about your cart price rule configuration? Screenshots will be helpful,too.

AirmanAJK commented 7 years ago

My cart price rule is based on geographic regions and custom attributes and would likely just over-complicate things if provided.

To create a "Free Shipping" cart price rule for testing: Add a rule with a condition section that will evaluate to true (ie Total Weight greater than zero), an action section that sets "Free Shipping" to "For matching items only", and an apply section that targets just one of the products used to test (ie a specific SKU).

AirmanAJK commented 7 years ago

I probably won't be checking back here since I've already implemented my suggested fix above, so it would be a shame if this issue gets "closed due to inactivity" since I've already provided the "who, what, where, why, and how to fix".

This bug cost us over $100 last week when we had to ship a product using priority overnight for free, and even by examining the code the bug is apparent without testing. My experience here has been that bug reports like this one will get closed due to an insignificant question being asked and unanswered for too long, despite a clearly described and analyzed problem. If that happens with this issue, it's unlikely that I'll be reporting any more problems that I discover and fix in Magento.

In other words, I believe my work here is done. Please ensure this issue isn't closed without a fix committed as it can be quite expensive for merchants using FedEx.

orlangur commented 7 years ago

@AirmanAJK please check https://github.com/magento/magento2/wiki/Magento-Issue-Gates out. Your bug report already successfully passed G1 and G2, the only chance it can be closed is if whoever will check it will not be able to reproduce using the details provided (and it is absolutely fine as irreproducible bug cannot be fixed).

Please note that much more natural way to suggest a bugfix is pull request creation. Not just it is much more convenient to review than a plain text in comment but that you also grant permission to use your code by signing the CLA.

magento-engcom-team commented 6 years ago

@AirmanAJK, thank you for your report. We've created internal ticket(s) MAGETWO-83457 to track progress on the issue.

magento-engcom-team commented 5 years ago

Hi @TomashKhamlai. Thank you for working on this issue. Looks like this issue is already verified and confirmed. But if your want to validate it one more time, please, go though the following instruction:

[x] 1. Add/Edit Component: XXXXX label(s) to the ticket, indicating the components it may be related to.
[x] 2. Verify that the issue is reproducible on 2.3-develop branch
Details
- Add the comment @magento-engcom-team give me 2.3-develop instance to deploy test instance on Magento infrastructure.
- If the issue is reproducible on 2.3-develop branch, please, add the label Reproduced on 2.3.x.
- If the issue is not reproducible, add your comment that issue is not reproducible and close the issue and stop verification process here!
[ ] 3. Verify that the issue is reproducible on 2.2-develop branch.
Details
- Add the comment @magento-engcom-team give me 2.2-develop instance to deploy test instance on Magento infrastructure.
- If the issue is reproducible on 2.2-develop branch, please add the label Reproduced on 2.2.x
[ ] 4. If the issue is not relevant or is not reproducible any more, feel free to close it.

TomashKhamlai commented 5 years ago

@magento-engcom-team give me 2.3-develop instance

magento-engcom-team commented 5 years ago

Hi @TomashKhamlai. Thank you for your request. I'm working on Magento 2.3-develop instance for you

magento-engcom-team commented 5 years ago

Hi @TomashKhamlai, here is your Magento instance. Admin access: http://34.228.235.121/i-10902-2-3-develop//admin Login: admin Password: 123123q Instance will be terminated in up to 3 hours.

TomashKhamlai commented 5 years ago

The issue was re-tested and we can confirm that it was fixed on the 2.3 release branch. We closing this issue as fixed due to upcoming 2.3 release that will be available soon.

magento / magento2