search

OpenMage / magento-lts

Official OpenMage LTS codebase | Migrate easily from Magento Community Edition in minutes! Download the source code for free or contribute to OpenMage LTS | Security vulnerability patches, bug fixes, performance improvements and more.
https://www.openmage.org
Open Software License 3.0
863 stars 438 forks source link

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

Closed fballiano closed 1 month ago

fballiano commented 1 month ago

Fixes https://github.com/OpenMage/magento-lts/pull/3976#issuecomment-2125203784

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

Screenshot 2024-05-22 alle 21 43 58
fballiano commented 1 month ago

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

ragnese commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.