Fix issue with comma separators for Shipping Rates

[jorgemd24]

Changes proposed in this Pull Request:

While working on PR #2526, I noticed that if the flat rate cost is entered with a comma separator, it's considered invalid because the following function returns false:

google-listings-and-ads/src/Shipping/ZoneMethodsParser.php

Lines 130 to 133 in 40a068a

	if ( is_numeric( $cost ) ) {
	$flat_cost = (float) $cost;
	$rate = $flat_cost;
	}

This PR addresses the issue by handling the value similarly to how WooCommerce does it here:

https://github.com/woocommerce/woocommerce/blob/91afd71b96e55338affb9284d22df0c38297ee09/plugins/woocommerce/includes/shipping/flat-rate/class-wc-shipping-flat-rate.php#L85-L112

Specifically, it replaces the decimal separators with a ..

Screenshots:

Detailed test instructions:

Flat Rate

1. Create a Shipping Zone for some countries, and create a flat rate method, when you introduce the cost set this to ,
2. Call the following endpoint: GET mc/shipping/rates/suggestions?country_codes[]=YOUR_COUNTRY
3. See that the response contains the rate that you created. Check out develop and see that the rate is not returned.

Flat Rate with "No Shipping Class Cost" and Free Shipping

• Set the "No Shipping Class Cost" using comma decimals.
• Create a Free Shipping method with a minimum requirement. The decimal values don't matter since only integer numbers are allowed.
• The expected result should be the flat rate cost plus the "No Shipping Class Cost," as the other classes are ignored.

Check in MC

Follow the steps mentioned here: #2470 and check that the shipping rates sync correctly, whether you use commas or dots as the separator.

Additional details:

I noticed that if you use "Select All Countries," save, and then refresh the page, it appears that WooCommerce isn’t saving the option, and GFW doesn’t detect any Shipping Zone either. I didn’t have time to investigate further this week, but I’ll look into it next week to see if there’s an issue with WooCommerce.

Changelog entry

Fix - issue with comma separators for Shipping Rates.

[codecov]

Codecov Report

Attention: Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.

Project coverage is 65.0%. Comparing base (4c96cd7) to head (fcf936d).
Report is 12 commits behind head on develop.

Files	Patch %	Lines
src/PluginHelper.php	85.7%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             develop   #2527   +/-   ##
=========================================
  Coverage       65.0%   65.0%           
- Complexity      4588    4590    +2     
=========================================
  Files            475     475           
  Lines          17900   17907    +7     
=========================================
+ Hits           11640   11646    +6     
- Misses          6260    6261    +1     

Flag	Coverage Δ
php-unit-tests	65.0% <90.9%> (+<0.1%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
src/Shipping/ZoneMethodsParser.php	100.0% <100.0%> (ø)
src/PluginHelper.php	83.3% <85.7%> (+0.4%)	⬆️

[mikkamp]

Thanks for the change. It works as expected, but as mentioned in the comment I think we should move this to a helper function, it would also make it easier to add a test for it in the ZoneMethodsParserTest.

Also probably not a big deal since in most currencies shipping costs won't surpass a thousand. But it would still ignore shipping costs with a thousand separator. I suppose since that's semi-broken in WooCommerce as well (allowed to enter but doesn't show up correctly as a shipping cost), we can ignore this.

[mikkamp]

This looks like a great candidate for a helper function, both to clarify what is being done here as well as making it reusable. It also seems like WC already has wc_format_decimal. Should we add that to the WC proxy class, so we can simplify testing as a bonus?

[jorgemd24]

I tried using the wc_format_decimal function, but there is an issue. If your decimal separator is . and you use wc_format_decimal('2,4', ''), it returns 24.00, which is incorrect. The issue is that wc_format_decimal doesn't specifically replace the comma; it just uses the decimal settings stored in WooCommerce and the local environment:

https://github.com/woocommerce/woocommerce/blob/91afd71b96e55338affb9284d22df0c38297ee09/plugins/woocommerce/includes/wc-formatting-functions.php#L292

On the other hand, the Shipping Flat Rate function explicitly replaces the comma:

https://github.com/woocommerce/woocommerce/blob/91afd71b96e55338affb9284d22df0c38297ee09/plugins/woocommerce/includes/shipping/flat-rate/class-wc-shipping-flat-rate.php#L86

[mikkamp]

Thanks for clarifying that I failed to see the difference of the extra , being added to the array.

My question though, is that the right way to handle it? Because if we enter a shipping rate of 2,399.99, then it will be evaluated as 2.399.99, which I don't think is correct either, and won't pass an is_numeric test. If I have a store setup with . as the decimal separator and ',' as the thousand separator, then I don't expect it to be replacing , for decimals. So in that case I'd see 2,4 being replaced with 24 as the "more correct" way of handling it.

[jorgemd24]

Thanks for the discussion. It's a tricky situation because the issue arises from the flexibility users have when setting prices in the modal. It allows any characters as decimal or thousand separators.

The solution I proposed aligns best with how the current WC Shipping Flat Rate handles these cases, although it was not ideal. However, I’ve adjusted the function to strip out any thousand separators and convert the string into a valid decimal number. For example, 1.234,01 will be converted to 1234.01. The thousand separator is more of a formatting issue and doesn’t affect syncing rates or operating with the numbers itself. If we want to format the number, we can use the string returned from the function and apply any desired decimal or thousand separators.

[mikkamp]

Thanks for the clarification, it is indeed tricky.

Although I don't totally agree with the validation of the modal as if I leave it to the default with no thousand separator then I'm only allowed to use a . and a comma is not allowed.

If I do have both symbols included then it accepts any combination as long as they are either numbers or a thousand separator, and a single decimal separator. So entering 1,,23.45,,99 is totally acceptable even though it won't make sense. In your case that will be converted to 12345.99

That's fine for now as generally we will expect sane values being entered, but I think in the long run it's the modal that should do a bit better validation there.

[mikkamp]

Can we also link issue #2470 so we don't work on this twice. This is also another reason why we might need to move it to a helper, since the issue describes multiple locations where the number is parsed incorrectly.

[jorgemd24]

Thanks, @mikkamp, for the suggestion! I moved the logic to a helper function and updated the other part of the code to address all the points mentioned in issue #2470. I realized there's no need to handle decimal places for the minimum value in Free Shipping Methods since it's converted to an integer anyway, so the main issue was with the flat rate and the classes.

https://github.com/woocommerce/woocommerce/blob/91afd71b96e55338affb9284d22df0c38297ee09/plugins/woocommerce/includes/shipping/free-shipping/class-wc-shipping-free-shipping.php#L30

Do you mind to have a second look? Thanks!

[jorgemd24]

The PHP units are failing because it seems that there is an issue with the latest WC and mapping the attributes and the sku, but it doesn't seem related to this PR. Will look into this next week if no one has done it before.

[mikkamp]

The PHP units are failing because it seems that there is an issue with the latest WC and mapping the attributes and the sku, but it doesn't seem related to this PR. Will look into this next week if no one has done it before.

Fixed in #2559

[mikkamp]

Thanks for the additional changes, I understand the inconsistency so I see we can't account for everything.

The shipping rates are now syncing correctly. There is one error I'm still getting:

action failed via Async Request: { "error": { "code": 400, "message": "[services[1].rateGroups[0].singleValue] Precision for price 12.156 exceeds maximum precision of 2 decimal digits.", "errors": [ { "message": "[services[1].rateGroups[0].singleValue] Precision for price 12.156 exceeds maximum precision of 2 decimal digits.", "domain": "global", "reason": "invalid" } ] } }

I think regardless of the decimal setting in WC we need to always ensure it doesn't exceed 2 places. Note that the modal doesn't allow this many decimal places, but if you right click the edit button to open in a new tab, it goes to the old input form where more decimal places can be added. Most 3PD shipping rates don't support the modal so we might not always have that validation in place.

Either way though since all the expected shipping rates are syncing I'll go ahead and approve the PR and we can just add a small tweak to always round to a max of 2 decimals.

[jorgemd24]

Thanks @mikkamp for fixing the issue with the unit tests and for catching this:

There is one error I'm still getting:

action failed via Async Request: { "error": { "code": 400, "message": "[services[1].rateGroups[0].singleValue] Precision for price 12.156 exceeds maximum precision of 2 decimal digits.", "errors": [ { "message": "[services[1].rateGroups[0].singleValue] Precision for price 12.156 exceeds maximum precision of 2 decimal digits.", "domain": "global", "reason": "invalid" } ] } }

I'll add the tweak in a follow-up PR.