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

REF Ensure that getAmount includes 0s in decimal places up to 2 places #20074

Merged
merged 1 commit into from
Apr 15, 2021

Conversation

seamuslee001
Copy link
Contributor

Overview

This fixes the test failures on the current 5.37 up merge into master

Before

params of 20.00 gets returned as 20

After

params of 20.00 gets returned a 20.00

ping @eileenmcnaughton this fixed the test failures for me locally does this make sense to you?

@civibot
Copy link

civibot bot commented Apr 15, 2021

(Standard links)

@eileenmcnaughton
Copy link
Contributor

Yep agree

eileenmcnaughton added a commit to eileenmcnaughton/nz.co.fuzion.omnipaymultiprocessor that referenced this pull request Apr 15, 2021
With civicrm/civicrm-core#20074 merged it makes more sense to use the main function for getAmount - which is more in sync with most processors and has the deprecation
@seamuslee001 seamuslee001 merged commit 4e3fffd into civicrm:5.37 Apr 15, 2021
@seamuslee001 seamuslee001 deleted the fix_format_money branch April 15, 2021 23:30
@mattwire
Copy link
Contributor

@seamuslee001 @eileenmcnaughton I didn't see this or I'd have commented. Putting the locale function back in there negates the use of filter_var and breaks sites with decimal separator that is not ".". Eg. ","

Something like this would work:

return round(filter_var($params['amount'], FILTER_SANITIZE_NUMBER_FLOAT, FILTER_FLAG_ALLOW_FRACTION), CRM_Utils_Money::getCurrencyPrecision());

@eileenmcnaughton
Copy link
Contributor

Argh - this was supposed to be formatUSLocaleNumericRounded() - somehow in hte confusion we put the wrong function!!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants