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

CRM-17157 support multiple decimalplaces formrule money #10827

Conversation

mattwire
Copy link
Contributor

@mattwire mattwire commented Aug 7, 2017

Ref CRM-20772 and CRM-17157. CRM_Utils_Rule::money currently assumes all currencies have 2 decimal places (this is not true, eg. bitcoin). Also to allow proper calculation of tax where the tax inclusive amount should be a round number we have to save more than 2 decimal places in the database. This is part of that puzzle!

I've also added a unit test :-)
@eileenmcnaughton


…es such as bitcoin, also helps towards CRM-20772 which requires higher precision decimals to be stored in the database to calculate tax inclusive amounts correctly
@KarinG
Copy link
Contributor

KarinG commented Aug 8, 2017

Jenkins, test this please

@KarinG
Copy link
Contributor

KarinG commented Aug 8, 2017

@eileenmcnaughton - I believe you mentioned you're already doing this for WMF? I like it when we can note real world experience on PRs. It would put you in the best position to ok this.

@eileenmcnaughton
Copy link
Contributor

@KarinG we did it slightly differently for WMF - the regex is (^-?\d+.?\d*$)|(^-?\d*.\d+$). However, I don't think that difference is material & the tests look good. I tried a few more over here http://regexr.com - mostly to check lack of preceding number ie. .35678 -.35678 0.35678 -0.35678 & they all worked.

I'm going to merge because we have agreed (on the other PR) that this change makes sense to do & the test @mattwire wrote is really good & gives us great cover.

From what I've seen mysql still trims decimal places if the field does not support more than 2 places.

@mattwire wins the star of the day prize for the test!

@eileenmcnaughton eileenmcnaughton merged commit 581e991 into civicrm:master Aug 8, 2017
@eileenmcnaughton
Copy link
Contributor

(note that I feel the regex is still safe in that it does not permit any characters other than digits, dots & +/- indicator & currency

@KarinG
Copy link
Contributor

KarinG commented Aug 9, 2017

Awesome - all done this one.

@mattwire mattwire deleted the CRM-17157_support_multiple_decimalplaces_formrule_money branch August 28, 2017 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants