Automattic / woocommerce-payments

Accept payments via credit card. Manage transactions within WordPress.
https://wordpress.org/plugins/woocommerce-payments/
Other
173 stars 69 forks source link

Remove inline `*100` (hack) in `format_item_price_data` & refactor `WC_Payments_Utils::prepare_amount` so it can be used by client code wanting float or int return value (follow up to #4321) #4345

Open haszari opened 2 years ago

haszari commented 2 years ago

Describe the bug

WC_Payments_Utils::prepare_amount() is our standard utility for converting a (user facing) currency value into a value suitable for passing to Stripe APIs. It includes logic to multiply by 100 for decimal currencies (e.g. USD).

In #4321 we added an inline * 100 calculation to cover a subscription calculation which required a floating point value.

https://github.com/Automattic/woocommerce-payments/blob/0b67331b41cf087751961e9a1b2bbc6366806bce/includes/subscriptions/class-wc-payments-subscription-service.php#L276-L279

Ideally we'd use a single code path to cover all use cases for prepare_amount so we don't have multiple places to patch if something changes or we discover a bug. As a fallback, we could implement two functions closely located in source code, so it's easy for future devs to apply fixes to both.


Suggestion from @vbelolapotkov on the PR:

  1. Extend signature of prepare_amount to accept precision as optional third argument (0 by default).
  2. Use it as: WC_Payments_Utils::prepare_amount( $unit_amount, $currency, wc_get_rounding_precision() )
  3. Update return result from prepare_amount:
return $precision > 0 ? 
    round( (float) $amount * $conversion_rate, $precision ) :
    (int) round( (float) $amount * $conversion_rate, $precision ); 

Also this comment:

One caveat I'm not fully sure about is when wc_get_rounding_precision() returns 0. Is it ok to pass unit_amount_decimal as int? If not you could make it explicit as (float). 'unit_amount_decimal' => (float) WC_Payments_Utils::prepare_amount( $unit_amount, $currency, wc_get_rounding_precision() )

To Reproduce

TBC - please check #4321 for details and add info here when this is picked up 🙇

haszari commented 2 years ago

Assigning this to @james-allan as follow up from the PR - let me know if you have bandwidth, can reassign if needed.

haszari commented 2 years ago

Marking as low priority, because there's no merchant impact of this. i.e. this is a code refactor / preventative task.

IMO it's worth getting tidied up though :)

vbelolapotkov commented 2 years ago

@haszari how do you handle those low prio issues? Is there a chance it will be handled during cooldown? Despite it's a small task its consequences might be large x100 charges for zero-decimal currencies, so I'd rather fix it sooner than later.

haszari commented 2 years ago

Thanks for the reminder about this @vbelolapotkov . In Helix we don't have a defined process for handling maintenance tasks – this is something I need to look at. For this issue I'll see if someone can pick it up during cooldown or if not will include it in next cycle to get it sorted.

haszari commented 2 years ago

Bumping up the priority so we can get this tidied up. I agree there's risk here and it should be quick to tidy up.