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 64763f978a64..ea08da320688 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 2b3d81f5ece6..4aad3fa8d453 100644 --- a/CRM/Financial/BAO/FinancialType.php +++ b/CRM/Financial/BAO/FinancialType.php @@ -299,7 +299,7 @@ public static function getAvailableFinancialTypes(&$financialTypes = NULL, $acti */ public static function getAvailableMembershipTypes(&$membershipTypes = NULL, $action = CRM_Core_Action::VIEW) { if (empty($membershipTypes)) { - $membershipTypes = CRM_Member_PseudoConstant::membershipType(); + $membershipTypes = CRM_Member_BAO_Membership::buildOptions('membership_type_id'); } if (!self::isACLFinancialTypeStatus()) { return $membershipTypes; @@ -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 2c969f8a51ae..0dc2d25b1d14 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 31594b86c2fc..e88e563afee0 100644 --- a/ext/financialacls/tests/phpunit/Civi/Financialacls/FinancialTypeTest.php +++ b/ext/financialacls/tests/phpunit/Civi/Financialacls/FinancialTypeTest.php @@ -47,7 +47,7 @@ public function testPermissionedFinancialTypes(): void { foreach ($actions as $action => $action_ts) { $this->assertEquals( [ - ts("CiviCRM: %1 contributions of type %2", [1 => $action_ts, 2 => $type]), + ts('CiviCRM: %1 contributions of type %2', [1 => $action_ts, 2 => $type]), ts('%1 contributions of type %2', [1 => $action_ts, 2 => $type]), ], $permissions[$action . ' contributions of type ' . $type] @@ -71,25 +71,4 @@ public function testGetIncomeFinancialType(): void { $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` IS NULL OR (`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` IS NULL OR (`civicrm_contribution`.`financial_type_id` IN (1,4,2)))', $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); }