From 536660990a30485ae64de7581b136f9ca528d5f0 Mon Sep 17 00:00:00 2001
From: eileen <emcnaughton@wikimedia.org>
Date: Wed, 16 Jan 2019 13:50:14 +1300
Subject: [PATCH] Fix bug in Annual summary query (on contact dashboard)
 whereby a left join gives the wrong results

    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
---
 CRM/Contribute/BAO/Contribution.php           | 10 +--
 .../CRM/Contribute/BAO/ContributionTest.php   | 18 ++++++
 .../CRMTraits/Financial/PriceSetTrait.php     | 61 +++++++++++++++++++
 3 files changed, 80 insertions(+), 9 deletions(-)
 create mode 100644 tests/phpunit/CRMTraits/Financial/PriceSetTrait.php

diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php
index 21d40d568a5a..7cf97b715cfd 100644
--- a/CRM/Contribute/BAO/Contribution.php
+++ b/CRM/Contribute/BAO/Contribution.php
@@ -5581,14 +5581,7 @@ public static function getAnnualQuery($contactIDs) {
     }
     $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 @@ public static function getAnnualQuery($contactIDs) {
              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 21ac309634ae..16a9d0eb4781 100644
--- 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 @@ public function testAnnualQueryWithFinancialACLsEnabled() {
     $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
index 000000000000..48d102066960
--- /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);
+  }
+
+}