[wip] CRM-19585, fixed Calculation of Tax amount when contribution is updated #9682
Conversation
pradpnayak
commented
Jan 16, 2017
•
- CRM-19585: Sales tax issue
|
Depends on #9683 |
There was a problem hiding this comment.
Not sure why it was decided to build this feature using different dependant PR's. Seems it could've been just as easily built in a single PR holding all changes. If there is a reason for this, could you explain it on the PR's description? This could really help bring other developers into the loop and understand the decisions that guided this.
|
Jenkins, test this please |
|
Hi @MiyaNoctem We generally split PRs into chunks of individual functions along with unit tests to support them if the size of the patch is too large. There are many benefits to this, especially when readability is concerned. A large PR with many commits is also more difficult to rebase incase it goes stale. HTH, |
| @@ -374,7 +374,8 @@ public static function calculateMissingAmountParams(&$params, $contributionID) { | |||
| $params['fee_amount'] = 0; | |||
| } | |||
| } | |||
| if (!isset($params['net_amount'])) { | |||
| // recalculate net amount if it is not set or if tax amount is set | |||
| if (!isset($params['net_amount']) || isset($params['tax_amount'])) { | |||
There was a problem hiding this comment.
There was discussion on how net amount relates to tax_amount elsewhere , I feel both @KarinG and @JoeMurray argued it was net of fees, not of taxes
There was a problem hiding this comment.
Right - tax_amount has nothing to do with net_amount - this is obsolete.
| @@ -3568,16 +3581,15 @@ public static function updateFinancialAccounts(&$params, $context = NULL, $skipT | |||
| if ($context == 'changeFinancialType' || self::isContributionStatusNegative($params['contribution']->contribution_status_id)) { | |||
| $diff = -1; | |||
| } | |||
| if (!empty($params['is_quick_config'])) { | |||
| * | ||
| * @return float | ||
| */ | ||
| public static function calculateFinancialItemAmount($params, $amountParams, $context) { | ||
| public static function calculateFinancialItemAmount($params, $amountParams, $context, &$fieldValues) { |
There was a problem hiding this comment.
this feels like a code smell. adding an extra pass by reference at the end is generally a clue to it being a bad practice. Is there another way? Could this be dealt with in the calling function?
There was a problem hiding this comment.
I agree with @eileenmcnaughton on this.
There was a problem hiding this comment.
So my main issue here is administrative - I don't really understand what is being fixed. The JIRA is just a big umbrella and I don't know how to replicate this bug without it being described in it's own JIRA.
The code looks generally ok-ish. I liked the removal of the is_quick but I fear it was actually just pushed into another function. The test seems to give end to end coverage. But I need the extra step of a JIRA that adequately explains what I'm looking at to do a full review
| @@ -1228,4 +1228,60 @@ public function createContributionWithTax() { | |||
| return array($contribution, $financialAccount); | |||
| } | |||
|
|
|||
| /** | |||
| * Test for change in FT | |||
There was a problem hiding this comment.
Can we add some comments as to what you are testing here? In general the contributionCreate function is more used in setting up for tests than the actual test, but I think you are using it for $this->callAPISuccess('Contribution', 'create', $params) & hence are testing the flow through the api (good). But it's hard work figuring out what is actually being tested here.
There was a problem hiding this comment.
+1 Give a sentence or two.
|
Setting to WIP, pending a ticket & description of what this is solving |
|
What's the Issue - there are plenty scenarios where Tax amount is correct when Contribution is updated - in latest dmaster. |
| @@ -3218,7 +3219,7 @@ public static function recordFinancialAccounts(&$params, $financialTrxnValues = | |||
| $params['trxnParams'] = $trxnParams; | |||
|
|
|||
| if (!empty($params['prevContribution'])) { | |||
| $updated = FALSE; | |||
| $updated = $ignoreChangeAmount = FALSE; | |||
There was a problem hiding this comment.
I'm concerned that you are not setting $ignoreChangeAmount in all use cases. This PR also uses a function self::calculateAmountsOnChange() to set it below that is not yet merged in core. It would be good to indicate the dependence of this PR on that other one, perhaps in comment if not title.
3d40b16 to
6a52ffd
Compare
---------------------------------------- * CRM-19585: Sales tax issue https://issues.civicrm.org/jira/browse/CRM-19585
…for contribution like Amount, FT, PI ---------------------------------------- * 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 |