From 40ae908c05e12bd70b66f7746f2138674036e83e Mon Sep 17 00:00:00 2001
From: eileen <emcnaughton@wikimedia.org>
Date: Wed, 24 Apr 2019 16:20:17 +1200
Subject: [PATCH] Fix financial acl permissions to respect check_permissions

This enables a test whereby check_permissions is passed in & fixes by following the todo. It also removes
a couple of lines whereby the permissions are double-added in the contribution where clause - but
only if a field for which no special handling exists -we should rely on the main initialize function
(& over time make the inclusion more generic & consistent with our selectWhere approach) to avoid
1) unnecessary joins
2) ignoring the check_permission flag
---
 CRM/Contact/BAO/Query.php                     | 4 ++--
 CRM/Contribute/BAO/Query.php                  | 2 --
 tests/phpunit/api/v3/FinancialTypeACLTest.php | 1 -
 3 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/CRM/Contact/BAO/Query.php b/CRM/Contact/BAO/Query.php
index e5f9b4c604..09662a57d9 100644
--- a/CRM/Contact/BAO/Query.php
+++ b/CRM/Contact/BAO/Query.php
@@ -541,8 +541,8 @@ class CRM_Contact_BAO_Query {
     if (array_key_exists('civicrm_membership', $this->_whereTables)) {
       $component = 'membership';
     }
-    if (isset($component)) {
-      // @todo should be if (isset($component && !$this->_skipPermission)
+    if (isset($component) && !$this->_skipPermission) {
+      // Unit test coverage in api_v3_FinancialTypeACLTest::testGetACLContribution.
       CRM_Financial_BAO_FinancialType::buildPermissionedClause($this->_whereClause, $component);
     }
 
diff --git a/CRM/Contribute/BAO/Query.php b/CRM/Contribute/BAO/Query.php
index 3c51f78c23..c8b16b983f 100644
--- a/CRM/Contribute/BAO/Query.php
+++ b/CRM/Contribute/BAO/Query.php
@@ -486,8 +486,6 @@ class CRM_Contribute_BAO_Query extends CRM_Core_BAO_Query {
       default:
         //all other elements are handle in this case
         $fldName = substr($name, 13);
-        CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($financialTypes);
-        $query->_where[$grouping][] = CRM_Contact_BAO_Query::buildClause("civicrm_contribution.financial_type_id", 'IN', array_keys($financialTypes), 'String');
         if (!isset($fields[$fldName])) {
           // CRM-12597
           CRM_Core_Session::setStatus(ts(
diff --git a/tests/phpunit/api/v3/FinancialTypeACLTest.php b/tests/phpunit/api/v3/FinancialTypeACLTest.php
index ccccba6bc2..5c25bf0f77 100644
--- a/tests/phpunit/api/v3/FinancialTypeACLTest.php
+++ b/tests/phpunit/api/v3/FinancialTypeACLTest.php
@@ -197,7 +197,6 @@ class api_v3_FinancialTypeACLTest extends CiviUnitTestCase {
     $this->addFinancialAclPermissions([['view', 'Donation']]);
     $this->callAPISuccessGetSingle('contribution', $params);
     $this->callAPISuccessGetCount('contribution', ['financial_type_id' => 'Member Dues', 'check_permissions' => 1], 0);
-    $this->markTestIncomplete('check_permissions = 0 should be respected but is not - I have added a todo at the right place but not changed it as yet');
     $this->callAPISuccessGetCount('contribution', ['financial_type_id' => 'Member Dues'], 1);
   }
 
-- 
2.25.1