[WIP]CRM-19585, Sales tax fixes #9340
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -374,7 +374,8 @@ public static function calculateMissingAmountParams(&$params, $contributionID) { | |
| $params['fee_amount'] = 0; | ||
| } | ||
| } | ||
| // recalculate net amount if it is not set or if tax amount is set | ||
| if (!isset($params['net_amount']) || isset($params['tax_amount'])) { | ||
| if (!$contributionID) { | ||
| $params['net_amount'] = $params['total_amount'] - $params['fee_amount']; | ||
| } | ||
|
|
@@ -3218,7 +3219,7 @@ public static function recordFinancialAccounts(&$params, $financialTrxnValues = | |
| $params['trxnParams'] = $trxnParams; | ||
|
|
||
| if (!empty($params['prevContribution'])) { | ||
| $updated = $ignoreChangeAmount = FALSE; | ||
| $params['trxnParams']['total_amount'] = $trxnParams['total_amount'] = $params['total_amount'] = $params['prevContribution']->total_amount; | ||
| $params['trxnParams']['fee_amount'] = $params['prevContribution']->fee_amount; | ||
| $params['trxnParams']['net_amount'] = $params['prevContribution']->net_amount; | ||
|
|
@@ -3263,9 +3264,9 @@ public static function recordFinancialAccounts(&$params, $financialTrxnValues = | |
| } | ||
| } | ||
| self::updateFinancialAccounts($params, 'changeFinancialType'); | ||
| $params['financial_account_id'] = $newFinancialAccount; | ||
| list($changeFTAmount, $ignoreChangeAmount) = self::calcluateFTChangeAmount($params, $trxnParams['total_amount']); | ||
| $params['total_amount'] = $params['trxnParams']['total_amount'] = $params['trxnParams']['net_amount'] = $changeFTAmount; | ||
| self::updateFinancialAccounts($params); | ||
| $params['trxnParams']['to_financial_account_id'] = $trxnParams['to_financial_account_id']; | ||
| $updated = TRUE; | ||
|
|
@@ -3341,7 +3342,7 @@ public static function recordFinancialAccounts(&$params, $financialTrxnValues = | |
| $params['trxnParams']['net_amount'] = CRM_Utils_Array::value('net_amount', $params); | ||
| $params['trxnParams']['total_amount'] = $trxnParams['total_amount'] = $params['total_amount'] = $totalAmount; | ||
| $params['trxnParams']['trxn_id'] = $params['contribution']->trxn_id; | ||
| if (!$ignoreChangeAmount && isset($totalAmount) && | ||
| $totalAmount != $params['prevContribution']->total_amount | ||
| ) { | ||
| //Update Financial Records | ||
|
|
@@ -3444,6 +3445,12 @@ public static function updateFinancialAccounts(&$params, $context = NULL, $skipT | |
| } | ||
| if ($context == 'changedAmount' || $context == 'changeFinancialType') { | ||
| $itemAmount = $params['trxnParams']['total_amount'] = $params['trxnParams']['net_amount'] = $params['total_amount'] - $params['prevContribution']->total_amount; | ||
| if (isset($params['prevContribution']->tax_amount)) { | ||
|
There was a problem hiding this comment. I am not sure about the logic here since I don't have enough context .. but shouldn't this be : if (isset($params['tax_amount'])) {There was a problem hiding this comment. @pradpnayak @Edzelopez could you reply to each comment explaining how it is addressed or why you don't think it should be? It appears the same changes have been resubmitted as a new PR but the commented code may still appear, eg https://github.com/civicrm/civicrm-core/pull/9682/files#diff-7a5b0e2d131dabc49178460fc63328c0R3453 There was a problem hiding this comment. The logic here (and below) is intentional. This code here handles the case where a contribution with a taxable financial type is edited to change the financial type to another taxable/non-taxable one. In such a case, previously, the taxes were not subtracted/added to the total amount. The reason why the conditions look reversed is that the taxes usually have reversed mathematical signs. So for example, if the previous total amount was $110 with a contribution of $100 and $10 tax, on changing this to a non-taxable financial type, we would need to subtract the $10. For this reason, we have checked if there was a previous tax applied to a contribution and subtracted it from the new total amount. Similarly, for the next condition, if we have changed the financial type to a new taxable financial type, say amounting to 20% tax, we will need to first subtract the previous tax amount from the contribution to avoid calculating 20% on $110 rather than $100. HTH, |
||
| $itemAmount -= CRM_Utils_Array::value('tax_amount', $params, 0); | ||
| } | ||
| if (isset($params['tax_amount'])) { | ||
|
There was a problem hiding this comment. same here .. shouldn't this be if (isset($params['prevContribution']->tax_amount))so basically flip the if condition with the one above it ... or am I missing something ? |
||
| $itemAmount += $params['prevContribution']->tax_amount; | ||
| } | ||
| } | ||
| if ($context == 'changedStatus') { | ||
| //get all the statuses | ||
|
|
@@ -3546,6 +3553,7 @@ public static function updateFinancialAccounts(&$params, $context = NULL, $skipT | |
| } | ||
| } | ||
| $trxn = CRM_Core_BAO_FinancialTrxn::create($params['trxnParams']); | ||
| $previousLineItem = CRM_Price_BAO_LineItem::getLineItemsByContributionID($params['prevContribution']->id); | ||
| $params['entity_id'] = $trxn->id; | ||
| if ($context != 'changePaymentInstrument') { | ||
| $itemParams['entity_table'] = 'civicrm_line_item'; | ||
|
|
@@ -3568,15 +3576,15 @@ public static function updateFinancialAccounts(&$params, $context = NULL, $skipT | |
| if ($context == 'changeFinancialType' || self::isContributionStatusNegative($params['contribution']->contribution_status_id)) { | ||
| $diff = -1; | ||
| } | ||
|
|
||
| // calculate financial item amount | ||
| $amountParams = array( | ||
| 'diff' => $diff, | ||
| 'line_total' => $fieldValues['line_total'], | ||
| 'previous_line_total' => CRM_Utils_Array::value('line_total', CRM_Utils_Array::value($fieldValues['id'], $previousLineItem)), | ||
| 'item_amount' => $itemAmount, | ||
| ); | ||
| $amount = self::calcluateFinancialItemAmount($params, $amountParams, $context); | ||
|
|
||
| $itemParams = array( | ||
| 'transaction_date' => $receiveDate, | ||
|
|
@@ -3593,13 +3601,25 @@ public static function updateFinancialAccounts(&$params, $context = NULL, $skipT | |
| $params['line_item'][$fieldId][$fieldValueId]['deferred_line_total'] = $amount; | ||
| $params['line_item'][$fieldId][$fieldValueId]['financial_item_id'] = $financialItem->id; | ||
|
|
||
| $taxAmount = CRM_Utils_Array::value('tax_amount', $fieldValues); | ||
| $taxFinancialType = $fieldValues['financial_type_id']; | ||
| if ($context == 'changeFinancialType') { | ||
| $taxAmount = $previousLineItem[$fieldValues['id']]['tax_amount']; | ||
| $taxFinancialType = $previousLineItem[$fieldValues['id']]['financial_type_id']; | ||
| } | ||
|
|
||
| if ($taxAmount) { | ||
| $invoiceSettings = Civi::settings()->get('contribution_invoice_settings'); | ||
| $taxTerm = CRM_Utils_Array::value('tax_term', $invoiceSettings); | ||
| if ($context == 'changedAmount') { | ||
| $itemParams['amount'] = $diff * ($fieldValues['tax_amount'] - $previousLineItem[$fieldValues['id']]['tax_amount']); | ||
| } | ||
| else { | ||
| $itemParams['amount'] = $diff * $taxAmount; | ||
| } | ||
| $itemParams['description'] = $taxTerm; | ||
| if ($taxFinancialType) { | ||
| $itemParams['financial_account_id'] = self::getFinancialAccountId($taxFinancialType); | ||
| } | ||
| CRM_Financial_BAO_FinancialItem::create($itemParams, NULL, $trxnIds); | ||
| } | ||
|
|
@@ -5336,4 +5356,61 @@ public static function recordAlwaysAccountsReceivable(&$trxnParams, $contributio | |
| $trxnParams['from_financial_account_id'] = $params['to_financial_account_id']; | ||
| } | ||
|
|
||
| /** | ||
| * Calculate amount when Financial type for contribution is changed. | ||
| * | ||
| * @param array $params | ||
| * contribution params | ||
| * @param float $totalAmount | ||
| * financial trxn total amount | ||
| * | ||
| * @return array | ||
| */ | ||
| public static function calcluateFTChangeAmount($params, $totalAmount) { | ||
| $changeFTAmount = $totalAmount; | ||
| $ignoreChangeAmount = FALSE; | ||
| 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; | ||
|
|
||
| if ($changesinTaxAmount == 0) { | ||
| $ignoreChangeAmount = TRUE; | ||
| $changeFTAmount = $totalAmount; | ||
| } | ||
| } | ||
|
|
||
| return array($changeFTAmount, $ignoreChangeAmount); | ||
| } | ||
|
|
||
| /** | ||
| * Calculate financial item amount when contribution is updated. | ||
| * | ||
| * @param array $params | ||
| * contribution params | ||
| * @param array $amountParams | ||
| * | ||
| * @param string $context | ||
| * | ||
| * @return float | ||
| */ | ||
| public static function calcluateFinancialItemAmount($params, $amountParams, $context) { | ||
| if (!empty($params['is_quick_config'])) { | ||
| $amount = $amountParams['item_amount']; | ||
| if (!$amount) { | ||
| $amount = $params['total_amount']; | ||
| if ($context === NULL) { | ||
| $amount -= CRM_Utils_Array::value('tax_amount', $params, 0); | ||
| } | ||
| } | ||
| } | ||
| else { | ||
| $amount = $amountParams['line_total']; | ||
| if ($context == 'changedAmount') { | ||
| $amount -= $amountParams['previous_line_total']; | ||
| } | ||
| $amount *= $amountParams['diff']; | ||
| } | ||
| return $amount; | ||
| } | ||
|
|
||
| } | ||
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
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
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
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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we torture our poor users by asking them to enter net_amount rather than always calculating it from fee amount, tax amount etc - has anyone else felt the love of altering total & then failing to save because net amount fails validation.
Erm that was a tangent