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

Add test for api money, fix net_amount calc #11801

Merged
merged 1 commit into from
Mar 13, 2018

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Mar 13, 2018

Overview

Go back to cleaning api money input -in the api layer

Before

money values 5.000,77 & 5,000.77 not handled & they are converted to '5' by mysql insert.

After

These values are handled. However, risk is added that double cleaning could mess them up.

Technical Details

This is problematic because if values are cleaned (converted to 5000.77) twice then they will be incorrect. The code that used to do a reasonable 'guess' at whether it was already cleaned stopped working once we messed with precision (a few point releases ago).

The fix is that all parameters should be cleaned as close to input as possible. When I last analysed the api I thought it was only accepting clean values but I now think I was wrong. This puts us back to the api doing cleaning, but at risk that it could double clean if skipCleanMoney not used. I can't quite figure out how to find the light at the end of this now because by putting this back we put back the potential for on ongoing whack-a-mole, but not sure we can justify NOT putting it back by default.

One BIG thing that has happened since this whole issue originally blew up is the addition of a LOT of unit tests - grep for $this->setCurrencySeparators($thousandSeparator); - so that might give some confidence (maybe) the form layer is not cleaning the values & then passing to the api which re-cleans / breaks them

Comments

@eileenmcnaughton
Copy link
Contributor Author

@mattwire @mlutfy @monishdeb I thought we have gotten to the end of the whack-a-mole on the 'guess how many times it has been cleaned' currency issue - but no :-(

@monishdeb
Copy link
Member

Make sense, tested on my local, doesn't cause any unintended regression.

(r-explain) Pass
(r-test) Pass
(r-code) Pass
(r-doc) Pass
(r-maint) Pass
(r-run) Pass - Pass
(r-user) Pass
(r-tech) Pass

Hence merging.

@monishdeb monishdeb merged commit ed477ea into civicrm:master Mar 13, 2018
@eileenmcnaughton eileenmcnaughton deleted the api_test branch March 13, 2018 19:41
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