CRM-20772 Add database changes to support > 2 decimal places

[mattwire]

Overview

Database changes required for CRM-20772

Before

Database fields used for calculations restrict amounts to 2 decimal places.

After

Database fields used for calculations are not restricted to 2 decimal places.

Technical Details

From dus
There are also currencies with more than two decimal places (https://en.wikipedia.org/wiki/ISO_4217#Active_codes) and bitcoin supports even more.

Now, the problem is that most amounts are stored in the DB as decimal(10,2) which means any precision greater than 2 decimal places is lost. For this particular issue (CRM-20772) I am only proposing changing a single field in MembershipType.minimum_fee as that is the only one that impacts on this issue (civicrm_price_field_value.amount is where most monetary values end up and is stored as varchar(512) (though probably shouldn't be)).

What I would like to identify, is what is a sensible datatype that I should change minimum_fee to, and that we should standardise on in future for monetary amounts?

Most fields holding monetary amounts in the db are decimal(10,2), some are decimal(20,2), tax_rate is decimal(10,8).
We should probably change civicrm_price_field_value.amount from varchar(512) to something more sensible - but that's probably too risky a change at this stage.
We should define a datatype that should be used to hold monetary amounts that supports more than the current two decimals.
The current datatype decimal(10,2) supports a maximum value of ten million: 10,000,000.00
Decimal(30,10) would support 10,000,000,000,000,000.0000000000
MySQL stores 9 decimal places in 4 bytes, decimals and fractions separately (https://dev.mysql.com/doc/refman/5.7/en/storage-requirements.html) meaning multiples of 9 are the most sensible values to use.
Currencies support up to 4 decimal places, bitcoin(BTC) apparently support 8 decimal places. So following GAAP accounting principals 10 decimal places gives the maximum required precision. So, 9 or 18 for decimal places?
For decimals, 9 gives 100,000,000 but we currently store 10, 18 gives 100,000,000,000,000,000 (one hundred thousand million million) which should be plenty?
So: decimal(18,9)?

Comments

This should be committed before other PRs on CRM-20772 as they are dependent on this one.

---

• CRM-20772: (WIP) Price set calculation precision when sales tax enabled

[monishdeb]

@mattwire why only for those two fields and not for all other decimal type fields like contribution.total_amount/fee_amount etc. But first I think we need to address this change to our community that would be on https://chat.civicrm.org/civicrm/channels/dev-financial .

[mattwire]

@monishdeb Those two fields are used to store values BEFORE calculation. All other fields are used to store values AFTER calculation. So with this change the final values will still be 2dp but the values used for calculation can be more than 2dp. This is also important because the size of the two tables I am changing are likely to be small on most installs, while the contribution/line-item tables are likely to be huge on some installs and any changes to the data types on those tables could have a large impact on size/performance. The rationale behind this change was discussed at some length here: https://lists.civicrm.org/lists/arc/civicrm-dev/2017-08/msg00012.html and I've just posted it to dev-financial as you suggested.

[mattwire]

Rebased and updated following 4.7.26/4.7.27 jiggery.

[eileenmcnaughton]

I'm going to merge this based on the following considerations

• Matt has put this out to discussion and I feel there was consensus on changing the fields used to calculate totals to this decimal format. There was also discussion about changing the actual amount fields (e.g in the contribution tables) and I think there are still concerns about this.

However, individual sites can change their tables and perhaps in the future we will look at changing them - probably in a similar way to changes to the way we have with timestamps. One of the concerns is that it could trigger a looong slow upgrade process.

I did an upgrade and the existing data seemed fine (2 decimals was still 2 decimals) and made a purchase and did not notice any weirdness.

I think this is a step in the right direction.

[magnolia61]

This PR has bad consequences for our installation. Where previously amount had values like

• 10 or
• -5,
  after the upgrade (ALTER TABLE civicrm_price_field_value CHANGE amount amount DECIMAL(18,9) NOT NULL COMMENT 'Price field option amount';) the values are:
• 10.000000000
• -5.000000000
  Which our installation interprets as 10 billion (since we have . as a Thousands Separator and , as Decimal Delimiter (as is common in the Netherlands).

We found our because our payment provided does not allow us to process payments as big as 10 billion...

[colemanw]

@mlutfy @eileenmcnaughton I think we need to get very clear about something: Do we want to store localized thousands & decimal delimiters in the database? Or store in English format and only localize them during display to the end user? My vote is for the latter.

[magnolia61]

I understand long term choices have to be made here, but right now this is happening with production code :-) The amount looks beautiful though!

(Workaround for us is to change the thousands & decimal delimiters to english settings, which will temporarily lead to confusion but also to the proper amounts.)

[JoeMurray]

We should not store a localized amount. My guess is that the fix here is to format the value retrieved from db to local currency before display or use.

[mlutfy]

This is not storing localized amounts. I agree with Joe, the change most probably caused another bug to creep up. We definitely need to have more tests that use "," as a decimal separator.

[eileenmcnaughton]

I have logged a critical issue for this - https://issues.civicrm.org/jira/browse/CRM-21562

The problem comes from a mis-fix years ago https://issues.civicrm.org/jira/browse/CRM-10974 that is exposed by this (it was actually identified as a mis-fix at the time but was committed anyway).

I can replicate a variant of this in a unit test

[eileenmcnaughton]

@magnolia61 - the fix for this appears to be the removal of one line of code https://github.com/civicrm/civicrm-core/pull/11412/files#diff-e57ed384488b5985e5a0a5c166a6b0f0L361 - we are looking to drop 4.7.29 quickly to address this but you might like to try that now

[colemanw]

@magnolia61 please share the results of your testing asap.

[magnolia61]

I will be able to test this afternoon. Glad there is a solution!

[eileenmcnaughton]

I've added some further fixes to that PR that affect back office contribution entry #11412