From 1c5787ad47eb79388ad78ac46ee978c82612aeeb Mon Sep 17 00:00:00 2001
From: Eileen McNaughton <emcnaughton@wikimedia.org>
Date: Thu, 13 Jan 2022 13:27:44 +1300
Subject: [PATCH] Clean up buildPermissionedWhereClause

---
 CRM/Contact/BAO/Query.php                     |  6 ++-
 CRM/Financial/BAO/FinancialType.php           | 37 +++++++++----------
 .../Civi/Financialacls/FinancialTypeTest.php  | 23 +++++++++++-
 3 files changed, 44 insertions(+), 22 deletions(-)

diff --git a/CRM/Contact/BAO/Query.php b/CRM/Contact/BAO/Query.php
index a487b37af4..27b5de1bfc 100644
--- a/CRM/Contact/BAO/Query.php
+++ b/CRM/Contact/BAO/Query.php
@@ -581,7 +581,11 @@ class CRM_Contact_BAO_Query {
     }
     if (isset($component) && !$this->_skipPermission) {
       // Unit test coverage in api_v3_FinancialTypeACLTest::testGetACLContribution.
-      CRM_Financial_BAO_FinancialType::buildPermissionedClause($this->_whereClause, $component);
+      $clauses = CRM_Financial_BAO_FinancialType::buildPermissionedClause($component);
+      if (!empty($this->_whereClause) && !empty($clauses)) {
+        $this->_whereClause .= ' AND ';
+      }
+      $this->_whereClause .= $clauses;
     }
 
     $this->_fromClause = self::fromClause($this->_tables, NULL, NULL, $this->_primaryLocation, $this->_mode, $apiEntity);
diff --git a/CRM/Financial/BAO/FinancialType.php b/CRM/Financial/BAO/FinancialType.php
index d5e8618961..e5489e8ff9 100644
--- a/CRM/Financial/BAO/FinancialType.php
+++ b/CRM/Financial/BAO/FinancialType.php
@@ -343,35 +343,34 @@ class CRM_Financial_BAO_FinancialType extends CRM_Financial_DAO_FinancialType im
   /**
    * Function to build a permissioned sql where clause based on available financial types.
    *
-   * @param array $whereClauses
-   *   (reference ) an array of clauses
    * @param string $component
    *   the type of component
-   * @param string $alias
-   *   the alias to use
    *
+   * @return string $clauses
    */
-  public static function buildPermissionedClause(&$whereClauses, $component = NULL, $alias = NULL) {
+  public static function buildPermissionedClause(string $component): string {
+    $clauses = [];
     // @todo the relevant addSelectWhere clause should be called.
     if (!self::isACLFinancialTypeStatus()) {
-      return FALSE;
+      return '';
     }
-    if ($component == 'contribution') {
-      $types = self::getAllEnabledAvailableFinancialTypes();
-      $column = "financial_type_id";
+    if ($component === 'contribution') {
+      $types = array_keys(self::getAllEnabledAvailableFinancialTypes());
+      if (empty($types)) {
+        $types = [0];
+      }
+      $clauses[] = ' civicrm_contribution.financial_type_id IN (' . implode(',', $types) . ')';
     }
-    if ($component == 'membership') {
+    if ($component === 'membership') {
       self::getAvailableMembershipTypes($types, CRM_Core_Action::VIEW);
-      $column = "membership_type_id";
-    }
-    if (!empty($whereClauses)) {
-      $whereClauses .= ' AND ';
-    }
-    if (empty($types)) {
-      $whereClauses .= " civicrm_{$component}.{$column} IN (0)";
-      return;
+      $types = array_keys($types);
+      if (empty($types)) {
+        $types = [0];
+      }
+      $clauses[] = ' civicrm_membership.membership_type_id IN (' . implode(',', $types) . ')';
+
     }
-    $whereClauses .= " civicrm_{$component}.{$column} IN (" . implode(',', array_keys($types)) . ")";
+    return implode(' AND ', $clauses);
   }
 
   /**
diff --git a/ext/financialacls/tests/phpunit/Civi/Financialacls/FinancialTypeTest.php b/ext/financialacls/tests/phpunit/Civi/Financialacls/FinancialTypeTest.php
index bb68b08eab..4c07331e88 100644
--- a/ext/financialacls/tests/phpunit/Civi/Financialacls/FinancialTypeTest.php
+++ b/ext/financialacls/tests/phpunit/Civi/Financialacls/FinancialTypeTest.php
@@ -16,8 +16,6 @@ class FinancialTypeTest extends BaseTestClass {
   /**
    * Test that a message is put in session when changing the name of a
    * financial type.
-   *
-   * @throws \CRM_Core_Exception
    */
   public function testChangeFinancialTypeName(): void {
     Civi::settings()->set('acl_financial_type', TRUE);
@@ -73,4 +71,25 @@ class FinancialTypeTest extends BaseTestClass {
     $this->assertEquals([1 => 'Donation'], $type);
   }
 
+  /**
+   * Check method test buildPermissionedClause()
+   */
+  public function testBuildPermissionedClause(): void {
+    Civi::settings()->set('acl_financial_type', 1);
+    $this->setPermissions([
+      'view contributions of type Donation',
+      'view contributions of type Member Dues',
+    ]);
+    $whereClause = \CRM_Financial_BAO_FinancialType::buildPermissionedClause('contribution');
+    $this->assertEquals(' civicrm_contribution.financial_type_id IN (1,2)', $whereClause);
+    $this->setPermissions([
+      'view contributions of type Donation',
+      'view contributions of type Member Dues',
+      'view contributions of type Event Fee',
+    ]);
+
+    $whereClause = \CRM_Financial_BAO_FinancialType::buildPermissionedClause('contribution');
+    $this->assertEquals(' civicrm_contribution.financial_type_id IN (1,4,2)', $whereClause);
+  }
+
 }
-- 
2.25.1