Fix for tax rates being mangled on contribution update

Update sample code

Handle tax_amount as empty string

m

CRM/Contribute/BAO/Contribution.php

@@ -174,9 +174,23 @@ public static function add(&$params) {
       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 @@ public static function getPaymentInfo($id, $component = 'contribution', $getTrxn
    *   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 @@ public static function getContributionBalance($contributionId, $contributionTota
     );
   }
 
-  /**
-   * 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.
    *

CRM/Price/BAO/LineItem.php

@@ -52,11 +52,8 @@ public static function create(&$params) {
         $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 @@ public static function getLineItemArray(&$params, $entityId = NULL, $entityTable
       $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 @@ public static function getLineItemArray(&$params, $entityId = NULL, $entityTable
           }
           $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 @@ public static function getLineItemArray(&$params, $entityId = NULL, $entityTable
           '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 @@ protected static function getPriceFieldMetaData($key) {
     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

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',

tests/phpunit/CRM/Contribute/Form/ContributionTest.php

@@ -918,6 +918,10 @@ public function testEnterNegativeContribution() {
    *
    * @param string $thousandSeparator
    *
+   * @throws \CRM_Core_Exception
+   * @throws \CiviCRM_API3_Exception
+   * @throws \Civi\Payment\Exception\PaymentProcessorException
+   *
    * @dataProvider getThousandSeparators
    */
   public function testSubmitUpdate(string $thousandSeparator): void {

tests/phpunit/CRM/Event/Form/Task/BatchTest.php

@@ -30,11 +30,15 @@ public function testSubmit() {
   /**
    * 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');
 

tests/phpunit/api/v3/ContributionPageTest.php

@@ -1866,7 +1866,7 @@ public function hook_civicrm_alterPaymentProcessorParams($paymentObj, $rawParams
     // 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.");
     }
 

tests/phpunit/api/v3/OrderTest.php

@@ -675,6 +675,17 @@ public function testOrderWithMixedTax(): void {
       '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();

tests/phpunit/api/v3/TaxContributionPageTest.php

@@ -392,8 +392,8 @@ public function testCreateUpdateContributionChangeTotal(): void {
     ]);
 
     $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']));
   }
 
   /**