Tax amount not added in event confirmation

Tax amount does not cumulate the tax amount from each line item and only show the amount for the last line :

PR to do but this should fix it :

diff --git a/CRM/Taxcalculator/BAO/CDNTaxes.php b/CRM/Taxcalculator/BAO/CDNTaxes.php
index 8deaa52..bed8268 100644
--- a/CRM/Taxcalculator/BAO/CDNTaxes.php
+++ b/CRM/Taxcalculator/BAO/CDNTaxes.php
@@ -113,7 +113,7 @@ class CRM_Taxcalculator_BAO_CDNTaxes extends CRM_Core_DAO  {
           $total_tax_rate = 0;
 
           foreach ($taxes as &$tax) {
-            $tax['amount'] = round($tax['rate'] * $item['line_total'] / 100, 2);
+            $tax['amount'] += round($tax['rate'] * $item['line_total'] / 100, 2);
             $total_tax_amount += $tax['amount'];
             $total_tax_rate += $tax['rate'];
           }

There's an issue with the $taxes variable: although it is passed by reference, it gets redefined along the way, which makes it impossible to add a per tax total to the original parameter.

This patch appears to fix the issue. It can only work if we assume that the &$taxes passed to the function has the same structure as the $event_taxes variable.

diff --git a/CRM/Taxcalculator/BAO/CDNTaxes.php b/CRM/Taxcalculator/BAO/CDNTaxes.php
index 8deaa52..0e29b24 100644
--- a/CRM/Taxcalculator/BAO/CDNTaxes.php
+++ b/CRM/Taxcalculator/BAO/CDNTaxes.php
@@ -57,6 +57,10 @@ class CRM_Taxcalculator_BAO_CDNTaxes extends CRM_Core_DAO  {
       }
     }
 
+    foreach ($taxes as &$tax) {
+      $tax['amount'] = 0;
+    }
+
     foreach ($lineItems as &$items) {
       foreach ($items as &$item) {
         if (!empty($taxableFinancialTypes[$item['financial_type_id']])) {
@@ -97,7 +101,7 @@ class CRM_Taxcalculator_BAO_CDNTaxes extends CRM_Core_DAO  {
               1 => [$item['entity_id'], 'Positive'],
             ]);
 
-            $taxes = CRM_Taxcalculator_BAO_CDNTaxes::getTaxesForEvent($event_id, $contact_id);
+            $event_taxes = CRM_Taxcalculator_BAO_CDNTaxes::getTaxesForEvent($event_id, $contact_id);
 
             CRM_Taxcalculator_BAO_CDNTaxes::trace('recalculateTaxesOnLineItems: found an event line item, loaded new tax_rate', [
               'contact_id' => $contact_id,
@@ -112,9 +116,9 @@ class CRM_Taxcalculator_BAO_CDNTaxes extends CRM_Core_DAO  {
           $total_tax_amount = 0;
           $total_tax_rate = 0;
 
-          foreach ($taxes as &$tax) {
-            $tax['amount'] = round($tax['rate'] * $item['line_total'] / 100, 2);
-            $total_tax_amount += $tax['amount'];
+          foreach ($event_taxes as $i => &$tax) {
+            $taxes[$i]['amount'] += round($tax['rate'] * $item['line_total'] / 100, 2);
+            $total_tax_amount += $taxes[$i]['amount'];
             $total_tax_rate += $tax['rate'];
           }

We definitely need a unitest with several line items to check that.

@urlisse Can you describe a way to reproduce the problem ?

You can reproduce the problem by generating an invoice with multiple taxed events.

The issue is that the $tax['amount'] value gets reset on every loop when encountering an event. In order to fix this, I renamed the $taxes variable to $event_taxes to distinguish it from the $taxes parameter passed as reference.

This solution only works if we can assume the $taxes and $event_taxes variables reflect the same taxes, which is beyond my current ability to confirm.

The above patch should be dismissed since it obviously breaks on non-event contributions.

The following patch could be used as a reference to provide a real solution.

diff --git a/CRM/Taxcalculator/BAO/CDNTaxes.php b/CRM/Taxcalculator/BAO/CDNTaxes.php
index 8deaa52..fc87c5f 100644
--- a/CRM/Taxcalculator/BAO/CDNTaxes.php
+++ b/CRM/Taxcalculator/BAO/CDNTaxes.php
@@ -57,6 +57,10 @@ class CRM_Taxcalculator_BAO_CDNTaxes extends CRM_Core_DAO  {
       }
     }
 
+    foreach ($taxes as &$tax) {
+      $tax['amount'] = 0;
+    }
+
     foreach ($lineItems as &$items) {
       foreach ($items as &$item) {
         if (!empty($taxableFinancialTypes[$item['financial_type_id']])) {
@@ -86,6 +90,9 @@ class CRM_Taxcalculator_BAO_CDNTaxes extends CRM_Core_DAO  {
             }
           }
 
+          $total_tax_amount = 0;
+          $total_tax_rate = 0;
+
           // This is possibly overkill, but to handle Event line items,
           // we need to check the tax rate for each line event, which may be in different locations.
           if ($entity_table == 'civicrm_participant') {
@@ -97,7 +104,7 @@ class CRM_Taxcalculator_BAO_CDNTaxes extends CRM_Core_DAO  {
               1 => [$item['entity_id'], 'Positive'],
             ]);
 
-            $taxes = CRM_Taxcalculator_BAO_CDNTaxes::getTaxesForEvent($event_id, $contact_id);
+            $event_taxes = CRM_Taxcalculator_BAO_CDNTaxes::getTaxesForEvent($event_id, $contact_id);
 
             CRM_Taxcalculator_BAO_CDNTaxes::trace('recalculateTaxesOnLineItems: found an event line item, loaded new tax_rate', [
               'contact_id' => $contact_id,
@@ -105,17 +112,24 @@ class CRM_Taxcalculator_BAO_CDNTaxes extends CRM_Core_DAO  {
               'financial_type_id' => $item['financial_type_id'],
               'tax_rates' => $taxes,
             ]);
-          }
-
-          // @todo Having a total tax rate seems wrong, since we have to separate
-          // the display of taxes.
-          $total_tax_amount = 0;
-          $total_tax_rate = 0;
 
-          foreach ($taxes as &$tax) {
-            $tax['amount'] = round($tax['rate'] * $item['line_total'] / 100, 2);
-            $total_tax_amount += $tax['amount'];
-            $total_tax_rate += $tax['rate'];
+            foreach ($event_taxes as $i => &$tax) {
+              // Assuming that the $taxes array is similar to the $event_taxes array (is this a given?)
+              $taxes[$i]['amount'] += round($tax['rate'] * $item['line_total'] / 100, 2);
+              $total_tax_amount += $taxes[$i]['amount'];
+              $total_tax_rate += $tax['rate'];
+            }
+          } else {
+            // @todo Having a total tax rate seems wrong, since we have to separate
+            // the display of taxes.
+            foreach ($taxes as &$tax) {
+              if (!isset($tax['amount'])) {
+                continue;
+              }
+              $tax['amount'] += round($tax['rate'] * $item['line_total'] / 100, 2);
+              $total_tax_amount += $tax['amount'];
+              $total_tax_rate += $tax['rate'];
+            }
           }
 
           // Required for Invoice Payment, where the tax amount is incorrect.

mentioned in commit 85bdbdb5

mentioned in merge request !5 (merged)

@urlisse @samuelsov Can you test !5 (merged) ?

I ran into this indirectly because I was trying to override the tax calculation using a hook (ex: for selling books in Canada, GST-only for all provinces).

mentioned in commit 3255dd64

Closing in favour of !5 (merged).

closed