Skip to content
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

Fix issue with comma separators for Shipping Rates #2527

Merged
merged 11 commits into from
Aug 27, 2024
26 changes: 26 additions & 0 deletions src/PluginHelper.php
Original file line number Diff line number Diff line change
Expand Up @@ -198,4 +198,30 @@
protected function strip_url_protocol( string $url ): string {
return preg_replace( '#^https?://#', '', untrailingslashit( $url ) );
}

/**
* It tries to convert a string to a decimal number using the dot as the decimal separator.
* This is useful with functions like is_numeric as it doesn't recognize commas as decimal separators or any thousands separator.
* Note: Using wc_format_decimal with a dot as the decimal separator (WC -> Settings -> General) will strip out commas but won’t replace them with dots.
* For example, wc_format_decimal('2,4') will return 24 instead of 2.4.
*
* @param string $numeric_string The number to convert.
*
* @return string The number as a standard decimal. 1.245,63 -> 1245.65
*/
protected function convert_to_standard_decimal( string $numeric_string ): string {
$locale = localeconv();

$separators = [ wc_get_price_decimal_separator(), wc_get_price_thousand_separator(), $locale['thousands_sep'], $locale['mon_thousands_sep'], $locale['decimal_point'], $locale['mon_decimal_point'], ',' ];

if ( wc_get_price_decimals() > 0 ) {
// Replace all posible separators with dots.
$numeric_string = str_replace( $separators, '.', $numeric_string );
// Leave only the last dot that is the decimal separator.
return (string) preg_replace( '/\.(?=.*\.)/', '', $numeric_string );
} else {
// If no decimals remove all separators.
return str_replace( $separators, '', $numeric_string );

Check warning on line 224 in src/PluginHelper.php

View check run for this annotation

Codecov / codecov/patch

src/PluginHelper.php#L224

Added line #L224 was not covered by tests
}
}
}
11 changes: 7 additions & 4 deletions src/Shipping/ZoneMethodsParser.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

use Automattic\WooCommerce\GoogleListingsAndAds\Infrastructure\Service;
use Automattic\WooCommerce\GoogleListingsAndAds\Proxies\WC;
use Automattic\WooCommerce\GoogleListingsAndAds\PluginHelper;
use WC_Shipping_Method;
use WC_Shipping_Zone;

Expand All @@ -19,6 +20,8 @@
*/
class ZoneMethodsParser implements Service {

use PluginHelper;

public const METHOD_FLAT_RATE = 'flat_rate';
public const METHOD_FREE = 'free_shipping';

Expand Down Expand Up @@ -123,15 +126,15 @@ protected function get_flat_rate_method_rate( object $method ): ?float {
$rate = null;

$flat_cost = 0;
$cost = $method->get_option( 'cost' );
$cost = $this->convert_to_standard_decimal( (string) $method->get_option( 'cost' ) );
// Check if the cost is a numeric value (and not null or a math expression).
if ( is_numeric( $cost ) ) {
$flat_cost = (float) $cost;
$rate = $flat_cost;
}

// Add the no class cost.
$no_class_cost = $method->get_option( 'no_class_cost' );
$no_class_cost = $this->convert_to_standard_decimal( (string) $method->get_option( 'no_class_cost' ) );
if ( is_numeric( $no_class_cost ) ) {
$rate = $flat_cost + (float) $no_class_cost;
}
Expand All @@ -155,7 +158,7 @@ protected function get_flat_rate_method_class_rates( object $method ): array {
$class_rates = [];

$flat_cost = 0;
$cost = $method->get_option( 'cost' );
$cost = $this->convert_to_standard_decimal( (string) $method->get_option( 'cost' ) );
// Check if the cost is a numeric value (and not null or a math expression).
if ( is_numeric( $cost ) ) {
$flat_cost = (float) $cost;
Expand All @@ -164,7 +167,7 @@ protected function get_flat_rate_method_class_rates( object $method ): array {
// Add shipping class costs.
$shipping_classes = $this->wc->get_shipping_classes();
foreach ( $shipping_classes as $shipping_class ) {
$shipping_class_cost = $method->get_option( 'class_cost_' . $shipping_class->term_id );
$shipping_class_cost = $this->convert_to_standard_decimal( (string) $method->get_option( 'class_cost_' . $shipping_class->term_id ) );
if ( is_numeric( $shipping_class_cost ) ) {
// Add the flat rate cost to the shipping class cost.
$class_rates[ $shipping_class->slug ] = [
Expand Down
73 changes: 73 additions & 0 deletions tests/Unit/Plugin/PluginHelperTraitTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
<?php
declare( strict_types=1 );

namespace Automattic\WooCommerce\GoogleListingsAndAds\Tests\Unit\API\Site\Controllers;

use PHPUnit\Framework\TestCase;
use Automattic\WooCommerce\GoogleListingsAndAds\PluginHelper;


/**
* Class PluginHelperTraitTest
*
* @package Automattic\WooCommerce\GoogleListingsAndAds\Tests\Unit\API\Site\Controllers
*/
class PluginHelperTraitTest extends TestCase {


/** @var PluginHelper $trait */
protected $trait;

/**
* Runs before each test is executed.
*/
public function setUp(): void {
parent::setUp();

// phpcs:ignore Universal.Classes.RequireAnonClassParentheses.Missing
$this->trait = new class {
use PluginHelper {
convert_to_standard_decimal as public;
}
};
}


public function test_comma_decimals_gets_converted_to_dot_decimals() {
$this->assertEquals( '10.5', $this->trait->convert_to_standard_decimal( '10,5' ) );
}

public function test_dot_decimals_remain_unchanged() {
$this->assertEquals( '10.5', $this->trait->convert_to_standard_decimal( '10.5' ) );
}

public function test_invalid_numbers() {
$this->assertEquals( 'no valid. number', $this->trait->convert_to_standard_decimal( 'no valid, number' ) );
}

public function test_with_thousands_separator() {
$this->assertEquals( '1234.5', $this->trait->convert_to_standard_decimal( '1' . wc_get_price_thousand_separator() . '234,5' ) );
}

public function test_with_no_decimals() {
$this->assertEquals( '12345', $this->trait->convert_to_standard_decimal( '12345' ) );
}

public function test_with_different_wc_decimal_separator() {
add_filter(
'wc_get_price_decimal_separator',
[ $this, 'callback_filter_decimal_separator' ]
);

$this->assertEquals( '10.5', $this->trait->convert_to_standard_decimal( '10-5' ) );
}

public function callback_filter_decimal_separator() {
return '-';
}

public function tearDown(): void {
parent::tearDown();
remove_filter( 'wc_get_price_decimal_separator', [ $this, 'callback_filter_decimal_separator' ] );
}
}
54 changes: 48 additions & 6 deletions tests/Unit/Shipping/ZoneMethodsParserTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,48 @@ public function test_returns_flat_rate_methods() {
$this->assertEquals( 10, $shipping_rates[0]->get_rate() );
}

public function test_returns_flat_rate_methods_with_decimals_and_comma() {
$flat_rate = $this->createMock( WC_Shipping_Flat_Rate::class );
$flat_rate->id = ZoneMethodsParser::METHOD_FLAT_RATE;
$flat_rate->expects( $this->any() )
->method( 'get_option' )
->willReturnMap(
[
[ 'cost', null, '10,6' ],
]
);

$zone = $this->createMock( WC_Shipping_Zone::class );
$zone->expects( $this->any() )
->method( 'get_shipping_methods' )
->willReturn( [ $flat_rate ] );

$shipping_rates = $this->methods_parser->parse( $zone );
$this->assertCount( 1, $shipping_rates );
$this->assertEquals( 10.6, $shipping_rates[0]->get_rate() );
}

public function test_returns_flat_rate_methods_with_decimals_and_dot() {
$flat_rate = $this->createMock( WC_Shipping_Flat_Rate::class );
$flat_rate->id = ZoneMethodsParser::METHOD_FLAT_RATE;
$flat_rate->expects( $this->any() )
->method( 'get_option' )
->willReturnMap(
[
[ 'cost', null, '10.6' ],
]
);

$zone = $this->createMock( WC_Shipping_Zone::class );
$zone->expects( $this->any() )
->method( 'get_shipping_methods' )
->willReturn( [ $flat_rate ] );

$shipping_rates = $this->methods_parser->parse( $zone );
$this->assertCount( 1, $shipping_rates );
$this->assertEquals( 10.6, $shipping_rates[0]->get_rate() );
}

public function test_returns_flat_rate_methods_including_shipping_classes() {
// Return three sample shipping classes.
$light_class = new \stdClass();
Expand All @@ -69,10 +111,10 @@ public function test_returns_flat_rate_methods_including_shipping_classes() {
->willReturnMap(
[
[ 'cost', null, 10 ],
[ 'class_cost_0', null, 5 ],
[ 'class_cost_0', null, '5,20' ], // Comma decimal separator will be converted to dot.
[ 'class_cost_1', null, 15 ],
[ 'class_cost_2', null, '[qty] / 10' ],
[ 'no_class_cost', null, 2 ],
[ 'no_class_cost', null, '6,7' ],
]
);

Expand Down Expand Up @@ -103,11 +145,11 @@ function ( ShippingRate $shipping_rate ) {

foreach ( $shipping_rates as $shipping_rate ) {
if ( [] === $shipping_rate->get_applicable_classes() ) {
// The `no_class_cost` should be added to the flat rate method cost (10+2=12).
$this->assertEquals( 12, $shipping_rate->get_rate() );
// The `no_class_cost` should be added to the flat rate method cost (10+6.7=16.7).
$this->assertEquals( 16.7, $shipping_rate->get_rate() );
} elseif ( [ 'light' ] === $shipping_rate->get_applicable_classes() ) {
// The shipping class costs should be added to the flat rate method cost (10+5=15).
$this->assertEquals( 15, $shipping_rate->get_rate() );
// The shipping class costs should be added to the flat rate method cost (10+5.20=15.20).
$this->assertEquals( 15.20, $shipping_rate->get_rate() );
} elseif ( [ 'heavy' ] === $shipping_rate->get_applicable_classes() ) {
// The shipping class costs should be added to the flat rate method cost (10+15=25).
$this->assertEquals( 25, $shipping_rate->get_rate() );
Expand Down