dev/core#2003 Civicrm_price_field_value.amount truncation when localisation in play

[eileenmcnaughton]

Overview

This fixes the bug that derailed the fix for https://lab.civicrm.org/dev/core/-/issues/1603

On further testing I agree with Jitendra that the price field form was mis-saving the longer values (I only
tested with Euro style decimal separators but maybe on both) and it was bad data caused by this
that made the merged fix look like a regression.

I also observed that the Money functions intended to round only appeared to since it was rounding
just fine to 2 places, as tested, but it was still rounding to 2 places when we wanted more.

This fixes both the New Price field screen (when options are entered) and the edit options screen

Before

This amount string is truncated on save - ie the decimals below don't hit the civicrm_price_field_option_value table

After

Amount is saved correctly into the db - displays rounded to 2 digits (unchanged)

Technical Details

@jaapjansma

Comments

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

test this please

[seamuslee001]

@eileenmcnaughton just thinking it might be worthwhile somewhere having a comment about what this actually looks like i.e. '%!.2i' But this seems to work according to the docs (and here we go again on the money_format)

[jaapjansma]

@eileenmcnaughton we have tested this PR and our conlcusion is that this fixes one problem but introduces new problems and this PR needs further work.

We have tested the following:

• A price field with checkbox
• A price field with radio
• A price field with Text/numeric

We tested the following variations when it came to decimal separator and thousand separator:

• , [comma]
• . [dot]

We tested the following amounts:

• 123,456,789.987654321
• 12.987654321

And we tested it on dmaster (without this patch) and an environment with this patch

Results
The results below are the results of testing this in an environment with this patch

Amount entered	Decimal Separator	Thousand Separator	Field	Amount when pressing edit
123,456,789.987654321	.	,	Checkbox	123,456,789.980000019 a
12.987654321	.	,	Checkbox	12.987654321
123,456,789.987654321	.	,	Radio	123,456,789.980000019 a
12.987654321	.	,	Radio	12.987654321
123,456,789.987654321	.	,	Text / Numeric	123,456,789.99 b
12.987654321	.	,	Text / Numeric	12.99 b
123.456.789,987654321	,	.	Checkbox	123.456.789,980000019 a
12,987654321	,	.	Checkbox	12,987654321
123.456.789,987654321	,	.	Radio	123.456.789,980000019 a
12,987654321	,	.	Radio	12,987654321
123.456.789,987654321	,	.	Text / Numeric	123.456.789,99 b
12,987654321	,	.	Text / Numeric	12,99 b

a) This is really strange behaviour, some numbers are suddenly a 0 and it ends with 19 it looks like it does something random with the numbers behind the decimal
b) The text/numeric field does not implement this fix.

We discovered a new issue with price fields in dmaster where 12,987654321 is converted into 12,00 and we reported that issue in lab/2003

To prevent we are dealing, with incidents around different scenario's, our advice is that the issue with the money fields and decimal separator (and/or VAT) should be addressed within a broader scope to come up with an overall solution. (We will ask within CiviCooP whether we can address this and take this upon us)

[eileenmcnaughton]

@jaapjansma did you check what is saved to the db as this patch is primarily about the amount saved I'm happy if the amount displayed stays out of scope

[jaapjansma]

No I did not check the database and we strongly think that this patch introduces new problems. When you enter 123,456,789.987654321 and it becomes 123,456,789.9800000019 then something is clearly wrong.

[eileenmcnaughton]

@jaapjansma I have updated this so it ONLY fixes what is saved to the db. It still presents the value rounded in the edit screen so you can't tell from the UI whether it has worked or not.

Note that the issue you hit is that php deals with floats in strange ways at times - the fix we have selected for this is to use the brickmoney library. This now ships with core but as it relies on the php-intl extension our intent is to give the status checks a few months to get sites updated before actually using it. When you want to dig into this & go deep on floats please use brickmoney

[eileenmcnaughton]

test this please

Removing has-test as scope is narrowed

[eileenmcnaughton]

I note it's impossible to submit 123,456,789.987654321 on membership type form (as min_fee) as it breaks form validation as being too long.

[seamuslee001]

I took a look at this one and I found that I was able to generate a unit test for the storing in the database #18414 and found that if you included a , in the option amount column when adding a new price field at least according to the unit test the amount would not be saved correctly. My unit test passes with this merged and as this is aimed solely on the storing in the database not on the presentation layer this looks good to me to merge.

I would also add that I agree the presentation layer problems are a bug and need to be fixed