diff --git a/CRM/Contact/BAO/Query.php b/CRM/Contact/BAO/Query.php index e9fda0f66560..e69e958d3f98 100644 --- 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 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..068518f4d213 100644 --- 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 index 36302a4c2acd..4aad3fa8d453 100644 --- 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 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/BaseTestClass.php b/ext/financialacls/tests/phpunit/Civi/Financialacls/BaseTestClass.php index 7732d722ad95..caa72e9bba50 100644 --- 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 index 8d45ee9bafa2..e88e563afee0 100644 --- 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 index 8cad0ad89ff5..e14a1291d83d 100644 --- 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); }