From 9dece7f53a4c6365e3bfb3dddf12fa35f1d64d95 Mon Sep 17 00:00:00 2001 From: Eileen McNaughton <emcnaughton@wikimedia.org> Date: Thu, 13 Jan 2022 16:59:41 +1300 Subject: [PATCH] Move financial acls for membership types to the extension --- CRM/Contact/BAO/Query.php | 7 ++-- CRM/Contribute/BAO/Contribution.php | 19 +++++---- CRM/Core/DAO.php | 7 +++- CRM/Financial/BAO/FinancialType.php | 13 +------ ext/financialacls/financialacls.php | 39 +++++++++++++++++-- .../Civi/Financialacls/FinancialTypeTest.php | 15 ++++--- .../CRM/Contribute/BAO/ContributionTest.php | 8 ++-- 7 files changed, 68 insertions(+), 40 deletions(-) diff --git a/CRM/Contact/BAO/Query.php b/CRM/Contact/BAO/Query.php index e9fda0f66560..24e46e13095e 100644 --- a/CRM/Contact/BAO/Query.php +++ b/CRM/Contact/BAO/Query.php @@ -582,10 +582,9 @@ 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 '; + if ($clauses) { + $this->_whereClause .= ' AND ' . $clauses; } - $this->_whereClause .= $clauses; } $this->_fromClause = self::fromClause($this->_tables, NULL, NULL, $this->_primaryLocation, $this->_mode, $apiEntity); @@ -2112,7 +2111,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 index 45f2f87863ef..65e11a059180 100644 --- 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 index 5eab8615f8dc..5b9db0197ace 100644 --- a/CRM/Core/DAO.php +++ b/CRM/Core/DAO.php @@ -3082,8 +3082,11 @@ 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']] = CRM_Utils_SQL::mergeSubquery('Contact'); + } } // Clause for an entity_table/entity_id combo if ($fieldName === 'entity_id' && isset($fields['entity_table'])) { diff --git a/CRM/Financial/BAO/FinancialType.php b/CRM/Financial/BAO/FinancialType.php index 36302a4c2acd..3e0410c9fa6b 100644 --- a/CRM/Financial/BAO/FinancialType.php +++ b/CRM/Financial/BAO/FinancialType.php @@ -349,22 +349,11 @@ public static function addACLClausesToWhereClauses(&$whereClauses) { * @return string $clauses */ public static function buildPermissionedClause(string $component): string { - $clauses = []; - // @todo the relevant addSelectWhere clause should be called. - if (!self::isACLFinancialTypeStatus()) { - return ''; - } 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); } diff --git a/ext/financialacls/financialacls.php b/ext/financialacls/financialacls.php index d0b445b3baa2..d93076bb8fe0 100644 --- 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/FinancialTypeTest.php b/ext/financialacls/tests/phpunit/Civi/Financialacls/FinancialTypeTest.php index 8d45ee9bafa2..66d3aeed7c12 100644 --- a/ext/financialacls/tests/phpunit/Civi/Financialacls/FinancialTypeTest.php +++ b/ext/financialacls/tests/phpunit/Civi/Financialacls/FinancialTypeTest.php @@ -86,25 +86,30 @@ public function testBuildPermissionedClause(): void { 'period_type' => 'fixed', ])->execute()->first()['id']; $this->setPermissions([ + 'view all contacts', + 'access deleted contacts', + 'access CiviMember', '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); + $this->assertEquals(' AND (`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->assertEquals(' AND (`civicrm_membership`.`membership_type_id` IS NULL OR (`civicrm_membership`.`membership_type_id` = 0))', $whereClause); $this->setPermissions([ + 'view all contacts', + 'access deleted contacts', + 'access CiviMember', '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); + $this->assertEquals(' AND (`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); - + $this->assertEquals(' AND (`civicrm_membership`.`membership_type_id` IS NULL OR (`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 index 8cad0ad89ff5..feee52567ec3 100644 --- a/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php +++ b/tests/phpunit/CRM/Contribute/BAO/ContributionTest.php @@ -343,7 +343,7 @@ 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); @@ -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); }