Fix bug in Annual summary query (on contact dashboard) whereby a left join gives...

    As demonstrated in the test the left join does not do any filtering but it DOES join in as many line items as match the
    permitted types - resulting in too many rows & the sum calculated on them being incorrect.

    As this doesn't work it should be removed - I would aniticpate that sites with the financialreportacls extensions
    would get this adequately added through CRM_Financial_BAO_FinancialType::addACLClausesToWhereClauses
    but regardless this should go as it causes a bug without achieving it's intended result and adding a test prevents
    a later fix from re-breaking

diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php
index 21d40d568a5a162484dacf015089213cb0b9ee1c..7cf97b715cfd9ef9f25691f831634733ca229114 100644 (file)
--- a/CRM/Contribute/BAO/Contribution.php
+++ b/CRM/Contribute/BAO/Contribution.php
@@ -5581,14 +5581,7 @@ LIMIT 1;";
     }
     $startDate = "$year$monthDay";
     $endDate = "$nextYear$monthDay";
-    $financialTypes = [];
-    CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($financialTypes);
-    // this is a clumsy way of saying never return anything
-    // @todo improve!
-    $liWhere = " AND i.financial_type_id IN (0)";
-    if (!empty($financialTypes)) {
-      $liWhere = " AND i.financial_type_id NOT IN (" . implode(',', array_keys($financialTypes)) . ")";
-    }
+
     $whereClauses = [
       'contact_id' => 'IN (' . $contactIDs . ')',
       'contribution_status_id' => '= ' . (int) CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Completed'),
@@ -5609,7 +5602,6 @@ LIMIT 1;";
              AVG(total_amount) as average,
              currency
       FROM civicrm_contribution b
-      LEFT JOIN civicrm_line_item i ON i.contribution_id = b.id AND i.entity_table = 'civicrm_contribution' $liWhere
       WHERE " . $whereClauseString . "
       GROUP BY currency
       ";

diff --git a/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php b/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php
index 21ac309634ae000c7fcfeba95a151a6430803704..16a9d0eb47816d4f4c12484dfd50772da3b5aba6 100644 (file)
--- a/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php
+++ b/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php
@@ -32,6 +32,7 @@
 class CRM_Contribute_BAO_ContributionTest extends CiviUnitTestCase {
 
   use CRMTraits_Financial_FinancialACLTrait;
+  use CRMTraits_Financial_PriceSetTrait;
 
   /**
    * Clean up after tests.
@@ -324,6 +325,23 @@ class CRM_Contribute_BAO_ContributionTest extends CiviUnitTestCase {
     $this->disableFinancialACLs();
   }
 
+  /**
+   * Test the annual query returns a correct result when multiple line items are present.
+   */
+  public function testAnnualWithMultipleLineItems() {
+    $contactID = $this->createLoggedInUserWithFinancialACL();
+    $this->createContributionWithTwoLineItemsAgainstPriceSet([
+      'contact_id' => $contactID]
+    );
+    $this->enableFinancialACLs();
+    $sql = CRM_Contribute_BAO_Contribution::getAnnualQuery([$contactID]);
+    $result = CRM_Core_DAO::executeQuery($sql);
+    $result->fetch();
+    $this->assertEquals(300, $result->amount);
+    $this->assertEquals(1, $result->count);
+    $this->disableFinancialACLs();
+  }
+
   /**
    * Test that financial type data is not added to the annual query if acls not enabled.
    */

diff --git a/tests/phpunit/CRMTraits/Financial/PriceSetTrait.php b/tests/phpunit/CRMTraits/Financial/PriceSetTrait.php
new file mode 100644 (file)
index 0000000..48d1020
--- /dev/null
+++ b/tests/phpunit/CRMTraits/Financial/PriceSetTrait.php
@@ -0,0 +1,61 @@
+<?php
+/*
+ +--------------------------------------------------------------------+
+ | CiviCRM version 5                                                  |
+ +--------------------------------------------------------------------+
+ | Copyright CiviCRM LLC (c) 2004-2019                                |
+ +--------------------------------------------------------------------+
+ | This file is a part of CiviCRM.                                    |
+ |                                                                    |
+ | CiviCRM is free software; you can copy, modify, and distribute it  |
+ | under the terms of the GNU Affero General Public License           |
+ | Version 3, 19 November 2007 and the CiviCRM Licensing Exception.   |
+ |                                                                    |
+ | CiviCRM is distributed in the hope that it will be useful, but     |
+ | WITHOUT ANY WARRANTY; without even the implied warranty of         |
+ | MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.               |
+ | See the GNU Affero General Public License for more details.        |
+ |                                                                    |
+ | You should have received a copy of the GNU Affero General Public   |
+ | License and the CiviCRM Licensing Exception along                  |
+ | with this program; if not, contact CiviCRM LLC                     |
+ | at info[AT]civicrm[DOT]org. If you have questions about the        |
+ | GNU Affero General Public License or the licensing of CiviCRM,     |
+ | see the CiviCRM license FAQ at http://civicrm.org/licensing        |
+ +--------------------------------------------------------------------+
+ */
+
+/**
+ * Trait PriceSetTrait
+ *
+ * Trait for working with Price Sets in tests
+ */
+trait CRMTraits_Financial_PriceSetTrait {
+
+  /**
+   * Create a contribution with 2 line items.
+   *
+   * This also involves creating t
+   *
+   * @param $params
+   */
+  protected function createContributionWithTwoLineItemsAgainstPriceSet($params) {
+    $params = array_merge(['total_amount' => 300, 'financial_type_id' => 'Donation'], $params);
+    $priceFields = $this->createPriceSet('contribution');
+    foreach ($priceFields['values'] as $key => $priceField) {
+      $params['line_items'][]['line_item'][$key] = [
+        'price_field_id' => $priceField['price_field_id'],
+        'price_field_value_id' => $priceField['id'],
+        'label' => $priceField['label'],
+        'field_title' => $priceField['label'],
+        'qty' => 1,
+        'unit_price' => $priceField['amount'],
+        'line_total' => $priceField['amount'],
+        'financial_type_id' => $priceField['financial_type_id'],
+        'entity_table' => 'civicrm_contribution',
+      ];
+    }
+    $this->callAPISuccess('order', 'create', $params);
+  }
+
+}