Fixed unable to select "free method" in UPS backend configuration

[fballiano]

Fixes #3976 (comment)

It was impossible to select a "free method" for UPS shipping method

[fballiano]

@mingjyou @ragnese could you please take a look at this?

[ragnese]

In the phtml file, what does line 113, this.originShipmentObj['default'] = <?php echo $_coreHelper->jsonEncode($defShipArr) ?>; actually do?

That's not a line that you changed, but I feel like I need to understand it. The function updateAllowedMethod is passed an argument, originShipmentTitle- when is that argument ever equal to 'default'? If it ever is equal to 'default' (maybe for a brand new installation?), should it use the REST API's codes instead of the XML codes, since the REST is the default now?

I wonder if the opening PHP code in the phtml file needs to be more thorough. Shouldn't we also filter the $storedAllowedMethods based on whatever the $storedUpsType ends up being?

I'll give this some more thought and play around with it later, and maybe do a PR against your branch tomorrow with my thoughts.

[fballiano]

I don't really know but I wouldn't worry too much about that validation, this checking if the method is within that array is ok but also kinda too much, in the sense that the only way to save that "free method" is selecting in a combobox so we can also avoid validating it that strictly.

my solution is for sure dirty but.. if later we will remove all of the XML API code... I wouldn't put too much care now.

[ragnese]

if later we will remove all of the XML API code... I wouldn't put too much care now.

Oh, yeah. Good point. In that case, I just have one suggestion that I'll put in the review I'm about to do.

[ragnese]

The only small issue here is that a lot of the elements in this array will be repeats, which is going to make it several times bigger than it needs to be. An easy way to fix that would be to use the 'value' as a key in $arr, and then take just the values without the keys, like this:

    public function toOptionArray()
    {
        $ups = Mage::getSingleton('usa/shipping_carrier_ups');
        $arr = [];

        // necessary after the add of Rest API
        $origins = $ups->getCode('originShipment');
        foreach ($origins as $origin) {
            foreach ($origin as $k => $v) {
                $arr[$k] = ['value' => $k, 'label' => Mage::helper('usa')->__($v)];
            }
        }

        // old XML API codes
        foreach ($ups->getCode('method') as $k => $v) {
            $arr[$k] = ['value' => $k, 'label' => Mage::helper('usa')->__($v)];
        }

        return array_values($arr);
    }

[fballiano]

yes but we're talking 20 elements instead of 10, I don't think it's something that needs to be optimized.

plus it will be removed later this year

I'll take a look at it later but

[ragnese]

I agree, it doesn't really matter. But, there's also the old saying "There's nothing more permanent than a temporary fix." :)

I went ahead and made an "approved" review. I don't feel strongly about this change, so it doesn't matter if you leave it as-is or tweak it.

[fballiano]

it is true, but I will personally remove it when the XML API will go down.

I'm all for cleaning the old mess and probably half of my PRs are cleanups heheheh