[wip] CRM-19585, added function to Calculate amounts when Financial type fo… #9683
Conversation
|
depends on #9684 |
| ); | ||
| if (isset($params['prevContribution']->total_amount) || isset($params['tax_amount'])) { | ||
| $taxAmount = CRM_Utils_Array::value('tax_amount', $params, 0); | ||
| $changesinTaxAmount = $totalAmount - $params['prevContribution']->total_amount + $params['prevContribution']->tax_amount - $taxAmount; |
There was a problem hiding this comment.
Testing for a change in amounts using this formula could yield false negatives, if for example I changed amount by -$10 and changed tax by +$10. I'm not sure if this case is possible, given the business logic outside of this function. Still, it would be nice to have a function that worked on all cases, despite any outside logic, to decrease coupling. Of course, unless its intentional to just check if (total amount + tax) has any change.
There was a problem hiding this comment.
@pradpnayak @JoeMurray what can we do to prevent this from happening?
There was a problem hiding this comment.
Thanks, @MiyaNoctem ! @pradpnayak please review all of the logic here. It seems we need to do the calculations if a previous contribution exists, rather than if its total_amount is non-empty. Also, we need to do it regardless of whether there is tax amount on current trxn.
| @@ -5457,4 +5457,37 @@ public static function createProportionalFinancialEntries($entityParams, $lineIt | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * Calculate amounts when Financial type for contribution is changed. | |||
| * | |||
There was a problem hiding this comment.
It would be nice to have better documentation for this method, to know what the intended functionality is supposed to be. For example, the current description says the function calculates amounts when financial type for contribution is changed. However, the code doesn't seem to check financial type at all. A better description could be something like:
Calculate amounts if total amount and/or tax amount has changed vs previous contribution.
| * Calculate amounts when Financial type for contribution is changed. | ||
| * | ||
| * @param array $params | ||
| * contribution params |
There was a problem hiding this comment.
It would be great if you could add a better description for this array, concentrating on the parameters that are used within the function, like 'prevContribution' and 'tax_amount'. I mean, by the code, you could assume $params['tax_amount'] is the new tax amount, but it's always better to be explicit.
| * | ||
| * @return array | ||
| */ | ||
| public static function calculateFTChangeAmount(&$params, $trxnAmount, &$totalAmount) { |
There was a problem hiding this comment.
The name for this function doesn't seem to properly reflect what the function does, since financial type is not checked. Maybe calculateAmountsOnChange?
b09cbff to
21db357
Compare
|
Jenkins, test this please |
There was a problem hiding this comment.
I get that it's good practice to commit a new function with a test, and I like the chunking. But, can we also make sure that there is at least one call to the function in the PR, with a test that shows the functionality working in a full flow.
comments per previous PRs about opening an issue for the bug apply. I'm not really sure what you are fixing in this PR or how to replicate it
|
Setting to WIP - need a ticket describing what this is trying to solve, we should aim to have tests that test the full flow |
|
+1 re: Eileen's comment about a) including at least one call to new function in the PR committing the new function I will want to see the call to the new function to see if it is a new test that is required (most likely) or perhaps a modification of an existing test. |
…r contribution is changed. ---------------------------------------- * CRM-19585: Sales tax issue https://issues.civicrm.org/jira/browse/CRM-19585
---------------------------------------- * CRM-19585: Sales tax issue https://issues.civicrm.org/jira/browse/CRM-19585
|
Merged into #10172 |
…r contribution is changed.
https://issues.civicrm.org/jira/browse/CRM-19585