CRM-17647 fix for submitting payment with thousand separator #11539
Conversation
|
This is failing because I added this line to identify all the other risk places. We need to grandfather cleaning of money out of CRM_Contribute_BAO_Contribution::create as it should be done on input so we can ensure it ONLY GETS DONE ONCE. Adding a deprecated causes all those places to fail tests. However, it seems in many cases the bad call is from the tests themselves so some cleanup on tests needed as step one |
|
note - without the deprecated this could go on 4.7.30 rc |
|
Many of the 71 failures represent real bugs of this sort - but some are just the test suite. I added a 'trap' for all cases where the BAO was being allowed to 'clean' money (which should always always happen at the form layer) We can get rid of a lot of the false positives with #11547 & will need some follow ups but is an example where we are at risk of messing with our money because it is being cleaned in the BAO whereas it should be only cleaned at the form layer & the BAO should receive 'skipCleanMoney' with a view to switching to removing that from the BAO altogether |
|
test this please |
|
(merged some test-only fixes - let's see how much it dropped by) |
|
@eileenmcnaughton now there's only 54 :/ |
3a360e4 to
65be216
Compare
|
more patches up-merged - let's count again. Also, I got rid of one e-notice scenario |
|
down to 32 - 2 real problems that I can see so far |
|
test this please |
| @@ -132,6 +132,7 @@ public static function add(&$params, $ids = array()) { | |||
| else { | |||
| // @todo put a deprecated here - this should be done in the form layer. | |||
There was a problem hiding this comment.
@eileenmcnaughton if this passes i think we should remove this to do
|
21 now - this will clear the next handful #11575 |
|
@monishdeb I don't expect this to pass yet - but #11575 should be mergeable I think |
|
@eileenmcnaughton yup I mentioned the test rebuild command too early :( Well I have merged #11575 and triggered the test build again. Lets see |
|
cool |
|
Jenkins test this please |
|
Latest test build shows 13 failures from 21 (-8). Will submit a separate PR for that. |
|
test this please |
|
@monishdeb down to 6 fails. (am not working on them at the moment :-) |
65be216 to
8604585
Compare
8604585 to
9d9f252
Compare
|
@monishdeb I tested the participant add & the money is appropriately 'clean' by the time it's being processed & I re-applied those minor test fixes. Crossing fingers this passes & we can merge. NB my money says would could just remove cleanMoney from the BAO now but I guess it won't hurt to go a couple of releases with the deprecation message |
|
@eileenmcnaughton there were 2 test failures which I have fixed now and added my commit in this PR. Hopefully, it will pass now. |
|
Jenkins test this please |
|
Test fail unrelated |
Overview
Fix for truncation of amount when a payment separator is used
Before
Amount mis-recorded
After
Amount correctly recorded
Technical Details
The general fix for this is to convert amounts ASAP after form submission & not try to handle them in the BAO which does not know if they are already converted
Comments
Unfortunately there are a few places this can happen - we have a way of adding them to unit tests - I replicated one variant from the ticket in a test but not the other.