From e967ce8fe2b58b94e2163dde395542e55599da13 Mon Sep 17 00:00:00 2001 From: Eileen McNaughton Date: Thu, 20 May 2021 15:12:42 +1200 Subject: [PATCH] Fix for tax rates being mangled on contribution update Update sample code Handle tax_amount as empty string m --- CRM/Contribute/BAO/Contribution.php | 99 +++---------------- CRM/Price/BAO/LineItem.php | 26 +++-- api/v3/examples/Contribution/Create.ex.php | 2 +- .../CRM/Contribute/Form/ContributionTest.php | 4 + .../phpunit/CRM/Event/Form/Task/BatchTest.php | 12 ++- tests/phpunit/api/v3/ContributionPageTest.php | 2 +- tests/phpunit/api/v3/OrderTest.php | 11 +++ .../api/v3/TaxContributionPageTest.php | 4 +- 8 files changed, 62 insertions(+), 98 deletions(-) diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php index c5f2320e28..e792ac3173 100644 --- a/CRM/Contribute/BAO/Contribution.php +++ b/CRM/Contribute/BAO/Contribution.php @@ -174,9 +174,23 @@ class CRM_Contribute_BAO_Contribution extends CRM_Contribute_DAO_Contribution { CRM_Price_BAO_LineItem::getLineItemArray($params, $contributionID ? [$contributionID] : NULL); } + // We should really ALWAYS calculate tax amount off the line items. + // In order to be a bit cautious we are just messaging rather than + // overwriting in cases where we were not previously setting it here. + $taxAmount = $lineTotal = 0; + foreach ($params['line_item'] ?? [] as $lineItems) { + foreach ($lineItems as $lineItem) { + $taxAmount += (float) ($lineItem['tax_amount'] ?? 0); + $lineTotal += (float) ($lineItem['line_total'] ?? 0); + } + } if (!isset($params['tax_amount']) && $setPrevContribution && (isset($params['total_amount']) || isset($params['financial_type_id']))) { - $params = CRM_Contribute_BAO_Contribution::checkTaxAmount($params); + $params['tax_amount'] = $taxAmount; + $params['total_amount'] = $taxAmount + $lineTotal; + } + if (isset($params['tax_amount']) && $params['tax_amount'] != $taxAmount && empty($params['skipLineItem'])) { + CRM_Core_Error::deprecatedWarning('passing in incorrect tax amounts is deprecated'); } CRM_Utils_Hook::pre($action, 'Contribution', $contributionID, $params); @@ -3956,6 +3970,7 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac * Optional amount to override the saved amount paid (e.g if calculating what it WILL be). * * @return float + * @throws \CRM_Core_Exception */ public static function getContributionBalance($contributionId, $contributionTotal = NULL) { if ($contributionTotal === NULL) { @@ -3969,88 +3984,6 @@ INNER JOIN civicrm_activity ON civicrm_activity_contact.activity_id = civicrm_ac ); } - /** - * Get the tax amount (misnamed function). - * - * @param array $params - * - * @return array - * @throws \CiviCRM_API3_Exception - */ - protected static function checkTaxAmount($params) { - $taxRates = CRM_Core_PseudoConstant::getTaxRates(); - - // Update contribution. - if (!empty($params['id'])) { - // CRM-19126 and CRM-19152 If neither total or financial_type_id are set on an update - // there are no tax implications - early return. - if (!isset($params['total_amount']) && !isset($params['financial_type_id'])) { - return $params; - } - if (empty($params['prevContribution'])) { - $params['prevContribution'] = self::getOriginalContribution($params['id']); - } - - foreach (['total_amount', 'financial_type_id', 'fee_amount'] as $field) { - if (!isset($params[$field])) { - if ($field == 'total_amount' && $params['prevContribution']->tax_amount) { - // Tax amount gets added back on later.... - $params['total_amount'] = $params['prevContribution']->total_amount - - $params['prevContribution']->tax_amount; - } - else { - $params[$field] = $params['prevContribution']->$field; - if ($params[$field] != $params['prevContribution']->$field) { - } - } - } - } - - self::calculateMissingAmountParams($params, $params['id']); - if (!array_key_exists($params['financial_type_id'], $taxRates)) { - // Assign tax Amount on update of contribution - if (!empty($params['prevContribution']->tax_amount)) { - $params['tax_amount'] = 'null'; - foreach ($params['line_item'] as $setID => $priceField) { - foreach ($priceField as $priceFieldID => $priceFieldValue) { - $params['line_item'][$setID][$priceFieldID]['tax_amount'] = $params['tax_amount']; - } - } - } - } - } - - // New Contribution and update of contribution with tax rate financial type - if (isset($params['financial_type_id']) && array_key_exists($params['financial_type_id'], $taxRates) && - empty($params['skipLineItem'])) { - $taxRateParams = $taxRates[$params['financial_type_id']]; - $taxAmount = CRM_Contribute_BAO_Contribution_Utils::calculateTaxAmount(CRM_Utils_Array::value('total_amount', $params), $taxRateParams); - $params['tax_amount'] = round($taxAmount['tax_amount'], 2); - - foreach ($params['line_item'] as $setID => $priceField) { - foreach ($priceField as $priceFieldID => $priceFieldValue) { - $params['line_item'][$setID][$priceFieldID]['tax_amount'] = $params['tax_amount']; - } - } - $params['total_amount'] = CRM_Utils_Array::value('total_amount', $params) + $params['tax_amount']; - } - elseif (isset($params['api.line_item.create'])) { - // Update total amount of contribution using lineItem - $taxAmountArray = []; - foreach ($params['api.line_item.create'] as $key => $value) { - if (isset($value['financial_type_id']) && array_key_exists($value['financial_type_id'], $taxRates)) { - $taxRate = $taxRates[$value['financial_type_id']]; - $taxAmount = CRM_Contribute_BAO_Contribution_Utils::calculateTaxAmount($value['line_total'], $taxRate); - $taxAmountArray[] = round($taxAmount['tax_amount'], 2); - } - } - $params['tax_amount'] = array_sum($taxAmountArray); - $params['total_amount'] = $params['total_amount'] + $params['tax_amount']; - } - - return $params; - } - /** * Check financial type validation on update of a contribution. * diff --git a/CRM/Price/BAO/LineItem.php b/CRM/Price/BAO/LineItem.php index 962a575ba5..183019dbe5 100644 --- a/CRM/Price/BAO/LineItem.php +++ b/CRM/Price/BAO/LineItem.php @@ -52,11 +52,8 @@ class CRM_Price_BAO_LineItem extends CRM_Price_DAO_LineItem { $params['unit_price'] = 0; } } - - $taxRates = CRM_Core_PseudoConstant::getTaxRates(); - if (isset($params['financial_type_id'], $params['line_total'], $taxRates[$params['financial_type_id']])) { - $taxRate = $taxRates[$params['financial_type_id']]; - $params['tax_amount'] = ($taxRate / 100) * $params['line_total']; + if (isset($params['financial_type_id'], $params['line_total'])) { + $params['tax_amount'] = self::getTaxAmountForLineItem($params); } $lineItemBAO = new CRM_Price_BAO_LineItem(); @@ -493,7 +490,7 @@ WHERE li.contribution_id = %1"; $totalAmount = $params['total_amount'] ?? 0; $financialType = $params['financial_type_id'] ?? NULL; foreach ($priceSetDetails as $values) { - if ($entityTable == 'membership') { + if ($entityTable === 'membership') { if ($isRelatedID != $values['membership_type_id']) { continue; } @@ -502,7 +499,7 @@ WHERE li.contribution_id = %1"; } $financialType = $values['financial_type_id']; } - $params['line_item'][$values['setID']][$values['priceFieldID']] = [ + $lineItem = [ 'price_field_id' => $values['priceFieldID'], 'price_field_value_id' => $values['priceFieldValueID'], 'label' => $values['label'], @@ -512,6 +509,8 @@ WHERE li.contribution_id = %1"; 'financial_type_id' => $financialType, 'membership_type_id' => $values['membership_type_id'], ]; + $lineItem['tax_amount'] = self::getTaxAmountForLineItem($lineItem); + $params['line_item'][$values['setID']][$values['priceFieldID']] = $lineItem; break; } } @@ -1071,6 +1070,19 @@ WHERE li.contribution_id = %1"; return Civi::$statics[__CLASS__]['price_fields'][$priceFieldID]; } + /** + * Get the tax rate for the given line item. + * + * @param array $params + * + * @return float + */ + protected static function getTaxAmountForLineItem(array $params): float { + $taxRates = CRM_Core_PseudoConstant::getTaxRates(); + $taxRate = $taxRates[$params['financial_type_id']] ?? 0; + return ($taxRate / 100) * $params['line_total']; + } + /** * Helper function to retrieve financial trxn parameters to reverse * for given financial item identified by $financialItemID diff --git a/api/v3/examples/Contribution/Create.ex.php b/api/v3/examples/Contribution/Create.ex.php index 1d47cb3a30..c8fedf7f64 100644 --- a/api/v3/examples/Contribution/Create.ex.php +++ b/api/v3/examples/Contribution/Create.ex.php @@ -81,7 +81,7 @@ function contribution_create_expectedresult() { 'check_number' => '', 'campaign_id' => '', 'creditnote_id' => '', - 'tax_amount' => '', + 'tax_amount' => 0, 'revenue_recognition_date' => '', 'is_template' => '', 'contribution_type_id' => '1', diff --git a/tests/phpunit/CRM/Contribute/Form/ContributionTest.php b/tests/phpunit/CRM/Contribute/Form/ContributionTest.php index 5e6f051642..bb5aae45a2 100644 --- a/tests/phpunit/CRM/Contribute/Form/ContributionTest.php +++ b/tests/phpunit/CRM/Contribute/Form/ContributionTest.php @@ -918,6 +918,10 @@ Price Field - Price Field 1 1 $ 100.00 $ 100.00 * * @param string $thousandSeparator * + * @throws \CRM_Core_Exception + * @throws \CiviCRM_API3_Exception + * @throws \Civi\Payment\Exception\PaymentProcessorException + * * @dataProvider getThousandSeparators */ public function testSubmitUpdate(string $thousandSeparator): void { diff --git a/tests/phpunit/CRM/Event/Form/Task/BatchTest.php b/tests/phpunit/CRM/Event/Form/Task/BatchTest.php index d9e590cfbd..bfb4e3ffa3 100644 --- a/tests/phpunit/CRM/Event/Form/Task/BatchTest.php +++ b/tests/phpunit/CRM/Event/Form/Task/BatchTest.php @@ -30,11 +30,15 @@ class CRM_Event_Form_Task_BatchTest extends CiviUnitTestCase { /** * Test the the submit function on the event participant submit function. * - * Test is to establish existing behaviour prior to code cleanup. It turns out the existing - * code ONLY cancels the contribution as well as the participant record if is_pay_later is true - * AND the source is 'Online Event Registration'. + * Test is to establish existing behaviour prior to code cleanup. It turns + * out the existing code ONLY cancels the contribution as well as the + * participant record if is_pay_later is true AND the source is 'Online Event + * Registration'. + * + * @throws \CRM_Core_Exception + * @throws \CiviCRM_API3_Exception */ - public function testSubmitCancel() { + public function testSubmitCancel(): void { $this->createEventOrder(['source' => 'Online Event Registration', 'is_pay_later' => 1]); $participantCancelledStatusID = CRM_Core_PseudoConstant::getKey('CRM_Event_BAO_Participant', 'status_id', 'Cancelled'); diff --git a/tests/phpunit/api/v3/ContributionPageTest.php b/tests/phpunit/api/v3/ContributionPageTest.php index 52aa8fd34b..7c98dfe9d3 100644 --- a/tests/phpunit/api/v3/ContributionPageTest.php +++ b/tests/phpunit/api/v3/ContributionPageTest.php @@ -1866,7 +1866,7 @@ class api_v3_ContributionPageTest extends CiviUnitTestCase { // Ensure total_amount are the same if they're both given. $total_amount = $rawParams['total_amount'] ?? NULL; $amount = $rawParams['amount'] ?? NULL; - if (!empty($total_amount) && !empty($amount) && $total_amount !== $amount) { + if (!empty($total_amount) && !empty($amount) && round($total_amount, 2) !== round($amount, 2)) { throw new CRM_Core_Exception("total_amount '$total_amount' and amount '$amount' differ."); } diff --git a/tests/phpunit/api/v3/OrderTest.php b/tests/phpunit/api/v3/OrderTest.php index 24af7808cc..74f27905ab 100644 --- a/tests/phpunit/api/v3/OrderTest.php +++ b/tests/phpunit/api/v3/OrderTest.php @@ -675,6 +675,17 @@ class api_v3_OrderTest extends CiviUnitTestCase { 'tax_amount' => 9.69, ]); + $contribution = Contribution::get(FALSE) + ->addWhere('trxn_id', '=', 'WooCommerce Order - 1859') + ->setSelect(['tax_amount', 'total_amount'])->execute()->first(); + + $this->assertEquals('9.69', $contribution['tax_amount']); + $this->assertEquals('109.69', $contribution['total_amount']); + Contribution::update()->setValues([ + 'source' => 'new one', + 'financial_type_id' => $this->ids['FinancialType']['woo'], + ])->addWhere('id', '=', $contribution['id'])->execute(); + $contribution = Contribution::get(FALSE) ->addWhere('trxn_id', '=', 'WooCommerce Order - 1859') ->setSelect(['tax_amount', 'total_amount'])->execute()->first(); diff --git a/tests/phpunit/api/v3/TaxContributionPageTest.php b/tests/phpunit/api/v3/TaxContributionPageTest.php index cd17b75261..cbec27ee0a 100644 --- a/tests/phpunit/api/v3/TaxContributionPageTest.php +++ b/tests/phpunit/api/v3/TaxContributionPageTest.php @@ -392,8 +392,8 @@ class api_v3_TaxContributionPageTest extends CiviUnitTestCase { ]); $this->assertEquals('300.00', $lineItems); - $this->assertEquals('300.00', $this->_getFinancialTrxnAmount($contribution['id'])); - $this->assertEquals('300.00', $this->_getFinancialItemAmount($contribution['id'])); + $this->assertEquals('320.00', $this->_getFinancialTrxnAmount($contribution['id'])); + $this->assertEquals('320.00', $this->_getFinancialItemAmount($contribution['id'])); } /** -- 2.25.1