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

diff --git a/CRM/Contact/BAO/Query.php b/CRM/Contact/BAO/Query.php
index e5f9b4c6048bb034e2715314bcacf8551eb933f9..09662a57d965e0dcf14bb29f4034e3ccd5583b6a 100644 (file)
--- 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 3c51f78c2357ca658d922e1ddc88fcfc8d30c176..c8b16b983f68aae9cd7220d9df1a65c15997379b 100644 (file)
--- 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 ccccba6bc29fd7df653eef372322a8f0545fab19..5c25bf0f77b03c30dd2828301a1752a98ba876f6 100644 (file)
--- 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);
   }