From dbaa9d7d82f8b06de490d4d581ef963e416946eb Mon Sep 17 00:00:00 2001 From: eileen Date: Fri, 2 Feb 2018 13:25:58 +1300 Subject: [PATCH] CRM-19752 Improve performance for non-ACL users on summaryQuery --- CRM/Contact/BAO/Query.php | 20 ++-- CRM/Core/ClassLoader.php | 2 +- CRM/Financial/BAO/FinancialType.php | 34 +++--- tests/phpunit/CRM/Contact/BAO/QueryTest.php | 37 +++--- .../CRMTraits/Financial/FinancialACLTrait.php | 109 ++++++++++++++++++ tests/phpunit/api/v3/EventTest.php | 2 +- tests/phpunit/api/v3/FinancialTypeACLTest.php | 90 ++++++--------- 7 files changed, 194 insertions(+), 100 deletions(-) create mode 100644 tests/phpunit/CRMTraits/Financial/FinancialACLTrait.php diff --git a/CRM/Contact/BAO/Query.php b/CRM/Contact/BAO/Query.php index 7655e8e6e2..9ee1370997 100644 --- a/CRM/Contact/BAO/Query.php +++ b/CRM/Contact/BAO/Query.php @@ -5040,9 +5040,8 @@ SELECT COUNT( conts.total_amount ) as total_count, $where .= " AND contact_a.is_deleted = 0 "; } - $query = $this->getSummaryQuery($where, $from); - $where = $query[0]; - $from = $query[1]; + $query = $this->appendFinancialTypeWhereAndFromToQueryStrings($where, $from); + // make sure contribution is completed - CRM-4989 $completedWhere = $where . " AND civicrm_contribution.contribution_status_id = 1 "; @@ -5173,16 +5172,14 @@ SELECT COUNT( conts.total_amount ) as cancel_count, } /** - * Function for getting the SummaryQuery of the respective financial type - * - * @param $where - * @param $from + * Append financial ACL limits to the query from & where clauses, if applicable. * - * @return array + * @param string $where + * @param string $from */ - public function getSummaryQuery($where, $from) { - if (CRM_Financial_BAO_FinancialType::isACLFinancialTypeStatus()) { - $financialTypes = CRM_Contribute_PseudoConstant::financialType(); + public function appendFinancialTypeWhereAndFromToQueryStrings(&$where, &$from) { + if (!CRM_Financial_BAO_FinancialType::isACLFinancialTypeStatus()) { + return; } CRM_Financial_BAO_FinancialType::getAvailableFinancialTypes($financialTypes); if (!empty($financialTypes)) { @@ -5194,7 +5191,6 @@ SELECT COUNT( conts.total_amount ) as cancel_count, else { $where .= " AND civicrm_contribution.financial_type_id IN (0)"; } - return array($where, $from, $financialTypes); } /** diff --git a/CRM/Core/ClassLoader.php b/CRM/Core/ClassLoader.php index 9b69635426..3a455777bb 100644 --- a/CRM/Core/ClassLoader.php +++ b/CRM/Core/ClassLoader.php @@ -205,7 +205,7 @@ class CRM_Core_ClassLoader { if ( // Only load classes that clearly belong to CiviCRM. // Note: api/v3 does not use classes, but api_v3's test-suite does - (0 === strncmp($class, 'CRM_', 4) || 0 === strncmp($class, 'api_v3_', 7) || 0 === strncmp($class, 'WebTest_', 8) || 0 === strncmp($class, 'E2E_', 4)) && + (0 === strncmp($class, 'CRM_', 4) || 0 === strncmp($class, 'CRMTraits', 9) || 0 === strncmp($class, 'api_v3_', 7) || 0 === strncmp($class, 'WebTest_', 8) || 0 === strncmp($class, 'E2E_', 4)) && // Do not load PHP 5.3 namespaced classes. // (in a future version, maybe) FALSE === strpos($class, '\\') diff --git a/CRM/Financial/BAO/FinancialType.php b/CRM/Financial/BAO/FinancialType.php index b8a95c53d3..9b24f0381d 100644 --- a/CRM/Financial/BAO/FinancialType.php +++ b/CRM/Financial/BAO/FinancialType.php @@ -304,18 +304,17 @@ class CRM_Financial_BAO_FinancialType extends CRM_Financial_DAO_FinancialType { CRM_Core_Action::ADD => 'add', CRM_Core_Action::DELETE => 'delete', ); - // check cached value - if (CRM_Utils_Array::value($action, self::$_availableFinancialTypes) && !$resetCache) { - $financialTypes = self::$_availableFinancialTypes[$action]; - return self::$_availableFinancialTypes[$action]; - } - foreach ($financialTypes as $finTypeId => $type) { - if (!CRM_Core_Permission::check($actions[$action] . ' contributions of type ' . $type)) { - unset($financialTypes[$finTypeId]); + + if (!isset(\Civi::$statics[__CLASS__]['available_types_' . $action])) { + foreach ($financialTypes as $finTypeId => $type) { + if (!CRM_Core_Permission::check($actions[$action] . ' contributions of type ' . $type)) { + unset($financialTypes[$finTypeId]); + } } + \Civi::$statics[__CLASS__]['available_types_' . $action] = $financialTypes; } - self::$_availableFinancialTypes[$action] = $financialTypes; - return $financialTypes; + $financialTypes = \Civi::$statics[__CLASS__]['available_types_' . $action]; + return \Civi::$statics[__CLASS__]['available_types_' . $action]; } /** @@ -459,15 +458,14 @@ class CRM_Financial_BAO_FinancialType extends CRM_Financial_DAO_FinancialType { * @return bool */ public static function isACLFinancialTypeStatus() { - if (array_key_exists('acl_financial_type', self::$_statusACLFt)) { - return self::$_statusACLFt['acl_financial_type']; - } - $contributeSettings = Civi::settings()->get('contribution_invoice_settings'); - self::$_statusACLFt['acl_financial_type'] = FALSE; - if (CRM_Utils_Array::value('acl_financial_type', $contributeSettings)) { - self::$_statusACLFt['acl_financial_type'] = TRUE; + if (!isset(\Civi::$statics[__CLASS__]['is_acl_enabled'])) { + \Civi::$statics[__CLASS__]['is_acl_enabled'] = FALSE; + $contributeSettings = Civi::settings()->get('contribution_invoice_settings'); + if (CRM_Utils_Array::value('acl_financial_type', $contributeSettings)) { + \Civi::$statics[__CLASS__]['is_acl_enabled'] = TRUE; + } } - return self::$_statusACLFt['acl_financial_type']; + return \Civi::$statics[__CLASS__]['is_acl_enabled']; } } diff --git a/tests/phpunit/CRM/Contact/BAO/QueryTest.php b/tests/phpunit/CRM/Contact/BAO/QueryTest.php index 7524172138..15a8b0a908 100644 --- a/tests/phpunit/CRM/Contact/BAO/QueryTest.php +++ b/tests/phpunit/CRM/Contact/BAO/QueryTest.php @@ -5,6 +5,7 @@ * @group headless */ class CRM_Contact_BAO_QueryTest extends CiviUnitTestCase { + use CRMTraits_Financial_FinancialACLTrait; /** * @return CRM_Contact_BAO_QueryTestDataProvider @@ -499,27 +500,35 @@ civicrm_relationship.is_active = 1 AND $this->fail('Test failed for some reason which is not good'); } + + /** + * Test the summary query does not add an acl clause when acls not enabled.. + */ public function testGetSummaryQueryWithFinancialACLDisabled() { - $where = NULL; - $from = NULL; - $CRM_Contact_BAO_Query = new CRM_Contact_BAO_Query(); - $query = $CRM_Contact_BAO_Query->getSummaryQuery($where, $from); - $this->assertEquals(" AND civicrm_contribution.financial_type_id IN (" . implode(',', array_keys($query[2])) . ") AND li.id IS NULL", $query[0]); + $where = $from = NULL; + $queryObject = new CRM_Contact_BAO_Query(); + $query = $queryObject->appendFinancialTypeWhereAndFromToQueryStrings($where, + $from); + $this->assertEquals($where, $query[0]); + $this->assertEquals($from, $query[1]); } + /** + * Test the summary query accurately adds financial acl filters. + */ public function testGetSummaryQueryWithFinancialACLEnabled() { - $where = NULL; - $from = NULL; - $cid = $this->createLoggedInUser(); - CRM_Core_Config::singleton()->userPermissionClass->permissions = array('access CiviCRM', 'CiviCRM: view contributions of type Donation'); - $CRM_Contact_BAO_Query = new CRM_Contact_BAO_Query(); - $query = $CRM_Contact_BAO_Query->getSummaryQuery($where, $from); - $from = $query[1]; - $financialTypes = $query[2]; + $where = $from = NULL; + $this->enableFinancialACLs(); + $this->createLoggedInUserWithFinancialACL(); + $queryObject = new CRM_Contact_BAO_Query(); + $query = $queryObject->appendFinancialTypeWhereAndFromToQueryStrings($where, + $from); + $donationTypeID = CRM_Core_PseudoConstant::getKey('CRM_Contribute_BAO_Contribution', 'financial_type_id', 'Donation'); $this->assertEquals( " LEFT JOIN civicrm_line_item li ON civicrm_contribution.id = li.contribution_id AND - li.entity_table = 'civicrm_contribution' AND li.financial_type_id NOT IN (" . implode(',', array_keys($financialTypes)) . ") ", $from); + li.entity_table = 'civicrm_contribution' AND li.financial_type_id NOT IN ({$donationTypeID}) ", $from); + $this->disableFinancialACLs(); } } diff --git a/tests/phpunit/CRMTraits/Financial/FinancialACLTrait.php b/tests/phpunit/CRMTraits/Financial/FinancialACLTrait.php new file mode 100644 index 0000000000..1217c5cf9a --- /dev/null +++ b/tests/phpunit/CRMTraits/Financial/FinancialACLTrait.php @@ -0,0 +1,109 @@ +get('contribution_invoice_settings'); + $this->callAPISuccess('Setting', 'create', [ + 'contribution_invoice_settings' => array_merge($contributeSettings, ['acl_financial_type' => TRUE]) + ]); + unset(\Civi::$statics['CRM_Financial_BAO_FinancialType']); + } + + /** + * Disable financial ACLs. + */ + protected function disableFinancialACLs() { + $contributeSettings = Civi::settings()->get('contribution_invoice_settings'); + $this->callAPISuccess('Setting', 'create', [ + 'contribution_invoice_settings' => array_merge($contributeSettings, ['acl_financial_type' => FALSE]) + ]); + unset(\Civi::$statics['CRM_Financial_BAO_FinancialType']); + } + + /** + * Create a logged in user limited by ACL permissions. + * + * @param array $aclPermissions + * Array of ACL permissions in the format + * [[$action, $financialType], [$action, $financialType]) + */ + protected function createLoggedInUserWithFinancialACL($aclPermissions = [['view', 'Donation']]) { + CRM_Core_Config::singleton()->userPermissionClass->permissions = ['access CiviCRM']; + $this->createLoggedInUser(); + $this->addFinancialAclPermissions($aclPermissions); + } + + /** + * Add a permission to the financial ACLs. + * + * @param array $aclPermissions + * Array of ACL permissions in the format + * [[$action, $financialType], [$action, $financialType]) + */ + protected function addFinancialAclPermissions($aclPermissions) { + $permissions = CRM_Core_Config::singleton()->userPermissionClass->permissions; + foreach ($aclPermissions as $aclPermission) { + $permissions[] = $aclPermission[0] . ' contributions of type ' . $aclPermission[1]; + } + $this->setPermissions($permissions); + } + + /** + * Add a permission to the permissions array. + * + * @param array $permissions + * Array of permissions to add - e.g. ['access CiviCRM','access CiviContribute'], + */ + protected function addPermissions($permissions) { + $permissions = array_merge(CRM_Core_Config::singleton()->userPermissionClass->permissions, $permissions); + $this->setPermissions($permissions); + } + + /** + * Set ACL permissions, overwriting any existing ones. + * + * @param array $permissions + * Array of permissions e.g ['access CiviCRM','access CiviContribute'], + */ + protected function setPermissions($permissions) { + CRM_Core_Config::singleton()->userPermissionClass->permissions = $permissions; + if (isset(\Civi::$statics['CRM_Financial_BAO_FinancialType'])) { + unset(\Civi::$statics['CRM_Financial_BAO_FinancialType']); + } + } + +} diff --git a/tests/phpunit/api/v3/EventTest.php b/tests/phpunit/api/v3/EventTest.php index 1abeb53bef..377c2612fb 100644 --- a/tests/phpunit/api/v3/EventTest.php +++ b/tests/phpunit/api/v3/EventTest.php @@ -773,7 +773,7 @@ class api_v3_EventTest extends CiviUnitTestCase { 'title' => 'le cake is a tie', 'check_permissions' => TRUE, ); - $config = &CRM_Core_Config::singleton(); + $config = CRM_Core_Config::singleton(); $config->userPermissionClass->permissions = array('access CiviCRM'); $result = $this->callAPIFailure('event', 'create', $params); $this->assertEquals('API permission check failed for Event/create call; insufficient permission: require access CiviCRM and access CiviEvent and edit all events', $result['error_message'], 'lacking permissions should not be enough to create an event'); diff --git a/tests/phpunit/api/v3/FinancialTypeACLTest.php b/tests/phpunit/api/v3/FinancialTypeACLTest.php index e2dea8467c..0d69b98ba1 100644 --- a/tests/phpunit/api/v3/FinancialTypeACLTest.php +++ b/tests/phpunit/api/v3/FinancialTypeACLTest.php @@ -33,6 +33,8 @@ */ class api_v3_FinancialTypeACLTest extends CiviUnitTestCase { + use CRMTraits_Financial_FinancialACLTrait; + /** * Assume empty database with just civicrm_data. */ @@ -109,29 +111,14 @@ class api_v3_FinancialTypeACLTest extends CiviUnitTestCase { public function tearDown() { $this->quickCleanUpFinancialEntities(); $this->quickCleanup(array('civicrm_uf_match')); - CRM_Financial_BAO_FinancialType::$_availableFinancialTypes = array(); - CRM_Financial_BAO_FinancialType::$_statusACLFt = array(); - $params = array( - 'domain_id' => 1, - 'contribution_invoice_settings' => array('acl_financial_type' => 0), - ); - } - - public function setACL() { - CRM_Financial_BAO_FinancialType::$_availableFinancialTypes = array(); - CRM_Financial_BAO_FinancialType::$_statusACLFt = array(); - $params = array( - 'domain_id' => 1, - 'contribution_invoice_settings' => array('acl_financial_type' => 1), - ); - $this->callAPISuccess('setting', 'create', $params); + $this->disableFinancialACLs(); } /** * Test Get. */ public function testCreateACLContribution() { - $this->setACL(); + $this->enableFinancialACLs(); $p = array( 'contact_id' => $this->_individualId, 'receive_date' => '2010-01-20', @@ -146,28 +133,29 @@ class api_v3_FinancialTypeACLTest extends CiviUnitTestCase { 'contribution_status_id' => 1, 'check_permissions' => TRUE, ); - $config = &CRM_Core_Config::singleton(); - $config->userPermissionClass->permissions = array( + + $this->setPermissions([ 'access CiviCRM', 'access CiviContribute', 'edit contributions', - ); + ]); $result = $this->callAPIFailure('contribution', 'create', $p); $this->assertEquals('You do not have permission to create this contribution', $result['error_message']); - $config->userPermissionClass->permissions[] = 'add contributions of type Donation'; + $this->addFinancialAclPermissions([['add', 'Donation']]); + $contribution = $this->callAPISuccess('contribution', 'create', $p); $params = array( 'contribution_id' => $contribution['id'], ); - $config->userPermissionClass->permissions = array( + $this->setPermissions([ 'access CiviCRM', 'access CiviContribute', 'edit contributions', 'view contributions of type Donation', 'delete contributions of type Donation', - ); + ]); $contribution = $this->callAPISuccess('contribution', 'get', $params); @@ -191,14 +179,14 @@ class api_v3_FinancialTypeACLTest extends CiviUnitTestCase { * Test that acl contributions can be retrieved. */ public function testGetACLContribution() { - $this->setACL(); - $config = &CRM_Core_Config::singleton(); - $config->userPermissionClass->permissions = array( + $this->enableFinancialACLs(); + + $this->setPermissions([ 'access CiviCRM', 'access CiviContribute', 'view all contacts', 'add contributions of type Donation', - ); + ]); $contribution = $this->callAPISuccess('Contribution', 'create', $this->_params); $params = array( @@ -208,9 +196,7 @@ class api_v3_FinancialTypeACLTest extends CiviUnitTestCase { $contribution = $this->callAPISuccess('contribution', 'get', $params); $this->assertEquals($contribution['count'], 0); - CRM_Financial_BAO_FinancialType::$_availableFinancialTypes = NULL; - - $config->userPermissionClass->permissions[3] = 'view contributions of type Donation'; + $this->addFinancialAclPermissions([['view', 'Donation']]); $contribution = $this->callAPISuccess('contribution', 'get', $params); $this->assertEquals($contribution['count'], 1); @@ -220,7 +206,7 @@ class api_v3_FinancialTypeACLTest extends CiviUnitTestCase { * Test checks that passing in line items suppresses the create mechanism. */ public function testCreateACLContributionChainedLineItems() { - $this->setACL(); + $this->enableFinancialACLs(); $params = array( 'contact_id' => $this->_individualId, 'receive_date' => '20120511', @@ -251,30 +237,26 @@ class api_v3_FinancialTypeACLTest extends CiviUnitTestCase { ), ); - $config = CRM_Core_Config::singleton(); - $config->userPermissionClass->permissions = array( + $this->setPermissions([ 'access CiviCRM', 'access CiviContribute', 'edit contributions', 'delete in CiviContribute', 'add contributions of type Donation', 'delete contributions of type Donation', - ); + ]); $this->callAPIFailure('contribution', 'create', $params, 'Error in call to LineItem_create : You do not have permission to create this line item'); // Check that the entire contribution has rolled back. $contribution = $this->callAPISuccess('contribution', 'get', array()); $this->assertEquals(0, $contribution['count']); - CRM_Financial_BAO_FinancialType::$_availableFinancialTypes = NULL; - - $config = CRM_Core_Config::singleton(); - $config->userPermissionClass->permissions = array_merge($config->userPermissionClass->permissions, array( - 'add contributions of type Member Dues', - 'view contributions of type Donation', - 'view contributions of type Member Dues', - 'delete contributions of type Member Dues', - )); + $this->addFinancialAclPermissions([ + ['add', 'Member Dues'], + ['view', 'Donation'], + ['view', 'Member Dues'], + ['delete', 'Member Dues'], + ]); $contribution = $this->callAPISuccess('contribution', 'create', $params); $lineItemParams = array( @@ -299,7 +281,7 @@ class api_v3_FinancialTypeACLTest extends CiviUnitTestCase { * Test that acl contributions can be edited. */ public function testEditACLContribution() { - $this->setACL(); + $this->enableFinancialACLs(); $contribution = $this->callAPISuccess('Contribution', 'create', $this->_params); $params = array( @@ -307,16 +289,16 @@ class api_v3_FinancialTypeACLTest extends CiviUnitTestCase { 'check_permissions' => TRUE, 'total_amount' => 200.00, ); - $config = CRM_Core_Config::singleton(); - $config->userPermissionClass->permissions = array( + + $this->setPermissions([ 'access CiviCRM', 'access CiviContribute', 'edit contributions', 'view contributions of type Donation', - ); + ]); $this->callAPIFailure('Contribution', 'create', $params); - $config->userPermissionClass->permissions[] = 'edit contributions of type Donation'; + $this->addFinancialAclPermissions([['edit', 'Donation']]); $contribution = $this->callAPISuccess('Contribution', 'create', $params); $this->assertEquals($contribution['values'][$contribution['id']]['total_amount'], 200.00); @@ -326,24 +308,24 @@ class api_v3_FinancialTypeACLTest extends CiviUnitTestCase { * Test that acl contributions can be deleted. */ public function testDeleteACLContribution() { - $this->setACL(); - $config = CRM_Core_Config::singleton(); - $config->userPermissionClass->permissions = array( + $this->enableFinancialACLs(); + + $this->setPermissions([ 'access CiviCRM', 'access CiviContribute', 'view all contacts', 'add contributions of type Donation', - ); + ]); $contribution = $this->callAPISuccess('Contribution', 'create', $this->_params); $params = array( 'contribution_id' => $contribution['id'], 'check_permissions' => TRUE, ); - $config->userPermissionClass->permissions[3] = 'delete in CiviContribute'; + $this->addPermissions(['delete in CiviContribute']); $this->callAPIFailure('Contribution', 'delete', $params); - $config->userPermissionClass->permissions[] = 'delete contributions of type Donation'; + $this->addFinancialAclPermissions([['delete', 'Donation']]); $contribution = $this->callAPISuccess('Contribution', 'delete', $params); $this->assertEquals($contribution['count'], 1); -- 2.25.1