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.

Sign up for GitHub

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

MCCY: Round to lowest denomination before applying price rounding from MCCY settings #9932

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

MCCY: Round to lowest denomination before applying price rounding from MCCY settings #9932

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ff38f59
mccy: round to lowest denomination before applying rounding from sett…
reykjalin Dec 12, 2024
2baa680
mccy: add comment explaining rounding operations
reykjalin Dec 20, 2024
117f72e
add changelog
reykjalin Dec 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions changelog/add-round-to-nearest-lowest-denomination-before-applying-rounding-from-settings
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: patch
Type: update

round to nearest lowest denominator instead of ceiling before applying currency rounding settings.
72 changes: 30 additions & 42 deletions includes/multi-currency/MultiCurrency.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -832,21 +832,23 @@ public function get_price( $price, string $type ): float {
return (float) $price;
}

// We must ceil the converted price here so that we don't introduce rounding errors when
// summing up costs. Consider, e.g. a converted price of 10.003 for a 2-decimal currency.
// A single product would cost 10.00, but 2 of them would cost 20.01, _unless_ we round
// the individual parts correctly.
$converted_price = ( (float) $price ) * $currency->get_rate();
$converted_price = $this->ceil_price_for_currency( $converted_price, $currency );

if ( 'tax' === $type || 'coupon' === $type || 'exchange_rate' === $type ) {
return $converted_price;
// We must make sure the price is rounded properly before returning it, otherwise we
// may end up with inconsistent prices in the cart.
$num_decimals = absint(
$this->localization_service->get_currency_format(
$currency->get_code()
)['num_decimals']
);
return round( $converted_price, $num_decimals );
}

$charm_compatible_types = [ 'product', 'shipping' ];
$apply_charm_pricing = $this->get_apply_charm_only_to_products()
? 'product' === $type
: in_array( $type, $charm_compatible_types, true );
? 'product' === $type
: in_array( $type, $charm_compatible_types, true );

return $this->get_adjusted_price( $converted_price, $apply_charm_pricing, $currency );
}
Expand Down Expand Up @@ -1336,7 +1338,26 @@ public function is_initialized(): bool {
* @return float The adjusted price.
*/
protected function get_adjusted_price( $price, $apply_charm_pricing, $currency ): float {
$price = $this->ceil_price( $price, (float) $currency->get_rounding() );
$rounding = (float) $currency->get_rounding();

// If rounding is configured to be `0.00` we still need to round to the nearest lowest
// currency denomination.
// Otherwise we ceil the price to the configured rounding option.
// NOTE: We don't round if currency rounding is > 0.00 because in those cases we want to
// ceil the amount. For example: if $price = 1.251 and currency rounding = 0.25 we
// want that amount ceiled to 1.50. If we round( 1.251 ) to 1.25 before ceiling the
// price to the nearest 0.25 amount the final amount will be 1.25, which is incorrect.
if ( 0.00 === $rounding ) {
$num_decimals = absint(
$this->localization_service->get_currency_format(
$currency->get_code()
)['num_decimals']
);

$price = round( $price, $num_decimals );
} else {
$price = $this->ceil_price( $price, (float) $currency->get_rounding() );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit: Could we re-use the rounding value from above here?

}

if ( $apply_charm_pricing ) {
$price += (float) $currency->get_charm();
Expand All @@ -1361,39 +1382,6 @@ protected function ceil_price( float $price, float $rounding ): float {
return ceil( $price / $rounding ) * $rounding;
}

/**
* Ceils the price to the precision dictated by the number of decimals in the provided currency.
*
* For example: US$10.0091 -> US$10.01, JPY 1001.01 -> JPY 1002.
*
* @param float $price The price to be ceiled.
* @param Currency $currency The currency used to figure out the ceil precision.
*
* @return float The ceiled price.
*/
protected function ceil_price_for_currency( float $price, Currency $currency ): float {
// phpcs:disable Squiz.PHP.CommentedOutCode.Found, example comments look like code.

// Example to explain the math:
// $price = 10.003.
// expected rounding = 10.01.

// $num_decimals = 2.
// $factor. = 10^2 = 100.
$num_decimals = absint(
$this->localization_service->get_currency_format(
$currency->get_code()
)['num_decimals']
);
$factor = 10 ** $num_decimals; // 10^{$num_decimals}.

// ceil( 10.003 * $factor ) = ceil( 1_000.3 ) = 1_001.
// 1_001 / 100 = 10.01.
return ceil( $price * $factor ) / $factor; // = 10.01.

// phpcs:enable Squiz.PHP.CommentedOutCode.Found
}

/**
* Sets up the available currencies, which are alphabetical by name.
*
Expand Down
15 changes: 8 additions & 7 deletions tests/unit/multi-currency/test-class-multi-currency.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -621,26 +621,26 @@ public function test_get_price_returns_converted_coupon_price_without_adjustment
add_filter( 'wcpay_multi_currency_apply_charm_only_to_products', '__return_false' );

// 0.708099 * 10 = 7.08099.
// ceil( 7.08099, 2 ) = 7.09.
$this->assertSame( 7.09, $this->multi_currency->get_price( '10.0', 'coupon' ) );
// round( 7.08099, 2 ) = 7.08.
$this->assertSame( 7.08, $this->multi_currency->get_price( '10.0', 'coupon' ) );
}

public function test_get_price_returns_converted_exchange_rate_without_adjustments() {
WC()->session->set( WCPay\MultiCurrency\MultiCurrency::CURRENCY_SESSION_KEY, 'GBP' );
add_filter( 'wcpay_multi_currency_apply_charm_only_to_products', '__return_false' );

// 0.708099 * 10 = 7.08099.
// ceil( 7.08099, 2 ) = 7.09.
$this->assertSame( 7.09, $this->multi_currency->get_price( '10.0', 'exchange_rate' ) );
// round( 7.08099, 2 ) = 7.08.
$this->assertSame( 7.08, $this->multi_currency->get_price( '10.0', 'exchange_rate' ) );
}

public function test_get_price_returns_converted_tax_price() {
WC()->session->set( WCPay\MultiCurrency\MultiCurrency::CURRENCY_SESSION_KEY, 'GBP' );
add_filter( 'wcpay_multi_currency_apply_charm_only_to_products', '__return_false' );

// 0.708099 * 10 = 7.08099.
// ceil( 7.08099, 2 ) = 7.09.
$this->assertSame( 7.09, $this->multi_currency->get_price( '10.0', 'tax' ) );
// round( 7.08099, 2 ) = 7.08.
$this->assertSame( 7.08, $this->multi_currency->get_price( '10.0', 'tax' ) );
}

/**
Expand Down Expand Up @@ -1017,7 +1017,8 @@ public function test_set_new_customer_currency_meta_does_not_update_user_meta_if

public function get_price_provider() {
return [
[ '5.2499', '0.00', 5.25 ], // Even though the precision is 0.00 we make sure the amount is ceiled to the currency's number of digits.
[ '5.2401', '0.00', 5.24 ], // Even though the precision is 0.00 we make sure the amount is rounded to the currency's number of digits.
[ '5.2499', '0.00', 5.25 ], // Even though the precision is 0.00 we make sure the amount is rounded to the currency's number of digits.
[ '5.2499', '0.25', 5.25 ],
[ '5.2500', '0.25', 5.25 ],
[ '5.2501', '0.25', 5.50 ],
Expand Down
Loading