Move financial acls for membership types to the extension

civicrm · Feb 3, 2022 · 56f5e9d · 56f5e9d
1 parent 6af3aeb
commit 56f5e9d
Show file tree

Hide file tree

Showing 8 changed files with 77 additions and 78 deletions.
diff --git a/CRM/Contact/BAO/Query.php b/CRM/Contact/BAO/Query.php
@@ -581,11 +581,16 @@ public function initialize($apiEntity = NULL) {
     }
     if (isset($component) && !$this->_skipPermission) {
       // Unit test coverage in api_v3_FinancialTypeACLTest::testGetACLContribution.
-      $clauses = CRM_Financial_BAO_FinancialType::buildPermissionedClause($component);
-      if (!empty($this->_whereClause) && !empty($clauses)) {
-        $this->_whereClause .= ' AND ';
+      $clauses = [];
+      if ($component === 'contribution') {
+        $clauses = CRM_Contribute_BAO_Contribution::getSelectWhereClause();
+      }
+      if ($component === 'membership') {
+        $clauses = CRM_Member_BAO_Membership::getSelectWhereClause();
+      }
+      if ($clauses) {
+        $this->_whereClause .= ' AND ' . implode(' AND ', $clauses);
       }
-      $this->_whereClause .= $clauses;
     }
 
     $this->_fromClause = self::fromClause($this->_tables, NULL, NULL, $this->_primaryLocation, $this->_mode, $apiEntity);
@@ -2112,7 +2117,7 @@ public function whereClause($isForcePrimaryEmailOnly = NULL): string {
       }
     }
 
-    return implode(' AND ', $andClauses);
+    return $andClauses ? implode(' AND ', $andClauses) : ' 1 ';
   }
 
   /**

diff --git a/CRM/Contribute/BAO/Contribution.php b/CRM/Contribute/BAO/Contribution.php
@@ -4371,30 +4371,29 @@ public static function getAnnualQuery($contactIDs) {
     }
     $startDate = "$year$monthDay";
     $endDate = "$nextYear$monthDay";
-
-    $whereClauses = [
-      'contact_id' => 'IN (' . $contactIDs . ')',
-      'is_test' => ' = 0',
-      'receive_date' => ['>=' . $startDate, '<  ' . $endDate],
-    ];
     $havingClause = 'contribution_status_id = ' . (int) CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'contribution_status_id', 'Completed');
-    CRM_Financial_BAO_FinancialType::addACLClausesToWhereClauses($whereClauses);
+
+    $contributionBAO = new CRM_Contribute_BAO_Contribution();
+    $whereClauses = $contributionBAO->addSelectWhereClause();
 
     $clauses = [];
     foreach ($whereClauses as $key => $clause) {
-      $clauses[] = 'b.' . $key . " " . implode(' AND b.' . $key, (array) $clause);
+      $clauses[] = 'b.' . $key . ' ' . implode(' AND b.' . $key, (array) $clause);
     }
+    $clauses[] = 'b.contact_id IN (' . $contactIDs . ')';
+    $clauses[] = 'b.is_test = 0';
+    $clauses[] = 'b.receive_date >=' . $startDate . ' AND b.receive_date < ' . $endDate;
     $whereClauseString = implode(' AND ', $clauses);
 
     // See https://github.com/civicrm/civicrm-core/pull/13512 for discussion of how
     // this group by + having on contribution_status_id improves performance
-    $query = "
+    $query = '
       SELECT COUNT(*) as count,
              SUM(total_amount) as amount,
              AVG(total_amount) as average,
              currency
       FROM civicrm_contribution b
-      WHERE " . $whereClauseString . "
+      WHERE ' . $whereClauseString . "
       GROUP BY currency, contribution_status_id
       HAVING $havingClause
       ";

diff --git a/CRM/Core/DAO.php b/CRM/Core/DAO.php
@@ -3082,11 +3082,14 @@ public function addSelectWhereClause() {
     $fields = $this->fields();
     foreach ($fields as $fieldName => $field) {
       // Clause for contact-related entities like Email, Relationship, etc.
-      if (strpos($fieldName, 'contact_id') === 0 && CRM_Utils_Array::value('FKClassName', $field) == 'CRM_Contact_DAO_Contact') {
-        $clauses[$fieldName] = CRM_Utils_SQL::mergeSubquery('Contact');
+      if (strpos($field['name'], 'contact_id') === 0 && CRM_Utils_Array::value('FKClassName', $field) == 'CRM_Contact_DAO_Contact') {
+        $contactClause = CRM_Utils_SQL::mergeSubquery('Contact');
+        if (!empty($contactClause)) {
+          $clauses[$field['name']] = $contactClause;
+        }
       }
       // Clause for an entity_table/entity_id combo
-      if ($fieldName === 'entity_id' && isset($fields['entity_table'])) {
+      if ($field['name'] === 'entity_id' && isset($fields['entity_table'])) {
         $relatedClauses = [];
         $relatedEntities = $this->buildOptions('entity_table', 'get');
         foreach ((array) $relatedEntities as $table => $ent) {

diff --git a/CRM/Financial/BAO/FinancialType.php b/CRM/Financial/BAO/FinancialType.php
@@ -346,27 +346,21 @@ public static function addACLClausesToWhereClauses(&$whereClauses) {
    * @param string $component
    *   the type of component
    *
+   * @deprecated
+   *
    * @return string $clauses
    */
   public static function buildPermissionedClause(string $component): string {
-    $clauses = [];
-    // @todo the relevant addSelectWhere clause should be called.
-    if (!self::isACLFinancialTypeStatus()) {
-      return '';
-    }
+    CRM_Core_Error::deprecatedFunctionWarning('no alternative');
+    // There are no non-test usages of this function (including in a universe
+    // search).
     if ($component === 'contribution') {
       $clauses = CRM_Contribute_BAO_Contribution::getSelectWhereClause();
     }
     if ($component === 'membership') {
-      self::getAvailableMembershipTypes($types, CRM_Core_Action::VIEW);
-      $types = array_keys($types);
-      if (empty($types)) {
-        $types = [0];
-      }
-      $clauses[] = ' civicrm_membership.membership_type_id IN (' . implode(',', $types) . ')';
-
+      $clauses = CRM_Member_BAO_Membership::getSelectWhereClause();
     }
-    return implode(' AND ', $clauses);
+    return 'AND ' . implode(' AND ', $clauses);
   }
 
   /**

diff --git a/ext/financialacls/financialacls.php b/ext/financialacls/financialacls.php
@@ -3,6 +3,7 @@
 require_once 'financialacls.civix.php';
 // phpcs:disable
 use Civi\Api4\EntityFinancialAccount;
+use Civi\Api4\MembershipType;
 use CRM_Financialacls_ExtensionUtil as E;
 // phpcs:enable
 
@@ -143,6 +144,10 @@ function financialacls_civicrm_selectWhereClause($entity, &$clauses) {
       $clauses['financial_type_id'] = _financialacls_civicrm_get_type_clause();
       break;
 
+    case 'Membership':
+      $clauses['membership_type_id'] = _financialacls_civicrm_get_membership_type_clause();
+      break;
+
     case 'FinancialType':
       $clauses['id'] = _financialacls_civicrm_get_type_clause();
       break;
@@ -192,12 +197,40 @@ function _financialacls_civicrm_get_accounts_clause(): string {
  * @return string
  */
 function _financialacls_civicrm_get_type_clause(): string {
+  return 'IN (' . implode(',', _financialacls_civicrm_get_accessible_financial_types()) . ')';
+}
+
+/**
+ * Get an array of the ids of accessible financial types.
+ *
+ * If none then it will be [0]
+ *
+ * @return int[]
+ */
+function _financialacls_civicrm_get_accessible_financial_types(): array {
   $types = [];
   CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($types);
-  if ($types) {
-    return 'IN (' . implode(',', array_keys($types)) . ')';
+  if (empty($types)) {
+    $types = [0];
+  }
+  return array_keys($types);
+}
+
+/**
+ * Get the clause to limit available membership types.
+ *
+ * @return string
+ *
+ * @throws \API_Exception
+ */
+function _financialacls_civicrm_get_membership_type_clause(): string {
+  $financialTypes = _financialacls_civicrm_get_accessible_financial_types();
+  if ($financialTypes === [0]) {
+    return 0;
   }
-  return '= 0';
+  $membershipTypes = (array) MembershipType::get(FALSE)
+    ->addWhere('financial_type_id', 'IN', $financialTypes)->execute()->indexBy('id');
+  return empty($membershipTypes) ? '= 0' : ('IN (' . implode(',', array_keys($membershipTypes)) . ')');
 }
 
 /**

diff --git a/ext/financialacls/tests/phpunit/Civi/Financialacls/BaseTestClass.php b/ext/financialacls/tests/phpunit/Civi/Financialacls/BaseTestClass.php
@@ -58,6 +58,7 @@ protected function setupLoggedInUserWithLimitedFinancialTypeAccess(): void {
       'access CiviMember',
       'edit contributions',
       'delete in CiviContribute',
+      'view all contacts',
       'view contributions of type Donation',
       'delete contributions of type Donation',
       'add contributions of type Donation',

diff --git a/ext/financialacls/tests/phpunit/Civi/Financialacls/FinancialTypeTest.php b/ext/financialacls/tests/phpunit/Civi/Financialacls/FinancialTypeTest.php
@@ -71,40 +71,4 @@ public function testGetIncomeFinancialType(): void {
     $this->assertEquals([1 => 'Donation'], $type);
   }
 
-  /**
-   * Check method test buildPermissionedClause()
-   *
-   * @throws \API_Exception
-   */
-  public function testBuildPermissionedClause(): void {
-    Civi::settings()->set('acl_financial_type', 1);
-    $membershipTypeID = Civi\Api4\MembershipType::create()->setValues([
-      'financial_type_id:name' => 'Event Fee',
-      'name' => 'event access',
-      'member_of_contact_id' => \CRM_Core_Config::domainID(),
-      'duration_unit' => 'year',
-      'period_type' => 'fixed',
-    ])->execute()->first()['id'];
-    $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` IS NULL OR (`civicrm_contribution`.`financial_type_id` IN (1,2)))', $whereClause);
-    $whereClause = \CRM_Financial_BAO_FinancialType::buildPermissionedClause('membership');
-    $this->assertEquals(' civicrm_membership.membership_type_id IN (0)', $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` IS NULL OR (`civicrm_contribution`.`financial_type_id` IN (1,4,2)))', $whereClause);
-    $whereClause = \CRM_Financial_BAO_FinancialType::buildPermissionedClause('membership');
-    $this->assertEquals(' civicrm_membership.membership_type_id IN (' . $membershipTypeID . ')', $whereClause);
-
-  }
-
 }
diff --git 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 {
    * Clean up after tests.
    */
   public function tearDown(): void {
+    $this->disableFinancialACLs();
     $this->quickCleanUpFinancialEntities();
     parent::tearDown();
   }
@@ -308,18 +309,17 @@ public function testCreateAndGetHonorContact() {
   /**
    * Test that financial type data is not added to the annual query if acls not enabled.
    */
-  public function testAnnualQueryWithFinancialACLsEnabled() {
+  public function testAnnualQueryWithFinancialACLsEnabled(): void {
     $this->enableFinancialACLs();
     $this->createLoggedInUserWithFinancialACL();
     $permittedFinancialType = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'financial_type_id', 'Donation');
     $sql = CRM_Contribute_BAO_Contribution::getAnnualQuery([1, 2, 3]);
     $this->assertStringContainsString('SUM(total_amount) as amount,', $sql);
-    $this->assertStringContainsString('WHERE b.contact_id IN (1,2,3)', $sql);
+    $this->assertStringContainsString('b.contact_id IN (1,2,3)', $sql);
     $this->assertStringContainsString('b.financial_type_id IN (' . $permittedFinancialType . ')', $sql);
 
     // Run it to make sure it's not bad sql.
     CRM_Core_DAO::executeQuery($sql);
-    $this->disableFinancialACLs();
   }
 
   /**
@@ -343,10 +343,10 @@ public function testAnnualWithMultipleLineItems(): void {
   /**
    * Test that financial type data is not added to the annual query if acls not enabled.
    */
-  public function testAnnualQueryWithFinancialACLsDisabled() {
+  public function testAnnualQueryWithFinancialACLsDisabled(): void {
     $sql = CRM_Contribute_BAO_Contribution::getAnnualQuery([1, 2, 3]);
     $this->assertStringContainsString('SUM(total_amount) as amount,', $sql);
-    $this->assertStringContainsString('WHERE b.contact_id IN (1,2,3)', $sql);
+    $this->assertStringContainsString('b.contact_id IN (1,2,3)', $sql);
     $this->assertStringNotContainsString('b.financial_type_id', $sql);
     //$this->assertNotContains('line_item', $sql);
     // Run it to make sure it's not bad sql.
@@ -356,12 +356,12 @@ public function testAnnualQueryWithFinancialACLsDisabled() {
   /**
    * Test that financial type data is not added to the annual query if acls not enabled.
    */
-  public function testAnnualQueryWithFinancialHook() {
+  public function testAnnualQueryWithFinancialHook(): void {
     $this->hookClass->setHook('civicrm_selectWhereClause', [$this, 'aclIdNoZero']);
     $sql = CRM_Contribute_BAO_Contribution::getAnnualQuery([1, 2, 3]);
     $this->assertStringContainsString('SUM(total_amount) as amount,', $sql);
-    $this->assertStringContainsString('WHERE b.contact_id IN (1,2,3)', $sql);
-    $this->assertStringContainsString('b.id NOT IN (0)', $sql);
+    $this->assertStringContainsString('b.contact_id IN (1,2,3)', $sql);
+    $this->assertStringContainsString('WHERE b.id NOT IN (0)', $sql);
     $this->assertStringNotContainsString('b.financial_type_id', $sql);
     CRM_Core_DAO::executeQuery($sql);
   }