CRM-17647 fix ContributionForm to use skipCleanMoney on update & upda… #11575
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -1040,7 +1040,7 @@ public static function processContribution( | |||
|
|
|||
| $contribParams['skipLineItem'] = 1; | |||
| // create contribution record | |||
| $contribution = CRM_Contribute_BAO_Contribution::add($contribParams, $ids); | |||
There was a problem hiding this comment.
$ids is empty & deprecated
| @@ -1636,7 +1632,7 @@ protected function submit($submittedValues, $action, $pledgePaymentID) { | |||
| if (!empty($params['note']) && !empty($submittedValues['note'])) { | |||
| unset($params['note']); | |||
| } | |||
| $contribution = CRM_Contribute_BAO_Contribution::create($params, $ids); | |||
| $contribution = CRM_Contribute_BAO_Contribution::create($params); | |||
There was a problem hiding this comment.
$ids is deprecated & we have $params['id'] set
eileenmcnaughton
force-pushed
the
currency_contribution_update
branch
2 times, most recently
from
35a7090 to
7637daa
Compare
|
jenkins, test this please |
…te tests to cover
eileenmcnaughton
force-pushed
the
currency_contribution_update
branch
from
3dd3242 to
66ea983
Compare
|
@monishdeb are you able to review this? Am trying to get instances of 'BAO is called without skipCleanMoney' down to zero so we can deprecate & remove the (unreliable) BAO-cleaning - per #11539 |
|
I have tested the patch in my local and fixes are straighforward, doesn't cause any unintended regression. Changes in unit-tests makes sense. Happy with this patch, merging now. |
|
thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
As part of resolving issues around currency separators this PR adds tests to submitting the contribution form in update mode. Identified instances of mis-recording amounts have been fixed but there may well be more & we need to fix the underlying issue.
Before
tests don't cover both separator variants, BAO is cleaning the values when they should be & are being cleaned in the form
After
Additional tests. BAO not being permitted to cleanMoney
Technical Details
The underlying issue is the attempt to 'cleanMoney' in the Contribution_BAO is unreliable. At some point it was determined that money might have already been cleaned before this point (true) & some unreliable extra handling was added (bad).
To get back to reliability around handling numbers with a thousand separator we need to
a) make sure that all core places in code that call Contribute_BAO_Contribution::create pass 'skipCleanMoney' (the api already does)
b) add a deprecation notice so that any code that calls it without skipCleanMoney gets a notice for a few versions
c) remove cleaning money from the contribution BAO.
Comments
#11539 has tests identifying where the BAO is being allowed to attempt to clean money